Skip to content

fix: retain cancellations received before start - #1042

Draft
sbalabanov wants to merge 1 commit into
wt/sqc011from
wt/sqc012
Draft

sbalabanov wants to merge 1 commit into
wt/sqc011from
wt/sqc012

Conversation

@sbalabanov

@sbalabanov sbalabanov commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Why?

Start and cancel use independent topics. A cancellation can arrive before its Request exists; treating that read as a failure can discard accepted cancellation intent.

What?

Hold the original cancellation delivery without expiry or failure-budget consumption until the Request appears. Replay retained cancel dead letters through cancellation's own handler, preserving the original reason instead of manufacturing Error. Best-effort Landing outcomes and other error classification remain unchanged; batch claim/promotion races are outside this change. Builds on #1040.

Test Plan

Repro

  1. Run ./tool/bazel test //test/integration/submitqueue/orchestrator:go_default_test --test_output=errors, including TestCancelBeforeStartRetainsIntent for primary and retained-DLQ cancellations.
  2. Each fixture delivers cancel before start and observes four committed postponements beyond a three-attempt primary budget, with no Request, terminal log, or new dead letter while absent.
  3. Release start and verify the Cancelled snapshot and log retain the original reason. This is a controlled pre-enrollment window, not a promise that cancellation beats Landing.

All six focused unit/shared/integration targets passed; full orchestrator integration took 26.8s and uncached consumer integration 9.5s. The extended TestProcess_CancelsUnbatchedRequest also proves matching Cancelled-log repair after publication failure. Cancel coverage: 184/201 lines (91.5%); the new Hold body is exercised. Formatting, lint, tidy, Gazelle, and mock checks passed.

Revert Plan

Revert this PR; no schema migration is involved. Preserve held and final cancellation deliveries before rollback, because the previous handler can discard them while the Request is absent.

Issue Links

None — no public issue was supplied.

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.
@sbalabanov

Copy link
Copy Markdown
Contributor Author

[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 AGENTS.md explicitly covers PR bodies. No code change or new tests are needed.

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 991bcc01102840d090ae333fc24d17d605e2eac8.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

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

@sbalabanov

Copy link
Copy Markdown
Contributor Author

[independent-review] Should fix — public commit-message boundary (95% confidence).

The Test Plan in commit 991bcc01102840d090ae333fc24d17d605e2eac8 still names an organization-internal optional validation wrapper, although the PR body was corrected. Please remove only that sentence from the commit message and preserve the native verification evidence. There is no implementation change or need for more tests.

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 .agents/skills/autoreview workflow; finding rechecked against PR #1042 at 991bcc01102840d090ae333fc24d17d605e2eac8.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

[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 991bcc01102840d090ae333fc24d17d605e2eac8; I am awaiting explicit user approval before rewriting its history. No code or tests changed, and no force-push has been performed.

This branch has not been deployed

No deployments
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.

2 participants