fix(agent-org): harden recovery, task orchestration, and run finality#373
fix(agent-org): harden recovery, task orchestration, and run finality#373ShiboSheng wants to merge 4 commits into
Conversation
Fixes #272 by making Agent Org recovery event-driven, wake dispatch idempotent, and run/task finality transactional. Enforce coordinator-owned assignment, add durable planner approval flows, improve lifecycle recovery and UI state projection, and cover the production paths with Rust, frontend, and E2E tests. Pre-commit hook ran. Total eslint: 0, total circular: 0
…t-org-recovery-invariants # Conflicts: # src-tauri/crates/agent-core/src/specialization/external_import/tests.rs Pre-commit hook ran. Total eslint: 0, total circular: 0
…ent-org-recovery-invariants Pre-commit hook ran. Total eslint: 0, total circular: 0
…ent-org-recovery-invariants # Conflicts: # src-tauri/crates/agent-core/src/core/session/turn/processor/inbox_drain/mod.rs # src-tauri/src/agent_sessions/event_pipeline/extractors/tests/extractors_tests.rs
Harry19081
left a comment
Harry19081
left a comment
There was a problem hiding this comment.
High severity (confirmed in code)
- reconcile_run_finality is a permanent write-lock heartbeat.
The old reconcile_if_terminal was read-only with an early exit when the root session wasn't terminal. The new function (agent_org_runs/store.rs:217-455 on the PR branch) unconditionally takes the process-global sessions writer mutex + BEGIN IMMEDIATE, then runs: a recursive CTE with ROW_NUMBER() window over all descendant agent_sessions, a code_sessions read, a task aggregate, an unread-inbox EXISTS, a second recursive CTE over session_turn_intents, and an unconditional UPDATE agent_member_interventions — even when the run is obviously still active. It's called from:
the run-view command polled every 2.5s per open org panel (up to 3 concurrent pollers exist: ChatView, WorkStation Communication, AppSwitcher);
lifecycle.rs:476 after every member/coordinator turn end;
the watchdog, per tick per running run;
task mutation commands.
Every open org chat panel now serializes an exclusive write transaction against all session persistence roughly every 2.5 seconds for the run's entire life. The same poll also calls active_for_member(coordinator), which the PR turned into a write-on-read (agent_member_interventions.rs:205-210), so it's two writer-lock acquisitions per poll — and the coordinator-clear UPDATE is executed twice.
-
task_list — a pure read tool — takes the global writer lock too.
task_list_get.rs:109 → completion_snapshot (agent_org_runs/store.rs:656+) wraps the same recursive-CTE + full-board pipeline in with_sessions_writer + IMMEDIATE transaction. The PR's new prompt text explicitly instructs the coordinator to call task_list after every TaskCompleted and before finishing, and members call it at turn start — so this read blocks all app-wide DB writes many times per run. (One agent claim I refuted: the code_sessions query is indexed by idx_code_sessions_parent_org_member; the cost is the lock scope, not a table scan.) -
agent_inbox is insert-only and gets fully rescanned.
There is zero DELETE FROM agent_inbox in the tree, and the PR makes rows far bigger (TaskAssigned now embeds up to 50KB of inline dependency outputs; plan-approval requests ~20KB) and more numerous (TaskCompleted + MemberIdle rows per task, watchdog redelivery). Two consumers scan the whole table per run:
the watchdog (agent_org_watchdog.rs:556-562) loads and JSON-decodes every row once per 60s tick per quiescent running run, just to collect historical TaskAssigned ids;
the run-view command ships the entire inbox (read + unread, full payload JSON) over the Tauri bridge every 2.5s poll, along with full pending-plan markdown.
CPU, RAM, and IPC per tick/poll grow monotonically with run age. A SQL-side filter for the watchdog and a LIMIT/read-cutoff for the view would make both O(small).
- N+1 full task-board loads with fat metadata.
Completed task outputs (up to 20KB each) are now stored inside metadata_json, and the board is repeatedly reloaded and re-parsed: update_inner loads the full board 3× per update, two byte-identical (agent_org_tasks/store.rs:666, 710, 761); a completing task_update cascades into ~6+U full-board loads (its own list, dispatch_task_completed, dispatch_ready_assigned_tasks_unblocked_by, plus one more per unblocked task inside enqueue_task_assigned_to); the turn-end/stop-gate/prompt paths each list the board again per turn. A 100-task board with ~10KB average outputs means megabytes of redundant JSON parsing per turn, much of it inside the write transaction. Related: task tool responses serialize each task's output twice (tasks.rs:559-560 — the extracted output field plus the raw metadata still containing it), doubling the bytes fed into the coordinator's LLM context on every task_list.
Medium severity
5. Un-closable-run treadmill. The watchdog's terminal_candidate gate is weaker than reconcile_run_finality's (which additionally requires no pending turn intents, root quiescence, and coordinator_observed_latest_tasks). A run passing the first but failing the second — e.g., a task updated after the coordinator's last terminal turn with nothing left to wake it — never closes, and re-runs the full reconcile write-transaction every tick forever plus every 2.5s while its panel is open. This is a liveness bug as much as a CPU one.
-
Idle-member rewakes bypass the recovery budget. agent_org_watchdog.rs:194-202 returns true unconditionally for Idle members with unread mail, and no budget row is recorded for Idle wakes (inbox_wake.rs:292-302), so the 1/5/15-minute backoff can never engage. If a drain persistently fails to mark rows read (member-id resolution failure, drain-guard commit failure), that's a real LLM turn dispatched every 60 seconds indefinitely — unbounded API cost with no escalation path.
-
Unbounded durable growth elsewhere. agent_org_plan_approvals stores full plan_content per revision with no length cap and no delete path (a planner iterating 30 revisions of an 80KB plan permanently adds ~2.4MB, duplicated again into task metadata and inbox rows); agent_org_task_events and session_turn_intents are also insert-only. None of these is scanned on a hot path today, so it's disk/cold-query growth rather than CPU.
-
Blocking work in wrong contexts. approve() does std::fs::write inside the writer lock + IMMEDIATE transaction (agent_org_plan_approvals.rs:359); the new org tools call synchronous SQLite directly on async executor threads with no spawn_blocking (unlike sibling tools that do); and the watchdog opens a fresh SQLite connection per query — roughly R×(10 + 2M + 3T) connection opens per tick (~350/min for 3 runs × 8 members × 30 tasks). Churn, not blow-up, but it compounds items 1–3.
Frontend (mild)
resolveOrgTaskOperationOutcome (orgTaskOutcome.ts:56-86) unconditionally JSON.parses the tool result before its early-return for events that already carry an outcome, and OrgTaskAdapter calls it in the render body unmemoized — O(events × task-list size) parses per chat re-render pass, sustained because the run-view poll delivers a fresh object every 2.5s. Cheap fix: hoist the early return, add useMemo.
Full pending-plan markdown rides the 2.5s poll (bounded by the memoized Markdown renderer, but the IPC payload itself repeats).
Otherwise clean: no new timers/intervals/listeners, effect deps correct, two mount-time RPCs actually removed.
Verified fine (so you don't re-check)
The pre-PR unbounded REWAKE_BUDGETS HashMap is genuinely fixed (moved to a SQLite table pruned every tick); the watchdog uses MissedTickBehavior::Skip in spawn_blocking with no overlap or duplicate spawns; wake coalescing works (stable agent-org-wake:{run}:{member} key held until turn completion, pinned by a 20-concurrent-wakes test); the claimed O(T²·M) scan removal is real; inbox payload size caps are enforced at insert; the drain hot path lost 2–4 queries per turn vs develop.
Two side findings worth passing to a correctness review: task_graph_create events never reach extract_org_task (the dispatcher gate in extractors.rs:84-97 doesn't include it, so its chat block likely renders without task data), and the watchdog discards inner scan errors while the first failing run aborts recovery for all later runs that tick.
|
All five dimension reviews are complete and cross-verified. Here is the consolidated report. TL;DR Critical — should block merge
High — silent-failure and stuck-state class
Medium Per the architecture-audit skill's requirements: layers 2–5 and 7–10 were covered; layer 1 (compile/clippy gates) and dynamic wire measurements were skipped — worth running cargo clippy and tsc --noEmit in CI before merge (the frontend reviewer did verify tsc --noEmit and the changed vitest suites pass). Top asks for the author, in order: fix the Abandoned gate to respect unread/intents (finding 1); add TASK_GRAPH_CREATE to the extractor dispatch gate and a routing test (2); log and isolate per-run watchdog failures (4); give declined-reconcile terminal candidates an escalation path instead of an empty plan (5); scope approval cancellation to terminal runs only, not paused (6); and get the e2e-test crate running against a real binary (3). The five full reviewer reports (with evidence snippets and checked-and-fine lists) are preserved in this session if you want any single finding expanded, and the PR branch worktree is still at scratchpad/pr373. If you'd like, I can also draft these as GitHub review comments on the PR. |
2 participants
Summary
This PR turns Agent Org recovery from a collection of best-effort wake heuristics into a durable, task-driven orchestration system.
It started as a fix for the watchdog and eligibility gaps tracked in (#272), including:
Live Agent Org runs then exposed adjacent design gaps: duplicate wakes, stale coordinator intervention, incomplete task graphs, missing dependencies, workers modifying the wrong tasks, ownerless auto-claim ambiguity, Planner approval deadlocks, output not reaching downstream tasks, premature run completion, and UI panels disagreeing about task state.
The final result establishes one coherent model for:
Fixes #272.
Scope
Compared with the current
developtarget, this PR changes approximately:A substantial part of the added code is state-matrix, concurrency, transaction, frontend, and E2E coverage.
Issue #272 coverage
EXISTSfor unread checks.inbox_drainfixtures so ownerless tasks contain valid claimant eligibility. The unrelated bundled-skill baseline remains outside this PR.The fixture failures and their relationship to the eligibility whitelist were confirmed in the [#272 issue discussion](#272 (comment)).
Design and implementation overview
Chapter 1 — Separate sources of truth
The baseline sometimes inferred one subsystem’s state from another—for example, treating an idle session as evidence that a run was finished, or treating an unread inbox row as proof that a wake had been scheduled.
This PR keeps each state dimension independent:
AgentOrgRunagent_org_runsAgentSessionagent_sessionsAgentOrgTaskagent_org_tasksAgentInboxagent_inboxPlanApprovalagent_org_plan_approvalsagent_org_recovery_attemptsImportant consequences:
Idledoes not mean the Run is complete.Runningdoes not mean every member is healthy.Pendingdoes not mean every eligible worker may claim a task.Unreaddoes not mean a Wake was accepted.The UI is a projection of these durable states, not another source of truth.
Chapter 2 — Coordinator, workers and the Rust control plane
The Coordinator and workers remain LLM sessions, but recovery and consistency are enforced by deterministic Rust code.
flowchart TD U["User goal"] --> C["Coordinator LLM"] C --> G["Create a dynamic task graph"] G --> DB["SQLite durable state"] DB --> W["Wake assigned member"] W --> M["Worker LLM executes"] M --> DB DB --> A["Recovery Analyzer<br/>Rust rules, no model call"] A --> E["Recovery Executor<br/>Rust side effects"] E --> W E --> C DB --> F["Run Finality Reconciler<br/>Rust transaction"] F --> END["Run terminal state"]Responsibilities are now explicit:
Analyzer and Executor are not additional agents and consume no model tokens.
Chapter 3 — Pure recovery analysis and persistent budgets
The previous watchdog mixed inspection, budget mutation, notification and Wake dispatch.
Recovery is now split into:
RecoveryPlan.A plan may contain multiple actions in one tick. One unread member no longer suppresses an unrelated repair or terminal check.
Recovery attempts are persisted with:
Only an accepted Wake consumes an attempt. Coalesced, rejected, paused and no-work outcomes do not.
Chapter 4 — Wake becomes an idempotent doorbell
Every Agent Org Wake uses a deterministic key:
Concurrent requests for the same member coalesce into one real turn.
Wake outcomes distinguish:
EnqueuedCoalescedDeferredPausedDeferredInterventionNoWorkRunTerminalSessionUnavailableFailedSession status is no longer written to
Runningbefore enqueue:Running;Before a queued Wake reaches the provider, it rechecks the Run:
If Inbox drain produces no real input, the turn returns
WakeNoopinstead of sending an empty model nudge.Turn intents also receive explicit terminal states such as
coalesced,rejected,staleandfailed, preventing orphaned queued intents from blocking finality.Chapter 5 — Coordinator intervention no longer causes Wake storms
A stale Coordinator intervention was the upstream cause of hundreds of repeated Coordinator Wake intents during live testing.
The final rule is:
While a worker is under direct user intervention:
Intervention identity is based on persisted
member_id, not UI labels or guessed agent types.Chapter 6 — Atomic dynamic task graphs
Coordinator task creation is no longer limited to a sequence of independent
task_createcalls.task_graph_createvalidates and writes a complete graph atomically:Either the entire graph is committed or no task is created.
The backend does not hard-code:
The Coordinator may create:
The backend guarantees graph correctness, not a fixed organizational recipe.
Chapter 7 — Explicit dispatch and dependency confirmation
Each task explicitly declares a dispatch policy:
immediateafter_dependenciesafter_dependenciesmust include valid dependency IDs and cannot form a cycle.To prevent the Coordinator’s prose from saying “after A and B” while structured data only lists A, task creation includes a local dependency-confirmation gate:
Explicit parallelism uses:
This analysis is deterministic local Rust code and does not consume tokens.
The check is repeated inside the writer lock and SQLite immediate transaction so a concurrent new task cannot slip between validation and insertion.
Chapter 8 — Task authority and the meaning of Ownerless
Task authority is separate from message routing.
Hierarchy Mode controls who may communicate with whom. It does not automatically grant permission to mutate another member’s task.
Final task authority rules:
owner = NULLnow means:It no longer means an open worker claim pool.
eligible_member_idsis a candidate constraint for Coordinator assignment, not ownership and not permission to self-claim.Ownerless auto-claim was removed from:
A normal message cannot bypass task authority. Work-bearing messages to a worker must reference a ready, unresolved task already owned by that worker.
Chapter 9 — Durable TaskOutput and downstream delivery
A completed task now records a durable output rather than only a status bit.
Task output can include:
When an upstream task completes:
TaskCompletedevent is emitted.TaskAssignedwith bounded dependency outputs.Tasks that have downstream consumers cannot complete without an output summary.
This prevents “completed” tasks whose reviewers receive no artifact or context.
Task state is monotonic: a completed task cannot be silently reopened. Changed requirements use follow-up tasks so Run finality remains explainable.
Task mutations return transaction-local before/after outcomes, ensuring assignment, completion and dependency side effects fire once rather than from stale pre-transaction reads.
Chapter 10 — Planner execution and durable approval
Execution mode travels with the task:
execution_mode=planexecution_mode=buildThe root Coordinator is no longer pushed into generic Plan Mode during an active Agent Org Run. Planning is performed by a member that owns a real in-progress Plan task.
create_planrequires:Agent Org configuration now supports three approval policies:
CoordinatorUserAutomaticPlan approval is durable and revisioned. It records:
For user approval, Group Chat renders an approval card where the user can:
Requesting changes atomically updates approval state and writes Planner feedback to Inbox.
Approving atomically:
Waiting for plan approval is intentionally quiet:
AwaitingPlanApproval;Chapter 11 — Worker lifecycle and failure disposition
Before a Build worker turn stops, Rust checks whether the worker still owns an in-progress Build task.
If the worker produced a result but forgot to complete the task:
Plan tasks are excluded because they may legitimately remain in progress while awaiting approval.
MemberIdleandTaskCompletednow have different meanings:MemberIdlesays a member turn ended;TaskCompletedsays a specific task produced a durable completion.If work remains unfinished,
MemberIdlecarries the relevant task IDs so the Coordinator knows exactly what needs repair.On explicit member failure:
A merely old
updated_attimestamp no longer steals ownership from a still-Running member. Staleness produces a Coordinator notice; only explicit failure, cancellation, timeout, shutdown or restart recovery may change ownership.Chapter 12 — Pause, resume and restart recovery
Paused Runs are quiet:
Resume rebuilds progress from persisted state and durable resume/inbox events, not the old in-memory queue. Task existence alone does not generate an empty worker turn.
Application restart recovery now classifies abandoned turn intents:
It then reconciles:
Running Runs are selected directly in SQL:
They are no longer hidden behind a “load 500 arbitrary runs, then filter in Rust” limit.
Chapter 13 — Atomic Run finality and completion certificate
Run completion is no longer inferred from “the task cards look done.”
The completion certificate checks:
Task mutation and Run reconciliation share the same writer lock and SQLite immediate transaction.
This removes the race:
Only two serializable outcomes are possible:
Chapter 14 — CLI capability boundary and UI projection
CLI Agent Org members are explicitly disabled until they have production parity.
Current CLI members do not have the full combination of:
This PR therefore:
The frontend also gains:
The right-side live monitor remains a runtime projection rather than a new persistent historical dashboard. Group Chat Team Tasks remain the durable task view.
Chapter 15 — Performance, tests and audit coverage
Performance and data-boundary changes include:
SELECT EXISTSfor unread checks;MissedTickBehavior::Skip.Automated coverage includes:
Final targeted verification after merging the latest
develop:cargo check -p org2Earlier full-suite verification on the feature diff produced:
Those remaining failures were in unchanged repository baseline areas and were kept outside this PR.
After merging
origin/develop@09894ca7d, fullpnpm typecheckalso reports an existing target-branch error at:That file is identical to the current target
developand is outside this PR’s feature diff.Architecture and frontend audits are included under:
docs/architecture-audit-2026-07-12/docs/architecture-audit-2026-07-13/docs/architecture-audit-2026-07-14/docs/frontend-ui-audit-2026-07-12/docs/frontend-ui-audit-2026-07-13/docs/frontend-ui-audit-2026-07-14/Chapter 16 — End-to-end behavior after this PR
An Agent Org Run now follows this model:
flowchart TD U["User submits a goal"] --> C["Coordinator designs a dynamic Task Graph"] C --> G["Atomically create Tasks, Owners and Dependencies"] G --> R["Dispatch only ready root Tasks"] R --> M["Assigned Worker executes"] M --> O["Persist TaskOutput and emit TaskCompleted"] O --> N["Unlock only real downstream dependencies"] N --> M M --> P{"Plan Task?"} P -->|"Yes"| A["Wait for Coordinator, User or Automatic approval"] A --> N O --> Q["Coordinator reads the completion certificate"] Q -->|"Blockers remain"| WAIT["Wait for a real Inbox or Task event"] Q -->|"Everything is complete"| F["Coordinator gives the final user response"] F --> END["Complete the Run inside a transaction"] WAIT --> WD["Watchdog inspects durable state"] WD -->|"Real recovery action exists"| CThe final design can be summarized in eight rules:
Compatibility and migration
Mandarin Chinese Tech Doc of this PR
issue272_agentorg_changes.html
Agent Org Run Testing
tested on:
issue #305
screenshot: