Repository navigation
Conversation
RedisTokenManager's two subscriber tasks ran under ensure_task's default limit, so five errors within a minute ended them. After a Redis outage of more than a few seconds, clients already connected to a worker stopped receiving updates produced on other workers until a new socket connected to that worker and restarted the tasks. ensure_task now accepts exception_limit=None: the task retries without a limit, the delay doubles after each error in a row up to exception_delay_max (30 seconds by default), only the first error of a run is logged, and the coroutine function is passed a `recovered` callable that logs the end of the run and resets the delay. Both subscribers use it and call `recovered` when Redis confirms the subscription. The defaults are unchanged for other callers, and CancelledError and RuntimeError still propagate. On every confirmed subscription the socket record subscriber also stores this instance's own socket records that are missing from Redis, since a record that expired while it was not listening is not announced again. Fixes reflex-dev#7835
Contributor
[Medium impact] The PR appears safe to merge. SummaryThe Redis subscribers now keep retrying through outages and restore missing socket records when they reconnect.
YusefSyed notes that a disconnect during restoration can leave a stale Redis record until it expires; this race also exists on Reviews (2) · Last reviewed commit: "Adopt socket records claimed during an o..." · Reviewed by Greptile |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
The restore's SET NX keeps a record that another instance stored while the subscriber was not listening, but this instance kept its own local record for that token: updates produced here still went to its own socket, and a disconnect of that socket deleted the other instance's record. The restore now reads each key back in the same pipeline and, where Redis holds a different record than the one it wrote, refreshes the local entry through _get_token_owner, as _handle_socket_record_del does when its own SET NX is rejected. Without an exception limit, an exception_delay above exception_delay_max was used as is for the first retry and again after each recovery. Both now use the smaller of the two.
This branch has not been deployed
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.
All Submissions:
Type of change
Changes To Core Features:
Description
Fixes #7835
RedisTokenManager's two subscriber tasks ran underensure_task's default limit, so five errors within a minute ended them, and nothing started them again until a new socket reached that worker. This follows the fix direction in the issue:ensure_taskand_run_foreveracceptexception_limit=None. The task then retries without a limit, the delay doubles after each error in a row up toexception_delay_max(30 s by default), which also caps the first delay, only the first error of a run is logged, and the coroutine function is passed arecoveredcallable that logs the end of the run and resets the delay. The defaults are unchanged, andCancelledErrorandRuntimeErrorstill propagate.exception_limit=Noneand callrecoveredwhenlisten()yields thepsubscribeconfirmation, the signalStateManagerRedis._subscribe_lock_updatesalready waits for. An outage that raises in a subscriber logs two lines for it, one ERROR and one INFO:_subscribe_lost_and_found_updates: task error, retrying until it recovers: <error>and_subscribe_lost_and_found_updates: task recovered.SET NXand reads each key back in the same pipeline, so a record that expired while it was not listening exists again. Where Redis holds a different record, such as one another worker stored for that token in the meantime, that record is left alone and the local entry is refreshed through_get_token_owner, as_handle_socket_record_deldoes when its ownSET NXis rejected. A failed write is logged once and the subscription stays up.I left out the restart from the emit path. I tried it on
mainwith twoRedisTokenManagers over one fake Redis: re-ensuring the task on the instance that publishes left the task on the instance that owns the socket stopped, and the delta reached no subscriber.emit_lost_and_foundruns on the publisher, so it cannot restart the owner's task. It would restart the publisher's own subscribers; I can add that if you want it.What this does not cover
main.main; that worker's subscriber writes it back when it receives the deletion.mock_redis; the same interleaving leaves the same record onmainthrough_handle_socket_record_del.CONFIG SETis logged once and then retried with no further lines.StateManagerRedis._ensure_lock_taskkeeps the default limit. It is re-ensured on each lock wait.Tests
tests/units/utils/test_token_manager.py, onmock_rediswith a pubsub that can be made to fail:test_lost_and_found_subscriber_survives_redis_outage(two managers: a delta is delivered, the pubsub fails seven times, the two log lines are compared in full, the next delta is delivered),test_socket_record_subscriber_survives_redis_outage(the record deleted during the outage is stored again, one claimed by another instance is left alone and adopted, and the disconnect of the socket that lost it does not delete it),test_socket_record_subscriber_retries_refused_configandtest_socket_record_subscriber_restores_records_on_resubscribe. Onmainthe first three fail on the log comparison, after the task has ended withtask exceeded exception limit 5 within 60.0s, and the fourth because the record is not restored.test_restore_socket_records_adopts_newer_recordsruns the restore directly: a missing record is written back, an unchanged one is not read again, a newer one is adopted, one deleted between the pipeline and the read leaves the local record as it was, and a failed pipeline is logged once and reads nothing. Onmainit fails because the method does not exist.tests/units/utils/test_tasks.py:test_ensure_task_no_limitcompares the delays (1, 2, 4, 8, 16, 30, 30 s, then 1 s after a recovery, and the cap throughout when the first delay is above it) and the log records in full;test_ensure_task_no_limit_cancel_during_retry_delaycancels during a 30 s delay. Both use the new argument, so onmainthey fail with aTypeErrorand a timeout, not with the issue's message.test_ensure_task_default_limitandtest_close_during_subscriber_retry_delaypin behaviour that does not change and pass onmaintoo.mock_redisgainspublish, with a test intests/units/test_mock_redis.py.Run locally on macOS arm64 with Python 3.13.14 and 3.11.15, same result on both:
The 5 skipped tests need a Redis URL. The full unit suite also passes on both (
pytest tests/units --cov --no-cov-on-fail --cov-report=: 15314 tests pass and 33 are skipped on 3.13, 15306 pass and 34 are skipped on 3.11), and the twelve new test cases passed 30 runs in a row.ruff check .andruff format --check .pass,pyright --pythonversion 3.14 reflex testsreports 0 errors, and the pre-commit hooks pass withbiome-formatskipped. Changelog fragment:news/+redis-subscriber-outage.bugfix.md.Not run: a real Redis server (none is installed here), two worker processes and the integration tests. I ran redis-py 8.1.0's client against a small RESP stub, which is not Redis: after a dropped connection that can be re-established it resubscribes by itself and
listen()yields a newpsubscribeconfirmation without raising, so the restore runs then too.This change was prepared with an AI coding assistant (Claude Code) working at my direction on my machine.