fix(server): clear auth cleanup intervals to prevent flaky test exits#5576
Conversation
The three `setInterval` timers in auth.ts (session cache, DB refresh, ban cache) were never cleared. When vitest exits while an interval callback is still in-flight, the worker can produce a corrupted response and flake the `require-admin-cross-tenant` test. Capture each timer handle from `startBackgroundCleanup` and export a `stopAuthTimers()` helper so test teardown can clear them cleanly. Closes adcontextprotocol#5393 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
IPR Policy Agreement Required@garvitkaushik-123 — thanks for the contribution. Before this PR can be merged, the AgenticAdvertising.Org IPR Policy requires your agreement. To agree, post a new comment on this PR with the exact phrase: Your signature is recorded once and covers all contributions to AAO repositories. See |
There was a problem hiding this comment.
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.
This is an automated message from the Argus AI review workflow.
|
I have read the IPR Policy |
IPR Policy — signedThanks, @garvitkaushik-123. Your agreement to the IPR Policy is recorded at |
bokelley
left a comment
There was a problem hiding this comment.
Reviewed for triage cleanup: narrow, green, and no 3.2-triggering changeset.
There was a problem hiding this comment.
Clean teardown fix. Production behavior is unchanged — the timers were already unref()'d, so this only buys a deterministic vitest exit, which is the right shape for a flaky-exit fix.
Things I checked
- Timer capture is sound.
startBackgroundCleanupreturnsReturnType<typeof setInterval>(auth.ts:67), three handles captured into module-scopeconsts (auth.ts:75, 99, 117), andstopAuthTimers(auth.ts:127) is declared after all three are initialized — no TDZ.clearIntervalis the correct API for asetIntervalhandle. - No auth-path impact.
security-reviewer: no security issue. The diff touches only the cleanup-timer factory and adds the clearer — validation, cache keys (still per-cookie/per-token hashes), and tenant scoping are untouched. - Stopping cleanup cannot bypass auth. Every read path re-checks
expiresAt > nowat use: ban cache (auth.ts:132), bearer JWT (auth.ts:346), session cache (auth.ts:905, 1884), dead-session cache (auth.ts:889-891). The timers are memory reclamation, not correctness-bearing. A lifted ban still expires by TTL within 60s regardless of cleanup. - No unbounded-growth DoS if cleanup is ever stopped. Independent size caps exist:
banCache.size > 10_000full-clear (auth.ts:137),DEAD_SESSION_MAX_SIZE = 50_000(auth.ts:48). Stopping the timer only delays reclamation. - No changeset needed. Server app code + test only, no protocol/wire surface. Correct to omit.
afterAll(() => stopAuthTimers())registered at file top level (require-admin-cross-tenant.test.ts:33) — runs once per file, correct scope.
Test-plan honesty
The box that actually confirms the fix — vitest run --no-file-parallelism server/tests/ — is unchecked. The isolated 16/16 run and tsc --noEmit are green, but the full sequential suite is the run that proves the cross-test race is gone, and it's the primary thing this PR claims to fix. The production-side change is verified inert by both experts, so this isn't a block — but the race-fix itself ships unvalidated against the path it targets. Run that box before merge.
Follow-ups (non-blocking — file as issues)
- The DB-refresh timer is still fire-and-forget.
cleanExpiredRefreshes().then(...)(auth.ts:99-105) is not awaited.clearIntervalstops new invocations but cannot cancel a query already in flight atafterAll. The 10-minute interval plusunref()makes a mid-flight hit very unlikely, so this materially reduces the race — but doesn't provably eliminate every variant. (code-reviewer: Medium, partial-but-right-mechanism.) - This only covers one test file. A sibling unit test that imports
auth.jsand exits under parallelism would still observe the timers. Worth a shared teardown helper if the pattern recurs.
Minor nits (non-blocking)
- Document the intent. Add a one-line comment at auth.ts:127 marking
stopAuthTimers()as a test/shutdown teardown helper, not a runtime toggle — it's an exported function whose only safe caller is teardown.
Approving on the strength of two clean expert verdicts plus unchanged production behavior. Close the --no-file-parallelism box before you land it.
Summary
Fixes the flaky
require-admin-cross-tenant.test.tsthat fails under full-suite parallelism but passes in isolation (#5393).Root cause: The three
setIntervaltimers started bystartBackgroundCleanupinauth.ts(session cache, DB refresh, ban cache) are never cleared. When vitest exits while an interval callback is still in-flight, the worker can produce a corrupted response on the affected test.Fix:
startBackgroundCleanupnow returns the timer handle instead ofvoidstopAuthTimers()export callsclearIntervalon all threeafterAll(() => stopAuthTimers())for clean teardownNon-breaking — adds one optional export to
auth.ts, no protocol surface change.Test plan
vitest run server/tests/unit/require-admin-cross-tenant.test.ts— 16/16 passvitest run --no-file-parallelism server/tests/— full sequential suite to confirm the race is gonetsc --noEmit— compiles cleanCloses #5393