Fix/title gen nonblocking - #17
Conversation
…propagate run_ctx Store _active_run_ctx and _iteration_task as instance variables so interrupt() can cancel the LLM API call from a different task (e.g., OpenCode abort_session). Previously, iteration_task was local and only cancelled indirectly through the consumer's finally block, which could time out and leave the LLM call running. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…t QUEUED labels and conversation history loss When a user cancels a running agent (ESC in TUI), the CancelledError handler now: (1) sets time.completed and MessageAbortedError so the TUI's pending memo can move past the message, and (2) adds the aborted assistant message to the agent's in-memory conversation so the LLM receives complete history on subsequent runs. Previously, the stale pending message caused all new user messages to display QUEUED, and the agent's conversation was missing the partial response. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ssage hang Title generation was awaited inline in _process_message_locked() before the agent started streaming. A slow or unresponsive title model (e.g. svc/kimi-k2 with cold-start delay) would block the entire first message response for seconds. Changes: - message_routes: fire-and-forget via state.create_background_task() instead of await — title arrives asynchronously via metadata_generated signal / SessionUpdatedEvent SSE event - storage/manager: add asyncio.wait_for(timeout=15s) in _generate_title_from_prompt() as zombie-task safety net Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Tests verify: - _process_message_locked returns fast despite slow title gen - Title still appears asynchronously after generation completes - on_title_generated callback still fires - 15s timeout prevents zombie tasks on stuck title model Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of agent interruption and prevents title generation from blocking message processing. Key changes include tracking active run contexts and iteration tasks at the instance level to ensure they are properly cancelled during an interrupt, making title generation a background task with a 15-second timeout, and ensuring assistant messages are correctly finalized with an aborted state upon user cancellation. Feedback identifies a bug where the agent's internal cancelled flag is not reset for new runs, potential race conditions and history pollution when an agent instance is shared across concurrent sessions, and redundant state tracking for stream tasks.
| run_ctx.cancelled = False | ||
| run_ctx.current_task = asyncio.current_task() |
There was a problem hiding this comment.
The agent's instance-level self._cancelled flag is set to True during interrupt(), but it is never reset to False when a new run_stream starts. This causes is_cancelled() to return True for all subsequent runs on the same agent instance. The test test_subsequent_run_after_interrupt in test_interrupt.py actually manually resets this flag, which highlights the issue. It should be reset here alongside the run_ctx.cancelled flag.
run_ctx.cancelled = False
self._cancelled = False
run_ctx.current_task = asyncio.current_task()| # Store run_ctx as instance variable so interrupt() can find it | ||
| # from a different task (ContextVar is task-scoped and returns None | ||
| # when read from outside the run_stream task) | ||
| self._active_run_ctx = run_ctx |
There was a problem hiding this comment.
Storing run_ctx in the instance variable self._active_run_ctx creates a race condition if the agent instance is shared across concurrent sessions (as seen in the server's ServerState). If session A is streaming and session B starts, _active_run_ctx will point to session B. A subsequent interrupt() call for session A (without an explicit run_ctx) would then incorrectly target and cancel session B. This is particularly relevant given the server's architecture where a single agent instance appears to be shared.
| iteration_task = self._iteration_task | ||
| if iteration_task is not None and not iteration_task.done(): | ||
| iteration_task.cancel() |
There was a problem hiding this comment.
The instance variable self._iteration_task is subject to the same race condition as _active_run_ctx. In a multi-session environment with a shared agent, calling interrupt for one session could inadvertently cancel the LLM iteration task of another session if that session was the most recent one to start. This state should be managed in a way that is isolated per run or session.
| # reloading because agent.session_id matches, so the LLM receives | ||
| # incomplete history — it doesn't know it already (partially) responded. | ||
| chat_msg = opencode_to_chat_message(assistant_msg_with_parts, session_id=session_id) | ||
| agent.conversation.add_chat_messages([chat_msg], extend_last=True) |
There was a problem hiding this comment.
Adding the aborted message to agent.conversation while the agent instance is shared across sessions can lead to history pollution. If session A is aborted, its partial response is added to the shared conversation object, which might then be visible to session B if it's running concurrently or if the agent state isn't reloaded. While this fixes history for the current session, it highlights the risks of the shared agent architecture.
There was a problem hiding this comment.
Code Review
This pull request addresses bugs in agent interruption and session title generation by introducing tracking for active run contexts and implementing timeouts for title generation. It also ensures that assistant messages are correctly finalized and persisted when a user cancels a request. The code review feedback highlights critical concurrency issues: storing run contexts and iteration tasks as instance variables on the agent is unsafe for concurrent use, as it leads to race conditions and potential history contamination between sessions. Additionally, a duplicate fixture was identified in the test files.
| self._current_stream_task = run_ctx.current_task | ||
| # Store run_ctx as instance variable so interrupt() can find it | ||
| # from a different task (ContextVar is task-scoped and returns None | ||
| # when read from outside the run_stream task) | ||
| self._active_run_ctx = run_ctx |
There was a problem hiding this comment.
Storing run_ctx and the stream task as instance variables (self._active_run_ctx, self._current_stream_task) is not safe for concurrent run_stream calls on the same agent instance. A subsequent call will overwrite these references, and the finally block of the first call to complete will clear them (lines 701-702). This creates a race condition where active runs can become untracked and un-interruptible if the agent is used concurrently (e.g., in a multi-session server environment).
|
|
||
| # Start the agent iteration task | ||
| iteration_task = asyncio.create_task(agent_iteration_task()) | ||
| self._iteration_task = iteration_task |
There was a problem hiding this comment.
| iteration_task = self._iteration_task | ||
| if iteration_task is not None and not iteration_task.done(): | ||
| iteration_task.cancel() |
There was a problem hiding this comment.
The _interrupt method cancels self._iteration_task unconditionally. If multiple runs are active on the same agent instance, this could interrupt an iteration task belonging to a different session than the one intended. This task should be tracked per run_ctx or task to ensure correct scoped cancellation.
| # reloading because agent.session_id matches, so the LLM receives | ||
| # incomplete history — it doesn't know it already (partially) responded. | ||
| chat_msg = opencode_to_chat_message(assistant_msg_with_parts, session_id=session_id) | ||
| agent.conversation.add_chat_messages([chat_msg], extend_last=True) |
There was a problem hiding this comment.
Adding the aborted message to agent.conversation while the agent instance is shared across sessions can lead to history contamination. If another session has called load_session on the same agent instance before this line executes, the aborted message will be appended to the wrong session's history. This highlights a broader issue with sharing a stateful agent instance across concurrent sessions in the server.
… state, add concurrency guards - Reset self._cancelled=False in run_stream() so subsequent runs after interrupt() are not stuck in cancelled state - Remove redundant self._current_stream_task (already tracked via _active_run_ctx.current_task); update ACP/AGUI subclasses accordingly - Add concurrency guard: warn when run_stream() starts while another is active (shared agent instance is single-session) - Add similar guard for _iteration_task in native_agent - Document shared-agent history mutation limitation in message_routes Fixes: #17 (review comments) Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
- Remove duplicate fast_agent fixture (lines 102-107) - Update docstring: ContextVar → _active_run_ctx (correct mechanism) - Update test assertion: _current_stream_task → _active_run_ctx.current_task Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Derivative RFC of RFC-0021 addressing concurrent safety regression from PR #17. Proposes dict[session_id, AgentRunContext] registry for cross-task interrupt routing, replacing instance variables _active_run_ctx and _iteration_task that violate per-run isolation. Key decisions: - self.session_id internal reads migrated to run_ctx (all agent types) - self._cancelled retained for background runs only - No fallback heuristic; interrupt() requires session_id or run_ctx - self.parent_session_id also migrated to AgentRunContext Reviewed by Metis (pre-planning) and Oracle (2 rounds). Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Derivative RFC of RFC-0021 addressing concurrent safety regression from PR #17. Proposes dict[session_id, AgentRunContext] registry for cross-task interrupt routing, replacing instance variables _active_run_ctx and _iteration_task that violate per-run isolation. Key decisions: - self.session_id internal reads migrated to run_ctx (all agent types) - self._cancelled retained for background runs only - No fallback heuristic; interrupt() requires session_id or run_ctx - self.parent_session_id also migrated to AgentRunContext Reviewed by Metis (pre-planning) and Oracle (2 rounds). Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* docs(rfc): add RFC-0023 session-scoped interrupt routing Derivative RFC of RFC-0021 addressing concurrent safety regression from PR #17. Proposes dict[session_id, AgentRunContext] registry for cross-task interrupt routing, replacing instance variables _active_run_ctx and _iteration_task that violate per-run isolation. Key decisions: - self.session_id internal reads migrated to run_ctx (all agent types) - self._cancelled retained for background runs only - No fallback heuristic; interrupt() requires session_id or run_ctx - self.parent_session_id also migrated to AgentRunContext Reviewed by Metis (pre-planning) and Oracle (2 rounds). Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix(opencode): expose skills in TUI slash command autocomplete Skills were returned with source='skill' from GET /command, but the OpenCode TUI autocomplete explicitly filters out source==='skill' commands (autocomplete.tsx:363). Native OpenCode's oh-my-openagent plugin injects skills with source='command', bypassing the filter. Changes: - Change skill command source from 'skill' to 'command' in GET /command endpoint (agent_routes.py) to match native OpenCode behavior - Expand Command model to match native SDK Command.Info type: add agent, model, subtask, hints fields; make description and source optional - Add _extract_hints() helper to extract / placeholders from skill templates, matching native Command.hints() utility - Fix server.py CommandStore update: replace broken add_commands() call with register_command(cmd, replace=True) loop - Add fallback skill lookup via pool.skill_commands in session_routes when CommandStore misses dynamically-added skills * chore: update rfc. * feat(opencode): per-session agent isolation (RFC-0026) Replace the OpenCode server's single shared agent usage with per-session agent instances so concurrent clients stop clobbering each other's mutable state. Key changes: - ServerState: add _session_agents dict + _session_agent_locks for per-session agent registry with double-check locking pattern - get_or_create_agent(session_id): race-free creation using NativeAgentConfig.get_agent() (creates NEW instances, not cached) - _create_session_agent(): wraps agent creation in ConfigContextManager to resolve relative tool schema paths - cleanup_all_session_agents() / remove_session_agent(): graceful shutdown for per-session agents with partial-init safety - Cache _pool, _storage, _agent_config in __post_init__ so non- session-scoped deps remain accessible without shared agent - session_routes: all session flows migrated to get_or_create_agent() - Removed agent_lock / bind_agent_to_session() usage - Removed original_model save/restore in init_session() - abort_session() targets correct session agent - fork_session() creates independent agent instance - message_routes: _process_message_locked() uses per-session agent directly, no agent_lock, no model save/restore - config_routes: model propagation to _session_agents on set_model - 8 new test files with ~55 tests covering: - Session agent registry isolation and race-free creation - Server lifecycle cleanup on shutdown - Session isolation (cross-talk, fork divergence, abort targeting) - Message isolation (concurrent processing, model scoping) - Route discovery and module loading - Session cleanup (delete-session, partial-init safety) - Concurrency (independent sessions, interrupt isolation, FIFO) - Model/fork/input-provider isolation regression Fixes: FileNotFoundError in QuestionProvider when creating session agents at runtime (ConfigContextManager exited before request handlers run, leaving _config_dir_global = None) * fix(opencode): cancel pending questions on abort to stop agent stream When abort_session was called while the agent had a pending question (e.g., question_for_user / get_elicitation), the question's Future remained unresolved. The agent would continue streaming after the user answered the question, even though the session was marked idle. Root cause: abort_session interrupted the agent and cancelled background tasks, but did not cancel pending question Futures. The LLM call had already completed and created a tool call event for a question. When the user answered, the Future resolved and the agent resumed. Fix: Add cancel_session_pending_questions(session_id) to ServerState and call it at the top of abort_session(), BEFORE cancelling background tasks and interrupting the agent. Cancelling the Future causes get_elicitation() to catch CancelledError and return ElicitResult(action='cancel'), which raises RunAbortedError through the stream, properly finalizing the assistant message. * fix(opencode): track and cancel active message task on abort - Register current task in _process_message_locked so abort_session can cancel it even for sync send_message path - Add cancel_active_message_task() to ServerState for direct task cancellation - Re-cancel pending questions after interrupt to close TOCTOU race (agent may ask questions between initial cancel and interrupt) * fix(opencode): address PR #18 review findings - Call __aenter__ on per-session agents in get_or_create_agent so MCP subprocesses and tool schemas are initialized (only for agents created from config; test mocks skip this path) - Load copied conversation history into fork agent so the LLM sees the full context on the first message after forking - Use list() snapshot when iterating _session_agents.items() with await inside the loop body to prevent RuntimeError on concurrent mutation - Add NOTE comment about subagent shared-instance limitation in _process_message_locked * docs(rfc): mark RFC-0023 and RFC-0026 as IMPLEMENTED Move RFC-0023 (session-scoped interrupt routing) from draft/ to implemented/ and RFC-0026 (per-session agent isolation) from accepted/ to implemented/. Both have been fully implemented in the current PR. * diag: add DIAG: logging for cross-session message contamination investigation 5 diagnostic logging points to confirm two bug hypotheses: - Bug 1 (cross-session): shared state.agent leaks conversation across sessions - Bug 2 (duplicate): user msg appears in both message_history and prompts Points: _create_session_agent, _process_message_locked agent resolution, _stream_events history vs prompts overlap. Search 'DIAG:' to find/remove. * fix: resolve cross-session message contamination and duplication bugs Bug1a: _agent_config.name was None because manifest stores original config references (name set from dict key, not YAML). Pool's model_copy only affects local copy. Fix: model_copy in __post_init__ to set name. Bug1c: request.agent matching default agent name replaced per-session agent with shared pool singleton. Fix: skip delegation when request.agent matches the session's default agent config name. Bug2: User message appeared in both message_history and prompts of agentlet.iter() because _run_stream_once() adds user_msg to conversation before _stream_events() calls get_history(). Fix: exclude last history entry when it matches user_msg (identity check). Also removes all 5 DIAG: diagnostic logging statements. * fix: add synthetic ToolReturnPart for Pending/Running tool calls in opencode_to_chat_message When a question (elicitation) is cancelled by user (ESC) and abort_session cancels both pending questions and the active message task, the abort handler adds the partial assistant message to agent.conversation via opencode_to_chat_message(). This conversion was only adding ToolReturnPart for ToolStateCompleted and ToolStateError, leaving ToolStatePending and ToolStateRunning tool calls without corresponding results. Pydantic-AI validates that every ModelResponse.tool_calls has a matching ToolReturnPart in the next ModelRequest. Without this, the next user message triggers: 'Cannot provide a new user prompt when the message history contains unprocessed tool calls.' Fix: Add a synthetic ToolReturnPart with 'Tool call was aborted before completion' for Pending/Running tool states. --------- Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary
_maybe_generate_title()wasawait-ed inline in_process_message_locked()before the agent started streaming. A slow or unresponsive title model (e.g.svc/kimi-k2with cold-start delay) blocked the entire first message response for seconds.message_routes.py): Changedawait _maybe_generate_title()to fire-and-forget viastate.create_background_task(). Title arrives asynchronously via the existingmetadata_generatedsignal /SessionUpdatedEventSSE event.storage/manager.py): Addedasyncio.wait_for(timeout=15.0)in_generate_title_from_prompt()as a zombie-task safety net. Prevents background title-generation tasks from hanging forever on a stuck model.Test plan
_process_message_lockedreturns in <0.5s even with a 2s title model (previously took 2.0s)_generate_title_coremock also returns faston_title_generatedcallback still fires