diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 645c2a7aa..fab4e3500 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -86,14 +86,11 @@ type ProcessResult struct { // 15min (matching the RI-exchange stale-sweep threshold in pkg/exchange) // leaves a wide safety margin. // -// Note: the purchase.Reaper (reaper.go) uses DefaultReapAfter (10m) to cover -// both "approved" and "running" rows via an atomic CAS. These two thresholds -// serve different sweep paths and the 15m vs 10m difference is intentional: -// the reaper is CAS-protected and can safely reap "running" rows (the real -// executor wins the CAS race if it is still alive), while this legacy sweep -// only targets "approved" rows from the cron path and is deliberately -// conservative (05-N3). A future consolidation should align both under a -// single env-configurable threshold. +// Note: the stuck-purchase reaper (reaper.go) fails "running" rows after +// DefaultReapAfter (10m) but leaves "approved" rows alone until +// minApprovedReapAfter, which is derived from this constant plus one process +// sweep interval, so this recovery always gets to re-drive a stranded +// approval before the reaper fails it (issue #894). const staleApprovedThreshold = 15 * time.Minute // NotificationResult holds the result of sending notifications. diff --git a/internal/purchase/reaper.go b/internal/purchase/reaper.go index e844c2971..a3c0b2d0a 100644 --- a/internal/purchase/reaper.go +++ b/internal/purchase/reaper.go @@ -12,12 +12,13 @@ import ( ) // stuckStatuses are the in-flight execution statuses that the reaper will -// flip to "failed" when the row has been sitting in one of them for longer -// than the configured reapAfter duration. These are the only intermediate -// states the synchronous executor passes through between the user clicking -// approve and the row landing in a terminal state (completed/failed) — -// pending/notified executions are advanced by the scheduler tick on its -// own cadence, not by us. See ApproveExecution + executeAndFinalize for +// flip to "failed" when the row has been sitting in one of them for too long: +// reapAfter for "running", approvedReapAfter(reapAfter) for "approved" (see +// reapPasses). These are the only intermediate states the synchronous +// executor passes through between the user clicking approve and the row +// landing in a terminal state (completed/failed) — pending/notified +// executions are advanced by the scheduler tick on its own cadence, not by +// us. See ApproveExecution + executeAndFinalize for // the originating state machine. // // Kept as a package-level var (not a const slice) so tests can read it @@ -29,13 +30,13 @@ var stuckStatuses = []string{"approved", "running"} // migrating to a typed status enum) shows up at the one call site. const failedStatus = "failed" -// DefaultReapAfter is the default age at which an approved/running execution -// is considered stuck. The issue (#678) calls for "configurable via env var, -// default 10m" — the threshold is intentionally conservative: the happy-path -// executor completes in <60s; the longest legitimately-slow paths -// (multi-account fan-out, provider rate-limit backoff) settle in <3min. 10 -// minutes leaves multiple multiples of headroom so the reaper never fights -// a real executor. +// DefaultReapAfter is the default age at which a running execution is +// considered stuck (approved rows use approvedReapAfter). The issue (#678) +// calls for "configurable via env var, default 10m" — the threshold is +// intentionally conservative: the happy-path executor completes in <60s; the +// longest legitimately-slow paths (multi-account fan-out, provider rate-limit +// backoff) settle in <3min. 10 minutes leaves multiple multiples of headroom +// so the reaper never fights a real executor. const DefaultReapAfter = 10 * time.Minute // reapAfterEnvVar is the env var read by ParseReapAfterFromEnv. Centralized @@ -98,12 +99,59 @@ func ParseReapAfterFromEnv() time.Duration { return d } +// processSweepInterval is the default cadence of the process_scheduled_purchases +// sweep, which is what runs RecoverStrandedApprovals (terraform: +// process_scheduled_purchases_schedule defaults to "rate(1 hour)"). +const processSweepInterval = time.Hour + +// processSweepSlack covers the process sweep's own run time and EventBridge +// delivery jitter, so a recovery run that starts on schedule can still claim +// the row before the reaper's next tick. +const processSweepSlack = 5 * time.Minute + +// minApprovedReapAfter is the earliest age at which the reaper may fail an +// "approved" row (issue #894). RecoverStrandedApprovals only sees approved rows +// older than staleApprovedThreshold, and it runs once per process sweep, so a +// stranded approval first becomes recoverable somewhere in +// (staleApprovedThreshold, staleApprovedThreshold + processSweepInterval]. The +// reaper must wait past that window, or it fails the row first and the +// idempotent re-drive never runs. "running" rows are not affected: recovery +// never re-drives them, so the reaper stays their only backstop. +const minApprovedReapAfter = staleApprovedThreshold + processSweepInterval + processSweepSlack + +// approvedReapAfter returns the age threshold the reaper applies to "approved" +// rows: the configured reapAfter, raised to minApprovedReapAfter when lower. +func approvedReapAfter(reapAfter time.Duration) time.Duration { + return max(reapAfter, minApprovedReapAfter) +} + +// reapPass is one SELECT of the sweep: the statuses it lists and the age past +// which those rows count as stuck. +type reapPass struct { + statuses []string + after time.Duration +} + +// reapPasses splits the sweep by status so "approved" rows get the later +// threshold that leaves room for RecoverStrandedApprovals (issue #894). +func reapPasses(reapAfter time.Duration) []reapPass { + return []reapPass{ + {statuses: []string{"running"}, after: reapAfter}, + {statuses: []string{"approved"}, after: approvedReapAfter(reapAfter)}, + } +} + // ReapStuckExecutions runs one sweep that finds purchase_executions stuck in -// approved/running longer than reapAfter and atomically transitions them to -// "failed" via the existing TransitionExecutionStatus CAS. Each successful -// transition also persists a canonical error message so the History UI -// (issue #621) can show why the row was reaped and the operator knows it's -// safe to retry. +// running longer than reapAfter, or in approved longer than +// approvedReapAfter(reapAfter), and atomically transitions them to "failed" via +// the existing TransitionExecutionStatus CAS. Each successful transition also +// persists a canonical error message so the History UI (issue #621) can show +// why the row was reaped and the operator knows it's safe to retry. +// +// Approved rows get the later threshold so the process sweep's +// RecoverStrandedApprovals can re-drive them first (issue #894); the reaper +// only fails the ones recovery never picked up, e.g. on deployments that do +// not schedule the process sweep. // // Safety properties: // - Local-status-only: the reaper never touches provider commitments. If @@ -118,25 +166,27 @@ func ParseReapAfterFromEnv() time.Duration { // - Per-row error-isolation: a failure on row N never blocks rows // N+1..K. Counts are aggregated in ReapResult for the caller to log. func (m *Manager) ReapStuckExecutions(ctx context.Context, reapAfter time.Duration) (*ReapResult, error) { - stuck, err := m.config.ListStuckExecutions(ctx, stuckStatuses, reapAfter) - if err != nil { - return nil, fmt.Errorf("failed to list stuck executions: %w", err) + result := &ReapResult{} + now := time.Now() + for _, pass := range reapPasses(reapAfter) { + stuck, err := m.config.ListStuckExecutions(ctx, pass.statuses, pass.after) + if err != nil { + return nil, fmt.Errorf("failed to list stuck executions: %w", err) + } + result.Found += len(stuck) + for i := range stuck { + m.reapOne(ctx, &stuck[i], pass.after, now, result) + } } - result := &ReapResult{Found: len(stuck)} - if len(stuck) == 0 { - logging.Debugf("purchase reaper: no stuck executions (threshold %s)", reapAfter) + if result.Found == 0 { + logging.Debugf("purchase reaper: no stuck executions (threshold running=%s approved=%s)", + reapAfter, approvedReapAfter(reapAfter)) return result, nil } - now := time.Now() - for _rvc := range stuck { - exec := stuck[_rvc] - m.reapOne(ctx, &exec, reapAfter, now, result) - } - - logging.Infof("purchase reaper sweep complete: found=%d reaped=%d race_lost=%d errored=%d (threshold %s)", - result.Found, result.Reaped, result.RaceLost, result.Errored, reapAfter) + logging.Infof("purchase reaper sweep complete: found=%d reaped=%d race_lost=%d errored=%d (threshold running=%s approved=%s)", + result.Found, result.Reaped, result.RaceLost, result.Errored, reapAfter, approvedReapAfter(reapAfter)) return result, nil } diff --git a/internal/purchase/reaper_ordering_test.go b/internal/purchase/reaper_ordering_test.go new file mode 100644 index 000000000..e0824a507 --- /dev/null +++ b/internal/purchase/reaper_ordering_test.go @@ -0,0 +1,123 @@ +package purchase + +import ( + "context" + "fmt" + "maps" + "slices" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// defaultProcessSchedule is the process_scheduled_purchases sweep cadence the +// terraform modules default to ("rate(1 hour)"). RecoverStrandedApprovals runs +// on that sweep, so a stranded approval may wait this long past +// staleApprovedThreshold before recovery sees it. +const defaultProcessSchedule = time.Hour + +// listedThresholds runs one reaper sweep against a store that returns no rows +// and records the age threshold each ListStuckExecutions call used per status. +func listedThresholds(t *testing.T, reapAfter time.Duration) map[string]time.Duration { + t.Helper() + ctx := context.Background() + store := new(MockConfigStore) + got := map[string]time.Duration{} + store.On("ListStuckExecutions", ctx, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + for _, s := range args.Get(1).([]string) { + got[s] = args.Get(2).(time.Duration) + } + }). + Return([]config.PurchaseExecution{}, nil) + + _, err := newReaperManager(store).ReapStuckExecutions(ctx, reapAfter) + require.NoError(t, err) + return got +} + +// Issue #894: with the default thresholds the reaper must not list approved +// rows until RecoverStrandedApprovals has had a full process sweep past +// staleApprovedThreshold to re-drive them. Before the fix approved rows were +// listed at reapAfter (10m) and failed before recovery could see them (15m). +func TestReapStuckExecutions_ApprovedRowsWaitForStrandedRecovery(t *testing.T) { + got := listedThresholds(t, DefaultReapAfter) + + require.Contains(t, got, "approved", "the reaper must still back-stop approved rows") + assert.Greater(t, got["approved"], staleApprovedThreshold+defaultProcessSchedule, + "approved rows must outlive the stranded-approval recovery window") + require.Contains(t, got, "running") + assert.Equal(t, DefaultReapAfter, got["running"], + "running rows are not re-driven by recovery and keep the configured threshold") +} + +// A reapAfter configured below the recovery window (PURCHASE_APPROVED_REAP_AFTER) +// must not reopen the race for approved rows. +func TestReapStuckExecutions_ShortReapAfterDoesNotBeatRecovery(t *testing.T) { + got := listedThresholds(t, 2*time.Minute) + + assert.Greater(t, got["approved"], staleApprovedThreshold+defaultProcessSchedule) + assert.Equal(t, 2*time.Minute, got["running"]) +} + +// A reapAfter configured above the recovery window is honored as is for both +// statuses: the floor only ever delays the approved pass, never hastens it. +func TestReapStuckExecutions_LongReapAfterIsHonoredForApproved(t *testing.T) { + long := 3 * time.Hour + got := listedThresholds(t, long) + + assert.Equal(t, long, got["approved"]) + assert.Equal(t, long, got["running"]) +} + +// The approved and running passes together must cover exactly stuckStatuses, +// so splitting the sweep neither drops a status nor widens it. +func TestReapStuckExecutions_PassesCoverStuckStatuses(t *testing.T) { + got := listedThresholds(t, DefaultReapAfter) + + listed := slices.Sorted(maps.Keys(got)) + want := slices.Clone(stuckStatuses) + slices.Sort(want) + assert.Equal(t, want, listed) +} + +// The per-row reap annotation reports the threshold of the pass that listed +// the row, so an approved row is not described as stuck for only 10 minutes. +func TestReapStuckExecutions_ApprovedAnnotationUsesApprovedThreshold(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + row := stuckExec("exec-old-approved", "approved") + flipped := row + flipped.Status = failedStatus + expectStuckLists(ctx, store, DefaultReapAfter, row) + store.On("TransitionExecutionStatus", ctx, row.ExecutionID, stuckStatuses, failedStatus, (*string)(nil)). + Return(&flipped, nil) + var saved *config.PurchaseExecution + store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }).Return(nil) + + result, err := newReaperManager(store).ReapStuckExecutions(ctx, DefaultReapAfter) + + require.NoError(t, err) + assert.Equal(t, 1, result.Reaped) + require.NotNil(t, saved) + wantMinutes := int(approvedReapAfter(DefaultReapAfter) / time.Minute) + assert.Contains(t, saved.Error, fmt.Sprintf("reaped after %dm in approved state", wantMinutes)) + store.AssertExpectations(t) +} + +// expectStuckLists stubs both reaper passes: running rows listed at reapAfter +// and approved rows at approvedReapAfter(reapAfter). Rows are routed to the +// pass matching their status. +func expectStuckLists(ctx context.Context, store *MockConfigStore, reapAfter time.Duration, rows ...config.PurchaseExecution) { + byStatus := map[string][]config.PurchaseExecution{"running": {}, "approved": {}} + for _, r := range rows { + byStatus[r.Status] = append(byStatus[r.Status], r) + } + store.On("ListStuckExecutions", ctx, []string{"running"}, reapAfter).Return(byStatus["running"], nil) + store.On("ListStuckExecutions", ctx, []string{"approved"}, approvedReapAfter(reapAfter)).Return(byStatus["approved"], nil) +} diff --git a/internal/purchase/reaper_test.go b/internal/purchase/reaper_test.go index 50606420a..d943f8253 100644 --- a/internal/purchase/reaper_test.go +++ b/internal/purchase/reaper_test.go @@ -69,8 +69,7 @@ func TestReapStuckExecutions_StaleApprovedFlippedToFailed(t *testing.T) { transitioned := row transitioned.Status = failedStatus - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-A", stuckStatuses, failedStatus, (*string)(nil)). Return(&transitioned, nil) store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { @@ -104,7 +103,7 @@ func TestReapStuckExecutions_EC2RowIsNotAdvertisedAsSafeToRetry(t *testing.T) { row := stuckExecWithAWSEC2Rec("exec-ec2", "running") transitioned := row transitioned.Status = failedStatus - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter).Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-ec2", stuckStatuses, failedStatus, (*string)(nil)).Return(&transitioned, nil) var saved *config.PurchaseExecution store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). @@ -128,8 +127,7 @@ func TestReapStuckExecutions_StaleRunningFlippedToFailed(t *testing.T) { transitioned := row transitioned.Status = failedStatus - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-B", stuckStatuses, failedStatus, (*string)(nil)). Return(&transitioned, nil) store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { @@ -156,8 +154,7 @@ func TestReapStuckExecutions_YoungerThanThresholdNotTouched(t *testing.T) { store := new(MockConfigStore) reapAfter := 10 * time.Minute - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{}, nil) + expectStuckLists(ctx, store, reapAfter) // No TransitionExecutionStatus / SavePurchaseExecution expectations // — mock.AssertExpectations will fail if either is called. @@ -173,30 +170,37 @@ func TestReapStuckExecutions_YoungerThanThresholdNotTouched(t *testing.T) { func TestReapStuckExecutions_TerminalStatusNotTouched(t *testing.T) { // The store filters out terminal statuses via the WHERE clause; the - // reaper never sees completed/failed/canceled rows. Verify the - // reaper passes only stuckStatuses (approved/running) to the store - // — never the terminal set. This regression-guards a future change - // that accidentally widens stuckStatuses. + // reaper never sees completed/failed/canceled rows. Verify every pass + // passes only stuckStatuses (approved/running) to the store — never + // the terminal set — and that together the passes list both. This + // regression-guards a future change that accidentally widens + // stuckStatuses. ctx := context.Background() store := new(MockConfigStore) reapAfter := 10 * time.Minute + seen := map[string]bool{} store.On("ListStuckExecutions", ctx, mock.MatchedBy(func(statuses []string) bool { - // Must contain "approved" and "running" and NOT contain any - // terminal status. - seen := map[string]bool{} + // NOT contain any terminal or pre-approval status. for _, s := range statuses { - seen[s] = true + if s != "approved" && s != "running" { + return false + } } - return seen["approved"] && seen["running"] && - !seen["completed"] && !seen["failed"] && !seen["canceled"] && !seen["pending"] && !seen["notified"] - }), reapAfter). + return len(statuses) > 0 + }), mock.Anything). + Run(func(args mock.Arguments) { + for _, s := range args.Get(1).([]string) { + seen[s] = true + } + }). Return([]config.PurchaseExecution{}, nil) mgr := newReaperManager(store) _, err := mgr.ReapStuckExecutions(ctx, reapAfter) require.NoError(t, err) store.AssertExpectations(t) + assert.True(t, seen["approved"] && seen["running"], "both stuck statuses must be listed, got %v", seen) } func TestReapStuckExecutions_CASRaceLostNoError(t *testing.T) { @@ -211,8 +215,7 @@ func TestReapStuckExecutions_CASRaceLostNoError(t *testing.T) { reapAfter := 10 * time.Minute row := stuckExec("exec-race", "approved") - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-race", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-race cannot transition from %q to %q", config.ErrExecutionNotInExpectedStatus, "completed", "failed")) @@ -242,8 +245,7 @@ func TestReapStuckExecutions_RowVanishedTreatedAsRaceLost(t *testing.T) { reapAfter := 10 * time.Minute row := stuckExec("exec-gone", "approved") - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-gone", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, fmt.Errorf("%w: execution exec-gone", config.ErrNotFound)) @@ -269,8 +271,7 @@ func TestReapStuckExecutions_HardDBErrorClassifiedAsErrored(t *testing.T) { reapAfter := 10 * time.Minute row := stuckExec("exec-dbflake", "running") - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-dbflake", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, errors.New("connection refused")) @@ -294,8 +295,7 @@ func TestReapStuckExecutions_CASReturnsNilNilTreatedAsRaceLost(t *testing.T) { reapAfter := 10 * time.Minute row := stuckExec("exec-nilnil", "running") - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-nilnil", stuckStatuses, failedStatus, (*string)(nil)). Return(nil, nil) @@ -319,7 +319,7 @@ func TestReapStuckExecutions_ThreeStuckRowsAllReaped(t *testing.T) { stuckExec("exec-2", "running"), stuckExec("exec-3", "approved"), } - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter).Return(rows, nil) + expectStuckLists(ctx, store, reapAfter, rows...) for _, r := range rows { flipped := r @@ -348,7 +348,7 @@ func TestReapStuckExecutions_ListStoreError(t *testing.T) { store := new(MockConfigStore) reapAfter := 10 * time.Minute - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). + store.On("ListStuckExecutions", ctx, []string{"running"}, reapAfter). Return(nil, errors.New("db down")) mgr := newReaperManager(store) @@ -358,6 +358,24 @@ func TestReapStuckExecutions_ListStoreError(t *testing.T) { store.AssertExpectations(t) } +// A failure on the approved pass propagates the same way as one on the +// running pass: the sweep must not report success with approved rows unseen. +func TestReapStuckExecutions_ApprovedListStoreError(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + reapAfter := 10 * time.Minute + + store.On("ListStuckExecutions", ctx, []string{"running"}, reapAfter). + Return([]config.PurchaseExecution{}, nil) + store.On("ListStuckExecutions", ctx, []string{"approved"}, approvedReapAfter(reapAfter)). + Return(nil, errors.New("db down")) + + _, err := newReaperManager(store).ReapStuckExecutions(ctx, reapAfter) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to list stuck executions") + store.AssertExpectations(t) +} + func TestReapStuckExecutions_SaveErrorAfterCASStillCountsAsReaped(t *testing.T) { // The CAS already flipped the row to failed; the follow-up // canonical-error save is best-effort. A save failure must not @@ -370,8 +388,7 @@ func TestReapStuckExecutions_SaveErrorAfterCASStillCountsAsReaped(t *testing.T) row := stuckExec("exec-saveflake", "approved") flipped := row flipped.Status = failedStatus - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-saveflake", stuckStatuses, failedStatus, (*string)(nil)). Return(&flipped, nil) store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). @@ -459,8 +476,7 @@ func TestReapStuckExecutions_AzureSPNotSafeToRetry(t *testing.T) { transitioned := row transitioned.Status = failedStatus - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-azsp", stuckStatuses, failedStatus, (*string)(nil)). Return(&transitioned, nil) store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { @@ -490,8 +506,7 @@ func TestReapStuckExecutions_EmptyRecsNotSafeToRetry(t *testing.T) { transitioned := row transitioned.Status = failedStatus - store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter). - Return([]config.PurchaseExecution{row}, nil) + expectStuckLists(ctx, store, reapAfter, row) store.On("TransitionExecutionStatus", ctx, "exec-empty", stuckStatuses, failedStatus, (*string)(nil)). Return(&transitioned, nil) store.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { diff --git a/internal/server/handler.go b/internal/server/handler.go index 8614259b0..71c1659ad 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -45,8 +45,9 @@ const ( TaskCollectAnalytics ScheduledTaskType = "analytics_collect" TaskRIExchangeReshape ScheduledTaskType = "ri_exchange_reshape" // TaskReapStuckPurchases sweeps purchase_executions stuck in - // approved/running longer than PURCHASE_APPROVED_REAP_AFTER and flips - // them to "failed" via the existing TransitionExecutionStatus CAS. + // running longer than PURCHASE_APPROVED_REAP_AFTER (approved rows wait + // longer, see purchase.minApprovedReapAfter) and flips them to "failed" + // via the existing TransitionExecutionStatus CAS. // Backstop for synchronous-executor crashes (Lambda timeout, OOM, // network hang) that leave rows orphaned in an in-flight state. // See internal/purchase/reaper.go + issue #678. diff --git a/terraform/environments/aws/variables.tf b/terraform/environments/aws/variables.tf index c0acf8168..14b8f7b7a 100644 --- a/terraform/environments/aws/variables.tf +++ b/terraform/environments/aws/variables.tf @@ -313,7 +313,7 @@ variable "fire_scheduled_purchases_schedule" { } variable "enable_process_scheduled_purchases_schedule" { - description = "Enable the scheduled process_scheduled_purchases sweep. Opt-in, default false. Note (#894): the stuck-purchase reaper can fail stranded approved rows before process re-drives them." + description = "Enable the scheduled process_scheduled_purchases sweep. Opt-in, default false." type = bool default = false } diff --git a/terraform/modules/compute/aws/lambda/variables.tf b/terraform/modules/compute/aws/lambda/variables.tf index 4d858da30..b3e96921c 100644 --- a/terraform/modules/compute/aws/lambda/variables.tf +++ b/terraform/modules/compute/aws/lambda/variables.tf @@ -232,7 +232,7 @@ variable "fire_scheduled_purchases_schedule" { } variable "enable_process_scheduled_purchases_schedule" { - description = "Enable the scheduled process_scheduled_purchases sweep (proposes purchases that are due). Opt-in: default false so no purchase proposal runs unless an operator asks for it. Note (#894): the stuck-purchase reaper can fail stranded approved rows before process re-drives them." + description = "Enable the scheduled process_scheduled_purchases sweep (proposes purchases that are due). Opt-in: default false so no purchase proposal runs unless an operator asks for it." type = bool default = false } @@ -268,7 +268,7 @@ variable "finalize_revocations_schedule" { } variable "purchase_approved_reap_after" { - description = "Threshold age for the stuck-purchase reaper. Any execution sitting in approved/running longer than this gets flipped to failed on the next sweep. Parsed via Go time.ParseDuration (e.g. \"10m\", \"15m\", \"1h\"). Empty string falls back to the in-code default (10m)." + description = "Threshold age for the stuck-purchase reaper. Any execution sitting in running longer than this gets flipped to failed on the next sweep; approved rows wait at least 1h20m so the process sweep can re-drive them first (#894). Parsed via Go time.ParseDuration (e.g. \"10m\", \"15m\", \"1h\"). Empty string falls back to the in-code default (10m)." type = string default = "" }