feat(desktop): bind turns to a desktop and enforce its deadlines from the row - #8650
waleedlatif1 wants to merge 4 commits into
Conversation
|
@cubic-dev-ai review this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
e152d13 to
c931789
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
c931789 to
4d32e76
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
4d32e76 to
3620c2a
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 19 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
3620c2a to
b6972e7
Compare
There was a problem hiding this comment.
All reported issues were addressed across 33 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
This comment has been minimized.
This comment has been minimized.
|
Both outside-diff findings were real; fixed in a31aaa8, each with a regression test that fails without the fix.
|
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 33 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
… the row A turn sent from a desktop whose background executor is registered to the same session (and with mothership-desktop-background-executor on) is bound to that device at admission: copilot_runs.desktop_device_id. Its desktop calls are persisted pending, offered to the device and claimed through the executor's own fenced routes; the chat view's authorize and confirm answer 409 for them. There is no supervisor. The single desktop wait (waitForDesktopToolCall) branches on the binding: a bound call is offered (pickup_deadline_at, a new nullable column) and the device's doorbell rung, then the existing durable wait enforces every deadline from the row on each 5 s check, beside the lease revocation that already lives there: - an unclaimed call whose device is offline fails at once as not started (reason offline); one still unclaimed past its pickup window fails the same way (reason not_responding), as the inverse CAS of the claim; - a claimed call whose lease lapsed fails as outcome unknown, revoking the device's token so its late result is superseded, and rings it to cancel. Every settlement is sealed like the device's own result. The stale-execution cron settles, the same way, bound calls whose waiter died with its process. One not-started builder carries a reason (chat_not_open, offline, not_responding), and the server-owned failure helper now settles through the shared client settlement. The device is rung when a bound call needs approval, when the user answers, and on Stop.
…r leases to their own settlement A device could claim an offered call after its pickup deadline and before the wait's next check settled it, running an action the turn was about to report as not started. The claim and the inbox now treat a closed window as no longer offered. The generic Sim lease sweep no longer settles a desktop executor's lapsed lease with the Sim interrupted result: the bound wait and the stale-execution cron settle it as outcome unknown, so the model is told the action may already have taken effect.
…nd only run offered calls Stop on a call the background executor held never settled the turn's tool executions unless the device acknowledged: the executor's claim marked a Sim execution as started, and Stop does not settle that execution. A device that was asleep, offline or signed out left abortRun unsettled and the next turn's workbench pending. The executor's claim now takes only its owner token and lease; no Sim handler runs for it, so Sim-execution quiescence ignores it, as it already ignores the chat view's desktop claims. An overdue unclaimed call now settles from its row when presence cannot be read, and never fails early as offline on a failed read; each call in the cron's sweep settles in its own try/catch; presence writes are best effort, so a Redis error no longer fails a pull or a renewal. The executor takes only a call Sim offered it, within its pickup window, and the inbox lists unclaimed calls only while offered or waiting for the user. A call Sim never got to offer gets an implicit deadline one pickup window after it could first run, so the cron still settles it. A revoked install id stays revoked on re-registration, a claim racing the device binding answers "no longer waiting", Stop rings the device only after its chat is validated, the two device lookups are one, and the desktop inbox E2E runs in the http-e2e CI job against its own app with Redis.
a31aaa8 to
72a5ba3
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 39 files
Confidence score: 3/5
- In
presence.ts, a failed heartbeat can leave an awake device’s pending call reported asnotStartedwhen a laterEXISTSfinds no key. Preserve an unknown state after heartbeat failures instead of treating the missing key as offline.
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="apps/sim/lib/desktop/executor/presence.ts">
<violation number="1" location="apps/sim/lib/desktop/executor/presence.ts:31">
P2: Swallowing a failed heartbeat can make an awake device’s pending call settle as `notStarted`: a subsequent successful `EXISTS` sees no key, and the waiter treats `false` as offline. Preserve an unknown state for failed refreshes, or otherwise prevent that absence from taking the offline settlement path.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
| if (!redis) return | ||
| try { | ||
| await redis.set(presenceKey(deviceId), '1', 'EX', DESKTOP_PRESENCE_TTL_SECONDS) | ||
| } catch (error) { |
There was a problem hiding this comment.
P2: Swallowing a failed heartbeat can make an awake device’s pending call settle as notStarted: a subsequent successful EXISTS sees no key, and the waiter treats false as offline. Preserve an unknown state for failed refreshes, or otherwise prevent that absence from taking the offline settlement path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At apps/sim/lib/desktop/executor/presence.ts, line 31:
<comment>Swallowing a failed heartbeat can make an awake device’s pending call settle as `notStarted`: a subsequent successful `EXISTS` sees no key, and the waiter treats `false` as offline. Preserve an unknown state for failed refreshes, or otherwise prevent that absence from taking the offline settlement path.</comment>
<file context>
@@ -15,11 +19,18 @@ export function isDesktopPresenceAvailable(): boolean {
+ if (!redis) return
+ try {
+ await redis.set(presenceKey(deviceId), '1', 'EX', DESKTOP_PRESENCE_TTL_SECONDS)
+ } catch (error) {
+ logger.warn('Could not record desktop presence', { deviceId, error: toError(error).message })
+ }
</file context>
…through abortRun The desktop tests asserted that mocks were or were not called. They now assert what the caller sees: the 409, the sweep's settled count, and the device's own doorbell, which Stop now rings through the real abortRun against a stand-in worker.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 38 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
This branch was previously deployed
1 participant
Summary
Second of three Phase 1 PRs for the desktop background executor. It connects the run loop to the device protocol from #8644. It stays inert until a desktop that speaks the protocol registers with
mothership-desktop-background-executoron: no shipped desktop build sends a device id, so no run is bound.Built on #8643, #8652 and #8644, all on
staging.What changed in this rework
supervisor.ts, its per-call polling loop and the lease-sweep opt-out are gone. Deadlines are enforced where lease revocation already lives:waitForToolConfirmation's durable check (at subscribe, on each wake-up, and every 5 s) gains asettleOverduehook, and the stale-execution cron is the backstop.waitForDesktopToolCallfrom fix(mothership): fail unclaimed desktop calls fast and stop dropping them silently #8652 branches on the run's binding. A chat-view turn keeps fix(mothership): fail unclaimed desktop calls fast and stop dropping them silently #8652's 15 s grace. A bound turn offers the call (pickup_deadline_at, a new nullable column; nothing overloads the lease column any more), rings the device, and waits withsettleOverdue.reason:chat_not_open(its existing text),offline,not_responding.sealClientToolSettlementand settled through feat(desktop): device registry, inbox and leased claims for a background executor #8644'ssettleClientToolCall; fix(mothership): fail unclaimed desktop calls fast and stop dropping them silently #8652's server-owned failure helper (call-failure.ts) now settles through it too.Final-review fixes
execution_started_at), but Stop cancels the row without settling that execution, soareStreamToolExecutionsSettledstayed false until the device acknowledged. A device that was asleep, offline or signed out never would:abortRunansweredsettled:falseand the next turn's workbench reportedhandlersPending. The executor's claim now takes only its owner token and lease. No Sim handler runs for it, so Sim-execution quiescence ignores it, exactly as it already ignored the chat view's desktop claims; the executor keeps its own token, lease, settled and revoked fences. Autopsy: every earlier Stop test had the device acknowledge (post its late result) before anything checked settlement, so the unacknowledged path was never exercised.not_responding, and a failed read never fails a call early asoffline. Each call in the cron's sweep is settled in its own try/catch, and presence writes are best effort, so a Redis error no longer fails a pull or a renewal.executoroption, replaces the two near-identical ones;scripts/test-desktop-inbox-e2e.tsruns in CI:test:desktop-inbox:e2e, in thehttp-e2ejob, against its own app with a Redis service.What this PR does
Binding at admission. The composer adds
deviceIdandexecutortodesktopCapabilitiesonly when the shell exposes a registered background executor (SimDesktopApi.desktopExecutor.getDevice(), an optional bridge field that Phase 2's preload will provide).admitChatTurnwritescopilot_runs.desktop_device_idonly when the flag is on for the user and the device is registered to this user and the caller's own session, is not revoked, and advertisesexecutor >= 1. Otherwise the turn stays with the chat view. Assistant turns, turns with every desktop surface off, and web turns never bind (product decision 1).Every desktop call on a bound run is persisted
pending, local reads included, and is claimed only by the device's executor./api/desktop/tool/authorizeand/api/copilot/confirmanswer 409 for a bound run's desktop calls, so a second window or an older desktop showing the same chat can neither run them nor fail them.Deadlines, all read from the row on the database clock (
pickup_deadline_atis migration0398):{notStarted:true, reason:'offline'}; the turn continues without desktop tools (product decision 3);{notStarted:true, reason:'not_responding'};{outcomeUnknown:true, doNotRetry:true}, revoking the device's token (the fence gains alapsedlease mode, so a renewal wins), and the device is rung to cancel it.Each is a CAS against the device's own transitions (the not-started one is the inverse of the claim), sealed like the device's result, and published so the waiter wakes at once. A call whose pickup window closed is no longer listed or claimable, even before the wait's next check settles it. The generic Sim lease sweep leaves a desktop executor's lease alone: its lapse is settled here, as outcome unknown, rather than with the Sim tool's "interrupted" result.
Restart. Replay never re-dispatches a tool call, so a waiter that dies with its process is not resumed.
cleanup-stale-executionssettles bound calls overdue by more than 60 s the same way, sealed, so the resumed run restores them.Doorbells. The device is rung when a bound call needs approval, when the user answers (
/api/copilot/tool-permission), when a call is offered, when a lease lapses, and on Stop (abortRun).Budgets. The resume gate gives a bound desktop call the full client budget, since its deadlines settle it first; a long terminal command lives as long as the device renews its lease.
Test plan
lib/desktop/executor/bound-turn.integration.ts(real PostgreSQL and Redis), 9 tests, driving the production pathprePersist → sseHandlers.tool → device claim/renew/complete:offline) in one durable check, and a late claim is refused;not_responding);cancelring, asupersededlate result, and a 410 on renewal;approvalring on the gated call, another on the answer through the real tool-permission route, then the offer;requestRunStop: the turn gets the stop result, renewal is refused, the inbox listscancel;runCleanupStaleExecutionssettles both from their rows alone, and a resumed waiter restores the sealed not-started and outcome-unknown results. This is the row-only path a restarted process relies on, not a fresh-module restart;handlersPending: false);not_respondingonce its window closes.bound-turn.integration.ts. 14 turn it red: dispatch ignoring the binding, nosettleOverdue, offline not failed fast, a closed pickup window ignored, no cancel ring on lapse, no ring on offer, unsealed settlements, no approval ring, no ring on the user's answer, chat-view claim allowed, cron not settling, binding ignoring the flag, binding accepting a non-executor, and bound turns not claiming local reads. The survivor is the earlyleaseLapsedread, which sits in front of the SQLlapsedfence (lease <= clock_timestamp()); that fence still refuses the write, so the read is only a shortcut.lib/desktop,lib/mothership/tools/clientandlib/mothership/async-runs(91 tests).browser-download-claim.integration.tshand-builds the tool-call table, so it gains the new column.lint,type-check,check:audits,check:migrations(0397, 0398),docs-manifest:check, block registry,drizzle-kit generate(no drift), andbun run test(37,235 passed).Notes