[fix] Converge the desktop session when a gate is answered elsewhere (11/12) - #5690
[fix] Converge the desktop session when a gate is answered elsewhere (11/12)#5690ardaerzin wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
6101ac7 to
f957a88
Compare
28314ae to
cc14c6a
Compare
f957a88 to
f34d0e1
Compare
cc14c6a to
0aa2522
Compare
f34d0e1 to
4223629
Compare
0aa2522 to
cfb9c84
Compare
4223629 to
fed6233
Compare
cfb9c84 to
defca8b
Compare
fed6233 to
0e3c2cb
Compare
defca8b to
de37daa
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
0e3c2cb to
86c74ff
Compare
de37daa to
94e03ca
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
86c74ff to
ea4d80e
Compare
94e03ca to
52d6e1f
Compare
ea4d80e to
e13f3f2
Compare
52d6e1f to
4c08deb
Compare
e13f3f2 to
e1bd8b8
Compare
4c08deb to
09c9335
Compare
e1bd8b8 to
2ddfa94
Compare
09c9335 to
02fb123
Compare
02fb123 to
24ac72c
Compare
2ddfa94 to
d13945a
Compare
24ac72c to
02804ea
Compare
dosubot
Bot
added
size:XL
Railway Preview Environment
|
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
🤖 The AI agent says:
@ardaerzin I found five concurrency and convergence issues in this pass: four P1 findings and one P2 finding.
- The records worker can cancel a newer turn's live gate.
- Heartbeat lock handling can let tombstoned beats retain replica affinity or reacquire
aliveafter cancellation. - The desktop watch can drop the terminal record notification needed to settle a resumed approval.
- The runner can publish
donebefore it durably persists the in-band verdict, allowing reconciliation to record abandonment instead.
I added an inline comment at each site with a concrete fix direction.
| ) | ||
| if latest is None or str(latest.turn_id) != str(turn_id): | ||
| continue | ||
| cancelled = await self.interactions_service.cancel_session_pending( |
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Keep new-turn gates out of stale cancellation
When a new turn begins and parks after latest_turn() returns but before this session-wide cancellation executes, its live pending gate is cancelled along with the orphan. The latest-turn query is therefore a TOCTOU guard. Scope the update to interactions belonging to turns no newer than the finished turn, or perform the ledger check and cancellation atomically.
| # turn that already lost the nest, so the tombstone written at the moment it lost it | ||
| # is the discriminator the locks alone cannot provide. Refusing early also stops the | ||
| # zombie's own turn-end beat from clearing the LIVE turn's `running`. | ||
| if request.turn_id and await is_turn_superseded( |
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Reject superseded beats before claiming affinity
For a tombstoned turn, claim_owner() has already renewed or recreated the replica owner key before this check returns is_current_turn=false. After kill or handover, a late beat can retain dead-replica affinity and block another replica from claiming the session for the owner TTL, indefinitely if beats repeat. Check supersession before mutating the owner lock.
| displaced_running = await clear_running( | ||
| self._lock, project_id=str(project_id), session_id=session_id | ||
| ) | ||
| await self._supersede_turns( |
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Close the heartbeat race when tombstoning a turn
If a heartbeat passes the supersession check immediately before cancel reaches this call, cancel can delete and tombstone the turn while that heartbeat subsequently reacquires alive. The heartbeat then returns non-current but still mirrors is_alive=true, resurrecting the cancelled session until TTL expiry. Make displacement and acquisition atomic, or recheck and release after acquisition before writing the row.
|
|
||
| const notify = () => { | ||
| const now = Date.now() | ||
| if (now - lastNotifiedAt < MIN_INTERVAL_MS) return |
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Coalesce throttled record notifications
When resumed-turn record batches arrive less than three seconds apart, this drops every later notification instead of scheduling a trailing refresh. A first fetch can adopt a partial replay while the final done batch that settles the approval is discarded. Because a parked session has no runningElsewhere poll, the desktop can remain stale indefinitely. Coalesce a trailing callback, or throttle only reconnect notifications as the mobile hook does.
| // Settle the durable rows this turn's in-band answers consumed but no harness gate resolved. | ||
| // Idempotent (guarded by the resolved-token set) and never throws, so it is safe on the error | ||
| // and cancel paths too — a row whose gate is gone is unanswerable however the turn ended. | ||
| settleInBandInteractions?.(); |
There was a problem hiding this comment.
🤖 The AI agent says:
[P2] Persist in-band verdicts before flushing done
This fire-and-forget settlement runs only after run.finish() and run.flush() have published the terminal record. The records worker can consume that done first and cancel the still-pending row. The later resolve then fails because cancelled is terminal, recording an actual approval or denial as abandonment. Complete the durable transition before exposing the terminal record to reconciliation, or otherwise enforce this sequencing.
A turn approved elsewhere (e.g. from mobile) replayed as still parked: the durable record log carries the paused turn's interaction_request but no interaction_response, so the folded resume kept its tool part in approval-requested. The desktop adoption guard reads that as 'the server is parked too' and refused to adopt, leaving a reloaded session stuck on the pre-approval transcript with the approval dock up. Records that continue past a paused done mean the gate was answered (a deny settles its own part via tool_result denied), so settle whatever is left awaiting.
…h relay Subscribes the ACTIVE conversation to GET /sessions/streams/watch (the M3 SSE relay mobile already consumes) and, on records-changed, revalidates the durable records and re-applies the same guarded adoption the revalidate-on-open pass uses — so a turn that advances elsewhere lands in an open tab within seconds instead of only on reload. One foreground-only EventSource per panel, throttled, skipped while this tab streams; a fatal close retries every 60s and the reload path stays the fallback.
…solved A resume whose harness never re-raises the gate (cold replay, or a client-built resume that lands as a fresh turn) consumed the human's decision without transitioning the durable row, leaving it pending and forever actionable on every surface. The runner now settles those rows from the turn's in-band answers on every exit path, with the verdict the human gave, and the records worker cancels any gate whose turn reached a terminal record without pausing — guarded so a live park is never swept.
…biguity `alive` outlives its turn and a turn parked awaiting approval also clears `running`, so "alive held by another turn + no running" cannot tell a lapsed previous turn (a legitimate handover) from a live-but-parked one. Resolving it as a handover — which it must be, or every follow-up turn on a warm session aborts — let a zombie beat from an older turn take the nest of a parked session, after which the user's approval resume reported is_current_turn=false and aborted. Record what IS knowable at the moment it happens: every displacement (heartbeat handover, cancel, steer, kill, orphan sweep) now tombstones the turn it displaced, and a tombstoned turn's beats are refused before touching any lock or the stream row. A zombie is by definition a turn that already lost the nest, so `displaced => dead` is the discriminator the locks cannot provide. The ambiguous state keeps resolving as a handover; only a never-displaced turn can reach it. Also stops a superseded turn's is_running=false beat from clearing the LIVE turn's `running`, and a cancelled turn's beat from re-acquiring `alive`. The heartbeat wire is unchanged: `is_current_turn: false` already means "abort", so the runner needs no change and no restart.
Mobile showed assistant text through `whitespace-pre-wrap`, so a reply containing markdown displayed literal fences, `#` headings and `-` bullets. Desktop renders the same content properly. Adopt Streamdown (the renderer the mobile design and AI Elements already name) for assistant TEXT parts. User messages stay literal — markdown in your own words inside a bubble is surprising. Reasoning stays plain: it is collapsed by default and streams as fragments that incomplete-markdown repair would make flicker. - `isLiveTextItem` restricts incomplete-markdown repair to the one text item the stream is still appending to; settled text must not get it or a message legitimately ending in `**` is silently rewritten. - Streamdown's defaults are re-scaled to the app's 12px type ramp via descendant selectors, and its `sidebar` role (absent from the token bridge) is re-surfaced onto `muted`. Semantic tokens only. - Code blocks keep their own `overflow-x-auto`; the prose root wraps long unbroken tokens so the page body never scrolls horizontally. - Raw HTML is neutered by Streamdown's default rehype-raw -> rehype-sanitize -> rehype-harden pipeline; links get an explicit `rel="noopener noreferrer"`.
The SSE relay never told the client how long to wait before reconnecting, so the interval was implementation-defined — an API restart or deploy dropped every open stream and they all came back at once. The generator now leads with a `retry:` preamble (AGENTA_SESSIONS_WATCH_RETRY_ MILLISECONDS, default 5s), emitted after SUBSCRIBE so no event can land in an unsubscribed window between the client's `open` and the first frame.
At every access-token refresh boundary the whole client 401s for one round trip. The Fern/axios layers refresh and retry transparently; an EventSource has no interceptor, so the relay died and then sat blind for a fixed 60s — and a persistent failure retried on a flat cadence with no jitter. Both hooks now reopen on a jittered exponential backoff (1s → 30s, reset on `open`) and attempt a session refresh first, so an expired token costs seconds instead of a minute. Mobile also throttles the reconnect revalidation to the 3s desktop already used, so a reconnect loop can no longer fan out into one full records refetch per attempt (real `records-changed` events stay instant).
d13945a to
247651d
Compare
02804ea to
200a483
Compare
2 participants
Context
Once a phone can answer approvals, the desktop tab looking at the same session has to notice. It did not: an approval answered elsewhere left the desktop showing "Approval needed to continue" until a manual reload, and sometimes after one.
Changes
Desktop settles a resumed turn's gate on replay and converges an open session through the watch relay from lane 9, refreshing the session before reopening a relay it finds dead.
Two backend correctness fixes sit underneath. Gates whose turn was consumed but never resolved are settled rather than orphaned as
pendingforever. And a displaced turn is treated as dead, which closes the parked-session lock ambiguity: previously a displaced turn's late heartbeat could re-arm a lock that a newer turn owned.Mobile also renders assistant messages as markdown here, so a reply reads the same on both surfaces.
Tests / notes
AgentChatSlicehooks and the transcript adapter. Small diff, high scrutiny: this is the shared agent chat.What to QA