Skip to content

Commit f1298b3

Browse files
test(runtime): judge each share-link password pair once in the refusal-order file, so its loop cases fit the 5000 ms default (#24704)
Fixes #24633 Clause-②: no ## What changes One test file: `packages/runtime/src/domains/share-links-refusal-order.test.ts`. Its `beforeAll` wraps the `ShareLinkService` instance's own password verifier in a memo keyed by the `(password, hash)` pair. The first request that presents a given pair is judged by the production verifier (scrypt at the account parameters). Every later request with the same pair replays that verdict. What stays the same: - Every request is still made. Every row of the table and every cell of the three loop cases' matrices is still asserted, against both mounts. - The service is still built with both password members at their defaults, as production builds it, so its legacy-hash upgrade stays on. No row in this file is legacy. - No case is skipped, quarantined, re-timed or re-asserted. - No per-case timeout, no `testTimeout` change and no `// test-budget:` header. The production verifier is not exported from `@objectstack/plugin-sharing`, so the memo reads it off the instance. If that member is renamed, `beforeAll` throws with a message that names the member. It does not quietly fall back to paying for every call. ## Measured cause: production scrypt verifications, not the resolves `resolveToken` calls the verifier once for each password presented to a live, password-protected link: - on a `signed_in` link, once the caller is signed in; - on an `email` link, whatever email was sent (the same-work rule from #23474). Each call derives a scrypt key at N=16384, r=16. I counted the calls with a temporary instrumented copy of the file. The copy wrapped the instance's verifier and hasher with counters keyed by `expect.getState().currentTestName`. It was deleted after the run and never committed (`git status` clean after each run). | | before (`eadea5720`) | after (`a70fd811f`) | |:--|--:|--:| | verifier calls per file run | 70 | 70 | | ...of which reach the production scrypt verifier | 70 | **6** | | mints with a password (`hashPassword`, in `beforeAll`) | 4 | 4 | | **scrypt derivations per file run** | **74** | **10** | | `:334` no-password-oracle loop (13 resolves), per mount | 8 | 0 | | `:346` allowlist-membership loop (8 resolves), per mount | 4 | 0 | | `:356` unknown-token loop (18 resolves), per mount | 10 | 0 | | most derivations in any one case | 10 | 1 | The 6 distinct pairs are the 3 live protected links (`signed_in + password`, `public + password`, `email + password`), each with a wrong password and with the correct one. The revoked link is refused before verification. A verdict depends only on its pair, so replaying it changes no answer. ## Readings **CI, before.** Run `39298888283`: - Attempt 1, job `118219765573` (`Test Core (6/6)`): the card's red. `:356`, dispatcher twin, failed with `Test timed out in 5000ms`. - Attempt 2, job `118235895669`: green on the same head. - The `Test Core` aggregate job `118247422913` prints the suite-ceiling report into its log. It lists this file as `@objectstack/runtime`'s slowest: `34.84s src/domains/share-links-refusal-order.test.ts [local]`. That is over the 10 s line in AGENTS.md (Build & Test). - The `test-core-timings-6-of-6` artifact could not be read from here. The artifact download redirects to blob storage, and this container's egress proxy answers that redirect 403. **Local runs.** The box has 4 vCPUs and is shared with sibling agents. Every run took the verify lock. Durations come from vitest's JSON reporter. The "file" figure is vitest's module line for the file. | reading | before (`eadea5720`) | after (`810176fb8`, `a70fd811f`) | |:--|:--|:--| | file, idle, `--maxWorkers=2` | 7778 / 7725 ms | 1330 / 1215 / 1296 ms | | `:334`, idle (plugin route / twin) | 796, 779 / 819, 803 ms | 17, 15 / 14, 13 ms | | `:346`, idle | 386, 410 / 398, 391 ms | 13, 8 / 8, 8 ms | | `:356`, idle | 1148, 1024 / 966, 1024 ms | 20, 19 / 18, 18 ms | | slowest case, idle | 1148 ms | 116 / 99 ms (a row with one fresh verification) | | contention: the file pinned to CPU 3 beside 5 busy loops on CPU 3, `--maxWorkers=1` | **3 failed, `Test timed out in 5000ms`**: `:356` plugin route 5050 ms, `:356` twin 5002 ms, `:334` twin 5001 ms. `:334` plugin route passed at 4808 ms. File 44531 ms. | **60 passed**. File 8685 ms. Loop cases 72 to 169 ms. Slowest case 638 ms. | Notes on these readings: - The "before" contention run reproduces the card's failure shape on the card's case: a timeout, not an assertion. It also shows `:334` is the next case to go. - The contention load is heavier than the CI shard that read 34.84 s: 44.53 s against 34.84 s, about 1.28x. - The two runs taken after the change differ only in a comment and a named type alias. The idle reading at the final head, `a70fd811f`, is the third one (1296 ms). **CI, after: NOT MEASURED** until this PR's `Test Core` runs. Two projections, both under 10 s: - 5.5 to 6.0 s, from the idle figures and the before ratio of CI to idle (34.84 / 7.75, about 4.5x); - about 6.8 s, from the contention figure divided by the contention-to-CI ratio (8685 ms / 1.28). ## Why not the suggested per-case timeout, and why there is no `test-budget` header - **A per-case timeout leaves the file over the 10 s line.** It would keep 4 to 10 derivations inside each loop case, and the file would stay near 35 s in CI. AGENTS.md says a file over 10 s of wall is its lane's card, unless the file declares a measured budget with its class. - **No class fits this file, so it cannot declare a budget.** `TEST_BUDGET_CLASSES` in `scripts/measure-test-shard-timings.mjs` is a closed list of three classes. Its reader refuses any other class: - `spawns the real producer once per single-cause fixture`; - `whole-package import`; - `compiles the package type graph once`. None of the three describes key derivations. - **So the change removes the cost instead.** The file now projects under the line, and the loop cases no longer need a budget. The slowest case under contention read 638 ms, 12.8% of the 5000 ms default. - **The precedents were read from their landed diffs.** - `b9222dc70` (#22426) sized per-case budgets for files whose subject is the hashing itself, and its rule was never to lower the scrypt parameters. That rule holds here: every verdict still comes from production scrypt at the production parameters. - `7b6c65290` (#21925, for #21920) moved a module load out of the clocked window. The shape does not transfer, because the cost here is the cases' own derivations. - #22356 has no landed diff to read. Its timeline's `closed` event carries no commit, and no commit on `origin/main` cites it. As a control, the same grep finds `b9222dc70` for #22426. ## Mechanism assumptions, checked 1. *"About 18 sequential resolves per loop case."* **Partly falsified.** - The three cases make 13, 8 and 18 resolves. - Their cost is the 8, 4 and 10 production scrypt verifications among those resolves. The same resolves now read 8 to 20 ms per whole case. - The contended-shard reading holds: attempt 2 passed on the same head. 2. *"The matrix is needed whole."* **Confirmed, and kept.** No cell is dropped. ## Tests and gates All at `a70fd811f` (`git rev-parse --short HEAD`), the PR head. - `pnpm --filter '@objectstack/runtime^...' build` under the verify lock: `VERDICT command-exit 0`. - `pnpm --filter @objectstack/runtime exec vitest run --project local --maxWorkers=2 src/domains/share-links-refusal-order.test.ts`, under the lock: `Tests 60 passed (60)`, file line 1296 ms. - `pnpm --filter @objectstack/runtime typecheck`, under the lock: `VERDICT command-exit 0`. This includes `check:test-typecheck: OK`. The file is in `tsconfig.test.json`'s program: `tsc --listFilesOnly` counts it once. - `node scripts/pm/dispatch-gates.mjs --commands` derived 53 commands. Results: - 51 exited 0. - 2 exited 3 (`PREREQUISITE NOT MET`), so they are NOT MEASURED here: - `node scripts/check-plugin-teardown-shape.mjs --self-test` needs an unshallowed clone; - `pnpm check:dual-build-cjs-loads` needs the whole workspace's `dist`. CI runs both. - `--ran` reconciliation: `53 derived famil(ies) accounted for — 51 run, 2 NOT-MEASURED`, `0 UNRUN`. - `pnpm lint` (eslint over the repo): exit 0, no findings. - **Declared narrowing:** the whole `@objectstack/runtime` `local` suite was not run here. It reads about 1000 s in CI. The diff is one self-contained test file that shares no state with its siblings. `Test Core` runs the full suite. - No ablation: no tested behaviour changes. The before and after timings above are the evidence. ## No changeset (`skip-changeset`) Only a `*.test.ts` file changes. `@objectstack/runtime`'s `files[]` is `dist`, `README.md` and `CHANGELOG.md`, and tsup's only entry is `src/index.ts`. I built `dist` after the final commit and searched it: - the change's identifier `productionVerify`: 0 hits; - control: `handleShareLinksRequest`: 2 hits. ## Acceptance notes - **The memo reads a private member.** It reads `ShareLinkService`'s `verifyPassword` member, which is `private readonly` in TypeScript and is not exported. - If the member is renamed, `beforeAll` throws and names it. - If the service ever stops reading the verifier from that member, the memo stops applying and nothing else changes. The file gets slower again but never wrong, and the per-file timing report shows it. - **`:334` and `:346` are implied by `:356`.** `:356` proves every refused cell equals the unknown-token answer, and the other two cases' references are among those cells. This was not acted on: merging or deleting cases is held off, and none of the three costs measurable time now. - **A sibling file is also over the 10 s line.** `src/domains/share-links-messages-audience.test.ts` read 13.51 s in the same CI report. It mints with the same production hasher over a password matrix, so it probably has the same cause. It is not touched here: it is outside this card's file surface. --- _Generated by [Claude Code](https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 014863d commit f1298b3

1 file changed

Lines changed: 35 additions & 1 deletion

File tree

‎packages/runtime/src/domains/share-links-refusal-order.test.ts‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@
5757
* probes recover it the fail-closed way), ONE `ShareLinkService` with the
5858
* production password hasher, and the password sent the way the console's
5959
* landing page sends it: the `X-Share-Password` header. Refusals are read as
60-
* the ADR-0112 envelope's `status` + `error.code`.
60+
* the ADR-0112 envelope's `status` + `error.code`. Every password verdict is
61+
* the production verifier's own; a pair it already judged is answered from
62+
* a memo of that verdict (see `beforeAll`).
6163
*/
6264

6365
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
@@ -170,6 +172,38 @@ beforeAll(async () => {
170172
await engine.insert(OBJECT, { id: RECORD, title: 'Shared doc' }, { context: { isSystem: true } } as any);
171173

172174
const svc = new ShareLinkService({ engine: engine as any });
175+
// [#24633] One scrypt verification per distinct (password, hash) pair, not
176+
// one per request. The production verifier derives a key at the account
177+
// parameters (scrypt N=16384, r=16) on every call, and `resolveToken` calls
178+
// it for each password presented to a live protected link, once a
179+
// `signed_in` link's caller is signed in, and on an `email` link whatever
180+
// email was sent. Counted at `eadea5720`: 70 calls, plus
181+
// the 4 mints below, and they were the file's wall time. It read 34.84 s in
182+
// CI (run 39298888283, Test Core 6/6, attempt 2), and the loop case with
183+
// 10 calls timed out at vitest's 5000 ms default on attempt 1. The calls
184+
// hold only 6 distinct pairs (3 protected live links x a wrong and the
185+
// correct password), and a verdict is a pure function of its pair, so each
186+
// pair is judged once by the real verifier and its verdict replayed.
187+
// Every request is still made, so every answer is still a real request's,
188+
// and the service is still built with both password members at their
189+
// defaults, as production builds it (so its legacy-hash upgrade stays on;
190+
// no row here is legacy). The memo wraps the instance's own verifier,
191+
// which is not exported. If that member moves, this throws rather than
192+
// leave the file paying for every call again.
193+
type Verify = (plain: string, hash: string) => Promise<boolean>;
194+
const productionVerify: unknown = (svc as any).verifyPassword;
195+
if (typeof productionVerify !== 'function') {
196+
throw new Error('ShareLinkService no longer keeps its password verifier as `verifyPassword`; re-point the memo below');
197+
}
198+
const verdicts = new Map<string, Map<string, Promise<boolean>>>();
199+
const memoVerify: Verify = (plain, hash) => {
200+
let byPlain = verdicts.get(hash);
201+
if (!byPlain) verdicts.set(hash, (byPlain = new Map()));
202+
let verdict = byPlain.get(plain);
203+
if (!verdict) byPlain.set(plain, (verdict = (productionVerify as Verify)(plain, hash)));
204+
return verdict;
205+
};
206+
(svc as any).verifyPassword = memoVerify;
173207
const creator = { isSystem: true, userId: 'usr_creator' } as any;
174208
const mint = async (audience: 'public' | 'signed_in' | 'email', password?: string) =>
175209
svc.createLink(

0 commit comments

Comments
 (0)