reconnect workspace adapters after liveness loss#526
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zomux
left a comment
zomux
left a comment
There was a problem hiding this comment.
Thanks for this — the reconnect design is solid (generation-counter model, AND-gated trigger, bounded jittered backoff, cursor/dedup preserved, never POSTs /leave on auto-exit). A few things to address before merge:
1. (Required) Control events issued during an outage are silently dropped on reconnect.
_runConnectedSession calls _skipExistingControlEvents() on every session start, including reconnects, which advances _lastControlId to head. So a /restart, /stop, or set_mode a user issues while the agent is disconnected is skipped after reconnect and never acted on. This is asymmetric with message events, which you correctly preserve via _lastEventId. Please gate _skipExistingControlEvents() on this._firstConnect (mirroring how _skipExistingEvents() is already gated), so stale control events are only skipped on the first connect after a fresh daemon start, not on in-process reconnects.
2. (Recommended) Classify HTTP 429 (and probably 408) as retryable.
_classifyError currently routes unexpected 4xx (incl. 429 rate-limit) to ambiguous, which is capped at 5 attempts then terminal join_failed. During a mass fleet reconnect after a server restart (the #492 scenario this helps with), 429s could exhaust the ambiguous budget and permanently drop agents that should keep retrying. Treat 429/408 as retryable.
3. (Please verify) Control poller isn't generation-scoped.
_controlPollerLoop loops on _running only, not gen. On reconnect the finally awaits the per-session control poller after _wakeControlPoller(); under a slow control endpoint this could stall re-join. A gen === this._activeGen guard in the control loop (or starting it once in the supervisor instead of per-session) would make this robust. The 728-line test stubs _controlPollerLoop to a no-op, so this path is currently untested — a test exercising the real loop on reconnect would help.
Also note: this branch is now CONFLICTING against develop (base.js has changed substantially via the merged #527 readiness work). It'll need a rebase. Happy to re-review promptly once these are addressed.
0f83f3a to
19acb21
Compare
…08, gen-scope control poller 1. Control events issued during an outage are no longer dropped: _skipExistingControlEvents() is now gated on _firstConnect (mirroring _skipExistingEvents), so the control cursor is only jumped to head on the first connect after a fresh daemon start. On reconnect the cursor is preserved and /stop, /restart, set_mode issued while disconnected are delivered once the session is re-established. 2. HTTP 429/408 classify as retryable instead of ambiguous, so a mass fleet reconnect that gets rate-limited keeps backing off instead of exhausting the 5-attempt ambiguous budget and going terminal. 3. _controlPollerLoop is generation-scoped (legacy no-arg behavior unchanged): on reconnect the invalidated poller exits on wake without issuing another _pollControl, so a slow control endpoint can't stall the awaited teardown and delay re-join. Each fix is covered by a test that fails without it; the control-poller tests run the real loop instead of the no-op stub.
25099a9 to
affdd22
Compare
2 participants
Fix workspace adapters getting stuck running but offline.
Adds automatic join retry and reconnect after heartbeat failures, while avoiding unsafe remote leave calls during session changes.