docs: plan concurrent task execution; archive completed tasks plan
New plan grounds three concurrency issues found by tracing the actual code
(not assumed): the assent/destructive windows are keyed by agent id only
(no session dimension), so an approved plan in one task can auto-run
unapproved actions in a concurrently-running task; nomos shares one
mutex-guarded MCP client across all sessions, so a single slow `run` call
serializes every other task's tool calls behind it; and chat.ts's SSE
callback has no session guard, so switching tasks mid-stream lets the
backgrounded task's events corrupt whatever's now displayed. Proposes
session-scoping the windows (critical/first), a frontend stream guard
(contained/second), a per-session MCP client pool (throughput/third), and
an optional concurrency cap (deferred pending real usage data).
Also archives the goal-oriented-chat-control-panel plan to done/ — all 7
phases shipped and are live in production (SHA e30813a) — fixing its
internal relative links for the new depth and pointing forward to the new
concurrency plan as follow-up hardening.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
230
plans/2026-07-11-concurrent-task-execution.md
Normal file
230
plans/2026-07-11-concurrent-task-execution.md
Normal file
@@ -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:<id>.target:<slug>`, 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:<id>.session:<sessionID>` (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.
|
||||
Reference in New Issue
Block a user