Repository navigation
Conversation
github-actions
Bot
added
bug
fd9d57d to
f405ce2
Compare
f405ce2 to
55c766f
Compare
github-actions
Bot
added
bug
Signed-off-by: Ada Lundhe <adalundhe@lundhe.audio>
55c766f to
344ddf3
Compare
github-actions
Bot
added
bug
jjamroga
left a comment
jjamroga
left a comment
There was a problem hiding this comment.
Putting recordDiscardedSessionMessages in the admission transaction makes sense, and the boundary/dedupe tests look solid. Most of my questions are about where the Substrate observation runs.
| session = proto.CloneOf(session) | ||
| session.Recovery, err = s.workflow.Recovery(ctx, session) | ||
| if err != nil { | ||
| logging.FromContext(ctx).WarnContext(ctx, "observe session recovery", "session_id", session.Id, "error", err) | ||
| } | ||
| return session, nil |
There was a problem hiding this comment.
Should the recovery observation only run for GetSession?
Service.Get now calls workflow.Recovery, which reads the runtime revision and calls GetActor with a 5s timeout. But Service.Get is also how MCP resolves the tenant before every invoke and task get/cancel/update (mcp/server.go:153, mcp/tasks.go:227). Those callers throw Recovery away but still pay for a Substrate round trip, so a slow control plane adds up to 5s to every MCP call. #3398 and #3399 keep the same wiring.
I tried splitting it:
- Leave
GetasgetAuthorized. - Add
Service.Observewith the current body ofGet. - Only call
ObservefromsessionServer.GetSession.
It also lets you drop the test-only readOnlySessionWorkflow and fakeSessionWorkflow.Recovery stubs this PR adds. Would that work for you?
Tested locally on 344ddf3: with fakeSessionWorkflow.Recovery changed to panic, the mcp tests blow up in TestTaskUpdateContinuesA2ATask before the split and pass after. The session service tests (with TestSessionGetPreservesHistoryWhenRecoveryObservationFails switched to Observe) and TestRevertSessionThroughGRPC pass too.
| revision, err := w.store.GetRuntimeRevision(ctx, session.PreparedRevision) | ||
| if err != nil { | ||
| return recovery, fmt.Errorf("load Session recovery revision: %w", err) | ||
| } | ||
| actor, err := w.actors.GetActor(ctx, revision.ActorTemplateAtespace, substrate.ActorName(session.Id)) | ||
| if err != nil { | ||
| return recovery, fmt.Errorf("observe Session recovery Actor: %w", err) | ||
| } | ||
| if err := w.verifyActor(ctx, session, revision, actor); err != nil { | ||
| return recovery, err | ||
| } | ||
| recovery.Needed = new(actor.GetStatus().GetState() == ateapipb.ActorState_ACTOR_STATE_CRASHED) |
There was a problem hiding this comment.
Can Recovery and Revert share the crash check?
These lines repeat Revert's sequence further down in this file (L97-117): GetRuntimeRevision, GetActor, verifyActor, check ACTOR_STATE_CRASHED. I think one helper should own that so what we offer and what we actually do can't drift apart.
| WITH retained AS ( | ||
| SELECT COALESCE(MAX(sequence), 0) AS sequence FROM session_task_event | ||
| WHERE history_id = $1 AND $2::text <> '' AND snapshot_uri = $2 | ||
| AND snapshot_fidelity = $3 AND snapshot_atespace = $4 | ||
| ) |
There was a problem hiding this comment.
What if the restored snapshot isn't in this history?
retained falls back to sequence 0 when no event has the Actor's latest snapshot URI, so every user input gets marked failed. That's right when there's no snapshot, but can the URI be set and still untagged? Two cases I'm thinking of:
- A quiesce where the Substrate call succeeded but
FinishSessionQuiescencenever committed. - A fork whose Actor reports a different URI than the checkpoint tag on the copied boundary event.
Either way, marking the whole transcript failed seems worse than failing admission or leaving it unknown. Thoughts? (Didn't repro these two cases, this is from reading the code.)
| Recovery records exact task and message identities for user inputs after the | ||
| retained snapshot boundary, including accepted inputs not yet published. Chat | ||
| marks those inputs as failed to send and retains the public transcript; completed | ||
| A2A tasks and their history are not rewound. The annotations remain on the |
There was a problem hiding this comment.
"Failed to send" is too generic. This one is on me, I think I recommended this wording verbatim. But if a task completed after the snapshot, the transcript still shows the agent's reply, so it's not even accurate since the message did send. What we actually mean is the runtime lost it, so something like "the runtime doesn't remember this turn" seems clearer. (Same wording is in docs/lifecycle-retries.md:54.)
| optional bool needed = 1; | ||
| // User messages whose runtime state a revert discarded. Their A2A history | ||
| // remains available, including completed task results. | ||
| repeated SessionMessageReference failed_messages = 3; |
There was a problem hiding this comment.
failed_messages lives in session.data and comes back from ListSessions. Is it intentional that it keeps growing for the life of the Session?
| // Unset when the recorded runtime's recovery state could not be observed. | ||
| optional bool needed = 1; |
There was a problem hiding this comment.
Nit: this says unset means "could not be observed," but Create, List, UpdateName and Revert responses leave it unset too. Can it say only GetSession populates it?
Summary
Report read-only recovery observations in
GetSessionand persist exact task/message references for inputs a revert discarded.Actor observation failures leave recovery state unknown while history remains readable. Message annotations commit atomically with recovery admission and settlement. Completed A2A tasks and their public history remain intact.
Stack
Split requested in the review discussion.
Part 4 of 6. 17 files, +863/-162 against the immediate parent.
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 targetsmainand also shows part 1's commit until #3255 merges. Parts 3–6 target their immediate parents for a focused diff. The stack is rebased ontomainafter #3409, which removed the public Session suspend and resume controls.Testing
On
mainafter #3409, the fulldatabaseand Session race suites pass against PostgreSQL at the top of the stack, and the part builds and vets on its own. Tests cover snapshot boundaries, repeated recoveries, duplicate message IDs across tasks, malformed history, rollback and public RPC observations. Protobuf lint and official generation pass.Upstream CI runs on this PR as part of the stack on
main.🤖 Generated with Claude Code