fix(auth): reject RotateAPIKey when SoftDelete affects 0 rows by devin-ai-integration[bot] · Pull Request #822 · workweave/router

devin-ai-integration · 2026-07-23T13:07:10Z

Summary

Compliance rewrite of #819 by @rohith500 (thanks!). Fixes #817: RotateAPIKey no longer mints a successor when its own SoftDelete matches 0 rows (a lost race against a concurrent Rotate or a Delete), which previously produced untracked "zombie" credentials.

Production logic and tests are preserved verbatim from #819; the only delta is trimming godoc/SQL comments to the repo's comment convention (root CLAUDE.md line 15: concise, no multi-paragraph, no pre-fix narrative, no caller references).

Behavior:

SoftDeleteModelRouterAPIKey: :exec -> :execrows
APIKeyRepository.SoftDelete: error -> (int64, error)
RotateAPIKey: n == 0 -> ErrAPIKeyNotFound (no IssueAPIKey)
DeleteAPIKey: still discards count (idempotent success unchanged)

Coverage (unchanged from #819): in-memory fake-repo race tests in internal/auth/rotate_race_test.go (concurrent Rotate → exactly one success; Rotate-vs-Delete → no zombie), plus live-Postgres repro scripts under scripts/{rotate_key_race_check,rotate_key_race_ext_check,upsert_external_key_race_check} (per the AGENTS.md rule that DB-backed checks live in scripts/, not internal/*_test.go).

make generate + make precommit green.

Co-authored-by: rohith500 rohithreddy2202@gmail.com

Link to Devin session: https://app.devin.ai/sessions/03fe47e3946a4829b431a2e2a35c1606
Requested by: @steventohme

Deterministic fake-repo tests for concurrent RotateAPIKey and Rotate-vs-Delete SoftDelete races. Both fail on current :exec SoftDelete behavior (loser still mints a successor). Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>

Switch SoftDeleteModelRouterAPIKey to :execrows and have RotateAPIKey bail with ErrAPIKeyNotFound when SoftDelete matches no row, closing the concurrent-rotate and rotate-vs-delete zombie-key races from #817. DeleteAPIKey keeps discarding the count for idempotent success. Also lands the DB-backed repro scripts referenced by #817. Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: N Rohith Reddy <rohithreddy2202@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: rohith500 <rohith500@users.noreply.github.com>

devin-ai-integration · 2026-07-23T13:07:13Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment, CI, and merge conflict monitoring

greptile-apps · 2026-07-23T13:07:14Z

PR author is not in the allowed authors list.

claude · 2026-07-23T13:07:25Z

Claude finished @devin-ai-integration[bot]'s task —— View job

🤖 Claude Code Review

Gather diff of Go changes
Analyze added comment lines for verbosity
Post review if issues found

Posted advisory comment-length nits on 2 blocks:

internal/auth/api_key.go lines 35–37 — 3-line interface godoc restates the return type; fits in one sentence.
scripts/rotate_key_race_check/main.go lines 83–86 — 4-line inline comment explaining timing; fits in two lines.

workweave-bot

Advisory only — comment-length nits. Won't block merge.

workweave-bot · 2026-07-23T13:09:41Z

 	ListForInstallation(ctx context.Context, installationID string) ([]*APIKey, error)
 	MarkUsed(ctx context.Context, id string) error
-	SoftDelete(ctx context.Context, installationID, id string) error
+	// SoftDelete soft-deletes the key when it is still active and returns the


Suggested change

ListForInstallation(ctx context.Context, installationID string) ([]*APIKey, error)

MarkUsed(ctx context.Context, id string) error

SoftDelete(ctx context.Context, installationID, id string) error

// SoftDelete soft-deletes the key when it is still active and returns the

// SoftDelete soft-deletes the key and returns the rows-affected count; 0 means the key was already gone.

SoftDelete(ctx context.Context, installationID, id string) (int64, error)

Was 3 lines; the return type already signals the count, the why fits in one sentence.

Applied — condensed to the one-line version. Thanks!

workweave-bot · 2026-07-23T13:09:41Z

+	// Hold every caller between ListForInstallation and SoftDelete so the
+	// TOCTOU window is observable. A start-only barrier is not enough: the
+	// List→SoftDelete critical section is microseconds, so one goroutine
+	// usually soft-deletes before the others even list (see earlier runs).


Suggested change

// Hold every caller between ListForInstallation and SoftDelete so the

// TOCTOU window is observable. A start-only barrier is not enough: the

// List→SoftDelete critical section is microseconds, so one goroutine

// usually soft-deletes before the others even list (see earlier runs).

// Hold every caller between List and SoftDelete to open the TOCTOU window;

// a start-only barrier misses it because List→SoftDelete is microseconds.

listHold := newListHold(concurrentRotations)

Was 4 lines; the timing rationale fits in two.

Applied — condensed the TOCTOU comment to two lines. Thanks!

claude · 2026-07-23T13:11:50Z

Claude finished @devin-ai-integration[bot]'s task —— View job

Claude finished @devin-ai-integration[bot]'s task — View job

🤖 Claude Code Review

Gather diff of Go changes
Analyze added comment lines for verbosity
Post review if issues found

Posted advisory comment-length nits on 2 blocks in internal/auth/rotate_race_test.go:

Lines 189–191 — 3-line comment before TestRotateAPIKey_ConcurrentRace_OnlyOneSucceeds; the function name already conveys the scenario, so the comment restates rather than adds WHY.
Lines 240–242 — 3-line comment before TestRotateAPIKey_LosingToDelete_DoesNotIssue; same issue — trim to the non-obvious constraint (no zombie on lost SoftDelete).

workweave-bot

Advisory only — comment-length nits. Won't block merge.

workweave-bot · 2026-07-23T13:15:20Z

+// TestRotateAPIKey_ConcurrentRace_OnlyOneSucceeds is the #817 unit repro:
+// two RotateAPIKey callers both observe the key as active, then SoftDelete;
+// exactly one must mint a successor, the other ErrAPIKeyNotFound.


Suggested change

// TestRotateAPIKey_ConcurrentRace_OnlyOneSucceeds is the #817 unit repro:

// two RotateAPIKey callers both observe the key as active, then SoftDelete;

// exactly one must mint a successor, the other ErrAPIKeyNotFound.

// TestRotateAPIKey_ConcurrentRace_OnlyOneSucceeds verifies the #817 fix:

// the losing racer must return ErrAPIKeyNotFound, not mint an orphan key.

Was 3 lines; the function name already conveys the scenario — trim to the non-obvious constraint.

Applied — trimmed to two lines focused on the constraint. Thanks!

workweave-bot · 2026-07-23T13:15:20Z

+// TestRotateAPIKey_LosingToDelete_DoesNotIssue is the #817 Delete-vs-Rotate
+// unit repro: Delete wins SoftDelete after Rotate has Listed; Rotate must
+// return ErrAPIKeyNotFound and must not issue a zombie replacement.


Suggested change

// TestRotateAPIKey_LosingToDelete_DoesNotIssue is the #817 Delete-vs-Rotate

// unit repro: Delete wins SoftDelete after Rotate has Listed; Rotate must

// return ErrAPIKeyNotFound and must not issue a zombie replacement.

// TestRotateAPIKey_LosingToDelete_DoesNotIssue verifies the #817 Delete-vs-Rotate

// fix: Rotate must return ErrAPIKeyNotFound and not mint a zombie successor.

Was 3 lines restating the function name; trim to the non-obvious constraint (no zombie on lost SoftDelete).

Applied — trimmed to two lines focused on the constraint. Thanks!

claude · 2026-07-23T13:16:49Z

Claude finished @devin-ai-integration[bot]'s task —— View job

🤖 Claude Code Review

Gather diff of Go changes
Analyze added comment lines for verbosity
Post review if issues found

Posted advisory comment-length nits on 2 blocks:

internal/auth/rotate_race_test.go lines 17–20 — 4-line struct godoc restates what the SoftDelete method makes obvious; fits in two lines.
scripts/rotate_key_race_check/main.go lines 10–16 — 7-line timing note; the pool-limiter and :exec path sentences are filler — the TOCTOU insight fits in three lines.

View job

workweave-bot

Advisory only — comment-length nits. Won't block merge.

workweave-bot · 2026-07-23T13:19:09Z

+// statefulAPIKeyRepo is an in-memory APIKeyRepository that models real
+// SoftDelete semantics: matching active row → soft-delete; already-deleted
+// or missing → silent no-op (0 rows). Used to exercise RotateAPIKey races
+// without Postgres (see #817).


Suggested change

// statefulAPIKeyRepo is an in-memory APIKeyRepository that models real

// SoftDelete semantics: matching active row → soft-delete; already-deleted

// or missing → silent no-op (0 rows). Used to exercise RotateAPIKey races

// without Postgres (see #817).

// statefulAPIKeyRepo is an in-memory APIKeyRepository whose SoftDelete

// counts rows affected (0 when already gone), matching live Postgres semantics.

Was 4 lines; the SoftDelete method itself shows the semantics, and the purpose is clear from the file.

Applied — condensed to two lines. Thanks!

workweave-bot · 2026-07-23T13:19:09Z

+// Timing note: a start-only barrier (line up, then release into RotateAPIKey)
+// does not hit the race in practice — List→SoftDelete is microseconds, so one
+// goroutine soft-deletes before the others list (they then get
+// ErrAPIKeyNotFound). The pool is not the limiter (peak acquired == N). A
+// second barrier inside ListForInstallation holds every caller after the
+// pre-delete read and before SoftDelete, which is the actual TOCTOU window.
+// SoftDelete stays the real :exec SQLC path (silent no-op on 0 rows).


Suggested change

// Timing note: a start-only barrier (line up, then release into RotateAPIKey)

// does not hit the race in practice — List→SoftDelete is microseconds, so one

// goroutine soft-deletes before the others list (they then get

// ErrAPIKeyNotFound). The pool is not the limiter (peak acquired == N). A

// second barrier inside ListForInstallation holds every caller after the

// pre-delete read and before SoftDelete, which is the actual TOCTOU window.

// SoftDelete stays the real :exec SQLC path (silent no-op on 0 rows).

// Timing note: a start-only barrier misses the race — List→SoftDelete is

// microseconds, so one goroutine soft-deletes before the others List. A second

// barrier inside ListForInstallation holds every caller at the TOCTOU window.

Was 7 lines; the pool note and SoftDelete path note are filler — the TOCTOU insight fits in three.

Applied — condensed the timing note to three lines. Thanks!

claude · 2026-07-23T13:20:53Z

Claude finished @devin-ai-integration[bot]'s task —— View job

🤖 Claude Code Review

Gather diff of Go changes
Analyze added comment lines for verbosity
Post review if issues found

Posted advisory comment-length nits on 3 blocks:

scripts/rotate_key_race_check/main.go lines 1–17 — Package doc has a filler paragraph (lines 6–8) explaining "It is a separate main package (not a _test.go)…" — the scripts/ path and the if dsn == "" guard already communicate this; drop to 13 lines.
scripts/rotate_key_race_check/main.go lines 73–75 — 3-line func doc narrates implementation steps; fits in two.
scripts/rotate_key_race_ext_check/main.go lines 176–178 — 3-line func doc; "Expected buggy outcome" sentence is filler — two lines capture the constraint.

workweave-bot

Advisory only — comment-length nits. Won't block merge.

workweave-bot · 2026-07-23T13:23:33Z

+// Command rotate_key_race_check reproduces a concurrent RotateAPIKey race
+// against a live Postgres: N goroutines soft-delete + re-issue the same key
+// with no transaction / rows-affected check, so more than one successor key
+// can end up live from a single rotation intent.
+//
+// It is a separate main package (not a _test.go), so `go test ./...` never
+// touches Postgres. It is gated on ROUTER_TEST_DATABASE_URL (a DSN to a
+// database with the router migrations applied) and is a no-op without it.
+//
+// Timing note: a start-only barrier misses the race — List→SoftDelete is
+// microseconds, so one goroutine soft-deletes before the others List. A second
+// barrier inside ListForInstallation holds every caller at the TOCTOU window.
+//
+// Usage (from the repo root, against the docker-compose Postgres):
+//
+//	ROUTER_TEST_DATABASE_URL="postgres://router:router@localhost:5433/router?search_path=router" \
+//	    go run ./scripts/rotate_key_race_check


Suggested change

// Command rotate_key_race_check reproduces a concurrent RotateAPIKey race

// against a live Postgres: N goroutines soft-delete + re-issue the same key

// with no transaction / rows-affected check, so more than one successor key

// can end up live from a single rotation intent.

//

// It is a separate main package (not a _test.go), so `go test ./...` never

// touches Postgres. It is gated on ROUTER_TEST_DATABASE_URL (a DSN to a

// database with the router migrations applied) and is a no-op without it.

//

// Timing note: a start-only barrier misses the race — List→SoftDelete is

// microseconds, so one goroutine soft-deletes before the others List. A second

// barrier inside ListForInstallation holds every caller at the TOCTOU window.

//

// Usage (from the repo root, against the docker-compose Postgres):

//

// ROUTER_TEST_DATABASE_URL="postgres://router:router@localhost:5433/router?search_path=router" \

// go run ./scripts/rotate_key_race_check

// Command rotate_key_race_check reproduces a concurrent RotateAPIKey race

// against a live Postgres: N goroutines soft-delete + re-issue the same key

// with no transaction / rows-affected check, so more than one successor key

// can end up live from a single rotation intent.

//

// Timing note: a start-only barrier misses the race — List→SoftDelete is

// microseconds, so one goroutine soft-deletes before the others List. A second

// barrier inside ListForInstallation holds every caller at the TOCTOU window.

//

// Usage (from the repo root, against the docker-compose Postgres):

//

// ROUTER_TEST_DATABASE_URL="postgres://router:router@localhost:5433/router?search_path=router" \

// go run ./scripts/rotate_key_race_check

Was 17 lines; the "separate main package / not a _test.go" paragraph restates what the scripts/ path and the if dsn == "" guard already communicate.

Leaving this one as-is. The "separate main package / not a _test.go" note documents an intentional AGENTS.md invariant (DB-backed checks live under scripts/, never as internal/*_test.go), which isn't obvious from the path alone — worth keeping for the next reader. These are advisory nits and the PR already meets the documented comment convention with CI green, so I'll stop here rather than keep churning the doc comments. Thanks!

workweave-bot · 2026-07-23T13:23:33Z

+// checkRotateKeyRace creates a fresh installation + one API key, then fires
+// concurrentRotations barrier-synchronized RotateAPIKey calls on that same
+// keyID and reports how many succeeded vs how many active keys remain.


Suggested change

// checkRotateKeyRace creates a fresh installation + one API key, then fires

// concurrentRotations barrier-synchronized RotateAPIKey calls on that same

// keyID and reports how many succeeded vs how many active keys remain.

// checkRotateKeyRace seeds one key, fires concurrentRotations barrier-held

// RotateAPIKey calls, and reports successes vs. active-key count.

Was 3 lines narrating implementation steps; fits in two.

Leaving as-is — this is a reasonable, accurate function godoc. Advisory nit only; stopping here to avoid further comment churn now that the PR meets the convention and CI is green. Thanks!

workweave-bot · 2026-07-23T13:23:33Z

+// checkRotateVsDelete forces DeleteAPIKey to win SoftDelete after Rotate has
+// already Listed the key as active. Expected buggy outcome: Rotate still
+// IssueAPIKeys a live successor even though the key was deleted (not rotated).


Suggested change

// checkRotateVsDelete forces DeleteAPIKey to win SoftDelete after Rotate has

// already Listed the key as active. Expected buggy outcome: Rotate still

// IssueAPIKeys a live successor even though the key was deleted (not rotated).

// checkRotateVsDelete forces DeleteAPIKey to win SoftDelete after Rotate has

// already listed the key as active, then counts zombie successors minted.

Was 3 lines; "Expected buggy outcome" sentence is filler — two lines capture the constraint.

Leaving as-is — the "expected buggy outcome" line documents what the check is asserting against (the pre-fix zombie-successor behavior), which is the point of the repro. Advisory nit only; stopping here to avoid churn. Thanks!

rohith500 and others added 3 commits July 22, 2026 21:06

fix(auth): reject RotateAPIKey when SoftDelete affects 0 rows

f27834c

Co-authored-by: rohith500 <rohith500@users.noreply.github.com>

devin-ai-integration Bot mentioned this pull request Jul 23, 2026

fix(auth): reject RotateAPIKey when SoftDelete affects 0 rows #819

Open

3 tasks

workweave-bot reviewed Jul 23, 2026

View reviewed changes

docs(auth): tighten SoftDelete comments per review

5eed997

workweave-bot reviewed Jul 23, 2026

View reviewed changes

docs(auth): tighten race-test godocs per review

9caf45d

workweave-bot reviewed Jul 23, 2026

View reviewed changes

docs: tighten repo/script comments per review

e537c00

workweave-bot reviewed Jul 23, 2026

View reviewed changes

Uh oh!

Conversation

devin-ai-integration Bot commented Jul 23, 2026

Summary

Uh oh!

devin-ai-integration Bot commented Jul 23, 2026

🤖 Devin AI Engineer

Uh oh!

greptile-apps Bot commented Jul 23, 2026

Uh oh!

claude Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Claude Code Review

Uh oh!

workweave-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Claude Code Review

Uh oh!

workweave-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Claude Code Review

Uh oh!

workweave-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Claude Code Review

Uh oh!

workweave-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 23, 2026 •

edited

Loading

claude Bot commented Jul 23, 2026 •

edited

Loading

claude Bot commented Jul 23, 2026 •

edited

Loading

claude Bot commented Jul 23, 2026 •

edited

Loading