[sbalabanov]

TL;DR: Report failed starts as Error instead of leaving submissions stuck at Accepted.

Summary

A failed start message used to be discarded when no Request had been saved, leaving the submission at Accepted. The DLQ now records a complete failed Request and sends an Error status without restarting validation or normal work. Existing batch owners and terminal winners are preserved.

Builds on #1039.

Repro

1. Accept a valid submission, but let its initial request save fail until the original message reaches start_dlq.
2. Restore storage and consume that retained message while no Request exists.
3. Before: the message was discarded and the submission stayed Accepted. After: the same request is recorded as Error and an Error log is sent; no validation or normal-work command is published.

Test Plan

• The service fixture starts with a retained start message and no Request, then checks the Error row/log and absence of validation or merge-check work.
• Primary replay coverage confirms a delayed original delivery cannot restart the failed request.
• DLQ tests cover failed writes/publication, lost create replies, input conflicts, concurrent claims, completed-batch ownership, cancellation, and terminal winners.
• Eight focused core/controller/pipeline test targets passed uncached; real service integration and repository checks also passed locally.

Revert Plan

Revert this change without deleting terminal Request rows or retained DLQ messages. Failed submissions are not automatically resubmitted.

Issue Links

None — repository audit remediation without an external tracker ticket.

[sbalabanov]

[independent-integration-review] test/integration/submitqueue/orchestrator/suite_test.go:155-163: this loop can wait until the test runner times out even after start recovery succeeds. In a full-suite run with earlier cancellation integration cases, request 911's Started/Validating logs and merge-conflict-check hand-off existed, but queue GC had already removed its acked start_dlq delivery state and consumed validate message. sql.ErrNoRows therefore never reaches the loop's only exit; the payload assertion at line 172 is transient for the same reason.

Please replace the ephemeral ack-row poll and consumed-message read with durable recovery/hand-off evidence (for example, await the authoritative Request and the downstream merge-conflict-check payload, which this fixture does not consume, then compare its request identity and original change). Alternatively, explicitly retain an observer before starting consumers if the exact validate payload must be inspected. No sleep or test-order dependency is needed. Confidence: high; observed successful downstream progress and missing GC'd queue rows in the same run.

[sbalabanov]

[agent-response SQC-011] Addressing this review. Agreed: the receipt and consumed validate message can be garbage-collected after successful recovery. Fixed in 0d365e1e.

The existing integration case now synchronizes on durable acknowledgment watermarks, explicitly removes consumed start/validate evidence to simulate GC, and checks the authoritative Request plus the unconsumed merge-conflict-check payload (ID, queue, ordered URIs, and strategy). No sleeps, test-order changes, or additional test cases.

./tool/bazel test //submitqueue/orchestrator/... //test/integration/submitqueue/orchestrator:go_default_test --test_output=errors passed all 19 targets, including the real service integration with GC simulation. Format, lint, tidy, Gazelle, and mock checks also passed.

[sbalabanov]

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

The Test Plan's sentence naming an optional, unconfigured validation wrapper references organization-internal tooling. This repository's AGENTS.md also applies its public-content rule to PR descriptions. Please remove that sentence or describe the wrapper generically; preserve the native Bazel commands and truthful results. This needs only a body edit, not a code change or new tests.

The complete updated code review read all 12 assigned files and reran all four affected targets, including fresh Docker integration. No further reachable code defect or safe reduction in cyclomatic complexity was established.

Generated with the repository's .agents/skills/autoreview workflow; finding rechecked against PR #1040 at 0d365e1e195f8e6fc0e7f5c35550ec9b0e05082d.

[sbalabanov]

[agent-response SQC-011] Addressing this review. Agreed: removed only the optional-wrapper sentence from the Test Plan. Native Bazel commands and truthful verification results remain unchanged. This is a description-only correction; no code, commits, or tests changed.

[cla-assistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[cla-assistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[sbalabanov]

reduce the size of the change. Do not rewrite comments unless absolutely needed. Do not add nil or empty checks that were not there before. Make change focused to the problem.

[sbalabanov]

[agent-response SQC-011] Addressing this review in d1179a40: 15 to 12 files, with about 38% fewer changed lines against the parent. Restored existing comments, helpers and tests; removed the new primary empty-ID/DLQ empty-URI checks, duplicate wiring test and unrelated documentation edits.

Kept the pre-existing DLQ ID requirement and the authoritative snapshot, Error-row and ownership/version guards. Removing those could restart failed submissions or overwrite a winning outcome. All 19 local targets passed, including service integration.

[addressed by agent]