Skip to content

fix(server): expire a host cancel nobody collects - #734

Merged
davidmckayv merged 2 commits into
CopilotKit:mainfrom
aniruddhaadak80:fix/expire-a-cancel-nobody-collects
Oct 5, 2026
Merged

davidmckayv merged 2 commits into
CopilotKit:mainfrom
aniruddhaadak80:fix/expire-a-cancel-nobody-collects

Conversation

@aniruddhaadak80

Copy link
Copy Markdown
Contributor

What this changes

When a host operation fails, is stopped, revoked, or times out, the broker queues a cancel for the native desktop worker so that a worker still holding the command learns to let it go. That cancel was inserted with no expiry and no timer:

operations.set(cancelOperation.operationId, {
  operation: cancelOperation,
  ...
  leasedUntil: null,
  expiresAt: null,
  expiryTimer: null,
  resolve: () => {},
  reject: () => {},
  settled: false,
});

Nothing ever removed it except resolveDesktopOperation for that exact operationId (broker.ts:316). So a worker that stopped polling left the entry in operations for the life of the process — one per abandoned operation, unbounded — and statusFor reported it to the person as pending forever (broker.ts:395).

This is the ordinary disconnect case, not an edge one. A desktop worker that loses its connection has its lease lapse, every in-flight operation is failed, and each one queues a cancel for a worker that is by definition no longer there. Nothing reaps them, and the Host access panel keeps showing operations that can never finish.

The cancel is still delivered to a desktop that reconnects. That is deliberate and already tested — server/tests/host-access-broker.test.ts:217 ("returns cancellation on reconnect") and the stop() test both assert it. So the fix is not to stop queueing an undeliverable cancel, which would have quietly broken those. The cancel now expires on the same clock as any other queued operation (operationTtlMs), which keeps the window open long enough for a reconnecting desktop to be told to stop and gives up on one that never comes back.

It is deleted rather than failed, because a cancel has no caller left to reject, and routing it through failOperation would make giving up indistinguishable from delivering it.

Where it runs

  • New state that outlives a request? A cancel already outlived the request — that is the bug. This change bounds it. operations is still an in-process map that already existed and is already documented as per-process host state; this PR adds no new store and does not make an existing in-memory map newly load-bearing. It only stops one entry type from being immortal. I did not move the host-access broker into Postgres — see the honest note below.
  • What happens on the second replica? No worse, and no better: unchanged. operations and grants were already per-process maps, so a two-replica deployment already cannot deliver a cancel across processes. This change does not alter that topology, and does not introduce any new cross-replica expectation. If host access is meant to work behind more than one replica, that is a separate and much larger piece of work — flagged below rather than smuggled in here.
  • Anything serialised? Nothing new. The cancel carries targetOperationId, which it already did; this adds a timer, not a write.
  • Anything fanned out to a browser? No. This is the server-to-desktop-worker poll path, not a browser socket.
  • New listener, port, or schedule? None. No listener, port or schedule is added. The change adds one setTimeout per cancel, which is unref'd exactly like the operation TTL timer already beside it, so it cannot hold the process open.

Boundary and audit

  • Every acting call still goes through the gateway: resolve, decide, audit, then act.

    Not an acting call. A cancel is the withdrawal of an action. No host_* tool call is dispatched, permitted, or refused differently by this change.

  • New refusals and new failures each write a row.

    No new refusal and no new failure. The operation it belongs to was already failed and already audited before the cancel was queued; the cancel carries no result and this change does not settle an operation early.

  • Nothing new is trusted from the client that the server can resolve itself.

    Nothing new is read. The expiry is derived from the server's own operationTtlMs option, not from anything a client or desktop sent.

Changelog

  • A line in CHANGELOG.md under Unreleased.

    Added. The deployment-visible difference is that the Host access panel stops listing an operation as pending once it can no longer be delivered.

Proof

bun test hangs on this test file on the machine I work on (Windows), on main as well as on this branch — it prints only bun test v1.3.14 and never starts reporting. That is pre-existing and unrelated to this change; I confirmed it by stashing and re-running on main. So I could not use bun test as the instrument here.

Instead I drove createHostAccessBroker directly through a throwaway script (since deleted, never committed) that reproduces exactly what the new test asserts, and ran it against main and against this branch.

On main, before the fix:

handed to desktop: read_file
pending 10ms after the operation failed: ["cancel"]
pending 160ms after it failed (nobody collected the cancel): ["cancel"]     <-- leaked
reconnect case, desktop polls in time and is told to stop: "cancel"

On this branch, after the fix:

handed to desktop: read_file
pending 10ms after the operation failed: ["cancel"]
pending 160ms after it failed (nobody collected the cancel): []             <-- reaped
reconnect case, desktop polls in time and is told to stop: "cancel"         <-- unchanged

The third line is the one I most wanted to keep green, and it is: a desktop that reconnects inside the window is still told to stop.

The committed test is that same check expressed in the file's own style — a cancel nobody collects does not stay pending for the life of the process, asserting pending holds one cancel after the operation times out and is [] afterwards. On the ubuntu-latest runner that CI's test job uses, it should go red on main and green here. I expect it to, but I did not watch it happen, and I would rather say so than claim otherwise.

Also run:

  • bun test server/tests/host-access-tools.test.ts server/tests/host-access-routes.test.ts — 10 pass, 0 fail (the two suites that exercise the broker from outside and were runnable here).
  • bun run --filter server typecheck — exit 0, no diagnostics.
  • bunx biome lint --error-on-warnings and bunx biome format on both changed files — clean.

One thing I deliberately did not do

operations and grants are per-process maps (broker.ts:59-60), while POST /grants awaits a promise only the serving replica can settle and the worker polls GET /desktop/next, which behind a load balancer can reach any replica. If host access is ever expected to run behind more than one replica, that protocol cannot work as written — a folder grant request would land on replica A and the picker's poll on B.

I did not touch that. It is a much larger change than this PR, it depends on whether maintainers consider host access permanently single-host (the token is currently only set by the Tauri desktop app, which spawns a local single-process stack), and guessing wrong would be worse than leaving it. Flagging it rather than bundling it: if it is in scope, it deserves its own issue and PR.

Every host operation that is failed, stopped, revoked or timed out queues a
cancel for the desktop worker, so a worker that is still holding the command
learns to let it go. That cancel was inserted with expiresAt null and no
timer:

  operations.set(cancelOperation.operationId, {
    ..., expiresAt: null, expiryTimer: null, settled: false,
  });

Nothing removed it but resolveDesktopOperation for that exact operationId.
A worker that stopped polling therefore left the entry in operations for
the life of the process: one per abandoned operation, unbounded, and
statusFor reported it to the person as pending forever.

This is the ordinary disconnect case. The cancel exists to tell a desktop
that is there to stop working; when it does not come back there is nothing
to tell, and the entry only misleads.

The cancel now expires on the same clock as any other queued operation, so
a desktop that reconnects is still told to stop, and one that does not is
given up on. Deleted rather than failed, because a cancel has no caller left
to reject and failing it would be indistinguishable from delivering it.

The existing lease-expiry and stop tests both assert the reconnect path and
are unchanged: they collect the cancel immediately.
Copilot AI balanced review requested due to automatic review settings October 4, 2026 09:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@davidmckayv
davidmckayv merged commit 63f99cf into CopilotKit:main Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants