Skip to content

feat(acp,desktop): resume interrupted turns after app restart - #2177

Open
wpfleger96 wants to merge 11 commits into
mainfrom
duncan/acp-resume-ledger
Open

feat(acp,desktop): resume interrupted turns after app restart#2177
wpfleger96 wants to merge 11 commits into
mainfrom
duncan/acp-resume-ledger

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Jul 20, 2026

Copy link
Copy Markdown
Member

Summary

Managed agents lose in-flight work when the app restarts mid-turn. This adds a durable pending-turn ledger to the buzz-acp harness so an interrupted turn is replayed on the next boot, plus a per-agent Desktop toggle so the behavior can be turned off for a given agent.

Harness (crates/buzz-acp)

  • ledger.rs mirrors the in-memory EventQueue to disk after every queue mutation, persisting through a temp file plus rename.
  • Boot recovery in lib.rs: staged ledger load → membership gate → TTL expiry → chunked REST fetch with per-event reconciliation (signature/id/channel verification) → ordered queue import → unresolved ordering-barrier registration → one atomic commit.
  • A dedicated tokio::time::Sleep arm in the main select! loop fires at the earliest unresolved barrier deadline, so the barrier timeout is a real bound even on a completely quiet harness.
  • Live-admission seam: recovered events resolved over WebSocket bypass the steer/interrupt path and dispatch with recovery framing; a suppression set prevents duplicate processing across boot fetch and live delivery.
  • Shutdown-drain completion classifies results through complete_batch + ledger sync at both grace-period drain sites; the join-error arm applies a queue-only panic disposition.
  • queue.rs gains complete_batch (atomic payload-owning completion), shared enforce_cap, import_recovered/admit_recovered, set_unresolved_barrier/next_unresolved_barrier_deadline, and dirty_channels tracking.
  • Membership removal purges the removed channel's durable state completely: drain_channel drops the in-flight batch's recovery mirror alongside the queued events, so the trailing ledger sync cannot re-persist a trigger Ledger::invalidate_channel just purged. in_flight_channels/in_flight_deadlines stay preserved so the live task still completes or expires.
  • config.rs gains --resume-on-restart / --resume-ttl-secs, with BUZZ_ACP_RESUME_ON_RESTART as the env equivalent. Resume is on by default.
  • format_prompt emits a recovery header and per-event [restart recovery] markers for recovered batches.
  • The windows-rust CI job runs cargo test -p buzz-acp ledger::, giving the temp-file-plus-rename persist path Windows-native coverage instead of relying on POSIX rename semantics.
  • When resume_on_restart is false, boot best-effort deletes the ledger file so toggling off truly drops pending work — a future re-enable starts clean.

Desktop toggle

  • ManagedAgentRecord.resume_on_restart defaults to true via serde, so existing stores deserialize unchanged and no migration is needed.
  • BUZZ_ACP_RESUME_ON_RESTART is reserved (strips from user env-var editor and on-disk records) and emitted unconditionally from spawn so the UI toggle is the sole control plane in both directions — no ambient parent-process leak when ON, no user env override when OFF.
  • The field participates in spawn_hash, so flipping it raises the restart-required badge rather than leaving a stale process running the old setting.
  • "Resume interrupted turns" switch in the Advanced section of the agent Edit dialog, persisted through the standalone set_managed_agent_resume_on_restart command in the same shape as the auto-restart toggle.

Dialog routing fix

The agent-profile pencil path (handleEditAgent) previously short-circuited to the definition dialog for any definition-backed agent, making all instance-level settings unreachable from the UI for agents created via the create flow. The pencil now unconditionally opens the instance-edit dialog, which already handles linked agents (inherited values shown, "edit linked definition" escape hatch wired). The separate handleEditPersona callback retains the definition-edit surface for surfaces that explicitly target it.

Comment blocks in the touched desktop files are condensed so each file stays within the differential file-size ratchet.

Deferred follow-ups

These were raised as MINOR findings and are non-blocking per reviewer sign-off. Each has a clear fix shape and bounded scope.

  • Boot recovery gated on membership, not subscription scope (CLI-only). When multiple --channels are passed, boot_recover only attempts recovery for channels whose membership the harness fetched at startup. A channel added to the CLI args but not yet joined (e.g. a new invite) may have ledger records that silently skip recovery. Fix shape: intersect the staged ledger's channel set with the --channels subscription set rather than the membership set. Does not affect the Desktop path, which always passes the full subscribed channel list.
  • Permanently unfetchable unresolved records re-arm the 60s ordering barrier each boot. If a ledger entry's event can never be fetched (deleted from relay, bad signature), it stays in the unresolved set indefinitely and the ordering barrier fires on every restart. Fix shape: track per-record fetch-attempt count; after a configurable cap (e.g. 3), classify the record as permanently unresolvable and drop it from the ledger.

@wpfleger96
wpfleger96 requested a review from a team as a code owner July 20, 2026 16:29
@wpfleger96
wpfleger96 force-pushed the duncan/acp-resume-ledger branch from 8d72643 to a17863c Compare July 20, 2026 16:54
@wpfleger96
wpfleger96 marked this pull request as draft July 20, 2026 16:57
Comment thread .github/workflows/signed-macos-canary.yml Fixed
@wpfleger96
wpfleger96 marked this pull request as ready for review July 28, 2026 18:59
@wesbillman

Copy link
Copy Markdown
Collaborator

Thanks for the substantial work on recovery and persistence here. I reviewed the current head (82520f6fbb0a68307e15e32bf40abe78827713c0) and found two correctness issues that need to be fixed before this is safe to merge:

1. Membership removal can re-persist an in-flight batch after invalidation

In crates/buzz-acp/src/lib.rs, the membership-removal path drains the channel and calls ledger.invalidate_channel(ch), but the later sync_dirty can write the channel back immediately:

  • drain_channel deliberately preserves the channel's in-flight state.
  • recoverable_triggers includes in_flight_batch_triggers.
  • The subsequent ledger sync therefore includes the trigger that was just invalidated.

If the harness crashes before that prompt completes, a later boot after the agent is re-added can recover work that membership removal intended to purge.

Please make invalidation win over the dirty sync—for example, consume/sync the dirty queue state first and perform channel invalidation as the final persistence operation, or introduce a channel-removal transition that excludes/removes that channel's in-flight recovery triggers.

Please also add a regression test that dispatches a batch, removes membership while the batch is in flight, immediately reloads the ledger, and verifies that the channel is absent.

2. Replacing an existing ledger is not portable to Windows

crates/buzz-acp/src/ledger.rs currently persists through a fixed temporary sibling followed by:

std::fs::rename(&tmp_path, path)

On Windows, std::fs::rename fails when the destination already exists. The first persist creates the ledger, but later updates cannot replace it, leaving stale recovery state on disk.

Please use a cross-platform atomic-replacement implementation. Deleting the destination before renaming would not be sufficient because it creates a crash window with no ledger.

Please add Windows-executed coverage that persists snapshot A, persists a changed snapshot B to the same path, reloads it, and observes B. The current Windows CI lane compiles buzz-acp but does not execute this filesystem behavior.

The delta since the originally reviewed head 53392cc55e31e251aceb96ea33aedd51520e9ec3 only deletes tests; production code is unchanged, so both issues remain at the current head. The branch is also currently conflicting with main. Once the two fixes and discriminating regression tests are in place, please refresh from main and run fresh CI; the follow-up review can then focus on that delta.

@wpfleger96
wpfleger96 force-pushed the duncan/acp-resume-ledger branch from 82520f6 to 9ba32db Compare July 28, 2026 21:59
@cameronhotchkies cameronhotchkies added the triage-ready Appropriate for agentic review label Jul 30, 2026
@wpfleger96 wpfleger96 changed the title feat(buzz-acp): durable pending-turn ledger for agent auto-resume after app restart feat(acp,desktop): resume interrupted turns after app restart Jul 30, 2026
…er restart

Managed agents lost any in-flight or queued turn when the app restarted:
the event queue lived only in memory, so a mention being worked on at
shutdown was silently dropped and never answered.

Adds `ledger.rs`, a durable pending-turn mirror written after every queue
mutation, and a boot-recovery pipeline in `lib.rs`: staged load, membership
gate, TTL expiry, chunked REST fetch with per-event signature/id/channel
reconciliation, ordered queue import, unresolved ordering barrier, then one
atomic commit. Recovered turns re-run whole (there is no mid-turn
checkpoint) and carry a `[restart recovery]` marker so the agent re-reads
the thread before redoing work.

Exactly-once is enforced on two fronts: a suppression set keyed by event id
prevents the boot fetch and live WebSocket delivery from both admitting the
same event, and events whose REST fetch failed stay unresolved behind an
ordering barrier with a real deadline — a dedicated timer arm in the main
`select!` loop fires it even on a completely quiet harness, so the timeout
is a bound rather than a hope.

`queue.rs` gains payload-owning `complete_batch`, shared `enforce_cap`,
`import_recovered`/`admit_recovered`, barrier registration and
`dirty_channels` tracking; shutdown-drain now classifies results through
`complete_batch` plus a ledger sync at both grace-period sites so a turn
completed during drain is not resurrected on the next boot. Off-by-default
paths are opt-out via `--resume-on-restart`/`--resume-ttl-secs`.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Membership removal runs drain_channel -> Ledger::invalidate_channel ->
sync_dirty. drain_channel preserved in_flight_batch_triggers along with the
liveness tables, but recoverable_triggers reads that mirror, so the trailing
sync re-persisted the trigger invalidate_channel had just purged. A crash
before the prompt completed, followed by a re-add, resurrected discarded work.

drain_channel now drops the mirror as part of the removal transition, which
makes the trailing sync write the channel empty in one shot. Reordering the
two persists instead would leave a crash window between them. in_flight_channels
and in_flight_deadlines stay preserved: removing either would disable
auto-expiry and wedge the channel behind a task that never completes.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The ledger persists by writing <path>.tmp and renaming it over the live file.
That rename's destination-exists case had no coverage, and the windows-rust CI
job only tested buzz-dev-mcp, so no Windows runner ever executed the ledger's
filesystem paths. std::fs::rename passes MOVEFILE_REPLACE_EXISTING on Windows,
but that is worth proving on the platform rather than reading from std.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The harness reads BUZZ_ACP_RESUME_ON_RESTART to decide whether to replay its
pending-turn ledger on boot, but the only way to turn it off was the raw
per-agent env-vars editor. Surfacing it as a first-class field makes the
behavior discoverable and lets two agents be A/B tested against each other.

The env var is emitted only when the toggle is off, so the harness default
stays authoritative for every existing agent and the serde default keeps
upgraded stores on. The field is hashed into the spawn config so flipping it
raises the restart-required badge instead of leaving a stale process.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The stateless file-size ratchet added in #3352 lets no already-over-limit file
grow, and a mandatory ManagedAgentRecord field adds one line to every struct
literal. Reclaimed the delta by condensing verbose comment blocks in the same
files, keeping every fact each block carried.

One reclaim is a correction rather than a reflow: the BUZZ_ACP_SETUP_PAYLOAD
note claimed the reserved-key strip depends on running after the user-env
write, but guard 1 (RESERVED_ENV_KEYS) is order-independent, so the ordering
caveat was misleading.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The pencil-path handleEditAgent in UserProfilePanel was routing
definition-backed agents to the persona/definition dialog, making all
instance-level settings — including the resume-on-restart toggle and the
pre-existing auto-restart-on-config-change switch — unreachable for any
agent created via the create flow.

Drop the resolvedPersona short-circuit in handleEditAgent so the pencil
always opens AgentInstanceEditDialog. The instance dialog already handles
linked agents correctly: inherited values are shown, and an 'edit linked
definition' escape hatch (onEditLinkedPersona) is wired at the call site.
The separate handleEditPersona callback (passed as a distinct prop) retains
the definition-edit surface for surfaces that explicitly target it.

Add a regression test in agentDialogRouting.test.mjs asserting that a
persona-linked agent (personaId present) routes to AgentInstanceEditDialog
via mode="instance-edit", not to AgentDefinitionDialog.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Main's d40a332 (install ceiling PR #3368) grew types.rs by +5 lines
after the 8c87f57 paydown, pushing the merge-commit count to 1005
(limit 1000). Reclaim 7 readability-neutral lines from doc comment
blocks in ManagedAgentRecord and ManagedAgentSummary — three field
comments reflowed to eliminate wrapped-word lines
(turn_timeout_seconds, model, provider, persona_source_version) and
two pairs condensed in the Summary section (agent_command_override,
mcp_command/turn_timeout_seconds). No semantic change.

Simulated merge count: 998 (verified via git merge-tree --write-tree
origin/main HEAD).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…by state

When a managed instance exists, the profile-panel Edit pencil opens the
instance-edit dialog (AgentInstanceEditDialog) — giving definition-backed
agents access to all instance-level settings, including the
resume-on-restart toggle and auto-restart-on-config-change switch. When
only a persona/definition exists with no running instance yet, it falls
back to the definition editor so Edit remains usable for unstarted
definitions.

The prior fix (decdb3e) unconditionally called setEditAgentOpen(true)
regardless of whether a managed instance existed. Since the instance dialog
mounts only when canEditAgent && managedAgent (UserProfilePanel.tsx:892),
clicking Edit on a definition-only profile became a no-op — nothing opened.

Test changes:
- e2e: replace the stale single-case spec (which asserted persona-linked
  agents opened the definition editor) with two discriminating cases:
  (1) persona-linked managed agent → edit-agent-dialog opens (not persona-dialog)
  (2) definition-only/unstarted persona → persona-dialog opens (not nothing)
- agentDialogRouting.test.mjs: remove the non-discriminating pass-3 test
  that called AgentDialog({ mode: 'instance-edit' }) directly — it never
  exercised handleEditAgent and passed on the pre-fix code unchanged.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… routing

Cases 10 and 11 in global-agent-config-screenshots.spec.ts each created a
persona-linked managed agent and clicked the profile Edit pencil, then asserted
persona-dialog opened directly. Under the corrected routing (managedAgent →
instance editor first), that click now opens edit-agent-dialog instead.

Follow the new supported flow in both cases: assert edit-agent-dialog is
visible, click the linked-definition escape ("Edit avatar") inside the
instance editor, then retain all original persona-dialog and definition-field
assertions unchanged (display-name seed, Save changes submit text, provider
picker visibility/enabled state, no submit-block reason).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 force-pushed the duncan/acp-resume-ledger branch from 59bb02b to 5d109fe Compare July 31, 2026 14:31
wpfleger96 and others added 2 commits July 31, 2026 17:43
Two control-plane bugs fixed:

1. BUZZ_ACP_RESUME_ON_RESTART was not reserved, so a saved user env var
   or an ambient parent-process value could silently override the UI
   toggle. The key is now in RESERVED_ENV_KEYS (stripped at save time
   and at spawn time from merged_user_env) and emitted unconditionally
   from spawn — the desktop writes the record value in both directions
   so the running agent always matches the UI-visible setting.

2. Ledger::disabled() left the ledger file untouched on disk, so turning
   the toggle off didn't actually drop pending work — a later re-enable
   would resume a stale turn from a prior session. Boot now calls
   delete_ledger_file (best-effort, silent NotFound) before constructing
   the disabled ledger, making "off" mean off.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The disabled-ledger deletion was unreachable when BUZZ_ACP_SETUP_PAYLOAD
triggered the early setup-listener return. A resume-off not-ready spawn
left the ledger file on disk; re-enabling later resurrected stale turns
despite the UI promise that disabled turns are dropped and never picked up.

Move the best-effort delete_ledger_file call to immediately after
Config::from_cli() — before the setup-payload branch — so the invariant
holds for every operational mode. The later disabled arm now only
constructs Ledger::disabled() (deletion already done).

For the ambient-env precedence seam: extract the env_remove + unconditional
env pair into apply_resume_env() in env_vars.rs; spawn_agent_child calls
it via super::env_vars::apply_resume_env, and the two ambient-precedence
tests route through the same production function. Add an integration test
(setup_mode_ledger.rs) that seeds the ledger file, spawns the binary with
resume=false + BUZZ_ACP_SETUP_PAYLOAD, and asserts the file is gone —
discriminating against a helper-only test that passes even when the
call site in lib.rs is unreachable.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-ready Appropriate for agentic review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants