fix(submitqueue): repair terminal request status on retry by sbalabanov · Pull Request #1039 · uber/submitqueue

sbalabanov · 2026-11-06T22:26:56Z

TL;DR: Rejected or cancelled requests reach their final displayed status even when publishing the terminal log initially fails.

Summary

A rejected or cancelled request can be finished internally but still look unfinished if its final status update fails to send. A retry now resends the final status without repeating validation, cancellation, or a state change. Repairs identify the consistency resend and warn that original details may be unavailable; the first attempt can still include its available reason.

Repro

Submit a request that is rejected during validation, or cancel a request before it joins a batch.
Let the final state write succeed, fail the final status publication, then retry the same queued message after publication works again.
Before this change, the retry skips the update and the request still looks unfinished. With this change, it resends the final status and warns that original details may be unavailable, without changing the request state or version again.

Test Plan

Existing controller tests retain their original assertions for initial status publication, publication failures, and unrelated halted outcomes; only the fixture and store-call expectations required by the changed behavior are adjusted.
Shared termination tests cover same-state replay and leaving a different terminal winner untouched. First-attempt error and cancellation details remain covered.

Six focused core/controller test targets and four affected libraries passed locally; formatting, lint, Gazelle, and module checks passed. No storage integration run is needed: this PR no longer changes storage or schema.

Stack

Based on #1037 (wt/sqc010); this is the current branch base, not a required runtime dependency.

Rollout

No schema migration is required. Replace old consumers so matching terminal retries reach status repair.

Revert Plan

Revert this PR to restore the earlier guard behavior; terminal retries may again leave the displayed status unfinished.

Issue Links

None; repository audit work, no external ticket.

Summary: Persist the terminal timestamp, error, and metadata in the winning request snapshot. Retry matching terminal outcomes without another transition or provider call, and prevent incomplete legacy repairs from erasing known diagnostics. Reproduction: 1. Reject or cancel a request while the terminal state write succeeds and its log publication fails. 2. Redeliver the original validation, conflict-signal, or cancellation message. 3. Confirm the retry publishes the original terminal facts without another terminal write; a different terminal winner remains unchanged. Test Plan: The existing Process regression tests exercise failed publication followed by successful replay and compare the original diagnostics, timestamp, and request version. TestTerminateRequest_ReplaysWinningFactsAfterPublishFailure also covers changed retry diagnostics and duplicate delivery. TestRequestTerminalDetailsMigration demonstrates the explicit additive migration, readable legacy NULL details, preserved new diagnostics, and rejection of malformed non-NULL details against MySQL. Materializer tests verify history/mapping repair without overwriting known diagnostics. Verified with ./tool/bazel test //submitqueue/... --test_output=errors (59 targets), the MySQL storage integration target, focused coverage, and repository formatting/lint/module/build/mock checks. Revert Plan: Revert the code while retaining the additive terminal_details column and its persisted facts. Retain failed deliveries for replay; reverting terminal Process repair reopens the missing-log failure.

Summary: Keep terminal-status repair on validation, conflict-signal, and cancellation retries without retaining the original log content. A replay explicitly reports that original terminal details were not retained, preserves the current request version, and uses the same publication identity. Remove the terminal-details entity fields, database schema and migration, storage serialization, gateway legacy protection, and their retention-only tests. Existing termination still publishes the available first-attempt diagnostic; matching-state retries do not repeat the state transition or validation. Test Plan: Six focused request-core and controller targets pass, including the existing three failed-publication/retry regressions. These check status/version stability, explicit unavailable diagnostics, and no repeated state write/provider/validator work. Four affected libraries build; formatting, lint, Gazelle, and module checks pass. Revert Plan: Revert this follow-up commit to restore the original larger draft; reverting the whole PR reopens terminal status repair failures.

sbalabanov · 2026-11-12T18:26:03Z

 		"partition_key", msg.PartitionKey,
 	)

+	if request.State == entity.RequestStateCancelled {


quick comment why this is necessary, same for other controllers in the change

[agent-response SQC-009] Added short comments at all three retry guards: a terminal write may succeed before its final log is published. Fixed in 63f2104.

[addressed by agent]

sbalabanov · 2026-11-12T18:28:46Z

+		return corerequest.PublishTerminalRequestLog(ctx, c.registry, request)
+	}
+
 	if entity.IsRequestStateTerminal(request.State) {


explain why previous new check is not added under this branch

[agent-response SQC-009] Documented why cancellation only resends Cancelled: other terminal outcomes belong to their winning writer and stay untouched. Fixed in 63f2104.

[addressed by agent]

sbalabanov · 2026-11-12T19:13:38Z

+	"github.com/uber/submitqueue/submitqueue/entity"
+)
+
+const terminalDetailsNotRetained = "original terminal details were not retained"


no need for a const, embed
also rephrase the error that this is a resend, likely due to consistency, and original error may not be retained

[agent-response SQC-009] Inlined the diagnostic and identified it as a consistency resend, without claiming that an earlier publication failed or that original details definitely disappeared. Fixed in 63f2104.

[addressed by agent]

sbalabanov · 2026-11-12T19:14:50Z

+
+// PublishTerminalRequestLog repairs a terminal request's status without another state write.
+// Original diagnostics are not retained; the repair explicitly reports their absence.
+func PublishTerminalRequestLog(ctx context.Context, registry consumer.TopicRegistry, request entity.Request) error {


call it something like "resendTerminalRequestLog" to indicate the usage

[agent-response SQC-009] Renamed the helper to ResendTerminalRequestLog and updated the callers. The affected child call sites are coordinated and checked before advancing this PR. Fixed in 63f2104.

[addressed by agent]

sbalabanov · 2026-11-12T19:21:49Z

-		func(_ context.Context, topic string, _ entityqueue.Message) error {
+		func(_ context.Context, topic string, msg entityqueue.Message) error {
 			publishedTopics = append(publishedTopics, topic)
+			entry, err := sqmq.UnmarshalRequestLog(msg.Payload)


this and other tests: do not modify existing tests and asserts. Try to reduce the amount of code changed in this PR, even if it somewhat sacrifices coverage.

[agent-response SQC-009] Restored the original controller test assertion bodies and removed the expanded replay scenarios. Only the halted-state fixtures and shared-store expectations required by the new behavior remain changed. Fixed in 63f2104.

[addressed by agent]

Summary: Explain why matching terminal retries resend a status log and why cancellation leaves other terminal outcomes to their winning writer. Name the helper ResendTerminalRequestLog and make its inline diagnostic identify a consistency resend without claiming the original details are definitely lost. Restore the original controller test assertions and remove the expanded replay scenarios. Keep only fixture and shared-store-call expectations required by the changed guards and shared termination path. Test Plan: Six focused core/controller test targets pass; the conflict-signal target also passes after tightening its shared-store expectation. Four affected libraries build, and formatting, lint, Gazelle, and module checks pass. Revert Plan: Revert this follow-up with the dependent helper call-site update to restore the prior exported helper name.

cla-assistant · 2026-11-12T22:06:14Z

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 · 2026-11-12T22:06:14Z

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

address comments, republish PR, make it ready for review

sbalabanov · 2026-11-12T23:01:06Z

+)
+
+// ResendTerminalRequestLog resends the request's terminal status without another state write.
+// The log identifies the resend; original diagnostics may be unavailable.


Add an example when it can happen that justifies calling this function

[agent-response SQC-009] Added the concrete validation example in c2484d1: Error can be stored, then the process can crash before publishing its status log. Redelivery resends the status without another request-state write. The update is published and the PR is ready for review after the prescribed readiness gate passed.

[addressed by agent]

Summary: Give the concrete example of validation storing Error and crashing before publishing its status log. Redelivery must resend the status without another request-state write. Test Plan: Comments-only change. Existing request-core unit target passes from cache; repository lint and Gazelle checks pass. No runtime behavior or tests changed. Revert Plan: Revert this comments-only commit.

This was referenced Nov 6, 2026

fix(start): reconcile dead-lettered submissions to Error #1040

Draft

fix(gateway): recover dead-lettered request logs #1037

Open

sbalabanov changed the title ~~fix(submitqueue): replay persisted terminal request logs~~ fix(submitqueue): repair terminal request status on retry Nov 10, 2026

sbalabanov commented Nov 12, 2026

View reviewed changes

sbalabanov marked this pull request as ready for review November 12, 2026 23:58

sbalabanov requested review from a team and behinddwalls as code owners November 12, 2026 23:58

mnoah1 approved these changes Nov 13, 2026

View reviewed changes

Conversation

sbalabanov commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Repro

Test Plan

Stack

Rollout

Revert Plan

Issue Links

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cla-assistant Bot commented Nov 12, 2026

Uh oh!

cla-assistant Bot commented Nov 12, 2026

Uh oh!

sbalabanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbalabanov commented Nov 6, 2026 •

edited

Loading