fix(daemon): isolate disconnect cancellation and resource teardown

[devin-ai-integration]

Summary

Scopes disconnect cancellation to the affected request and makes session teardown failure-isolated, addressing #1220 without changing public wire contracts or Apple runner retention policy.

• HTTP close/aborted paths cancel incomplete requests even before response headers, while completed responses remain unaffected.
• Socket/HTTP disconnects no longer call the global Apple-runner abort. Request AbortSignals instead flow through artifact preparation, xcodebuild build-for-testing, and runner launch; canceled preparations invalidate only their newly created session.
• Request-canceled errors remain top-level across the build boundary, so lifecycle cleanup recognizes cancellation instead of reporting a generic xcodebuild/cache failure.
• Teardown runs independent cleanup steps after failures and aggregates cleanup diagnostics. Targeted platform-close errors retain their original AppError, successful-close actions are not recorded on failure, and required physical-device/macOS pre-close runner-stop failures prevent close dispatch.
• Regression coverage exercises pre-header HTTP disconnects, cross-request isolation, abort-during-build/launch semantics, session invalidation, teardown continuation, exact close-error preservation, and required pre-close ordering.

Validation after rebasing onto current main: format, typecheck, lint, layering, build, and 154 focused unit/provider-integration tests pass.

Link to Devin session: https://app.devin.ai/sessions/b1c3093bf2b248ca82a1a5df89152d02
Requested by: @thymikee

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.7 MB	1.7 MB	+1.1 kB
JS gzip	536.7 kB	537.1 kB	+319 B
npm tarball	646.3 kB	646.6 kB	+274 B
npm unpacked	2.3 MB	2.3 MB	+1.1 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.6 ms	26.5 ms	-0.1 ms
CLI --help	56.6 ms	58.0 ms	+1.4 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/session.js	+834 B	+243 B
dist/src/internal/daemon.js	+150 B	+52 B
dist/src/runner-client.js	+101 B	+30 B
dist/src/apps.js	+1 B	+2 B
dist/src/selector-runtime.js	0 B	-1 B

[devin-ai-integration]

P1: request-scoped cancellation still does not cover runner startup/build, so removing the global fallback can leave the exact long-running prep process it previously killed. runPrepareAttempt calls ensureRunnerSession(device, options) before any signal-aware transport; startRunnerSessionWithLease calls ensureXctestrunArtifact, whose xcodebuild build-for-testing uses runCmdStreaming without signal, and prep children are tracked only in the global runnerPrepProcesses Set. A disconnect now only aborts the request controller, so that build can continue until its normal timeout and consume/produce runner state after the client is gone. Please thread the request signal through runner session/artifact preparation (or add request-owned prep tracking and scoped abort), then add a test that disconnecting request A terminates A’s prep while request B’s prep/session remains alive. Do not restore a global abort.

[thymikee]

Re-review at 3f0f7c6 found two blockers:

1. P1 — request cancellation does not cover runner startup. executeRunnerCommand obtains the request signal, but ensureRunnerSession(...) at src/platforms/apple/core/runner/runner-lifecycle.ts:266 does not consume it. A disconnect during xctestrun build/launch can therefore continue and retain a runner/lease after the global abort fallback was removed. Add a blocked-startup disconnect regression proving only that request's prep/session is stopped or never retained.

2. P2 — targeted close failures are misreported as cleanup failures. src/daemon/handlers/session-close.ts wraps the actual platform close dispatch in attemptCleanup, then records Closed before throwing the aggregate. This loses the original AppError details/hint and can log success for a failed close. Preserve the original close failure semantics while still attempting subsequent resource cleanup, with a regression for a distinct thrown AppError.

CI is green, but this head is not ready.

[devin-ai-integration]

@thymikee both blockers addressed at 9647092:

1. Request cancellation now covers blocked runner startup/build/launch, no global abort. startRunnerSessionWithLease pulls getRequestSignal(options.requestId) and threads it through ensureXctestrunArtifact → buildRunnerXctestrun → runCmdStreaming({ signal }) (the xcodebuild build-for-testing) and into the runCmdBackground({ signal }) runner launch — both killProcessTree on abort, so a disconnected request's prep is actually terminated. It stays request-scoped: getRequestSignal returns only that request's controller, an unrelated concurrent build on another device is untouched, and an internal caller (daemon shutdown) gets undefined. To guarantee a canceled prep is never retained, handlePrepareHealthFailure now invalidates the just-created session on a request-canceled error before rethrowing. abortAllIosRunnerSessions remains internal-only for deliberate shutdown. New test (runner-client.test.ts): two concurrent builds on different devices/signals — aborting A kills A's build while B's signal stays unaborted and B completes; asserts each build got its own signal.

2. Targeted platform-close error is preserved. The close dispatch is no longer wrapped in attemptCleanup; dispatchTargetedPlatformClose captures its AppError and it is rethrown with original code/details/hint. Closed is recorded only when the close succeeded, and subsequent independent cleanup (apple_runner stop, runtime hints, materialized paths) + lease release + session delete still run on failure. New regression (session-close-shutdown.test.ts) asserts the exact AppError is rethrown, no Closed action is recorded, subsequent cleanup ran, and the session was deleted. (handleCloseCommand was split into focused helpers to stay under the complexity gate.)

Rebased onto latest main (incl. #1224, clean). Checks: format:check, lint, typecheck, check:layering, fallow pass; focused suites (165 tests) + both integration files (AGENT_DEVICE_REQUIRE_LOOPBACK_TESTS=1) pass. check:affected build (tsdown) still fails only on the pre-existing environmental missing-unrun peer dep (identical on clean main); CI's build is authoritative.

[devin-ai-integration]

P2 (dependency concern): making the pre-close runner stop best-effort changes non-simulator close semantics — a failed pre-close stop now proceeds with the platform close.

On origin/main this same stopAppleRunnerForClose call was an un-wrapped await inside the close block, so a throw skipped dispatchCommand('close') entirely (propagated out via the try/finally, close never dispatched). Wrapping it in attemptCleanup swallows the error into cleanupFailures and falls straight through to the try { await dispatchCommand(... 'close' ...) } below.

The stop is intentionally ordered before dispatch "to avoid runner/app races" (per the comment in stopOrRetainAppleRunnerAfterClose and shouldStopAppleRunnerBeforeTargetedClose = apple && !iosSimulator, i.e. real device / macOS). So a failed pre-close stop now dispatches close while the runner may still be alive — the exact race the ordering guards against. The post-close stopOrRetainAppleRunnerAfterClose re-attempt only mitigates after the racy close.

Also note: if the pre-close stop fails but the close then succeeds, platformCloseError is undefined, so Closed is recorded and the cleanup aggregate (containing apple_runner_pre_close) is thrown — a Closed-and-error result.

Suggest either keeping the pre-close stop error-propagating for non-simulator targets (skip the close as before, letting the close error path own it), or, if proceeding is intentional, documenting why it's safe and adding a non-simulator regression. No test covers this path — the Blocker-2 test uses kind: 'simulator', so shouldStopAppleRunnerBeforeTargetedClose is false and the pre-close branch never runs.

[devin-ai-integration]

Fixed in 4d729f0. The pre-close runner stop for non-simulator Apple targets is no longer best-effort. dispatchTargetedPlatformClose now awaits stopAppleRunnerForClose(session) directly and, on failure, returns that error and skips the close dispatch entirely — preserving the original code/details/hint and never recording Closed. Later independent cleanup (stopOrRetainAppleRunnerAfterClose, runtime hints, materialized paths) still runs, and lease release + session delete are unchanged. This also resolves the Closed-and-error case you flagged: a failed pre-close stop now sets platformCloseError, so Closed is never recorded and the original pre-close error is what's rethrown.

Added a real-device-shaped regression in session-close-shutdown.test.ts (kind: 'device', so shouldStopAppleRunnerBeforeTargetedClose is true): pre-close stop rejects → dispatchCommand is never called, the original AppError code/details/hint are preserved, no Closed action is recorded, later cleanup still re-attempts the runner stop, and the session is still deleted.

[devin-ai-integration]

P2 (Blocker 1 — proof gap, not a correctness bug): the runtime wiring is correct by code, but the new runner-client.test.ts test proves only the artifact-level signal threading and leaves the parts the blocker actually asks to prove untested.

What I verified by reading (all correct):

• disconnect → markRequestCanceled(requestId) aborts the controller registered by registerRequestAbort at request start (http-server / transport);
• requestId flows meta → dispatch ctx (interactions.ts:78) → runner options → startRunnerSessionWithLease getRequestSignal(options.requestId);
• signal → ensureXctestrunArtifact → buildRunnerXctestrun → runCmdStreaming({signal}) and → launch runCmdBackground({signal}), both killProcessTree on abort (watchCommandAbort);
• retention: a build abort throws before runnerSessions.set/writeRunnerLease (no session/lease); a launch abort (allowFailure:true) stores the session+lease, then the health check runs with the aborted signal, the transport maps signal.aborted → createRequestCanceledError(), and this branch invalidates it via invalidateRunnerSessionBestEffort (removes the stored session + releases the lease).

What no test exercises:

1. the request-registry resolution — getRequestSignal(options.requestId) in startRunnerSessionWithLease (the test passes a signal directly to ensureXctestrunArtifact, bypassing the registry), so an actual disconnect's requestId→controller reaching startup is unproven;
2. the launch runCmdBackground({signal}) cancellation;
3. this session/lease-retention branch (handlePrepareHealthFailure request-canceled → invalidate) — the PR's core "never retained" guarantee has zero coverage.

Suggest one test at prepareLocalIosRunner/ensureRunnerSession level driven through the registry: registerRequestAbort(reqA), start prep for reqA, abort mid-launch, assert reqA's session/lease is stopped/not retained while a concurrent reqB on a different device keeps its session. That would actually prove the blocker rather than the artifact seam.

[devin-ai-integration]

Fixed in 4d729f0. Added an integration-level test in runner-session.test.ts driven through the request registry rather than the artifact seam: prepareLocalIosRunner cancellation stops only the disconnected request and preserves unrelated concurrent prep.

It uses registerRequestAbort(reqA)/registerRequestAbort(reqB) and exercises the real prepareLocalIosRunner → ensureRunnerSession → startRunnerSessionWithLease path (no artifact-level mock). Request B prepares normally and its session is retained. Request A is canceled mid-startup: the launch stores the session + lease, then the health check runs with A's now-aborted signal and rejects with a request-canceled error, driving the handlePrepareHealthFailure request-canceled branch → invalidateRunnerSessionBestEffort (this branch). Assertions:

• A's runCmdBackground launch received A's request signal, and that signal is aborted (a real launch would be killed via the process tree).
• getRunnerSessionSnapshot(deviceA) is null — A's session/lease is not retained.
• getRunnerSessionSnapshot(deviceB) is still alive — the unrelated request survives.

[thymikee]

Re-review at 4d729f0: the prior runner-retention and targeted-close blockers are fixed. One cancellation-propagation issue remains.

In runner-artifact.ts, runCmdStreaming receives the request signal and aborts with reason request_canceled, but the catch at the build-for-testing boundary wraps every error as a new generic COMMAND_FAILED and nests the original details. isRequestCanceledError can no longer recognize it, so explicit cancellation during runner build is reported as an xcodebuild failure with a cache/build hint.

Preserve the top-level request-canceled semantics before wrapping, and add a focused abort-during-build regression. The ready-for-human label is being removed pending that fix. No fixer dispatched.

[devin-ai-integration]

Fixed at a9752f22a: the build boundary now rethrows request-canceled errors unchanged before generic xcodebuild wrapping, and the concurrent build regression asserts exact error identity plus isRequestCanceledError. Rebased onto current main; focused 154 tests and deterministic checks pass.