fix: drop held feedback across a reconnect instead of resending it - #5
Merged
Merged
Conversation
The feedback path retried on a new connection after every close except an explicit resync. That looks like resilience and is the opposite. A transition is built across two messages. The infer the server answered is where it recorded the previous observation, the policy step state and the done flags, and all of that lives in the connection handler. A new connection starts with none of it. So a feedback resent after a reconnect does not save the step: the server completes the transition from an empty observation and stores it, and nothing anywhere reports an error. The buffer just quietly gets a hole in it, and gets more of them the more often the connection drops. The resync branch already knew this and returned. It was the only branch that did, and resync is not the common way a connection dies - a keepalive ping timeout is, which is what turned this up. So every close now drops the transition and lets the next infer resynchronise, which is the whole point of the exchange starting with an infer. The infer path deliberately still retries. A fresh infer on a fresh connection is exactly how the two sides get back in step, and the tests assert that too, so a later cleanup does not "fix" both paths the same way. Specified as SPEC section 7.6 in PlugRL/plugrl-protocol#3; the server half, which stops provoking the reconnect in the first place, is PlugRL/plugrl-server#4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 11, 2026
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.
The bug
feedback()retried its send on a new connection after every close except an explicit resync. That reads as resilience. It is the opposite.A transition spans two messages. The
inferthe server answered is where it recorded the previous observation, the policy step state and the done flags — and all of that lives inside the connection handler. A new connection starts with none of it.So a resent
feedbackdoes not save the step. The server completes the transition from an empty observation and stores it. No exception, no warning, one corrupt transition in the training buffer per reconnect.How it was found, and one correction
A clean-machine run of the documented quickstart dropped its connection and reconnected. The first diagnosis blamed WebSocket keepalive pings colliding with a CPU-bound learn step; that was then measured and is false —
plugrl-serverrunslearnoff the event loop, and learns of 190 s produce no ping timeout. The actual cause was the machine suspending for nearly two hours mid-run.That correction does not touch this pull request. The state-loss path was read out of the server's handler and reproduced in a unit test, not inferred from the incident. Only the cause was inferred, and the cause is the part that was wrong. Every reconnect costs the same state, whatever produced it: a suspend, a flaky link, an operator restarting the server.
The measurement is written up in PlugRL/plugrl-server#4 as
experiments/e8-keepalive-hypothesis/.E6, the learning-curve experiment, was checked against this: one connection per seed, zero reconnects, so that data is unaffected.
The change
inferreconnects and resynchronises, which is what the exchange beginning with aninferis for;The resync branch already did the right thing. It was simply applied to the rarest close reason instead of all of them.
Tests
tests/test_reconnect_drops_feedback.py, 4 cases: keepalive timeout, plain close, resync, and the infer mirror image. Verified to fail against the previous behaviour rather than passing vacuously. Full suite 135 passed, 10 skipped.ruff check --exclude third_party .clean.Specified as SPEC §7.6 in PlugRL/plugrl-protocol#3.
🤖 Generated with Claude Code