Fix four correctness defects on the dependency and fail-open paths - #5
Merged
Conversation
All four were found by reviewing this library on the authorization path of a multi-tenant service, and each was reproduced live against Valkey before being fixed. Every fix has a test that was confirmed to fail when the fix is reverted. 1. A shared dependency set could have its TTL shortened Set issued an unconditional EXPIRE on each reverse-dependency set. Those sets are shared by every entry that depends on the same thing, so the last writer won even when its TTL was shorter. With jittered TTLs that is ordinary rather than rare: at 10% jitter on a 12h hard TTL, entries land in 10.8h-13.2h, so a dependency set can expire more than two hours before an entry still listed in it. An Invalidate landing in that window finds an empty set, cascades to nothing, and reports success — in the service that found this, a revoked role kept working until the dependent entry expired on its own. The expiry may now only be raised: EXPIRE NX sets it when the set has none, EXPIRE GT raises it when a longer-lived dependent joins. Both are needed; GT treats a key with no expiry as infinite and would refuse to set one, leaving the leak the TTL exists to prevent. 2. A failed encode failed the request fetchAndCache handled the two halves of a write inconsistently. A failing cache.Set was logged and swallowed, correctly, because the value had already been fetched — but a failing encode three lines earlier was returned to the caller. A serialization problem, which is purely a caching concern, became user-visible even though the expensive round trip had succeeded. The library is fail-open everywhere else; this one branch was fail-closed, and it turned "caching silently does not work" into "the service is down". Encode and wrapper-marshal failures are now logged and the fetched value is returned uncached, matching Set. 3. A payload that no longer decoded failed every read for the whole TTL A corrupt wrapper was already treated as a miss and refetched. A corrupt payload inside a valid wrapper was a hard error returned to the caller, so changing EncoderType on a warm cache — or a cached type whose shape moves during a rolling deploy where two versions share one server — broke every read of that key until its hard TTL expired, with no self-healing. A payload that cannot be decoded is now treated as a miss. 4. The invalidation cascade was O(nodes) sequential round trips The breadth-first walk issued one SMEMBERS per node, plus one more per dependent found, each as its own round trip. A wide dependency graph became hundreds of serial commands on the write path, against a server the read path deliberately fast-fails on. The walk now batches per level with DoMulti: one round trip for the level's dependency sets, one for their dependents' forward lists. Cost is now proportional to graph depth rather than node count. Verified against Valkey 9.1.1: full suite green with -race, 89.3% coverage, golangci-lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while reviewing
c3eon the authorization path of a multi-tenant service, where it now backs every permission decision. All four were reproduced live against Valkey before being fixed, and every fix has a test that was confirmed to fail when the fix is reverted.1. A shared dependency set could have its TTL shortened
Setissued an unconditionalEXPIREon each reverse-dependency set. Those sets are shared by every entry depending on the same thing, so the last writer won even when its TTL was shorter.With jittered TTLs that's ordinary, not rare — at 10% jitter on a 12h hard TTL entries land in 10.8h–13.2h, so a dependency set can expire more than two hours before an entry still listed in it. An
Invalidatelanding in that window finds an empty set, cascades to nothing, and reports success.In the service that found this, a revoked role kept working until the dependent entry expired on its own.
The expiry may now only be raised:
EXPIRE … NXsets it when the set has none,EXPIRE … GTraises it when a longer-lived dependent joins.Both are needed.
GTtreats a key with no expiry as infinite and refuses to set one, soGTalone would leave these sets persistent forever — exactly the leak the TTL exists to prevent. That trap is worth knowing about.2. A failed encode failed the request
fetchAndCachehandled the two halves of a write inconsistently: a failingcache.Setwas logged and swallowed — correctly, the value was already fetched — but a failing encode three lines earlier was returned to the caller.A serialization problem, which is purely a caching concern, became user-visible even though the expensive round trip had succeeded. The library is fail-open everywhere else; this one branch was fail-closed, and it's what turned "caching silently doesn't work" into "the service is down".
Encode and wrapper-marshal failures now log and return the fetched value uncached, matching
Set.fetchAndCachereturns a smallfetchResultsoblockingFetchcan serve the raw value when there's nothing to decode.3. A payload that no longer decoded failed every read for the whole TTL
A corrupt wrapper was already treated as a miss and refetched. A corrupt payload inside a valid wrapper was a hard error returned to the caller — so changing
EncoderTypeon a warm cache, or a cached type whose shape moves during a rolling deploy where two versions share one server, broke every read of that key until its hard TTL expired, with no self-healing.Now treated as a miss.
4. The invalidation cascade was O(nodes) sequential round trips
The breadth-first walk issued one
SMEMBERSper node plus one more per dependent found, each as its own round trip. A wide dependency graph became hundreds of serial commands on the write path — against a server the read path deliberately fast-fails on.The walk now batches per level with
DoMulti: one round trip for the level's dependency sets, one for their dependents' forward lists. Cost is proportional to graph depth rather than node count. Behaviour is unchanged, including the de-duplication of entries reachable from two sets in the same level.Tests
New
correctness_test.go, all against a real Valkey (skipping when none is reachable, matching the existing convention):Each was reverted-and-rerun to confirm it fails without its fix:
Verification
Against Valkey 9.1.1: full suite green under
-racewith-tags=integration, 89.3% coverage,golangci-lintclean,go vetclean,gofmtclean.docs.goupdated for the new dependency-TTL rule, the decode-error handling, and the batched cascade.🤖 Generated with Claude Code