Skip to content

Commit e78fff1

Browse files
committed
test: give slow-work budgets one home and one rule
Four commits have now raised a Windows budget individually (3e95ba3, 217db04, 81f3f68, d2eb93d), each deciding a number on its own. That is how you end up with 52 timeout literals and no way to tell a justified budget from a silenced flake. tests/helpers/test-budget.ts states the rule instead of the number. A budget may be raised only when both hold: the wait is intrinsic to what the test asserts, and the ablation still fails. The first without the second is how a vacuous test gets locked in, which is the fault this round has been chasing since 020. The contrast is written down because it is the whole judgment: the tray test launches PowerShell, then Bun, then rebinds the port, so the processes ARE the proof and it gets a budget. The sidebar route tests spawned gh incidentally while claiming route reachability, so they had the spawn deleted instead. Same symptom, opposite treatment. INTERNAL_DEADLINE_MS covers the case that cost the most time to see: a deadline inside a test, shorter than the test budget, fails faster than a timeout and reads as a logic error. WS-REBIND-01 died at 748ms against its own hardcoded 1s while the budget was 5s. storage-cleanup's 22 literals now come from one constant. Its slowest test runs in ~7ms locally and blew a 20s budget on CI -- a gap of three orders of magnitude, which is the argument for sizing these to contention rather than to a local stopwatch.
1 parent d2eb93d commit e78fff1

4 files changed

Lines changed: 79 additions & 26 deletions

File tree

tests/helpers/test-budget.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/**
2+
* Test budgets for work that is genuinely slow, not for making red go away.
3+
*
4+
* Bun's default is 5s. That is generous for a pure function and thin for anything
5+
* that spawns a process, binds a server, or writes SQLite on a loaded CI runner —
6+
* which is why a run of windows-latest failures kept blaming a different test each
7+
* time. The blame moved; the cause did not.
8+
*
9+
* ## When you may raise a budget
10+
*
11+
* Both must hold:
12+
*
13+
* 1. **The wait is intrinsic to the assertion.** The tray socket-inheritance test
14+
* launches PowerShell, which launches Bun, then rebinds the port to prove the
15+
* child never inherited the listen socket. Those processes ARE the proof, so it
16+
* gets a budget. The sidebar route tests spawned `gh` only incidentally while
17+
* claiming route reachability, so they got the spawn DELETED instead. Ask which
18+
* kind you have before reaching for a number.
19+
* 2. **The ablation still fails.** Revert the production behaviour the test covers
20+
* and confirm the test goes red. A budget that hides a vacuous test is worse than
21+
* the flake, because it converts a reliability signal into silence.
22+
*
23+
* If only (1) holds, you have not finished. If neither holds, delete the wait.
24+
*
25+
* ## Why these numbers
26+
*
27+
* They are headroom against runner contention, not measured durations. Local timings
28+
* are typically two to four orders of magnitude smaller: the storage cleanup test
29+
* that blew a 20s CI budget runs in ~7ms here. Sizing to the local number is what
30+
* produced the original 5s failures.
31+
*/
32+
33+
/** Real child process: PowerShell, a CLI smoke test, an external binary. */
34+
export const SPAWN_BUDGET_MS = 45_000;
35+
36+
/** Binds a real server or opens a real socket, including restart-and-reconnect flows. */
37+
export const SERVER_BUDGET_MS = 30_000;
38+
39+
/** Touches SQLite or the filesystem repeatedly. Slow on Windows for reasons outside our code. */
40+
export const STORE_BUDGET_MS = 30_000;
41+
42+
/**
43+
* A deadline *inside* a test, for an await that would otherwise hang forever.
44+
*
45+
* Keep these at least a few times under the surrounding budget. An internal deadline
46+
* shorter than the test budget fails faster than a timeout and reads as a logic error:
47+
* WS-REBIND-01 died at 748ms against its own hardcoded 1s while the budget was 5s, and
48+
* that mismatch is exactly why it looked random rather than slow.
49+
*/
50+
export const INTERNAL_DEADLINE_MS = 15_000;

tests/server-auth.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { tmpdir } from "node:os";
66
import { join } from "node:path";
77
import { saveCodexAccountCredential } from "../src/codex/account-store";
88
import { clearCodexWebSocketRegistry, getTrackedCodexWebSocketCountForAccount } from "../src/codex/websocket-registry";
9+
import { INTERNAL_DEADLINE_MS, SERVER_BUDGET_MS } from "./helpers/test-budget";
910
import { clearAccountNeedsReauth, clearAccountQuota, getAccountQuota, isAccountNeedsReauth, markAccountNeedsReauth, updateAccountQuota } from "../src/codex/auth-api";
1011
import {
1112
CODEX_THREAD_AFFINITY_IDLE_TTL_MS,
@@ -1294,7 +1295,7 @@ describe("server local API auth", () => {
12941295
if (globalThis.fetch === matrixFetch) globalThis.fetch = originalGlobalFetch;
12951296
await upstream.stop(true);
12961297
}
1297-
}, { timeout: 30_000 });
1298+
}, { timeout: SERVER_BUDGET_MS });
12981299

12991300
test("internal web-search and vision never forward a non-ChatGPT bearer as Direct sidecar auth", async () => {
13001301
if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true });
@@ -2109,7 +2110,7 @@ describe("server local API auth", () => {
21092110
// WebSocket, and waits for a two-hop retry across accounts — a second of that on a
21102111
// contended runner is optimistic. The assertions below are unchanged; only the
21112112
// room to reach them grew.
2112-
setTimeout(() => reject(new Error("websocket retry timed out")), 15_000);
2113+
setTimeout(() => reject(new Error("websocket retry timed out")), INTERNAL_DEADLINE_MS);
21132114
});
21142115
expect(harness.dispatches).toEqual(["acct-pool-a", "acct-pool-b"]);
21152116
expect(registrySnapshots).toEqual([[1, 0], [0, 1]]);

tests/storage-cleanup.test.ts

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
selectOldestPercent,
2626
type ExecuteCleanupOptions,
2727
} from "../src/storage/cleanup";
28+
import { STORE_BUDGET_MS } from "./helpers/test-budget";
2829

2930
const OLD = new Date("2026-01-01T00:00:00Z");
3031
const MID = new Date("2026-02-01T00:00:00Z");
@@ -418,7 +419,7 @@ describe("executeArchivedCleanup", () => {
418419
try { logsRead?.close(); } catch { /* */ }
419420
try { memoriesRead?.close(); } catch { /* */ }
420421
}
421-
}, { timeout: 20_000 });
422+
}, { timeout: STORE_BUDGET_MS });
422423

423424
test("rolls back staged renames when a later rename fails", () => {
424425
home = buildHome();
@@ -639,7 +640,7 @@ describe("executeArchivedCleanup", () => {
639640
const ids = state.query<{ id: string }, []>("SELECT id FROM threads").all().map(r => r.id);
640641
state.close();
641642
expect(ids).toEqual(["active"]);
642-
}, { timeout: 20_000 });
643+
}, { timeout: STORE_BUDGET_MS });
643644

644645
test("threads read failure leaves every file and database unchanged", () => {
645646
home = buildHome({ withSatelliteStores: true });
@@ -666,7 +667,7 @@ describe("executeArchivedCleanup", () => {
666667
expect(Buffer.compare(beforeGoals, readFileSync(join(home, "goals_1.sqlite")))).toBe(0);
667668
expect(Buffer.compare(beforeMemories, readFileSync(join(home, "memories_1.sqlite")))).toBe(0);
668669
expect(Buffer.compare(beforeState, readFileSync(join(home, "state_5.sqlite")))).toBe(0);
669-
}, { timeout: 20_000 });
670+
}, { timeout: STORE_BUDGET_MS });
670671

671672
// Windows CI: injected satellite rollback paths (especially goals) can measure 6–13s
672673
// there and trip bun's default 5s harness timeout.
@@ -723,7 +724,7 @@ describe("executeArchivedCleanup", () => {
723724
expect(stateAfter.query("SELECT id, rollout_path, archived FROM threads ORDER BY id").all()).toEqual(threads);
724725
stateAfter.close();
725726
},
726-
{ timeout: 30_000 },
727+
{ timeout: STORE_BUDGET_MS },
727728
);
728729

729730
test("satellite restore failure keeps recovery trashDir and manifest", () => {
@@ -922,7 +923,7 @@ describe("executeArchivedCleanup", () => {
922923
job_key: (expected as { job_key: string }).job_key,
923924
});
924925
}
925-
}, { timeout: 30_000 });
926+
}, { timeout: STORE_BUDGET_MS });
926927

927928
test("permanent cleanup works with logs-only satellite store", () => {
928929
home = buildHome({ satellites: "logs" });
@@ -1012,7 +1013,7 @@ describe("executeArchivedCleanup", () => {
10121013
"active", "tmid", "tnew", "told",
10131014
]);
10141015
state.close();
1015-
}, { timeout: 30_000 });
1016+
}, { timeout: STORE_BUDGET_MS });
10161017

10171018
// Windows CI: same multi-satellite restore profile as above (timed out at 5s on PR #558).
10181019
test("concurrent consolidate enqueue watermark change is preserved on restore", () => {
@@ -1050,7 +1051,7 @@ describe("executeArchivedCleanup", () => {
10501051
"active", "tmid", "tnew", "told",
10511052
]);
10521053
state.close();
1053-
}, { timeout: 30_000 });
1054+
}, { timeout: STORE_BUDGET_MS });
10541055
});
10551056

10561057
describe("listTrashEntries + restoreTrashEntry", () => {
@@ -1441,7 +1442,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
14411442
has_user_event: 1,
14421443
archived: 1,
14431444
});
1444-
}, { timeout: 20_000 });
1445+
}, { timeout: STORE_BUDGET_MS });
14451446

14461447
test.each([
14471448
["failAfterStateCommit", { failAfterStateCommit: true }, "db_reconcile_failed"],
@@ -1498,7 +1499,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
14981499
expect(logsAfter.query("SELECT COUNT(*) AS n FROM logs WHERE thread_id='told'").get()).toEqual({ n: 1 });
14991500
logsAfter.close();
15001501
},
1501-
{ timeout: 20_000 },
1502+
{ timeout: STORE_BUDGET_MS },
15021503
);
15031504

15041505
test("late failure after logs commit keeps metadata, persists pending sections, and resume preserves pre-existing rows", () => {
@@ -1602,7 +1603,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
16021603
goals.query("SELECT COUNT(*) AS n FROM thread_goals WHERE thread_id IN ('told','tmid','tnew')").get(),
16031604
).toEqual({ n: 2 }); // fixture seeds told+tmid only
16041605
goals.close();
1605-
}, { timeout: 20_000 });
1606+
}, { timeout: STORE_BUDGET_MS });
16061607

16071608
test("leftover-stage failure never restages files and retry accepts destinations", () => {
16081609
// Regression for reverse-move failure after metadata compensation: restage
@@ -1660,7 +1661,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
16601661
logsAfter.query("SELECT ts, target FROM logs WHERE id=1").get(),
16611662
).toEqual({ ts: 42, target: "pre" });
16621663
logsAfter.close();
1663-
}, { timeout: 20_000 });
1664+
}, { timeout: STORE_BUDGET_MS });
16641665

16651666
test("initial restore-pending write failure moves no files", () => {
16661667
home = buildHome({ withSatelliteStores: true });
@@ -1684,7 +1685,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
16841685
expect(retried.ok).toBe(true);
16851686
expect(retried.count).toBe(3);
16861687
expect(existsSync(stage)).toBe(false);
1687-
}, { timeout: 20_000 });
1688+
}, { timeout: STORE_BUDGET_MS });
16881689

16891690
test("interrupted pending update preserves the previous valid marker", () => {
16901691
home = buildHome({ withSatelliteStores: true });
@@ -1721,7 +1722,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
17211722
expect(retried.ok).toBe(true);
17221723
expect(retried.error).toBeUndefined();
17231724
expect(existsSync(stage)).toBe(false);
1724-
}, { timeout: 20_000 });
1725+
}, { timeout: STORE_BUDGET_MS });
17251726

17261727
test("crash after file move retries without dest_exists or fs_failed", () => {
17271728
home = buildHome({ withSatelliteStores: true });
@@ -1747,7 +1748,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
17471748
expect(retried.error).not.toBe("fs_failed");
17481749
expect(retried.count).toBe(3);
17491750
expect(existsSync(stage)).toBe(false);
1750-
}, { timeout: 20_000 });
1751+
}, { timeout: STORE_BUDGET_MS });
17511752

17521753
test("mid-move failure keeps placed dest, marker, and resumes without dest_exists", () => {
17531754
// First rename succeeds, second throws. Do not reverse the first file or drop
@@ -1801,7 +1802,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
18011802
expect(existsSync(join(home, "archived_sessions", "rollout-old.jsonl"))).toBe(true);
18021803
expect(existsSync(join(home, "archived_sessions", "rollout-mid.jsonl"))).toBe(true);
18031804
expect(existsSync(join(home, "archived_sessions", "rollout-new.jsonl"))).toBe(true);
1804-
}, { timeout: 20_000 });
1805+
}, { timeout: STORE_BUDGET_MS });
18051806

18061807
test("malformed restore-pending.json is not treated as a fresh restore", () => {
18071808
home = buildHome({ withSatelliteStores: true });
@@ -1819,7 +1820,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
18191820
expect(existsSync(join(stage, "rollout-old.jsonl"))).toBe(true);
18201821
expect(existsSync(join(home, "archived_sessions", "rollout-old.jsonl"))).toBe(false);
18211822
expect(readFileSync(join(stage, "restore-pending.json"), "utf8")).toBe("{not-valid-json");
1822-
}, { timeout: 20_000 });
1823+
}, { timeout: STORE_BUDGET_MS });
18231824

18241825
test("resume with owed satellite sections and missing backup fails closed", () => {
18251826
home = buildHome({ withSatelliteStores: true });
@@ -1850,7 +1851,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
18501851
expect(failed.error).toBe("db_reconcile_failed");
18511852
expect(existsSync(stage)).toBe(true);
18521853
expect(existsSync(join(stage, "manifest.json"))).toBe(true);
1853-
}, { timeout: 20_000 });
1854+
}, { timeout: STORE_BUDGET_MS });
18541855

18551856
test("resume with owed logs section but missing logs in backup fails closed", () => {
18561857
home = buildHome({ withSatelliteStores: true });
@@ -1881,7 +1882,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
18811882
expect(existsSync(stage)).toBe(true);
18821883
expect(existsSync(join(stage, "manifest.json"))).toBe(true);
18831884
expect(existsSync(join(stage, "restore-pending.json"))).toBe(true);
1884-
}, { timeout: 20_000 });
1885+
}, { timeout: STORE_BUDGET_MS });
18851886

18861887
test("failed tombstone rename keeps stage recoverable and listed", () => {
18871888
home = buildHome();
@@ -1905,7 +1906,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
19051906
expect(retried.ok).toBe(true);
19061907
expect(existsSync(stage)).toBe(false);
19071908
expect(listTrashEntries(home)).toEqual([]);
1908-
}, { timeout: 20_000 });
1909+
}, { timeout: STORE_BUDGET_MS });
19091910

19101911
test("tombstone delete failure reports success without phantom trash entry", () => {
19111912
home = buildHome();
@@ -1925,7 +1926,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
19251926
const trashRoot = join(home, ".trash");
19261927
const tombstones = readdirSync(trashRoot).filter(n => n.startsWith(".tombstone-"));
19271928
expect(tombstones.length).toBe(1);
1928-
}, { timeout: 20_000 });
1929+
}, { timeout: STORE_BUDGET_MS });
19291930

19301931
test("cleanup rejects overlap with accepted restore-pending destinations after state-commit failure", () => {
19311932
home = buildHome({ withSatelliteStores: true });
@@ -1961,7 +1962,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
19611962
const retry = restoreTrashEntry(trashId, { codexHome: home });
19621963
expect(retry.ok).toBe(true);
19631964
expect(existsSync(join(home, restoredRel))).toBe(true);
1964-
}, { timeout: 20_000 });
1965+
}, { timeout: STORE_BUDGET_MS });
19651966

19661967
test("cleanup rejects overlap with accepted restore-pending destinations after file-move failure", () => {
19671968
home = buildHome();
@@ -1996,7 +1997,7 @@ describe("listTrashEntries + restoreTrashEntry", () => {
19961997

19971998
const retry = restoreTrashEntry(trashId, { codexHome: home });
19981999
expect(retry.ok).toBe(true);
1999-
}, { timeout: 20_000 });
2000+
}, { timeout: STORE_BUDGET_MS });
20002001

20012002
test("percent preview backfills past pending oldest archive for manual cleanup", () => {
20022003
home = buildHome();

tests/windows-tray.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
} from "../src/tray/windows";
2020
import { handleManagementAPI } from "../src/server/management-api";
2121
import type { OcxConfig } from "../src/types";
22+
import { INTERNAL_DEADLINE_MS, SPAWN_BUDGET_MS } from "./helpers/test-budget";
2223

2324
const entry: WindowsTrayEntry = {
2425
bun: "C:\\사용자 공간\\%TEMP% ! ^ ( ) & 검증\\bun.exe",
@@ -257,8 +258,8 @@ describe("Windows tray packaging and command safety", () => {
257258
// round the sidebar route tests were fixed by DELETING their real `gh` spawn, because
258259
// spawning a binary was incidental to what those tests claimed. The distinction is
259260
// whether the wait is intrinsic to the assertion. Here it is; there it was not.
260-
const PID_FILE_WAIT_MS = 20_000;
261-
const TRAY_LAUNCH_TIMEOUT_MS = 45_000;
261+
const PID_FILE_WAIT_MS = INTERNAL_DEADLINE_MS;
262+
const TRAY_LAUNCH_TIMEOUT_MS = SPAWN_BUDGET_MS;
262263

263264
test("launches the detached tray host without retaining the proxy listen socket", async () => {
264265
if (process.platform !== "win32") return;

0 commit comments

Comments
 (0)