diff --git a/framework.manifest.json b/framework.manifest.json index 208bfd5f..122b85d6 100644 --- a/framework.manifest.json +++ b/framework.manifest.json @@ -1,6 +1,6 @@ { "version": 1, - "buildId": "446f7ca0d3c11b3b154cec2a0b2989d7c886a6b723dfe845742f9ce6337cfe5b", + "buildId": "f9c3583fd8cdc1bf7ccd795fa38ae4cde5e2509d3ca5c09c123f702f6c5a0275", "tiers": { "0": [ "core", @@ -240,6 +240,11 @@ "owner": "action", "at": "packages/action/src/errors.ts" }, + { + "code": "X_ACTION_PATH_DUPLICATE", + "owner": "action", + "at": "packages/action/src/errors.ts" + }, { "code": "X_ACTION_POLICY_MISSING", "owner": "action", @@ -1525,6 +1530,11 @@ "owner": "render", "at": "packages/render/src/errors.ts" }, + { + "code": "X_SUBSCRIPTION_ID_TAKEN", + "owner": "realtime", + "at": "packages/realtime/src/errors.ts" + }, { "code": "X_SUBSCRIPTION_LIMIT", "owner": "realtime", diff --git a/packages/action/CLAUDE.md b/packages/action/CLAUDE.md index 8521c74a..23fc6f89 100644 --- a/packages/action/CLAUDE.md +++ b/packages/action/CLAUDE.md @@ -129,6 +129,12 @@ Owns the `action` + `mutator` primitives and their six projections. Tier 3. same-named helpers overwrite each other with no `X_ACTION_DUPLICATE` to raise. The type does the same filter, so `rpc()` offers only what registered. - `rpc` is the only name for the map-wide typed client. There is no `createClient` alias. +- **`registerAction` guards the derived PATH as well as the name.** `X_ACTION_DUPLICATE` only ever + asked about the name, so `archiveOrder` and `archiveOrders` — one route, by `pluralize`'s + deliberate "a trailing `s` is already plural" rule — both registered and both projected: the + router table seated whichever came last and the other was unreachable over HTTP while its + OpenAPI operation and MCP tool still advertised it. `paths` is a second index, cleared by + `resetRegistry` with the first, and the refusal is `X_ACTION_PATH_DUPLICATE`. - No policy at registration → `X_ACTION_POLICY_MISSING`. No exceptions, no flag. - `serializeOpenApi` output must be byte-stable: sorted keys, sorted registry, no clock. - `client.ts` stays free of server imports — it is bundled into the browser. diff --git a/packages/action/README.md b/packages/action/README.md index 6d46b3a4..9a8e7943 100644 --- a/packages/action/README.md +++ b/packages/action/README.md @@ -86,7 +86,9 @@ Names come from **export names** — that is what makes the path, the tool name OpenAPI `operationId` derivable everywhere without a second declaration. Registration stamps the name onto the action the module exported, so the binding you imported is the one that projects; a projection attempted before boot is `X_ACTION_UNREGISTERED`. Two -features exporting one name collide with `X_ACTION_DUPLICATE` rather than merging. +features exporting one name collide with `X_ACTION_DUPLICATE` rather than merging, and two +names deriving one route collide with `X_ACTION_PATH_DUPLICATE` — `pluralize` leaves a trailing +`s` alone, so `archiveOrder` and `archiveOrders` are two exports and one `POST /api/orders/archive`. `registerActions` / `registerQueries` are what `defineApi` composes. An app calling them directly is a second path. @@ -226,6 +228,7 @@ never a pass — the assertion says which code got in the way and names `input:` | Code | When | Fix | |---|---|---| | `X_ACTION_DUPLICATE` | two actions registered under one name | rename one export | +| `X_ACTION_PATH_DUPLICATE` | two actions derive one HTTP path (`archiveOrder` / `archiveOrders`) | rename one export | | `X_ACTION_POLICY_MISSING` | registration without `policy:` | add `policy: can('…')` | | `X_INPUT_INVALID` | input failed the Standard Schema | `x actions describe --json` | | `X_IDEMPOTENCY_CONFLICT` | key reused with a new payload / still in flight | new key, or retry later | diff --git a/packages/action/src/errors.ts b/packages/action/src/errors.ts index 81d99c53..f8654110 100644 --- a/packages/action/src/errors.ts +++ b/packages/action/src/errors.ts @@ -24,6 +24,7 @@ const docs = errorDocsUrl; */ const OWNED_TITLES: Readonly> = { X_ACTION_DUPLICATE: 'two actions are registered under one name', + X_ACTION_PATH_DUPLICATE: 'two actions derive one HTTP path', X_ACTION_FOREIGN: 'a value that is not an action was projected as one', X_ACTION_POLICY_MISSING: 'an action was registered without a policy', X_ACTION_UNREGISTERED: 'an action was projected before it was registered', @@ -128,6 +129,23 @@ export class ActionDuplicateError extends UltimateError { } } +/** + * Two distinct action names, one derived route. `X_ACTION_DUPLICATE` guards the NAME; nothing + * guarded the path, so `archiveOrder` and `archiveOrders` both registered, both projected to + * `POST /api/orders/archive`, and whichever the router seated last silently shadowed the other — + * while the shadowed action's OpenAPI operation and MCP tool went on advertising it. + */ +export class ActionPathDuplicateError extends UltimateError { + constructor(input: { name: string; existing: string; path: string }) { + super({ + code: 'X_ACTION_PATH_DUPLICATE', + cause: `actions "${input.name}" and "${input.existing}" both derive ${input.path}`, + fix: `rename one export so the two derive different paths — x actions list --json prints every derived route`, + docs: docs('X_ACTION_PATH_DUPLICATE'), + }); + } +} + export class ActionPolicyMissingError extends UltimateError { constructor(name: string) { super({ diff --git a/packages/action/src/index.ts b/packages/action/src/index.ts index 434019ea..8c446e4b 100644 --- a/packages/action/src/index.ts +++ b/packages/action/src/index.ts @@ -44,6 +44,7 @@ export { ActionDeniedError, ActionDuplicateError, ActionForeignError, + ActionPathDuplicateError, ActionPolicyMissingError, ActionUnregisteredError, ContractDriftError, diff --git a/packages/action/src/registry.test.ts b/packages/action/src/registry.test.ts index 0bb71f3c..d683ae0c 100644 --- a/packages/action/src/registry.test.ts +++ b/packages/action/src/registry.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, test } from 'bun:test'; import { can } from '@ultimat3/policy'; import { t } from '@ultimat3/schema'; import { type ActionDef, action } from './action'; +import { derivePath } from './naming'; import { describeActions, getAction, @@ -119,3 +120,53 @@ describe('registry', () => { expect(getAction('publishPost')).toBeUndefined(); }); }); + +describe('one derived path, one action', () => { + beforeEach(() => { + resetRegistry(); + }); + + const declare = define; + + // `pluralize` leaves a trailing `s` alone by design, so these are two names and one route. + // `X_ACTION_DUPLICATE` only guards names: both registered, both projected, and the router + // seated whichever came last — the other unreachable over HTTP while its OpenAPI operation + // and MCP tool went on advertising it. + test('refuses a second action deriving a path another already owns', () => { + registerAction('archiveOrder', declare()); + + expect(() => registerAction('archiveOrders', declare())).toThrow('X_ACTION_PATH_DUPLICATE'); + expect(getAction('archiveOrders')).toBeUndefined(); + expect(derivePath('archiveOrder').path).toBe(derivePath('archiveOrders').path); + }); + + test('the refusal names both actions and the path they collide on', () => { + registerAction('archiveOrder', declare()); + const failure = (() => { + try { + registerAction('archiveOrders', declare()); + return undefined; + } catch (error: unknown) { + return error as { cause?: string }; + } + })(); + + expect(failure?.cause).toBe( + 'actions "archiveOrders" and "archiveOrder" both derive /api/orders/archive', + ); + }); + + test('two actions with different paths both register', () => { + registerAction('archiveOrder', declare()); + registerAction('publishOrder', declare()); + + expect(getAction('archiveOrder')).toBeDefined(); + expect(getAction('publishOrder')).toBeDefined(); + }); + + test('re-registering the same action under the same name is still one registration', () => { + const target = declare(); + registerAction('archiveOrder', target); + expect(() => registerAction('archiveOrder', target)).not.toThrow(); + }); +}); diff --git a/packages/action/src/registry.ts b/packages/action/src/registry.ts index 19f4f242..bfd576c0 100644 --- a/packages/action/src/registry.ts +++ b/packages/action/src/registry.ts @@ -6,10 +6,19 @@ import type { ActionDescriptor, AnyAction } from './action'; import { isAction, nameAction } from './action'; -import { ActionDuplicateError, ActionPolicyMissingError } from './errors'; +import { ActionDuplicateError, ActionPathDuplicateError, ActionPolicyMissingError } from './errors'; +import { derivePath } from './naming'; const registry = new Map(); +/** + * Derived route -> the action name that owns it. A second index because the name is not the + * path: `pluralize` leaves a trailing `s` alone by design, so `archiveOrder` and `archiveOrders` + * are two names and one route. Nothing downstream can refuse that — the router seats whichever + * came last and the shadowed action stays in the OpenAPI document and the MCP tool list. + */ +const paths = new Map(); + /** * Register one action under an explicit name. The name lands on the action you * passed, so the module's own export is projectable after boot and there is no @@ -28,8 +37,14 @@ export function registerAction(name: string, target: A): A if (target.policy === undefined || target.policy === null) { throw new ActionPolicyMissingError(name); } + const { path } = derivePath(name); + const owner = paths.get(path); + if (owner !== undefined && owner !== name) { + throw new ActionPathDuplicateError({ name, existing: owner, path }); + } const named = nameAction(target, name); registry.set(name, named); + paths.set(path, name); return named; } @@ -63,6 +78,7 @@ export function describeActions(): readonly ActionDescriptor[] { /** Test-only. Production registers once at boot and never unregisters. */ export function resetRegistry(): void { registry.clear(); + paths.clear(); } function byName(a: readonly [string, AnyAction], b: readonly [string, AnyAction]): number { diff --git a/packages/ai/CLAUDE.md b/packages/ai/CLAUDE.md index 566931e7..28069817 100644 --- a/packages/ai/CLAUDE.md +++ b/packages/ai/CLAUDE.md @@ -78,13 +78,24 @@ local `=== true`. An in-app agent and an external one must be offered exactly th - A budget throws `X_AI_BUDGET_EXCEEDED` **before** the provider call. Never truncate. - Anthropic body: no `temperature`/`top_p`/`top_k`, no `budget_tokens`, `effort` inside `output_config`. All 400s otherwise. +- **`MODEL_IDS` is ordered MOST CAPABLE FIRST, and `moreCapableThan` is the only reader of that + order.** A refusal is worth retrying upward and nowhere else: `MODEL_IDS.find((id) => id !== + refused)` answered a refusal on the default model with the next entry DOWN, so `X_LLM_REFUSED`'s + fix line told an operator to buy the same refusal from a weaker model. The ladder needs no + second list — the ordering is the catalogue's own, and `models.test.ts` pins it against the + prices. When there is no rung above, `alternative` is `undefined` and the fix line drops the + suggestion rather than inventing a downgrade. - **The reasoning half of the body is PER MODEL, and `models.ts` owns which model takes what.** `output_config.effort` and adaptive thinking arrived with 4.6, so one body sent to the whole catalogue is a guaranteed 400 on the oldest entry — which is how `claude-haiku-4-5` shipped blessed and uncallable. A control the caller never asked for is omitted; a control they DID ask for is refused locally with `X_AI_REQUEST_INVALID`, never dropped, because a declaration reading `effort: 'max'` that quietly runs at the default is the failure nobody can see. Adding - a model is a row in `MODELS`, never an `if` in the request builder. + a model is a row in `MODELS`, never an `if` in the request builder. Omission is literal: an + absent `thinking` sends no block at all, where `(thinking ?? 'adaptive')` sent an adaptive one + for every adaptive-capable model — harmless on the wire, since adaptive is the server default, + but it made a defaulted control indistinguishable from a declared one, which is the whole + distinction this rule draws. - Model IDs are exact aliases. Never append a date suffix. - The introductory price on a model is deliberately not modelled. A price that lapses on a date makes a recorded cost depend on when it was read, and under-reporting spend after the lapse is diff --git a/packages/ai/README.md b/packages/ai/README.md index 510f3a6d..a23639ba 100644 --- a/packages/ai/README.md +++ b/packages/ai/README.md @@ -38,6 +38,9 @@ const answer = await ai.scope({ actorKey: actor.id, orgKey: actor.orgId }, async | A control the model lacks is **refused**, never dropped | a declaration reading `effort: 'max'` that quietly runs at the default is the failure nobody can see | | A control nobody asked for is **omitted**, never defaulted | a default sent as a request is indistinguishable on the wire from one that was declared | | A refusal is `X_LLM_REFUSED`, not a schema failure | it is a 200 with no answer in it, and a repair turn buys the same refusal again | +| The refusal's `alternative` is only ever a **more capable** model | `MODEL_IDS` is ordered most-capable-first and `moreCapableThan` walks it upward; retrying a refusal on a weaker model is the one retry that cannot help, so an unbeatable model gets no suggestion at all | +| The repair turn replays the tool call's arguments, never an empty `text` | an answer through the `respond` tool leaves `text` empty, and an empty text block is a 400 — the repair came back as `X_AI_PROVIDER_UNAVAILABLE` | +| `reserve()` **debits** the estimate and takes a turn | three concurrent calls otherwise read the same `spent()`, all pass, and all three record against a ceiling only one of them fitted; `record` reconciles and `release` gives it back | | A refusal is never cached | a cached one keeps serving a classifier decision after the prompt was fixed | | Retries use **full jitter** | synchronised retries from N workers reproduce the rate limit | | A 4xx is never retried | the same body gets the same rejection and burns the budget | diff --git a/packages/ai/src/budget.test.ts b/packages/ai/src/budget.test.ts index 89d78f5d..c04ce9f7 100644 --- a/packages/ai/src/budget.test.ts +++ b/packages/ai/src/budget.test.ts @@ -6,6 +6,7 @@ */ import { describe, expect, test } from 'bun:test'; +import type { BudgetStore } from './budget'; import { BudgetLedger, estimateSpend, MemoryBudgetStore } from './budget'; import type { GenerateRequest } from './provider'; @@ -105,10 +106,13 @@ describe('derive tightens, never widens', () => { const parent = new BudgetLedger({ limits: { actor: 1_000 }, actorKey: 'actor:u1', store }); const child = parent.derive({ costPerCall: usd(500) }); - await child.reserve(estimateSpend(request('hi'))); + // The reservation is handed to `record`, so the estimate it debited is reconciled away and + // what remains is the provider's real count. + const reservation = await child.reserve(estimateSpend(request('hi'))); await child.record( { inputTokens: 900, outputTokens: 50, cacheReadTokens: 0, cacheWriteTokens: 0 }, usd(1), + reservation, ); expect(store.spent('actor:u1')).toBe(950); await expect(child.reserve(estimateSpend(request('hi')))).rejects.toMatchObject({ @@ -118,8 +122,86 @@ describe('derive tightens, never widens', () => { test('an unset scope on either side stays unset rather than defaulting to zero', async () => { const child = new BudgetLedger({ limits: {} }).derive({}); - await expect( - child.reserve(estimateSpend(request('x'.repeat(100_000)))), - ).resolves.toBeUndefined(); + await expect(child.reserve(estimateSpend(request('x'.repeat(100_000))))).resolves.toMatchObject( + { tokens: expect.any(Number) }, + ); + }); +}); + +describe('the ceiling holds under parallelism', () => { + /** A store whose `spent` yields to the loop, so three reads can genuinely interleave. */ + function slowStore(): BudgetStore & { read(key: string): number } { + const inner = new MemoryBudgetStore(); + return { + async spent(key: string): Promise { + await Promise.resolve(); + return inner.spent(key); + }, + add: (key, tokens) => inner.add(key, tokens), + reset: (key) => inner.reset(key), + read: (key) => inner.spent(key), + }; + } + + // Measured: `budget: { actor: 10_000 }` and three concurrent calls estimating ~4k tokens each + // all read `spent() === 0`, all passed, and 12k was recorded against a 10k ceiling. + test('three concurrent reserves cannot all pass one actor ceiling', async () => { + const store = slowStore(); + const ledger = new BudgetLedger({ limits: { actor: 10_000 }, actorKey: 'actor:u1', store }); + const call = () => ledger.reserve(estimateSpend(request('hi', 4_000))); + + const outcomes = await Promise.allSettled([call(), call(), call()]); + + expect(outcomes.filter((o) => o.status === 'rejected')).toHaveLength(1); + expect(store.read('actor:u1')).toBeLessThanOrEqual(10_000); + }); + + test('the same holds for the org ceiling', async () => { + const store = slowStore(); + const ledger = new BudgetLedger({ limits: { org: 10_000 }, orgKey: 'org:o1', store }); + const call = () => ledger.reserve(estimateSpend(request('hi', 4_000))); + + const outcomes = await Promise.allSettled([call(), call(), call()]); + + expect(outcomes.filter((o) => o.status === 'rejected')).toHaveLength(1); + expect(store.read('org:o1')).toBeLessThanOrEqual(10_000); + }); + + // A refusal must not reject the reservations queued behind it on the turnstile. + test('a refused reservation lets the next one through on its own merits', async () => { + const store = slowStore(); + const ledger = new BudgetLedger({ limits: { actor: 10_000 }, actorKey: 'actor:u1', store }); + + await expect(ledger.reserve(estimateSpend(request('hi', 20_000)))).rejects.toMatchObject({ + cause: expect.stringContaining('actor:u1'), + }); + await expect(ledger.reserve(estimateSpend(request('hi', 100)))).resolves.toMatchObject({ + tokens: expect.any(Number), + }); + }); + + test('release gives an unspent reservation back in full', async () => { + const store = new MemoryBudgetStore(); + const ledger = new BudgetLedger({ limits: { actor: 10_000 }, actorKey: 'actor:u1', store }); + + const reservation = await ledger.reserve(estimateSpend(request('hi', 4_000))); + expect(store.spent('actor:u1')).toBeGreaterThan(0); + + await ledger.release(reservation); + expect(store.spent('actor:u1')).toBe(0); + }); + + test('record reconciles down to the real count, never on top of the estimate', async () => { + const store = new MemoryBudgetStore(); + const ledger = new BudgetLedger({ limits: { actor: 10_000 }, actorKey: 'actor:u1', store }); + + const reservation = await ledger.reserve(estimateSpend(request('hi', 4_000))); + await ledger.record( + { inputTokens: 10, outputTokens: 5, cacheReadTokens: 0, cacheWriteTokens: 0 }, + usd(1), + reservation, + ); + + expect(store.spent('actor:u1')).toBe(15); }); }); diff --git a/packages/ai/src/budget.ts b/packages/ai/src/budget.ts index 81f8c7e8..1a2091e0 100644 --- a/packages/ai/src/budget.ts +++ b/packages/ai/src/budget.ts @@ -62,10 +62,20 @@ export function estimateSpend(request: GenerateRequest): SpendEstimate { /** Where cross-request counters live. Swap for Redis in a multi-process deployment. */ export interface BudgetStore { spent(key: string): Promise | number; + /** `tokens` may be NEGATIVE: releasing a reservation the call never spent is a credit. */ add(key: string, tokens: number): Promise | void; reset(key?: string): Promise | void; } +/** + * What `reserve` debited, so `record` can reconcile it against the provider's real counts and + * `release` can give it back. Held by the caller rather than the ledger because one ledger serves + * every concurrent call in a request, and each one owns its own reservation. + */ +export interface BudgetReservation { + readonly tokens: number; +} + export class MemoryBudgetStore implements BudgetStore { private readonly counters = new Map(); @@ -108,6 +118,13 @@ export class BudgetLedger { private requestTokens = 0; private costMinor = 0; private readonly currency: string; + /** + * Reservations take turns. Check-then-debit spans an `await store.spent()`, and three callers + * interleaving inside it is the bypass this ledger exists to close — one event loop, so a + * promise chain IS the lock. A store shared across PROCESSES needs an atomic increment of its + * own; this closes the parallelism inside one. + */ + private turnstile: Promise = Promise.resolve(); constructor(input: BudgetLedgerInput) { this.limits = input.limits; @@ -118,11 +135,24 @@ export class BudgetLedger { } /** - * Check an estimate against every applicable scope BEFORE the call. Throws on the first - * scope that cannot cover it, naming that scope, so the fix line points at one knob rather - * than four. Nothing is debited here: `record` does that with the provider's real counts. + * Check an estimate against every applicable scope BEFORE the call, then DEBIT it. Throws on + * the first scope that cannot cover it, naming that scope, so the fix line points at one knob + * rather than four. + * + * The debit is what makes the ceiling hold under parallelism. Checking without debiting meant + * three concurrent calls under one ledger all read `spent() === 0`, all passed, and all three + * recorded against a ceiling only one of them fitted — an "un-bypassable" org budget bypassed + * by `Promise.all`. `record` replaces the estimate with the real counts; `release` gives it + * back when the call never happened. */ - async reserve(estimate: SpendEstimate): Promise { + async reserve(estimate: SpendEstimate): Promise { + const turn = this.turnstile.then(() => this.reserveNow(estimate)); + // Chained on a settled shadow: one refusal must not reject every reservation queued behind it. + this.turnstile = turn.catch(() => undefined); + return await turn; + } + + private async reserveNow(estimate: SpendEstimate): Promise { this.assertScope('request', this.limits.request, this.requestTokens, estimate.tokens); // Per call, so nothing is "already spent" against it. this.assertScope('tokensIn', this.limits.tokensIn, 0, estimate.inputTokens); @@ -135,6 +165,14 @@ export class BudgetLedger { this.assertScope(`org:${this.orgKey}`, this.limits.org, spent, estimate.tokens); } this.assertCost(estimate.cost); + await this.debit(estimate.tokens); + return { tokens: estimate.tokens }; + } + + /** Give a reservation back: a provider that threw, a stream abandoned before `done`. */ + async release(reservation: BudgetReservation | undefined): Promise { + if (reservation === undefined) return; + await this.debit(-reservation.tokens); } /** @@ -158,11 +196,21 @@ export class BudgetLedger { }); } - /** Debit ACTUAL usage after the call, replacing the estimate `reserve` worked from. */ - async record(usage: TokenUsage, cost: Money): Promise { - const tokens = totalTokens(usage); - this.requestTokens += tokens; + /** + * Debit ACTUAL usage after the call, replacing the estimate `reserve` worked from — so only + * the DIFFERENCE lands here. Called without the reservation it behaves as it always did and + * debits the full amount, which double-counts a reserved call: pass the handle `reserve` + * returned. + */ + async record(usage: TokenUsage, cost: Money, reservation?: BudgetReservation): Promise { this.costMinor += cost.minor; + await this.debit(totalTokens(usage) - (reservation?.tokens ?? 0)); + } + + /** The one write path. Negative credits a release or an over-estimate back. */ + private async debit(tokens: number): Promise { + if (tokens === 0) return; + this.requestTokens += tokens; if (this.actorKey !== undefined) await this.store.add(this.actorKey, tokens); if (this.orgKey !== undefined) await this.store.add(this.orgKey, tokens); } diff --git a/packages/ai/src/errors.ts b/packages/ai/src/errors.ts index 9b8c3d53..147409c2 100644 --- a/packages/ai/src/errors.ts +++ b/packages/ai/src/errors.ts @@ -137,8 +137,13 @@ export class LlmRefusedError extends UltimateError { constructor(input: { prompt: string; model: string; - /** A blessed model that is NOT the one that refused — the fix has to be pasteable. */ - alternative: string; + /** + * A blessed model MORE capable than the one that refused, or `undefined` when the refusal + * came from the most capable one this build knows. Retrying a refusal on a weaker model is + * the one retry that cannot help, so the fix line drops the suggestion rather than inventing + * a downgrade. + */ + alternative: string | undefined; category: string | undefined; explanation: string | undefined; }) { @@ -148,7 +153,10 @@ export class LlmRefusedError extends UltimateError { `model "${input.model}" declined prompt "${input.prompt}"` + `${input.category === undefined ? '' : ` (${input.category})`}` + `${input.explanation === undefined ? '' : `: ${input.explanation}`}`, - fix: `set model: '${input.alternative}' on the llm() declaration, or edit the template in definePrompt('${input.prompt}') and bump its version`, + fix: + input.alternative === undefined + ? `edit the template in definePrompt('${input.prompt}') and bump its version — no blessed model is more capable than '${input.model}'` + : `set model: '${input.alternative}' on the llm() declaration, or edit the template in definePrompt('${input.prompt}') and bump its version`, docs: docsFor('X_LLM_REFUSED'), meta: { model: input.model, category: input.category }, }); diff --git a/packages/ai/src/gateway.ts b/packages/ai/src/gateway.ts index 4be6e57e..bd79214a 100644 --- a/packages/ai/src/gateway.ts +++ b/packages/ai/src/gateway.ts @@ -98,10 +98,20 @@ class GatewayImpl implements Gateway { // cheap-in-tokens call on an expensive model is still a cost cap the app declared. // `record` below replaces the estimate with the provider's real counts. const ledger = currentBudget(); - await ledger?.reserve(estimateSpend(resolved)); - - const result = await this.attempt(model, (provider) => provider.generate(resolved)); - await ledger?.record(result.usage, result.cost); + // The estimate is DEBITED here, not merely checked: three concurrent calls under one ledger + // all read the same `spent()` otherwise, all pass, and all three record against a ceiling + // only one of them fitted. + const reservation = await ledger?.reserve(estimateSpend(resolved)); + + let result: GenerateResult; + try { + result = await this.attempt(model, (provider) => provider.generate(resolved)); + } catch (error) { + // A call that never landed must not go on holding its reservation. + await ledger?.release(reservation); + throw error; + } + await ledger?.record(result.usage, result.cost, reservation); // A refusal is not an answer, so it is not cached. Storing one would keep serving a decision // the classifier might not make twice, long after the prompt that provoked it was fixed. if (result.stopReason !== 'refusal') { @@ -114,14 +124,24 @@ class GatewayImpl implements Gateway { const model = request.model ?? this.config.defaultModel ?? DEFAULT_MODEL; const resolved: GenerateRequest = { ...request, model }; const ledger = currentBudget(); - await ledger?.reserve(estimateSpend(resolved)); + const reservation = await ledger?.reserve(estimateSpend(resolved)); // A stream is not retried mid-flight: the consumer has already seen tokens, and // replaying from the top would duplicate them. Only the handshake retries. const provider = this.providerFor(model); - for await (const chunk of provider.stream(resolved)) { - if (chunk.type === 'done') await ledger?.record(chunk.result.usage, chunk.result.cost); - yield chunk; + let settled = false; + try { + for await (const chunk of provider.stream(resolved)) { + if (chunk.type === 'done') { + settled = true; + await ledger?.record(chunk.result.usage, chunk.result.cost, reservation); + } + yield chunk; + } + } finally { + // A stream that threw, or that its consumer abandoned, never reached `done` — so nothing + // reconciled the reservation and it would hold the ceiling for the rest of the window. + if (!settled) await ledger?.release(reservation); } } diff --git a/packages/ai/src/index.ts b/packages/ai/src/index.ts index 02e2b4df..cff39840 100644 --- a/packages/ai/src/index.ts +++ b/packages/ai/src/index.ts @@ -89,7 +89,14 @@ export type { } from './llm'; export { llm } from './llm'; export type { Effort, ModelId, ModelReasoning, ModelSpec, ThinkingMode } from './models'; -export { DEFAULT_MODEL, EFFORTS, MODEL_IDS, MODELS, reasoningBody } from './models'; +export { + DEFAULT_MODEL, + EFFORTS, + MODEL_IDS, + MODELS, + moreCapableThan, + reasoningBody, +} from './models'; export type { PgVectorStoreInput } from './pg-vector'; export { PgVectorStore } from './pg-vector'; export type { diff --git a/packages/ai/src/llm.test.ts b/packages/ai/src/llm.test.ts index 8e2deea5..380d56ea 100644 --- a/packages/ai/src/llm.test.ts +++ b/packages/ai/src/llm.test.ts @@ -186,6 +186,45 @@ describe('structured output', () => { expect(seen[1]?.messages.at(-1)?.content).toContain('failed its schema'); }); + // The measured failure: the model answered through the `respond` tool, so `result.text` was the + // EMPTY STRING, and the repair turn replayed it as `{role:'assistant',content:''}` — a 400 + // (`text content blocks must be non-empty`) that surfaced as X_AI_PROVIDER_UNAVAILABLE instead + // of the X_LLM_OUTPUT_INVALID this loop exists to raise. + test('never replays an empty assistant turn after a tool-use answer', async () => { + const { provider, seen } = stub({ summary: 42 }, ANSWER); + install(provider); + const summarize = declare(promptFor()); + + await summarize({ postId: POST_ID }, { ctx: anonymousCtx() }); + + const replayed = seen[1]?.messages ?? []; + expect(replayed.some((message) => message.content === '')).toBe(false); + }); + + // The tool call's arguments ARE the answer on that path, so replaying them is what gives the + // repair turn something to repair. + test("echoes the tool call's own arguments back as the assistant turn", async () => { + const { provider, seen } = stub({ summary: 42 }, ANSWER); + install(provider); + const summarize = declare(promptFor()); + + await summarize({ postId: POST_ID }, { ctx: anonymousCtx() }); + + const assistant = (seen[1]?.messages ?? []).filter((m) => m.role === 'assistant'); + expect(assistant.at(-1)?.content).toBe(JSON.stringify({ summary: 42 })); + }); + + test('a prose answer is still echoed verbatim', async () => { + const { provider, seen } = stub('{"summary": 42}', ANSWER); + install(provider); + const summarize = declare(promptFor()); + + await summarize({ postId: POST_ID }, { ctx: anonymousCtx() }); + + const assistant = (seen[1]?.messages ?? []).filter((m) => m.role === 'assistant'); + expect(assistant.at(-1)?.content).toBe('{"summary": 42}'); + }); + test('two bad answers throw X_LLM_OUTPUT_INVALID rather than looping', async () => { const { provider, seen } = stub({ summary: 42 }); install(provider); diff --git a/packages/ai/src/llm.ts b/packages/ai/src/llm.ts index 31a1bfcf..f5784ee9 100644 --- a/packages/ai/src/llm.ts +++ b/packages/ai/src/llm.ts @@ -26,7 +26,7 @@ import { BudgetLedger, currentBudget, withBudget } from './budget'; import { embedOne, fnv1a } from './embeddings'; import { LlmOutputInvalidError, LlmRefusedError, LlmTruncatedError } from './errors'; import type { ModelId } from './models'; -import { DEFAULT_MODEL, MODEL_IDS } from './models'; +import { DEFAULT_MODEL, moreCapableThan } from './models'; import type { Prompt, PromptVars } from './prompt'; import type { AiMessage, GenerateRequest, GenerateResult } from './provider'; import { aiEmbedder, aiGateway, semanticCacheFor } from './runtime'; @@ -187,9 +187,10 @@ async function generate< throw new LlmRefusedError({ prompt: name, model: result.model, - // The fix names a model the caller can paste. `` is not one, and a - // refusal is exactly the moment nobody wants to go read the catalogue. - alternative: MODEL_IDS.find((id) => id !== result.model) ?? DEFAULT_MODEL, + // The fix names a model the caller can paste, and only ever a MORE capable one: + // "the first id that differs" answered a refusal on the default model with the next + // entry down the ladder, which is a retry that cannot succeed. + alternative: moreCapableThan(result.model), category: result.stopDetails?.category, explanation: result.stopDetails?.explanation, }); @@ -205,7 +206,11 @@ async function generate< throw new LlmTruncatedError({ prompt: name, maxTokens: request.maxTokens }); } issues = formatIssues(parsed.issues).join('; '); - messages = [...messages, { role: 'assistant', content: result.text }, repair(issues)]; + const echo = assistantEcho(result); + messages = + echo === undefined + ? [...messages, repair(issues)] + : [...messages, { role: 'assistant', content: echo }, repair(issues)]; } throw new LlmOutputInvalidError({ prompt: name, attempts: ATTEMPTS, issues }); }); @@ -251,6 +256,25 @@ function respondToolFor(output: StandardSchemaV1): LlmTool { }; } +/** + * What the model answered, as text the Messages API will accept — or nothing. + * + * `result.text` is the EMPTY STRING whenever the answer came through the `respond` tool, which + * is the dominant path: an empty text block is a 400 (`text content blocks must be non-empty`), + * so the repair turn came back as `X_AI_PROVIDER_UNAVAILABLE` and the caller never saw the + * `X_LLM_OUTPUT_INVALID` this loop exists to raise. The tool call's own arguments ARE the answer + * in that case, and replaying them is what gives the repair turn its context — `AiMessage` + * carries a string, so the `tool_use` block cannot survive the round trip as itself, and + * replaying it as text avoids the `tool_result` the API would then demand of the next message. + */ +function assistantEcho(result: GenerateResult): string | undefined { + if (result.text !== '') return result.text; + const call = result.toolCalls.find((c) => c.name === RESPOND) ?? result.toolCalls[0]; + if (call === undefined) return undefined; + const replayed = JSON.stringify(call.input); + return replayed === undefined || replayed === '' ? undefined : replayed; +} + /** * The tool call if the model made one, otherwise the text parsed as JSON — a model that * answers in prose is a schema failure, not a crash, so it flows into the repair turn. diff --git a/packages/ai/src/models.test.ts b/packages/ai/src/models.test.ts new file mode 100644 index 00000000..6f2f7370 --- /dev/null +++ b/packages/ai/src/models.test.ts @@ -0,0 +1,75 @@ +/** + * The catalogue's two claims about itself: `MODEL_IDS` is ordered most-capable-first, so a + * refusal can be answered with a real upgrade rather than a downgrade; and `reasoningBody` + * sends a control only when the caller asked for one. + */ + +import { describe, expect, test } from 'bun:test'; +import { AiRequestInvalidError } from './errors'; +import { DEFAULT_MODEL, MODEL_IDS, MODELS, moreCapableThan, reasoningBody } from './models'; + +describe('moreCapableThan', () => { + // The measured failure: `MODEL_IDS.find((id) => id !== result.model)` answered a refusal on + // the default model with `claude-sonnet-5` — the fix line told an operator to retry a refusal + // on a weaker model, which is the one retry that cannot help. + test('has no answer for the most capable model, rather than a downgrade', () => { + expect(moreCapableThan(DEFAULT_MODEL)).toBeUndefined(); + expect(moreCapableThan(MODEL_IDS[0] as (typeof MODEL_IDS)[number])).toBeUndefined(); + }); + + test('walks UP the ladder, never down', () => { + for (let i = 1; i < MODEL_IDS.length; i += 1) { + const model = MODEL_IDS[i]; + if (model === undefined) continue; + const better = moreCapableThan(model); + expect(better).toBe(MODEL_IDS[i - 1] as (typeof MODEL_IDS)[number]); + // "More capable" is not a vibe here: the ladder is priced, and the rung above costs more. + expect(MODELS[better ?? DEFAULT_MODEL].outputPerMillion.minor).toBeGreaterThan( + MODELS[model].outputPerMillion.minor, + ); + } + }); + + test('the catalogue is ordered most capable first, which is what makes the walk sound', () => { + const prices = MODEL_IDS.map((id) => MODELS[id].outputPerMillion.minor); + expect(prices).toEqual([...prices].sort((a, b) => b - a)); + }); +}); + +describe('reasoningBody omits what nobody asked for', () => { + // The comment above `reasoningBody` claimed this; the code emitted an adaptive block for every + // adaptive-capable model whether or not the declaration mentioned thinking. + test('sends no thinking block when the declaration named no mode', () => { + for (const model of MODEL_IDS) { + expect(reasoningBody(model, undefined, undefined)['thinking']).toBeUndefined(); + } + }); + + test('sends no output_config when the declaration named no effort', () => { + for (const model of MODEL_IDS) { + expect(reasoningBody(model, undefined, undefined)['output_config']).toBeUndefined(); + } + }); + + test('sends exactly what the declaration DID name', () => { + const body = reasoningBody(DEFAULT_MODEL, 'high', 'adaptive'); + expect(body['output_config']).toEqual({ effort: 'high' }); + expect(body['thinking']).toEqual({ type: 'adaptive', display: 'summarized' }); + }); + + test('still refuses a control the model does not have', () => { + expect(() => reasoningBody('claude-haiku-4-5', 'high', undefined)).toThrow( + AiRequestInvalidError, + ); + expect(() => reasoningBody('claude-haiku-4-5', undefined, 'adaptive')).toThrow( + AiRequestInvalidError, + ); + }); + + test("still refuses 'disabled' above the model's cap", () => { + expect(() => reasoningBody(DEFAULT_MODEL, 'max', 'disabled')).toThrow(AiRequestInvalidError); + expect(reasoningBody(DEFAULT_MODEL, 'high', 'disabled')['thinking']).toEqual({ + type: 'disabled', + }); + }); +}); diff --git a/packages/ai/src/models.ts b/packages/ai/src/models.ts index 370862f7..38a486a9 100644 --- a/packages/ai/src/models.ts +++ b/packages/ai/src/models.ts @@ -103,6 +103,20 @@ export const MODELS: Readonly> = { const rankOf = (effort: Effort): number => EFFORTS.indexOf(effort); +/** + * The blessed model one rung ABOVE `model`, or `undefined` when it is already the most capable + * one this catalogue holds. `MODEL_IDS` is ordered most-capable-first — "the others are explicit + * downgrades" — so the ladder needs no second list to walk. + * + * A refusal is only worth retrying UPWARD. `MODEL_IDS.find((id) => id !== refused)` answered a + * refusal on the default model with the next entry DOWN, which is the one retry that cannot help: + * the fix line told an operator to buy the same refusal from a weaker model. + */ +export function moreCapableThan(model: ModelId): ModelId | undefined { + const at = MODEL_IDS.indexOf(model); + return at <= 0 ? undefined : MODEL_IDS[at - 1]; +} + /** * The reasoning half of a Messages body, shaped for one model. Everything it refuses, it refuses * LOCALLY with a real code — a round trip to learn a rule this file already states costs latency @@ -143,12 +157,16 @@ export function reasoningBody( return body; } - if ((thinking ?? 'adaptive') === 'disabled') { + if (thinking === 'disabled') { assertDisableAllowed(model, rules, effort ?? 'high'); body['thinking'] = { type: 'disabled' }; return body; } - body['thinking'] = { type: 'adaptive', display: 'summarized' }; + // Nothing asked for, nothing sent — the rule this file states, now the rule it follows. + // Adaptive is the server's own default on every model that has it, so emitting the block + // unrequested bought nothing and made a defaulted control indistinguishable on the wire from + // a declared one. + if (thinking === 'adaptive') body['thinking'] = { type: 'adaptive', display: 'summarized' }; return body; } diff --git a/packages/ai/src/provider.test.ts b/packages/ai/src/provider.test.ts index 6a205d6b..71fd6984 100644 --- a/packages/ai/src/provider.test.ts +++ b/packages/ai/src/provider.test.ts @@ -75,7 +75,7 @@ describe('Anthropic request body', () => { expect(JSON.stringify(body)).not.toContain('budget_tokens'); }); - test('effort lives inside output_config and thinking defaults to adaptive', () => { + test('effort lives inside output_config, and a control nobody asked for is omitted', () => { const body = provider.body({ messages: [{ role: 'user', content: 'hi' }], maxTokens: 1_000, @@ -83,6 +83,17 @@ describe('Anthropic request body', () => { }); expect(body['output_config']).toEqual({ effort: 'xhigh' }); expect(body['effort']).toBeUndefined(); + // Adaptive is the server's own default, so sending the block unrequested bought nothing and + // made a defaulted control indistinguishable on the wire from a declared one. + expect(body['thinking']).toBeUndefined(); + }); + + test('a thinking mode the caller DID ask for is sent', () => { + const body = provider.body({ + messages: [{ role: 'user', content: 'hi' }], + maxTokens: 1_000, + thinking: 'adaptive', + }); expect(body['thinking']).toEqual({ type: 'adaptive', display: 'summarized' }); }); @@ -169,8 +180,17 @@ describe('Anthropic request body', () => { ...(reasoning.effort ? { effort: 'high' as const } : {}), }); expect(body['output_config'] !== undefined).toBe(reasoning.effort); - expect(body['thinking'] !== undefined).toBe(reasoning.adaptive); + // Nothing asked for a thinking mode, so nothing is sent — on every model in the table. + expect(body['thinking']).toBeUndefined(); expect(body['temperature']).toBeUndefined(); + + const asked = provider.body({ + model, + messages: [{ role: 'user', content: 'hi' }], + maxTokens: 1_000, + ...(reasoning.adaptive ? { thinking: 'adaptive' as const } : {}), + }); + expect(asked['thinking'] !== undefined).toBe(reasoning.adaptive); } }); diff --git a/packages/jobs/CLAUDE.md b/packages/jobs/CLAUDE.md index 8aca2324..50d601cf 100644 --- a/packages/jobs/CLAUDE.md +++ b/packages/jobs/CLAUDE.md @@ -189,6 +189,29 @@ Tier 3. The `job` + `task` primitives, durable steps, transactional outbox, queu create the same table. A driver that ships none (`driver-redis`, `driver-nats`, a hand-rolled one) runs backfills with NO bookkeeping rather than refusing them: nothing blocks a completed name there, and that degradation is the price of one install point. +- **`run-once` fires ONE catch-up, and the watermark is what makes it one.** `dispatch()` marks + the occurrence it ran; under `run-once` that is the EARLIEST missed one, so the next round found + the rest still due and fired the second, then the third — 24 nightly digests a second apart after + a day down. The branch now marks `at` after dispatching, because dropping an occurrence means + moving past it, and `at` rather than the last element of `due`, which `maxCatchUp` truncates. + `skip` still fires the latest occurrence WITHIN the cap rather than the true latest missed — + named in the README, unchanged here. +- **Every timer body catches before it finalises.** `worker.ts`, `scheduler.ts` and the outbox + relay all spell `void work().catch(log).finally(...)`. The relay's missing `.catch` made a + rejected `store.claim()` an unhandled rejection, and Bun ends the process on one — with every + staged, unpublished row still staged. +- **The memory outbox store DELETES a published row.** `markPublished` rewriting it in place held + every payload ever enqueued for the process's lifetime and made `claim()`/`pendingCount()` walk + all of them each tick. `retained()` is the seam that makes the bound assertable; `published_at` + stays a pg-only audit column. +- **`claimName` reads a Set; `usedNames()` is a bounded window.** `Array.includes` per claim made + a `backfill()` over a million rows quadratic — 20,000 steps, ~200M string compares — and carried + a 20,000-entry array to the end of the run. The trace keeps `MAX_TRACE_NAMES` (200), most recent, + and duplicate detection never reads it: a name that scrolled out is still refused. +- **A `-fixture.ts` file is test material and does not ship** (`!src/**/*-fixture.ts` in `files`). + `backfill-pass-fixture.ts` raises `BackfillHandleFailure`, a plain `Error` subclass on purpose: + a backfill `handle` is app code and the pass propagates what it threw, so a framework code there + would exercise a path no app takes. - Suspension is control flow: `StepSuspension` -> `nack({ countsAsAttempt: false })`. Never log it as an error, never let it burn an attempt. - Step results are persisted BEFORE the step returns. Keep it that way or replay breaks. diff --git a/packages/jobs/README.md b/packages/jobs/README.md index 541c9c14..8bf17f90 100644 --- a/packages/jobs/README.md +++ b/packages/jobs/README.md @@ -226,6 +226,11 @@ The relay publishes *then* marks published, so a crash re-publishes — collapse idempotency key. Set `mode: 'required'` to make an enqueue outside a transaction an `X_OUTBOX_NO_TX` error instead of a direct publish. +The memory store (`createMemoryOutboxStore`, `x dev` and tests) **drops** a published row — +`retained()` is the relay's backlog, not a running total; the pg store keeps `published_at` as +the audit trail this map is not. A relay pass that throws is logged as `jobs.outbox.tick-failed` +and the loop re-arms: an unobserved rejection would end the process with rows still staged. + ## Drivers One interface: `enqueue`, `claim` (visibility timeout), `ack`, `nack` (backoff), @@ -261,10 +266,15 @@ non-empty string is not a timezone: `tz: 'Bogota'` would resolve every occurrenc run five hours off, silently, forever. `0 3 * * *` in a DST zone runs twice or zero times on the switch day. Catch-up after downtime is explicit: `skip` (default) fires the latest missed occurrence and drops the older ones, `run-once` fires the earliest missed one, `run-all` fires -every one of them. `maxCatchUp` (default 10) bounds EVERY mode, not just `run-all`: one tick -walks at most that many occurrences forward from the last fire, and the policy then picks from -what that walk found — so after a long outage `skip` fires the latest occurrence *within the -cap*, and `run-once` the earliest one, not the true latest/earliest missed. +every one of them. `maxCatchUp` (default 10) bounds the WALK for every mode, not just `run-all`: +one tick walks at most that many occurrences forward from the last fire, and the policy then +picks from what that walk found — so after a long outage `skip` fires the latest occurrence +*within the cap*, not the true latest missed. + +`run-once` fires **once**, not once per tick. Dropping the rest means the watermark passes them +too, so a scheduler back up after a day down enqueues one catch-up and then waits for the next +real occurrence. It used to leave the watermark on the occurrence it had just run, which made an +hourly task fire 24 catch-ups a second apart. ## Retries diff --git a/packages/jobs/package.json b/packages/jobs/package.json index ff363eba..25064b27 100644 --- a/packages/jobs/package.json +++ b/packages/jobs/package.json @@ -19,6 +19,7 @@ "files": [ "src", "!src/**/*.test.ts", + "!src/**/*-fixture.ts", "README.md", "LICENSE" ], diff --git a/packages/jobs/src/backfill-pass-fixture.ts b/packages/jobs/src/backfill-pass-fixture.ts index c7546c6e..97ec543f 100644 --- a/packages/jobs/src/backfill-pass-fixture.ts +++ b/packages/jobs/src/backfill-pass-fixture.ts @@ -15,6 +15,19 @@ import { createMemoryDriver } from './driver-memory'; import type { StepRecord, StepStore } from './steps'; import { createMemoryStepStore, createStepRunner } from './steps'; +/** + * What a failing `handle` raises. Deliberately NOT an `UltimateError`: a backfill handler is app + * code, the pass propagates whatever it threw, and a fixture raising a framework code would + * exercise a path no app takes. Named rather than anonymous so a suite can assert on the type. + * A `-fixture.ts` file is excluded from the package tarball — this is test material. + */ +export class BackfillHandleFailure extends Error { + constructor(readonly index: number) { + super(`batch ${String(index)} failed`); + this.name = 'BackfillHandleFailure'; + } +} + export const rows = entity('backfill_test_rows', { columns: { id: uuid().primaryKey(), orgId: uuid(), title: text({ max: 40 }) }, }); @@ -143,7 +156,7 @@ export const harness = ( ...(options.batch === undefined ? {} : { batch: options.batch }), source: () => watchedChain(table.where({ orgId: ORG }), watch), handle: ({ rows: page, index }) => { - if (state.failOn.has(index)) throw new Error(`batch ${String(index)} failed`); + if (state.failOn.has(index)) throw new BackfillHandleFailure(index); seen.push(page.map((entry) => entry.title)); }, }); diff --git a/packages/jobs/src/backfill-pass.test.ts b/packages/jobs/src/backfill-pass.test.ts index 186b8611..5f310b4b 100644 --- a/packages/jobs/src/backfill-pass.test.ts +++ b/packages/jobs/src/backfill-pass.test.ts @@ -7,7 +7,15 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import { memoryRepo, tableFor } from '@ultimat3/entity'; import { backfill, DEFAULT_BACKFILL_BATCH } from './backfill'; -import { ctx, harness, ORG, type Row, RUN_ID, rows } from './backfill-pass-fixture'; +import { + BackfillHandleFailure, + ctx, + harness, + ORG, + type Row, + RUN_ID, + rows, +} from './backfill-pass-fixture'; import { resetJobDriver } from './driver'; import { resetJobs } from './job'; import type { StepStore } from './steps'; @@ -106,7 +114,9 @@ describe('resume', () => { const pass = harness({ batch: 3 }); pass.failOn = new Set([1]); - await expect(pass.run()).rejects.toThrow('batch 1 failed'); + // The app's own failure, propagated verbatim — a named class rather than the bare `Error` + // this fixture used to raise from a file that shipped inside `src`. + await expect(pass.run()).rejects.toBeInstanceOf(BackfillHandleFailure); // Two statements: the batch that worked and the one that failed. expect(pass.watch.reads).toBe(2); diff --git a/packages/jobs/src/index.ts b/packages/jobs/src/index.ts index 276dd0e5..19858904 100644 --- a/packages/jobs/src/index.ts +++ b/packages/jobs/src/index.ts @@ -131,6 +131,7 @@ export { createLimiter, NO_TENANT, tenantKeyFrom } from './limits'; export type { EnqueueOptions, JobsFacade, + MemoryOutboxStore, OutboxDeps, OutboxRecord, OutboxRelay, @@ -176,6 +177,7 @@ export { createMemoryStepStore, createStepRunner, isStepSuspension, + MAX_TRACE_NAMES, StepSuspension, } from './steps'; export type { diff --git a/packages/jobs/src/outbox.test.ts b/packages/jobs/src/outbox.test.ts index 6be9e681..c1ae77f2 100644 --- a/packages/jobs/src/outbox.test.ts +++ b/packages/jobs/src/outbox.test.ts @@ -198,3 +198,76 @@ describe('handle.enqueue through the installed facade', () => { expect(await store.pendingCount()).toBe(0); }); }); + +describe('the relay loop', () => { + /** A store whose `claim` fails N times, then behaves. Models a pool timeout in a failover. */ + function flakyStore(failures: number): OutboxStore & { claims: number } { + const inner = createMemoryOutboxStore(); + let left = failures; + return { + claims: 0, + stage: (tx, record) => inner.stage(tx, record), + commit: (tx) => inner.commit(tx), + rollback: (tx) => inner.rollback(tx), + claim(limit) { + this.claims += 1; + if (left > 0) { + left -= 1; + return Promise.reject(new Error('connection pool timeout')); + } + return inner.claim(limit); + }, + markPublished: (id, at) => inner.markPublished(id, at), + pendingCount: () => inner.pendingCount(), + }; + } + + // `void tick().finally(...)` with no `.catch` makes a rejected `claim()` an unhandled + // rejection, and Bun's default for one is to end the process — with the staged rows still + // unpublished. Bun's test runner fails this file on an unhandled rejection, which is what + // makes this a test rather than a claim. + test('survives a claim that rejects and publishes on the next pass', async () => { + const driver = createMemoryDriver(); + const store = flakyStore(1); + const tx = fakeTx(); + setJobsFacade(createJobsFacade({ store, driver }, () => tx)); + await notify.enqueue({ orgId: 'org-relay' }); + await store.commit(tx); + + const relay = createOutboxRelay({ store, driver, intervalMs: 5 }); + relay.start(); + try { + const deadline = Date.now() + 2_000; + while (store.claims < 3 && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } + } finally { + relay.stop(); + } + + expect(store.claims).toBeGreaterThanOrEqual(2); + expect(((await driver.introspect?.list()) ?? []).length).toBe(1); + expect(await store.pendingCount()).toBe(0); + }); +}); + +describe('createMemoryOutboxStore retention', () => { + // Rewriting the record in place kept every payload ever enqueued for the process's lifetime, + // and made `claim()` and `pendingCount()` walk all of them every 200ms tick. + test('holds nothing once a row is published', async () => { + const driver = createMemoryDriver(); + const store = createMemoryOutboxStore(); + const relay = createOutboxRelay({ store, driver, batchSize: 500 }); + + for (let i = 0; i < 200; i += 1) { + const tx = fakeTx(); + await enqueueInTx({ store, driver }, tx, notify, { orgId: `org-${i}` }); + await store.commit(tx); + } + expect(store.retained()).toBe(200); + + expect(await relay.tick()).toBe(200); + expect(store.retained()).toBe(0); + expect(await store.pendingCount()).toBe(0); + }); +}); diff --git a/packages/jobs/src/outbox.ts b/packages/jobs/src/outbox.ts index 2946f1a8..816b9e1e 100644 --- a/packages/jobs/src/outbox.ts +++ b/packages/jobs/src/outbox.ts @@ -45,12 +45,21 @@ export interface OutboxStore { pendingCount(): Promise; } +export interface MemoryOutboxStore extends OutboxStore { + /** + * Committed rows this process is still holding. The relay's backlog and nothing else — a + * published row is dropped, so this is a bound, not a total. `x dev` and the tests are the + * only readers; a pg deployment reads `x_outbox` instead. + */ + retained(): number; +} + /** * Default store. Staged rows hang off the `Tx` object itself in a WeakMap, so the "same * transaction" guarantee needs no cooperation from the DB layer and rollback is a delete. * The pg store swaps this for a real `x_outbox` table written by the same connection. */ -export function createMemoryOutboxStore(): OutboxStore { +export function createMemoryOutboxStore(): MemoryOutboxStore { const staged = new WeakMap(); const committed = new Map(); @@ -80,9 +89,12 @@ export function createMemoryOutboxStore(): OutboxStore { .slice(0, limit); return Promise.resolve(ready); }, - markPublished(id, at) { - const record = committed.get(id); - if (record !== undefined) committed.set(id, { ...record, publishedAt: at }); + markPublished(id, _at) { + // Deleted, not stamped. A published row is out of the relay's reach either way, and the + // pg store's `published_at` column is a retained audit trail this map is not: rewriting + // it in place held every payload ever enqueued — arbitrary job input — for the life of + // the process, and made `claim()` and `pendingCount()` walk all of them every 200ms. + committed.delete(id); return Promise.resolve(); }, pendingCount() { @@ -92,6 +104,7 @@ export function createMemoryOutboxStore(): OutboxStore { } return Promise.resolve(count); }, + retained: () => committed.size, }; } @@ -282,9 +295,19 @@ export function createOutboxRelay(options: RelayOptions): OutboxRelay { timer = setInterval(() => { if (running) return; running = true; - void tick().finally(() => { - running = false; - }); + // `.catch` before `.finally`, the shape every other loop in this package uses. `tick()` + // guards each publish but not `store.claim()` — one pool timeout during a failover + // rejects here unobserved, and Bun's default for an unhandled rejection is to end the + // process, taking every staged, unpublished row with it. + void tick() + .catch((error: unknown) => { + logger.error('jobs.outbox.tick-failed', { + error: error instanceof Error ? error.message : String(error), + }); + }) + .finally(() => { + running = false; + }); }, intervalMs); }, stop() { diff --git a/packages/jobs/src/scheduler.test.ts b/packages/jobs/src/scheduler.test.ts index a2700dd6..af4564e9 100644 --- a/packages/jobs/src/scheduler.test.ts +++ b/packages/jobs/src/scheduler.test.ts @@ -45,6 +45,14 @@ const dailyAt3: CronResolver = (cron, options) => { return new Date(at3 > from ? at3 : at3 + dayMs); }; +/** Deterministic stand-in for @ultimat3/time: fires on the hour, UTC. */ +const hourly: CronResolver = (cron, options) => { + expect(cron).toBe('0 * * * *'); + const hourMs = 3_600_000; + const from = options.from.getTime(); + return new Date(Math.floor(from / hourMs) * hourMs + hourMs); +}; + // 2026-07-26T00:00:00Z, a Sunday. const T0 = Date.UTC(2026, 6, 26, 0, 0, 0); @@ -180,6 +188,102 @@ describe('scheduler', () => { expect(byCatching.length).toBe(4); }); + // The measured failure: an hourly task, a scheduler down 24 hours, `run-once` — 24 dispatches + // over 24 one-second ticks, occurrences 2..25, because the watermark was left on the occurrence + // that just ran instead of past the ones the policy drops. + test('catch-up: "run-once" fires exactly one catch-up, however many ticks follow', async () => { + const clock = fakeClock(T0); + const driver = createMemoryDriver({ clock }); + const once = task({ + name: 'hourlyOnce', + cron: '0 * * * *', + tz: 'UTC', + catchUp: 'run-once', + enqueue: () => [[sendDigest, {}]], + }); + const scheduler = createScheduler({ + driver, + clock, + cron: hourly, + state: createMemorySchedulerState(), + tasks: [once], + }); + + await scheduler.tick(); // Arms it. + clock.advance(24 * 3_600_000); // Down a full day: 24 occurrences missed. + + const first = await scheduler.tick(); + expect(first.length).toBe(1); + expect(first[0]?.catchUp).toBe(true); + + // 24 further ticks at the real interval. Every one of these used to dispatch. + let later = 0; + for (let i = 0; i < 24; i += 1) { + clock.advance(1_000); + later += (await scheduler.tick()).length; + } + expect(later).toBe(0); + expect(((await driver.introspect?.list()) ?? []).length).toBe(1); + }); + + test('catch-up: "run-once" fires the EARLIEST missed occurrence, not the latest', async () => { + const clock = fakeClock(T0); + const driver = createMemoryDriver({ clock }); + const once = task({ + name: 'hourlyOnce', + cron: '0 * * * *', + tz: 'UTC', + catchUp: 'run-once', + enqueue: () => [[sendDigest, {}]], + }); + const scheduler = createScheduler({ + driver, + clock, + cron: hourly, + state: createMemorySchedulerState(), + tasks: [once], + }); + + await scheduler.tick(); + clock.advance(5 * 3_600_000); + + const dispatched = await scheduler.tick(); + expect(new Date(dispatched[0]?.occurrenceMs ?? 0).toISOString()).toBe( + '2026-07-26T01:00:00.000Z', + ); + }); + + // The next occurrence after the outage still fires: the watermark moved past what was + // dropped, never past what has not happened yet. + test('catch-up: "run-once" leaves the next real occurrence due', async () => { + const clock = fakeClock(T0); + const driver = createMemoryDriver({ clock }); + const once = task({ + name: 'hourlyOnce', + cron: '0 * * * *', + tz: 'UTC', + catchUp: 'run-once', + enqueue: () => [[sendDigest, {}]], + }); + const scheduler = createScheduler({ + driver, + clock, + cron: hourly, + state: createMemorySchedulerState(), + tasks: [once], + }); + + await scheduler.tick(); + clock.advance(5 * 3_600_000); + await scheduler.tick(); + + clock.advance(3_600_000); // 06:00 arrives. + const next = await scheduler.tick(); + expect(next.length).toBe(1); + expect(next[0]?.catchUp).toBe(false); + expect(new Date(next[0]?.occurrenceMs ?? 0).toISOString()).toBe('2026-07-26T06:00:00.000Z'); + }); + test('a late dispatch builds its payload from the occurrence, not the wall clock', async () => { const dated = datedDigestJob(); const nightly = task({ diff --git a/packages/jobs/src/scheduler.ts b/packages/jobs/src/scheduler.ts index 852bef1b..ba63103b 100644 --- a/packages/jobs/src/scheduler.ts +++ b/packages/jobs/src/scheduler.ts @@ -205,7 +205,17 @@ export function createScheduler(options: SchedulerOptions): Scheduler { } if (handle.catchUp === 'run-once') { const first = due[0]; - if (first !== undefined) dispatched.push(await dispatch(handle, first, due.length > 1)); + if (first !== undefined) { + dispatched.push(await dispatch(handle, first, due.length > 1)); + // `dispatch` leaves the watermark on the occurrence it RAN — the earliest missed one + // here — so the next round found occurrences 2..n still due and fired the second, then + // the third, one per tick until the backlog drained: 24 nightly digests a second apart + // after a day down. "One catch-up" means the rest are DROPPED, and dropping an + // occurrence is moving the watermark past it. `at` rather than the last element of + // `due`, which `maxCatchUp` truncates: every occurrence at or before `at` is missed by + // definition, and this policy fires none of them. + if (first !== at) await schedulerState.markFired(handle.name, at); + } continue; } for (const occurrence of due) { diff --git a/packages/jobs/src/steps.test.ts b/packages/jobs/src/steps.test.ts index 26735250..82cca3e8 100644 --- a/packages/jobs/src/steps.test.ts +++ b/packages/jobs/src/steps.test.ts @@ -3,7 +3,12 @@ import type { Clock } from '@ultimat3/core'; import { StepDuplicateError } from './errors'; import { createMemoryEventBus } from './events'; import type { StepStore } from './steps'; -import { createMemoryStepStore, createStepRunner, isStepSuspension } from './steps'; +import { + createMemoryStepStore, + createStepRunner, + isStepSuspension, + MAX_TRACE_NAMES, +} from './steps'; function fakeClock(startMs: number): Clock & { advance(ms: number): void } { let current = startMs; @@ -298,3 +303,46 @@ describe('a step timeout cancels the body it is timing', () => { expect(observed?.aborted).toBe(true); }); }); + +describe('the attempt trace is bounded, and duplicate detection is not', () => { + // `Array.includes` per claim made a long run quadratic: 20,000 steps is ~200M string compares + // plus a 20,000-entry array carried to the end of the run and read by `x jobs show`. + test('keeps the most recent MAX_TRACE_NAMES names, not one per step', async () => { + const runner = createStepRunner({ runId: 'run-long', jobName: 'sweep', store }); + + for (let i = 0; i < MAX_TRACE_NAMES * 3; i += 1) { + await runner.step.run(`batch:${i}`, () => i); + } + + const used = runner.usedNames(); + expect(used).toHaveLength(MAX_TRACE_NAMES); + expect(used[used.length - 1]).toBe(`batch:${MAX_TRACE_NAMES * 3 - 1}`); + }); + + // The trace forgetting a name must never make it re-claimable: the store is keyed by it. + test('still refuses a duplicate whose name scrolled out of the trace', async () => { + const runner = createStepRunner({ runId: 'run-dup', jobName: 'sweep', store }); + + await runner.step.run('batch:0', () => 0); + for (let i = 1; i <= MAX_TRACE_NAMES * 2; i += 1) { + await runner.step.run(`batch:${i}`, () => i); + } + + expect(runner.usedNames()).not.toContain('batch:0'); + await expect(runner.step.run('batch:0', () => 0)).rejects.toThrow(StepDuplicateError); + }); + + test('bounds the replay trace the same way', async () => { + const first = createStepRunner({ runId: 'run-replay', jobName: 'sweep', store }); + for (let i = 0; i < MAX_TRACE_NAMES + 10; i += 1) { + await first.step.run(`batch:${i}`, () => i); + } + + const second = createStepRunner({ runId: 'run-replay', jobName: 'sweep', store }); + for (let i = 0; i < MAX_TRACE_NAMES + 10; i += 1) { + await second.step.run(`batch:${i}`, () => i); + } + + expect(second.replayedNames()).toHaveLength(MAX_TRACE_NAMES); + }); +}); diff --git a/packages/jobs/src/steps.ts b/packages/jobs/src/steps.ts index 8148ba78..53341475 100644 --- a/packages/jobs/src/steps.ts +++ b/packages/jobs/src/steps.ts @@ -152,17 +152,36 @@ export interface StepRunnerOptions { export interface StepRunner { readonly step: StepApi; - /** Names used in THIS attempt, in order — the trace shown by `x jobs show`. */ + /** + * Names used in THIS attempt, in order — the trace shown by `x jobs show`. Bounded at + * `MAX_TRACE_NAMES`, oldest dropped: duplicate detection reads its own set, so this is a + * window on a long run and never the run's record. + */ usedNames(): readonly string[]; - /** Names that were served from storage instead of executed. */ + /** Names that were served from storage instead of executed. Bounded the same way. */ replayedNames(): readonly string[]; } /** Stands in for an absent run signal, so the fence has one shape and no `undefined` branch. */ const NEVER_ABORTED = new AbortController().signal; +/** + * What one attempt's trace keeps. The trace is a diagnostic `x jobs show` renders, never the + * run's record — `driver.steps.list(runId)` is that — and a `backfill()` over a million rows + * claims 20,000 names in a single attempt, all of them carried to the end of the run. + */ +export const MAX_TRACE_NAMES = 200; + +/** Most recent first out: the tail of a long run is the half an operator is reading. */ +function trace(into: string[], name: string): void { + into.push(name); + if (into.length > MAX_TRACE_NAMES) into.shift(); +} + export function createStepRunner(options: StepRunnerOptions): StepRunner { const { runId, jobName, store } = options; + /** Every name this attempt has claimed. Membership only — the trace is `used`. */ + const claimed = new Set(); const used: string[] = []; const replayed: string[] = []; const clock = options.clock; @@ -170,8 +189,11 @@ export function createStepRunner(options: StepRunnerOptions): StepRunner { const runSignal = options.signal ?? NEVER_ABORTED; const claimName = (name: string): void => { - if (used.includes(name)) throw new StepDuplicateError({ job: jobName, step: name }); - used.push(name); + // The Set decides, the array only reports. `Array.includes` made a long run quadratic — + // `backfill({ batch: 50 })` over a million rows is 20,000 steps and ~200M string compares. + if (claimed.has(name)) throw new StepDuplicateError({ job: jobName, step: name }); + claimed.add(name); + trace(used, name); }; const cancelled = (): boolean => runSignal.aborted; @@ -193,7 +215,7 @@ export function createStepRunner(options: StepRunnerOptions): StepRunner { claimName(name); const existing = await store.get(runId, name); if (existing?.status === 'completed') { - replayed.push(name); + trace(replayed, name); return existing.output as T; } diff --git a/packages/mcp/CLAUDE.md b/packages/mcp/CLAUDE.md index e38767bb..715da5cf 100644 --- a/packages/mcp/CLAUDE.md +++ b/packages/mcp/CLAUDE.md @@ -118,6 +118,11 @@ import. The CLI wires it. - The caps run in the **tool**, not the host. A host that forgets them answers a million rows into a model's context. `guards` names the layers that engaged; a layer that could not engage is absent from the list, never assumed present. +- **Every authentication answer lands before `request.json()`.** A missing token, a token + `resolveToken` rejects and a non-agent actor all return before the body is read: parsing first + answered `400 parse error` for a malformed payload and `401` for a well-formed one under the + SAME rejected token, which is precisely the oracle the pre-parse 401 exists to remove. The parse + error still exists — it is what an authenticated agent gets. - `transport-stdio.ts` never writes stdout except the wire. Diagnostics → stderr. - New mutating tool ⇒ set `destructive: true`, or it is metered as cheap read chatter. diff --git a/packages/mcp/src/transport-http.test.ts b/packages/mcp/src/transport-http.test.ts index f3dd7096..e428f2e7 100644 --- a/packages/mcp/src/transport-http.test.ts +++ b/packages/mcp/src/transport-http.test.ts @@ -170,3 +170,61 @@ describe('mcpHttpRoute.handle: successful calls', () => { expect(payload.error.code).toBe(-32601); }); }); + +describe('an unauthenticated caller learns nothing about its own request', () => { + /** The oracle: two requests differing only in body shape must be indistinguishable. */ + const malformed = (headers: Record): Request => + new Request('http://local/mcp', { method: 'POST', headers, body: '{' }); + + // Measured: `Bearer garbage` with `{` answered `400 parse error` and with valid JSON answered + // `401` — the body was parsed before the token was resolved. + test('a rejected token answers 401 whether or not the JSON parses', async () => { + const route = mcpHttpRoute({ server, resolveToken: () => null }); + + const bad = await route.handle(malformed({ authorization: 'Bearer garbage' })); + const good = await route.handle( + request( + { jsonrpc: '2.0', id: 1, method: 'initialize' }, + { + authorization: 'Bearer garbage', + }, + ), + ); + + expect(bad.status).toBe(401); + expect(good.status).toBe(401); + expect(await bad.text()).toBe(await good.text()); + }); + + test('a non-agent actor answers the same way for either body', async () => { + const route = mcpHttpRoute({ + server, + resolveToken: () => ({ actor: userActor({ id: 'u1' }), scopes: [] }), + }); + + const bad = await route.handle(malformed({ authorization: 'Bearer t' })); + const good = await route.handle( + request( + { jsonrpc: '2.0', id: 1, method: 'initialize' }, + { + authorization: 'Bearer t', + }, + ), + ); + + expect(bad.status).toBe(good.status); + expect(await bad.text()).toBe(await good.text()); + }); + + // The parse error still exists — it is what an authenticated agent gets for a broken payload. + test('an authenticated agent still gets the parse error', async () => { + const route = mcpHttpRoute({ + server, + resolveToken: () => ({ actor: agentActor({ id: 'a1' }), scopes: [] }), + }); + + const res = await route.handle(malformed({ authorization: 'Bearer t' })); + expect(res.status).toBe(400); + expect(await res.text()).toContain('not valid JSON'); + }); +}); diff --git a/packages/mcp/src/transport-http.ts b/packages/mcp/src/transport-http.ts index c9c20fa9..4032e55e 100644 --- a/packages/mcp/src/transport-http.ts +++ b/packages/mcp/src/transport-http.ts @@ -63,12 +63,16 @@ export function mcpHttpRoute(input: McpHttpTransportInput): McpRouteDescriptor { rateLimitClass: (body) => server.classify(body), async handle(request: Request): Promise { + // Every authentication answer lands BEFORE the body is read. Parsing first meant a caller + // holding a rejected token still learned whether its JSON was well formed — `400 parse + // error` for one payload and `401` for the next is exactly the oracle the 401 exists to + // remove, and it costs nothing to close: the body is not an input to any of these. const token = bearerToken(request); - if (token === null) { - // 401 before parsing: an unauthenticated caller learns nothing about the catalog, - // not even whether its JSON was well formed. - return unauthorized(); - } + if (token === null) return unauthorized(); + + const resolved = await input.resolveToken(token); + if (resolved === null) return unauthorized(); + if (!isAgentActor(resolved.actor)) return notAnAgent(); let body: unknown; try { @@ -77,10 +81,6 @@ export function mcpHttpRoute(input: McpHttpTransportInput): McpRouteDescriptor { return json(errorResponse(null, PARSE_ERROR, 'request body is not valid JSON'), 400); } - const resolved = await input.resolveToken(token); - if (resolved === null) return unauthorized(); - if (!isAgentActor(resolved.actor)) return notAnAgent(); - const caller: McpCaller = { actor: resolved.actor, scopes: resolved.scopes, diff --git a/packages/pwa/CLAUDE.md b/packages/pwa/CLAUDE.md index e8fabd2c..9ce8a628 100644 --- a/packages/pwa/CLAUDE.md +++ b/packages/pwa/CLAUDE.md @@ -14,6 +14,7 @@ Tier 4. May import tiers 0–3: `core`, `schema`, `i18n`, `money`, `time`, `cach | Route input | `PwaRoute` is a **structural** view of render's `RouteDescriptor`. Never import render. | | Strategy choice | derived from render mode via `MODE_STRATEGY`. Per-route override only. | | Precache revision | content hash. Never the build id — that re-downloads everything per deploy. | +| Precache KEY | the bare URL, always. The revision addresses the FETCH (`?v=`), never the key: every strategy looks an entry up with `caches.match(req)` and `ignoreSearch` defaults to `false`, so an entry left keyed under `?v=` is a permanent miss — offline serves the fallback document instead of the precached page, and online every precached byte is downloaded twice. `addAll` still does the fetching, because its all-or-nothing failure is what stops a half-populated precache from activating; the install block only re-keys what it stored. `service-worker.test.ts` executes the emitted `sw.js` against stub `caches`/`fetch` rather than asserting its text. | | Cache names | always `cacheNamespace(buildId, kind)`. An unkeyed cache name is a rejected change. | | Offline fallback | `requireOfflineFallback` runs inside `generateServiceWorker`. Never optional. | | Capabilities | gate the manifest member **and** the SW block. Disabled → zero bytes. | diff --git a/packages/pwa/src/service-worker.test.ts b/packages/pwa/src/service-worker.test.ts index 7263133d..9663e6f9 100644 --- a/packages/pwa/src/service-worker.test.ts +++ b/packages/pwa/src/service-worker.test.ts @@ -4,6 +4,7 @@ import { PwaNoOfflineFallbackError, SwScopeInvalidError } from './errors'; import type { ServiceWorkerConfig } from './service-worker'; import { generateServiceWorker } from './service-worker'; import type { PwaRoute } from './strategies'; +import { cacheNamespace } from './version-skew'; const routes: readonly PwaRoute[] = [ { @@ -115,3 +116,195 @@ describe('generateServiceWorker', () => { ).toThrow(SwScopeInvalidError); }); }); + +/** + * The emitted `sw.js` run for real. Every other test here asserts the *text* of the artifact; + * this one executes it against stub `caches`/`fetch` so a precache entry keyed where no + * strategy looks for it is a failing test rather than an offline page nobody sees until + * production. + */ +type SwListener = (event: SwEvent) => void; + +interface SwEvent { + readonly request?: Request; + waitUntil(work: Promise): void; + respondWith(work: Promise): void; +} + +const SW_ORIGIN = 'https://app.test'; + +/** A service worker resolves a relative URL against its scope; Bun's global `Request` cannot. */ +class SwRequest extends Request { + constructor(input: Request | string, init?: RequestInit) { + super(typeof input === 'string' ? new URL(input, SW_ORIGIN).href : input, init); + } +} + +/** Keyed by absolute URL, exactly as `Cache` is with `ignoreSearch` at its default `false`. */ +class StubCache { + readonly entries = new Map(); + #fetch: (request: Request) => Promise; + + constructor(fetcher: (request: Request) => Promise) { + this.#fetch = fetcher; + } + + #key(request: Request | string): string { + return typeof request === 'string' ? new URL(request, SW_ORIGIN).href : request.url; + } + + async match(request: Request | string): Promise { + return this.entries.get(this.#key(request)); + } + + async put(request: Request | string, response: Response): Promise { + this.entries.set(this.#key(request), response); + } + + async delete(request: Request | string): Promise { + return this.entries.delete(this.#key(request)); + } + + /** All-or-nothing, like the real one: a non-ok response rejects the whole install. */ + async addAll(requests: readonly Request[]): Promise { + const responses = await Promise.all(requests.map((request) => this.#fetch(request))); + responses.forEach((response, i) => { + if (!response.ok) throw new TypeError('addAll: request failed'); + const request = requests[i]; + if (request !== undefined) this.entries.set(request.url, response); + }); + } +} + +function swHarness() { + const fetched: string[] = []; + let offline = false; + const fetcher = async (request: Request | string): Promise => { + const url = typeof request === 'string' ? request : request.url; + fetched.push(url); + if (offline) throw new TypeError('network down'); + return new Response(`bytes for ${new URL(url).pathname}`, { status: 200 }); + }; + + const caches = new Map(); + const cacheStorage = { + async open(name: string): Promise { + const existing = caches.get(name); + if (existing !== undefined) return existing; + const created = new StubCache(fetcher); + caches.set(name, created); + return created; + }, + async keys(): Promise { + return [...caches.keys()]; + }, + async delete(name: string): Promise { + return caches.delete(name); + }, + }; + + const listeners = new Map(); + const self = { + location: { origin: SW_ORIGIN }, + addEventListener(type: string, listener: SwListener): void { + listeners.set(type, listener); + }, + clients: { claim: async (): Promise => undefined, matchAll: async () => [] }, + skipWaiting: (): void => undefined, + }; + + return { + caches, + fetched, + goOffline: (): void => { + offline = true; + }, + load(source: string): void { + const factory = new Function('self', 'caches', 'fetch', 'Request', source) as ( + scope: typeof self, + storage: typeof cacheStorage, + fetcher: (request: Request | string) => Promise, + request: typeof SwRequest, + ) => void; + factory(self, cacheStorage, fetcher, SwRequest); + }, + async install(): Promise { + let work: Promise = Promise.resolve(); + listeners.get('install')?.({ + waitUntil: (p) => { + work = p; + }, + respondWith: () => undefined, + }); + await work; + }, + async request(path: string): Promise { + let answer: Promise | undefined; + listeners.get('fetch')?.({ + request: new SwRequest(path), + waitUntil: () => undefined, + respondWith: (p) => { + answer = p; + }, + }); + if (answer === undefined) throw new Error(`no handler answered ${path}`); + return await answer; + }, + }; +} + +describe('the emitted install block, executed', () => { + const precached: readonly PwaRoute[] = [ + { path: '/', surface: 'site', mode: 'static', offline: 'precache', revision: 'aaaa1111' }, + { + path: '/pricing', + surface: 'site', + mode: 'static', + offline: 'precache', + revision: 'bbbb2222', + }, + ]; + + test('keys every precached entry under the bare URL, which is where strategies look', async () => { + const sw = swHarness(); + sw.load(generateServiceWorker(precached, config, 'build-1').source); + await sw.install(); + + const cache = sw.caches.get(cacheNamespace('build-1', 'precache')); + expect([...(cache?.entries.keys() ?? [])].sort()).toEqual([ + 'https://app.test/', + 'https://app.test/offline', + 'https://app.test/pricing', + ]); + }); + + test('fetches each entry revision-addressed, so a deploy re-downloads only what changed', async () => { + const sw = swHarness(); + sw.load(generateServiceWorker(precached, config, 'build-1').source); + await sw.install(); + + expect(sw.fetched).toContain('https://app.test/pricing?v=bbbb2222'); + }); + + // The measured failure: offline, `cacheFirst` looked up `/pricing`, the entry was stored as + // `/pricing?v=bbbb2222`, and the user got the offline document instead of the precached page. + test('serves a precached page offline instead of the offline fallback', async () => { + const sw = swHarness(); + sw.load(generateServiceWorker(precached, config, 'build-1').source); + await sw.install(); + sw.goOffline(); + + expect(await (await sw.request('/pricing')).text()).toBe('bytes for /pricing'); + }); + + // Online, the same miss cost a second download of every precached byte. + test('answers online from the precache without a second network request', async () => { + const sw = swHarness(); + sw.load(generateServiceWorker(precached, config, 'build-1').source); + await sw.install(); + const afterInstall = sw.fetched.length; + + await sw.request('/pricing'); + expect(sw.fetched).toHaveLength(afterInstall); + }); +}); diff --git a/packages/pwa/src/service-worker.ts b/packages/pwa/src/service-worker.ts index 0b8550d8..e0b9e002 100644 --- a/packages/pwa/src/service-worker.ts +++ b/packages/pwa/src/service-worker.ts @@ -203,12 +203,26 @@ function serializeRules(rules: readonly RouteRule[]): string { return `[${rows.join(',')}]`; } +/** + * The revision addresses the **fetch**, never the **key**. Every strategy looks an entry up with + * `caches.match(req)` on the bare URL — `ignoreSearch` defaults to `false` — so an entry left + * keyed under `?v=` is a permanent miss: offline serves the fallback document instead + * of the page that was precached, and online every precached byte is downloaded a second time. + * `addAll` still does the fetching, because its all-or-nothing failure is what stops a + * half-populated precache from activating; the second pass only re-keys what it stored. + */ const INSTALL_BLOCK = ` self.addEventListener('install',(event)=>{ event.waitUntil((async()=>{ const cache=await caches.open(PRECACHE); // Revision is a content hash: unchanged assets are not re-downloaded across deploys. - await cache.addAll(PRECACHE_MANIFEST.map((e)=>new Request(e.url+'?v='+e.revision,{cache:'reload'}))); + const fetched=PRECACHE_MANIFEST.map((e)=>new Request(e.url+'?v='+e.revision,{cache:'reload'})); + await cache.addAll(fetched); + for(let i=0;i { await this.#entries.delete(key); } + + async invalidateTags(tags: readonly CacheTag[]): Promise { + return await this.#entries.invalidateTags(tags); + } } /** A source that hangs until released, so a second reader is provably concurrent with the first. */ @@ -289,7 +289,7 @@ describe('readThrough', () => { expect(ttl?.value).toBe('rows'); expect(ttl?.expiresAt).toBeGreaterThanOrEqual(before + 60_000); expect(ttl?.expiresAt).toBeLessThanOrEqual(after + 60_000); - expect(forever).toEqual({ value: 'rows', expiresAt: null }); + expect(forever).toEqual({ value: 'rows', expiresAt: null, tags: [] }); expect(tier.writes).toHaveLength(2); }); @@ -393,17 +393,3 @@ describe('readFresh', () => { expect(await requestMemo(ctx).get('k')).toBe('newer'); }); }); - -describe('MemoryReadCache', () => { - test('drops an entry whose expiry has passed, and keeps one that has not', async () => { - const memory = new MemoryReadCache(); - // One `now` for the write and for the assertion, and a horizon no test machine crosses: a - // millisecond ticking between the two would expire the live entry for the clock's reasons. - const now = Date.now(); - await memory.set('stale', { value: 'rows', expiresAt: now - 1 }); - await memory.set('live', { value: 'rows', expiresAt: now + 60_000 }); - - expect(await memory.get('stale')).toBeUndefined(); - expect(await memory.get('live')).toEqual({ value: 'rows', expiresAt: now + 60_000 }); - }); -}); diff --git a/packages/query/src/cache.ts b/packages/query/src/cache.ts index b8acfaf6..cfb19690 100644 --- a/packages/query/src/cache.ts +++ b/packages/query/src/cache.ts @@ -1,62 +1,16 @@ /** - * Read caching, two layers: a per-request memo (`readOnce` — same query twice in one render - * costs one execution, whether the second read follows the first or races it) and, for a query - * that declares `cache:`, a tag-keyed tier behind the `ReadCache` interface (`readThrough`). - * Every read gets the memo; the tier is the half a query opts into. Invalidation is never - * local — it goes through @ultimat3/cache so an action's `invalidates` and a query's `tags` - * meet in one graph. + * The read path: a per-request memo (`readOnce` — same query twice in one render costs one + * execution, whether the second read follows the first or races it) and, for a query that + * declares `cache:`, the fill through the tier `read-cache.ts` owns (`readThrough`). Every read + * gets the memo; the tier is the half a query opts into. */ import type { CacheTag } from '@ultimat3/cache'; -import { invalidateTags } from '@ultimat3/cache'; import type { Ctx } from '@ultimat3/core'; +import { getReadCache } from './read-cache'; import { fingerprint } from './stable'; import { tagKeys } from './tags'; -export interface ReadCacheEntry { - readonly value: unknown; - readonly expiresAt: number | null; -} - -export interface ReadCache { - get(key: string): Promise; - set(key: string, entry: ReadCacheEntry): Promise; - delete(key: string): Promise; -} - -/** In-memory default. Production installs the tiered cache from @ultimat3/cache. */ -export class MemoryReadCache implements ReadCache { - readonly #entries = new Map(); - - async get(key: string): Promise { - const entry = this.#entries.get(key); - if (entry === undefined) return undefined; - if (entry.expiresAt !== null && entry.expiresAt <= Date.now()) { - this.#entries.delete(key); - return undefined; - } - return entry; - } - - async set(key: string, entry: ReadCacheEntry): Promise { - this.#entries.set(key, entry); - } - - async delete(key: string): Promise { - this.#entries.delete(key); - } -} - -let tier: ReadCache = new MemoryReadCache(); - -export function setReadCache(cache: ReadCache): void { - tier = cache; -} - -export function getReadCache(): ReadCache { - return tier; -} - /** * Request-scoped memo. Keyed by ctx identity so it dies with the request. * @@ -129,27 +83,36 @@ async function publish( } } -/** Memo first, then the tier, then the source — what a query with `cache:` reads through. */ +/** + * Memo first, then the tier, then the source — what a query with `cache:` reads through. + * + * `tags` is what the written entry is dropped by; an entry stored without them is reachable + * only by its key and can therefore only expire. + */ export function readThrough( ctx: Ctx, key: string, ttlMs: number | null, run: () => Promise, + tags: readonly CacheTag[] = [], ): Promise { - return readOnce(ctx, key, () => fill(key, ttlMs, run)); + return readOnce(ctx, key, () => fill(key, ttlMs, tags, run)); } /** The read itself — tier, then the source. Runs once per key per request; the rest join it. */ -async function fill(key: string, ttlMs: number | null, run: () => Promise): Promise { +async function fill( + key: string, + ttlMs: number | null, + tags: readonly CacheTag[], + run: () => Promise, +): Promise { + // Read per call, never captured: `setReadCache` after the first read has to be honoured, and a + // module-level binding here would be a second handle on a tier the seam exists to swap. + const tier = getReadCache(); const cached = await tier.get(key); if (cached !== undefined) return cached.value as T; const value = await run(); - await tier.set(key, { value, expiresAt: ttlMs === null ? null : Date.now() + ttlMs }); + await tier.set(key, { value, expiresAt: ttlMs === null ? null : Date.now() + ttlMs, tags }); return value; } - -/** The one invalidation path. Actions call the same function via their `cache`. */ -export async function invalidateQueryTags(tags: readonly CacheTag[]): Promise { - await invalidateTags(tags); -} diff --git a/packages/query/src/index.ts b/packages/query/src/index.ts index b83d930b..89fa733a 100644 --- a/packages/query/src/index.ts +++ b/packages/query/src/index.ts @@ -9,17 +9,7 @@ /** Re-exported so a `query` file needs one import, not two. Same object as schema's. */ export type { Infer } from '@ultimat3/schema'; export { t } from '@ultimat3/schema'; -export type { ReadCache, ReadCacheEntry } from './cache'; -export { - cacheKeyFor, - getReadCache, - invalidateQueryTags, - MemoryReadCache, - readOnce, - readThrough, - requestMemo, - setReadCache, -} from './cache'; +export { cacheKeyFor, readOnce, readThrough, requestMemo } from './cache'; export type { FetchLike, QueryCallOptions, @@ -76,6 +66,16 @@ export type { export { describeQuery, isQuery, nameQuery, query, queryHash } from './query'; /** The one read path. `defOf` stays unexported — that is the enforcement. */ export { queryName, runQuery, sourceFor } from './read'; +export type { ReadCache, ReadCacheEntry } from './read-cache'; +export { + DEFAULT_READ_CACHE_MAX_BYTES, + DEFAULT_READ_CACHE_TTL_MS, + getReadCache, + invalidateQueryTags, + MemoryReadCache, + setReadCache, +} from './read-cache'; + export { describeQueries, getQuery, diff --git a/packages/query/src/read-cache.test.ts b/packages/query/src/read-cache.test.ts new file mode 100644 index 00000000..db3156ba --- /dev/null +++ b/packages/query/src/read-cache.test.ts @@ -0,0 +1,140 @@ +// Single responsibility: the tier itself — what the default cache retains, what drops it, and +// what `invalidateQueryTags` reaches. The read path's use of it is `cache.test.ts`, and the +// end-to-end pairing (a `cache:` query, an action's `invalidates`, the next request) is +// `read.test.ts`. Here the tier is driven directly, so a failure names the tier and not the read. + +import { afterAll, beforeEach, describe, expect, test } from 'bun:test'; +import { tag } from '@ultimat3/cache'; +import type { ReadCache, ReadCacheEntry } from './read-cache'; +import { + DEFAULT_READ_CACHE_MAX_BYTES, + DEFAULT_READ_CACHE_TTL_MS, + getReadCache, + invalidateQueryTags, + MemoryReadCache, + setReadCache, +} from './read-cache'; + +const original = getReadCache(); +let tier = new MemoryReadCache(); + +beforeEach(() => { + tier = new MemoryReadCache(); + setReadCache(tier); +}); + +// The installed tier is process-wide; a leaked one reroutes every later read in this process. +afterAll(() => { + setReadCache(original); +}); + +describe('the installed tier', () => { + test('setReadCache swaps what getReadCache answers with', () => { + expect(getReadCache()).toBe(tier); + const replacement = new MemoryReadCache(); + setReadCache(replacement); + expect(getReadCache()).toBe(replacement); + }); +}); + +describe('invalidateQueryTags', () => { + // The defect this pins: the entry was written with no tags and the read tier was never + // reachable from a fan-out, so a cached list survived the write that changed it — for the + // life of the process when the read declared no `ttlMs`. + test('drops an entry the installed tier holds under the same tag', async () => { + await tier.set('list', { value: 'rows', expiresAt: null, tags: [tag('post')] }); + + await invalidateQueryTags([tag('post')]); + + expect(await tier.get('list')).toBeUndefined(); + }); + + // A row write must bust the lists that held the row — the asymmetry @ultimat3/cache's + // `tagMatches` defines, reached here rather than re-derived. + test('a row tag drops an entry cached under the bare collection', async () => { + await tier.set('list', { value: 'rows', expiresAt: null, tags: [tag('post')] }); + + await invalidateQueryTags([tag('post', '1')]); + + expect(await tier.get('list')).toBeUndefined(); + }); + + test('leaves an entry cached under a different entity alone', async () => { + await tier.set('comments', { value: 'rows', expiresAt: null, tags: [tag('comment')] }); + + await invalidateQueryTags([tag('post')]); + + expect((await tier.get('comments'))?.value).toBe('rows'); + }); + + test('does not throw when the installed tier cannot invalidate by tag', async () => { + class Untagged implements ReadCache { + readonly #entries = new Map(); + async get(key: string): Promise { + return this.#entries.get(key); + } + async set(key: string, entry: ReadCacheEntry): Promise { + this.#entries.set(key, entry); + } + async delete(key: string): Promise { + this.#entries.delete(key); + } + } + setReadCache(new Untagged()); + + expect(await invalidateQueryTags([tag('post')])).toBeUndefined(); + }); +}); + +describe('MemoryReadCache', () => { + test('drops an entry whose expiry has passed, and keeps one that has not', async () => { + const memory = new MemoryReadCache(); + // One `now` for the write and for the assertion, and a horizon no test machine crosses: a + // millisecond ticking between the two would expire the live entry for the clock's reasons. + const now = Date.now(); + await memory.set('stale', { value: 'rows', expiresAt: now - 1 }); + await memory.set('live', { value: 'rows', expiresAt: now + 60_000 }); + + expect(await memory.get('stale')).toBeUndefined(); + expect(await memory.get('live')).toEqual({ value: 'rows', expiresAt: now + 60_000 }); + }); + + // The unbounded default: one immortal entry per distinct input, and a paginated read keyed by + // `{ orgId, cursor }` has as many distinct inputs as the deployment has tenants. + test('is bounded — the least recently used entry goes when the budget is spent', async () => { + const memory = new MemoryReadCache({ maxBytes: 512 }); + const filler = 'x'.repeat(100); + + for (let i = 0; i < 20; i += 1) { + await memory.set(`k${i}`, { value: filler, expiresAt: null }); + } + + expect(await memory.get('k0')).toBeUndefined(); + expect((await memory.get('k19'))?.value).toBe(filler); + }); + + test('skips a value too large for the whole budget rather than failing the read', async () => { + const memory = new MemoryReadCache({ maxBytes: 64 }); + + await memory.set('huge', { value: 'x'.repeat(4096), expiresAt: null }); + + expect(await memory.get('huge')).toBeUndefined(); + }); + + test('drops every entry carrying an invalidated tag, and nothing else', async () => { + const memory = new MemoryReadCache(); + await memory.set('a', { value: 1, expiresAt: null, tags: [tag('post')] }); + await memory.set('b', { value: 2, expiresAt: null, tags: [tag('post', '7')] }); + await memory.set('c', { value: 3, expiresAt: null, tags: [tag('comment')] }); + + expect(await memory.invalidateTags([tag('post')])).toHaveLength(2); + expect(await memory.get('a')).toBeUndefined(); + expect(await memory.get('b')).toBeUndefined(); + expect((await memory.get('c'))?.value).toBe(3); + }); + + test('the defaults are the numbers the read path is documented against', () => { + expect(DEFAULT_READ_CACHE_TTL_MS).toBe(60_000); + expect(DEFAULT_READ_CACHE_MAX_BYTES).toBe(32 * 1024 * 1024); + }); +}); diff --git a/packages/query/src/read-cache.ts b/packages/query/src/read-cache.ts new file mode 100644 index 00000000..f312ec76 --- /dev/null +++ b/packages/query/src/read-cache.ts @@ -0,0 +1,115 @@ +/** + * The read tier: the `ReadCache` seam a `cache:` query reads through, the bounded in-memory + * default behind it, and the one invalidation hop. Split from `cache.ts` because the tier and + * the request memo answer different questions — the memo asks "did THIS request already read + * this?", the tier asks "did anyone, and is that answer still true?". + */ + +import type { CacheTag, LruOptions } from '@ultimat3/cache'; +import { CacheTooLargeError, invalidateTags, LruCache } from '@ultimat3/cache'; + +export interface ReadCacheEntry { + readonly value: unknown; + readonly expiresAt: number | null; + /** + * What the entry is dropped by. Optional so an existing `ReadCache` implementation still + * compiles — but an entry written without them can only ever expire, never be invalidated. + */ + readonly tags?: readonly CacheTag[]; +} + +export interface ReadCache { + get(key: string): Promise; + set(key: string, entry: ReadCacheEntry): Promise; + delete(key: string): Promise; + /** + * Drop every entry carrying one of `tags`. Optional because it arrived after the interface + * shipped; a tier that omits it keeps entries until they expire, which is why the default + * below implements it. + */ + invalidateTags?(tags: readonly CacheTag[]): Promise; +} + +/** + * A `cache:` block with no `ttlMs`. Tag invalidation is the primary eviction, so this is the + * backstop for the read whose tags never fire — one number, the same 60s `@ultimat3/cache`'s + * LRU tier defaults to. + */ +export const DEFAULT_READ_CACHE_TTL_MS = 60_000; + +/** 32 MiB: half the LRU tier's budget, because a read cache is not the whole cache. */ +export const DEFAULT_READ_CACHE_MAX_BYTES = 32 * 1024 * 1024; + +/** + * In-memory default. Production installs the tiered cache from @ultimat3/cache. + * + * Backed by that package's `LruCache` rather than a bare `Map`: the bound and the tag→keys + * index are the two things a read cache cannot go without, and re-deriving "which keys does + * this tag bust" here would be a second definition of tag matching. + */ +export class MemoryReadCache implements ReadCache { + readonly #entries: LruCache; + + constructor(options: LruOptions = {}) { + this.#entries = new LruCache({ maxBytes: DEFAULT_READ_CACHE_MAX_BYTES, ...options }); + } + + async get(key: string): Promise { + return this.#entries.get(key)?.value; + } + + async set(key: string, entry: ReadCacheEntry): Promise { + const now = Date.now(); + // Already stale on arrival: storing it would hand the next reader an entry `get` has to + // throw away, and `ttl <= 0` is how the LRU spells "no expiry" — the opposite answer. + if (entry.expiresAt !== null && entry.expiresAt <= now) { + this.#entries.del(key); + return; + } + // The entry is stored whole so `get` returns the absolute expiry it was given back, + // unrounded by the tier's own clock. + try { + this.#entries.set(key, entry, { + // A `null` expiry is "the caller named none", never "never": @ultimat3/cache's tiers + // refuse a non-positive `ttlMs` and have no immortal entry to offer, so it falls to the + // tier's own default — which is the backstop an unbounded read cache was missing. + ...(entry.expiresAt === null ? {} : { ttlMs: entry.expiresAt - now }), + tags: entry.tags ?? [], + }); + } catch (error) { + // A row set too large to cache is a miss on the next read, never a failed read. + if (!(error instanceof CacheTooLargeError)) throw error; + } + } + + async delete(key: string): Promise { + this.#entries.del(key); + } + + async invalidateTags(tags: readonly CacheTag[]): Promise { + return this.#entries.invalidateTags(tags); + } +} + +let tier: ReadCache = new MemoryReadCache(); + +export function setReadCache(cache: ReadCache): void { + tier = cache; +} + +export function getReadCache(): ReadCache { + return tier; +} + +/** + * The one invalidation path. Actions call the same function via their `cache`. + * + * Two drops, one call: the graph @ultimat3/cache owns reaches every registered tier, ISR route, + * CDN path and live query, and the read tier is dropped by the same tags in the same hop. The + * read tier is not a registered `CacheTier` — it is this package's seam, replaceable per + * deployment through `setReadCache` — so the fan-out cannot reach it and this must. + */ +export async function invalidateQueryTags(tags: readonly CacheTag[]): Promise { + await invalidateTags(tags); + await tier.invalidateTags?.(tags); +} diff --git a/packages/query/src/read.test.ts b/packages/query/src/read.test.ts index 5ef47d78..a8bfc8b3 100644 --- a/packages/query/src/read.test.ts +++ b/packages/query/src/read.test.ts @@ -3,13 +3,22 @@ // ordering proved with a spy, impersonation via `options.actor`, and `buildSource`'s `total()`. import { describe, expect, test } from 'bun:test'; +import { tag } from '@ultimat3/cache'; import { createContext, userActor } from '@ultimat3/core'; import { allow, can } from '@ultimat3/policy'; import { t } from '@ultimat3/schema'; +import { cacheKeyFor } from './cache'; import { QueryDeniedError, QueryForeignError, QueryUnregisteredError } from './errors'; import type { AnyQuery } from './query'; import { query } from './query'; import { defOf, hasDef, queryName, runQuery, sourceFor } from './read'; +import { + DEFAULT_READ_CACHE_TTL_MS, + getReadCache, + invalidateQueryTags, + MemoryReadCache, + setReadCache, +} from './read-cache'; import type { SqlSource } from './source'; import { from } from './source'; @@ -270,3 +279,62 @@ describe("buildSource's live surface: total() when the source implements it", () expect(live).toBe(noTotalSource); }); }); + +describe("a cache: read's tier entry, and what drops it", () => { + /** Fresh tier per test: the module default is a process-wide singleton other files share. */ + function defineCachedQuery(ttlMs?: number) { + const counts = { executed: 0 }; + const target = query({ + input: Input, + policy: allow(), + cache: ttlMs === undefined ? { tags: [tag('post')] } : { tags: [tag('post')], ttlMs }, + sql: () => + from('rows', async () => { + counts.executed += 1; + return rows; + }), + }).named(`cachedFeed${counts.executed}${ttlMs ?? 'default'}`); + return { target, counts }; + } + + // The measured failure: the entry was immortal and no fan-out could reach it, so the + // pre-write list was served for the life of the process. + test('an invalidateQueryTags fan-out drops it, and the next request re-reads', async () => { + setReadCache(new MemoryReadCache()); + const { target, counts } = defineCachedQuery(); + + await runQuery(target, { orgId: ORG }, { ctx: createContext({ actor: allowedActor }) }); + await runQuery(target, { orgId: ORG }, { ctx: createContext({ actor: allowedActor }) }); + expect(counts.executed).toBe(1); // second request served from the tier + + await invalidateQueryTags([tag('post')]); + + await runQuery(target, { orgId: ORG }, { ctx: createContext({ actor: allowedActor }) }); + expect(counts.executed).toBe(2); + }); + + test('a cache: block with no ttlMs still writes a bounded expiry', async () => { + setReadCache(new MemoryReadCache()); + const { target } = defineCachedQuery(); + const before = Date.now(); + + await runQuery(target, { orgId: ORG }, { ctx: createContext({ actor: allowedActor }) }); + + const key = cacheKeyFor(queryName(target), { orgId: ORG }, [tag('post')]); + const entry = await getReadCache().get(key); + expect(entry?.expiresAt).toBeGreaterThanOrEqual(before + DEFAULT_READ_CACHE_TTL_MS); + expect(entry?.tags).toEqual([tag('post')]); + }); + + test('a declared ttlMs is honoured over the default', async () => { + setReadCache(new MemoryReadCache()); + const { target } = defineCachedQuery(5_000); + const before = Date.now(); + + await runQuery(target, { orgId: ORG }, { ctx: createContext({ actor: allowedActor }) }); + + const key = cacheKeyFor(queryName(target), { orgId: ORG }, [tag('post')]); + const entry = await getReadCache().get(key); + expect(entry?.expiresAt).toBeLessThan(before + DEFAULT_READ_CACHE_TTL_MS); + }); +}); diff --git a/packages/query/src/read.ts b/packages/query/src/read.ts index b97b8343..0eb471c6 100644 --- a/packages/query/src/read.ts +++ b/packages/query/src/read.ts @@ -21,6 +21,7 @@ import { cacheKeyFor, readFresh, readOnce, readThrough } from './cache'; import { QueryForeignError, QueryInputInvalidError, QueryUnregisteredError } from './errors'; import { actorOf, guard } from './policy-gate'; import type { AnyQuery, AnyQueryDef, Query, QueryOptions, SourceOptions } from './query'; +import { DEFAULT_READ_CACHE_TTL_MS } from './read-cache'; import type { SqlSource } from './source'; /** @@ -117,7 +118,8 @@ async function readRows( const read = (): Promise => withSpan(`query.${name}`, () => source.execute()); // The source came from this query's own `sql()`, so its rows are TRow throughout — // which is what the typed overload above states, and this body never has to assert. - const key = cacheKeyFor(name, raw, def.cache?.tags ?? []); + const tags = def.cache?.tags ?? []; + const key = cacheKeyFor(name, raw, tags); // `fresh` is the caller saying no cache may answer this one — the memo included, a memo being // a cache whose lifetime is the request. It still *publishes* into the memo: this read is the // newest answer the request has, so the next plain read of the key joins it rather than the @@ -125,9 +127,12 @@ async function readRows( if (options.fresh === true) return await readFresh(ctx, key, read); // `cache:` buys the tier, never the memo: a read asked twice in one request is one execution // whether or not its author opted into caching. + // A declared `cache:` with no `ttlMs` gets one anyway. Tags are the primary eviction, but a + // read whose tags never fire would otherwise hold one entry per distinct input for the life of + // the process — a paginated feed over 10k tenants is 10k immortal entries. return def.cache === undefined ? await readOnce(ctx, key, read) - : await readThrough(ctx, key, def.cache.ttlMs ?? null, read); + : await readThrough(ctx, key, def.cache.ttlMs ?? DEFAULT_READ_CACHE_TTL_MS, read, tags); } async function buildSource( diff --git a/packages/realtime/CLAUDE.md b/packages/realtime/CLAUDE.md index e5eb5460..cda5eb76 100644 --- a/packages/realtime/CLAUDE.md +++ b/packages/realtime/CLAUDE.md @@ -204,6 +204,24 @@ Tier 3 package. Channels, live queries, local-first sync. One protocol for all t same outage as never arming, and nothing awaits a timer — a throw out of one is an uncaught exception that can kill the process that was going to retry. Only the timer owns the chain and only the timer reports — a `connect()` the app called itself throws to the app and arms nothing. +- **A `sid` is CLIENT data, so a subscription is keyed by `(socket, sid)` — never by `sid` alone.** + `LiveQueryRegistry.unsubscribe(socketId, sid)` and `.subscription(socketId, sid)` both take the + owner. Keyed by the sid alone, socket B reusing socket A's sid overwrote A's slot: A's + subscription stayed in its query entry's `subscribers` map with nothing able to reach it, so + `unsubscribeSocket(A)` freed nothing, `subscribers.size` never hit zero, and the entry's matcher + and shared window were pinned for the process's life while every change fanned out to a dead + socket. A `{op:'drop', sid}` frame from B ended A's stream with no error on either side. + `sync-node` passes `socket.id` on the drop path for that reason. Reusing a sid the SAME socket + already holds is `X_SUBSCRIPTION_ID_TAKEN` — refused rather than replaced, because replacing is + the strand. `subscription-book.ts` owns that identity and is the only place it is spelled: the + query entry's own `subscribers` map takes the same composite key, so one `unsubscribe` reaches + both by one identity. +- **`connect()` closes the socket it is replacing, and a frame speaks only for its own socket.** + A remount calling `connect()` on a live client left the previous socket open: its `onMessage` + kept folding patches into the live registrations, and the node held two sockets for one client — + double presence membership, double fanout — until the tab closed. `#socket` is nulled before the + close so the corpse's `onClose` takes its early return, and `onMessage` carries the same identity + guard `onClose` already had. - Deny by default on topics. No guard = `X_TOPIC_FORBIDDEN`. - Never a bare `Error`. Never `any`. Never `Date.now()` — take a `Clock` (`clock.now()` is a `Date`; use `monotonic()` for durations). @@ -223,7 +241,10 @@ Tier 3 package. Channels, live queries, local-first sync. One protocol for all t | `nats-fake.ts` | an in-memory bus implementing the port — server semantics, not wire bytes; the only way to prove multi-node fanout under a sealed network | | `cursor.ts` / `change-buffer.ts` / `thundering-herd.ts` | reconnect — the highest-risk area | | `local-store.ts` / `offline-queue.ts` / `rebase.ts` | tier 3 | -| `client.ts` / `sync-node.ts` | the two halves — `client.test.ts` owns the reconnect timer | +| `client.ts` / `sync-node.ts` | the two halves — connection lifecycle, subscriptions, mutations | +| `client-frames.ts` | what a RECEIVED frame does to client state, and `ClientFrameTarget` — the only inbound surface the client exposes. The mirror of `sync-node.ts`'s handler | +| `client-harness-fixture.ts` | the injected socket + scheduler + harness both client suites drive. Excluded from the tarball | +| `subscription-book.ts` | who holds which subscription, keyed by `(socket, sid)`, and the per-socket/per-tenant caps answered from it | | `apply-patches.ts` | folding a patch list onto a row list — the client's one stateless piece | | `hooks.ts` | the ambient client seam + the four component hooks — the only file an app imports | | `query-hook.ts` | the typed projection: one declared query bound to one named hook | diff --git a/packages/realtime/README.md b/packages/realtime/README.md index e0d54f0b..8ddc4600 100644 --- a/packages/realtime/README.md +++ b/packages/realtime/README.md @@ -225,7 +225,8 @@ injected `Scheduler`, so a test fires it by hand instead of sleeping. ## Errors -`X_TOPIC_FORBIDDEN` · `X_SUBSCRIPTION_LIMIT` · `X_PROTOCOL_VERSION` · `X_CURSOR_STALE` · +`X_TOPIC_FORBIDDEN` · `X_SUBSCRIPTION_LIMIT` · `X_SUBSCRIPTION_ID_TAKEN` · +`X_PROTOCOL_VERSION` · `X_CURSOR_STALE` · `X_REBASE_CONFLICT` · `X_TRANSPORT_UNAVAILABLE` · `X_TRANSPORT_PROTOCOL` · `X_REPLICATION_FAILED` · `X_REPLICATION_PROTOCOL` · `X_REPLICATOR_SLOT_HELD` · `X_LIVE_CLIENT_MISSING` · `X_LIVE_QUERY_UNKNOWN` · `X_NOT_IMPLEMENTED` @@ -233,6 +234,10 @@ injected `Scheduler`, so a test fires it by hand instead of sleeping. Topics deny by default: a topic with no matching guard is forbidden. An authz hole is not a config option someone forgot to set. +A `sid` belongs to the socket that chose it. A subscription is keyed by `(socket, sid)`, a drop +frame is scoped to the socket that sent it, and reusing a sid the same socket already holds is +`X_SUBSCRIPTION_ID_TAKEN` — one client can neither take over nor end another's live stream. + A `subscribe` frame naming a query this node never registered is `X_LIVE_QUERY_UNKNOWN`, not `X_PROTOCOL_VERSION`: the frame parsed and the version matched, so "rebuild and redeploy the client" is the one instruction that cannot help — a rebuilt client spells the name the same way. diff --git a/packages/realtime/package.json b/packages/realtime/package.json index 1c8a71d9..40ff3486 100644 --- a/packages/realtime/package.json +++ b/packages/realtime/package.json @@ -19,6 +19,7 @@ "files": [ "src", "!src/**/*.test.ts", + "!src/**/*-fixture.ts", "README.md", "LICENSE" ], diff --git a/packages/realtime/src/client-frames.ts b/packages/realtime/src/client-frames.ts new file mode 100644 index 00000000..e66297fd --- /dev/null +++ b/packages/realtime/src/client-frames.ts @@ -0,0 +1,131 @@ +// What a RECEIVED frame does to client state — the mirror of `sync-node.ts`'s inbound handler, +// and the only inbound surface `client.ts` exposes. `ClientFrameTarget` is the point: it names +// every piece of the client a frame may touch, so the blast radius of a new frame kind is a +// reviewable list rather than "whatever the router could reach through `this`". + +import { applyPatches } from './apply-patches'; +import type { LiveCursor } from './cursor'; +import type { JsonObject, JsonValue, Row } from './json'; +import type { LocalStore, TableMap } from './local-store'; +import type { OfflineQueue } from './offline-queue'; +import { type RebaseLog, reconcile } from './rebase'; +import type { Frame, PresenceMember } from './sync-protocol'; + +/** One live query this client holds. Mutable: the rows and cursor a frame advances live here. */ +export interface Registration { + readonly sid: string; + readonly name: string; + readonly input: JsonValue; + readonly setRows: (rows: readonly Row[]) => void; + readonly setState: (state: LiveState) => void; + readonly setCursor: (cursor: LiveCursor | null) => void; + rows: readonly Row[]; + cursor: LiveCursor | null; +} + +export type LiveState = 'loading' | 'live' | 'stale' | 'offline'; + +/** + * Everything an inbound frame is allowed to reach. Narrow on purpose — a router that took the + * client itself could touch the reconnect timer, the socket and the outbound path, none of which + * a received frame has any business writing. + */ +export interface ClientFrameTarget { + registration(sid: string): Registration | undefined; + topicHandlers(topic: string): ReadonlySet<(message: JsonObject) => void> | undefined; + readonly queue: OfflineQueue | undefined; + readonly store: LocalStore | undefined; + readonly log: RebaseLog | undefined; + /** A newer build is live; the app decides when to reload. */ + setUpdate(buildId: string | null): void; + /** The node assigned this socket its own delay before closing it. */ + scheduleReconnect(afterMs: number | null): void; + closeSocket(code: number, reason: string): void; + notifyQueueChange(): void; +} + +/** Presence members cross the topic channel as plain JSON, like every other channel message. */ +function memberJson(member: PresenceMember): JsonValue { + return { id: member.id, actorId: member.actorId, meta: member.meta, updatedAt: member.updatedAt }; +} + +export function applyFrame(frame: Frame, target: ClientFrameTarget): void { + switch (frame.type) { + case 'snapshot': { + const registration = target.registration(frame.sid); + if (!registration) return; + registration.rows = frame.rows; + registration.cursor = frame.cursor; + registration.setRows(frame.rows); + registration.setCursor(frame.cursor); + registration.setState('live'); + return; + } + case 'patch': { + const registration = target.registration(frame.sid); + if (registration) { + registration.rows = applyPatches(registration.rows, frame.patches); + registration.setRows(registration.rows); + registration.setState('live'); + return; + } + // No registration: it is a tier-1 channel message on `sid = topic`. + const handlers = target.topicHandlers(frame.sid); + if (!handlers) return; + for (const patch of frame.patches) { + if (patch.row === null) continue; + for (const handler of handlers) handler(patch.row); + } + return; + } + case 'ack': { + const queue = target.queue; + // `ack`/`fail` mutate the queue synchronously and persist asynchronously; chaining rather + // than notifying right after the call keeps this correct even if that ordering ever + // changes, and it still fires exactly once the persisted write actually lands. + const settled = frame.error ? queue?.fail(frame.ref, frame.error) : queue?.ack(frame.ref); + void settled?.then(() => target.notifyQueueChange()); + return; + } + case 'rebase': { + const store = target.store; + const log = target.log; + if (!store || !log) return; + reconcile({ + store, + log, + ack: { + key: frame.key, + entity: frame.entity, + id: frame.row?.id ?? frame.key, + row: frame.row, + }, + }); + return; + } + case 'reconnect': { + // Order is load-bearing: arming first is what makes the close this triggers keep the delay + // the node assigned to *this* socket instead of falling back to a local backoff. + target.scheduleReconnect(frame.afterMs); + target.closeSocket(1001, frame.reason); + return; + } + case 'update-available': { + target.setUpdate(frame.buildId); + return; + } + case 'presence': { + const handlers = target.topicHandlers(frame.topic); + if (!handlers) return; + const message: JsonObject = { op: frame.op, members: frame.members.map(memberJson) }; + for (const handler of handlers) handler(message); + return; + } + case 'hello': + case 'subscribe': + case 'mutate': + // Client-authored frames: never received. Ignored rather than thrown, so a future + // bidirectional use of the same kind cannot break an old client. + return; + } +} diff --git a/packages/realtime/src/client-harness-fixture.ts b/packages/realtime/src/client-harness-fixture.ts new file mode 100644 index 00000000..1dff02be --- /dev/null +++ b/packages/realtime/src/client-harness-fixture.ts @@ -0,0 +1,159 @@ +// The doubles two client suites drive: an injected socket, an injected scheduler, and the +// harness that wires a `LiveClient` to both. Shared rather than copied because `client.test.ts` +// and `client-reconnect.test.ts` must exercise the SAME client wiring — two harnesses that +// drifted would be two clients agreeing only by construction. A `-fixture.ts` file is test +// material and is excluded from the package tarball. + +import { frozenClock } from '@ultimat3/core'; +import { type ClientSocket, LiveClient, type SignalFactory } from './client'; +import { decode, type Frame } from './sync-protocol'; +import type { BackoffPolicy, Scheduler } from './thundering-herd'; + +/** A harness driven wrongly by a test, never a framework fault — so it carries no `X_*` code. */ +export class HarnessMisuse extends Error { + override readonly name = 'HarnessMisuse'; +} + +/** Synchronous and closure-backed: enough to prove an accessor re-reads, with no reactive runtime. */ +export const signal: SignalFactory = (initial: T) => { + let value = initial; + return [ + () => value, + (next: T) => { + value = next; + }, + ]; +}; + +/** No jitter, so a delay is a number the test can name rather than a range it has to bracket. */ +export const backoff: BackoffPolicy = { baseMs: 500, maxMs: 30_000, factor: 2, jitter: 'none' }; + +export class FakeSocket implements ClientSocket { + readonly sent: string[] = []; + readonly closes: { code: number | undefined; reason: string | undefined }[] = []; + #open: (() => void) | null = null; + #message: ((data: string) => void) | null = null; + #closed: ((code: number) => void) | null = null; + + send(data: string): void { + this.sent.push(data); + } + + close(code?: number, reason?: string): void { + this.closes.push({ code, reason }); + this.#closed?.(code ?? 1000); + } + + onOpen(handler: () => void): void { + this.#open = handler; + } + + onMessage(handler: (data: string) => void): void { + this.#message = handler; + } + + onClose(handler: (code: number) => void): void { + this.#closed = handler; + } + + open(): void { + this.#open?.(); + } + + deliver(frame: Frame): void { + this.#message?.(JSON.stringify(frame)); + } + + frames(): readonly Frame[] { + return this.sent.map((data) => decode(data)); + } +} + +/** The sid the client minted for its one registration, read off the subscribe frame it sent. */ +export function decodeSid(socket: FakeSocket | undefined): string { + const frame = socket?.frames().find((sent) => sent.type === 'subscribe'); + return frame?.type === 'subscribe' ? frame.sid : ''; +} + +/** The fake timer. `fire()` is all that advances a reconnect — no wall clock, nothing sleeps. */ +export class ManualScheduler { + #armed: { fn: () => void; ms: number } | null = null; + /** Every delay ever armed, in order, so a backoff curve is assertable after the fact. */ + readonly delays: number[] = []; + + readonly schedule: Scheduler = (fn, ms) => { + this.#armed = { fn, ms }; + this.delays.push(ms); + const mine = this.#armed; + return () => { + if (this.#armed === mine) this.#armed = null; + }; + }; + + get pending(): number | null { + return this.#armed?.ms ?? null; + } + + fire(): void { + const armed = this.#armed; + // A throw, not `expect.unreachable`: `tsconfig.json` excludes tests, so a non-test file that + // reaches for a matcher is invisible to `tsc` until the gate says otherwise. Same shape as + // `@ultimat3/jobs`' backfill fixture — test material, kept out of the tarball by `files`. + if (armed === null) throw new HarnessMisuse('fire() with nothing armed'); + this.#armed = null; + armed.fn(); + } +} + +export interface Harness { + readonly client: LiveClient; + readonly timers: ManualScheduler; + /** Every socket the client dialled, oldest first. A reconnect is a new entry here. */ + readonly sockets: FakeSocket[]; + readonly clock: ReturnType; + /** Everything the client reported through `onError`, in order. The host's `window.onerror`. */ + readonly errors: unknown[]; + /** Makes the next `count` dials throw — the socket constructor a browser is allowed to refuse. */ + failNextDials(count: number): void; +} + +export function harness(): Harness { + const timers = new ManualScheduler(); + const sockets: FakeSocket[] = []; + const errors: unknown[] = []; + const clock = frozenClock(1_000); + let failures = 0; + const client = new LiveClient({ + signal, + connect: () => { + if (failures > 0) { + failures -= 1; + // What a browser actually throws when it refuses `new WebSocket(...)`, so the fixture is + // the real failure rather than a framework error this code path can never produce. + throw new TypeError('socket refused'); + } + const socket = new FakeSocket(); + sockets.push(socket); + return socket; + }, + buildId: 'build-1', + backoff, + clock, + scheduler: timers.schedule, + onError: (error) => { + errors.push(error); + }, + }); + return { + client, + timers, + sockets, + clock, + errors, + failNextDials: (count) => { + failures = count; + }, + }; +} + +export const feed = { name: 'feed' }; diff --git a/packages/realtime/src/client-reconnect.test.ts b/packages/realtime/src/client-reconnect.test.ts new file mode 100644 index 00000000..43698e01 --- /dev/null +++ b/packages/realtime/src/client-reconnect.test.ts @@ -0,0 +1,153 @@ +// The reconnect timer: the one behaviour of `client.ts` no other suite can reach, because +// `hooks.test.ts` drives the client but has to call `connect()` a second time by hand. Everything +// here is the timer — that only the live socket's close arms one, that it dials, that the server's +// delay survives the close it triggers, that a refused dial is reported rather than thrown out of +// a timer nobody awaits, and that `close()` cancels it. The scheduler is injected, so nothing sleeps. + +import { describe, expect, test } from 'bun:test'; +import { feed, harness } from './client-harness-fixture'; +import type { Row } from './json'; +import { PROTOCOL_VERSION } from './sync-protocol'; + +describe('LiveClient reconnect', () => { + test('a dropped socket arms a timer that actually dials again', () => { + const { client, timers, sockets } = harness(); + client.connect(); + sockets[0]?.open(); + expect(client.connected).toBe(true); + + sockets[0]?.close(1006); + expect(client.connected).toBe(false); + expect(sockets).toHaveLength(1); // nothing dials synchronously — the delay is the whole point + expect(timers.pending).toBe(500); + + timers.fire(); + expect(sockets).toHaveLength(2); // the timer called connect(), which is the bug this closes + sockets[1]?.open(); + expect(client.connected).toBe(true); + }); + + test('reconnectAt is the armed delay, and clears once the socket is back', () => { + const { client, timers, sockets, clock } = harness(); + client.connect(); + sockets[0]?.open(); + expect(client.reconnectAt()).toBeNull(); + + sockets[0]?.close(1006); + expect(client.reconnectAt()).toBe(clock.now().getTime() + 500); + + timers.fire(); + // Still set while dialling: a countdown that blinks to null mid-attempt reads as "connected". + expect(client.reconnectAt()).toBe(1_500); + sockets[1]?.open(); + expect(client.reconnectAt()).toBeNull(); + }); + + test('successive failures back off, and a successful open resets the curve', () => { + const { client, timers, sockets } = harness(); + client.connect(); + sockets[0]?.open(); + + sockets[0]?.close(1006); + timers.fire(); + sockets[1]?.close(1006); // dialled, never opened + timers.fire(); + sockets[2]?.close(1006); + expect(timers.delays).toEqual([500, 1000, 2000]); + + timers.fire(); + sockets[3]?.open(); // this one lands + sockets[3]?.close(1006); + expect(timers.delays.at(-1)).toBe(500); // attempt counter reset on open + }); + + test('the whole subscription set is re-established on the automatic reconnect', () => { + const { client, timers, sockets } = harness(); + client.connect(); + sockets[0]?.open(); + client.useLive(feed, { orgId: 'o1' }); + + sockets[0]?.close(1006); + timers.fire(); + sockets[1]?.open(); + + const kinds = sockets[1]?.frames().map((frame) => frame.type) ?? []; + expect(kinds).toEqual(['hello', 'subscribe']); + }); + + test('a server-assigned delay survives the close it triggers', () => { + const { client, timers, sockets } = harness(); + client.connect(); + sockets[0]?.open(); + + sockets[0]?.deliver({ + type: 'reconnect', + v: PROTOCOL_VERSION, + afterMs: 7_777, + reason: 'drain', + }); + + // The close the frame triggers must not overwrite the node's spread slot with a local backoff. + expect(timers.delays).toEqual([7_777]); + expect(timers.pending).toBe(7_777); + expect(sockets[0]?.closes).toEqual([{ code: 1001, reason: 'drain' }]); + }); + + test('a close never stacks a second timer on top of an armed one', () => { + const { client, timers, sockets } = harness(); + client.connect(); + sockets[0]?.open(); + + sockets[0]?.close(1006); + sockets[0]?.close(1006); // a socket that reports its close twice + expect(timers.delays).toEqual([500]); + + timers.fire(); + expect(sockets).toHaveLength(2); + }); + + test('a dial that throws is reported, not rethrown, and the next attempt is armed', () => { + const { client, timers, sockets, errors, failNextDials } = harness(); + client.connect(); + sockets[0]?.open(); + sockets[0]?.close(1006); + + failNextDials(1); + // Nothing awaits a timer: a throw out of one is `window.onerror` in a tab and an uncaught + // exception under Bun — the retry killing the process that was going to run it. + expect(() => timers.fire()).not.toThrow(); + expect(errors).toHaveLength(1); // reported through the seam instead + expect(errors[0]).toBeInstanceOf(TypeError); + expect(String(errors[0])).toBe('TypeError: socket refused'); + expect(sockets).toHaveLength(1); // the dial produced nothing… + expect(timers.pending).toBe(1000); // …and the chain is still armed, one attempt further on + + timers.fire(); + sockets[1]?.open(); + expect(client.connected).toBe(true); + }); + + test('a connect() the caller made itself arms nothing when it throws', () => { + const { client, timers, errors, failNextDials } = harness(); + failNextDials(1); + + // The timer owns the chain; a direct call is the app's, and swallowing it here would retry + // behind the back of a caller who is holding the error — so it is never reported either. + expect(() => client.connect()).toThrow('socket refused'); + expect(timers.pending).toBeNull(); + expect(errors).toEqual([]); + }); + + test('an explicit connect() cancels the pending reconnect instead of racing it', () => { + const { client, timers, sockets } = harness(); + client.connect(); + sockets[0]?.open(); + sockets[0]?.close(1006); + expect(timers.pending).toBe(500); + + client.connect(); + expect(timers.pending).toBeNull(); + sockets[1]?.open(); + expect(sockets).toHaveLength(2); // the cancelled timer never dialled a third + }); +}); diff --git a/packages/realtime/src/client.test.ts b/packages/realtime/src/client.test.ts index 55840756..e5107e6e 100644 --- a/packages/realtime/src/client.test.ts +++ b/packages/realtime/src/client.test.ts @@ -1,323 +1,85 @@ -// The reconnect: the one behaviour of `client.ts` no other suite can reach, because `hooks.test.ts` -// drives the client but has to call `connect()` a second time by hand. Everything here is the timer -// — that only the live socket's close arms one, that it dials, that the server's delay survives the -// close it triggers, that a refused dial is reported rather than thrown out of a timer nobody -// awaits, and that `close()` cancels it. The scheduler is injected, so nothing sleeps. +// The socket lifecycle around `client.ts`: which close speaks for which socket, that a replaced +// socket can neither end the live connection nor apply a frame to it, that a write to a dead +// socket is a no-op rather than a throw, and that every handle a subscription hands back tears +// down exactly once. The reconnect timer is `client-reconnect.test.ts`. import { describe, expect, test } from 'bun:test'; -import { frozenClock } from '@ultimat3/core'; -import type { Topic } from './channel'; -import { type ClientSocket, LiveClient, type SignalFactory } from './client'; +import { decodeSid, feed, harness } from './client-harness-fixture'; import type { Row } from './json'; import { decode, type Frame, PROTOCOL_VERSION } from './sync-protocol'; -import type { BackoffPolicy, Scheduler } from './thundering-herd'; - -/** Synchronous and closure-backed: enough to prove an accessor re-reads, with no reactive runtime. */ -const signal: SignalFactory = (initial: T) => { - let value = initial; - return [ - () => value, - (next: T) => { - value = next; - }, - ]; -}; - -/** No jitter, so a delay is a number the test can name rather than a range it has to bracket. */ -const backoff: BackoffPolicy = { baseMs: 500, maxMs: 30_000, factor: 2, jitter: 'none' }; - -class FakeSocket implements ClientSocket { - readonly sent: string[] = []; - readonly closes: { code: number | undefined; reason: string | undefined }[] = []; - #open: (() => void) | null = null; - #message: ((data: string) => void) | null = null; - #closed: ((code: number) => void) | null = null; - - send(data: string): void { - this.sent.push(data); - } - - close(code?: number, reason?: string): void { - this.closes.push({ code, reason }); - this.#closed?.(code ?? 1000); - } - - onOpen(handler: () => void): void { - this.#open = handler; - } - - onMessage(handler: (data: string) => void): void { - this.#message = handler; - } - - onClose(handler: (code: number) => void): void { - this.#closed = handler; - } - - open(): void { - this.#open?.(); - } - - deliver(frame: Frame): void { - this.#message?.(JSON.stringify(frame)); - } - - frames(): readonly Frame[] { - return this.sent.map((data) => decode(data)); - } -} - -/** The fake timer. `fire()` is all that advances a reconnect — no wall clock, nothing sleeps. */ -class ManualScheduler { - #armed: { fn: () => void; ms: number } | null = null; - /** Every delay ever armed, in order, so a backoff curve is assertable after the fact. */ - readonly delays: number[] = []; - - readonly schedule: Scheduler = (fn, ms) => { - this.#armed = { fn, ms }; - this.delays.push(ms); - const mine = this.#armed; - return () => { - if (this.#armed === mine) this.#armed = null; - }; - }; - - get pending(): number | null { - return this.#armed?.ms ?? null; - } - - fire(): void { - const armed = this.#armed; - if (armed === null) expect.unreachable('nothing armed'); - this.#armed = null; - armed.fn(); - } -} - -interface Harness { - readonly client: LiveClient; - readonly timers: ManualScheduler; - /** Every socket the client dialled, oldest first. A reconnect is a new entry here. */ - readonly sockets: FakeSocket[]; - readonly clock: ReturnType; - /** Everything the client reported through `onError`, in order. The host's `window.onerror`. */ - readonly errors: unknown[]; - /** Makes the next `count` dials throw — the socket constructor a browser is allowed to refuse. */ - failNextDials(count: number): void; -} - -function harness(): Harness { - const timers = new ManualScheduler(); - const sockets: FakeSocket[] = []; - const errors: unknown[] = []; - const clock = frozenClock(1_000); - let failures = 0; - const client = new LiveClient({ - signal, - connect: () => { - if (failures > 0) { - failures -= 1; - // What a browser actually throws when it refuses `new WebSocket(...)`, so the fixture is - // the real failure rather than a framework error this code path can never produce. - throw new TypeError('socket refused'); - } - const socket = new FakeSocket(); - sockets.push(socket); - return socket; - }, - buildId: 'build-1', - backoff, - clock, - scheduler: timers.schedule, - onError: (error) => { - errors.push(error); - }, - }); - return { - client, - timers, - sockets, - clock, - errors, - failNextDials: (count) => { - failures = count; - }, - }; -} - -const feed = { name: 'feed' }; - -describe('LiveClient reconnect', () => { - test('a dropped socket arms a timer that actually dials again', () => { + +describe('LiveClient close events', () => { + test("the live socket's own close goes offline and arms a reconnect", () => { const { client, timers, sockets } = harness(); client.connect(); sockets[0]?.open(); - expect(client.connected).toBe(true); + const handle = client.useLive(feed, { orgId: 'o1' }); sockets[0]?.close(1006); expect(client.connected).toBe(false); - expect(sockets).toHaveLength(1); // nothing dials synchronously — the delay is the whole point + expect(handle.state()).toBe('offline'); expect(timers.pending).toBe(500); - - timers.fire(); - expect(sockets).toHaveLength(2); // the timer called connect(), which is the bug this closes - sockets[1]?.open(); - expect(client.connected).toBe(true); - }); - - test('reconnectAt is the armed delay, and clears once the socket is back', () => { - const { client, timers, sockets, clock } = harness(); - client.connect(); - sockets[0]?.open(); - expect(client.reconnectAt()).toBeNull(); - - sockets[0]?.close(1006); - expect(client.reconnectAt()).toBe(clock.now().getTime() + 500); - - timers.fire(); - // Still set while dialling: a countdown that blinks to null mid-attempt reads as "connected". - expect(client.reconnectAt()).toBe(1_500); - sockets[1]?.open(); - expect(client.reconnectAt()).toBeNull(); - }); - - test('successive failures back off, and a successful open resets the curve', () => { - const { client, timers, sockets } = harness(); - client.connect(); - sockets[0]?.open(); - - sockets[0]?.close(1006); - timers.fire(); - sockets[1]?.close(1006); // dialled, never opened - timers.fire(); - sockets[2]?.close(1006); - expect(timers.delays).toEqual([500, 1000, 2000]); - - timers.fire(); - sockets[3]?.open(); // this one lands - sockets[3]?.close(1006); - expect(timers.delays.at(-1)).toBe(500); // attempt counter reset on open }); - test('the whole subscription set is re-established on the automatic reconnect', () => { + test('a close from a socket the client already replaced changes nothing', () => { const { client, timers, sockets } = harness(); client.connect(); sockets[0]?.open(); - client.useLive(feed, { orgId: 'o1' }); + const handle = client.useLive(feed, { orgId: 'o1' }); - sockets[0]?.close(1006); - timers.fire(); + const stale = sockets[0]; + client.connect(); // e.g. a forced redial after an auth refresh sockets[1]?.open(); - const kinds = sockets[1]?.frames().map((frame) => frame.type) ?? []; - expect(kinds).toEqual(['hello', 'subscribe']); + stale?.close(1006); // the replaced socket's close lands late + expect(client.connected).toBe(true); // the live connection is not the corpse's to end + expect(handle.state()).toBe('loading'); // untouched: only the live socket's close moves it + expect(timers.pending).toBeNull(); // a backoff here dials a third socket behind a healthy one + expect(timers.delays).toEqual([]); }); - test('a server-assigned delay survives the close it triggers', () => { - const { client, timers, sockets } = harness(); + // A remount calling `connect()` on a live client left the previous socket open: its `onMessage` + // kept running, so every patch frame applied twice, and the node held two sockets for one + // client — double presence membership and double fanout — until the tab closed. + test('closes the socket it is replacing, so nothing keeps two live', () => { + const { client, sockets } = harness(); client.connect(); sockets[0]?.open(); - sockets[0]?.deliver({ - type: 'reconnect', - v: PROTOCOL_VERSION, - afterMs: 7_777, - reason: 'drain', - }); - - // The close the frame triggers must not overwrite the node's spread slot with a local backoff. - expect(timers.delays).toEqual([7_777]); - expect(timers.pending).toBe(7_777); - expect(sockets[0]?.closes).toEqual([{ code: 1001, reason: 'drain' }]); - }); - - test('a close never stacks a second timer on top of an armed one', () => { - const { client, timers, sockets } = harness(); client.connect(); - sockets[0]?.open(); - sockets[0]?.close(1006); - sockets[0]?.close(1006); // a socket that reports its close twice - expect(timers.delays).toEqual([500]); - - timers.fire(); + expect(sockets[0]?.closes).toEqual([{ code: 1000, reason: 'reconnect' }]); expect(sockets).toHaveLength(2); }); - test('a dial that throws is reported, not rethrown, and the next attempt is armed', () => { - const { client, timers, sockets, errors, failNextDials } = harness(); - client.connect(); - sockets[0]?.open(); - sockets[0]?.close(1006); - - failNextDials(1); - // Nothing awaits a timer: a throw out of one is `window.onerror` in a tab and an uncaught - // exception under Bun — the retry killing the process that was going to run it. - expect(() => timers.fire()).not.toThrow(); - expect(errors).toHaveLength(1); // reported through the seam instead - expect(errors[0]).toBeInstanceOf(TypeError); - expect(String(errors[0])).toBe('TypeError: socket refused'); - expect(sockets).toHaveLength(1); // the dial produced nothing… - expect(timers.pending).toBe(1000); // …and the chain is still armed, one attempt further on - - timers.fire(); - sockets[1]?.open(); - expect(client.connected).toBe(true); - }); - - test('a connect() the caller made itself arms nothing when it throws', () => { - const { client, timers, errors, failNextDials } = harness(); - failNextDials(1); - - // The timer owns the chain; a direct call is the app's, and swallowing it here would retry - // behind the back of a caller who is holding the error — so it is never reported either. - expect(() => client.connect()).toThrow('socket refused'); - expect(timers.pending).toBeNull(); - expect(errors).toEqual([]); - }); - - test('an explicit connect() cancels the pending reconnect instead of racing it', () => { - const { client, timers, sockets } = harness(); - client.connect(); - sockets[0]?.open(); - sockets[0]?.close(1006); - expect(timers.pending).toBe(500); - - client.connect(); - expect(timers.pending).toBeNull(); - sockets[1]?.open(); - expect(sockets).toHaveLength(2); // the cancelled timer never dialled a third - }); -}); - -describe('LiveClient close events', () => { - test("the live socket's own close goes offline and arms a reconnect", () => { - const { client, timers, sockets } = harness(); + test('a frame from the replaced socket is not applied a second time', () => { + const { client, sockets } = harness(); client.connect(); sockets[0]?.open(); const handle = client.useLive(feed, { orgId: 'o1' }); + const orphan = sockets[0]; - sockets[0]?.close(1006); - expect(client.connected).toBe(false); - expect(handle.state()).toBe('offline'); - expect(timers.pending).toBe(500); - }); - - test('a close from a socket the client already replaced changes nothing', () => { - const { client, timers, sockets } = harness(); client.connect(); - sockets[0]?.open(); - const handle = client.useLive(feed, { orgId: 'o1' }); - - const stale = sockets[0]; - client.connect(); // e.g. a forced redial after an auth refresh — the old socket is still open sockets[1]?.open(); + const sid = decodeSid(sockets[1]); + sockets[1]?.deliver({ + type: 'snapshot', + v: PROTOCOL_VERSION, + sid, + rows: [{ id: 'p1', likes: 1 }], + cursor: { qid: 'q', lsn: '1', digest: 'd1', ids: ['p1'], count: 1, at: 0 }, + }); + expect(handle.rows()).toEqual([{ id: 'p1', likes: 1 }]); - stale?.close(1006); // the replaced socket's close lands late - expect(client.connected).toBe(true); // the live connection is not the corpse's to end - expect(handle.state()).toBe('loading'); // untouched: only the live socket's close moves it - expect(timers.pending).toBeNull(); // a backoff here dials a third socket behind a healthy one - expect(timers.delays).toEqual([]); + // The orphan replaying the same subscription's frame used to overwrite the live one's state. + orphan?.deliver({ + type: 'snapshot', + v: PROTOCOL_VERSION, + sid, + rows: [{ id: 'p1', likes: 99 }], + cursor: { qid: 'q', lsn: '0', digest: 'd0', ids: ['p1'], count: 1, at: 0 }, + }); + expect(handle.rows()).toEqual([{ id: 'p1', likes: 1 }]); }); }); diff --git a/packages/realtime/src/client.ts b/packages/realtime/src/client.ts index 69321c6c..96ca45f2 100644 --- a/packages/realtime/src/client.ts +++ b/packages/realtime/src/client.ts @@ -4,14 +4,19 @@ // with nothing about the subscription changing — that is the ladder's whole promise. import { type Clock, systemClock, uuid } from '@ultimat3/core'; -import { applyPatches } from './apply-patches'; import type { Topic } from './channel'; +import { + applyFrame, + type ClientFrameTarget, + type LiveState, + type Registration, +} from './client-frames'; import type { LiveCursor } from './cursor'; import type { JsonObject, JsonValue, Row } from './json'; import type { LocalStore, LocalTx, TableMap } from './local-store'; import { mutateFrame, type OfflineQueue } from './offline-queue'; -import { type ConflictStrategy, type RebaseLog, reconcile } from './rebase'; -import { decode, encode, type Frame, PROTOCOL_VERSION, type PresenceMember } from './sync-protocol'; +import type { ConflictStrategy, RebaseLog } from './rebase'; +import { decode, encode, type Frame, PROTOCOL_VERSION } from './sync-protocol'; import { type BackoffPolicy, backoffDelay, @@ -21,6 +26,9 @@ import { timeoutScheduler, } from './thundering-herd'; +/** The four states a live subscription renders. Declared with the router that writes them. */ +export type { LiveState } from './client-frames'; + /** Injected reactive primitive. `createSignal` from Solid satisfies this exactly. */ export type SignalFactory = (initial: T) => [get: () => T, set: (next: T) => void]; @@ -33,8 +41,6 @@ export interface ClientSocket { onClose(handler: (code: number) => void): void; } -export type LiveState = 'loading' | 'live' | 'stale' | 'offline'; - export interface LiveHandle extends Disposable { /** The reactive accessor. In an app this is the Solid signal `useLive` returns. */ readonly rows: () => readonly R[]; @@ -84,17 +90,6 @@ const reportToConsole = (error: unknown): void => { console.error(error); }; -interface Registration { - readonly sid: string; - readonly name: string; - readonly input: JsonValue; - readonly setRows: (rows: readonly Row[]) => void; - readonly setState: (state: LiveState) => void; - readonly setCursor: (cursor: LiveCursor | null) => void; - rows: readonly Row[]; - cursor: LiveCursor | null; -} - export class LiveClient { readonly #options: LiveClientOptions; readonly #clock: Clock; @@ -150,6 +145,13 @@ export class LiveClient { connect(): void { this.#closed = false; this.#cancelReconnect(); + // The socket we are replacing goes first. Left open, its `onMessage` keeps running: every + // patch frame applies twice, and the node holds two sockets for one client — double presence + // membership and double fanout — until the tab closes. Nulled before the close so the corpse's + // `onClose` takes its own early return rather than marking the new connection offline. + const previous = this.#socket; + this.#socket = null; + previous?.close(1000, 'reconnect'); const socket = this.#options.connect(); this.#socket = socket; socket.onOpen(() => { @@ -170,7 +172,11 @@ export class LiveClient { void this.drain(); }); socket.onMessage((data) => { - this.#onFrame(decode(data)); + // A frame speaks only for its own socket, the same rule `onClose` follows. A replaced socket + // that is still draining bytes would otherwise fold its patches into the live registrations + // a second time, over newer state. + if (this.#socket !== socket) return; + applyFrame(decode(data), this.#frameTarget); }); socket.onClose(() => { // A close speaks only for its own socket: `connect()` may already have installed a newer one, @@ -365,85 +371,22 @@ export class LiveClient { }); } - #onFrame(frame: Frame): void { - switch (frame.type) { - case 'snapshot': { - const registration = this.#registrations.get(frame.sid); - if (!registration) return; - registration.rows = frame.rows; - registration.cursor = frame.cursor; - registration.setRows(frame.rows); - registration.setCursor(frame.cursor); - registration.setState('live'); - return; - } - case 'patch': { - const registration = this.#registrations.get(frame.sid); - if (registration) { - registration.rows = applyPatches(registration.rows, frame.patches); - registration.setRows(registration.rows); - registration.setState('live'); - return; - } - // No registration: it is a tier-1 channel message on `sid = topic`. - const handlers = this.#topics.get(frame.sid); - if (!handlers) return; - for (const patch of frame.patches) { - if (patch.row === null) continue; - for (const handler of handlers) handler(patch.row); - } - return; - } - case 'ack': { - const queue = this.#options.queue; - // `ack`/`fail` mutate the queue synchronously and persist asynchronously; chaining rather - // than notifying right after the call keeps this correct even if that ordering ever - // changes, and it still fires exactly once the persisted write actually lands. - const settled = frame.error ? queue?.fail(frame.ref, frame.error) : queue?.ack(frame.ref); - void settled?.then(() => this.#notifyQueueChange()); - return; - } - case 'rebase': { - const store = this.#options.store; - const log = this.#options.log; - if (!store || !log) return; - reconcile({ - store, - log, - ack: { - key: frame.key, - entity: frame.entity, - id: frame.row?.id ?? frame.key, - row: frame.row, - }, - }); - return; - } - case 'reconnect': { - // Order is load-bearing: arming first is what makes the close this triggers keep the delay - // the node assigned to *this* socket instead of falling back to a local backoff. - this.#scheduleReconnect(frame.afterMs); - this.#socket?.close(1001, frame.reason); - return; - } - case 'update-available': { - this.#setUpdate(frame.buildId); - return; - } - case 'presence': { - const handlers = this.#topics.get(frame.topic); - if (!handlers) return; - const message: JsonObject = { op: frame.op, members: frame.members.map(memberJson) }; - for (const handler of handlers) handler(message); - return; - } - case 'hello': - case 'subscribe': - case 'mutate': - // Client-authored frames: never received. Ignored rather than thrown, so a future - // bidirectional use of the same kind cannot break an old client. - return; - } + /** + * The client's inbound surface, handed to the router. Built once: a frame reaches exactly these + * members and nothing else on the client. + */ + get #frameTarget(): ClientFrameTarget { + return { + registration: (sid) => this.#registrations.get(sid), + topicHandlers: (topic) => this.#topics.get(topic), + queue: this.#options.queue, + store: this.#options.store, + log: this.#options.log, + setUpdate: (buildId) => this.#setUpdate(buildId), + scheduleReconnect: (afterMs) => this.#scheduleReconnect(afterMs), + closeSocket: (code, reason) => this.#socket?.close(code, reason), + notifyQueueChange: () => this.#notifyQueueChange(), + }; } /** @@ -493,7 +436,3 @@ export class LiveClient { for (const listener of this.#queueListeners) listener(); } } - -function memberJson(member: PresenceMember): JsonValue { - return { id: member.id, actorId: member.actorId, meta: member.meta, updatedAt: member.updatedAt }; -} diff --git a/packages/realtime/src/errors.test.ts b/packages/realtime/src/errors.test.ts index e4672054..393044a6 100644 --- a/packages/realtime/src/errors.test.ts +++ b/packages/realtime/src/errors.test.ts @@ -38,6 +38,7 @@ const ADDED_SINCE = [ 'X_LIVE_ROW_UNIDENTIFIED', 'X_QUERY_NOT_SUBSCRIBABLE', 'X_LIVE_QUERY_UNKNOWN', + 'X_SUBSCRIPTION_ID_TAKEN', ]; /** Widened once: these lists are compared against plain strings, not against the literal union. */ diff --git a/packages/realtime/src/errors.ts b/packages/realtime/src/errors.ts index 9c27a9a9..80c9ed93 100644 --- a/packages/realtime/src/errors.ts +++ b/packages/realtime/src/errors.ts @@ -7,6 +7,7 @@ import { registerErrorCodes, UltimateError } from '@ultimat3/core'; export const REALTIME_OWNED_ERROR_CODES = [ 'X_TOPIC_FORBIDDEN', 'X_SUBSCRIPTION_LIMIT', + 'X_SUBSCRIPTION_ID_TAKEN', 'X_PROTOCOL_VERSION', 'X_CURSOR_STALE', 'X_REBASE_CONFLICT', @@ -51,6 +52,7 @@ export const REALTIME_CLIENT_FAULT_CODES: ReadonlySet = new Set([ ...POLICY_DENIAL_CODES, 'X_TOPIC_FORBIDDEN', 'X_SUBSCRIPTION_LIMIT', + 'X_SUBSCRIPTION_ID_TAKEN', 'X_PROTOCOL_VERSION', 'X_LIVE_QUERY_UNKNOWN', 'X_CURSOR_STALE', @@ -90,6 +92,7 @@ export type RealtimeErrorCode = (typeof REALTIME_ERROR_CODES)[number]; export const REALTIME_ERROR_TITLES: Readonly> = { X_TOPIC_FORBIDDEN: 'the actor may not subscribe to this topic', X_SUBSCRIPTION_LIMIT: 'socket or tenant hit its subscription cap', + X_SUBSCRIPTION_ID_TAKEN: 'a subscribe frame reused a sid this socket already holds', X_PROTOCOL_VERSION: 'client and sync node disagree on the wire protocol', X_CURSOR_STALE: 'the resume LSN is outside the change buffer', X_REBASE_CONFLICT: 'a local mutation could not be rebased', @@ -148,6 +151,21 @@ export class SubscriptionLimitError extends RealtimeError { } } +/** + * The client chose a subscription id it is already using on this socket. Refused rather than + * replaced: attaching over it would strand the earlier subscription inside its query entry, where + * nothing can unsubscribe it and the entry's matcher and shared window are never freed. + */ +export class SubscriptionIdTakenError extends RealtimeError { + constructor(args: { sid: string; socketId: string }) { + super({ + code: 'X_SUBSCRIPTION_ID_TAKEN', + cause: `socket ${args.socketId} already holds a live subscription with sid "${args.sid}"`, + fix: 'send a fresh sid with each subscribe frame — crypto.randomUUID() is what the bundled client uses', + }); + } +} + /** * Client and server disagree on the wire format — a version mismatch or a malformed frame. * Both are the same class of bug (a peer speaking a shape we do not have), so both get one code. diff --git a/packages/realtime/src/live-query-failures.test.ts b/packages/realtime/src/live-query-failures.test.ts index 396c3b13..09c7450b 100644 --- a/packages/realtime/src/live-query-failures.test.ts +++ b/packages/realtime/src/live-query-failures.test.ts @@ -171,7 +171,7 @@ describe('reauthorize', () => { const dropped = await registry.reauthorize(alice.socket); expect(dropped).toEqual([subscription.sid]); - expect(registry.subscription(subscription.sid)).toBeUndefined(); + expect(registry.subscription(alice.socket.id, subscription.sid)).toBeUndefined(); expect(registry.gateFailures).toBe(0); }); @@ -200,7 +200,7 @@ describe('reauthorize', () => { // Not dropped: a database timeout is not a revoked grant, and a client does not resubscribe // to a denial. Desynced instead, so the next flush re-reads under the new actor. expect(dropped).toEqual([]); - expect(registry.subscription(subscription.sid)).toBeDefined(); + expect(registry.subscription(alice.socket.id, subscription.sid)).toBeDefined(); expect(alice.socket.desynced.has(subscription.sid)).toBe(true); expect(registry.gateFailures).toBe(1); expect(failures[0]?.stage).toBe('authorize'); @@ -224,3 +224,80 @@ describe('reauthorize', () => { expect(registry.gateFailures).toBe(0); }); }); + +/** + * A `sid` is client-supplied. Keyed by it alone, one socket could take over — or end — another + * socket's live subscription, and the subscription it displaced was stranded inside its query + * entry where nothing could ever free it. + */ +describe('a subscription is owned by the socket that opened it', () => { + const registryWithFeed = (): LiveQueryRegistry => + new LiveQueryRegistry({ source: new RingChangeBuffer() }).register(definitionWith({})); + + test('two sockets may hold the same sid, and each keeps its own subscription', async () => { + const registry = registryWithFeed(); + const alice = socketFor('s-alice', actor('alice')); + const bob = socketFor('s-bob', actor('bob')); + + await registry.subscribe({ socket: alice.socket, name: 'liveFeed', input, sid: 'S' }); + await registry.subscribe({ socket: bob.socket, name: 'liveFeed', input, sid: 'S' }); + + expect(registry.subscription('s-alice', 'S')?.socket.id).toBe('s-alice'); + expect(registry.subscription('s-bob', 'S')?.socket.id).toBe('s-bob'); + expect(registry.subscriberCount(registry.subscription('s-alice', 'S')?.qid ?? '')).toBe(2); + }); + + test("a drop frame from one socket cannot end another socket's stream", async () => { + const registry = registryWithFeed(); + const alice = socketFor('s-alice', actor('alice')); + const bob = socketFor('s-bob', actor('bob')); + await registry.subscribe({ socket: alice.socket, name: 'liveFeed', input, sid: 'S' }); + await registry.subscribe({ socket: bob.socket, name: 'liveFeed', input, sid: 'S' }); + + registry.unsubscribe('s-bob', 'S'); + + expect(registry.subscription('s-bob', 'S')).toBeUndefined(); + expect(registry.subscription('s-alice', 'S')).toBeDefined(); + }); + + // The leak the reused sid caused: `unsubscribeSocket` walked the sid index, found nothing for + // the displaced subscription, and its query entry — matcher, shared window and all — was + // pinned for the life of the process, fanning every change out to a socket that had gone. + test('a closed socket frees its subscriptions even when another reused its sid', async () => { + const registry = registryWithFeed(); + const alice = socketFor('s-alice', actor('alice')); + const bob = socketFor('s-bob', actor('bob')); + const { subscription } = await registry.subscribe({ + socket: alice.socket, + name: 'liveFeed', + input, + sid: 'S', + }); + await registry.subscribe({ socket: bob.socket, name: 'liveFeed', input, sid: 'S' }); + + registry.unsubscribeSocket('s-alice'); + registry.unsubscribeSocket('s-bob'); + + expect(registry.subscription('s-alice', 'S')).toBeUndefined(); + expect(registry.subscription('s-bob', 'S')).toBeUndefined(); + expect(registry.subscriberCount(subscription.qid)).toBe(0); + }); + + test('refuses a sid the same socket already holds, rather than stranding the first', async () => { + const registry = registryWithFeed(); + const alice = socketFor('s-alice', actor('alice')); + const { subscription } = await registry.subscribe({ + socket: alice.socket, + name: 'liveFeed', + input, + sid: 'S', + }); + + await expect( + registry.subscribe({ socket: alice.socket, name: 'liveFeed', input, sid: 'S' }), + ).rejects.toThrow('X_SUBSCRIPTION_ID_TAKEN'); + + expect(registry.subscription('s-alice', 'S')).toBe(subscription); + expect(registry.subscriberCount(subscription.qid)).toBe(1); + }); +}); diff --git a/packages/realtime/src/live-query-window.test.ts b/packages/realtime/src/live-query-window.test.ts index 33fac7b8..ebe7c93f 100644 --- a/packages/realtime/src/live-query-window.test.ts +++ b/packages/realtime/src/live-query-window.test.ts @@ -140,7 +140,7 @@ async function feedWithOneSlowGate(): Promise<{ }); bob.ws.frames.length = 0; slowNext = true; - return { registry, ws: bob.ws, sid: subscription.sid }; + return { registry, ws: bob.ws, sid: subscription.sid, socketId: bob.socket.id }; } describe('a delivery holds the query id it is fanning out', () => { @@ -160,7 +160,7 @@ describe('a delivery holds the query id it is fanning out', () => { }); test('the cursor ends on the newest change, never rewound by a slower one', async () => { - const { registry, sid } = await feedWithOneSlowGate(); + const { registry, sid, socketId } = await feedWithOneSlowGate(); await Promise.all([ registry.deliver(change(2, { ...bobsRow, likes: 1 }, bobsRow)), @@ -169,7 +169,7 @@ describe('a delivery holds the query id it is fanning out', () => { // A rewound cursor is a reconnect that replays patches the client already applied, on top of // newer ones it also applied — the row ends up at the older value and stays there. - expect(registry.subscription(sid)?.cursor.lsn).toBe(formatLsn(3)); + expect(registry.subscription(socketId, sid)?.cursor.lsn).toBe(formatLsn(3)); }); test('a fanout that throws does not wedge every later change for that query id', async () => { diff --git a/packages/realtime/src/live-query.ts b/packages/realtime/src/live-query.ts index a8587772..43aa3605 100644 --- a/packages/realtime/src/live-query.ts +++ b/packages/realtime/src/live-query.ts @@ -16,7 +16,7 @@ import { type ResumeSource, resumeFrom, } from './cursor'; -import { isPolicyDenial, LiveQueryUnknownError, SubscriptionLimitError } from './errors'; +import { isPolicyDenial, LiveQueryUnknownError, SubscriptionIdTakenError } from './errors'; import { canonicalJson, fnv1a, type JsonValue, type Row, type RowPatch } from './json'; import { applyToWindow, @@ -26,6 +26,7 @@ import { } from './matcher-bridge'; import type { SyncSocket } from './socket'; import { type Subscriber, SubscriberGate, type SubscriberGateOptions } from './subscriber-gate'; +import { SubscriptionBook, subscriptionKey } from './subscription-book'; import { type Frame, PROTOCOL_VERSION } from './sync-protocol'; import { WindowLock } from './window-lock'; @@ -102,7 +103,8 @@ interface QueryEntry { export class LiveQueryRegistry { readonly #definitions = new Map(); readonly #entries = new Map(); - readonly #bySid = new Map(); + /** Keyed by `(socket, sid)`, never by `sid` alone — `subscription-book.ts` owns why. */ + readonly #book = new SubscriptionBook(); readonly #options: LiveQueryRegistryOptions; readonly #clock: Clock; readonly #gate: SubscriberGate; @@ -136,8 +138,9 @@ export class LiveQueryRegistry { return this.#entries.get(qid)?.subscribers.size ?? 0; } - subscription(sid: string): LiveSubscription | undefined { - return this.#bySid.get(sid); + /** One socket's subscription. A sid alone does not identify one — see `subscription-book.ts`. */ + subscription(socketId: string, sid: string): LiveSubscription | undefined { + return this.#book.get(socketId, sid); } /** @@ -156,15 +159,21 @@ export class LiveQueryRegistry { // matched, and one string in it names nothing. Reporting it as `X_PROTOCOL_VERSION` handed the // client "x build && redeploy the client" for a typo no rebuild changes. if (!definition) throw new LiveQueryUnknownError({ name: args.name }); - this.#assertLimits(args.socket); + this.#book.assertCapacity(args.socket, this.#options); await definition.authorize?.({ actor: args.socket.actor, input: args.input }); // After this subscriber's own decision, never before it: resolving a shape for a caller who // may not subscribe is work an unauthorized client gets to schedule. await definition.prepare?.(args.input); const qid = qidOf(args.name, args.input); - const entry = this.#entryFor(qid, definition, args.input); const sid = args.sid ?? uuid(); + // Before the entry is built: a sid this socket already holds would overwrite that + // subscription's slot and strand it inside its query entry, where nothing could reach it + // again. The client picked the id, so the client is the one told to pick another. + if (this.#book.has(args.socket.id, sid)) { + throw new SubscriptionIdTakenError({ sid, socketId: args.socket.id }); + } + const entry = this.#entryFor(qid, definition, args.input); const now = this.#clock.now().getTime(); if (args.cursor) { @@ -217,22 +226,23 @@ export class LiveQueryRegistry { }; } - unsubscribe(sid: string): void { - const subscription = this.#bySid.get(sid); + /** Scoped to the socket that asked: a client may only drop its own subscription. */ + unsubscribe(socketId: string, sid: string): void { + const subscription = this.#book.get(socketId, sid); if (!subscription) return; - this.#bySid.delete(sid); + this.#book.delete(socketId, sid); subscription.socket.queries.delete(sid); subscription.socket.clearDesynced(sid); const entry = this.#entries.get(subscription.qid); if (!entry) return; - entry.subscribers.delete(sid); + entry.subscribers.delete(subscriptionKey(socketId, sid)); // An entry with no subscribers stops costing a matcher and a change window. if (entry.subscribers.size === 0) this.#entries.delete(subscription.qid); } unsubscribeSocket(socketId: string): void { - for (const subscription of [...this.#bySid.values()]) { - if (subscription.socket.id === socketId) this.unsubscribe(subscription.sid); + for (const subscription of this.#book.ofSocket(socketId)) { + this.unsubscribe(socketId, subscription.sid); } } @@ -244,8 +254,7 @@ export class LiveQueryRegistry { */ async reauthorize(socket: SyncSocket): Promise { const dropped: string[] = []; - for (const subscription of [...this.#bySid.values()]) { - if (subscription.socket.id !== socket.id) continue; + for (const subscription of this.#book.ofSocket(socket.id)) { try { await subscription.definition.authorize?.({ actor: socket.actor, @@ -253,7 +262,7 @@ export class LiveQueryRegistry { }); } catch (error) { if (isPolicyDenial(error)) { - this.unsubscribe(subscription.sid); + this.unsubscribe(socket.id, subscription.sid); dropped.push(subscription.sid); continue; } @@ -384,8 +393,10 @@ export class LiveQueryRegistry { definition: entry.definition, cursor, }; - entry.subscribers.set(sid, subscription); - this.#bySid.set(sid, subscription); + // The entry's own map takes the SAME composite key: a sid alone would collide across sockets + // here exactly as it did in the book, and `unsubscribe` deletes from both by one identity. + entry.subscribers.set(subscriptionKey(socket.id, sid), subscription); + this.#book.add(subscription); socket.queries.set(sid, entry.qid); socket.clearDesynced(sid); return subscription; @@ -459,23 +470,6 @@ export class LiveQueryRegistry { void reading.then(done, done); return reading; } - - #assertLimits(socket: SyncSocket): void { - const perSocket = this.#options.maxPerSocket ?? 128; - if (socket.queries.size >= perSocket) { - throw new SubscriptionLimitError({ scope: 'socket', id: socket.id, limit: perSocket }); - } - const perTenant = this.#options.maxPerTenant; - const tenant = this.#options.tenantOf?.(socket.actor) ?? null; - if (perTenant === undefined || tenant === null) return; - let count = 0; - for (const subscription of this.#bySid.values()) { - if ((this.#options.tenantOf?.(subscription.socket.actor) ?? null) === tenant) count += 1; - } - if (count >= perTenant) { - throw new SubscriptionLimitError({ scope: 'tenant', id: tenant, limit: perTenant }); - } - } } function orgIdOf(input: JsonValue): string | null { diff --git a/packages/realtime/src/subscription-book.ts b/packages/realtime/src/subscription-book.ts new file mode 100644 index 00000000..e184ba2c --- /dev/null +++ b/packages/realtime/src/subscription-book.ts @@ -0,0 +1,86 @@ +// Who holds which subscription, and the composite identity that makes that answerable. A `sid` +// is CLIENT data — unique only to the socket that chose it — so every lookup here takes the +// owner too, and the per-socket and per-tenant caps are answered from this book because it is +// the only thing that knows what exists. + +import type { Actor } from '@ultimat3/core'; +import { SubscriptionLimitError } from './errors'; +import type { LiveSubscription } from './live-query'; +import type { SyncSocket } from './socket'; + +/** + * The identity of one subscription. `\u0000` because a socket id and a sid are both opaque + * strings and nothing else can appear in one, so no pair of them can collide with another. + */ +export function subscriptionKey(socketId: string, sid: string): string { + return `${socketId}\u0000${sid}`; +} + +export interface SubscriptionCaps { + readonly maxPerSocket?: number; + readonly maxPerTenant?: number; + readonly tenantOf?: (actor: Actor | null) => string | null; +} + +/** Sockets may open this many live queries before `X_SUBSCRIPTION_LIMIT`. */ +export const DEFAULT_MAX_PER_SOCKET = 128; + +/** + * Every live subscription on this node, keyed by `(socket, sid)`. + * + * Keyed by the sid alone, socket B reusing socket A's sid overwrote A's entry — A's subscription + * stayed in its query entry's `subscribers` map, unreachable, so `unsubscribeSocket(A)` freed + * nothing and that entry's matcher and shared window were pinned for the process's life, fanning + * every change out to a dead socket. A `drop` frame from B likewise ended A's stream with no + * error either side. + */ +export class SubscriptionBook { + readonly #bySid = new Map(); + + get(socketId: string, sid: string): LiveSubscription | undefined { + return this.#bySid.get(subscriptionKey(socketId, sid)); + } + + has(socketId: string, sid: string): boolean { + return this.#bySid.has(subscriptionKey(socketId, sid)); + } + + add(subscription: LiveSubscription): void { + this.#bySid.set(subscriptionKey(subscription.socket.id, subscription.sid), subscription); + } + + delete(socketId: string, sid: string): void { + this.#bySid.delete(subscriptionKey(socketId, sid)); + } + + /** A copy, because every caller mutates the book while walking it. */ + all(): readonly LiveSubscription[] { + return [...this.#bySid.values()]; + } + + /** One socket's subscriptions — the drop list when it closes, the retry list when it reauths. */ + ofSocket(socketId: string): readonly LiveSubscription[] { + return this.all().filter((subscription) => subscription.socket.id === socketId); + } + + /** + * Refuse a subscribe that would exceed a cap. Load shedding, not a crash: both scopes throw + * `X_SUBSCRIPTION_LIMIT` naming which one refused, so the fix line points at one knob. + */ + assertCapacity(socket: SyncSocket, caps: SubscriptionCaps): void { + const perSocket = caps.maxPerSocket ?? DEFAULT_MAX_PER_SOCKET; + if (socket.queries.size >= perSocket) { + throw new SubscriptionLimitError({ scope: 'socket', id: socket.id, limit: perSocket }); + } + const perTenant = caps.maxPerTenant; + const tenant = caps.tenantOf?.(socket.actor) ?? null; + if (perTenant === undefined || tenant === null) return; + let count = 0; + for (const subscription of this.#bySid.values()) { + if ((caps.tenantOf?.(subscription.socket.actor) ?? null) === tenant) count += 1; + } + if (count >= perTenant) { + throw new SubscriptionLimitError({ scope: 'tenant', id: tenant, limit: perTenant }); + } + } +} diff --git a/packages/realtime/src/sync-node.ts b/packages/realtime/src/sync-node.ts index 6a92086a..a453c399 100644 --- a/packages/realtime/src/sync-node.ts +++ b/packages/realtime/src/sync-node.ts @@ -167,7 +167,9 @@ export function createSyncNode(options: SyncNodeOptions): SyncNode { return; } if (frame.op === 'drop') { - options.registry.unsubscribe(frame.sid); + // Scoped to this socket: a sid is client data, and an unscoped drop let one client + // end another's live stream by guessing — or reusing — its id. + options.registry.unsubscribe(socket.id, frame.sid); return; } const { frame: reply } = await options.registry.subscribe({ diff --git a/packages/render/CLAUDE.md b/packages/render/CLAUDE.md index 6a21a78d..95a8cba1 100644 --- a/packages/render/CLAUDE.md +++ b/packages/render/CLAUDE.md @@ -25,6 +25,9 @@ Tier 4. May import tiers 0–3: `core`, `schema`, `i18n`, `money`, `time`, `cach | Registry input | descriptors only. `registerRoute` refuses a raw declaration with `X_ROUTE_UNNORMALIZED` — `defineRoute` is the one normalizer, and every reader downstream assumes it ran. | | Descriptors | `describeRoutes()` must stay JSON-safe, sorted by path, deterministic. | | Boundary | `surfaces.ts` throws; it never warns. Type-only edges are not violations. | +| Stream cancellation | the underlying source has a `cancel()`, and `write` is guarded on it. A client that disconnects mid-stream aborts `StreamHole.resolve(signal)` and every later `write`/`close` is a no-op — `settle()` on a cancelled controller threw out of a `void`ed promise, one unhandled rejection per response, while the resolved holes kept doing their database work with nowhere to write. | +| ISR detach | `attach()`'s returned function clears the revalidator as well as the dependents — and only if the slot is still its own, tracked in `installedRevalidator` because `@ultimat3/cache` holds ONE and offers no read back. Left installed, a detached controller and its whole store stayed reachable and kept receiving revalidations while the live one's pages never went stale. | +| ISR store bound | `memoryIsrStore()` caps at `DEFAULT_ISR_MAX_ENTRIES` (1,000), least recently generated first — a route table supports `:params` and `*`, so `/blog/:slug` retains one full HTML string per slug ever requested, 404-shaped ones included. | | Errors | `errors.ts` subclasses only. Never a bare `Error`, never a bare `TODO`. | | Policy | render checks *presence* only. Evaluation belongs to `@ultimat3/policy`. | | Responses | return `RenderResult`. `@ultimat3/http` builds the `Response`. | diff --git a/packages/render/README.md b/packages/render/README.md index c809d9de..9655a58c 100644 --- a/packages/render/README.md +++ b/packages/render/README.md @@ -166,9 +166,13 @@ types-only. `action({ cache: { invalidates: [tag.post] } })` reaches ISR in the same hop as memo, LRU, Redis and the CDN, and regenerates exactly the dependent pages — nobody lists pages by hand, so nobody forgets one. `controller.attach()` installs it as the - framework's `Revalidator`. + framework's `Revalidator`, and the function it returns releases **both** halves — the + dependents and the revalidator slot, the latter only while it is still this controller's. + The default store (`memoryIsrStore`) is capped at `DEFAULT_ISR_MAX_ENTRIES` (1,000) pages, + least recently generated evicted first. - **`stream`** flushes the shell first, then reveals holes in completion order with a - ~200-byte inline script. Solid's compiled templates and signals mean the shell costs zero + ~200-byte inline script. A client that disconnects mid-stream cancels it: `StreamHole.resolve` + is handed an `AbortSignal` so the work stops, and nothing more is enqueued. Solid's compiled templates and signals mean the shell costs zero hydration work, so streaming buys TTFB *and* TBT here, not just TTFB. - **`hydrate: 'interaction'`** replays the event that woke the island; without replay the first click on a cold island is silently lost. diff --git a/packages/render/src/index.ts b/packages/render/src/index.ts index 1a11c9c2..14c589bd 100644 --- a/packages/render/src/index.ts +++ b/packages/render/src/index.ts @@ -109,9 +109,11 @@ export type { IsrServeResult, IsrState, IsrStore, + MemoryIsrStoreOptions, } from './render-isr'; export { createIsrController, + DEFAULT_ISR_MAX_ENTRIES, invalidateAndRevalidate, memoryIsrStore, parseTtlMs, diff --git a/packages/render/src/render-isr.test.ts b/packages/render/src/render-isr.test.ts index 4baa749c..66943772 100644 --- a/packages/render/src/render-isr.test.ts +++ b/packages/render/src/render-isr.test.ts @@ -1,8 +1,8 @@ import { beforeEach, describe, expect, test } from 'bun:test'; import type { CacheTag } from '@ultimat3/cache'; -import { resetGraph, tag } from '@ultimat3/cache'; +import { invalidateTags, resetGraph, tag } from '@ultimat3/cache'; import { clearRoutes, describeRoutes, registerRoute } from './registry'; -import { createIsrController, parseTtlMs } from './render-isr'; +import { createIsrController, memoryIsrStore, parseTtlMs } from './render-isr'; import type { RenderResult, RouteMetaFn } from './route'; import { defineRoute } from './route'; @@ -174,3 +174,79 @@ describe('tag-driven revalidation', () => { expect(controller.revalidateByTags([orgTag])).toEqual([]); }); }); + +describe('detach releases the revalidator, not only the dependents', () => { + // The `x dev` hot reload: controller A attaches, the reload detaches it and creates B, and + // `invalidateTags` still called A's `markStale` — so B's pages never went stale and A's whole + // store stayed reachable from the cache graph. + test('a detached controller stops receiving revalidations', async () => { + isrRoute('apps/web/site/team/page.tsx', [orgTag]); + const a = createIsrController({ routes: describeRoutes }); + const detachA = a.attach(); + await a.serve('/team', () => '

a

'); + detachA(); + + // The reload's other half: a new controller renders the same path — which puts it back in + // the graph — and never attaches, so the only revalidator installed is the detached one's. + const b = createIsrController({ routes: describeRoutes }); + await b.serve('/team', () => '

b

'); + + await invalidateTags([orgTag]); + + expect(a.store().get('/team')?.stale).toBe(false); + expect(b.store().get('/team')?.stale).toBe(false); + }); + + // Detaching in the wrong order must not silence the controller that owns the slot now. + test('a stale detach never clears a revalidator another controller installed', async () => { + isrRoute('apps/web/site/team/page.tsx', [orgTag]); + const a = createIsrController({ routes: describeRoutes }); + const detachA = a.attach(); + const b = createIsrController({ routes: describeRoutes }); + b.attach(); + await b.serve('/team', () => '

b

'); + + detachA(); + await invalidateTags([orgTag]); + + expect(b.store().get('/team')?.stale).toBe(true); + }); +}); + +describe('the default ISR store is bounded', () => { + test('evicts the least recently generated page once the cap is spent', () => { + const store = memoryIsrStore({ maxEntries: 3 }); + const entry = (path: string) => ({ + path, + html: `

${path}

`, + hash: path, + generatedAt: 0, + ttlMs: null, + stale: false, + }); + + for (const path of ['/a', '/b', '/c', '/d']) store.set(entry(path)); + + expect(store.paths()).toEqual(['/b', '/c', '/d']); + expect(store.get('/a')).toBeUndefined(); + }); + + test('re-generating a page makes it the most recent, not a second entry', () => { + const store = memoryIsrStore({ maxEntries: 2 }); + const entry = (path: string, generatedAt: number) => ({ + path, + html: `

${path}

`, + hash: path, + generatedAt, + ttlMs: null, + stale: false, + }); + + store.set(entry('/a', 1)); + store.set(entry('/b', 2)); + store.set(entry('/a', 3)); // /a regenerates, so /b is now the oldest + store.set(entry('/c', 4)); + + expect(store.paths()).toEqual(['/a', '/c']); + }); +}); diff --git a/packages/render/src/render-isr.ts b/packages/render/src/render-isr.ts index aaaceafd..a9eb81b2 100644 --- a/packages/render/src/render-isr.ts +++ b/packages/render/src/render-isr.ts @@ -5,7 +5,7 @@ * and tag-driven staleness (an action's `invalidates` marks exactly the dependent routes). */ -import type { CacheTag } from '@ultimat3/cache'; +import type { CacheTag, Revalidator } from '@ultimat3/cache'; import { dependentsOfKind, invalidateTags, @@ -38,12 +38,34 @@ export interface IsrStore { paths(): readonly string[]; } -export function memoryIsrStore(): IsrStore { +/** + * How many rendered pages the default store holds. A route table supports `:params` and `*`, so + `/blog/:slug` has as many ISR paths as the blog has slugs — 404-shaped ones that still render + * included. Unbounded, a crawler over 100k slugs is 100k HTML strings resident for the life of + * the process. + */ +export const DEFAULT_ISR_MAX_ENTRIES = 1_000; + +export interface MemoryIsrStoreOptions { + /** Pages retained. The least recently generated goes first. */ + readonly maxEntries?: number; +} + +export function memoryIsrStore(options: MemoryIsrStoreOptions = {}): IsrStore { + const maxEntries = options.maxEntries ?? DEFAULT_ISR_MAX_ENTRIES; const map = new Map(); return { get: (path) => map.get(path), set: (entry) => { + // Re-inserted rather than overwritten, so the Map's iteration order IS generation order and + // the first key is the least recently generated page. + map.delete(entry.path); map.set(entry.path, entry); + while (map.size > maxEntries) { + const oldest = map.keys().next(); + if (oldest.done === true) break; + map.delete(oldest.value); + } }, delete: (path) => { map.delete(path); @@ -109,6 +131,15 @@ export interface IsrController { attach(): () => void; } +/** + * `@ultimat3/cache` holds ONE revalidator and offers no read back, so detaching has to know + * whether the slot is still this controller's — a controller that attached after it owns it now. + */ +let installedRevalidator: Revalidator | undefined; + +/** What `registerRevalidator` is handed on detach: the framework's "nothing to revalidate". */ +const NO_REVALIDATION: Revalidator = () => undefined; + export function createIsrController(options: IsrControllerOptions = {}): IsrController { const store = options.store ?? memoryIsrStore(); const now = options.now ?? (() => Date.now()); @@ -223,12 +254,23 @@ export function createIsrController(options: IsrControllerOptions = {}): IsrCont attach() { // The cache fanout owns the trigger; render owns only "what does stale mean here". - registerRevalidator((path) => { + const revalidate: Revalidator = (path) => { markStale(path); - }); + }; + registerRevalidator(revalidate); + installedRevalidator = revalidate; return () => { for (const path of registered) unregisterDependent({ kind: 'isr-route', id: path }); registered.clear(); + // Left installed, this closure — and the whole store behind it — stayed reachable from + // the cache graph and kept receiving revalidations. `x dev`'s hot reload detached A and + // created B, and `invalidateTags` still called A's `markStale`: B's pages never went + // stale and A's store was never collected. Only if the slot is still OURS: a controller + // that attached after us owns it, and clearing that one is this bug pointed backwards. + if (installedRevalidator === revalidate) { + registerRevalidator(NO_REVALIDATION); + installedRevalidator = undefined; + } }; }, }; diff --git a/packages/render/src/render-stream.test.ts b/packages/render/src/render-stream.test.ts index 90592d3e..dcc7bee7 100644 --- a/packages/render/src/render-stream.test.ts +++ b/packages/render/src/render-stream.test.ts @@ -91,3 +91,75 @@ describe('out-of-order streaming', () => { expect(html).toBe('

static

'); }); }); + +describe('a client that disconnects mid-stream', () => { + /** Three holes, none of which resolves until the test says so. */ + function threeHoles(): { + plan: StreamPlan; + signals: AbortSignal[]; + settle: (id: string, html: string) => void; + } { + const gates = new Map void>(); + const signals: AbortSignal[] = []; + const hole = (id: string) => ({ + id, + fallback: '', + resolve: (signal: AbortSignal): Promise => { + signals.push(signal); + return new Promise((res) => gates.set(id, res)); + }, + }); + return { + plan: { + head: '', + shell: `${holeMarker('a', '')}${holeMarker('b', '')}${holeMarker('c', '')}`, + holes: [hole('a'), hole('b'), hole('c')], + }, + signals, + settle: (id, html) => gates.get(id)?.(html), + }; + } + + // The measured failure: `settle()` ran `write(tail)` and `controller.close()` on a cancelled + // controller, throwing out of the final `.then(settle, settle)` whose promise is `void`ed — + // one unhandled rejection per response. Bun's test runner fails this file on one. + test('a hole resolving after the cancel enqueues nothing and throws nothing', async () => { + const { plan, settle } = threeHoles(); + const stream = renderStreamHtml(plan, { buildId: 'b1' }); + const reader = stream.getReader(); + await reader.read(); // the shell + + await reader.cancel('client gone'); + + settle('a', 'a'); + settle('b', 'b'); + settle('c', 'c'); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + // Meanwhile the resolved holes kept doing their database work with nowhere to write it. + test('aborts every hole still running', async () => { + const { plan, signals } = threeHoles(); + const stream = renderStreamHtml(plan, { buildId: 'b1' }); + const reader = stream.getReader(); + await reader.read(); + + expect(signals).toHaveLength(3); + expect(signals.every((signal) => signal.aborted)).toBe(false); + + await reader.cancel('client gone'); + + expect(signals.every((signal) => signal.aborted)).toBe(true); + }); + + test('a live stream leaves the holes un-aborted', async () => { + const { plan, signals, settle } = threeHoles(); + const stream = renderStreamHtml(plan, { buildId: 'b1' }); + settle('a', 'a'); + settle('b', 'b'); + settle('c', 'c'); + + expect(await collectStream(stream)).toContain(''); + expect(signals.some((signal) => signal.aborted)).toBe(false); + }); +}); diff --git a/packages/render/src/render-stream.ts b/packages/render/src/render-stream.ts index da52ec5f..2d95722d 100644 --- a/packages/render/src/render-stream.ts +++ b/packages/render/src/render-stream.ts @@ -19,7 +19,11 @@ export interface StreamHole { readonly id: string; /** Rendered synchronously into the first flush (the ``). */ readonly fallback: string; - readonly resolve: () => Promise; + /** + * `signal` aborts when the response is cancelled — a client that disconnected mid-stream. A + * hole that ignores it still finishes; it just finishes into a document nobody reads. + */ + readonly resolve: (signal: AbortSignal) => Promise; } export interface StreamPlan { @@ -75,20 +79,38 @@ export function renderStreamHtml( const tail = plan.tail ?? ''; const errorFallback = options.errorFallback ?? ((id) => ``); + /** + * The response's own lifetime. A client that disconnects mid-stream cancels the stream, and + * both halves of that have to be honoured: nothing more may be enqueued — `settle`'s + * `write(tail)`/`close()` on a cancelled controller threw out of a `void`ed promise, one + * unhandled rejection per response — and the holes still running must be told to stop doing + * their database work for a document nobody will read. + */ + const holes = new AbortController(); + let closed = false; return new ReadableStream({ start(controller) { const write = (chunk: string): void => { + // `desiredSize` is null once the controller is closed or errored, which is the half a + // cancellation flag cannot see on its own. + if (closed || controller.desiredSize === null) return; controller.enqueue(encoder.encode(chunk)); }; + const close = (): void => { + if (closed) return; + closed = true; + controller.close(); + }; + // No holes, no reveal script: a page that streams nothing pays nothing. write(plan.head + (plan.holes.length > 0 ? REVEAL_SCRIPT : '') + plan.shell); let pending = plan.holes.length; if (pending === 0) { write(tail); - controller.close(); + close(); return; } @@ -96,13 +118,13 @@ export function renderStreamHtml( pending -= 1; if (pending === 0) { write(tail); - controller.close(); + close(); } }; for (const hole of plan.holes) { void hole - .resolve() + .resolve(holes.signal) .then( (html) => { write(revealChunk(hole.id, html)); @@ -117,6 +139,12 @@ export function renderStreamHtml( .then(settle, settle); } }, + + /** The client went away. Stop enqueueing, and stop the work that was going to be enqueued. */ + cancel(reason: unknown) { + closed = true; + holes.abort(reason); + }, }); } diff --git a/wiki/Error-Codes.md b/wiki/Error-Codes.md index 1dedaeb4..00cdbace 100644 --- a/wiki/Error-Codes.md +++ b/wiki/Error-Codes.md @@ -183,6 +183,7 @@ A denial is `X_FORBIDDEN`, above — `@ultimat3/policy` owns it and every surfac | Code | Means | Typical cause | Fix | |---|---|---|---| | `X_ACTION_DUPLICATE` | two actions registered under one name | duplicate export names across features | rename one; names are global. `x actions list --json` | +| `X_ACTION_PATH_DUPLICATE` | two actions derive one HTTP path | two distinct names that pluralize onto the same route — `archiveOrder` and `archiveOrders` both derive `POST /api/orders/archive`. Names differ so `X_ACTION_DUPLICATE` does not fire, and the action seated last silently shadows the other while its OpenAPI operation and MCP tool still advertise it | rename one export so the two derive different paths — `x actions list --json` prints every derived route | | `X_INPUT_INVALID` | input failed the primitive's schema — an action's, and a query's too | wrong shape from a client or an agent; over HTTP it is a **400**, never a 500 | `x actions describe --json` (`x queries describe --json` for a read) | | `X_OUTPUT_INVALID` | the handler returned a value its `output` schema rejects | the handler drifted from the declared output | `x actions describe --json`, then fix the handler or the schema | | `X_ACTION_FOREIGN` | a value that is not an action was projected as one | a hand-rolled object with `kind: 'action'`, or an action from a duplicated copy of `@ultimat3/action` | declare it as `export const name = action({ input, output, policy, handle })` | @@ -225,6 +226,7 @@ synthesizes `https://ultimate.dev/errors/` for a code no page here documen |---|---|---|---| | `X_TOPIC_FORBIDDEN` | the actor may not subscribe to this topic | no guard, or the guard denied | `hub.guard('', ({ actor }) => …)` | | `X_SUBSCRIPTION_LIMIT` | socket or tenant hit its subscription cap | a component subscribing in a loop | raise `realtime.limits.perSocket` / `perTenant`, or unsubscribe unused live queries | +| `X_SUBSCRIPTION_ID_TAKEN` | a subscribe frame reused a `sid` this socket already holds | a hand-rolled client incrementing its own subscription ids, or a remount that reuses one. Subscriptions are keyed by `(socket, sid)`, so a reused id would otherwise orphan the earlier subscription — its entry never reaching zero subscribers and never being freed | send a fresh sid with each subscribe frame — `crypto.randomUUID()` is what the bundled client uses | | `X_PROTOCOL_VERSION` | client and sync node disagree on the wire protocol | a client from an older build | `x build` and redeploy the client; the node sends `update-available` before draining | | `X_CURSOR_STALE` | the resume LSN is outside the change buffer | a long disconnect | pass `snapshot` to `resumeFrom()` so the fallback re-snapshots | | `X_REBASE_CONFLICT` | a local mutation could not be rebased | server state moved incompatibly | set `conflict: 'server-wins'`, or return a row from `custom(merge)` |