fix(work): make establishment durable and identity-safe - #715
Conversation
3d34484 to
f656d74
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of f656d74, from first principles and without treating backward compatibility as a constraint.
Assessment: not ready to merge. Separating durable operation identity from delivery/invocation identity is the right foundation, but the recovery protocol still violates four correctness invariants detailed inline:
- A committed continuation must resume the same proposal/task identities, not re-author a different plan.
- An admitted semantic decision must be recovered before asking a model to decide again.
- Deferred cancel/replace/add obligations must survive the same crash boundary as the initial plan.
- An operation requiring activation must not become Complete without an actual assignment receipt.
These affect user-visible behavior: a transient failure can make continuation unretryable, a restarted turn can reject its own admitted request, a deferred cancellation/replacement can disappear, and a blocked assignment can be reported as a successful continuation.
Testing: the new MatrixOne operation-admission/CAS/concurrency tests are useful, but they do not exercise the host-to-handler recovery protocol at its transaction seams. Each inline finding includes a concrete regression scenario. Please use a fresh host/executor after injected failures and assert durable rows, exact proposal/task/attempt identities, and whether side effects were admitted—not only serialized status strings or hash/helper equality.
Verification performed:
- git diff --check: passed.
- scripts/schema/test_schema_inventory.py: 47 tests passed.
- scripts/harness/test_fresh_database_contract.py: 16 tests ran, with 2 environment errors because this runner denies Unix sockets; these are not being reported as PR regressions.
- Rust/Cargo is unavailable locally, so the Rust failure scenarios below are code-path findings, not claimed executed reproductions.
- GitHub Static Checks failed in Clippy at crates/runtime/src/turn/agentic_loop/lifecycle.rs:682–683 (unnecessary_lazy_evaluations; then should be then_some): Static Checks log. The Test Suite was still running when checked.
Submitting COMMENT because the authenticated account is also the PR author; this is not an approval.
| let result = continue_bound_work( | ||
| executor, | ||
| binding, | ||
| &establishment, | ||
| &establishment_request, |
There was a problem hiding this comment.
[P1] Resume the committed continuation instead of regenerating its proposal
Every nonterminal continuation reaches continue_bound_work(), including an operation already in AwaitingAssignment. That helper reloads the current plan context and allocates the first unused task-N IDs, while proposal_invocation above fixes the proposal ID to the operation ID.
For example, start with task-1, commit a continuation adding task-2, then fail assignment (or crash before recording the next phase). Retrying the same operation sees task-2 in the graph and proposes task-3 with a new context_id. tool_work_plan::propose finds the already accepted proposal under the same ID, but verify_retry_identity rejects the changed source_ref as work_plan_invocation_conflict, retryable=false. A transient failure has made the durable operation unrecoverable through its advertised retry.
Persist/reuse the original proposal context and task allocation, and resume assignment from the accepted proposal/phase instead of recompiling. Recovery must also handle a committed proposal whose phase advance did not commit; checking AwaitingAssignment alone does not cover that window.
Regression test: drive the real continuation handler with a bound graph; inject failures immediately after plan commit, after phase advance, and after assignment commit. Retry the same operation with a fresh invocation/executor. Assert one accepted proposal, exactly the original added task IDs, no duplicate attempt, and eventual Complete.
| let admission_authority_acquired = self.admit_semantic_work_operation(state).await?; | ||
| if !admission_authority_acquired { | ||
| self.hydrate_pending_work_establishment(state).await?; |
There was a problem hiding this comment.
[P1] Recover the persisted admission before recomputing semantic intent
The only production hydration call runs after the new semantic judgment and admit_semantic_work_operation(). Consider a crash after the pending operation is committed but before genesis binds Work. A fresh host is still unbound, so it runs the judge again. Even a wording change in goal/objective/expected_result changes start_work_operation_id(), which hashes the full arguments.
Admission then encounters the original pending operation. Since both requests belong to the same turn-chain, cancel_pending_for_new_turn correctly refuses replacement and admission returns ContractViolation. The ? here exits before hydrate_pending_work_establishment can recover the exact persisted payload. Recovery therefore depends on an LLM reproducing byte-identical output.
Load and validate pending operation authority during bootstrap, before new semantic admission/provider execution, and resume that immutable decision. Preserve the same-turn conflict guard; it is the ordering/source of authority that needs fixing.
Regression test: persist admission, terminate the host before genesis, and recreate it with the same turn-chain but a judge deliberately returning different task text (and a separate unavailable-judge case). Recovery must use the original operation/graph without creating another row or dispatching the regenerated plan.
| self.pending_work_establishment = Some(PendingWorkEstablishment { | ||
| call, | ||
| call_id, | ||
| attempts: 1, | ||
| }); |
There was a problem hiding this comment.
[P1] Restore deferred graph obligations together with the lifecycle carrier
This recovery path restores only start_work arguments. A Required decision also contains deferred_graph_mutations, but canonical_start_work_payload persists only goal/tasks/activation plus turn_chain_id. The deferred mutations are copied into pending_work_graph_mutations only by the fresh-admission branch of take_admitted_work_establishment_call; an already hydrated pending carrier returns before that branch.
If the user requested "after the first result, cancel/replace the second task", a crash after genesis but before establishment finishes restores the original task graph while losing that cancellation/replacement. The new host initializes the mutation list empty, and Work-bound admission normally skips the judge. Both the pending mutation prompt and the execution gate therefore disappear, allowing the original second task to proceed. The existing semantic admission test asserts an in-memory mutation count, so it does not protect this boundary.
Persist the admitted mutation obligations and their consumed/pending state, then restore them before dispatch. Their durability must last until the corresponding accepted graph mutations, not merely until establishment is marked Complete.
Regression test: admit two tasks plus an exact deferred cancel/replace, restart after genesis and again after establishment completion, settle the first task, and assert that the original second task cannot execute before the required mutation is accepted exactly once.
| if let Err(error) = establishment | ||
| .advance_phase( | ||
| &establishment_request, | ||
| WorkEstablishmentPhase::AwaitingAssignment, | ||
| WorkEstablishmentPhase::Complete, |
There was a problem hiding this comment.
[P1] Require an assigned result before completing an activating operation
continue_bound_work treats every non-error, parseable JSON response from execute_run_next_work_item as successful assignment, then returns status=continued with dispatch_error=null. This block consequently seals the operation as Complete without checking next_task.status. The initial-establishment branch has the same issue.
The scheduler explicitly returns non-error statuses needs_recovery, blocked, in_flight, and complete as well as assigned. A concrete continuation case is an existing failed/undelivered task: next_foreground_task prioritizes NeedsRecovery over newly appended Ready tasks. No attempt is assigned, yet start_work reports "execute_assigned_task_then_call_settle_work_item" and permanently removes the operation from pending recovery.
Use a typed assignment outcome and validate the assigned item/attempt identity before committing Complete for activation=start. Non-assignment outcomes need an explicit blocked/recovery disposition, not an execution-success receipt; activation=defer should remain a separate intentional no-assignment case.
Regression test: exercise start_work against a bound graph with a failed undelivered task and against an in-flight task owned by another run. Assert no false Complete or execute-assigned receipt and no unauthorized attempt. Cover the full outcome matrix plus the successful resumed-assignment case.
f656d74 to
386716f
Compare
Summary
Verification
cargo test -p astra-runtime --lib(5250 passed, 0 failed, 43 ignored)cargo test -p astra-runtime-env --lib(112 passed)make lint(passed)cargo fmt --all -- --check(passed)The implementation does not alter prompt-visible cache-prefix assembly; it only carries typed runtime authority and durable recovery metadata.