Skip to content

fix(worker): bug-bash batch — email-forwarder drop, unbounded queries, lease/ctx safety - #80

Merged
mastermanas805 merged 1 commit into
masterfrom
fix/bugbash-batch-2026-06-03
Jun 3, 2026
Merged

fix(worker): bug-bash batch — email-forwarder drop, unbounded queries, lease/ctx safety#80
mastermanas805 merged 1 commit into
masterfrom
fix/bugbash-batch-2026-06-03

Conversation

@mastermanas805

Copy link
Copy Markdown
Member

Summary

Bug-bash batch for the worker. One genuine open bug fixed (BUG 2); the other findings were verified already-fixed or intentional in current master and are documented below so they aren't re-reported.

Fixed — BUG 2 (HIGH): unbounded SELECT in orphan_sweep_reconciler.fetchLiveStackIDs

fetchLiveStackIDs loaded the entire stacks table into one result set/allocation (SELECT id::text FROM stacks). Replaced with keyset pagination (WHERE id::text > $1 ORDER BY id::text ASC LIMIT $2, batches of orphanLiveIDsBatchLimit = 1000).

  • The complete live-id set is still returned — PASS 5 must see every live id to avoid deleting a live namespace — but it's assembled incrementally so the server-side cursor + per-fetch memory stay bounded as the table grows.
  • Keyset (not OFFSET): rides the PK index, stable under concurrent inserts/deletes mid-sweep. A stack inserted during the sweep can only ever preserve a namespace, never wrongly delete one.

Tests added (fetchLiveStackIDs now 100% covered):

  • TestOrphanSweep_FetchLiveStackIDs_KeysetPagination — full page 1 (== limit) forces a second query whose cursor arg $1 == page 1's tail id; both pages merge into the set.
  • TestOrphanSweep_FetchLiveStackIDs_SecondPageError — a DB error on a later page propagates (no partial set).
  • TestOrphanSweep_FetchLiveStackIDs_RowsErr — a rows.Err() mid-stream propagates.
  • Updated the four existing PASS 5 / fetch-stack-ids query expectations to the new SQL.

Not changed — already fixed / intentional in master

Brief item Finding
BUG 1 — event_email_forwarder missing-renderer drop Already fixed. Master writes a forwarder_sent permanent_drop ledger row before advancing the cursor (event_email_forwarder.go ~740-773), covered by TestEventForwarder_MissingRenderer_LoudErrorDropAndAdvance. The brief described pre-fix behavior.
propagation_runner lease Already fixed (D22-P3, 2026-05-21): pickEligible bumps next_attempt_at by propagationLeaseDuration inside the pick transaction — the crash-safe lease requested.
customer_backup_runner finalize ctx The fresh context.Background() is the P2-W4 fix (2026-05-18), not a bug. Using parentCtx would re-strand the row at running mid-shutdown after the S3 object is durable — reverting reintroduces the documented bug.

Skipped — behavior-changing / non-trivial

  • billing_reconciler keyset: LIMIT 100 is an intentional per-tick Razorpay-rate-limit fan-out cap, not a pure memory bound. Keyset-draining all teams per tick changes documented fan-out behavior — out of scope for a clean batch.
  • orphan_sweep PASS 6 timeout: the per-namespace timeout derives from the job ctx and fails-open by design; job-level time budgeting is governed by River JobTimeout. Restructuring the sweep's time budget is non-trivial.

Gate

make gate green (build + vet + go test ./... -short -count=1, the exact CI deploy.yml test step). All 16 packages pass.

🤖 Generated with Claude Code

…, lease/ctx safety

BUG 2 (HIGH) — orphan_sweep_reconciler.fetchLiveStackIDs loaded the ENTIRE
stacks table into one unbounded SELECT/result set. Replace with keyset
pagination (WHERE id::text > $1 ORDER BY id::text ASC LIMIT $2, batches of
orphanLiveIDsBatchLimit=1000). The complete live-id set is still returned —
PASS 5 needs every id to avoid deleting a live namespace — but it is now
assembled incrementally so the server-side cursor + per-fetch allocation stay
bounded regardless of table size. Keyset (not OFFSET) rides the PK index and is
stable under concurrent inserts; a stack inserted mid-sweep can only ever
PRESERVE a namespace, never wrongly delete one.

Tests: keyset multi-page iteration (full page 1 forces a second query whose
cursor == page 1 tail), second-page query error, and rows.Err() mid-stream
both propagate (no partial set). fetchLiveStackIDs now 100% covered. Updated
the four existing PASS 5 / fetch-stack-ids query expectations to the new SQL.

NOT CHANGED (verified already-fixed or intentional in current master):
- BUG 1 (event_email_forwarder missing-renderer): master already writes a
  forwarder_sent permanent_drop ledger row BEFORE advancing the cursor
  (lines ~740-773), with full coverage in
  TestEventForwarder_MissingRenderer_LoudErrorDropAndAdvance. The brief
  described pre-fix behavior; nothing to do.
- propagation_runner lease: D22-P3 (2026-05-21) already bumps next_attempt_at
  by propagationLeaseDuration inside the pick transaction — the crash-safe
  lease the brief asks for.
- customer_backup_runner finalize ctx: the fresh context.Background() is the
  P2-W4 (2026-05-18) FIX, not a bug — using parentCtx would re-strand the row
  at 'running' mid-shutdown after the S3 object is durable. Reverting would
  reintroduce the documented bug.

SKIPPED (behavior-changing / non-trivial, out of scope for a clean batch):
- billing_reconciler keyset: the LIMIT 100 is an intentional per-tick
  Razorpay-rate-limit fan-out cap, not a pure memory bound; adding keyset
  pagination to drain all teams per tick changes documented fan-out behavior.
- orphan_sweep PASS 6 timeout: per-namespace timeout derives from the job ctx
  and fails-open by design; job-level time budgeting is governed by River
  JobTimeout, restructuring it is out of scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mastermanas805
mastermanas805 enabled auto-merge (squash) June 3, 2026 18:41
@mastermanas805
mastermanas805 merged commit 54d5f28 into master Jun 3, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant