test(plugin-auth): load the batch-6 admin endpoint graphs at module top, not inside each clocked case - #15914
Conversation
…op, not inside each clocked case `durability-swallow-repair.test.ts`'s batch-6 cases reached `runAdminCreateUser` / `runAdminImportUsers` through `await import(...)` written inside the `it()` body. vitest wraps test bodies in `withTimeout(...)` and has no timeout knob covering module loading, so the FIRST of each group of structurally identical siblings charged that module graph's cold transform-and-import to its own 10 000 ms `testTimeout` while the rest hit the warm module cache. Under a loaded CI shard the first sibling ran out of budget and reddened PRs that read no part of this code. Loading at module top removes the clock rather than widening it: `collectTests()` awaits `runner.importFile()` bare and only records the duration for reporters. This is the repo-wide convention already stated in AGENTS.md and enforced for cross-package specifiers by `check-test-source-alias`. No assertion changed. The file has no `vi.mock`/`vi.resetModules`, so every case already shared one module instance via the module cache; the only edit to a case body is the removal of its import line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
…ld-import-timeout
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
Clause-② contract review — PR #15914 (card #14998)Tier. Independence. Dev line = a separate Subject verified. Head ⭐ The load-bearing premise, read against vitest 4.1.10 (lockfile-resolved, installed)
1. Coverage identical — PASS (blocking bar)
2. Mutation leg — PASS (blocking bar)Re-run in my worktree under Same hashes as the dev's report. The target reds on an assertion in 8 ms (dev: 9 ms), not a timeout; the two siblings that assert 3. Scaled-budget demonstration — PASS, honestly labelledCode held fixed; test file swapped on disk to the CI's signature — first sibling only — reproduces at 2000 ms, and the dev's "at 500 ms all three red" claim reproduces too. The PR body labels this as "a scaled budget, not a reproduction of CI's load" ("Honest boundary first"); the report matches. These are shared-box seconds, as the lock wrapper itself prints; the ratios are what carry. 4. The
|
PM sign-off — undrafted and armed, with three corrections recorded rather than reworkedReview is PASS and both blocking bars were re-measured independently, not read off the report. Undrafting on that.
|
Fixes #14998
Fixes #15852
Referenced and deliberately left open: #15603 — see "What each report does on merge" below.
Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y (stated here in prose because a body PATCH downgrades the footer's session link to the bare form).
durability-swallow-repair.test.ts's batch-6 cases reachedrunAdminCreateUser/runAdminImportUsersthrough anawait import(...)written inside theit()body. vitest wraps test bodies inwithTimeout(...)and offers exactly three timeout knobs (testTimeout,hookTimeout,teardownTimeout), none of which covers module loading — so the first of each group of structurally identical siblings charged that module graph's cold transform-and-import to its own 10 000 ms budget, while the rest hit the warm module cache. Only the first ever failed, and it failed on load, not on behaviour.The fix moves both loads to module top. That is not a widened budget — it removes the clock:
collectTests()awaitsrunner.importFile(filepath, 'collect')bare and only records the duration for reporters.The defect sat in the un-enforced notch of a rule this repo already has
scripts/check-test-source-alias.mjsalready enforces this convention — its own failure text carries it, and AGENTS.md § Build & Test states it for authors:It did not fire here for one reason: its population is specifiers that resolve through
dist/, and./admin-import-users.jsis a relative in-package specifier. So this was not an unknown hazard — it was the same defect one notch below an existing gate's population. ⛔ Widening that gate is out of scope for this PR and is not attempted here.Why this shape rather than a longer timeout
The precedents #14998 names (#5421, #3662) widened a budget because in those tests the cold load WAS the subject under test. Here it is not: the subject is a
warnline. The principle transfers — don't make the test do less — while the remedy does not.The card enumerated three options (per-case timeout,
beforeAllhoist, file-leveltestTimeout); module top is a fourth, and it was chosen on a measurement the enumeration predates:beforeAllmoves the load, it does not unclock it.beforeAllis wrapped byhookTimeout, which is also10_000in this package'svitest.config.ts. Option B would put a 3299.6 ms load into an equally sized window — it stops the load being shared with assertions but keeps a clock on it.Measurements
A green run proves nothing on this card (attempt 2 was already green on the same commit), so every number below is a differential.
Cold import cost of
./admin-import-users.js, in this file's environment — measured by a throwaway probe replicating this file's module-top import set, run and deleted inside one trapped script:./admin-user-endpoints.js./admin-import-users.jsThe heavy leg is
@objectstack/rest(prepareImportRequest,runImport), whichadmin-import-users.tsvalue-imports. Measured on a shared box under the repo's verify lock, so it is not an idle-box figure; the ratios below are what survive contention. Note the 69× spread between the two cold loads above — it is the reason cost cannot be assumed for the sites in the census.Headroom. The paying case's assertions need 3.78 ms (vitest json reporter, post-fix).
The failure, demonstrated before the fix. Honest boundary first: this is a scaled budget, not a reproduction of CI's load. Holding the code fixed and shrinking the budget to 2000 ms puts the cold import at 1.6 budgets — the same inequality CI hit at 10 000 ms — and reproduces CI's signature exactly, only the first sibling reds:
Same site
:673every report names; the two warm siblings pass in both runs. (At 500 ms all three red, because the 3.3 s load then spans six budgets and the later siblings await the same in-flight module — which is why 2000 ms is the faithful scaling.)The assertions still discriminate. The real risk with any timeout change is widening a window until a broken test passes, so
admin-import-users.tswas mutated to make the pin's claim false — the #12981 durability report short-circuited to a no-op, reintroducing the swallow:Exactly one case reds, it reds on an assertion in 9 ms rather than a timeout, and its two siblings (which assert
warnwas not called) stay green — so the pin discriminates on the behaviour, precisely, and the widened-window failure mode is ruled out. Restore ran under atrap … EXIT INT TERMwith absolute paths. No rebuild leg is owed: the specifier is relative and vite transformssrc/directly, so nodist/sits between the mutation and the test — which the red itself proves.Coverage is identical
Not asserted — counted, before vs after:
The only removed lines are the 7 import statements (
git diff -U0 | grep '^-'shows nothing else). Nothing is skipped, disabled or quarantined, and no assertion was deleted. There is novi.mock/vi.resetModulesin this file, so every case already shared one module instance through the module cache; module-top loading gives them the same instance.What each report does on merge
Three cards describe this one defect at this one line. They do not all end here, and the split is deliberate:
domain:engine, from PR #14926domain:devx, 2 reds on PR #15791:673, same10000ms, same mechanism; its own body records that its dedup was blind (/search/issues403 for that seat), so it is a genuine duplicate report of a defect this PR genuinely fixesdomain:devx, 3 PR-CI reds in one hour#15603 is a card about the shard, not about this test. Its subject is "three unrelated PRs red on it in one hour while merge-group runs of the same shard passed". This PR removes the one site whose cost has actually been measured (3299.6 ms). The census filed as #15916 records five more first-sibling-pays sites in
plugin-authtest files, all of which the sameTest Coreshard runs, and none of them measured — and the 69× spread between the two cold loads above shows that cheap-or-expensive is not knowable without measuring. Closing #15603 on this merge would assert the shard-level symptom is gone while five sites of identical shape remain. So it stays open, to be judged on an observation of the shard rather than on this merge.Census (the sweep #14998 asks for)
Anchor: an
await import('…')whose nearest enclosing construct is anit()/test()body, over all 97plugin-authtest files. Control that fires:credential-at-rest-posture.test.ts→@better-auth/scimis detected with its explicit60_000— so aNONEreading is a real measurement, not a blind spot; three module-scope sites are also correctly classified as compliant.The card's specific question — do the other describe blocks in this file share the shape? The sibling batch-6 block did (
admin-user-endpoints :: writeAdminAudit, 4 sites, first payerit@576) and is fixed by this same edit. Batch 5's two blocks never had it: they import./auth-managerand./auth-pluginstatically at module top.Sites in this file: 7 → 0. Package-wide: 22 → 15. The remaining 5, all with no explicit per-case timeout and no measured cost, are filed as #15916 rather than widened into this PR:
admin-user-endpoints.test.ts./admin-user-endpoints.jsit@74auth-manager.test.tsbetter-auth/apiit@2209auth-manager.test.ts./rate-limit-storage.jsit@2358admin-impersonate-endpoint.test.tsbetter-authit@358managed-extension-fields.test.tsbetter-auth/pluginsit@1046One disclosed scanner artifact: a row for specifier
xinrate-limit-storage-isolation.test.ts:128is prose inside a comment (the scanner does not mask comments); it is classified module-scope and is not in the finding set.Verification
All at head
6b3c135fe, withorigin/mainmerged in (never rebased, never force-pushed).pnpm --filter @objectstack/plugin-auth exec vitest run→ exit 0,Test Files 97 passed (97),Tests 2067 passed (2067)pnpm --filter @objectstack/plugin-auth typecheck→ exit 0 (all three legs; needs the package's owndist/, so it is built first)check:test-typecheck→ exit 0, its own verdict line:OK — @objectstack/plugin-auth's test layer compiles under packages/plugins/plugin-auth/tsconfig.test.json.tsc -p tsconfig.jsonexcludes*.test.ts(--listFileshit count for the edited file: 0), so that leg is NOT what covers this change;tsconfig.test.jsonis, and--listFilesthere returns 1 for the edited file with a positive control (auth-manager.ts= 1) and a negative control (README.md= 0).node scripts/check-adr-0087-registration.mjs --base origin/main --head 6b3c135fe→ exit 0node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackfrom the actual change set (1 path), never a hand-built list: 55 families, 52 exit 0. The three that are not:check:dual-build-cjs-loadsandcheck:published-readme-exports→ exit 3 = PREREQUISITE NOT MET (they read built output; this worktree has no fullpnpm build) andcheck:react-declaration-parity→ exit 1 for a missing browser-producedsdui.manifest.json. None is a pass and none is a red on this diff — all three are structurally independent of a singleplugin-authtest file. CI has the prerequisites.skip-changeset:plugin-authpublishesfiles: ["dist","README.md","CHANGELOG.md"], so asrc/*.test.tsedit releases nothing.🤖 Generated with Claude Code