diff --git a/internal/api/handler_execution_settle.go b/internal/api/handler_execution_settle.go new file mode 100644 index 000000000..0676f40e8 --- /dev/null +++ b/internal/api/handler_execution_settle.go @@ -0,0 +1,157 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "strings" + "time" + "unicode/utf8" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" +) + +const ( + executionSettleReasonMin = 10 + executionSettleReasonMax = 300 + executionSettleBodyMax = 2048 +) + +// executionSettleRequest is the body of POST /api/purchases/{id}/settle. +type executionSettleRequest struct { + Reason string `json:"reason"` + Acknowledge bool `json:"acknowledge_unknown_outcome"` +} + +type executionSettleResponse struct { + ExecutionID string `json:"execution_id"` + Status string `json:"status"` + SettledAt string `json:"outcome_settled_at"` + SettledBy string `json:"outcome_settled_by"` + Settlement string `json:"outcome_settlement"` + Replayed bool `json:"replayed"` +} + +// executionSettleStore is the part of the store the endpoint needs. +type executionSettleStore interface { + SettleFailedExecution(ctx context.Context, req config.ExecutionSettleRequest) (*config.ExecutionSettlement, error) +} + +// settleExecution records an operator's audited settlement of a failed plan +// execution whose outcome is unknown (issue #999), so it stops blocking the +// delete of its editor-managed plan. The row stays failed and keeps counting +// as possibly bought; it can no longer be retried. Gated like a purchase: +// execute:purchases, account access to the execution, and the grant's account +// constraints with strict scope. +func (h *Handler) settleExecution(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) { + if err := validateUUID(id); err != nil { + return nil, err + } + session, err := h.requirePermission(ctx, req, auth.ActionExecute, auth.ResourcePurchases) + if err != nil { + return nil, err + } + body, err := decodeExecutionSettle(req.Body) + if err != nil { + return nil, err + } + if authErr := h.authorizeExecutionSettle(ctx, session, id); authErr != nil { + return nil, authErr + } + store, ok := h.config.(executionSettleStore) + if !ok { + return nil, NewClientError(503, "Execution settle storage is unavailable") + } + res, err := store.SettleFailedExecution(ctx, config.ExecutionSettleRequest{ + ExecutionID: id, Actor: session.UserID, Reason: body.Reason, Acknowledge: body.Acknowledge, + }) + if err != nil { + return nil, mapExecutionSettleError(err) + } + if !res.Replayed { + logging.Infof("purchase execution settled by operator: execution=%s actor=%q via=%s acknowledged_unknown_outcome=true settlement=%q", + id, session.UserID, credentialKind(ctx, session), res.Settlement) + } + return executionSettleResponse{ + ExecutionID: res.ExecutionID, Status: res.Status, SettledAt: res.SettledAt.UTC().Format(time.RFC3339), + SettledBy: res.SettledBy, Settlement: res.Settlement, Replayed: res.Replayed, + }, nil +} + +// authorizeExecutionSettle checks account access to the execution, then the +// execute:purchases constraints against what the execution would have bought +// (its recommendations' providers, services, regions and total commitment) +// and its accounts. StrictScope makes a dimension the execution leaves +// unknown, such as the provider of a step with no recommendations, require an +// unconstrained grant. +func (h *Handler) authorizeExecutionSettle(ctx context.Context, session *Session, id string) error { + if err := h.requireExecutionAccess(ctx, session, id); err != nil { + return err + } + execution, err := h.config.GetExecutionByID(ctx, id) + if errors.Is(err, config.ErrNotFound) || (err == nil && execution == nil) { + return errNotFound + } + if err != nil { + return fmt.Errorf("failed to get execution: %w", err) + } + accounts, err := h.executionAccounts(ctx, execution, nil) + if err != nil { + return err + } + ids := make([]string, 0, len(accounts)) + for i := range accounts { + ids = append(ids, accounts[i].ID) + } + sets := purchaseConstraintSets(execution.Recommendations) + if len(sets) == 0 { + sets = []auth.PermissionConstraints{{}} + } + for i := range sets { + sets[i].AccountIDs, sets[i].StrictScope = ids, true + } + return h.requirePermissionConstraints(ctx, session, auth.ActionExecute, auth.ResourcePurchases, sets) +} + +func mapExecutionSettleError(err error) error { + switch { + case errors.Is(err, config.ErrExecutionSettleNotFound): + return errNotFound + case errors.Is(err, config.ErrExecutionSettleInvalid): + return NewClientError(400, err.Error()) + case errors.Is(err, config.ErrExecutionSettleConflict): + return NewClientError(409, err.Error()) + } + return err +} + +func decodeExecutionSettle(raw string) (executionSettleRequest, error) { + var body executionSettleRequest + if len(raw) > executionSettleBodyMax { + return body, NewClientError(400, "Settle body is too large") + } + decoder := json.NewDecoder(strings.NewReader(raw)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&body); err != nil { + return body, NewClientError(400, "Invalid settle body") + } + if err := decoder.Decode(new(any)); err != io.EOF { + return body, NewClientError(400, "Expected one settle body") + } + body.Reason = strings.TrimSpace(body.Reason) + if !body.Acknowledge { + return body, NewClientError(400, "acknowledge_unknown_outcome must be true: this purchase may have been submitted; check the cloud account first") + } + if n := utf8.RuneCountInString(body.Reason); n < executionSettleReasonMin || n > executionSettleReasonMax { + return body, NewClientError(400, fmt.Sprintf("reason must be %d to %d characters", executionSettleReasonMin, executionSettleReasonMax)) + } + if hasControlRune(body.Reason) { + return body, NewClientError(400, "reason must not contain control characters") + } + return body, nil +} diff --git a/internal/api/handler_execution_settle_integration_test.go b/internal/api/handler_execution_settle_integration_test.go new file mode 100644 index 000000000..2e3812592 --- /dev/null +++ b/internal/api/handler_execution_settle_integration_test.go @@ -0,0 +1,112 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const settleBody = `{"reason":"checked the subscription: nothing was bought","acknowledge_unknown_outcome":true}` + +var purchaserPerms = []auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases}, +} + +func (f *createConcurrencyFixture) settledAt(ctx context.Context, t *testing.T, id string) *time.Time { + t.Helper() + var at *time.Time + require.NoError(t, f.pool.QueryRow(ctx, `SELECT outcome_settled_at FROM purchase_executions WHERE execution_id = $1`, id).Scan(&at)) + return at +} + +// The issue #999 scenario through the HTTP surface: a failed row with an +// unknown outcome makes the managed delete 409 until a purchaser settles it. +func TestSettleUnblocksManagedPlanDeleteOverHTTP(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + setPlanManaged(ctx, t, f, true) + id := f.saveRow(ctx, t, "failed", false, time.Now(), true) + requireLiveExecutionsRefusal(t, f.deletePlan(ctx, f.handler)) + + h := limitedHandler(f, purchaserPerms, nil) + h.auth.(*MockAuthService).On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil) + req := managedGuardRequest(settleBody) + req.Headers["X-CSRF-Token"] = "test-csrf" + req.RequestContext.HTTP.Method = "POST" + req.RequestContext.HTTP.Path = "/api/purchases/" + id + "/settle" + resp, err := h.HandleRequest(ctx, req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode, resp.Body) + var got executionSettleResponse + require.NoError(t, json.Unmarshal([]byte(resp.Body), &got)) + require.Equal(t, id, got.ExecutionID) + require.Equal(t, "failed", got.Status) + require.Equal(t, "00000000-0000-0000-0000-000000000001", got.SettledBy) + require.Contains(t, got.Settlement, `reason="checked the subscription: nothing was bought"`) + require.False(t, got.Replayed) + + require.NoError(t, f.deletePlan(ctx, f.handler)) + row, err := f.store.GetExecutionByID(ctx, id) + require.NoError(t, err) + require.Equal(t, "failed", row.Status) + require.NotNil(t, f.settledAt(ctx, t, id)) +} + +func TestSettleAuthorizationAndValidation(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + setPlanManaged(ctx, t, f, true) + id := f.saveRow(ctx, t, "failed", false, time.Now(), true) + completed := f.saveRow(ctx, t, "completed", false, time.Now(), true) + var acct string + require.NoError(t, f.pool.QueryRow(ctx, `SELECT account_id::text FROM plan_accounts WHERE plan_id = $1`, f.planID).Scan(&acct)) + constrained := func(c auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{{Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &c}} + } + cases := []struct { + name string + perms []auth.Permission + scoped []string + id string + body string + status int + }{ + {"admin without execute", []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, nil, id, settleBody, http.StatusForbidden}, + {"out of account scope", purchaserPerms, []string{"other-account"}, id, settleBody, http.StatusNotFound}, + {"execute constrained to another account", constrained(auth.PermissionConstraints{AccountIDs: []string{"other-account"}}), nil, id, settleBody, http.StatusForbidden}, + {"execute constrained to another provider", constrained(auth.PermissionConstraints{AccountIDs: []string{acct}, Providers: []string{"aws"}}), nil, id, settleBody, http.StatusForbidden}, + {"no acknowledgement", purchaserPerms, nil, id, `{"reason":"checked the subscription"}`, http.StatusBadRequest}, + {"short reason", purchaserPerms, nil, id, `{"reason":"ok","acknowledge_unknown_outcome":true}`, http.StatusBadRequest}, + {"unknown field", purchaserPerms, nil, id, `{"reason":"checked the subscription","acknowledge_unknown_outcome":true,"force":true}`, http.StatusBadRequest}, + {"completed row", purchaserPerms, nil, completed, settleBody, http.StatusConflict}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := limitedHandler(f, tc.perms, tc.scoped) + _, err := h.settleExecution(ctx, managedGuardRequest(tc.body), tc.id) + var ce *clientError + if errors.As(err, &ce) { + require.Equal(t, tc.status, ce.code, ce.message) + } else { + require.True(t, IsNotFoundError(err) && tc.status == http.StatusNotFound, "got %v", err) + } + require.Nil(t, f.settledAt(ctx, t, tc.id), "a refused settle writes nothing") + }) + } + + t.Run("constrained to the plan's account", func(t *testing.T) { + h := limitedHandler(f, constrained(auth.PermissionConstraints{AccountIDs: []string{acct}}), nil) + _, err := h.settleExecution(ctx, managedGuardRequest(settleBody), id) + require.NoError(t, err) + }) +} diff --git a/internal/api/handler_execution_settle_test.go b/internal/api/handler_execution_settle_test.go new file mode 100644 index 000000000..d4e7e942d --- /dev/null +++ b/internal/api/handler_execution_settle_test.go @@ -0,0 +1,45 @@ +package api + +import ( + "errors" + "fmt" + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/require" +) + +func TestDecodeExecutionSettle(t *testing.T) { + ok, err := decodeExecutionSettle(`{"reason":" checked the account ","acknowledge_unknown_outcome":true}`) + require.NoError(t, err) + require.Equal(t, "checked the account", ok.Reason) + + for name, body := range map[string]string{ + "empty": ``, + "no ack": `{"reason":"checked the account"}`, + "ack false": `{"reason":"checked the account","acknowledge_unknown_outcome":false}`, + "short": `{"reason":" short ","acknowledge_unknown_outcome":true}`, + "long": `{"reason":"` + strings.Repeat("x", executionSettleReasonMax+1) + `","acknowledge_unknown_outcome":true}`, + "control": `{"reason":"checked the\naccount","acknowledge_unknown_outcome":true}`, + "unknown field": `{"reason":"checked the account","acknowledge_unknown_outcome":true,"x":1}`, + "two bodies": `{"reason":"checked the account","acknowledge_unknown_outcome":true}{}`, + "too large": strings.Repeat(" ", executionSettleBodyMax+1), + } { + _, err := decodeExecutionSettle(body) + var ce *clientError + require.True(t, errors.As(err, &ce), name) + require.Equal(t, 400, ce.code, name) + } +} + +func TestMapExecutionSettleError(t *testing.T) { + require.True(t, IsNotFoundError(mapExecutionSettleError(config.ErrExecutionSettleNotFound))) + for sentinel, code := range map[error]int{config.ErrExecutionSettleInvalid: 400, config.ErrExecutionSettleConflict: 409} { + var ce *clientError + require.True(t, errors.As(mapExecutionSettleError(fmt.Errorf("%w: detail", sentinel)), &ce)) + require.Equal(t, code, ce.code) + } + other := errors.New("db down") + require.Equal(t, other, mapExecutionSettleError(other), "an internal error is not a client error") +} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ce5212ded..eca01d7be 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2646,8 +2646,10 @@ func (h *Handler) persistRetrySuccessorTx(ctx context.Context, failedExec, newEx map[string]any{}) } if lostRace { + // Zero rows linked: a concurrent retry won, or an operator settled the + // row (issue #999), which retires it from retry for good. return NewClientErrorWithDetails(409, - fmt.Sprintf("execution %s was already retried; act on its descendant instead", failedExec.ExecutionID), + fmt.Sprintf("execution %s was already retried or settled; reload it", failedExec.ExecutionID), map[string]any{}) } return fmt.Errorf("failed to save retry execution: %w", err) diff --git a/internal/api/router.go b/internal/api/router.go index 48fe7a20c..d9576eadf 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -186,6 +186,10 @@ func (r *Router) registerRoutes() { // AuthUser: router-level gate; per-handler requirePermission is the // real security boundary. {PathPrefix: "/api/purchases/", PathSuffix: "/revoke", Method: "POST", Handler: r.revokePurchaseHandler, Auth: AuthUser}, + // Settle a failed plan execution whose outcome is unknown (issue #999), + // so it stops blocking the delete of its editor-managed plan. The + // handler enforces execute:purchases, account access and constraints. + {PathPrefix: "/api/purchases/", PathSuffix: "/settle", Method: "POST", Handler: r.settleExecutionHandler, Auth: AuthUser}, // GET /api/purchases/{id}/revoke/calculate: returns the Azure refund quote // (amount + currency) for the two-step quote-then-confirm revoke UX // (issue #290 Finding #4). No state mutation; result used to populate @@ -627,6 +631,10 @@ func (r *Router) cancelPurchaseHandler(ctx context.Context, req *events.LambdaFu return r.h.cancelPurchase(ctx, req, params["id"], token) } +func (r *Router) settleExecutionHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.settleExecution(ctx, req, params["id"]) +} + func (r *Router) retryPurchaseHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.retryPurchase(ctx, req, params["id"]) } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 668d1bd27..72c04aa49 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1193,7 +1193,8 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, // // Returns (true, nil) when this call won the race and wrote the linkage, // (false, nil) when zero rows were affected because a concurrent retry -// already claimed the row (or it is no longer 'failed'), and (false, err) +// already claimed the row (or it is no longer 'failed', or an operator +// settled it: SettleFailedExecution, issue #999), and (false, err) // on a real DB error. Must be called inside the same tx that inserts the // successor row, after that insert, so the FK on retry_execution_id is // satisfied. @@ -1205,6 +1206,7 @@ func (s *PostgresStore) LinkRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, WHERE execution_id = $1 AND status = 'failed' AND retry_execution_id IS NULL + AND outcome_settled_at IS NULL ` tag, err := tx.Exec(ctx, q, executionID, retryExecutionID) if err != nil { diff --git a/internal/config/store_postgres_execution_settle.go b/internal/config/store_postgres_execution_settle.go new file mode 100644 index 000000000..e22a907c3 --- /dev/null +++ b/internal/config/store_postgres_execution_settle.go @@ -0,0 +1,233 @@ +package config + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/jackc/pgx/v5" +) + +// Outcomes of an operator settle that the store distinguishes. The handler +// maps them to 404, 400 and 409. +var ( + ErrExecutionSettleNotFound = errors.New("execution not found") + ErrExecutionSettleInvalid = errors.New("invalid execution settle request") + ErrExecutionSettleConflict = errors.New("execution cannot be settled") +) + +// executionSettleReasonMax bounds the stored record; the column has no CHECK. +const executionSettleReasonMax = 1000 + +// ExecutionSettleRequest asks to settle a failed plan execution whose outcome +// is unknown. Actor is the acting principal's id. Acknowledge must be true: +// the operator states they checked the cloud account and accept that the row +// will never be retried. +type ExecutionSettleRequest struct { + ExecutionID string + Actor string + Reason string + Acknowledge bool +} + +// ExecutionSettlement is the stored settlement. Replayed is true when an +// identical earlier request had already settled the row and nothing was +// written. +type ExecutionSettlement struct { + ExecutionID string + Status string + SettledAt time.Time + SettledBy string + Settlement string + Replayed bool +} + +// settleTarget is the locked row SettleFailedExecution judges. +type settleTarget struct { + status string + noSubmission bool + retryID *string + planID *string + settledAt *time.Time + settledBy *string + settlement *string +} + +// SettleFailedExecution records an operator's audited settlement of a failed +// plan execution that has no no_submission proof, so it stops blocking the +// delete of its editor-managed plan (requireManagedDeleteTx). It never changes +// the status and never makes the row count as unbought: the ramp ledger and +// the retention trigger keep treating it as possibly bought. A settled row can +// no longer be retried (LinkRetryExecutionAtomic), because a retry of a row +// whose plan was deleted would buy with no plan. +// +// Locks, in the order the editor command and DeletePurchasePlan use: the plan +// row FOR SHARE, the execution FOR UPDATE, then its retry-chain tail FOR +// UPDATE. It refuses while the chain may still buy (a tail that is not +// terminal, a failed tail that can still be retried, or a chain longer than +// LadderChainMaxDepth). +func (s *PostgresStore) SettleFailedExecution(ctx context.Context, req ExecutionSettleRequest) (*ExecutionSettlement, error) { + if err := validateSettleRequest(req); err != nil { + return nil, err + } + var res *ExecutionSettlement + err := s.WithTx(ctx, func(tx pgx.Tx) error { + target, err := lockSettleTarget(ctx, tx, req.ExecutionID) + if err != nil { + return err + } + if target.settledAt != nil { + res, err = replaySettlement(req, target) + return err + } + if checkErr := checkSettleTarget(target); checkErr != nil { + return checkErr + } + tail, err := lockSettleChainTail(ctx, tx, req.ExecutionID, target.retryID) + if err != nil { + return err + } + record := settlementRecord(req, tail) + res = &ExecutionSettlement{ExecutionID: req.ExecutionID, Status: target.status, SettledBy: req.Actor, Settlement: record} + return tx.QueryRow(ctx, `UPDATE purchase_executions + SET outcome_settled_at = NOW(), outcome_settled_by = $2, outcome_settlement = $3 + WHERE execution_id = $1 AND outcome_settled_at IS NULL + RETURNING outcome_settled_at`, req.ExecutionID, req.Actor, record).Scan(&res.SettledAt) + }) + if err != nil { + return nil, err + } + return res, nil +} + +func validateSettleRequest(req ExecutionSettleRequest) error { + switch { + case !req.Acknowledge: + return fmt.Errorf("%w: the operator must acknowledge the unknown outcome", ErrExecutionSettleInvalid) + case req.Actor == "": + return fmt.Errorf("%w: settling needs an actor", ErrExecutionSettleInvalid) + case req.Reason == "" || len(req.Reason) > executionSettleReasonMax: + return fmt.Errorf("%w: reason must be 1 to %d bytes", ErrExecutionSettleInvalid, executionSettleReasonMax) + } + return nil +} + +// lockSettleTarget takes the plan lock, then the execution lock, and re-reads +// the row under both. A row whose plan changed in between (deleted, so +// plan_id went NULL) is refused rather than judged against the wrong plan. +func lockSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*settleTarget, error) { + var planID *string + err := tx.QueryRow(ctx, `SELECT plan_id::text FROM purchase_executions WHERE execution_id = $1`, executionID).Scan(&planID) + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrExecutionSettleNotFound + } + if err != nil { + return nil, fmt.Errorf("read execution %s: %w", executionID, err) + } + if planID == nil { + return nil, fmt.Errorf("%w: only an execution of a purchase plan can be settled", ErrExecutionSettleConflict) + } + if _, lockErr := tx.Exec(ctx, `SELECT 1 FROM purchase_plans WHERE id = $1 FOR SHARE`, *planID); lockErr != nil { + return nil, fmt.Errorf("lock plan %s: %w", *planID, lockErr) + } + var t settleTarget + err = tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, plan_id::text, + outcome_settled_at, outcome_settled_by, outcome_settlement + FROM purchase_executions WHERE execution_id = $1 FOR UPDATE`, executionID).Scan( + &t.status, &t.noSubmission, &t.retryID, &t.planID, &t.settledAt, &t.settledBy, &t.settlement) + if err != nil { + return nil, fmt.Errorf("lock execution %s: %w", executionID, err) + } + if t.planID == nil || *t.planID != *planID { + return nil, fmt.Errorf("%w: the execution's plan changed while settling", ErrExecutionSettleConflict) + } + return &t, nil +} + +// checkSettleTarget allows only the rows that block a managed delete for an +// unknown outcome: failed without a no_submission proof. +func checkSettleTarget(t *settleTarget) error { + if t.status != StatusFailed { + return fmt.Errorf("%w: execution is %s; only a failed execution can be settled", ErrExecutionSettleConflict, t.status) + } + if t.noSubmission { + return fmt.Errorf("%w: execution is proven never submitted and needs no settlement", ErrExecutionSettleConflict) + } + return nil +} + +// settleChainTail is the last execution of the retry chain from the target. +type settleChainTail struct { + id string + status string +} + +// lockSettleChainTail follows retry_execution_id from the target to the tail +// and locks the tail. It refuses while the chain may still buy: a tail outside +// the terminal allowlist, a failed tail that can still be retried (no +// no_submission proof and not settled itself), or a chain cut short by the +// depth cap or changed between the walk and the lock. +func lockSettleChainTail(ctx context.Context, tx pgx.Tx, executionID string, next *string) (settleChainTail, error) { + tail, err := walkSettleChain(ctx, tx, executionID, next) + if err != nil || tail.id == executionID { + return tail, err + } + var retryID *string + var noSubmission, settled bool + err = tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, outcome_settled_at IS NOT NULL + FROM purchase_executions WHERE execution_id = $1 FOR UPDATE`, tail.id).Scan(&tail.status, &noSubmission, &retryID, &settled) + if err != nil { + return tail, fmt.Errorf("lock execution %s: %w", tail.id, err) + } + if retryID != nil { + return tail, fmt.Errorf("%w: the retry chain changed while settling", ErrExecutionSettleConflict) + } + retryable := tail.status == StatusFailed && !noSubmission && !settled + if !ladderResolvableNodeStatus(tail.status) || retryable { + return tail, fmt.Errorf("%w: retry %s is %s and may still buy or be retried; let it finish or settle it first", ErrExecutionSettleConflict, tail.id, tail.status) + } + return tail, nil +} + +// walkSettleChain reads the retry chain from the target, unlocked, and returns +// its last node; the target itself when it was never retried. +func walkSettleChain(ctx context.Context, tx pgx.Tx, executionID string, next *string) (settleChainTail, error) { + tail := settleChainTail{id: executionID, status: StatusFailed} + for depth := 0; next != nil; depth++ { + if depth >= LadderChainMaxDepth { + return tail, fmt.Errorf("%w: retry chain is longer than %d executions", ErrExecutionSettleConflict, LadderChainMaxDepth) + } + node, err := readChainNode(ctx, tx, *next, false) + if err != nil { + return tail, err + } + tail, next = settleChainTail{id: node.ExecutionID, status: node.Status}, node.RetryExecutionID + } + return tail, nil +} + +// settlementRecord is the exact string stored in outcome_settlement. Fields are +// fixed and quoted, so a repeat of the same request compares equal and a +// different one cannot. +func settlementRecord(req ExecutionSettleRequest, tail settleChainTail) string { + return fmt.Sprintf("operator:settled_unknown_outcome: ack=true chain_tail=%s tail_status=%s reason=%q", + tail.id, tail.status, req.Reason) +} + +// replaySettlement answers a request for a row that is already settled: the +// stored settlement when this actor sent the same reason, else a conflict. +func replaySettlement(req ExecutionSettleRequest, t *settleTarget) (*ExecutionSettlement, error) { + if t.settledBy == nil || t.settlement == nil { + return nil, fmt.Errorf("execution %s has an incomplete settlement", req.ExecutionID) + } + if *t.settledBy != req.Actor || !settlementHasReason(*t.settlement, req.Reason) { + return nil, fmt.Errorf("%w: already settled by %s at %s", ErrExecutionSettleConflict, *t.settledBy, t.settledAt.UTC().Format(time.RFC3339)) + } + return &ExecutionSettlement{ExecutionID: req.ExecutionID, Status: t.status, SettledAt: *t.settledAt, SettledBy: *t.settledBy, Settlement: *t.settlement, Replayed: true}, nil +} + +func settlementHasReason(record, reason string) bool { + suffix := fmt.Sprintf(" reason=%q", reason) + return len(record) >= len(suffix) && record[len(record)-len(suffix):] == suffix +} diff --git a/internal/config/store_postgres_execution_settle_integration_test.go b/internal/config/store_postgres_execution_settle_integration_test.go new file mode 100644 index 000000000..67be9d2ad --- /dev/null +++ b/internal/config/store_postgres_execution_settle_integration_test.go @@ -0,0 +1,217 @@ +//go:build integration +// +build integration + +package config + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/require" +) + +const settleActor = "00000000-0000-0000-0000-0000000000a9" + +// managedSettleFixture is a managed 4-step ramp plan with one account. +func managedSettleFixture(ctx context.Context, t *testing.T) *rampFixture { + t.Helper() + f := newRampFixture(ctx, t, 1, 1, 4) + _, err := f.store.db.Exec(ctx, `UPDATE purchase_plans SET ramp_schedule = ramp_schedule || '{"dates_managed":true}' WHERE id = $1`, f.planID) + require.NoError(t, err) + return f +} + +func (f *rampFixture) settle(ctx context.Context, execID, reason string) (*ExecutionSettlement, error) { + return f.store.SettleFailedExecution(ctx, ExecutionSettleRequest{ExecutionID: execID, Actor: settleActor, Reason: reason, Acknowledge: true}) +} + +func (f *rampFixture) settled(ctx context.Context, t *testing.T, execID string) bool { + t.Helper() + var settled bool + require.NoError(t, f.store.db.QueryRow(ctx, `SELECT outcome_settled_at IS NOT NULL FROM purchase_executions WHERE execution_id = $1`, execID).Scan(&settled)) + return settled +} + +// A failed row with an unknown outcome blocks the managed delete until an +// operator settles it; then the delete detaches it without changing it, and the +// ledger still counts it as possibly bought (issue #999). +func TestSettleFailedExecutionLetsManagedDeleteProceed(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + rec := baselineRec(f.accounts[0], "m5.large", 4) + failed := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, false, rec) + retry := f.retryInOneTx(ctx, t, failed, uuid.NewString(), StatusCompleted) + retry.Recommendations = []RecommendationRecord{rec} + require.NoError(t, f.store.SavePurchaseExecution(ctx, retry)) + ledger := f.committed(ctx, t) + + require.ErrorIs(t, f.store.DeletePurchasePlan(ctx, f.planID), ErrManagedPlanLiveExecutions) + + res, err := f.settle(ctx, failed.ExecutionID, "checked the account: retry bought it") + require.NoError(t, err) + require.False(t, res.Replayed) + require.Equal(t, settleActor, res.SettledBy) + require.Contains(t, res.Settlement, "chain_tail="+retry.ExecutionID+" tail_status=completed") + require.Equal(t, ledger, f.committed(ctx, t), "settling must not change what the ledger counts") + + require.NoError(t, f.store.DeletePurchasePlan(ctx, f.planID)) + got, err := f.store.GetExecutionByID(ctx, failed.ExecutionID) + require.NoError(t, err) + require.Equal(t, StatusFailed, got.Status, "the settled row keeps its failed status") + require.False(t, got.NoSubmission, "settling is not a no_submission proof") + require.Empty(t, got.PlanID) + require.True(t, f.settled(ctx, t, failed.ExecutionID), "the audit record survives the delete") +} + +func TestSettleFailedExecutionRefusals(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + acct := &f.accounts[0] + conflict := map[string]string{ + "completed": f.row(ctx, t, 1, acct, StatusCompleted).ExecutionID, + "pending": f.row(ctx, t, 1, acct, "pending").ExecutionID, + "canceled": f.row(ctx, t, 1, acct, StatusCanceled).ExecutionID, + "failed no_submission": f.rowWithRecs(ctx, t, 1, acct, StatusFailed, true).ExecutionID, + } + for _, tail := range []string{"pending", "running", StatusFailed} { + head := f.row(ctx, t, 2, acct, StatusFailed) + f.retryInOneTx(ctx, t, head, uuid.NewString(), tail) + conflict["retry tail "+tail] = head.ExecutionID + } + adHoc := &PurchaseExecution{ExecutionID: uuid.NewString(), Status: StatusFailed, StepNumber: 1} + require.NoError(t, f.store.SavePurchaseExecution(ctx, adHoc)) + conflict["no plan"] = adHoc.ExecutionID + + for name, id := range conflict { + _, err := f.settle(ctx, id, "operator checked the account") + require.ErrorIs(t, err, ErrExecutionSettleConflict, name) + require.False(t, f.settled(ctx, t, id), name) + } + _, err := f.settle(ctx, uuid.NewString(), "operator checked the account") + require.ErrorIs(t, err, ErrExecutionSettleNotFound) + + failed := f.row(ctx, t, 3, acct, StatusFailed) + for name, req := range map[string]ExecutionSettleRequest{ + "no ack": {ExecutionID: failed.ExecutionID, Actor: settleActor, Reason: "checked"}, + "no actor": {ExecutionID: failed.ExecutionID, Reason: "checked", Acknowledge: true}, + "no reason": {ExecutionID: failed.ExecutionID, Actor: settleActor, Acknowledge: true}, + } { + _, err := f.store.SettleFailedExecution(ctx, req) + require.ErrorIs(t, err, ErrExecutionSettleInvalid, name) + } + require.False(t, f.settled(ctx, t, failed.ExecutionID)) +} + +// A failed tail blocks its predecessor's settle only while it can still be +// retried: once the tail is settled, the head can be too. +func TestSettleFailedExecutionChainOfUnknownOutcomes(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + head := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + tail := f.retryInOneTx(ctx, t, head, uuid.NewString(), StatusFailed) + + _, err := f.settle(ctx, head.ExecutionID, "operator checked the account") + require.ErrorIs(t, err, ErrExecutionSettleConflict) + _, err = f.settle(ctx, tail.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.ErrorIs(t, f.store.DeletePurchasePlan(ctx, f.planID), ErrManagedPlanLiveExecutions, "the unsettled head still blocks") + _, err = f.settle(ctx, head.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.NoError(t, f.store.DeletePurchasePlan(ctx, f.planID)) +} + +func TestSettleFailedExecutionReplayAndRetry(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + first, err := f.settle(ctx, failed.ExecutionID, "operator checked the account") + require.NoError(t, err) + + again, err := f.settle(ctx, failed.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.True(t, again.Replayed) + require.True(t, first.SettledAt.Equal(again.SettledAt), "a replay writes nothing") + _, err = f.settle(ctx, failed.ExecutionID, "a different reason entirely") + require.ErrorIs(t, err, ErrExecutionSettleConflict) + + successor := &PurchaseExecution{ExecutionID: uuid.NewString(), PlanID: f.planID, Status: "pending", StepNumber: 1} + require.NoError(t, f.store.WithTx(ctx, func(tx pgx.Tx) error { + require.NoError(t, f.store.SavePurchaseExecutionTx(ctx, tx, successor)) + linked, linkErr := f.store.LinkRetryExecutionAtomic(ctx, tx, failed.ExecutionID, successor.ExecutionID) + require.NoError(t, linkErr) + require.False(t, linked, "a settled row must not be retried") + return nil + })) +} + +// A retry that links the row first wins: the settle waits on the row lock, +// then sees the live successor and refuses. +func TestSettleWaitsForRetryAndRefusesItsLiveSuccessor(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + retryTx, err := f.store.db.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = retryTx.Rollback(ctx) }) + successor := &PurchaseExecution{ExecutionID: uuid.NewString(), PlanID: f.planID, Status: "pending", StepNumber: 1} + require.NoError(t, f.store.SavePurchaseExecutionTx(ctx, retryTx, successor)) + linked, err := f.store.LinkRetryExecutionAtomic(ctx, retryTx, failed.ExecutionID, successor.ExecutionID) + require.NoError(t, err) + require.True(t, linked) + + done := make(chan error, 1) + go func() { + _, settleErr := f.settle(ctx, failed.ExecutionID, "operator checked the account") + done <- settleErr + }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "%FOR UPDATE%") + require.NoError(t, retryTx.Commit(ctx)) + require.ErrorIs(t, testhelpers.Await(t, done), ErrExecutionSettleConflict) + require.False(t, f.settled(ctx, t, failed.ExecutionID)) +} + +// A delete that holds the plan lock first wins: the settle waits, then finds +// the row detached and refuses instead of settling it against no plan. +func TestSettleWaitsForDeleteAndRefusesDetachedRow(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 1, 4) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + del := testhelpers.HoldCommandFootprint(ctx, t, f.store.db, f.planID) + + done := make(chan error, 1) + go func() { + _, settleErr := f.settle(ctx, failed.ExecutionID, "operator checked the account") + done <- settleErr + }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "%FOR SHARE%") + _, err := del.Exec(ctx, `DELETE FROM purchase_plans WHERE id = $1`, f.planID) + require.NoError(t, err) + require.NoError(t, del.Commit(ctx)) + require.ErrorIs(t, testhelpers.Await(t, done), ErrExecutionSettleConflict) + require.False(t, f.settled(ctx, t, failed.ExecutionID)) +} + +// A delete that queues behind a settle sees the committed settlement. +func TestDeleteWaitsForSettleAndSeesIt(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + settleTx, err := f.store.db.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = settleTx.Rollback(ctx) }) + // The settle's own lock footprint and write, paused before commit. + _, err = settleTx.Exec(ctx, `SELECT 1 FROM purchase_plans WHERE id = $1 FOR SHARE`, f.planID) + require.NoError(t, err) + _, err = settleTx.Exec(ctx, `UPDATE purchase_executions SET outcome_settled_at = now(), + outcome_settled_by = $2, outcome_settlement = 'operator:settled_unknown_outcome' WHERE execution_id = $1`, failed.ExecutionID, settleActor) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- f.store.DeletePurchasePlan(ctx, f.planID) }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "%FOR UPDATE%") + require.NoError(t, settleTx.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) +} diff --git a/internal/config/store_postgres_execution_settle_test.go b/internal/config/store_postgres_execution_settle_test.go new file mode 100644 index 000000000..9018e7fc6 --- /dev/null +++ b/internal/config/store_postgres_execution_settle_test.go @@ -0,0 +1,40 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestBlocksManagedDelete(t *testing.T) { + cases := []struct { + status string + noSubmission bool + settled bool + blocks bool + }{ + {StatusFailed, false, false, true}, + {StatusFailed, false, true, false}, + {StatusFailed, true, false, false}, + {"pending", false, true, true}, + {"running", false, true, true}, + {StatusCompleted, false, false, false}, + {StatusPartiallyCompleted, false, false, false}, + {StatusCanceled, false, false, false}, + {StatusExpired, false, false, false}, + {"some_future_status", false, true, true}, + } + for _, tc := range cases { + e := &PurchaseExecution{Status: tc.status, NoSubmission: tc.noSubmission} + require.Equal(t, tc.blocks, blocksManagedDelete(e, tc.settled), "%+v", tc) + } +} + +func TestSettlementRecordReplayMatchesOnlyTheSameReason(t *testing.T) { + req := ExecutionSettleRequest{Reason: `checked "eu" account`} + record := settlementRecord(req, settleChainTail{id: "tail", status: StatusCompleted}) + require.Equal(t, `operator:settled_unknown_outcome: ack=true chain_tail=tail tail_status=completed reason="checked \"eu\" account"`, record) + require.True(t, settlementHasReason(record, req.Reason)) + require.False(t, settlementHasReason(record, "account")) + require.False(t, settlementHasReason(record, `checked "eu" account2`)) +} diff --git a/internal/config/store_postgres_managed_guard.go b/internal/config/store_postgres_managed_guard.go index fc4b0064b..bcb734307 100644 --- a/internal/config/store_postgres_managed_guard.go +++ b/internal/config/store_postgres_managed_guard.go @@ -123,21 +123,37 @@ func sameAccountSet(a, b []string) bool { // hold or buy capacity: the FK would detach it (ON DELETE SET NULL) and it // would then buy with no plan. Judged under the caller's plan lock, so a root // inserted by a command that committed first is seen. Settled history -// (completed, canceled, expired, failed without submission) does not block. +// (completed, canceled, expired, failed without submission) does not block, +// and neither does a failed row with an unknown outcome that an operator +// settled (SettleFailedExecution): it can no longer be retried and keeps its +// failed status, so detaching it hides nothing. func requireManagedDeleteTx(ctx context.Context, tx pgx.Tx, planID string) error { - rows, err := tx.Query(ctx, `SELECT status, no_submission FROM purchase_executions WHERE plan_id = $1`, planID) + rows, err := tx.Query(ctx, `SELECT status, no_submission, outcome_settled_at IS NOT NULL FROM purchase_executions WHERE plan_id = $1`, planID) if err != nil { return fmt.Errorf("failed to read plan executions: %w", err) } defer rows.Close() for rows.Next() { var e PurchaseExecution - if err := rows.Scan(&e.Status, &e.NoSubmission); err != nil { + var settled bool + if err := rows.Scan(&e.Status, &e.NoSubmission, &settled); err != nil { return fmt.Errorf("failed to scan plan execution: %w", err) } - if holdsCapacity(&e) && e.Status != StatusCompleted && e.Status != StatusPartiallyCompleted { + if blocksManagedDelete(&e, settled) { return fmt.Errorf("%w: plan %s", ErrManagedPlanLiveExecutions, planID) } } return rows.Err() } + +// blocksManagedDelete reports whether a row of a managed plan may still hold +// or buy capacity that no operator has accounted for. +func blocksManagedDelete(e *PurchaseExecution, settled bool) bool { + switch { + case !holdsCapacity(e), e.Status == StatusCompleted, e.Status == StatusPartiallyCompleted: + return false + case e.Status == StatusFailed: + return !settled + } + return true +} diff --git a/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.down.sql b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.down.sql new file mode 100644 index 000000000..46b11cc0e --- /dev/null +++ b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.down.sql @@ -0,0 +1,8 @@ +-- Reverse 000143 (issue #999). Lossy: operator settlements are dropped, so a +-- settled failed row blocks its managed plan's delete again. +ALTER TABLE purchase_executions + DROP CONSTRAINT IF EXISTS purchase_executions_outcome_settlement_complete; +ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS outcome_settlement, + DROP COLUMN IF EXISTS outcome_settled_by, + DROP COLUMN IF EXISTS outcome_settled_at; diff --git a/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.up.sql b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.up.sql new file mode 100644 index 000000000..55894f5d4 --- /dev/null +++ b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.up.sql @@ -0,0 +1,21 @@ +-- Migration 000143 (issue #999): an operator's audited settlement of a failed +-- execution whose outcome is unknown (failed without no_submission proof). +-- +-- Settling never changes status and never makes the row count as unbought: +-- the ramp ledger (holdsCapacity, ramp_ledger_holds) and the retention trigger +-- still treat it as possibly bought. It only lets a managed plan's delete +-- detach the row, and it stops the row from being retried (a retry of a row +-- detached from its plan would buy with no plan). +-- +-- outcome_settled_by is the acting principal as text (a users.id, or the admin +-- API key sentinel), so the audit survives the user being deleted. +-- outcome_settlement is the complete, fixed-format record the store wrote. +ALTER TABLE purchase_executions + ADD COLUMN IF NOT EXISTS outcome_settled_at TIMESTAMPTZ NULL, + ADD COLUMN IF NOT EXISTS outcome_settled_by TEXT NULL, + ADD COLUMN IF NOT EXISTS outcome_settlement TEXT NULL; + +ALTER TABLE purchase_executions + ADD CONSTRAINT purchase_executions_outcome_settlement_complete + CHECK ((outcome_settled_at IS NULL) = (outcome_settled_by IS NULL) + AND (outcome_settled_at IS NULL) = (outcome_settlement IS NULL)); diff --git a/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement_test.go b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement_test.go new file mode 100644 index 000000000..5d78393a9 --- /dev/null +++ b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement_test.go @@ -0,0 +1,68 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000143 up, down and +// the all-or-nothing CHECK (#999). +func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + const version = 143 + prev := previousMigrationVersion(t, version) + + columns := func() int { + var n int + require.NoError(t, pool.QueryRow(ctx, ` + SELECT count(*) FROM information_schema.columns + WHERE table_name = 'purchase_executions' + AND column_name IN ('outcome_settled_at', 'outcome_settled_by', 'outcome_settlement')`).Scan(&n)) + return n + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + require.Zero(t, columns(), "before 000143 the columns must not exist") + _, err := pool.Exec(ctx, ` + INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations) + VALUES ('00000000-0000-0000-0000-0000000009a1', 'failed', 1, now(), '[]')`) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + require.Equal(t, 3, columns()) + var settled bool + require.NoError(t, pool.QueryRow(ctx, `SELECT outcome_settled_at IS NOT NULL FROM purchase_executions + WHERE execution_id = '00000000-0000-0000-0000-0000000009a1'`).Scan(&settled)) + require.False(t, settled, "existing rows start unsettled") + + _, err = pool.Exec(ctx, `UPDATE purchase_executions + SET outcome_settled_at = now(), outcome_settled_by = 'admin-api-key', outcome_settlement = 'operator:settled' + WHERE execution_id = '00000000-0000-0000-0000-0000000009a1'`) + require.NoError(t, err, "a complete settlement is valid") + for name, set := range map[string]string{ + "time only": `outcome_settled_at = now()`, + "no record": `outcome_settled_at = now(), outcome_settled_by = 'u'`, + "no actor": `outcome_settled_at = now(), outcome_settlement = 's'`, + "record only": `outcome_settlement = 's'`, + } { + _, err = pool.Exec(ctx, `INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations) + VALUES (gen_random_uuid(), 'failed', 1, now(), '[]')`) + require.NoError(t, err) + _, err = pool.Exec(ctx, `UPDATE purchase_executions SET `+set+` WHERE outcome_settled_at IS NULL AND outcome_settlement IS NULL`) + require.Error(t, err, "CHECK must reject a partial settlement (%s)", name) + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + require.Zero(t, columns(), "down must drop the columns") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + require.Equal(t, 3, columns(), "up must re-apply cleanly after down") +} diff --git a/known_issues/35_legacy_guard_followups.md b/known_issues/resolved/35_legacy_guard_followups.md similarity index 73% rename from known_issues/35_legacy_guard_followups.md rename to known_issues/resolved/35_legacy_guard_followups.md index 606912ee7..8bf2a872d 100644 --- a/known_issues/35_legacy_guard_followups.md +++ b/known_issues/resolved/35_legacy_guard_followups.md @@ -1,6 +1,6 @@ # Known Issues: follow-ups from the legacy-writer guards (#631) -> **Audit status (2026-10-20):** `1 needs triage · 1 resolved` +> **Audit status (2026-10-23):** `0 needs triage · 2 resolved` ## ~~LOW: cleanup lambda deletes by expires_at with no status predicate~~ — RESOLVED @@ -16,7 +16,7 @@ **Resolved by:** `deleteExpired` and `dryRunCount` now share `expiredExecutionsWhere`: a lapsed `expires_at` deletes a row only when its status is in `TerminalExecutionStatuses` and not in `HealthScoredExecutionStatuses`, so live rows survive and failed or canceled rows stay with `CleanupOldExecutions`. Covered by the real-Postgres tests in `cmd/cleanup-lambda/main_integration_test.go`, including a writer that turns a settled row live while the DELETE waits for its lock. -## MEDIUM: a failed-with-submission row blocks deleting a managed plan forever +## ~~MEDIUM: a failed-with-submission row blocks deleting a managed plan forever~~ — RESOLVED **Files:** `internal/config/store_postgres_managed_guard.go` (`requireManagedDeleteTx`), `internal/config/store_postgres_ramp_baseline.go` (`holdsCapacity`) @@ -26,4 +26,6 @@ **Issue:** #999 (follow-up of #631) -**Status:** Needs triage. +**Status:** ✔️ Resolved + +**Resolved by:** an explicit, audited operator settle, `POST /api/purchases/{id}/settle` (`SettleFailedExecution`, migration 000143). It needs `execute:purchases`, account access and the grant's constraints, a reason and `acknowledge_unknown_outcome: true`, and refuses while the row's retry chain may still buy. The row stays `failed` and keeps counting as possibly bought in the ramp ledger; once settled it can no longer be retried and no longer blocks `requireManagedDeleteTx`. Delete itself gained no flag.