Repository navigation
fix(sse): de-flake timing-sensitive combo cooldown/breaker tests (#6803) - #6897
Conversation
Extracts 3 wall-clock-sensitive assertions (combo-quota-share cooldown ceiling x2, circuit-breaker HALF_OPEN race) into tests/unit/serial/ (--test-concurrency=1, the repo's established remedy for this class of test) and widens their margins, since a starved CI-runner event loop can blow even a serialized test's timing window. Also adds an explicit 30s vitest timeout to the MCP audit shutdown test, which had no override and inherited vitest's 5000ms default. Regression proof: reproduced RED locally under real devbox CPU contention (2644ms/1796ms elapsed vs the old 1500ms ceiling, exactly the reported failure mode); confirmed GREEN after the fix under the same contention.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request de-flakes timing-sensitive combo cooldown and breaker tests by extracting them into serial tests under tests/unit/serial/ to avoid parallelism contention, and adds a generous timeout to the MCP audit database shutdown test. The reviewer feedback suggests saving and restoring modified environment variables in the new serial test file to prevent pollution, adding retry logic to resetStorage to handle SQLite file locks on CI/Windows, and applying a suite-level timeout in audit.test.ts to cover all dynamic import tests uniformly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omr-combo-cooldown-wait-timing-")); | ||
| process.env.DATA_DIR = TEST_DATA_DIR; | ||
| process.env.API_KEY_SECRET = "test-combo-cooldown-wait-timing-secret"; |
There was a problem hiding this comment.
The environment variables process.env.DATA_DIR and process.env.API_KEY_SECRET are modified globally but never restored in test.after. This can pollute the environment and cause side effects or failures in other tests if they run in the same process or share state. Saving and restoring the original values is a safer practice.
const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omr-combo-cooldown-wait-timing-"));
const ORIGINAL_DATA_DIR = process.env.DATA_DIR;
const ORIGINAL_API_KEY_SECRET = process.env.API_KEY_SECRET;
process.env.DATA_DIR = TEST_DATA_DIR;
process.env.API_KEY_SECRET = "test-combo-cooldown-wait-timing-secret";| test.after(async () => { | ||
| clearAllModelLockouts(); | ||
| try { | ||
| core.resetDbInstance(); | ||
| fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); | ||
| } catch { | ||
| /* best effort */ | ||
| } | ||
| }); |
There was a problem hiding this comment.
Restore the original environment variables in test.after to prevent global environment pollution and ensure test isolation.
test.after(async () => {
clearAllModelLockouts();
if (ORIGINAL_DATA_DIR === undefined) {
delete process.env.DATA_DIR;
} else {
process.env.DATA_DIR = ORIGINAL_DATA_DIR;
}
if (ORIGINAL_API_KEY_SECRET === undefined) {
delete process.env.API_KEY_SECRET;
} else {
process.env.API_KEY_SECRET = ORIGINAL_API_KEY_SECRET;
}
try {
core.resetDbInstance();
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
} catch {
/* best effort */
}
});| async function resetStorage() { | ||
| core.resetDbInstance(); | ||
| fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); | ||
| fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
The resetStorage function does not have any retry logic when calling fs.rmSync. SQLite file locks on Windows or CI environments can cause fs.rmSync to fail intermittently. Adding a retry loop (similar to cleanupTestDataDir in the other timing test file) makes this test much more robust against Windows file locking flakes.
async function resetStorage() {
let lastError: unknown;
for (let attempt = 0; attempt < 5; attempt += 1) {
try {
core.resetDbInstance();
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
fs.mkdirSync(TEST_DATA_DIR, { recursive: true });
return;
} catch (error: unknown) {
lastError = error;
await new Promise((resolve) => setTimeout(resolve, 25));
}
}
if (lastError) throw lastError;
}| // Explicit generous timeout (vitest default is 5000ms): under contended | ||
| // CI-runner load, vi.resetModules() + a fresh dynamic import + mocked DB | ||
| // calls can exceed the default budget though the behavior is correct | ||
| // (issue #6803). | ||
| 30000 | ||
| ); |
There was a problem hiding this comment.
While adding a 30000ms timeout to this test prevents flakes, the other two tests in this file (still closes the audit database when checkpoint fails and falls back to node:sqlite when better-sqlite3 binding is missing) also perform vi.resetModules() and dynamic imports, making them equally susceptible to timing out under CI contention. Consider setting a suite-level timeout on the describe block (e.g., describe("MCP audit shutdown", () => { ... }, 30000)) to cover all tests in this file uniformly.
…ression count - stryker.conf.json: add tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts and tests/unit/serial/combo-strategy-fallbacks-half-open-timing.test.ts to tap.testFiles so their mutant kills count for accountFallback.ts and circuitBreaker.ts (PR #6897 added these files but didn't register them). - eslint-suppressions.json: combo-strategy-fallbacks.test.ts's no-explicit-any suppression count was stale (35) after this PR trimmed 2 any-usages out of the file when extracting the half-open timing test; corrected to 33. Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com>
…ake and #6862 stronger deepEqual (test-masking)
…gosouzapw#6803) (diegosouzapw#6897) * fix(sse): de-flake timing-sensitive combo cooldown/breaker tests (diegosouzapw#6803) Extracts 3 wall-clock-sensitive assertions (combo-quota-share cooldown ceiling x2, circuit-breaker HALF_OPEN race) into tests/unit/serial/ (--test-concurrency=1, the repo's established remedy for this class of test) and widens their margins, since a starved CI-runner event loop can blow even a serialized test's timing window. Also adds an explicit 30s vitest timeout to the MCP audit shutdown test, which had no override and inherited vitest's 5000ms default. Regression proof: reproduced RED locally under real devbox CPU contention (2644ms/1796ms elapsed vs the old 1500ms ceiling, exactly the reported failure mode); confirmed GREEN after the fix under the same contention. * fix(quality): register new serial timing tests + prune stale any-suppression count - stryker.conf.json: add tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts and tests/unit/serial/combo-strategy-fallbacks-half-open-timing.test.ts to tap.testFiles so their mutant kills count for accountFallback.ts and circuitBreaker.ts (PR diegosouzapw#6897 added these files but didn't register them). - eslint-suppressions.json: combo-strategy-fallbacks.test.ts's no-explicit-any suppression count was stale (35) after this PR trimmed 2 any-usages out of the file when extracting the half-open timing test; corrected to 33. Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com> --------- Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com>
…w#6897 de-flake and diegosouzapw#6862 stronger deepEqual (test-masking)
…gosouzapw#6803) (diegosouzapw#6897) * fix(sse): de-flake timing-sensitive combo cooldown/breaker tests (diegosouzapw#6803) Extracts 3 wall-clock-sensitive assertions (combo-quota-share cooldown ceiling x2, circuit-breaker HALF_OPEN race) into tests/unit/serial/ (--test-concurrency=1, the repo's established remedy for this class of test) and widens their margins, since a starved CI-runner event loop can blow even a serialized test's timing window. Also adds an explicit 30s vitest timeout to the MCP audit shutdown test, which had no override and inherited vitest's 5000ms default. Regression proof: reproduced RED locally under real devbox CPU contention (2644ms/1796ms elapsed vs the old 1500ms ceiling, exactly the reported failure mode); confirmed GREEN after the fix under the same contention. * fix(quality): register new serial timing tests + prune stale any-suppression count - stryker.conf.json: add tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts and tests/unit/serial/combo-strategy-fallbacks-half-open-timing.test.ts to tap.testFiles so their mutant kills count for accountFallback.ts and circuitBreaker.ts (PR diegosouzapw#6897 added these files but didn't register them). - eslint-suppressions.json: combo-strategy-fallbacks.test.ts's no-explicit-any suppression count was stale (35) after this PR trimmed 2 any-usages out of the file when extracting the half-open timing test; corrected to 33. Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com> --------- Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com>
1 participant
Closes #6803
Root cause
4 timing-sensitive tests flake under CI-runner load because they assert wall-clock behavior with tight/default budgets while running on a contended shared self-hosted pool:
tests/unit/combo-quota-share-cooldown-wait.test.ts— 2 assertions hard-codeelapsed < 1500ms ceilings around ahandleComboChat()call that also performs real SQLite I/O.tests/unit/combo-strategy-fallbacks.test.ts— the HALF_OPEN recovery test races an 80ms realsetTimeoutagainst a 40ms circuit-breakerresetTimeout.open-sse/mcp-server/__tests__/audit.test.ts— the shutdown test has notimeoutargument and neither vitest config setstest.testTimeout, so it inherits vitest's 5000ms default while doingvi.resetModules()+ a fresh dynamic import + mocked DB calls.Fix
tests/unit/serial/(the repo's established--test-concurrency=1remedy, already hosting 4 similar files), removing intra-suite parallelism as a contention source.resetTimeout/wait 40/80ms→300/600ms) — serialization alone was not sufficient under today's real devbox contention (see regression proof below), so a generous absolute margin is needed too. The functional assertions (calls === 1,deepEqualon dispatch targets) remain the primary signal; the elapsed-time checks are secondary sanity bounds.it(...)timeout to the MCP audit shutdown test.Regression test / TDD proof (Hard Rule #18)
Reproduced RED on unfixed code under real (not synthetic) devbox CPU contention from concurrent parallel sessions on this machine:
Confirmed GREEN after the fix under the same contention:
MCP audit shutdown test verified green via
npx vitest run --config vitest.mcp.config.ts open-sse/mcp-server/__tests__/audit.test.ts(900ms, well under 5000ms — the 30000ms explicit timeout is headroom for CI load).Gates run (all green)
node scripts/check/check-file-size.mjsnode scripts/check/check-complexity.mjs(2050 vs baseline 2054)node scripts/check/check-cognitive-complexity.mjs(885, no change)node scripts/check/check-changelog-integrity.mjsnpm run typecheck:corenpx eslint --suppressions-location config/quality/eslint-suppressions.json <changed files>tests/unit/combo-quota-share-cooldown-wait.test.ts+tests/unit/combo-strategy-fallbacks.test.ts(26/26 pass)tests/unit/serial/*new files (3/3 pass)npx vitest run --config vitest.mcp.config.ts open-sse/mcp-server/__tests__/audit.test.ts(3/3 pass)