Use one authoritative workflow ID across dispatch - #351
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
crates/orchestrator-daemon-runtime/src/dispatch/process_manager.rs:ProcessManager::poll_completed—let workflow_id = latest_runner_workflow_id(&events).or_else(|| workflow_target.clone());still lets a divergent runner-emitted ID override the kernel-selected/queue-stored authoritative ID. That is the same identity-split failure mode TASK-1072 is intended to eliminate: completion and environment cleanup can be correlated to the runner ID while the queue remains assigned toworkflow_target. REQUIRED CHANGE: whenworkflow_targetis present, make it the workflow ID used for reconciliation; compare anylatest_runner_workflow_id(&events)against it, emit an explicit mismatch diagnostic containing both IDs, and fail the completion rather than switching identities. Only use the runner-emitted ID when there is no kernel target (the legacy ad-hoc path). Add this as a small helper if needed so timeout, signal, and normal-exit paths retain the same invariant. Verification: add a unit test inprocess_manager.rs(or the completion-reconciliation test module) that supplies targetqueue-idplus a runner event forrunner-idand asserts the completed process is failed/reconciled underqueue-idwhile diagnostics contain both values; runcargo test -p orchestrator-daemon-runtimeand expect all tests to pass.
The pre-lease UUID allocation, propagation through --new-workflow-id, and use of that ID for the environment broker are well-scoped and directly address the primary dispatch-path split. The fresh-vs-resume command separation is also clear.
There was a problem hiding this comment.
VERDICT: APPROVE
No blocking findings. The change preallocates workflow IDs before the atomic queue lease, propagates the leased ID through dispatch planning and fresh runner bootstrap (--new-workflow-id), keeps resume semantics distinct, and makes completion reconciliation fail closed while retaining the authoritative ID on mismatch. The environment broker and early-exit fallback now use the same durable identity, matching TASK-1072 without unrelated changes.
Verification: GitHub reports all configured checks green for head 601b857f6fd21db200d4275473d01b592e828c40, including workspace cargo-check, clippy, rustfmt, docs-sync, dependency guardrail, smoke-help, and Vercel. I also inspected all six changed files against TASK-1072; the focused tests cover fresh-ID CLI propagation and mismatch reconciliation. The implementation is cohesive, minimal, and preserves the legacy ad-hoc and explicit resume paths.
What changed
--new-workflow-idand keep resume IDs distinct.Why
TASK-1072 reproduced a production queue UUID / journal UUID mismatch. The daemon leased without supplying IDs while the runner independently minted its own, so queue lookup could report
not_foundfor a live workflow and node.Impact
Queue, journal, runner events, environment broker, and completion reconciliation use one identity. This PR requires a runner release that supports
--new-workflow-idand the corrected consolidated Postgres backend.Validation
cargo check -p orchestrator-cligit diff --check