Skip to content

fix(quota): close quota bypass + address P1 reliability findings - #1

Merged
jeffzakrzewski merged 1 commit into
mainfrom
feat/quota-review-fixes
Apr 24, 2026
Merged

fix(quota): close quota bypass + address P1 reliability findings#1
jeffzakrzewski merged 1 commit into
mainfrom
feat/quota-review-fixes

Conversation

@jeffzakrzewski

Copy link
Copy Markdown
Contributor

Summary

Addresses findings from the post-ship code review on main (commit fdd6b7f). Main is live in production; this PR doesn't block anything there but closes real gaps in the bill-defense system.

  • P0 quota bypassbuildTree, DELETE /api/sessions/:id, and the post-regen wpilog GET were calling env.BLOBS.{list,get,delete} directly, letting a cookie holder burn R2 Class A/B ops for free
  • P1 waitUntil silencing — a throw after the alerted_* latch flipped lost both operator email and audit row for the whole UTC day
  • P1 Resend hangs — no AbortSignal meant a hung Resend connection burned the full waitUntil budget
  • P1 DO double-charge — D1-batch failures forced the uploader to retry, which re-charged the quota counter for the same logical batch
  • P2 concurrent first-breach race — two concurrent racers could both see alerted_*=false and both fire the alert email
  • P2 extra D1 round-tripchargeQuota ran UPSERT then SELECT; collapsed to one batched statement
  • P2 serialized Class B chargesstreamSessionBatches charged per object in the loop

Plus P3 polish (DO emits X-Quota-Breach-Date, res.clone() in passthrough, CAP sanity check, jitter on retry backoff, cleanups).

Changes by file

  • storage/r2.ts — new listBlobs / getBlob / deleteBlob wrappers that charge quota; streamSessionBatches batches Class B; split putBatchJsonl into encodeBatchJsonl + putBatchJsonlBytes so the DO can dedup charges on retry
  • ingest-do/session-ingest-do.ts — tracks chargedSeq in DO storage (persisted before R2 PUT), skips the charge on retry; emits X-Quota-Breach-Date header on first-breach 429
  • quota/daily-quota.ts — UPSERT with CASE-based conditional latch flip inside a db.batch that also captures the pre-snapshot; halves D1 round-trips and closes the double-email race; adds CAP_* > 0 load-time assertion
  • quota/http.tsguardWaitUntil wraps every scheduled task; scheduleDoAlert now reads the D1 row keyed on the DO-emitted breach date; removed unused exports
  • auth/email.ts — shared postToResend with 5s AbortSignal + jittered retry; used by both sendMagicLink and sendOperatorAlert
  • ingest/tree-builder.ts, routes/sessions.ts (DELETE), routes/wpilog.ts (post-regen GET) — switched to the charged wrappers; wrapped callers in try/catch for QuotaExceededError
  • routes/telemetry.tsres.clone() before passing to scheduleDoAlert

Test plan

  • pnpm typecheck — both web + worker clean
  • pnpm test — 125 worker + 12 web pass (was 124 worker; added a concurrent-race unit test that asserts exactly one firstBreach=true across N concurrent chargeQuota callers)
  • Post-deploy: trigger a /data batch that crosses CAP_BYTES (or use a temporarily lowered constant) and verify one and only one operator email arrives
  • Post-deploy: check Wrangler tail for [quota/scheduleAlertAndAudit] waitUntil task threw if anything fails — previously silenced

Residual findings not addressed here (advisory only)

  • Cross-tenant DoS via global counter — documented tradeoff; single stolen key can 429 every other workspace until midnight. Revisit when a per-workspace billing model exists.
  • 3 alert emails per UTC day possible — attacker crossing bytes, then Class A, then Class B caps in sequence. Collapsing to a single alerted_today flag would lose per-metric signal.
  • Pre-charge before R2 PUT — documented as "safety rail, not ledger"; acceptable drift.
  • No admin endpoint to query current counters — agent-native gap; follow-up work.

Bundles the fixes from the post-ship code review (F1-F10). The P0 bypass
was shipping live; P1s compounded operational risk.

P0: Close the quota bypass. tree-builder.ts, DELETE /api/sessions/:id,
and the post-regen wpilog GET were calling env.BLOBS.{list,get,delete}
directly. Added listBlobs / getBlob / deleteBlob wrappers in
storage/r2.ts that charge before the op, and routed all four call sites
through them. A cookie holder can no longer burn R2 ops for free.

P1: ctx.waitUntil closures in quota/http.ts now run under guardWaitUntil
which logs exceptions via console.error. Previously a throw after the
alerted_* latch flipped lost both the operator email and the audit row
for the full UTC day with no observability.

P1: Resend fetch takes a 5s AbortSignal (postToResend helper, shared by
sendMagicLink and sendOperatorAlert). Retry backoff now adds 0-30%
jitter to avoid synchronized retries during a Resend brownout.

P1: DO charge ordering. Split putBatchJsonl into encodeBatchJsonl +
putBatchJsonlBytes; SessionIngestDO now tracks chargedSeq (persisted
before the R2 PUT), so a D1 batch failure that forces a retry doesn't
double-charge the daily counter. Charge happens once per logical seq
regardless of how many retries land.

P2: First-breach race (F5) + UPSERT RETURNING (F6). chargeQuota's old
three-statement pattern (UPSERT, SELECT, UPDATE-where-0) was racy at
the alert layer — two concurrent racers could both observe
alerted_*=false and both fire sendOperatorAlert. Collapsed into one
db.batch pair: pre-snapshot SELECT + UPSERT with CASE expressions that
atomically flip each alerted_* latch when THIS charge crosses the cap.
Concurrent racers now produce exactly one firstBreach=true. Also halves
the D1 round-trip count per charge.

P2: streamSessionBatches charges Class B once (classB: keys.length)
after the list instead of per-iteration, removing N-1 serialized D1
round-trips on a wpilog regen.

P3: DO emits X-Quota-Breach-Date; worker's scheduleDoAlert uses it to
SELECT the right row even when waitUntil fires after UTC midnight.
res.clone() in /data 429 passthrough. CAP_* positive-value invariant
checked at load time. Unused exports and parameters removed.

New tests:
- daily-quota.test.ts: concurrent first-breach race produces exactly
  one alert (14 tests total)
- quota-enforcement.test.ts: documented why cookie-route integration
  tests for 429 are handled at unit level

124 -> 125 worker tests, typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jeffzakrzewski
jeffzakrzewski merged commit 5aef647 into main Apr 24, 2026
1 check passed
@jeffzakrzewski
jeffzakrzewski deleted the feat/quota-review-fixes branch April 24, 2026 02:38
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