fix(google-content-sync): harden against the poison-page class, make the snapshot obligation durable, type-check the lambda - #1621
Conversation
…ning the sync queue
The prod alarm aistudio-prod-google-content-sync-oldest-message fired because
two connectors (b1429b0b-..., 1f870c92-...) had synced nothing since
2026-08-06 ~12:00 UTC and 904 messages had accumulated in the DLQ.
Root cause
----------
Google's Drive Changes feed (GET /drive/v3/changes) is not exclusively
file-scoped. Entries with changeType "drive" describe the Shared Drive itself
- rename, membership change, restriction change - and carry NO fileId.
driveChangeSchema in lib/repositories/google-drive/drive-client.ts declared
fileId as a required z.string(), so changesListSchema.parse() threw a ZodError
inside listChanges the moment such an entry appeared in a page. The throw
happened before reconcileChanges could persist the cursor, so every SQS
redelivery re-fetched the identical page and failed identically. The message
never drained, its age breached the 30-minute alarm threshold, and after
maxReceiveCount it landed in the DLQ - repeatedly.
Changes
-------
lib/repositories/google-drive/drive-client.ts
- driveChangeSchema.fileId is now z.string().optional().
- Added changeType: z.string().optional(). Deliberately a plain string and
NOT a z.enum: an enum would re-poison the queue the first time Google
ships a changeType value we did not anticipate.
- listChanges now requests changeType in its fields projection, so the API
actually returns the discriminator the guard reads. Without this the
projection would silently strip it and every entry would look untyped.
infra/lambdas/google-content-sync/changes.ts (new)
- isFileScopedDriveChange(): the single place that decides whether a change
entry names a file the sync path can act on. Returns false for
changeType === "drive" and for a missing/empty fileId; a type predicate so
the caller gets fileId narrowed to string. Extracted into its own module
because index.ts is not importable from the root Jest gate.
infra/lambdas/google-content-sync/index.ts
- processGoogleDriveChange() now guards FIRST: a non-file-scoped entry is
logged and skipped with return false, before markSourceMissing() or any
other side effect can run. Returning false leaves requiresSelectionSnapshot
untouched, so reconcileChanges still persists the cursor for that page and
the loop advances.
- Drive deletion and access loss remain covered by the existing 403/404 ->
markConnectorAccessLost path; nothing about that behaviour changes.
- Extracted the catch-block outcome recording into recordDriveChangeFailure()
- behaviour-identical, and it brings the function back under the
complexity-15 lint ceiling that the new guard pushed it over.
Tests
-----
- tests/unit/lib/repositories/google-drive-changes-feed.test.ts: a mixed page
containing a changeType "drive" entry with no fileId plus a normal file
entry now parses and returns both values; an unknown future changeType
parses; and the fields projection is asserted to request changeType.
- tests/unit/google-content-sync-change-scope.test.ts: full truth table for
isFileScopedDriveChange (drive-scope wins even if a fileId is present,
empty/missing fileId skipped, unknown changeType with a fileId processed),
the fileId type narrowing, plus source-level assertions that the guard
precedes the first side effect in processGoogleDriveChange and that
reconcileChanges still persists the cursor on the skip path.
Operational notes
-----------------
No alarm thresholds changed. Existing DLQ messages are intentionally left to
expire under the 14-day retention rather than being redriven or purged. The
affected connectors self-heal on their next webhook or scheduled tick once the
Lambda is deployed.
…s removed Review follow-up (Codex P1 on PR #1617). The first commit skipped every non-file-scoped change entry unconditionally. That is correct for the metadata noise the outage was actually about - Shared Drive rename, membership change, restriction change - but it also silently consumed the one drive-scoped entry that carries real meaning: removed: true, which Google emits when the Shared Drive is deleted or the account loses access to it. Consuming that is a data-correctness bug. Sources already imported from that drive become unreachable, and the connector gets no other signal: - A connector scoped to a shared drive would eventually surface the loss, because its next listChanges/watch call against that driveId returns 403/404 and the existing markConnectorAccessLost path fires. - A connector reading the global changes feed would NOT. Its subsequent listChanges and watch calls keep succeeding, so the stale content stays active indefinitely with nothing to retire it. Fix --- infra/lambdas/google-content-sync/changes.ts - New isDriveRemovalChange(): true when an entry is not file-scoped and reports removed. Deliberately NOT keyed on changeType === "drive" - the legacy "teamDrive" value and any future scope Google introduces must take this path too. What identifies the case is "removed, and not about a specific file", which the caller has already established. - DriveChangeIdentity gained the optional removed flag. infra/lambdas/google-content-sync/index.ts - processGoogleDriveChange() now returns isDriveRemovalChange(change) from the non-file-scoped branch rather than a bare false. Returning true sets requiresSelectionSnapshot, so reconcileChanges runs reconcileSelectionSnapshot -> markUnseenSourcesMissing and retires whatever is no longer reachable; a hard access loss still surfaces through the existing 403/404 -> markConnectorAccessLost path. - The cursor advances on both branches. Skips take the per-page persistSyncCursor; removals take the post-snapshot persistSyncCursor. The invariant the outage broke stays intact either way. - The skip/removal log line now also carries driveId and time, so a spike in either can be correlated against Drive audit logs. Tests ----- tests/unit/google-content-sync-change-scope.test.ts - Truth table for isDriveRemovalChange: drive-scoped removal flagged; "teamDrive", an unknown future scope, and a changeType-less removal all covered; rename/membership noise (removed false or absent) ignored; a file-scoped removal explicitly NOT claimed, since that is the file path's job via markSourceMissing; a drive-scoped entry that also carries a fileId still treated as drive-scoped. - Wiring assertions that the removal signal is returned rather than swallowed, and that reconcileChanges persists the cursor on BOTH branches (asserted as exactly two persistSyncCursor call sites in the loop). Verification: eslint --max-warnings 0 clean; 573 suites / 5434 tests passing (the single failing suite, tests/unit/agent-workspace-identity.test.ts, fails identically on the clean tree - @aws-sdk/client-ecs is absent from package.json and bun.lock entirely).
…e class
Follow-up to the drive-scoped change-entry hotfix. That fix removed one
specific poison entry; these three changes remove the failure class it
belonged to, plus the blind spot that let it ship.
1. Per-entry parsing in the Drive client (lib/repositories/google-drive)
changesListSchema and filesListSchema parsed their arrays as
z.array(entrySchema) and .parse()d the whole payload, so ONE malformed
entry among a thousand rejected the entire page. listChanges then threw
before the cursor advanced and the connector retried the same page
forever - the exact shape of the fileId incident.
Two-pronged:
(a) Validators that were stricter than Google's actual contract were
loosened, each after checking its downstream reader:
- shortcutDetails.targetId / targetMimeType -> optional. Only
targetId is dereferenced (resolveShortcut, and the selections
route), and both already raise a graceful "no target" error.
targetMimeType has no reader anywhere, so requiring it could
only ever poison a page.
- modifiedTime / change.time: .datetime({offset:true}) -> plain
string. Consumers are new Date(...) and a revision string.
- size: the ^\d+$ regex is gone. assertGoogleSourceMetadataSize
re-checks the numeric shape and the streaming byte bound is
authoritative regardless.
- webViewLink / iconLink: .url() -> plain string, but a
non-http(s) scheme is DROPPED rather than persisted, so
loosening the validator cannot open an href injection path.
- file, driveId, parents, owners, trashed and the optional
strings accept an explicit null as well as an omission.
(b) Defense in depth: the list envelopes now parse entries as
z.unknown() and safeParse each one individually. A failing entry is
reported on the page as a GoogleDriveSkippedEntry (index, any
extractable id, the zod issues) and DROPPED. It is never
reinterpreted - in particular markSourceMissing is never called for
it, because a parse failure says nothing about whether the file
still exists and reclassifying it would trade a stalled queue for
silent data loss. The accepted cost (a missed update until that
file changes again) is stated in the log line the Lambda emits.
Because a dropped entry makes an enumeration incomplete, the snapshot
path now gates markUnseenSourcesMissing behind
shouldRetireUnseenSources(): retiring unseen sources is only sound
after a COMPLETE pass, so the sweep is skipped (loudly) and left to the
next clean snapshot.
2. Durable selection-snapshot obligation (infra/lambdas/google-content-sync)
reconcileChanges deferred persistSyncCursor for the whole run once any
page demanded a selection snapshot. A 900s Lambda timeout or a snapshot
budget failure therefore threw away every already-processed change page
and replayed them from the original cursor on the next attempt -
indefinitely if the snapshot kept failing.
The obligation is now durable instead of implied by cursor position: a
selectionSnapshotPendingAt timestamp on repository_connectors.metadata
(existing jsonb - no migration), written before the cursor moves past
the page that raised it, with the cursor advancing per page exactly as
the non-snapshot path already did. The flag is cleared only after a
COMPLETE snapshot, and every sync start discharges an outstanding
obligation before consuming new changes. The flag is written with a
jsonb merge/delete rather than a read-modify-write so it cannot clobber
the webhook path's concurrent metadata writes.
The loop itself moved to reconcile.ts behind injected collaborators, so
the cursor/obligation ordering is provable without a database, Google
credentials, or the Lambda's module-level environment requirements.
Mid-enumeration checkpointing remains out of scope; partial enumeration
still never feeds markUnseenSourcesMissing.
3. The lambda is now type-checked
infra/lambdas/google-content-sync had no tsconfig.json: excluded from
the root tsc ("exclude": ["infra"]) and from the infra tsc ("exclude":
["lambdas/**/*"]), with esbuild only transpiling. Adds a tsconfig.json
modeled on unified-content-processor (extends the root config so the
lambda's repo-root lib/ imports resolve) and a
typecheck:google-content-sync script chained into infra's build, which
is what the cdk-validate CI job runs - the same route
typecheck:unified-content takes. Fixes the P2 that motivated it:
GoogleDriveChange was re-derived locally via
Awaited<ReturnType<...>>["values"][number] and now imports the exported
type.
Tests: new google-content-sync-reconcile.test.ts covers cursor-advances-
per-page-while-pending, flag-written-before-the-cursor-moves, marked once,
cleared only on a complete snapshot, retained on snapshot throw, and the
unseen-sweep gate. google-drive-changes-feed.test.ts gains real-payload
coverage for every loosened field (including the javascript:/data: drop)
and for one-malformed-entry-among-many across listChanges, listChildren
and listSharedDriveFiles. The reconcile source-string assertion in
google-content-sync-change-scope.test.ts is replaced by those behavioral
tests and now only pins that index.ts still delegates to the shared loop.
…it ::text cast `jsonb - $1` is defined for text, integer and text[]. The key arrives as an untyped bind parameter, so the overload is resolved by Postgres preference rather than by the statement. Casting the parameter to text states the intent and removes the dependency on that preference. Verified against local Postgres end to end: setting the flag merges selectionSnapshotPendingAt into repository_connectors.metadata without disturbing sibling keys, and clearing it removes only that key.
…leted rebuild reconcileInitial gated the clear on the connector row loaded at the start of the run. That row is stale on the path that matters: after a 410 cursor expiry, reconcileChanges may already have written the flag during this same run, so the in-memory copy still says "not pending" and the obligation survived a full, complete rebuild that had just discharged it. Removing an absent jsonb key is a no-op and an initial rebuild is rare, so the clear is now unconditional on a complete snapshot.
… of failing the run Adversarial review of the per-entry list parsing found the one remaining door for the poison-page pattern: GoogleDriveClient.getFile() still hard-parsed its response, so a single malformed single-entity record -- a shortcut target, a parent during the selection walk, or a selection root -- threw a raw ZodError. recordDriveChangeFailure rethrows anything it cannot classify, which failed the whole run before the page's cursor persisted and replayed the same page until the DLQ. Changes: - drive-client.ts: getFile() now safeParses and throws a typed GoogleDriveUnreadableFileError carrying the fileId and the zod issues, sharing the issue formatting with parseDriveEntries via a new formatEntryIssues helper. The error class documents why its handling must be the opposite of a 403/404: the file very likely still exists, so no caller may reinterpret it as a removal. - index.ts recordDriveChangeFailure: a new branch ahead of the catch-all rethrow fails the one record (counters.failed + recordSourceFailure) and returns. Never markSourceMissing, never rethrow -- the page's cursor keeps advancing. This covers the change-processing paths (resolveShortcut and the selectedViaForFile parent walk, both of which correctly propagate the error up to this classifier). - index.ts enumerateInitialFiles: an unreadable shortcut target (caught in the add closure) or selection root (caught in the selection loop) is now recorded as a skipped entry via a shared recordUnreadable helper, marking the enumeration incomplete -- which suppresses markUnseenSourcesMissing exactly like a dropped list entry -- instead of failing the entire snapshot. - index.ts reconcileInitial: an incomplete initial or 410-triggered rebuild now records the durable snapshot obligation (setSelectionSnapshotPending(context, !complete)) instead of silently proceeding, so the next run repeats the snapshot rather than leaving files hidden behind a dropped entry unrecovered until each happens to change again. This aligns the initial path with the changes-loop path, which already retried its obligation. Untouched on purpose: - retryDeferredDownloads already isolates generic per-source failures, so the new error flows into its existing recordSourceFailure path. - The interactive selections route fails the picker request either way; the typed error is strictly more informative than the ZodError it replaces. Tests: behavioral getFile coverage (typed error with fileId + issue paths, healthy record parses) in the drive-client suite; wiring assertions for the classifier ordering and the reconcileInitial obligation in the change-scope suite, consistent with that file's established source-reading pattern.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f621c7a0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review: fix(google-content-sync) — poison-page hardening, durable snapshot obligation, lambda typecheckI read through the full diff ( Correctness
Minor / non-blocking
SecurityNo concerns beyond the Test coverageStrong. The three new suites cover the three gaps proportionally to their risk:
I didn't find anything I'd block on. The two "minor" items above are worth a look but aren't correctness or security issues. Reviewed via automated PR review by Claude. |
…alid Date Codex P2 on PR #1621. Loosening `modifiedTime` to a plain string kept a malformed value from poisoning the page, but let it reach `sourceIdentityFields`, which hands it to `new Date(...)` and a timestamp column. An Invalid Date fails the import transaction, and because the entry *validated*, the snapshot never counts it as a skipped entry — so the cursor advances, `shouldRetireUnseenSources` is not suppressed, and the file stays unimported indefinitely instead of being retried. - New `optionalTimestamp` validator: accepts anything Google sends (no rejected entries), keeps only what `Date.parse` actually accepts, and normalizes the rest to `undefined` so the field falls back to its absent behaviour. - Applied to `googleDriveFileSchema.modifiedTime`. `driveChangeSchema.time` is left as a plain string: its only consumer is a log field, never `new Date(...)`. - Contract test covering the normalization and that the entry is still returned rather than skipped. Refs PR #1621 review comment 3739858653
Codex P1 on PR #1621. When a selected Shared Drive is deleted or access to it is revoked, `processGoogleDriveChange` requests a selection snapshot so `markUnseenSourcesMissing` can retire the sources imported from it. But `enumerateInitialFiles` rethrew 403/404 specifically for `selectionKind === "drive"`, so that snapshot never completed: `handleSyncFailure` called `markConnectorAccessLost`, which marks *every* source on the connector — file and folder selections that are still perfectly readable included — and the deletion grace period then removed their content. One removed drive escalated into connector-wide data loss. - The missing-error branch in `enumerateInitialFiles` no longer excludes `drive` selections: the selection is counted as inaccessible, logged with its `selectionKind`, and enumeration continues. Retirement is left to `markUnseenSourcesMissing`, which only retires what the enumeration did not see, so unrelated selections are untouched and an individually shared file living in the removed drive survives if another selection still reaches it. - A total loss of access still converges: every selection lands in the same branch, the seen set is empty, and the sweep retires all sources into the same grace period `markConnectorAccessLost` would have used (`applyDeletionGrace` covers both `missing` and `access_lost`). - Corrected the now-stale comment in `processGoogleDriveChange` that claimed a hard access loss surfaces through the 403/404 path. - Guard-wiring test pinning that the branch is not gated on `selectionKind`. Refs PR #1621 review comment 3739858648
|
🤖 pr-fix routine round 1 complete.
Two things worth a human eye:
Verification caveat — read before merging. I could not run the local gate: The Claude review comment on this PR was read and needed no code changes: its three "minor / non-blocking" items (the Generated by Claude Code |
Review: fix(google-content-sync) — poison-page hardening, durable snapshot obligation, lambda typecheckThorough PR, well-documented, and well-tested (16 drive-client contract tests, 12 reconcile-loop behavior tests, change-scope wiring assertions). The core design — per-entry validation instead of all-or-nothing page parsing, and a durable One finding that I think is worth addressing before merge:
|
Automated review finding on PR #1621, and a real regression this PR introduced. Loosening `shortcutDetails.targetId` from `z.string()` to optional makes the "shortcut with no target" state reachable on a well-formed page — Drive does not retroactively clean up `shortcutDetails` when a target is deleted — but `resolveShortcut` still threw a bare `Error` for it, and a bare `Error` falls through every classification branch: - `enumerateInitialFiles`' `add()` catches only `GoogleDriveUnreadableFileError` and rethrows everything else, and the per-selection catch classifies only that type and `isGoogleDriveMissingError` — so a plain `Error` aborts the entire selection snapshot, not one entry. - `recordDriveChangeFailure` falls through to `if (!isGoogleDriveMissingError(error)) throw error;` and rethrows. `reconcileChangePages` persists the cursor only after the whole page loop completes, so the throw happens first and the cursor never advances. Deterministic on every run that reaches the entry: the poison-page failure this Lambda exists to prevent, relocated from Zod validation to shortcut resolution, with no skippedEntries accounting and no recordSourceFailure. - `resolveShortcut` now throws `GoogleDriveUnreadableFileError` with a `shortcutDetails.targetId` issue. Both call sites already classify that type: the enumeration path records a skipped entry (which correctly suppresses the unseen-source sweep, since a dangling shortcut says nothing about the target's existence), and the change path fails the one record via `recordSourceFailure` while the cursor advances. - Guard-wiring test pinning the typed throw and the absence of a bare `Error`. - Corrected the schema and test comments that claimed the "no target" path was already graceful — it was not, which is what made this reachable. Refs PR #1621
|
🤖 pr-fix routine round 2 complete.
The
And it is genuinely reachable rather than theoretical: this PR is what loosened Fix is the suggested one — One tradeoff I made rather than decided for you: the enumeration path counts this as a skipped entry, which suppresses Round-1 verification caveat still applies: Generated by Claude Code |
ReviewWent through the full diff (drive-client.ts, index.ts, the new reconcile.ts/changes.ts, schema, and the three new test files). This is a well-scoped, carefully reasoned hardening PR — the invariant-driven approach (durable obligation before cursor advance, never-reinterpret-a-parse-failure-as-a-removal) is exactly the right way to close this class of bug, and the comments consistently explain why, not what. Correctness — looks solid
Minor / worth a look
Test coverageStrong — per-entry skip/tolerance behavior, the cursor/obligation ordering invariants (including the crash-between-mark-and-persist and throw-during-snapshot cases), and the change-scope classification ( SecurityNo new injection surface — the href-scheme filtering on Nice work — the "never reinterpret a parse failure as a removal" invariant and the durable-obligation-before-cursor-advance ordering are exactly the right fixes for the two failure classes described, and the reasoning is easy to audit thanks to the comments. |
|
🤖 pr-fix routine round 3 — nothing further to change; marking All 7 checks green on Resolved the two Codex threads (P1 and P2), both fixed and replied to. What I did not change, and whyThe round-2 review left three "minor / worth a look" notes. None are change requests, but one deserves your explicit call rather than my silent judgement: Envelope fields are still strict-parsed. I left it alone. It looks deliberate to me — you wrote The other two notes need no action: the Standing caveats for whoever merges
Removing Generated by Claude Code |
krishagel
added
the
pr-fix-done
Claude
Summary
Hardens the Google Drive content-sync pipeline against the whole class of poison-page failures that caused the 2026-08-06 prod incident, closes the durable-obligation gap around selection snapshots, and puts the lambda under the TypeScript compiler for the first time.
Follow-up scope approved by @hagelk after #1617 deliberately shipped root-cause-only.
Gap 1 — all-or-nothing page parsing (
lib/repositories/google-drive/drive-client.ts)changesListSchema/filesListSchemaused toz.array(entrySchema).parse()the whole payload: one malformed entry among 1000 rejected the page, the cursor never advanced, and the connector stalled — the originalfileIdincident, generalized.unknown[], then validate one at a time (parseDriveEntries). A malformed member costs that one entry, not the page. Skipped entries surface asskippedEntries(index, best-effort id, zod issues) and are logged loudly at every call site. A parse failure is never reinterpreted as a removal — dropped entries suppress the unseen-source sweep instead (see Gap 2).modifiedTime/timeplain strings (consumers donew Date(...));sizeplain string (the streaming byte bound is authoritative);shortcutDetails.targetId/targetMimeTypeoptional (resolveShortcutalready has the graceful "no target" path;targetMimeTypehas no reader);.nullish()+ normalizing transforms where Google could send explicitnull.webViewLink/iconLink: strict.url()dropped, but a non-http(s) scheme (javascript:,data:) is discarded rather than persisted, so the loosening cannot open an href-injection path.getFile()(single-entity sibling of the same gap): nowsafeParses and throws a typedGoogleDriveUnreadableFileError(fileId + issues) instead of a rawZodError. Callers classify it explicitly: a change-processing failure fails that one record (recordSourceFailure, nevermarkSourceMissing, cursor advances); an unreadable shortcut target or selection root during enumeration counts as a skipped entry. Found by adversarial review of the first commit — a ZodError escapinggetFilemid-change-page would have replayed the page until the DLQ, the exact pattern this PR eliminates.Gap 2 — snapshot obligation now durable (
infra/lambdas/google-content-sync/)Previously, a change page that demanded a selection snapshot suppressed every per-page cursor write until the snapshot finished. A 900s Lambda timeout or budget failure threw away all change-page work and replayed it from the original cursor — indefinitely, if the snapshot kept failing.
reconcile.tsextracts the changes-feed loop against injected collaborators (same pattern aschanges.ts/safety.ts;index.tscannot be imported from the root jest gate). Ordering invariants, each behaviorally tested:metadata.selectionSnapshotPendingAt, jsonb merge — no migration needed) before the cursor moves past the page that raised it.resolveSyncCursor).shouldRetireUnseenSources:markUnseenSourcesMissingonly runs after a complete enumeration — a dropped entry says nothing about its file's existence, so retiring on partial data would be silent data loss.reconcileInitialrecords the obligation on an incomplete initial/410-rebuild too (setSelectionSnapshotPending(context, !complete)), aligning it with the changes-loop path (correctness-review finding).Gap 3 — lambda now type-checked
infra/lambdas/google-content-sync/had no tsconfig and was excluded from both the root and infratscruns — esbuild only transpiles. Addedtsconfig.json(modeled onunified-content-processor) andtypecheck:google-content-sync, wired intoinfra'sbuildscript exactly like the sibling check. The duplicate local re-derivation ofGoogleDriveChangeinindex.tsis replaced by the exported type.Deliberately unchanged
enumerateInitialFiles's 403/404-on-selection-root behavior (retire via the missing path) is pre-existing and intentional parity with file-level 403/404 semantics — not widened here.Verification
bun run lint— zero warningsbun run typecheck+typecheck:google-content-sync— cleandocument-generation-servicexlsx test hit its 5s timeout under machine load; passes in isolation in 1.1s)nexus/model-routerfailure + 10 flaky-passed-on-retry, all Nexus/Atrium UI specs untouched by this backend diff. The failed spec was rerun in isolation and passed (10 passed / 1 flaky-passed-on-retry / 0 failed) — load-induced flake from two sessions' gates sharing the machine, not a regressiongetFileerror), 12 reconcile-loop behavior tests (cursor/obligation ordering, incomplete-snapshot retention), change-scope wiring assertionsNamed E2E flows: none — backend Lambda path, not reachable via Playwright; covered by unit/contract tests above.