Summary
DCRCredentialStore has two conditional writes: StoreDCRCredentialsIfAbsent (SET NX) and UpdateDCRCredentialsIfPresent (SET XX, added for #6673). There is no way to overwrite a row only if it still holds the value the caller last read. Without that, a caller that coordinates DCR re-registration across replicas cannot make its overwrite safe against a concurrent writer.
Use case
When a stored DCR row can no longer be used (for example, its sealed fields can't be decrypted after an encryption-key rotation), a consumer re-registers with the upstream and overwrites the row in place. The consumer runs several replicas, so it serialises recovery with a lease-based Redis lock, renews the lease while registering, and re-checks ownership immediately before the SET XX.
That still leaves one window. Suppose a holder passes its ownership check and then stalls for longer than the lease before writing (a GC pause or a frozen process). Another replica can then take the lock, register, and persist its client. When the stalled holder resumes, its UpdateDCRCredentialsIfPresent overwrites the newer row, and replicas end up running different upstream clients.
The ownership check and the row write cannot be made atomic from outside toolhive:
- The lock key and the DCR row key hash to different Redis Cluster slots, so a script or transaction over both fails with
CROSSSLOT.
- The DCR key format (
redisDCRKey) and the stored encoding (marshalDCRCredentialsForStore, including the TTL rules) are unexported, so a consumer can't issue the write itself without duplicating storage internals.
Proposal
Add a single-key compare-and-set to DCRCredentialStore, for example:
// UpdateDCRCredentialsIfUnchanged overwrites the row at creds.Key only if the
// stored row is still the one the caller read (expected). It returns ErrNotFound
// when the row is absent and a distinct sentinel (e.g. ErrDCRCredentialsChanged)
// when it no longer matches; in both cases nothing is written.
UpdateDCRCredentialsIfUnchanged(ctx context.Context, creds *DCRCredentials, expected *DCRCredentials) (*DCRCredentials, error)
- Redis: a Lua script on the one row key,
GET then compare then SET … XX PX. It is atomic and Cluster-safe because only one key is touched. The comparison could use the stored JSON bytes, or a version or etag field written with the row, which avoids having to compare decrypted or decorated values.
- Memory: a mutex-guarded compare.
- Decorators that transform fields (for example, encryption wrappers) need to be able to pass the stored form of
expected. An opaque version or etag returned by GetDCRCredentials may be the cleaner contract than comparing whole values.
Acceptance
- If the row changed since
expected was read, a concurrent overwrite is refused and nothing is written.
- If the row is unchanged, it is overwritten and its TTL follows the same rules as
UpdateDCRCredentialsIfPresent.
- The Redis implementation works in standalone, Sentinel and Cluster modes.
Summary
DCRCredentialStorehas two conditional writes:StoreDCRCredentialsIfAbsent(SET NX) andUpdateDCRCredentialsIfPresent(SET XX, added for #6673). There is no way to overwrite a row only if it still holds the value the caller last read. Without that, a caller that coordinates DCR re-registration across replicas cannot make its overwrite safe against a concurrent writer.Use case
When a stored DCR row can no longer be used (for example, its sealed fields can't be decrypted after an encryption-key rotation), a consumer re-registers with the upstream and overwrites the row in place. The consumer runs several replicas, so it serialises recovery with a lease-based Redis lock, renews the lease while registering, and re-checks ownership immediately before the
SET XX.That still leaves one window. Suppose a holder passes its ownership check and then stalls for longer than the lease before writing (a GC pause or a frozen process). Another replica can then take the lock, register, and persist its client. When the stalled holder resumes, its
UpdateDCRCredentialsIfPresentoverwrites the newer row, and replicas end up running different upstream clients.The ownership check and the row write cannot be made atomic from outside toolhive:
CROSSSLOT.redisDCRKey) and the stored encoding (marshalDCRCredentialsForStore, including the TTL rules) are unexported, so a consumer can't issue the write itself without duplicating storage internals.Proposal
Add a single-key compare-and-set to
DCRCredentialStore, for example:GETthen compare thenSET … XX PX. It is atomic and Cluster-safe because only one key is touched. The comparison could use the stored JSON bytes, or a version or etag field written with the row, which avoids having to compare decrypted or decorated values.expected. An opaque version or etag returned byGetDCRCredentialsmay be the cleaner contract than comparing whole values.Acceptance
expectedwas read, a concurrent overwrite is refused and nothing is written.UpdateDCRCredentialsIfPresent.