perf(redis): skip refills behind observed watermarks - #143
Conversation
2c64339 to
86caf56Compare86caf56 to
a0150bdCompare
lan17
left a comment
There was a problem hiding this comment.
Overall, I like the optimization and the implementation direction. This faithfully implements #141: tracked MGET misses can now preserve an authoritative watermark, caller/shadow fills can avoid serializer/compression/Redis work when the replacement is already known to be fenced, admitted fills do not add a Redis round trip, and legacy custom adapters can continue returning DecodedRedisFrame | null without opting into the optimization. The test coverage is also broad, including the equal-boundary case, admitted writes, large-payload serialization/compression avoidance, shadow fills, confirmation behavior, both bundled adapters, real Redis/Valkey, and packed consumers.
I do think there are two changes we should make before merging, plus one API-shape recommendation that is worth considering while this surface is new.
1. Blocking: admitted refills now start their logical TTL before serialization/compression
The important semantic change is in RedisCache.putWithLayer:
if(key.trackForInvalidation&&watermarkMiss!==undefined){createdAtMs=Date.now();
...
if(createdAtMs<=watermarkMiss.observedWatermarkMs){returnfalse;}}constserialized=awaitserializer.dump(value);// compression
...
awaitclient.write({ ...,createdAtMs});Before this PR, the bundled adapters sampled Date.now() immediately before frame construction / SET dispatch. With this PR, a typed-miss refill that is admitted samples createdAtMs before potentially asynchronous serializer.dump, synchronous compression, size work, and any delay before the adapter dispatches the write.
That matters because createdAtMs is not only an invalidation token. Core also uses it as the logical age origin for TTL and stale-on-error decisions:
age = reader Date.now() - frame.createdAtMs
So serialization/compression time now consumes the stored value's logical TTL even though Redis starts its physical PX TTL only when the SET executes.
Concrete example:
remote TTL = 1s
candidate timestamp = t=0
serializer/compression = 1.5s
SET executes = t=1.5s
Redis receives a brand-new key with PX 1000, but the frame is already logically 1.5s old. The next DialCache read rejects it immediately and can go back to source, while the physically present key remains in Redis for another second. A slow async custom serializer makes this particularly easy to hit, but a large synchronous payload can consume a meaningful fraction of a short TTL as well.
I think we can preserve the optimization without changing the old dispatch-adjacent timestamp semantics:
constobservedWatermarkMs=watermarkMiss?.observedWatermarkMs;// Cheap preflight: lets us avoid serializer/compression entirely when already fenced.if(observedWatermarkMs!==undefined){constpreflightCreatedAtMs=Date.now();assertValidRedisTimestampMs(preflightCreatedAtMs);if(preflightCreatedAtMs<=observedWatermarkMs){returnfalse;}}constserialized=awaitserializeAndCompress(value);if(shouldWrite!==undefined&&!shouldWrite()){returnfalse;}letcreatedAtMs: number|undefined;if(observedWatermarkMs!==undefined){// Final timestamp is the one that actually goes into the frame.createdAtMs=Date.now();assertValidRedisTimestampMs(createdAtMs);// Recheck because the wall clock may have moved backwards while we serialized.if(createdAtMs<=observedWatermarkMs){returnfalse;}}awaitclient.write({
valueKey,
cacheTtlMs,value: serialized,
...(createdAtMs===undefined ? {} : { createdAtMs }),});This gives us both desirable properties:
- if the clock is already behind/equal to the observed watermark, we still skip before
serializer.dump, compression, allocation, andSET; - if the fill is admitted, the timestamp written into the frame remains close to actual dispatch, preserving the pre-PR logical-TTL behavior.
The second check is also useful for a wall-clock rollback during an async serializer: the final frame timestamp should be the same timestamp used for the final admission decision.
I would add a focused test with an async dump() that advances fake time by more than a short TTL and assert that the frame written after serialization is still fresh on the subsequent read. I would also test a backwards clock move between the preflight and final sample and verify that we suppress the write if the final timestamp is again behind the observed watermark.
There is a smaller semantic point here too: even with the two-sample approach, the early preflight can skip a fill that would have become admissible by the time a slow serializer finished. I think that is a reasonable conservative optimization because the point is to avoid doing expensive work while we currently know the candidate is fenced, but it is worth treating this as intentional behavior rather than claiming the optimization is entirely semantics-neutral.
2. Blocking: this is a public breaking change but the current perf commit will release as a patch
The PR correctly calls out public type-surface changes, but the release metadata does not currently reflect them.
Two externally observable changes are breaking for exhaustive/direct consumers:
DialCacheRedisClient.read()can now return a truthyRedisWatermarkMisswhere a bundled adapter previously returnednullfor the same semantic miss.ShadowValidationOutcomeadds"fill_fenced", which breaks exhaustive switches andRecord<ShadowValidationOutcome, ...>values.
A direct consumer compiled against the old contract could have code like:
constframe=awaitadapter.read(request);if(frame!==null){consume(frame.payload);}With the new bundled adapter behavior, a fenced tracked miss enters that branch with { observedWatermarkMs }. Recompilation against the new declarations catches this, but existing JS / loosely typed consumers can see a runtime behavior change.
The current commit is:
perf(redis): skip refills behind observed watermarks
and this repo's semantic-release config maps plain perf to a patch while pre-1.0 breaking changes are mapped to a minor. So as currently authored this would be released with the wrong semver classification.
I suggest making the squash/title explicitly breaking, e.g.:
perf(redis)!: skip refills behind observed watermarks
with a footer along the lines of:
BREAKING CHANGE: DialCacheRedisClient.read may return RedisWatermarkMiss for
tracked semantic misses, and ShadowValidationOutcome adds fill_fenced.
RedisWriteRequest.createdAtMs being optional is source-compatible by itself; the widened read behavior and outcome union are the pieces I would specifically call out.
3. Non-blocking but strongly recommended: use an explicit discriminant for RedisWatermarkMiss
The new public variant currently relies on structural absence/presence checks:
interfaceRedisWatermarkMiss{observedWatermarkMs: number;payload?: never;createdAtMs?: never;}and core repeats a guard like:
"observedWatermarkMs"inresult&&!("payload"inresult)&&!("createdAtMs"inresult)in multiple files.
Because this is a brand-new public union variant, I think we have a good opportunity to make the semantic type explicit:
exportinterfaceRedisWatermarkMiss{readonlykind: "watermark_miss";readonlyobservedWatermarkMs: number;}Then narrowing becomes both simpler and more robust:
functionisRedisWatermarkMiss(result: RedisReadResult,): result is RedisWatermarkMiss{returnresult?.kind==="watermark_miss";}This removes the duplicated duck-typing logic and gives custom-adapter authors a much clearer contract. The current code does intentionally preserve a structurally augmented frame containing observedWatermarkMs, which is good, but an explicit discriminator expresses that distinction directly instead of inferring it from the absence of two unrelated fields.
If we keep the current shape, I would at least centralize/export one guard rather than maintaining three subtly independent copies.
Additional test cases worth locking down
The existing coverage is very good. A few small cases would make the public adapter boundary harder to misuse:
- invalid typed misses (
NaN, negative, fractional, unsafe integer) should be normalized to a generic miss and follow the ordinary refill path rather than activating suppression; - a custom adapter returning a typed miss for an untracked request should not activate the optimization;
- the dispatch-adjacent timestamp / short-TTL test described above;
- backwards wall-clock movement between preflight and final write timestamp;
- if we add a discriminator, a custom frame carrying arbitrary extra metadata should still remain a hit while only
{ kind: "watermark_miss", ... }is treated as the miss variant.
Things I think are correct as-is
- Keeping
decodeTrackedRedisFrameas the legacyDecodedRedisFrame | nullhelper and adding a separate opt-indecodeTrackedRedisReadResultis a good compatibility split. - Passing the exact admitted timestamp through
RedisWriteRequest.createdAtMsis necessary; otherwise the fence decision and stored frame could diverge. - The optimization should remain snapshot-based. A watermark advancing after the authoritative
MGETcan still fence an admitted write, and avoiding that would require server-side coordination / another protocol change. I agree that is out of scope here. fill_fencedis the right shadow outcome. Reportingfilledwhen noSETwas dispatched would make the telemetry misleading.- Confirmation turning a typed C1 watermark miss into
supersededis correct: it tells us the original C0 observation no longer survives the current tracked-read semantics. - Missing/malformed watermark metadata falling back to the legacy generic-miss behavior is appropriately conservative.
- No extra Redis read /
TIME/ Lua / transaction is introduced, which preserves the main performance constraint of #141.
So I consider the timestamp/TTL semantic regression and the release classification blocking before merge. Once those are fixed, I think the core optimization is in good shape. The discriminated-union cleanup would be ideal to do now while the type is new, but I would not block the PR solely on that if we deliberately prefer the current structural surface.
Uh oh!
There was an error while loading. Please reload this page.
Closes#141
Summary
Tracked Redis misses now preserve a trustworthy numeric watermark when the existing authoritative
MGETobserved one. The public miss variant is explicitly discriminated, and conditional refills use two application-clock samples:The preflight still removes serializer, compression, frame-allocation, network, Redis, replication, and AOF work for refills already known to remain unreadable. Admitted fills use the final dispatch-adjacent timestamp, so payload preparation does not consume the stored value's logical TTL. The final recheck also suppresses a write if the wall clock moves back behind the observed watermark.
This adds no Redis command or round trip and changes no key, frame, or watermark encoding.
Design
Adapter boundary
RedisReadResult = DecodedRedisFrame | RedisWatermarkMiss | null.RedisWatermarkMissas{ kind: "watermark_miss", observedWatermarkMs }and centralize its runtime guard.decodeTrackedRedisReadResultfor adapters that intentionally opt into discriminated watermark misses.decodeTrackedRedisFrameand its establishedDecodedRedisFrame | nullbehavior for legacy custom adapters.RedisWriteRequest.createdAtMs. Adapters returning discriminated misses must encode a supplied value exactly; ordinary and direct writes that omit it retain adapter-side clock sampling.Core flow
preflightCreatedAtMs <= observedWatermarkMsskips all payload and write work.SETwhen the final timestamp is fenced; otherwise pass that exact final sample to the adapter.nullmisses.fill_fenced; discriminated confirmation misses remainsuperseded.The early preflight is intentionally conservative: DialCache does not perform expensive work merely to see whether a later timestamp might clear the fence.
Compatibility and safety
DecodedRedisFrame | nullremain correct and keep their prior write request/timestamp behavior; they simply do not receive this optimization.DialCacheRedisClient.read()may now return a truthyRedisWatermarkMiss, andShadowValidationOutcomeaddsfill_fenced; direct and exhaustive consumers must handle both changes.Validation
corepack pnpm checkon Node 22.22.0 — typecheck, 633 unit tests with coverage thresholds, build/declarations, and packed ESM/CJS consumer checks pass.corepack pnpm test:integration— 143 tests pass across node-redis and GLIDE on Redis 6.2 and Valkey 8; the two GLIDE Cluster cases skip locally because Docker Desktop cannot route announced container IPs, while CI remains fail-closed for those cases.corepack pnpm benchmark:redis-writeagainst Redis 6.2 — admitted writes remain exactly oneSETeach with zero Lua and zeroTIMEcalls.git diff --checkpasses.BREAKING CHANGE: DialCacheRedisClient.read() may now return RedisWatermarkMiss for tracked semantic misses, and ShadowValidationOutcome adds fill_fenced.