Repository navigation
feat(session): add opt-in crash recovery on send - #3399
Conversation
github-actions
Bot
added
enhancement
5ac87ab to
4972bde
Compare
4972bde to
2eafd11
Compare
github-actions
Bot
added
enhancement
2eafd11 to
270ea5c
Compare
github-actions
Bot
added
enhancement
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
enhancement
| FROM session_record i | ||
| WHERE ($1::text = '' OR i.id > $1::uuid) | ||
| AND i.state = 'RUNTIME_STATE_FAILED' AND i.operation = 'RUNTIME_OPERATION_RESUME' | ||
| AND (($3 AND i.state = 'RUNTIME_STATE_READY' AND i.operation = 'RUNTIME_OPERATION_NONE') |
There was a problem hiding this comment.
Agreed. The scan needed one interval per 100 READY Sessions and spent one GetActor on each healthy runtime. As of 94026a0, each interval lists the Actors in the atespaces that hold a READY Session, in pages of 1,000, and reads only the CRASHED ones individually. Revert re-observes the Actor before it admits anything, and PostgreSQL paging now covers admitted recoveries only. On a dev fleet with about 505 READY Sessions and a 15s interval, a worker loss took 69–73s to recover before and 0.4–11s after. An idle controller went from 507 GetActor calls a minute to 6 ListActors calls (crash-detection.txt, substrate-rpcs-idle.txt):
before READY=506 recovered 1m13.1s after the worker deletion (4.9 intervals)
after READY=507 recovered 11s after the worker deletion (0.7 intervals)
idle 60s, interval 10s, READY=507: before 507 GetActor after 6 ListActors
|
Review follow-up: a failed automatic recovery must still leave the user a way to revert or fork. The review thread that raised this is gone, so I'm answering here. Agreed, and my first fix (02bd7b6) only half closed this. It added Revert to the alert in automatic mode, but disabled the button while the Session's operation was RESUME. An admitted recovery keeps RESUME through every failed attempt, so after the first automatic attempt failed, Revert spun until a later attempt succeeded. 5ae5c00 disables it only while the page's own request is in flight. A click joins the admitted recovery. After merging #3398 (11f9abb, 40e3702), Fork lives on each saved snapshot's line in the transcript, which renders in automatic mode too, so there is one place to fork. On a dev fleet I held the automatic revert failing. Before, at 3fc96de: After, pending, forked, and reverted: |
749d4b4 to
8d4cff0
Compare
There was a problem hiding this comment.
🤖 AI-generated review
The values.yaml change and the helm-unittest guard look good. A default install no longer renders the key, so a rollback can still read the config. With the flag off, sends keep their old refusals, and TestSendRecoveryDisabled covers all three send paths. The comments below are small.
| source = client.sendStreamingMessage(request, options); | ||
| continue; | ||
| } | ||
| if (!received && !signal?.aborted && a2aRefusal(error)) throw refusedSendError(error, SEND_REFUSED); |
There was a problem hiding this comment.
Optional: this tags every A2A refusal that arrives before the first frame, so chat says the runtime may have crashed for any of them. The reason: "SOMETHING_ELSE" test case expects SEND_REFUSED too.
The case this targets is the open-turn conflict from interactionStoreError. Could the gateway name that refusal in ErrorInfo metadata, as it does for KAGENT_SEND_NOT_ACCEPTED? Then chat could tag only that reason, and other refusals would keep the plain error with Retry. Recover is harmless either way, so this only affects what the user reads.
There was a problem hiding this comment.
Done in 03aa8f22, and narrowed to open turns in 01f14d6a. The gateway now names the open-turn refusal KAGENT_TURN_ACTIVE in its ErrorInfo metadata; the code and message are the same as before. Chat offers Recover only for that reason and KAGENT_SEND_NOT_ACCEPTED. Other refusals keep the plain error with Retry.
| // The mock chat client has no A2A push store; hide the control rather | ||
| // than showing a backend error over an otherwise scripted transcript. | ||
| // A conversation that is not ready has its own alert with Recover. | ||
| onRecover={canSend ? () => void recover() : undefined} |
There was a problem hiding this comment.
Small one: the two new props sit between onManageTaskPush and its comment about the mock push store. That comment now reads as if it describes onRecover. Could the new props move above it?
Related: the onRecover doc in ChatTranscript.tsx says Recover is offered when sends are refused as not accepted. It is now offered for outright refusals too.
There was a problem hiding this comment.
Fixed in 03aa8f22. The new props now sit above that comment, and the onRecover doc lists both refusals that offer Recover.
| import { CheckpointDivider } from "./CheckpointDivider"; | ||
| import { groupByCheckpoint } from "./messageCheckpoints"; | ||
| import { scrollbarStyles } from "@/components/agent/controlStyles"; | ||
| import { isSendNotAccepted, mayNeedRecovery } from "@/api/chat/a2aGrpcChatClient"; |
There was a problem hiding this comment.
Optional: this is the only component that imports from the gRPC chat client. isNotFound and the other error predicates live in ApiError.ts. Could SEND_NOT_ACCEPTED, SEND_REFUSED, isSendNotAccepted, and mayNeedRecovery move there? Then the transcript depends only on ApiError.
There was a problem hiding this comment.
Moved in 03aa8f22. The reasons and predicates live in ApiError.ts and are exported from @/api, so the transcript no longer imports the chat client.
| req.Config = &config | ||
| } | ||
| id, err := s.reserveDispatch(ctx, session.Id, initialID) | ||
| if s.automaticRecovery && errors.Is(err, database.ErrConflict) { |
There was a problem hiding this comment.
ErrConflict here also covers a checkpoint in CREATING and a Session that isn't READY, so healthy Sessions call Revert (and GetActor) too. And if that Revert fails with Unavailable or Aborted, the caller gets ErrSendNotAccepted in place of the busy refusal, so a Substrate outage turns a definitive "busy" into a retry loop.
Can we fall back to the original refusal when recovery isn't available, like the unauthorized case already does?
There was a problem hiding this comment.
Fixed in 01f14d6a. Only an open turn starts recovery now. The store reports it as ErrActiveTurn, which is still an ErrConflict, so a checkpoint in progress or a Session that isn't READY keeps its refusal without a revert. If the revert can't run (outage, contention, or no update access), the send gets the same busy refusal as before, not ErrSendNotAccepted. A gateway test covers the outage case.
| if err != nil { | ||
| return err | ||
| } | ||
| if err := s.recoverRuntime(ctx, session.Id); err != nil { |
There was a problem hiding this comment.
Small one: PrepareSend falls back to the original refusal on ErrUnauthorized, but this path and the admitted path in resolveSend return permission denied, even for a transient not-accepted send on a healthy runtime, because Service.Revert authorizes before it checks health. The built-in authorizer grants create and update together, so it needs a custom authorizer to show up, but can the three be consistent?
There was a problem hiding this comment.
Done in 01f14d6a. All three paths now share one helper and handle a failed revert the same way. If no recovery is admitted, the send gets the answer it would give without recovery. If one is admitted, it gets the new recovering answer. Missing update access is one of those cases, so none of the paths returns permission denied for it any more.
| // outages answer ErrSendNotAccepted, so the caller retries. The revert outlives a | ||
| // caller that disconnects, so a retry finds it finished. | ||
| func (s *InteractionService) recoverRuntime(ctx context.Context, sessionID string) error { | ||
| _, err := s.sessions.Revert(context.WithoutCancel(ctx), sessionID) |
There was a problem hiding this comment.
finishSend bounds its context to 5s, but WithoutCancel drops that deadline, and workflow.Revert then runs under the 2 minute RuntimeOperationTimeout. So a send can wait ~2 minutes before it answers, on all three paths. Is that intended?
I think the send should either answer "retry" once recovery is admitted and let the revert finish on its own, or have an explicit bound.
There was a problem hiding this comment.
Not intended. Fixed in 01f14d6a: a send now waits at most 10 seconds for the recovery it starts. If the revert isn't done by then, the attempt releases its claim and the send answers that the runtime is recovering. The client's next retry continues the admitted recovery, so nothing keeps running after the request ends.
| if err := s.recoverRuntime(ctx, session.Id); err != nil { | ||
| return nil, err | ||
| } | ||
| return nil, ErrSendNotAccepted |
There was a problem hiding this comment.
During recovery the client gets KAGENT_SEND_NOT_ACCEPTED with retryAfterMs: 100, the same as an ordinary busy dispatch, so it can't tell "we're reverting, come back shortly" from "busy".
Can we return a separate reason for it, e.g. KAGENT_RUNTIME_RECOVERING, with a retryAfterMs in seconds? The UI could show "recovering" from that directly, and A2A clients get the signal without calling GetSession.
There was a problem hiding this comment.
Added in 01f14d6a, but as metadata rather than a new reason. reason stays KAGENT_SEND_NOT_ACCEPTED, and the error adds state: RECOVERING and retryAfterMs: 1000. The CLI, the e2e helpers and enterprise's SendMessageWithRetry only retry on that exact reason, so a new reason would make recovery a hard failure for clients that are already released. Chat already shows "recovering" from the Session while it retries, and now also says so if its retry window runs out.
| @@ -50,39 +57,63 @@ func (w *ActorWorkflow) observeCrash(ctx context.Context, session *apiv1.Session | |||
| TemplateAtespace: generation.TemplateAtespace, TemplateName: generation.TemplateName} | |||
| actor, err := w.actors.GetActor(ctx, binding.Atespace, binding.Name) | |||
| if status.Code(err) == codes.NotFound { | |||
There was a problem hiding this comment.
Question: one GetActor NotFound marks the Session FAILED for good and fails every open task, including ones waiting on input. Can Substrate ever return NotFound for an Actor that still exists, e.g. while it's restarting? I didn't find a case, but since there's no way back from RuntimeLost I want to make sure a single observation is enough.
There was a problem hiding this comment.
One observation is enough. In Substrate v1.0.0, GetActor is a primary-key read of ate-api's own PostgreSQL actors table (cmd/ateapi/internal/store/atepg/actor.go), with no cache in between. Crashes, restarts, suspends and reverts all update that row. Only DeleteActor removes it, and it refuses while a worker still holds the Actor. The atespace foreign key is ON DELETE RESTRICT, so no cascade removes it either. A database error comes back as Internal, not NotFound.
| reverted, err := fixture.sessions.RevertSession(fixture.ctx, request) | ||
| _, next := newMessageRequest(t, "What is 2+2?") | ||
| next.Tenant, next.Message.ContextId = fixture.tenant, fixture.sessionID | ||
| answered, err := sendMessageWithRetry(fixture.ctx, fixture.client, next) |
There was a problem hiding this comment.
This crashes mid-turn and sends, so it only exercises the conflict path in PrepareSend. RecoverRejectedSend through finishSend has no e2e.
The rename also dropped the e2e for an explicit RevertSession on a crashed runtime, which is what the new Recover button calls; it's now only called on a healthy runtime. Can we keep that case as its own test?
There was a problem hiding this comment.
Restored in 01f14d6a. TestSessionRevertCrashedRuntime is back as its own test. It calls RevertSession on the crashed runtime, including a retry after a lost response, and sits beside the send-recovery test; they share a crash helper. Both kagent subtests pass on kind with the flag on.
I couldn't add a reliable e2e for RecoverRejectedSend. It needs a crash after the dispatch is reserved but before the runtime saves the input, and a test can't time that. Gateway unit tests cover it.
|
Added in 27f9b072: recovery for a runtime that crashes during the post-turn suspension. Why. After every turn, kagent suspends the Actor. If the worker is lost during that suspend (a node drain, rollout, scale-down or spot loss, the events in #394), the Actor ends What changed.
Evidence (gist): before/after on kind, with the worker force-deleted while the Actor was
The gist includes the full logs and explains how the suspend window was widened for the reproduction. There's no e2e test, because hitting the window needs control of object storage. |
codyhartsook
left a comment
codyhartsook
left a comment
There was a problem hiding this comment.
🤖 AI-assisted review.
One note on the client retry guide, inline.
| A send recovers a crashed runtime. It returns `KAGENT_SEND_NOT_ACCEPTED` after reverting, so retry the send as usual. | ||
| A send refused because a crashed turn is still active recovers the runtime first and then proceeds. | ||
| A send while recovery is pending joins it. Recovering through a send needs Session update access. |
There was a problem hiding this comment.
automaticRecovery is off by default, and then none of these lines hold: a crashed turn is refused with KAGENT_TURN_ACTIVE, and a send to a FAILED/REVERT Session is refused outright. Line 57 has the same problem.
Could these be scoped to the flag, with the default case saying to call RevertSession? A line on state: RECOVERING would help too.
There was a problem hiding this comment.
Fixed in a8e3b812. The section now starts with the default: sends don't recover, a crashed turn is refused with KAGENT_TURN_ACTIVE, and a send to a pending recovery is refused, so the client calls RevertSession and then sends again. Recovery on send is described separately under automaticRecovery, including state: RECOVERING and its retryAfterMs. Line 57 now says to call RevertSession again, with a retried send also continuing the recovery when the flag is on.
codyhartsook
left a comment
codyhartsook
left a comment
There was a problem hiding this comment.
🤖 AI-assisted review.
One non-blocking bug in the chat send retry, inline.
| return; | ||
| } catch (error) { | ||
| const resendAfter = received ? undefined : unacceptedSendRetryMs(error); | ||
| if (resendAfter !== undefined && !signal?.aborted) { |
There was a problem hiding this comment.
Non-blocking, and probably rare: Stop doesn't end this loop. Before a task has a name, cancel() in useChat only sets cancel-when-named and never aborts signal. So a stopped message keeps re-sending, and an accepted re-send runs the turn. It takes Stop before the first frame plus a not-accepted refusal, with the flag on or off.
Repro on kind with the PR-head UI, holding a suspension claim in PostgreSQL so each send is refused after 10s:
- +3.2s: Stop, and chat shows Cancelled
- +10.2s: chat re-sends, then the claim is released
- +10.6s: the re-send is accepted, the model is called, then
CancelTask - The agent's reply to the stopped message appears in chat
This fix made the same repro send nothing after Stop:
// useChat.ts, per run
+const stopResending = new AbortController();
cancelWhenNamedRef.current = () => {
cancelWhenNamed = true;
+ stopResending.abort();
};
...
+stopResending: stopResending.signal, // new SendMessageInput field
// here
-if (resendAfter !== undefined && !signal?.aborted) {
+if (resendAfter !== undefined && !signal?.aborted && !stopResending?.aborted) {
...
await pause(resendWait, signal);
+ if (stopResending?.aborted) throw error;A useChat-level test would cover it. The client test aborts signal directly, which Stop never does.
There was a problem hiding this comment.
Good catch, fixed in fb9a31d1, close to your diff. useChat passes a per-run stopResending signal that Stop aborts. After Stop, the client starts no new attempt, ends the backoff wait early, and ends the turn with the last refusal. That refusal proved nothing was accepted, so nothing is left running. An attempt already in flight still goes through the existing cancel-by-name path.
The new useChat test drives the real client through a transport that refuses every send as not accepted, then presses Stop after the first refusal. Without the fix it re-sent 16 times; with it, it sends once, and the unaccepted message is taken back off the screen.
|
|
||
| // ErrRuntimeUnrecoverable answers a send to a Session whose recorded Actor is | ||
| // missing or was replaced. Retrying cannot succeed. | ||
| var ErrRuntimeUnrecoverable = a2atype.NewError(a2atype.ErrUnsupportedOperation, "Session runtime cannot be recovered; start another conversation") |
There was a problem hiding this comment.
Nit for a follow-up, can't this technically return an error instructing the user how to manually call RevertSession
There was a problem hiding this comment.
Done in a follow-up, #3666, stacked on this branch. The refusal now ends with "To retry recovery manually, call kagent.api.v1.SessionService/RevertSession with this Session's ID." The gRPC code and ErrorInfo reason are unchanged. One caveat, also in the PR: this error only fires when the recorded Actor is missing or was replaced, and for that case RevertSession answers FAILED_PRECONDITION and confirms the loss instead of restoring it.
Add `Session.recovery` with `SessionRecovery.failed_messages`, a list of `SessionMessageReference` task and message identities. When admission reverts a crashed runtime, it records each user input after the retained snapshot boundary, including accepted inputs not yet published, in the same transaction that fails the interrupted turn. The boundary is the newest history event tagged with the snapshot the Actor reverts to. Without a snapshot every input is discarded. If no event records the snapshot, the boundary is unknown and no new annotations are added. One CTE resolves the boundary and reads the discarded messages. Each message's identity is still validated in Go, and a malformed event rejects the admission. Signed-off-by: Ada Lundhe <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
`GetSession` now reports `recovery.needed`. It is true for an admitted recovery, false for a Session with a lifecycle operation in flight, and, for an idle Session, the result of the same `observeCrash` check `RevertSession` admits by. Chat can therefore never offer a revert the API would refuse. An observation costs one fence read and one Substrate call. If the observation fails, needed is left unset rather than failing the read, so an outage cannot hide history. `GetSession` is the only read that observes Substrate. `Get`, which MCP calls before every invoke, serves the stored record without observing. `ListSessions`, rename, revert, and delete omit `recovery`, so a list does not carry every conversation's annotations. `Service.Observe` always has a workflow; transport tests use a stand-in that reports stored annotations instead of a nil workflow. Signed-off-by: Ada Lundhe <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
The API client now maps `GetSession`'s `recovery` (needed and the discarded message references) into the conversation domain type, and adds a `revert` operation that calls `RevertSession`. The mock transport passes recovery through. It refuses a revert with FAILED_PRECONDITION, because fixtures have no crashed runtime that could authorize one. Signed-off-by: Ada Lundhe <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
When `GetSession` reports that a conversation's runtime needs recovery, chat shows an alert offering to revert it to its last snapshot. The existing fork action stays available for a saved checkpoint. An idle conversation re-reads the Session on the existing four-second live refresh while the tab is visible, so a crash after a turn settles is found without keeping the one-second lifecycle poll running. The shared page does the same. Inputs that the restored runtime does not remember are marked "Not remembered after recovery" across their whole turn, and the public transcript is kept. Forking a crashed conversation is left to the checkpoint lines. Signed-off-by: Ada Lundhe <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
TestMCPToolApproval and TestSessionAskUserSurvivesSuspension wait for the Actor to suspend, then send the reply once. Waking the Actor can fail before the runtime accepts the reply. In a gVisor run, the restored runtime's TaskStore dial to the controller timed out, so the gateway revoked the dispatch and answered KAGENT_SEND_NOT_ACCEPTED. The test treated that retryable answer as a failure. Both resumes now go through sendMessageWithRetry, which every other send in the suite already uses. The server returns this error only after proving the input was not persisted, and a repeated message ID returns the accepted task, so the retry cannot duplicate the reply. Verified with go vet on the e2e package; the suite runs in CI. Signed-off-by: Ada Lundhe <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
The base branch now returns the Session from RevertSession when it is READY and its recorded Actor is SUSPENDED, so a retry after a lost revert response succeeds. TestSessionRecoveryObservation still expected KAGENT_SESSION_RUNTIME_NOT_CRASHED for its healthy case, whose Actor is suspended. The healthy case now expects the unchanged Session, and a running case keeps the NOT_CRASHED refusal covered. Every case asserts that only a confirmed crash reaches RevertActor. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
RevertSession now returns the Session for a runtime that is not crashed, so chat no longer needs the NOT_CRASHED re-read. GetSession reports recovery as `optional bool recovery_needed` instead of a SessionRecovery message. The discarded-message annotations (failed_messages) are removed: the admission CTE that recorded them, the transcript marking, the shared page's Session refresh, and withoutRecovery, which existed only because the annotations were stored and grew with each revert. Showing users what a revert lost is a separate follow-up. Revert also stays usable while an earlier attempt's recovery is still admitted. The alert disables the button only while this page's own request is in flight (isReverting), which is separate from isRecovering. This came in with the automatic-recovery commit, which is no longer in this history. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Remove comments added for recovery observation and the chat Revert flow that narrate the code, restate names, repeat assertions, or describe earlier behavior. Keep the ordering, polling, and retry rationale, shortened. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
… guard - The CheckpointDivider fork check fixes forking from non-READY checkpoints, which predates crash recovery, so it is not part of this PR. - The revert double-click ref is unnecessary: a second RevertSession joins the same REVERT or returns the Session. - refreshConversation had one caller, so the live refresh inlines Promise.allSettled. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Substrate cannot resume a crashed Actor, so an A2A send to one fails before the runtime accepts it. The gateway revokes the dispatch and answers KAGENT_SEND_NOT_ACCEPTED, and every retry failed the same way. When the gateway proves a failed send was not accepted, it now runs the Session's Revert before answering. A healthy runtime returns at once. A crashed one is reverted in place, so the client's ordinary retry reaches the restored runtime. A send to a Session with recovery already admitted joins that recovery instead of being refused. The revert uses the authorization the send already checked and continues if the caller disconnects. A missing or replaced Actor answers the send with a non-retryable error rather than the retry signal. No background work runs: recovery starts only from a client request. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Address review feedback on crash recovery: - Drop Session.recovery_needed, Service.Observe, and ActorWorkflow.RecoveryNeeded. No Session read calls Substrate. Chat sends normally and retries KAGENT_SEND_NOT_ACCEPTED with the same message ID; it shows a pending recovery and a lost runtime from the stored Session state. - Record a missing, replaced, or retemplated Actor as FAILED/NONE with failure reason RuntimeLost, in one transaction guarded by the observed operation ID. Open tasks fail and a pending dispatch is cleared. Later sends and reverts fail from PostgreSQL without asking Substrate. - Recover through Service.Revert so a send needs update access to revert, as RevertSession does. - Return Session errors from RecoverRejectedSend instead of mapping them to KAGENT_SEND_NOT_ACCEPTED. - A crash during a turn leaves the turn active, which refuses every send. A send refused for active work now runs Revert once and retries its reservation, so a crashed runtime is restored and a busy healthy one refuses the send as before. - The crash e2e recovers through a send instead of RevertSession. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Add sessions.automaticRecovery, disabled by default. Gate all three send recovery paths while keeping explicit revert, migrations, and persistence unconditional. Enable recovery in the CI fleet and cover disabled refusals and config rendering. Gateway, config, PostgreSQL Session, gRPC, scheduled-run, protobuf, and lint checks pass. Signed-off-by: Ada Lundhe <[email protected]>
…dered Chat offers Recover, which calls RevertSession, when a recovery is pending and when sends stay refused as not accepted after the retry window. It works with automatic recovery off, retries a failed recovery, and records a lost runtime. Re-sends now back off from retryAfterMs to two seconds. Only refusals before the first frame are tagged as not accepted. Keep sessions.automaticRecovery out of the rendered chart values, so a default install stays readable by releases that predate it; helm-unittest guards that. Report a lost Actor as a typed actorLostError instead of a crashObservation field, and share database.LifecycleIdle. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Vitest's default of one worker per core less one oversubscribed an 18-core machine: each worker rendering a page keeps about 1.5 cores busy, so heavy page tests stretched from about one second to 5-9 s and hit the 5 s timeout or the 1 s findBy, a different few each run. Cap workers at eight, and one less than the cores below that, so 4-vCPU CI runners keep their three. Three consecutive full runs pass, 846/846, at load 11-13. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
With automatic recovery off, a runtime that crashes mid-turn leaves that turn open. The next send is refused outright as a conflict while the Session still reads READY, so chat showed only Retry, which cannot succeed. Tag A2A refusals that arrive before the first frame and offer Recover for them, as for sends refused as not accepted. RevertSession leaves a busy, healthy runtime alone. The transcript offers Recover only for a ready conversation; one that is not ready has its own alert. Verified on kind with the flag off: worker deleted mid-turn, the refused send offers Recover, Recover restores the runtime, and the next message is answered. Recover during a healthy running turn leaves it running. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
The gateway now sends reason KAGENT_SESSION_BUSY in the ErrorInfo metadata when an open turn or a checkpoint holds the Session. The refusal keeps its code and message. Chat offers Recover for that reason only, so other A2A refusals keep the plain error with Retry. Move the send-refusal reasons and predicates to ApiError, beside isNotFound, so the transcript depends only on @/api. Keep the push-store comment beside the prop it describes, and document every refusal that offers Recover. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Recover on send only for an open turn. ReserveSessionDispatch now reports one as ErrActiveTurn, which still matches ErrConflict, so a checkpoint in progress no longer triggers a revert. The open-turn refusal's reason is renamed from KAGENT_SESSION_BUSY to KAGENT_TURN_ACTIVE; its code and message are unchanged. All three send paths share one helper. It waits at most 10 seconds for the revert; a cancelled attempt releases its claim, so the client's retry continues the admitted recovery. If the revert cannot run, whether for missing update access, contention, or an outage, the send keeps the answer it gives without recovery. If a recovery is admitted, the send answers ErrRuntimeRecovering. That error keeps the KAGENT_SEND_NOT_ACCEPTED reason, so existing clients still retry, and adds state RECOVERING and a one-second retryAfterMs. Chat reports it when its retry window runs out. Restore the e2e test for an explicit RevertSession on a crashed runtime, beside the send-recovery test, using a shared crash helper. Both kagent subtests pass on kind with automatic recovery on. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Why: when a worker is lost while the Actor suspends after a turn, for example during a node drain or scale-down, the Actor ends CRASHED and the Session still reads READY. The idle worker keeps the suspension's claim because the outcome is unknown, and nothing takes the claim over on a timeout. Every later send waits out the 10-second busy period and is told to retry, so the conversation never recovers, even with automatic recovery on. Only a crash recovery retires the claim. Reservation now reports a held suspension claim as ErrDispatchSuspending, a kind of ErrDispatchBusy. requireSettledRuntime's other callers still see ErrFailedPrecondition. With automatic recovery on, a send whose wait ends on that claim, which a healthy suspension does not reach, attempts the bounded recovery once. It then reserves again without waiting. Clients still get ErrSendNotAccepted, so with the flag off nothing changes. Reproduced on kind before and after: the worker was deleted while the Actor was SUSPENDING. Before the fix, every send waited 10 seconds and was refused, still stuck after six minutes. After it, the first send recovered the Session, including one left stuck from the earlier run, and was answered. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
Stop before a task is named keeps the stream open, so a task the attempt in flight starts can still be cancelled by name. It never aborted the send, so the client's not-accepted retry loop kept re-sending. An accepted re-send then ran the turn the reader had stopped. useChat now passes a per-run stopResending signal that Stop aborts. The client starts no new attempt after it, ends the backoff early, and ends the turn with the last refusal. That refusal proved nothing was accepted, so nothing is left running. A useChat test drives the real client through a transport that refuses every send: without the fix it sent 16 times after Stop, and with it, once. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
By default, sends do not recover a crashed runtime: a crashed turn is refused with KAGENT_TURN_ACTIVE, and a send to a pending recovery is refused outright. Say to call RevertSession in those cases, describe recovery on send under the flag, and document state RECOVERING with its retry delay. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Ada Lundhe <[email protected]>
| logging.FromContext(ctx).ErrorContext(ctx, "release runtime dispatch", "session_id", message.ContextID, "error", err) | ||
| } else if revoked && sendErr != nil && a2atype.ErrorReason(sendErr) == a2atype.ErrorReason(a2atype.ErrInternalError) { | ||
| return sessionsvc.ErrSendNotAccepted | ||
| return g.interactions.RecoverRejectedSend(ctx, agent, message) |
There was a problem hiding this comment.
I think this is a good start, and i think it's ok to stop here, but in the future I wonder if we can do this without ever even retrying, just holding the request and then returning the task.
Builds on #3395, now merged; rebased onto main.
Recover crashed runtimes during A2A sends when
config.sessions.automaticRecovery: true. The setting defaults tofalse. The chart only mentions it in a comment, so a default install renders the samecontroller.yamlas before and a rollback to v1.0.0 can still read it.The flag controls joining an admitted recovery, recovering a send refused for an open turn, recovering a send blocked by a suspension whose worker was lost (the Actor crashed during the post-turn suspend), and reverting after a proven unaccepted send. A send waits at most 10 seconds for recovery; a recovery still running is answered
KAGENT_SEND_NOT_ACCEPTEDwithstate: RECOVERING, and the client's retry continues it. If recovery can't run, the send keeps the answer it gives without recovery. With the flag off, sends keep their existing refusals. ExplicitRevertSessioncalls remain available.Missing or replaced runtimes are recorded as FAILED/NONE with reason
RuntimeLost. These persistence changes apply with either flag value. The flag does not control migrations, schema, or stored data.Chat reads recovery status from stored Session state. It retries
KAGENT_SEND_NOT_ACCEPTEDbefore the first stream frame, with the same message ID. The wait doubles fromretryAfterMsup to 2 seconds, within a 30-second window. Chat offers Recover, which callsRevertSession, in three cases: when the window runs out; when a send is refused for an open turn (KAGENT_TURN_ACTIVE), which is what a mid-turn crash produces with the flag off while the Session still reads READY; and while a recovery is pending. A recovery whose attempt failed stays FAILED/REVERT until a client retries it. Recover returns a healthy runtime unchanged and records a lost one. Session reads do not query Substrate.Validation:
min(cores - 1, 8)); the default oversubscribed large machines and made heavy page tests time out. Three consecutive full runs pass, 846/846.🤖 Generated with Claude Code