[nwparker]

	Files	Added	Deleted	Net
Test	778	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​65967	$\color{#cf222e}{\Huge{\mathbf{−}}}$​5317	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​60650
Prod	2222	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​252017	$\color{#cf222e}{\Huge{\mathbf{−}}}$​15784	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​236233

Two teardown defects in the relay origin pool and control origin. Both are live on main; both have a failing-test-first proof and a mutation table. Based on main at pinned SHA fa3d29fa11e38fba80d77b45dcbe401edd3ec53f.

1. RelayOriginPool.closeNow() never disarms the drain retry — live on main, regression from a rename

src/main/runtime/relay/relay-origin-pool.ts:104

     this.rotation.cancel()
-    this.drainRetry.reset()
+    this.drainRetry.cancel()
     this.retirement.clear()

RelayDrainRetrySchedule.reset() only zeroes the backoff counter. cancel() is the verb that clears the armed timer (and then calls reset() itself). This call site used cancel() until the retry schedule was generalised in #20106; the change to reset() came with that rename.

This is not a latent nicety. Every pool close that happens with a drain retry armed leaves a non-unref'd setTimeout of up to five minutes on the main-process event loop, retaining the origin, its controls, the transport, and the whole closure graph. It fires after the quit and sign-out fences. Nothing reopens — handleDrain's isCurrent() guard neuters the callback — but under director-outage reconnect churn one retained graph accumulates per closed pool, and the timer is exactly the class of post-fence survivor the quit work was written to eliminate.

The two calls sit on adjacent lines. One clears its timer, the other only zeroes a counter. Read together, the omission is invisible, which is how it survived the rename.

Why the existing test could not catch it

backs off drain resolution failures without duplicate retries or post-close work asserts that no assign happens after close. The isCurrent() guard satisfies that on its own, so the assertion stays green with the timer still armed. Two guards pinning different properties, one of them silently removable — that shape has now appeared repeatedly in this subsystem and is worth watching for in review.

The new test asserts the property the old one could not: that no timer survives closeNow().

2. RelayControlOrigin.rebind() leaks a live control when activate() throws

src/main/runtime/relay/relay-control-origin.ts:117

openControl() cleans up its own connect failure (controls.delete + closeNow). The activate() step that follows has no equivalent, so a throw there leaves the control it just opened both registered in this.controls and live on the wire. activate() throws on invalid_relay_generation and — the reachable trigger — when CloudRelayTransport.setGeneration() refuses a generation change while data sockets are attached (relay-transport.ts:99-105), i.e. a phone is attached and the cell could not resume the control generation.

The drain path survives it: resolveDrainTarget falls back to activateTarget, and closing the old origin sweeps the leaked control. The rotation path has no fresh-generation fallback — relay-control-rotation.ts catches and re-arms every 5–15s, leaking one live control websocket plus its silence-watchdog interval per attempt, until the lease expires and the cell drops the socket.

Mutation table

Measured against pinned main fa3d29fa11e, vitest run --config config/vitest.config.ts.

Tree state	pool-teardown test	rebind-activation test	relay suite
main unmodified	FAIL — 1 timer armed after close	FAIL — new control never closed	22 files / 179 pass
drainRetry.reset() → cancel() only	PASS	FAIL	179 pass
both fixes (this PR)	PASS	PASS	22 files / 179 pass
both fixes, but rotation.cancel() removed	FAIL — 1 timer armed	—	—

The last row is the one that makes the others mean anything: with drainRetry.cancel() restored, deleting a different timer cancellation still fails the test. The assertion is "no timer survives close", not something incidental to the drain retry.

Gates

• vitest run --config config/vitest.config.ts src/main/runtime/relay/ — 22 files, 179 tests, pass. Runner verified to exit non-zero on a deliberate failure (exit=1), so a green run is meaningful here.
• oxlint on all four changed files — exit 0 (it prints nothing on success; exit code is the only signal, and it was observed returning 1 on a bad invocation in the same session).
• tsc --noEmit -p config/tsconfig.node.json --composite false — exit 0, empty output.

Please do not merge without a second read on the rotation-path fallback question below.

Follow-up, not fixed here

Fixing the leak bounds the damage but does not restore the session: the rotation path still retries a rebind that cannot succeed until the control lease expires, at which point the cell closes the socket and onClose routes into the drain path, which does have the fresh-generation fallback. Giving rotation the same fallback is a behavioural change I could not justify from the code alone, so it is left out deliberately.

The teardown ratchet now covers every armed timer

c2d6cf352a4. The original test pinned one state — a drain retry armed by a director refusal. That is the bug this PR fixes, but it is not the shape of the bug.

Teardown here is a hand-maintained list of verbs against three helper classes that each spell "disarm" differently: rotation.cancel(), drainRetry.cancel(), retirement.clear(). drainRetry.reset() sat next to rotation.cancel() looking symmetric while only zeroing a counter. Every behavioural test stayed green, because the guard at fire time meant nothing reopened — the leak was a retained timer with no behaviour at all. Counting timers is the only assertion that can see it.

It is now an it.each table over every timer the pool and its broker can leave armed, each reached the way production reaches it:

1. a live broker doing nothing else — control rotation (armed by openInitial) and the broker's lease refresh (armed by connect)
2. a drain retry armed by a director refusal
3. an origin retirement grace armed by a rotation to a different cell, with something still answering through the old origin so maybeClose does not retire it on the spot

Each case asserts something was actually armed before closeNow(), so a case that arms nothing cannot pass as 0 === 0 — which is exactly how the original leak stayed invisible.

Mutation-verified per verb:

mutation	result
drop rotation.cancel()	all three cases fail
drainRetry.cancel() → reset() (the original bug)	only the drain-retry case fails
drop retirement.clear()	only the retirement case fails

Merge order: this must land before #19870

#19870 replaces relay-drain-retry-schedule.ts with relay-retry-schedule.ts and does not touch closeNow, so landing it first would leave closeNow back on reset() and recreate this trap.

The fix is at the call site only. closeNow must call cancel(). Making cancel() itself call reset() was considered and explicitly rejected: it would defeat the coordinator's if (resetRetry) gate and flatten the auth backoff ladder. Leave the two verbs distinct and fix the caller.

Resolved as of #19870's rebase — it carries closeNow calling cancel() and has taken this branch's ratchet wholesale.

#19870 has since ported this file; when it rebases, it should take this branch's version wholesale rather than hand-merging. The mock needs fakes.pendingRequestCount on the hoisted fakes and pendingRequestCount as a getter on the mocked RelayControlClient rather than an instance field — a hand-merge that keeps the field makes case 3 pass vacuously.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: df144085-0c9b-440a-b919-34d0ef5f8f9a

📥 Commits

Reviewing files that changed from the base of the PR and between fa3d29f and c2d6cf3.

📒 Files selected for processing (4)

• src/main/runtime/relay/relay-control-origin-rebind-activation.test.ts
• src/main/runtime/relay/relay-control-origin.ts
• src/main/runtime/relay/relay-origin-pool-teardown.test.ts
• src/main/runtime/relay/relay-origin-pool.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

RelayControlOrigin.rebind now closes a newly opened control when activation fails, then rethrows the error. A test covers the invalid generation transition and verifies that only the new control closes. RelayOriginPool.closeNow now cancels the drain retry schedule. New teardown tests verify that control, drain, and retirement timers are cleared.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to c2d6c

The relay teardown changes clean up failed control activation and pending retry timers without an identified regression. The PR is ready to merge.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check	⚠️ Warning	The description references issues #20106 and #19870, but it does not identify the issue addressed by this pull request with the required Fixes # linkage.	Add the issue addressed by this pull request using the repository's required Fixes #<issue-number> format.
Description check	⚠️ Warning	The description provides detailed context, implementation changes, rationale, tests, mutation results, scope boundaries, and merge-order information. However, it does not follow several required templ…	Add the required template sections. Provide the linked issue in the Fixes #... field, state N/A with a reason under Visual Proof because this is not a UI change, complete or explicitly mark AI Disclosure, Review, Agent skill upstream bo…

✅ Passed checks (2 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changes stay within scope. The rotation-path fresh-generation fallback is explicitly identified as follow-up work and is not included.
Title check	✅ Passed	The title clearly identifies both primary fixes: disarming the drain retry on close and cleaning up failed rebind controls.

Full details: Description check

Explanation

The description provides detailed context, implementation changes, rationale, tests, mutation results, scope boundaries, and merge-order information. However, it does not follow several required template sections, including an explicit Linked Issue, Visual Proof or N/A statement, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist.

Resolution

Add the required template sections. Provide the linked issue in the Fixes #... field, state N/A with a reason under Visual Proof because this is not a UI change, complete or explicitly mark AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist items, and retain the existing testing details.

• Fix all pre-merge checks with AI

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[pullfrog]

✅ No new issues found.

Reviewed changes

Both defects are real and both fixes are the right size — no speculative machinery, no unused cleanups.

• Disarm the drain retry on pool close (relay-origin-pool.ts:104). reset() only zeroes the backoff counter; cancel() is what clears the armed timer. Without this, a close with a retry pending left a non-unref'd setTimeout (up to 5 min) retaining the origin and its closure graph past the quit/sign-out fences. The retry callback was already inert via handleDrain's isCurrent() guard, so the leak was timer/handle retention, not post-fence work — correctly characterised.
• Close a control whose activation throws (relay-control-origin.ts:117-125). openControl() cleans up connect failures but activate() had no equivalent. The reachable trigger (CloudRelayTransport.setGeneration() refusing a generation change while data sockets are attached) throws before activeControl is reassigned, so closeRetiredControl's activeControl !== control guard holds and the freshly opened control is deleted and closed. The drain path already fell back to activateTarget; this stops the rotation path from leaking one live control per 5–15s retry.

Tests. I verified both new tests are load-bearing rather than theatre by mutating the source locally: reverting cancel()→reset() leaves exactly 1 armed timer and fails relay-origin-pool-teardown.test.ts; removing the rebind try/catch fails relay-control-origin-rebind-activation.test.ts. The getTimerCount() === 0 assertion pins the actual property (no timer survives close) rather than an incidental one that the isCurrent() guard could satisfy alone. Full relay suite (22 files / 179 tests), oxlint on the 4 changed files, and typecheck:tsc:node all pass.

Non-blocking open question. The rotation path still retries a rebind that cannot succeed when the cell hands back a fresh generation while data sockets are attached; it recovers only once the lease expires and the cell drains the socket (re-entering the drain path, which does have the activateTarget fallback). This is a limitation the PR bounds rather than introduces, and the author has called it out explicitly as deliberate follow-up — flagging only so it stays visible on merge.

  ^{｜ View workflow run ｜ Using openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

The delta since the prior review is a single test commit (c2d6cf3); production code is unchanged.

• Ratchet every armed pool timer, not just the drain retry (relay-origin-pool-teardown.test.ts). Converted the single drain-retry case into an it.each census of three armed states — live control rotation + lease refresh, drain retry after a director refusal, and a retirement grace after a cross-cell rotation — with a getTimerCount() > 0 precondition so a case that armed nothing cannot pass vacuously.
• Hoisted the mock control's pendingRequestCount behind a mutable getter so the cross-cell case can keep a retiring origin's grace timer armed.

I verified the new cases are load-bearing rather than theatre by mutating the source locally: deleting retirement.clear() leaves case 3 with 1 armed timer, and deleting rotation.cancel() fails all three. Full relay suite (22 files / 181 tests), oxlint on the four changed files, and typecheck:node all pass.

ℹ️ The ARMED_STATES docstring claims more coverage than the table provides

The comment on ARMED_STATES says it covers "every timer the pool and its broker can leave armed", but the optional RelayRegionRefresh timer is not among the cases — it is only constructed when measureRegionDecision is configured, which this test does not set. It is unref'd and regionRefresh.close() already clears it, so there is no leak here; the gap is only that a future break in regionRefresh?.close() would not be ratcheted by this test. Narrowing the docstring or adding a measureRegionDecision-bearing state would align the claim with the coverage.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}