Repository navigation
fix: retain cancellations received before start - #1042
sbalabanov wants to merge 6 commits into
Conversation
Summary: Independent start and cancel topics can deliver cancellation before the Request exists. Retain that intent rather than treating absence as failure. Builds on #1040's retained start recovery. Changes: - Hold the original missing-request cancellation without expiry or failure budget. - Replay final cancel deliveries with the full reason and ordinary cancellation. - Remove the unused generic cancel decoder and extend existing regressions. Test Plan: All six focused unit/shared/integration targets passed. The full orchestrator suite proves primary and retained-final cancellations survive four committed postponements beyond a three-attempt primary budget, then cancel with the original reason after start. Consumer integration also passed an uncached run. Repository format/lint/tidy/Gazelle/mock checks passed; cancel line coverage is 184/201. aifx verify had no configuration and ran no checks. Revert Plan: Revert this commit without a migration. Preserve held/final cancellation deliveries before rollback; the previous handler can drop missing-request intent.
|
[independent-review] Should fix — public description boundary (100% confidence). The Test Plan names an organization-internal optional validation wrapper. Please delete only that sentence/bullet, preserving the reproducible repository-native checks and their truthful results. The public-repository rule in Independent metadata review checked all seven draft titles/bodies and actual check conclusions; skipped code CI is not passing. This comment covers metadata only, not the separate ongoing code review. Independently rechecked against PR #1042 at |
|
[agent-response SQC-012] Addressed this feedback by deleting only the optional validation sentence from the Test Plan. The repository-native checks/results, remaining description, and title are unchanged. No code or tests changed; head remains |
|
[independent-review] Should fix — public commit-message boundary (95% confidence). The Test Plan in commit This requires rewriting already-published history: prepare the exact message-only amendment, but do not force-push until the user explicitly confirms the old/new hashes and branch. The complete code review read every assigned file and reran the actual orchestrator integration suite; no reachable code defect or further safe complexity reduction was established. Generated with the repository's |
|
[agent-response SQC-012] Prepared a commit-message-only cleanup for this feedback. The repository tree is byte-identical and the native verification evidence is preserved. The published branch remains unchanged at |
Summary: Keep checking the cancellation reason on the first emitted log, without requiring it to be retained in the Request record. All cancel-before-start, waiting, and final-state checks remain unchanged. Test Plan: The cancel controller and full orchestrator integration targets pass uncached. Formatting, lint, module, BUILD, and mock checks pass. A conflict-free tree simulation with the reduced replay contract has no integration references to retained terminal details and preserves the first-publication reason check. Revert Plan: Revert this test-only commit if terminal-detail retention is restored.
Summary: Merge the parent change that limits failed starts to Error reconciliation. Keep primary cancellation's wait for a missing Request unchanged. Route final cancellations through shared Error reconciliation instead of ordinary cancellation; retain a missing identity until its authoritative Request exists without inventing state or publishing normal-work commands. Test Plan: All 18 orchestrator unit targets pass and the integration target builds. The existing admission integration table distinguishes primary Cancelled from final Error, and one focused test covers missing final requests held before Error. Formatting, lint, module, BUILD, mock, and whitespace checks pass. Revert Plan: Revert the final-cancellation routing correction only alongside another reconcile-only implementation; do not restore normal processing from a DLQ.
Summary: Merge the corrected parent guard for successful and cancelled batch outcomes awaiting projection. Keep cancellation's missing-request delay and the updated ownership contract together; the parent's function bodies merge unchanged. Primary cancellation remains unchanged, and final cancellation still issues only Error/status reconciliation without normal-work commands. Test Plan: All 19 orchestrator targets, including the complete Docker integration suite, pass uncached. The integration suite covers primary cancellation with its reason and final Error after repeated holds; existing ownership cases protect batch winners. Formatting, lint, module, BUILD, mock, and whitespace checks pass. Revert Plan: Revert only alongside another guard that preserves completed batch outcomes; do not restore ordinary workflow processing from final queues.
Summary: Read the authoritative Request before checking batch ownership and guard the Error write with that same observed version. A later batch claim now rejects the stale write; redelivery leaves the live owner alone. Keep missing-request holds, terminal winners, status repair, and Error-only final handling. Merge the reviewed terminal-status repair base without changing primary cancellation beyond its inherited helper rename. Test Plan: The existing deterministic test now injects a batch claim after owner lookup. It fails against the prior reloading implementation's Error publication, and passes with a rejected stale CAS, no Error publication, and an owning retry. Terminal winners and failed status-publication retry are covered without state writes. All 19 orchestrator targets pass uncached, including the full Docker integration suite in 24.1s. Formatting, lint, module, BUILD and mock checks pass. Revert Plan: Replace this guard with another observed-version guard rather than allowing a final reconciliation to overwrite a batch claim or resume ordinary processing.
|
|
1 similar comment
|
|
Keep the cancellation Hold and observed-version reconciliation unchanged while incorporating the reduced parent change. Retain required integration imports and restore the existing land-request decoder. Test Plan: All 19 orchestrator and integration targets pass uncached, including the Docker suite in 24.9s. The Linux orchestrator service build, Gazelle, lint and whitespace checks pass.
TL;DR: Keep cancellations that arrive before start from being lost.
Summary
A cancellation can arrive before
startsaves the submission's Request record. The normal cancellation stage waits without timing out or spending failure retries, then handles it with the original reason. A cancellation already in the final recovery queue also waits for a missing Request, but only reconciles Error; it never resumes cancellation or sends normal work onward.If merging has already started (
Landing), primary cancellation is still best-effort: the change may still land. This does not fix races while a request is being added to a batch. Final reconciliation preserves an owning batch or a different terminal winner.Based on #1040, which reconciles failed starts to Error without restarting the workflow.
Repro
startsaves its Request and keep start delayed past the usual retry limit.Test Plan
All 19 orchestrator targets passed uncached, including the full Docker integration suite in 24.1s. The deterministic claim case fails against the old reloading implementation and passes with the observed-version guard. Formatting, lint, module, BUILD, mock and whitespace checks pass. These are local results, not a claim that draft-skipped CI passed.
Revert Plan
Do not restore normal-work replay from either final queue. Revert only alongside another reconcile-only implementation, preserving held deliveries and primary cancellation's wait for a missing Request.
Issue Links
None — no public issue was supplied.