Repository navigation
fix(cache,query,jobs,core,db)!: a cached query answered the wrong tenant, and five ways an interrupted process never recovered - #110
Merged
Conversation
…ant, and five ways an interrupted process never recovered Slices 02 and 06 of the deep-dive bug audit, minus the realtime half that landed as #107. Four agents on disjoint package sets in one checkout. The one that matters: `cacheKeyFor` keyed on name + input + tags and never on the caller, while `sql(input, ctx)` derives every tenant predicate from `ctx.actor.orgId`. Reproduced — an `org-b` actor was answered with `{id:'a1', orgId:'org-a', secret:'ALPHA'}`. The key now carries the read's authority, and a new `cache.scope` defaults to `'actor'`: the narrowest, which is what makes forgetting it safe. Three defects had to land together or make things worse. Reversing the Redis `set` ordering alone lets a bust `SREM` the membership while the later `SET` publishes a row unreachable by any tag; it ships with an `SISMEMBER` re-check. Fixing the cursor hash alone would have left a 32-bit hash over client-chosen input as the shared cache key. And an invalidation racing a read was invisible until the read cache lived inside the tier registry rather than beside it. The drain deadline flipped from opt-in to bounded by default at 25s. It was built opt-in as briefed, but `jobs` and `realtime` declare no budget, so the proven symptom — a worker pod SIGKILLed mid-job — stayed unfixed. Read X_SHUTDOWN_TIMEOUT literally: a hook past the deadline is abandoned, not stopped, and both fix: lines now name `terminationGracePeriodSeconds` beside `configureLifecycle`. Enforcement gap closed in passing: nothing caught a dropped `await`. Enabling `noFloatingPromises` cost 1.1s of lint and exposed that `dummy/social-media-clone` — the deployed app — set `"root": false` with no `extends`, so it was linted against nothing at all. BREAKING CHANGE: `drainDeadlineMs()` returns `number` always; `cacheKeyFor` takes a required fourth `authority` argument; `fingerprint` is SHA-256/16, so cursors minted before this are rejected once as X_CURSOR_INVALID; `semantic.remember` rejects a TTL the tiers would reject; `OutboxRelay.stop()` returns `Promise<void>`; `TierFailure.tier` widens to `TierLabel`. New codes: X_JOB_SLOT_LOST, X_QUERY_CACHE_TTL_INVALID. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D
…o saying 3 skipped Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 57 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 78 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (82)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
sebyx07
added a commit
that referenced
this pull request
Aug 17, 2026
auth and entity needed nothing — every finding naming them was already closed by #104 and #106, verified file-by-file before #112 was scoped. That is what took the PR from an estimated ~70 files to 43. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D
sebyx07
added a commit
that referenced
this pull request
Aug 17, 2026
…her, and ten codes paged the on-call for a caller's mistake (#112) * fix(action,http,core)!: one caller's idempotent response went to another, and ten codes paged the on-call for a caller's mistake The http and action remainder of audit slices 02 and 06, plus the gate step slice 02 asks for by name. auth and entity are absent because they are already closed — every finding naming them landed in #104 and #106, verified before this wave was scoped. `idempotencyKeyFor(actionName, key)` namespaced by action name only, with no actor anywhere, so alice's stored `charge` response was returned to bob whenever bob sent the same key; a differing payload gave bob X_IDEMPOTENCY_CONFLICT instead, which is a cross-actor denial of service against any key. And `Headers.get()` answers '' rather than null for `Idempotency-Key:`, so a blank header was a live key every blank sender shared. The status table is closed, so a code with no row falls to 500 — and stages.ts reports every status >= 500 to the error monitor. Ten caller-caused codes were in that state: a reused key, an expired cursor, a weak password, a duplicate signup. The sharpest was the framework contradicting its own published contract, action/http.ts:151 declaring '409' for X_IDEMPOTENCY_CONFLICT while the runtime answered 500. Nothing would have caught the eleventh, so the errors step gained a fourth host rule. The specified predicate — every code owned by a tier <= 4 package needs a row — flags 237 of 394, which is a step an agent disables in week one; and whether a code can reach a request is not derivable, since X_MIGRATION_DESTRUCTIVE and X_TENANCY_CROSS_DENIED are the same tier and the same shape and blanket re-exports collapse import reachability to the whole package. So it is a ratchet on the expectedRed idiom: 226 undecided codes pinned with a reason, the list may only shrink, and a pin says "nobody has decided yet" rather than "this can never reach a request". It caught two codes on its first run, both added by its own teammates in this PR. Also: a second server after a drain bound a port it could never serve from and kept accepting connections after its own stop() returned; ?__proto__= replaced the parsed query object's prototype, which also let a schema coerce an inherited function through `key in record`; an empty issues array read as validation success and reached a handler as an impossible undefined; the third copy of FNV-1a/32 over client-chosen input, here backing both the idempotency requestHash and the job dedupe key; and an unfenced settle overwriting a record already replaced. BREAKING CHANGE: `idempotencyKeyFor` takes a required third Actor argument; the stored key's shape changed, so on the shared Postgres store a retry crossing the deploy boundary re-runs the handler inside the 24h window (`truncate x_idempotency` makes that state honest); `Idempotency-Key` is now enforced at the 255 characters OpenAPI already published; action's `fingerprint` is SHA-256/16, changing job-handle.ts's dedupe key; `markReady()` throws X_LIFECYCLE_DRAINED on a drained lifecycle instead of declining silently. New codes: X_IDEMPOTENCY_KEY_INVALID, X_LIFECYCLE_DRAINED, X_ERROR_STATUS_MISSING, X_ERROR_STATUS_BACKLOG_STALE, X_ERROR_STATUS_UNKNOWN_CODE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D * docs(plan): slices 02 and 06 are done across #107, #110 and #112 auth and entity needed nothing — every finding naming them was already closed by #104 and #106, verified file-by-file before #112 was scoped. That is what took the PR from an estimated ~70 files to 43. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Slices 02 (tier 2–3 bugs) and 06 (concurrency & lifecycle) from
docs/plans/2026/08/16/101-deep-dive-bug-audit—the non-realtime half. The realtime half landed as #107.
Four agents on disjoint package sets in one checkout:
packages/jobs·packages/cache·packages/query+packages/cli·packages/core+packages/db.The one that matters
A
cache:query served one actor's rows to the next.cacheKeyForreturnedquery:<name>:<fingerprint(input)>:<tags>— no actor, no tenant — whilesql(input, ctx)is handedthe
Ctxand@ultimat3/entityderives every tenant predicate fromctx.actor.orgIdrather thanfrom the input. Reproduced: a query declaring
cache: { tags: [], ttlMs: 60_000 }and filtering onctx.actor.orgIdanswered anorg-bactor with{id:'a1', orgId:'org-a', secret:'ALPHA'}.Cross-tenant disclosure with no attacker: two logged-in users and one cached query.
The key now carries the read's authority —
query:<name>:<authority>:<fingerprint>:<tags>, theauthority being
JSON.stringify([kind, id, orgId ?? null]). JSON rather than a joined stringbecause an actor id is app data that may contain the separator, and a value that can spell a
boundary can spell someone else's — the rule
@ultimat3/entity'sscopeKeyalready states. A newcache.scopepicks the sharing width:'actor'(default),'tenant','global'. The defaultis the narrowest, which is what makes forgetting safe; a fourth scope is an
assertNevercompileerror. The request memo was never affected — it keys on
Ctxidentity.Findings, one row each
querycache-authority.test.tscli/querycache.invalidatesbusted nothing without Redis;invalidateQueryTagshad zero production callersdev-cache.test.tscache/queryinvalidation-race.test.ts,cache-fence.test.tscacheSMEMBERS, orphaning members on a refusedDELcachesetwrote the value before the tagSADD— and reversing it alone is worseredis-ordering.test.tscachetiers.test.tsjobsfleetSlots.acquire()leaked the limiter lease → theworkerrole died permanently and silentlyworker-fleet-slots.test.tsjobsX_JOB_SLOT_LOSTjobsoptions.context()ran after the heartbeat and slot timers started — app code that throws left both running foreverworker-run.tsjobsOutboxRelay.stop()returned underneath the pass in flightoutbox.test.tsjobsx jobs lsagainstx devpaged the oldest hundred rows — the limit landed after the sortdriver-parity.test.ts(new mechanism)corejobs,realtime) declared nonelifecycle.test.tsdbpgliteobserver straggler ran against a closed handle after the transaction settledpglite-observer.test.tsqueryfingerprintwas FNV-1a/32 over client-chosen input, backing both cursors and the shared cache keystable.test.tscachesemantic.rememberbypassedassertTtlsemantic.test.tscliawaitnoFloatingPromisesenforcedclidummy/social-media-clone/biome.jsonset"root": falsewith noextendsscripts--workers 8, twiceThe intermittent, finally explained
An unexplained shard failure recurred five times across this audit. It is four
scripts/testspaying a whole-repo cost against bun's default 5000ms budget while eight shards compete for the
same cores — and which shard a file lands in depends on the file count, so it read as flake rather
than as a slow test. It surfaced now because this PR adds files: every repo-scanning test got
slower.
The repo had already diagnosed this once and fixed it for
error-contract.test.ts, whose commentends "Same shape as
scripts/verify.test.ts" — naming one of the files that kept failing. Thediagnosis was written down and never applied. Both
collectSourceFilestests moved, not only theone observed failing, because they call the identical scan and fixing one relocates the failure.
Proven by reproducing under the gate's own command and re-running after the fix, once plain and
once with four extra CPU hogs: 16 of 16 shard processes exit 0.
One correction it forced on my own brief:
verify.test.ts's cost is not a directory walk. It runson a small temp dir; the expense is
registeredErrorCodes()dynamically importing all 29 packagesregardless.
Premises the hive falsified
Nine brief premises were wrong and the agents said so with the line. The ones that changed the fix:
setordering." Half a fix, and worse alone:SADD-then-SETwith nore-check lets the bust
SREMthe membership and the laterSETpublish a row unreachable byany tag — permanently uninvalidatable. Shipped with an
SISMEMBERre-check that deletes thevalue it just wrote when a bucket says it is gone, and only a literal
0counts as evidence.queryHash." Too narrow.fingerprintalso backscacheKeyFor, so fixing only thecursor path would have left a 32-bit hash over client-chosen input as the shared cache entry
key — strictly worse than the case named.
attemptnegative." Impossible:nackis fenced onstate === 'running', which onlyclaimsets, andclaimincrements.configureLifecycle({deadlineMs})is effectively unused." False —http/src/server.ts:97declares one on every
createServer.X_SHUTDOWN_TIMEOUTis new." Already shipped atcore/src/error-codes.ts:61.bestEffortis exported fromtier-failures.ts." It was not exported at all, and itsTierNameparameter has no rung for query's read tier — henceTierLabel.Date.now()in four cache files." The list is empty; fixed in an earlier PR.One agent also overturned my call: I leaned "loud is usually right" on the pglite straggler and D
showed with evidence that it is not. And D caught its own overclaim — it reported the whole-drain
budget as proved when the mutation passed 18/18; it was implemented but unenforced until the test at
lifecycle.test.ts:263.The coordinator decision
The drain deadline is now bounded by default (25s) rather than opt-in. D built it opt-in as
briefed, then showed that
jobsandrealtimedeclare no budget — so the proven symptom, a workerpod
SIGKILLed mid-job, stayed unfixed. Shipping a mechanism that does not fix the symptom whileclaiming the deadline works is a claim wider than what is enforced: the third instance of that class
this run, after "time-to-consistent" and "0 lost". The flip broke nothing — jobs 411 pass, realtime
675 pass.
Read
X_SHUTDOWN_TIMEOUTliterally: a hook past the deadline is abandoned, not stopped. It isstill running when the process exits. The framework cannot cancel app code it did not write, and
both
fix:lines now name the pair that must move together —configureLifecycle({ deadlineMs })and
terminationGracePeriodSeconds.Enforcement gap closed in-PR
Nothing caught a dropped
await. MeasurednoFloatingPromisesat 2 violations across 3560 files in7s and enabled it rather than deferring (
bun run lint6.0s → 7.1s). Enabling it is what exposedfinding 17: the deployed demo app inherited none of the repo's lint rules.
Breaking
Six, all in
CHANGELOG.mdunder### Changed.[Unreleased]already carries a dozen BREAKINGentries from #105–#107, so the next release is a major regardless and none of these forces a new
decision.
drainDeadlineMs()returnsnumberalways ·cacheKeyFortakes a required fourth argument ·fingerprintis SHA-256/16 so pre-existing cursors are rejected once asX_CURSOR_INVALID·semantic.rememberrejects a TTL the tiers would reject ·OutboxRelay.stop()returnsPromise<void>·TierFailure.tierwidens toTierLabel.Codes
X_JOB_SLOT_LOST,X_QUERY_CACHE_TTL_INVALID— both inwiki/Error-Codes.md, manifestregenerated (389 codes, 29 packages).
Known gaps recorded, not fixed
and bust on the other. Cross-node needs a Redis-side epoch and a wire change.
createCacheStackhas zero production callers, so its copy of the fence is dormant. The livefenced path is
runQuery → readRows → readThrough → fill.recentTierFailures()has no reader outsidecache, and/_x's invalidations source isunwired— routed to the tiers 4–5 slice.invalidateWireTagshas zero callers repo-wide, but it is not dead: it is the wire-form doorx cache bustwill call, andreceiveInvalidationBroadcastis the sibling door that already hasa production caller. Deleting it would leave the planned command with no entry point. Recorded
for the dead-code slice to weigh, not fixed here.
Gate
bun run verify— 17 steps.bun run scripts/reference-app-gate.ts— both tracked apps on theirratchet.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.