authserver/storage: add UpdateDCRCredentialsIfPresent

[tgrunnagle]

Summary

DCRCredentialStore only exposed GetDCRCredentials and the create-only
StoreDCRCredentialsIfAbsent, leaving no supported way to rewrite an existing
record in place. Calling StoreDCRCredentialsIfAbsent on a key that already
exists is a no-op that silently discards the caller's changes and returns the
old value. This blocks a storage decorator that needs to rewrite a DCR
credential's persisted representation (re-encoding, compression, a checksum,
etc.) without changing its RFC 7591 identity or values — it can Get a record
but has nowhere to write the modified copy back.

This PR adds a CAS-style update method, symmetric with the existing IfAbsent
naming, and implements it in both shipped backends:

• Add UpdateDCRCredentialsIfPresent(ctx, creds) to the DCRCredentialStore
  interface: it replaces the record at creds.Key iff one currently exists,
  and returns a wrapped ErrNotFound otherwise — it never creates.
• Implement it for MemoryStorage as a presence-gated map overwrite that
  stores a defensive copy.
• Implement it for RedisStorage as a WATCH/MULTI compare-and-set (the
  same pattern StoreDCRCredentialsIfAbsent uses), so an update racing a
  concurrent delete or TTL eviction fails with ErrNotFound instead of
  re-creating the row.
• Regenerate the DCRCredentialStore mock for the new method.

Closes #6673

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)

New unit tests cover both backends: update replaces an existing record,
update on an absent key returns ErrNotFound without creating, an
expired-but-present row is still updatable (presence is physical, not
liveness), invalid input is rejected, and the stored copy is isolated from
later caller mutation. Redis adds TTL-derivation (including the bounded
past-expiry TTL), concurrent-update, and connection-failure tests, plus a
real-Redis (Sentinel) integration test — skipped when no cluster is available —
for the replace-and-refresh-TTL (with a wire-level TTL assertion) and
absent-key paths.

Changes

File	Change
pkg/authserver/storage/types.go	Add UpdateDCRCredentialsIfPresent to the DCRCredentialStore interface with contract docs (presence vs. liveness, TTL, defensive copy)
pkg/authserver/storage/memory.go	Implement the method for MemoryStorage (presence-gated overwrite storing a defensive copy)
pkg/authserver/storage/redis.go	Implement the method for RedisStorage via WATCH/MULTI CAS with bounded, jittered-backoff retries; add dcrUpdateIfPresent transaction body (presence checked with EXISTS)
pkg/authserver/storage/mocks/mock_storage.go	Regenerated mock for the new interface method
pkg/authserver/storage/memory_test.go	Unit tests for the in-memory implementation
pkg/authserver/storage/redis_test.go	Unit tests for the Redis implementation, including TTL and concurrency
pkg/authserver/storage/redis_integration_test.go	Real-Redis (Sentinel) integration test, skipped without a cluster — replace-and-refresh-TTL (wire-level TTL) and absent-key paths

Does this introduce a user-facing change?

No. This is an additive storage-interface method with no CLI, API, or operator
surface changes.

Special notes for reviewers

• Presence is physical, not liveness. Unlike StoreDCRCredentialsIfAbsent,
  which treats an expired row as absent so a fresh registration can reclaim the
  slot, Update gates on physical presence: any row GET returns is
  updatable, including one whose ClientSecretExpiresAt has passed but whose
  Redis key has not yet self-evicted. This is intentional — GetDCRCredentials
  does not filter on expiry, so gating Update on liveness would break the
  Get → transform → Update round-trip the method exists for.
• TTL handling matches the initial store. The rewritten row's backend TTL
  is derived from the incoming creds via marshalDCRCredentialsForStore
  exactly as StoreDCRCredentialsIfAbsent derives it, so an update can extend,
  shorten, or clear the TTL.
• StoreDCRCredentialsIfAbsent's create semantics are unchanged; this change
  is purely additive.
• Intentionally consumer-less for now. The new interface method has no
  production caller in this PR; it is the forward-looking write path for the
  storage decorator described in authserver/storage: DCRCredentialStore has no way to update an existing record #6673, and ships implemented, mocked, and
  tested. It is not orphaned API surface.
• Update's retry contention profile. The WATCH/MULTI retry bound is
  shared with StoreDCRCredentialsIfAbsent, but every Update caller writes
  inside MULTI (Store losers mostly take a read-only branch), so retries back
  off with jitter to avoid synchronized re-collision. WATCH still guarantees
  correctness regardless of the bound — a losing EXEC writes nothing — so
  exhausting retries returns a transient, retryable error, never a torn write.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.15%. Comparing base (84e3bea) to head (154edf9).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/storage/redis.go	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6674      +/-   ##
==========================================
+ Coverage   79.11%   79.15%   +0.03%     
==========================================
  Files         785      789       +4     
  Lines       78460    79048     +588     
==========================================
+ Hits        62076    62567     +491     
- Misses      16379    16476      +97     
  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, api-design, test-coverage, general-quality

Consensus Summary

#	Finding	Consensus	Severity	Action
1	Retry bound reused for Update's heavier contention profile	9/10	MEDIUM	Fix
2	Interface "Defensive copy" doc section not updated	9/10	MEDIUM	Fix
3	Redis GET-error branch untested	9/10	MEDIUM	Fix
4	Presence check uses GET instead of EXISTS	8/10	MEDIUM	Fix
5	Integration test's TTL claim unverified at wire level	8/10	MEDIUM	Fix
6	No backoff/jitter between retries	7/10	LOW	Fix (optional)
7	maxDCRClaimRetries comment names only one caller	7/10	LOW	Fix
8	UpdateTTL test omits past-expiry assertion	7/10	LOW	Fix
9	"Why key embedded" doc section names only one method	7/10	LOW	Fix (optional)
10	PR body mischaracterizes integration test as "miniredis"	7/10	LOW	Fix
11	New method has no production caller yet	7/10	INFO	Note
12	GET result intentionally discarded (confirmed correct)	7/10	INFO	Note

Overall

This PR adds UpdateDCRCredentialsIfPresent to the DCRCredentialStore interface, implementing it in both the in-memory and Redis backends as a presence-gated compare-and-set that never creates a row. The implementation is a faithful, low-risk mirror of the already-shipped StoreDCRCredentialsIfAbsent pattern - same WATCH/MULTI retry structure, same defensive-copy and TTL-derivation contracts, same RFC6749 error wrapping - and its one genuinely new design decision (gating on physical presence rather than TTL-liveness, unlike the sibling method) is well-justified and documented consistently across the interface and both backends.

None of the findings below are blocking. The most substantive one is that the WATCH/MULTI retry bound (maxDCRClaimRetries=3) was copied without re-examining it: StoreDCRCredentialsIfAbsent's losers mostly take a read-only branch and rarely reach MULTI, but every Update caller unconditionally writes, so N concurrent updaters genuinely race the watched key on every call - a materially different contention profile than the one the bound was tuned for. Worth a second look before this ships as the intended write path for a future storage decorator, though not a reason to hold the PR.

The rest are polish: a real, cheap efficiency/consistency fix (the presence check does a full GET where five other call sites in this file use EXISTS), a couple of doc-comment sections that weren't threaded through to mention the new method, a generic Redis-error branch that's untested (true for the sibling Store method too, not unique to this PR), and one integration test whose own comment claims a wire-level TTL check it doesn't actually perform.

Documentation

• pkg/authserver/storage/types.go:385-389 - the interface-level "Defensive copy" section names Store and Get but not the new Update method.
• pkg/authserver/storage/types.go:403-414 - the "why the key is embedded" rationale applies equally to Update's identical (ctx, creds) shape but isn't mentioned there.
• pkg/authserver/storage/redis.go:42 - maxDCRClaimRetries's doc comment still says it bounds only StoreDCRCredentialsIfAbsent's retry loop.
• PR description's Changes table calls the new redis_integration_test.go coverage "miniredis integration coverage," but that test runs against a real Redis Sentinel cluster and skips without one.

---

Generated with Claude Code

[tgrunnagle]

Addressed the review-body documentation note: the PR description no longer calls the redis_integration_test.go coverage "miniredis integration coverage" — it now correctly describes it as a real-Redis (Sentinel) integration test that is skipped without a cluster, and the replace-and-refresh-TTL subtest now includes a wire-level TTL assertion (5ffead3). Fixes across two commits: 6c7b2a4 (source: EXISTS presence check, jittered backoff, doc sync) and 5ffead3 (tests).

[jhrozek]

Went through this with a couple of separate passes (a security-focused one and a Go-quality one), plus had the Redis WATCH/MULTI approach specifically checked against plain go-redis semantics. Nothing wrong with correctness, tests are solid, but I think the Redis implementation of UpdateDCRCredentialsIfPresent is more machinery than the job needs. Left the main note inline.