Repository navigation
feat(client): accept an AbortSignal per call and close the request on abort - #3200
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
![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 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Coverage fails on this PR, and I found one gap in the tests at 7fa0dc6. The failing check is related to the change. There are no conflicts. Smoke Tests was still queued, so I have no result for it. The next step before merge is to fix the Coverage failure and add one test for the real client route. The line that makes the daemon see a client abort is The Coverage job fails Not blocking, and you can take or leave these. The The open inline threads on the health check, artifact downloads without a signal, the contract doc, the missing requestId and the spurious socket-error diagnostic still stand, along with 8 lower-priority threads. The thread on abort during request preparation does not apply: I did not run the added tests or the eager-closure gate locally, and I took the Coverage result from the job log. I did not check whether every custom command writer passes |
7fa0dc6 to
add99f4
Compare
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Both asks are addressed and pushed.
Two things that fell out of running the full gate on the head, both fixed there: the iOS provider-scenario gateway built its alert interactor with an empty runner context, which only matched the scripted provider while requests carried no
|
…quester-side guard Declare `signal?: AbortSignal` once at the owning contract types — the per-call overrides, the transport context, and the internal request envelope — so no per-site guard repeats it (#3178). Add `createRequestGuard` to @agent-device/host-kit/request beside the daemon-side cancellation machinery it answers for: an already-aborted call is refused with `dispatched: 'no'`, and an abort racing an in-flight send wins with the typed canceled-request error so a custom transport that ignores the signal cannot outlive the caller's promise.
…d both transports Wire the declared signal from the client's execute seam down to the transport context and the request guard, and from the built-in transport into both daemon wires (#3178): - `sendToDaemon` refuses an already-aborted call before daemon startup and guards each client-side phase; an abort during the artifact upload now combines with the lease beat's own signal so the upload stops for either owner. - The socket transport destroys its connection on abort, and the HTTP transport destroys its request; either way the daemon sees a client disconnect and marks the request canceled. Every settle path detaches the abort listener, and the timeout timer is cleared on abort so an abort never triggers the timeout path's runner sweep or daemon reset. - Retries and fallback attempts start behind a refusal check, so nothing is sent after an abort.
… in the guard module `abortedRequestError` and `refuseAbortedRequest` complete the request-cancel seam the daemon request transports consume, so every rejection a caller's abort produces is built by the module that declares the cancellation contract rather than restated per transport.
…d both transports #3178 acceptance: an abort before send sends nothing (`dispatched: 'no'`); an abort mid-request closes that one request so the daemon marks it canceled (`dispatched: 'unknown'`, proven daemon-side through the request-cancel registry), and the daemon keeps serving follow-up requests. Client-level cases pin the guard half: the signal rides the transport context, and a custom transport that ignores it cannot outlive the caller's promise. Regression evidence: all eight wiring cases fail on the pre-fix sources (verified by running these files against fc7df6c's client/daemon-client/ transport) and pass with the fix.
#3178's Done-when: the public client docs list `signal`, including the already-aborted refusal (`dispatched: 'no'`), the mid-flight cancel (`reason: 'request_canceled'`), and the promise that an abort never takes the timeout path's runner cleanup or daemon reset.
…rt seam Add the paired regression cases for #3178's never-a-timeout promise: with the timeout seam mocked, an abort with an armed budget leaves the seam never called, while the same budget without a signal reaches it. Counterfactual verified: deleting the abort path's `clearTimeout` fails only the abort case. Clear the ignoring-transport test's late resolve so it keeps no timer alive.
…he review - Move the requester-side AbortSignal guard out of @agent-device/host-kit/request into daemon-client-transport.ts, the module the CLI eager closure already loads, so the caller-side half and the transport half share one owner and the ADR-0019 loading-shape probe stops counting 9 extra modules against cli.ts and the replay-port facade. - Generate the request id in execute before guarding, so a canceled call names itself in request.meta and matches the daemon's diagnostic for the same request. - Report dispatched 'no' when a guard is entered already aborted: the refusal proves send never ran, so 'unknown' would suppress a safe retry. - Keep a non-Error abort reason as the cause unchanged. - Return before handleTransportError once a request is settled, so the error an intentional destroy emits cannot log daemon_request_socket_error. - Cut the remote-retry health probe on the caller's signal and recheck the signal before timeout and retry handling, so a canceled call never reads as timed out. - Forward the caller's abort into the lease beat, which owns its own connection. - Prove the signal reaches the daemon through the DEFAULT transport: one loopback test per transport drives createAgentDeviceClient with no injected transport and asserts the daemon saw the cancel. - Prove the RPC handler still serves after an HTTP cancellation, not just /health. - Qualify the signal contract and the docs with the two phases the guarantee does not cover: a response-artifact download already underway, and a canceled one-shot replay's existing daemon cleanup.
…me the beat's outcomes The code-quality audit found two exports with no consumer outside the transport module and two functions whose new branches pushed them over the complexity threshold. Nothing imported the helpers, so the guard's error factory and its send-attempt refusal stay module-private beside the two callers that use them. The lease beat now says what a failed beat means — one path ends the protection, one reports a transient miss — and the retry path names the budget a deadline-capped probe proves, instead of inlining both decisions.
… request's execution metadata The scenario gateway built its Apple interactor once at bind time with an empty runner context, so an alert leg asked the runner for a request id it never had. While the client sent no meta.requestId the unkeyed provider scope still matched, but every call now carries one: the request-scoped scripted provider is keyed by (deviceId, requestId), the empty lookup missed it, and the leg fell through to the local XCTest runner — which spawns real tooling and hangs the in-process harness. Per-call resolution mirrors the production alert binding, which projects the input's execution metadata into the interactor context.
…se fails net.Server.close() waits for accepted connections, and a hanging-handler request keeps its connection open until the client's abort destroys it. When an assertion before that fails, the daemon-side handler waits forever and close() hangs the lane, turning the regression signal the abort tests exist to provide into a test timeout. Track the accepted sockets and destroy them in teardown at the three socket suites whose handlers never answer. http.Server already has closeAllConnections(); net.Server has no equivalent.
The compatibleChanges entry must carry the post-change digest; the declaration moved when the probe began composing the #3178 caller signal with its own timeout budget. The /health request and accepted payload are untouched, so protocol 2 peers parse it exactly as before.
e2848d5 to
b431157
Compare
|
Rebased onto current |
The caller's per-call abort guard moved beside the transports it serves, while `@agent-device/host-kit/request` kept the daemon-side cancel registry and progress. The declaration-site line still credited the whole seam to host-kit, pointing the next reader at a module that no longer exports the guard.
|
This PR is ready. The fixes for the 7fa0dc6 findings are in bb8e19d: the real-route loopback test, the eager-closure budget, listener cleanup, and abort-versus-timeout on the retry probe. All 22 checks pass at bb8e19d, and I know of no conflicts. Not blocking, and you can take or leave these: (1) In daemon-client-lease-beat.ts:218, stopBeating() now aborts beatControl on every phase settle, so a beat still in flight when an upload finishes normally is closed and dropped silently. A late LEASE_EXPIRED answer can then no longer reach the lease_lost_after_phase diagnostic, and the test at daemon-client-lease-beat.test.ts:207 passes only because its fake heartbeat ignores the signal. Aborting beatControl only from the caller-abort listener would keep the old behavior. (2) Three new branches in daemon-client-transport.ts:387 have no test through sendRequest: the caller signal in the retry health probe, the The earlier threads that are fixed at this head, and can be resolved, are #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment), #3200 (comment) and #3200 (comment). Two threads do not apply: #3200 (comment) (execute refuses with dispatched 'no' before dispatch, so only local preparation delays the rejection) and #3200 (comment) (a detached daemon launch outliving an aborted call matches the timeout path, and #3178 scopes the request phase). I did not run the new loopback suites or the counterfactual of deleting the signal handoff, so the claim that they fail without it comes from reading the code. I also did not check whether a lease heartbeat canceled mid-handle on the daemon still applies its renewal. |
thymikee
added
the
ready-for-human
1 participant
What
Every typed-client call now accepts an
AbortSignal. Aborting closes that request's connection so the daemon marks the request canceled — it is never treated as a timeout, and the daemon and session stay alive.Closes #3178
How
signalis declared once, onAgentDeviceRequestOverrides,AgentDeviceDaemonTransportContext, andInternalRequestOptions, so it reaches every method through the same override path. The requester-side half lives beside the transports it serves, insrc/daemon-client/daemon-client-transport.ts:createRequestGuardgives the client arefuseIfAbortedcheck and aguardwrapper, and owns therequest_cancelederror factory so both transports reject with one shape.@agent-device/host-kit/requestis untouched — the guard is a client concern, not a shared request primitive.details.dispatched: 'no'. The guard is checked before the daemon-ensure, the artifact-upload, and each transport attempt, so a pre-aborted call opens nothing.details.reason: 'request_canceled'anddetails.dispatched: 'unknown', which is the honest answer: the request had left the process, whether the daemon finished it is not knowable from here.handleRequestTimeout, so the pkill sweep and daemon reset that a deadline triggers do not run.daemon-client-abort-timeout.test.tsproves this at the seam by mocking the timeout handler, with a paired no-signal case that reaches it.Every call now carries a
meta.requestId— generated before the guard so a canceled call and the daemon's diagnostics for the request the transport goes on to cancel name the same id. The caller signal rides the transport context, never the wire — verified against the actual envelopes for flags, meta, and input (gesture input is rebuilt from an explicit field table, so an unknown key cannot ride along). No CLI flag is added: a per-call signal is a Node-caller concept, and the CLI has its own process lifecycle. The remote health probe composes the caller signal with its own timeout budget, and an in-flight lease beat is aborted through the control the lease beat now owns, so a canceled upload cuts the same request everywhere.Tests
16 files, +1207/−85 overall; the test files alone are 9 files, +847/−13.
daemon-client-request-guard.test.ts— guard semantics, including that a guard installed after an abort still refuses and thatsignal.reasonpasses through unchanged.daemon-client-abort-default-transport.test.ts— loopback against the realcreateSocketServer/createDaemonHttpServer, asserting the daemon-side cancel and that the daemon's own record of the request carries the samerequestIdthe client's rejection names.daemon-client-abort.test.ts— transport-level abort on both built-in transports, with the settle-before-destroy ordering and the spurious socket-error diagnostic both pinned.client-abort-signal.test.ts— the client surface: pre-aborted, in-flight, and that the daemon survives an abort.daemon-client-abort-timeout.test.ts— the abort-is-never-a-timing-out case and its no-signal counterfactual.The wiring tests fail against
fc7df6c4band pass here, so they test the change rather than the harness. Counterfactuals pin details that are easy to lose: deleting thesignal: options.signalhandoff fails the default-transport proofs, and dropping the wire token from the fixtures fails only after the typecheck fix.Known limits
materializeRemoteArtifacts(the response-artifact download) is not signal-threaded. The caller promise still rejects typed, but the download may run to completion and leave a partial file. This is a partial fix for fix(agent-device): clean up screenshots after cancelled captures tester-army/e2e#206, not a complete one.cleanupDaemonAfterRequest. This matches the existing timeout behavior rather than adding a new teardown path, so I left it alone rather than widen this change.Gate
Rebased onto
origin/main(fae0c39); upstream #3199 landed the same record-stop timeout-policy fix my branch carried, so those hunks dropped out as already-applied and the diff is now purely abort-signal work.pnpm check:affected --runpassed on the exact pushed headb431157c5: all runnable checks green — format, lint, typecheck, layering,check:fallow --base origin/main, build, the affected vitest set (163 files / 1045 tests, including the provider-integration alert scenario), anddaemon-wire-compatreporting the protocol unchanged againstv0.21.18(191 declarations, 0 removed, 2 added; both health-probe digests re-acked — the wire envelope is untouched, only the in-process probe composes an extra signal). CI Integration Tests and Coverage jobs on this head are the remaining external evidence.