Skip to content

redis: retry the eviction subscription properly, and keep reconnect off the lock - #77

Draft
b7r6 wants to merge 2 commits into
mainfrom
fix/redis-subscription-followups
Draft

b7r6 wants to merge 2 commits into
mainfrom
fix/redis-subscription-followups

Conversation

@b7r6

@b7r6 b7r6 commented Oct 22, 2026

Copy link
Copy Markdown
Owner

What and why

redis: retry the eviction subscription properly, and keep reconnect off the lock

Follow-ups from review of the subscription-lifecycle fix.

reconnect: subscribing the new connection to patterns recorded mid-reconnect
happened while holding the connection write lock, so every get_connection
(every Redis operation) waited on those round trips. Subscribing now happens
without the lock; the lock is taken only to check nothing is still missing and
to publish, repeating if a pattern arrived in between. A pattern recorded
after the final check still waits for the lock in get_connection and then
subscribes on the new connection, so the original race stays closed.

Eviction subscription retry:

  • The task holds a weak reference, so a dropped store ends it instead of
    being kept alive and retried forever.
  • It logs the first failure as an error, then only at powers of two, so a
    permanent failure (an ACL forbidding PSUBSCRIBE) doesn't log every 5 s.
  • On a connection-type failure it re-resolves the connection, as the store's
    other retry paths do, instead of retrying a dead handle.
  • Reading notify-keyspace-events stays advisory, now logged at warn with its
    own message so it can't be mistaken for the subscription failing.
    redis: require Send of the connection in RedisStore's inherent impl

warn_if_eviction_events_are_disabled holds a ClientWithPermit<C> across
an await, which the nightly clippy lane (future_not_send) rejects while
C is only Sync there. Every connection type used is Send, and the
StoreDriver impl already requires it.

How was this verified?

Tests:

  • The existing init test only covered the advisory CONFIG GET and passed
    without any retry. A new test fails the first PSUBSCRIBE and checks the
    subscription is still established; it fails if the retry loop gives up.
  • A new test checks a dropped store ends the retry task; it fails if the task
    holds a strong reference.
  • A new unit test checks get_connection is not blocked while reconnect
    catches up; it fails against the previous locking.
  • The reconnect race test uses #[nativelink_test] instead of a hand-built
    runtime, so the clippy and lint exemptions are gone, and the test names now
    describe the fixed behaviour.
  • The fake Redis gains make_fake_redis_with_first_responses, which answers a
    command differently the first time only.

Also part of the soak branch (#73): upstream HEAD plus every pending fix, run through the railgun fleet for an hour of continuous remote builds with no errors.

Risk

Reconnect no longer holds the connection write lock across PSUBSCRIBE round trips; the lock is taken only to check and publish. The eviction-subscription retry task holds a weak reference and backs off its logging. The last commit adds a Send bound the nightly clippy lane requires (future_not_send); every connection type in use is Send.

AI assistance

Written with Claude Code; b7r6 reviewed the change. Draft until the author has read the CI results.

🤖 Generated with Claude Code

b7r6 and others added 2 commits October 22, 2026 14:54
…ff the lock

Follow-ups from review of the subscription-lifecycle fix.

reconnect: subscribing the new connection to patterns recorded mid-reconnect
happened while holding the connection write lock, so every get_connection
(every Redis operation) waited on those round trips. Subscribing now happens
without the lock; the lock is taken only to check nothing is still missing and
to publish, repeating if a pattern arrived in between. A pattern recorded
after the final check still waits for the lock in get_connection and then
subscribes on the new connection, so the original race stays closed.

Eviction subscription retry:
- The task holds a weak reference, so a dropped store ends it instead of
  being kept alive and retried forever.
- It logs the first failure as an error, then only at powers of two, so a
  permanent failure (an ACL forbidding PSUBSCRIBE) doesn't log every 5 s.
- On a connection-type failure it re-resolves the connection, as the store's
  other retry paths do, instead of retrying a dead handle.
- Reading notify-keyspace-events stays advisory, now logged at warn with its
  own message so it can't be mistaken for the subscription failing.

Tests:
- The existing init test only covered the advisory CONFIG GET and passed
  without any retry. A new test fails the first PSUBSCRIBE and checks the
  subscription is still established; it fails if the retry loop gives up.
- A new test checks a dropped store ends the retry task; it fails if the task
  holds a strong reference.
- A new unit test checks get_connection is not blocked while reconnect
  catches up; it fails against the previous locking.
- The reconnect race test uses #[nativelink_test] instead of a hand-built
  runtime, so the clippy and lint exemptions are gone, and the test names now
  describe the fixed behaviour.
- The fake Redis gains make_fake_redis_with_first_responses, which answers a
  command differently the first time only.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
`warn_if_eviction_events_are_disabled` holds a `ClientWithPermit<C>` across
an await, which the nightly clippy lane (`future_not_send`) rejects while
`C` is only `Sync` there. Every connection type used is `Send`, and the
`StoreDriver` impl already requires it.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant