fix(daemon): scope client timeout recovery to the timed-out request - #3193
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
66031ea to
14912a9
Compare
Size Report
Startup median (7 runs, lower is better):
|
Review findings on #3193: - The reset deleted daemon.json and daemon.lock unconditionally after a probe window during which a replacement daemon can publish. Re-read the registration through readRegisteredDaemonOwnership and clear it only on `match`; a replacement's record survives. The protocol lock is no longer touched at all: ADR 0030 gives reclaim to the acquirer under its mutation guard, so an out-of-band delete is the legacy-reclaimer pattern. - The probe's detached HTTP build folded a malformed port into a negative answer instead of an unhandled rejection. - The HTTP error listener returns when the timeout already claimed the outcome, so a destroyed request no longer also diagnoses a transport failure. The claim stays with the guarded reject, so a genuine socket death still settles. - The reset hint stops claiming runner children stopped with a daemon killed by pid alone. - Route tests count TCP connections instead of requests, seed ownership shaped registrations and a protocol lock dir, and assert no transport failure rides along with a timeout. Probe tests pin the /health path, gate the socket answer on the HTTP leg's receipt, and cover the malformed port.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Thanks for the PR. The change looks right to me at 7d78066, but one live check is still missing, and Smoke Tests is still running. The PR removes the client pkill of every runner xcodebuild and gates the daemon SIGKILL behind the probe (daemon-client-timeout.ts#L51). A timed-out request's runner work now stops only if the daemon-side cancel reaches the Apple runner build or launch signal through markRequestCanceled. The unit tests simulate the probe results, not a real daemon holding a cold runner, and the PR body says the "Done when" run from #3177 was not done. So two things are unproven on a device: that session B survives A's timed-out open, and that A's cold xcodebuild is stopped now that the sweep is gone, not left orphaned. Please run this on one daemon. Open session A and session B on two iOS simulators. Make Not blocking, and you can take or leave these: Would the probe be simpler if the HTTP leg used I did not run the tests. I checked that the new assertions fail against the old Smoke Tests was still in progress at review time. Every CLI request in that suite goes through the changed |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Tried the Fallow caught the cycle. The two hosts that break it are worse than the duplication.
So the leg builds its own GET again. The two readers want genuinely different things anyway: the probe needs an absolute 1 s budget and a handle to retire the request when the socket leg answers first, while the reachability reader wants the 500 ms cap and a You were right that the module comment over-claimed — the idle-timeout argument doesn't hold for the HTTP leg, whose reader already passes an absolute On the live check: agreed it's still owed, and I want to be precise about where I got. I could not force a cold runner build on this host — an mtime-only touch leaves the byte-digest artifact reuse intact (products identical, so |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/daemon-client/daemon-client-liveness-probe.ts">
<violation number="1" location="src/daemon-client/daemon-client-liveness-probe.ts:96">
P2: This request inherits Node’s keep-alive global agent and can reuse an idle socket from an earlier request instead of testing a fresh connection. Set `agent: false` so the health probe always opens its own connection.</violation>
</file>
|
The earlier comment's code concerns are now answered at c153fe4, but the live evidence is still missing. The remaining work is one device run. The PR removes the host-wide runner xcodebuild kill and gates the daemon SIGKILL behind the liveness probe (daemon-client-timeout.ts#L51). A timed-out open's cold xcodebuild now stops only if markRequestCanceled reaches the Apple runner build or launch on the daemon. No device run shows that, and none shows that the sibling session survives. So two things are unproven: session B might not survive A's timed-out open, and A's cold xcodebuild might be left orphaned now that the sweep is gone. Please run both sessions on one daemon with two iOS simulators. Push A's On the open inline threads, the liveness probe socket thread (P2) still applies. The probe request passes no CI is green, with all 19 checks passing at c153fe4. The change since the last review is comments plus one unit test, so CI has not exercised any new behavior. I did not run tests, fallow, or the ADR 0019 closure gate, and I checked the cycle and gate claims from the import graph only. I also did not check on a device whether cancellation stops a cold runner build. No conflicts are known. The device run above must pass before merge. |
|
Ran the live two-session check from the checklist. Both sessions share one daemon ( Setup: one dev daemon (pid 85431), session Obs 1 — hint + diagnostics ✅ Per-request diagnostic {"timeoutMs":8000,"requestId":"2902538d23cb282f","command":"replay",
"daemonPreservedAfterTimeout":true,"daemonLivenessProbeAnswered":true}Probe answered affirmatively → reset path never taken. Obs 2 — daemon pid stable ✅ 85431 before and after; Obs 3 — sibling B unaffected ✅ after A's timeout, on the same daemon:
Obs 4 — runner teardown is request-scoped ✅ sampled
B's runner is never killed and never has to cold-start again, and no host-wide sweep happens — A's Reproduce: |
|
Pushed c05c923 for the two cubic findings (keep-alive reuse was valid against the current code and is fixed with The live two-session evidence above was captured at c153fe4. This commit only changes the probe's own connection shape, so rather than re-run the full device choreography I re-verified the shipped
Same four observations therefore still stand; the delta at this head is probe-internal and verified at the artifact level. |
|
T3 flagged Android / Smoke Tests failing on
Current head |
Review findings on #3193: - The reset deleted daemon.json and daemon.lock unconditionally after a probe window during which a replacement daemon can publish. Re-read the registration through readRegisteredDaemonOwnership and clear it only on `match`; a replacement's record survives. The protocol lock is no longer touched at all: ADR 0030 gives reclaim to the acquirer under its mutation guard, so an out-of-band delete is the legacy-reclaimer pattern. - The probe's detached HTTP build folded a malformed port into a negative answer instead of an unhandled rejection. - The HTTP error listener returns when the timeout already claimed the outcome, so a destroyed request no longer also diagnoses a transport failure. The claim stays with the guarded reject, so a genuine socket death still settles. - The reset hint stops claiming runner children stopped with a daemon killed by pid alone. - Route tests count TCP connections instead of requests, seed ownership shaped registrations and a protocol lock dir, and assert no transport failure rides along with a timeout. Probe tests pin the /health path, gate the socket answer on the HTTP leg's receipt, and cover the malformed port.
c05c923 to
f7d4ed6
Compare
|
Rebased onto |
Review findings on #3193: - The reset deleted daemon.json and daemon.lock unconditionally after a probe window during which a replacement daemon can publish. Re-read the registration through readRegisteredDaemonOwnership and clear it only on `match`; a replacement's record survives. The protocol lock is no longer touched at all: ADR 0030 gives reclaim to the acquirer under its mutation guard, so an out-of-band delete is the legacy-reclaimer pattern. - The probe's detached HTTP build folded a malformed port into a negative answer instead of an unhandled rejection. - The HTTP error listener returns when the timeout already claimed the outcome, so a destroyed request no longer also diagnoses a transport failure. The claim stays with the guarded reject, so a genuine socket death still settles. - The reset hint stops claiming runner children stopped with a daemon killed by pid alone. - Route tests count TCP connections instead of requests, seed ownership shaped registrations and a protocol lock dir, and assert no transport failure rides along with a timeout. Probe tests pin the /health path, gate the socket answer on the HTTP leg's receipt, and cover the malformed port.
f7d4ed6 to
921097f
Compare
|
The two earlier cubic-dev-ai points are fixed at 921097f: the timeout-only liveness probe now sets The PR removes the client's I did not run tests or reproduce your device run, and I took the replay-run evidence from your comment. I also did not trace whether markRequestCanceled kills an in-flight |
|
At 921097f this branch now conflicts with main after today's merges. Please rebase onto main. The earlier review findings still apply; I will review the rebased head. |
Review findings on #3193: - The reset deleted daemon.json and daemon.lock unconditionally after a probe window during which a replacement daemon can publish. Re-read the registration through readRegisteredDaemonOwnership and clear it only on `match`; a replacement's record survives. The protocol lock is no longer touched at all: ADR 0030 gives reclaim to the acquirer under its mutation guard, so an out-of-band delete is the legacy-reclaimer pattern. - The probe's detached HTTP build folded a malformed port into a negative answer instead of an unhandled rejection. - The HTTP error listener returns when the timeout already claimed the outcome, so a destroyed request no longer also diagnoses a transport failure. The claim stays with the guarded reject, so a genuine socket death still settles. - The reset hint stops claiming runner children stopped with a daemon killed by pid alone. - Route tests count TCP connections instead of requests, seed ownership shaped registrations and a protocol lock dir, and assert no transport failure rides along with a timeout. Probe tests pin the /health path, gate the socket answer on the HTTP leg's receipt, and cover the malformed port.
921097f to
477508e
Compare
|
Thanks for the rebase. At 477508e the code change looks right. The rebase only adapts the patch to main's caller-abort work, and every One thing still blocks merge: the cold-build |
|
Ran the cold-build check on Setup: one daemon (pid 48564, shared First, a route constraint you'll want to know: on a local Simulator a plain A's run: Artifact 1 — diagnostic (daemon.log, 11:52:03Z): {"phase":"daemon_request_timeout","session":"default","requestId":"e7afc49265a2b310","command":"replay",
"data":{"timeoutMs":35000,"daemonPreservedAfterTimeout":true,"daemonLivenessProbeAnswered":true}}CLI error: Artifact 2 — daemon pid: Artifact 3 — FAILS: Artifact 4 — partial: with the orphan still burning cores, B's Why the cancel misses (code chain):
The Cleanup after the run: planted Not merging. Happy to wire the waiter-signal into the prep-process kill path on this branch if you want it in scope; otherwise this belongs as a follow-up issue since main's sweep was papering over it. |
|
Thanks for the cold-build run on 477508e. It settles the open evidence item, and the result is a blocker. The timed-out request cancels only its own signal. The runner build is tied to the earlier The smallest fix is the one you name: when a request that waits on the build under the runner session lock is canceled, stop the prep processes through the existing |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Follow-up fix landed and re-proven live on the new head. Commit: Live proof on
Cleanup after the run: planted source removed (tree clean), A's polluted cache CI on |
|
The earlier finding is fixed in 90cdee1: a timed-out request now scopes recovery to itself, and the live cancel run in the earlier comment shows it. One issue remains, so this is not ready to merge yet. The close ordering in The other open thread, #3193 (comment), does not apply. The daemon's device execution lock in Not blocking, and you can take or leave these: a start whose last waiter canceled should spawn no more prep work, but a cancel that lands before the spawn (reuse check, lease, boot, artifact check) finds an empty ledger, so I did not re-run the live cancel run; I took it from the author's comment. Smoke Tests and Coverage are still running on 90cdee1 and no failure has been seen. This change touches the Apple runner start and close route that iOS smoke exercises, so a failure there would likely be related. There are no conflicts. Fixing the close ordering is the next step before merge. |
|
Close-ordering fix + live close-during-build run + timeout re-confirm (heads Close ordering — fixed in Live close-during-build on
Timeout-cancel artifacts re-confirmed on Thread Take-or-leave decisions:
CI: |
|
Smoke triage for the iOS lane failure on
No owning-type fix owed here from this failure; the residual flake shape (restart inside a 60 s first wait) is the budget family #2894/#3063 already own, not this PR's route. |
|
The PR is ready for human review: I found no new problems at ade11bb, and the earlier findings from 90cdee1 (#3193 (comment)) are now fixed. CI is green on ade11bb, including the Smoke Tests job that runs the Apple runner start and close route this change touches. There are no conflicts. Not blocking, and you can take or leave these: (1) close stops the device's prep processes only once, so in your live run the start still holding the lock re-entered after the kill and spawned a second build-for-testing, which lived until Would the smaller design work here, where you delete the owner-scoped ledger and its second stop entry point, and put one abort flag on the in-flight start? That flag would cover the real remaining gap and replace any ownership filtering. Deleting the filter needs nothing first, while the start-side flag needs its own measurement, as you propose. On the open threads, the prep-stop ordering thread is fixed at this head (the stop now runs before I did not reproduce the close-during-build run or the timeout-cancel run, and both come from your comment 5981188182. I did not run the new unit tests, so the claim that they fail before the fix comes from reading the code. I also did not trace which path the prewarm health retry takes when it re-enters the start after a prep kill, so the second-build observation rests on your live run. |
thymikee
added
the
ready-for-human
|
The code verdict for ade11bb is unchanged, but the branch now conflicts with main. I removed the ready-for-human label until it is rebased. After the rebase, I will check only the conflict resolutions. |
thymikee
removed
the
ready-for-human
ade11bb to
22025bc
Compare
|
Reconciled #3193 with main's landed #3127 ownership work, preserving the accepted timeout policy and native runner behavior.
Earlier native cancellation and close-during-build evidence remains attributed to b6cdd3c in #3193 (comment). Runner executable behavior is unchanged versus ade11bb (only its comment and an unused type re-export changed); this is not a new native run. The new liveness-gating mutation produced 3 failures / 6 passes, restored to 9 passing route controls. The seven focused files passed 57 controls. The affected-gate result and final head are recorded in the updated PR description; new-head GitHub CI and your conflict-resolution review remain separate. Please recheck the reconciliation before restoring ready-for-human. No merge or label was applied. |
|
The PR is ready. The conflict with main is resolved at 22025bc, and the daemon timeout recovery now stays scoped to the timed-out request. It keeps main's retirement path from #3127 and adds only the liveness-probe gate in front of it. Checks are green: 19 checks, 0 not passing on 22025bc. There are no conflicts. The earlier review threads were already resolved, and nothing new is open. I did not rerun the focused suites or the liveness-gating mutation, so the 3-fail/6-pass result is as the author reported it. I also did not trace whether the replacement-registration route test reaches "registration-replaced" rather than "lock-busy" when process-lock reclaims the seeded lock. That fence belongs to main, not this change. No new native run was made on this head, because the runner delta is comment-only plus one unused re-export, so the earlier b6cdd3c evidence still applies. Not blocking, and you can take or leave these: the TEST_DAEMON_START_TIME comment in daemon-client-timeout-route.test.ts says the tests "never signal a process", but the reset rows and the EPERM row now spawn and SIGKILL real registration-owner children, so the comment can be dropped or limited to the seeded preserve rows. Also, daemonPidForceKilled in daemon-client-timeout.ts is now reported only when status is "retired", so a retained retirement whose SIGKILL succeeded (for example lock-busy) emits undefined; gating on |
thymikee
added
the
ready-for-human
|
…esh (#3220) Review round on the fence found the over-refusal: a caller that merely QUEUED behind a close is not that teardown's retry, and must not wake to a canceled start once the close has settled. The lock task now re-routes such a start to a fresh admission once the fence has left the device — waiter interest follows the redirect — while a start woken WHILE the fence governs the device stays refused, exactly the mid-close retry the fence exists for. The settle clears the fence inside the session lock so a woken start never sees a half-settled teardown. The starting request also counts as an interested waiter on surfaces that pass no caller signal, so a joiner's cancellation can no longer outvote the live owner and stop its build — the protection the removed #3193 owner sniff carried, now by mechanism.
…3220) A non-retained close killed the in-flight `build-for-testing` before waiting for the session lock the start holds, and the same caller's health retry answered the kill with a second build while close waited — close timed out on the build it just asked to stop. One start-owned admission now answers who may prepare a device: it is captured when a start requests the device, shared by every start that queues behind that work, closed by a teardown BEFORE it stops prep children or takes the session lock, and closed by the LAST interested waiter's cancellation while any other waiter still preserves the work. The preparation-spawn seam and the session-publish points both read it immediately before they act, so a retired start's retry is refused before the replacement child exists, and a retired start can never publish into a fresh start. A start that merely QUEUED behind a close re-routes to a fresh admission once that close has settled — the fence refuses mid-close retries, never innocent waiters; the settle clears the fence inside the session lock. The starting request counts as an interested waiter even on surfaces that pass no caller signal, so a joiner's cancellation cannot outvote the live owner and stop its build. Removed the #3193 request-owner prep filter (`runnerPrepProcessChildrenWithoutActiveOwner` / `stopRunnerPrepProcessesWithoutActiveOwner`): the admission's waiter count supersedes the owner-liveness sniff. The shutdown-detach mechanism moved to runner-adoption.ts beside the adoption it hands off to (pure move). Closes #3220
…3220) A non-retained close killed the in-flight `build-for-testing` before waiting for the session lock the start holds, and the same caller's health retry answered the kill with a second build while close waited — close timed out on the build it just asked to stop. One start-owned admission now answers who may prepare a device: it is captured when a start requests the device, shared by every start that queues behind that work, closed by a teardown BEFORE it stops prep children or takes the session lock, and closed by the LAST interested waiter's cancellation while any other waiter still preserves the work. The preparation-spawn seam and the session-publish points both read it immediately before they act, so a retired start's retry is refused before the replacement child exists, and a retired start can never publish into a fresh start. A start that merely QUEUED behind a close re-routes to a fresh admission once that close has settled — the fence refuses mid-close retries, never innocent waiters; the settle clears the fence inside the session lock. The starting request counts as an interested waiter even on surfaces that pass no caller signal, so a joiner's cancellation cannot outvote the live owner and stop its build. Removed the #3193 request-owner prep filter (`runnerPrepProcessChildrenWithoutActiveOwner` / `stopRunnerPrepProcessesWithoutActiveOwner`): the admission's waiter count supersedes the owner-liveness sniff. The shutdown-detach mechanism moved to runner-adoption.ts beside the adoption it hands off to (pure move). Closes #3220
1 participant
Summary
A timed-out request cancels its own connection without a host-wide runner sweep. Reset-eligible commands probe fresh HTTP health and socket RPC connections concurrently on a one-second absolute deadline; a responsive shared daemon survives.
An unresponsive daemon is retired through #3127's owning
stopAndRetireDaemon, preserving process-birth checks, protected metadata/lock cleanup, termination evidence and typed retained-state outcomes. Main's abort/deadline transport implementation is retained unchanged.Apple cancellation stops applicable owned prep children. Non-retained close stops current device prep before waiting on the runner session lock. Later prep respawn is explicitly outside this one-shot stop and tracked in #3220.
17 files; existing client-recovery/runner-close scope retained. Closes #3177.
Validation
Head
22025bcfab, rebased onto7faae56e99. Frozen install/build, 57 focused controls, and liveness-gating mutation proof (3 red, restored 9 green route controls).pnpm check:affected --runpassed: 3,666 related tests/477 files, 12 documentation controls and all selected runnable checks.Earlier live cancellation, sibling snapshot/press, and close-during-build evidence remains attributed to
b6cdd3c49; runner executable behavior is unchanged through this reconciliation. That run also measured the residual respawn now tracked in #3220. No new native run is claimed.New-head GitHub checks and fresh conflict-resolution approval remain pending. No agent merge or ready-for-human label applied.