Skip to content

Refactor session ownership, turn cancellation, and tool execution - #198

Merged
stippi merged 15 commits into
mainfrom
refactor/session-runtime-consistency
Sep 15, 2026
Merged

stippi merged 15 commits into
mainfrom
refactor/session-runtime-consistency

Conversation

@stippi

@stippi stippi commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Summary

Implements three incremental architecture improvements, with regression tests written and run red before the corresponding fixes.

1. Session state ownership and canonical conversation

  • Make the conversation tree authoritative; keep prompt recovery/compaction adjustments in a separate projection rather than deleting canonical evidence.
  • Preserve formatted tool inputs and response corrections across persistence round-trips.
  • Stop agent checkpoints from overwriting session-owned settings with stale run configuration.
  • Add a per-session locked read–modify–write boundary for settings and checkpoints.

2. Turn lifecycle and cancellation

  • Introduce bounded per-session command lanes, independent slow-I/O dispatch, and a separate control mailbox.
  • Reserve a run before asynchronous preparation, without holding the shared manager lock across the SessionService MCP/LLM preparation path.
  • Tie cancellation and TurnHandle cancellation to the specific run; wake provider and permission waits even when no streaming chunks arrive.
  • Preserve newer model selections during preparation and reject externally locked starts before appending a message.
  • Use owned cleanup guards for preparation, cancellation registrations, and sleep inhibition.

3. Tool execution and recovery

  • Share mandatory scope/permission checks and completion handling between sequential, parallel, and intercepted calls.
  • Run completion hooks and preserve modified inputs for parallel calls; retain sequential ordering barriers.
  • Journal unstarted/uncertain invocations and persist completed results individually, without waiting for the whole batch.
  • Stop further dispatch on persistence failure; do not let UI publication failures erase completed tool evidence.
  • Finalize sub-agent errors/cancellation with structured output, preserve completed child-tool evidence and usage, and unregister on errors, panics, and task drops.
  • Fix a separately reproduced streaming panic on empty tool input ({}).

Review order

  1. 4bb7282c — canonical conversation and session-owned checkpoint boundaries
  2. ca6b02e6 — run preparation, command lanes, and cancellation
  3. 9ff102f8 — shared tool pipeline and individual outcome journal
  4. aecda236 — empty JSON input streaming fix
  5. 35916ad3 — sub-agent finalization, cleanup, and cancellation propagation

Validation

Locally verified on macOS, without the document-conversion feature:

  • 700 tests passed, 2 ignored across tools_core, agent_core, code_assistant_core, and mcp_client.
  • Clippy for those crates, all targets, with -D warnings: passed.
  • Test compilation with GPUI, terminal, ACP, and MCP-server frontends enabled: passed.
  • Formatting and git diff --check: passed.

Commands:

cargo test --locked --offline --no-default-features -p tools_core -p agent_core -p code_assistant_core -p mcp_client --lib -- --test-threads=1
cargo clippy --locked --offline --no-default-features -p tools_core -p agent_core -p code_assistant_core -p mcp_client --all-targets -- -D warnings
cargo check --locked --offline -p code-assistant --no-default-features --features gpui-frontend,acp-frontend,terminal-frontend,mcp-server --tests
cargo fmt --all -- --check
git diff --check

Scope and limitations

  • read-only / outward classification semantics are unchanged.
  • Persistence remains JSON-snapshot-based; this is not an event-sourcing/database migration or a general revision/CAS protocol. Legacy full-replacement APIs remain.
  • Unknown external side effects are not automatically retried. A local checkpoint cannot guarantee exactly-once execution for arbitrary shell/MCP actions.
  • Ordinary cancellation does not blindly drop already executing side-effecting tools; they finish or cooperate with cancellation.
  • Full sub-agent transcripts remain unpersisted. Structured final outcomes and captured child-tool activity are retained instead.

The runtime hands its persistence the nodes and journal entries that
changed since the last checkpoint instead of a full copy of the session.
Conversation tracks edited nodes, ToolJournal tracks recorded entries, and
the session store merges the delta by id. This removes the per-checkpoint
JSON round-trip of every tool result, the dead linear-history copy, and
the metadata-notifying persistence wrapper: the manager publishes the
session metadata itself after the merge.
Read-only tools no longer write a started record before running; a
missing record means the call did not happen and repeating it is
harmless. The batch-wide not-started records are gone for the same
reason. A completion is committed with one checkpoint that also carries
what the hooks derived from it.
…d tokens

The runtime races its own token against the permission check; the
mediator and ToolPermissions no longer carry a copy of it, and the
mediator's guard settles a dropped prompt. Sub-agents get a child token
that is cancelled with the parent run, which replaces the propagation
task and the double select.
…terruption

The dispatcher spawns one task per lane that runs its commands in
arrival order and plain tasks for read-only queries; the select loop with
its queue map and join set is gone, and so is the block_on-inside-
spawn_blocking detour. Run preparation is still abandoned on stop, but
the commit under the manager lock now runs to completion.
…ings

clear_context wiped the in-memory conversation without persisting it, and
the next message reloaded the session from disk, so the history came
back. History must never be lost anyway: /clear now creates a new session
that inherits project, worktree, model, sandbox policy and permission
tier, and the terminal switches to it.
MockLLMProvider can replay its responses through the streaming callback
and hand out a client factory; PendingLLMProvider replaces the three
never-answering stubs. The scripted, waiting, failing and two-call
providers the new tests had defined for themselves are gone, and the test
modules read like the rest of the code base.
…eeds

message_contains_invocation existed only for the history normalization
that deleted dangling tool calls on load; restores keep them now.
@stippi
stippi merged commit 731392f into main Sep 15, 2026
9 of 10 checks passed
@stippi
stippi deleted the refactor/session-runtime-consistency branch September 15, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant