fix(sync): report uncorrelated missing-thread failures; E2E timeout-watcher race - #79
Merged
Merged
Conversation
The claude round was putting mismatched identity on the wire:
{"agent_name":"zed-agent", "request_id":"req-phase9-queue-claude"}
For a thread already in contextMappings, sendChatMessage deliberately uses the
PRODUCTION send path (srv.SendChatMessage) rather than QueueCommand -- which is
the point, it exercises real code. But that path ignores the agentName the
harness passes and derives it from getAgentNameForSession(), which reads
session.Metadata.ZedAgentName and otherwise falls back to "zed-agent".
websocket_external_agent_sync.go then persists that fallback back onto the
session, so it is sticky.
In production the fallback never fires: the value resolves from the spec task's
app code_agent_runtime. The E2E has no such app, so every session was stamped
"zed-agent" regardless of round, and the claude round was only nominally
claude on that path.
Sessions created during a round inherit ZedAgentName from the originating
session, so stamping the seed session at round start propagates it correctly.
This is a harness-fidelity gap, not a product bug -- recording it as such. It is
NOT yet proven to be the cause of the intermittent Phase 9 claude timeout: the
mismatch was present on every claude run, including six consecutive passes. It
is removed here so the next failure is diagnosed without it as a confounder.
go build: clean.
Root cause of the intermittent "PHASE 9 TIMED OUT" claude-round failures, caught
by running the suite to a target of ten consecutive passes.
The phase-timeout watcher aborts solely on `d.phase != phase`. But every advance
path deliberately settles BEFORE bumping d.phase: 500ms in the completion
handler, plus a further 2s inside advanceAfterCompletion ("let Zed settle").
That leaves a ~2.5s window in which the phase has already succeeded, d.phase is
still the old value, and the watcher will happily declare a timeout.
Caught red-handed in the log -- success and timeout one second apart:
11:43:57 [claude] Phase 9: Received enough completions -- thread did not hang
11:43:58 [claude] PHASE 9 TIMED OUT after 1m30s
It shows up in the claude round because claude is slower than zed-agent, so its
completions routinely land near the end of the 90s budget; heavy machine load
widens the window further. The zed-agent round almost always finishes with
seconds to spare, which is why this looked agent-specific rather than structural.
Adds phaseSucceeded, set the moment a success condition is met and checked by
the watcher alongside the existing d.phase comparison. Marked at the top of
advanceAfterCompletion (covering all six callers) and explicitly for phases 8
and 9, which settle before calling it.
Deliberately NOT fixed by enlarging the timeout: the phase was passing, so a
bigger budget would only have hidden a genuine bookkeeping race and made every
real hang take longer to surface.
go build: clean.
Auto-repair for a reaped/missing ACP thread was fully built on the Helix side and never fired, because Zed withheld the event that triggers it. report_thread_open_failure() bailed out early unless the ThreadOpenRequest carried a request_id. The case that matters carries none: the open_thread Zed sends automatically on reconnect is not tied to a user request. So when a task's workspace had been reaped and load_session failed with "Resource not found", the error was logged locally and never sent. Helix kept a zed_thread_id pointing at a session claude-agent-acp no longer had, every later reconnect re-failed the same way, and the user saw a permanently blank agent panel labelled "New Zed Agent Thread" -- which reads as lost agent identity but is actually a dangling pointer. Confirmed against the live incident: grep for thread_load_error in that container's Zed.log returns 0 hits, despite load_session failing. Helix's recovery gate is: isAuthoritativeMissingThreadError(errorMsg) && acpThreadID != "" It does not require a request_id -- that is only used for the optional HTTP-streaming reply -- so the correlation check was gating on something the consumer never needed. Reporting with an empty request_id lets recoverMissingThread clear the stale pointer, so the next message forks a clean thread instead of failing forever. The error string keeps the "Failed to load thread: " prefix that isAuthoritativeMissingThreadError parses, and the test now asserts that prefix so the two sides cannot drift apart silently. Inverts uncorrelated_thread_open_failure_emits_no_event, which asserted the buggy behaviour, into uncorrelated_thread_open_failure_still_reports. cargo test -p external_websocket_sync: 56 passed. cargo check -p zed --features external_websocket_sync: clean.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
lukemarsden
added a commit
to helixml/helix
that referenced
this pull request
Aug 10, 2026
Pins helixml/zed#79, which makes the dangling-thread case self-healing. Together with the reaper fix in this PR that is the full chain for the incident: Keep Alive is now honoured so the workspace is not deleted in the first place; and if a thread ever does go missing, Zed now reports it so Helix clears the stale zed_thread_id automatically instead of showing a blank agent panel.
…dog-state-based # Conflicts: # crates/external_websocket_sync/e2e-test/helix-ws-test-server/main.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #74, plus the root-cause fix for a live incident where a session came back with a permanently blank agent panel.
Paired Helix PR (pins
ZED_COMMIT): linked below.1. Report missing-thread failures that have no
request_id(the incident fix)Auto-repair for a reaped/missing ACP thread was already fully built on the Helix side and never fired, because Zed withheld the event that triggers it.
report_thread_open_failure()bailed out unless the request carried arequest_id. The case that matters carries none — theopen_threadZed sends automatically on reconnect isn't tied to a user request. So when a task's workspace had been reaped andload_sessionfailed withResource not found, the error was logged locally and never sent. Confirmed on the live container:grep -c thread_load_errorreturns 0, despite the load failing.Helix's recovery gate is:
It never needed a
request_id— that's only used for the optional HTTP-streaming reply. So the correlation check gated on something the consumer didn't want, and starvedrecoverMissingThreadof the one event that clears the stale pointer. Result:zed_thread_idkept pointing at a sessionclaude-agent-acpno longer had, every reconnect re-failed identically, and the user saw a blank "New Zed Agent Thread" — which reads as lost agent identity but is a dangling pointer.Inverts
uncorrelated_thread_open_failure_emits_no_event, which asserted the buggy behaviour, intouncorrelated_thread_open_failure_still_reports. The test also pins the"Failed to load thread: "prefix that Helix parses, so the two sides can't drift apart silently.2. E2E: timeout watcher failed already-successful phases
Caught red-handed, one second apart:
The watcher aborts solely on
d.phase != phase, but every advance path settles before bumpingd.phase— 500ms in the completion handler plus 2s insideadvanceAfterCompletion. That ~2.5s window is enough to fail a passing phase whenever completions land near the deadline, which is routine for the slower claude round and likelier under load.Adds
phaseSucceeded, set the moment a success condition is met and checked by the watcher. Deliberately not fixed by raising the timeout — the phase was passing, so a bigger budget would have hidden a real bookkeeping race.3. E2E: stamp the round's agent on the seed session
The claude round put contradictory identity on the wire (
agent_name: "zed-agent"withrequest_id: "req-phase9-queue-claude"), because the production send path derives the name fromgetAgentNameForSession()which falls back to"zed-agent"whenZedAgentNameis unset. Harness-fidelity gap, not a product bug.Validation
cargo check -p zed --features external_websocket_sync— cleancargo test -p external_websocket_sync— 56 passedzed-e2e-testwill exercise it post-merge.🤖 Generated with Claude Code