Repository navigation
fix(runtime-events): plug waitForRuntimeEvent handle leak (CONNECTION_ENDED race) - #1176
Conversation
…_ENDED race) Root cause: waitForRuntimeEvent's IIFE awaits followRuntimeEvents, which internally drains before returning. If the awaited event matches during that initial drain, finish() resolves the outer Promise with handle still null; the IIFE then assigns the handle (active 250ms poller) but settled===true, so handle.stop() never runs. The leaked poller calls listRuntimeEvents → getConnection() forever, racing against subsequent test files' resetConnection() and producing CONNECTION_ENDED rejections that occasionally land on a real test's mailbox.send(). Two layered changes in src/lib/runtime-events.ts: 1. waitForRuntimeEvent: handle the late-handle case (stop the just-arrived handle when settled was already flipped during drain), and try/catch the IIFE so followRuntimeEvents exceptions reject the Promise instead of becoming unhandled rejections. 2. followRuntimeEvents stop(): wrap drainChain await and listener.unlisten() in try/catch so stop() never throws CONNECTION_ENDED. Mirrors the swallow pattern already used in src/lib/audit.ts:170-174. Related to upstream 74aaa02 fix(db): null sqlClient before ending — that fix narrowed the race window but did not cover sql references already captured into closures by leaked pollers. Regression test for the rare race is staged follow-up — would require forced-race fixtures (controllable resetConnection timing relative to a mid-flight poll). Out of scope for this minimal correctness fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 enhances the robustness of runtime event handling by adding error boundaries around cleanup tasks and event polling, ensuring that connection teardown errors are gracefully swallowed. Additionally, waitForRuntimeEvent now correctly handles promise rejections and manages late-arriving handles to prevent resource leaks. I have no feedback to provide.
Summary
Plugs the leak that produced the rare
CONNECTION_ENDEDflake on PR #1171's push-CI (run 24589853586). HIGH-confidence tracer diagnosis pinned towaitForRuntimeEventinsrc/lib/runtime-events.ts.Root cause (tracer, 1-2 sentences)
waitForRuntimeEvent's IIFE awaitsfollowRuntimeEvents, which internally drains before returning. When the awaited event matches during that initial drain,finish()resolves the outer Promise withhandlestillnull; the IIFE then assigns the handle (active 250ms poller) butsettled === true, sohandle.stop()never runs. The leaked poller callslistRuntimeEvents → getConnection()for the rest of the test process and racesresetConnection()calls from later test files, occasionally landing aCONNECTION_ENDEDrejection on a real test'smailbox.send().Changes (one file:
src/lib/runtime-events.ts)waitForRuntimeEvent: handle the late-handle case (stop the just-arrived handle whensettledwas already flipped during drain), andtry/catchthe IIFE sofollowRuntimeEventsexceptions reject the outer Promise instead of becoming unhandled rejections.followRuntimeEvents.stop(): wrapawait drainChainandawait listener.unlisten()intry/catchsostop()never throwsCONNECTION_ENDED. Mirrors the swallow pattern atsrc/lib/audit.ts:170-174.Relation to upstream
74aaa022 fix(db): null sqlClient before endingRelated but incomplete.
74aaa022narrowed the window whereresetConnection()would expose torn-down clients viagetConnection(), but it does not coversqlreferences already captured into closures by leaked pollers. This fix removes the leak source, so even captured-then-stale references stop being created.Regression coverage
Regression test for the rare race is staged follow-up — would require forced-race fixtures (controllable
resetConnection()timing relative to an in-flightfollowRuntimeEventspoll). Out of scope for this minimal correctness fix.Test plan
bun test src/lib/runtime-events.test.ts— 7 pass / 0 failbun test src/lib/runtime-events.test.ts src/term-commands/log.test.ts src/lib/mailbox.test.ts— 21 pass / 0 failbun run check— 2581 pass / 0 failSpawnTargetPicker > Escflake (confirmed pre-existing on baselineorigin/dev: 3/8 fails on the same TUI test without this patch)src/lib/runtime-events.ts)🤖 Generated with Claude Code