Skip to content

fix(work): make establishment durable and identity-safe - #715

Open
XuPeng-SH wants to merge 10 commits into
matrixorigin:mainfrom
XuPeng-SH:fix/lean-context-surface
Open

fix(work): make establishment durable and identity-safe#715
XuPeng-SH wants to merge 10 commits into
matrixorigin:mainfrom
XuPeng-SH:fix/lean-context-surface

Conversation

@XuPeng-SH

Copy link
Copy Markdown
Collaborator

Summary

  • persist semantic Work establishment before the synthetic lifecycle carrier
  • recover exact pending operations by session and turn-chain identity
  • separate provider-native tool identity from public aliases and fail closed when absent
  • keep lifecycle phases monotonic and return errors instead of fake success on planning/assignment failure
  • use fresh provider invocation IDs while retaining stable operation identity for retries

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)
  • services full unit suite: one known flaky workspace-concurrency test failed in the full run and passed on isolated rerun; targeted Work schema test passed

The implementation does not alter prompt-visible cache-prefix assembly; it only carries typed runtime authority and durable recovery metadata.

@XuPeng-SH
XuPeng-SH force-pushed the fix/lean-context-surface branch 2 times, most recently from 3d34484 to f656d74 Compare September 6, 2026 16:24

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. A committed continuation must resume the same proposal/task identities, not re-author a different plan.
  2. An admitted semantic decision must be recovered before asking a model to decide again.
  3. Deferred cancel/replace/add obligations must survive the same crash boundary as the initial plan.
  4. 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.

Comment on lines +1025 to +1029
let result = continue_bound_work(
executor,
binding,
&establishment,
&establishment_request,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +15566 to +15568
let admission_authority_acquired = self.admit_semantic_work_operation(state).await?;
if !admission_authority_acquired {
self.hydrate_pending_work_establishment(state).await?;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +6475 to +6479
self.pending_work_establishment = Some(PendingWorkEstablishment {
call,
call_id,
attempts: 1,
});

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +1081 to +1085
if let Err(error) = establishment
.advance_phase(
&establishment_request,
WorkEstablishmentPhase::AwaitingAssignment,
WorkEstablishmentPhase::Complete,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@XuPeng-SH
XuPeng-SH force-pushed the fix/lean-context-surface branch from f656d74 to 386716f Compare September 7, 2026 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant