Repository navigation
feat(api): owner-token compare-and-clear for collection in-flight marker (closes #261) - #1516
Conversation
Adds a nullable UUID column that pairs with last_collection_started_at as a compare-and-clear guard for issue #261: the scheduler's unconditional defer-clear wipes another run's in-flight marker when a cron run overlaps a user-triggered collection. This column lets the clear be scoped to the caller that actually owns the marker.
…ion marker (#261) MarkCollectionStarted now stamps a fresh owner token alongside last_collection_started_at, and ClearCollectionStarted only clears when the caller's token still matches last_collection_owner_id. This closes the race where a cron run's unconditional clear could wipe a concurrent user-triggered run's in-flight marker: previously, any caller reaching CollectRecommendations cleared the marker on exit regardless of whether it owned it. The token is generated by MarkCollectionStarted and threaded through the whole async chain: the refresh handler passes it into the Lambda self-invoke payload (owner_token), ParseScheduledEvent extracts it into ScheduledTaskParams, and HandleScheduledTask carries it to CollectRecommendations. Callers that never won a marker (cron, cold-start, background refresh) pass an empty token, and the deferred clear becomes an explicit, logged skip rather than an unconditional wipe. An empty token passed to ClearCollectionStarted directly is a boundary error; a stale/mismatched token is a documented silent no-op. Adds a store-level regression test replaying the exact overlap from the issue (run A wins, goes stale, run B wins with a new token, run A's late clear must not affect run B's marker), plus scheduler and handler unit tests covering the empty-token skip, the owner-token clear (with a deadline-bearing context), the async-invoke rollback path, and the payload's owner_token field.
cristim
added
triaged
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
📝 WalkthroughWalkthroughRecommendation collection now uses per-run owner tokens persisted with in-flight markers. Tokens flow through refresh handlers, Lambda payloads, schedulers, and scheduled-task dispatch, while compare-and-clear cleanup prevents stale runs from clearing newer markers. ChangesRecommendation collection ownership
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RefreshAPI
participant PostgresStore
participant Lambda
participant Scheduler
RefreshAPI->>PostgresStore: MarkCollectionStarted
RefreshAPI->>Lambda: invoke with owner_token
Lambda->>Scheduler: CollectRecommendations with owner token
Scheduler->>PostgresStore: compare-and-clear marker
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/config/store_postgres_recommendations.go (1)
496-510: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider logging when the compare-and-clear is a no-op.
RowsAffected()is discarded, so there's no signal distinguishing "cleared" from "silent no-op due to stale/mismatched owner." Since this no-op is the exact safety mechanism protecting issue#261's race, a debug-level log on the no-op path would help confirm the guard is firing as intended in production without changing behavior.🔍 Optional observability improvement
- if _, err := s.db.Exec(ctx, ` + tag, err := s.db.Exec(ctx, ` UPDATE recommendations_state SET last_collection_started_at = NULL, last_collection_owner_id = NULL WHERE id = 1 AND last_collection_owner_id = $1 - `, token); err != nil { + `, token) + if err != nil { return fmt.Errorf("failed to clear collection started: %w", err) } + if tag.RowsAffected() == 0 { + logging.Debugf("ClearCollectionStarted: no-op, token %q did not match current owner", token) + } return nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/store_postgres_recommendations.go` around lines 496 - 510, Update ClearCollectionStarted to inspect the RowsAffected result from the compare-and-clear UPDATE and emit a debug-level log when it is zero, indicating a stale or mismatched owner token; preserve the existing error handling and successful clear behavior without changing the database operation.internal/server/lambda_test.go (1)
223-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert non-empty owner-token propagation end to end.
These cases only exercise the empty-token path. Add an event with
"owner_token":"tok-1"and assert the scheduler mock receives"tok-1"; otherwise dropping the token between parsing and collection still passes, leaving the marker uncleared.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/lambda_test.go` around lines 223 - 245, Extend the lambda test cases around the collect_recommendations events to include an event containing owner_token "tok-1", and update the scheduler mock assertion to verify CollectRecommendations receives that exact token. Preserve the existing empty-token coverage while ensuring propagation from event parsing through collection is validated.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/handler.go`:
- Around line 363-371: Update ParseScheduledEvent to validate event.OwnerToken
before returning the mapped task type: allow an empty token for cron events, but
reject any non-empty token that is not a valid UUID and return a descriptive
error. Keep forwarding valid tokens through ScheduledTaskParams and preserve the
existing action mapping behavior.
---
Nitpick comments:
In `@internal/config/store_postgres_recommendations.go`:
- Around line 496-510: Update ClearCollectionStarted to inspect the RowsAffected
result from the compare-and-clear UPDATE and emit a debug-level log when it is
zero, indicating a stale or mismatched owner token; preserve the existing error
handling and successful clear behavior without changing the database operation.
In `@internal/server/lambda_test.go`:
- Around line 223-245: Extend the lambda test cases around the
collect_recommendations events to include an event containing owner_token
"tok-1", and update the scheduler mock assertion to verify
CollectRecommendations receives that exact token. Preserve the existing
empty-token coverage while ensuring propagation from event parsing through
collection is validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a7751e4-5b47-4558-be39-87b339d62695
📒 Files selected for processing (30)
cmd/server/main.gointernal/analytics/collector_test.gointernal/api/handler_recommendations_refresh.gointernal/api/handler_test.gointernal/api/mocks_test.gointernal/api/types.gointernal/config/interfaces.gointernal/config/store_postgres_recommendations.gointernal/config/store_postgres_recommendations_test.gointernal/database/postgres/migrations/000093_recommendations_state_owner_id.down.sqlinternal/database/postgres/migrations/000093_recommendations_state_owner_id.up.sqlinternal/database/postgres/migrations/000093_recommendations_state_owner_id_test.gointernal/mocks/stores.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_overrides_test.gointernal/scheduler/scheduler_suppressions_test.gointernal/scheduler/scheduler_test.gointernal/server/app_test.gointernal/server/handler.gointernal/server/handler_ladder_test.gointernal/server/handler_ri_exchange_test.gointernal/server/handler_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/integration_test.gointernal/server/interfaces.gointernal/server/lambda.gointernal/server/lambda_test.gointernal/server/test_helpers_test.gointernal/testutil/mocks.go
…er marker (#261) SetRecommendationsCollectionError unconditionally cleared last_collection_started_at with no owner check. It is called mid-run from persistCollection on every CollectRecommendations invocation that hits a provider error, including tokenless cron/cold-start/background runs, so a tokenless run's routine provider error could wipe a concurrent owner run's in-flight marker. This reopened the exact race the compare-and-clear guard in the previous commit exists to close. The only place that may clear last_collection_started_at now is the deferred, token-guarded clearCollectionStartedBestEffort in CollectRecommendations, which already runs on both the success and failure exit paths. SetRecommendationsCollectionError now only writes last_collection_error. Adds a regression test replaying the scenario: MarkCollectionStarted stamps an owner token, a tokenless caller invokes SetRecommendationsCollectionError, and the owner's marker must survive.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Ready for human merge (closes #261).
Design: owner token threads explicitly through the async-invoke payload ( Held for your merge. |
A collect run that won MarkCollectionStarted (so it owns the in-flight marker) but is then skipped by the advisory lock never reaches CollectRecommendations, whose deferred token-scoped clear would normally release the marker. It therefore sat stranded for the full 5-minute auto-recovery window, and every refresh the user attempted in that window was rejected with 409 "collection already in progress" while no run backed by that marker existed. Before the compare-and-clear guard the overlapping run holding the lock happened to cover this case with its unconditional clear. Scoping the clear to its owner correctly stopped that cross-run wipe, so the abandoning run now has to release its own marker explicitly. Only the lock-skip path releases: a lock-check error is returned to the caller, which lets the Lambda async invoke retry the same event with the same owner token and still run the collect, so the marker must survive there. The release is scoped by the owner token, so it can only ever clear the marker this run owns, never a concurrent run's, and tokenless callers (cron, the /api/scheduled/ HTTP path, the --task CLI) own nothing and are skipped. Regression test asserts the clear fires with the owning token on a skip, and that tokenless runs, lock errors, and other task types never touch the marker. Verified failing before the fix and passing after. Refs #261
The store-level compare-and-clear test described run B as "a cron run", but both runs in it go through MarkCollectionStarted, which only the POST /api/recommendations/refresh handler calls. Cron reaches CollectRecommendations directly and never marks, so run B actually models a second user-triggered refresh taking the marker over after run A's 5-minute window lapses. Points the cron half of the race at the scheduler test that does pin it (TestScheduler_CollectRecommendations_EmptyTokenSkipsClear), so both halves of the guard are traceable from either side. Comments only, no behavior change. Refs #261
Adversarial review passReviewed the full diff against Confirmed defect: owner marker stranded when the collect run is lock-skipped
Concrete sequence:
This is a regression introduced by the PR, not a pre-existing bug: before the compare-and-clear guard the cron run's unconditional Fix ( Deliberately not released on the lock-check-error path: that path returns an error, so the Lambda async invoke retries the same event with the same token and the collect can still run. Clearing there would strand the retry. There's a subtest pinning that. Regression test Comment accuracy
Probed and found clean
Considered and deliberately not changed
Gates
@coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
owner_token arrives as raw event input and is forwarded to a UUID-typed column, so validate it in ParseScheduledEvent instead of letting a corrupt value reach the persistence layer. Empty stays legitimate: EventBridge cron, the /api/scheduled/ HTTP path and the --task CLI never win MarkCollectionStarted and own no marker. A non-empty value can only have come from asyncInvokeSelf, which always sends a uuid.New(), so anything else means the payload is corrupt. Failing loud is deliberate: a token that cannot match any owner would strand the marker it belongs to for the full 5-minute recovery window regardless, so a buried error log on the eventual clear is strictly worse than refusing the event. uuid.Parse's error text describes the shape only and never echoes the value, so wrapping it does not leak the token into logs. Adds end-to-end coverage that a well-formed token survives parsing and reaches CollectRecommendations, which is what scopes the deferred clear to that run: dropping it anywhere in between now fails the test rather than silently stranding the marker. The captured token is asserted from the subtest goroutine rather than inside the mock callback so a failed assertion never calls FailNow off the owning goroutine. Addresses CodeRabbit review findings on PR #1516. Refs #261
✅ Final: ready for human merge (closes #261; also resolves #1524)Full review chain complete — all green, all independently verified:
One accepted, documented trade-off (independent review, recommended accept): under Lambda at-least-once delivery, a duplicate of the same event could take the skip path and release the marker the still-running original owns. Fully bounded — the advisory lock still prevents any concurrent collection; worst case is the in-flight signal dropping early, so a redundant collect runs after the lock frees. No data corruption, no cross-owner wipe. Strictly better than the pre-fix behavior (marker stranded 5 min, user 409'd). Design: owner token threads explicitly via the async-invoke payload; no DB read-back; tokenless runs pass "" and skip the clear. Migration |
…cope) Revives the deferred "API keys usage stats" sub-task from #340/#344 (prior PR #380 closed stale). Fresh implementation against current main using #380's body as spec. Backend: migration 000094 adds request_count_total/request_count_24h counters to api_keys, plus an atomic RecordAPIKeyUsage store method that increments both alongside last_used_at with a rolling 24h window. New GET /api/api-keys/usage-stats aggregates the calling user's own keys into a section summary (active count, 24h/lifetime totals, top-3 most active). OpenAPI updated. Frontend: Admin -> API Keys now shows a 3-tile summary card (active keys, requests 24h, requests lifetime) + a top-3 most-active list, plus per-row Requests (24h) / Requests (total) columns. Loading skeletons via lib/skeleton; summary errors stay isolated from list errors. Migration numbered 000094 (not #380's stale 000051) since 000093 is already claimed by in-flight PR #1516.
When asyncInvokeSelf fails, runMarkedCollection rolls back this caller's own in-flight collection marker. It reused the request context to do so, but the dominant cause of an invoke failure is that context itself expiring or being canceled (API Gateway deadline, client disconnect, a slow SDK credential refresh), so the rollback failed in exactly the case that produced it. The marker then sat stranded for the full 5-minute auto-recovery window in MarkCollectionStarted, rejecting every refresh the user attempted with 409 while no collection was running. Extract rollbackMarkedCollection, which issues the token-scoped clear on a detached 5-second context. This is the same pattern the scheduler's deferred clear and Application.releaseSkippedCollectionMarker already use for the same reason; this sibling path was the one that had not adopted it. The regression test drives a request context that dies mid-invoke and pins the clear context to one that is deadline-bounded and still live. The matcher sits on the mock expectation rather than a trailing AssertCalled, since testify's AssertCalled compares arguments by value and does not honor MatchedBy: pre-fix the clear still happened, just with the canceled context, so asserting only that Clear was called would pass with the bug present.
ParseScheduledEvent wrapped uuid.Parse's error with %w, and the comment asserted that error "describes the shape only and never echoes the token value". That is false for one input shape: at 45 characters uuid.Parse takes its urn-prefix branch and formats the error as `invalid urn prefix: %q` over the value's first nine bytes, which then lands in the Lambda logs. No token a legitimate producer issues can reach that branch (asyncInvokeSelf always sends a 36-character uuid.New()), so this is defense in depth rather than a live leak, but the repo forbids logging token material and the comment claimed a guarantee the code did not provide. Return a fixed message instead of wrapping. The parse failure's shape carries no diagnostic value the fixed message does not already give, so the value is dropped rather than masked. The regression test uses the 45-character input specifically: a shorter malformed token passes with the bug present, because uuid.Parse reports those as a bare length/format error containing nothing sensitive.
Both tests pinning "this path must not clear the collection marker" passed with the bug present. MockConfigStore short-circuits any method with no registered expectation before reaching mock.Called, so the call never lands in m.Calls, and testify's AssertNotCalled walks m.Calls. Demonstrated by deleting the empty-token guard in clearCollectionStartedBestEffort: TestScheduler_CollectRecommendations_ EmptyTokenSkipsClear stayed green, leaving the central invariant of the compare-and-clear guard unenforced. The same held for the happy-path assertion in TestRunMarkedCollection_AsyncInvokeSuccess_ PayloadCarriesOwnerToken. Register the expectation as .Maybe() in both, so an unwanted call is recorded and the assertion can actually fail. This is the treatment the sibling cases in internal/server/handler_test.go already had; these two were missed. Verified both now fail when the guarded behavior is removed.
releaseSkippedCollectionMarker scopes its clear by owner token, but the token identifies the MARKER, not the invocation. It therefore cannot distinguish "the advisory lock is held by a tokenless cron run" (release is required, or the marker strands for the full 5-minute window) from "the lock is held by a concurrent duplicate delivery of my own event" (release is premature: that sibling carries the same token and is still collecting). Lambda async invocation is at-least-once, so the second case is reachable. There the marker is cleared mid-run: the frontend banner drops early, and a refresh issued during the remainder wins a fresh marker only to be lock-skipped and released again, returning 202 without collecting. Bounded and self-healing, and it touches no purchase or money path. Releasing remains strictly better than not releasing, since cron overlap is routine while duplicate delivery is rare. Closing the gap properly needs an invocation-scoped identity distinct from the marker token (for example, the lock winner re-stamping last_collection_owner_id with a fresh token), which is a design change kept out of this PR. Documented here so the trade-off is explicit rather than an undiscovered trap.
Independent adversarial review passRe-derived the compare-and-clear design from scratch rather than trusting the earlier pass, plus a second independent reviewer on the same diff. Four commits pushed ( Confirmed and fixed1. Async-invoke rollback clear reused the dying request context — When Regression test drives a request context that dies mid-invoke and pins the clear context to deadline-bounded-and-live. The matcher sits on the mock expectation rather than a trailing 2. Rejected owner-token bytes could reach the logs —
The regression test uses the 45-char input specifically; a shorter malformed token passes with the bug present. 3. Both "must not clear the marker" tests were vacuous —
Documented, not fixed4. Duplicate-delivery gap in The token identifies the marker, not the invocation, so a skipped run cannot distinguish "the lock is held by a tokenless cron run" (release required, else the marker strands) from "the lock is held by a concurrent duplicate delivery of my own event" (release premature — that sibling holds the same token and is still collecting). Lambda async invocation is at-least-once, so the second case is reachable, and there the marker clears mid-run: the banner drops early, and a refresh during the remainder wins a fresh marker only to be lock-skipped and released again, returning 202 without collecting. Bounded, self-healing, no purchase/money path. Releasing is still strictly better than not releasing, because cron overlap is routine while duplicate delivery is rare. Closing it properly needs an invocation-scoped identity distinct from the marker token (e.g. the lock winner re-stamping Verified clean, no action
Pre-existing, out of scope
Gates on the pushed head: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Commit range =="
git log --oneline --decorate b16d59fb3^..c47179567
echo
echo "== Changed files =="
git diff --name-status b16d59fb3^..c47179567
echo
echo "== Diff: production and directly related tests =="
git diff --unified=50 b16d59fb3^..c47179567 -- \
internal/api/handler_recommendations_refresh.go \
internal/api/handler_test.go \
internal/server/handler.go \
internal/server/handler_test.go \
internal/scheduler/scheduler_test.go
echo
echo "== Relevant symbols =="
ast-grep outline internal/api/handler_recommendations_refresh.go --items all --type function --match 'runMarkedCollection|rollbackMarkedCollection|asyncInvokeSelf'
ast-grep outline internal/server/handler.go --items all --type function --match 'ParseScheduledEvent|HandleScheduledTask|releaseSkippedCollectionMarker'
ast-grep outline internal/api/handler_test.go --items all --type function --match 'TestRunMarkedCollection'
ast-grep outline internal/scheduler/scheduler_test.go --items all --type function --match 'TestScheduler_CollectRecommendations_EmptyTokenSkipsClear'
echo
echo "== All production ClearCollectionStarted call sites =="
rg -n -C 5 --glob '*.go' '\bClearCollectionStarted\s*\(' internalLength of output: 58332 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Scheduler collection lifecycle =="
ast-grep outline internal/scheduler/scheduler.go --items all --type function --match 'CollectRecommendations|clearCollectionStartedBestEffort'
sed -n '300,395p' internal/scheduler/scheduler.go
echo
echo "== Advisory-lock marker-release tests =="
rg -n -A220 -B15 'TestHandleScheduledTaskReleasesMarkerWhenSkipped' internal/server/handler_test.go
echo
echo "== Parser tests and known test owner token =="
rg -n -A160 -B15 'TestParseScheduledEvent_MalformedTokenNotEchoedInError|func TestParseScheduledEvent\b|testOwnerToken' internal/server/handler_test.go
echo
echo "== Mock expectation behavior =="
sed -n '1325,1370p' internal/mocks/stores.go
rg -n -A20 -B5 'func isExpected\b' internal/mocks
echo
echo "== UUID dependency version and parser implementation =="
rg -n -C2 'github.com/google/uuid' go.mod go.sum
uuid_file="$(go env GOPATH 2>/dev/null)/pkg/mod/github.com/google/uuid@*/uuid.go"
if [ -n "$uuid_file" ]; then
printf '%s\n' "$uuid_file"
fiLength of output: 24920 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== CollectRecommendations entry and cleanup defer =="
rg -n -A115 -B15 'func \(s \*Scheduler\) CollectRecommendations\b|clearCollectionStartedBestEffort\s*\(' internal/scheduler/scheduler.go
echo
echo "== Token-guarded persistence operations =="
sed -n '430,535p' internal/config/store_postgres_recommendations.go
echo
echo "== Rollback helper references and timeout constants =="
rg -n -C4 '\brollbackMarkedCollection\s*\(|markedCollectionRollbackTimeout|skippedCollectionMarkerClearTimeout' internal
echo
echo "== All owner-token parsing and scheduled-task dispatch paths =="
rg -n -C4 '\bParseScheduledEvent\s*\(|OwnerToken|CollectRecommendations\(ctx,.*ownerToken' internal/server internal/apiLength of output: 40134
Confirmed:
The documented Lambda duplicate-delivery limitation is accurately bounded and appropriately kept out of this PR’s scope. 🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
ClearCollectionStarted discarded RowsAffected, so nothing in production distinguished "cleared the marker" from "declined to clear someone else's marker". That no-op branch IS the safety mechanism issue #261 adds, and a guard that never fires looks identical to a guard that is broken. Capture the command tag and emit a debug-level line when zero rows matched. Behavior is unchanged. Addresses a CodeRabbit nitpick, with one deviation: the suggested diff logged the token value (`token %q`). The token is the capability that controls the marker, and this PR already stopped rejected token bytes from reaching the logs via ParseScheduledEvent, so the message names the condition without echoing the token.
|
Swept the nitpicks from the earlier review round ( Addressed — One deliberate deviation from the suggested diff: it logged Already addressed — Gates on @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Latest commit and changed files =="
git show --no-ext-diff --format=fuller --name-status 036cb1366
echo
echo "== Targeted diff =="
git diff --no-ext-diff --unified=40 036cb1366^ 036cb1366 -- \
internal/config/store_postgres_recommendations.go \
internal/server/lambda_test.go
echo
echo "== Store lifecycle symbols =="
ast-grep outline internal/config/store_postgres_recommendations.go \
--items all --type function --match 'ClearCollectionStarted|MarkCollectionStarted'
echo
echo "== ClearCollectionStarted implementation and logger context =="
rg -n -A90 -B15 'func \(s \*PostgresStore\) ClearCollectionStarted\b|func \(s \*PostgresStore\) MarkCollectionStarted\b' \
internal/config/store_postgres_recommendations.go
echo
echo "== Lambda propagation test =="
rg -n -A120 -B20 'async self-invoke carries owner_token through to the scheduler|func TestHandleLambdaScheduledEvent\b' \
internal/server/lambda_test.go
echo
echo "== Owner-token log/rendering references in production code =="
rg -n -i -C3 --glob '*.go' \
'(debug|info|warn|error|printf|sprintf|sprint|log)\w*\(.*(owner.?token|owner_token|token)|owner.?token.*(debug|info|warn|error|printf|sprint)' \
internal/config internal/server internal/api internal/scheduler || true
echo
echo "== Clear command-tag API usages =="
rg -n -C3 --glob '*.go' '\.(RowsAffected|String)\(\)|CommandTag|Exec\(' \
internal/config/store_postgres_recommendations.goLength of output: 76514
✏️ Learnings added
|
|
CodeRabbit was rate-limited when the current head landed, so its response to this head is a limit notice rather than a review. Treating that as a clean review would be a false clean: a throttled pass skips the in-flight commits. Re-requesting a full pass (not incremental) now that the limit window has elapsed. @coderabbitai full review |
|
✅ Action performedFull review finished. |
…cope) Revives the deferred "API keys usage stats" sub-task from #340/#344 (prior PR #380 closed stale). Fresh implementation against current main using #380's body as spec. Backend: migration 000094 adds request_count_total/request_count_24h counters to api_keys, plus an atomic RecordAPIKeyUsage store method that increments both alongside last_used_at with a rolling 24h window. New GET /api/api-keys/usage-stats aggregates the calling user's own keys into a section summary (active count, 24h/lifetime totals, top-3 most active). OpenAPI updated. Frontend: Admin -> API Keys now shows a 3-tile summary card (active keys, requests 24h, requests lifetime) + a top-3 most-active list, plus per-row Requests (24h) / Requests (total) columns. Loading skeletons via lib/skeleton; summary errors stay isolated from list errors. Migration numbered 000094 (not #380's stale 000051) since 000093 is already claimed by in-flight PR #1516.
…cope) Revives the deferred "API keys usage stats" sub-task from #340/#344 (prior PR #380 closed stale). Fresh implementation against current main using #380's body as spec. Backend: migration 000094 adds request_count_total/request_count_24h counters to api_keys, plus an atomic RecordAPIKeyUsage store method that increments both alongside last_used_at with a rolling 24h window. New GET /api/api-keys/usage-stats aggregates the calling user's own keys into a section summary (active count, 24h/lifetime totals, top-3 most active). OpenAPI updated. Frontend: Admin -> API Keys now shows a 3-tile summary card (active keys, requests 24h, requests lifetime) + a top-3 most-active list, plus per-row Requests (24h) / Requests (total) columns. Loading skeletons via lib/skeleton; summary errors stay isolated from list errors. Migration numbered 000094 (not #380's stale 000051) since 000093 is already claimed by in-flight PR #1516.
…e) (#1523) * feat(api+frontend/admin): per-API-key usage stats (closes #340/#344 scope) Revives the deferred "API keys usage stats" sub-task from #340/#344 (prior PR #380 closed stale). Fresh implementation against current main using #380's body as spec. Backend: migration 000094 adds request_count_total/request_count_24h counters to api_keys, plus an atomic RecordAPIKeyUsage store method that increments both alongside last_used_at with a rolling 24h window. New GET /api/api-keys/usage-stats aggregates the calling user's own keys into a section summary (active count, 24h/lifetime totals, top-3 most active). OpenAPI updated. Frontend: Admin -> API Keys now shows a 3-tile summary card (active keys, requests 24h, requests lifetime) + a top-3 most-active list, plus per-row Requests (24h) / Requests (total) columns. Loading skeletons via lib/skeleton; summary errors stay isolated from list errors. Migration numbered 000094 (not #380's stale 000051) since 000093 is already claimed by in-flight PR #1516. * fix(auth+api+frontend): correct API-key usage-window naming, extract oversized files, fix migration gap CodeRabbit findings on PR #1523: - request_count_24h was a fixed/tumbling window counter, not a true rolling 24h total (a request just before a window reset was silently dropped from the count). Renamed to request_count_window across the DB column, Go types, OpenAPI schema, and frontend, and exposed the new request_count_window_start field so API consumers can see exactly which period the count covers instead of assuming "last 24h". - Split the new usage-stats rendering out of frontend/src/apikeys.ts into frontend/src/apikeys_usage.ts, bringing apikeys.ts back under the 500-line limit. - Split the new usage-stats handler and its router wrapper out of internal/api/handler_apikeys.go / router.go into a focused internal/api/handler_apikeys_usage.go (tests moved alongside into handler_apikeys_usage_test.go). Also fixes the failing Integration Tests job: migration 000094 left a numbering gap after 000092 (000093 was skipped), so TestMigrations_AutoHealDirty's rollback-lands-one-version-below-head assertion failed -- rolling back landed on 92, not 93, since no migration owns that version. Renumbered the migration to 000093 to close the gap. * fix(auth): correct API-key usage stats staleness, scope and counting Adversarial review follow-up on the per-API-key usage stats added in this PR. Three confirmed correctness bugs, each with a regression test that fails without its fix. 1. Expired windows were reported as current activity. request_count_window is only rewritten by the key's NEXT request, so a key that went idle kept its closed window's count on the row indefinitely. The read path summed and ranked that column verbatim, so the summary card could report thousands of requests "in the window" for keys unused for months, and rank a long-dead key as most active. Window counters now go through effectiveWindowUsage, which reports zero with no window start once the window has closed. 2. Expired keys were counted as active. TotalActive tested only the revocation flag, so a key past its expires_at was summarised as active while the keys table directly below it rendered the same key as "Expired". It now uses validateAPIKeyStatus, the same predicate the authentication path applies. 3. Concurrent requests were silently dropped from the counters. singleflight collapses concurrent flushes for one key into a single DB write, and the flush incremented by a fixed 1, so every request arriving during an in-flight write was lost. The undercount grew with request rate, i.e. was worst on exactly the busy keys the stats exist to surface. Requests are now accumulated in memory and the flush writes the whole delta, with a bounded re-drain so a request is never stranded until the key's next use. singleflight still bounds the write rate to one in-flight DB write per key. The 24h window length is now a single Go constant passed into the SQL via make_interval, so the write path and the read-side expiry check cannot drift. RecordAPIKeyUsage rejects a non-positive delta rather than issuing a no-op UPDATE. * fix(auth,frontend): split API-key store file, guard stale summary loads Addresses the two still-open CodeRabbit findings on this PR. store_postgres.go was past the project's 500-line ceiling and this PR pushed it further. Its API-key surface moves to store_postgres_apikeys.go (856 + 301 lines). Pure move: no query, scanning or signature changes. loadApiKeysUsageStats could render a stale result. Concurrent refreshes resolve in arbitrary order, so a slow earlier request could overwrite a newer summary, or paint its error over a summary that had loaded fine. A generation counter makes a load a no-op once a newer one has started, on both the success and the failure path. Adds apikeys-usage.test.ts, which the summary module had been missing entirely: render path, top-keys list, markup-in-key-name staying literal text, count-abbreviation boundaries, the error path, and both ordering guards. The two ordering tests fail against the unguarded loader. * fix(auth,frontend): report unmeasured lifetime counts as unknown, not 0 Migration 000093 added request_count_total with DEFAULT 0 and no backfill, so every API key that already existed reads as zero regardless of how much traffic it actually served. The UI presented that as "Requests (total): 0", stating a request volume nobody measured, and the summary card folded those fabricated zeros into an exact-looking lifetime sum. RecordAPIKeyUsage is the only writer of last_used_at on the request path and it always bumps the counter in the same statement, so "used at least once yet carrying a zero lifetime count" identifies exactly the rows that predate the counter. Those now report null, and the table renders "n/a" instead of a number. A key that has genuinely never been used still reports 0. The section summary excludes unknown keys from total_requests_lifetime and sets lifetime_partial, which the card renders as a "+" suffix so the total reads as a lower bound rather than an exact figure. The invariant this relies on is documented on effectiveLifetimeUsage: a new caller of the legacy UpdateAPIKeyLastUsed, which bumps only the timestamp, would break it. It currently has no production callers. * fix(db): renumber API-key usage-counter migration 000093 -> 000094 main gained 000093_recommendations_state_owner_id after this branch claimed the number, so the merge ref carried two 000093 migrations and CI's check-migration-conflicts hook failed. The collision is invisible locally because each ref on its own is consistent. Renamed via git mv and updated every reference to the number in the Go doc comments, the OpenAPI descriptions, the frontend type comments and the tests. Verified against the merge ref (origin/main union this branch): no migration number appears twice. * fix(auth,frontend,db): align usage-counter docs and test timeout with shipped semantics Addresses the three actionable findings from the CodeRabbit pass on e20ca1e. All three are accuracy defects introduced by earlier correct fixes that did not carry their documentation along. - frontend/src/api/apikeys.ts: the getApiKeysUsageStats doc comment still described top keys as ranked "by 24h activity", contradicting the rename of request_count_24h to request_count_window. The counter is a fixed/tumbling window, not a rolling 24h total, so the comment now names the field and its windowing semantics to match the wording already used on APIKeyInfo and APIKeysUsageStats. - internal/auth/service_apikeys_test.go: raise the async RecordAPIKeyUsage deadline from 500ms to 5s, matching the two sibling tests that wait on the same kind of background write. 500ms is a real flake hazard on a loaded CI runner. The signalling channel and the t.Fatal on timeout are unchanged, so the test still fails if usage recording is dropped entirely. - internal/database/postgres/migrations/000094: the comment claimed all existing rows display "0 requests". The read path no longer does that. effectiveLifetimeUsage treats a zero request_count_total on a key with a non-null last_used_at as unknown rather than zero, surfacing it as lifetime_partial, precisely so a pre-migration key is not reported as having served no traffic. The comment now describes that split and points at the function enforcing it. Comment-only and test-only; no production behaviour changes. * fix(auth): book API key usage per request, not per validation The usage counters added by this PR were incremented inside auth.Service.ValidateUserAPIKey, i.e. once per CREDENTIAL VALIDATION. A single HTTP request validates the same key several times over: validateSecurityContext resolves the principal (1), then every permission check re-validates it (1 more), and the multi-verb gates re-validate once per verb. Reported volume was therefore 2x on a plain gate and 5x on the four-verb planned-purchase gate: 100 requests were reported as 200-400. The multiplier varies per endpoint, so the stored totals cannot be corrected retroactively by dividing them down. The multiplicity is pre-existing and was harmless while the async write was an idempotent last_used_at timestamp. Turning it into an additive counter converted it into a systematic overcount, made worse by the singleflight accumulator, which correctly conserves the whole pending delta and so preserves every surplus booking. ValidateUserAPIKey is now a pure validation primitive with no usage side effect, and usage is booked exactly once in Handler.validateSecurityContext, the one code path guaranteed to run once per inbound request. Principal carries the resolved API key ID so the booking needs no second lookup, and principalFromUserAPIKey now fails closed when validation yields no usable key record rather than authenticating a key it cannot attribute usage to. Regression coverage sits at the HandleRequest layer, where the defect actually lives; a service-level test cannot see it, because it books correctly when called once. internal/server drives a real auth.Service behind a real api.Handler and asserts one store booking per request on both the single-verb and four-verb paths. Both fail on the pre-fix code (2 and 5 bookings respectively). internal/api adds the handler-side half: which principal kinds book, how often, and against which key ID. Also addresses three CodeRabbit findings: - formatRequestCount's doc claimed an em dash; NO_COUNT_DATA renders "n/a". - Migration 000094 pointed at store_postgres.go; RecordAPIKeyUsage lives in store_postgres_apikeys.go. - CreateAPIKeyAPI read the window counters raw while every other exposure site goes through effectiveWindowUsage. It cannot misreport today (keyInfo is built in memory and its counters are necessarily zero), but it now shares the one implementation so the invariant holds structurally.
1 participant
Summary
Closes #261.
MarkCollectionStartednow stamps a fresh owner token (UUID) alongsidelast_collection_started_at, andClearCollectionStartedonly clears the marker when the caller's token still matcheslast_collection_owner_id. This closes the race where a cron run's unconditional clear could wipe a concurrent user-triggered async run's in-flight marker.The token is threaded end-to-end:
postRefreshRecommendationsgets the token fromMarkCollectionStartedand passes it torunMarkedCollection.owner_tokenfield.ScheduledEvent/ParseScheduledEvent/ScheduledTaskParams/HandleScheduledTaskcarry the token through toCollectRecommendations./api/scheduled/HTTP path,--taskCLI, scheduler cold-start, background stale-cache refresh) pass an empty token; the deferred clear becomes an explicit, logged skip instead of an unconditional wipe.ClearCollectionStarted("")is a boundary error (fail loud); a stale/mismatched non-empty token is a documented silent no-op (someone else's marker).Design note: the design intentionally does not use a
GetCollectionOwnerTokenDB read-back (an earlier stranded WIP attempt on this issue used that approach) — a read-back would let a cron run adopt whatever token is currently persisted, which re-creates the exact race the issue describes. The token must be threaded through the payload from the handler that actually won the race.Migration
000093addsrecommendations_state.last_collection_owner_id UUID(nullable). Verified free: highest migration onmainis000092, and none of the 3 currently-open PRs (#1512, #1504, #1495) touch a migration file.Behavior deltas
MarkCollectionStarted, plus the frontend'slast_collected_at > started_atcompletion signal.HandleScheduledTaskwhile cron already holds it, the user's marker persists until the 5-minute window elapses. This is strictly better than today's behavior (a false "done" from an unconditional clear).owner_tokennever appears in any API response;RefreshResponseunchanged), Terraform (the EventBridge cron payload already sendsaction; an absentowner_tokensimply decodes to"").internal/api/handler.go:114has a stale comment referencing a removedtriggerColdStartCollectfunction; not touched since no adjacent hunk in this PR touches that line.Regression evidence (fail pre-fix / pass post-fix)
New test:
TestPostgresStore_ClearCollectionStarted_CompareAndClear(internal/config/store_postgres_recommendations_test.go) replays the issue's exact race: run A wins and getstokenA; a concurrent Mark is blocked; A's marker is forced stale; run B wins with a newtokenB; A's lateClear(tokenA)must be a no-op leaving B's marker intact; B's ownClear(tokenB)then actually clears;Clear("")errors.ClearCollectionStartedto the old unconditional-clear body, ignoring the token):go test -tags=integration -run TestPostgresStore_ClearCollectionStarted_CompareAndClear ./internal/config/...→ exit 1, failing exactly at the regression assertion (store_postgres_recommendations_test.go:459, "run B's marker must survive run A's stale-token clear" —Expected value not to be nil, i.e. it was nil, B's marker got wiped).--- PASS.Gates (all exit 0)
go build ./...→ 0go vet ./...→ 0go test ./internal/scheduler/... ./internal/api/... ./internal/server/... ./internal/analytics/... ./internal/config/... ./internal/testutil/... ./cmd/...→ 0 (4071 passed)go test ./...(full repo) → 0go test -tags=integration -run 'TestMigration_RecommendationsStateOwnerID|TestPostgresStore_ClearCollectionStarted_CompareAndClear' ./internal/database/postgres/migrations/... ./internal/config/...→ 0 (2 passed)gocyclo -over 10 .→ pre-existing violations only, all in test files this PR does not touch (none of the touched files appear in the output)golangci-lint(exact CI-pinned v2.10.1 binary) → exit 0, "0 issues"Test plan
uuid, nullable) and its rollback, pinned to version 93/92 (never HEAD)AssertNotCalled), owner-token clear withmock.MatchedByasserting the clear context carries a deadlineowner_tokenin the payload and does not call Clear itselfParseScheduledEventtest case forowner_tokenextraction from the EventBridge-style payload🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests