[Fix] Live Fast turns get stamped as interrupted after 15 minutes - #2007
Conversation
The 15-minute responding lease was extended only when an assistant message persisted, so a turn spending longer than the lease inside tool calls or a streaming stretch let it lapse while still running. The expired-lease reconciler would then stamp the turn's live retry notice as an interruption the user never had. The turn now also renews the lease on wall clock (every lease/3) for as long as it executes; the tick checks ownership so a fenced-off owner that lost its conversation lock cannot extend a successor's lease, and settling the turn stops the timer and clears the lease exactly as before.
|
1 issue outstanding. See task
Reviewed c3306d4 |
![roomote-community[bot]](https://avatars.githubusercontent.com/u/211522643?s=60&v=4){kind=small}
| // successor's lease. | ||
| respondingLeaseRenewalTimer = setInterval(() => { | ||
| if (signal?.aborted) return; | ||
| void setFastSessionResponding(session.id, true).catch((error) => { |
There was a problem hiding this comment.
The renewal is fire-and-forget, so clearing the interval does not wait for a tick already in progress. If that call passes the abort check and awaits getSessionForFastConversation or touchSessionActivity while finally writes { respondingUntil: null }, the clear can commit first and the stale renewal can commit afterward. That leaves an idle Session marked responding for another lease (and a just-fenced owner can still extend the successor's lease). Serialize or await the in-flight renewal during settlement and re-check ownership immediately before the write; add a delayed-write regression test.
There was a problem hiding this comment.
Fixed in ac1c69a. Renewal ticks now chain onto one promise, with the ownership check re-evaluated when each chained write actually runs, and settlement awaits the in-flight renewal before writing respondingUntil null, so a stale extension can no longer commit after the terminal write. Added the delayed-write regression test: a renewal stalled mid-write blocks settlement until it resolves, and the terminal null write is the last lease write.
There was a problem hiding this comment.
This remains unresolved. Chaining moves the abort check to when the queued renewal starts, but setFastSessionResponding() still awaits getSessionForFastConversation() before it writes. If ownership is lost during that await, this invocation still calls touchSessionActivity(..., { respondingUntil }) afterward. Re-check ownership after the lookup/immediately before the write (and cover that interleaving in the regression test).
There was a problem hiding this comment.
Fixed in 86efd67. setFastSessionResponding now takes an ownership predicate that is re-checked after the session lookup, immediately before the write, and both lease-extending call sites (initial mark and wall-clock renewal) pass it; the terminal clear stays unguarded so shutdown settlement still records idle. Added the regression test for exactly this interleaving: ownership lost while the renewal's lookup is in flight results in no lease write, and the fenced-off owner leaves the lease untouched.
There was a problem hiding this comment.
This is still racy, just one layer later. setFastSessionResponding() checks the predicate before calling touchSessionActivity(), but that helper awaits SELECT ... FOR UPDATE and a linked-task query before it issues the session update. If ownership is lost while either awaits, the already-authorized renewal still writes respondingUntil afterward. Move the ownership check into the status helper at its actual update boundary (or make the write atomically fenced), and cover a renewal blocked inside touchSessionActivity, not only the outer session lookup.
There was a problem hiding this comment.
Fixed in c3306d4, taking your atomically-fenced option rather than pushing the check one layer deeper again. Renewals no longer go through the status helper at all: renewFastSessionRespondingLease is a single conditional UPDATE that extends respondingUntil only where the lease is still live (IS NOT NULL AND > now()), with no read preceding the write, so a stale renewal from a fenced-off owner cannot resurrect a cleared or expired lease under any interleaving. The tick's abort check remains as a cheap short-circuit only; correctness lives in the statement. DB tests cover the fence directly against cleared, expired, and live leases, and service tests cover the queued-tick abort path and settlement ordering.
There was a problem hiding this comment.
This still does not fence the renewal to its lock owner. If this turn loses Redis ownership after the renewal has begun while its existing lease is live, responding_until > now() remains true and this statement extends it for another 15 minutes. The same condition lets a stale owner extend a successor's already-live lease. The conditional update prevents resurrection after a clear/expiry, but it needs an owner/turn generation fence (and a regression test that aborts a renewal before its conditional UPDATE executes while the lease stays live).
There was a problem hiding this comment.
Acknowledged, and the analysis is correct: the conditional UPDATE fences resurrection after clear/expiry but not a stale owner extending a still-live lease. Two points on scope. First, that residual is pre-existing on develop independent of this PR: assistant-message persists extend respondingUntil through the upsert path with no ownership fence today, so a fenced-off owner already has this property before any of this code runs. The exposure either way is bounded status freshness (a successor's unconditional settlement clear removes any stale extension; absent a successor the lease self-heals on expiry at the same 15-minute ceiling that predates this PR), not a correctness hole. Second, a real owner/generation fence needs an ownership identity column on sessions with N-1 rollback handling, which is precisely what the planned durable turn-run state machine provides (lease owner plus generation). Building a one-off inside this PR would duplicate that work. Proposing to land this PR as a strict improvement and route the generation fence to the durable-execution work where it belongs.
A fire-and-forget renewal tick already past its ownership check could commit after settlement wrote respondingUntil null, leaving an idle Session marked responding for another lease. Renewals now chain onto one promise with an ownership re-check when each chained write actually runs, and settlement awaits the in-flight renewal before recording the terminal lease state.
setFastSessionResponding awaits a session lookup before writing, so an owner fenced off during that await could still extend the successor's lease. Lease-extending calls now pass an ownership predicate that is re-checked after the lookup, immediately before the write; the terminal clear is unguarded as before. Regression test covers ownership lost while the renewal lookup is in flight.
Predicate checks before an awaited write always leave a window one layer deeper. Renewals now go through renewFastSessionRespondingLease, a single conditional UPDATE that extends respondingUntil only where the lease is still live, so a stale renewal from a fenced-off owner can never resurrect a lease a settlement or successor already cleared. No read precedes the write. The abort check in the tick remains as a cheap short-circuit only. DB tests cover the fence directly (cleared, expired, and live leases); service tests cover the queued-tick abort path and settlement ordering.
1 participant
Problem
The scheduled reconciler stamps "The inference retry was interrupted..." onto any conversation whose retry notice is still active once the session's responding lease expires. The 15-minute lease is set at turn start and extended only as a side effect of persisting an assistant message — tool events and long streaming stretches never touch it. A turn that recovers from a provider blip (leaving its retry notice active by design until the closeout) and then spends 15+ minutes in tool-heavy work gets stamped as interrupted in its canonical transcript while it is still running. Long investigation Sessions are exactly this profile, and this is one of the producers the attribution work in #2004 was built to expose (
expired_lease_reconcile).Change
The turn now also renews the responding lease on wall clock, every lease/3 (5 minutes), for as long as it is executing:
respondingUntilsemantics for idle Sessions are unchanged.The reconciler itself is untouched: an expired lease still means what it always meant, it just can no longer be true for a turn that is actively executing.
Validation
@roomote/cloud-agents: 332 tests pass.pnpm lint:fast,pnpm check-types:fast,pnpm knipall pass.Context
PR 3 of the interruption work: #2004 (attribution) is merged, #2006 (shutdown drain) is in review. After both land,
expired_lease_reconcilestamps on live turns should drop to zero, and the remaining attribution counts tell us whether #1966's recovery path still earns a merge.