diff --git a/stovepipe/controller/issuetrack/README.md b/stovepipe/controller/issuetrack/README.md index 1d7606dc0..5f9c79a20 100644 --- a/stovepipe/controller/issuetrack/README.md +++ b/stovepipe/controller/issuetrack/README.md @@ -5,7 +5,7 @@ Issue tracking maintains current issue state and opening/closing history from pe ## Applying one issue 1. Read whether the build reports the issue and load its current state. -2. Reject a source older than the issue's last processed build, using Request order rather than opaque Build IDs. +2. Skip a source older than the issue's last processed build, using Request order rather than opaque Build IDs. 3. Before changing the issue, conditionally save its task/unit key as the current queue cursor **after** reading the issue row. Repeat this conditional write even when resuming the same cursor. 4. Conditionally create or update the issue row. Record the processed build even when its open/closed status stays the same; update the changing build and timestamp only on an actual transition. 5. Retain opening or closing history from the issue row's changing build and timestamp, then materialize its summary and open-issue lookup. On retry, the inclusive cursor revisits that row and repairs all three before proceeding. @@ -14,7 +14,7 @@ Closed issue rows are retained, so their versions cannot reset on reopening. A r ## Ordering requirements -During issue application, the caller must finish the current cursor and its history before moving on or admitting another build. If a replacement worker advances, completing that cursor changes the issue version first. A paused worker's earlier issue write then fails its row guard. A worker that reads later is rejected by source ordering or the conditional cursor write. +During issue application, the caller must finish the current cursor and its history before moving on or admitting another build. If a replacement worker advances, completing that cursor changes the issue version first. A paused worker's earlier issue write then fails its row guard. A worker that reads later skips an issue already processed by a newer source or fails the conditional cursor write. The [issue-log materializer](../../core/issuelog/README.md) also orders summary updates by source request, with closing after opening within the same request. A paused projection writer cannot overwrite a newer summary: its conditional write fails, and reloading the winner repeats the source-order check. Lookup entries are then maintained from the latest summary, not the incoming transition. Summary and lookup failures keep the cursor unfinished, just like history failures. @@ -40,7 +40,7 @@ No closure occurs before all accumulated build issues have been processed. A tas Every delivery processes at most 100 task/unit keys from one ordered range, including a revisited cursor and retained closed rows. Refreshing reads build issues; finalizing reads issue states. The cursor is always inclusive: revisit it to finish the issue and its history, then continue after it. A full batch retains its active phase and requests redelivery. Moving from refreshing to finalizing conditionally saves a reset cursor before any closing work begins. -An unfinished pass completes or is safely abandoned before another source is admitted. A notification that helps finish a different pass requests redelivery before handling its own source. Completed notifications and older sources are ignored, as are refreshes after the same Request has finalized or its terminal pass was abandoned. A terminal pass requires a terminal build chosen by its Request in both active phases. +An unfinished pass completes or is safely abandoned before another source is admitted. A notification that helps finish a different pass requests redelivery before handling its own source. Completed notifications and sources at or below `FinalizedRequestID` are ignored. This terminal boundary advances only when a terminal pass completes or is abandoned, and survives every later refresh. It is independent of the source of the current pass: a newer running observation, empty or nonempty, cannot discard an older terminal pass. Each issue instead retains its newest processed source; older passes skip that issue and continue their range. A newer completed terminal pass still supersedes all earlier notifications, including observations for issues with no retained state. A terminal pass requires a terminal build chosen by its Request in both active phases. Build IDs are opaque; source order comes from Request IDs. All scans are bounded reads under leading primary-key prefixes. There is no transaction spanning progress, issue state, and history. @@ -51,7 +51,7 @@ An error returns to normal consumer retry. The next delivery resumes the cursor 1. Conditionally enter `abandoning`, retaining the source, notification, and cursor. Normal deliveries hold for 30 seconds while this phase is present; they cannot start another pass or resume issue application. 2. If there is a cursor, read its issue and conditionally advance progress after that read. Advance the issue's version without changing its status or transition metadata. This invalidates a paused worker's earlier row guard. If the row has not been created yet, finish that one opening instead: the normal worker can only have saved such a cursor after finding a reported build issue. Creating it prevents the delayed create from succeeding. Closed issue rows are never deleted. 3. Repair the cursor's opening or closing history, summary, and open-issue lookup from the retained row. Record an `issue_tracking_abandoned` Request-history event keyed by the failed notification, with its build ID. -4. Conditionally enter `abandoned`. Keep the source-order boundary and notification identity, so the same delivery and older builds cannot restart the abandoned work. +4. Conditionally enter `abandoned`. Keep the terminal boundary and notification identity, so the same delivery cannot restart the abandoned work. A terminal abandonment also advances `FinalizedRequestID`, preventing that Request and older sources from restarting after later refreshes. Accepted opens and closes remain in place. Recovery does not scan either issue range, close the remaining issues, change Request or Build outcomes, or release build slots. An abandoned ordinary refresh still permits a new refresh or terminal notification for that Request; an abandoned terminal pass permits only newer Requests. A later successful build can reconcile issues left unchanged by the failed pass. @@ -61,8 +61,10 @@ A notification can fail while helping an older pass. Recovery abandons that save Storage and history failures return for retry; hosts must pair recovery with an always-retryable, unlimited-retry DLQ consumer. Only recovery work repeats. Malformed message identities, unresolvable queues, and missing source records are logged and acknowledged. -The reference service registers queue-partitioned `issuetrack` and `issuetrack_dlq` subscriptions. Normal processing uses backend error classification and the extended bounded retry policy, while recovery uses the existing always-retryable DLQ consumer. The DLQ consumer starts before normal consumption and stops after it. Deploy recovery-aware controllers and provision the issue tables before enabling consumption; older controllers do not recognize the abandonment phases. Completed opening passes publish repair notifications; recovery does not. The reference host also consumes repair notifications with a no-op executor; fix dead-letter recovery remains separate. +The reference service registers queue-partitioned `issuetrack` and `issuetrack_dlq` subscriptions. Normal processing uses backend error classification and the extended bounded retry policy, while recovery uses the existing always-retryable DLQ consumer. The DLQ consumer starts before normal consumption and stops after it. Deploy recovery-aware controllers and provision the issue tables before enabling consumption; older controllers do not recognize the abandonment phases. Completed opening passes publish repair notifications; recovery does not. Final pass admission, completion, and superseded final notifications are logged at info level with their build and request IDs. The reference host also consumes repair notifications with a no-op executor; fix dead-letter recovery remains separate. ## Analysis handoff Build analysis publishes refresh or final notifications after persisting build issues. Final notification requires successful terminal detection and the Request's recorded terminal build. Terminal detection returns the complete cumulative issue set, including issues returned by earlier calls, so finalization does not depend on earlier workers finishing their writes. Nonterminal detection may omit earlier issues without removing them from the build's accumulated results. The reference host uses a no-op detector; deployments supply their own detector and register the same normal and recovery paths. + +The issue-tracking table must include `finalized_request_id` before deploying these controllers. It is a non-null binary request ID with an empty default. Existing completed or abandoned terminal records supply their own Request ID as the initial boundary; existing refresh records have no retained terminal boundary. Upgrade normal and recovery controllers together: older writers do not preserve the terminal boundary, and older readers reject closing passes containing task-wide failures. diff --git a/stovepipe/controller/issuetrack/delivery_test.go b/stovepipe/controller/issuetrack/delivery_test.go index 1545efe4c..a05574407 100644 --- a/stovepipe/controller/issuetrack/delivery_test.go +++ b/stovepipe/controller/issuetrack/delivery_test.go @@ -614,3 +614,128 @@ func TestRetryResetsCursorBeforeClosing(t *testing.T) { require.Equal(t, int32(1), f.states[issueKey{"task", "unit"}].Version) require.Len(t, f.logs, 2) } + +func TestNewerRefreshDoesNotDiscardOlderFinalization(t *testing.T) { + for _, observation := range []string{"empty", "unrelated failure", "same failure"} { + t.Run(observation, func(t *testing.T) { + f := newFixture(t) + f.source("build/100", "100", entity.BuildStatusSucceeded) + f.source("build/101", "101", entity.BuildStatusRunning) + f.seedIssue("old", "unit", entity.IssueStatusOpen) + if observation == "unrelated failure" { + f.reportIssue("build/101", "new", "unit") + } else if observation == "same failure" { + f.reportIssue("build/101", "old", "unit") + } + require.NoError(t, f.deliver(f.message("build/101", false), false)) + final := f.message("build/100", true) + require.NoError(t, f.deliver(final, true)) + require.NoError(t, f.deliver(final, false)) + require.Equal(t, entity.IssueTrackingPhaseFinalized, f.progress.Phase) + if observation == "same failure" { + require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{"old", "unit"}].Status) + require.Equal(t, "build/101", f.states[issueKey{"old", "unit"}].LastProcessedBuildID) + require.Empty(t, f.logs) + } else { + require.Equal(t, entity.IssueStatusClosed, f.states[issueKey{"old", "unit"}].Status) + require.Contains(t, f.logs, logKey{issueKey{"old", "unit"}, "build/100", entity.IssueStatusClosed}) + } + if observation == "unrelated failure" { + require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{"new", "unit"}].Status) + require.Equal(t, "build/101", f.states[issueKey{"new", "unit"}].LastProcessedBuildID) + } + }) + } +} + +func TestFinalizedBoundarySurvivesLaterRefreshes(t *testing.T) { + f := newFixture(t) + f.source("build/100", "100", entity.BuildStatusSucceeded) + f.source("build/101", "101", entity.BuildStatusSucceeded) + f.source("build/102", "102", entity.BuildStatusRunning) + f.seedIssue("resolved", "unit", entity.IssueStatusOpen) + final := f.message("build/101", true) + require.NoError(t, f.deliver(final, true)) + require.NoError(t, f.deliver(final, false)) + f.reportIssue("build/102", "new", "unit") + require.NoError(t, f.deliver(f.message("build/102", false), false)) + before := f.progress + for _, build := range []string{"build/100", "build/101"} { + f.reportIssue(build, "resolved", "unit") + f.reportIssue(build, "stale", "unit") + for _, terminal := range []bool{false, true} { + require.NoError(t, f.deliver(f.message(build, terminal), false)) + } + } + require.Equal(t, before, f.progress) + require.Equal(t, entity.IssueStatusClosed, f.states[issueKey{"resolved", "unit"}].Status) + require.NotContains(t, f.states, issueKey{"stale", "unit"}) + require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{"new", "unit"}].Status) + require.Len(t, f.logs, 2) +} + +func TestOlderFinalizationSkipsNewerIssuesAcrossBatches(t *testing.T) { + f := newFixture(t) + f.source("build/100", "100", entity.BuildStatusFailed) + f.source("build/101", "101", entity.BuildStatusRunning) + f.seedIssue("zzz", "resolved", entity.IssueStatusOpen) + for i := 0; i < 105; i++ { + unit := fmt.Sprintf("unit/%03d", i) + f.reportIssue("build/100", "task", unit) + f.reportIssue("build/101", "task", unit) + } + refresh := f.message("build/101", false) + require.NoError(t, f.deliver(refresh, true)) + require.NoError(t, f.deliver(refresh, false)) + final := f.message("build/100", true) + require.NoError(t, f.deliver(final, true)) + require.NoError(t, f.deliver(final, true)) + require.NoError(t, f.deliver(final, true)) + require.NoError(t, f.deliver(final, false)) + require.Equal(t, entity.IssueTrackingPhaseFinalized, f.progress.Phase) + for key, state := range f.states { + if key.task == "task" { + require.Equal(t, entity.IssueStatusOpen, state.Status) + require.Equal(t, "build/101", state.LastProcessedBuildID) + } + } + require.Equal(t, entity.IssueStatusClosed, f.states[issueKey{"zzz", "resolved"}].Status) + require.Len(t, f.logs, 106) +} + +func TestFailedCompletionCheckpointDoesNotLoseFinalization(t *testing.T) { + f := newFixture(t) + f.source("build/100", "100", entity.BuildStatusSucceeded) + f.source("build/101", "101", entity.BuildStatusRunning) + f.seedIssue("old", "unit", entity.IssueStatusOpen) + final := f.message("build/100", true) + require.NoError(t, f.deliver(final, true)) + f.failOnPhase = entity.IssueTrackingPhaseFinalized + require.ErrorIs(t, f.deliver(final, false), assert.AnError) + refresh := f.message("build/101", false) + require.NoError(t, f.deliver(refresh, true)) + require.NoError(t, f.deliver(refresh, false)) + f.reportIssue("build/100", "stale", "unit") + require.NoError(t, f.deliver(f.message("build/100", false), false)) + require.NotContains(t, f.states, issueKey{"stale", "unit"}) + require.Equal(t, entity.IssueStatusClosed, f.states[issueKey{"old", "unit"}].Status) + require.Len(t, f.logs, 1) +} + +func TestLegacyTerminalBoundarySurvivesANewerRefresh(t *testing.T) { + f := newFixture(t) + f.source("build/100", "100", entity.BuildStatusSucceeded) + f.source("build/101", "101", entity.BuildStatusRunning) + legacy := currentCursor() + legacy.Finalize, legacy.Phase = true, entity.IssueTrackingPhaseFinalized + f.retain(legacy) + f.reportIssue("build/101", "new", "unit") + require.NoError(t, f.deliver(f.message("build/101", false), false)) + before := f.progress + f.reportIssue("build/100", "stale", "unit") + require.NoError(t, f.deliver(f.message("build/100", false), false)) + require.NoError(t, f.deliver(f.message("build/100", true), false)) + require.Equal(t, before, f.progress) + require.NotContains(t, f.states, issueKey{"stale", "unit"}) + require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{"new", "unit"}].Status) +} diff --git a/stovepipe/controller/issuetrack/dlq.go b/stovepipe/controller/issuetrack/dlq.go index 14440e55c..e49fbed0c 100644 --- a/stovepipe/controller/issuetrack/dlq.go +++ b/stovepipe/controller/issuetrack/dlq.go @@ -138,20 +138,24 @@ func (c *DLQController) abandonPass(ctx context.Context, store storage.Storage, return progress, c.recordAbandonment(ctx, store, request.Queue, progress) } } else { - order, err := entity.CompareRequestID(request.ID, progress.RequestID) + superseded, err := notificationSuperseded(progress, request.ID) if err != nil { return entity.IssueTrackingState{}, err } - if order < 0 || (order == 0 && progress.Finalize) { + if superseded { return entity.IssueTrackingState{}, nil } + order, err := entity.CompareRequestID(request.ID, progress.RequestID) + if err != nil { + return entity.IssueTrackingState{}, err + } switch progress.Phase { case entity.IssueTrackingPhaseRefreshing, entity.IssueTrackingPhaseFinalizing, entity.IssueTrackingPhaseAbandoning: - if order == 0 { + if order <= 0 { return entity.IssueTrackingState{}, nil } default: - progress = entity.IssueTrackingState{Version: progress.Version} + progress = entity.IssueTrackingState{Version: progress.Version, FinalizedRequestID: finalizedRequestID(progress)} } } } diff --git a/stovepipe/controller/issuetrack/dlq_test.go b/stovepipe/controller/issuetrack/dlq_test.go index af723ae5f..6e3f4d1f5 100644 --- a/stovepipe/controller/issuetrack/dlq_test.go +++ b/stovepipe/controller/issuetrack/dlq_test.go @@ -436,3 +436,37 @@ func TestRecoveryAcknowledgesUnusableMessages(t *testing.T) { }) } } + +func TestRecoveryRetainsTerminalBoundaryAcrossANewerAbandonedRefresh(t *testing.T) { + f := newRecoveryFixture(t) + f.source("build/100", "100", entity.BuildStatusSucceeded) + f.source("build/101", "101", entity.BuildStatusRunning) + final := f.message("build/100", true) + require.NoError(t, f.deliver(final, true)) + require.NoError(t, f.deliver(final, false)) + require.NoError(t, f.recover(f.message("build/101", false), false)) + before := f.progress + f.reportIssue("build/100", "stale", "unit") + require.NoError(t, f.deliver(f.message("build/100", false), false)) + require.NoError(t, f.recover(f.message("build/100", true), false)) + require.Equal(t, before, f.progress) + require.Empty(t, f.states) +} + +func TestRecoveryOfOlderTerminalAfterNewerRefreshRetainsNewerEvidence(t *testing.T) { + f := newRecoveryFixture(t) + f.source("build/100", "100", entity.BuildStatusSucceeded) + f.source("build/101", "101", entity.BuildStatusRunning) + f.reportIssue("build/101", "new", "unit") + require.NoError(t, f.deliver(f.message("build/101", false), false)) + newer := f.states[issueKey{"new", "unit"}] + require.NoError(t, f.recover(f.message("build/100", true), false)) + require.Equal(t, entity.IssueTrackingPhaseAbandoned, f.progress.Phase) + require.Equal(t, "build/100", f.progress.BuildID) + require.Equal(t, newer, f.states[issueKey{"new", "unit"}]) + require.Len(t, f.events, 1) + require.NoError(t, f.deliver(f.message("build/101", false), false)) + before := f.progress + require.NoError(t, f.recover(f.message("build/100", true), false)) + require.Equal(t, before, f.progress) +} diff --git a/stovepipe/controller/issuetrack/issuetrack.go b/stovepipe/controller/issuetrack/issuetrack.go index 61840440b..fca361c1a 100644 --- a/stovepipe/controller/issuetrack/issuetrack.go +++ b/stovepipe/controller/issuetrack/issuetrack.go @@ -121,13 +121,15 @@ func (c *Controller) trackBuild(ctx context.Context, delivery consumer.Delivery, if progress.NotificationID == notificationID { return nil } - order, err := entity.CompareRequestID(request.ID, progress.RequestID) + superseded, err := notificationSuperseded(progress, request.ID) if err != nil { return err } - if order < 0 || (order == 0 && progress.Finalize) { - c.logger.Debugw("ignoring superseded issue tracking notification", - "queue", queue, "build_id", build.ID, "request_id", request.ID, "current_request_id", progress.RequestID) + if superseded { + if notification.GetFinalize() { + c.logger.Infow("ignoring superseded issue finalization", + "queue", queue, "build_id", build.ID, "request_id", request.ID, "finalized_request_id", finalizedRequestID(progress)) + } return nil } } @@ -136,12 +138,13 @@ func (c *Controller) trackBuild(ctx context.Context, delivery consumer.Delivery, return fmt.Errorf("issue tracking version exhausted") } next := entity.IssueTrackingState{ - RequestID: request.ID, - BuildID: build.ID, - NotificationID: notificationID, - Phase: entity.IssueTrackingPhaseRefreshing, - Finalize: notification.GetFinalize(), - Version: progress.Version + 1, + RequestID: request.ID, + FinalizedRequestID: finalizedRequestID(progress), + BuildID: build.ID, + NotificationID: notificationID, + Phase: entity.IssueTrackingPhaseRefreshing, + Finalize: notification.GetFinalize(), + Version: progress.Version + 1, } if next.Finalize { if err := requireTerminalBuild(build, request); err != nil { @@ -160,6 +163,9 @@ func (c *Controller) trackBuild(ctx context.Context, delivery consumer.Delivery, if err != nil { return fmt.Errorf("start issue tracking pass: %w", err) } + if next.Finalize { + c.logger.Infow("starting issue finalization", "queue", queue, "build_id", build.ID, "request_id", request.ID, "notification_id", notificationID) + } progress = next } @@ -274,7 +280,11 @@ func (c *Controller) reconcileBatch(ctx context.Context, store storage.Storage, next.Phase = entity.IssueTrackingPhaseFinalizing next.CursorTaskID, next.CursorUnitID = "", "" } - return saveProgress(ctx, store.GetIssueTrackingStore(), next) + checkpoint, err := saveProgress(ctx, store.GetIssueTrackingStore(), next) + if err == nil && checkpoint.Phase == entity.IssueTrackingPhaseFinalized { + c.logger.Infow("issue finalization complete", "queue", queue, "build_id", build.ID, "request_id", request.ID, "notification_id", progress.NotificationID, "status", string(build.Status)) + } + return checkpoint, err } if cursorNeedsCheckpoint { return saveProgress(ctx, store.GetIssueTrackingStore(), progress) @@ -327,6 +337,8 @@ func (c *Controller) processIssue(ctx context.Context, store storage.Storage, pr } } + next := progress + next.CursorTaskID, next.CursorUnitID = key.task, key.unit if !missing && current.LastProcessedBuildID != progress.BuildID { previous, err := loader.ByID(ctx, current.LastProcessedBuildID, store.GetBuildStore().Get, "last processed build") if err != nil { @@ -337,12 +349,10 @@ func (c *Controller) processIssue(ctx context.Context, store storage.Storage, pr return entity.IssueTrackingState{}, err } if order < 0 { - return entity.IssueTrackingState{}, fmt.Errorf("issue already processed a newer source: %w", storage.ErrVersionMismatch) + return next, nil } } - next := progress - next.CursorTaskID, next.CursorUnitID = key.task, key.unit if !missing && current.LastProcessedBuildID == progress.BuildID && current.Status == desired { if key == (issueKey{progress.CursorTaskID, progress.CursorUnitID}) { return next, c.recordHistory(ctx, store, current) @@ -411,6 +421,9 @@ func saveProgress(ctx context.Context, store storage.IssueTrackingStore, next en if next.Version == math.MaxInt32 { return entity.IssueTrackingState{}, fmt.Errorf("issue tracking version exhausted") } + if next.Finalize && (next.Phase == entity.IssueTrackingPhaseFinalized || next.Phase == entity.IssueTrackingPhaseAbandoned) { + next.FinalizedRequestID = next.RequestID + } newVersion := next.Version + 1 if err := store.Update(ctx, next, next.Version, newVersion); err != nil { return entity.IssueTrackingState{}, fmt.Errorf("checkpoint issue tracking: %w", err) @@ -419,6 +432,24 @@ func saveProgress(ctx context.Context, store storage.IssueTrackingStore, next en return next, nil } +func finalizedRequestID(progress entity.IssueTrackingState) string { + // Completed records written before the terminal boundary existed still fence their source. + if progress.FinalizedRequestID == "" && progress.Finalize && + (progress.Phase == entity.IssueTrackingPhaseFinalized || progress.Phase == entity.IssueTrackingPhaseAbandoned) { + return progress.RequestID + } + return progress.FinalizedRequestID +} + +func notificationSuperseded(progress entity.IssueTrackingState, requestID string) (bool, error) { + boundary := finalizedRequestID(progress) + if boundary == "" { + return false, nil + } + order, err := entity.CompareRequestID(requestID, boundary) + return order <= 0, err +} + func requireTerminalBuild(build entity.Build, request entity.Request) error { if !build.Status.IsTerminal() { return fmt.Errorf("final issue tracking requires a terminal build") diff --git a/stovepipe/controller/issuetrack/issuetrack_test.go b/stovepipe/controller/issuetrack/issuetrack_test.go index b8f167fa7..ba9451aec 100644 --- a/stovepipe/controller/issuetrack/issuetrack_test.go +++ b/stovepipe/controller/issuetrack/issuetrack_test.go @@ -21,7 +21,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/submitqueue/stovepipe/entity" - "github.com/uber/submitqueue/stovepipe/extension/storage" ) func TestBatchRejectsInvalidClosingPass(t *testing.T) { @@ -197,7 +196,7 @@ func TestOlderSourceCannotChangeTheIssue(t *testing.T) { f.retain(progress) _, err := f.controller.processIssue(context.Background(), f.store, progress, issueKey{"task", "unit"}, true) - require.ErrorIs(t, err, storage.ErrVersionMismatch) + require.NoError(t, err) require.Equal(t, current, f.states[issueKey{"task", "unit"}]) require.Equal(t, progress, f.progress) require.Empty(t, f.logs) diff --git a/stovepipe/entity/issue_tracking_state.go b/stovepipe/entity/issue_tracking_state.go index 05c0f3a92..38d5b909a 100644 --- a/stovepipe/entity/issue_tracking_state.go +++ b/stovepipe/entity/issue_tracking_state.go @@ -40,10 +40,13 @@ const ( IssueTrackingPhaseAbandoned IssueTrackingPhase = "abandoned" ) -// IssueTrackingState is the queue-scoped source-order boundary and resumable reconciliation state. +// IssueTrackingState is the queue-scoped terminal boundary and resumable reconciliation state. type IssueTrackingState struct { - // RequestID is the newest accepted source request, compared by ingest order. + // RequestID identifies the source request of the current or most recently completed pass. RequestID string `json:"request_id"` + // FinalizedRequestID is the newest completed or abandoned terminal source, compared by ingest order. + // Empty means no terminal boundary has been retained. + FinalizedRequestID string `json:"finalized_request_id"` // BuildID identifies the source build whose pass is underway or most recently completed. BuildID string `json:"build_id"` // NotificationID identifies the message that owns this reconciliation pass. @@ -68,6 +71,11 @@ func (s IssueTrackingState) Validate() error { if err := id.Validate(s.RequestID); err != nil { return fmt.Errorf("invalid issue tracking request ID: %w", err) } + if s.FinalizedRequestID != "" { + if err := id.Validate(s.FinalizedRequestID); err != nil { + return fmt.Errorf("invalid finalized issue tracking request ID: %w", err) + } + } if s.BuildID == "" { return fmt.Errorf("issue tracking build ID must not be empty") } diff --git a/stovepipe/entity/issue_tracking_state_test.go b/stovepipe/entity/issue_tracking_state_test.go index c7f2d581b..be3d55884 100644 --- a/stovepipe/entity/issue_tracking_state_test.go +++ b/stovepipe/entity/issue_tracking_state_test.go @@ -31,6 +31,7 @@ func TestIssueTrackingState(t *testing.T) { name string change func(*IssueTrackingState) }{ + {"invalid finalized request", func(s *IssueTrackingState) { s.FinalizedRequestID = "invalid" }}, {"missing request", func(s *IssueTrackingState) { s.RequestID = "" }}, {"noncanonical request", func(s *IssueTrackingState) { s.RequestID = "010" }}, {"nonnumeric request", func(s *IssueTrackingState) { s.RequestID = "request/10" }}, diff --git a/stovepipe/extension/storage/README.md b/stovepipe/extension/storage/README.md index 47645b848..cf81c0ae6 100644 --- a/stovepipe/extension/storage/README.md +++ b/stovepipe/extension/storage/README.md @@ -80,13 +80,13 @@ History persistence does not alter issue state or validate transitions against i ## Issue reconciliation progress -`IssueTrackingStore` retains one queue-scoped operational `IssueTrackingState`, independently of open-issue membership and user-facing summaries. It records the source request and build, the refresh or finalization phase, the owning notification, one task/unit cursor, and its optimistic version. The zero cursor starts the current phase's range. Every active pass revisits the cursor issue before scanning later keys, including when the previous delivery already finished that issue. `Finalize` retains the terminal notification intent during refreshing, while `HasTaskWideIssues` records whether a task-wide issue has been encountered in that pass. Once all build issues have been applied, a non-canceled terminal pass resets the cursor and finalizes against issue states. A task-wide failure suppresses closures only for missing targets of the same task. These fields and the cursor are saved together under the same version guard. Notification identity lets the controller distinguish its own continuation from helping another pass. The record remains after all issues close to preserve the source-order boundary. The MySQL table is keyed only by queue; creates, reads, and updates use complete primary keys with no secondary indexes or cross-table transaction. +`IssueTrackingStore` retains one queue-scoped operational `IssueTrackingState`, independently of open-issue membership and user-facing summaries. It records the source request and build, the refresh or finalization phase, the owning notification, one task/unit cursor, its optimistic version, and `FinalizedRequestID`, the latest completed or abandoned terminal source. The terminal boundary is independent of the current pass and is retained across later refreshes; empty and nonempty running observations cannot supersede an older finalization. The zero cursor starts the current phase's range. Every active pass revisits the cursor issue before scanning later keys, including when the previous delivery already finished that issue. `Finalize` retains the terminal notification intent during refreshing, while `HasTaskWideIssues` records whether a task-wide issue has been encountered in that pass. Once all build issues have been applied, a non-canceled terminal pass resets the cursor and finalizes against issue states. A task-wide failure suppresses closures only for missing targets of the same task. These fields and the cursor are saved together under the same version guard. Notification identity lets the controller distinguish its own continuation from helping another pass. The record remains after all issues close to preserve the terminal boundary. The MySQL table is keyed only by queue; creates, reads, and updates use complete primary keys with no secondary indexes or cross-table transaction. The store does not decide source ordering, apply issue transitions, or advance versions. Its caller supplies the guard version and replacement version. A later controller will own acceptance, resumption of cursor work, and completion; no runtime writer is enabled here. Recovery uses the same record's `abandoning` and `abandoned` phases; no additional schema fields or storage operations are required. The source, notification, cursor, and terminal intent remain present after abandonment. See [issue tracking recovery](../../controller/issuetrack/README.md#recovery-and-activation) for the conditional-write ordering that protects the current cursor before newer work is admitted. -The issue-tracking table must include the notification, cursor, `finalize`, and `has_task_wide_issues` columns before updated readers or writers are deployed. Both boolean columns default to false. They describe one pass, not analysis state for every build in the queue; terminal intent is required before absence-based reconciliation is permitted. No runtime writer is enabled by the storage layer. Notification IDs allow up to 1020 bytes, matching the queue message ID byte capacity; cursor limits match the issue stores. +The issue-tracking table must include the notification, cursor, `finalize`, `has_task_wide_issues`, and `finalized_request_id` columns before updated readers or writers are deployed. Both boolean columns default to false; the terminal request ID defaults to an empty binary string. They describe one pass, not analysis state for every build in the queue; terminal intent is required before absence-based reconciliation is permitted. No runtime writer is enabled by the storage layer. Notification IDs allow up to 1020 bytes, matching the queue message ID byte capacity; cursor limits match the issue stores. ### Applied transition facts diff --git a/stovepipe/extension/storage/mysql/issue_tracking_store.go b/stovepipe/extension/storage/mysql/issue_tracking_store.go index 9e81deeb9..6f4bc873f 100644 --- a/stovepipe/extension/storage/mysql/issue_tracking_store.go +++ b/stovepipe/extension/storage/mysql/issue_tracking_store.go @@ -41,12 +41,12 @@ func (s *issueTrackingStore) Get(ctx context.Context) (state entity.IssueTrackin op := metrics.Begin(s.scope, "get", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() err := s.db.QueryRowContext(ctx, - `SELECT request_id, build_id, notification_id, + `SELECT request_id, finalized_request_id, build_id, notification_id, cursor_task_id, cursor_unit_id, finalize, has_task_wide_issues, phase, version FROM issue_tracking WHERE queue = ?`, s.queue). Scan( - &state.RequestID, &state.BuildID, &state.NotificationID, + &state.RequestID, &state.FinalizedRequestID, &state.BuildID, &state.NotificationID, &state.CursorTaskID, &state.CursorUnitID, &state.Finalize, &state.HasTaskWideIssues, &state.Phase, &state.Version, @@ -67,10 +67,10 @@ func (s *issueTrackingStore) Create(ctx context.Context, state entity.IssueTrack return err } _, err := s.db.ExecContext(ctx, - `INSERT INTO issue_tracking (queue, request_id, build_id, notification_id, + `INSERT INTO issue_tracking (queue, request_id, finalized_request_id, build_id, notification_id, cursor_task_id, cursor_unit_id, - finalize, has_task_wide_issues, phase, version) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, - s.queue, state.RequestID, state.BuildID, state.NotificationID, + finalize, has_task_wide_issues, phase, version) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + s.queue, state.RequestID, state.FinalizedRequestID, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, state.Version, ) @@ -95,11 +95,11 @@ func (s *issueTrackingStore) Update(ctx context.Context, state entity.IssueTrack return fmt.Errorf("issue tracking guard version must be positive") } result, err := s.db.ExecContext(ctx, - `UPDATE issue_tracking SET request_id = ?, build_id = ?, notification_id = ?, + `UPDATE issue_tracking SET request_id = ?, finalized_request_id = ?, build_id = ?, notification_id = ?, cursor_task_id = ?, cursor_unit_id = ?, finalize = ?, has_task_wide_issues = ?, phase = ?, version = ? WHERE queue = ? AND version = ?`, - state.RequestID, state.BuildID, state.NotificationID, + state.RequestID, state.FinalizedRequestID, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, newVersion, s.queue, oldVersion, diff --git a/stovepipe/extension/storage/mysql/issue_tracking_store_test.go b/stovepipe/extension/storage/mysql/issue_tracking_store_test.go index 935ea6682..7a8427467 100644 --- a/stovepipe/extension/storage/mysql/issue_tracking_store_test.go +++ b/stovepipe/extension/storage/mysql/issue_tracking_store_test.go @@ -31,7 +31,7 @@ import ( ) func trackingFixture() entity.IssueTrackingState { - return entity.IssueTrackingState{RequestID: "10", BuildID: "build/10", NotificationID: "notification/10", CursorTaskID: "task", CursorUnitID: "unit", Phase: entity.IssueTrackingPhaseFinalizing, Finalize: true, Version: 1} + return entity.IssueTrackingState{RequestID: "10", FinalizedRequestID: "9", BuildID: "build/10", NotificationID: "notification/10", CursorTaskID: "task", CursorUnitID: "unit", Phase: entity.IssueTrackingPhaseFinalizing, Finalize: true, Version: 1} } func TestIssueTrackingStoreGet(t *testing.T) { @@ -43,7 +43,7 @@ func TestIssueTrackingStoreGet(t *testing.T) { state := trackingFixture() store := NewIssueTrackingStore(db, testMetrics(), "queue") - query := mock.ExpectQuery(regexp.QuoteMeta(`SELECT request_id, build_id, notification_id, cursor_task_id, cursor_unit_id, finalize, has_task_wide_issues, phase, version FROM issue_tracking WHERE queue = ?`)).WithArgs("queue") + query := mock.ExpectQuery(regexp.QuoteMeta(`SELECT request_id, finalized_request_id, build_id, notification_id, cursor_task_id, cursor_unit_id, finalize, has_task_wide_issues, phase, version FROM issue_tracking WHERE queue = ?`)).WithArgs("queue") switch kind { case "missing": query.WillReturnError(sql.ErrNoRows) @@ -54,7 +54,7 @@ func TestIssueTrackingStoreGet(t *testing.T) { if kind == "scan error" { request = nil } - query.WillReturnRows(sqlmock.NewRows([]string{"request_id", "build_id", "notification_id", "cursor_task_id", "cursor_unit_id", "finalize", "has_task_wide_issues", "phase", "version"}).AddRow(request, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, state.Version)) + query.WillReturnRows(sqlmock.NewRows([]string{"request_id", "finalized_request_id", "build_id", "notification_id", "cursor_task_id", "cursor_unit_id", "finalize", "has_task_wide_issues", "phase", "version"}).AddRow(request, state.FinalizedRequestID, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, state.Version)) } got, err := store.Get(context.Background()) @@ -94,8 +94,8 @@ func TestIssueTrackingStoreCreate(t *testing.T) { state := trackingFixture() store := NewIssueTrackingStore(db, testMetrics(), "queue") - query := mock.ExpectExec(regexp.QuoteMeta(`INSERT INTO issue_tracking (queue, request_id, build_id, notification_id, cursor_task_id, cursor_unit_id, finalize, has_task_wide_issues, phase, version) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`)). - WithArgs("queue", state.RequestID, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, state.Version) + query := mock.ExpectExec(regexp.QuoteMeta(`INSERT INTO issue_tracking (queue, request_id, finalized_request_id, build_id, notification_id, cursor_task_id, cursor_unit_id, finalize, has_task_wide_issues, phase, version) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`)). + WithArgs("queue", state.RequestID, state.FinalizedRequestID, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, state.Version) if tt.dbErr != nil { query.WillReturnError(tt.dbErr) } else { @@ -133,8 +133,8 @@ func TestIssueTrackingStoreUpdate(t *testing.T) { state := trackingFixture() store := NewIssueTrackingStore(db, testMetrics(), "queue") - query := mock.ExpectExec(regexp.QuoteMeta(`UPDATE issue_tracking SET request_id = ?, build_id = ?, notification_id = ?, cursor_task_id = ?, cursor_unit_id = ?, finalize = ?, has_task_wide_issues = ?, phase = ?, version = ? WHERE queue = ? AND version = ?`)). - WithArgs(state.RequestID, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, int32(7), "queue", int32(1)) + query := mock.ExpectExec(regexp.QuoteMeta(`UPDATE issue_tracking SET request_id = ?, finalized_request_id = ?, build_id = ?, notification_id = ?, cursor_task_id = ?, cursor_unit_id = ?, finalize = ?, has_task_wide_issues = ?, phase = ?, version = ? WHERE queue = ? AND version = ?`)). + WithArgs(state.RequestID, state.FinalizedRequestID, state.BuildID, state.NotificationID, state.CursorTaskID, state.CursorUnitID, state.Finalize, state.HasTaskWideIssues, state.Phase, int32(7), "queue", int32(1)) switch { case tt.dbErr != nil: query.WillReturnError(tt.dbErr) diff --git a/stovepipe/extension/storage/mysql/schema/issue_tracking.sql b/stovepipe/extension/storage/mysql/schema/issue_tracking.sql index 9d367d84c..c3c37ecc3 100644 --- a/stovepipe/extension/storage/mysql/schema/issue_tracking.sql +++ b/stovepipe/extension/storage/mysql/schema/issue_tracking.sql @@ -2,6 +2,7 @@ CREATE TABLE IF NOT EXISTS issue_tracking ( queue VARBINARY(255) NOT NULL, request_id VARBINARY(255) NOT NULL, + finalized_request_id VARBINARY(255) NOT NULL DEFAULT '', build_id VARBINARY(1020) NOT NULL, notification_id VARBINARY(1020) NOT NULL, diff --git a/test/integration/stovepipe/extension/storage/mysql/issue_tracking_test.go b/test/integration/stovepipe/extension/storage/mysql/issue_tracking_test.go index e30f5aaa5..be29616a8 100644 --- a/test/integration/stovepipe/extension/storage/mysql/issue_tracking_test.go +++ b/test/integration/stovepipe/extension/storage/mysql/issue_tracking_test.go @@ -32,7 +32,7 @@ func testIssueTrackingStore(t *testing.T, ctx context.Context, factory storage.F store := bound.GetIssueTrackingStore() _, err = store.Get(ctx) require.ErrorIs(t, err, storage.ErrNotFound) - state := entity.IssueTrackingState{RequestID: "10", BuildID: "build/10", NotificationID: "notification/10", Phase: entity.IssueTrackingPhaseRefreshing, Version: 1} + state := entity.IssueTrackingState{RequestID: "10", FinalizedRequestID: "9", BuildID: "build/10", NotificationID: "notification/10", Phase: entity.IssueTrackingPhaseRefreshing, Version: 1} require.NoError(t, store.Create(ctx, state)) require.ErrorIs(t, store.Create(ctx, state), storage.ErrAlreadyExists) got, err := store.Get(ctx) @@ -53,6 +53,7 @@ func testIssueTrackingStore(t *testing.T, ctx context.Context, factory storage.F require.NoError(t, store.Update(ctx, state, 2, 3)) state.Version = 3 state.Phase = entity.IssueTrackingPhaseFinalized + state.FinalizedRequestID = "11" require.NoError(t, store.Update(ctx, state, 3, 4)) state.Version = 4 got, err = store.Get(ctx) @@ -66,13 +67,13 @@ func testIssueTrackingStore(t *testing.T, ctx context.Context, factory storage.F bound, err := factory.For(storage.Config{QueueName: "tracking/terminal"}) require.NoError(t, err) store := bound.GetIssueTrackingStore() - opening := entity.IssueTrackingState{RequestID: "10", BuildID: "build/10", NotificationID: "terminal", Phase: entity.IssueTrackingPhaseRefreshing, Finalize: true, HasTaskWideIssues: true, CursorTaskID: "task", CursorUnitID: "unit", Version: 1} + opening := entity.IssueTrackingState{RequestID: "10", FinalizedRequestID: "9", BuildID: "build/10", NotificationID: "terminal", Phase: entity.IssueTrackingPhaseRefreshing, Finalize: true, HasTaskWideIssues: true, CursorTaskID: "task", CursorUnitID: "unit", Version: 1} require.NoError(t, store.Create(ctx, opening)) got, err := store.Get(ctx) require.NoError(t, err) require.Equal(t, opening, got) finished := opening - finished.Phase = entity.IssueTrackingPhaseFinalized + finished.Phase = entity.IssueTrackingPhaseFinalizing require.NoError(t, store.Update(ctx, finished, 1, 2)) finished.Version = 2 got, err = store.Get(ctx) @@ -99,7 +100,7 @@ func testIssueTrackingStore(t *testing.T, ctx context.Context, factory storage.F state := entity.IssueTrackingState{ RequestID: "10", BuildID: "build/10", NotificationID: "failed-notification", CursorTaskID: "task", CursorUnitID: "unit", - Finalize: true, HasTaskWideIssues: true, + Finalize: true, HasTaskWideIssues: true, FinalizedRequestID: "9", Phase: entity.IssueTrackingPhaseAbandoning, Version: 1, } require.NoError(t, store.Create(ctx, state))