Skip to content

Commit 575ce83

Browse files
claude[bot]claude
andauthored
perf(core): cheapen the authz transport scan so it stops aborting the shard (#13656)
`authz-store-unavailable.test.ts` rebuilt its transport ledger from source TWICE per run — once in the CONTROL test, once in the SET EQUALITY test — and each rebuild walked all of `packages/`, `statSync`'d every directory entry, and read every `.ts` file into a UTF-8 string before discarding 59% of them for their path. Measured on this tree: 10,152 file opens and 147.5 MB decoded into transient JS strings per run, against vitest's inherited 5000 ms default. On CI that timed out, and a timeout ABORTS THE SHARD — so one slow test cost eleven other packages their entire run, on PRs that never touched authorization. Four changes, none of which touch what the suite asserts: - the scaffolding path filter runs BEFORE the read instead of after it. A path belongs to the result iff it both contains the call and is not scaffolding, and set intersection does not care which half is tested first, so the reordering is semantically free: 2,979 of 5,076 files are no longer read in full only to be thrown away. - the needle is matched against BYTES. The needle is pure ASCII and an ASCII byte never occurs inside a multi-byte UTF-8 sequence, so a byte hit and a decoded-string hit are the same hit — with no 147 MB decode in between. - `readdirSync(dir, { withFileTypes: true })` answers "is this a directory?" from the readdir result, replacing 5,926 `statSync` calls. The symlink limb keeps the old follow-the-link semantics exactly, so a transport behind a symlinked directory still cannot escape the ledger. - the enumeration is computed once per PROCESS. It is still rebuilt FROM SOURCE on every run, which is the guarantee the #13279 ruling requires; it is simply not rebuilt twice for one answer. The 5000 ms budget was inherited, never chosen, and was measurably the wrong budget for a filesystem scan. The two scanning tests now state one explicitly. It is a budget, not a timing assertion — deliberately not `expect(elapsed).toBeLessThan(n)`, which on a shared runner is flaky by construction. Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L Co-authored-by: Claude <noreply@anthropic.com>
1 parent 967402a commit 575ce83

1 file changed

Lines changed: 79 additions & 8 deletions

File tree

packages/core/src/security/authz-store-unavailable.test.ts

Lines changed: 79 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -192,39 +192,110 @@ const isScaffolding = (rel: string) =>
192192
|| /\.fixtures?\.ts$/.test(rel) || rel.includes(`${sep}dogfood${sep}`);
193193

194194
function walk(dir: string, out: string[] = []): string[] {
195-
for (const entry of readdirSync(dir)) {
196-
if (entry === 'node_modules' || entry === 'dist' || entry === '.turbo') continue;
197-
const full = join(dir, entry);
198-
if (statSync(full).isDirectory()) walk(full, out);
195+
// `withFileTypes` answers "is this a directory?" from the `readdir` result
196+
// itself, so the walk costs one syscall per DIRECTORY instead of one per
197+
// ENTRY (5,926 `statSync` calls on this tree). The symlink limb preserves the
198+
// old `statSync` semantics exactly, and is not optional: `Dirent.isDirectory()`
199+
// describes the LINK, not its target, so without it a symlinked directory
200+
// would stop being descended and a transport behind one would drop out of the
201+
// ledger's reach — the one silence this suite exists to prevent.
202+
for (const entry of readdirSync(dir, { withFileTypes: true })) {
203+
if (entry.name === 'node_modules' || entry.name === 'dist' || entry.name === '.turbo') continue;
204+
const full = join(dir, entry.name);
205+
if (entry.isDirectory() || (entry.isSymbolicLink() && statSync(full).isDirectory())) walk(full, out);
199206
else if (full.endsWith('.ts')) out.push(full);
200207
}
201208
return out;
202209
}
203210

211+
/**
212+
* The call this ledger tracks, as BYTES. `readFileSync(f)` hands back the
213+
* file's bytes with no UTF-8 decode, and `Buffer.prototype.includes` searches
214+
* them directly.
215+
*
216+
* The two spellings cannot disagree: the needle is pure ASCII, and an ASCII
217+
* byte never occurs inside a multi-byte UTF-8 sequence (continuation bytes are
218+
* all >= 0x80), so a byte hit and a decoded-string hit are the same hit — with
219+
* no lossy-replacement step in between. Measured on this tree, decoding was
220+
* 147 MB of transient JS strings per suite run and roughly half the scan's cost.
221+
*/
222+
const TRANSPORT_CALL = Buffer.from('resolveAuthzContext({');
223+
204224
/** Every PRODUCTION file that calls `resolveAuthzContext`, rebuilt from source. */
205-
function discoverTransports(): string[] {
225+
function scanTransports(): string[] {
206226
return walk(join(REPO_ROOT, 'packages'))
207-
.filter((f) => readFileSync(f, 'utf8').includes('resolveAuthzContext({'))
208227
.map((f) => relative(REPO_ROOT, f).split(sep).join('/'))
228+
// ⭐ The scaffolding filter runs BEFORE the read, not after it. A path
229+
// belongs to the result iff it BOTH contains the call AND is not
230+
// scaffolding; set intersection does not care which half is tested first,
231+
// so the reordering is semantically free. What it removes is 2,979 of
232+
// 5,076 files (56% of the bytes) that were read into memory in full and
233+
// only then thrown away for their path.
209234
.filter((rel) => !isScaffolding(rel.split('/').join(sep)))
235+
.filter((rel) => readFileSync(join(REPO_ROOT, rel)).includes(TRANSPORT_CALL))
210236
.sort();
211237
}
212238

239+
/**
240+
* ⛔ Computed once per PROCESS, and only ever FROM SOURCE.
241+
*
242+
* Both tests below need the whole enumeration, and deriving it twice walked and
243+
* read the tree twice for one answer — the cost that timed this suite out at
244+
* the default 5000 ms and aborted the shard around it.
245+
*
246+
* What this cache must NEVER become is a checked-in list, a snapshot fixture,
247+
* or a cache keyed on anything that can outlive a commit. The enumeration is
248+
* rebuilt from source on every run precisely so a transport added later cannot
249+
* inherit the old silence unnoticed; a curated list would answer only "the
250+
* doors I remembered". A fresh process — that is, every run of this suite —
251+
* walks the tree again. The copy is returned so no caller can mutate the
252+
* enumeration out from under the other test.
253+
*/
254+
let SCANNED: readonly string[] | undefined;
255+
function discoverTransports(): string[] {
256+
return [...(SCANNED ??= scanTransports())];
257+
}
258+
259+
/**
260+
* The budget for the two tests that SCAN, stated rather than inherited.
261+
*
262+
* vitest's default 5000 ms is the budget for a test that does no I/O, and this
263+
* suite inherited it silently. It was not a decision, and it was measurably the
264+
* wrong one: the scan blew it on CI while passing on a developer box, so the
265+
* only signal anyone got was `Test timed out in 5000ms` on PRs that had not
266+
* touched authorization — and because a timeout ABORTS THE SHARD, one slow test
267+
* cost eleven other packages their entire run.
268+
*
269+
* That asymmetry is why this number is generous rather than tight. A budget set
270+
* close to the observed cost buys nothing (the scan is not a thing we want to
271+
* race) and risks the catastrophic, non-local failure again on a slow runner; a
272+
* generous one costs nothing when the test passes. Measured on this tree with a
273+
* COLD page cache, the scan is 255 ms, so this is ~118x its measured cost.
274+
*
275+
* ⛔ This is a BUDGET, not an assertion about speed — deliberately not
276+
* `expect(elapsed).toBeLessThan(n)`, which on a shared CI runner is flaky by
277+
* construction and would just re-file this card's successor. And it is not the
278+
* repair: the repair is that the scan reads 2,097 files once instead of 5,076
279+
* twice. If this budget is ever reached, the tree has outgrown a linear scan and
280+
* the answer is to re-engineer it, ⛔ never to raise this number.
281+
*/
282+
const SCAN_BUDGET_MS = 30_000;
283+
213284
describe('[#13279] every transport that authorizes through resolveAuthzContext', () => {
214285
it('CONTROL: the scanner finds transports at all, and finds THIS repo', () => {
215286
// Without this, a broken walk would return [] and the set-equality audit
216287
// below would be comparing two empty sets and passing.
217288
const found = discoverTransports();
218289
expect(found.length).toBeGreaterThanOrEqual(8);
219290
expect(found).toContain('packages/rest/src/rest-server.ts');
220-
});
291+
}, SCAN_BUDGET_MS);
221292

222293
it('⭐ SET EQUALITY: the ledger names exactly the transports source contains', () => {
223294
// A NEW transport is red here until it is classified — which is the whole
224295
// point: the ruling is about every transport, including the ones written
225296
// after it.
226297
expect(discoverTransports()).toEqual(Object.keys(TRANSPORT_LEDGER).sort());
227-
});
298+
}, SCAN_BUDGET_MS);
228299

229300
it.each(Object.entries(TRANSPORT_LEDGER))(
230301
'%s (%s) — a fail-closed catch re-raises the outage instead of degrading it',

0 commit comments

Comments
 (0)