From d7edc596721857fb3a8e70e9a2401a94e3d8c23e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:03:30 -0700 Subject: [PATCH 1/3] fix(api): move expireIfStale async; exempt empty-account rows from scoped drop (closes #1032) Two coupled defects on the getHistory GET read path: 1. expireIfStale called TransitionExecutionStatus inline in fetchExecutionsAsHistory, making the GET a racy write: concurrent loads both attempt the transition and the loser's error is swallowed. Fix: collect stale execution IDs during the read phase; fire TransitionExecutionStatus in a best-effort background goroutine (via expireStaleExecutionsAsync) after the response is assembled. The response reflects current DB state; the next History load sees the updated status. The transition remains idempotent via the FROM-status guard in TransitionExecutionStatus. 2. executionToHistoryRow sets AccountID = "" when exec.CloudAccountID is nil and recs carry no common account (ambient/multi-account path). filterPurchaseHistoryByAllowedAccounts then calls MatchesAccount(allowed, "", "") == false for any scoped user, silently dropping in-flight financial actions the user owns -- a regression of #621's silent-disappearance fix. Fix: pass rows with empty AccountID through the filter unconditionally for scoped users; these carry no account-specific data that would violate cross-tenant isolation, and the view:purchases gate already fired. Regression tests added that fail against the pre-fix code: - TestHandler_getHistory_GetIsReadOnly: asserts the handler returns before TransitionExecutionStatus completes (gate/channel pattern). - TestHandler_getHistory_ScopedUserSeesEmptyAccountRows: asserts a scoped user sees ambient-credential executions (AccountID == ""). Updated TestHandler_getHistory_ExpireIfStale to reflect the new async behaviour: response shows pre-transition status; goroutine fires in the background, verified via done channel (no sleep). --- internal/api/handler_history.go | 113 ++++++++--- internal/api/handler_history_test.go | 283 +++++++++++++++++++++++---- 2 files changed, 327 insertions(+), 69 deletions(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index e33fbac62..754e25caf 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -53,7 +53,13 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR // here (in-memory against the synthesised row's recommendations and // scheduled_date) so the two halves of the merged response are // consistently scoped (issue #701). - extra := h.fetchExecutionsAsHistory(ctx, filters) + // + // Stale pending/notified executions are expired asynchronously AFTER the + // response is assembled so the GET handler is a pure read (issue #1032): + // the caller sees current DB state; the transitions fire in a goroutine + // and the next History load reflects the updated status. + extra, staleExecs := h.fetchExecutionsAsHistory(ctx, filters) + h.expireStaleExecutionsAsync(staleExecs) all := append(completed, extra...) //nolint:gocritic // intentional new slice @@ -116,28 +122,28 @@ const approvalExpiryWindow = 7 * 24 * time.Hour // to "multiple" because a single execution can span providers. The approver // address is looked up from global config once and attached to pending/ // notified rows so the UI can tell the user exactly whose inbox holds the -// approval link. Pending executions older than approvalExpiryWindow are -// lazily transitioned to "expired" in the store so a stale approval link -// can't be clicked and the History badge reflects reality. A listing error -// is logged and skipped — completed history must still render. +// approval link. Stale pending/notified executions are expired asynchronously +// after this call returns (see expireStaleExecutionsAsync) so the GET is a +// pure read: the response rows carry the pre-transition status and the +// next request sees the updated state. A listing error is logged and +// skipped — completed history must still render. // // The filter set (issue #701) is applied in Go against the synthetic row: // provider via the recs' collapsed provider, account via CloudAccountID, -// date via ScheduledDate. Filtering after expireIfStale so a row that -// transitioned to expired during this request still gets evaluated against -// the same predicates as a DB row. -func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyFilters) []config.PurchaseHistoryRecord { +// date via ScheduledDate. +func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyFilters) ([]config.PurchaseHistoryRecord, []config.PurchaseExecution) { executions, err := h.config.GetExecutionsByStatuses(ctx, historyExecutionStatuses, config.DefaultListLimit) if err != nil { logging.Warnf("history: failed to load non-completed executions: %v", err) - return nil + return nil, nil } if len(executions) == 0 { - return nil + return nil, nil } approver := h.resolvePendingApproverEmail(ctx) userEmailCache := h.resolveUserEmails(ctx, executions) out := make([]config.PurchaseHistoryRecord, 0, len(executions)) + var staleExecs []config.PurchaseExecution for _, exec := range executions { // Dedup: a normal completed execution is already represented by its // purchase_history rows. Skip it here so it shows exactly once. Only @@ -147,7 +153,13 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF if exec.Status == "completed" && exec.Error == "" { continue } - exec = h.expireIfStale(ctx, exec) + // Collect stale pending/notified executions for the post-response + // async expire sweep. We do NOT mutate status here to keep the GET + // read-only: the response reflects current DB state; the background + // goroutine fires the transition so the next request sees "expired". + if isStaleExecution(exec) { + staleExecs = append(staleExecs, exec) + } if !filters.matchesExecution(exec) { continue } @@ -157,7 +169,46 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF } out = append(out, executionToHistoryRow(exec, approver, createdByEmail)) } - return out + return out, staleExecs +} + +// isStaleExecution reports whether the execution is a pending/notified +// approval older than approvalExpiryWindow that should be transitioned to +// "expired". Extracted so both fetchExecutionsAsHistory and the async sweep +// share one staleness check. +func isStaleExecution(exec config.PurchaseExecution) bool { + if exec.Status != "pending" && exec.Status != "notified" { + return false + } + return time.Since(exec.ScheduledDate) >= approvalExpiryWindow +} + +// expireStaleExecutionsAsync fires TransitionExecutionStatus for each stale +// execution in a best-effort goroutine that outlives the request context. +// Using context.Background() ensures the transitions are not cancelled when +// the HTTP handler returns. Errors are logged and skipped — a missed +// transition leaves the row "pending" until the next History load, which is +// better than blocking the read response. +// +// The goroutine is idempotent per execution ID: TransitionExecutionStatus is +// guarded by the FROM-status list ("pending","notified"), so a concurrent +// caller that wins the race causes the loser's update to affect 0 rows and +// return an error, which is already handled by the Warnf below. Two +// simultaneous GET requests can both spawn a goroutine for the same stale +// row; only one transition commits — this is safe and expected. +func (h *Handler) expireStaleExecutionsAsync(staleExecs []config.PurchaseExecution) { + if len(staleExecs) == 0 { + return + } + go func() { + ctx := context.Background() + for _, exec := range staleExecs { + _, err := h.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "expired") + if err != nil { + logging.Warnf("history: async expire of execution %s failed: %v", exec.ExecutionID, err) + } + } + }() } // resolveUserEmails builds a map of user-ID to email by calling GetUser once @@ -185,25 +236,6 @@ func (h *Handler) resolveUserEmails(ctx context.Context, executions []config.Pur return out } -// expireIfStale transitions a pending/notified execution to "expired" when -// its ScheduledDate is older than approvalExpiryWindow. Returns the possibly- -// updated execution. Transition failures are non-fatal — the row still -// renders, just with its original status. -func (h *Handler) expireIfStale(ctx context.Context, exec config.PurchaseExecution) config.PurchaseExecution { - if exec.Status != "pending" && exec.Status != "notified" { - return exec - } - if time.Since(exec.ScheduledDate) < approvalExpiryWindow { - return exec - } - updated, err := h.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified"}, "expired") - if err != nil { - logging.Warnf("history: failed to expire execution %s: %v", exec.ExecutionID, err) - return exec - } - return *updated -} - // resolvePendingApproverEmail returns the notification email the approval // link was sent to (or would have been, if SES failed). Single-tenant // deployments share one value across every pending row, so this is looked up @@ -875,6 +907,16 @@ func appendMissing(dst []string, vals ...string) []string { // filterPurchaseHistoryByAllowedAccounts drops records whose AccountID/Name // is outside the session's allowed_accounts. Admin/unrestricted sessions pass // through unchanged. +// +// Rows with an empty AccountID are exempt from the drop for scoped users +// (issue #1032, regression of #621). An empty AccountID means the execution +// was ambient (exec.CloudAccountID == nil) AND its recommendations carry no +// common account. These are in-flight financial actions the user owns that +// cannot be attributed to a specific cloud account — dropping them silently +// re-introduces the #621 disappearance bug for non-admin users. Passing them +// through is safe: the session's allowed_accounts gate already ensures the +// user has view:purchases permission, and unattributed rows carry no +// account-specific data that would violate cross-tenant isolation. func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, session *Session, purchases []config.PurchaseHistoryRecord) ([]config.PurchaseHistoryRecord, error) { allowed, err := h.getAllowedAccounts(ctx, session) if err != nil { @@ -886,6 +928,13 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se nameByID := h.resolveAccountNamesByID(ctx) filtered := purchases[:0] for _, p := range purchases { + // Empty AccountID: unattributed ambient/multi-account synthesized row. + // Pass through so scoped users see in-flight financial actions that + // cannot be pinned to a single account (issue #1032 / #621 regression). + if p.AccountID == "" { + filtered = append(filtered, p) + continue + } if auth.MatchesAccount(allowed, p.AccountID, nameByID[p.AccountID]) { filtered = append(filtered, p) } diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index e5872a43c..effddf86f 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -174,17 +174,18 @@ func TestHandler_getHistory_CustomLimit(t *testing.T) { require.NoError(t, err) } -// TestHandler_getHistory_ExpireIfStale exercises the lazy approval-window -// transition in expireIfStale. The History render seeds two non-completed -// executions: one fresh (left untouched) and one with ScheduledDate older than -// the 7-day approvalExpiryWindow. The stale row must be transitioned to -// "expired" via a single TransitionExecutionStatus call against its ID, and -// the rendered row must reflect the new status + the canonical -// "approval link expired …" StatusDescription. The fresh row stays pending -// and proves expireIfStale's age guard short-circuits before touching the -// store for non-stale rows. A second sub-test covers the error fallback: -// when TransitionExecutionStatus returns an error the original pending row -// must still render (no panic, no row dropped, status stays pending). +// TestHandler_getHistory_ExpireIfStale exercises the async approval-window +// expiry sweep (issue #1032). The History read fires a best-effort background +// goroutine to transition stale pending/notified executions to "expired" so +// the GET handler itself is a pure read. The goroutine runs AFTER the response +// is assembled, so the response shows the pre-transition status ("pending"); +// the next History load sees "expired" from the DB. +// +// Each sub-test wires a done channel into the mock's Run callback to +// synchronize with the async goroutine — no time.Sleep per project memory. +// The fresh-row sub-test asserts TransitionExecutionStatus is never called for +// a non-stale execution (the staleness guard short-circuits before enqueueing +// the ID). // // Closes the gap flagged in known_issues/32_history_lazy_expire_test.md. func TestHandler_getHistory_ExpireIfStale(t *testing.T) { @@ -207,7 +208,7 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { } // Stale: ScheduledDate older than approvalExpiryWindow (7 days) → must - // trigger TransitionExecutionStatus once with the row's ID. + // trigger TransitionExecutionStatus once with the row's ID (asynchronously). staleExec := func(status string) config.PurchaseExecution { return config.PurchaseExecution{ ExecutionID: staleID, @@ -221,18 +222,29 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { } } - t.Run("transitions stale pending row to expired", func(t *testing.T) { + // waitForCall returns a Run option that signals done when the mock is + // called. Use it to synchronize with the background goroutine without + // sleeping. + waitForCall := func(done chan<- struct{}) func(mock.Arguments) { + return func(_ mock.Arguments) { close(done) } + } + + t.Run("async expire fires for stale pending row; response shows pre-transition status", func(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) expired := staleExec("pending") expired.Status = "expired" + done := make(chan struct{}) mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). Return([]config.PurchaseExecution{freshExec(), staleExec("pending")}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) - mockStore.On("TransitionExecutionStatus", ctx, staleID, []string{"pending", "notified"}, "expired"). + // The goroutine uses context.Background(); context.Background() == ctx in + // this test, so the matcher fires correctly. + mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired"). + Run(waitForCall(done)). Return(&expired, nil).Once() mockAuth, req := adminHistoryReq(ctx) @@ -241,9 +253,17 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { result, err := handler.getHistory(ctx, req, map[string]string{}) require.NoError(t, err) - // Single Transition call, only for the stale row. + // Wait for the background goroutine to complete before asserting the + // call count. No sleep: we block on the done channel. + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("TransitionExecutionStatus goroutine did not fire within 5s") + } + + // Exactly one Transition call, only for the stale row. mockStore.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) - mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, staleID, []string{"pending", "notified"}, "expired") + mockStore.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired") historyResp := result.(HistoryResponse) require.Len(t, historyResp.Purchases, 2, "both executions must render as history rows") @@ -260,26 +280,28 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { require.NotNil(t, staleRow, "stale execution must render as a history row") require.NotNil(t, freshRow, "fresh execution must render as a history row") - assert.Equal(t, "expired", staleRow.Status, "stale row must reflect the expired transition") - assert.NotEmpty(t, staleRow.StatusDescription, "expired row must carry an explanatory description") - assert.Contains(t, staleRow.StatusDescription, "approval link expired", - "expired description must use the canonical user-facing wording") - - assert.Equal(t, "pending", freshRow.Status, "fresh row must stay pending — age guard short-circuits the transition") + // The GET is now a pure read: the response reflects the pre-transition + // status ("pending"). The background goroutine fires the transition so + // the NEXT History load sees "expired". This is the post-fix invariant. + assert.Equal(t, "pending", staleRow.Status, + "issue #1032: GET must not mutate state — stale row shows pre-transition status in this response") + assert.Equal(t, "pending", freshRow.Status, "fresh row must stay pending — staleness guard short-circuits enqueue") }) - t.Run("transitions stale notified row to expired", func(t *testing.T) { + t.Run("async expire fires for stale notified row; response shows pre-transition status", func(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) expired := staleExec("notified") expired.Status = "expired" + done := make(chan struct{}) mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). Return([]config.PurchaseExecution{staleExec("notified")}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) - mockStore.On("TransitionExecutionStatus", ctx, staleID, []string{"pending", "notified"}, "expired"). + mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired"). + Run(waitForCall(done)). Return(&expired, nil).Once() mockAuth, req := adminHistoryReq(ctx) @@ -288,47 +310,234 @@ func TestHandler_getHistory_ExpireIfStale(t *testing.T) { result, err := handler.getHistory(ctx, req, map[string]string{}) require.NoError(t, err) + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("TransitionExecutionStatus goroutine did not fire within 5s") + } mockStore.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) historyResp := result.(HistoryResponse) require.Len(t, historyResp.Purchases, 1) - assert.Equal(t, "expired", historyResp.Purchases[0].Status, - "stale notified rows must transition the same as stale pending rows") - assert.Equal(t, 1, historyResp.Summary.TotalExpired, - "expired transition must be reflected in the summary bucket") - assert.Equal(t, 0, historyResp.Summary.TotalPending) + // Response shows pre-transition status; the DB is updated asynchronously. + assert.Equal(t, "notified", historyResp.Purchases[0].Status, + "issue #1032: GET must not mutate — stale notified row shows pre-transition status in this response") + assert.Equal(t, 1, historyResp.Summary.TotalPending, + "pre-transition stale notified row counts as pending in this response") + assert.Equal(t, 0, historyResp.Summary.TotalExpired, + "the expired state is not visible in this response; next load will reflect it") }) - t.Run("transition error falls back to pending row without panic", func(t *testing.T) { + t.Run("async expire error is non-fatal; response and row visibility are unaffected", func(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) + done := make(chan struct{}) mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). Return([]config.PurchaseExecution{staleExec("pending")}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) - mockStore.On("TransitionExecutionStatus", ctx, staleID, []string{"pending", "notified"}, "expired"). + mockStore.On("TransitionExecutionStatus", mock.Anything, staleID, []string{"pending", "notified"}, "expired"). + Run(waitForCall(done)). Return(nil, errors.New("simulated store failure")).Once() mockAuth, req := adminHistoryReq(ctx) handler := &Handler{auth: mockAuth, config: mockStore} - // The whole point: a Transition error is non-fatal. No panic, no - // dropped row, status stays pending so the user still sees the - // approval in the history view. + // A Transition error must be non-fatal. No panic, no dropped row — + // the stale row still renders (with its pre-transition status) so the + // user can see the pending approval even when the background expire fails. result, err := handler.getHistory(ctx, req, map[string]string{}) require.NoError(t, err) + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("TransitionExecutionStatus goroutine did not fire within 5s") + } mockStore.AssertNumberOfCalls(t, "TransitionExecutionStatus", 1) historyResp := result.(HistoryResponse) - require.Len(t, historyResp.Purchases, 1, "fallback must render the original row, not drop it") + require.Len(t, historyResp.Purchases, 1, "transition error must not drop the row") assert.Equal(t, staleID, historyResp.Purchases[0].PurchaseID) assert.Equal(t, "pending", historyResp.Purchases[0].Status, - "transition failure must leave the row in its original status") - assert.Equal(t, 1, historyResp.Summary.TotalPending, "summary must reflect the un-transitioned status") + "row is visible with pre-transition status; the transition failed so next load may retry it") + assert.Equal(t, 1, historyResp.Summary.TotalPending, "stale row counts as pending since transition failed") assert.Equal(t, 0, historyResp.Summary.TotalExpired) }) + + t.Run("fresh row does not trigger TransitionExecutionStatus", func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). + Return([]config.PurchaseExecution{freshExec()}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) + + // No goroutine should have been spawned — assert zero calls directly. + // There is no done channel because the goroutine must not exist. + mockStore.AssertNumberOfCalls(t, "TransitionExecutionStatus", 0) + + historyResp := result.(HistoryResponse) + require.Len(t, historyResp.Purchases, 1) + assert.Equal(t, freshID, historyResp.Purchases[0].PurchaseID) + assert.Equal(t, "pending", historyResp.Purchases[0].Status) + }) +} + +// TestHandler_getHistory_GetIsReadOnly is the issue #1032 defect-1 regression +// guard. The GET /api/history handler must not call TransitionExecutionStatus +// synchronously — it must not block on or return from a write. Pre-fix the +// transition was inlined in fetchExecutionsAsHistory, so the handler wrote DB +// state on every call for any stale pending row, racing concurrent loads. +// +// This test seeds one stale pending execution. It wires the +// TransitionExecutionStatus mock behind a channel gate and asserts that +// getHistory returns BEFORE the goroutine releases the gate — i.e. the +// response is produced without waiting for the write to complete. +// +// Fails pre-fix: the old inline expireIfStale runs synchronously so the +// handler blocks until TransitionExecutionStatus returns; opening the gate +// AFTER getHistory returns would mean the mock was never called in the first +// place (0 calls, but the mock has Once() and the close() never fires, so +// AssertNumberOfCalls panics on the missed expectation). +func TestHandler_getHistory_GetIsReadOnly(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + approverEmail := "ops@example.com" + + stale := config.PurchaseExecution{ + ExecutionID: "stale-ro-exec", + Status: "pending", + ScheduledDate: time.Now().Add(-8 * 24 * time.Hour), + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1"}, + }, + } + + // gate blocks the TransitionExecutionStatus mock until the test releases it. + // Pre-fix: the handler blocks on this gate and returns only after it is + // closed — proving the write was synchronous. Post-fix: the handler returns + // immediately and the goroutine blocks on the gate independently. + gate := make(chan struct{}) + transitionCalled := make(chan struct{}) + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{stale}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + mockStore.On("TransitionExecutionStatus", mock.Anything, "stale-ro-exec", []string{"pending", "notified"}, "expired"). + Run(func(_ mock.Arguments) { + close(transitionCalled) // signal that the goroutine reached the transition + <-gate // block until the test releases it + }). + Return(nil, nil).Maybe() + + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + // getHistory must return without waiting for TransitionExecutionStatus. + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err, "handler must not error even when the background expire has not run yet") + + // At this point the handler has returned. The transition goroutine may or + // may not have started yet — we do NOT assert its call count here. We assert + // the response shape (the read-only view): the row is present with its + // pre-transition status, not dropped. + resp := result.(HistoryResponse) + require.Len(t, resp.Purchases, 1, "stale execution must appear in response (pre-transition status)") + assert.Equal(t, "pending", resp.Purchases[0].Status, + "issue #1032: GET must return current DB state without waiting for the expire write") + + // Now wait for the goroutine to call the transition, then release the gate + // so the test teardown can complete cleanly. + select { + case <-transitionCalled: + case <-time.After(5 * time.Second): + // If we timed out it means the goroutine never fired — that would be a + // bug (the expire must still happen, just not synchronously). + t.Fatal("expire goroutine did not call TransitionExecutionStatus within 5s") + } + close(gate) // unblock the goroutine so it can finish +} + +// TestHandler_getHistory_ScopedUserSeesEmptyAccountRows is the issue #1032 +// defect-2 / #621 regression guard. An execution with CloudAccountID == nil +// and no common per-rec account (accountID == "") must remain visible to a +// scoped (non-admin) user. Pre-fix, filterPurchaseHistoryByAllowedAccounts +// called auth.MatchesAccount(allowed, "", "") == false for any non-empty +// allowed list, silently dropping the row — re-introducing the #621 +// disappearance bug for non-admins. Post-fix the filter passes empty-AccountID +// rows through unconditionally (they carry no account-specific data that could +// violate cross-tenant isolation). +// +// Fails pre-fix: the empty-account execution row is dropped and +// resp.Purchases is empty, causing the require.Len assertion to fail. +func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) { + ctx := context.Background() + + // Ambient execution: CloudAccountID == nil, recommendations also have no + // account — this is the multi-account/ambient-credentials path. + ambientExec := config.PurchaseExecution{ + ExecutionID: "ambient-exec-1", + Status: "pending", + ScheduledDate: time.Now(), + Recommendations: []config.RecommendationRecord{ + // No CloudAccountID on either rec → collapseRecommendationAccount + // returns "" → executionToHistoryRow sets AccountID = "". + {Provider: "aws", Service: "ec2", Region: "us-east-1"}, + {Provider: "aws", Service: "rds", Region: "us-east-1"}, + }, + } + + mockStore := new(MockConfigStore) + approverEmail := "ops@example.com" + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{ambientExec}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) + // resolveAccountNamesByID must not error — return an empty accounts list + // so MatchesAccount falls back to ID-only matching (safe for our assertions). + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return nil, nil + } + + // Scoped user: only allowed to access one specific account UUID. + scopedUser := &Session{ + UserID: "scoped-user-id", + Email: "scoped@example.com", + } + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "scoped-token").Return(scopedUser, nil) + mockAuth.On("HasPermissionAPI", ctx, "scoped-user-id", "view", "purchases").Return(true, nil) + // allowed_accounts is non-empty → user is scoped (not unrestricted). + mockAuth.On("GetAllowedAccountsAPI", ctx, "scoped-user-id").Return([]string{"aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa"}, nil) + + handler := &Handler{ + auth: mockAuth, + config: mockStore, + } + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer scoped-token"}, + } + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) + + resp := result.(HistoryResponse) + require.Len(t, resp.Purchases, 1, + "issue #1032 / #621 regression: a scoped user must see in-flight ambient-account executions; dropping them re-creates the financial-action-vanishes bug") + assert.Equal(t, "ambient-exec-1", resp.Purchases[0].PurchaseID) + assert.Equal(t, "", resp.Purchases[0].AccountID, + "the AccountID is empty (ambient execution) and must remain visible to scoped users") + assert.Equal(t, "pending", resp.Purchases[0].Status) + assert.Equal(t, 1, resp.Summary.TotalPending) } // TestHandler_getHistory_PermissionDenied asserts that a non-admin user without From a74990b4676d013a3faf98ec5c7531dba5493ac5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:17:22 -0700 Subject: [PATCH 2/3] fix(api): use strconv.Atoi for limit; fresh slice in filter/merge (closes #1061) Three code-quality fixes in handler_history.go (FOLD-1043, findings 01-M1, 01-L1, 01-L2 history site): 01-M1: parseHistoryFilters now uses strconv.Atoi for the limit query param and returns a 400 ClientError on parse failure instead of silently ignoring a bad value via fmt.Sscanf. limit < 1 is clamped to DefaultListLimit (previously the <= 0 guard covered negatives but not the zero case explicitly). Aligns with feedback_strict_int_parse. 01-L1: Replace append(completed, extra...) + nolint:gocritic comment with an explicit pre-allocated slice (make + two appends). The nolint comment was accurate but obscure; the explicit allocation is self- documenting and sidesteps the lint concern without a suppression. 01-L2 (history site): filterPurchaseHistoryByAllowedAccounts used purchases[:0] as the filter output, aliasing the input's backing array. Replace with make([]..., 0, len(purchases)) for a true fresh slice. Regression tests added (TestHandler_getHistory_LimitParsing): six table-driven sub-tests covering non-integer, float, negative, zero, over-max, and valid limits. Fails pre-fix on the non-integer and float cases (fmt.Sscanf returns no error and falls back to default silently). --- internal/api/handler_history.go | 15 ++++-- internal/api/handler_history_test.go | 80 ++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 754e25caf..a9734b7d8 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "sort" + "strconv" "time" "github.com/LeanerCloud/CUDly/internal/auth" @@ -61,7 +62,9 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR extra, staleExecs := h.fetchExecutionsAsHistory(ctx, filters) h.expireStaleExecutionsAsync(staleExecs) - all := append(completed, extra...) //nolint:gocritic // intentional new slice + all := make([]config.PurchaseHistoryRecord, 0, len(completed)+len(extra)) + all = append(all, completed...) + all = append(all, extra...) all, err = h.filterPurchaseHistoryByAllowedAccounts(ctx, session, all) if err != nil { @@ -626,9 +629,13 @@ func parseHistoryFilters(params map[string]string) (historyFilters, error) { limit := config.DefaultListLimit if s := params["limit"]; s != "" { - fmt.Sscanf(s, "%d", &limit) + n, err := strconv.Atoi(s) + if err != nil { + return f, NewClientError(400, "invalid limit: must be a positive integer") + } + limit = n } - if limit <= 0 { + if limit < 1 { limit = config.DefaultListLimit } if limit > config.MaxListLimit { @@ -926,7 +933,7 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se return purchases, nil } nameByID := h.resolveAccountNamesByID(ctx) - filtered := purchases[:0] + filtered := make([]config.PurchaseHistoryRecord, 0, len(purchases)) for _, p := range purchases { // Empty AccountID: unattributed ambient/multi-account synthesized row. // Pass through so scoped users see in-flight financial actions that diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index effddf86f..bc5ff4022 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -1687,6 +1687,86 @@ func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) { // QA reproduction scenario from issue #736: start with N approved purchases, // observe KPI totals, then add a cancelled execution and assert the totals // are unchanged. +// TestHandler_getHistory_LimitParsing is the 01-M1 regression guard. +// Prior to the fix, parseHistoryFilters used fmt.Sscanf to parse the limit +// query param, which silently swallows non-integer input (callers get the +// default with no feedback) and never returns a 400. strconv.Atoi now +// rejects non-integer values, and limit < 1 is clamped to default rather +// than being treated as an override (issue #1061). +func TestHandler_getHistory_LimitParsing(t *testing.T) { + cases := []struct { + name string + limitParam string + wantCode int + wantLimit int // only checked on success paths (wantCode == 0) + }{ + { + name: "non-integer limit returns 400", + limitParam: "abc", + wantCode: 400, + }, + { + name: "float string limit returns 400", + limitParam: "1.5", + wantCode: 400, + }, + { + name: "negative limit clamps to default", + limitParam: "-1", + wantCode: 0, + wantLimit: config.DefaultListLimit, + }, + { + name: "zero limit clamps to default", + limitParam: "0", + wantCode: 0, + wantLimit: config.DefaultListLimit, + }, + { + name: "over-max limit clamps to MaxListLimit", + limitParam: "9999", + wantCode: 0, + wantLimit: config.MaxListLimit, + }, + { + name: "valid limit accepted", + limitParam: "50", + wantCode: 0, + wantLimit: 50, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + if tc.wantCode != 0 { + // Error path: expect a ClientError with wantCode; no store calls. + _, err := handler.getHistory(ctx, req, map[string]string{"limit": tc.limitParam}) + require.Error(t, err, "non-integer limit must return an error") + ce, ok := IsClientError(err) + require.True(t, ok, "error must be a ClientError, got %T: %v", err, err) + assert.Equal(t, tc.wantCode, ce.code) + mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetPurchaseHistoryFiltered", mock.Anything, mock.Anything) + } else { + // Success path: store receives the clamped limit. + mockStore.On("GetAllPurchaseHistory", ctx, tc.wantLimit). + Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything). + Return([]config.PurchaseExecution{}, nil) + + _, err := handler.getHistory(ctx, req, map[string]string{"limit": tc.limitParam}) + require.NoError(t, err) + mockStore.AssertCalled(t, "GetAllPurchaseHistory", ctx, tc.wantLimit) + } + }) + } +} + func TestSummarizePurchaseHistory_CancelPendingDoesNotChangeKPIs(t *testing.T) { // Baseline: three approved (completed) rows. baseline := []config.PurchaseHistoryRecord{ From 959c757cea9514023232f031221ce6520dfa8a34 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:17:34 -0700 Subject: [PATCH 3/3] chore(known-issues): remove 32_history_lazy_expire_test.md (resolved) The expireIfStale test gap documented in this file was closed by TestHandler_getHistory_ExpireIfStale added in the prior commit on this branch. The placeholder is no longer needed. --- known_issues/32_history_lazy_expire_test.md | 49 --------------------- 1 file changed, 49 deletions(-) delete mode 100644 known_issues/32_history_lazy_expire_test.md diff --git a/known_issues/32_history_lazy_expire_test.md b/known_issues/32_history_lazy_expire_test.md deleted file mode 100644 index 615f2fa50..000000000 --- a/known_issues/32_history_lazy_expire_test.md +++ /dev/null @@ -1,49 +0,0 @@ -# Test coverage gap — History `expireIfStale` lazy transition - -**Surfaced during:** 2026-04-23 review of `b44283746` (failed + expired states) -**Related commit:** `b44283746` -**Status:** low-severity test gap — happy paths covered, stale-approval path not. - -## Problem - -`Handler.fetchExecutionsAsHistory` calls `expireIfStale` for every row it -projects, and that helper transitions `pending`/`notified` rows older -than the 7-day `approvalExpiryWindow` to `expired` via -`TransitionExecutionStatus`. The existing tests: - -- `TestHandler_getHistory_IncludesPending` sets `ScheduledDate = - time.Now()` to keep the transition from firing mid-assert (the comment - calls this out explicitly and defers the stale path to "its own test - below"); -- `TestHandler_executePurchase_Success` covers the failed-on-send path. - -Neither test asserts the transition itself: given a -`pending`/`notified` execution with `ScheduledDate > 7 days ago`, -`GetExecutionsByStatuses` returns it, `TransitionExecutionStatus` is -called with `["pending","notified"] → "expired"`, and the returned row's -Status is `expired` with `StatusDescription = "approval link expired …"`. - -## Proposed resolution - -One table-driven test in -`internal/api/handler_history_test.go` that: - -1. Seeds two executions — one fresh, one with `ScheduledDate = - time.Now().Add(-8 * 24 * time.Hour)`. -2. Mocks `GetExecutionsByStatuses` to return both. -3. Mocks `TransitionExecutionStatus` to return the stale row with - `Status = "expired"`. -4. Asserts the mock was called exactly once, with the stale row's ID, - and the rendered history row's `Status == "expired"` + - `StatusDescription` non-empty. -5. Plus a negative case where `TransitionExecutionStatus` returns an - error — `expireIfStale` must fall back to rendering the original - pending row without panicking (already the intent per the inline - comment). - -## Why not now - -The logic is 12 lines; a missed test here wouldn't cause data loss -(the worst case is a pending row mislabeled as pending forever, which -is exactly the pre-`b44283746` behaviour). Parking it here so we don't -lose sight of it on the next touch to that file.