Fix : agent queue resilience#122
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves resilience and UX around agent-run lifecycle, especially for batch runs and backend restarts, by adding a deterministic “owed agent run” marker, a third boot-time reconcile pass, clearer queued-vs-running UI states, and safer cancellation semantics during deletes.
Changes:
- Backend: add
discussions.awaiting_agentmarker + boot-time reconcile to prevent silent loss of “owed but never started” agent runs, broadcastingagent_runs_interrupted. - Backend: cancel live agent tokens before deleting discussions or batch runs to avoid FK errors and leaked semaphore permits.
- Batch UX: introduce
batch_run_child_queuedWS event and frontendqueuedMaprendering to distinguish queued children from actually running ones.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types/generated.ts | Updates generated WS message union for new event variants. |
| frontend/src/pages/DiscussionsPage.tsx | Adds queuedMap handling and reacts to new boot reconcile WS event. |
| frontend/src/pages/Dashboard.tsx | Lifts queuedMap state to persist across navigation. |
| frontend/src/pages/tests/DiscussionsPage.test.tsx | Updates lifted props to include the new queued state. |
| frontend/src/lib/i18n.ts | Adds i18n strings for “queued” and “agent runs interrupted” toast. |
| frontend/src/components/SwipeableDiscItem.tsx | Renders distinct queued indicator (non-spinner) in the sidebar item. |
| frontend/src/components/DiscussionSidebar.tsx | Threads queuedMap and shows queued/running counts in batch status pill. |
| backend/src/workflows/batch_step.rs | Broadcasts BatchRunChildQueued up-front instead of “started” for all children. |
| backend/src/models/multiuser.rs | Adds new WS variants + serde snake_case round-trip tests. |
| backend/src/main.rs | Adds third boot reconcile phase and broadcasts AgentRunsInterrupted. |
| backend/src/db/workflows.rs | Marks batch child discussions as awaiting_agent at creation time. |
| backend/src/db/sql/074_disc_awaiting_agent.sql | Adds awaiting_agent column + partial index migration. |
| backend/src/db/migrations.rs | Registers migration 074. |
| backend/src/db/discussions.rs | Implements set_awaiting_agent + reconcile_awaiting_agents. |
| backend/src/db/discussions_test.rs | Adds unit tests for reconcile_awaiting_agents. |
| backend/src/api/workflows.rs | Cancels run + child discussion tokens before deleting a batch run (+ tests). |
| backend/src/api/discussions/streaming.rs | Clears awaiting_agent on agent delivery and on agent-start failure persistence. |
| backend/src/api/discussions/messaging.rs | Marks awaiting_agent before spawning agent for send_message. |
| backend/src/api/discussions/crud.rs | Cancels a discussion’s live agent token before deletion (+ tests). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
backend/src/workflows/batch_step.rs:216
- T3 says the up-front fan-out should emit
BatchRunChildQueued(and reserveBatchRunChildStartedfor the real per-child start). Butbackend/src/api/mcp_remote.rsstill broadcastsBatchRunChildStartedfor every child up front, which will reintroduce the “N identical spinners” behavior (and also duplicates the per-streamBatchRunChildStarted). This path should be updated to sendBatchRunChildQueuedup front instead, consistent with this new contract.
// ── Queued state for EVERY child, up front (T3) ─────────────────────
// The semaphore below throttles agents, so most children sit QUEUED
// (created but not yet streaming) for a while. Broadcast a QUEUED signal
// for all children NOW so none looks crashed — but a distinct one from
// "started", so the sidebar can show "en file (n/N)" vs "en cours"
// instead of N identical spinners. Each child's real
// `BatchRunChildStarted` (streaming.rs, when its agent actually begins)
// flips it to running; `BatchRunProgress`/`Finished` clear it.
for disc_id in &outcome.discussion_ids {
let _ = state.ws_broadcast.send(WsMessage::BatchRunChildQueued {
run_id: outcome.run_id.clone(),
discussion_id: disc_id.clone(),
});
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
backend/src/api/discussions/streaming.rs:1268
- The checkpoint/awaiting marker is cleared unconditionally here, even if the final agent message failed to persist earlier (insert_message is best-effort and only logs on error). If the insert failed, clearing
partial_response/awaiting_agentcan silently lose the last durable trace and prevent boot recovery from retrying.
let partial = crate::db::discussions::set_partial_response(conn, &did_clear, None);
let awaiting = crate::db::discussions::set_awaiting_agent(conn, &did_clear, false);
partial.and(awaiting)
6804078 to
df4b954
Compare
Signed-off-by: Romuald Priol <romuald.priol@protonmail.com>
df4b954 to
9e1fb68
Compare
Summary
Changes
1 — Owed-run tracking (
awaiting_agent)discussions.awaiting_agent(migration 074, same spirit aspartial_response): set when a run becomes owed, cleared only on delivery/error — a mid-run interruption stays flagged, zero blind windowmake_agent_stream, after every preflight early-return (not in the callers): a failed preflight can never leave a stuck flag → no bogus "interrupted" notice at the next boot. Batch children are additionally marked at enqueue (create_batch_run); the re-set at spawn is idempotentreconcile_awaiting_agents, after partial recovery): runs that were owed but never started get an "interrupted" notice + anAgentRunsInterruptedbroadcast — never a re-spawn (an interruption may be deliberate: the user shut their machine down)?short-circuit between the two ops)2 — Cancel cascade on deletes
delete_batch_runandDELETE /discussions/:idfire the cancel tokens (run + child discs viadiscussions.workflow_run_id) before the DB delete — exact reuse of thecancel_runpatternFOREIGN KEY constraint failed) or zombie-held semaphore permitsunwrap_or_default) ; the existing stickyincrement_batch_progresscovers the late-child racecancel_runcascade extended to direct children (workflow_run_id = run_id, not onlyparent_run_id): cancelling a batch run directly now reaches its live children, and children with no live token get theirawaiting_agentcleared — a deliberate cancel is never mislabelled as an interruption at the next boot (running children keep the flag: their own cancelled path persists the ⏹️ footer and clears it, crash-safe)3 — Queued vs running (honest batch UX)
BatchRunChildQueuedbroadcast up-front for every child (replaces the burst ofStartedthat caused 23 identical spinners);BatchRunChildStartedfires at the real start — after the global agent-semaphore permit is acquired, not atmake_agent_streamentry (a child waiting for a slot no longer shows as running, and a preflight failure no longer leaves an unclearable spinner)queuedMapdistinct fromsendingMap— ⏳ hourglass for "queued" vs active spinner for "running"; batch group pill reads⏳ done/total · N▶ · N⏸(i18n ×3)awaiting_agentis serialized in theDiscussionDTO and the sidebar derives queued = DB flag OR live WS frame (running always wins) — the WS burst fired while another page was mounted is no longer lostPROJECT_LOOSE_LIMITcap, so a running disc can no longer hide behind "show more"wait_for_completionhanging to its timeout4 — Review hardening (13 Copilot rounds, all converged)
role="img", exhaustive-deps, batched state updates, observability on swallowed DB errors…)