[adalundhe]

Summary

Expose SessionService.RevertSession through the public API and generated clients.

The service requires Session update access and uses the existing workflow. RPC tests cover ownership, read-only shares, identity checks, durable results and retry outcomes. The worker-loss E2E exercises explicit recovery and a subsequent completed message.

Stack

Split requested in the review discussion.

Part 3 of 6. 15 files, +753/-111 against the immediate parent.

Part	Scope	Review
1	add durable recovery primitives	PR
2	implement manual crashed-runtime recovery	PR
3	expose manual crashed-session revert	PR
4	report recovery and preserve discarded message references	PR
5	offer manual recovery for crashed conversations	PR
6	add opt-in automatic crash recovery	PR

Parts 2–6 form a GitHub stack on main, so CI runs on each part. A stack cannot include a fork branch, so part 2 targets main and also shows part 1's commit until #3255 merges. Parts 3–6 target their immediate parents for a focused diff. The stack is rebased onto main after #3409, which removed the public Session suspend and resume controls.

Testing

On main after #3409, RevertSession is the only Session RPC this part adds. Protobuf lint and official generation pass. The worker-loss E2E holds a turn open to keep the Actor running, because only an interaction runs it now; E2E compiles. Session, gRPC and MCP race suites pass at the top of the stack, except TestSandboxTemplateCatalog, which needs envtest binaries.

Live cluster E2E runs in this PR's CI.

Upstream CI runs on this PR as part of the stack on main.

🤖 Generated with Claude Code

[EItanya]

🤖 AI-generated review.

Reviewed with #3409 landing first and this stack rebased on top.

[EItanya]

Once #3409 lands first, SuspendSession, ResumeSession, and their request messages are gone, so this E2E package no longer compiles. Can we keep the automatic snapshot established above and wake the Actor through a normal A2A interaction before inducing worker loss? That exercises the lifecycle available after the rebase while preserving the healthy-runtime rejection, Actor identity, history, and post-revert interaction checks.

[adalundhe]

Done in the rebase onto #3409 (08019da1). The E2E no longer calls Suspend or Resume. It keeps the automatic snapshot and holds a second A2A turn open at the model, so the Actor is running when its worker is deleted. It also keeps the checks for healthy-runtime rejection, Actor UID, retained history and the follow-up turn. f0251704 additionally checks the refusal reason on the healthy revert, and on a revert retried after recovery. On a kind cluster with gVisor workers, it passes for all four harnesses:

--- PASS: TestSessionRevertCrashedRuntime (89.75s)
    --- PASS: TestSessionRevertCrashedRuntime/kagent (23.78s)
    --- PASS: TestSessionRevertCrashedRuntime/codex (26.63s)
    --- PASS: TestSessionRevertCrashedRuntime/claude (21.71s)
    --- PASS: TestSessionRevertCrashedRuntime/byo-adk (17.64s)

e2e-summary.txt

[jjamroga]

The RPC itself makes sense to me. After #3409 there's no other way to say "restore a confirmed-crashed runtime," and an explicit action beats overloading an existing one. Auth, validation, the handler and generated code all look good (make proto-generate is clean). My questions are mostly about status codes, since docs/lifecycle-retries.md turns them into client retry behavior.

[jjamroga]

Telling "already recovered" apart from "can't recover": if the UNAVAILABLE → FAILED_PRECONDITION change on service.go:298 lands, FAILED_PRECONDITION means two different things here: "the Actor is healthy" (including a retry after a revert whose response got lost) and "the Actor is gone/replaced, nothing to recover." The first is basically success for the client, the second isn't. Should this say to check GetSession (READY with no Failure = done), or would distinct messages/details be better?

[adalundhe]

I went with distinct details. Checking GetSession cannot separate the two cases: a Session whose Actor was replaced is also READY with no Failure. Each refusal in f0251704 now carries a google.rpc.ErrorInfo in domain kagent.dev. KAGENT_SESSION_RUNTIME_NOT_CRASHED covers a healthy or suspended runtime, including a retry after a lost success response. KAGENT_SESSION_RUNTIME_UNRECOVERABLE covers a missing or replaced Actor. docs/lifecycle-retries.md gives the client action for each, so clients need not parse the message. On the cluster, a revert retried after a successful recovery returns:

step=crashed-actor        code=OK state=RUNTIME_STATE_READY
step=retry-after-recovery code=FailedPrecondition reasons=[kagent.dev/KAGENT_SESSION_RUNTIME_NOT_CRASHED]

after/revert-outcomes.txt

[jjamroga]

Small one: ActorWorkflow.Revert returns ErrConflict for anything that isn't READY/NONE (recovery.go:61-63), which lands here as ABORTED. That includes a Session suspended by scheduled-run deadline cleanup, where nothing is in progress, so retrying never converges. FAILED_PRECONDITION seems like a better fit there, keeping ABORTED for real contention.

[adalundhe]

Good catch, fixed in f0251704. Revert now returns ABORTED only while a lifecycle operation is active, which a retry outlasts. A Session at rest outside READY returns FAILED_PRECONDITION with KAGENT_SESSION_RUNTIME_NOT_CRASHED. That includes one suspended by deadline cleanup, which waits for an interaction to resume it. TestExplicitRevertSeparatesContentionFromRestingState pins a SUSPENDED Session at rest as a precondition failure, and suspension, resume and creation in progress as conflicts:

--- PASS: TestExplicitRevertSeparatesContentionFromRestingState/suspended_by_deadline_cleanup
--- PASS: TestExplicitRevertSeparatesContentionFromRestingState/suspension_in_progress
--- PASS: TestExplicitRevertSeparatesContentionFromRestingState/resume_in_progress
--- PASS: TestExplicitRevertSeparatesContentionFromRestingState/creation_in_progress

unit-tests.txt

[jjamroga]

+1, and the tests now lock this in: session_recovery_test.go:151 and :155 expect Unavailable for a same-name replacement and a missing Actor. The architecture doc says these "cannot authorize a revert" (runtime-and-lifecycle.md:212), but the errors table tells clients to retry UNAVAILABLE, so a client following the docs just retries until its deadline.

Should the READY branch in ActorWorkflow.Revert wrap these with ErrFailedPrecondition? Background retries go through the FAILED/RESUME branch, so they're unaffected, and transient Substrate/DB errors still map to UNAVAILABLE. I tried it locally (+11/−1 in recovery.go):

actor, err := w.actors.GetActor(ctx, revision.ActorTemplateAtespace, substrate.ActorName(session.Id))
if status.Code(err) == codes.NotFound {
	return nil, fmt.Errorf("runtime actor is missing: %w", database.ErrFailedPrecondition)
}
if err != nil {
	return nil, err
}
if !validActorIdentity(actor, revision, substrate.ActorName(session.Id)) {
	return nil, fmt.Errorf("runtime actor identity or template changed: %w", database.ErrFailedPrecondition)
}
if err := w.verifyActor(ctx, session, revision, actor); errors.Is(err, database.ErrNotFound) {
	return nil, fmt.Errorf("runtime actor was replaced: %w", database.ErrFailedPrecondition)
} else if err != nil {
	return nil, err
}

With the two assertions flipped to FailedPrecondition, TestRevertSessionThroughGRPC (all 7 cases) and the session service tests pass.

[adalundhe]

Folded into #3395, which now carries the RevertSession RPC, its access policy and refusal reasons, and the e2e test together with the recovery workflow they use. The rest of the stack (#3397, #3398, #3399) is being rebuilt on top of #3395.