From 9459a97aa6510807d5b70081444ef6e5ad8d37ad Mon Sep 17 00:00:00 2001 From: sergeyb Date: Fri, 6 Nov 2026 22:23:58 +0000 Subject: [PATCH] fix(submitqueue): replay persisted terminal request logs 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. --- submitqueue/entity/request.go | 13 +++ submitqueue/entity/request_log.go | 3 + submitqueue/extension/storage/README.md | 4 + .../gateway/core/request/materializer.go | 3 + .../gateway/core/request/materializer_test.go | 56 +++++++++++ .../orchestrator/controller/cancel/cancel.go | 4 + .../controller/cancel/cancel_test.go | 47 +++++++-- .../controller/conclude/conclude_test.go | 11 ++- .../orchestrator/controller/dlq/dlq_test.go | 11 ++- .../landconflictsignal/landconflictsignal.go | 39 ++------ .../landconflictsignal_test.go | 42 ++++++-- .../controller/validate/validate.go | 9 +- .../controller/validate/validate_test.go | 42 ++++++-- .../orchestrator/core/request/BUILD.bazel | 2 + .../orchestrator/core/request/terminal_log.go | 46 +++++++++ .../core/request/terminal_log_test.go | 67 +++++++++++++ .../orchestrator/core/request/terminate.go | 23 +++-- .../core/request/terminate_test.go | 64 +++++++++++- .../001_request_terminal_details.sql | 1 + .../storage/mysql/migration/BUILD.bazel | 1 + .../extension/storage/mysql/request_store.go | 43 ++++++-- .../storage/mysql/request_store_test.go | 99 +++++++++++++++---- .../extension/storage/mysql/schema/README.md | 6 ++ .../storage/mysql/schema/request.sql | 1 + .../extension/storage/mysql/BUILD.bazel | 2 + .../extension/storage/mysql/storage_test.go | 38 +++++++ .../submitqueue/extension/storage/suite.go | 27 +++-- 27 files changed, 587 insertions(+), 117 deletions(-) create mode 100644 submitqueue/orchestrator/core/request/terminal_log.go create mode 100644 submitqueue/orchestrator/core/request/terminal_log_test.go create mode 100644 submitqueue/orchestrator/extension/storage/mysql/migration/001_request_terminal_details.sql create mode 100644 submitqueue/orchestrator/extension/storage/mysql/migration/BUILD.bazel diff --git a/submitqueue/entity/request.go b/submitqueue/entity/request.go index 0399518ed..f16d4ccf0 100644 --- a/submitqueue/entity/request.go +++ b/submitqueue/entity/request.go @@ -66,6 +66,17 @@ func IsRequestStateHalted(s RequestState) bool { return IsRequestStateTerminal(s) || s == RequestStateCancelling } +// RequestTerminalDetails contains the immutable facts of a terminal request outcome. +// A zero TimestampMs denotes unavailable details on a legacy snapshot. +type RequestTerminalDetails struct { + // TimestampMs is the terminal outcome time in milliseconds since Unix epoch. + TimestampMs int64 `json:"timestamp_ms"` + // LastError is the diagnostic associated with the terminal outcome, or empty. + LastError string `json:"last_error"` + // Metadata contains additional diagnostic facts, or nil when absent. + Metadata map[string]string `json:"metadata"` +} + // Request defines a request to land a set of code changes on the source control repository's target branch. // The object is immutable after creation. type Request struct { @@ -89,6 +100,8 @@ type Request struct { // State is the current state of the land request. State RequestState `json:"state"` + // TerminalDetails contains the facts of the terminal outcome, or its zero value when unavailable. + TerminalDetails RequestTerminalDetails `json:"terminal_details"` // Version is the version of the object. It is used for optimistic locking. // Versioning starts at 1 and is incremented for each change to the object. Version int32 `json:"version"` diff --git a/submitqueue/entity/request_log.go b/submitqueue/entity/request_log.go index ea29018f6..c1e2926e9 100644 --- a/submitqueue/entity/request_log.go +++ b/submitqueue/entity/request_log.go @@ -84,6 +84,9 @@ const ( RequestStatusCancelled RequestStatus = "cancelled" ) +// TerminalDetailsUnavailable marks a legacy terminal log whose original diagnostics are unavailable. +const TerminalDetailsUnavailable = "terminal_details_unavailable" + // RequestEvent is something that happened to a request while it sat at a status, // rather than a status of its own. // diff --git a/submitqueue/extension/storage/README.md b/submitqueue/extension/storage/README.md index 026429d84..a9c98e56e 100644 --- a/submitqueue/extension/storage/README.md +++ b/submitqueue/extension/storage/README.md @@ -72,3 +72,7 @@ Take the first branch that applies: 3. **Map** — a pipeline controller needs the lookup at runtime → a dedicated mapping store keyed by the attribute. The bar for 3 is a hot-path need: one mapping per access path, never per attribute, and never for ops/debug queries — run those against SQL replicas directly. But don't contort 1–2 to dodge a legitimate 3; a primary key that hashes half the entity's fields, or an aggregate bloated into listing everything, is the same duplication hidden in a worse place. + +### Terminal request snapshots + +A terminal Request replacement includes its outcome timestamp, last error, and metadata in the same conditional write as its state. Same-state replay publishes those persisted facts without another state transition; incoming retry diagnostics cannot replace the winning outcome. Legacy snapshots without details emit an explicitly incomplete log, which can repair a non-terminal summary but cannot erase a known same-status, same-request-version terminal diagnostic. The gateway protection must be deployed before enabling legacy fallback publication. diff --git a/submitqueue/gateway/core/request/materializer.go b/submitqueue/gateway/core/request/materializer.go index 273bb68b4..6a4298b60 100644 --- a/submitqueue/gateway/core/request/materializer.go +++ b/submitqueue/gateway/core/request/materializer.go @@ -188,6 +188,9 @@ func logWins(log entity.RequestLog, summary entity.RequestSummary) bool { if log.RequestVersion != summary.RequestVersion { return log.RequestVersion > summary.RequestVersion } + if log.Status == summary.Status && log.Metadata[entity.TerminalDetailsUnavailable] == "true" && summary.Metadata[entity.TerminalDetailsUnavailable] != "true" { + return false + } } return log.TimestampMs > summary.StatusTimestampMs } diff --git a/submitqueue/gateway/core/request/materializer_test.go b/submitqueue/gateway/core/request/materializer_test.go index e35ccab36..c4766f546 100644 --- a/submitqueue/gateway/core/request/materializer_test.go +++ b/submitqueue/gateway/core/request/materializer_test.go @@ -63,6 +63,20 @@ func TestMaterializer_PersistLog(t *testing.T) { ) require.NoError(t, f.materializer.PersistLog(context.Background(), log)) }) + + t.Run("legacy repair appends history and repairs mappings without erasing diagnostics", func(t *testing.T) { + f := newMaterializerReceiptFixture(gomock.NewController(t)) + known := base + known.Status, known.RequestVersion, known.StatusTimestampMs = entity.RequestStatusError, 2, 20 + known.LastError, known.Metadata = "original failure", map[string]string{"source": "validator"} + fallback := entity.NewRequestStatusLog(base.Queue, base.RequestID, known.Status, known.RequestVersion, "", map[string]string{entity.TerminalDetailsUnavailable: "true"}) + f.logs.EXPECT().Insert(gomock.Any(), fallback).Return(nil) + f.summaries.EXPECT().Get(gomock.Any(), base.RequestID).Return(known, nil) + f.expectURIMappings(known, nil) + f.receipts.EXPECT().Create(gomock.Any(), receiptFromTestSummary(known)).Return(nil) + require.NoError(t, f.materializer.PersistLog(context.Background(), fallback)) + }) + } func TestMaterializer_RecordsLifecycleMetrics(t *testing.T) { @@ -313,6 +327,48 @@ func TestLogWins(t *testing.T) { Status: entity.RequestStatusLanded, RequestVersion: 2, TimestampMs: 200, }, }, + { + name: "incomplete legacy repair preserves known same terminal outcome", + current: entity.RequestSummary{Status: entity.RequestStatusError, RequestVersion: 2, StatusTimestampMs: 100, Metadata: map[string]string{"source": "validator"}}, + incoming: entity.RequestLog{Status: entity.RequestStatusError, RequestVersion: 2, TimestampMs: 200, Metadata: map[string]string{entity.TerminalDetailsUnavailable: "true"}}, + want: false, + }, + { + name: "legacy repair promotes non-terminal", + current: entity.RequestSummary{Status: entity.RequestStatusStarted, RequestVersion: 0, StatusTimestampMs: 300, Metadata: nil}, + incoming: entity.RequestLog{Status: entity.RequestStatusError, RequestVersion: 2, TimestampMs: 200, Metadata: map[string]string{entity.TerminalDetailsUnavailable: "true"}}, + want: true, + }, + { + name: "legacy repair respects higher version", + current: entity.RequestSummary{Status: entity.RequestStatusError, RequestVersion: 3, StatusTimestampMs: 100, Metadata: nil}, + incoming: entity.RequestLog{Status: entity.RequestStatusError, RequestVersion: 2, TimestampMs: 200, Metadata: map[string]string{entity.TerminalDetailsUnavailable: "true"}}, + want: false, + }, + { + name: "legacy repair permits newer version", + current: entity.RequestSummary{Status: entity.RequestStatusError, RequestVersion: 1, StatusTimestampMs: 100, Metadata: nil}, + incoming: entity.RequestLog{Status: entity.RequestStatusError, RequestVersion: 2, TimestampMs: 200, Metadata: map[string]string{entity.TerminalDetailsUnavailable: "true"}}, + want: true, + }, + { + name: "legacy repair retains different-status ordering", + current: entity.RequestSummary{Status: entity.RequestStatusLanded, RequestVersion: 2, StatusTimestampMs: 100, Metadata: nil}, + incoming: entity.RequestLog{Status: entity.RequestStatusError, RequestVersion: 2, TimestampMs: 200, Metadata: map[string]string{entity.TerminalDetailsUnavailable: "true"}}, + want: true, + }, + { + name: "legacy repair may replace incomplete diagnostics", + current: entity.RequestSummary{Status: entity.RequestStatusError, RequestVersion: 2, StatusTimestampMs: 100, Metadata: map[string]string{entity.TerminalDetailsUnavailable: "true"}}, + incoming: entity.RequestLog{Status: entity.RequestStatusError, RequestVersion: 2, TimestampMs: 200, Metadata: map[string]string{entity.TerminalDetailsUnavailable: "true"}}, + want: true, + }, + { + name: "ordinary same-terminal log retains timestamp ordering", + current: entity.RequestSummary{Status: entity.RequestStatusError, RequestVersion: 2, StatusTimestampMs: 100, Metadata: nil}, + incoming: entity.RequestLog{Status: entity.RequestStatusError, RequestVersion: 2, TimestampMs: 200, Metadata: nil}, + want: true, + }, } for _, tt := range tests { diff --git a/submitqueue/orchestrator/controller/cancel/cancel.go b/submitqueue/orchestrator/controller/cancel/cancel.go index 11ebbb064..c867f575e 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel.go +++ b/submitqueue/orchestrator/controller/cancel/cancel.go @@ -147,6 +147,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "partition_key", msg.PartitionKey, ) + if request.State == entity.RequestStateCancelled { + return corerequest.PublishTerminalRequestLog(ctx, c.registry, request) + } + if entity.IsRequestStateTerminal(request.State) { metrics.NamedCounter(c.metricsScope, opName, "already_terminal", 1, metrics.TagsFromContext(ctx)...) return nil diff --git a/submitqueue/orchestrator/controller/cancel/cancel_test.go b/submitqueue/orchestrator/controller/cancel/cancel_test.go index 0b944db64..136a6ec96 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel_test.go +++ b/submitqueue/orchestrator/controller/cancel/cancel_test.go @@ -17,6 +17,7 @@ package cancel import ( "context" "fmt" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -64,6 +65,16 @@ func requestWithState(request entity.Request, state entity.RequestState) entity. return request } +func terminalRequestWithState(request entity.Request, state entity.RequestState) gomock.Matcher { + return gomock.Cond(func(got entity.Request) bool { + if got.TerminalDetails.TimestampMs <= 0 { + return false + } + got.TerminalDetails = request.TerminalDetails + return reflect.DeepEqual(got, requestWithState(request, state)) + }) +} + // cancelPayload serializes a CancelRequest to JSON bytes for test message payloads. func cancelPayload(t *testing.T, id, reason string) []byte { payload, err := sqmq.Marshal(sqmq.CancelFromEntity(entity.CancelRequest{ID: id, Queue: "q", Reason: reason})) @@ -161,7 +172,7 @@ func TestProcess_AlreadyTerminal_NoOp(t *testing.T) { reqStore := storagemock.NewMockRequestStore(ctrl) reqStore.EXPECT().Get(gomock.Any(), "1").Return(entity.Request{ - ID: "1", Queue: "q", State: entity.RequestStateCancelled, Version: 5, + ID: "1", Queue: "q", State: entity.RequestStateLanded, Version: 5, }, nil) store := orchstoragemock.NewMockStorage(ctrl) @@ -194,10 +205,17 @@ func TestProcess_CancelsUnbatchedRequest(t *testing.T) { ctrl := gomock.NewController(t) registry, pub := newRegistry(t, ctrl) + var terminalLogs []entity.RequestLog var publishedTopics []string pub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - 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) + require.NoError(t, err) + terminalLogs = append(terminalLogs, entry) + if len(terminalLogs) == 1 { + return fmt.Errorf("publish unavailable") + } return nil }).AnyTimes() @@ -206,11 +224,17 @@ func TestProcess_CancelsUnbatchedRequest(t *testing.T) { cancelling := entity.Request{ID: "1", Queue: "q", State: entity.RequestStateCancelling, Version: 3} // Two-step transition: mark Cancelling, then — after TerminateRequest re-reads // the request — the terminal Cancelled CAS. + var terminal entity.Request gomock.InOrder( reqStore.EXPECT().Get(gomock.Any(), "1").Return(started, nil), reqStore.EXPECT().Update(gomock.Any(), requestWithState(started, entity.RequestStateCancelling), int32(2), int32(3)).Return(nil), reqStore.EXPECT().Get(gomock.Any(), "1").Return(cancelling, nil), - reqStore.EXPECT().Update(gomock.Any(), requestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).Return(nil), + reqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).DoAndReturn(func(_ context.Context, snapshot entity.Request, _, newVersion int32) error { + terminal = snapshot + terminal.Version = newVersion + return nil + }), + reqStore.EXPECT().Get(gomock.Any(), "1").DoAndReturn(func(context.Context, string) (entity.Request, error) { return terminal, nil }), ) batchStore := storagemock.NewMockBatchStore(ctrl) @@ -223,10 +247,15 @@ func TestProcess_CancelsUnbatchedRequest(t *testing.T) { controller := newController(t, store, registry) err := controller.Process(context.Background(), newDelivery(t, ctrl, cancelPayload(t, "1", "user changed mind"), "1")) - require.NoError(t, err) + require.Error(t, err) + require.NoError(t, controller.Process(context.Background(), newDelivery(t, ctrl, cancelPayload(t, "1", "changed reason"), "1"))) + require.Len(t, terminalLogs, 2) + assert.Equal(t, terminalLogs[0], terminalLogs[1]) + assert.Equal(t, "user changed mind", terminalLogs[1].Metadata["reason"]) + assert.Equal(t, int32(4), terminalLogs[1].RequestVersion) // Only the request log entry should have been published. - assert.Equal(t, []string{"log"}, publishedTopics) + assert.Equal(t, []string{"log", "log"}, publishedTopics) } // TestProcess_AlreadyCancelling_SkipsMarkCancelling exercises the idempotent @@ -244,7 +273,7 @@ func TestProcess_AlreadyCancelling_SkipsMarkCancelling(t *testing.T) { // see Cancelling (the prior pass already recorded intent). reqStore.EXPECT().Get(gomock.Any(), "1").Return(cancelling, nil).Times(2) // Only the terminal CAS — the mark-cancelling step is a no-op. - reqStore.EXPECT().Update(gomock.Any(), requestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).Return(nil) + reqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).Return(nil) batchStore := storagemock.NewMockBatchStore(ctrl) @@ -301,7 +330,7 @@ func TestProcess_UnbatchedVersionMismatch_Retryable(t *testing.T) { reqStore.EXPECT().Get(gomock.Any(), "1").Return(started, nil), reqStore.EXPECT().Update(gomock.Any(), requestWithState(started, entity.RequestStateCancelling), int32(2), int32(3)).Return(nil), reqStore.EXPECT().Get(gomock.Any(), "1").Return(cancelling, nil), - reqStore.EXPECT().Update(gomock.Any(), requestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)). + reqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)). Return(storage.ErrVersionMismatch), ) @@ -579,7 +608,7 @@ func TestProcess_BatchedWithoutMatchCancelsRequest(t *testing.T) { requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil), requestStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateCancelling), int32(2), int32(3)).Return(nil), requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(cancelling, nil), - requestStore.EXPECT().Update(gomock.Any(), requestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).Return(nil), + requestStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).Return(nil), ) batchStore := storagemock.NewMockBatchStore(ctrl) @@ -615,7 +644,7 @@ func TestProcess_CreatingBatchDoesNotSuppressRequestCancellation(t *testing.T) { requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil), requestStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateCancelling), int32(2), int32(3)).Return(nil), requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(cancelling, nil), - requestStore.EXPECT().Update(gomock.Any(), requestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).Return(nil), + requestStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(cancelling, entity.RequestStateCancelled), int32(3), int32(4)).Return(nil), ) batchStore := storagemock.NewMockBatchStore(ctrl) diff --git a/submitqueue/orchestrator/controller/conclude/conclude_test.go b/submitqueue/orchestrator/controller/conclude/conclude_test.go index 21c5f1c18..fa7f6adc8 100644 --- a/submitqueue/orchestrator/controller/conclude/conclude_test.go +++ b/submitqueue/orchestrator/controller/conclude/conclude_test.go @@ -17,6 +17,7 @@ package conclude import ( "context" "fmt" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -46,9 +47,15 @@ func (f staticStorageFactory) For(orchstorage.Config) (orchstorage.Storage, erro return f.store, nil } -func requestWithState(request entity.Request, state entity.RequestState) entity.Request { +func requestWithState(request entity.Request, state entity.RequestState) gomock.Matcher { request.State = state - return request + return gomock.Cond(func(got entity.Request) bool { + if got.TerminalDetails.TimestampMs <= 0 { + return false + } + got.TerminalDetails = request.TerminalDetails + return reflect.DeepEqual(got, request) + }) } // batchIDPayload serializes a BatchID to JSON bytes for test message payloads. diff --git a/submitqueue/orchestrator/controller/dlq/dlq_test.go b/submitqueue/orchestrator/controller/dlq/dlq_test.go index 02fd82063..07a63e949 100644 --- a/submitqueue/orchestrator/controller/dlq/dlq_test.go +++ b/submitqueue/orchestrator/controller/dlq/dlq_test.go @@ -17,6 +17,7 @@ package dlq import ( "context" "fmt" + "reflect" "testing" orchstorage "github.com/uber/submitqueue/submitqueue/orchestrator/extension/storage" @@ -64,9 +65,15 @@ func batchWithState(batch entity.Batch, state entity.BatchState) entity.Batch { return batch } -func requestWithState(request entity.Request, state entity.RequestState) entity.Request { +func requestWithState(request entity.Request, state entity.RequestState) gomock.Matcher { request.State = state - return request + return gomock.Cond(func(got entity.Request) bool { + if got.TerminalDetails.TimestampMs <= 0 { + return false + } + got.TerminalDetails = request.TerminalDetails + return reflect.DeepEqual(got, request) + }) } // failRequest diff --git a/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal.go b/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal.go index 97ff06fe0..5b1b6a447 100644 --- a/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal.go +++ b/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal.go @@ -114,7 +114,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "partition_key", msg.PartitionKey, ) - // Short-circuit halted requests: the cancel path owns driving them terminal. + if request.State == entity.RequestStateError { + return corerequest.PublishTerminalRequestLog(ctx, c.registry, request) + } + if entity.IsRequestStateHalted(request.State) { metrics.NamedCounter(c.metricsScope, opName, "skipped_halted", 1, metrics.TagsFromContext(ctx)...) c.logger.Infow("skipping landconflict signal for halted request", @@ -165,39 +168,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return nil // Success - message will be acked } -// failRequest drives the request to terminal RequestStateError and records the -// conflict reason on the request log. A not-landable verdict is an expected -// terminal outcome of the check, so the request is concluded here directly. -// -// Idempotent under at-least-once delivery: a redelivery whose request is already -// in Error skips the state CAS but still publishes the log (so a prior attempt -// that flipped the state but failed before logging is repaired); a request that -// reached a different terminal state (e.g. a racing cancel) is left untouched. func (c *Controller) failRequest(ctx context.Context, store orchstorage.Storage, request entity.Request, reason string) error { - switch { - case request.State == entity.RequestStateError: - // Idempotent retry: a prior delivery already wrote Error. Fall through to - // the log publish. - case entity.IsRequestStateTerminal(request.State): - c.logger.Warnw("request already in different terminal state, skipping fail", - "request_id", request.ID, - "state", string(request.State), - ) - return nil - default: - newVersion := request.Version + 1 - request.State = entity.RequestStateError - if err := store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { - return fmt.Errorf("failed to update request %s state to error: %w", request.ID, err) - } - request.Version = newVersion - } - - logEntry := entity.NewRequestStatusLog(request.Queue, request.ID, entity.RequestStatusError, request.Version, reason, nil) - if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID, ""); err != nil { - return fmt.Errorf("failed to publish request log for %s: %w", request.ID, err) - } - return nil + _, err := corerequest.TerminateRequest(ctx, store, c.registry, request.ID, entity.RequestStateError, reason, nil) + return err } // publishRequestID publishes a request ID to the given topic key, stamped with diff --git a/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal_test.go b/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal_test.go index ef2571383..9923f0d66 100644 --- a/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal_test.go +++ b/submitqueue/orchestrator/controller/landconflictsignal/landconflictsignal_test.go @@ -17,6 +17,7 @@ package landconflictsignal import ( "context" "fmt" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -51,6 +52,16 @@ func requestWithState(request entity.Request, state entity.RequestState) entity. return request } +func terminalRequestWithState(request entity.Request, state entity.RequestState) gomock.Matcher { + return gomock.Cond(func(got entity.Request) bool { + if got.TerminalDetails.TimestampMs <= 0 { + return false + } + got.TerminalDetails = request.TerminalDetails + return reflect.DeepEqual(got, requestWithState(request, state)) + }) +} + func resultPayload(t *testing.T, res runwaymq.MergeResult) []byte { if res.QueueName == "" { res.QueueName = testQueue @@ -146,15 +157,18 @@ func TestProcess_NotLandableMarksRequestError(t *testing.T) { reqStore := storagemock.NewMockRequestStore(ctrl) request := entity.Request{ID: testRequestID, Queue: testQueue, State: entity.RequestStateStarted, Version: 1} - reqStore.EXPECT().Get(gomock.Any(), testRequestID).Return(request, nil) + reqStore.EXPECT().Get(gomock.Any(), testRequestID).DoAndReturn(func(context.Context, string) (entity.Request, error) { return request, nil }).Times(3) // The request is driven to terminal Error inline (version 1 -> 2). - reqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) + reqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(request, entity.RequestStateError), int32(1), int32(2)).DoAndReturn(func(_ context.Context, snapshot entity.Request, _, newVersion int32) error { + request = snapshot + request.Version = newVersion + return nil + }) store := orchstoragemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(reqStore).AnyTimes() - // One publish is expected — the terminal log entry to the log topic. A publish - // to the batch topic would be a bug (gomock fails on the unexpected 2nd call). + var terminalLogs []entity.RequestLog var gotTopic string var gotPayload []byte pub := queuemock.NewMockPublisher(ctrl) @@ -162,9 +176,15 @@ func TestProcess_NotLandableMarksRequestError(t *testing.T) { func(_ context.Context, topic string, msg entityqueue.Message) error { gotTopic = topic gotPayload = msg.Payload + entry, err := sqmq.UnmarshalRequestLog(msg.Payload) + require.NoError(t, err) + terminalLogs = append(terminalLogs, entry) + if len(terminalLogs) == 1 { + return fmt.Errorf("publish unavailable") + } return nil }, - ) + ).Times(2) q := queuemock.NewMockQueue(ctrl) q.EXPECT().Publisher().Return(pub).AnyTimes() registry, err := consumer.NewTopicRegistry( @@ -180,10 +200,13 @@ func TestProcess_NotLandableMarksRequestError(t *testing.T) { res := runwaymq.MergeResult{Id: testRequestID, Outcome: runwaypb.Outcome_FAILED, Reason: "conflict in foo.go"} msg := entityqueue.NewMessage(testRequestID, resultPayload(t, res), testQueue, nil) - // Not-landable is an expected terminal outcome, so Process acks (no error). + require.Error(t, controller.Process(context.Background(), newDelivery(ctrl, msg))) + res.Outcome, res.Reason = runwaypb.Outcome_SUCCEEDED, "different retry result" + msg = entityqueue.NewMessage(testRequestID, resultPayload(t, res), testQueue, nil) require.NoError(t, controller.Process(context.Background(), newDelivery(ctrl, msg))) + require.Len(t, terminalLogs, 2) + assert.Equal(t, terminalLogs[0], terminalLogs[1]) - // The single publish is the terminal log entry carrying the conflict reason. assert.Equal(t, "log", gotTopic) logEntry, err := sqmq.UnmarshalRequestLog(gotPayload) require.NoError(t, err) @@ -197,10 +220,11 @@ func TestFailRequest_UpdateFailureLeavesRequestUnchanged(t *testing.T) { request := entity.Request{ID: testRequestID, Queue: testQueue, State: entity.RequestStateStarted, Version: 1} reqStore := storagemock.NewMockRequestStore(ctrl) - reqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(fmt.Errorf("db down")) + reqStore.EXPECT().Get(gomock.Any(), testRequestID).Return(request, nil) + reqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(fmt.Errorf("db down")) store := orchstoragemock.NewMockStorage(ctrl) - store.EXPECT().GetRequestStore().Return(reqStore) + store.EXPECT().GetRequestStore().Return(reqStore).AnyTimes() controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, consumer.TopicRegistry{}, runwaymq.TopicKeyMergeConflictCheckSignal, "orchestrator-landconflictsignal") diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index 99d1ea363..ab4b4d676 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -133,10 +133,11 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "partition_key", msg.PartitionKey, ) - // Short-circuit if the request has been halted — either it already reached a - // terminal state, or the cancel controller has recorded a cancellation intent - // (RequestStateCancelling). Without this guard we would still publish to batch - // and spawn a batch for a request that should never proceed. + if request.State == entity.RequestStateError { + return corerequest.PublishTerminalRequestLog(ctx, c.registry, request) + } + + // Cancellation intent halts forward progress even before the terminal outcome. if entity.IsRequestStateHalted(request.State) { coremetrics.NamedCounter(c.metricsScope, "process", "skipped_halted", 1, coremetrics.TagsFromContext(ctx)...) c.logger.Infow("skipping validate for halted request", diff --git a/submitqueue/orchestrator/controller/validate/validate_test.go b/submitqueue/orchestrator/controller/validate/validate_test.go index 4c2e96edb..b4e740352 100644 --- a/submitqueue/orchestrator/controller/validate/validate_test.go +++ b/submitqueue/orchestrator/controller/validate/validate_test.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -80,6 +81,16 @@ func requestWithState(request entity.Request, state entity.RequestState) entity. return request } +func terminalRequestWithState(request entity.Request, state entity.RequestState) gomock.Matcher { + return gomock.Cond(func(got entity.Request) bool { + if got.TerminalDetails.TimestampMs <= 0 { + return false + } + got.TerminalDetails = request.TerminalDetails + return reflect.DeepEqual(got, requestWithState(request, state)) + }) +} + // requestIDPayload serializes a RequestID to JSON bytes for test message payloads. func requestIDPayload(t *testing.T, id string) []byte { payload, err := sqmq.MarshalID(sqmq.TopicKeyValidate, id, "test-queue") @@ -536,7 +547,7 @@ func TestController_Process_DuplicateDetection(t *testing.T) { if tt.wantRejected { // A detected duplicate is terminated (Started → Error) rather than // dead-lettered, then the delivery is acked. - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) + mockReqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) } store := orchstoragemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() @@ -614,7 +625,7 @@ func TestController_Process_TerminalShortCircuit(t *testing.T) { for _, state := range []entity.RequestState{ entity.RequestStateCancelled, entity.RequestStateLanded, - entity.RequestStateError, + entity.RequestStateCancelling, } { t.Run(string(state), func(t *testing.T) { ctrl := gomock.NewController(t) @@ -709,7 +720,7 @@ func TestController_Process_CustomValidatorFails(t *testing.T) { store, mockReqStore := newMockStorage(ctrl, request) store.EXPECT().GetChangeStore().Return(newMockChangeStore(ctrl)).AnyTimes() // A validator rejection terminates the request (Started → Error) rather than dead-lettering it. - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) + mockReqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) logger := zaptest.NewLogger(t).Sugar() @@ -843,10 +854,18 @@ func TestController_Process_CustomValidatorFailure_TerminationPublishFails(t *te State: entity.RequestStateStarted, Version: 1, } - store, mockReqStore := newMockStorage(ctrl, request) + mockReqStore := storagemock.NewMockRequestStore(ctrl) + mockReqStore.EXPECT().Get(gomock.Any(), request.ID).DoAndReturn(func(context.Context, string) (entity.Request, error) { return request, nil }).Times(3) + store := orchstoragemock.NewMockStorage(ctrl) + store.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() store.EXPECT().GetChangeStore().Return(newMockChangeStore(ctrl)).AnyTimes() - mockReqStore.EXPECT().Update(gomock.Any(), requestWithState(request, entity.RequestStateError), int32(1), int32(2)).Return(nil) + mockReqStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(request, entity.RequestStateError), int32(1), int32(2)).DoAndReturn(func(_ context.Context, snapshot entity.Request, _, newVersion int32) error { + request = snapshot + request.Version = newVersion + return nil + }) + var terminalLogs []entity.RequestLog mockPub := queuemock.NewMockPublisher(ctrl) // Fail only the terminal log. The "validating" entry this stage now emits on // the way in goes to the same topic, so failing every publish would abort @@ -855,7 +874,10 @@ func TestController_Process_CustomValidatorFailure_TerminationPublishFails(t *te func(_ context.Context, _ string, msg entityqueue.Message) error { entry, err := sqmq.UnmarshalRequestLog(msg.Payload) if err == nil && entry.Status == entity.RequestStatusError { - return fmt.Errorf("publish boom") + terminalLogs = append(terminalLogs, entry) + if len(terminalLogs) == 1 { + return fmt.Errorf("publish boom") + } } return nil }, @@ -871,7 +893,7 @@ func TestController_Process_CustomValidatorFailure_TerminationPublishFails(t *te require.NoError(t, err) cpFactory := changeprovidermock.NewMockFactory(ctrl) - cpFactory.EXPECT().For(gomock.Any()).Return(&mockChangeProvider{}, nil).AnyTimes() + cpFactory.EXPECT().For(gomock.Any()).Return(&mockChangeProvider{}, nil) mockValidator := validatormock.NewMockValidator(ctrl) mockValidator.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(fmt.Errorf("%w: some validation error", validator.ErrRejected)) @@ -890,4 +912,10 @@ func TestController_Process_CustomValidatorFailure_TerminationPublishFails(t *te err = controller.Process(context.Background(), delivery) require.Error(t, err) assert.ErrorContains(t, err, "failed to terminate rejected request") + require.NoError(t, controller.Process(context.Background(), delivery)) + require.Len(t, terminalLogs, 2) + assert.Equal(t, terminalLogs[0], terminalLogs[1]) + assert.Contains(t, terminalLogs[1].LastError, "some validation error") + assert.Equal(t, request.TerminalDetails.TimestampMs, terminalLogs[1].TimestampMs) + assert.Equal(t, int32(2), terminalLogs[1].RequestVersion) } diff --git a/submitqueue/orchestrator/core/request/BUILD.bazel b/submitqueue/orchestrator/core/request/BUILD.bazel index ae3aa7e02..4caac60f7 100644 --- a/submitqueue/orchestrator/core/request/BUILD.bazel +++ b/submitqueue/orchestrator/core/request/BUILD.bazel @@ -4,6 +4,7 @@ go_library( name = "go_default_library", srcs = [ "log.go", + "terminal_log.go", "terminate.go", ], importpath = "github.com/uber/submitqueue/submitqueue/orchestrator/core/request", @@ -23,6 +24,7 @@ go_test( name = "go_default_test", srcs = [ "log_test.go", + "terminal_log_test.go", "terminate_test.go", ], embed = [":go_default_library"], diff --git a/submitqueue/orchestrator/core/request/terminal_log.go b/submitqueue/orchestrator/core/request/terminal_log.go new file mode 100644 index 000000000..bb0a26fa3 --- /dev/null +++ b/submitqueue/orchestrator/core/request/terminal_log.go @@ -0,0 +1,46 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package request + +import ( + "context" + "fmt" + "maps" + + "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/submitqueue/entity" +) + +// PublishTerminalRequestLog replays the terminal facts of a persisted request snapshot. +// Legacy snapshots without terminal details publish an explicitly incomplete diagnostic. +func PublishTerminalRequestLog(ctx context.Context, registry consumer.TopicRegistry, request entity.Request) error { + status, err := terminalStateToStatus(request.State) + if err != nil { + return err + } + details := request.TerminalDetails + logEntry := entity.NewRequestStatusLog(request.Queue, request.ID, status, request.Version, details.LastError, maps.Clone(details.Metadata)) + if details.TimestampMs == 0 { + logEntry.LastError = "" + logEntry.Metadata = map[string]string{entity.TerminalDetailsUnavailable: "true"} + } else { + logEntry.TimestampMs = details.TimestampMs + } + // No occurrence: repeats are redeliveries of the same terminal outcome. + if err := PublishLog(ctx, registry, logEntry, request.ID, ""); err != nil { + return fmt.Errorf("failed to publish request log for %s: %w", request.ID, err) + } + return nil +} diff --git a/submitqueue/orchestrator/core/request/terminal_log_test.go b/submitqueue/orchestrator/core/request/terminal_log_test.go new file mode 100644 index 000000000..3d9c25ea5 --- /dev/null +++ b/submitqueue/orchestrator/core/request/terminal_log_test.go @@ -0,0 +1,67 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package request + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/submitqueue/entity" + "go.uber.org/mock/gomock" +) + +func TestPublishTerminalRequestLog(t *testing.T) { + for _, tc := range []struct { + name string + state entity.RequestState + details entity.RequestTerminalDetails + wantStatus entity.RequestStatus + }{ + {name: "landed", state: entity.RequestStateLanded, details: entity.RequestTerminalDetails{TimestampMs: 123}, wantStatus: entity.RequestStatusLanded}, + {name: "cancelled", state: entity.RequestStateCancelled, details: entity.RequestTerminalDetails{TimestampMs: 123, Metadata: map[string]string{"reason": "original"}}, wantStatus: entity.RequestStatusCancelled}, + {name: "legacy", state: entity.RequestStateError, wantStatus: entity.RequestStatusError}, + {name: "non-terminal rejected", state: entity.RequestStateStarted}, + } { + t.Run(tc.name, func(t *testing.T) { + registry, logs := recordingRegistry(t, gomock.NewController(t), nil) + request := entity.Request{ID: "1", Queue: "q", State: tc.state, Version: 3, TerminalDetails: tc.details} + err := PublishTerminalRequestLog(context.Background(), registry, request) + if tc.wantStatus == entity.RequestStatusUnknown { + require.Error(t, err) + assert.Empty(t, *logs) + return + } + require.NoError(t, err) + require.Len(t, *logs, 1) + log := (*logs)[0] + assert.Equal(t, tc.wantStatus, log.Status) + assert.Equal(t, request.Version, log.RequestVersion) + if tc.details.TimestampMs == 0 { + assert.Positive(t, log.TimestampMs) + assert.Empty(t, log.LastError) + assert.Equal(t, map[string]string{entity.TerminalDetailsUnavailable: "true"}, log.Metadata) + } else { + assert.Equal(t, tc.details.TimestampMs, log.TimestampMs) + assert.Equal(t, tc.details.LastError, log.LastError) + assert.Equal(t, len(tc.details.Metadata), len(log.Metadata)) + for key, value := range tc.details.Metadata { + assert.Equal(t, value, log.Metadata[key]) + } + } + }) + } +} diff --git a/submitqueue/orchestrator/core/request/terminate.go b/submitqueue/orchestrator/core/request/terminate.go index 73f559ef7..234624151 100644 --- a/submitqueue/orchestrator/core/request/terminate.go +++ b/submitqueue/orchestrator/core/request/terminate.go @@ -18,6 +18,8 @@ import ( "context" "errors" "fmt" + "maps" + "time" "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/submitqueue/entity" @@ -86,7 +88,7 @@ type TerminationResult struct { // - the request is not found: nothing is done (TerminationOutcomeNotFound). // // The caller owns all logging and metrics — TerminationResult carries the state context needed to do so. -// lastError and metadata are attached to the published RequestLog for diagnosis. +// lastError and metadata are captured only by the winning terminal write. // // A storage.ErrVersionMismatch from the conditional write is returned as-is (it is // intrinsically retryable) so the caller's next attempt re-reads and re-evaluates. @@ -99,7 +101,7 @@ func TerminateRequest( lastError string, metadata map[string]string, ) (TerminationResult, error) { - status, err := terminalStateToStatus(targetState) + _, err := terminalStateToStatus(targetState) if err != nil { return TerminationResult{}, err } @@ -112,11 +114,7 @@ func TerminateRequest( return TerminationResult{}, fmt.Errorf("failed to get request %s: %w", requestID, err) } - // logVersion is the request version reflected in the published terminal log. - // It stays at the current version on the idempotent same-state path and - // advances to the new version only after a successful reconciling write. beforeState := request.State - logVersion := request.Version outcome := TerminationOutcomeSuccess switch { case request.State == targetState: @@ -134,18 +132,19 @@ func TerminateRequest( default: newVersion := request.Version + 1 request.State = targetState + request.TerminalDetails = entity.RequestTerminalDetails{ + TimestampMs: time.Now().UnixMilli(), + LastError: lastError, + Metadata: maps.Clone(metadata), + } if err := store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { return TerminationResult{}, fmt.Errorf("failed to update request %s state to %s: %w", requestID, targetState, err) } request.Version = newVersion - logVersion = request.Version } - logEntry := entity.NewRequestStatusLog(request.Queue, requestID, status, logVersion, lastError, metadata) - // No occurrence: a request reaches a terminal status once, so every repeat - // is a redelivery of that one event and should dedupe away. - if err := PublishLog(ctx, registry, logEntry, requestID, ""); err != nil { - return TerminationResult{}, fmt.Errorf("failed to publish request log for %s: %w", requestID, err) + if err := PublishTerminalRequestLog(ctx, registry, request); err != nil { + return TerminationResult{}, err } return TerminationResult{ diff --git a/submitqueue/orchestrator/core/request/terminate_test.go b/submitqueue/orchestrator/core/request/terminate_test.go index 87b483189..f83d2726a 100644 --- a/submitqueue/orchestrator/core/request/terminate_test.go +++ b/submitqueue/orchestrator/core/request/terminate_test.go @@ -17,6 +17,7 @@ package request import ( "context" "fmt" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -38,6 +39,16 @@ func requestWithState(request entity.Request, state entity.RequestState) entity. return request } +func terminalRequestWithState(request entity.Request, state entity.RequestState) gomock.Matcher { + return gomock.Cond(func(got entity.Request) bool { + if got.TerminalDetails.TimestampMs <= 0 { + return false + } + got.TerminalDetails = request.TerminalDetails + return reflect.DeepEqual(got, requestWithState(request, state)) + }) +} + // recordingRegistry returns a registry whose publisher appends every published // request log to *logs and returns publishErr. It lets tests assert both that a // terminal log was (or was not) published and what version/status it carried. @@ -109,7 +120,7 @@ func TestTerminateRequest(t *testing.T) { metadata: map[string]string{"source": "validate"}, mockFunc: func(rs *storagemock.MockRequestStore) { rs.EXPECT().Get(gomock.Any(), requestID).Return(validated, nil) - rs.EXPECT().Update(gomock.Any(), requestWithState(validated, entity.RequestStateError), int32(3), int32(4)).Return(nil) + rs.EXPECT().Update(gomock.Any(), terminalRequestWithState(validated, entity.RequestStateError), int32(3), int32(4)).Return(nil) }, wantResult: TerminationResult{ Outcome: TerminationOutcomeSuccess, @@ -137,6 +148,7 @@ func TestTerminateRequest(t *testing.T) { wantLog: func(t *testing.T, log entity.RequestLog) { assert.Equal(t, entity.RequestStatusError, log.Status) assert.Equal(t, int32(5), log.RequestVersion) + assert.Equal(t, "true", log.Metadata[entity.TerminalDetailsUnavailable]) }, }, "already in target terminal state returns republish error": { @@ -165,17 +177,26 @@ func TestTerminateRequest(t *testing.T) { targetState: entity.RequestStateError, mockFunc: func(rs *storagemock.MockRequestStore) { rs.EXPECT().Get(gomock.Any(), requestID).Return(validated, nil) - rs.EXPECT().Update(gomock.Any(), requestWithState(validated, entity.RequestStateError), int32(3), int32(4)).Return(storage.ErrVersionMismatch) + rs.EXPECT().Update(gomock.Any(), terminalRequestWithState(validated, entity.RequestStateError), int32(3), int32(4)).Return(storage.ErrVersionMismatch) }, wantResult: TerminationResult{Outcome: TerminationOutcomeUnknown}, errMsg: "version mismatch", errIs: storage.ErrVersionMismatch, }, + "update storage error does not publish": { + targetState: entity.RequestStateError, + mockFunc: func(rs *storagemock.MockRequestStore) { + rs.EXPECT().Get(gomock.Any(), requestID).Return(validated, nil) + rs.EXPECT().Update(gomock.Any(), terminalRequestWithState(validated, entity.RequestStateError), int32(3), int32(4)).Return(fmt.Errorf("db down")) + }, + wantResult: TerminationResult{Outcome: TerminationOutcomeUnknown}, + errMsg: "db down", + }, "publish error after reconcile": { targetState: entity.RequestStateError, mockFunc: func(rs *storagemock.MockRequestStore) { rs.EXPECT().Get(gomock.Any(), requestID).Return(validated, nil) - rs.EXPECT().Update(gomock.Any(), requestWithState(validated, entity.RequestStateError), int32(3), int32(4)).Return(nil) + rs.EXPECT().Update(gomock.Any(), terminalRequestWithState(validated, entity.RequestStateError), int32(3), int32(4)).Return(nil) }, publishErr: fmt.Errorf("connection refused"), wantResult: TerminationResult{Outcome: TerminationOutcomeUnknown}, @@ -215,3 +236,40 @@ func TestTerminateRequest(t *testing.T) { }) } } + +func TestTerminateRequest_ReplaysWinningFactsAfterPublishFailure(t *testing.T) { + ctrl := gomock.NewController(t) + ctx := context.Background() + stored := entity.Request{ID: "1", Queue: "q", State: entity.RequestStateStarted, Version: 3} + metadata := map[string]string{"reason": "original"} + requestStore := storagemock.NewMockRequestStore(ctrl) + requestStore.EXPECT().Get(gomock.Any(), stored.ID).DoAndReturn(func(context.Context, string) (entity.Request, error) { return stored, nil }).Times(3) + requestStore.EXPECT().Update(gomock.Any(), terminalRequestWithState(stored, entity.RequestStateError), int32(3), int32(4)).DoAndReturn( + func(_ context.Context, snapshot entity.Request, oldVersion, newVersion int32) error { + assert.Equal(t, oldVersion, snapshot.Version) + stored = snapshot + stored.Version = newVersion + metadata["reason"] = "caller mutation" + return nil + }) + store := orchstoragemock.NewMockStorage(ctrl) + store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() + failedRegistry, failedLogs := recordingRegistry(t, ctrl, fmt.Errorf("publisher unavailable")) + _, err := TerminateRequest(ctx, store, failedRegistry, stored.ID, entity.RequestStateError, "original failure", metadata) + require.Error(t, err) + assert.Equal(t, "original", stored.TerminalDetails.Metadata["reason"]) + + registry, logs := recordingRegistry(t, ctrl, nil) + for range 2 { + result, err := TerminateRequest(ctx, store, registry, stored.ID, entity.RequestStateError, "different retry failure", map[string]string{"reason": "different"}) + require.NoError(t, err) + assert.Equal(t, TerminationOutcomeAlreadyInTargetState, result.Outcome) + } + require.Len(t, *failedLogs, 1) + require.Len(t, *logs, 2) + assert.Equal(t, (*failedLogs)[0], (*logs)[0]) + assert.Equal(t, (*logs)[0], (*logs)[1]) + assert.Equal(t, stored.TerminalDetails.TimestampMs, (*logs)[0].TimestampMs) + assert.Equal(t, int32(4), (*logs)[0].RequestVersion) + assert.Equal(t, "original failure", (*logs)[0].LastError) +} diff --git a/submitqueue/orchestrator/extension/storage/mysql/migration/001_request_terminal_details.sql b/submitqueue/orchestrator/extension/storage/mysql/migration/001_request_terminal_details.sql new file mode 100644 index 000000000..a348b825d --- /dev/null +++ b/submitqueue/orchestrator/extension/storage/mysql/migration/001_request_terminal_details.sql @@ -0,0 +1 @@ +ALTER TABLE request ADD COLUMN terminal_details JSON NULL; diff --git a/submitqueue/orchestrator/extension/storage/mysql/migration/BUILD.bazel b/submitqueue/orchestrator/extension/storage/mysql/migration/BUILD.bazel new file mode 100644 index 000000000..33388863e --- /dev/null +++ b/submitqueue/orchestrator/extension/storage/mysql/migration/BUILD.bazel @@ -0,0 +1 @@ +exports_files(["001_request_terminal_details.sql"]) diff --git a/submitqueue/orchestrator/extension/storage/mysql/request_store.go b/submitqueue/orchestrator/extension/storage/mysql/request_store.go index 2de1ea5aa..e287f1178 100644 --- a/submitqueue/orchestrator/extension/storage/mysql/request_store.go +++ b/submitqueue/orchestrator/extension/storage/mysql/request_store.go @@ -48,12 +48,12 @@ func (r *requestStore) Get(ctx context.Context, id string) (ret entity.Request, defer func() { storage.CompleteReadMetricsOp(op, retErr) }() var req entity.Request - var changeURIsJSON []byte + var changeURIsJSON, terminalDetailsJSON []byte err := r.db.QueryRowContext(ctx, - "SELECT id, queue, change_uri, land_strategy, state, version FROM request WHERE queue = ? AND id = ?", + "SELECT id, queue, change_uri, land_strategy, state, version, terminal_details FROM request WHERE queue = ? AND id = ?", r.queue, id, - ).Scan(&req.ID, &req.Queue, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version) + ).Scan(&req.ID, &req.Queue, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version, &terminalDetailsJSON) if errors.Is(err, sql.ErrNoRows) { return entity.Request{}, storage.WrapNotFound(err) @@ -67,6 +67,15 @@ func (r *requestStore) Get(ctx context.Context, id string) (ret entity.Request, return entity.Request{}, fmt.Errorf("failed to unmarshal change URIs for request id=%s: %w", id, err) } + if terminalDetailsJSON != nil { + if err := json.Unmarshal(terminalDetailsJSON, &req.TerminalDetails); err != nil { + return entity.Request{}, fmt.Errorf("failed to decode terminal details for request id=%s: %w", id, err) + } + if req.TerminalDetails.TimestampMs <= 0 { + return entity.Request{}, fmt.Errorf("invalid terminal details timestamp for request id=%s", id) + } + } + return req, nil } @@ -85,9 +94,14 @@ func (r *requestStore) Create(ctx context.Context, request entity.Request) (retE return fmt.Errorf("failed to marshal change URIs for request id=%s: %w", request.ID, err) } + terminalDetailsJSON, err := marshalRequestTerminalDetails(request.TerminalDetails) + if err != nil { + return fmt.Errorf("failed to encode terminal details for request id=%s: %w", request.ID, err) + } + _, err = r.db.ExecContext(ctx, - "INSERT INTO request (id, queue, change_uri, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?)", - request.ID, request.Queue, changeURIsJSON, request.LandStrategy, request.State, request.Version, + "INSERT INTO request (id, queue, change_uri, land_strategy, state, version, terminal_details) VALUES (?, ?, ?, ?, ?, ?, ?)", + request.ID, request.Queue, changeURIsJSON, request.LandStrategy, request.State, request.Version, terminalDetailsJSON, ) if err != nil { var mysqlErr *mysql.MySQLError @@ -115,9 +129,14 @@ func (r *requestStore) Update(ctx context.Context, request entity.Request, oldVe return fmt.Errorf("failed to marshal change URIs for request id=%s: %w", request.ID, err) } + terminalDetailsJSON, err := marshalRequestTerminalDetails(request.TerminalDetails) + if err != nil { + return fmt.Errorf("failed to encode terminal details for request id=%s: %w", request.ID, err) + } + result, err := r.db.ExecContext(ctx, - "UPDATE request SET change_uri = ?, land_strategy = ?, state = ?, version = ? WHERE queue = ? AND id = ? AND version = ?", - changeURIsJSON, request.LandStrategy, request.State, newVersion, request.Queue, request.ID, oldVersion, + "UPDATE request SET change_uri = ?, land_strategy = ?, state = ?, version = ?, terminal_details = ? WHERE queue = ? AND id = ? AND version = ?", + changeURIsJSON, request.LandStrategy, request.State, newVersion, terminalDetailsJSON, request.Queue, request.ID, oldVersion, ) if err != nil { return fmt.Errorf( @@ -143,3 +162,13 @@ func (r *requestStore) Update(ctx context.Context, request entity.Request, oldVe return nil } + +func marshalRequestTerminalDetails(details entity.RequestTerminalDetails) ([]byte, error) { + if details.TimestampMs == 0 && details.LastError == "" && len(details.Metadata) == 0 { + return nil, nil + } + if details.TimestampMs <= 0 { + return nil, fmt.Errorf("terminal details require a positive timestamp") + } + return json.Marshal(details) +} diff --git a/submitqueue/orchestrator/extension/storage/mysql/request_store_test.go b/submitqueue/orchestrator/extension/storage/mysql/request_store_test.go index 76e9bdca2..d3d3f6986 100644 --- a/submitqueue/orchestrator/extension/storage/mysql/request_store_test.go +++ b/submitqueue/orchestrator/extension/storage/mysql/request_store_test.go @@ -67,9 +67,9 @@ func TestRequestStore_Get(t *testing.T) { name: "found", id: want.ID, setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"id", "queue", "change_uri", "land_strategy", "state", "version"}). - AddRow(want.ID, want.Queue, changeURIsJSON, string(want.LandStrategy), string(want.State), want.Version) - mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version FROM request"). + rows := sqlmock.NewRows([]string{"id", "queue", "change_uri", "land_strategy", "state", "version", "terminal_details"}). + AddRow(want.ID, want.Queue, changeURIsJSON, string(want.LandStrategy), string(want.State), want.Version, nil) + mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version, terminal_details FROM request"). WithArgs("monorepo", want.ID). WillReturnRows(rows) }, @@ -79,7 +79,7 @@ func TestRequestStore_Get(t *testing.T) { name: "not found", id: "missing", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version FROM request"). + mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version, terminal_details FROM request"). WithArgs("monorepo", "missing"). WillReturnError(sql.ErrNoRows) }, @@ -90,7 +90,7 @@ func TestRequestStore_Get(t *testing.T) { name: "query error", id: "bad", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version FROM request"). + mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version, terminal_details FROM request"). WithArgs("monorepo", "bad"). WillReturnError(fmt.Errorf("connection reset")) }, @@ -100,9 +100,9 @@ func TestRequestStore_Get(t *testing.T) { name: "malformed change URIs JSON", id: "malformed", setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"id", "queue", "change_uri", "land_strategy", "state", "version"}). - AddRow(want.ID, want.Queue, []byte("not json"), string(want.LandStrategy), string(want.State), want.Version) - mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version FROM request"). + rows := sqlmock.NewRows([]string{"id", "queue", "change_uri", "land_strategy", "state", "version", "terminal_details"}). + AddRow(want.ID, want.Queue, []byte("not json"), string(want.LandStrategy), string(want.State), want.Version, nil) + mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version, terminal_details FROM request"). WithArgs("monorepo", "malformed"). WillReturnRows(rows) }, @@ -110,6 +110,36 @@ func TestRequestStore_Get(t *testing.T) { }, } + for _, tc := range []struct { + name string + json string + want entity.RequestTerminalDetails + wantErr bool + }{ + {name: "terminal diagnostics", json: `{"timestamp_ms":123,"last_error":"original failure","metadata":{"reason":"conflict"}}`, want: entity.RequestTerminalDetails{TimestampMs: 123, LastError: "original failure", Metadata: map[string]string{"reason": "conflict"}}}, + {name: "nil metadata", json: `{"timestamp_ms":123,"metadata":null}`, want: entity.RequestTerminalDetails{TimestampMs: 123}}, + {name: "empty metadata", json: `{"timestamp_ms":123,"metadata":{}}`, want: entity.RequestTerminalDetails{TimestampMs: 123, Metadata: map[string]string{}}}, + {name: "invalid terminal JSON", json: `not json`, wantErr: true}, + {name: "JSON null is not legacy SQL NULL", json: `null`, wantErr: true}, + {name: "missing timestamp is not legacy SQL NULL", json: `{}`, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + db, mock, store := setupRequestStoreTest(t) + defer db.Close() + rows := sqlmock.NewRows([]string{"id", "queue", "change_uri", "land_strategy", "state", "version", "terminal_details"}). + AddRow(want.ID, want.Queue, changeURIsJSON, string(want.LandStrategy), string(entity.RequestStateError), want.Version, []byte(tc.json)) + mock.ExpectQuery("SELECT id, queue, change_uri, land_strategy, state, version, terminal_details FROM request").WithArgs("monorepo", want.ID).WillReturnRows(rows) + got, err := store.Get(context.Background(), want.ID) + if tc.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tc.want, got.TerminalDetails) + } + require.NoError(t, mock.ExpectationsWereMet()) + }) + } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { db, mock, store := setupRequestStoreTest(t) @@ -142,17 +172,25 @@ func TestRequestStore_Create(t *testing.T) { Version: 1, } - tests := []struct { + type testCase struct { name string + request *entity.Request setup func(mock sqlmock.Sqlmock) wantErr bool wantErrIs error - }{ + } + tests := []testCase{ + { + name: "diagnostics without timestamp are rejected", + request: &entity.Request{ID: "1", Queue: "monorepo", TerminalDetails: entity.RequestTerminalDetails{LastError: "must not be erased"}}, + setup: func(sqlmock.Sqlmock) {}, + wantErr: true, + }, { name: "success", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request"). - WithArgs(request.ID, request.Queue, sqlmock.AnyArg(), request.LandStrategy, request.State, request.Version). + WithArgs(request.ID, request.Queue, sqlmock.AnyArg(), request.LandStrategy, request.State, request.Version, []byte(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, @@ -160,7 +198,7 @@ func TestRequestStore_Create(t *testing.T) { name: "duplicate id returns ErrAlreadyExists", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request"). - WithArgs(request.ID, request.Queue, sqlmock.AnyArg(), request.LandStrategy, request.State, request.Version). + WithArgs(request.ID, request.Queue, sqlmock.AnyArg(), request.LandStrategy, request.State, request.Version, []byte(nil)). WillReturnError(&mysql.MySQLError{Number: mysqlErrDuplicateEntry}) }, wantErr: true, @@ -170,13 +208,24 @@ func TestRequestStore_Create(t *testing.T) { name: "other exec error", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request"). - WithArgs(request.ID, request.Queue, sqlmock.AnyArg(), request.LandStrategy, request.State, request.Version). + WithArgs(request.ID, request.Queue, sqlmock.AnyArg(), request.LandStrategy, request.State, request.Version, []byte(nil)). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, }, } + for _, metadata := range []map[string]string{nil, {}, {"reason": "original"}} { + terminal := request + terminal.State = entity.RequestStateError + terminal.TerminalDetails = entity.RequestTerminalDetails{TimestampMs: 123, LastError: "original failure", Metadata: metadata} + encoded, err := json.Marshal(terminal.TerminalDetails) + require.NoError(t, err) + tests = append(tests, testCase{name: fmt.Sprintf("terminal metadata %v", metadata), request: &terminal, setup: func(mock sqlmock.Sqlmock) { + mock.ExpectExec("INSERT INTO request").WithArgs(terminal.ID, terminal.Queue, sqlmock.AnyArg(), terminal.LandStrategy, terminal.State, terminal.Version, encoded).WillReturnResult(sqlmock.NewResult(0, 1)) + }}) + } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { db, mock, store := setupRequestStoreTest(t) @@ -184,7 +233,11 @@ func TestRequestStore_Create(t *testing.T) { tt.setup(mock) - err := store.Create(context.Background(), request) + input := request + if tt.request != nil { + input = *tt.request + } + err := store.Create(context.Background(), input) if tt.wantErr { require.Error(t, err) if tt.wantErrIs != nil { @@ -218,12 +271,18 @@ func TestRequestStore_Update(t *testing.T) { wantErr bool wantErrIs error }{ + { + name: "invalid terminal timestamp is rejected", + request: entity.Request{ID: "1", Queue: "monorepo", TerminalDetails: entity.RequestTerminalDetails{TimestampMs: -1}}, + setup: func(sqlmock.Sqlmock) {}, + wantErr: true, + }, { name: "success", request: request, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request"). - WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, request.Queue, request.ID, oldVersion). + WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, []byte(nil), request.Queue, request.ID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, @@ -232,7 +291,7 @@ func TestRequestStore_Update(t *testing.T) { request: request, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request"). - WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, request.Queue, request.ID, oldVersion). + WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, []byte(nil), request.Queue, request.ID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, @@ -243,7 +302,7 @@ func TestRequestStore_Update(t *testing.T) { request: request, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request"). - WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, request.Queue, request.ID, oldVersion). + WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, []byte(nil), request.Queue, request.ID, oldVersion). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, @@ -253,7 +312,7 @@ func TestRequestStore_Update(t *testing.T) { request: request, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request"). - WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, request.Queue, request.ID, oldVersion). + WithArgs(changeURIsJSON, request.LandStrategy, request.State, newVersion, []byte(nil), request.Queue, request.ID, oldVersion). WillReturnResult(sqlmock.NewErrorResult(fmt.Errorf("driver error"))) }, wantErr: true, @@ -263,7 +322,7 @@ func TestRequestStore_Update(t *testing.T) { request: entity.Request{ID: request.ID, Queue: request.Queue, LandStrategy: request.LandStrategy, State: request.State, Version: request.Version}, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request"). - WithArgs([]byte("null"), request.LandStrategy, request.State, newVersion, request.Queue, request.ID, oldVersion). + WithArgs([]byte("null"), request.LandStrategy, request.State, newVersion, []byte(nil), request.Queue, request.ID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, @@ -279,7 +338,7 @@ func TestRequestStore_Update(t *testing.T) { }, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request"). - WithArgs([]byte("[]"), request.LandStrategy, request.State, newVersion, request.Queue, request.ID, oldVersion). + WithArgs([]byte("[]"), request.LandStrategy, request.State, newVersion, []byte(nil), request.Queue, request.ID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, diff --git a/submitqueue/orchestrator/extension/storage/mysql/schema/README.md b/submitqueue/orchestrator/extension/storage/mysql/schema/README.md index 14c09d619..af79d1d59 100644 --- a/submitqueue/orchestrator/extension/storage/mysql/schema/README.md +++ b/submitqueue/orchestrator/extension/storage/mysql/schema/README.md @@ -27,3 +27,9 @@ Terminal-state records (and their batches) accumulate as the queue processes wor ### Composite primary key: `(queue, uri, request_id)` The `change` table records per-URI claims by in-flight requests. `request_id` is part of the primary key so that concurrent claims on the same URI by different requests coexist as distinct rows — a same-request retry collides on the PK and is a no-op (`INSERT IGNORE`), while a different-request claim is a new row that `GetByURI` surfaces for overlap detection. `queue` leads the key so queue-scoped lookups are primary-key-prefix scans and the table is shardable by queue. + +## Request terminal details + +The request snapshot stores its terminal timestamp and diagnostics atomically with its state and version. SQL NULL denotes unavailable legacy details or a non-terminal request; malformed non-NULL details are a read error, not a legacy fallback. + +For an existing database, apply `migration/001_request_terminal_details.sql` before deploying new writers. Fresh databases use only this directory's CREATE TABLE scripts. Deploy the gateway's incomplete-repair protection before enabling terminal fallback publishers, and replace old consumers before relying on replay repair. Keep the additive column on code rollback so persisted diagnostics survive. diff --git a/submitqueue/orchestrator/extension/storage/mysql/schema/request.sql b/submitqueue/orchestrator/extension/storage/mysql/schema/request.sql index 937b6db35..eb193cacc 100644 --- a/submitqueue/orchestrator/extension/storage/mysql/schema/request.sql +++ b/submitqueue/orchestrator/extension/storage/mysql/schema/request.sql @@ -5,5 +5,6 @@ CREATE TABLE IF NOT EXISTS request ( land_strategy VARCHAR(64) NOT NULL, state VARCHAR(64) NOT NULL, version INT NOT NULL, + terminal_details JSON NULL, PRIMARY KEY (queue, id) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel b/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel index fc8b7f484..cb25f2e76 100644 --- a/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel +++ b/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel @@ -6,6 +6,7 @@ go_test( data = [ "docker-compose.yml", "//submitqueue/gateway/extension/storage/mysql/schema", + "//submitqueue/orchestrator/extension/storage/mysql/migration:001_request_terminal_details.sql", "//submitqueue/orchestrator/extension/storage/mysql/schema", ], tags = [ @@ -13,6 +14,7 @@ go_test( "requires-network", ], deps = [ + "//submitqueue/entity:go_default_library", "//submitqueue/gateway/extension/storage:go_default_library", "//submitqueue/gateway/extension/storage/mysql:go_default_library", "//submitqueue/orchestrator/extension/storage:go_default_library", diff --git a/test/integration/submitqueue/extension/storage/mysql/storage_test.go b/test/integration/submitqueue/extension/storage/mysql/storage_test.go index bbc0f07a2..54cc5140b 100644 --- a/test/integration/submitqueue/extension/storage/mysql/storage_test.go +++ b/test/integration/submitqueue/extension/storage/mysql/storage_test.go @@ -17,8 +17,10 @@ package mysql import ( "context" "database/sql" + "os" "testing" + "github.com/uber/submitqueue/submitqueue/entity" orchstorage "github.com/uber/submitqueue/submitqueue/orchestrator/extension/storage" _ "github.com/go-sql-driver/mysql" @@ -127,3 +129,39 @@ type gatewayMySQLFactory struct { func (f gatewayMySQLFactory) For(config gwstorage.Config) (gwstorage.Storage, error) { return f.backend.For(config.QueueName) } + +func (s *MySQLStorageIntegrationSuite) TestRequestTerminalDetailsMigration() { + t := s.T() + db, err := s.stack.ConnectMySQLService("mysql") + require.NoError(t, err) + defer db.Close() + db.SetMaxOpenConns(1) + ctx := context.Background() + // The connection-local table isolates the old-schema migration from the shared contract suite. + _, err = db.ExecContext(ctx, `CREATE TEMPORARY TABLE request ( + queue VARCHAR(255) NOT NULL, id VARCHAR(255) NOT NULL, + change_uri JSON NOT NULL, land_strategy VARCHAR(64) NOT NULL, + state VARCHAR(64) NOT NULL, version INT NOT NULL, + PRIMARY KEY (queue, id) + )`) + require.NoError(t, err) + _, err = db.ExecContext(ctx, `INSERT INTO request (queue, id, change_uri, land_strategy, state, version) VALUES ('legacy', '1', '[]', '', 'error', 2)`) + require.NoError(t, err) + migration, err := os.ReadFile(testutil.Runfile("submitqueue/orchestrator/extension/storage/mysql/migration/001_request_terminal_details.sql")) + require.NoError(t, err) + _, err = db.ExecContext(ctx, string(migration)) + require.NoError(t, err) + store := mysqlstorage.NewRequestStore(db, tally.NoopScope, "legacy") + request, err := store.Get(ctx, "1") + require.NoError(t, err) + require.Equal(t, entity.RequestTerminalDetails{}, request.TerminalDetails) + request.TerminalDetails = entity.RequestTerminalDetails{TimestampMs: 123, LastError: "persisted failure", Metadata: map[string]string{"reason": "migration"}} + require.NoError(t, store.Update(ctx, request, 2, 3)) + retrieved, err := store.Get(ctx, "1") + require.NoError(t, err) + require.Equal(t, request.TerminalDetails, retrieved.TerminalDetails) + _, err = db.ExecContext(ctx, `UPDATE request SET terminal_details = 'null' WHERE queue = 'legacy' AND id = '1'`) + require.NoError(t, err) + _, err = store.Get(ctx, "1") + require.Error(t, err) +} diff --git a/test/integration/submitqueue/extension/storage/suite.go b/test/integration/submitqueue/extension/storage/suite.go index a85ad023e..c75540c2f 100644 --- a/test/integration/submitqueue/extension/storage/suite.go +++ b/test/integration/submitqueue/extension/storage/suite.go @@ -89,9 +89,10 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { ctx := s.ctx request := entity.Request{ - ID: "test/create-get", - Queue: "test-queue", - State: entity.RequestStateStarted, + ID: "test/create-get", + Queue: "test-queue", + State: entity.RequestStateError, + TerminalDetails: entity.RequestTerminalDetails{TimestampMs: 123, LastError: "original failure", Metadata: map[string]string{"reason": "test"}}, Change: change.Change{ URIs: []string{"github://github.example.com/uber/storage-test/pull/123/abcdef0123456789abcdef0123456789abcdef01"}, }, @@ -114,6 +115,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { assert.Equal(t, request.Change.URIs, retrieved.Change.URIs) assert.Equal(t, request.LandStrategy, retrieved.LandStrategy) assert.Equal(t, request.Version, retrieved.Version) + assert.Equal(t, request.TerminalDetails, retrieved.TerminalDetails) } // TestStorage_CreateAndGet_StackedPRs tests creating and retrieving a request with stacked PRs @@ -175,7 +177,8 @@ func (s *StorageContractSuite) TestStorage_Update() { updated := request updated.Change.URIs = []string{"github://github.example.com/uber/monorepo/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"} updated.LandStrategy = mergestrategy.MergeStrategySquashRebase - updated.State = entity.RequestStateProcessing + updated.State = entity.RequestStateError + updated.TerminalDetails = entity.RequestTerminalDetails{TimestampMs: 123, LastError: "original failure", Metadata: map[string]string{"source": "test"}} err = s.forQueue("test-queue").GetRequestStore().Update(ctx, updated, request.Version, request.Version+1) require.NoError(t, err, "failed to update request") @@ -207,7 +210,8 @@ func (s *StorageContractSuite) TestStorage_OptimisticLocking() { updated := request updated.Change.URIs = []string{"github://github.example.com/uber/monorepo/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"} updated.LandStrategy = mergestrategy.MergeStrategySquashRebase - updated.State = entity.RequestStateProcessing + updated.State = entity.RequestStateError + updated.TerminalDetails = entity.RequestTerminalDetails{TimestampMs: 123, LastError: "original failure", Metadata: map[string]string{"source": "test"}} err = s.forQueue("test-queue").GetRequestStore().Update(ctx, updated, 1, 2) require.NoError(t, err, "update with correct version should succeed") @@ -216,6 +220,7 @@ func (s *StorageContractSuite) TestStorage_OptimisticLocking() { stale.Change.URIs = []string{"github://github.example.com/uber/monorepo/pull/3/cccccccccccccccccccccccccccccccccccccccc"} stale.LandStrategy = mergestrategy.MergeStrategyRebase stale.State = entity.RequestStateLanded + stale.TerminalDetails = entity.RequestTerminalDetails{TimestampMs: 456, Metadata: map[string]string{"replacement": "losing"}} err = s.forQueue("test-queue").GetRequestStore().Update(ctx, stale, 1, 3) assert.Error(t, err, "update with stale version should fail") assert.ErrorIs(t, err, storage.ErrVersionMismatch, "should return ErrVersionMismatch") @@ -233,11 +238,12 @@ func (s *StorageContractSuite) TestStorage_UpdateChangeURIs() { ctx := s.ctx tests := []struct { - name string - uris []string + name string + uris []string + metadata map[string]string }{ - {name: "nil", uris: nil}, - {name: "empty", uris: []string{}}, + {name: "nil", uris: nil, metadata: nil}, + {name: "empty", uris: []string{}, metadata: map[string]string{}}, } for i, tt := range tests { @@ -254,11 +260,14 @@ func (s *StorageContractSuite) TestStorage_UpdateChangeURIs() { updated := request updated.Change.URIs = tt.uris + updated.State = entity.RequestStateCancelled + updated.TerminalDetails = entity.RequestTerminalDetails{TimestampMs: 123, Metadata: tt.metadata} require.NoError(t, s.forQueue("test-queue").GetRequestStore().Update(ctx, updated, request.Version, request.Version+1)) retrieved, err := s.forQueue("test-queue").GetRequestStore().Get(ctx, request.ID) require.NoError(t, err) assert.Equal(t, tt.uris, retrieved.Change.URIs) + assert.Equal(t, updated.TerminalDetails, retrieved.TerminalDetails) }) } }