feat(mail): drop the Pub/Sub requirement — scheduled fetch as primary intake (HT-94) - #107
Merged
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughGmail inbound processing now treats Pub/Sub push as optional, adds an independent per-minute reconciliation sweep, limits watch maintenance to push deployments, and updates connection, configuration, health, cron routing, tests, deployment scheduling, and operational guidance. ChangesGmail inbound flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Cron as reconcile-sweep cron
participant Sweep as runGmailReconcileSweep
participant Mailboxes as MailboxStore
participant State as GmailWatchStateStore
participant Queue as QueueProvider
Cron->>Sweep: invoke sweep
Sweep->>Mailboxes: listActiveMailboxes()
loop active mailboxes
Sweep->>State: getCursor(mailboxId)
Sweep->>Queue: enqueue reconcile job
end
Sweep-->>Cron: return sweep report
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zaridan
force-pushed
the
docs/ht-92-scheduled-fetch-intake
branch
from
July 21, 2026 16:30
a5bd06c to
7e76182
Compare
zaridan
force-pushed
the
feat/ht-94-scheduled-fetch-primary
branch
from
July 21, 2026 17:22
aa89eb9 to
2c0e528
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specs/deploy/gmail-inbound-runbook.md (1)
363-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlert table reuses existing codes as a new compound row — confusing lookup.
Row 364 repeats the
queue-drain-stalled/queue-dead-letter-growthcodes already defined at line 358-359 as if they were a distinct table entry, just with "(on a push-free deployment)" appended to the key. Since G2 states eachalerts[]entry is one stable<code>: <detail>pair, an operator scanning this table mid-incident could reasonably wonder whether this is a third, different code rather than push-free-specific guidance for the same two codes.Consider folding this into the existing rows (e.g., append the push-free implication as an extra sentence in the original
queue-drain-stalled/queue-dead-letter-growth"First response" cells) rather than a separate table row with a compound key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/deploy/gmail-inbound-runbook.md` around lines 363 - 364, Remove the separate push-free `queue-drain-stalled` / `queue-dead-letter-growth` row from the alert table. Fold its push-free deployment implication and troubleshooting guidance into the existing rows for those stable alert codes, preserving one code-to-detail entry per `alerts[]` item and avoiding a compound key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specs/deploy/gmail-inbound-runbook.md`:
- Around line 363-364: Remove the separate push-free `queue-drain-stalled` /
`queue-dead-letter-growth` row from the alert table. Fold its push-free
deployment implication and troubleshooting guidance into the existing rows for
those stable alert codes, preserving one code-to-detail entry per `alerts[]`
item and avoiding a compound key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0825891d-f46e-4a7b-bfbd-cb1fc4e23865
📒 Files selected for processing (16)
specs/deploy/gmail-inbound-runbook.mdsrc/composition/app.test.tssrc/composition/app.tssrc/composition/config.test.tssrc/composition/config.tssrc/composition/health.test.tssrc/composition/health.tssrc/composition/root.test.tssrc/composition/root.tssrc/mail/gmail-connect.tssrc/mail/gmail-reconcile-sweep.test.tssrc/mail/gmail-reconcile-sweep.tssrc/mail/gmail-watch-maintenance.test.tssrc/mail/gmail-watch-maintenance.tssrc/store/gmail-watch-state.tsvercel.json
…HT-94) First half of dropping the Pub/Sub requirement. The engine previously refused to boot without GMAIL_PUBSUB_TOPIC, GMAIL_PUBSUB_SUBSCRIPTION and GMAIL_PUSH_SERVICE_ACCOUNT — three vars whose provisioning is the majority of the Google Cloud setup burden, and the half that fails silently (the domain-restricted-sharing org-policy block, and the missing serviceAccountTokenCreator grant). Config: the three become ONE optional object rather than three optional strings, so a half-configured push is unrepresentable — you cannot arm watch() against a topic without also being able to authenticate the resulting push. A partial config is a boot error naming the missing vars, never a silent fallback to "push off": an operator who set a topic and forgot the service account has a broken push they believe works, which is the precise failure this work exists to remove. Connect: with no topic, step 4 skips the watch() arm entirely and seeds the baseline cursor from getProfile(). That substitution is safe for exactly the reason gmail-connect.md gives for rejecting it in the push case — getProfile's separately-read historyId "could straddle the arm," and with no arm there is nothing to straddle. No extra API call: step 3 already calls getProfile to resolve the mailbox address, and its response carries the historyId. Store: seedBaseline's watchExpiration becomes optional. No migration — gmail_watch_state.watch_expiration is already nullable; only the TypeScript signature was stricter than the schema. Root: the push webhook deps are built ONLY when push is configured, so an endpoint that cannot verify what it receives is never routable. The watch-maintenance cron stays routed but reports a skip, because vercel.json is static and a daily 404 would read like a fault. Verified: tsc exit 0, biome exit 0, 1512 tests pass across 75 files. NOT yet done — the sweep still runs daily, so a push-free deployment would only ingest once a day. Extracting it to its own every-minute cron is the next commit and is what makes this coherent. Worth noting the sweep needs no access token (it reads a cursor and enqueues), unlike the watch re-arm it is currently welded to — which matters at 1/min cadence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-94) Second half of dropping the Pub/Sub requirement. The first commit made push optional; without this one a push-free deployment would ingest mail once a day, because the reconciliation sweep was welded to the daily watch-maintenance cron. The sweep moves to src/mail/gmail-reconcile-sweep.ts and runs every minute as its own cron. Two reasons it became a separate entry point rather than a flag on the existing one: - Cadence. As a push backstop it ran daily; as the primary transport it runs every minute. Those cannot share a cron. - Cost, and this is the load-bearing one. Watch renewal must acquire an access token per mailbox because it calls users.watch(). The sweep must not — it reads a stored cursor and enqueues, making no Gmail call at all. Keeping them welded would have meant a token refresh per mailbox per MINUTE against Google's token endpoint, for a call the sweep never makes. The reconcile consumer acquires its own token when it actually talks to Gmail. Nothing about reconciliation itself changes: the sweep enqueues the SAME GMAIL_RECONCILE_TOPIC job the push webhook enqueues, consumed by the same handler. A deployment without Pub/Sub ingests through exactly the code path a deployment with it does, triggered by a clock instead of a notification. That equivalence is what makes this safe under the mail-semantics invariant. Enqueues still carry no dedupeKey. At every-minute cadence the consumer's lease (HT-48) stops being an efficiency guard and becomes structural: ticks WILL overlap a still-running reconcile on a busy mailbox, and the lease is what makes that a no-op rather than duplicated fetching. watch-maintenance is now renewal-only, still daily, and its module doc no longer describes behavior that moved. The now-dead queue dependency is removed from its deps rather than left unused. Runbook: Part A leads with A3/A4 being optional and why skipping them is recommended — six fewer steps, no billing requirement, and neither of the two silent-failure traps. The cron list is corrected from a stale "three" to the actual five, which also closes one of the doc-drift defects found while mapping the current setup path. Verified: tsc exit 0, biome exit 0, 1517 tests pass across 76 files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e (HT-94)
Three independent reviews (two Opus-tier, one Codex) attacked this PR.
Two of the three claims I asked them to scrutinize did not survive.
**Health returned 503 permanently on the install path the runbook now
recommends.** health.ts alerts `watch-expiring` when an active mailbox has
no watch() expiration — which is the DESIGNED steady state once push is
optional. Since the endpoint's contract is a single boolean, that permanent
false alarm made every real alert invisible. runHealthCheck now takes
`pushConfigured` and gates the watch alerts on it. Deliberately a config
fact, not inferred from data: a NULL expiration means "no push configured"
on one deployment and "renewal cron is broken" on another, and only the
config distinguishes them. The gate is scoped to watch alerts —
mailbox-needs-attention still fires either way, with a test proving it.
I verified the column was nullable and stopped there, never checking what
READS it.
**The sweep now enqueues with `dedupeKey: mailboxId`.** Carrying "no
dedupeKey" from a DAILY cadence to an every-minute one was the actual
mistake, and it was not benign:
- The consumer lease does not make contention free. A failed claim returns
retry, the queue counts attempts, and jobs DEAD-LETTER at the cap. A
reconcile outrunning the retry window made every tick behind it burn its
attempts and dead-letter — tripping queue-dead-letter-growth, a second
permanent 503. The lease prevents duplicated work, not duplicated rows.
- There was no backpressure at all. Enqueue was unconditional per mailbox
per minute against a bounded drain batch shared with webhook delivery.
The original reasoning ("a quiet mailbox must still be swept") argues
against a COMPOSITE key, not the bare mailboxId: the partial unique index
only suppresses against still-live jobs, so a completed job unblocks the
next tick. Tests now prove that against the REAL Postgres queue over
PGlite, not a fake — a fake cannot exercise the index and would be
tautological. This also aligns the sweep with the push path, which has
always used a dedupe key.
**Coverage for the three headline branches**, all previously untested:
push-free health, resolveGmailPush (all-unset succeeds; every partial
throws naming the missing vars), and the new cron endpoint's routing and
CRON_SECRET enforcement.
**Runbook**: watch-expiring documented as push-only, and its remediation
corrected — a manual GET of watch-maintenance is a no-op on a push-free
deployment, so seeing that alert there is a bug, not a mailbox problem.
Added the two new log events and queue guidance for the push-free case.
**Corrections**: root.ts logged the maintenance skip under a different
event name than the module itself uses; the vestigial queue dependency and
its test helper are removed rather than left unused.
Not fixed, reported instead: my "exactly the same code path" claim was
overstated — push dedupes and (before this change) the sweep did not, and
`job.historyId` carries semantically opposite values depending on trigger.
Harmless while nothing reads it; noted in specs/mail/mailbox-connection.md.
One reviewer claim I overruled: Codex held that revoked OAuth grants would
go undetected without the old per-sweep token probe. listActiveMailboxes
excludes needs_reconnect at the SQL level and the consumer acquires a token
per job, so detection is now FASTER (~1h vs 24h) and a flagged mailbox
drops out of the sweep. What was lost is three counters — observability,
not correctness.
Verified: tsc exit 0, biome exit 0, 1531 tests pass across 76 files.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
zaridan
force-pushed
the
feat/ht-94-scheduled-fetch-primary
branch
from
July 25, 2026 15:33
2c0e528 to
328d2cb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #102 (the charter amendment this implements). Base is
docs/ht-92-scheduled-fetch-intake, notmain— review them together, since #102 is what makes this permissible under §2.Why
About half the ~26-step setup exists to make Gmail push work, and two of those steps fail silently: the domain-restricted-sharing org policy blocking the Pub/Sub IAM grant, and a CLI-created subscription missing
roles/iam.serviceAccountTokenCreator. Pub/Sub also forces billing on the Cloud project; the Gmail API alone does not.What changes
Push becomes optional. The three
GMAIL_PUBSUB*/GMAIL_PUSH*vars become one optional object rather than three optional strings, so half-configured push is unrepresentable. A partial config is a boot error naming the missing vars — an operator who set a topic and forgot the service account has a broken push they believe works, which is the exact failure this removes.Connect works with no topic. Step 4 skips the
watch()arm and seeds the baseline cursor fromgetProfile(). That's safe for precisely the reason gmail-connect.md:28-31 rejects it in the push case — getProfile's historyId "could straddle the arm," and with no arm there's nothing to straddle. No extra API call either: step 3 already callsgetProfilefor the address.The sweep becomes the primary transport, moved to
src/mail/gmail-reconcile-sweep.tson an every-minute cron.The two findings that shaped this
Reconciliation already works this way.
gmail-reconcile.tsreads the mailbox's stored cursor, explicitly never the push notification'shistoryId. Push only makes the same job run sooner. So the sweep enqueues the identicalGMAIL_RECONCILE_TOPICjob the webhook does — a push-free deployment ingests through exactly the code path a push deployment does. That equivalence is what makes this safe under the mail-semantics invariant.The sweep needs no access token — it reads a cursor and enqueues. Watch renewal needs one because it calls
users.watch(). Keeping them welded would have meant a token refresh per mailbox per minute against Google's token endpoint for a call the sweep never makes. That's why this is a module split rather than a flag.No migration
gmail_watch_state.watch_expirationwas already nullable — only the TypeScript signature was stricter than the schema. Verified against the live database.Verification
tsc --noEmitbiome check .npm testReviewer attention
dedupeKeyis still absent, deliberately. At every-minute cadence the HT-48 consumer lease stops being an efficiency guard and becomes structural — ticks will overlap a running reconcile on a busy mailbox, and the lease is what makes that a no-op instead of duplicated fetching. Worth confirming you agree that reasoning holds at 60× the old rate.*/1cron. Estimated well under a dollar a month in compute; the sweep is the cheapest of the five since it makes no external call. I could not retrieve Vercel's rate card to confirm the figure — the magnitude (~1.8 GB-hours/month) is solid, the dollars are an estimate.watch-maintenancestays routed when push is off and reports a skip rather than 404-ing, becausevercel.jsonis static and a daily not-found would read like a fault.Also
Runbook Part A now leads with A3/A4 being optional and why skipping is recommended, and its cron list is corrected from a stale "three" to the actual five — closing one of the doc-drift defects found while mapping the setup path.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation