feat(csc): full-duplex reader-miss coalescing + invalidation batching - #3965
feat(csc): full-duplex reader-miss coalescing + invalidation batching#3965ndyakov wants to merge 45 commits into
Conversation
The buffered-push-data notice in isHealthyConn fired unconditionally. With client-side caching every tracked write parks an invalidate frame on idle pooled conns, so at default log level the line floods the log on nearly every pool Get under write-heavy workloads. It documents the normal healthy path, not a problem - gate it behind LogLevelDebug like the equivalent notices in maintnotifications.
…nvalidation batching
Builds on the CSC refresh-on-invalidate + base miss-coalescing PR. Adds:
- Coalescing MODES for the reader-miss path, selected by config:
* "workers" (default): a small pinned worker pool fetches reserved misses.
* "fullduplex": one held connection with a writer + reader goroutine pair
pipelines the reserved misses (concurrent, replies streamed back). Ordering
is preserved: the miss-coalescer's reservation dedups concurrent misses of
the same key to a single fetch, and each caller blocks on its own request's
completion — the caching client is blocking per goroutine, so no goroutine
ever sees its own reads reordered; full-duplex only overlaps independent
goroutines' fetches on the wire.
- Invalidation batching: coalesce invalidation-driven cache deletes within a
configurable window instead of one delete per push.
Config (no environment variables): new AutoPipeline-free Options fields —
ClientSideCacheCoalesceMisses (enable), ClientSideCacheCoalesceMode
("workers"/"fullduplex"), ClientSideCacheCoalesceWorkers, and
ClientSideCacheInvalidationBatchWindow. Prototype-only read-path telemetry
(READPATH_LOG + LocalCache stat counters) removed; no stats surface added.
Measured on a 50ms-RTT WAN proxy under invalidation churn, the coalescing fixes
turn published v9.22.0's miss-stampede collapse (p99 up to ~1.1s, throughput
floored) into ~5x throughput at ~1 RTT p99, matching rueidis; the full-duplex
mode gives the tighter mid-range tail at fewer connections. See
AP_CSC_TWOCLIENT_VS_RUEIDIS.md.
Depends on: feature/csc-refresh-and-miss-coalescing.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb8681e82b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- lint (the only red CI check): drop the redundant `chan struct{}` type from the
recycle declaration (ST1023) and //nolint:unused the deliberately-off, measured
cscRefreshCooldown knob.
- Reject the "pinned" PROTOTYPE engine from the public ClientSideCacheCoalesceMode
option: it holds a connection with no idle invalidation drain and can serve
stale values (cursor HIGH). It now falls back to "workers" and is reachable
only via an internal benchmark hook (cscForcePinned).
- Pass the new CSC miss-coalescing / invalidation-batching knobs through
UniversalOptions.Simple() (ClientSideCacheRefreshOnInvalidate, CoalesceMisses,
CoalesceMode, CoalesceWorkers, InvalidationBatchWindow) so UniversalClient
users can enable them (codex P2).
- Give the invalidation batcher a stop path: it is stopped and cleared when the
last user releases the binding (releaseLocked), and ensureBatcher refuses to
start one for an already-released binding, so its goroutine no longer lives
past the binding re-arming its timer forever; a later re-acquire starts fresh.
- Full-duplex session: acquire the pool connection only after the first miss
arrives, and release it if CSC serving was disabled meanwhile, so an idle
session no longer starves a small pool (PoolSize:1) until PoolTimeout.
Adds unit tests for the pinned-mode rejection and the UniversalOptions passthrough.
💡 Codex Reviewgo-redis/csc_miss_coalesce_modes.go Lines 362 to 364 in 2f6a9e5  Close the socket when stopping full-duplex sessions
In full-duplex CSC, go-redis/csc_miss_coalesce_modes.go Lines 166 to 168 in 2f6a9e5 Apply the limiter to each full-duplex miss
With go-redis/csc_miss_coalesce_modes.go Lines 373 to 374 in 2f6a9e5 Join the supervisor before re-pooling the connection
After a clean full-duplex recycle, ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR extends the client-side caching (CSC) subsystem by moving miss-coalescing and invalidation-batching controls into Options/UniversalOptions, adding a full-duplex miss-coalescing engine, and introducing windowed background batching for invalidation-driven deletes.
Changes:
- Add new CSC configuration knobs to
Optionsand propagate them throughUniversalOptions.Simple(). - Introduce selectable miss-coalescing modes (
workersdefault,fullduplex), including a new full-duplex session engine. - Add optional windowed invalidation batching to offload cache deletes from the push-notification read path.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| universal.go | Adds new CSC config fields to UniversalOptions and propagates them into Options via Simple(). |
| options.go | Adds new CSC config fields to Options and documents coalescing/batching behavior. |
| csc_refresh_support.go | Removes env-var support helpers now that options drive behavior. |
| csc_refresh_on_invalidate.go | Removes env-var gates/knobs and switches refresh publishing to tracked main-pool connections. |
| csc_miss_coalesce.go | Refactors miss coalescer to be options-driven and to support multiple engines with shared helpers/stats. |
| csc_miss_coalesce_modes.go | Adds the pinned prototype and full-duplex miss-coalescing engines and related session lifecycle logic. |
| csc_miss_coalesce_modes_test.go | Adds coverage for full-duplex idle push draining correctness (positive + negative control). |
| csc_inval_batch.go | Introduces the windowed background invalidation batcher implementation. |
| csc_integration.go | Integrates invalidation batching into the push invalidation handler and threads the window from Options. |
| csc_coalesce_options_test.go | Adds tests for public mode selection behavior and UniversalOptions.Simple() propagation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f6a9e55ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…; doc - Full-duplex readOne now applies the read reply and lets fulfillCached gate only the cache publish on the captured conn id/generation -- matching the workers/pinned engines -- instead of failing the caller with ErrClosed and losing a good reply on a mid-flight id/gen change. - peekAndProcessPushNotifications also drains when the reader has buffered bytes (HasBufferedData), not only when the socket is readable (MaybeHasData), so a buffered invalidation is processed on the idle tick. - Workers-mode doc corrected: a tracked connection is acquired/released per batch, not held across batches.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5a231afa5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
csc_miss_coalesce_modes.go:14
- The file header labels these engines as "PROTOTYPE", but the fullduplex engine is selectable via the public ClientSideCacheCoalesceMode option and described as feature-complete in this PR. This comment is misleading for maintainers/users reading the code.
// Alternate miss-coalescer engines (PROTOTYPE, benchmark comparison).
csc_miss_coalesce_modes.go:134
- The comment says a clean recycle continues immediately, but the code always waits for cscModeBackoff after every session end. Either skip the backoff on clean recycle, or update the comment so behavior and documentation match.
// Session ended on a connection error or a clean recycle. On error, back
// off briefly so a persistent dial failure does not hot-spin; a clean
// recycle continues immediately.
FTHybridCmd.readReply used ReadSlice, which reads a RESP3 map header's length as an element count and consumes only half its frames, leaving the connection desynced. Peek the type and read a map with ReadReply, like the other FT.* parsers.
- A coalesced miss re-runs uncached when CSC serving is disabled after the miss was reserved (RESP3 downgrade / CLIENT TRACKING loss during a conn re-init), via an internal retry-uncached sentinel that processCached catches, instead of surfacing a spurious pool.ErrClosed for a valid cacheable read. - A caller whose context cancels mid-fetch no longer races the coalescer: a CAS interlock (claimAbandon/claimApply) hands the Cmder to exactly one of the caller or the applying worker, so the worker never writes a Cmder the caller has taken back. The reply is still classified and published to the shared cache, and a cancelled Get returns the context error deterministically (matching the non-coalesced path). - A fetch after coalescer shutdown returns pool.ErrClosed instead of hanging on a post-drain enqueue race. Adds TestClassifyCachedReply, TestCSCMissReqClaimInterlock, TestCSCMissCoalesceAbandonedFetchNoRace, TestFullDuplexDisabledMidMissRetriesUncached.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
csc_miss_coalesce_modes.go:216
- sessErr is an atomic.Value that is only Store()'d on I/O failure, but errored()/reasonErr() call Load() unconditionally. atomic.Value panics on Load before the first Store, so the graceful-recycle path can crash the client. Initialize sessErr with a non-nil typed holder (or switch to atomic.Pointer) so Load is always safe.
errored := func() bool { _, ok := sessErr.Load().(error); return ok }
reasonErr := func() error {
if e, ok := sessErr.Load().(error); ok {
return e
}
csc_miss_coalesce.go:25
- The header comment still says miss coalescing is "env-gated", but the gating was moved to Options.ClientSideCacheCoalesceMisses. This is now misleading for readers and users.
// Reader-miss coalescing (PROTOTYPE, env-gated).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cae5e11b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ange - A full cache flush (FLUSHDB/FLUSHALL) cleared the cache but left the invalidation batcher's queued per-key deletes, which then fired and evicted entries repopulated after the flush (an extra miss within the window). Drop the batcher's pending queue on flush. - A running batcher's window is fixed at creation, so a second client binding to the same shared handler with a stricter window kept the old cadence and its staleness bound did not hold. setInvalBatchWindow now drops the running batcher on a window change so the next invalidation starts a fresh one with the new window.
setup-go's "1.26.x" resolved to go1.26.5, which govulncheck flags for two standard-library vulnerabilities fixed in go1.26.6: GO-2026-6090 (crypto/tls) and GO-2026-5972 (encoding/asn1). Track the latest stable toolchain so future security patches are picked up automatically instead of pinning a patch.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
csc_inval_batch.go:60
- The buffered
dropChsignal does not order ahead of the timer case and cannot stop anapplyalready in progress. Around a window boundary,runcan selectt.Cand apply the pre-flush batch aftercache.Flush()(and after a reader repopulates the key), evicting the fresh entry despite this method's guarantee. Coordinate flush/drop with an epoch or mutex and wait for/neutralize any in-progress apply so all pre-FLUSH deletes are definitively superseded.
// Signal run() to clear its in-progress batch; the cap-1 buffer means a signal
// is never lost even if run() is not currently selecting.
select {
case b.dropCh <- struct{}{}:
default:
csc_miss_coalesce.go:336
getConncallsLimiter.Allowonly once for the whole batch, while every request inbatchis a separate client operation. This lets batches bypass per-operation rate/circuit limits and reports only one aggregate result, contrary to theLimitercontract inoptions.go:37-45. Check and report the limiter per request, excluding denied requests from the wire batch; use_getConnfor the underlying batch connection so it does not consume an extra limiter operation.
cn, err := c.getConn(ctx)
csc_miss_coalesce_modes.go:168
- This acquires one limiter permit via
getConnand holds it for the entire full-duplex session, which can execute thousands of independent operations for up to 30 seconds. A concurrency limiter may therefore reject unrelated commands while this permit sits idle, and a rate/circuit limiter never sees individual miss results. ApplyAllow/ReportResultper request and acquire the session connection through_getConninstead.
getCtx, getCancel := opCtx()
cn, err := c.getConn(getCtx)
getCancel()
csc_miss_coalesce_modes.go:158
- Waiting for the first miss does not prevent idle starvation after that miss completes: the session keeps the connection until the 30-second recycle timer. With the valid
PoolSize: 1configuration, a miss succeeds but the caller's next non-cacheable command (for exampleSETorPING) cannot acquire the sole pool turn and times out. Return the session connection once its in-flight queue drains and no miss is queued, or reject/fallback from full-duplex mode when the pool cannot reserve another connection.
// Do not hold a pool connection while idle: wait for the first miss BEFORE
// acquiring. An eagerly-held session connection would, at a small pool
// (PoolSize:1), starve non-cacheable commands (PING/SET/uncached reads) until
// PoolTimeout while the session sat waiting for work. The pulled miss is
// written first by the writer below.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce21ffb50f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Attribute write-side failures to the held session connection: the miss coalescer sets req.servedBy for the whole batch before the write, so a WithWriter error (or ctx cancel during the inflight handoff) settles with the connection attribution the reply path records, not a nil conn / zero attempts. - Tolerate fragmented push frames on held sessions: once readiness is established, the drain uses the longer cscDrainHardReadCap budget (as the background drainer does) instead of the 1ms probe cap, which could time out mid-frame over TLS and fatally close a healthy session. The 1ms cap is reserved for speculative no-data probes (opaque transports). - Route a WithTimeout clone's CSC push-handler Close through the owner wrapper: the clone's cscClientWeak now points at the canonical owner, so closeCanonical calls owner.Close() (full CSC + autopipeliner teardown) instead of baseClient.Close closing shared pools while the owner's drainer runs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e6d30f98e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (3)
redis.go:2669
- This cleanup can re-arm a deadline instead of clearing it.
WithReaderHardDeadlinealready clears its deadline on return; callingWithReader(..., 0, ...)afterward runsgetEffectiveReadTimeout, so an active maintenance relaxation installs a positive deadline. ForReadTimeout < 0, later reads intentionally skipSetReadDeadlineand can then fail at that stale deadline. Remove this defer and rely onWithReaderHardDeadline's clear.
defer func() {
if c.opt.ReadTimeout < 0 {
_ = cn.WithReader(context.Background(), 0, func(*proto.Reader) error { return nil })
}
csc_miss_coalesce.go:325
- Passing
context.Background()makes this error callback differ from the normal command path, which passes the caller context (redis.go:1276). Custom recorders and OpenTelemetry exemplars therefore lose the request's trace/context specifically for coalesced reply errors, despite the promised telemetry parity. Preserve the fetch context on the request and pass it here.
errorCallback(context.Background(), errorType, req.servedBy, statusCode, isInternal, 0)
csc_miss_coalesce.go:377
- This transport-error callback also discards the caller context, unlike
processWithRetry(redis.go:1276,1288). As a result, coalesced acquisition/write/read failures cannot be correlated with the originating trace by custom or OpenTelemetry recorders. Store the request context and pass it through here.
errorCallback(context.Background(), errorType, req.servedBy, statusCode, isInternal, 0)
Two fixes to the refresh-on-invalidate reply reader, matching the miss-coalescer / background-drainer policy: - Propagate push-processor errors instead of logging and continuing. A surfaced error means bytes may have been consumed mid-frame; reading the next reply on a desynchronized stream could publish a push fragment under the wrong cache key. Returning the error aborts the batch so withConn retires the connection. - Use the nonblocking cscHandlerClient adapter for the drain. This reader is part of the refresher's waitgroup, so a custom push handler calling Close() on the raw client would deadlock shutdown (Close waits on the refresher goroutine parked in the handler).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc297923ee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ed (#3969) * Update options.go * Update options_test.go * Update options.go * fix flaky test * fix(maintnotifications): skip endpoint DNS detect when mode is disabled
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
#3972) * perf(proto): add zero-copy semantics to scan, remove redundant data conversions in reader * chore: unpin govulncheck version * chore: require govulncheck to use latest go version * chore: pin govulncheck and resolve go-version to stable --------- Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>
A caller that cancels its context while waiting on a coalesced miss returned early without recording an error metric: the serving session still completes the background fetch (applyAndSettle success or settleErr), so neither records THIS caller's cancellation, undercounting the cancellation rate whenever miss coalescing is enabled. Report it here, attributed to the caller's ctx, matching processWithRetry. The conn is passed as nil deliberately: req.servedBy is written by the session and only safe to read after the req.done receive (the happens-before edge), which has not happened on the cancellation branch, and is nil anyway when the cancel beats the serve. The metric recorder treats a nil conn as "no peer attributes".
When getConn fails because acquireCtx was cancelled by mc.stop during coalescer teardown, the session settled the pulled miss and drained the queue with the raw context.Canceled. processCached only retries uncached on the errCSCRetryUncached sentinel, so in-flight coalesced reads surfaced a spurious cancellation even though the caller's context was still valid, instead of re-running on the still-open pool like the other stop paths. acquireCtx derives from context.Background() and is cancelled only by mc.stop (its sole non-deadline cancel source), so a context.Canceled here uniquely means teardown: settle first and the queue drain with errCSCRetryUncached and report stopped so the loop exits without backing off. Genuine acquire failures (dial error, deadline) keep the raw error and the backoff path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90180a1163
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The full-duplex miss coalescer mapped a getConn cancellation to errCSCRetryUncached by testing errors.Is(err, context.Canceled). But getConn runs the dialer, credentials callbacks, and init hooks, any of which can return context.Canceled from their OWN context while the coalescer is not stopping. Treating the error value as proof that mc.stop fired permanently exited the sole full-duplex worker; the current miss retried uncached, but later misses still enqueued through the still-live coalescer with no worker to settle them, hanging deadline-free callers and leaking IN_PROGRESS reservations. Detect teardown by checking mc.stop directly instead. mc.stop is closed only by stopWorkers, and close(mc.stop) is ordered before acquireCtx cancels its context (before getConn returns), so a real teardown is always visible. On stop, settle retry-uncached and exit; on any other acquire failure — including an unrelated context.Canceled — fail the batch and back off so the worker stays alive. This supersedes the errors.Is check added in the previous round.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee25c4e66c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ClientSideCacheCoalesceMisses holds one connection per coalescer session and serves many misses on it, so Options.Limiter is admitted (Allow/ReportResult) once per session — per held connection — not per coalesced miss, unlike the plain per-command path. This is inherent to the held-connection model; a per-miss Allow would defeat the coalescing and re-admit a connection already held. Document it on the option so callers that need strict per-command admission or circuit-breaking know not to enable miss coalescing.
Bumps [rojopolis/spellcheck-github-actions](https://github.com/rojopolis/spellcheck-github-actions) from 0.63.0 to 0.66.0. - [Release notes](https://github.com/rojopolis/spellcheck-github-actions/releases) - [Changelog](https://github.com/rojopolis/spellcheck-github-actions/blob/master/CHANGELOG.md) - [Commits](rojopolis/spellcheck-github-actions@0.63.0...0.66.0) --- updated-dependencies: - dependency-name: rojopolis/spellcheck-github-actions dependency-version: 0.66.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a0cb0fe9d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The held-session push drain (pushDrainWithin) dispatched the built-in processor through ProcessPendingNotifications, which swallows an error that occurs AFTER partially consuming a frame — e.g. a RESP3 attribute whose remainder arrives slower than the hard read cap. A swallowed mid-frame error left the connection desynchronized, so later fragments could be misread as command replies and cached under the wrong key, or fail unrelated misses. Use ProcessPendingNotificationsBuffered for the built-in processor, exactly as drainPushNotifications already does, so a post-consumption error is propagated and the caller retires the desynced session. Custom processors keep the unbuffered call (their contract is "invoked only when notifications are known present").
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d33c190. Configure here.
| // still applies — its post-flush delete must not be lost. | ||
| if batcher != nil { | ||
| batcher.drop() | ||
| } |
There was a problem hiding this comment.
Flush misses replaced batcher
Medium Severity
HandlePushNotification snapshots batcher under RLock then calls drop() only on that pointer before Flush. A concurrent window/refresh rebuild can stop() and nil the handler batcher first, so FLUSHDB/FLUSHALL never bumps that batcher’s epoch. Its async stop-drain still applies the pre-flush deletes afterward and can evict entries repopulated after the flush, the failure mode TestInvalBatchDroppedOnFlush already pins for the non-racy path.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d33c190. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d33c190d69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // which is treated as fatal and fatally closes a healthy session (failing its | ||
| // in-flight misses). The 1ms cap is reserved for speculative no-data probes | ||
| // (timedPushDrain, the opaque-transport fallback). | ||
| return c.pushDrainWithin(ctx, cn, cscDrainHardReadCap) |
There was a problem hiding this comment.
When miss coalescing runs over TLS with a custom NotificationProcessor, raw-socket readiness does not prove that a RESP notification exists: MaybeHasData unwraps TLS, while internal/pool/conn_check.go explicitly notes that the readable bytes may be only a post-handshake record. This call then invokes the custom processor under the 50 ms hard deadline; unlike the built-in buffered processor, a conforming custom processor may surface the resulting empty-read timeout, which the session reader treats as fatal at csc_miss_coalesce_modes.go:431-433, closing a healthy connection and failing any in-flight misses. Confirm an application-level push before invoking a custom processor, or restrict this transport-readiness path to the built-in processor.
AGENTS.md reference: AGENTS.md:L126-L132
Useful? React with 👍 / 👎.
| case <-ctx.Done(): | ||
| mc.c.csc.Cancel(cacheKey, token) | ||
| return ctx.Err() | ||
| return nil, ctx.Err() |
There was a problem hiding this comment.
When the coalescer queue is full, or its worker is stalled acquiring a connection, a caller can expire in this enqueue select. This path cancels the reservation and returns ctx.Err() without invoking GetMetricErrorCallback; unlike the later post-enqueue cancellation path, no worker owns the request and no background settlement can record the error, so cancellation metrics are silently undercounted precisely during overload. Emit the same cancellation callback here before returning.
Useful? React with 👍 / 👎.
Full-duplex reader-miss coalescing + invalidation batching
Builds on the CSC refresh-on-invalidate + base reader-miss coalescing PR
(
feature/csc-refresh-and-miss-coalescing). Two additions, both config-driven.1. Full-duplex miss coalescing (reader-miss path)
Concurrent cache misses of the same key are already deduped to a single fetch
via a per-key reservation (one owner fetches; the rest block on it and share the
result). This PR adds the engine that dispatches the owners' fetches: a held
tracked connection with a writer + reader goroutine pair that pipelines
reserved misses — commands stream out while replies stream back.
Misses are caller-blocking (a real request waits on every fetch), so the
engine is latency-first:
what is already queued), never waited-for;
no batch phase-lock, no pool
Geton the hot path;next miss, so an idle coalescer holds zero connections;
held connection even while idle, and returns the connection to the pool for
handoff/lifetime/pool-hook processing (idle grace, recycle age bounded by the
connection's remaining
ConnMaxLifetime,ShouldHandoffchecks).Ordering is preserved. Each caller blocks on its own request's completion,
so a goroutine never issues its next command until this value is in hand — the
engine only overlaps independent callers' fetches on the wire.
Earlier engine variants ("workers": N pooled connections with half-duplex
batches; "pinned": a benchmark prototype) were removed during review: the
per-batch round trip added tail latency to exactly the path where a caller is
waiting, and the batching advantage is preserved by the writer's opportunistic
packing. One engine, no tuning surface.
2. Invalidation batching
Coalesce invalidation-driven cache deletes within a configurable window instead
of one delete per push frame — smooths bursty invalidation churn. Background
traffic is batching-first by design (nobody waits on it): windowed, deduped,
with the delete queue preserved across batcher rebuilds and dropped on
FLUSHDB/FLUSHALL(a full flush supersedes queued per-key deletes).Config (no environment variables)
New
Optionsfields (flatClientSideCache*, matchingClientSideCacheRefreshOnInvalidate), mirrored inUniversalOptions:ClientSideCacheCoalesceMisses bool— enables the coalescer (requires thebuilt-in
LocalCache; ignored for customCacheimplementations).ClientSideCacheInvalidationBatchWindow time.Duration— 0 (default) appliesinvalidations inline; a nonzero window batches them (set it no larger than the
cache
MaxStaleness). Shared-handler clients fold windows strictest-wins.Observability rides the client's normal telemetry: coalesced misses fire the
otel operation-duration and error callbacks like any other command path (no
separate stats API — the engine's internal counters are test-only).
Hardening (from review)
The review rounds hardened the engine's lifecycle and edge behavior, including:
single-flight token settlement on every path (no caller ever hangs, no
reservation leaks); a CAS ownership interlock so a ctx-cancelled caller's
Cmderis never written concurrently;
Closeinterruption of blocked acquisitions andsocket reads (bounded drain, then conn close); retry-uncached fallback when CSC
is disabled mid-miss; GC cleanup for clients dropped without
Close;WithTimeoutclones sharing the coalescer; and held-connection probe safety(no deadline clobbering of concurrent I/O; opaque-transport idle-drain fallback).
Note
High Risk
Large changes to CSC concurrency, connection holding, invalidation timing, and push draining on the hot path; incorrect behavior could cause stale cache, hangs, or connection desync, though coverage is extensive.
Overview
Adds config-driven client-side cache (CSC) improvements: full-duplex reader-miss coalescing, windowed invalidation batching, and extensive lifecycle/correctness hardening. Env-var gates for coalescing/refresh are removed in favor of
Options/UniversalOptionsfields.Miss coalescing (
ClientSideCacheCoalesceMisses,LocalCacheonly) replaces the prototype worker pool with a held tracked connection using concurrent writer/reader goroutines: misses stream out while replies return (~1 RTT), idle sessions release the conn after a grace period, and the reader drains invalidations on the held socket. Includes CAS-basedCmderownership on cancel, wire snapshots at enqueue,errCSCRetryUncachedwhen CSC stops mid-miss, GC cleanup for clients dropped withoutClose, andWithTimeoutclones that bypass the owner’s coalescer when timeouts differ.Invalidation batching (
ClientSideCacheInvalidationBatchWindow) defers per-key deletes to a background batcher with epoch-based supersede on full flush, strictest-window-wins across shared handlers, and refresh-queue stacking so closing one client does not break siblings.Other notable changes: refresh refetches use the main tracked pool (not pipeline); push drains use longer hard deadlines and
HasBufferedData; poolcheckForDataclears stale read deadlines;FT.HYBRIDRESP3 map parsing fixed;proto.Scancopies escaping types;SECURITY.md+ disclosure updates; minor CI/doc bumps.Reviewed by Cursor Bugbot for commit d33c190. Bugbot is set up for automated code reviews on this repo. Configure here.