fix(datastore): two namespace_encryption_keys read/write defects from the S04-A review round

Two real defects in the merged namespace_encryption_keys store, surfaced by the !1113 (merged) review round. Both were verified by forced interleavings against a real PostgreSQL instance, not by reading. Neither can land in !1113 (merged) — the store is main-resident and outside that branch's diff — so both belong in one MR targeting main. They touch the same file and want the same test seam, so splitting them would review the same surface twice.

Distinct from #398, which batches hardening items that are not live problems. These two are.

1. GetKeyByVersion returns a tombstone as a usable record

GetKeyByVersion runs two statements against the pool — an isNamespaceShredded probe, then the version fetch — so two independent READ COMMITTED snapshots. The fetch filters only (namespace_id, version): no shredded_at IS NULL, no wrapped_key <> '', no active. A shred committing between the two statements passes the probe and then returns the freshly tombstoned row with wrapped_key = '\x'. toRecord drops shredded_at, so the manager cannot see it is a tombstone; it tries to unwrap empty ciphertext.

Observed chain:

unwrapping namespace key: unwrap key (wrapped by "", 1 keys tried): ciphertext too short

GetActiveKey is immune — its fetch filters active = true, which the shred clears, so the same race yields a clean ErrKeyNotFound. GetKeyByVersion's comment claiming it "shares the same accepted two-snapshot read race — see GetActiveKey's doc comment" is therefore false and needs correcting alongside the fix.

This contradicts the store contract in the S04-A spec, which requires ErrNamespaceShredded here so DecryptRow gets a clear signal "rather than a GCM failure on a zeroed key", and states the read methods "never hand back a tombstone as a usable record".

Severity: low. Fail-closed holds — the key material is already destroyed and the caller still gets an error. The defect is the error code: callers branching on ErrNamespaceShredded miss, and operators see what reads like corruption. The window is sub-millisecond and needs a shred racing a by-version read of the same namespace.

Fix: filter the version fetch to live rows, or re-probe the tombstone when wrapped_key comes back empty, so a tombstone is never returned as a usable record; correct the parity comment; add a shred-vs-GetKeyByVersion case to the concurrency suite. That suite currently covers write-path races only, which is why this survived — !1116 (merged)'s barrierHookTx seam is the tool for it.

2. Auto-create rotates instead of adopting

GetKey's auto-create path calls the rotation-capable InsertKey, and adopts a concurrent creator's key only when it sees ErrKeyConflict. When manager B's GetActiveKey miss precedes manager A's committed version 1, B's insert runs after that commit: the FOR UPDATE barrier sees A's row, deactivatePriorActive clears it, nextVersion returns 2, and the conditional insert's guard — which checks only for a tombstone — passes. Version 2 is genuinely free, so no unique violation fires and the adoption branch is never reached.

Reproduced with two separate manager instances over the real store, a real LocalProvider, and a one-shot hook sequencing the interleaving:

HOOK: manager A GetKey   -> version=1
SHIM InsertKey           -> version=2 active=true err=<nil> (ErrKeyConflict=false)
RESULT: manager B GetKey -> version=2
DB ROWS: version=1 active=false | version=2 active=true   (2 rows)

Severity: low, with an accumulation property. No data loss and no correctness violation — deactivated versions are retained by design, every row stays decryptable, and A's cached v1 window is the same stale-cache state the rotation workflow already tolerates. But each occurrence leaves a permanent orphaned version that nothing reaps, and the trigger is the realistic deploy-and-first-push pattern across replicas.

Fix (built and verified, then withdrawn from !1113 (merged) to keep that MR self-contained): an expect-absent insert whose existence guard is namespace_id = $ns rather than tombstone-only, with version pinned to 1, deactivatePriorActive skipped, and an empty RETURNING disambiguated inside the same transaction — tombstone → ErrNamespaceShredded, otherwise → ErrKeyConflict, so the manager's existing adoption branch fires. Verified: B adopts version 1, one row, deliberate rotation still rotates (v1 → v2 → v3), and a shredded namespace still fails closed. Full repo unit suite, the complete datastore integration suite, FIPS, race, and lint were green.

Three caveats to carry, all found by mutation testing rather than assumed:

  • The widened guard is not independently pinned — reverting it while keeping the version pin still passes, because the pin is what produces adoption (the loser collides on UNIQUE(namespace_id, version)). Both mechanisms are in the code; only one is load-bearing.
  • Skipping the FOR UPDATE barrier under expect-absent is argued, not tested. The argument is that version-pinning plus the unique index subsume it; !1116 (merged)'s barrierHookTx seam could test it.
  • classifyAbsentMiss's delete-in-between degradation (a re-enable removing the tombstone between insert and re-probe reports ErrKeyConflict) is documented but untested.

Sibling companions required when this lands: widening crypto.NamespaceKeyStore breaks rotation_test.go's fakeKeyStore (needs an InsertInitialKey stub) and !1116 (merged)'s direct insertKeyTx call (needs , false). Confirmed sufficient in a throwaway combined tree.

Related: #398 (store hardening that is not problematic).