fix(agent): restart codex agents after config changes - #430
Conversation
|
@xxx7xxxx Please help review this. |
|
@wanghaojie124 Better to have bullet points for the summary |
| if err := s.provisionRuntimeForAgent(ctx, runtimeImpl, got, ""); err != nil { | ||
| return Agent{}, fmt.Errorf("provision codex agent for restart: %w", err) | ||
| } | ||
| state, err := runtimeImpl.Start(ctx, handle) |
There was a problem hiding this comment.
This restart enters Runtime.Start, but that path still creates SessionSpec without reading the new conversation_sessions field. ensureSession then persists the restarted session with an empty map, so every automatic profile/MCP restart erases all room-to-thread mappings. I reproduced this with Stop -> Start: the saved room-1 mapping was absent from the restart spec. Load the mapping in the normal Start path and add a stop/start regression test.
There was a problem hiding this comment.
Fixed in 52def89. Runtime.Start now reads the persisted session metadata and passes ConversationSessions into the new SessionSpec. I also added a Stop -> Start regression test that verifies the room-to-thread mapping reaches the restarted session.
| spec: spec, | ||
| appClient: appClient, | ||
| conversationSessions: make(map[string]string), | ||
| conversationSessions: conversationSessions, |
There was a problem hiding this comment.
These persisted IDs are copied into memory but never resumed into the fresh app-server process. EnsureSession returns a cached ID immediately, then Prompt sends turn/start without a preceding thread/resume; a protocol-faithful reproduction fails with thread not loaded (-32000). Lazily resume restored room threads before returning them, handle resume fallback by updating persistence, and test through Prompt rather than only checking the cached ID.
There was a problem hiding this comment.
Fixed in 52def89. Restored room threads are now lazily resumed before EnsureSession returns them. If resume fails, a new thread is created and the replacement mapping is persisted. The regression test now sends a Prompt through the restored thread to verify it is loaded in the fresh app-server process.
| return existing, nil | ||
| } | ||
| live.conversationSessions[conversationKey] = threadID | ||
| conversations := cloneConversationSessions(live.conversationSessions) |
There was a problem hiding this comment.
The snapshot is persisted after releasing live.mu, so overlapping create/reset operations can complete writes out of order. I reproduced a newer {room-1, room-2} snapshot being written first and then overwritten by a delayed {room-1} snapshot, which loses room-2 after restart. Serialize persistence per live session or add versioning, and cover concurrent updates.
There was a problem hiding this comment.
Fixed in 52def89. Conversation mapping mutations and persistence are serialized per live session, preventing older snapshots from being written after newer ones. Added a concurrent persistence regression test and verified the Codex runtime tests with the race detector.
| } | ||
| if restartRequired && runtimeRunning && !isGatewayRuntimeKind(runtimeKind) { | ||
| if restartRequired && runtimeRunning && strings.EqualFold(runtimeKind, RuntimeKindCodex) { | ||
| if _, err := s.restartCodexRuntime(ctx, id); err != nil { |
There was a problem hiding this comment.
This new restart path makes the existing Codex MCP API subtest terminate its own test process: apiFakeCodexManager stores os.Getpid() and its Stop is a no-op, so Runtime.Stop sends SIGINT to the package. go test ./internal/api -run '^TestHandleAgentsMCPServersClosedLoopForSupportedRuntimes$/^codex$' fails deterministically with signal: interrupt. Update the fixture to model a safely stoppable process and assert the restart behavior.
There was a problem hiding this comment.
Fixed in 52def89. The API fixture now launches a dedicated test child process and tracks/stops it through the fake manager instead of using the package test PID. The exact Codex MCP subtest now passes, including with the race detector.
3 participants
Uh oh!
There was an error while loading. Please reload this page.