diff --git a/stovepipe/controller/issuetrack/README.md b/stovepipe/controller/issuetrack/README.md index 866349edc..701af184f 100644 --- a/stovepipe/controller/issuetrack/README.md +++ b/stovepipe/controller/issuetrack/README.md @@ -8,7 +8,7 @@ Issue tracking maintains current issue state and opening/closing history from pe 2. Reject 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. On retry, the inclusive cursor revisits that row and repairs both before proceeding. +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. Closed issue rows are retained, so their versions cannot reset on reopening. A repeated result from the same build needs no new issue write. Its history is repaired when revisiting the current cursor. @@ -16,7 +16,7 @@ Closed issue rows are retained, so their versions cannot reset on reopening. A r 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. -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. Summary failures keep the cursor unfinished, just like history failures. +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. The post-read cursor write and processed-build updates, including unchanged statuses, are required for this ordering. A progress version alone is not a lock around issue writes. These operations use individual K-V reads, conditional writes, and immutable history creates; no cross-record transaction or stored future-change payload is needed. @@ -50,7 +50,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 and summary from the retained row. Record an `issue_tracking_abandoned` Request-history event keyed by the failed notification, with its build ID. +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. 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. diff --git a/stovepipe/controller/issuetrack/dlq_test.go b/stovepipe/controller/issuetrack/dlq_test.go index 78705e5a4..af723ae5f 100644 --- a/stovepipe/controller/issuetrack/dlq_test.go +++ b/stovepipe/controller/issuetrack/dlq_test.go @@ -193,7 +193,7 @@ func TestRecoveryDoesNotReopenAnExistingClosedCursor(t *testing.T) { } func TestRecoveryRetriesOnlyRecoveryWork(t *testing.T) { - for _, step := range []string{"build/get", "request/get", "progress/get", "progress/update", "issue/get", "issue/update", "history/create", "history/get", "summary/get", "recovery/history", "finish abandonment"} { + for _, step := range []string{"build/get", "request/get", "progress/get", "progress/update", "issue/get", "issue/update", "history/create", "history/get", "summary/get", "open-issue/get", "recovery/history", "finish abandonment"} { t.Run(step, func(t *testing.T) { f := newRecoveryFixture(t) f.source("build/100", "100", entity.BuildStatusFailed) @@ -221,7 +221,7 @@ func TestRecoveryRetriesOnlyRecoveryWork(t *testing.T) { } func TestRecoveryRetainsAnOpeningAcrossRetries(t *testing.T) { - for _, step := range []string{"issue/create", "history/create", "summary/create", "recovery/history"} { + for _, step := range []string{"issue/create", "history/create", "summary/create", "open-issue/create", "recovery/history"} { t.Run(step, func(t *testing.T) { f := newRecoveryFixture(t) f.source("build/100", "100", entity.BuildStatusRunning) @@ -237,6 +237,7 @@ func TestRecoveryRetainsAnOpeningAcrossRetries(t *testing.T) { issue := f.states[issueKey{"task", "unit"}] log := f.logs[logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusOpen}] summary := f.summaries[issueKey{"task", "unit"}] + require.Equal(t, issue.FirstSeenBuildID, f.openIssues[issueKey{"task", "unit"}].FirstSeenBuildID) require.Equal(t, entity.IssueStatusOpen, summary.Status) require.Equal(t, issue.FirstSeenBuildID, summary.FirstSeenBuildID) require.Equal(t, issue.ChangedAtMs, summary.ChangedAtMs) @@ -251,6 +252,29 @@ func TestRecoveryRetainsAnOpeningAcrossRetries(t *testing.T) { } } +func TestRecoveryRepairsClosedIssueLookupBeforeAbandonment(t *testing.T) { + f := newRecoveryFixture(t) + f.source("build/100", "100", entity.BuildStatusFailed) + f.seedIssue("task", "unit", entity.IssueStatusClosed) + key := issueKey{"task", "unit"} + f.openIssues[key] = entity.OpenIssue{TaskID: key.task, UnitID: key.unit, FirstSeenBuildID: "older"} + progress := currentCursor() + msg := f.message("build/100", true) + progress.Finalize, progress.NotificationID = true, msg.ID + f.retain(progress) + f.failures["open-issue/delete"] = 1 + + require.ErrorIs(t, f.recover(msg, false), assert.AnError) + require.Equal(t, entity.IssueTrackingPhaseAbandoning, f.progress.Phase) + require.Contains(t, f.openIssues, key) + + require.NoError(t, f.recover(msg, false)) + require.Equal(t, entity.IssueTrackingPhaseAbandoned, f.progress.Phase) + require.NotContains(t, f.openIssues, key) + require.Equal(t, entity.IssueStatusClosed, f.states[key].Status) + require.Equal(t, entity.IssueStatusClosed, f.summaries[key].Status) +} + func TestPrimaryWaitsForRecoveryThenAdmitsANewerBuild(t *testing.T) { f := newRecoveryFixture(t) f.source("build/100", "100", entity.BuildStatusFailed) diff --git a/stovepipe/controller/issuetrack/fixture_test.go b/stovepipe/controller/issuetrack/fixture_test.go index 4ab7d03db..232abd91b 100644 --- a/stovepipe/controller/issuetrack/fixture_test.go +++ b/stovepipe/controller/issuetrack/fixture_test.go @@ -50,6 +50,7 @@ type fixture struct { store storage.Storage states map[issueKey]entity.IssueState summaries map[issueKey]entity.IssueSummary + openIssues map[issueKey]entity.OpenIssue logs map[logKey]entity.IssueLog progress entity.IssueTrackingState hasProgress bool @@ -72,17 +73,20 @@ func newFixture(t *testing.T) *fixture { f.requests = map[string]entity.Request{} f.buildIssues = map[string]map[issueKey]entity.BuildIssue{} f.summaries = map[issueKey]entity.IssueSummary{} + f.openIssues = map[issueKey]entity.OpenIssue{} f.source("older", "1", entity.BuildStatusSucceeded) store := storagemock.NewMockStorage(f.mocks) f.store = store states := storagemock.NewMockIssueStateStore(f.mocks) history := storagemock.NewMockIssueLogStore(f.mocks) summaries := storagemock.NewMockIssueSummaryStore(f.mocks) + openIssues := storagemock.NewMockOpenIssueStore(f.mocks) tracking := storagemock.NewMockIssueTrackingStore(f.mocks) f.factory.EXPECT().For(storage.Config{QueueName: testQueue}).Return(store, nil).AnyTimes() store.EXPECT().GetIssueStateStore().Return(states).AnyTimes() store.EXPECT().GetIssueLogStore().Return(history).AnyTimes() store.EXPECT().GetIssueSummaryStore().Return(summaries).AnyTimes() + store.EXPECT().GetOpenIssueStore().Return(openIssues).AnyTimes() store.EXPECT().GetIssueTrackingStore().Return(tracking).AnyTimes() states.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, task, unit string) (entity.IssueState, error) { @@ -180,6 +184,49 @@ func newFixture(t *testing.T) *fixture { f.summaries[key] = summary return nil }).AnyTimes() + openIssues.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, task, unit string) (entity.OpenIssue, error) { + if err := f.fail("open-issue/get"); err != nil { + return entity.OpenIssue{}, err + } + issue, ok := f.openIssues[issueKey{task, unit}] + if !ok { + return entity.OpenIssue{}, storage.ErrNotFound + } + return issue, nil + }).AnyTimes() + openIssues.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, issue entity.OpenIssue) error { + if err := f.fail("open-issue/create"); err != nil { + return err + } + key := issueKey{issue.TaskID, issue.UnitID} + if _, ok := f.openIssues[key]; ok { + return storage.ErrAlreadyExists + } + f.openIssues[key] = issue + return nil + }).AnyTimes() + openIssues.EXPECT().Replace(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, issue entity.OpenIssue, expected string) (bool, error) { + if err := f.fail("open-issue/replace"); err != nil { + return false, err + } + key := issueKey{issue.TaskID, issue.UnitID} + if current, ok := f.openIssues[key]; !ok || current.FirstSeenBuildID != expected { + return false, nil + } + f.openIssues[key] = issue + return true, nil + }).AnyTimes() + openIssues.EXPECT().Delete(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, issue entity.OpenIssue) (bool, error) { + if err := f.fail("open-issue/delete"); err != nil { + return false, err + } + key := issueKey{issue.TaskID, issue.UnitID} + if current, ok := f.openIssues[key]; !ok || current.FirstSeenBuildID != issue.FirstSeenBuildID { + return false, nil + } + delete(f.openIssues, key) + return true, nil + }).AnyTimes() tracking.EXPECT().Get(gomock.Any()).DoAndReturn(func(context.Context) (entity.IssueTrackingState, error) { if err := f.fail("progress/get"); err != nil { return entity.IssueTrackingState{}, err @@ -358,4 +405,7 @@ func (f *fixture) seedIssue(task, unit string, status entity.IssueStatus) { f.initialLogs[logKey{issueKey{task, unit}, "older", status}] = entity.IssueLog{TaskID: task, UnitID: unit, BuildID: "older", Status: status, TimestampMs: 500} f.summaries[issueKey{task, unit}] = entity.IssueSummary{TaskID: task, UnitID: unit, Status: status, FirstSeenBuildID: "older", RequestID: "1", BuildID: "older", ChangedAtMs: 500, Version: 1} + if status == entity.IssueStatusOpen { + f.openIssues[issueKey{task, unit}] = entity.OpenIssue{TaskID: task, UnitID: unit, FirstSeenBuildID: "older"} + } } diff --git a/stovepipe/controller/issuetrack/issuetrack_test.go b/stovepipe/controller/issuetrack/issuetrack_test.go index 7c2af9ed6..b28e8ec32 100644 --- a/stovepipe/controller/issuetrack/issuetrack_test.go +++ b/stovepipe/controller/issuetrack/issuetrack_test.go @@ -95,11 +95,13 @@ func TestProcessIssueOpensAndCloses(t *testing.T) { require.Equal(t, "build/100", issue.LastChangedBuildID) require.Equal(t, int64(1000), issue.ChangedAtMs) if reported { + require.Equal(t, issue.FirstSeenBuildID, f.openIssues[issueKey{"task", "unit"}].FirstSeenBuildID) require.Equal(t, entity.IssueStatusOpen, issue.Status) require.Equal(t, "build/100", issue.FirstSeenBuildID) require.Equal(t, int32(1), issue.Version) require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusOpen}) } else { + require.NotContains(t, f.openIssues, issueKey{"task", "unit"}) require.Equal(t, entity.IssueStatusClosed, issue.Status) require.Equal(t, "older", issue.FirstSeenBuildID) require.Equal(t, int32(8), issue.Version) @@ -202,3 +204,27 @@ func TestOlderSourceCannotChangeTheIssue(t *testing.T) { require.Equal(t, progress, f.progress) require.Empty(t, f.logs) } + +func TestOpenIssueFailureKeepsTheCursorForRetry(t *testing.T) { + f := newFixture(t) + f.source("build/100", "100", entity.BuildStatusRunning) + f.reportIssue("build/100", "task", "unit") + progress := currentCursor() + f.retain(progress) + f.failures["open-issue/create"] = 1 + + _, err := f.controller.processIssue(context.Background(), f.store, progress, issueKey{"task", "unit"}, true) + require.ErrorIs(t, err, assert.AnError) + accepted := f.states[issueKey{"task", "unit"}] + require.Contains(t, f.summaries, issueKey{"task", "unit"}) + require.Empty(t, f.openIssues) + require.Equal(t, entity.IssueTrackingPhaseRefreshing, f.progress.Phase) + require.Equal(t, "task", f.progress.CursorTaskID) + require.Equal(t, "unit", f.progress.CursorUnitID) + + _, err = f.controller.processIssue(context.Background(), f.store, f.progress, issueKey{"task", "unit"}, false) + require.NoError(t, err) + require.Equal(t, accepted, f.states[issueKey{"task", "unit"}]) + require.Equal(t, accepted.FirstSeenBuildID, f.openIssues[issueKey{"task", "unit"}].FirstSeenBuildID) + require.Len(t, f.logs, 1) +} diff --git a/stovepipe/core/issuelog/README.md b/stovepipe/core/issuelog/README.md index d2f609ca0..c1df208f6 100644 --- a/stovepipe/core/issuelog/README.md +++ b/stovepipe/core/issuelog/README.md @@ -1,9 +1,17 @@ # Issue history and summaries -The materializer retains an issue transition before updating its summary. Issue tracking calls it inline during normal processing and recovery; there is no separate consumer. A failed summary write returns an error, so the current issue remains at the cursor for retry. Replaying an existing transition reads its original timestamp and still repairs the summary. +The materializer retains an issue transition before updating its summary and open-issue lookup. Issue tracking calls it inline during normal processing and recovery; there is no separate consumer. A failed projection write returns an error, so the current issue remains at the cursor for retry. Replaying an existing transition reads its original timestamp and repairs both projections. The source issue state supplies the opening build ID because closing log entries identify the closing build, not the opening. That value comes from the same retained issue snapshot as the transition; the materializer does not look up a potentially newer issue state to reconstruct an older opening. Summary ordering uses the source build's request ID in ingest order. Within one request, closing supersedes opening. Build IDs and timestamps do not establish order. Older transitions remain in history but cannot replace newer summaries. Conditional-create and CAS conflicts reload the winning summary and repeat the ordering check. Closed summaries remain present across reopening, with increasing projection versions. -This materializer does not backfill existing history, maintain an open-only mapping, or expose a list API. Source builds must remain readable while their transitions can be replayed. +## Open-issue lookup + +After summary materialization, the lookup is repaired from the latest summary, including when an incoming transition was a duplicate or older than the summary. An open summary requires a reference to its opening; a closed summary requires no open-issue row. + +Each iteration reads the open-issue row before the summary. Otherwise, a worker could read an old closed summary, then read a newer opening and use that opening's ID to delete it. Reading the open-issue row first means a concurrent reopening changes the expected ID and prevents the stale delete or replacement. Creates require absence; replacement and deletion compare the opening that was read. Both successful writes and guard mismatches repeat the reads. A delayed create can briefly leave a stale reference after a close; the next iteration removes it. Storage implementations must satisfy the causal read-after-write contract. + +The two projections are not updated atomically. A future list reader must still verify that each referenced summary is open and its opening matches. No status filtering, cross-record transaction, or queue-wide scan is required to maintain one open-issue row. + +This materializer does not backfill existing history or expose a list API. Source builds must remain readable while their transitions can be replayed. diff --git a/stovepipe/core/issuelog/materializer.go b/stovepipe/core/issuelog/materializer.go index a84cce234..72e167b74 100644 --- a/stovepipe/core/issuelog/materializer.go +++ b/stovepipe/core/issuelog/materializer.go @@ -28,9 +28,9 @@ import ( "github.com/uber/submitqueue/stovepipe/extension/storage" ) -// Materializer retains immutable issue history and updates its summary. +// Materializer retains issue history and updates its summary and open-issue lookup. type Materializer interface { - // PersistLog repairs the summary even when the transition already exists. + // PersistLog repairs the summary and lookup even when the transition already exists. // firstSeenBuildID identifies the opening retained by the source issue state. PersistLog(ctx context.Context, stores storage.Storage, log entity.IssueLog, firstSeenBuildID string) error } @@ -76,7 +76,10 @@ func (m *materializer) PersistLog(ctx context.Context, stores storage.Storage, l if err := summary.Validate(); err != nil { return fmt.Errorf("invalid issue summary: %w", err) } - return materializeIssueSummary(ctx, stores.GetIssueSummaryStore(), summary) + if err := materializeIssueSummary(ctx, stores.GetIssueSummaryStore(), summary); err != nil { + return err + } + return materializeOpenIssue(ctx, stores, summary.TaskID, summary.UnitID) } func retainIssueLog(ctx context.Context, logs storage.IssueLogStore, log entity.IssueLog) (entity.IssueLog, error) { @@ -153,4 +156,52 @@ func materializeIssueSummary(ctx context.Context, summaries storage.IssueSummary } } +func materializeOpenIssue(ctx context.Context, stores storage.Storage, taskID, unitID string) error { + openIssues := stores.GetOpenIssueStore() + summaries := stores.GetIssueSummaryStore() + for { + if err := ctx.Err(); err != nil { + return err + } + + // Read the open-issue row before the summary so a stale closed + // summary cannot authorize deleting a newer opening. + current, err := openIssues.Get(ctx, taskID, unitID) + missing := errors.Is(err, storage.ErrNotFound) + if err != nil && !missing { + return fmt.Errorf("read open issue: %w", err) + } + summary, err := summaries.Get(ctx, taskID, unitID) + if err != nil { + return fmt.Errorf("read issue summary for open issue: %w", err) + } + + switch summary.Status { + case entity.IssueStatusOpen: + if !missing && current.FirstSeenBuildID == summary.FirstSeenBuildID { + return nil + } + desired := entity.OpenIssue{TaskID: taskID, UnitID: unitID, FirstSeenBuildID: summary.FirstSeenBuildID} + if missing { + err = openIssues.Create(ctx, desired) + if errors.Is(err, storage.ErrAlreadyExists) { + continue + } + } else { + _, err = openIssues.Replace(ctx, desired, current.FirstSeenBuildID) + } + case entity.IssueStatusClosed: + if missing { + return nil + } + _, err = openIssues.Delete(ctx, current) + default: + return fmt.Errorf("invalid issue summary status %q", summary.Status) + } + if err != nil { + return fmt.Errorf("materialize open issue: %w", err) + } + } +} + var _ Materializer = (*materializer)(nil) diff --git a/stovepipe/core/issuelog/materializer_test.go b/stovepipe/core/issuelog/materializer_test.go index 48e425b44..36af850a5 100644 --- a/stovepipe/core/issuelog/materializer_test.go +++ b/stovepipe/core/issuelog/materializer_test.go @@ -38,8 +38,12 @@ type materializerFixture struct { stores *storagemock.MockStorage logs map[logKey]entity.IssueLog summary entity.IssueSummary + openIssue entity.OpenIssue failures map[string]error beforeCreate, beforeUpdate func() + beforeOpenIssueGet func() + beforeOpenIssueWrite func() + openIssueDeletes int } func newMaterializerFixture(t *testing.T) *materializerFixture { @@ -49,9 +53,11 @@ func newMaterializerFixture(t *testing.T) *materializerFixture { logs: make(map[logKey]entity.IssueLog), failures: make(map[string]error)} logs := storagemock.NewMockIssueLogStore(ctrl) summaries := storagemock.NewMockIssueSummaryStore(ctrl) + openIssues := storagemock.NewMockOpenIssueStore(ctrl) builds := storagemock.NewMockBuildStore(ctrl) f.stores.EXPECT().GetIssueLogStore().Return(logs).AnyTimes() f.stores.EXPECT().GetIssueSummaryStore().Return(summaries).AnyTimes() + f.stores.EXPECT().GetOpenIssueStore().Return(openIssues).AnyTimes() f.stores.EXPECT().GetBuildStore().Return(builds).AnyTimes() logs.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, log entity.IssueLog) error { if err := f.failures["log/create"]; err != nil { @@ -126,6 +132,61 @@ func newMaterializerFixture(t *testing.T) *materializerFixture { f.summary = summary return nil }).AnyTimes() + openIssues.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(context.Context, string, string) (entity.OpenIssue, error) { + if f.beforeOpenIssueGet != nil { + hook := f.beforeOpenIssueGet + f.beforeOpenIssueGet = nil + hook() + } + if err := f.failures["open_issue/get"]; err != nil { + return entity.OpenIssue{}, err + } + if f.openIssue.TaskID == "" { + return entity.OpenIssue{}, storage.ErrNotFound + } + return f.openIssue, nil + }).AnyTimes() + runOpenIssueWriteHook := func() { + if f.beforeOpenIssueWrite != nil { + hook := f.beforeOpenIssueWrite + f.beforeOpenIssueWrite = nil + hook() + } + } + openIssues.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, issue entity.OpenIssue) error { + runOpenIssueWriteHook() + if err := f.failures["open_issue/create"]; err != nil { + return err + } + if f.openIssue.TaskID != "" { + return storage.ErrAlreadyExists + } + f.openIssue = issue + return nil + }).AnyTimes() + openIssues.EXPECT().Replace(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, issue entity.OpenIssue, expected string) (bool, error) { + runOpenIssueWriteHook() + if err := f.failures["open_issue/replace"]; err != nil { + return false, err + } + if f.openIssue.TaskID == "" || f.openIssue.FirstSeenBuildID != expected { + return false, nil + } + f.openIssue = issue + return true, nil + }).AnyTimes() + openIssues.EXPECT().Delete(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, issue entity.OpenIssue) (bool, error) { + f.openIssueDeletes++ + runOpenIssueWriteHook() + if err := f.failures["open_issue/delete"]; err != nil { + return false, err + } + if f.openIssue != issue { + return false, nil + } + f.openIssue = entity.OpenIssue{} + return true, nil + }).AnyTimes() return f } @@ -154,6 +215,11 @@ func TestMaterializerOpenCloseReopenAndReplay(t *testing.T) { require.NoError(t, f.m.PersistLog(context.Background(), f.stores, step.log, step.opening)) require.Equal(t, step.status, f.summary.Status) require.Equal(t, step.version, f.summary.Version) + if step.status == entity.IssueStatusClosed { + require.Empty(t, f.openIssue) + } else { + require.Equal(t, f.summary.FirstSeenBuildID, f.openIssue.FirstSeenBuildID) + } } require.Equal(t, "11", f.summary.RequestID) require.Equal(t, reopened.BuildID, f.summary.FirstSeenBuildID) @@ -306,3 +372,148 @@ func TestMaterializerHonorsCanceledContext(t *testing.T) { require.Empty(t, f.logs) require.Empty(t, f.summary) } + +func TestMaterializerRepairsOpenIssueOnRetry(t *testing.T) { + for _, step := range []string{"open_issue/get", "open_issue/create", "open_issue/replace", "open_issue/delete"} { + t.Run(step, func(t *testing.T) { + f := newMaterializerFixture(t) + log := testIssueLog("build/z", entity.IssueStatusOpen) + opening := log.BuildID + switch step { + case "open_issue/replace": + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, log, opening)) + log = testIssueLog("build/a", entity.IssueStatusOpen) + opening = log.BuildID + case "open_issue/delete": + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, log, opening)) + log = testIssueLog("build/a", entity.IssueStatusClosed) + } + f.failures[step] = assert.AnError + require.ErrorIs(t, f.m.PersistLog(context.Background(), f.stores, log, opening), assert.AnError) + persisted := f.summary + require.Equal(t, log.BuildID, persisted.BuildID) + delete(f.failures, step) + + retry := log + retry.TimestampMs = 9999 + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, retry, opening)) + require.Equal(t, persisted, f.summary) + if persisted.Status == entity.IssueStatusClosed { + require.Empty(t, f.openIssue) + } else { + require.Equal(t, entity.OpenIssue{TaskID: log.TaskID, UnitID: log.UnitID, FirstSeenBuildID: opening}, f.openIssue) + } + }) + } +} + +func TestOlderTransitionsRepairTheLatestOpening(t *testing.T) { + for _, stale := range []entity.IssueStatus{entity.IssueStatusOpen, entity.IssueStatusClosed} { + t.Run(string(stale), func(t *testing.T) { + f := newMaterializerFixture(t) + opened := testIssueLog("build/z", entity.IssueStatusOpen) + closed := testIssueLog("build/a", entity.IssueStatusClosed) + reopened := testIssueLog("build/m", entity.IssueStatusOpen) + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, opened, opened.BuildID)) + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, closed, opened.BuildID)) + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, reopened, reopened.BuildID)) + if stale == entity.IssueStatusOpen { + f.openIssue = entity.OpenIssue{} + } else { + f.openIssue.FirstSeenBuildID = opened.BuildID + } + + log := opened + if stale == entity.IssueStatusClosed { + log = closed + } + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, log, opened.BuildID)) + require.Equal(t, reopened.BuildID, f.openIssue.FirstSeenBuildID) + require.Equal(t, reopened.BuildID, f.summary.FirstSeenBuildID) + }) + } +} + +func TestDelayedOpenIssueCreateUsesTheLatestSummary(t *testing.T) { + for _, status := range []entity.IssueStatus{entity.IssueStatusOpen, entity.IssueStatusClosed} { + t.Run(string(status), func(t *testing.T) { + f := newMaterializerFixture(t) + opened := testIssueLog("build/z", entity.IssueStatusOpen) + newer := testIssueLog("build/a", status) + opening := opened.BuildID + if status == entity.IssueStatusOpen { + opening = newer.BuildID + } + f.beforeOpenIssueWrite = func() { + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, newer, opening)) + } + + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, opened, opened.BuildID)) + if status == entity.IssueStatusClosed { + require.Equal(t, entity.IssueStatusClosed, f.summary.Status) + require.Empty(t, f.openIssue) + } else { + require.Equal(t, newer.BuildID, f.openIssue.FirstSeenBuildID) + } + }) + } +} + +func TestDelayedOpenIssueWritesPreserveANewerOpening(t *testing.T) { + for _, status := range []entity.IssueStatus{entity.IssueStatusOpen, entity.IssueStatusClosed} { + t.Run(string(status), func(t *testing.T) { + f := newMaterializerFixture(t) + opened := testIssueLog("build/z", entity.IssueStatusOpen) + incoming := testIssueLog("build/a", status) + newer := testIssueLog("build/m", entity.IssueStatusOpen) + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, opened, opened.BuildID)) + f.beforeOpenIssueWrite = func() { + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, newer, newer.BuildID)) + } + opening := opened.BuildID + if status == entity.IssueStatusOpen { + opening = incoming.BuildID + } + + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, incoming, opening)) + require.Equal(t, newer.BuildID, f.openIssue.FirstSeenBuildID) + require.Equal(t, newer.BuildID, f.summary.FirstSeenBuildID) + }) + } +} + +func TestOpenIssueReadsTheSummaryAfterTheOpeningReference(t *testing.T) { + f := newMaterializerFixture(t) + opened := testIssueLog("build/z", entity.IssueStatusOpen) + closed := testIssueLog("build/a", entity.IssueStatusClosed) + newer := testIssueLog("build/m", entity.IssueStatusOpen) + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, opened, opened.BuildID)) + f.failures["open_issue/delete"] = assert.AnError + require.ErrorIs(t, f.m.PersistLog(context.Background(), f.stores, closed, opened.BuildID), assert.AnError) + delete(f.failures, "open_issue/delete") + deletes := f.openIssueDeletes + f.beforeOpenIssueGet = func() { + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, newer, newer.BuildID)) + } + + require.NoError(t, f.m.PersistLog(context.Background(), f.stores, closed, opened.BuildID)) + require.Equal(t, newer.BuildID, f.openIssue.FirstSeenBuildID) + require.Equal(t, deletes, f.openIssueDeletes) +} + +func TestOpenIssueRequiresAReadableSummary(t *testing.T) { + f := newMaterializerFixture(t) + err := materializeOpenIssue(context.Background(), f.stores, "validation", "unit") + require.ErrorIs(t, err, storage.ErrNotFound) + require.Empty(t, f.openIssue) +} + +func TestOpenIssueRetryHonorsCanceledContext(t *testing.T) { + f := newMaterializerFixture(t) + log := testIssueLog("build/z", entity.IssueStatusOpen) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + f.beforeOpenIssueWrite = cancel + + require.ErrorIs(t, f.m.PersistLog(ctx, f.stores, log, log.BuildID), context.Canceled) +} diff --git a/stovepipe/extension/storage/README.md b/stovepipe/extension/storage/README.md index e896596bb..872108b9c 100644 --- a/stovepipe/extension/storage/README.md +++ b/stovepipe/extension/storage/README.md @@ -66,7 +66,7 @@ Create returns `ErrAlreadyExists` for any existing task/unit key. Replace compar Listing traverses a bounded queue-prefix primary-key range in ascending bytewise task/unit order. The zero cursor starts the range; otherwise the cursor excludes its key and all earlier keys, even if that open-issue row has been deleted. Empty units identify task-wide issues, while task IDs must be nonempty. Membership can change between reads. A consumer that skips stale references must still advance after the last returned open-issue row, not the last accepted issue. -Open-issue rows are not authoritative issue status. A delayed create can leave a reference to a closed issue, so readers must verify that the summary is open and its opening matches before returning a result. The store does not read summaries, enforce source ordering, backfill existing rows, or populate open-issue rows automatically. No runtime writer or public list API uses this store yet. +Open-issue rows are not authoritative issue status. A delayed create can leave a reference to a closed issue, so readers must verify that the summary is open and its opening matches before returning a result. The store does not read summaries, enforce source ordering, backfill existing rows, or populate open-issue rows automatically. The [issue-log materializer](../../core/issuelog/README.md#open-issue-lookup) maintains open-issue rows after summary persistence during normal processing and recovery. No public list API uses this store yet. ## Issue lifecycle history diff --git a/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel b/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel index 1f82fa4f6..b0351076a 100644 --- a/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel +++ b/test/integration/stovepipe/extension/storage/mysql/BUILD.bazel @@ -11,6 +11,7 @@ go_test( "issue_state_test.go", "issue_summary_test.go", "issue_tracking_test.go", + "open_issue_materializer_test.go", "open_issue_test.go", "project_fact_by_degree_test.go", "queue_state_transitions_test.go", @@ -27,6 +28,7 @@ go_test( "requires-network", ], deps = [ + "//stovepipe/core/issuelog:go_default_library", "//stovepipe/core/queuelog:go_default_library", "//stovepipe/core/queuestate:go_default_library", "//stovepipe/core/requestlog:go_default_library", diff --git a/test/integration/stovepipe/extension/storage/mysql/open_issue_materializer_test.go b/test/integration/stovepipe/extension/storage/mysql/open_issue_materializer_test.go new file mode 100644 index 000000000..5fc6a2207 --- /dev/null +++ b/test/integration/stovepipe/extension/storage/mysql/open_issue_materializer_test.go @@ -0,0 +1,90 @@ +// Copyright (c) 2026 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 mysql + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "github.com/uber/submitqueue/stovepipe/core/issuelog" + "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/storage" +) + +func testOpenIssueMaterialization(t *testing.T, ctx context.Context, factory storage.Factory) { + t.Helper() + stores, err := factory.For(storage.Config{QueueName: "open-issue-materialization/main"}) + require.NoError(t, err) + for _, build := range []entity.Build{ + {ID: "build/z", RequestID: "9", Status: entity.BuildStatusFailed, Version: 1}, + {ID: "build/a", RequestID: "10", Status: entity.BuildStatusSucceeded, Version: 1}, + {ID: "build/m", RequestID: "11", Status: entity.BuildStatusFailed, Version: 1}, + } { + require.NoError(t, stores.GetBuildStore().Create(ctx, build)) + } + materializer := issuelog.NewMaterializer(tally.NoopScope) + openIssues := stores.GetOpenIssueStore() + opened := entity.IssueLog{TaskID: "validation", UnitID: "unit", BuildID: "build/z", Status: entity.IssueStatusOpen, TimestampMs: 1000} + closed := opened + closed.BuildID, closed.Status, closed.TimestampMs = "build/a", entity.IssueStatusClosed, 2000 + reopened := opened + reopened.BuildID, reopened.TimestampMs = "build/m", 3000 + for _, step := range []struct { + log entity.IssueLog + opening, expectedOpening string + }{ + {opened, opened.BuildID, opened.BuildID}, + {closed, opened.BuildID, ""}, + {opened, opened.BuildID, ""}, + {reopened, reopened.BuildID, reopened.BuildID}, + {closed, opened.BuildID, reopened.BuildID}, + } { + require.NoError(t, materializer.PersistLog(ctx, stores, step.log, step.opening)) + refs, err := openIssues.List(ctx, storage.OpenIssueRange{Limit: 2}) + require.NoError(t, err) + summary, err := stores.GetIssueSummaryStore().Get(ctx, opened.TaskID, opened.UnitID) + require.NoError(t, err) + if step.expectedOpening == "" { + require.Empty(t, refs) + require.Equal(t, entity.IssueStatusClosed, summary.Status) + } else { + require.Equal(t, []entity.OpenIssue{{TaskID: opened.TaskID, UnitID: opened.UnitID, FirstSeenBuildID: step.expectedOpening}}, refs) + require.Equal(t, entity.IssueStatusOpen, summary.Status) + require.Equal(t, step.expectedOpening, summary.FirstSeenBuildID) + } + } + + current, err := openIssues.Get(ctx, opened.TaskID, opened.UnitID) + require.NoError(t, err) + deleted, err := openIssues.Delete(ctx, current) + require.NoError(t, err) + require.True(t, deleted) + require.NoError(t, materializer.PersistLog(ctx, stores, opened, opened.BuildID)) + repaired, err := openIssues.Get(ctx, opened.TaskID, opened.UnitID) + require.NoError(t, err) + require.Equal(t, current, repaired) + + stale := current + stale.FirstSeenBuildID = opened.BuildID + replaced, err := openIssues.Replace(ctx, stale, current.FirstSeenBuildID) + require.NoError(t, err) + require.True(t, replaced) + require.NoError(t, materializer.PersistLog(ctx, stores, closed, opened.BuildID)) + repaired, err = openIssues.Get(ctx, opened.TaskID, opened.UnitID) + require.NoError(t, err) + require.Equal(t, current, repaired) +} diff --git a/test/integration/stovepipe/extension/storage/mysql/storage_test.go b/test/integration/stovepipe/extension/storage/mysql/storage_test.go index 259326d9a..868833b91 100644 --- a/test/integration/stovepipe/extension/storage/mysql/storage_test.go +++ b/test/integration/stovepipe/extension/storage/mysql/storage_test.go @@ -120,6 +120,11 @@ func TestMySQLStorage(t *testing.T) { testOpenIssueStore(t, ctx, factory) }) + t.Run("OpenIssueMaterialization", func(t *testing.T) { + resetStorage(t, db) + testOpenIssueMaterialization(t, ctx, factory) + }) + t.Run("IssueStateList", func(t *testing.T) { resetStorage(t, db) testIssueStateList(t, ctx, factory)