Skip to content

fix(acp): preserve resumable sessions across failures - #6990

Open
jimallen wants to merge 4 commits into
block:mainfrom
jimallen:chief/review-pr-6088
Open

jimallen wants to merge 4 commits into
block:mainfrom
jimallen:chief/review-pr-6088

Conversation

@jimallen

Copy link
Copy Markdown

Outcome

Completes and corrects the restart-resume work from #6088 so a channel retains its ACP session—and thread-scoped goals—across harness restarts without silently recording unsupported model or permission configuration as effective.

Supersedes #6088.

Corrections beyond #6088

  • Indeterminate session/load failures never age into receipt deletion or a competing fresh session; explicit rotation remains the abandonment mechanism.
  • Durable receipts record only model and permission configuration actually applied. Rejected or unsupported configuration records the agent default and is not later resumed as though the request succeeded.
  • Structured tracing and documentation match the resulting lifecycle contract.

Verification

  • cargo fmt --all -- --check: passed.
  • cargo test -p buzz-acp: 814 library tests + 9 lifecycle integration tests passed; doc/main tests passed.
  • Full run used the pinned Hermit environment, local loopback access, and removed only managed harness variables that otherwise override two CLI default-value tests.

jhgaylor and others added 4 commits August 17, 2026 22:45
…nel on session/new

A harness restart — a desktop relaunch, a config-change restart, or a hosted
harness moved between nodes on a deploy — emptied the in-memory channel →
session map, and the next mention in every channel got a fresh session/new.
For an agent whose session *is* its workspace (a sandbox per session, as with
the `fountain acp` gateway) that discarded the channel's memory and files each
time; the base prompt tells the agent to "resume silently after a session
restart", which assumes the opposite.

Two changes:

* `SessionStore`: a small JSON file (`<state-dir>/sessions-<pubkey>.json`,
  atomic writes) remembering channel → session id. Before opening a new
  session for a channel with no live one, the pool `session/load`s the
  remembered id when the agent advertised `loadSession` at initialize; any
  failure forgets the id and falls back to `session/new`, so a resume that
  cannot happen never costs the turn. Leaving a channel and `!rotate` forget
  the entry. `--state-dir` / `BUZZ_ACP_STATE_DIR` (default `<cwd>/.buzz-acp`),
  `--no-resume-sessions` / `BUZZ_ACP_NO_RESUME_SESSIONS` to opt out.

* `session/new` now carries `_meta.channelId` and `_meta.channelType`
  (`AcpClient::session_new_with_origin`; `session_new_full` is unchanged and
  delegates). The prompt names the channel only as text; an agent or gateway
  that wants to key sessions by channel — to resume the same conversation
  after this harness restarts even when its own state directory is not
  persistent — had nothing machine-readable to key on. Absent for heartbeat
  sessions.

Tests: store round-trip/malformed/disabled; client sends channelId/type and
merges into existing _meta; loadSession capability recorded; session/load
request shape and tolerance of the pre-response replay; error surfaces;
config defaults and opt-out. Not covered here: a scripted end-to-end through
the pool's get-or-create path (the pool has no scripted-agent test harness
today).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
…container volume note

Reviewers asked for the README configuration table to list the two settings
introduced with session resumption, and for an operator note that the default
<cwd>/.buzz-acp state dir is ephemeral in containers and must be pointed at
mounted persistent storage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
Addresses review feedback on block#6088. The durable channel -> session map was
a plain cache, and several paths that deliberately abandon a session left
the receipt behind for the next turn to resume.

Lifecycle. `invalidate_for_fresh_session` is now the single seam for "the
next turn must not reuse this session": `!rotate` (idle and busy), a model
switch (idle and busy), a token/turn-limit rotation, a cancel, and a failed
cleanup all clear RAM and disk together. Agent death (`invalidate_all`)
deliberately does not — the harness still wants the conversation back when
the agent respawns, and a session the agent truly lost is forgotten by
`session/load` failing. Without this a `!rotate` during an in-flight turn,
or any MaxTokens rotation, resumed the very session that was discarded, and
an idle model switch silently no-oped.

Scope and compatibility. The file records the (relay, agent pubkey) pair it
belongs to and is ignored otherwise, so one agent identity serving several
communities cannot resume across them. Each receipt records the model and
permission mode the session was created under and is refused when either
changed: both are resolved from the `session/new` response, which
`session/load` does not return, so resuming across such a change would
silently run the old configuration. Nothing else is fingerprinted — the
system prompt, team instructions, MCP list and memory are delivered per
turn, and invalidating on those would wipe every channel on a prompt tweak.

Failure classification. A JSON-RPC error is the agent answering, so the
receipt is dropped and a fresh session opened. A timeout, broken pipe, or
mid-load exit is not an answer: the receipt survives and the turn fails
retryably rather than opening a session that could compete with one that
loaded remotely. Bounded at three consecutive attempts.

Stream origin. The resume path passed `None` for channel type, so a resumed
public stream got `BUZZ_GIT_ORIGIN_AGENT_NAME` instead of
`BUZZ_GIT_ORIGIN_CHANNEL_ID` and lost git provenance across a restart. It
now uses the same resolved type as `session/new`.

Store durability. Every write re-reads and merges under the lock, so two
overlapping harnesses cannot lose each other's channels to a stale
snapshot. Directory 0700, file 0600, unique `create_new` temp file, fsync
of file and parent, size and version bounds on parse.

Observer replay. `session/update`s replayed during `session/load` no longer
reach the owner Activity feed as fresh output; everything else on the wire
during a load still does.

Tests. Causal coverage through the real `run_prompt_task` against a
scripted ACP agent and an on-disk store: happy restart (one `session/load`,
no second `session/new`), definitive vs indeterminate failure, a
configuration change refusing the receipt, origin parity between fresh and
resumed, and the lifecycle seam. Verified by mutation — disabling
persistence, resume, origin wiring, or replay suppression each fails a test.

Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
Signed-off-by: Jim Allen <jimallen@gmail.com>
@jimallen
jimallen requested a review from a team as a code owner August 28, 2026 16:03
@github-actions

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 86b9142a09f2af3ba2fff7effa6a6cd53b40f51c...486fa44a66c430e90155fa190c44db583fd072bd.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 486fa44a66c430e90155fa190c44db583fd072bd to authorize a new review.
Any previous review applies only to its recorded range.

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.

2 participants