fix(tier3-4): query cache invalidated nothing, precache always missed, run-once fired 24 times - #82
Merged
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes 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 (69)
Comment |
…, run-once fired 24 times Tiers 3–4 of a four-PR audit sweep. Every finding was reproduced by running the code before it was fixed, and every fix ships a test verified failure-first by reverting the source change and watching it go red. **Stacked on #80** — the read cache now uses `@ultimat3/cache`'s `LruCache` and its TTL rule, so this needs the tier-1 change beneath it. ## The invalidation that never invalidated `query/src/cache.ts:50` — **the default read-cache tier was never registered with `@ultimat3/cache`, so `invalidateQueryTags()` — and therefore every action's `cache.invalidates` — evicted nothing.** With no `ttlMs` the entry was immortal. Measured: `readThrough` → `1`; `invalidateQueryTags([tag('post')])`; `registeredTiers()` is `[]`; second `readThrough` → still `1`, entry `{value:1, expiresAt:null}`. In an app: create a post, read the list, publish through an action declaring `cache: { invalidates: [tag.post] }`, read again — **the pre-publish list is served for the life of the process.** `setReadCache` was exported and called from nowhere in the repo, and `cache.test.ts` had no invalidation test. The module header claimed "Invalidation is never local — it goes through @ultimat3/cache so an action's `invalidates` and a query's `tags` meet in one graph." That is now true. The tier is also bounded (32 MiB LRU) and defaults a 60s TTL, where it was previously an unbounded map of immortal entries keyed on every distinct input — a paginated feed over 10k orgs wrote 10k+ permanent, unevictable entries per process. ## Other correctness | Where | Defect | |---|---| | `pwa/src/service-worker.ts:211` | **Every precached byte was a permanent miss.** Entries were written under `url + '?v=' + revision`, but every strategy read `caches.match(req)` on the bare URL with the default `ignoreSearch:false`. Offline, a route with `offline:'precache'` missed its own precache and fell through to the offline document; online, every precached asset was downloaded twice. Entries are now re-keyed onto the bare URL after `addAll` fetches them revision-addressed, preserving its all-or-nothing failure. The new test *executes* the emitted `sw.js` against stub `caches`/`fetch`. | | `jobs/src/scheduler.ts:206` | **`catchUp: 'run-once'` fired once per tick until every missed occurrence drained.** Measured: an hourly task with the scheduler down 24h produced **24 dispatches**, not one — a nightly digest becomes 24 digests a second apart. `task.ts:23` documents the opposite and no test asserted it. Note the fix marks the watermark at `at`, not at `due[last]` as first proposed: `maxCatchUp` truncates `due` to 10, so `due[last]` would still have fired three times. | | `ai/src/llm.ts:208` | The repair turn appended `{role:'assistant', content: result.text}` — the **empty string** whenever the model answered through the `respond` tool, which is the dominant path. The Messages API rejects an empty text block, so a schema-invalid response produced `X_AI_PROVIDER_UNAVAILABLE` from a 400 instead of the repair it was trying to do, and the `tool_use` block was dropped from the replayed history. | | `ai/src/budget.ts:125` | `reserve()` checked the ceiling but **debited nothing** — only `record()` did — so concurrent calls under one ledger all passed the same check. Three parallel `summarize` calls estimating 4k tokens each all read `spent() === 0`, all passed, and recorded 12k against a 10k ceiling. The "un-bypassable" org budget was bypassable by parallelism. `reserve()` now debits and returns a reservation that `record()` reconciles and `release()` returns; reservations serialise on a turnstile because check-then-debit spans an `await`. | | `realtime/src/live-query.ts:388` | **`sid` was client-supplied and unvalidated.** Socket B could reuse socket A's `sid`: `#bySid[S]` then pointed at B while A's subscription stayed in the entry's subscriber set, so A's disconnect freed nothing — the entry, its matcher and its shared window leaked permanently and every subsequent change fanned out to a dead socket. A `{op:'drop', sid:S}` from B also killed A's live stream, with no error on either side. Subscriptions are now keyed by `(socketId, sid)`, and a sid the same socket already holds is refused. | | `jobs/src/outbox.ts:285` | The relay's interval body was `void tick().finally(...)` with **no `.catch`** — a rejection from `store.claim()` is an unhandled rejection, and Bun's default terminates the process. One connection-pool timeout during a failover killed the worker with staged, unpublished jobs. Every other loop in the package already used `.catch(...)` first. | | `jobs/src/outbox.ts:55` | The memory store never removed published rows, so `claim()` and `pendingCount()` walked every row ever enqueued on each 200ms tick and no payload was ever freed. | | `jobs/src/steps.ts:173` | `claimName` was an O(n) `Array.includes` over every step name in the attempt — a `backfill()` over 1M rows at batch 50 is 20,000 steps, so ~200M string comparisons plus a 20k-entry array carried into `x jobs show`. | | `action/src/naming.ts:59` | Two distinct actions could derive **one** HTTP path with nothing refusing it — `archiveOrder` and `archiveOrders` both derive `POST /api/orders/archive`. `X_ACTION_DUPLICATE` only guards names, so both registered and whichever the router seated last silently shadowed the other, while the shadowed action's OpenAPI operation and MCP tool kept advertising it. Now `X_ACTION_PATH_DUPLICATE`. | | `render/src/render-stream.ts:81` | A client disconnecting mid-stream produced one unhandled rejection per late hole and left every hole running with nowhere to write. The source gained `cancel()` to abort the holes, and writes are guarded on a closed controller. | | `render/src/render-isr.ts:224` | `attach()`'s returned detach unregistered dependents but never cleared the revalidator it installed, so a hot reload left the old controller receiving revalidations — the new one's pages never went stale and the old store was never collected. | | `render/src/render-isr.ts:41` | The default ISR store was an unbounded map keyed by rendered path on a route table supporting `:params` — a crawler hitting 100k slugs held 100k HTML strings for the process's lifetime. Now LRU-capped. | | `realtime/src/client.ts:153` | `connect()` replaced `#socket` without closing the previous one, so the orphan's frame handler kept mutating live registrations — patches applied twice and the server held two sockets per client until the tab closed. | | `mcp/src/transport-http.ts:75` | The body was parsed **before** the token was resolved, contradicting the stated "401 before parsing" property: an invalid token got `400 parse error` for malformed JSON and `401` for well-formed, which is exactly the oracle the comment says the 401 exists to remove. | | `ai/src/models.ts:12` | `LlmRefusedError`'s alternative was `MODEL_IDS.find(id => id !== result.model)` — the first id that differs — so a refusal on the most capable model suggested retrying on a **weaker** one. Now walks the capability ladder upward and drops the suggestion when there is no rung above. | | `ai/src/models.ts:133` | `reasoningBody` always emitted `thinking: {type:'adaptive'}` for any adaptive-capable model, including when the declaration asked for neither — while the comment two lines above claims "a control the caller never asked for is OMITTED rather than defaulted". Harmless (adaptive is the server default), but the stated invariant was not the code's. | | `jobs/src/backfill-pass-fixture.ts:146` | A bare `Error` in shipped `src/`. Now a named class, and `"!src/**/*-fixture.ts"` keeps test material out of the tarball. | ## Dropped — the audit was wrong The sweep flagged the manifest as omitting `mutator: true`. It does not: the brand survives registration, `describeAction` reads it, `sources.ts` emits it, `build.ts` carries it through, and `examples/dummy/x.manifest.json` **already carries `"mutator": true`** on all three mutators. No top-level `mutators` key exists, but that is by design — a mutator *is* an action on the same authz path, and a second list would be the same objects twice. No change made. ## Not done, and why `claude-fable-5` was **not** added to the model catalogue. Its pricing and limits are verifiable, but it can never disable thinking, which `ModelReasoning.disableThinkingUpTo: undefined` currently reads as "every effort may disable" — expressing it needs a shape change plus decisions about retention and refusal classifiers. The underlying bug (suggesting a downgrade) is fixed by capability rank instead, which is the safer half. Rather than invent a capability row, this is left named. `skip`-mode catch-up has the same `maxCatchUp` truncation shape and was left alone as out of scope: after a >10-occurrence outage it fires occurrence 10, then 20, then 24 — three dispatches, two of them stale. Worth its own finding. ## New error codes `X_ACTION_PATH_DUPLICATE`, `X_SUBSCRIPTION_ID_TAKEN` — both documented in `wiki/Error-Codes.md`, registered, in the manifest. ## Semver — needs a decision before the next release `LiveQueryRegistry.unsubscribe` and `.subscription` gained a `socketId` parameter. Breaking on an exported class; no in-repo caller outside realtime's own tests. Also: a `cache:` query with no `ttlMs` now expires at 60s where it previously never did. ## Gate `bun run verify` green on this branch: 14/17, same three structural skips as `main`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sebyx07
force-pushed
the
fix/tier34-runtime
branch
from
August 15, 2026 07:23
bc57edb to
fa0a0b0
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.
Tiers 3–4 of a four-PR audit sweep. Every finding was reproduced by running the code before it was fixed, and every fix ships a test verified failure-first by reverting the source change and watching it go red.
Stacked on #80 — the read cache now uses
@ultimat3/cache'sLruCacheand its TTL rule, so this needs the tier-1 change beneath it.The invalidation that never invalidated
query/src/cache.ts:50— the default read-cache tier was never registered with@ultimat3/cache, soinvalidateQueryTags()— and therefore every action'scache.invalidates— evicted nothing. With nottlMsthe entry was immortal.Measured:
readThrough→1;invalidateQueryTags([tag('post')]);registeredTiers()is[]; secondreadThrough→ still1, entry{value:1, expiresAt:null}. In an app: create a post, read the list, publish through an action declaringcache: { invalidates: [tag.post] }, read again — the pre-publish list is served for the life of the process.setReadCachewas exported and called from nowhere in the repo, andcache.test.tshad no invalidation test.The module header claimed "Invalidation is never local — it goes through @ultimat3/cache so an action's
invalidatesand a query'stagsmeet in one graph." That is now true. The tier is also bounded (32 MiB LRU) and defaults a 60s TTL, where it was previously an unbounded map of immortal entries keyed on every distinct input — a paginated feed over 10k orgs wrote 10k+ permanent, unevictable entries per process.Other correctness
pwa/src/service-worker.ts:211url + '?v=' + revision, but every strategy readcaches.match(req)on the bare URL with the defaultignoreSearch:false. Offline, a route withoffline:'precache'missed its own precache and fell through to the offline document; online, every precached asset was downloaded twice. Entries are now re-keyed onto the bare URL afteraddAllfetches them revision-addressed, preserving its all-or-nothing failure. The new test executes the emittedsw.jsagainst stubcaches/fetch.jobs/src/scheduler.ts:206catchUp: 'run-once'fired once per tick until every missed occurrence drained. Measured: an hourly task with the scheduler down 24h produced 24 dispatches, not one — a nightly digest becomes 24 digests a second apart.task.ts:23documents the opposite and no test asserted it. Note the fix marks the watermark atat, not atdue[last]as first proposed:maxCatchUptruncatesdueto 10, sodue[last]would still have fired three times.ai/src/llm.ts:208{role:'assistant', content: result.text}— the empty string whenever the model answered through therespondtool, which is the dominant path. The Messages API rejects an empty text block, so a schema-invalid response producedX_AI_PROVIDER_UNAVAILABLEfrom a 400 instead of the repair it was trying to do, and thetool_useblock was dropped from the replayed history.ai/src/budget.ts:125reserve()checked the ceiling but debited nothing — onlyrecord()did — so concurrent calls under one ledger all passed the same check. Three parallelsummarizecalls estimating 4k tokens each all readspent() === 0, all passed, and recorded 12k against a 10k ceiling. The "un-bypassable" org budget was bypassable by parallelism.reserve()now debits and returns a reservation thatrecord()reconciles andrelease()returns; reservations serialise on a turnstile because check-then-debit spans anawait.realtime/src/live-query.ts:388sidwas client-supplied and unvalidated. Socket B could reuse socket A'ssid:#bySid[S]then pointed at B while A's subscription stayed in the entry's subscriber set, so A's disconnect freed nothing — the entry, its matcher and its shared window leaked permanently and every subsequent change fanned out to a dead socket. A{op:'drop', sid:S}from B also killed A's live stream, with no error on either side. Subscriptions are now keyed by(socketId, sid), and a sid the same socket already holds is refused.jobs/src/outbox.ts:285void tick().finally(...)with no.catch— a rejection fromstore.claim()is an unhandled rejection, and Bun's default terminates the process. One connection-pool timeout during a failover killed the worker with staged, unpublished jobs. Every other loop in the package already used.catch(...)first.jobs/src/outbox.ts:55claim()andpendingCount()walked every row ever enqueued on each 200ms tick and no payload was ever freed.jobs/src/steps.ts:173claimNamewas an O(n)Array.includesover every step name in the attempt — abackfill()over 1M rows at batch 50 is 20,000 steps, so ~200M string comparisons plus a 20k-entry array carried intox jobs show.action/src/naming.ts:59archiveOrderandarchiveOrdersboth derivePOST /api/orders/archive.X_ACTION_DUPLICATEonly guards names, so both registered and whichever the router seated last silently shadowed the other, while the shadowed action's OpenAPI operation and MCP tool kept advertising it. NowX_ACTION_PATH_DUPLICATE.render/src/render-stream.ts:81cancel()to abort the holes, and writes are guarded on a closed controller.render/src/render-isr.ts:224attach()'s returned detach unregistered dependents but never cleared the revalidator it installed, so a hot reload left the old controller receiving revalidations — the new one's pages never went stale and the old store was never collected.render/src/render-isr.ts:41:params— a crawler hitting 100k slugs held 100k HTML strings for the process's lifetime. Now LRU-capped.realtime/src/client.ts:153connect()replaced#socketwithout closing the previous one, so the orphan's frame handler kept mutating live registrations — patches applied twice and the server held two sockets per client until the tab closed.mcp/src/transport-http.ts:75400 parse errorfor malformed JSON and401for well-formed, which is exactly the oracle the comment says the 401 exists to remove.ai/src/models.ts:12LlmRefusedError's alternative wasMODEL_IDS.find(id => id !== result.model)— the first id that differs — so a refusal on the most capable model suggested retrying on a weaker one. Now walks the capability ladder upward and drops the suggestion when there is no rung above.ai/src/models.ts:133reasoningBodyalways emittedthinking: {type:'adaptive'}for any adaptive-capable model, including when the declaration asked for neither — while the comment two lines above claims "a control the caller never asked for is OMITTED rather than defaulted". Harmless (adaptive is the server default), but the stated invariant was not the code's.jobs/src/backfill-pass-fixture.ts:146Errorin shippedsrc/. Now a named class, and"!src/**/*-fixture.ts"keeps test material out of the tarball.Dropped — the audit was wrong
The sweep flagged the manifest as omitting
mutator: true. It does not: the brand survives registration,describeActionreads it,sources.tsemits it,build.tscarries it through, andexamples/dummy/x.manifest.jsonalready carries"mutator": trueon all three mutators. No top-levelmutatorskey exists, but that is by design — a mutator is an action on the same authz path, and a second list would be the same objects twice. No change made.Not done, and why
claude-fable-5was not added to the model catalogue. Its pricing and limits are verifiable, but it can never disable thinking, whichModelReasoning.disableThinkingUpTo: undefinedcurrently reads as "every effort may disable" — expressing it needs a shape change plus decisions about retention and refusal classifiers. The underlying bug (suggesting a downgrade) is fixed by capability rank instead, which is the safer half. Rather than invent a capability row, this is left named.skip-mode catch-up has the samemaxCatchUptruncation shape and was left alone as out of scope: after a >10-occurrence outage it fires occurrence 10, then 20, then 24 — three dispatches, two of them stale. Worth its own finding.New error codes
X_ACTION_PATH_DUPLICATE,X_SUBSCRIPTION_ID_TAKEN— both documented inwiki/Error-Codes.md, registered, in the manifest.Semver — needs a decision before the next release
LiveQueryRegistry.unsubscribeand.subscriptiongained asocketIdparameter. Breaking on an exported class; no in-repo caller outside realtime's own tests. Also: acache:query with nottlMsnow expires at 60s where it previously never did.Gate
bun run verifygreen on this branch: 14/17, same three structural skips asmain.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.