Skip to content

fix(host-kit): preserve cancellation of live requests #4164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thymikee merged 2 commits into from
Nov 2, 2026
Merged

fix(host-kit): preserve cancellation of live requests #4164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f80438f
fix(host-kit): retain live request abort registrations
thymikee Nov 2, 2026
d88649c
docs(adr): correct runner replacement replay safety
thymikee Nov 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/adr/0005-ios-runner-interaction-lifecycle.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@
An id the runner refused before dispatch is left free, which is what the read-only busy resend depends on.
`status` answers `notAccepted` only for an id the journal does not hold.
The daemon cannot read `notAccepted` as a safe terminal state, so evicting a running command would push it to invalidate a session whose runner is still executing that command.
The closed-id window holds 4096 ids, 64x the entry window, and it is a memory safety valve, not an admission boundary. Its residual bound, accepted here: within one runner process, a spent id is refused for the whole window, and the valve releases only the oldest id, so a resend arriving more than 4096 completed dispatches after a command can re-execute it. The resends this bound speaks of are lost-response resends of a dispatched command: a read-only `RUNNER_BUSY` resend reuses a `commandId` the runner refused before dispatch, and such an id never enters the window, so no chain of refusals can consume it. A runner restart starts a new journal, which answers `notAccepted` for every id; a replay across a runner replacement is governed by charge settlement, not by this window.
The closed-id window holds 4096 ids, 64x the entry window, and it is a memory safety valve, not an admission boundary. Its residual bound, accepted here: within one runner process, a spent id is refused for the whole window, and the valve releases only the oldest id, so a resend arriving more than 4096 completed dispatches after a command can re-execute it. The resends this bound speaks of are lost-response resends of a dispatched command: a read-only `RUNNER_BUSY` resend reuses a `commandId` the runner refused before dispatch, and such an id never enters the window, so no chain of refusals can consume it. A runner restart starts a new journal, which answers `notAccepted` for every id. After a runner replacement, replay a mutating command only when dispatch classification proves `dispatched: no`. `RunnerCommandAccounting` controls runner handoff.

Check warning on line 139 in docs/adr/0005-ios-runner-interaction-lifecycle.md

View check run for this annotation

apex-sentinel-staging / Apex Sentinel

minor: Removed 50k/10k bound on the request-abort registry is not recorded in any doc or help the repository owns 

The registry bound in `request-cancel.ts` was a deliberate memory safety valve for the long-lived daemon, and this change removes it, changing a daemon-wide resource-lifetime property. Nothing in the tree records it: the only doc references to this module (docs/adr/0023-end-state-hop-trace.md:80, docs/adr/0033-collocation-decision-table.md:83) speak about `throwIfRequestCanceled` and file placement, and the only ADR hunk in this PR (docs/adr/0005 line 139) is about the runner's closed-id window, an unrelated journal inside the runner process. The repository's own instruction says "When behavior changes, update the owning help and user docs", and ADR 0005 shows this project documents comparable safety valves and their accepted residual bounds. The unbounded registry with no stated bound or residual-risk acceptance is the kind of decision that belongs in an ADR (the change already ships an ADR edit, so the place exists).

### Also raised here

Also raised at this location by senior-developer: Request-abort registry lost its only bound; a leaked registration now permanently poisons its request id — Removing `evictOldestEntries(requestAbortControllers())` means the `requestAbortControllers` map is no longer bounded at all: an entry now leaves only through `clearRequestCanceled`/`clearRequestAbortRegistration`. `canceledRequestIds` keeps its bound (`evictOldestSetEntries`, line 105), so the map is now the one unbounded piece of per-runtime state in this module, and every one of the 10 production call sites relies on an explicit cleanup running (http-server `lifetime.release()` in the `handleRpcRequest`/`invokeAdmittedRpcRequest` finallys, transport.ts finally at line 104, `withRequestDeadline`'s finally, `bindReplayTestAttemptCancellation.release()`). The consequence of any missed cleanup is worse than unbounded memory: the entry keeps `requestAbortControllers().has(requestId)` true forever, so `registerRequestAbort` throws `duplicate_request_id` (line 84-89) for every …

@apex-sentinel-staging apex-sentinel-staging Bot Nov 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor · Removed 50k/10k bound on the request-abort registry is not recorded in any doc or help the repository owns

The registry bound in request-cancel.ts was a deliberate memory safety valve for the long-lived daemon, and this change removes it, changing a daemon-wide resource-lifetime property. Nothing in the tree records it: the only doc references to this module (docs/adr/0023-end-state-hop-trace.md:80, docs/adr/0033-collocation-decision-table.md:83) speak about throwIfRequestCanceled and file placement, and the only ADR hunk in this PR (docs/adr/0005 line 139) is about the runner's closed-id window, an unrelated journal inside the runner process. The repository's own instruction says "When behavior changes, update the owning help and user docs", and ADR 0005 shows this project documents comparable safety valves and their accepted residual bounds. The unbounded registry with no stated bound or residual-risk acceptance is the kind of decision that belongs in an ADR (the change already ships an ADR edit, so the place exists).

Suggested change

Add an ADR (or a paragraph in the daemon-lifecycle ADR) stating that requestAbortControllers is retained until explicit cleanup, why eviction of live entries was wrong (uncancelable early requests), and what now bounds it or why unbounded is accepted.

Details

Also raised here

Also raised at this location by senior-developer: Request-abort registry lost its only bound; a leaked registration now permanently poisons its request id — Removing evictOldestEntries(requestAbortControllers()) means the requestAbortControllers map is no longer bounded at all: an entry now leaves only through clearRequestCanceled/clearRequestAbortRegistration. canceledRequestIds keeps its bound (evictOldestSetEntries, line 105), so the map is now the one unbounded piece of per-runtime state in this module, and every one of the 10 production call sites relies on an explicit cleanup running (http-server lifetime.release() in the handleRpcRequest/invokeAdmittedRpcRequest finallys, transport.ts finally at line 104, withRequestDeadline's finally, bindReplayTestAttemptCancellation.release()). The consequence of any missed cleanup is worse than unbounded memory: the entry keeps requestAbortControllers().has(requestId) true forever, so registerRequestAbort throws duplicate_request_id (line 84-89) for every later request that reuses that id. …

Also raised at this location by javascript: Removing the controllers map's safety valve leaves no bound and permanently poisons a leaked request id — registerRequestAbort no longer prunes requestAbortControllers, so the map is bounded only by every caller reaching its cleanup. The sibling collection keeps its bound (markRequestCanceled still calls evictOldestSetEntries, request-cancel.ts:105), so the module now relies on the unproved invariant that all five production call sites always settle their work: http-server.ts beginRpcRequestLifetime().release() in the finally at http-server.ts:313, transport.ts:104, withRequestDeadline's finally at request-cancel.ts:247, and bindReplayTestAttemptCancellation().release() in the .finally() at session-test-runtime.ts:95-97. Concrete consequence: a handler that never settles — a wedged runner is a documented state in this codebase (RUNNER_WEDGED) — leaks its registration. …

…and 1 more (maintainability), left out to fit the comment.

persona: senior-developer, javascript, maintainability

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.

A watermark over issued ids was rejected: the daemon mints `commandId` as a random UUID per request, reuses one id across retries and across runner generations, and persists nothing, so no ordering exists to compare.
Eviction on a daemon resend deadline was rejected: the daemon holds no per-id "never resend after this instant" value — its resend budgets are attempt counts and per-exchange timeouts redrawn per attempt, and a restart-and-replay can legitimately post the same id minutes later — so a deadline-based rule would need a wire-contract change this change does not make.

Expand Down
20 changes: 20 additions & 0 deletions packages/host-kit/src/internal/request-cancel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@agent-device/kernel/errors';
import {
clearRequestAbortRegistration,
clearRequestCanceled,
markRequestCanceled,
registerRequestAbort,
resolveRequestTrackingId,
Expand All @@ -16,6 +17,25 @@ import {
withRequestDeadline,
} from './request-cancel.ts';

test('an early live request remains cancelable after more than 50,000 registrations', () => {
const requestId = 'early-live-request';
const registration = registerRequestAbort(requestId)!;
try {
for (let index = 0; index < 50_001; index++) {
registerRequestAbort(`later-live-request-${index}`);
}
markRequestCanceled(requestId);
assert.equal(registration.controller.signal.aborted, true);
assert.equal(isRequestCanceledError(registration.controller.signal.reason), true);
assert.equal(getRequestSignal(requestId), registration.controller.signal);
} finally {
clearRequestCanceled(requestId);
for (let index = 0; index < 50_001; index++) {
clearRequestCanceled(`later-live-request-${index}`);
}
}
});

test('resolveRequestTrackingId generates unique ids for fallback seeds', () => {
const first = resolveRequestTrackingId(undefined, 42);
const second = resolveRequestTrackingId(undefined, 42);
Expand Down
11 changes: 0 additions & 11 deletions packages/host-kit/src/internal/request-cancel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,6 @@ export function resolveRequestTrackingId(
const COLLECTION_HIGH_WATERMARK = 50_000;
const COLLECTION_EVICT_COUNT = 10_000;

function evictOldestEntries<K, V>(map: Map<K, V>): void {
if (map.size <= COLLECTION_HIGH_WATERMARK) return;
let removed = 0;
for (const key of map.keys()) {
if (removed >= COLLECTION_EVICT_COUNT) break;
map.delete(key);
removed++;
}
}

function evictOldestSetEntries<V>(set: Set<V>): void {
if (set.size <= COLLECTION_HIGH_WATERMARK) return;
let removed = 0;
Expand All @@ -97,7 +87,6 @@ export function registerRequestAbort(
reason: 'duplicate_request_id',
});
}
evictOldestEntries(requestAbortControllers());
const controller: RequestAbortController = new AbortController();
const parent = parentRequestId ? requestAbortControllers().get(parentRequestId) : undefined;
controller.executionDeadlineMs = parent?.executionDeadlineMs;
Expand Down
Loading