test(daemon): repeated endpoint churn against real sockets - #4506
Conversation
cave-58eoq.4 — third increment. Covers the bead's "repeated lifecycle stress detects orphaned processes and stale endpoint state" criterion. Scoped deliberately narrow, because two neighbouring criteria are already met and padding them would be noise rather than coverage: - sleep/wake is pinned by daemon-connection-supervisor.test.ts (501 lines, 14 tests) — hiding clears timers and aborts active work without publishing or backing off, foregrounding triggers exactly one immediate fresh request, plus the full backoff ladder and jitter bounds - single-transition endpoint faults are pinned by daemon-endpoint-faults.test.ts from increment 2 What neither covers is the REPEATED case, and that is where the real failures live. A single bind/close cycle can pass while the tenth fails: an address that reports occupied after its owner left, a listener that cannot rebind because something held the path, or a probe that leaks one connection per cycle and slowly starves the daemon of accept slots. Each shows up to a user as "the daemon won't start" or as a duplicate daemon, and none of them reproduces on the first try. Four tests: 12 bind/release cycles asserting a clean occupied/free transition every time; rebind after sustained churn; concurrent probes agreeing with the endpoint's real state through churn, since a launcher, a status poll and a recovery banner can all probe at once and one stale "occupied" wrongly refuses a launch; and a per-cycle orphan check. Addresses are platform-appropriate — a named pipe on Windows, a temp socket elsewhere — so the same cases run on both legs. Verified: 4 tests in 140ms, check:tests-wired (1607), typecheck, lint.
There was a problem hiding this comment.
Pull request overview
Adds a new daemon connectivity test suite that repeatedly churns real socket/pipe endpoints to catch lifecycle drift issues that single-transition tests can miss (stale “occupied” state, rebind failures, and probe-connection residue).
Changes:
- Introduces
daemon-endpoint-churn.test.tswith 4 churn-focused tests exercising the realconnect({ path })probe across multiple bind/release cycles. - Wires the new test file into the
test:app/test:api/test:mobilerunner viascripts/run-tests.mjs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/lib/daemon-endpoint-churn.test.ts |
Adds repeated endpoint churn tests over real sockets/named pipes to detect drift and connection residue across cycles. |
scripts/run-tests.mjs |
Registers the new churn test in SUITES so CI picks it up. |
Suppressed comments (1)
src/lib/daemon-endpoint-churn.test.ts:155
- In the orphan/leak test, the per-cycle
serveris not added tocleanups. If the test fails mid-cycle (e.g. unexpected rejection frominspectDaemonAddress/unbind), the listener can remain open and leak resources. Add each created server tocleanupsimmediately after construction.
for (let cycle = 1; cycle <= CYCLES; cycle += 1) {
const server = createServer(() => {});
server.on("connection", (socket) => {
everOpened += 1;
stillOpen += 1;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| async function bind(socketPath: string): Promise<Server> { | ||
| const server = createServer(() => {}); | ||
| await new Promise<void>((resolve, reject) => { | ||
| server.once("error", reject); | ||
| server.listen(socketPath, () => resolve()); | ||
| }); | ||
| return server; | ||
| } |
Raised in review on #4506, and correct. bind() created a real server but never registered it in the file-level cleanups list. These tests assert mid-cycle, between bind() and unbind(). An assertion that throws therefore skips its unbind and leaves a listener holding the address, so one genuine failure cascades into a hung suite — the exact behaviour the fault harness in increment 1 exists to prevent. Increment 2's listenOn() already registers correctly; this file did not. The review named the helper. The orphan-check test creates its own server inline, because it counts connections and cannot use the helper, and it had the identical flaw — so both are fixed rather than only the reported line. Closing twice is harmless: the cleanup runner swallows it. Verified: 4 tests in 105ms, typecheck and lint exit 0.
|
Fixed in
One addition beyond the reported line: the orphan-check test creates its own server inline (it counts connections, so it cannot use the helper) and had the identical flaw. Both are fixed. |
Third increment of
cave-58eoq.4. Covers the bead's "repeated lifecycle stress detects orphaned processes, stale endpoint state, unsafe retries, and false healthy/fixed diagnostics" criterion.Scoped narrow on purpose
Two neighbouring criteria are already met, and padding them would be noise rather than coverage:
daemon-connection-supervisor.test.ts— 501 lines, 14 tests, including hiding clears timers and aborts active work without new publication or backoff and foregrounding a started supervisor triggers exactly one immediate fresh request, plus the full backoff ladder and jitter bounds.daemon-endpoint-faults.test.tsfrom increment 2 (test(daemon): endpoint fault coverage against real sockets #4500).What was actually missing
The repeated case. A single bind/close cycle can pass while the tenth fails, and that is where the real failures live:
occupiedafter its owner leftEach of those reaches a user as "the daemon won't start" or as a duplicate daemon — and none reproduces on the first try, which is exactly why single-shot tests miss them.
The four tests
occupiedafter release, or a cycle that stops readingfreeEADDRINUSEon the next launch because a released path was left behindoccupiedwrongly refuses a launchAddresses are platform-appropriate — a named pipe on Windows, a temp socket elsewhere — so the same cases run on both legs.
Verification
4 tests in 140ms;
check:tests-wired(1607 files),pnpm typecheck, andpnpm lintall exit 0.Remaining on the parent bead
Real-daemon (process-level) startup/shutdown stress, which needs spawning the actual binary rather than a socket stand-in. Per-PR macOS coverage is
cave-5tpxy.Built in the existing
cave-58eoq.4.1worktree rather than creating another — the checkout is at 31 against a 28 budget with writer-intent contention, and adding to that to avoid agit checkout -bwould have been the wrong trade.