[sbalabanov]

TL;DR: Report failed submissions as Error even when no Request was saved.

Summary

The start DLQ reports failures that occurred before a Request was saved by publishing an Error request log with request version 0. Saved Requests retain batch ownership, terminal winners and observed-version conditional writes. The DLQ publishes no normal-work command.

Repro

1. Accept a valid submission, but let its initial Request save fail until the message reaches start_dlq.
2. Process the dead letter while the Request is absent.
3. An Error request log with version 0 reports the failed submission.

Test Plan

• Existing DLQ tests cover version-zero failure logs for missing Requests, repeated delivery and publication failures, identity validation, saved-row ownership, terminal and conditional-write behavior, and error counting on every failed delivery.
• The messagequeue, primary Start, orchestrator and DLQ unit-test targets passed uncached. make lint, make check-gazelle and make check-tidy passed.

Revert Plan

Revert this PR; existing Request data and request-log history remain intact.

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]