diff --git a/plans/2026-07-11-concurrent-task-execution.md b/plans/2026-07-11-concurrent-task-execution.md new file mode 100644 index 0000000..4403296 --- /dev/null +++ b/plans/2026-07-11-concurrent-task-execution.md @@ -0,0 +1,230 @@ +# 2026-07-11 — Concurrent task execution: safety + throughput + frontend correctness + +**Status:** Planned + +## Goal + +Multiple tasks already run "at the same time" at the HTTP/goroutine level — +nothing in nomos serializes whole turns. But tracing the actual code (not +assuming) surfaces three real gaps that make concurrent tasks unsafe or +broken today, in decreasing severity: a **cross-task authorization bleed**, a +**throughput bottleneck** that makes concurrency mostly illusory, and a +**frontend state-corruption bug**. This plan fixes all three. + +## Findings (grounded) + +### 1. CRITICAL — the assent window is scoped to the agent, not the task + +[store.go:816](../cmd/nomos/store.go), [agent.go:129-143](../cmd/nomos/agent.go): +`openAssentWindow`/`assentWindowActive` key on `"assent_window.agent:" + +a.agentID.String()` — there is exactly one `agent:nomos` entity, so this key +is **global across every task**. It's checked at three MCP call sites +([internal/mcp/server.go:454](../internal/mcp/server.go), :488, :1363) purely +as "does *the agent* currently have an open window," with no way to know +which task's tool call is asking. + +**Concrete failure**: operator approves Task A's plan → 30-minute window +opens → operator starts Task B while that window is still open → Task B's +`run`/`request_execution` config-mutation calls **also auto-execute**, +because the check has no session dimension. The operator never approved +Task B's plan. + +[store.go:313-345](../cmd/nomos/store.go) `destructiveWindowKey`/ +`openDestructiveWindow`/`destructiveWindowActive` have the same shape (keyed +`agent:.target:`, no session) — narrower blast radius (needs a +second task hitting the *same target* within 15 minutes of an explicit typed +confirmation elsewhere) but the same class of bug. + +### 2. Tool calls across ALL tasks funnel through one mutex — concurrency is mostly illusory + +[main.go:45](../cmd/nomos/main.go): nomos creates exactly **one** +`*mcpClient` at startup, shared by every `handleChat` goroutine. Its `mu +sync.Mutex` ([main.go:419](../cmd/nomos/main.go)) is held for the full +duration of each `doRequest` round-trip. `run`'s MCP handler executes the SSH +command *synchronously inside that round-trip* and is capped at up to **10 +minutes**. So while Task A is mid-`run`, every other task's tool calls — +even a trivial `get_entity` — queue behind that single mutex until it +returns. Tasks can think (LLM calls) in parallel, but cannot act in parallel; +one slow task stalls all others' progress. + +The MCP *server* side has no session-scoped in-memory state to protect — +`newServer(pool, agentID)` returns one shared `*mcp.Server` instance whose +tool handlers close only over `pool` (safe for concurrent use — pgxpool is a +connection pool) and `agentID` ([internal/mcp/server.go:51-74](../internal/mcp/server.go)). +The mutex exists purely because nomos's *client* reuses one stateful +transport session, not because the server needs it. This is fixable without +touching the server. + +### 3. Frontend: the chat store is a global singleton — switching tasks mid-stream corrupts the view + +[chat.ts:160-269](../web/src/lib/stores/chat.ts) `sendMessage`'s SSE callback +mutates `messages`/`currentSession` by reaching for `ms[ms.length - 1]` — +i.e. it assumes the array it's mutating still belongs to the task it was +opened for. Nothing in the callback checks that. [chat.ts:111-117](../web/src/lib/stores/chat.ts) +`loadSessionMessages` (fired when you click a different task in the sidebar +or the board) does not cancel or otherwise account for a still-open stream +from the task you're leaving — it just calls `messages.set(...)` and +`currentSession.set(sessionId)`. + +**Concrete failure**: start Task A, while it's still streaming click into +Task B from the Tasks board → `messages`/`currentSession` now reflect Task +B → Task A's still-open SSE stream delivers its next `tool_use`/`text_delta` +→ the callback appends it onto what is now *Task B's* last message, and on +`done` calls `currentSession.set(taskAId)`, flipping the app back to Task A +underneath the operator. This is a real bug independent of anything else in +this plan — it's why "switch away from a running task to start another" +currently looks broken even though the backend handles it fine. + +(By contrast, [workspace.ts](../web/src/lib/stores/workspace.ts)'s live +events are already correctly session-scoped — `applyEvent` checks +`ev.correlation_id !== sid` before doing anything — because that mechanism +was built for this from phase 6. The bug is confined to the older, +per-turn `chat.ts` streaming path.) + +### Already fine, no change needed + +- **DB access**: `pgxpool.Pool` is a connection pool; concurrent queries from + multiple task goroutines are its normal use case. +- **Auto-continuation worker** ([continue.go](../cmd/nomos/continue.go)): + already scoped per session (`pendingContinuation.SessionID`) — processes + its poll batch sequentially (5/tick) but never mixes state across + sessions. Sequential processing is a throughput nit, not a correctness bug; + not in scope here. +- **Task board** ([Tasks.svelte](../web/src/pages/Tasks.svelte)): event-driven + refresh already handles any number of concurrently-changing tasks correctly + — it re-lists, it doesn't hold per-task live state. + +## Design + +### Fix 1 — session-scope the assent and destructive windows + +Thread `sessionID` through to the MCP call sites. nomos already knows the +session id when it calls a tool ([agent.go:363](../cmd/nomos/agent.go)); the +MCP wire protocol doesn't restrict tool-call args to the declared schema +(`argsMap` just unmarshals whatever JSON object arrives), so nomos can inject +an internal `_session_id` into the args it sends over the wire — invisible to +the model (never in the tool's `InputSchema`, so it never appears in what the +LLM sees or is asked to supply) but readable server-side. + +- `agent.go`: build a wire-args copy with `_session_id` added just before + `a.client.callTool(...)` (leave the args used for history/logging + unmodified — the model's own tool-call record shouldn't show an internal + field it never set). +- `internal/mcp/server.go`: `assentWindowActive`/`openAssentWindow`/ + `destructiveWindow*` gain a `sessionID` parameter; the key becomes + `assent_window.agent:.session:` (and similarly for the + destructive window). Every call site (`run`, `request_execution`, the + `apt_upgrade`/`pct_create` sub-cases) reads `_session_id` from `argsMap` + and passes it through. +- `agent.go` `openAssentWindow` gains the same `sessionID` param, called from + its two existing call sites ([agent.go:253](../cmd/nomos/agent.go), :269), + which are already inside `chatWith` and have `sessionID` in scope. +- Fallback: if `_session_id` is missing (defensive — shouldn't happen since + nomos always sets it), treat as "no window" (fail closed, require + approval) rather than falling back to the old agent-wide key. + +### Fix 2 — per-session MCP client (remove the throughput bottleneck) + +Replace the single global `*mcpClient` with a small **map of clients keyed +by session id**, created lazily on first tool call for that session and +evicted after a period of inactivity (e.g. 10 minutes past the session's last +activity — long enough to outlive a slow `run`, short enough not to leak +connections for abandoned tasks). Guard the map itself with a mutex (cheap — +only held for map lookup/insert, not for the duration of a call); each +individual client keeps its own `mu` scoped to *its own* session's calls, +so Task A's slow `run` only serializes Task A's own tool calls (which are +already inherently sequential within one turn — the agent loop calls tools +one at a time) and never blocks Task B. + +- New `mcpClientPool` type in `cmd/nomos`: `get(sessionID) *mcpClient` + (creates+initializes on miss), `sweep()` (evicts idle clients, called on a + ticker alongside the existing continuation-worker ticker). + `"ephemeral"`/`""` session ids (no persisted session) get their own + dedicated client, not pooled per-request, to avoid a connection-per-message + churn for the no-DB-store path. +- `agent` holds the pool instead of one `client`; `handleQuery` (the + structured `/query` endpoint, [main.go:330](../cmd/nomos/main.go)) picks a + short-lived or dedicated client the same way. +- No server-side change needed (per finding 2's analysis — the server has no + per-connection state to protect). + +### Fix 3 — frontend: don't let a background stream corrupt the active view + +Minimal, contained fix (not a rearchitecture): capture the session id a +`sendMessage` stream belongs to, and have its callback check that +`currentSession` still matches before mutating `messages`/`streaming`. If the +operator has navigated away, the stream's events are silently dropped from +the UI (the task keeps running server-side regardless — the events are also +flowing on the global stream, and if the operator navigates back, +`loadSessionMessages`'s poll + REST hydration picks up whatever landed while +they were away, same as it already does for auto-continuation). + +- `chat.ts` `sendMessage`: capture `const streamSessionID = ...` once the + `'session'` event assigns it; every subsequent branch of the callback + (`tool_use`, `tool_result`, `text_delta`, `text`, `done`, `error`) first + checks `get(currentSession) === streamSessionID` (or the pre-assignment + optimistic session) before touching `messages`. +- `loadSessionMessages`: no change needed once the above guard exists — it + already correctly sets `messages`/`currentSession` for the task being + opened; the guard just stops the *other* task's stream from clobbering it + afterward. +- Out of scope for this pass: a genuine multi-pane "watch two tasks stream + live side by side" UI. Not needed for correctness — the Tasks board already + shows live status for every task via `workspace.ts`'s correctly-scoped + events; only the single-focus Chat transcript view needs this guard. + +### Fix 4 (optional) — a concurrency/cost guardrail + +Nothing currently stops an operator from starting many tasks in a tight loop, +each spending real LLM API budget in parallel. Consider a simple semaphore in +nomos (`NOMOS_MAX_CONCURRENT_TASKS`, default e.g. 5) that `handleChat` acquires +before starting a turn and releases on completion; over the cap, queue or +reject with a clear "N tasks already running, try again shortly" rather than +letting an unbounded burst hit OpenRouter. This is an operational safeguard, +not a correctness fix — flagged as optional/lower priority. + +## Implementation order + +1. **Fix 1 (assent/destructive window session-scoping)** — the only one that's + a genuine safety bug (auto-running unapproved actions in another task); + ship first regardless of anything else. +2. **Fix 3 (frontend stream guard)** — small, contained, fixes a visibly + broken UX (switching tasks looks corrupted) independent of Fix 2. +3. **Fix 2 (per-session MCP client pool)** — the throughput fix; more moving + parts (lifecycle/eviction), ship after the safety fix lands and is + verified, since both touch the same call sites (`agent.go` tool dispatch). +4. **Fix 4 (concurrency cap)** — optional, only if real usage shows a need. + +## Verification + +- **Fix 1**: approve Task A's plan (open its window); concurrently start Task + B and have it attempt a config-mutation `run` command *without* approving + Task B's plan — confirm Task B's action is queued for approval (not + auto-run), while Task A's own subsequent steps keep auto-running. + `SELECT key FROM autonomy_settings WHERE key LIKE 'assent_window%'` should + show session-scoped keys. +- **Fix 2**: start Task A with a `run` step that sleeps ~60s; concurrently + start Task B with a trivial `get_entity` call; confirm Task B's tool result + returns immediately rather than waiting on Task A. Confirm the client map + evicts idle entries (`sweep()` logged, connection count doesn't grow + unbounded across many sequential tasks). +- **Fix 3**: start Task A, before it finishes click into Task B on the + board, confirm Task B's transcript stays correct (no Task-A tool calls + appended) and `currentSession` doesn't flip back to Task A when its stream + eventually completes in the background. Navigate back to Task A afterward + and confirm its full transcript (including what happened while unwatched) + loads correctly via REST. + +## Open questions + +- **Idle eviction window for Fix 2**: 10 minutes was a guess balancing + "outlive a slow `run`" against "don't leak connections." Worth checking + actual `run` durations in production (`executions.duration_ms`) before + picking a final number. +- **Fix 4's cap and behavior on overflow**: queue vs. reject vs. no cap at + all — depends on real usage patterns once concurrent tasks are actually + safe (Fix 1) and performant (Fix 2). Defer the decision until there's + data. +- **Destructive-window session-scoping**: bundle into Fix 1 (same shape, same + PR) or treat as a follow-up given its narrower blast radius? Leaning bundle + — it's the same three-line change pattern applied to one more function. diff --git a/plans/2026-07-11-goal-oriented-chat-control-panel.md b/plans/done/2026-07-11-goal-oriented-chat-control-panel.md similarity index 88% rename from plans/2026-07-11-goal-oriented-chat-control-panel.md rename to plans/done/2026-07-11-goal-oriented-chat-control-panel.md index 3ff01ea..327aa60 100644 --- a/plans/2026-07-11-goal-oriented-chat-control-panel.md +++ b/plans/done/2026-07-11-goal-oriented-chat-control-panel.md @@ -1,11 +1,15 @@ # 2026-07-11 — Tasks: the chat page as goal-structured autonomous work -**Status:** Planned — supersedes the sidebar-only framing; this is the -definitive direction for the chat surface. Any earlier plan detail that -conflicts with "the chat page is a board of tasks" is overridden by this doc -(notably the Chat portion of -[control-room-webui](2026-07-08-control-room-webui.md), which described chat as -a free-form session list). +**Status:** Done — 2026-07-11. All 7 phases shipped and deployed (SHA +`e30813a`): task schema + entity anchor, `entity.touched`/`involves` live +tracking, the `complete_task`/knowledge-retrieval loop, structured +`propose_plan`/`update_plan_step` (with an append-not-replace fix for +mid-flight re-proposals), `ask_operator` pause/resume, the Tasks board, and +the live `TaskContextPanel`. Follow-up hardening tracked separately in +[concurrent-task-execution](../2026-07-11-concurrent-task-execution.md). +Supersedes the sidebar-only framing and the Chat portion of +[control-room-webui](../2026-07-08-control-room-webui.md), which described +chat as a free-form session list. ## The vision (operator, distilled) @@ -29,7 +33,7 @@ Three pillars: **task as the unit**, **one approval → autonomous execution**, ## The reframe Today a "session" is a title + a flat message list -([migrations/015](../migrations/015_agent_sessions.up.sql)); a "plan" is prose +([migrations/015](../../migrations/015_agent_sessions.up.sql)); a "plan" is prose the model types; there is no goal, status, outcome, or step object. We elevate the session into a **task**: @@ -39,8 +43,8 @@ the session into a **task**: context in the sidebar. - **The plan is approved once.** Machinery already exists — the assent window + event-driven auto-continuation shipped in - [autonomous-plan-execution](done/2026-07-10-autonomous-plan-execution.md) - ([continue.go](../cmd/nomos/continue.go), [assent.go](../cmd/nomos/assent.go)) + [autonomous-plan-execution](2026-07-10-autonomous-plan-execution.md) + ([continue.go](../../cmd/nomos/continue.go), [assent.go](../../cmd/nomos/assent.go)) already turn a single approval into an autonomy grant the agent runs to completion. This plan gives that flow a **structured surface**: the one thing the operator approves is a named, stepped plan, and progress is visible. @@ -48,17 +52,17 @@ the session into a **task**: entities involved *and to the task itself*, tagged success/failure — and **future tasks read it back at planning time.** The substrate exists: `upsert_knowledge` writes a knowledge doc-entity and a `documents` - relationship ([server.go:1519](../internal/mcp/server.go)); - `get_entity_knowledge` reads it ([server.go:170](../internal/mcp/server.go)). + relationship ([server.go:1519](../../internal/mcp/server.go)); + `get_entity_knowledge` reads it ([server.go:170](../../internal/mcp/server.go)). We add task-linkage, an outcome flavor, and retrieval-at-planning. ## Builds on / aligns with -- [general-gated-execution](2026-07-10-general-gated-execution.md) — the +- [general-gated-execution](../2026-07-10-general-gated-execution.md) — the classifier + `run` primitive is the execution substrate; a plan step is just a described unit of work mapping to a `run`/`request_execution` call. **No fixed step enum.** -- [autonomous-plan-execution](done/2026-07-10-autonomous-plan-execution.md) — +- [autonomous-plan-execution](2026-07-10-autonomous-plan-execution.md) — the single-approval autonomy window + auto-continuation loop. - The knowledge tools + relationships graph (`upsert_knowledge`, `get_entity_knowledge`, `get_relations`, the temporal `relationships` table). @@ -173,8 +177,8 @@ prior. SOUL makes this the first planning move. ### Task board (replaces the raw session rail / empty chat state) -[Sessions.svelte](../web/src/pages/Sessions.svelte) / -[SessionRail.svelte](../web/src/lib/components/SessionRail.svelte) become a +[Sessions.svelte](../../web/src/pages/Sessions.svelte) / +[SessionRail.svelte](../../web/src/lib/components/SessionRail.svelte) become a **board of task cards**. Each card: - goal as the title, one-line `summary`, - a **status pill** (running ◐ / awaiting you / done ✓ / failed ✗) with the @@ -188,7 +192,7 @@ replaces "new chat" — the empty state asks for a goal. ### Task detail = conversation + live context sidebar Center column: the existing chat transcript (tools, thinking, questions inline) -— unchanged rendering ([Chat.svelte](../web/src/pages/Chat.svelte)). +— unchanged rendering ([Chat.svelte](../../web/src/pages/Chat.svelte)). Right sidebar becomes `TaskContextPanel.svelte`, populated **in real time**, top to bottom: @@ -200,18 +204,18 @@ to bottom: Answering POSTs the answer and resumes the agent. Same card also renders inline in the transcript at the point it was raised. (The operator's "structured component with relevant context.") -4. **LiveEntityPanel** — the [SessionGraph](../web/src/lib/components/SessionGraph.svelte) +4. **LiveEntityPanel** — the [SessionGraph](../../web/src/lib/components/SessionGraph.svelte) upgraded from passive to live: `entity.touched` → the node **pulses** + "now touching `lxc:foo`"; `health.changed` → recolor + transient `healthy→degraded` diff badge. 5. **Outcome & Knowledge** — on completion: success/failure banner, the `summary`, and the knowledge notes recorded (links to the knowledge - entities), i.e. the [SessionDigest](../web/src/lib/components/SessionDigest.svelte) + entities), i.e. the [SessionDigest](../../web/src/lib/components/SessionDigest.svelte) evolved into a task-outcome card. ## Real-time event contract (global `/events/stream`) -The panel is driven by the **always-on** [events stream](../web/src/lib/stores/events.ts), +The panel is driven by the **always-on** [events stream](../../web/src/lib/stores/events.ts), not the per-turn chat SSE — so it stays live during server-side auto-continuation (when no chat turn is open) and survives a tab reload. New `type`s, each carrying `correlation_id = session_id`: @@ -227,7 +231,7 @@ auto-continuation (when no chat turn is open) and survives a tab reload. New | `knowledge.recorded` | `{ title, about, outcome }` | `entity.touched` is emitted from the `withActivityLogging` wrapper -([server.go:832](../internal/mcp/server.go)) — it wraps every tool call, so +([server.go:832](../../internal/mcp/server.go)) — it wraps every tool call, so touched-entity tracking needs **zero agent changes**; it also writes the `task —involved→ entity` relationship. `health.changed` already exists. @@ -239,7 +243,7 @@ in-process (event and row commit together): - `propose_plan(steps:[{title,detail?,target_slug?}])` - `update_plan_step(seq,status,execution_id?)` — plus the execution's terminal status **auto-closes** its linked step where - [phase3.go](../internal/httpapi/phase3.go) finalizes executions (belt and + [phase3.go](../../internal/httpapi/phase3.go) finalizes executions (belt and suspenders). - `ask_operator(prompt,options?,context_entities?,why?)` — creates the question, status→`awaiting_input`, ends the turn; answer resumes via the existing diff --git a/plans/index.md b/plans/index.md index 9f8afb0..6e7b5e8 100644 --- a/plans/index.md +++ b/plans/index.md @@ -13,7 +13,7 @@ went sideways, open an investigation. | 2026-07-08 | [Control room web UI](2026-07-08-control-room-webui.md) | In Progress | | 2026-07-08 | [Liveness, drift, and UX cohesion](2026-07-08-liveness-drift-and-ux-cohesion.md) | In Progress — Phase 5 deferred | | 2026-07-10 | [General gated execution: unlimited actions, gated by risk](2026-07-10-general-gated-execution.md) | In Progress — enum retirement + auto-act revival still open | -| 2026-07-11 | [Tasks: the chat page as goal-structured autonomous work](2026-07-11-goal-oriented-chat-control-panel.md) | Planned | +| 2026-07-11 | [Concurrent task execution: safety + throughput + frontend correctness](2026-07-11-concurrent-task-execution.md) | Planned | ## Done @@ -39,6 +39,7 @@ See [`done/`](done/) for executed plans: | 2026-07-09 | [Chat sessions: reliability, cost, and session-management fixes](done/2026-07-09-chat-sessions-improvements.md) | | 2026-07-09 | [Session execution, UX, and learning improvements](done/2026-07-09-session-execution-and-ux-fixes.md) | | 2026-07-10 | [Autonomous plan execution: close the observation gap](done/2026-07-10-autonomous-plan-execution.md) | +| 2026-07-11 | [Tasks: the chat page as goal-structured autonomous work](done/2026-07-11-goal-oriented-chat-control-panel.md) | ## Conventions