Conversation
A hard reset of an idle-but-live workflow worker created a fresh AgentSession in SessionManager while TaskAgentManager kept the old object: workflow provisioning treated idle executions as dormant ghosts, so the fresh object never received runtime MCP servers and mailbox delivery reported the existing session as not found. SessionManager now hands the exact fresh object to a dedicated reset provisioning path before publishing the reset event, and workflow provisioning admits reset replacements for idle, waiting_rebind, resumable, and post-approval workers with full runtime MCP attach. Ownership generations plus identity-conditional register/unregister fence teardown races, and MCP self-heal refuses noncanonical instances. Ordinary idle ghost lookups stay dormant.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 9c76f0ffd4
ℹ️ 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".
| const replacesLiveSession = | ||
| options.intent === 'reset-replacement' && | ||
| indexed !== undefined && | ||
| indexed !== session && | ||
| this.isAgentSessionAlive(indexed); |
There was a problem hiding this comment.
When SessionManager still caches a live workflow session but TaskAgentManager.agentSessionIndex has lost its entry—a state already supported by isSessionAlive—a hard reset passes the fresh object with intent: 'reset-replacement', but indexed === undefined makes replacesLiveSession false. For an idle or waiting_rebind execution, admission consequently takes the non-resumable skip path, while SessionManager still assumes the provider owns replay and cleans up the old object. The fresh session is therefore left unindexed and without node-agent, reproducing the delivery failure this change is intended to repair.
Useful? React with 👍 / 👎.
| await this.spaceRuntimeMcpProvider.provisionResetWorkflowSession(freshSession, { | ||
| startQuery: options.restartQuery, | ||
| replayPendingMessages: options.restartQuery, | ||
| }); | ||
| replayOwnedBySubscriber = true; |
There was a problem hiding this comment.
When a concurrent message lookup calls getSessionAsync(..., { startQuery: false, replayPendingMessages: false }) after the cache swap, this direct provider call is not represented in workflowMcpProvisioning, so both provisioning paths can run. If the non-starting lookup reaches rehydrateInFlight first, the reset path merely awaits and returns that result without applying its stronger start/replay options; this code nevertheless sets replayOwnedBySubscriber to true, suppressing fallback replay. The hard reset can then report success while its pending messages remain unreplayed and its query is not restarted.
Useful? React with 👍 / 👎.
| if ( | ||
| this.agentSessionIndex.get(sessionId) !== agentSession || | ||
| this.config.sessionManager.getCachedSession(sessionId) !== agentSession | ||
| ) { |
There was a problem hiding this comment.
When SessionManager.getCachedSession(sessionId) already returns target but agentSessionIndex has lost the same session, this condition labels the cache-owned object noncanonical and returns without healing it. Cached-but-unindexed workers are explicitly treated as live elsewhere, and this callback is their recovery path when query startup detects a missing node-agent; the missing server therefore remains absent and startup fails its MCP invariant. Reject an object only when a registry points to a different instance, and restore the task bookkeeping when the cache already owns target.
Useful? React with 👍 / 👎.
![hyperneo-ai-test[bot]](https://avatars.githubusercontent.com/in/4875581?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: CONCERNS — 1 blocking finding(s) (0 inline, 1 in the review body: line not in diff).
Engine claude (glm-5.3-flash) · Reviewed commit 9c76f0f · Trigger: open
The core mechanism is sound: identity-conditional register/unregister (CAS), per-session ownership generations, sync recheck-then-commit with no intervening await, and displaced-object-scoped teardown correctly close the stale-object race the PR targets, and the removed SessionManager-side generic replay for workflow sessions is deliberately superseded by the pre-event provider path. However, removing the workflow-sub-session early return from SpaceRuntimeService.reprovisionResetSession introduces a regression for post-approval workers: resolveSpaceMcpSessionPolicy classifies :post-approval: ids as ad_hoc_member (no node-execution row, no :exec: marker), so hard resets of post-approval workers now fall through to attachSpaceToolsToMemberSession — attaching member-scoped MCP servers (db-query, ad-hoc space-actions) to a workflow worker and, worse, triggering a second intent-less provisioning via getSessionAsync whose post-approval admission can call startStreamingQuery() even when the reset was requested with restartQuery: false. Also, the new sessionOwnershipGenerations map only ever grows.
dropped/sanitized output
- finding packages/daemon/src/lib/space/runtime/space-runtime-service.ts:1427 moved to the review body — no anchorable diff line
Blocking findings (line not in diff — posted here instead of inline):
- P1
packages/daemon/src/lib/space/runtime/space-runtime-service.ts:1427Post-approval reset sessions now fall through to ad-hoc member attachment and can auto-start after restartQuery:false resets (line not in diff — posted here instead of inline)
The PR deletes the workflow-sub-session early return from reprovisionResetSession. resolveSpaceMcpSessionPolicy only recognizes :exec: workers as workflow_worker (via nodeExecutionRepo lookup or the ':exec:' id marker); post-approval sessions (space❌task:y:post-approval:*, which have no NodeExecution row — identity is the task's postApprovalSessionId pointer, and applyPostApprovalGate exists precisely because their execution is null) resolve as ad_hoc_member with attachGenericSpaceTools: true. The old branch early-returned for them; now every hard reset of a post-approval worker runs attachSpaceToolsToMemberSession(session, {replayPendingMessages: restartQuery}) after provisionResetWorkflowSession already provisioned it. Consequences traced end-to-end: (1) member-scoped MCPs (db-query, space-actions with role 'ad_hoc_member') are merged into a workflow post-approval worker, defeating the workflow_worker policy (attachGenericSpaceTools: false); (2) attachSpaceToolsToMemberSession calls sessionManager.getSessionAsync(session.id) with default options, so provisionWorkflowMcpServers(session, {}) runs — the reset path bypasses workflowMcpProvisioned/workflowQueryStarted tracking, so a full intent-less provisionWorkflowSession lookup re-runs; the post-approval branch hits restorePostApprovalWorkerSession's already-indexed branch whereoptions.startQuery !== falseis true for undefined and applyPostApprovalGate admits (task approved), so startStreamingQuery() fires — meaning resetQuery({restartQuery: false}) (reachable via the session.resetQuery RPC) spontaneously starts the worker right after the reset deliberately cancelled/failed its pending deliveries; with restartQuery: true a duplicate provisioning + extra replay pass runs. Fix: early-return for isWorkflowSubSessionIdentity(session.id) in reprovisionResetSession (SessionManager now owns their provisioning via provisionResetWorkflowSession), or teach resolveWorkflowExecution/the policy to recognize recorded postApprovalSessionId sessions as workflow workers. No added test covers the subscriber path for post-approval resets.
Minor (P2, not blocking):
- P2
packages/daemon/src/lib/space/runtime/task-agent-manager.ts:1626sessionOwnershipGenerations map grows without bound
supersedeSessionOwnership inserts (and increments) an entry in sessionOwnershipGenerations for every session that is cancelled, stopped, restarted, rate-limit-respawned, or alive during cleanupAll/shutdown, and no code path ever deletes from the map. On a long-lived daemon this leaks one map entry per touched workflow sub-session forever. Suggested fix: delete the entry when the corresponding session is unregistered/detached (e.g., in detachSessionBookkeeping or after a successful unregisterSession), or stamp the generation on the session object instead of an ever-growing side map.
1 participant
Cause
A hard reset of an idle-but-live workflow worker created a fresh
AgentSessioninSessionManager, but the Space runtime kept the old object. Workflow provisioning treated the idle execution like an ordinary dormant ghost and refused to provision the fresh object, so:TaskAgentManagerkept referencing the cleaned-up old object whileSessionManagercached the fresh onenode-agent, etc.)Deterministic pre-fix reproduction confirmed the full chain locally.
Fix
session/session-manager.ts): hard resets hand the exact fresh object to a dedicatedprovisionResetWorkflowSessionprovider path before publishing the reset event; reset subscribers can claim replay; generic replay only runs when the exact fresh object is still canonical;registerSession/unregisterSessionaccept an expected-current object for identity CAS semantics.space/runtime/task-agent-manager.ts): explicitreset-replacementprovisioning intent admits reset replacements foridle,waiting_rebind, resumable, and post-approval workers, fully rehydrating them with runtime MCPs attached before start/replay. Per-session ownership generations invalidate in-flight replacements when teardown (stop/cancel/cleanup/shutdown) supersedes them; the final commit rechecks task/run/space/execution eligibility and CAS-registers by object identity with no interveningawait(newSpaceManager.getSpaceSync). MCP self-heal refuses noncanonical instances.waiting_rebindghost lookups remain dormant; the stale persistedqueued-state cleanup defect is intentionally out of scope.Tests
node-agentattached, delivery readiness resolves it instead ofnullwaiting_rebindlookups; post-approval routing and cooldownsVerification
5-space-b: 2,904 / 2,904bun run check(lint, format, typecheck, knip, guards): green