Fix GAgent durable terminal completion#657
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 274df47079
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (snapshot == null && !string.IsNullOrWhiteSpace(receipt.SessionId)) | ||
| snapshot = await _queryPort.GetBySessionIdAsync(receipt.ActorId, receipt.SessionId, ct); |
There was a problem hiding this comment.
When correlation lookup misses, this resolver accepts the first snapshot for actorId + sessionId without checking that the returned snapshot belongs to the current receipt correlation. If the same session has an older terminal document (common when users reuse a session/thread), a transient miss on the new correlation document will cause this command to be marked terminal from stale data. This can produce incorrect completion status for the wrong run; the same fallback pattern is also present in GAgentApprovalDurableCompletionResolver.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aee3b13. The durable resolvers now only accept correlation or session fallback snapshots when the snapshot matches the active receipt actor/correlation and the expected interaction kind (draft-run vs approval), so reused sessions cannot resolve from stale terminal documents. Added unit coverage for stale correlation, wrong interaction kind, and valid fallback for both draft-run and approval.
Only accept session fallback snapshots when they match the active receipt correlation and interaction kind, preventing reused sessions from resolving with stale terminal state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| LastEventId = eventId, | ||
| SessionId = completed.SessionId, | ||
| CorrelationId = correlationId, | ||
| InteractionKind = (int)ResolveInteractionKind(completed.SessionId, correlationId), |
There was a problem hiding this comment.
[Consensus: 3 models] severity=major, category=bug
ResolveInteractionKind infers Approval whenever sessionId != correlationId, but GAgentDraftRunCommandTargetBinder.ResolveSessionId returns command.SessionId.Trim() when the caller supplies one — so any draft-run with an explicit SessionId distinct from CorrelationId will be written into the readmodel as Approval. This makes InteractionKind unreliable for any downstream consumer (filtering, dashboards, future typed terminal-event mapping).
Fix: carry the interaction kind through the projection scope/context (e.g. add InteractionKind to GAgentRunTerminalProjectionContext from the activation site at GAgentDraftRunInteraction.cs / GAgentApprovalInteraction.cs) and read it from the context here, instead of inferring from session/correlation equality.
Per-model verbatim
- codex:
ResolveInteractionKindinfers approval wheneverSessionId != CorrelationId, but draft-run supports an explicitSessionId, so those draft-run terminal snapshots will be mislabeled asApproval. Carry the interaction kind through the activation context/port or store separate projection kinds instead of deriving it from the session/correlation equality. - gemini: The
ResolveInteractionKindlogic assumesDraftRunalways hasSessionId == CorrelationId. If a user provides a customSessionIdin theGAgentDraftRunCommand, this will incorrectly label the interaction asApproval. While non-critical for completion logic, it makes theInteractionKindfield in the read model potentially misleading. - kimi:
ResolveInteractionKinduses the heuristicsessionId == correlationIdto distinguish DraftRun from Approval. A draft-run with an explicitSessionId(different fromCorrelationId) would be misclassified as Approval. Consider documenting this assumption or deriving the kind from explicit event metadata when a typed terminal event is introduced.
There was a problem hiding this comment.
Fixed in 16eccf9. The terminal projection now carries interaction kind explicitly through the activation context using separate projection kinds for draft-run and approval, and the projector writes InteractionKind from context instead of deriving it from session/correlation equality. Added regression coverage for draft-run with explicit SessionId != CorrelationId.
| CancellationToken ct = default); | ||
| } | ||
|
|
||
| public interface IGAgentRunTerminalProjectionPort |
There was a problem hiding this comment.
[Consensus: 2 models] severity=major, category=arch
IGAgentRunTerminalProjectionPort only exposes EnsureProjectionAsync (correlation-scoped) with no corresponding ReleaseProjectionAsync / lease return. Each draft-run / approval command currently activates a new correlation-keyed durable materialization scope (GAgentApprovalInteraction.cs:181, GAgentDraftRunInteraction.cs:236) and never releases it. Since correlation is per-command, these scopes accumulate until Orleans deactivation — defeating the point of the new IProjectionSessionScopedMaterializationContext release-aware design described in the PR summary.
Fix: return a lease (analogous to IGAgentDraftRunProjectionLease) from EnsureProjectionAsync, and have the interaction cleanup path (GAgentApprovalCommandTarget.ReleaseAsync / draft-run equivalent) release it. Alternatively, make this an actor-scoped (not correlation-scoped) materialization.
Per-model verbatim
- codex (GAgentRunTerminalProjectionPort.cs:33): This activates a durable materialization scope keyed by correlation id, then discards the lease and exposes no release path. Because draft-run/approval create a new correlation per command, these projection actors/subscriptions can accumulate without cleanup; return a terminal projection lease from the port and release it from the interaction cleanup path, or make this an actor-scoped materialization instead of a per-correlation scope.
- gemini (GAgentRunTerminalModels.cs:43): The interface is missing a corresponding
ReleaseProjectionAsyncorReleaseIfIdleAsyncmethod. Given the PR summary explicitly mentions making durable materialization 'release session-aware', the port should expose this capability to allow the interaction service to clean up command-scoped materializers. - gemini (GAgentApprovalInteraction.cs:157): The terminal projection is ensured here but never released. Since these materialization actors are now partitioned by
CorrelationId(one per command), they will leak until Orleans deactivates them. TheGAgentApprovalCommandTargetshould hold a reference to theIGAgentRunTerminalProjectionPortand call a (yet to be exposed) release method in itsReleaseAsyncimplementation, similar to how the live_projectionPortis handled.
There was a problem hiding this comment.
Fixed in 16eccf9. IGAgentRunTerminalProjectionPort now returns an IGAgentRunTerminalProjectionLease and exposes ReleaseProjectionAsync. Draft-run and approval targets retain the terminal lease and release it during cleanup, including activation-failure cleanup after terminal materialization succeeds.
| { | ||
| var snapshot = await _queryPort.GetByCorrelationIdAsync(receipt.ActorId, receipt.CorrelationId, ct); | ||
| if (snapshot == null && !string.IsNullOrWhiteSpace(receipt.SessionId)) | ||
| snapshot = await _queryPort.GetBySessionIdAsync(receipt.ActorId, receipt.SessionId, ct); |
There was a problem hiding this comment.
[codex] severity=major, category=bug
The if (snapshot == null && !string.IsNullOrWhiteSpace(receipt.SessionId)) fallback accepts whatever GetBySessionIdAsync returns without checking that the returned snapshot's CorrelationId matches receipt.CorrelationId. If a session/thread is reused (which is the whole point of command.SessionId) and the correlation document is temporarily missing or evicted, this can complete the current command from an older terminal fact — silently delivering stale completion status.
Same pattern exists in GAgentApprovalDurableCompletionResolver (GAgentApprovalInteraction.cs:343-345).
Fix: only accept the session-fallback snapshot when its CorrelationId == receipt.CorrelationId, otherwise return Incomplete:
if (snapshot != null &&
!string.Equals(snapshot.CorrelationId, receipt.CorrelationId, StringComparison.Ordinal))
snapshot = null;Apply in both resolvers.
There was a problem hiding this comment.
Fixed in aee3b13. The fallback path now validates actor, active correlation, and expected interaction kind before accepting a session lookup result, so reused sessions cannot resolve the current command from stale terminal documents.
| @@ -151,6 +159,12 @@ public async Task HandleToolApprovalDecision(ToolApprovalDecisionEvent evt) | |||
| else | |||
There was a problem hiding this comment.
[kimi] severity=major, category=bug
The approval-denied else branch calls PersistApprovalTerminalFailureAsync without a try / catch. If this throws after ClearPendingApprovalEvent is already persisted, the actor event fails and retries; on retry State.PendingApproval is already null so the method returns early, permanently losing the terminal failure fact — exactly what the durable readmodel needs.
The same gap exists in two more call sites in this file:
HandleToolApprovalTimeoutno-remote-handler branch (around line 195)HandleToolApprovalTimeoutremote-failed/denied/timed-out branch (around line 249)
Only the exception path at lines 143-150 wraps it in try { … } catch { /* best effort */ }.
Fix: either wrap each call site in best-effort try/catch (matching the pattern at lines 143-150), or move the try/catch inside PersistApprovalTerminalFailureAsync itself so every caller is safe.
There was a problem hiding this comment.
Fixed in 16eccf9. Approval denied, no-remote timeout, and remote denied/failed/timed-out paths now persist the terminal failure fact before clearing PendingApproval, preserving the durable completion source if terminal persistence fails.
| @@ -0,0 +1,227 @@ | |||
| using Aevatar.AI.Abstractions; | |||
There was a problem hiding this comment.
[mimo-v2.5-pro] severity=major, category=test
Missing test for monotonic version guard. CLAUDE.md projection rules require: "旧 StateVersion 不覆盖新 readmodel". Add a test that sends two committed events with the same actorId + correlationId but a descending stateVersion, then asserts the readmodel retains the higher version (or that the projector / IProjectionWriteDispatcher rejects the stale write).
Without this test, a future change to _writeDispatcher.UpsertAsync semantics (or a parallel write path) could regress the monotonic guarantee without anyone noticing — the rest of the test suite only exercises forward-version cases.
There was a problem hiding this comment.
Fixed in 16eccf9. Added a monotonic StateVersion regression test for GAgentRunTerminalProjector using the projection write evaluator semantics so an older committed event cannot overwrite a newer terminal readmodel.
| try { await PersistDomainEventAsync(new ClearPendingApprovalEvent { RequestId = pending.RequestId }); } | ||
| catch { /* best effort */ } | ||
| try | ||
| { |
There was a problem hiding this comment.
[gemini] severity=nit, category=other
This try { … } catch { /* best effort */ } swallows persistence failures silently. If PersistApprovalTerminalFailureAsync fails here, the durable resolver will stay Incomplete indefinitely with no operator signal. Consider logging at warning level inside the catch (include pending.SessionId, pending.RequestId, the reason code, and ex.Message) so this is observable.
Same applies to the symmetric catch the kimi comment proposes adding at the denied / timeout sites — when those wraps are added, give them logging too.
There was a problem hiding this comment.
Fixed in 16eccf9. The approval continuation failure path now logs terminal failure persistence errors with request id, session id, and reason code, and logs pending-clear failures after the terminal fact has been persisted.
Carry terminal interaction kind explicitly through materialization, release correlation-scoped leases, and persist approval terminal failures before clearing pending state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the review feedback in 16eccf9. Summary:
Verification:
Note: full solution build is still blocked by existing Aevatar.Studio.Hosting missing AI abstraction references, unrelated to these changes. |
Fix-check follow-up (multi-model)Re-ran the 6-model review against the post-fix HEAD. All 7 inline comments are majority-resolved (5/6 or 6/6) — but three threads have credible dissenting verdicts worth a look before this merges.
Dissents#3 — Terminal projection lease is never released on success (codex; verified) The fix adds
But Possible directions:
#1 & #2 — Denied/timeout branches use the non- Literally accurate, but the author chose an ordering-based fix ( Reviewer panelAll 6 (deepseek-v4-pro, glm-5.1, mimo-v2.5-pro, kimi, gemini, codex) returned 7/7 valid verdicts this round. No timeouts. |
|
I checked the #3 dissent. I think this is already covered by the interaction lifecycle, so I do not think another code change is needed here.
So I read |
| { | ||
| try | ||
| { | ||
| await _terminalProjectionPort.ReleaseProjectionAsync(terminalProjectionLease, ct); |
There was a problem hiding this comment.
After a draft run enters pending approval, the live stream has already emitted TextMessageEnd, so interaction cleanup treats this interaction as observedCompleted=true and releases the TerminalProjectionLease. However, the RoleChatSessionCompletedEvent failure fact for approval local timeout, remote denied, or remote timeout is a later actor event and can be committed after the projection scope has already been Released. At that point, ProjectionScopeGAgentBase.HandleObservedEnvelopeAsync returns immediately, so GAgentRunTerminalProjector never materializes a GAgentRunTerminalReadModel. The actor has committed the terminal failure fact, but the durable read model layer cannot observe it, and recovery after a missed live terminal event can read an incomplete or stale state.
| legacyMessage); | ||
| } | ||
|
|
||
| if (content.StartsWith(LegacyLlmFailedPrefix, StringComparison.Ordinal)) |
There was a problem hiding this comment.
The normal provider / LLM exception path in RoleGAgent writes LLM request failed [tools=...]: ... when useWorkflowFailureMarker=false, but GAgentRunTerminalProjector.ResolveTerminal only recognizes [[AEVATAR_LLM_ERROR]] and the exact prefix LLM request failed:. Failure content that includes [tools=...] falls through to the default branch and is materialized as TextMessageCompleted, so the durable resolver later recovers a real failure as completed. This path persists provider / LLM exceptions as successful terminal states.
3 participants
Summary
Fixes #370 by replacing the placeholder durable completion resolvers for GAgent draft-run and approval interactions with a committed-fact -> projection readmodel -> query-port path.
This PR adds a GAgent run terminal readmodel and durable materialization scope that observes committed
RoleChatSessionCompletedEventfacts, then maps the resulting terminal snapshot back into draft-run / approval durable completion. It also fixes the approval live observation session key soCommandId != CorrelationIdstill receives the continuation terminal event.What changed
GAgentRunTerminalSnapshot,GAgentRunTerminalStatus,GAgentRunTerminalInteractionKind,IGAgentRunTerminalQueryPort, andIGAgentRunTerminalProjectionPort.GAgentRunTerminalReadModel, metadata provider, projector, query reader, and durable projection activation port.actorId + correlationId, then fallback toactorId + sessionIdwhere applicable.SessionIdfor fallback lookup.IProjectionSessionScopedMaterializationContext, so session/correlation-scoped terminal materialization can be released with the same key used for activation.docs/history/2026-05/documenting the architecture and follow-up decisions.Explicitly deferred
Two review points are intentionally not included in this PR:
Replacing legacy approval / LLM failure markers with a typed terminal event
RoleChatSessionCompletedEvent.Contentmarkers as a legacy fallback only.status/reason_code/reason_messageproto fields would change the committed event contract and should be designed/migrated separately.Changing
ReasonCodefrom string to enumReasonCodeis currently used for durable readmodel reason recording and completion mapping, not as a stable cross-module control-flow or filtering contract.Validation
Previously run on this branch:
Also started the GAgent service locally with in-memory projection/document/graph providers and auth disabled, then exercised related scope service endpoints. The RoleGAgent draft-run failure path committed
RoleChatSessionCompletedEventand wroteGAgentRunTerminalReadModelsuccessfully.Notes
Full build succeeds with existing warnings from NuGet source configuration, vulnerability advisories, obsolete APIs, complexity, and nullability warnings.