fix(agent): give every start_desktop its own IPC id so it stops self-colliding (#3107)

[ToddHebebrand]

Problem

A remote-desktop start could fail with desktop start failed after 2 attempts (helper keeps crashing) while the helper was healthy and had never crashed.

start_desktop is deliberately exempt from the heartbeat's command dedup (#434), because the viewer legitimately re-invokes the same commandId across reconnects and the command can arrive over both the WebSocket and the heartbeat response. So two invocations for one desktop session can be in flight at once — but every invocation derived the same IPC correlation id desk-<sessionID>, so the second was rejected by Session.SendCommand's ErrDuplicateCommand guard. The #434 exemption and the duplicate guard were in direct conflict.

The caller then compounded it in two ways:

• startDesktopOnSession returned helperDied = true for every SendCommand error, so the duplicate-id rejection was misclassified as helper death.
• The retry loop had no backoff, so both attempts burned in the same millisecond against the same still-healthy helper — exactly the reported log signature, where attempt=1 and attempt=2 share a timestamp and an error.

Changes

All in agent/internal/heartbeat/handlers_desktop_helper.go.

1. Unique IPC id per invocation. nextDesktopStartCommandID() numbers each start off an atomic.Uint64 — desk-<sessionID>-<n>. The desktop session id stays in the string so helper-side logs remain greppable. This resolves the #434-vs-duplicate-guard conflict, and as a bonus fixes a latent mis-correlation: with the shared id, a response from a timed-out attempt could be matched to a later attempt's pending entry and pass the response validator, handing the viewer an SDP answer for a peer connection that had already been torn down.

2. Single-flight per desktop session (joinOrRunDesktopStart). This is load-bearing, not a nicety. ErrDuplicateCommand was also the thing accidentally serializing concurrent starts; with unique ids both would reach the helper, and SessionManager.StartSession (remote/desktop/session_webrtc.go:41-58) unconditionally stops every existing session before registering the new one. So the second start would tear down the first, both callers would be told completed, and only one peer connection would be live. Worse, the first session's OnConnectionStateChange closure captures the session id rather than the *Session, so its late Closed callback calls StopSession(sessionID) and kills the second session, firing a spurious desktop_peer_disconnected. A joiner now takes the leader's result instead — "join or defer to it", as the issue asked.

3. Honest failure classification. desktopStartLostHelper() replaces the blanket "any error means the helper died":

Error	Classified as	Why
ErrDuplicateCommand	not a death, terminal	Another start is in flight against a live helper. Nothing crashed and a retry cannot help. Unreachable after changes 1-2 — kept as a guard against a future caller reintroducing a shared id.
ErrCommandTimeout	not a death, terminal	A helper that actually dies closes the session's done channel and surfaces as session closed while waiting for response, not as a timeout. Terminal because helperSessionForTarget does no liveness or pong-age check and would hand the retry back the very same session.
everything else	helper lost, retry	Failed socket write, session closed under us — a real transport failure. The default stays conservative.

4. Spaced retries + operator-facing messages. Retries are separated by desktopStartRetryBackoff (1s; a var so tests can shrink it, following the existing desktopLeaseRenewEvery pattern), selecting on stopChan so a pending retry never holds shutdown open. agentWs writes CommandResult.Error into remote_sessions.errorMessage and the viewer displays it, so both sentinels are now wrapped with a real message plus the helper/windows session ids (keeping %w) instead of surfacing as bare sessionbroker: command timed out. The terminal message carries the underlying cause instead of blaming a crash unconditionally, and the no capable helper available exit carries it too.

startDesktopOnDemand (the RDS path) picks up all of this for free — it shares startDesktopOnSession and runs inside the same single-flight.

Tests

New agent/internal/heartbeat/handlers_desktop_helper_collision_test.go. All three fix parts are mutation-verified — reverting each one fails the suite:

Mutation	Caught by
unique id → shared desk-<sessionID>	TestConcurrentStartDesktopOnSessionDoesNotSelfCollide, with exactly the reported error duplicate in-flight command id: "desk-desktop-1"
desktopStartLostHelper(err) → true	TestStartDesktopViaHelperDoesNotRetryWedgedHelper
single-flight bypassed	all four TestJoinOrRunDesktopStart*

Plus TestDesktopStartLostHelper (table-driven over the classification matrix, bare and wrapped sentinels), TestNextDesktopStartCommandIDIsUniquePerInvocation, TestStartDesktopViaHelperSpacesRetriesAndReportsRealError, TestJoinOrRunDesktopStartReleasesOnPanic, and TestJoinOrRunDesktopStartNeverRunsTwoAtOnce — which asserts the invariant no scheduling order can break (at most one start executing per session) rather than a count that could flake.

Verification: go test -race ./... green across the whole agent module; the new tests run 8/8 clean repeatedly; go build ./..., go vet, gofmt clean on the touched files.

Deliberately out of scope

Left as follow-ups, noted so they are not lost:

• Reaping a start whose outcome was never proven. SendCommand's timeout does not cancel helper-side work, so a timed-out start may still bring up a capture that nothing here owns. A compensating desktop_stop was implemented and then removed: the helper processes stop off StartSession's mutex, so a stop queued behind a wedged helper can land after the viewer's next start and silently kill a session the daemon believes is live — the same class of bug as this issue. Doing it safely needs a helper-side start generation so a stale stop cannot match a newer session. Meanwhile the orphan is self-limiting: the viewer never gets the answer, ICE never completes, and the peer connection tears itself down on the 15s failed timeout.
• Ownership on the failure path — rememberDesktopOwner is still only reached on success, so a genuine desktop_peer_disconnected after a failed start is dropped as "non-owned session". Pairs naturally with the item above.
• Retry window vs. respawn cadence — the retry still cannot outlast HelperLifecycleManager's 30s reconcileInterval, so a genuinely crashed helper can't be replaced inside the loop. Aligning the wait or kicking kickCh is a lifecycle-manager change with its own risk profile.

Also untouched by design: the adjacent ipc/protocol.go Conn.Send seq/HMAC concern (#3007 / #3178). On this base that critical section is already held under c.mu, and this PR goes nowhere near the send path.

Closes #3107

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying breeze with    Cloudflare Pages

Latest commit:	7e362e9
Status:	✅  Deploy successful!
Preview URL:	https://370cd882.breeze-9te.pages.dev
Branch Preview URL:	https://fix-3107-start-desktop-ipc-i.breeze-9te.pages.dev

View logs

[ToddHebebrand]

Review run: /pr-review-toolkit:review-pr — round 1: code-reviewer, pr-test-analyzer, silent-failure-hunter (all three in parallel). Round 2: code-reviewer, scoped to the round-1 fix delta only (a new concurrency primitive in agent code justified one re-review).

Findings: 8 raised across two rounds → all 8 addressed; 0 outstanding.

Round 1 (3 consequential):

• Critical — the fix unblocked a worse bug. ErrDuplicateCommand was also what accidentally serialized concurrent starts. With unique ids both reach the helper, and SessionManager.StartSession stops every existing session before registering the new one, so start chore(deps): bump node from 20-alpine to 25-alpine in /apps/web #2 tears down start chore(deps): bump node from 20-alpine to 25-alpine in /apps/api #1 while both report completed — and chore(deps): bump node from 20-alpine to 25-alpine in /apps/api #1's OnConnectionStateChange closure captures the session id, so its late Closed callback kills chore(deps): bump node from 20-alpine to 25-alpine in /apps/web #2. Fixed by adding joinOrRunDesktopStart (single-flight per desktop session) in 8ec8ad8. Verified the helper-side claim directly against remote/desktop/session_webrtc.go before acting on it.
• Critical — the classifier wasn't pinned. Mutating desktopStartLostHelper(err) → true at the call site passed the entire package, and TestStartDesktopViaHelperDoesNotRetryDuplicateCommand exercised the resp.Error branch rather than the behaviour in its name. Extracted desktopStartCommandTimeout as a shrinkable var and replaced that test with TestStartDesktopViaHelperDoesNotRetryWedgedHelper, which kills the mutant.
• High — raw sentinels reached the technician. agentWs writes CommandResult.Error into remote_sessions.errorMessage and the viewer shows it, so the most likely terminal case displayed sessionbroker: command timed out. Both sentinels now carry an operator-facing message plus the helper/windows session ids, keeping %w.

Round 2 (3 defects, all in the round-1 fix itself — 2 reverted rather than patched, in 0e48e38):

• Critical — the orphan reap escaped its own single-flight and could land a stale desktop_stop after the viewer's next start, silently killing a session the daemon believed live. Removed rather than patched: the reap fires exactly when the helper is wedged, which is exactly when the stop sits queued, so a synchronous variant wouldn't close the window either. The orphan is self-limiting (ICE never completes → 15s failed timeout tears it down), so a bounded self-healing orphan beats an unbounded silent kill. Follow-up needs a helper-side start generation.
• Important — the different-offer "take your own turn" branch was deterministically wrong on RDS. Leases are keyed per desktop session and share one owner, so the leader's failure path released the lease out from under the deferred caller, which then streamed unleased and got its helper reaped ~2 min in. Removed — a concurrent different-offer start for one desktop session isn't reachable in practice, and this also deleted the turn loop and its cap.
• Important — a panic in run() handed joiners a zero-valued CommandResult (Status "", no error text). call.result is now pre-set to a failure.

Tests: go test -race ./... green across the whole agent module. All three fix parts are mutation-verified — reverting the unique id, bypassing the classifier, or removing the single-flight each fails the suite. New tests 8/8 clean over repeated runs; the concurrency test asserts "at most one start executing per session", an invariant no scheduling order can flake. go build ./..., go vet, gofmt clean on touched files.

CI: the pull_request trigger did not fire for this branch — gh pr checks showed only a Cloudflare Pages entry, which reads green but means CI never ran. Dispatched by hand: run 31127460219 (workflow_dispatch), which picked up the full board including Test Agent, Test Agent (race), and Lint Agent (Go). It was still queued at hand-off — please confirm it went green before merging rather than trusting the PR check list.

Status: review-clean, awaiting maintainer merge. Not merged and #3107 left open per hand-off rules.