[tgrunnagle]

Summary

/oauth/register is unauthenticated, and every successful call persists a client row. On Redis nothing capped those rows, and a client that was never used lived 30 days, so the row count was limited only by rate × TTL: about 2.6M rows per replica. The memory store did have a cap, but enforced it by evicting idle clients, so anyone registering at the full rate could push out real clients and break their refresh tokens without the operator seeing it. This PR limits how much open registration can cost on both backends without evicting anyone.

• Hard cap, no eviction, on both backends. Only DCR-issued clients count; operator-configured and CIMD clients don't. At the cap, RegisterClient returns ErrClientCapacity and writes nothing. The 503 Retry-After is the number of seconds until the oldest counted client expires. On Redis, a sorted set of DCR client IDs scored by expiry time is the count, and it lives under the same hash-tagged prefix as the client rows. One Lua script trims expired entries, checks the cap, and writes the row together with its index entry, so the cap holds across replicas. A second script renews a row's TTL and its index score together, and ReconcileConfiguredClient drops any stale index entry for its ID in the same WATCH/MULTI transaction as the row. UpsertDCRIssuedClient (the CIMD path) refuses counted clients, so it can't bypass the cap. Redis storage has no client-delete path, so there is nothing to keep in step there.
• Upgrade backfill. At startup, a migration adds existing DCR rows to the index with their current TTL so they count against the cap. If the backfill fails, the server logs a WARN and starts anyway; if existing clients already fill the cap, it logs a WARN naming storage.max_dcr_clients.
• Unused clients expire early. A newly registered client gets DefaultUnusedDCRClientTTL (24h). The existing RenewClientTTL call on the first successful token exchange raises it to 30 days. This lowers the worst case to about 86,400 rows per replica. The memory store now uses the same expiry rules, so a full map drains on its own. minClientAge and DefaultMinClientAge are removed.
• Configurable register rate. registration_rate_limit (requests_per_second, burst) replaces the hardcoded 1/s burst 5, which stays the default.
• Visibility. A live-client gauge, read at collection time rather than on the request path, and a rejection counter. Rejections are also logged at WARN, at most once every 15 minutes, with the cap, the live count, Retry-After and the number of rejections held back. The volume is bounded by time rather than by the unauthenticated caller.

Closes #6861

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)

• E2E tests (task test-e2e)

• Linting (task lint-fix)

• Manual testing (describe below)

• Unit tests: full task test passed except pkg/container/docker TestCreateNetwork_ConcurrentSameName. That test is unrelated and fails locally because Docker reports "all predefined address pools have been fully subnetted". CI's Tests / Test Go Code job passes on 040b16a, along with every other check, including the E2E and operator suites.

• Linting: task lint passes.

• Integration: ran the integration-tagged Redis tests in pkg/authserver/storage/redis_dcr_cap_integration_test.go locally against redis:7-alpine (Sentinel, via testcontainers), on the current scripts. TestIntegration_DCRCap passes all four subtests: rejection at the cap with no row written, two storage instances racing never exceeding the cap, TTL and index score following registration and first use, and backfill. Slot release on expiry is covered by the miniredis unit tests (TestRedisStorage_DCRCap_FreesOnExpiry), not the integration tests. No CI workflow runs task test-integration for this package, so this local run is the only one against real Redis.

• Unrelated failure in the same package: TestIntegration_ClientAssertionJWT/known_JTI_is_invalid fails for a reason that has nothing to do with this PR: the test, which this PR does not touch, sets a 1h assertion lifetime, and Add private_key_jwt DCR client authentication #6427 capped lifetimes at 5m.

Changes

File	Change
pkg/authserver/storage/redis.go, redis_keys.go	DCR index sorted set; Lua scripts for capped register and renew; index maintenance in ReconcileConfiguredClient; UpsertDCRIssuedClient refuses counted clients; CountDCRClients
pkg/authserver/storage/redis_migrate.go	Startup backfill of existing DCR rows into the index
pkg/authserver/storage/memory.go, cimd_decorator.go	Removes eviction; reject-only DCR cap; separate cap on CIMD rows; DCR expiry; CountDCRClients
pkg/authserver/storage/dcr_cap.go, types.go, config.go	ClientCapacityError carrying Retry-After, DCRClientCounter, new TTL/cap defaults, max_dcr_clients
pkg/authserver/server/handlers/dcr.go, handler.go, token.go	Cap-derived Retry-After, configurable register limiter, metrics wiring
pkg/authserver/server/handlers/dcr_metrics.go, dcr_capacity_warner.go	Live-client gauge, capacity rejection counter, and the time-bounded cap WARN
pkg/authserver/config.go, server_impl.go, runner/embeddedauthserver.go	registration_rate_limit config, validation, and plumbing
docs/arch/11-auth-server-storage.md, docs/redis-storage.md, docs/observability.md	Cap, sizing, index, backfill, upgrade notes, TTL, metrics and alerting documentation
docs/server/*, sdk/go/*	Generated by task docs
*_test.go	Unit tests for the handler, config and both backends; Redis integration tests

Does this introduce a user-facing change?

Yes.

• Redis: DCR clients are now capped (release note). Before: no limit on DCR clients. After: capped at 10,000 by default, set with storage.max_dcr_clients (0 = unlimited). At the cap, /oauth/register returns 503 with server_error and a Retry-After equal to the seconds until the oldest counted client expires. A deployment that already holds more than 10,000 live DCR clients will reject new registrations right after upgrading until the count drops below the cap. To avoid this, raise max_dcr_clients or set it to 0 before upgrading. The operator CRD fields come in a stacked PR for Expose DCR client cap and register rate limit in the operator CRD #6864, planned for the same release. At startup, the server logs a WARN when existing clients already fill the cap. Backfilled rows keep their current TTL of up to 30 days. No Redis ACL change is needed: the scripts use only commands the documented ACL already grants.
• Memory store at the cap. Before: evicted the least-recently-used DCR client older than 1h, and returned 503 with a fixed Retry-After: 3600 only when no client was old enough. After: always rejects without evicting, and Retry-After is the time until the oldest counted client expires. CIMD rows are no longer counted against that cap; they get their own cap of 10,000 instead, and past it the best-effort CIMD write-through skips the row.
• Unused DCR client lifetime. Before: 30 days whether or not the client was used. After: 24h until the first successful token exchange, which raises the TTL to 30 days, renewed on each later exchange as before.
• Register rate limit. Before: hardcoded 1/s with a burst of 5. After: configurable with registration_rate_limit (requests_per_second, burst); unset keeps 1/s burst 5. The limiter is per process, so the effective rate is the configured rate × the replica count. The 429 Retry-After is now ceil(1/rate) instead of a fixed 1, which gives the same value at the default rate.
• New metrics. Gauge toolhive_authserver_dcr_clients and counter toolhive_authserver_dcr_capacity_rejections.
• New WARN logs. DCR client cap reached; rejecting new client registrations, at most once every 15 minutes while registrations are being rejected. On Redis, also one line at startup when the backfilled count is already at or above the cap, and one if the backfill fails.

Special notes for reviewers

• PR size. This PR is over the 400-line / 10-file guideline because the user chose one PR for the whole issue. A good share of the diff is tests and generated files (docs/server/* and sdk/go/*, from task docs).
• CIMD clients are not counted against the DCR cap, as the issue requires. On the memory backend they get a separate cap of 10,000 rows, replacing the bound the old whole-map cap used to give them. On Redis, CIMD rows are still bounded only by the per-process CIMD authorize rate limit and their 30-day TTL.
• Index scores use the replica clock, passed to the scripts as an argument, rather than Redis TIME. TIME is in the @fast ACL category, which the documented auth-server ACL doesn't grant, so using it would break ACL-restricted deployments on upgrade. Clock skew between replicas only shifts when an index entry is trimmed relative to its row's expiry.
• Backfill. It runs on every startup, and on Redis Cluster it scans the master that owns the slot. It skips rows it can't parse and logs one WARN line for them. A Redis error fails only the backfill, not startup: skipping it just leaves the cap undercounting older rows until the next restart, which is how the server behaved before the cap. The legacy upstream migrations stay fatal. It logs one INFO line with the rows scanned, the clients added, and the duration. Rows that old replicas write during a rolling upgrade stay uncounted until the next restart or until they expire.
• UpsertDCRIssuedClient refuses counted clients on both backends. Its only caller is the CIMD decorator, and counted clients have to go through RegisterClient, which enforces the cap.
• Follow-ups: operator CRD fields for max_dcr_clients and registration_rate_limit (Expose DCR client cap and register rate limit in the operator CRD #6864), as a stacked PR for the same release; the same Redis Cluster SCAN flaw in the existing legacy upstream migrations in redis_migrate.go (not filed).
• Two internal code-review passes ran. The first found 1 high, 4 medium and 1 low issue; the second found 2 low issues. All are fixed. All 18 findings from the multi-agent review are addressed in 5fb2bf9, d98fe26 and b25181b. The follow-up CI failures (SDK schema count, codespell) are fixed in fae8ff0. The operational review's backfill and logging notes are addressed in f6781fc and 040b16a.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 92.82051% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.73%. Comparing base (8aaed56) to head (ca782e8).

Files with missing lines	Patch %	Lines
pkg/authserver/storage/redis_migrate.go	88.76%	10 Missing ⚠️
pkg/authserver/storage/redis.go	85.96%	8 Missing ⚠️
pkg/authserver/server/handlers/dcr_metrics.go	85.29%	5 Missing ⚠️
pkg/authserver/storage/cimd_decorator.go	50.00%	2 Missing ⚠️
pkg/authserver/server/handlers/handler.go	97.56%	1 Missing ⚠️
pkg/authserver/server_impl.go	90.90%	1 Missing ⚠️
pkg/authserver/storage/memory.go	98.80%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6863      +/-   ##
==========================================
+ Coverage   79.70%   79.73%   +0.03%     
==========================================
  Files         822      825       +3     
  Lines       83843    84154     +311     
==========================================
+ Hits        66824    67102     +278     
- Misses      17014    17047      +33     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: concurrency-redis, security, memory-store, config-docs, test-coverage, review-friendliness, general

Consensus Summary

#	Finding	Consensus	Severity	Action
1	New TIME calls are outside the documented Redis ACL	8/10	HIGH	Fix
2	Operator cannot set max_dcr_clients / registration_rate_limit	9/10	MEDIUM	Fix or document
3	In-memory backend loses its only bound on CIMD rows	8/10	MEDIUM	Discuss
4	Sizing relationship and default-config lockout undocumented	9/10	MEDIUM	Fix
5	Upgrade behaviour only in the PR body, not in docs	8/10	MEDIUM	Fix
6	No test that startup runs the backfill	7/10	MEDIUM	Fix
7	No test that registration_rate_limit reaches the handler	7/10	MEDIUM	Fix
8	UpsertDCRIssuedClient counted branch unreachable, bypasses cap, extra TIME	9/10	LOW	Fix
9	Unchecked type assertions on the register script reply	7/10	LOW	Fix
10	Duplicate-ID vs at-capacity precedence differs between backends	8/10	LOW	Fix
11	Register limiter accepts NaN/Inf; 429 Retry-After doc says exact	8/10	LOW	Fix
12	No test pins index/row hash-slot co-location	8/10	LOW	Fix
13	Exact Retry-After test is clock-dependent; gauge untested via decorators	8/10	LOW	Fix
14	Lock-held O(n) scan, stale comment, partial clock injection (memory)	7/10	LOW	Fix
15	Cap options treat negative values as unlimited	7/10	LOW	Fix
16	Redis deployment requirements for the index undocumented	7/10	LOW	Fix
17	New metrics missing from docs/observability.md	7/10	LOW	Fix
18	PR body overstates the Redis paths (see below; no inline comment)	8/10	LOW	Fix

Rows 1-17 have an inline comment. Row 18 is described under PR description below.

Overall

This PR bounds open DCR registration without evicting anyone: a hash-tagged sorted-set index, a single Lua script that trims, checks the cap and writes the row plus its index entry, scores taken from Redis TIME so replica clock skew cannot desync the index from key expiry, and matching expiry rules on the memory store. The Redis atomicity, Cluster key co-location, unit handling (ms scores against PX), renew/replace/reconcile index maintenance and backfill idempotency all held up under review, and every acceptance criterion in the linked issue has an implementation and at least one test.

One finding should block merge: the new server-side TIME calls are not covered by the ACL that docs/redis-storage.md and the operator examples tell users to create, so on an ACL-restricted deployment registration, renewal and the startup backfill would fail (row 1). The remaining findings are mostly gaps around the change rather than in it: the operator cannot set the new knobs, so the documented upgrade remedy is unavailable there (row 2); the sizing formula and the 24h default-config lockout the issue thread asked to document are missing (rows 4 and 5); two wiring links, the startup backfill call and the registration_rate_limit plumbing, have no test (rows 6 and 7); and the memory store's CIMD rows lose their only bound with no linked follow-up (row 3).

The integration-tagged Redis tests have not been run, per the PR description, and they use unrestricted users, so they would not have caught row 1 either. They should pass in CI before merge.

PR description

The Summary and Changes table say there are "Lua scripts for capped register, renew, replace and delete" and that "RenewClientTTL, the replace path, and delete all keep the index in step". In the diff only register, renew and count are Lua scripts; the replace path (UpsertDCRIssuedClient) and ReconcileConfiguredClient are WATCH/MULTI transactions, and RedisStorage has no client-delete path, so that requirement is vacuous rather than implemented. Correcting the wording avoids reviewers hunting for scripts and a delete path that are not there. The PR also links the CIMD-bounding and operator-CRD follow-ups only in prose; tracking issues would make those accepted trade-offs checkable.

Documentation

• docs/arch/11-auth-server-storage.md:308 and :322: sizing formula, default-config lockout, 30-day promotion and drain caveats, and upgrade notes (rows 4 and 5).
• docs/redis-storage.md:255,263 and examples/operator/redis-storage/*.yaml: ACL needs +@fast or +time (row 1); :334: hash-tag and eviction requirements (row 16).
• docs/observability.md: add the two new auth-server metrics (row 17).

---

Generated with Claude Code

[tgrunnagle]

Review row 18 (PR description): fixed. The body now describes the Redis paths as they are. Only register and renew are Lua scripts. ReconcileConfiguredClient maintains the index inside its WATCH/MULTI transaction. UpsertDCRIssuedClient refuses counted clients. There's no client-delete path. The operator CRD follow-up is tracked in #6864, and the in-memory CIMD bound is now implemented in d98fe26 rather than left as a follow-up.

[tgrunnagle]

I checked the follow-up commits (5fb2bf9, d98fe26, b25181b) against each finding in the earlier review and resolved all 17 threads. Highlights:

• TIME / ACL: the scripts now take now as an argument, and the commands they run (EXISTS, SET, ZADD, ZREMRANGEBYSCORE, ZCARD, ZRANGE, ZCOUNT, PEXPIRE) fall within the documented ACL categories, so no ACL change is needed.
• CIMD rows on the memory store have their own cap. The decorator keeps resolving the client from its cache when a row is refused.
• Redis register script: it checks for an existing row before the cap, matching the memory store.
• New tests cover the startup backfill, registration_rate_limit reaching the endpoint, the index/row hash-tag layout, NaN/Inf limiter rejection, the CIMD cap, and the decorator-wrapped gauge.
• Docs: the sizing formula, the upgrade notes, and the observability and Redis deployment sections are in place and match the code.

Still open before merge:

1. CI: Go SDK / Verify generated Go SDK fails. sdk/go/client/client_test.go:225 asserts 202 schemas. The PR adds one (AuthenticationRegistrationRateLimitRunConfig), so the count should be 203.
2. CI: Codespell fails. It flags unparseable (suggesting unparsable) at pkg/authserver/storage/redis_migrate.go:418 and :489.
3. The current Redis scripts have not run against a real Redis. The only manual redis-cli check predates the TIME removal and the EXISTS-before-cap change, so today only miniredis covers them. The Redis integration tests (redis_dcr_cap_integration_test.go) run via task test-integration. I couldn't find a workflow that runs that task (the integration jobs under .github are operator-only), so they may only run if someone runs them by hand. Please run the DCR cap integration tests on a machine with working Docker and note the result here.

The lint, unit-test, swagger and codegen checks were still running when I looked, so I haven't seen their results.

Generated with Claude Code

[tgrunnagle]

Replying to #6863 (comment):

1. SDK schema count: addressed in fae8ff0 (202 → 203).
2. Codespell: addressed in fae8ff0. redis_migrate.go:418 and :489 now say "unparsable". I also renamed the matching test to TestRedisStorage_BackfillDCRClientIndex_SkipsUnparsableRows.
3. Current scripts against real Redis: I ran them locally against redis:7-alpine (Sentinel via testcontainers):

   go test -race -tags integration -count=1 -run 'TestIntegration_DCRCap' -v ./pkg/authserver/storage/
   --- PASS: TestIntegration_DCRCap
       --- PASS: .../TTL_and_index_score_follow_registration_and_first_use
       --- PASS: .../rejects_at_the_cap_without_writing_a_row
       --- PASS: .../concurrent_replicas_never_exceed_the_cap
       --- PASS: .../backfill_indexes_rows_written_before_the_index
   

   I ran a targeted go test instead of task test-integration because the task runs ./.... One other test in the package fails, TestIntegration_ClientAssertionJWT/known_JTI_is_invalid ("assertion lifetime exceeds maximum of 5m0s"), and it has nothing to do with this PR: the test sets a 1h assertion lifetime, and Add private_key_jwt DCR client authentication #6427 added the 5m cap. The PR body's test plan was also wrong to say the integration tests cover slot release on expiry. Only the miniredis unit tests cover it, and I've corrected the body.

Generated with Claude Code

[jhrozek]

A few operational notes on the cap, the backfill and the upgrade path. None of them block this from my side.

[jhrozek]

Thanks for the quick turnaround on the operational notes. The cap, the backfill and the upgrade path all look good to me.