Repository navigation
fix(api): move expireIfStale async; exempt empty-account rows from scoped drop - #1043
Merged
cristim merged 3 commits intoJun 7, 2026
Merged
Conversation
…oped 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).
cristim
added
bug
Contributor
|
Warning Review limit reached
More reviews will be available in 11 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…oses #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).
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.
Member
Author
FOLD-1043 additions (closes #1061)Three findings from 01-api-handlers.md folded in as two additional commits: Commit 1 (fix(api): use strconv.Atoi for limit; fresh slice in filter/merge):
Commit 2 (chore(known-issues): remove 32_history_lazy_expire_test.md): Removes the placeholder that documented the missing stale-expiry test -- the gap is closed by TestHandler_getHistory_ExpireIfStale added in this PR's original commit. Verification: go build ./..., go vet ./internal/api/..., go test ./internal/api/... -- 1502 tests pass. @coderabbitai review |
Contributor
|
Triggering an incremental review of the new commits now. 🧠 Learnings used✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1032
Two coupled defects on the
getHistoryGET read path:GET mutates DB (non-idempotent, racy):
expireIfStalecalledTransitionExecutionStatusinline insidefetchExecutionsAsHistory. A pure GET that writes races concurrent loads (two simultaneous requests both attempt the transition; the loser's error is swallowed) and makes HEAD/replay semantics unsafe.Scoped users lose in-flight financial actions (bug(purchases): approved AWS purchase vanishes from History — 'approved'/'running' execution statuses hidden by the History list (P0) #621 regression):
executionToHistoryRowsetsAccountID = ""whenexec.CloudAccountIDis nil and recs carry no common account (ambient/multi-account path).filterPurchaseHistoryByAllowedAccountsthen callsMatchesAccount(allowed, "", "")which isfalsefor any scoped non-admin user, silently dropping in-flight executions -- re-introducing the silent-disappearance bug bug(purchases): approved AWS purchase vanishes from History — 'approved'/'running' execution statuses hidden by the History list (P0) #621 fixed.Changes
Defect 1: Extracted stale-expiry out of the hot fetch loop. The GET now collects stale execution IDs during the read phase and fires
TransitionExecutionStatusin a best-effort background goroutine (expireStaleExecutionsAsync) after the response is assembled. The transition remains idempotent via the FROM-status guard in the store. Response reflects current DB state; next History load sees the updated status.Defect 2:
filterPurchaseHistoryByAllowedAccountsnow passes rows with emptyAccountIDthrough for scoped users. These carry no account-specific data (they are ambient/unattributed executions) so cross-tenant isolation is not affected; theview:purchasesgate already fired.Verification
go build ./...,go vet ./internal/api/...,go test ./internal/api/...-- 1502 tests pass, no race detector issues.Two new regression tests that fail against pre-fix code:
TestHandler_getHistory_GetIsReadOnly: asserts the handler returns beforeTransitionExecutionStatuscompletes (gate + channel pattern, no sleep). Pre-fix: handler blocks on the gate (deadlock); test fails with timeout.TestHandler_getHistory_ScopedUserSeesEmptyAccountRows: asserts a scoped user sees ambient-credential executions (AccountID == ""). Pre-fix: row dropped,require.Lenfails.Updated
TestHandler_getHistory_ExpireIfStaleto assert the new async behaviour: response shows pre-transition status ("pending"); goroutine fires in the background, verified via done channel (no sleep per project memory).References #621 (original silent-disappearance fix this partially regresses).
Scope expanded
Also closes #1061
Three additional findings from
docs/code-review/01-api-handlers.mdfolded into this PR (FOLD-1043):01-M1 (
parseHistoryFilterslimit parsing):fmt.Sscanfsilently swallowed non-integerlimitvalues, giving callers no feedback. Replaced withstrconv.Atoi+ 400 ClientError on parse failure.limit < 1now clamps toDefaultListLimit(perfeedback_strict_int_parse). Regression guard:TestHandler_getHistory_LimitParsing(6 sub-cases, fails pre-fix on the non-integer and float cases).01-L1 (
handler_history.gomerge slice):append(completed, extra...)with a//nolint:gocriticcomment was replaced by an explicit pre-allocated slice (make+ two appends) that is self-documenting and avoids the lint suppression.01-L2 (history filter site):
filterPurchaseHistoryByAllowedAccountsusedpurchases[:0]as the output slice, aliasing the input's backing array. Replaced withmake([]..., 0, len(purchases))for a true fresh slice.Additionally,
known_issues/32_history_lazy_expire_test.mdis removed: the test gap it documented (no test for the async stale-expiry path) is closed byTestHandler_getHistory_ExpireIfStaleadded in this PR's original commit.