fix(channels,kernel): scope /new to the calling channel + purge JSONL on delete (#4868)#4905
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
houko
force-pushed
the
fix/4868-channel-new-scope
branch
from
May 11, 2026 09:18
fb3e4d2 to
84bd626
Compare
houko
added a commit
that referenced
this pull request
May 11, 2026
…4868) Address review feedback on #4905: - `reset_all_sessions` previously held only `agent_msg_locks[agent_id]`, but `send_message_full` / streaming acquire EITHER that or `session_msg_locks[sid]` depending on `session_id_override` (#2959). A concurrent multi-tab WS or scoped HTTP turn could finish after the delete and UPSERT its session row back into existence. Snapshot the live sids under the agent lock, then acquire every per-session lock (sorted) and hold all guards through the delete + recreate. - Tighten the lock-order comment in `reset_one_session`: send_message_full is XOR (one lock or the other), not both — so the agent-then-session order here cannot cycle. - Map `LibreFangError::InvalidInput` from `reset_session` / `reboot_session` route handlers to HTTP 400 instead of the catch-all 500, matching the parallel branch in `update_schedule` (#4732). Today only forward-compat (callers always pass `ResetScope::Agent`); arms ready for a scoped admin endpoint. - info!() in both `reset_all_sessions` and `reset_one_session` now includes `op = "reset" | "reboot"` so log readers can tell the two paths apart without inspecting `save_summary`.
…NL on delete (#4868) Channel `/new`, `/reboot`, `/compact` previously called `kernel.reset_session(agent)` without a scope, which deleted EVERY session the agent owned (default + every per-channel sid + cron-spawned ones). Typing `/new` in Telegram silently nuked the dashboard transcript and every other channel's transcript too. Same code path also leaked the on-disk transcript: `delete_agent_sessions` clears `sessions` + `sessions_fts` rows transactionally but never touches the `<workspace>/sessions/{sid}.jsonl` mirror, so every reset accumulated orphan files indefinitely. This change introduces a `ResetScope { Agent, Session(SessionId) }` arg on `reset_session` / `reboot_session` so the channel layer can ask the kernel to delete exactly one sid (the one derived from `(channel, chat_id)` via `SessionId::for_sender_scope`), and adds best-effort JSONL cleanup to all three session-delete paths (`reset_session`, `reboot_session`, `clear_agent_history`). Behaviour summary: - Channel `/new` now resets only the per-channel session; sibling sessions on other surfaces are untouched. The session is recreated eagerly at the same deterministic sid so external `SessionStart` / `SessionReset` hooks fire symmetrically and the next inbound message lands on a fresh empty row. - Per-session reset does NOT clear agent-wide quota state (no grief vector for one channel to clear an agent-wide token-quota cap). - Cross-agent sids are rejected with `InvalidInput` instead of being silently deleted (defence-in-depth on the kernel boundary). - `POST /api/agents/{id}/session/reset`, `…/reboot`, the dashboard WebSocket `reset` / `reboot` commands, and `clear_agent_history` keep the historical agent-wide semantics by passing `ResetScope::Agent`. - All three SQL-delete paths now also remove the `<workspace>/sessions/{sid}.jsonl` mirror. Failures are logged with the orphan path but not propagated — SQL is the source of truth, leaving a ghost file is hygiene not privacy. Concurrency (review fix): - All three reset paths (reset / reboot / clear_agent_history) are now `async fn` and acquire the same per-agent / per-session message lock that `send_message_full` holds. Without this, an in-flight turn's later `save_session` UPSERT would silently revive the just-deleted row, defeating the entire fix. `reset_one_session` takes `agent_msg_locks[agent]` then `session_msg_locks[sid]` (same order as `send_message_full`'s lock acquisition — no deadlock can form). Single source of truth for the scope formula (review fix): - The channel-scope formula previously lived inline in four places: `kernel/messaging.rs::send_message_full`, `kernel/agent_execution.rs`, `kernel/mod.rs::resolve_dispatch_session_id`, and the channel-bridge reset helper. Any one drifting from the others silently regresses #4868. Extracted to `SessionId::for_sender_scope(agent, channel, chat_id)` in `librefang-types` and pinned via a unit test. Tests: - `crates/librefang-kernel/tests/session_reset_scope_test.rs` — 7 tokio integration tests: * per_session_reset_only_deletes_targeted_sid * agent_wide_reset_purges_all_sessions_and_jsonl * per_session_reset_rejects_cross_agent_sid * per_session_reset_on_unmaterialised_sid_is_ok * per_session_reboot_skips_summary_save * clear_agent_history_purges_jsonl_too * compact_with_id_targets_the_requested_session - Unit test `for_sender_scope_matches_legacy_inline_formula` in `librefang-api/src/channel_bridge.rs` pins the helper's output against the pre-refactor inline formula so the four kernel call sites can't drift. Out of scope (deferred per issue's own #3 follow-up): a `librefang sessions gc` admin tool to reconcile already-orphaned JSONL files on existing deployments. The new code stops accumulating orphans going forward; the backfill is a separate environmental cleanup.
…4868) Address review feedback on #4905: - `reset_all_sessions` previously held only `agent_msg_locks[agent_id]`, but `send_message_full` / streaming acquire EITHER that or `session_msg_locks[sid]` depending on `session_id_override` (#2959). A concurrent multi-tab WS or scoped HTTP turn could finish after the delete and UPSERT its session row back into existence. Snapshot the live sids under the agent lock, then acquire every per-session lock (sorted) and hold all guards through the delete + recreate. - Tighten the lock-order comment in `reset_one_session`: send_message_full is XOR (one lock or the other), not both — so the agent-then-session order here cannot cycle. - Map `LibreFangError::InvalidInput` from `reset_session` / `reboot_session` route handlers to HTTP 400 instead of the catch-all 500, matching the parallel branch in `update_schedule` (#4732). Today only forward-compat (callers always pass `ResetScope::Agent`); arms ready for a scoped admin endpoint. - info!() in both `reset_all_sessions` and `reset_one_session` now includes `op = "reset" | "reboot"` so log readers can tell the two paths apart without inspecting `save_summary`.
houko
force-pushed
the
fix/4868-channel-new-scope
branch
from
May 11, 2026 09:56
8224217 to
92344f4
Compare
This was referenced May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4868.
Summary
/new,/reboot,/compactpreviously deleted every session for the agent (default + every per-channel sid + cron-spawned). Typing/newin Telegram silently nuked the dashboard transcript and every other channel's transcript.delete_agent_sessionsclears SQL rows but never touched<workspace>/sessions/{sid}.jsonl, so every reset accumulated orphan transcripts indefinitely.ResetScope { Agent, Session(SessionId) }arg onreset_session/reboot_session, and adds best-effort JSONL cleanup to all three session-delete paths (reset_session,reboot_session,clear_agent_history).Behaviour change matrix
/new,/reboot,/compactSessionId::for_channel(agent, "<ch>:<chat>"); siblings untouchedPOST /api/agents/{id}/session/reset,…/rebootResetScope::Agent) — preservedreset/rebootcommandsDELETE /api/agents/{id}/history(clear_agent_history)Subtleties
SessionStart/SessionResethooks fire symmetrically (lazy creation in the agent loop only fires the internalSessionLifecycleEvent, not the external hook)./new).InvalidInputinstead of silently deleted (defence-in-depth — callers compute the sid from a trusted(channel, chat)pair, but the typed argument itself isn't).warn!so admins can clean up.API changes
librefang_types::agent::ResetScope { Agent, Session(SessionId) }.KernelHandle::compact_agent_session_with_id(agent, Option<SessionId>)— promoted from inherent method.ChannelBridgeHandle::{reset,reboot,compact}_channel_session(agent, channel, chat_id). Default impls returnErr("Not implemented")matching the trait's existing pattern.KernelHandle::{reset,reboot}_sessionnow takes aResetScopearg.Verification
cargo check --workspace --lib— clean.cargo clippy -p librefang-types -p librefang-kernel -p librefang-channels -p librefang-api --all-targets -- -D warnings— clean.cargo test -p librefang-kernel --test session_reset_scope_test— 4 new tests:per_session_reset_only_deletes_targeted_sid— telegram sid wiped, dashboard + slack survive (SQL + JSONL).agent_wide_reset_purges_all_sessions_and_jsonl— every sid + every JSONL gone.per_session_reset_rejects_cross_agent_sid— defence-in-depth.per_session_reset_on_unmaterialised_sid_is_ok—/newtyped in a fresh chat that has never received a message.cargo test -p librefang-api --lib channel_session_id_matches_inbound_resolver_formula— pins the channel-side sid derivation to the inbound resolver's formula so they cannot drift.cargo test -p librefang-api --test agents_routes_integration— 19 existing agent-route tests still pass.cargo test -p librefang-types -p librefang-kernel -p librefang-channels --lib— 2086 existing lib tests still pass.Out of scope
Per the issue's own #3 follow-up: a
librefang sessions gcadmin tool to reconcile already-orphaned JSONL files on existing deployments. The new code stops accumulating orphans going forward; the historical backfill is a separate environmental cleanup that would expand scope intolibrefang-cliand is best tracked separately.Test plan
/newdeletes only the targeted sid (kernel test)/newworks (kernel test)