diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b070d2f..9ad8e83d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -158,6 +158,28 @@ Semver applies from 1.0.0. A breaking change to a documented API needs a major ### Changed +- **BREAKING — `DESCRIPTION_MIN_LENGTH` is deleted from `@ultimat3/seo`.** It was exported, + documented as *"validate.ts enforces it"*, and read by no validator anywhere in the repo — a + length bound whose only effect was to be importable. Enforcing it instead would have needed a new + `X_SEO_*` code and would have newly failed both tracked apps (`dummy/social-media-clone`'s + `admin.home.description` is 32 characters), so the honest change is to stop shipping a gate that + does not exist. **Manual edit:** delete the import; there is no replacement, and no minimum + description length is checked. A test now pins that every bound `@ultimat3/seo` exports is one + `validateMeta` actually enforces, so this cannot recur. + +- **BREAKING — a metric redeclared with different `bounds` or a different `observe` is refused** + (`X_METRIC_NAME_INVALID`) rather than silently answering the first declaration's instrument. + `InstrumentOptions.maxSeries` documented "the first declaration of a name wins" and `bounds`/ + `observe` did not, so a second `histogram('x', { bounds })` kept the original buckets with no + signal. An **omitted** option is still a handle-fetch — `gauge(name)` is unchanged — and + `maxSeries`/`unit`/`description` keep their shipped first-wins rule. **Manual edit:** make the + second declaration state the same `bounds`/`observe`, or fetch the handle without options. + +- **`cachedFormatter` and `canonicalLocale` moved from `@ultimat3/time` to `@ultimat3/core`.** Both + are re-exported from `time`, so no import breaks. They moved because `@ultimat3/money` needed the + same bound and `money → time` is a **sideways** tier-1 import the boundary check refuses — the + choice was one mechanism in tier 0 or a second copy of it, and axiom 1 settles that. + - **BREAKING — `Seed.run()` resolves with a result object instead of `void`.** It now answers `SeedRun` — `{ name, tier, metrics: { inserted, updated, skipped } }` — which is what lets `x db seed` report a table and a `--json` body rather than "done". A caller that awaited it for @@ -172,6 +194,106 @@ Semver applies from 1.0.0. A breaking change to a documented API needs a major ### Fixed +- **SECURITY — a signed storage key differing only in the case of its `org/` prefix escaped the + tenancy gate.** `isTenantScoped` compared the first segment exactly, so `Org/org-2/secret.png` + read as *not* tenant-scoped and skipped the org check entirely — and `Org/` and `org/` are one + directory on APFS and NTFS, so the local driver then opened the other tenant's file. The + predicate now folds case on that segment, which also closes it in `x dev`'s asset and storage + routes, where the key is client-supplied with no signature at all. `isWithinOrg` stays + exact-case, so a folded prefix is refused outright rather than matched. + +- **No signed storage URL verified under the documented defaults.** `localDriver` signed under + `/_storage/local` while `verifySignedUrl` / `acceptSignedUpload` / `readSignedObject` defaulted to + `/_storage`, so the key parsed as `local/` and `grantUpload` → `acceptSignedUpload` — the + pair `docs/architecture/17-uploads.md` documents, neither call passing `baseUrl` — always failed + `X_STORAGE_URL_INVALID`. The base is now stated once as `signedUrlBaseFor(driverName)` and the + verify side derives it from `disk.name`, which the caller already passes. + +- **An app's own shared assets were unreachable through a signed URL.** `constraintsFor` gated on + `isWithinOrg` alone, so any key outside `org//` was refused `X_STORAGE_ORG_MISMATCH`. + `packages/storage/src/path.ts` had already written down that *"the pair is the question"* and + shipped `isTenantScoped` for it; only the dev route used the pair. Nine spoof keys — traversal, + encoded separator, longer-id borrow, reserved segment, leading slash, empty segment, Cyrillic + homoglyph and two case folds — are each pinned refused, every one signed with the real secret. + +- **`policy.requireChecksum` could only ever fail.** `acceptSignedUpload` is the sole production + caller of `validateUpload` and had no field to carry a checksum, so declaring the option broke + every signed upload it governed. `AcceptSignedUploadInput` now carries `checksum`, travelling + exactly as `declaredContentType` does. + +- **`t('valueOf')` threw out of the translator that documents "never throws".** The catalog lookup + was a raw index on a `{}`-prototyped object, so any key naming an `Object.prototype` member + resolved to the inherited value: `t('valueOf')` died with `TypeError: template.includes is not a + function`, and `t('constructor')` returned a **function** through a signature typed `string`. + Reachable wherever a key travels as data (`t(row.labelKey)`). The lookup now goes through the + `Object.hasOwn` guard that sat one line above it, `t.raw()` with it, and `flattenCatalog` / + `mergeCatalogs` build on `Object.create(null)` as `nestCatalog` already did. + +- **A cron day-of-week range that wraps with a step walked an 8-day week.** The wrap span was + `max - min + 1`, which is **8** for day-of-week because the field is 0–7 with two spellings of + Sunday: `0 3 * * sat-tue/2` fired Tue, Sat **and** Sun where Vixie gives Sat and Mon. A `task` + declaring one ran on the wrong days. The span is now stated (7) rather than recomputed, which + leaves every non-wrapping expression byte-identical — normalising the field to 0–6 instead would + have silently changed `0 0 * * 5/2` from `[5,7]` to `[5]`. + +- **Two functions whose entire contract is absorbing a refusal could themselves throw.** + `invalidateTags()` documents *"a dead Redis must not fail the write that triggered the bust"* and + `bestEffort` *"absorbs its refusal"*, but both rendered the caught value with + `error instanceof Error ? error.message : String(error)` — and on a value the framework did not + build, **both halves throw** (a `Proxy` trapping `getPrototypeOf` defeats `instanceof`; a + null-prototype object defeats `String`). All four sites now call `renderThrowable`, which exists + in core for exactly this. `checkDb` had the same line and backs `/readyz`, so a hostile driver + failure took the readiness probe with it. + +- **A rejecting `drain()` was an unhandled rejection that ended the process mid-drain.** + `installSignalHandlers` observed it with `.then()` alone, and the drain body logged outside any + `try` through an injectable sink — an app `Logger` that throws in `info` left `state` short of + `'stopped'`, made the memo re-reject on every later `drain()`, and stopped `release()` running at + all. The body is now total and the handler is attached on both settle paths. `readinessChecks` + had the identical hole off the drain path, where a throwing logger made `/readyz` throw. + +- **`applyFlagSnapshot` left a snapshot half-applied.** It validated and wrote key by key, so an + invalid targeting on the Nth flag threw with the first N−1 already retargeted and the report + discarded — while the doc block argued the throw protects the fleet. It now validates every + declared key before it writes any. + +- **`rollback({ steps: -1 })` reverted every migration but the last.** `slice(0, steps)` with a + negative value selects from the front; `steps` is now refused unless it is a positive integer, + before the advisory lock is taken. + +- **A migration deleted from the tree was invisible to the ledger audit in `x dev` and CI.** + `auditLedger` only refused an unknown row when its `app_version` differed from the running one, + and `runningAppVersion()` is `dev` for every development build — so drift then reported `ok: true` + against a database that still had the table. The predicate is now membership alone; the version + moved into the cause. + +- **`reapBranches` dropped a branch whose `createdAt` would not parse.** `NaN > cutoff` is `false`, + so an unparseable timestamp read as infinitely old and the database was dropped on the next + sweep regardless of `maxAgeMs`. + +- **`formatMoney` cached one `Intl.NumberFormat` per distinct locale tag, unbounded, keyed on a + request value.** 20,000 tags retained ~55 MB. It now shares core's bounded, canonicalising + formatter cache — the mechanism `@ultimat3/time` already had and `money` could not import. + `formatMoneyDecimal` built a second uncached formatter in the same file and shares it too. + +- **`zoneAbbrev` was the one `Intl` construction in `@ultimat3/time` outside its own cache**, built + fresh per call from the caller's raw zone and locale, and an invalid zone escaped as a bare + `RangeError` where every other entry point raises `X_TIMEZONE_INVALID`. + +- **`coerce` read submitted values off the prototype chain.** `key in record` on a `{}`-literal + record meant a schema field named `toString` or `constructor` read the inherited member as client + input and forwarded a **function** into validation. `Object.hasOwn` throughout, and `toRecord` + builds on `Object.create(null)`. The query-string path was already mitigated upstream by + `@ultimat3/http`; route params and form data were not. + +- **`responsiveImage`'s no-`srcset` fallback took the last width, not the largest** — correct only + because `DEFAULT_WIDTHS` happens to be ascending, so `widths: [1280, 640]` fell back to 640. + +- **`SchemaProvider.introspect`'s doc described an alternative that does not exist**, telling + implementers they could omit it "if the provider also supplies `toJsonSchema`" — a member + `SchemaProvider` does not declare, so following the doc produced `X_SCHEMA_UNSUPPORTED` on every + OpenAPI and MCP projection. + - **SECURITY — `verifyPassword` threw on a stored hash Bun cannot parse, which was an account-enumeration oracle.** `Bun.password.verify` *throws* rather than answering on a hash it cannot read (measured, bun 1.3.14: a Django `pbkdf2_sha256$…` row is `UnsupportedAlgorithm`, a diff --git a/packages/action/src/cache-gate.test.ts b/packages/action/src/cache-gate.test.ts index 54b26f16..c9b7042e 100644 --- a/packages/action/src/cache-gate.test.ts +++ b/packages/action/src/cache-gate.test.ts @@ -91,7 +91,7 @@ describe('the post-commit cache bust', () => { const report = await bustAfterCommit('publishPost', [tag('post')]); - expect(report?.errors).toEqual([{ tier: 'redis', message: 'redis is down' }]); + expect(report?.errors).toEqual([{ tier: 'redis', message: 'Error: redis is down' }]); // The tier that answered still cleared, and the caller still got a report to render. expect(report?.tiers.map((entry) => entry.tier)).toEqual(['lru']); }); diff --git a/packages/cache/CLAUDE.md b/packages/cache/CLAUDE.md index f09373d9..d2b2292a 100644 --- a/packages/cache/CLAUDE.md +++ b/packages/cache/CLAUDE.md @@ -40,6 +40,16 @@ Tier 1. Tagged caching + THE invalidation graph. `TierName` plus `'query-read'` — closed, and deliberately NOT a widening of `TierName`: a name missing from `TIER_ORDER` sorts to `-1`, ahead of the request memo. A label is a log facet; a `TierName` is a position on the ladder. +- **A refusal is rendered with `renderThrowable()`, never `error.message`** — the four sites that + absorb one (`bestEffort`'s log entry, and `fanOut`'s tier, ISR and broadcast catch blocks). A + tier, a revalidator and a broadcast are all app-supplied, so the value they reject with is too: + `instanceof` runs a `Proxy`'s `getPrototypeOf` trap and `String()` runs `Symbol.toPrimitive`, so + building the log line used to raise INSTEAD of absorbing the refusal — on the business write that + triggered the bust, which is the one caller both contracts promise to protect. The code field + keeps its own total probe (`ultimateCode` in `tier-failures.ts`) rather than core's `stringField`: + a driver error's `code` is a SQLSTATE and must never be reported as an `X_*` one. Consequence to + know: a recorded `message` carries the throwable's NAME (`Error: nats is down`, `"just a string"`), + which is what `renderThrowable` renders and what the tests here now pin. - Tier failures go into `report.errors`. A cache tier may never fail a business read or write. `createCacheStack` routes every `get`/`set`/`del` through `bestEffort()` for that reason — a refusal becomes "that tier did not answer" and lands in `recentTierFailures()`, the read side's diff --git a/packages/cache/src/broadcast.test.ts b/packages/cache/src/broadcast.test.ts index b55e2d64..681364b2 100644 --- a/packages/cache/src/broadcast.test.ts +++ b/packages/cache/src/broadcast.test.ts @@ -89,7 +89,9 @@ describe('cross-instance invalidation', () => { const report = await invalidateTags([tag('post')]); - expect(report.errors).toEqual([{ tier: 'broadcast', message: 'nats is down' }]); + // `renderThrowable`'s shape: the renderer that cannot itself throw is the only one a catch + // block absorbing an app-supplied refusal may use, and it carries the name. + expect(report.errors).toEqual([{ tier: 'broadcast', message: 'Error: nats is down' }]); // The local tiers still cleared: a partial bust, honestly reported. expect(report.tiers.map((entry) => entry.tier)).toEqual(['lru']); expect(await lru.get('feed')).toBeUndefined(); diff --git a/packages/cache/src/invalidate.test.ts b/packages/cache/src/invalidate.test.ts index f433246c..0fefa8f7 100644 --- a/packages/cache/src/invalidate.test.ts +++ b/packages/cache/src/invalidate.test.ts @@ -10,6 +10,7 @@ import { isolateTiers, recentInvalidations, registeredTiers, + registerInvalidationBroadcast, registerRevalidator, registerTier, resetTiers, @@ -219,6 +220,60 @@ describe('invalidateTags fan-out', () => { expect([...revalidated].sort()).toEqual(['/blog', '/blog/hello']); }); + /** + * "Never throws for a tier failure" is the contract at the top of `invalidateTags`, and it held + * only for refusals the framework itself built: a tier, a revalidator and a broadcast are all + * app-supplied, so the value they reject with is app-supplied too. `instanceof` runs a `Proxy`'s + * `getPrototypeOf` trap and `String()` runs `Symbol.toPrimitive`, so rendering the refusal for + * `report.errors` used to raise INSTEAD of the refusal — and the raise lands on the write that + * triggered the bust, which is the one caller this whole path exists to protect. + */ + describe('a refusal that fights being rendered still lands in report.errors', () => { + const trapped = (): unknown => + new Proxy( + {}, + { + getPrototypeOf() { + throw new TypeError('proxy trap'); + }, + }, + ); + + test('a tier rejecting with a hostile throwable', async () => { + registerTier({ + name: 'redis', + get: () => Promise.resolve(undefined), + set: () => Promise.resolve(), + del: () => Promise.resolve(), + invalidateTags: () => Promise.reject(trapped()), + }); + + const report = await invalidateTags([tag('post')]); + + expect(report.errors.map((entry) => entry.tier)).toEqual(['redis']); + expect(typeof report.errors[0]?.message).toBe('string'); + }); + + test('a revalidator rejecting with a hostile throwable', async () => { + registerDependent([tag('post')], { kind: 'isr-route', id: '/blog' }); + registerRevalidator(() => Promise.reject(Object.create(null) as unknown)); + + const report = await invalidateTags([tag('post')]); + + expect(report.errors.map((entry) => entry.tier)).toEqual(['isr']); + expect(report.isr).toEqual(['/blog']); + }); + + test('a broadcast rejecting with a hostile throwable', async () => { + registerInvalidationBroadcast(() => Promise.reject(trapped())); + + const report = await invalidateTags([tag('post')]); + + expect(report.errors.map((entry) => entry.tier)).toEqual(['broadcast']); + expect(typeof report.errors[0]?.message).toBe('string'); + }); + }); + test('an undeclared tag fails loudly once entities have been declared', async () => { declareTags(['post', 'user']); await expect(invalidateWireTags(['pots'])).rejects.toThrow(CacheTagUnknownError); @@ -415,7 +470,7 @@ describe('the suite baseline this file hands back', () => { restore(); expect(recentInvalidations().map((event) => event.tags)).toEqual([['post']]); - expect(recentTierFailures()[0]?.message).toBe('neighbour boom'); + expect(recentTierFailures()[0]?.message).toBe('Error: neighbour boom'); // The revalidator has no reader anywhere, so the only proof it is back is that it runs. revalidated.length = 0; diff --git a/packages/cache/src/invalidate.ts b/packages/cache/src/invalidate.ts index 42a05fc1..1d84dcba 100644 --- a/packages/cache/src/invalidate.ts +++ b/packages/cache/src/invalidate.ts @@ -4,7 +4,7 @@ // the returned report is what the `/_x` cache panel renders, so "did it actually clear?" is // answerable without a log dive. -import { currentSpan, logger, systemClock, withSpan } from '@ultimat3/core'; +import { currentSpan, logger, renderThrowable, systemClock, withSpan } from '@ultimat3/core'; import { markInvalidated } from './fence'; import { dependentsOfKind } from './graph'; import type { CacheTag } from './tags'; @@ -222,10 +222,11 @@ function fanOut(tags: readonly CacheTag[], options: FanOutOptions): Promise { tier: 'query-read', op: 'get', key: 'cache:posts', - message: 'read cache is down', + // `renderThrowable`'s shape, not `error.message`: the name is carried because the renderer + // that cannot throw is the only one a catch block may use. + message: 'Error: read cache is down', }); }); @@ -74,7 +76,7 @@ describe('bestEffort', () => { }); expect(answer).toBeUndefined(); - expect(recentTierFailures()[0]?.message).toBe('sync boom'); + expect(recentTierFailures()[0]?.message).toBe('Error: sync boom'); }); test('records tier, op, key, message and an ISO timestamp', async () => { @@ -84,7 +86,7 @@ describe('bestEffort', () => { expect(failure?.tier).toBe('redis'); expect(failure?.op).toBe('del'); expect(failure?.key).toBe('feed:org-1'); - expect(failure?.message).toBe('no socket'); + expect(failure?.message).toBe('Error: no socket'); expect(failure?.at).toMatch(/^\d{4}-\d{2}-\d{2}T/); }); @@ -106,10 +108,12 @@ describe('bestEffort', () => { expect(Object.hasOwn(failure ?? {}, 'code')).toBe(false); }); - test('stringifies a thrown non-Error rather than losing it', async () => { + test('renders a thrown non-Error rather than losing it', async () => { await bestEffort('redis', 'get', 'k', () => Promise.reject('just a string')); - expect(recentTierFailures()[0]?.message).toBe('just a string'); + // Quoted, because `renderCauseValue` is the one renderer that cannot throw on an arbitrary + // value, and a quoted string is what distinguishes a thrown `'null'` from a thrown `null`. + expect(recentTierFailures()[0]?.message).toBe('"just a string"'); }); }); @@ -165,3 +169,45 @@ describe('isolateTierFailures', () => { expect(recentTierFailures().map((failure) => failure.key)).toEqual(['neighbour']); }); }); + +/** + * The three sites `record()` reads a caught value at — `instanceof UltimateError`, `instanceof + * Error`, `String(error)` — are all *calls* on a value this package did not build, and each one + * can throw. A `bestEffort` that dies rendering the refusal it was absorbing replaces "that tier + * did not answer" with a `TypeError` on the caller's business read, which is the one thing this + * function exists to prevent. + */ +describe('bestEffort absorbs a throwable that fights being read', () => { + /** `instanceof` runs this trap, so both `instanceof` probes throw before any renderer runs. */ + const trapped = (): unknown => + new Proxy( + {}, + { + getPrototypeOf() { + throw new TypeError('proxy trap'); + }, + }, + ); + + test('a Proxy whose getPrototypeOf throws is still a recorded failure, not a rejection', async () => { + const answer = await bestEffort('lru', 'get', 'k', () => Promise.reject(trapped())); + + expect(answer).toBeUndefined(); + const [failure] = recentTierFailures(); + expect(failure?.tier).toBe('lru'); + expect(failure?.key).toBe('k'); + expect(typeof failure?.message).toBe('string'); + // Nothing claimed a code: the probe answered "not an UltimateError" instead of throwing. + expect(Object.hasOwn(failure ?? {}, 'code')).toBe(false); + }); + + test('a null-prototype object, which String() refuses to convert, is absorbed too', async () => { + const answer = await bestEffort('redis', 'set', 'k', () => + Promise.reject(Object.create(null) as unknown), + ); + + expect(answer).toBeUndefined(); + expect(recentTierFailures()).toHaveLength(1); + expect(typeof recentTierFailures()[0]?.message).toBe('string'); + }); +}); diff --git a/packages/cache/src/tier-failures.ts b/packages/cache/src/tier-failures.ts index e47a7b2e..4284e76e 100644 --- a/packages/cache/src/tier-failures.ts +++ b/packages/cache/src/tier-failures.ts @@ -3,7 +3,7 @@ // to return, so every swallowed refusal lands in one bounded log plus one `warn` — a stack // running degraded stays answerable instead of merely looking slow. -import { logger, systemClock, UltimateError } from '@ultimat3/core'; +import { logger, renderThrowable, systemClock, UltimateError } from '@ultimat3/core'; import type { TierLabel } from './tiers'; /** The three tier calls a stack makes on the value path. `invalidateTags` reports its own. */ @@ -77,14 +77,35 @@ export async function bestEffort( } } +/** + * The `X_*` code when the tier threw an `UltimateError`, and `undefined` for every other answer — + * "the probe itself threw" included. `instanceof` RUNS a `Proxy`'s `getPrototypeOf` trap and the + * read past it is a getter call, both on a value this package did not build; the one place the + * question is asked is the catch block absorbing a refusal, which has nothing left to answer with + * if asking it raises. Core's `isThrownError` is this guard for `Error` and `stringField` is it for + * a loose field — neither fits here, because a driver error's `code` is a SQLSTATE and must never + * be reported as an `X_*` one. + */ +function ultimateCode(error: unknown): string | undefined { + try { + if (!(error instanceof UltimateError)) return undefined; + return typeof error.code === 'string' ? error.code : undefined; + } catch { + return undefined; + } +} + function record(tier: TierLabel, op: TierOperation, key: string, error: unknown): void { + const code = ultimateCode(error); const failure: TierFailure = { at: systemClock.now().toISOString(), tier, op, key, - ...(error instanceof UltimateError ? { code: error.code } : {}), - message: error instanceof Error ? error.message : String(error), + ...(code === undefined ? {} : { code }), + // Never `error.message`: a rendering that throws replaces the absorbed refusal with a + // `TypeError` on the business read this function exists to keep alive. + message: renderThrowable(error), }; failureLog.unshift(failure); failureLog.length = Math.min(failureLog.length, MAX_TIER_FAILURES); diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 059fc647..188e8b8f 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -62,6 +62,7 @@ shape against a locally declared sample interface for exactly that reason. | `.env.example` | `env-example.ts` | a projection of the schema, never hand-maintained | | loading `.env` | **Bun**, not us | `envFileCandidates()` documents the measured order; there is no `.env.staging` | | a value that must not be printed | `secret.ts` | redacted by VALUE; `revealSecret()` is the one way out, on purpose greppable | +| an `Intl` formatter cache | `intl-cache.ts` (`cachedFormatter`, `canonicalLocale`, `MAX_CACHED_FORMATTERS`) | a locale and a zone arrive from a request header, so the key must be canonical AND the cache bounded — never a second copy of either half | | the committed encrypted values | `secrets.ts` (envelope) + `secrets-store.ts` (files, `installSecrets`) | plaintext is a flat map of ENV NAMES; there is no `secrets.get()` | `installSecrets()` is the ONLY path from `secrets.enc.json` to an app value, and it lands in @@ -70,6 +71,16 @@ shape against a locally declared sample interface for exactly that reason. implementations. The real environment always wins, which is what lets one image run in Compose and on K8s off one committed file. +`intl-cache.ts` is tier 0 because two tier-1 packages need it and tier 1 may not import sideways. +It was `@ultimat3/time`'s, internal, until 2.0.0, when `@ultimat3/money`'s `formatMoney` was found +keyed raw on the caller's locale into an unbounded `Map` — 20,000 valid `en-US-x-*` tags from one +`Accept-Language` header retained +55.1 MB of RSS (measured `As of 2026-08`). Copying the FIFO into +`money` would have been a second answer to one question (axiom 1); `money → time` is a sideways +import `bun run boundaries` refuses. The bound and the canonical key are **two halves of one rule** +and live in one file for that reason: a canonical key bounds nothing (an unknown `-u-` extension +value survives canonicalization as a distinct string) and the cap alone lets one locale evict +itself under three spellings. Never build an `Intl` formatter on a caller string without both. + `secrets-errors.ts` registers its seven codes through `registerErrorCodes()` rather than joining `CORE_CODE_TITLES` — the codes and the module that throws them ship together, and `registerErrorCodes` is the one mechanism that raises `X_ERROR_CODE_DUPLICATE` if anything else claims one. Consequence diff --git a/packages/core/README.md b/packages/core/README.md index ebed7a9e..dc32fed1 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -433,6 +433,34 @@ never a silently wrong page. | `usesDevCursorSecret()` | true while the shipped dev key is in use | | `resetCursorSigning()` | test seam: forget `configureCursorSigning` and fall back to the environment | +## One bounded cache for every `Intl` formatter + +```ts +import { cachedFormatter, canonicalLocale } from '@ultimat3/core'; + +const cache = new Map(); + +export function euroFormatter(locale: string): Intl.NumberFormat { + // `EN-us` and `en-latn-us` collapse to one key, so one locale cannot evict itself. + const tag = canonicalLocale(locale) ?? locale; + return cachedFormatter( + cache, + `${tag}|EUR`, + () => new Intl.NumberFormat(tag, { style: 'currency', currency: 'EUR' }), + ); +} +``` + +A locale arrives from `Accept-Language` and a zone from `x-timezone`, so an unbounded `Map` keyed +on that string is **memory the client chooses**. Measured `As of 2026-08`: 4,096 casings of one +zone name retained 31 MB, and 20,000 valid `en-US-x-*` tags through `formatMoney` retained 55.1 MB. +The bound +(`MAX_CACHED_FORMATTERS`, 512, FIFO) and the canonical key are two halves of one rule and neither +is sufficient alone — an unknown `-u-` extension value survives canonicalization as a distinct +string, and the cap alone lets one locale evict itself under three spellings. A miss costs one +`Intl` construction, never a wrong answer, which is what makes the bound safe. It lives here rather +than in `@ultimat3/time` because `@ultimat3/money` needs it too and tier 1 may not import sideways. + ## One image pipeline, everywhere ```ts diff --git a/packages/core/src/error-codes.ts b/packages/core/src/error-codes.ts index 6f1d31be..0cf383ed 100644 --- a/packages/core/src/error-codes.ts +++ b/packages/core/src/error-codes.ts @@ -46,7 +46,8 @@ const CORE_CODE_TITLES = { X_INVARIANT: 'invariant violated', X_METRIC_CARDINALITY: 'a metric exceeded its series ceiling and is folding into one overflow series', - X_METRIC_NAME_INVALID: 'metric name is malformed or already declared with another kind', + X_METRIC_NAME_INVALID: + 'metric name is malformed, or redeclared with a different kind, bounds or observer', X_METRIC_VALUE_INVALID: 'metric value is not recordable', X_NO_CONTEXT: 'no request context is active', X_NOT_IMPLEMENTED: 'this driver does not implement the requested feature', diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e6a2b91c..a2824aa2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -428,6 +428,7 @@ export { export type { ImageFit, ResizeSpec } from './image/resize'; export { fitBox, resizeRaster, scaledToFit } from './image/resize'; export { impersonate, impersonationReason, isImpersonating } from './impersonate'; +export { cachedFormatter, canonicalLocale, MAX_CACHED_FORMATTERS } from './intl-cache'; export type { HealthPayload, HealthReport, diff --git a/packages/core/src/intl-cache.test.ts b/packages/core/src/intl-cache.test.ts new file mode 100644 index 00000000..b5e0f00e --- /dev/null +++ b/packages/core/src/intl-cache.test.ts @@ -0,0 +1,80 @@ +// The one bounded formatter cache and the one canonical key it is keyed on — two halves of one +// rule. It must reuse on the second ask, evict oldest-first at the cap, and collapse every +// spelling of a locale, because the keys are locales and zones a request header chooses. + +import { describe, expect, test } from 'bun:test'; +import { cachedFormatter, canonicalLocale, MAX_CACHED_FORMATTERS } from './intl-cache'; + +describe('cachedFormatter', () => { + test('answers from the cache on the second ask', () => { + const cache = new Map(); + let built = 0; + const build = (): number => { + built += 1; + return built; + }; + expect(cachedFormatter(cache, 'en', build)).toBe(1); + expect(cachedFormatter(cache, 'en', build)).toBe(1); + expect(built).toBe(1); + }); + + test('a stored value is a hit even when it is `undefined` — membership, not truthiness', () => { + // Latent, not live: every shipped caller stores an `Intl.*` formatter. This pins the generic + // contract `T` advertises — reading a hit off `get(key) !== undefined` makes a cache + // instantiated with a nullable `T` rebuild on every single call, silently. + const cache = new Map(); + let built = 0; + const build = (): number | undefined => { + built += 1; + return undefined; + }; + expect(cachedFormatter(cache, 'en', build)).toBe(undefined); + expect(cachedFormatter(cache, 'en', build)).toBe(undefined); + expect(built).toBe(1); + }); + + test('evicts oldest-first at the cap, so a header cannot mint entries forever', () => { + // The whole point: a locale or a zone arrives from a request header, and an unbounded Map + // keyed on that string is memory the client chooses. + const cache = new Map(); + let built = 0; + const build = (): number => { + built += 1; + return built; + }; + for (let index = 0; index <= MAX_CACHED_FORMATTERS; index += 1) { + cachedFormatter(cache, `key-${index}`, build); + } + expect(cache.size).toBe(MAX_CACHED_FORMATTERS); + expect(built).toBe(MAX_CACHED_FORMATTERS + 1); + // FIFO: the first key is the one that went. + expect(cache.has('key-0')).toBe(false); + expect(cache.has(`key-${MAX_CACHED_FORMATTERS}`)).toBe(true); + cachedFormatter(cache, 'key-0', build); + expect(built).toBe(MAX_CACHED_FORMATTERS + 2); + }); +}); + +describe('canonicalLocale', () => { + test('every spelling of one locale collapses to one key', () => { + expect(canonicalLocale('EN-us')).toBe('en-US'); + expect(canonicalLocale('en-US')).toBe('en-US'); + expect(canonicalLocale('en-latn-us')).toBe('en-Latn-US'); + expect(canonicalLocale('DE')).toBe('de'); + // Casing inside a `-u-` extension collapses too; the *values* still do not, which is why + // `intl-cache.ts` keeps its bound as well as this key. + expect(canonicalLocale('de-DE-u-ca-Gregory')).toBe('de-DE-u-ca-gregory'); + }); + + test('a tag Intl cannot parse is undefined, never a silent passthrough', () => { + expect(canonicalLocale('en_US')).toBe(undefined); + expect(canonicalLocale('')).toBe(undefined); + expect(canonicalLocale('not a locale')).toBe(undefined); + }); + + test('well-formed but unknown to ICU is still a locale', () => { + // `Intl` falls back for `zz`; refusing it here would be stricter than the formatters this + // feeds, and would turn a fallback into an error for a tag that renders fine. + expect(canonicalLocale('zz')).toBe('zz'); + }); +}); diff --git a/packages/core/src/intl-cache.ts b/packages/core/src/intl-cache.ts new file mode 100644 index 00000000..b0ffa9c5 --- /dev/null +++ b/packages/core/src/intl-cache.ts @@ -0,0 +1,43 @@ +// One bounded cache, on one canonical key, for every `Intl` formatter the framework builds. +// A locale and a zone both arrive from a request header, so an unbounded `Map` keyed on the +// caller's spelling is memory the client chooses — 31 MB and 55.1 MB, measured `As of 2026-08` and +// written up in the README. The bound and the canonical key are two halves of ONE rule. + +/** + * Above the full canonical IANA set (445 zones as of tzdata 2025) so a correct app never evicts, + * and small enough that the worst case is a few megabytes rather than a leak. A miss costs one + * `Intl` construction, never a wrong answer — which is what makes a bound safe here at all. + */ +export const MAX_CACHED_FORMATTERS = 512; + +/** FIFO — a `Map` iterates in insertion order, so the first key inserted is the first evicted. */ +export function cachedFormatter(cache: Map, key: string, build: () => T): T { + // Membership decides, never truthiness: `T` is the caller's, so a stored `undefined` is a hit. + // The cast is sound because `has` just proved the key is present, which `get`'s signature cannot. + if (cache.has(key)) return cache.get(key) as T; + const formatter = build(); + if (cache.size >= MAX_CACHED_FORMATTERS) { + const oldest = cache.keys().next().value; + if (oldest !== undefined) cache.delete(oldest); + } + cache.set(key, formatter); + return formatter; +} + +/** + * The canonical BCP 47 spelling, or `undefined` when the tag is not structurally valid at all + * (`en_US`, `''`, `not a locale`). Well-formed but unknown to ICU (`zz`) is a locale — `Intl` + * falls back for it, and refusing here would be stricter than the formatters this feeds. + * + * Deliberately **not** memoised: this is string work, and a `Map` keyed on a header value is the + * unbounded cache the bound above exists to prevent. + */ +export function canonicalLocale(locale: string): string | undefined { + try { + // `getCanonicalLocales` runs the same IsStructurallyValidLanguageTag check that + // `supportedLocalesOf` throws on, and unlike it, hands back the canonical spelling. + return Intl.getCanonicalLocales(locale)[0]; + } catch { + return undefined; + } +} diff --git a/packages/core/src/lifecycle-deadline.test.ts b/packages/core/src/lifecycle-deadline.test.ts new file mode 100644 index 00000000..a2fa8bea --- /dev/null +++ b/packages/core/src/lifecycle-deadline.test.ts @@ -0,0 +1,190 @@ +// Single responsibility: the drain's time budget — one deadline for the WHOLE drain, a hook that +// overruns it abandoned rather than awaited, and the default that is enforced when no role +// declares one. The unit under it is `lifecycle-deadline.ts`; split from `lifecycle.test.ts` +// for the file-size ceiling. + +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { frozenClock, systemClock } from './clock'; +import { + configureLifecycle, + drain, + drainDeadlineMs, + lifecycleState, + markReady, + onShutdown, + resetLifecycle, +} from './lifecycle'; +import { createLogger } from './logger'; + +// Lifecycle state is process-global, and any suite that boots a server calls `markReady()` — so +// this resets on the way IN as well as out, or the first assertion reads another file's process. +beforeEach(() => { + resetLifecycle(); +}); + +afterEach(() => { + resetLifecycle(); +}); + +/** + * A promise a test resolves by hand. Races here are driven by these and never by a sleep: a + * shutdown-deadline assertion ordered on wall-clock time is exactly the shard that flakes. + */ +function deferred(): { readonly promise: Promise; readonly resolve: () => void } { + let resolve!: () => void; + const promise = new Promise((settle) => { + resolve = settle; + }); + return { promise, resolve }; +} + +describe('the drain deadline', () => { + test('a declared deadline bounds a slow hook: the drain abandons it and names it', async () => { + const lines: string[] = []; + const stuck = deferred(); + configureLifecycle({ + deadlineMs: 10, + logger: createLogger({ level: 'info', writer: (line) => lines.push(line) }), + }); + markReady(); + expect(drainDeadlineMs()).toBe(10); + + // A `worker` pod's real shape: `jobs`' hook awaits every in-flight job and then `driver.close()`. + // Nothing in it reads `reason.deadlineAt`, so before this the 10ms budget bounded nothing at + // all — `drain()` sat here until the kubelet SIGKILLed the process mid-job. + onShutdown('slow-accept', () => stuck.promise, { phase: 'accept' }); + let closed = 0; + onShutdown('close-db', () => { + closed += 1; + }); + + await drain('SIGTERM'); + + expect(lifecycleState()).toBe('stopped'); + // The code alone is not an instruction: an operator has to know WHICH hook to shorten. + const timeout = lines.find((line) => line.includes('X_SHUTDOWN_TIMEOUT')); + expect(timeout).toContain('slow-accept'); + expect(timeout).toContain('accept'); + // Abandoned, not merely logged — the phases after it still ran, which is the whole point of + // resolving: `installSignalHandlers` reaches `process.exit(0)` instead of being killed. + expect(closed).toBe(1); + + stuck.resolve(); + }); + + test('a hook abandoned at the deadline cannot crash the process when it later rejects', async () => { + const lines: string[] = []; + const stuck = deferred(); + configureLifecycle({ + deadlineMs: 10, + logger: createLogger({ level: 'info', writer: (line) => lines.push(line) }), + }); + let rejectLate!: (error: unknown) => void; + const late = new Promise((_resolve, reject) => { + rejectLate = reject; + }); + onShutdown('slow-accept', () => late, { phase: 'accept' }); + + await drain('SIGTERM'); + // The drain has moved on and nobody awaits this promise anymore. Unhandled, it would take + // down the process the drain exists to end cleanly. + rejectLate(new Error('closed after abandonment')); + stuck.resolve(); + await stuck.promise; + + expect(lifecycleState()).toBe('stopped'); + }); + + test('the budget bounds the WHOLE drain — a hook that spends it leaves none for the ones behind', async () => { + const lines: string[] = []; + const stuck = deferred(); + configureLifecycle({ + deadlineMs: 30, + logger: createLogger({ level: 'info', writer: (line) => lines.push(line) }), + }); + + onShutdown('spends-it', () => stuck.promise, { phase: 'accept' }); + // Deterministic, and not a stopwatch: both waits are timers in one queue, so they settle in + // due-time order however slow the machine is. Whole-drain, this hook's budget is already 0 and + // its own 15ms timer cannot beat it; per-hook, it would get a fresh 30ms and finish. + let finished = false; + onShutdown( + 'after-it', + () => + new Promise((resolve) => { + setTimeout(() => { + finished = true; + resolve(); + }, 15); + }), + { phase: 'close' }, + ); + + await drain('SIGTERM'); + + const overran = lines.filter((line) => line.includes('X_SHUTDOWN_TIMEOUT')); + expect(overran).toHaveLength(2); + expect(overran[1]).toContain('after-it'); + expect(finished).toBe(false); + + stuck.resolve(); + }); + + // The default is ENFORCED, not absent: `jobs`, `realtime` and `cli` declare no budget, and a + // deadline that bounded only the packages that happened to ask would be a mechanism claiming + // more than it enforces — the worker pod the finding proved would still be SIGKILLed. + test('an unset deadline is the DEFAULT budget, enforced — not the absence of one', async () => { + const order: string[] = []; + const entered = deferred(); + const release = deferred(); + configureLifecycle({ + logger: createLogger({ level: 'info', writer: () => undefined }), + }); + markReady(); + // 25s, the literal, because no stopwatch in a test can tell 25s from unbounded — so the value + // is pinned where it is decided, and `remainingBudget` has no second place to disagree from. + expect(drainDeadlineMs()).toBe(25_000); + onShutdown( + 'slow-accept', + async () => { + entered.resolve(); + await release.promise; + order.push('hook'); + }, + { phase: 'accept' }, + ); + + const drained = drain('SIGTERM').then(() => { + order.push('drained'); + }); + await entered.promise; + // Not an ordering assertion: a hook well inside the budget must be awaited to completion, so + // waiting longer only strengthens this. 30ms against a 25s budget is what makes a default that + // shrank — to 0, to a per-phase slice, to whatever a refactor thought "no budget" meant — show + // up here as an abandoned hook rather than as a green test. + await Bun.sleep(30); + expect(order).toEqual([]); + + release.resolve(); + await drained; + expect(order).toEqual(['hook', 'drained']); + }); + + test('the budget is REAL elapsed time — a frozen clock cannot extend a grace period', async () => { + const clock = frozenClock(0); + configureLifecycle({ deadlineMs: 5_000, clock }); + clock.advance(1_000_000); + let seen: number | undefined; + onShutdown('probe', (reason) => { + seen = reason.deadlineAt; + }); + + await drain('SIGTERM'); + + // `waitForIdle` sleeps on a real `setTimeout` while the budget was read off the injected + // clock, so the two disagreed: here the old arithmetic answered 1,005,000 — a 16-minute + // budget, on a clock a test controls, for a deadline the kubelet enforces in real seconds. + expect(seen).toBeLessThan(1_000_000); + expect(seen).toBeGreaterThan(systemClock.monotonic()); + }); +}); diff --git a/packages/core/src/lifecycle-logging.test.ts b/packages/core/src/lifecycle-logging.test.ts new file mode 100644 index 00000000..b16881df --- /dev/null +++ b/packages/core/src/lifecycle-logging.test.ts @@ -0,0 +1,86 @@ +// Single responsibility: an injected logger that THROWS cannot break the two answers that must +// arrive anyway — `drain()` still settles, /readyz still reports. `configureLifecycle({ logger })` +// takes whatever an app hands it, so every log call on those paths is an injection seam. Split +// from `lifecycle.test.ts` for the file-size ceiling. + +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { + configureLifecycle, + drain, + lifecycleState, + markReady, + onShutdown, + readyzPayload, + registerReadinessCheck, + resetLifecycle, +} from './lifecycle'; +import { createLogger } from './logger'; + +// Lifecycle state is process-global, and any suite that boots a server calls `markReady()` — so +// this resets on the way IN as well as out, or the first assertion reads another file's process. +beforeEach(() => { + resetLifecycle(); +}); + +afterEach(() => { + resetLifecycle(); +}); + +describe('the logger is an injection seam', () => { + test('a logger that throws cannot reject the drain', async () => { + // `log` is an injection seam — `configureLifecycle({ logger })` takes whatever an app hands + // it. `drain()`'s body logged outside any `try`, so a `Logger.info` that throws rejected + // `drainPromise`: `state` never reached 'stopped', the memo re-rejected for every later + // caller (`holdUntilShutdown`'s `await drain()` among them), and on Bun the unhandled + // rejection ends the process mid-drain, with the pool still open. + const fallback: string[] = []; + const base = createLogger({ level: 'info', writer: (line) => fallback.push(line) }); + configureLifecycle({ + logger: { + ...base, + info(): never { + throw new Error('the log sink is down'); + }, + }, + }); + let closed = 0; + onShutdown('good', () => { + closed += 1; + }); + + let settled = false; + await drain('SIGTERM').then(() => { + settled = true; + }); + + expect(settled).toBe(true); + expect(closed).toBe(1); + expect(lifecycleState()).toBe('stopped'); + // The memo is a resolved one, so the next caller joins a finished drain rather than a + // rejection nobody is left to handle. + await expect(drain('SIGTERM')).resolves.toBeUndefined(); + }); + + test('a logger that throws cannot break /readyz either', async () => { + // Same seam, same shape: a check that fails is reported through `log.warn`, so a logger that + // dies there replaced the readiness answer with a throw — the probe 500s and the pod is + // killed by the outage it was reporting on. + const fallback: string[] = []; + const base = createLogger({ level: 'warn', writer: (line) => fallback.push(line) }); + configureLifecycle({ + logger: { + ...base, + warn(): never { + throw new Error('the log sink is down'); + }, + }, + }); + markReady(); + registerReadinessCheck('db', () => { + throw new Error('pool is closed'); + }); + + expect(readyzPayload().status).toBe(503); + expect(readyzPayload().body.checks['db']).toBe('failing'); + }); +}); diff --git a/packages/core/src/lifecycle.test.ts b/packages/core/src/lifecycle.test.ts index 8967733e..1cd0f31d 100644 --- a/packages/core/src/lifecycle.test.ts +++ b/packages/core/src/lifecycle.test.ts @@ -1,11 +1,14 @@ +// Single responsibility: the lifecycle state machine — starting -> ready -> draining -> stopped, +// the shutdown hooks each phase runs, and the readiness checks /readyz reports. The drain's time +// budget is `lifecycle-deadline.test.ts`; a logger that throws on the way is +// `lifecycle-logging.test.ts`. + import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; -import { frozenClock, systemClock } from './clock'; import { UltimateError } from './errors'; import { beginWork, configureLifecycle, drain, - drainDeadlineMs, healthzPayload, idleWaiterCount, inflightCount, @@ -204,157 +207,6 @@ describe('lifecycle', () => { }); }); -describe('the drain deadline', () => { - test('a declared deadline bounds a slow hook: the drain abandons it and names it', async () => { - const lines: string[] = []; - const stuck = deferred(); - configureLifecycle({ - deadlineMs: 10, - logger: createLogger({ level: 'info', writer: (line) => lines.push(line) }), - }); - markReady(); - expect(drainDeadlineMs()).toBe(10); - - // A `worker` pod's real shape: `jobs`' hook awaits every in-flight job and then `driver.close()`. - // Nothing in it reads `reason.deadlineAt`, so before this the 10ms budget bounded nothing at - // all — `drain()` sat here until the kubelet SIGKILLed the process mid-job. - onShutdown('slow-accept', () => stuck.promise, { phase: 'accept' }); - let closed = 0; - onShutdown('close-db', () => { - closed += 1; - }); - - await drain('SIGTERM'); - - expect(lifecycleState()).toBe('stopped'); - // The code alone is not an instruction: an operator has to know WHICH hook to shorten. - const timeout = lines.find((line) => line.includes('X_SHUTDOWN_TIMEOUT')); - expect(timeout).toContain('slow-accept'); - expect(timeout).toContain('accept'); - // Abandoned, not merely logged — the phases after it still ran, which is the whole point of - // resolving: `installSignalHandlers` reaches `process.exit(0)` instead of being killed. - expect(closed).toBe(1); - - stuck.resolve(); - }); - - test('a hook abandoned at the deadline cannot crash the process when it later rejects', async () => { - const lines: string[] = []; - const stuck = deferred(); - configureLifecycle({ - deadlineMs: 10, - logger: createLogger({ level: 'info', writer: (line) => lines.push(line) }), - }); - let rejectLate!: (error: unknown) => void; - const late = new Promise((_resolve, reject) => { - rejectLate = reject; - }); - onShutdown('slow-accept', () => late, { phase: 'accept' }); - - await drain('SIGTERM'); - // The drain has moved on and nobody awaits this promise anymore. Unhandled, it would take - // down the process the drain exists to end cleanly. - rejectLate(new Error('closed after abandonment')); - stuck.resolve(); - await stuck.promise; - - expect(lifecycleState()).toBe('stopped'); - }); - - test('the budget bounds the WHOLE drain — a hook that spends it leaves none for the ones behind', async () => { - const lines: string[] = []; - const stuck = deferred(); - configureLifecycle({ - deadlineMs: 30, - logger: createLogger({ level: 'info', writer: (line) => lines.push(line) }), - }); - - onShutdown('spends-it', () => stuck.promise, { phase: 'accept' }); - // Deterministic, and not a stopwatch: both waits are timers in one queue, so they settle in - // due-time order however slow the machine is. Whole-drain, this hook's budget is already 0 and - // its own 15ms timer cannot beat it; per-hook, it would get a fresh 30ms and finish. - let finished = false; - onShutdown( - 'after-it', - () => - new Promise((resolve) => { - setTimeout(() => { - finished = true; - resolve(); - }, 15); - }), - { phase: 'close' }, - ); - - await drain('SIGTERM'); - - const overran = lines.filter((line) => line.includes('X_SHUTDOWN_TIMEOUT')); - expect(overran).toHaveLength(2); - expect(overran[1]).toContain('after-it'); - expect(finished).toBe(false); - - stuck.resolve(); - }); - - // The default is ENFORCED, not absent: `jobs`, `realtime` and `cli` declare no budget, and a - // deadline that bounded only the packages that happened to ask would be a mechanism claiming - // more than it enforces — the worker pod the finding proved would still be SIGKILLed. - test('an unset deadline is the DEFAULT budget, enforced — not the absence of one', async () => { - const order: string[] = []; - const entered = deferred(); - const release = deferred(); - configureLifecycle({ - logger: createLogger({ level: 'info', writer: () => undefined }), - }); - markReady(); - // 25s, the literal, because no stopwatch in a test can tell 25s from unbounded — so the value - // is pinned where it is decided, and `remainingBudget` has no second place to disagree from. - expect(drainDeadlineMs()).toBe(25_000); - onShutdown( - 'slow-accept', - async () => { - entered.resolve(); - await release.promise; - order.push('hook'); - }, - { phase: 'accept' }, - ); - - const drained = drain('SIGTERM').then(() => { - order.push('drained'); - }); - await entered.promise; - // Not an ordering assertion: a hook well inside the budget must be awaited to completion, so - // waiting longer only strengthens this. 30ms against a 25s budget is what makes a default that - // shrank — to 0, to a per-phase slice, to whatever a refactor thought "no budget" meant — show - // up here as an abandoned hook rather than as a green test. - await Bun.sleep(30); - expect(order).toEqual([]); - - release.resolve(); - await drained; - expect(order).toEqual(['hook', 'drained']); - }); - - test('the budget is REAL elapsed time — a frozen clock cannot extend a grace period', async () => { - const clock = frozenClock(0); - configureLifecycle({ deadlineMs: 5_000, clock }); - clock.advance(1_000_000); - let seen: number | undefined; - onShutdown('probe', (reason) => { - seen = reason.deadlineAt; - }); - - await drain('SIGTERM'); - - // `waitForIdle` sleeps on a real `setTimeout` while the budget was read off the injected - // clock, so the two disagreed: here the old arithmetic answered 1,005,000 — a 16-minute - // budget, on a clock a test controls, for a deadline the kubelet enforces in real seconds. - expect(seen).toBeLessThan(1_000_000); - expect(seen).toBeGreaterThan(systemClock.monotonic()); - }); -}); - /** * One process, one lifecycle. These pin the half of that rule the deadline work did not touch: * `drain()` memoizes and `state` never leaves `stopped`, so the SECOND thing in a process to call diff --git a/packages/core/src/lifecycle.ts b/packages/core/src/lifecycle.ts index 4139081c..443a23cb 100644 --- a/packages/core/src/lifecycle.ts +++ b/packages/core/src/lifecycle.ts @@ -6,7 +6,7 @@ import { type Clock, systemClock } from './clock'; import { UltimateError } from './errors'; import { settleWithin } from './lifecycle-deadline'; import { lifecycleDrained } from './lifecycle-errors'; -import { type Logger, logger as rootLogger } from './logger'; +import { type LogFields, type Logger, logger as rootLogger } from './logger'; export type HealthState = 'starting' | 'ready' | 'draining' | 'stopped'; @@ -165,6 +165,33 @@ export function readinessCheckCount(): number { return readiness.size; } +/** + * Every line this file emits, and the only way it emits one. `log` is an injection seam + * (`configureLifecycle({ logger })`), so an app's `Logger` decides whether a log call can throw — + * and a throw here does not lose a line, it replaces the event. Inside `drain()` it rejected + * `drainPromise`: `state` never reached 'stopped', the memo re-rejected for every later caller, + * and on Bun the unhandled rejection ended the process the drain was trying to end cleanly. + * Inside `readinessChecks()` it replaced the probe's answer with a throw. + * + * A lifecycle that cannot report is still a lifecycle: the line falls back to core's own + * `rootLogger`, which is total by construction (`logger.ts`), and failing that is dropped. + */ +function report(level: 'info' | 'warn' | 'error', message: string, fields: LogFields): void { + try { + log[level](message, fields); + return; + } catch { + // Fall through — the injected sink is gone, and the fallback below is the last one there is. + } + if (log === rootLogger) return; + try { + rootLogger[level](message, fields); + } catch { + // Both sinks are gone. Dropping the line is the only remaining option that still ends the + // process, which is the outcome every caller of this file depends on. + } +} + /** Every check, run now, by name. A check that throws is `failing` — never an unhandled error. */ export function readinessChecks(): Readonly> { const results: Record = {}; @@ -173,7 +200,7 @@ export function readinessChecks(): Readonly> { results[name] = check() ? 'ok' : 'failing'; } catch (thrown) { results[name] = 'failing'; - log.warn('readiness check threw', { check: name, error: thrown }); + report('warn', 'readiness check threw', { check: name, error: thrown }); } } return results; @@ -278,7 +305,7 @@ async function runPhase(phase: ShutdownPhase, reason: ShutdownReason): Promise entry.phase === phase)) { const outcome = await settleWithin(() => registration.hook(reason), remainingBudget(reason)); if (outcome.kind === 'failed') { - log.error('shutdown hook failed', { + report('error', 'shutdown hook failed', { hook: registration.name, phase, error: outcome.error, @@ -290,7 +317,7 @@ async function runPhase(phase: ShutdownPhase, reason: ShutdownReason): Promise { const reason: ShutdownReason = { signal, deadlineAt: systemClock.monotonic() + deadlineMs }; drainPromise = (async () => { - log.info('draining', { signal, deadlineMs, inflight }); - await runPhase('accept', reason); - - // Real monotonic, like `deadlineAt` itself: `waitForIdle` sleeps on a real `setTimeout`, and a - // budget read off an injected clock is a number that timer will never honour. - const remaining = Math.max(0, reason.deadlineAt - systemClock.monotonic()); - const idle = await waitForIdle(remaining); - if (!idle) { - log.warn('X_SHUTDOWN_TIMEOUT', { - code: 'X_SHUTDOWN_TIMEOUT', - cause: `${inflight} in-flight operations still running after ${deadlineMs}ms`, - fix: 'raise the budget past the slowest handler — configureLifecycle({ deadlineMs: 600_000 }) for a 10-minute one — and set terminationGracePeriodSeconds to at least as many seconds, or shorten the handler', - }); + try { + report('info', 'draining', { signal, deadlineMs, inflight }); + await runPhase('accept', reason); + + // Real monotonic, like `deadlineAt` itself: `waitForIdle` sleeps on a real `setTimeout`, and + // a budget read off an injected clock is a number that timer will never honour. + const remaining = Math.max(0, reason.deadlineAt - systemClock.monotonic()); + const idle = await waitForIdle(remaining); + if (!idle) { + report('warn', 'X_SHUTDOWN_TIMEOUT', { + code: 'X_SHUTDOWN_TIMEOUT', + cause: `${inflight} in-flight operations still running after ${deadlineMs}ms`, + fix: 'raise the budget past the slowest handler — configureLifecycle({ deadlineMs: 600_000 }) for a 10-minute one — and set terminationGracePeriodSeconds to at least as many seconds, or shorten the handler', + }); + } + + await runPhase('inflight', reason); + await runPhase('close', reason); + } catch (thrown) { + // Nothing above should reach here — every hook is caught by `settleWithin` and every line + // goes through `report`. If something does, the drain still ENDS: a rejected `drainPromise` + // is a memo that re-rejects for every later caller and an unhandled rejection that kills the + // process mid-drain, which is strictly worse than a drain that finished badly and said so. + report('error', 'drain failed', { signal, error: thrown }); + } finally { + state = 'stopped'; } - - await runPhase('inflight', reason); - await runPhase('close', reason); - state = 'stopped'; - log.info('stopped', { signal }); + report('info', 'stopped', { signal }); })(); return drainPromise; @@ -345,9 +381,14 @@ export function installSignalHandlers(options?: SignalHandlerOptions): () => voi for (const signal of signals) { const handler = (): void => { - void drain(signal).then(() => { + // Attached on BOTH settle paths, for the reason `settleWithin` gives: an unhandled rejection + // ends the process before the drain does, and the exit is what the kubelet is waiting for. + // `drain()` cannot reject today — that is the `try/finally` above, not luck — and this is + // the one line that keeps it true when someone changes the body. + const done = (): void => { if (options?.exit === true) process.exit(0); - }); + }; + void drain(signal).then(done, done); }; handlers.set(signal, handler); process.on(signal, handler); diff --git a/packages/core/src/metrics.test.ts b/packages/core/src/metrics.test.ts index aee8aef6..3b40b60f 100644 --- a/packages/core/src/metrics.test.ts +++ b/packages/core/src/metrics.test.ts @@ -106,6 +106,51 @@ describe('the instrument registry', () => { test('a name no exposition format accepts is refused at declaration', () => { expect(() => counter('HTTP.Requests')).toThrow('X_METRIC_NAME_INVALID'); }); + + test('a second declaration stating different bounds is refused, not silently dropped', () => { + // The first declaration won and the second's bounds were discarded without a word, so a + // module recording into buckets it chose was reading another module's. + histogram('test_redeclare_seconds', { bounds: [0.1, 1] }); + let caught: unknown; + try { + histogram('test_redeclare_seconds', { bounds: [1, 10] }); + } catch (thrown) { + caught = thrown; + } + expect(isUltimateError(caught)).toBe(true); + expect((caught as UltimateError).code).toBe('X_METRIC_NAME_INVALID'); + expect((caught as UltimateError).cause).toContain('0.1, 1'); + }); + + test('a second declaration stating a different observer is refused', () => { + gauge('test_redeclare_observed', { observe: () => 1 }); + let caught: unknown; + try { + gauge('test_redeclare_observed', { observe: () => 2 }); + } catch (thrown) { + caught = thrown; + } + // Asserted before the cast: with nothing thrown, `caught` is `undefined` and reading `.code` + // dies as a `TypeError` naming neither the metric nor the missing refusal. + expect(isUltimateError(caught)).toBe(true); + expect((caught as UltimateError).code).toBe('X_METRIC_NAME_INVALID'); + // The first observer is still the live one — the refusal changed nothing. + expect(pointsOf('test_redeclare_observed')[0]?.value).toBe(1); + }); + + test('omitting an option takes a handle on the existing instrument, and never conflicts', () => { + // `gauge(name)` is how a second module reads an instrument someone else declared; it states + // nothing, so there is nothing to disagree about. + const observed = () => 3; + gauge('test_redeclare_handle', { observe: observed, maxSeries: 4 }); + expect(() => gauge('test_redeclare_handle')).not.toThrow(); + expect(() => gauge('test_redeclare_handle', { observe: observed })).not.toThrow(); + expect(() => histogram('test_redeclare_bounds')).not.toThrow(); + const bounds = [0.5, 5]; + histogram('test_redeclare_bounds_stated', { bounds }); + expect(() => histogram('test_redeclare_bounds_stated', { bounds: [0.5, 5] })).not.toThrow(); + expect(pointsOf('test_redeclare_handle')[0]?.value).toBe(3); + }); }); describe('export', () => { diff --git a/packages/core/src/metrics.ts b/packages/core/src/metrics.ts index 2dc9661a..55652824 100644 --- a/packages/core/src/metrics.ts +++ b/packages/core/src/metrics.ts @@ -111,12 +111,20 @@ export interface InstrumentOptions { } export interface GaugeOptions extends InstrumentOptions { - /** Async instrument: read at collection time instead of being pushed. Never stale. */ + /** + * Async instrument: read at collection time instead of being pushed. Never stale. + * Stated twice for one name with two different callbacks is `X_METRIC_NAME_INVALID`, not a + * silent win for the first — see `assertSameDeclaration`. + */ readonly observe?: (() => number) | undefined; } export interface HistogramOptions extends InstrumentOptions { - /** Explicit bucket boundaries, ascending. Defaults to the OTel latency-in-seconds set. */ + /** + * Explicit bucket boundaries, ascending. Defaults to the OTel latency-in-seconds set. + * Stated twice for one name with two different sets is `X_METRIC_NAME_INVALID`, not a silent + * win for the first — see `assertSameDeclaration`. + */ readonly bounds?: readonly number[] | undefined; } @@ -263,6 +271,7 @@ function declare(name: string, kind: MetricKind, options: GaugeOptions & Histogr meta: { name, declared: existing.descriptor.kind, requested: kind }, }); } + assertSameDeclaration(name, existing, options); return existing; } const maxSeries = options.maxSeries ?? DEFAULT_MAX_SERIES; @@ -290,6 +299,39 @@ function declare(name: string, kind: MetricKind, options: GaugeOptions & Histogr return instrument; } +/** + * A second declaration that STATES a different shape is refused. The first declaration wins, so a + * second `histogram(name, { bounds })` recorded into buckets another module chose and a second + * `gauge(name, { observe })` was collected through the first module's observer — silently, in both + * cases, which is the whole failure. An OMITTED option is not a conflict: `gauge(name)` is how a + * module takes a handle on an instrument someone else declared, and `maxSeries` keeps its shipped + * first-declaration-wins rule because it decides a ceiling rather than what gets recorded. + */ +function assertSameDeclaration( + name: string, + existing: Instrument, + options: GaugeOptions & HistogramOptions, +): void { + const { bounds, observe } = options; + if (bounds !== undefined && !sameBounds(existing.bounds, bounds)) { + throw new MetricNameInvalidError({ + cause: `"${name}" is already declared with bounds [${existing.bounds.join(', ')}] and is redeclared with [${bounds.join(', ')}]; the first declaration wins, so the second set would never be used`, + fix: `declare "${name}" once and export the handle — import it where you record — or give the second instrument its own name`, + meta: { name, declared: existing.bounds.join(','), requested: bounds.join(',') }, + }); + } + if (observe !== undefined && observe !== existing.observe) { + throw new MetricNameInvalidError({ + cause: `"${name}" is already declared with an observe() callback and is redeclared with a different one; the first declaration wins, so the second callback would never be read`, + fix: `declare "${name}" once and export the handle — import it where you read — or give the second gauge its own name`, + meta: { name }, + }); + } +} + +const sameBounds = (left: readonly number[], right: readonly number[]): boolean => + left.length === right.length && left.every((bound, index) => bound === right[index]); + /** * Reported through the logger rather than thrown: the call site is `orderCounter.add(1, …)` deep * inside a request, and killing that request would turn a metrics bug into a user-visible outage diff --git a/packages/db/CLAUDE.md b/packages/db/CLAUDE.md index 74206bfd..9692a188 100644 --- a/packages/db/CLAUDE.md +++ b/packages/db/CLAUDE.md @@ -11,6 +11,7 @@ reaches down to this package for it. **Never** import `entity`, `jobs`, `http` o | SQL | `sql` binds `$n`; anything non-scalar and non-fragment throws `X_SQL_UNSAFE` | | Escape hatches | `raw()`, `identifier()`, `literal()` — each call is an audit point | | SQLSTATE | one reader, `sqlState()` (`sqlstate.ts`). Never read `error.code` for a SQLSTATE | +| Reading a caught value | `renderThrowable()` from core; never `error instanceof Error ? error.message : String(error)` — both halves RUN app code (a `Proxy` trap, `Symbol.toPrimitive`) and `checkDb` backs `/readyz`, where a render that throws is an exception in place of the report the kubelet asked for | | Errors | subclass `DbError`; never `throw new Error` **in source**. A test simulating a *database* failure throws `dbUnavailable()`; a test simulating the *caller's body* failing throws a bare `Error` on purpose — an arbitrary throw is exactly what rollback and disposal must survive, and a `DbError` there would prove the narrower thing | | New code | add to `DB_ERROR_CODES` **and** `DB_ERROR_TITLES` in `errors.ts` | | Exports | explicit in `src/index.ts`; no `export *` | @@ -318,6 +319,29 @@ block is the join of that fix with the engine it ships through — an entity des `generateMigration`, applied by `migrate()` itself against a real server, columns confirmed against `pg_indexes`, rather than either half alone. +**The ledger audit asks one question — does this build ship every migration the ledger records?** +`auditLedger`'s `foreign` filter is `!known.has(row.id)` and nothing else, `As of 2026-08`. It used +to also require `row.app_version !== appVersion`, which switched the audit OFF wherever the two +agree: `runningAppVersion()` answers `dev` for every development build, so a migration applied by an +earlier `dev` build and since deleted was invisible, and `expectedSchema` (`drift.ts`) then dropped +its table from the comparison — `x db drift` answering `ok: true` against a database that still has +the table. The version is a detail of the ANSWER and lives in the cause, never in the predicate. + +**`rollback({ steps })` refuses anything that is not a positive safe integer, before the lock.** +`steps` reaches `slice(0, steps)`, where a negative count counts from the END: `steps: -1` selected +every applied migration but the newest and reversed four of five. `X_INVARIANT` (core's generic +code, borrowed in `DB_BORROWED_ERROR_CODES` the way `@ultimat3/money`'s `roundRatio` borrows it — +a bad argument is not a fact about the ledger), thrown by `rollbackStepsInvalid` before the advisory +lock is taken and before the ledger is read. Same discipline as `poolMaxInvalid`: a number this +build cannot honour is refused, never reinterpreted. + +**`reapBranches` skips a `createdAt` it cannot parse; it never reads one as infinitely old.** +`NaN > cutoff` is `false`, which is the same answer "older than the cutoff" gives — so a +`COMMENT ON DATABASE` that was truncated or hand-edited used to be a database DROPPED on the next +nightly sweep whatever `maxAgeMs` said. `Date.parse` + `Number.isFinite`, the discipline +`@ultimat3/seo`'s `feed-dates.ts` applies to the same question. (Distinct from the open +source-blindness of the reaper, issue #133.) + **One send is one statement, so `migrate()` and `rollback()` split the script.** `tx.execute(raw( migration.up))` on a text holding two commands is where the two drivers disagreed, and the disagreement is the whole reason this is a bug rather than a preference: `pglite.ts` calls diff --git a/packages/db/src/branch.test.ts b/packages/db/src/branch.test.ts index 30cac4e5..b706c076 100644 --- a/packages/db/src/branch.test.ts +++ b/packages/db/src/branch.test.ts @@ -33,6 +33,66 @@ describe('reapBranches', () => { const dropped = await reapBranches({ client, maxAgeMs: 1_000 }); expect(dropped).toEqual(['stale']); }); + + /** + * A comment truncated by `pg_database.description`'s own limits, or hand-edited, parses to + * `NaN` — and `NaN > cutoff` is `false`, which is the same answer "older than the cutoff" + * gives. So an unreadable timestamp did not merely lose its age: it reaped the database on the + * next nightly sweep, `maxAgeMs` notwithstanding. An age nothing can read is not an old age. + */ + test('an unparseable createdAt is skipped, not read as infinitely old', async () => { + const client = createRecordingClient(); + client.on('pg_database', { + rows: [ + { name: 'truncated', comment: 'ultimate:branch:2026-01-0', size_bytes: 0 }, + { name: 'empty', comment: 'ultimate:branch:', size_bytes: 0 }, + { + name: 'stale', + comment: `ultimate:branch:${new Date(Date.now() - 10_000).toISOString()}`, + size_bytes: 0, + }, + ], + }); + + const dropped = await reapBranches({ client, maxAgeMs: 1_000 }); + + expect(dropped).toEqual(['stale']); + expect( + client.texts.some((text) => text.includes('drop database') && text.includes('truncated')), + ).toBe(false); + }); + + /** + * `Number.isFinite` is not the whole guard, because truncation does not always reach `NaN`: + * `'2020-01-01T00:00'` parses fine — as *local* time, an instant up to 14 hours from the one + * the string reads as, and never one `createBranch` wrote. `toISOString()` is the only writer + * (`branch.ts`), so a comment that does not round trip through it is not the framework's, and + * the reaper leaves it alone rather than acting on a date nobody wrote. + */ + test('a finite but non-canonical createdAt is skipped too, not reaped on a date nobody wrote', async () => { + const client = createRecordingClient(); + client.on('pg_database', { + rows: [ + // Both parse, both are finite, and both are far older than the cutoff — so the finite + // check alone drops them, and only the round trip spares them. + { name: 'truncated_local', comment: 'ultimate:branch:2020-01-01T00:00', size_bytes: 0 }, + { name: 'no_millis', comment: 'ultimate:branch:2020-01-01T00:00:00Z', size_bytes: 0 }, + { + name: 'stale', + comment: `ultimate:branch:${new Date(Date.now() - 10_000).toISOString()}`, + size_bytes: 0, + }, + ], + }); + + const dropped = await reapBranches({ client, maxAgeMs: 1_000 }); + + expect(dropped).toEqual(['stale']); + expect(client.texts.some((text) => text.includes('drop database'))).toBe(true); + expect( + client.texts.some((text) => text.includes('drop database') && !text.includes('"stale"')), + ).toBe(false); + }); }); /** diff --git a/packages/db/src/branch.ts b/packages/db/src/branch.ts index e06efa0e..80b21de3 100644 --- a/packages/db/src/branch.ts +++ b/packages/db/src/branch.ts @@ -134,7 +134,17 @@ export async function reapBranches(options: ReapOptions): Promise cutoff) continue; + const createdAtMs = Date.parse(branch.createdAt); + // `NaN > cutoff` is `false`, which is the same answer "older than the cutoff" gives — so a + // truncated or hand-edited comment used to be a database DROPPED on the next sweep, whatever + // `maxAgeMs` said. An age nothing can read is not an old age. + if (!Number.isFinite(createdAtMs)) continue; + // Finite is not enough: `'2026-08-18T10:00'` parses as LOCAL time, so a truncated comment + // names an instant hours from the one it reads as, and the sweep acts on a date nobody wrote. + // `createBranch` writes `toISOString()` and nothing else does, so a value that does not round + // trip through it is not ours — there is no legitimate non-canonical comment to strand. + if (new Date(createdAtMs).toISOString() !== branch.createdAt) continue; + if (createdAtMs > cutoff) continue; await dropBranch(branch.name, options); dropped.push(branch.name); } diff --git a/packages/db/src/client-checkdb.test.ts b/packages/db/src/client-checkdb.test.ts new file mode 100644 index 00000000..b3bab962 --- /dev/null +++ b/packages/db/src/client-checkdb.test.ts @@ -0,0 +1,61 @@ +// Single responsibility: `checkDb`, the readiness report behind /readyz — it answers, whatever +// the driver's refusal turns out to be. Split from `client.test.ts` for the file-size ceiling, +// along the seam that it needs none of that file's fake pool: a client that only rejects is the +// whole fixture. + +import { describe, expect, test } from 'bun:test'; +import { checkDb, type DbClient } from './client'; + +/** + * Stands in for what `Bun.SQL` itself throws — an error carrying no Ultimate code, which is + * precisely the shape that must never escape this module. + */ +class DriverFailure extends Error { + override readonly name = 'DriverFailure'; +} + +/** + * `/readyz` is what decides whether the kubelet keeps this pod, so "never throws" is not a + * nicety: an exception out of the probe is an unhandled rejection where a `{ ok: false }` report + * belongs. Rendering the driver's refusal used to be the throw — `instanceof` runs a `Proxy`'s + * `getPrototypeOf` trap and `String()` runs `Symbol.toPrimitive`, both on a value this package + * did not build. + */ +describe('checkDb reports, and never throws', () => { + const refusing = (thrown: unknown): DbClient => ({ + query: () => Promise.reject(thrown), + one: () => Promise.reject(thrown), + execute: () => Promise.reject(thrown), + }); + + test('an ordinary driver failure is a report', async () => { + const report = await checkDb(refusing(new DriverFailure('connection refused'))); + + expect(report.ok).toBe(false); + expect(report.error).toContain('connection refused'); + expect(typeof report.latencyMs).toBe('number'); + }); + + test('a Proxy whose getPrototypeOf throws is a report too', async () => { + const hostile = new Proxy( + {}, + { + getPrototypeOf() { + throw new TypeError('proxy trap'); + }, + }, + ); + + const report = await checkDb(refusing(hostile)); + + expect(report.ok).toBe(false); + expect(typeof report.error).toBe('string'); + }); + + test('a thrown symbol, which template interpolation cannot render, is a report too', async () => { + const report = await checkDb(refusing(Symbol('nope'))); + + expect(report.ok).toBe(false); + expect(report.error).toContain('nope'); + }); +}); diff --git a/packages/db/src/client-observer.test.ts b/packages/db/src/client-observer.test.ts new file mode 100644 index 00000000..7b6e52a9 --- /dev/null +++ b/packages/db/src/client-observer.test.ts @@ -0,0 +1,202 @@ +// Split out of `client.test.ts` for the file-size ceiling, along the seam `observe.ts` already +// draws — the same split `pglite-observer.test.ts` is on the other driver. What it pins: the one +// funnel every statement takes, pooled or pinned, and the attribution and expected-loop reason +// stamped onto the event it reports. + +import { afterEach, describe, expect, test } from 'bun:test'; +import { withStatementAttribution } from './attribution'; +import { createPostgresClient } from './client'; +import { DbError } from './errors'; +import { expectedQueryLoop } from './expected-loop'; +import type { StatementEvent, StatementObserver } from './observe'; +import { setStatementObserver } from './observe'; +import { sql } from './sql'; + +/** + * Stands in for what `Bun.SQL` itself throws — an error carrying no Ultimate code, which is + * precisely the shape that must never escape this module. + */ +class DriverFailure extends Error { + override readonly name = 'DriverFailure'; +} + +const TEST_URL = 'postgres://app@127.0.0.1:5432/ultimate_test'; + +// `Bun.SQL` is writable but not configurable, so the seam is assignment plus an afterEach restore. +const host = globalThis as unknown as { Bun: { SQL: unknown } }; +const realBunSql = host.Bun.SQL; + +afterEach(() => { + host.Bun.SQL = realBunSql; + // Process-wide, so a test that installs one and leaves it behind observes every later test. + setStatementObserver(undefined); +}); + +interface FakeSqlOptions { + readonly statementError?: DriverFailure | undefined; + /** What every statement resolves with — the row count the observer reports comes off this. */ + readonly rows?: readonly unknown[] | undefined; +} + +/** No recording of its own: what the statements were is the observer's answer, not the pool's. */ +function installFakeSql(options: FakeSqlOptions = {}): void { + const rows = options.rows ?? []; + host.Bun.SQL = class { + async unsafe(): Promise { + return rows; + } + async reserve(): Promise { + return { + unsafe: async (): Promise => { + if (options.statementError !== undefined) throw options.statementError; + return rows; + }, + release: (): void => undefined, + }; + } + async close(): Promise {} + }; +} + +/** The rejected value, or the resolved one — the assertion then says which of the two we got. */ +const rejection = (promise: Promise): Promise => + promise.catch((error: unknown) => error); + +function recorder(): StatementObserver & { readonly seen: StatementEvent[] } { + const seen: StatementEvent[] = []; + return { + seen, + onStatement(event: StatementEvent): void { + seen.push(event); + }, + }; +} + +describe('the statement observer', () => { + // `runOn` is the funnel, so pooled and pinned statements are the same event — a detector that + // only saw the pool would be blind to everything inside a transaction, which is where the read + // loops live. + test('sees every statement once, pooled and pinned alike', async () => { + const observer = recorder(); + setStatementObserver(observer); + installFakeSql({ rows: [{ id: 1 }, { id: 2 }] }); + const client = createPostgresClient({ url: TEST_URL }); + + await client.query(sql`select id from members where org = ${'o_1'}`); + using connection = await client.reserve(); + await connection.execute(sql`BEGIN`); + + expect(observer.seen.map((event) => event.text)).toEqual([ + 'select id from members where org = $1', + 'BEGIN', + ]); + expect(observer.seen[0]?.values).toEqual(['o_1']); + expect(observer.seen[0]?.rows).toBe(2); + expect(observer.seen[0]?.durationMs).toBeGreaterThanOrEqual(0); + expect(observer.seen[0]).not.toHaveProperty('error'); + }); + + test('reports a failed statement with the error the caller is about to be thrown', async () => { + const observer = recorder(); + setStatementObserver(observer); + installFakeSql({ statementError: new DriverFailure('deadlock detected') }); + const connection = await createPostgresClient({ url: TEST_URL }).reserve(); + + const caught = await rejection(connection.query(sql`select 1`)); + + expect((caught as DbError).code).toBe('X_DB_UNAVAILABLE'); + // Identity, not shape: the event carries the very error thrown, already wrapped by the funnel. + expect(observer.seen[0]?.error).toBe(caught); + expect(observer.seen[0]?.rows).toBe(0); + connection.release(); + }); + + // Strict test mode is an observer that throws, and the throw must arrive as itself. Notifying + // inside the statement's own `try` would re-report a statement that succeeded as X_DB_UNAVAILABLE. + test('a throwing observer reaches the caller as its own error, not a database failure', async () => { + installFakeSql(); + // A bare `Error` on purpose: the observer is app-supplied, an app can throw anything, and + // "arrives as itself" is the claim. Hoisted so the assertion can be identity — message + // equality passes on a re-wrapped copy, which is exactly the failure this test exists to see. + const thrown = new Error('n+1 in a strict test'); + setStatementObserver({ + onStatement(): void { + throw thrown; + }, + }); + + const caught = await rejection(createPostgresClient({ url: TEST_URL }).query(sql`select 1`)); + + expect(caught).not.toBeInstanceOf(DbError); + expect(caught).toBe(thrown); + }); + + test('reserving a connection and closing the pool are not statements', async () => { + const observer = recorder(); + setStatementObserver(observer); + installFakeSql(); + const client = createPostgresClient({ url: TEST_URL }); + + (await client.reserve()).release(); + await client.close(); + + expect(observer.seen).toEqual([]); + }); + + test('an uninstalled seam observes nothing, which is the production path', async () => { + const observer = recorder(); + setStatementObserver(observer); + setStatementObserver(undefined); + installFakeSql(); + + await createPostgresClient({ url: TEST_URL }).query(sql`select 1`); + + expect(observer.seen).toEqual([]); + }); + + test('carries the attribution declared by the scope, undefined outside every scope', async () => { + const observer = recorder(); + setStatementObserver(observer); + installFakeSql(); + const client = createPostgresClient({ url: TEST_URL }); + + await withStatementAttribution('members', 'findById', () => client.query(sql`select 1`)); + await client.query(sql`select 2`); + + expect(observer.seen.map((event) => event.attribution)).toEqual([ + { entity: 'members', op: 'findById' }, + undefined, + ]); + }); + + test('the failing statement path still carries the attribution', async () => { + const observer = recorder(); + setStatementObserver(observer); + installFakeSql({ statementError: new DriverFailure('deadlock detected') }); + const connection = await createPostgresClient({ url: TEST_URL }).reserve(); + + const caught = await rejection( + withStatementAttribution('members', 'findById', () => connection.query(sql`select 1`)), + ); + + expect((caught as DbError).code).toBe('X_DB_UNAVAILABLE'); + expect(observer.seen[0]?.attribution).toEqual({ entity: 'members', op: 'findById' }); + expect(observer.seen[0]?.rows).toBe(0); + connection.release(); + }); + + // Two independent scopes: an expected-loop reason does not crowd out the attribution. + test('attribution and an expected-loop reason are stamped together, independently', async () => { + const observer = recorder(); + setStatementObserver(observer); + installFakeSql(); + const client = createPostgresClient({ url: TEST_URL }); + + await withStatementAttribution('members', 'findMany', () => + expectedQueryLoop('one lookup per id', () => client.query(sql`select 1`)), + ); + + expect(observer.seen[0]?.attribution).toEqual({ entity: 'members', op: 'findMany' }); + expect(observer.seen[0]?.expected).toBe('one lookup per id'); + }); +}); diff --git a/packages/db/src/client.test.ts b/packages/db/src/client.test.ts index 821cd697..1b093e45 100644 --- a/packages/db/src/client.test.ts +++ b/packages/db/src/client.test.ts @@ -4,12 +4,8 @@ // as an untyped driver error instead of X_DB_UNAVAILABLE. import { afterEach, describe, expect, test } from 'bun:test'; -import { withStatementAttribution } from './attribution'; import { createPostgresClient } from './client'; import { DbError } from './errors'; -import { expectedQueryLoop } from './expected-loop'; -import type { StatementEvent, StatementObserver } from './observe'; -import { setStatementObserver } from './observe'; import { sql } from './sql'; const TEST_URL = 'postgres://app@127.0.0.1:5432/ultimate_test'; @@ -37,8 +33,6 @@ interface FakeSqlOptions { readonly reserveError?: DriverFailure | undefined; readonly statementError?: DriverFailure | undefined; readonly closeError?: DriverFailure | undefined; - /** What every statement resolves with — the row count the observer reports comes off this. */ - readonly rows?: readonly unknown[] | undefined; } // `Bun.SQL` is writable but not configurable, so the seam is assignment plus an afterEach restore. @@ -47,20 +41,17 @@ const realBunSql = host.Bun.SQL; afterEach(() => { host.Bun.SQL = realBunSql; - // Process-wide, so a test that installs one and leaves it behind observes every later test. - setStatementObserver(undefined); }); function installFakeSql(options: FakeSqlOptions = {}): FakePool { const pool: FakePool = { urls: [], statements: [], pinned: [], releases: 0, closes: 0 }; - const rows = options.rows ?? []; host.Bun.SQL = class { constructor(url: string) { pool.urls.push(url); } async unsafe(text: string): Promise { pool.statements.push(text); - return rows; + return []; } async reserve(): Promise { if (options.reserveError !== undefined) throw options.reserveError; @@ -69,7 +60,7 @@ function installFakeSql(options: FakeSqlOptions = {}): FakePool { pool.statements.push(text); pool.pinned.push(text); if (options.statementError !== undefined) throw options.statementError; - return rows; + return []; }, release: (): void => { pool.releases += 1; @@ -328,138 +319,3 @@ describe('close', () => { expect(pool.closes).toBe(1); }); }); - -function recorder(): StatementObserver & { readonly seen: StatementEvent[] } { - const seen: StatementEvent[] = []; - return { - seen, - onStatement(event: StatementEvent): void { - seen.push(event); - }, - }; -} - -describe('the statement observer', () => { - // `runOn` is the funnel, so pooled and pinned statements are the same event — a detector that - // only saw the pool would be blind to everything inside a transaction, which is where the read - // loops live. - test('sees every statement once, pooled and pinned alike', async () => { - const observer = recorder(); - setStatementObserver(observer); - installFakeSql({ rows: [{ id: 1 }, { id: 2 }] }); - const client = createPostgresClient({ url: TEST_URL }); - - await client.query(sql`select id from members where org = ${'o_1'}`); - using connection = await client.reserve(); - await connection.execute(sql`BEGIN`); - - expect(observer.seen.map((event) => event.text)).toEqual([ - 'select id from members where org = $1', - 'BEGIN', - ]); - expect(observer.seen[0]?.values).toEqual(['o_1']); - expect(observer.seen[0]?.rows).toBe(2); - expect(observer.seen[0]?.durationMs).toBeGreaterThanOrEqual(0); - expect(observer.seen[0]).not.toHaveProperty('error'); - }); - - test('reports a failed statement with the error the caller is about to be thrown', async () => { - const observer = recorder(); - setStatementObserver(observer); - installFakeSql({ statementError: new DriverFailure('deadlock detected') }); - const connection = await createPostgresClient({ url: TEST_URL }).reserve(); - - const caught = await rejection(connection.query(sql`select 1`)); - - expect((caught as DbError).code).toBe('X_DB_UNAVAILABLE'); - // Identity, not shape: the event carries the very error thrown, already wrapped by the funnel. - expect(observer.seen[0]?.error).toBe(caught); - expect(observer.seen[0]?.rows).toBe(0); - connection.release(); - }); - - // Strict test mode is an observer that throws, and the throw must arrive as itself. Notifying - // inside the statement's own `try` would re-report a statement that succeeded as X_DB_UNAVAILABLE. - test('a throwing observer reaches the caller as its own error, not a database failure', async () => { - installFakeSql(); - setStatementObserver({ - onStatement(): void { - throw new Error('n+1 in a strict test'); - }, - }); - - const caught = await rejection(createPostgresClient({ url: TEST_URL }).query(sql`select 1`)); - - expect(caught).not.toBeInstanceOf(DbError); - expect((caught as Error).message).toBe('n+1 in a strict test'); - }); - - test('reserving a connection and closing the pool are not statements', async () => { - const observer = recorder(); - setStatementObserver(observer); - installFakeSql(); - const client = createPostgresClient({ url: TEST_URL }); - - (await client.reserve()).release(); - await client.close(); - - expect(observer.seen).toEqual([]); - }); - - test('an uninstalled seam observes nothing, which is the production path', async () => { - const observer = recorder(); - setStatementObserver(observer); - setStatementObserver(undefined); - installFakeSql(); - - await createPostgresClient({ url: TEST_URL }).query(sql`select 1`); - - expect(observer.seen).toEqual([]); - }); - - test('carries the attribution declared by the scope, undefined outside every scope', async () => { - const observer = recorder(); - setStatementObserver(observer); - installFakeSql(); - const client = createPostgresClient({ url: TEST_URL }); - - await withStatementAttribution('members', 'findById', () => client.query(sql`select 1`)); - await client.query(sql`select 2`); - - expect(observer.seen.map((event) => event.attribution)).toEqual([ - { entity: 'members', op: 'findById' }, - undefined, - ]); - }); - - test('the failing statement path still carries the attribution', async () => { - const observer = recorder(); - setStatementObserver(observer); - installFakeSql({ statementError: new DriverFailure('deadlock detected') }); - const connection = await createPostgresClient({ url: TEST_URL }).reserve(); - - const caught = await rejection( - withStatementAttribution('members', 'findById', () => connection.query(sql`select 1`)), - ); - - expect((caught as DbError).code).toBe('X_DB_UNAVAILABLE'); - expect(observer.seen[0]?.attribution).toEqual({ entity: 'members', op: 'findById' }); - expect(observer.seen[0]?.rows).toBe(0); - connection.release(); - }); - - // Two independent scopes: an expected-loop reason does not crowd out the attribution. - test('attribution and an expected-loop reason are stamped together, independently', async () => { - const observer = recorder(); - setStatementObserver(observer); - installFakeSql(); - const client = createPostgresClient({ url: TEST_URL }); - - await withStatementAttribution('members', 'findMany', () => - expectedQueryLoop('one lookup per id', () => client.query(sql`select 1`)), - ); - - expect(observer.seen[0]?.attribution).toEqual({ entity: 'members', op: 'findMany' }); - expect(observer.seen[0]?.expected).toBe('one lookup per id'); - }); -}); diff --git a/packages/db/src/client.ts b/packages/db/src/client.ts index 0981e7fd..0e3ab13e 100644 --- a/packages/db/src/client.ts +++ b/packages/db/src/client.ts @@ -3,7 +3,7 @@ // pool like a `web` process behind a CDN. `Bun.SQL` is reached lazily so importing this module // never opens a socket (the CLI imports it to print help). -import { type Role, resolveRole } from '@ultimat3/core'; +import { type Role, renderThrowable, resolveRole } from '@ultimat3/core'; import { statementAttribution } from './attribution'; import { DbError, dbUnavailable, driverError, poolAcquireTimeout, poolMaxInvalid } from './errors'; import { expectedQueryLoopReason } from './expected-loop'; @@ -457,7 +457,9 @@ export async function checkDb(client: DbClient = baseClient()): Promise export const migrationIrreversible = (cause: string, fix: string): DbError => new DbError({ code: 'X_MIGRATION_IRREVERSIBLE', cause, fix }); +/** + * A rollback step count this build cannot honour. `steps` reaches `Array.prototype.slice`, where a + * negative count counts from the END: `steps: -1` selected every applied migration except the + * newest and reversed four of five, which is the one class of mistake a rollback cannot undo. + * Refused rather than coerced, exactly as `DATABASE_POOL_MAX` is — a number silently reinterpreted + * as a different one is the failure a validated argument exists to prevent. + */ +export const rollbackStepsInvalid = (received: number): DbError => + new DbError({ + code: 'X_INVARIANT', + cause: `rollback was asked to reverse ${String(received)} migrations, which is not a positive integer`, + fix: 'rollback({ migrations, steps: 1 }) # a whole number of migrations, newest first', + meta: { steps: received }, + }); + /** * `packages/db/migrations/0000_initial.snapshot.json` → `packages/db/migrations/0000_initial.*` — * every file that one migration owns, as one `rm` argument. Derived from the path the caller passed diff --git a/packages/db/src/index.ts b/packages/db/src/index.ts index b5478ba3..f94ebc7a 100644 --- a/packages/db/src/index.ts +++ b/packages/db/src/index.ts @@ -71,6 +71,7 @@ export { multipleStatements, poolAcquireTimeout, poolMaxInvalid, + rollbackStepsInvalid, serializationExhausted, sqlUnsafe, } from './errors'; diff --git a/packages/db/src/migrate-pin.test.ts b/packages/db/src/migrate-pin.test.ts new file mode 100644 index 00000000..025a99b8 --- /dev/null +++ b/packages/db/src/migrate-pin.test.ts @@ -0,0 +1,251 @@ +// Single responsibility: WHERE a migration's statements run — the advisory lock, the transaction +// and the DDL all on ONE pinned session, and the loop that declares itself to an N+1 detector. +// Split from `migrate.test.ts` for the file-size ceiling; `migrate-lock.test.ts` is the lock's +// other half, how long a migrator waits for it and what a statement's own lock wait is bounded to. + +import { beforeEach, describe, expect, test } from 'bun:test'; +import { type DbClient, type DbConnection, type ReservableClient, setDbClient } from './client'; +import { expectedQueryLoopReason } from './expected-loop'; +import { createRecordingClient, type RecordingClient } from './fake'; +import { type LedgerRow, type Migration, migrate, migrationChecksum, rollback } from './migrate'; + +const addPosts: Migration = { + id: '20260101000000_create_posts', + name: 'create posts', + up: 'create table "posts" ("id" uuid primary key);', + down: 'drop table "posts";', +}; + +const ledgerRow = (overrides: Partial = {}): LedgerRow => ({ + id: addPosts.id, + name: addPosts.name, + checksum: migrationChecksum(addPosts), + applied_at: '2026-01-01T00:00:00.000Z', + app_version: '1.5.0', + duration_ms: 4, + ...overrides, +}); + +let client: RecordingClient; + +beforeEach(() => { + client = createRecordingClient(); + setDbClient(client); +}); + +const squash = (text: string): string => text.replace(/\s+/g, ' ').trim(); + +interface PinnablePool { + readonly client: ReservableClient; + /** `reserve`, `release`, and every statement tagged with the handle that ran it. */ + readonly events: readonly string[]; +} + +/** + * A pool whose pin is observable. The defect this pins is invisible to the recording client: the + * statement texts are identical whether the lock landed on the session that runs the migration or + * on whatever connection the pool lent for that one statement, and only the tag says which. + */ +function pinnable(inner: DbClient): PinnablePool { + const events: string[] = []; + const through = (tag: string): DbClient => ({ + query: (fragment) => { + events.push(`${tag}:${squash(fragment.text)}`); + return inner.query(fragment); + }, + one: (fragment) => { + events.push(`${tag}:${squash(fragment.text)}`); + return inner.one(fragment); + }, + execute: (fragment) => { + events.push(`${tag}:${squash(fragment.text)}`); + return inner.execute(fragment); + }, + }); + return { + events, + client: { + ...through('pool'), + reserve: async (): Promise => { + events.push('reserve'); + let held = true; + const release = (): void => { + if (!held) return; + held = false; + events.push('release'); + }; + return { ...through('pin'), release, [Symbol.dispose]: release }; + }, + }, + }; +} + +interface Witness { + readonly client: DbClient; + readonly statements: readonly { readonly text: string; readonly reason: string | undefined }[]; +} + +/** + * Every statement paired with the `expectedQueryLoop` reason in force when it was issued. The + * recording client cannot answer this: it never passes a funnel, so the observer never fires and + * the scope has to be read where the statement is sent. + */ +function witnessed(inner: DbClient): Witness { + const statements: { text: string; reason: string | undefined }[] = []; + const note = (text: string): void => { + statements.push({ text: squash(text), reason: expectedQueryLoopReason() }); + }; + return { + statements, + client: { + query: (fragment) => { + note(fragment.text); + return inner.query(fragment); + }, + one: (fragment) => { + note(fragment.text); + return inner.one(fragment); + }, + execute: (fragment) => { + note(fragment.text); + return inner.execute(fragment); + }, + }, + }; +} + +/** + * Fails when nothing matched instead of answering `undefined` for it. `find(...)?.reason` alone + * collapses two different facts into one value — "this statement ran outside every scope" and + * "this statement never ran" — and the `toBeUndefined()` assertions below are the load-bearing + * half of both test names, so a reworded ledger read would leave them passing on the wrong one. + */ +const reasonFor = (witness: Witness, needle: string): string | undefined => { + expect(witness.statements.map((statement) => statement.text).join(' | ')).toContain(needle); + return witness.statements.find((statement) => statement.text.includes(needle))?.reason; +}; + +describe('the migration advisory lock', () => { + test('the lock, the migration and the unlock run on one pinned session', async () => { + client.on(/from x_migrations/, { rows: [] }); + const pool = pinnable(client); + + await migrate({ migrations: [addPosts], appVersion: '1.5.0', client: pool.client }); + + expect(pool.events[0]).toBe('reserve'); + expect(pool.events[1]).toContain('pg_try_advisory_lock'); + expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); + expect(pool.events.at(-1)).toBe('release'); + // Not one statement on the pool: `pg_advisory_lock` is session-scoped, so work done on any + // other connection is not under the lock, and the unlock would answer `false` on a session + // that never took it. On `ROLE=migrate` (`max: 1`) there is no other connection to run on. + expect(pool.events.filter((event) => event.startsWith('pool:'))).toEqual([]); + expect(pool.events).toContain('pin:BEGIN'); + expect(pool.events).toContain('pin:COMMIT'); + expect(pool.events.some((event) => event.includes('create table "posts"'))).toBe(true); + expect(pool.events.filter((event) => event === 'reserve')).toHaveLength(1); + }); + + test('a refused ledger unlocks and gives the pin back', async () => { + const foreign = ledgerRow({ id: '20260202000000_from_the_future', app_version: '1.6.0' }); + client.on(/from x_migrations/, { rows: [foreign] }); + const pool = pinnable(client); + + await expect( + migrate({ migrations: [addPosts], appVersion: '1.5.0', client: pool.client }), + ).rejects.toThrow('X_MIGRATION_CONFLICT'); + + expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); + expect(pool.events.at(-1)).toBe('release'); + }); + + test("lock: false takes no lock, and the only pin left is the transaction's own", async () => { + client.on(/from x_migrations/, { rows: [] }); + const pool = pinnable(client); + + await migrate({ + migrations: [addPosts], + appVersion: '1.5.0', + client: pool.client, + lock: false, + }); + + expect(client.texts.some((text) => text.includes('pg_advisory'))).toBe(false); + // The ledger runs unpinned, so the one reservation belongs to `withTransaction`, not to a + // lock scope that was never opened. + expect(pool.events[0]).toStartWith('pool:'); + expect(pool.events.filter((event) => event === 'reserve')).toHaveLength(1); + expect(pool.events.indexOf('reserve')).toBe(pool.events.indexOf('pin:BEGIN') - 1); + expect(client.texts.some((text) => text.includes('create table "posts"'))).toBe(true); + }); + + test('rollback takes the same lock, on its own pinned session', async () => { + client.on(/from x_migrations/, { rows: [ledgerRow()] }); + const pool = pinnable(client); + + const reverted = await rollback({ migrations: [addPosts], client: pool.client }); + + expect(reverted).toEqual([addPosts.id]); + expect(pool.events[0]).toBe('reserve'); + expect(pool.events[1]).toContain('pg_try_advisory_lock'); + expect(pool.events.some((event) => event.includes('drop table "posts"'))).toBe(true); + expect(pool.events.filter((event) => event.startsWith('pool:'))).toEqual([]); + expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); + expect(pool.events.at(-1)).toBe('release'); + }); + + test('a rollback that cannot reverse a row unlocks and gives the pin back', async () => { + client.on(/from x_migrations/, { rows: [ledgerRow({ id: '20260404000000_unknown' })] }); + const pool = pinnable(client); + + await expect(rollback({ migrations: [addPosts], client: pool.client })).rejects.toThrow( + 'X_MIGRATION_CONFLICT', + ); + + expect(pool.events.some((event) => event.includes('drop table "posts"'))).toBe(false); + expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); + expect(pool.events.at(-1)).toBe('release'); + }); + + // The framework's own deliberate loops declare themselves at source, so an N+1 detector reports + // the ones nobody argued for. A migration per transaction is the point, not a batch to be found. + test('the apply loop declares itself, and the ledger read before it does not', async () => { + client.on(/from x_migrations/, { rows: [] }); + client.on('insert into x_migrations', { affected: 1 }); + const witness = witnessed(client); + + await migrate({ + migrations: [addPosts], + appVersion: '1.5.0', + client: witness.client, + lock: false, + }); + + expect(reasonFor(witness, 'from x_migrations')).toBeUndefined(); + expect(reasonFor(witness, 'create table "posts"')).toContain('its own transaction'); + expect(reasonFor(witness, 'insert into x_migrations')).toContain('its own transaction'); + }); + + test('the rollback loop declares itself too, with its own reason', async () => { + client.on(/from x_migrations/, { rows: [ledgerRow()] }); + const witness = witnessed(client); + + await rollback({ migrations: [addPosts], client: witness.client, lock: false }); + + expect(reasonFor(witness, 'from x_migrations')).toBeUndefined(); + expect(reasonFor(witness, 'drop table "posts"')).toContain('newest first'); + expect(reasonFor(witness, 'delete from x_migrations')).toContain('newest first'); + }); + + test('rollback with lock: false takes no lock, for a private branch database', async () => { + client.on(/from x_migrations/, { rows: [ledgerRow()] }); + const pool = pinnable(client); + + const reverted = await rollback({ migrations: [addPosts], client: pool.client, lock: false }); + + expect(reverted).toEqual([addPosts.id]); + expect(client.texts.some((text) => text.includes('pg_advisory'))).toBe(false); + expect(pool.events[0]).toStartWith('pool:'); + expect(pool.events.filter((event) => event === 'reserve')).toHaveLength(1); + }); +}); diff --git a/packages/db/src/migrate.test.ts b/packages/db/src/migrate.test.ts index 22522a81..2a8b5832 100644 --- a/packages/db/src/migrate.test.ts +++ b/packages/db/src/migrate.test.ts @@ -1,6 +1,9 @@ +// Single responsibility: what `migrate()` and `rollback()` DO to the ledger — the forward path, +// the conflicts they refuse, the step count they validate, and how one script becomes one send per +// statement. Which session those statements run on is `migrate-pin.test.ts`. + import { beforeEach, describe, expect, test } from 'bun:test'; -import { type DbClient, type DbConnection, type ReservableClient, setDbClient } from './client'; -import { expectedQueryLoopReason } from './expected-loop'; +import { setDbClient } from './client'; import { createRecordingClient, type RecordingClient } from './fake'; import { type EntityDescriptionLike, generateMigration } from './generate'; import { @@ -37,98 +40,6 @@ beforeEach(() => { setDbClient(client); }); -const squash = (text: string): string => text.replace(/\s+/g, ' ').trim(); - -interface PinnablePool { - readonly client: ReservableClient; - /** `reserve`, `release`, and every statement tagged with the handle that ran it. */ - readonly events: readonly string[]; -} - -/** - * A pool whose pin is observable. The defect this pins is invisible to the recording client: the - * statement texts are identical whether the lock landed on the session that runs the migration or - * on whatever connection the pool lent for that one statement, and only the tag says which. - */ -function pinnable(inner: DbClient): PinnablePool { - const events: string[] = []; - const through = (tag: string): DbClient => ({ - query: (fragment) => { - events.push(`${tag}:${squash(fragment.text)}`); - return inner.query(fragment); - }, - one: (fragment) => { - events.push(`${tag}:${squash(fragment.text)}`); - return inner.one(fragment); - }, - execute: (fragment) => { - events.push(`${tag}:${squash(fragment.text)}`); - return inner.execute(fragment); - }, - }); - return { - events, - client: { - ...through('pool'), - reserve: async (): Promise => { - events.push('reserve'); - let held = true; - const release = (): void => { - if (!held) return; - held = false; - events.push('release'); - }; - return { ...through('pin'), release, [Symbol.dispose]: release }; - }, - }, - }; -} - -interface Witness { - readonly client: DbClient; - readonly statements: readonly { readonly text: string; readonly reason: string | undefined }[]; -} - -/** - * Every statement paired with the `expectedQueryLoop` reason in force when it was issued. The - * recording client cannot answer this: it never passes a funnel, so the observer never fires and - * the scope has to be read where the statement is sent. - */ -function witnessed(inner: DbClient): Witness { - const statements: { text: string; reason: string | undefined }[] = []; - const note = (text: string): void => { - statements.push({ text: squash(text), reason: expectedQueryLoopReason() }); - }; - return { - statements, - client: { - query: (fragment) => { - note(fragment.text); - return inner.query(fragment); - }, - one: (fragment) => { - note(fragment.text); - return inner.one(fragment); - }, - execute: (fragment) => { - note(fragment.text); - return inner.execute(fragment); - }, - }, - }; -} - -/** - * Fails when nothing matched instead of answering `undefined` for it. `find(...)?.reason` alone - * collapses two different facts into one value — "this statement ran outside every scope" and - * "this statement never ran" — and the `toBeUndefined()` assertions below are the load-bearing - * half of both test names, so a reworded ledger read would leave them passing on the wrong one. - */ -const reasonFor = (witness: Witness, needle: string): string | undefined => { - expect(witness.statements.map((statement) => statement.text).join(' | ')).toContain(needle); - return witness.statements.find((statement) => statement.text.includes(needle))?.reason; -}; - describe('migrate', () => { test('refuses and applies nothing when the ledger belongs to another app version', async () => { const foreign = ledgerRow({ id: '20260202000000_from_the_future', app_version: '1.6.0' }); @@ -335,127 +246,108 @@ describe('a multi-statement migration', () => { }); }); -describe('the migration advisory lock', () => { - test('the lock, the migration and the unlock run on one pinned session', async () => { - client.on(/from x_migrations/, { rows: [] }); - const pool = pinnable(client); - - await migrate({ migrations: [addPosts], appVersion: '1.5.0', client: pool.client }); - - expect(pool.events[0]).toBe('reserve'); - expect(pool.events[1]).toContain('pg_try_advisory_lock'); - expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); - expect(pool.events.at(-1)).toBe('release'); - // Not one statement on the pool: `pg_advisory_lock` is session-scoped, so work done on any - // other connection is not under the lock, and the unlock would answer `false` on a session - // that never took it. On `ROLE=migrate` (`max: 1`) there is no other connection to run on. - expect(pool.events.filter((event) => event.startsWith('pool:'))).toEqual([]); - expect(pool.events).toContain('pin:BEGIN'); - expect(pool.events).toContain('pin:COMMIT'); - expect(pool.events.some((event) => event.includes('create table "posts"'))).toBe(true); - expect(pool.events.filter((event) => event === 'reserve')).toHaveLength(1); - }); - - test('a refused ledger unlocks and gives the pin back', async () => { - const foreign = ledgerRow({ id: '20260202000000_from_the_future', app_version: '1.6.0' }); - client.on(/from x_migrations/, { rows: [foreign] }); - const pool = pinnable(client); +/** + * `steps` reaches `slice(0, steps)`, and a negative argument there is not "fewer" — it counts + * from the END, so `-1` selects every row but the newest and reverses 4 of 5 migrations. A + * rollback is the one operation whose mistakes are unrecoverable, so the count is validated + * before the lock is taken and before the ledger is read. + */ +describe('rollback validates its step count', () => { + const fiveRows = (): readonly LedgerRow[] => + ['a', 'b', 'c', 'd', 'e'].map((suffix, index) => + ledgerRow({ id: `2026010100000${index}_${suffix}` }), + ); - await expect( - migrate({ migrations: [addPosts], appVersion: '1.5.0', client: pool.client }), - ).rejects.toThrow('X_MIGRATION_CONFLICT'); + const migrationsFor = (rows: readonly LedgerRow[]): readonly Migration[] => + rows.map((row) => ({ ...addPosts, id: row.id, name: row.id })); - expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); - expect(pool.events.at(-1)).toBe('release'); - }); + // "Refused" alone is satisfied by a check that runs AFTER the lock and the ledger read, which is + // exactly the placement the fix exists to rule out — so each refusal also asserts that neither + // statement went out. Returns the offenders, so a failure names the statement that escaped. + const statementsBeforeTheRefusal = (): readonly string[] => + client.texts.filter( + (text) => text.includes('pg_try_advisory_lock') || text.includes('from x_migrations'), + ); - test("lock: false takes no lock, and the only pin left is the transaction's own", async () => { - client.on(/from x_migrations/, { rows: [] }); - const pool = pinnable(client); + test('a negative step count is refused, not read as "all but the newest"', async () => { + const rows = fiveRows(); + client.on(/from x_migrations/, { rows: [...rows] }); - await migrate({ - migrations: [addPosts], - appVersion: '1.5.0', - client: pool.client, - lock: false, - }); + const caught = await rollback({ migrations: migrationsFor(rows), steps: -1 }).catch( + (error: unknown) => error, + ); - expect(client.texts.some((text) => text.includes('pg_advisory'))).toBe(false); - // The ledger runs unpinned, so the one reservation belongs to `withTransaction`, not to a - // lock scope that was never opened. - expect(pool.events[0]).toStartWith('pool:'); - expect(pool.events.filter((event) => event === 'reserve')).toHaveLength(1); - expect(pool.events.indexOf('reserve')).toBe(pool.events.indexOf('pin:BEGIN') - 1); - expect(client.texts.some((text) => text.includes('create table "posts"'))).toBe(true); + expect((caught as { code: string }).code).toBe('X_INVARIANT'); + expect((caught as { cause: string }).cause).toContain('-1'); + expect((caught as { fix: string }).fix).toContain('rollback('); + // Nothing was reversed, and the lock was never taken nor the ledger read. + expect(client.texts.some((text) => text.includes('drop table'))).toBe(false); + expect(client.texts.some((text) => text.includes('delete from x_migrations'))).toBe(false); + expect(statementsBeforeTheRefusal()).toEqual([]); }); - test('rollback takes the same lock, on its own pinned session', async () => { - client.on(/from x_migrations/, { rows: [ledgerRow()] }); - const pool = pinnable(client); + test('zero is refused too — a rollback that reverses nothing is a typo, not an intent', async () => { + const rows = fiveRows(); + client.on(/from x_migrations/, { rows: [...rows] }); - const reverted = await rollback({ migrations: [addPosts], client: pool.client }); + const caught = await rollback({ migrations: migrationsFor(rows), steps: 0 }).catch( + (error: unknown) => error, + ); - expect(reverted).toEqual([addPosts.id]); - expect(pool.events[0]).toBe('reserve'); - expect(pool.events[1]).toContain('pg_try_advisory_lock'); - expect(pool.events.some((event) => event.includes('drop table "posts"'))).toBe(true); - expect(pool.events.filter((event) => event.startsWith('pool:'))).toEqual([]); - expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); - expect(pool.events.at(-1)).toBe('release'); + expect((caught as { code: string }).code).toBe('X_INVARIANT'); + expect(statementsBeforeTheRefusal()).toEqual([]); }); - test('a rollback that cannot reverse a row unlocks and gives the pin back', async () => { - client.on(/from x_migrations/, { rows: [ledgerRow({ id: '20260404000000_unknown' })] }); - const pool = pinnable(client); + test('a fractional step count is refused rather than truncated', async () => { + const rows = fiveRows(); + client.on(/from x_migrations/, { rows: [...rows] }); - await expect(rollback({ migrations: [addPosts], client: pool.client })).rejects.toThrow( - 'X_MIGRATION_CONFLICT', + const caught = await rollback({ migrations: migrationsFor(rows), steps: 1.5 }).catch( + (error: unknown) => error, ); - expect(pool.events.some((event) => event.includes('drop table "posts"'))).toBe(false); - expect(pool.events.at(-2)).toContain('pg_advisory_unlock'); - expect(pool.events.at(-1)).toBe('release'); + expect((caught as { code: string }).code).toBe('X_INVARIANT'); + expect(statementsBeforeTheRefusal()).toEqual([]); }); - // The framework's own deliberate loops declare themselves at source, so an N+1 detector reports - // the ones nobody argued for. A migration per transaction is the point, not a batch to be found. - test('the apply loop declares itself, and the ledger read before it does not', async () => { - client.on(/from x_migrations/, { rows: [] }); - client.on('insert into x_migrations', { affected: 1 }); - const witness = witnessed(client); + test('a positive integer still reverses exactly that many, newest first', async () => { + const rows = fiveRows(); + client.on(/from x_migrations/, { rows: [...rows] }); - await migrate({ - migrations: [addPosts], - appVersion: '1.5.0', - client: witness.client, - lock: false, - }); + const reverted = await rollback({ migrations: migrationsFor(rows), steps: 2 }); - expect(reasonFor(witness, 'from x_migrations')).toBeUndefined(); - expect(reasonFor(witness, 'create table "posts"')).toContain('its own transaction'); - expect(reasonFor(witness, 'insert into x_migrations')).toContain('its own transaction'); + expect(reverted).toEqual(['20260101000004_e', '20260101000003_d']); }); +}); - test('the rollback loop declares itself too, with its own reason', async () => { - client.on(/from x_migrations/, { rows: [ledgerRow()] }); - const witness = witnessed(client); +/** + * The audit's question is "does this build ship every migration the ledger records?", and the + * version is the ANSWER's detail, never part of the question. Gating on `app_version !== + * appVersion` made the audit blind in exactly the environment that deletes migrations: every + * development build resolves to `dev` (`runningAppVersion()`), so a migration applied by an + * earlier `dev` build and since deleted passed the audit, and `expectedSchema` then filtered its + * table out of the drift comparison — `ok: true` against a database that still has the table. + */ +describe('auditLedger refuses a migration this build does not ship', () => { + const gone = ledgerRow({ id: '20260202000000_deleted', app_version: 'dev' }); - await rollback({ migrations: [addPosts], client: witness.client, lock: false }); + test('even when the row was applied by a build naming the same version', () => { + let thrown: unknown; + try { + auditLedger([gone], [addPosts], 'dev'); + } catch (error) { + thrown = error; + } - expect(reasonFor(witness, 'from x_migrations')).toBeUndefined(); - expect(reasonFor(witness, 'drop table "posts"')).toContain('newest first'); - expect(reasonFor(witness, 'delete from x_migrations')).toContain('newest first'); + const error = thrown as { code: string; cause: string; fix: string }; + expect(error.code).toBe('X_MIGRATION_CONFLICT'); + expect(error.cause).toContain('20260202000000_deleted'); + // The version moved into the cause; it is still the fact an operator acts on. + expect(error.cause).toContain('"dev"'); + expect(error.fix).toContain("delete from x_migrations where id = '20260202000000_deleted'"); }); - test('rollback with lock: false takes no lock, for a private branch database', async () => { - client.on(/from x_migrations/, { rows: [ledgerRow()] }); - const pool = pinnable(client); - - const reverted = await rollback({ migrations: [addPosts], client: pool.client, lock: false }); - - expect(reverted).toEqual([addPosts.id]); - expect(client.texts.some((text) => text.includes('pg_advisory'))).toBe(false); - expect(pool.events[0]).toStartWith('pool:'); - expect(pool.events.filter((event) => event === 'reserve')).toHaveLength(1); + test('a ledger this build ships in full still passes', () => { + expect(() => auditLedger([ledgerRow()], [addPosts], 'dev')).not.toThrow(); }); }); diff --git a/packages/db/src/migrate.ts b/packages/db/src/migrate.ts index 8e975554..1da79c08 100644 --- a/packages/db/src/migrate.ts +++ b/packages/db/src/migrate.ts @@ -11,7 +11,7 @@ import { isReservable, poolProfileFor, } from './client'; -import { migrateConcurrent, migrationConflict } from './errors'; +import { migrateConcurrent, migrationConflict, rollbackStepsInvalid } from './errors'; import { expectedQueryLoop } from './expected-loop'; import type { SchemaDescription } from './introspect'; import { raw, sql } from './sql'; @@ -149,7 +149,13 @@ export function auditLedger( ): void { const known = new Map(migrations.map((migration) => [migration.id, migration])); - const foreign = ledger.filter((row) => !known.has(row.id) && row.app_version !== appVersion); + // The predicate is "this build does not ship it" and NOTHING else. It used to also require + // `row.app_version !== appVersion`, which switched the audit off wherever the two agree — + // `runningAppVersion()` answers `dev` for every development build, so a migration applied by an + // earlier `dev` build and since deleted was invisible here, and `expectedSchema` then dropped + // its table from the drift comparison: `ok: true` against a database that still has the table. + // The version is a detail of the ANSWER, so it moved into the cause. + const foreign = ledger.filter((row) => !known.has(row.id)); const first = foreign[0]; if (first !== undefined) { throw migrationConflict( @@ -374,6 +380,7 @@ export async function migrate(options: MigrateOptions): Promise export interface RollbackOptions { readonly migrations: readonly Migration[]; readonly client?: DbClient | undefined; + /** How many applied migrations to reverse, newest first. A positive integer; defaults to 1. */ readonly steps?: number | undefined; /** Skip the advisory lock. Only `x db branch` does this, against a private database. */ readonly lock?: boolean | undefined; @@ -387,6 +394,9 @@ export interface RollbackOptions { export async function rollback(options: RollbackOptions): Promise { const client = options.client ?? baseClient(); const steps = options.steps ?? 1; + // Before the lock and before the ledger read: `slice(0, -1)` is "all but the newest", not + // "one fewer", so an unvalidated count reverses migrations nobody asked about. + if (!Number.isSafeInteger(steps) || steps < 1) throw rollbackStepsInvalid(steps); const lockTimeoutMs = migrationLockTimeoutMs(options.lockTimeoutMs); const known = new Map(options.migrations.map((migration) => [migration.id, migration])); diff --git a/packages/flags/src/registry.test.ts b/packages/flags/src/registry.test.ts index 185215d5..96854f8a 100644 --- a/packages/flags/src/registry.test.ts +++ b/packages/flags/src/registry.test.ts @@ -82,4 +82,34 @@ describe('unit · applyFlagSnapshot', () => { ); expect(thrown).toBeUltimateError('X_FLAG_TARGETING_INVALID'); }); + + test('a refused payload lands nothing — not the keys ahead of the bad one', () => { + // The doc block says a bad targeting protects the fleet. It did not: the loop wrote each key + // as it validated it, so `a.first` had already moved when `m.middle` threw, and the caller — + // a poller or a realtime channel — sees only the throw and no record of the half that landed. + permanent('a.first'); + permanent('m.middle'); + const thrown = caught(() => + applyFlagSnapshot({ + 'a.first': { default: false, rollout: 10 }, + 'm.middle': { default: false, rollout: 0.5 }, + }), + ); + expect(thrown).toBeUltimateError('X_FLAG_TARGETING_INVALID'); + expect(allFlags().map((flag) => flag.targeting.rollout)).toEqual([undefined, undefined]); + }); + + test('an unknown key ahead of a bad one still does not land the valid ones behind it', () => { + permanent('a.first'); + permanent('z.last'); + const thrown = caught(() => + applyFlagSnapshot({ + 'shipped.tomorrow': { default: true }, + 'a.first': { default: false, rollout: 10 }, + 'z.last': { default: false, rollout: 0.5 }, + }), + ); + expect(thrown).toBeUltimateError('X_FLAG_TARGETING_INVALID'); + expect(allFlags()[0]?.targeting.rollout).toBe(undefined); + }); }); diff --git a/packages/flags/src/registry.ts b/packages/flags/src/registry.ts index 80f4734c..5d860b60 100644 --- a/packages/flags/src/registry.ts +++ b/packages/flags/src/registry.ts @@ -5,7 +5,7 @@ import { flagDuplicate, flagUnknown } from './errors'; import type { Flag, FlagDef } from './flag'; import { toFlag, withTargeting } from './flag'; -import type { FlagTargeting } from './targeting'; +import { assertTargeting, type FlagTargeting } from './targeting'; const flags = new Map(); @@ -54,11 +54,16 @@ export interface SnapshotResult { * kill switch that refuses to land because the payload also mentioned tomorrow's flag is a kill * switch that does not work on the day it is needed. A bad *targeting* still throws — landing a * `rollout: 0.5` would silently switch a feature off for everyone. + * + * **Two passes, because the throw above is only worth anything if nothing landed.** Validating and + * writing in one loop retargeted every key ahead of the bad one and then threw, so the caller — a + * poller, a job or a realtime channel — saw a failure and had no record that half the fleet's + * flags had already moved. Nothing here awaits, so no other code observes the gap between passes. */ export function applyFlagSnapshot( snapshot: Readonly>, ): SnapshotResult { - const applied: string[] = []; + const declared: [string, Flag, FlagTargeting][] = []; const unknown: string[] = []; for (const [key, targeting] of Object.entries(snapshot)) { const flag = flags.get(key); @@ -66,6 +71,11 @@ export function applyFlagSnapshot( unknown.push(key); continue; } + assertTargeting(key, targeting); + declared.push([key, flag, targeting]); + } + const applied: string[] = []; + for (const [key, flag, targeting] of declared) { flags.set(key, withTargeting(flag, targeting)); applied.push(key); } diff --git a/packages/i18n/CLAUDE.md b/packages/i18n/CLAUDE.md index dbfc80dc..a0cca509 100644 --- a/packages/i18n/CLAUDE.md +++ b/packages/i18n/CLAUDE.md @@ -29,6 +29,14 @@ Imported by every package that renders a string. for. Same reach `catalog.ts` shuts off by nesting into null-prototype nodes; never reintroduce either. The `interpolate` fast path must test **both** braces: `}}` un-escapes with no `{` in sight, and a `{`-only check gave one escape two meanings. +- **A catalog is read through `Object.hasOwn`, never a raw index, and every catalog this package + builds is `Object.create(null)`** — `flattenCatalog`, `mergeCatalogs` and `nestCatalog` all are. + On a `{}` catalog `catalog['valueOf']` resolved to the INHERITED function, so `t('valueOf')` + threw inside `interpolate`, `t('constructor')` returned a function through a signature typed + `string`, and `isMiss(t('__proto__'))` threw on an object — all three reachable wherever a key + travels as data (`t(row.labelKey)`). Both halves are load-bearing: the guard in `translator.ts` + makes it true of a catalog this package did not build, the null prototype makes `__proto__` an + ordinary key instead of one the setter silently swallows. Never reintroduce either. - Plural selection is `Intl.PluralRules`. Never `count === 1`. Variants are underscore suffixes on the leaf — a CLDR category (`_zero _one _two _few _many _other`), or `n` / `n_plural` as the two-form shortcut; pair `n_one` with `n_other`, never with `n_plural`. Never a nested diff --git a/packages/i18n/src/catalog.test.ts b/packages/i18n/src/catalog.test.ts index d4effc3d..47ac9f18 100644 --- a/packages/i18n/src/catalog.test.ts +++ b/packages/i18n/src/catalog.test.ts @@ -90,6 +90,41 @@ describe('flattenCatalog', () => { }); }); +describe('prototype safety', () => { + test('flattenCatalog keeps __proto__ as a key instead of dropping it on the setter', () => { + // `JSON.parse`, never an object literal: `{ __proto__: 'Hello' }` in source sets the + // prototype, so it could not reproduce what a catalog file on disk carries. + const flat = loadCatalog(JSON.parse('{"__proto__":"Hello","greeting":"Hi"}')); + + expect(Object.keys(flat).sort()).toEqual(['__proto__', 'greeting']); + expect(ownValue(flat, '__proto__')).toBe('Hello'); + expect(Object.getPrototypeOf(flat)).toBeNull(); + expect(Object.hasOwn(Object.prototype, 'greeting')).toBe(false); + }); + + test('a flat catalog reads absent for every Object.prototype member', () => { + const flat = loadCatalog({ greeting: 'Hi' }); + const inherited = ['valueOf', 'constructor', 'toString', 'hasOwnProperty', '__proto__']; + + // A RAW index, deliberately: that is what a consumer writes, and on a `{}` catalog every one + // of these answered an inherited function or object rather than `undefined`. + const read = inherited.map((key) => (flat as Record)[key]); + expect(read).toEqual([undefined, undefined, undefined, undefined, undefined]); + expect(inherited.filter((key) => Object.hasOwn(flat, key))).toEqual([]); + }); + + test('mergeCatalogs carries the null prototype through', () => { + const merged = mergeCatalogs( + loadCatalog(JSON.parse('{"__proto__":"Hello"}')), + loadCatalog({ greeting: 'Hi' }), + ); + + expect(Object.getPrototypeOf(merged)).toBeNull(); + expect(Object.keys(merged).sort()).toEqual(['__proto__', 'greeting']); + expect(ownValue(merged, '__proto__')).toBe('Hello'); + }); +}); + describe('mergeCatalogs', () => { test('later catalogs win so an app can override framework strings', () => { const framework = flattenCatalog({ errors: { notFound: { title: 'Page not found' } } }); diff --git a/packages/i18n/src/catalog.ts b/packages/i18n/src/catalog.ts index 243cbcb0..de271a16 100644 --- a/packages/i18n/src/catalog.ts +++ b/packages/i18n/src/catalog.ts @@ -16,9 +16,14 @@ const KEY_SEGMENT = /^[A-Za-z0-9_-]+$/; /** * Depth-first flatten. Throws `X_CATALOG_INVALID` on a non-string leaf (arrays and * numbers are the two mistakes translators actually make) or a duplicate flat key. + * + * `Object.create(null)`, for the reason `nestCatalog` states below: a catalog is untrusted input + * read off disk. On a `{}` literal `out['__proto__'] = 'Hello'` hits the `Object.prototype` SETTER + * and the key vanishes silently, and every other member of `Object.prototype` reads back as + * present when it is not. */ export function flattenCatalog(source: NestedCatalog, prefix = ''): Catalog { - const flat: Record = {}; + const flat = Object.create(null) as Record; walk(source, prefix, flat); return flat; } @@ -89,7 +94,9 @@ export function nestCatalog(catalog: Catalog): NestedCatalog { * app can override `errors.notFound.title` without forking the framework catalog. */ export function mergeCatalogs(...catalogs: readonly Catalog[]): Catalog { - const merged: Record = {}; + // Null-prototyped like every catalog this package builds — a merge must not hand back an + // object on which `catalog['toString']` reads as a function. + const merged = Object.create(null) as Record; for (const catalog of catalogs) { for (const key of Object.keys(catalog)) { const value = catalog[key]; diff --git a/packages/i18n/src/translator.test.ts b/packages/i18n/src/translator.test.ts index 5e6092cc..43edf4d2 100644 --- a/packages/i18n/src/translator.test.ts +++ b/packages/i18n/src/translator.test.ts @@ -35,6 +35,36 @@ describe('createTranslator', () => { expect(isMiss(t('nav.home'))).toBe(false); }); + test('an Object.prototype member is a miss, not the inherited value', () => { + // A `{}`-literal catalog, because that is what a raw index would read through: the whole + // point is that the translator no longer depends on who built the catalog. + const t = createTranslator({ greeting: 'Hi' }, 'en'); + + // Reached wherever a key travels as data — `t(row.labelKey)`, `t(titleKey)`. A raw index + // returned `Object.prototype.valueOf` here and `interpolate` threw on a non-string. + expect(t('valueOf', { n: 1 })).toBe('⟦valueOf⟧'); + expect(t('constructor')).toBe('⟦constructor⟧'); + expect(t('__proto__')).toBe('⟦__proto__⟧'); + expect(t('toString')).toBe('⟦toString⟧'); + expect(t('hasOwnProperty')).toBe('⟦hasOwnProperty⟧'); + // `isMiss` is the documented probe, and it threw on the object `__proto__` resolved to. + expect([t('valueOf'), t('constructor'), t('__proto__')].every(isMiss)).toBe(true); + + // The probes agree with the render — no key, no template. + expect(t.has('constructor')).toBe(false); + expect(t.raw('constructor')).toBeUndefined(); + expect(t.raw('__proto__')).toBeUndefined(); + expect(t.raw('valueOf')).toBeUndefined(); + // And a real key still renders. + expect(t('greeting')).toBe('Hi'); + }); + + test('plural selection cannot resolve onto a prototype member either', () => { + const t = createTranslator(flattenCatalog({ greeting: 'Hi' }), 'en'); + expect(t('valueOf', { count: 1 })).toBe('⟦valueOf⟧'); + expect(t('constructor', { count: 3 })).toBe('⟦constructor⟧'); + }); + test('interpolates and reports a missing variable loudly too', () => { const t = createTranslator(en, 'en'); expect(t('greeting', { name: 'Ada', count: 1 })).toBe('Hi Ada, you have 1 message'); diff --git a/packages/i18n/src/translator.ts b/packages/i18n/src/translator.ts index f19749b9..d942a537 100644 --- a/packages/i18n/src/translator.ts +++ b/packages/i18n/src/translator.ts @@ -64,7 +64,13 @@ export function createTranslator(catalog: Catalog, locale: Locale = DEFAULT_LOCA }; const translate = (key: string, vars?: TranslateVars): string => { - const template = catalog[resolveKey(key, vars)]; + // Through `hasExact`, never a raw index: a key travels as data (`t(row.labelKey)`), and on a + // `{}`-prototyped catalog `catalog['valueOf']` resolves to the INHERITED function instead of + // reading as absent — so `interpolate` threw on a non-string, `t('constructor')` returned a + // function through a signature typed `string`, and `isMiss(t('__proto__'))` threw on an object. + // Catalogs are null-prototyped now; this guard is what makes that true of any catalog. + const resolved = resolveKey(key, vars); + const template = hasExact(resolved) ? catalog[resolved] : undefined; if (template === undefined) return `⟦${key}⟧`; return vars === undefined ? template : interpolate(template, vars); }; @@ -75,7 +81,7 @@ export function createTranslator(catalog: Catalog, locale: Locale = DEFAULT_LOCA hasExact(`${key}_other`) || hasExact(`${key}_plural`) || hasExact(`${key}_one`), - raw: (key: string): string | undefined => catalog[key], + raw: (key: string): string | undefined => (hasExact(key) ? catalog[key] : undefined), keys: (): string[] => Object.keys(catalog).sort(), locale, }); diff --git a/packages/money/CLAUDE.md b/packages/money/CLAUDE.md index 4f0a5ad6..c977a7e0 100644 --- a/packages/money/CLAUDE.md +++ b/packages/money/CLAUDE.md @@ -79,6 +79,13 @@ shape is still additive and this is still a minor version. `USD/EUR: 0.92` names 23/25, so EUR→USD is exactly 25/23, where `1 / 0.92` is a double whose own decimal spelling rounds a large amount one minor unit low. `rate` stays the readable number the audit trail records; `convert` scales by `ratio` whenever the provider supplied one. +- **Never cache an `Intl` formatter on a raw caller string.** `locale` arrives from + `Accept-Language`, so an unbounded `Map` keyed on it is memory the client chooses: 20,000 valid + `en-US-x-*` tags through `formatMoney` retained +55.1 MB of RSS, at ~2.7 KB per + `Intl.NumberFormat`. Both halves, always — `canonicalLocale` for the key, `cachedFormatter` for + the bound, both `@ultimat3/core`'s and shared with `@ultimat3/time` (tier 1 may not import + sideways, so the mechanism lives a tier down rather than twice). Every formatter in `format.ts` + goes through that pair; a `new Intl.NumberFormat` outside one is the bug written again. - **One place decides a sign.** `formatMoney` is `formatMoneyParts` joined, and `accounting` reaches `Intl` as `currencySign` — so the locale places the minus and picks the parenthesised form, and a UI styling the parts cannot render a different format from the label beside it. diff --git a/packages/money/src/format.test.ts b/packages/money/src/format.test.ts index 3b5eb423..50b0ac6b 100644 --- a/packages/money/src/format.test.ts +++ b/packages/money/src/format.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from 'bun:test'; +import { MAX_CACHED_FORMATTERS } from '@ultimat3/core'; import { formatMoney, formatMoneyDecimal, formatMoneyParts } from './format'; import { fromDecimal, money } from './money'; @@ -98,3 +99,54 @@ describe('one place decides the sign', () => { ); }); }); + +describe('the formatter cache', () => { + test('is bounded — the oldest locale is evicted, not kept for the life of the process', () => { + // `locale` is whatever `Accept-Language` sent. Keyed raw into an unbounded `Map`, 20,000 + // distinct-but-valid tags (`en-US-x-a0` …) through this function retained +55.1 MB of RSS + // after `Bun.gc(true)` — memory the client chooses, at ~2.7 KB per `Intl.NumberFormat`. + // The heap does not show it (ICU allocates natively), so the bound is asserted where it is + // decided: the first key in is the first key out, and asking for it again rebuilds it. + const amount = money(129900, 'EUR'); + const built: unknown[] = []; + const real = Intl.NumberFormat; + Intl.NumberFormat = new Proxy(real, { + construct(target, args, newTarget) { + built.push(args[0]); + return Reflect.construct(target, args, newTarget); + }, + }); + try { + formatMoney(amount, 'en-US-x-oldest'); + for (let index = 0; index < MAX_CACHED_FORMATTERS; index += 1) { + formatMoney(amount, `en-US-x-a${index}`); + } + built.length = 0; + formatMoney(amount, 'en-US-x-oldest'); + expect(built).toEqual(['en-US-x-oldest']); + } finally { + Intl.NumberFormat = real; + } + }); + + test('a locale still inside the cap is answered from the cache, never rebuilt', () => { + const amount = money(129900, 'EUR'); + const built: unknown[] = []; + const real = Intl.NumberFormat; + formatMoney(amount, 'en-US-x-warm'); + Intl.NumberFormat = new Proxy(real, { + construct(target, args, newTarget) { + built.push(args[0]); + return Reflect.construct(target, args, newTarget); + }, + }); + try { + // The canonical tag is the key AND what reaches `Intl`, so a header spelling one locale + // three ways does not mint three permanent formatters. + expect(formatMoney(amount, 'EN-us-X-WARM')).toBe(formatMoney(amount, 'en-US-x-warm')); + expect(built).toEqual([]); + } finally { + Intl.NumberFormat = real; + } + }); +}); diff --git a/packages/money/src/format.ts b/packages/money/src/format.ts index d270c272..0c1e856f 100644 --- a/packages/money/src/format.ts +++ b/packages/money/src/format.ts @@ -3,6 +3,7 @@ * JPY renders without decimals and KWD with three, without a per-locale special case. */ +import { cachedFormatter, canonicalLocale } from '@ultimat3/core'; import { exponentOf } from './currency'; import { type Money, toDecimalNumber } from './money'; import { moneyScale } from './scale'; @@ -73,19 +74,40 @@ export function currencySymbol(currency: string, locale: string): string { /** Digits only, no symbol — for editable inputs and CSV exports. */ export function formatMoneyDecimal(amount: Money, locale: string): string { const digits = moneyScale(amount); - return new Intl.NumberFormat(locale, { - style: 'decimal', - minimumFractionDigits: digits, - maximumFractionDigits: digits, - useGrouping: false, - }).format(toDecimalNumber(amount)); + const tag = canonicalTag(locale); + // Through the same cache as `formatterFor`, for the same reason: this took the caller's raw + // locale too, and a second way to build a formatter in one file is a second place to forget. + return cachedFormatter( + decimalCache, + `${tag}|${digits}`, + () => + new Intl.NumberFormat(tag, { + style: 'decimal', + minimumFractionDigits: digits, + maximumFractionDigits: digits, + useGrouping: false, + }), + ).format(toDecimalNumber(amount)); } +/** + * A tag `Intl` cannot parse falls through unchanged, so the `Intl.NumberFormat` constructor still + * raises it — this seam decides a cache key, never whether a locale is acceptable. + */ +const canonicalTag = (locale: string): string => canonicalLocale(locale) ?? locale; + const cache = new Map(); +const decimalCache = new Map(); /** * `scale` is the amount's own, not the currency's: rendering $0.000002 with two digits shows * `$0.00`, which is the sub-cent bug back again, in the one place a human would read it. + * + * **Canonically keyed and hard-capped, because `locale` arrives from `Accept-Language`.** Keyed + * raw into an unbounded `Map`, 20,000 valid-but-distinct tags (`en-US-x-a0` …) retained 55 MB — + * memory the client chooses. `canonicalLocale` collapses `EN-us` and `en-US` onto one key and + * `cachedFormatter` caps the rest; neither half is sufficient alone, which is why both come from + * the one place `@ultimat3/time` reads them from too. */ function formatterFor( currency: string, @@ -96,13 +118,14 @@ function formatterFor( const digits = options.fractionDigits ?? (options.trimZeroFraction === true ? undefined : exponent); const sign = options.accounting === true ? 'accounting' : 'standard'; + const tag = canonicalTag(locale); // `exponent` is in the key because it stopped being derivable from `currency` the moment it // started coming from the amount's own scale. On the `trimZeroFraction` path `digits` is // `undefined`, so without it every scale of one currency shared a formatter: format 12.99 EUR // first and 12.990001 EUR then rendered as `12,99 €` — the sub-cent bug back, silently, in the // one place a human reads the number. const key = [ - locale, + tag, currency, options.display ?? 'symbol', digits ?? 'auto', @@ -110,19 +133,19 @@ function formatterFor( options.grouping ?? 'auto', sign, ].join('|'); - const cached = cache.get(key); - if (cached !== undefined) return cached; - - const formatter = new Intl.NumberFormat(locale, { - style: 'currency', - currency, - currencyDisplay: options.display ?? 'symbol', - currencySign: sign, - ...(digits === undefined - ? { minimumFractionDigits: 0, maximumFractionDigits: exponent } - : { minimumFractionDigits: digits, maximumFractionDigits: digits }), - ...(options.grouping === 'never' ? { useGrouping: false } : {}), - }); - cache.set(key, formatter); - return formatter; + return cachedFormatter( + cache, + key, + () => + new Intl.NumberFormat(tag, { + style: 'currency', + currency, + currencyDisplay: options.display ?? 'symbol', + currencySign: sign, + ...(digits === undefined + ? { minimumFractionDigits: 0, maximumFractionDigits: exponent } + : { minimumFractionDigits: digits, maximumFractionDigits: digits }), + ...(options.grouping === 'never' ? { useGrouping: false } : {}), + }), + ); } diff --git a/packages/schema/src/coerce.test.ts b/packages/schema/src/coerce.test.ts index 0b7312a1..b1362e39 100644 --- a/packages/schema/src/coerce.test.ts +++ b/packages/schema/src/coerce.test.ts @@ -11,6 +11,46 @@ const listPosts = t.object({ }); describe('coerceQuery', () => { + test('an Object.prototype member is never read as a submitted value', () => { + // A schema is allowed to declare a field called `toString` — a client that never sent one + // must not have the INHERITED member coerced in as if it had, and a function must never + // reach validation as a value the caller supplied. + const input = t.object({ + toString: t.optional(t.string), + valueOf: t.optional(t.number), + constructor: t.optional(t.string), + page: t.number.int().default(1), + }); + + const fromSearchParams = coerceQuery(input, new URLSearchParams('page=2')); + const fromRecord = coerceQuery(input, { page: '2' }); + + for (const coerced of [fromSearchParams, fromRecord]) { + expect(Object.hasOwn(coerced, 'toString')).toBe(false); + expect(Object.hasOwn(coerced, 'valueOf')).toBe(false); + expect(Object.hasOwn(coerced, 'constructor')).toBe(false); + expect(coerced['page']).toBe(2); + } + }); + + test("coerceNode's object branch coerces own properties only", () => { + const node = t.object({ toString: t.optional(t.string), n: t.optional(t.number) }).node; + const coerced = coerceNode(node, { n: '2' }) as Record; + + // `{ ...source }` already drops what a prototype carries; the `in` check put it back. + expect(Object.hasOwn(coerced, 'toString')).toBe(false); + expect(coerced['n']).toBe(2); + }); + + test('a __proto__ query key is data, not a prototype swap', () => { + const input = t.object({ page: t.number.int().default(1) }); + const coerced = coerceQuery(input, new URLSearchParams('__proto__=polluted&page=2')); + + expect(Object.getOwnPropertyDescriptor(coerced, '__proto__')?.value).toBe('polluted'); + expect(coerced['page']).toBe(2); + expect(Object.hasOwn(Object.prototype, 'polluted')).toBe(false); + }); + test('turns a query string into something the schema accepts', () => { const query = new URLSearchParams('page=3&live=yes&tags=alpha&tags=beta&since=2026-07-26'); const coerced = coerceQuery(listPosts, query); diff --git a/packages/schema/src/coerce.ts b/packages/schema/src/coerce.ts index 3595037d..86893141 100644 --- a/packages/schema/src/coerce.ts +++ b/packages/schema/src/coerce.ts @@ -81,7 +81,10 @@ export function coerceNode(node: SchemaNode, raw: unknown): unknown { const source = raw as Record; const out: Record = { ...source }; for (const [key, child] of Object.entries(node.properties)) { - if (key in source) out[key] = coerceNode(child, source[key]); + // `Object.hasOwn`, never `key in source`: `{ ...source }` above already dropped what a + // prototype carries, so an `in` check put it back — a schema field named `toString` or + // `valueOf` was coerced from the INHERITED member and handed validation a function. + if (Object.hasOwn(source, key)) out[key] = coerceNode(child, source[key]); } return out; } @@ -111,11 +114,17 @@ export function coerceNode(node: SchemaNode, raw: unknown): unknown { } } +/** + * `Object.create(null)`, for the reason `@ultimat3/http`'s `parseQuery` already uses one: this + * record is built from caller-controlled keys. On a `{}` literal `out['__proto__'] = …` hits the + * prototype accessor instead of declaring a key, and every member of `Object.prototype` reads + * back as present when the client sent nothing. + */ function toRecord(source: QuerySource): Record { + const out = Object.create(null) as Record; if (!(source instanceof URLSearchParams)) { - return { ...source }; + return Object.assign(out, source); } - const out: Record = {}; for (const key of new Set(source.keys())) { const all = source.getAll(key); out[key] = all.length > 1 ? all : (all[0] as string); @@ -134,7 +143,8 @@ export function coerceQuery(schema: unknown, source: QuerySource): Record = { ...record }; for (const [key, child] of Object.entries(node.properties)) { - if (!(key in record)) continue; + // See `toRecord`: a declared property is coerced only when the caller actually sent it. + if (!Object.hasOwn(record, key)) continue; const raw = record[key]; out[key] = coerceNode(child, child.kind === 'array' ? (raw ?? []) : normaliseSingle(raw)); } diff --git a/packages/schema/src/json-schema.test.ts b/packages/schema/src/json-schema.test.ts index fcb80b02..3722fe8e 100644 --- a/packages/schema/src/json-schema.test.ts +++ b/packages/schema/src/json-schema.test.ts @@ -155,6 +155,20 @@ describe('toJsonSchema', () => { } }); + test('a provider supplying toJsonSchema is not an alternative to introspect', () => { + configureSchemaProvider({ + vendor: 'phantom', + t: builtinT, + // @ts-expect-error — `SchemaProvider` has no `toJsonSchema` member. The doc clause that + // said `introspect` could be omitted "if the provider also supplies toJsonSchema" + // described an API that never existed: this path throws on every OpenAPI and MCP + // projection. If the member is ever really added, this line stops erroring and fails. + toJsonSchema: () => ({ type: 'object' }), + }); + + expect(() => toJsonSchema({ notASchema: true })).toThrow(/X_SCHEMA_UNSUPPORTED/); + }); + test('a swapped provider actually backs t', () => { let calls = 0; configureSchemaProvider({ diff --git a/packages/schema/src/provider.ts b/packages/schema/src/provider.ts index 75dcc094..2f8ae6ed 100644 --- a/packages/schema/src/provider.ts +++ b/packages/schema/src/provider.ts @@ -11,8 +11,10 @@ export interface SchemaProvider { readonly vendor: string; readonly t: TNamespace; /** - * Return the IR for one of this provider's schemas. Required for OpenAPI and MCP tool - * schemas; omit it only if the provider also supplies `toJsonSchema`. + * Return the IR for one of this provider's schemas. Required for OpenAPI, MCP tool schemas + * and the admin form generator: `toJsonSchema()` calls it unconditionally and throws + * `X_SCHEMA_UNSUPPORTED` without it. There is no second way to describe a schema — the IR is + * the one projection surface, and every generator reads it. */ introspect?(schema: unknown): SchemaNode | undefined; } diff --git a/packages/seo/CLAUDE.md b/packages/seo/CLAUDE.md index 341bf134..1d8893a1 100644 --- a/packages/seo/CLAUDE.md +++ b/packages/seo/CLAUDE.md @@ -28,6 +28,17 @@ Tier 1. May import `@ultimat3/core`, `@ultimat3/schema`, `@ultimat3/i18n`. Nothi over the route manifest and the build's own stats, throwing `@ultimat3/render`'s `X_BUDGET_EXCEEDED`. seo is tier 1 and cannot see a build's bytes, so it was never the package that could answer. `errors.test.ts` pins the code set, so re-adding one is a failing test. +- **A length bound with no enforcer does not ship.** `DESCRIPTION_MIN_LENGTH` (50) sat in + `meta.ts` under the comment "validate.ts enforces it", was re-exported from `index.ts`, and no + validator anywhere read it — a 10-character description passed the gate the constant claimed to + fail. Deleted `As of 2026-08`, comment included; `validateMeta` enforces maxima only. Adding a + minimum back means adding the check AND a new `X_SEO_*` code in the same change. + `meta.test.ts` pins the exported `*_LENGTH` set, so a bound with no enforcer is a failing test. +- **The `` fallback is the LARGEST usable width, chosen with `Math.max`.** + `usableWidths` preserves the CALLER's order, so `widths[widths.length - 1]` was the largest only + because `DEFAULT_WIDTHS` happens to ascend — `widths: [1200, 640]` handed every browser without + `srcset` support the 640 variant of a 1200-wide image. Never re-derive it from position, and + never sort inside `usableWidths`: the `srcset` order is the caller's to choose. - **Errors name the file, not the URL.** `RouteRecord.file` is in every cause and every fix; an agent must be able to open the source without guessing. - **Fail closed, and core reads the key.** `isIndexable()` is `environment === 'production'` and nothing else — `staging`, a laptop, a typo and an unset variable all disallow. `ULTIMATE_ENV` has diff --git a/packages/seo/src/images.test.ts b/packages/seo/src/images.test.ts index 0b5dc825..cb586af3 100644 --- a/packages/seo/src/images.test.ts +++ b/packages/seo/src/images.test.ts @@ -30,6 +30,27 @@ describe('responsiveImage', () => { expect(responsiveImage(INPUT).img.srcset).not.toContain('1536w'); }); + test('the no-srcset fallback is the LARGEST width, whatever order the caller gave', () => { + // `widths[widths.length - 1]` was the largest only because `DEFAULT_WIDTHS` happens to + // ascend; `usableWidths` preserves the caller's order, so a descending list handed every + // browser without `srcset` support the SMALLEST variant of a full-width image. + const descending = responsiveImage(INPUT, { widths: [1200, 640] }); + const ascending = responsiveImage(INPUT, { widths: [640, 1200] }); + + expect(descending.img.src).toContain(`${IMAGE_QUERY_KEYS.width}=1200`); + expect(descending.img.src).toBe(ascending.img.src); + // The srcset still carries the caller's order — only the fallback is chosen by size. + expect(descending.img.srcset).toBe('/img/hero.jpg?w=1200 1200w, /img/hero.jpg?w=640 640w'); + }); + + test('the fallback never upscales past the intrinsic width', () => { + // `usableWidths` drops 1920 and appends the intrinsic width, so the largest usable width + // is 1200 — `Math.max` over the usable list, never over what the caller asked for. + expect(responsiveImage(INPUT, { widths: [1920, 320] }).img.src).toContain( + `${IMAGE_QUERY_KEYS.width}=1200`, + ); + }); + test('a priority image is eager with high fetch priority', () => { const image = responsiveImage({ ...INPUT, priority: true }); expect(image.img.loading).toBe('eager'); diff --git a/packages/seo/src/images.ts b/packages/seo/src/images.ts index 9b707716..f5a44961 100644 --- a/packages/seo/src/images.ts +++ b/packages/seo/src/images.ts @@ -176,6 +176,11 @@ export function usableWidths(intrinsic: number, widths: readonly number[]): read return usable.includes(intrinsic) ? usable : [...usable, intrinsic]; } +/** The widest candidate, or `undefined` for an empty list — never `Math.max()`'s `-Infinity`. */ +function largestOf(widths: readonly number[]): number | undefined { + return widths.length === 0 ? undefined : Math.max(...widths); +} + export function srcsetFor( input: ImageInput, widths: readonly number[], @@ -212,7 +217,10 @@ export function responsiveImage( return { sources, img: { - src: urlFor(input.src, widths[widths.length - 1] ?? input.width, undefined), + // `Math.max`, not the last element: `usableWidths` preserves the CALLER's order, so the + // tail was the largest only because `DEFAULT_WIDTHS` happens to ascend. `widths: [1200, 640]` + // handed every no-`srcset` browser the 640 variant of a 1200-wide image. + src: urlFor(input.src, largestOf(widths) ?? input.width, undefined), srcset: srcsetFor(input, widths, undefined, urlFor), sizes, alt: input.alt, diff --git a/packages/seo/src/index.ts b/packages/seo/src/index.ts index 5335cf1b..b8b34d48 100644 --- a/packages/seo/src/index.ts +++ b/packages/seo/src/index.ts @@ -84,7 +84,6 @@ export type { export { applyTitleTemplate, DESCRIPTION_MAX_LENGTH, - DESCRIPTION_MIN_LENGTH, hreflangSet, renderMeta, robotsContent, diff --git a/packages/seo/src/meta.test.ts b/packages/seo/src/meta.test.ts index 46dacde5..041d40f8 100644 --- a/packages/seo/src/meta.test.ts +++ b/packages/seo/src/meta.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from 'bun:test'; +import * as seo from './index'; import { applyTitleTemplate, hreflangSet, type RouteMeta, renderMeta } from './meta'; const META: RouteMeta = { @@ -29,6 +30,19 @@ function find( return tags.filter((tag) => tag.attrs[attr] === value); } +describe('exported length bounds', () => { + test('every exported bound is one validateMeta actually enforces', () => { + // `DESCRIPTION_MIN_LENGTH` sat under the comment "validate.ts enforces it" and no validator + // ever read it, so a 10-character description passed a gate the constant said it would fail. + // A bound that ships without an enforcer is a promise, and a promise is not a build error. + const bounds = Object.keys(seo) + .filter((name) => name.endsWith('_LENGTH')) + .sort(); + + expect(bounds).toEqual(['DESCRIPTION_MAX_LENGTH', 'TITLE_MAX_LENGTH']); + }); +}); + describe('renderMeta', () => { test('applies the title template without doubling the brand', () => { expect(applyTitleTemplate('Ship it', '%s — Ultimate')).toBe('Ship it — Ultimate'); diff --git a/packages/seo/src/meta.ts b/packages/seo/src/meta.ts index be8a42fe..6c3d564f 100644 --- a/packages/seo/src/meta.ts +++ b/packages/seo/src/meta.ts @@ -4,9 +4,12 @@ import { absoluteUrl } from './xml'; -/** Search results truncate past this; validate.ts enforces it. */ +/** + * Search results truncate past these; `validate.ts` enforces both, and only these two exist for + * that reason — a `DESCRIPTION_MIN_LENGTH` shipped here with no validator reading it, so the + * comment promised a gate that never ran. A bound with no enforcer does not ship. + */ export const TITLE_MAX_LENGTH = 60; -export const DESCRIPTION_MIN_LENGTH = 50; export const DESCRIPTION_MAX_LENGTH = 160; export interface RobotsDirectives { diff --git a/packages/storage/CLAUDE.md b/packages/storage/CLAUDE.md index 1504323e..a1b60f60 100644 --- a/packages/storage/CLAUDE.md +++ b/packages/storage/CLAUDE.md @@ -157,6 +157,23 @@ Gotchas: - `acceptSignedUpload` refuses a URL signed with **no** content type (`unconstrained`). `grantUpload` always sets one, so such a URL is hand-rolled, and trusting the uploader's header instead is the only other option. +- **The signed base is declared ONCE, in `signedUrlBaseFor(driverName)`.** `localDriver` mints + under it and `accept.ts` defaults to `signedUrlBaseFor(disk.name)` — the same base, arrived at + from the disk the caller already passed. It was stated twice (`/_storage/local` in the driver, + `/_storage` in `verifySignedUrl`'s default), so with both defaults NO genuine URL verified: the + key parsed as `local/` and every grant died as `signature-mismatch`. `@ultimat3/cli`'s + `STORAGE_BASE_PATH` is a third statement of the mount prefix and should import + `DEFAULT_SIGNED_URL_BASE` instead. +- **`accept.ts` asks the `isTenantScoped`/`isWithinOrg` PAIR, exactly as `dev-storage.ts` does.** + `isWithinOrg` alone refused every un-scoped key, so an app's own `brand/logo.png` was unreachable + through a URL it had just signed. `isTenantScoped` is case-INSENSITIVE and `isWithinOrg` is not: + `Org/o2/x` and `org/o2/x` are one file on APFS/NTFS, so the fold has to count as tenant-scoped + and then fail the exact-case membership test. Do not "simplify" either half. +- **`AcceptSignedUploadInput.checksum` is what makes `uploadPolicy({ requireChecksum: true })` + reachable.** Without it that option could only ever fail, because nothing on the accept path + could declare a hash. It travels like `declaredContentType`: the route reads a header and hands + it over, and `validateUpload` hashes the bytes itself. The browser half does NOT send one — a + custom header on an S3 presigned PUT is a signature question this package cannot answer. - `orgId` is required on both halves of `accept.ts` and is the ACTOR's, never a request field. A signed URL is a capability; a leaked capability must still not cross a tenant. - `X_STORAGE_ORG_MISMATCH` maps to **404**, not 403 (`@ultimat3/http`'s `error-map.ts`). 403 would diff --git a/packages/storage/README.md b/packages/storage/README.md index a5f30d9b..25c9a954 100644 --- a/packages/storage/README.md +++ b/packages/storage/README.md @@ -89,7 +89,9 @@ fixing was built wrong. `scopedKey('org-1', 'avatars', 'a.png')` is `org/org-1/avatars/a.png`; guard every client-supplied key with `isWithinOrg(key, ctx.actor.orgId)`. A surface that serves objects pairs it with `isTenantScoped(key)`: only a key already inside `org/` is another tenant's to refuse, so -`disk().put('brand/logo.png', …)` stays reachable while `org/org-2/…` never is. +`disk().put('brand/logo.png', …)` stays reachable while `org/org-2/…` never is. `accept.ts` asks +the pair too. `isTenantScoped` folds case (`Org/`, `ORG/`) and `isWithinOrg` does not, so a +case-variant prefix — one directory, not two, on APFS or NTFS — is refused rather than matched. ## Signed URLs @@ -144,8 +146,10 @@ const { key } = await uploadFile({ file, grant: (request) => api.requestUpload(r // 3. server, in the route mounted at `/_storage`: take it back, or refuse const object = await acceptSignedUpload({ - url: request.url, secret, baseUrl: '/_storage/local', - disk: disk('uploads'), orgId: ctx.actor.orgId, + url: request.url, // baseUrl defaults to signedUrlBaseFor(disk.name) — the + secret, // same base the driver signed under. Pass one only for a + disk: disk('uploads'), // route mounted somewhere other than /_storage/. + orgId: ctx.actor.orgId, bytes, declaredContentType: request.headers.get('content-type') ?? undefined, policy: uploadPolicy({ maxBytes: 5e6 }), }); @@ -154,7 +158,12 @@ const object = await acceptSignedUpload({ `acceptSignedUpload` refuses on any of: a signature that does not verify, an expired grant, a `PUT` grant replayed as a `GET`, a key outside the actor's org, more bytes than the signature granted, a `Content-Type` the signature does not cover, or magic bytes that contradict it. -`readSignedObject` is the GET half and applies the same verification and the same org check. +`readSignedObject` is the GET half and applies the same verification and the same org check — +which is the `isTenantScoped`/`isWithinOrg` pair, so an app's own un-scoped `brand/logo.png` is +readable through a URL it signed and `org/org-2/…` still is not. +`uploadPolicy({ requireChecksum: true })` needs the request's declared hash, which travels as +`checksum` exactly as the content type travels as `declaredContentType`: a header the route reads +and hands over, hashed again here and refused on any disagreement. Neither owns a `Request`, a `Response` or a status number — mounting is the host's job, and `@ultimat3/http` is the only layer that turns an `X_*` code into a status. diff --git a/packages/storage/src/accept.test.ts b/packages/storage/src/accept.test.ts index c0d7df2f..86a43308 100644 --- a/packages/storage/src/accept.test.ts +++ b/packages/storage/src/accept.test.ts @@ -9,6 +9,7 @@ import { tmpdir } from 'node:os'; import { frozenClock } from '@ultimat3/core'; import { acceptSignedUpload, readSignedObject } from './accept'; import type { StorageDriver } from './driver'; +import { sha256Base64 } from './driver'; import { localDriver } from './driver-local'; import { isStorageError } from './errors'; import { grantUpload } from './grant'; @@ -246,3 +247,167 @@ describe('acceptSignedUpload', () => { expect(code).toBe('X_STORAGE_ORG_MISMATCH'); }); }); + +/** + * The documented pair, with NO `baseUrl` on either call. `localDriver` signs under + * `/_storage/` and the accept side has to arrive at the same base from the disk it was + * handed — a second default is a genuine URL that verifies nowhere. + */ +describe('the default base', () => { + test('a grant minted with no baseUrl verifies with no baseUrl', async () => { + const grant = await putGrant(48); + const stored = await acceptSignedUpload({ + url: grant.url, + secret: SECRET, + disk, + orgId: ORG, + bytes: genuinePng(), + declaredContentType: 'image/png', + policy: IMAGES, + clock, + }); + expect(stored.key).toBe(grant.key); + + const url = await disk.signedUrl(grant.key, { method: 'GET' }); + const read = await readSignedObject({ url, secret: SECRET, disk, orgId: ORG, clock }); + expect(read.bytes.byteLength).toBe(48); + }); +}); + +/** `requireChecksum` governs a path only if a request can carry a checksum at all. */ +describe('requireChecksum', () => { + const CHECKED = uploadPolicy({ + maxBytes: 1024, + allowedContentTypes: ['image/png'], + requireChecksum: true, + }); + + const acceptWith = (url: string, bytes: Uint8Array, checksum?: string): Promise => + acceptSignedUpload({ + url, + secret: SECRET, + disk, + orgId: ORG, + bytes, + declaredContentType: 'image/png', + policy: CHECKED, + clock, + ...(checksum === undefined ? {} : { checksum }), + }); + + test('accepts an upload whose declared checksum matches the bytes', async () => { + const grant = await putGrant(48); + const bytes = genuinePng(); + const stored = await acceptWith(grant.url, bytes, sha256Base64(bytes)); + expect(stored).toMatchObject({ key: grant.key, contentType: 'image/png', size: 48 }); + expect(await disk.exists(grant.key)).toBe(true); + }); + + // `meta.declared` is asserted, not just the code: a build that dropped the field on the floor + // would refuse this upload too, as "declared none" — the same code for a different reason. + test('refuses an upload whose declared checksum is a lie', async () => { + const grant = await putGrant(48); + let declared: unknown = 'no-error-thrown'; + try { + await acceptWith(grant.url, genuinePng(), 'not-the-hash'); + } catch (error) { + declared = isStorageError(error) ? error.meta?.['declared'] : String(error); + } + expect(declared).toBe('not-the-hash'); + expect(await disk.exists(grant.key)).toBe(false); + }); + + test('still refuses an upload that declares none, which is what the policy is for', async () => { + const grant = await putGrant(48); + expect(await codeOf(() => acceptWith(grant.url, genuinePng()))).toBe( + 'X_STORAGE_CHECKSUM_MISMATCH', + ); + }); +}); + +/** The code plus `meta.reason` flattened, so two gates cannot pass for each other. */ +async function outcomeOf(fn: () => Promise): Promise { + try { + await fn(); + } catch (error) { + if (!isStorageError(error)) return `not-a-storage-error: ${String(error)}`; + const reason = error.meta?.['reason']; + return typeof reason === 'string' ? `${error.code}:${reason}` : error.code; + } + return 'no-error-thrown'; +} + +/** A GET signed with the REAL secret, for any key — the attacker this gate must survive. */ +async function forgedGet(key: string, encodeWhole = false): Promise { + const constraints = { + key, + method: 'GET' as const, + expiresAt: clock.now().getTime() + 60_000, + maxBytes: undefined, + contentType: undefined, + }; + const params = new URLSearchParams({ + [SIGNED_URL_PARAMS.method]: 'GET', + [SIGNED_URL_PARAMS.expires]: String(constraints.expiresAt), + [SIGNED_URL_PARAMS.signature]: await signConstraints(SECRET, constraints), + }); + const path = encodeWhole + ? encodeURIComponent(key) + : key.split('/').map(encodeURIComponent).join('/'); + return `${BASE}/${path}?${params.toString()}`; +} + +describe('keys outside the tenant namespace', () => { + test("an app's own shared asset is readable through a signed URL", async () => { + await disk.put('brand/logo.png', genuinePng(), { contentType: 'image/png' }); + const url = await disk.signedUrl('brand/logo.png'); + const read = await readSignedObject({ url, secret: SECRET, disk, orgId: ORG, clock }); + expect(read.bytes.byteLength).toBe(48); + }); + + // Every spoof is signed with the REAL secret: a refusal that leaned on the HMAC would prove + // nothing about the tenant gate itself. `org-1` is the actor throughout. + const SPOOFS: readonly (readonly [string, string])[] = [ + ['org/org-2/secret.png', 'X_STORAGE_ORG_MISMATCH'], + // Case: `Org/` and `org/` are ONE directory on a case-insensitive filesystem (APFS, NTFS), + // so a case-folded prefix that read as "not tenant-scoped" would be a cross-tenant read. + ['Org/org-2/secret.png', 'X_STORAGE_ORG_MISMATCH'], + ['ORG/org-1/secret.png', 'X_STORAGE_ORG_MISMATCH'], + // The prefix ends in a slash, so a longer org id may not borrow a shorter one's namespace. + ['org/org-1x/secret.png', 'X_STORAGE_ORG_MISMATCH'], + // `new URL()` normalises the traversal away, so the key verified is not the key signed. + ['org/org-1/../../org-2/secret.png', 'X_STORAGE_URL_INVALID:signature-mismatch'], + ['org/org-1/./secret.png', 'X_STORAGE_URL_INVALID:signature-mismatch'], + // The sidecar namespace: reachable only if the org gate stopped being what refused it. + ['.meta/org/org-2/secret.png.json', 'X_STORAGE_URL_INVALID:unsafe-key'], + ['/org/org-2/secret.png', 'X_STORAGE_URL_INVALID:unsafe-key'], + ['org/org-2//secret.png', 'X_STORAGE_URL_INVALID:unsafe-key'], + ]; + + for (const [key, expected] of SPOOFS) { + test(`refuses a genuinely signed "${key}"`, async () => { + const url = await forgedGet(key); + expect( + await outcomeOf(() => readSignedObject({ url, secret: SECRET, disk, orgId: ORG, clock })), + ).toBe(expected); + }); + } + + test('a percent-encoded separator decodes before the tenant gate, never after', async () => { + const url = await forgedGet('org/org-2/secret.png', true); + expect(new URL(url, 'http://storage.invalid').pathname).toContain('%2F'); + expect( + await outcomeOf(() => readSignedObject({ url, secret: SECRET, disk, orgId: ORG, clock })), + ).toBe('X_STORAGE_ORG_MISMATCH'); + }); + + // A homoglyph prefix is not tenant-scoped and must not resolve to the object it imitates: + // no filesystem folds Cyrillic `о` onto ASCII `o`, so this is a different key entirely. + test('a homoglyph org prefix reads no object at all', async () => { + await disk.put('org/org-2/secret.png', genuinePng(), { contentType: 'image/png' }); + const url = await forgedGet('оrg/org-2/secret.png'); + expect( + await outcomeOf(() => readSignedObject({ url, secret: SECRET, disk, orgId: ORG, clock })), + ).toBe('X_STORAGE_NOT_FOUND'); + }); +}); diff --git a/packages/storage/src/accept.ts b/packages/storage/src/accept.ts index ad10887b..e1700941 100644 --- a/packages/storage/src/accept.ts +++ b/packages/storage/src/accept.ts @@ -9,9 +9,9 @@ import type { Clock } from '@ultimat3/core'; import type { SignedUrlMethod, StorageDriver, StorageObject, StorageRead } from './driver'; import { orgMismatch, signedUrlExpired, signedUrlRejected, tooLarge } from './errors'; -import { isWithinOrg } from './path'; +import { isTenantScoped, isWithinOrg } from './path'; import type { SignedUrlConstraints } from './signed-url'; -import { verifySignedUrl } from './signed-url'; +import { signedUrlBaseFor, verifySignedUrl } from './signed-url'; import type { UploadPolicy } from './upload'; import { normalizeContentType, uploadPolicy, validateUpload } from './upload'; @@ -19,6 +19,11 @@ export interface SignedRequestInput { /** Absolute or route-relative — `verifySignedUrl` parses both. */ readonly url: string; readonly secret: string; + /** + * Defaults to the base THIS disk signs under (`signedUrlBaseFor(disk.name)`), never to the bare + * mount prefix: a second default here made every URL `localDriver` mints a signature-mismatch, + * because the key parsed as `local/`. Pass one only for a route mounted somewhere else. + */ readonly baseUrl?: string | undefined; readonly disk: StorageDriver; /** @@ -33,6 +38,13 @@ export interface AcceptSignedUploadInput extends SignedRequestInput { readonly bytes: Uint8Array; /** The transport's `Content-Type`. Refused unless it equals the type the grant signed. */ readonly declaredContentType?: string | undefined; + /** + * The transport's declared base64 SHA-256, travelling exactly as `declaredContentType` does: a + * header the route reads and hands over, trusted for nothing — the bytes are hashed here and a + * disagreement is refused. Without this field `uploadPolicy({ requireChecksum: true })` could + * only ever fail, since nothing on this path could ever declare one. + */ + readonly checksum?: string | undefined; readonly policy?: UploadPolicy | undefined; } @@ -48,7 +60,7 @@ async function constraintsFor( const result = await verifySignedUrl({ url: input.url, secret: input.secret, - ...(input.baseUrl === undefined ? {} : { baseUrl: input.baseUrl }), + baseUrl: input.baseUrl ?? signedUrlBaseFor(input.disk.name), ...(input.clock === undefined ? {} : { clock: input.clock }), }); if (!result.ok) { @@ -63,8 +75,15 @@ async function constraintsFor( `the URL is signed for ${constraints.method}, and this is a ${method}`, ); } - if (!isWithinOrg(constraints.key, input.orgId)) { - throw orgMismatch(constraints.key, input.orgId); + // The PAIR is the question "does this key belong to somebody else?". `isWithinOrg` alone + // answered `false` for every un-scoped key, so an app's own `brand/logo.png` was unreachable + // through a URL it had just signed — `path.ts` says so and `dev-storage.ts` already asks it this + // way. An actor with no org is inside no org, so every tenant-scoped key is somebody else's; + // checked here because `isWithinOrg` reads an empty org as a malformed key and would blame the + // URL for the actor's missing claim. + const orgId = input.orgId; + if (isTenantScoped(constraints.key) && (orgId === '' || !isWithinOrg(constraints.key, orgId))) { + throw orgMismatch(constraints.key, orgId); } return constraints; } @@ -106,7 +125,12 @@ export async function acceptSignedUpload(input: AcceptSignedUploadInput): Promis const policy = input.policy ?? uploadPolicy(); const validated = validateUpload( - { key, declaredContentType: signed, bytes: input.bytes }, + { + key, + declaredContentType: signed, + bytes: input.bytes, + ...(input.checksum === undefined ? {} : { checksum: input.checksum }), + }, policy, ); return input.disk.put(validated.key, validated.bytes, { diff --git a/packages/storage/src/driver-local.ts b/packages/storage/src/driver-local.ts index 77b6b89b..90c0f97f 100644 --- a/packages/storage/src/driver-local.ts +++ b/packages/storage/src/driver-local.ts @@ -29,7 +29,7 @@ import { storageNotImplemented, } from './errors'; import { assertSafeKey, META_DIR } from './path'; -import { buildSignedUrl } from './signed-url'; +import { buildSignedUrl, signedUrlBaseFor } from './signed-url'; import { DEFAULT_MAX_UPLOAD_BYTES } from './upload'; const DRIVER_NAME = 'local'; @@ -130,7 +130,7 @@ export function localDriver(options: LocalDriverOptions): StorageDriver { const root = options.root.replace(/\/+$/, ''); const maxPutBytes = options.maxPutBytes ?? DEFAULT_MAX_UPLOAD_BYTES; const clock = options.clock ?? systemClock; - const baseUrl = options.baseUrl ?? `/_storage/${DRIVER_NAME}`; + const baseUrl = options.baseUrl ?? signedUrlBaseFor(DRIVER_NAME); // A dev disk must work with zero config. Outside development the fallback is refused rather // than used: the literal is published, so signing with it hands every reader the power to mint // a PUT for any key with any size and type limit — which `acceptSignedUpload` then trusts over diff --git a/packages/storage/src/index.ts b/packages/storage/src/index.ts index 6d1e1988..34105c08 100644 --- a/packages/storage/src/index.ts +++ b/packages/storage/src/index.ts @@ -141,6 +141,7 @@ export { SIGNED_URL_PARAMS, SIGNED_URL_VERSION, signConstraints, + signedUrlBaseFor, timingSafeEqual, verifySignedUrl, } from './signed-url'; diff --git a/packages/storage/src/path.test.ts b/packages/storage/src/path.test.ts index bdd8e086..aa218d17 100644 --- a/packages/storage/src/path.test.ts +++ b/packages/storage/src/path.test.ts @@ -88,6 +88,16 @@ describe('isTenantScoped', () => { expect(isTenantScoped('orgs/org-1/a.png')).toBe(false); }); + // `Org/o2/a.png` and `org/o2/a.png` are ONE file on a case-insensitive filesystem (APFS, NTFS), + // so a case-folded prefix that answered `false` would hand a caller another tenant's object on + // every macOS dev disk — and the fold is refused outright, because `org/` is the only spelling + // `scopedKey` ever mints. + test('a case-folded prefix is still the tenant namespace', () => { + expect(isTenantScoped('Org/org-2/a.png')).toBe(true); + expect(isTenantScoped('ORG/org-1/a.png')).toBe(true); + expect(isWithinOrg('Org/org-1/a.png', 'org-1')).toBe(false); + }); + test('the two guards together are what makes a foreign key unreadable', () => { const foreign = scopedKey('org-2', 'a.png'); expect(isTenantScoped(foreign) && !isWithinOrg(foreign, 'org-1')).toBe(true); diff --git a/packages/storage/src/path.ts b/packages/storage/src/path.ts index 360b7c7e..8599676a 100644 --- a/packages/storage/src/path.ts +++ b/packages/storage/src/path.ts @@ -102,9 +102,15 @@ export function isWithinOrg(key: string, orgId: string): boolean { * two apart: `isWithinOrg` alone would answer `false` for every un-scoped key and make an app's * own shared assets unreachable, and dropping the check would make one tenant's prefix readable * by another. The pair is the question "does this key belong to somebody else?". + * + * Case-INSENSITIVE, and that is the load-bearing half: `Org/o2/a.png` and `org/o2/a.png` are one + * file on a case-insensitive filesystem (APFS, NTFS), so an exact-case test would answer "not a + * tenant's" for a key that reads another tenant's bytes on every macOS dev disk. `isWithinOrg` is + * exact-case and stays so, so a folded prefix is refused outright rather than matched — `org/` is + * the only spelling `scopedKey` mints, so nothing legitimate arrives in any other. */ export function isTenantScoped(key: string): boolean { - return key.startsWith(`${ORG_PREFIX}/`); + return key.slice(0, ORG_PREFIX.length + 1).toLowerCase() === `${ORG_PREFIX}/`; } /** `org/o1/a/b.png` -> `org/o1/a`. Empty for a top-level key. */ diff --git a/packages/storage/src/signed-url.ts b/packages/storage/src/signed-url.ts index 8820ce68..e4000685 100644 --- a/packages/storage/src/signed-url.ts +++ b/packages/storage/src/signed-url.ts @@ -17,6 +17,17 @@ export const DEFAULT_SIGNED_URL_TTL_MS = 900_000; /** The dev server mounts the download/upload route here; S3 disks never use it. */ export const DEFAULT_SIGNED_URL_BASE = '/_storage'; +/** + * The base ONE disk's own URLs hang off: the mount prefix plus the driver's name, because the + * mounted route is `${DEFAULT_SIGNED_URL_BASE}/:disk/*key` and the disk segment is inside the + * path the HMAC is recovered from. Declared once and called by both halves — the driver that + * mints and `accept.ts` that verifies — because a base stated twice is a base that drifts, and a + * drifted base makes every genuine URL a `signature-mismatch`: the key parses as `local/`. + */ +export function signedUrlBaseFor(driverName: string): string { + return `${DEFAULT_SIGNED_URL_BASE}/${driverName}`; +} + export const SIGNED_URL_PARAMS = { method: 'x-method', expires: 'x-exp', diff --git a/packages/time/CLAUDE.md b/packages/time/CLAUDE.md index 37630cc9..43dbf897 100644 --- a/packages/time/CLAUDE.md +++ b/packages/time/CLAUDE.md @@ -10,8 +10,6 @@ | `instant.ts` | the UTC `Instant` brand, ISO/epoch conversion, `now(clock)`, `epoch()` | | `zones.ts` | IANA validation, `offsetAt` (minutes east), zone labels | | `zone-canonical.ts` | one zone, one key: `canonicalTimeZone` — the casing/alias collapse every cache keys on | -| `locale-canonical.ts` | one locale, one key: `canonicalLocale` — the same collapse for the `Accept-Language` half | -| `intl-cache.ts` | the one bounded FIFO every `Intl` formatter cache in this package uses | | `zoned.ts` | `toZoned` / `fromZoned` + gap and overlap policies. Everything depends on this. | | `format.ts` | `Intl` rendering. Every function takes `locale` **and** `zone`. | | `duration.ts` | `'2h30m'` ⇄ ms | @@ -36,7 +34,10 @@ possible version of the rule above. Never reintroduce either half. - **Never cache an `Intl` formatter on a raw caller string.** A zone and a locale both arrive from a request header, so the key must be canonical (`canonicalTimeZone` for a zone, `canonicalLocale` - for a locale) and the cache must be bounded (`cachedFormatter`, `intl-cache.ts`). An unbounded + for a locale) and the cache must be bounded (`cachedFormatter`). **`cachedFormatter`, + `MAX_CACHED_FORMATTERS` and `canonicalLocale` are `@ultimat3/core`'s as of 2.0.0**, not this + package's: `@ultimat3/money` hit the identical unbounded-`Map`-on-a-header bug and tier 1 may not + import sideways, so the mechanism moved down a tier rather than being copied. An unbounded `Map` keyed on `x-timezone` grew 31 MB for 4,096 casings of one zone name, and the casing space of a 13-letter zone is 2^12. **Both halves, always** — a canonical key does not bound anything (an unknown `-u-` extension value survives canonicalization as a distinct string) and the cap diff --git a/packages/time/README.md b/packages/time/README.md index bd22f515..b3c058a0 100644 --- a/packages/time/README.md +++ b/packages/time/README.md @@ -23,8 +23,10 @@ return it. Anything reading a zone off a request header should canonicalize befo One **locale** is one key for the same reason — `Accept-Language` spells one locale `EN-us`, `en-US` and `en-latn-us`, and `formatDateTime` and `describeCron` collapse the three before they -reach a formatter cache. The cap in `intl-cache.ts` stays either way: an unknown `-u-` extension -value survives canonicalization as a distinct string, so the key bounds nothing on its own. +reach a formatter cache. The cap stays either way: an unknown `-u-` extension value survives +canonicalization as a distinct string, so the key bounds nothing on its own. Both halves — +`canonicalLocale` and `cachedFormatter` — are `@ultimat3/core`'s as of 2.0.0, so `@ultimat3/money` +reads the same bound rather than a copy of it. Every value this package hands back is its own object: `instant(date)` copies rather than branding the caller's `Date`, and `epoch()` is a function — the `EPOCH` constant it replaces was one shared diff --git a/packages/time/src/cron-describe.ts b/packages/time/src/cron-describe.ts index 8988d5de..02d6ad4a 100644 --- a/packages/time/src/cron-describe.ts +++ b/packages/time/src/cron-describe.ts @@ -4,10 +4,9 @@ * ship English to every locale that forgot the argument — so injection is mandatory, not opt-in. */ +import { cachedFormatter, canonicalLocale } from '@ultimat3/core'; import { type CronExpression, parseCronOnce } from './cron-parse'; import { cronNotDescribable, localeInvalid } from './errors'; -import { cachedFormatter } from './intl-cache'; -import { canonicalLocale } from './locale-canonical'; export interface CronPhrases { everyMinute: string; @@ -134,8 +133,9 @@ function fill(template: string, vars: Readonly>) * header. `canonicalLocale` collapses the spellings of one locale — `EN-us`, `en-latn-us` — but it * still returns a distinct string for every unknown `-u-` extension value, so the key alone does * not bound anything and only the cap keeps the key space finite. Neither half is redundant. The - * cap and its FIFO live in `intl-cache.ts`, because `zones.ts` and `format.ts` needed the same - * rule and a hazard documented in one file is a hazard the other two repeat. + * cap and its FIFO live in `@ultimat3/core`'s `intl-cache.ts`, because `zones.ts`, `format.ts` and + * `@ultimat3/money`'s formatter all need the same rule, and a hazard documented in one file is a + * hazard every other one repeats. * * Both caches are fed the canonical `tag` by `describeCron` alone, never a caller string. */ diff --git a/packages/time/src/cron-occurrence.test.ts b/packages/time/src/cron-occurrence.test.ts index 32e940b3..e4d57ffd 100644 --- a/packages/time/src/cron-occurrence.test.ts +++ b/packages/time/src/cron-occurrence.test.ts @@ -33,6 +33,18 @@ describe('nextCronOccurrence', () => { expect(toIso(next)).toBe('2026-03-16T09:00:00.000Z'); }); + test('a wrapping stepped weekday range fires on the days it names, and no others', () => { + // 2026-03-14 is a Saturday. `sat-tue/2` is Saturday and Monday; a task on this schedule used + // to fire on Sunday and Tuesday instead, every week, because the stride walked an 8-day week. + const times = nextCronOccurrences('0 3 * * sat-tue/2', UTC, fromIso('2026-03-14T00:00:00Z'), 4); + expect(times.map(toIso)).toEqual([ + '2026-03-14T03:00:00.000Z', // Saturday + '2026-03-16T03:00:00.000Z', // Monday + '2026-03-21T03:00:00.000Z', // Saturday + '2026-03-23T03:00:00.000Z', // Monday + ]); + }); + test('day-of-month and day-of-week OR together (Vixie semantics)', () => { // "1st of the month OR any Monday" — both restricted means either matches. const next = nextCronOccurrence('0 0 1 * MON', UTC, fromIso('2026-03-14T00:00:00Z')); diff --git a/packages/time/src/cron-parse.test.ts b/packages/time/src/cron-parse.test.ts index 7fe9068a..7a8237d4 100644 --- a/packages/time/src/cron-parse.test.ts +++ b/packages/time/src/cron-parse.test.ts @@ -35,6 +35,31 @@ describe('parseCron', () => { expect(parseCron('0 22-2 * * *').hours).toEqual([0, 1, 2, 22, 23]); }); + test('a wrapping day-of-week range strides over a 7-day week, not an 8-day one', () => { + // The dow field is spelled 0-7 because 0 and 7 are both Sunday, so `max - min + 1` is 8 and a + // wrap walked a week with a phantom day in it: `sat-tue/2` answered sat, sun, tue instead of + // sat, mon — a task firing on days nobody scheduled, every week. + expect(parseCron('0 3 * * sat-tue/2').daysOfWeek).toEqual([1, 6]); // sat, mon + expect(parseCron('0 0 * * fri-mon/2').daysOfWeek).toEqual([5, 7]); // fri, sun + // A step that divides the wrap evenly lands on the far end; one that does not stops short. + expect(parseCron('0 0 * * fri-mon/3').daysOfWeek).toEqual([1, 5]); // fri, mon + expect(parseCron('0 0 * * sat-mon/3').daysOfWeek).toEqual([6]); // sat alone + // Step 1 across the wrap is every day in the range — the case that was right by accident. + expect(parseCron('0 0 * * sat-tue').daysOfWeek).toEqual([1, 2, 6, 7]); + expect(parseCron('0 0 * * fri-mon').daysOfWeek).toEqual([1, 5, 6, 7]); + // Sunday spelled 7 on either end of a wrap is the same Sunday as 0. + expect(parseCron('0 0 * * 7-2').daysOfWeek).toEqual([1, 2, 7]); + expect(parseCron('0 0 * * 6-0').daysOfWeek).toEqual([6, 7]); + // Non-wrapping dow ranges and steps are untouched by the span. + expect(parseCron('0 0 * * sun-sat').daysOfWeek).toEqual([1, 2, 3, 4, 5, 6, 7]); + expect(parseCron('0 0 * * 0-7').daysOfWeek).toEqual([1, 2, 3, 4, 5, 6, 7]); + expect(parseCron('0 0 * * 5-7').daysOfWeek).toEqual([5, 6, 7]); + expect(parseCron('0 0 * * 1-5/2').daysOfWeek).toEqual([1, 3, 5]); + expect(parseCron('0 0 * * */2').daysOfWeek).toEqual([2, 4, 6, 7]); + // `5/2` runs to the field maximum, and the dow maximum is 7 — fri and sun, as Vixie reads it. + expect(parseCron('0 0 * * 5/2').daysOfWeek).toEqual([5, 7]); + }); + test('rejects malformed expressions with X_CRON_INVALID and a working example', () => { expect(codeOf(() => parseCron('0 3 * *'))).toBe('X_CRON_INVALID'); expect(codeOf(() => parseCron('61 * * * *'))).toBe('X_CRON_INVALID'); diff --git a/packages/time/src/cron-parse.ts b/packages/time/src/cron-parse.ts index ac324f88..dedb2bec 100644 --- a/packages/time/src/cron-parse.ts +++ b/packages/time/src/cron-parse.ts @@ -65,7 +65,9 @@ export function parseCron(expression: string): CronExpression { const hours = parseField(expression, hourField ?? '*', 0, 23); const daysOfMonth = parseField(expression, domField ?? '*', 1, 31); const months = parseField(expression, monthField ?? '*', 1, 12, MONTH_NAMES, 1); - const rawDow = parseField(expression, dowField ?? '*', 0, 7, DAY_NAMES, 0); + // Span 7, not `max - min + 1` = 8: the dow field accepts 0-7 because Sunday has two spellings, + // so the modulus a wrap strides over is a week with a phantom day in it unless it is stated. + const rawDow = parseField(expression, dowField ?? '*', 0, 7, DAY_NAMES, 0, 7); // 0 and 7 are both Sunday in cron; ISO calls Sunday 7. const daysOfWeek = [...new Set(rawDow.map((day) => (day === 0 ? 7 : day)))].sort((a, b) => a - b); @@ -113,6 +115,13 @@ function parseField( max: number, names: readonly string[] = [], nameOffset = 0, + /** + * How many distinct values one full turn of this field has — `max - min + 1` for every field + * whose spelling is one-to-one. Day-of-week is the exception and the reason this is a parameter: + * it spells Sunday twice (0 and 7), so its 0-7 bounds describe 8 slots over a 7-day week, and a + * wrapping stride computed from the bounds walked a day that does not exist. + */ + span = max - min + 1, ): number[] { const values = new Set(); for (const part of field.split(',')) { @@ -144,7 +153,6 @@ function parseField( // Wrapping ranges (`fri-mon`, `22-2`) are a real cron idiom, and the stride CONTINUES across // the wrap: `23-3/2` is 23, 01, 03 — every second hour starting at 23. Restarting at `min` // answered 23, 00, 02, an hour off for every occurrence past midnight. - const span = max - min + 1; const length = to - from + span; for (let offset = 0; offset <= length; offset += step) { values.add(min + ((from - min + offset) % span)); diff --git a/packages/time/src/format.ts b/packages/time/src/format.ts index 7a16f988..7ceeebef 100644 --- a/packages/time/src/format.ts +++ b/packages/time/src/format.ts @@ -4,9 +4,8 @@ * because "the server's timezone" is never the answer to "what time is it for the user". */ +import { cachedFormatter, canonicalLocale } from '@ultimat3/core'; import { differenceMs, type Instant } from './instant'; -import { cachedFormatter } from './intl-cache'; -import { canonicalLocale } from './locale-canonical'; import { assertTimeZone, type TimeZone } from './zones'; export type DateTimeStyle = 'short' | 'medium' | 'long' | 'full'; diff --git a/packages/time/src/intl-cache.test.ts b/packages/time/src/intl-cache.test.ts deleted file mode 100644 index 7bf53a9a..00000000 --- a/packages/time/src/intl-cache.test.ts +++ /dev/null @@ -1,40 +0,0 @@ -// The one bounded formatter cache: it must reuse on the second ask and evict oldest-first at the -// cap, because the keys are locales and zones a request header chooses. - -import { describe, expect, test } from 'bun:test'; -import { cachedFormatter, MAX_CACHED_FORMATTERS } from './intl-cache'; - -describe('cachedFormatter', () => { - test('answers from the cache on the second ask', () => { - const cache = new Map(); - let built = 0; - const build = (): number => { - built += 1; - return built; - }; - expect(cachedFormatter(cache, 'en', build)).toBe(1); - expect(cachedFormatter(cache, 'en', build)).toBe(1); - expect(built).toBe(1); - }); - - test('evicts oldest-first at the cap, so a header cannot mint entries forever', () => { - // The whole point: a locale or a zone arrives from a request header, and an unbounded Map - // keyed on that string is memory the client chooses. - const cache = new Map(); - let built = 0; - const build = (): number => { - built += 1; - return built; - }; - for (let index = 0; index <= MAX_CACHED_FORMATTERS; index += 1) { - cachedFormatter(cache, `key-${index}`, build); - } - expect(cache.size).toBe(MAX_CACHED_FORMATTERS); - expect(built).toBe(MAX_CACHED_FORMATTERS + 1); - // FIFO: the first key is the one that went. - expect(cache.has('key-0')).toBe(false); - expect(cache.has(`key-${MAX_CACHED_FORMATTERS}`)).toBe(true); - cachedFormatter(cache, 'key-0', build); - expect(built).toBe(MAX_CACHED_FORMATTERS + 2); - }); -}); diff --git a/packages/time/src/intl-cache.ts b/packages/time/src/intl-cache.ts deleted file mode 100644 index a48d15df..00000000 --- a/packages/time/src/intl-cache.ts +++ /dev/null @@ -1,26 +0,0 @@ -/** - * One bounded cache for every `Intl` formatter this package builds. - * A locale and a zone both arrive from a request header, so an unbounded `Map` keyed on that - * string is memory the client chooses: 4,096 case-variants of one zone name retained 31 MB, - * ~7.7 KB per `Intl.DateTimeFormat`, and 600 zones times 2^12 casings has no ceiling at all. - */ - -/** - * Above the full canonical IANA set (445 zones as of tzdata 2025) so a correct app never evicts, - * and small enough that the worst case is a few megabytes rather than a leak. A miss costs one - * `Intl` construction, never a wrong answer — which is what makes a bound safe here at all. - */ -export const MAX_CACHED_FORMATTERS = 512; - -/** FIFO — a `Map` iterates in insertion order, so the first key inserted is the first evicted. */ -export function cachedFormatter(cache: Map, key: string, build: () => T): T { - const hit = cache.get(key); - if (hit !== undefined) return hit; - const formatter = build(); - if (cache.size >= MAX_CACHED_FORMATTERS) { - const oldest = cache.keys().next().value; - if (oldest !== undefined) cache.delete(oldest); - } - cache.set(key, formatter); - return formatter; -} diff --git a/packages/time/src/locale-canonical.test.ts b/packages/time/src/locale-canonical.test.ts deleted file mode 100644 index 6554c420..00000000 --- a/packages/time/src/locale-canonical.test.ts +++ /dev/null @@ -1,29 +0,0 @@ -// One locale, one formatter-cache key: every spelling `Intl` treats as the same locale has to -// collapse to one string before it reaches a cache, because the caller is `Accept-Language`. - -import { describe, expect, test } from 'bun:test'; -import { canonicalLocale } from './locale-canonical'; - -describe('canonicalLocale', () => { - test('every spelling of one locale collapses to one key', () => { - expect(canonicalLocale('EN-us')).toBe('en-US'); - expect(canonicalLocale('en-US')).toBe('en-US'); - expect(canonicalLocale('en-latn-us')).toBe('en-Latn-US'); - expect(canonicalLocale('DE')).toBe('de'); - // Casing inside a `-u-` extension collapses too; the *values* still do not, which is why - // `intl-cache.ts` keeps its bound as well as this key. - expect(canonicalLocale('de-DE-u-ca-Gregory')).toBe('de-DE-u-ca-gregory'); - }); - - test('a tag Intl cannot parse is undefined, never a silent passthrough', () => { - expect(canonicalLocale('en_US')).toBe(undefined); - expect(canonicalLocale('')).toBe(undefined); - expect(canonicalLocale('not a locale')).toBe(undefined); - }); - - test('well-formed but unknown to ICU is still a locale', () => { - // `Intl` falls back for `zz`; refusing it here would be stricter than the formatters this - // feeds, and would turn a fallback into an error for a tag that renders fine. - expect(canonicalLocale('zz')).toBe('zz'); - }); -}); diff --git a/packages/time/src/locale-canonical.ts b/packages/time/src/locale-canonical.ts deleted file mode 100644 index 63480a53..00000000 --- a/packages/time/src/locale-canonical.ts +++ /dev/null @@ -1,23 +0,0 @@ -/** - * One locale, one key. `Intl` accepts `EN-us`, `en-US` and `en-latn-us` as the same locale, so a - * cache keyed on the caller's spelling holds three formatters where one would do — and the caller - * is `Accept-Language`. The twin of `zone-canonical.ts`, for the other header-supplied string. - */ - -/** - * The canonical BCP 47 spelling, or `undefined` when the tag is not structurally valid at all - * (`en_US`, `''`, `not a locale`). Well-formed but unknown to ICU (`zz`) is a locale — `Intl` - * falls back for it, and refusing here would be stricter than the formatters this feeds. - * - * Deliberately **not** memoised: this is string work, and a `Map` keyed on a header value is the - * unbounded cache the whole `intl-cache.ts` bound exists to prevent. - */ -export function canonicalLocale(locale: string): string | undefined { - try { - // `getCanonicalLocales` runs the same IsStructurallyValidLanguageTag check that - // `supportedLocalesOf` throws on, and unlike it, hands back the canonical spelling. - return Intl.getCanonicalLocales(locale)[0]; - } catch { - return undefined; - } -} diff --git a/packages/time/src/zone-canonical.ts b/packages/time/src/zone-canonical.ts index f13452dd..d9cc211a 100644 --- a/packages/time/src/zone-canonical.ts +++ b/packages/time/src/zone-canonical.ts @@ -4,7 +4,7 @@ * A 13-letter name has 2^12 casings and a request header can name any of them. */ -import { cachedFormatter } from './intl-cache'; +import { cachedFormatter } from '@ultimat3/core'; /** ES2024 `Intl` accepts `+01:00` as a zone; we do not — a fixed offset has no DST rules. */ const NUMERIC_OFFSET = /^[+-]/; diff --git a/packages/time/src/zones.test.ts b/packages/time/src/zones.test.ts index 95b2c4b0..e08d34c3 100644 --- a/packages/time/src/zones.test.ts +++ b/packages/time/src/zones.test.ts @@ -6,6 +6,7 @@ import { describe, expect, test } from 'bun:test'; // the test measures *JavaScript heap* growth, and `Bun.unsafe.memoryFootprint()` reports the // process footprint, which moves with allocator behaviour rather than with retained formatters. import { memoryUsage } from 'node:process'; +import { isUltimateError, type UltimateError } from '@ultimat3/core'; import { fromIso } from './instant'; import { canonicalTimeZone } from './zone-canonical'; import { @@ -66,24 +67,24 @@ describe('isValidTimeZone', () => { }); }); -describe('one zone is one key', () => { - // `Intl` accepts every casing of an IANA name, and both formatter caches were keyed on the raw - // string. `x-timezone: eUrOpE/bErLiN` therefore minted a permanent `Intl.DateTimeFormat` per - // casing — 2^12 of them for a 13-letter zone, from a request header. - const CASINGS = 4096; - - function casing(zone: string, mask: number): string { - const chars = [...zone]; - let bit = 0; - for (let index = 0; index < chars.length; index += 1) { - const char = chars[index] ?? ''; - if (!/[a-z]/i.test(char)) continue; - chars[index] = (mask >> bit) & 1 ? char.toUpperCase() : char.toLowerCase(); - bit += 1; - } - return chars.join(''); +// `Intl` accepts every casing of an IANA name, and every formatter cache was keyed on the raw +// string. `x-timezone: eUrOpE/bErLiN` therefore minted a permanent `Intl.DateTimeFormat` per +// casing — 2^12 of them for a 13-letter zone, from a request header. +const CASINGS = 4096; + +function casing(zone: string, mask: number): string { + const chars = [...zone]; + let bit = 0; + for (let index = 0; index < chars.length; index += 1) { + const char = chars[index] ?? ''; + if (!/[a-z]/i.test(char)) continue; + chars[index] = (mask >> bit) & 1 ? char.toUpperCase() : char.toLowerCase(); + bit += 1; } + return chars.join(''); +} +describe('one zone is one key', () => { test('every casing canonicalizes to the same name', () => { expect(canonicalTimeZone('eUrOpE/bErLiN')).toBe('Europe/Berlin'); expect(canonicalTimeZone('utc')).toBe('UTC'); @@ -121,4 +122,22 @@ describe('zoneAbbrev', () => { expect(zoneAbbrev('Europe/Berlin', summer, 'en-US', 'shortOffset')).toBe('GMT+2'); expect(zoneAbbrev('Asia/Kathmandu', summer, 'en-US', 'shortOffset')).toBe('GMT+5:45'); }); + + test('refuses an unknown zone with X_TIMEZONE_INVALID, like every other entry point', () => { + // It built its own `Intl.DateTimeFormat` on the caller's raw string, so the one label an app + // renders from an `x-timezone` header answered a bare `RangeError` with no code and no fix. + let caught: unknown; + try { + zoneAbbrev('Mars/Olympus', summer); + } catch (error) { + caught = error; + } + expect(isUltimateError(caught)).toBe(true); + expect((caught as UltimateError).code).toBe('X_TIMEZONE_INVALID'); + }); + + test('every casing answers one label, because the key is the canonical name', () => { + expect(zoneAbbrev('eUrOpE/bErLiN', summer, 'en-US', 'shortOffset')).toBe('GMT+2'); + expect(zoneAbbrev('europe/berlin', summer, 'en-US', 'shortOffset')).toBe('GMT+2'); + }); }); diff --git a/packages/time/src/zones.ts b/packages/time/src/zones.ts index 5bb0c4ce..c8e59bc5 100644 --- a/packages/time/src/zones.ts +++ b/packages/time/src/zones.ts @@ -4,9 +4,9 @@ * is no offset table to keep in sync and no `date-fns-tz` dependency. */ +import { cachedFormatter, canonicalLocale } from '@ultimat3/core'; import { timezoneInvalid } from './errors'; import type { Instant } from './instant'; -import { cachedFormatter } from './intl-cache'; import { canonicalTimeZone } from './zone-canonical'; /** An IANA identifier: `Europe/Berlin`, `Asia/Kathmandu`, `UTC`. Never `CET`, never `+01:00`. */ @@ -117,13 +117,22 @@ export function zoneAbbrev( locale = 'en-US', style: 'short' | 'long' | 'shortOffset' | 'longOffset' = 'short', ): string { - const formatter = new Intl.DateTimeFormat(locale, { - timeZone: zone, - timeZoneName: style, - hourCycle: 'h23', + const canonical = assertTimeZone(zone); + // A tag `Intl` cannot parse falls through unchanged, exactly as in `format.ts`: this decides a + // cache key, never whether a locale is acceptable. + const tag = canonicalLocale(locale) ?? locale; + // The one `Intl` construction in this package that escaped the shared cache: it built a formatter + // per call on the caller's raw zone and locale, so an `x-timezone` an app renders a label from + // paid for a fresh `Intl.DateTimeFormat` every time and an unknown one escaped as a `RangeError`. + const formatter = cachedFormatter(labelFormatters, `${canonical}|${tag}|${style}`, () => { + return new Intl.DateTimeFormat(tag, { + timeZone: canonical, + timeZoneName: style, + hourCycle: 'h23', + }); }); const label = formatter.formatToParts(at).find((part) => part.type === 'timeZoneName')?.value; - return label ?? offsetLabel(offsetAt(zone, at)); + return label ?? offsetLabel(offsetAt(canonical, at)); } /** @@ -145,6 +154,7 @@ export function observesDst(zone: TimeZone, at: Instant): boolean { } const formatters = new Map(); +const labelFormatters = new Map(); function partsFormatterFor(zone: TimeZone): Intl.DateTimeFormat { // Keyed on the canonical name, so 4,096 casings of one zone are one entry rather than 4,096. diff --git a/wiki/Error-Codes.md b/wiki/Error-Codes.md index 659d2571..55146b00 100644 --- a/wiki/Error-Codes.md +++ b/wiki/Error-Codes.md @@ -59,7 +59,7 @@ The three series `docker/helm` autoscales on are `http_requests_total` (the coun | Code | Means | Typical cause | Fix | |---|---|---|---| -| `X_METRIC_NAME_INVALID` | metric name is malformed or already declared with another kind | a name that is not lowercase `snake_case` — dotted OTel names survive OTLP but not a Prometheus scrape, and the autoscaler reads the scrape — or one name declared as both a counter and a gauge | rename the instrument: `counter('http_requests_total')`, one metric name to one kind | +| `X_METRIC_NAME_INVALID` | metric name is malformed, or redeclared with a different kind, bounds or observer | a name that is not lowercase `snake_case` — dotted OTel names survive OTLP but not a Prometheus scrape, and the autoscaler reads the scrape — or one name declared as both a counter and a gauge, or a second `histogram('x', { bounds })` / `gauge('x', { observe })` whose stated option differs from the first. An **omitted** option is still a handle-fetch, so `gauge(name)` keeps working | rename the instrument: `counter('http_requests_total')`, one metric name to one kind; or make the second declaration state the same `bounds`/`observe` as the first | | `X_METRIC_VALUE_INVALID` | metric value is not recordable | a `NaN`/`Infinity` observation, or a counter decremented — a counter is a cumulative total and only goes up | pass a finite value, and use `gauge(name)` for a number that can fall | | `X_METRIC_CARDINALITY` | a metric exceeded its series ceiling and is folding into one overflow series | an unbounded label at the call site — a user id, a path, an email — so every distinct value mints a series and the scrape grows without limit. Past `maxSeries` every further label set folds into one `otel_metric_overflow="true"` series, reported once per instrument rather than once per observation. Also raised at declaration for a `maxSeries` that is not a positive integer | drop the unbounded label from the call site, or raise it deliberately: `counter('orders_total', { maxSeries: 4000 })` | | `X_OTLP_ENDPOINT_INVALID` | the OTLP collector endpoint is missing or malformed | `OTEL_EXPORTER_OTLP_ENDPOINT` unset with none passed, a value that is not a URL, a scheme that is not `http`/`https`, or the collector's gRPC receiver on `:4317` — OTLP/HTTP JSON is served on `:4318`, and an exporter pointed at the wrong port drops every batch with a transport error and no metrics | `set OTEL_EXPORTER_OTLP_ENDPOINT=http://otel-collector:4318`, or skip the exporter when `tryOtlpEndpoint('')` is undefined | diff --git a/wiki/Routes-And-Render-Modes.md b/wiki/Routes-And-Render-Modes.md index 27e25bc8..c1f009bb 100644 --- a/wiki/Routes-And-Render-Modes.md +++ b/wiki/Routes-And-Render-Modes.md @@ -145,7 +145,7 @@ Failures name the *cause* — the transitive import that added the bytes — bec |---|---| | `meta.title` missing on an indexable route | build error `X_SEO_META_MISSING`, `cause` naming `title` and the file | | `meta.description` missing on an indexable route | the **same** code, `X_SEO_META_MISSING`, naming `description` — one code for both fields, with `fix: add description to meta in ` | -| Title over 60 chars, description over 160 | build error `X_SEO_META_TOO_LONG`, with the measured length. A **too-short** description is not checked: `DESCRIPTION_MIN_LENGTH` (50) is exported by `@ultimat3/seo` and read by no validator, so a 10-character description passes the gate `As of 2026-08` | +| Title over 60 chars, description over 160 | build error `X_SEO_META_TOO_LONG`, with the measured length. A **too-short** description is not checked and no minimum is exported `As of 2026-08`: a 10-character description passes the gate. `DESCRIPTION_MIN_LENGTH` used to be exported and read by no validator, which is the shape a length bound must not ship in — `validateMeta` enforces every bound `@ultimat3/seo` exports, pinned by a test | | Duplicate title/description across routes | build error — duplicate meta is a ranking bug, not a style issue | | `og.image` missing on a shareable route | build error; the generated fallback OG image must be opted into explicitly | | Broken internal link | build error, resolved against the route table |