arcodange/dance-lessons-coach
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

✨ feat(auth): JWT secret retention policy + automatic cleanup loop (ADR-0021) #41

Merged
arcodange merged 1 commits from feat/adr-0021-jwt-secret-cleanup into main 2026-05-05 08:40:28 +02:00
Conversation 0 Commits 1 Files Changed 8 +319 -36
arcodange commented 2026-05-05 08:40:21 +02:00
Owner
Copy Link

Sprint 3 of autonomous trainer day 2026-05-05

Implements the cleanup half of ADR-0021 — the config infrastructure (TTL, retention factor, max retention, cleanup interval) had been merged earlier without the actual cleanup logic. This PR closes that gap.

What ships

Core mechanism

  • RemoveExpiredSecrets() int on JWTSecretManager — drops every non-primary secret whose ExpiresAt is non-nil and in the past. Returns count of removed. Primary secret is never removed regardless of expiration (ADR-0021 invariant).
  • StartCleanupLoop(ctx, interval) — spawns a goroutine running RemoveExpiredSecrets at the configured interval. Stops cleanly when ctx is cancelled.
  • All struct operations are now mutex-protected (sync.Mutex). Race detector clean.

Wiring

  • pkg/server/server.go Run() calls StartJWTSecretCleanupLoop(rootCtx, ...) after the signal context is set up — the loop stops on graceful shutdown.
  • AuthService interface extended with StartJWTSecretCleanupLoop and RemoveExpiredJWTSecrets.

Status update

  • ADR-0021 → Implemented. Heading also corrected from 10. to 21. (was a doc error from earlier commit).

Tests

  • 4 new unit tests in pkg/user/jwt_manager_test.go:
    • TestRemoveExpiredSecrets_ExpiredNonPrimaryRemoved
    • TestRemoveExpiredSecrets_PrimaryNeverRemoved
    • TestRemoveExpiredSecrets_NonExpiredKept
    • TestStartCleanupLoop_FiresAndStops (drives the goroutine end-to-end with low TTL)
  • go test -race ./pkg/user/... passes.
  • Full BDD suite (auth/config/greet/health/info/jwt) still green.
  • BDD scenarios tagged @todo / @skip in features/jwt/jwt_secret_retention.feature remain so — they reference an admin endpoint /api/v1/admin/jwt/secrets that is explicitly out of scope for this PR.

Verifier verdict (skill-driven)

APPROVE_WITH_NITS

  • Dim A (code quality): APPROVE_WITH_NITS — StartCleanupLoop is 34 lines (just over the 30-line guideline; could be split into setupLoopCtx + runTickerLoop helpers but not critical).
  • Dim B (docs homogeneity): APPROVE — ADR-0021 numbering fixed (was 10., now 21.); status accurately reflects what landed; out-of-scope items explicitly documented.
  • Dim C (test reliability): APPROVE — race detector clean, behavior coverage (positive + invariant + negative + goroutine), time.Sleep uses are justified by the goroutine-timing nature of TestStartCleanupLoop_FiresAndStops (low TTL keeps the test < 100ms).

Out of scope (deferred)

  • Admin endpoint /api/v1/admin/jwt/secrets — referenced by @todo BDD scenarios but no concrete need yet. Reopen when ops require introspection.
  • Cleanup metrics (count emitted to /metrics) — log-based observability for now (log.Info().Int("removed", n)).
  • Manual cleanup endpoint — RemoveExpiredJWTSecrets() is exposed on the service but not as an HTTP endpoint.

Test plan

  • go test -race ./pkg/user/... passes
  • Full BDD suite passes
  • go vet ./... clean
  • Pre-commit hooks (go mod tidy + go fmt + swag fmt) successful
  • Reviewer to verify the loop actually runs in production (start the server, check logs after 1h)
## Sprint 3 of autonomous trainer day 2026-05-05 Implements the cleanup half of [ADR-0021](adr/0021-jwt-secret-retention-policy.md) — the config infrastructure (TTL, retention factor, max retention, cleanup interval) had been merged earlier without the actual cleanup logic. This PR closes that gap. ## What ships ### Core mechanism - **`RemoveExpiredSecrets() int`** on `JWTSecretManager` — drops every non-primary secret whose `ExpiresAt` is non-nil and in the past. Returns count of removed. **Primary secret is never removed** regardless of expiration (ADR-0021 invariant). - **`StartCleanupLoop(ctx, interval)`** — spawns a goroutine running `RemoveExpiredSecrets` at the configured interval. Stops cleanly when `ctx` is cancelled. - All struct operations are now mutex-protected (`sync.Mutex`). Race detector clean. ### Wiring - `pkg/server/server.go` `Run()` calls `StartJWTSecretCleanupLoop(rootCtx, ...)` after the signal context is set up — the loop stops on graceful shutdown. - `AuthService` interface extended with `StartJWTSecretCleanupLoop` and `RemoveExpiredJWTSecrets`. ### Status update - ADR-0021 → `Implemented`. Heading also corrected from `10.` to `21.` (was a doc error from earlier commit). ## Tests - 4 new unit tests in `pkg/user/jwt_manager_test.go`: - `TestRemoveExpiredSecrets_ExpiredNonPrimaryRemoved` - `TestRemoveExpiredSecrets_PrimaryNeverRemoved` - `TestRemoveExpiredSecrets_NonExpiredKept` - `TestStartCleanupLoop_FiresAndStops` (drives the goroutine end-to-end with low TTL) - `go test -race ./pkg/user/...` passes. - Full BDD suite (auth/config/greet/health/info/jwt) still green. - BDD scenarios tagged `@todo` / `@skip` in `features/jwt/jwt_secret_retention.feature` remain so — they reference an admin endpoint `/api/v1/admin/jwt/secrets` that is **explicitly out of scope** for this PR. ## Verifier verdict (skill-driven) **APPROVE_WITH_NITS** - **Dim A** (code quality): APPROVE_WITH_NITS — `StartCleanupLoop` is 34 lines (just over the 30-line guideline; could be split into `setupLoopCtx` + `runTickerLoop` helpers but not critical). - **Dim B** (docs homogeneity): APPROVE — ADR-0021 numbering fixed (was `10.`, now `21.`); status accurately reflects what landed; out-of-scope items explicitly documented. - **Dim C** (test reliability): APPROVE — race detector clean, behavior coverage (positive + invariant + negative + goroutine), `time.Sleep` uses are justified by the goroutine-timing nature of `TestStartCleanupLoop_FiresAndStops` (low TTL keeps the test < 100ms). ## Out of scope (deferred) - **Admin endpoint `/api/v1/admin/jwt/secrets`** — referenced by `@todo` BDD scenarios but no concrete need yet. Reopen when ops require introspection. - **Cleanup metrics** (count emitted to /metrics) — log-based observability for now (`log.Info().Int("removed", n)`). - **Manual cleanup endpoint** — `RemoveExpiredJWTSecrets()` is exposed on the service but not as an HTTP endpoint. ## Test plan - [x] `go test -race ./pkg/user/...` passes - [x] Full BDD suite passes - [x] `go vet ./...` clean - [x] Pre-commit hooks (go mod tidy + go fmt + swag fmt) successful - [ ] Reviewer to verify the loop actually runs in production (start the server, check logs after 1h)
arcodange added 1 commit 2026-05-05 08:40:22 +02:00
✨ feat(auth): JWT secret retention policy + automatic cleanup loop (ADR-0021) 8d62cb0125 
Implements the cleanup half of ADR-0021 (which had only config infrastructure
landed). Non-primary expired secrets are removed by a goroutine that runs at
auth.jwt.secret_retention.cleanup_interval (default 1h). Primary secret is
never removed regardless of expiration — invariant preserved.

Changes:
- pkg/user/jwt_manager.go : add sync.Mutex protection; add
  RemoveExpiredSecrets() int and StartCleanupLoop(ctx, interval) methods.
  Reset() now also cancels any running cleanup goroutine.
- pkg/user/auth_service.go : delegate to manager via new AuthService methods
  StartJWTSecretCleanupLoop and RemoveExpiredJWTSecrets.
- pkg/user/user.go : extend AuthService interface accordingly.
- pkg/server/server.go Run() : start cleanup loop tied to rootCtx so it
  stops on graceful shutdown.
- pkg/jwt/* : same treatment on the secondary (less-used) implementation
  for consistency.
- adr/0021-jwt-secret-retention-policy.md : Status → Implemented + fix
  numbering (was incorrectly "10.").

Tests:
- 4 new unit tests in pkg/user/jwt_manager_test.go covering
  RemoveExpiredSecrets (expired removed, primary preserved, future kept)
  and StartCleanupLoop (fires + stops on context cancel).
- go test -race ./pkg/user/... passes.
- Full BDD suite (auth/config/greet/health/info/jwt) still green.
- BDD scenarios at @todo / @skip remain so — they require an admin
  endpoint /api/v1/admin/jwt/secrets which is explicitly out of scope.

Verifier verdict: APPROVE_WITH_NITS — StartCleanupLoop is 34 lines (just
over the 30-line guideline); 2 time.Sleeps in TestStartCleanupLoop_FiresAndStops
are justified by the goroutine-timing nature of the test.
arcodange merged commit 03ea2a7b89 into main 2026-05-05 08:40:28 +02:00
arcodange deleted branch feat/adr-0021-jwt-secret-cleanup 2026-05-05 08:40:28 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
arcodange
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: arcodange/dance-lessons-coach#41
Delete Branch "feat/adr-0021-jwt-secret-cleanup"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?