feat(buzz-acp): give each channel thread its own agent session - #6732
feat(buzz-acp): give each channel thread its own agent session#6732salman1993 wants to merge 12 commits into
Conversation
Lay the foundation for thread-scoped ACP sessions (rollout step 1: land behind the operator policy with channel scope as the fallback). - Add `scope` module with a hashable `SessionScope` (Conversation/Thread) and `SessionPolicy` (channel/thread). Scope is derived once at admission from policy + DM status + NIP-10 thread tags using the shared `buzz_core::nip10` canonical-root rules. DMs are always conversation-scoped; under the default `channel` policy every channel event collapses to a conversation scope, preserving today's behavior. - Add `--session-policy` / `BUZZ_ACP_SESSION_POLICY` (default `channel`) wired through `CliArgs` -> `Config` and the config summary; document it in `.env.example`. - Derive and log the resolved scope at event admission (telemetry only for now). Unit tests cover scope derivation for top-level mentions, direct/nested replies, repeated mentions, DMs, malformed thread tags, hashing/map-key use, and policy parsing/defaults. Follow-up (same ticket, later commits): partition queue/in-flight state by scope (step 2), partition provider-session state by scope (step 3), scope-correct context gathering (step 4), and the Settings->Experiments toggle + managed-agent deployment wiring (step 5). Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…step 2)
Carry the admission-time SessionScope through the queue instead of keying
everything on channel_id. All EventQueue partitions — pending queues,
in-flight tracking, deadlines, retry counters/backoff, cancelled-batch
carryover, and the goose-native steer side table — are now keyed by
SessionScope. Under the default `channel` policy every scope is
Conversation{channel_id}, so behavior is byte-identical; under `thread`
policy, distinct canonical roots in one channel become independent
partitions.
- QueuedEvent and FlushBatch carry the resolved SessionScope; dispatch,
completion, and requeue route by it. PromptSource::Channel now carries
the scope (with a channel_id() accessor) so a completed turn marks the
exact scope complete. No code rederives scope from the last event in a
batch.
- The mid-turn steer gate is now scope-level (is_scope_in_flight), and the
native-steer withhold/release/dedup + deadline extension target the
scope, so an unrelated thread's in-flight turn never steers this one.
- drain_channel performs channel-wide cleanup across every child thread
scope. Backlog protection is preserved with a per-scope cap plus an
aggregate per-channel cap, so per-thread partitioning cannot multiply the
admitted queue size.
- An IntoScope helper lets the queue API accept a bare channel Uuid
(conversation scope) or an explicit SessionScope, keeping the existing
channel-keyed unit tests intact.
Pool provider-session STATE is still channel-keyed in this commit (indexed
via scope.channel_id()); step 3 rekeys it by scope so repeated activity in
a thread reuses exactly that thread's provider session.
New queue tests: two threads in one channel are independent partitions,
events from different roots never share a batch, an in-flight scope blocks
only that scope, channel drain clears every child thread scope, and the
aggregate channel cap is not multiplied by threads. Full buzz-acp suite
(819 lib + integration) green; clippy and fmt clean.
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…p 3) Key the pool's provider-session state by SessionScope instead of only channel_id, so repeated activity in a thread reuses exactly that thread's provider session and unrelated threads in one channel never share session state, turn counters, context-delivery markers, or delivery-dedup state. - SessionState maps (sessions, turn_counts, core_sections, canvas_sections, deliveries) are now keyed by SessionScope. run_prompt_task resolves the session, core/canvas sections, standing-context-sent marker, turn count, and delivery ledger by the batch's scope; channel-level fetches (canvas, huddle, title, resolve) still use scope.channel_id(). - Scope-to-worker affinity: has_session_for/try_claim match the exact scope, so a temporarily busy worker cannot cause another worker to open a duplicate session for the same thread. TaskMeta carries the scope; send_steer and record_successful_steer route by it. - Channel-wide cleanup preserved: invalidate_channel clears every child thread scope for a channel (returns the count); invalidate_channel_sessions and the removed-channel path use it. Added invalidate_scope for single-session invalidation and mark_scope_delivery_success. - Model-switch targeting stays channel-level with an explicit TODO for channel-vs-thread control targeting (same open question as top-level !cancel / !rotate, deferred per the ticket). New pool tests: two threads in one channel get distinct sessions and reuse per root, invalidate_scope leaves a sibling thread untouched, and invalidate_channel clears every thread scope while sparing other channels. Full buzz-acp suite (822 lib + integration) green; clippy and fmt clean. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Drive conversation-context gathering from the batch's resolved SessionScope instead of re-inferring it from whichever event is last in the batch. - A Thread scope fetches only that canonical thread's history (all messages under the root, including intervening non-mention human messages), so no unrelated channel transcript is injected. A brand-new thread's first turn has no prior history — the trigger is delivered as the [Event] block. - Conversation scope preserves current behavior: DMs (and legacy channel-policy channels) fetch the reply chain for a threaded reply or recent DM history for a non-reply; a plain top-level channel message gets no supplementary context. - The existing scope-keyed delivery-delta filter (step 3) then strips events this session already received, so subsequent turns deliver only the intervening same-thread messages plus the trigger, without duplication. The routing decision is extracted into a pure `resolve_context_target` so it is unit-tested directly: thread scope wins over a divergent last-event tag, a new top-level thread resolves to its own root, a plain conversation-scope channel message gets no context, DM non-reply fetches DM history, and a conversation-scope reply uses its reply chain. Together with steps 1–3 this makes BUZZ_ACP_SESSION_POLICY=thread deliver real end-to-end isolation: distinct roots in one channel get distinct queue partitions (step 2), distinct provider sessions with scope-keyed worker affinity (step 3), and distinct canonical-thread context (this step); while the default `channel` policy is byte-identical to prior behavior. Full buzz-acp suite (827 lib + integration) green; clippy and fmt clean. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
|
🤖 Request changes at exact head Major — equivalent NIP-10 root spellings still split one relay thread into separate ACP sessions
Concrete scenario: a valid reply tags root Normalize validated marker IDs before constructing the key (or key on Verification
|
…re one scope The shared NIP-10 parser accepts and preserves uppercase ASCII hex in `e`-tag marker ids (`is_ascii_hexdigit`), but the relay decodes event ids to bytes on ingest — so a reply tagging its root as `AB…` and one tagging `ab…` are the SAME accepted relay thread. `SessionScope::Thread` keyed on the raw string, so under thread policy those equivalent spellings hashed to different keys and split one thread across two ACP sessions (queue partitions, provider sessions, worker affinity, and delivery ledgers), violating same-root reuse. Normalize the resolved root id to lowercase in `SessionScope::derive` before it becomes the scope key. `nostr::EventId::to_hex()` is already lowercase, so the top-level-mention path is unaffected. Adds a mixed-case regression test. Reported in review of PR #6732. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
|
🤖 Re-review: ready at exact head The prior mixed-case NIP-10 blocker is resolved. Fresh verification at this SHA:
No actionable findings remain. DMs were not re-exercised live in this pass. |
…ting Addresses three correctness gaps found in review of PR #6732. 1. Duplicate provider session for a busy thread (pool.rs, lib.rs). Worker affinity only scanned idle slots, so while the worker that owns a thread's session was checked out, a new message for that thread could be handed to another idle worker, forking a second session and splitting the thread's history/tool context. Added an authoritative `SessionScope -> worker` directory (`session_owners`) that survives while a worker is checked out. `dispatch_pending` now holds a batch (leaves it queued) when its session owner is busy, instead of forking a duplicate; the held batch dispatches to that exact worker when it returns. The directory is pruned on channel-wide session invalidation, and stale entries (rotation / crash) self-heal on the next dispatch. 2. Mid-turn steer/interrupt could target the wrong thread (lib.rs). The native-steer fallback and the steer-ack fallback routed by channel via `signal_in_flight_task`, which picks the first task for the channel — so a message in thread A could interrupt thread B in the same channel. Added `signal_in_flight_task_for_scope` (exact `SessionScope` match) and used it for both mid-turn fallbacks. The deferred channel-level control paths (`!cancel`, `!rotate`, observer `cancel_turn` / `switch_model`) keep channel-targeting intentionally. 3. Panicked thread stayed "in flight" (lib.rs). `recover_panicked_agent` called `mark_complete(channel_id)`, which resolves to `Conversation(channel_id)` via IntoScope and, under thread policy, left the real `Thread(...)` entry wedged in-flight until the ~2h backstop — blocking the batch it had just requeued. It now uses `meta.scope`. Tests: scope-exact signalling targets only the matching thread; a busy session owner holds the batch instead of forking a session (and the directory prunes on channel invalidation); panic recovery frees the exact Thread scope and requeues its batch. Full buzz-acp suite (831 lib + integration) green; clippy and fmt clean. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…cp-sessions * origin/main: (31 commits) fix(desktop): stop pulsing addressed agents on send (#6873) fix(desktop): prioritize sidebar channel status (#6861) feat(desktop): hyperlink selected composer text on link paste (#6684) chore(release): release Buzz Desktop version 0.5.20 (#6839) feat(desktop): add KLIPY GIF search to composers (#5554) fix(desktop): respect automatic mention preference after send (#6837) fix(release): attribute desktop candidates to the operator (#6831) fix(ci): check out source in docker.yml merge job (#6833) chore(release): release Buzz Desktop version 0.5.19 (#6828) Remove public relay signing key fallback (#6729) docs(nest): make commit attribution policy-neutral (#6707) fix(desktop-messages): preserve inline agent mentions with persistent addressing (#6793) Qualify canonical relay images for staged delivery (#6781) feat(desktop): persist agent addressing across composer messages (#6714) feat: navigate images across message threads (#6705) Add database pressure observability (#6700) revert fixed mention highlight (#6716) highlight search terms in results and messages (#6702) fix(desktop): make lightbox zoom controls interactive (#6710) Support community deletion in versioned media buckets (#6738) ... Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…hausted batch `requeue_preserve_timestamps` restored only `batch.events`, silently dropping `batch.cancelled_events` and `cancel_reason`. Both callers pass a full `FlushBatch` that can carry an interrupted turn's original request: - the new busy-owner affinity hold (thread scoping), and - the pre-existing "no agent available" pool-exhausted path. Reachable loss: thread A is interrupted (Buzz retains A's original request to re-prompt "original + follow-up"); older work in thread B grabs A's session-owning worker; A's merged batch is held; only the follow-up was put back and the original request vanished. The helper now restores the entire batch — cancelled carryover is returned to the pending cancelled-batches (ahead of any concurrently staged carryover) with its reason, so the next flush reconstructs the same merged prompt. Adds a queue-level round-trip regression asserting events + cancelled_events + cancel_reason all survive requeue -> mark_complete -> flush. Reported in review of PR #6732. Full buzz-acp suite (832 lib + integration) green; clippy and fmt clean. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
|
Good catch on the |
|
Tested PR head
This validates the harness/session routing. I did not test the full Desktop UI path. |
…cp-sessions * origin/main: fix(desktop): accent-colored mention badges that count thread mentions (#6900) Add Buzz benchmark evaluation layers (#6823) fix(desktop): show edited head content in thread panel (#6887) fix(desktop-tooltip): increase surface contrast (#6897) Deduplicate ACP thread prompt context (#6706) Apply access policy when reusing channel agents (#6838) feat(sidebar): prioritize unread DMs in overflow navigation (#6842) feat(projects): add agent and CLI project-home support (#6590) feat(desktop): restore message quick reactions (#6892) Use paired tags for standing & per-turn context (#6701) fix(cli): preserve signatures in event reads (#6884) refactor(db): finish replaceable event store extraction (#6777) Fix Admin feedback filter overflow (#6825) Signed-off-by: Salman Mohammed <smohammed@squareup.com> # Conflicts: # crates/buzz-acp/src/pool.rs
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8f8808890
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| scope.telemetry_label() | ||
| ); | ||
| agent.state.sessions.insert(*cid, sid.clone()); | ||
| agent.state.sessions.insert(scope.clone(), sid.clone()); |
There was a problem hiding this comment.
Bound retained sessions under thread policy
When BUZZ_ACP_SESSION_POLICY=thread, every sequential top-level mention gets a unique scope and this insertion retains the resulting provider session indefinitely; no queue cap evicts entries from SessionState.sessions. A busy channel can therefore accumulate an unbounded number of ACP sessions even with no queued work, and providers such as buzz-agent spawn a separate MCP registry for every session/new, eventually exhausting processes and memory. Add a bounded per-channel/session eviction policy that also closes or invalidates the provider session.
Useful? React with 👍 / 👎.
| /// Channel-targeted: picks the first in-flight task matching the channel. Used | ||
| /// only by the explicitly-deferred channel-level control paths (`!cancel`, | ||
| /// `!rotate`, observer `cancel_turn` / `switch_model`). For per-thread mid-turn | ||
| /// steering/interruption use [`signal_in_flight_task_for_scope`], which targets |
There was a problem hiding this comment.
Target thread control commands by scope
With thread policy enabled and two threads running concurrently in one channel, !cancel or !rotate still reaches the first task_map entry matching only the channel, whose order is arbitrary. A command posted in thread A can consequently cancel or rotate thread B while leaving A running. Derive the command event's SessionScope before consuming it and use the scope-exact signaling path; channel-wide observer controls should explicitly signal every matching scope if that is their intended meaning.
Useful? React with 👍 / 👎.
🔐 Codex Security Review
|
Two thread-isolation correctness bugs surfaced in review, both only reachable under BUZZ_ACP_SESSION_POLICY=thread; behavior under the default channel policy is unchanged (the scope is the channel's sole conversation). !cancel / !rotate could hit the wrong thread. Both routed through signal_in_flight_task, which matches on channel_id and takes the first HashMap entry — so with two threads running concurrently in one channel an owner's !cancel/!rotate could tear down an arbitrary thread's turn. They now derive the SessionScope from the command event's NIP-10 tags (the same resolver admission uses) and target it via signal_in_flight_task_for_scope, the scope-exact primitive steering already uses. The idle !rotate path now invalidates only that scope via the new AgentPool::invalidate_scope_session instead of the whole channel. signal_in_flight_task is now used only by the desktop observer control frames (cancel_turn / switch_model), which carry a bare channelId and no thread context. Typing state was channel-keyed. typing_channels keyed by channel_id, so two concurrent thread turns overwrote one entry and either turn's completion removed it — the indicator could reflect the wrong thread or stop while a sibling turn continued. It is now keyed by SessionScope: dispatch_pending returns the scope, completion/panic/ownership-removal clear the exact scope (via the new PromptSource::scope accessor), and the refresh loop publishes one indicator per active thread carrying that thread's NIP-10 tags. Tests: invalidate_scope_session targets one thread and drops its owner; PromptSource::scope exposes the thread scope and None for heartbeats. The reviewer's P1 (unbounded session/provider-resource retention) is a pre-existing lifecycle-hardening concern, not thread-scoping-specific, and is tracked separately rather than adding partial eviction here. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…cp-sessions * origin/main: test(db): use canonical channel roster fixtures (#6819) preserve channel description paragraph breaks (#6946) fix(cli): enrich template cardinality error with per-candidate presence and profile hints (#4825) Fix Codex security review authorization (#6913) fix(db): disable heartbeat vacuum truncation (#6898) chore(deps): update rui314/setup-mold digest to 7e4f20a (#6663) chore(deps): update dependency vitest to v4.1.11 (#6667) chore(deps): update dependency @tanstack/react-virtual to v3.14.10 (#6666) chore(deps): update ubuntu:24.04 docker digest to 33ceb71 (#6664) fix(projects): allow owners to delete agent projects (#6533) Fade expanded video controls on hover (#6926) fix(db): exclude kind:30179 ciphertext from brownfield FTS (#6822) fix(client): resurface hidden DMs from live activity (#6885) fix(desktop): keep the draft space when typing right after a mention pick (#6875) broker: define the agent-to-broker action contract (#6742) fix(desktop): keep project sheets independent from threads (#6901) Add gated security reviews (#6816) Signed-off-by: Salman Mohammed <smohammed@squareup.com>
|
Thanks for the review. Addressed in P2 — P2 — channel-keyed typing state (fixed). Both are only reachable under P1 — unbounded session / provider retention (split out). Agreed this isn't a clean PR-local fix — partial eviction of Buzz-side IDs would leak provider resources. It's pre-existing (not thread-scoping-specific) and needs a designed lifecycle: idle-TTL / LRU on the scope maps, a real per-session close contract that tears down |
What this does
In a channel, people often run several unrelated conversations at once (separate threads). Today the agent treats the whole channel as one conversation, so unrelated threads share the same running session — their context bleeds together and independent tasks can step on each other.
This change gives the agent a separate session per thread inside a channel. Direct messages stay as one conversation (unchanged). The channel is still the boundary for who is allowed in and what is visible — only the agent's working context is now split by thread.
How it is turned on
Off by default. Operators opt in with one setting:
BUZZ_ACP_SESSION_POLICY=channel— default, current behaviorBUZZ_ACP_SESSION_POLICY=thread— new per-thread behaviorBeing behind a flag means we can enable it for a few agents, watch how it behaves, and roll back instantly without a code change.
Key design decisions
Bugs found and fixed while iterating (from review)
Not in this PR
Testing
The full
buzz-acptest suite passes (830+ unit and integration tests), plus new focused tests for thread routing, session reuse, interrupt targeting, crash recovery, and request preservation. Behavior with the flag off is unchanged.