Add On Air reconnect tests and skip a disconnect in progress - #276
Draft
evnchn wants to merge 2 commits into
Draft
Add On Air reconnect tests and skip a disconnect in progress#276evnchn wants to merge 2 commits into
evnchn wants to merge 2 commits into
Conversation
`Air` decides whether it still needs to reconnect by checking one thing, `self.relay.connected`. That flag can get permanently stuck on `True` after the connection has died, and the keep-alive timer then returns early every five seconds forever, leaving the device unreachable until the process restarts. python-socketio assigns `connected = True` as the last statement of `connect()`, while the cleanup that would clear it is guarded by `if self.connected:`. A transport that dies in between falls through the gap: the cleanup sees an unset flag and does nothing, then `connect()` finishes and sets it anyway. The flag cannot be cleared afterwards -- Engine.IO's `disconnect()` is guarded by `if self.state == 'connected'`, so it never emits the event Socket.IO needs, and `shutdown()` delegates to it. So instead of repairing the wedged client, discard it and build a fresh one through the public API; handler registration moves into `_create_relay()` with the handler bodies unchanged. Auto-reconnect is disabled so the keep-alive timer is the only reconnect driver. This matters for the replacement: a discarded client with auto-reconnect still enabled keeps its own reconnect loop running and could open a second connection to the relay, and `shutdown()` cannot stop it. In-flight streams are closed on replacement, since the discarded client can no longer deliver `close-stream`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Treat only the definitively dead "disconnected" transport state as stale, so a keep-alive tick during a deliberate disconnect does not reconnect.
6 tasks
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.
Motivation
Two things, both on top of the existing fix rather than competing with it:
'connected','disconnecting','disconnected'. The guard isif self.relay.eio.state == 'connected': return, so a keep-alive tick that lands while a deliberate disconnect is running classifies it as stale, replaces the client and reconnects.app.shutdown()→air.disconnect()is exactly that path, and the keep-alive timer fires every 5 s.nicegui/air.pyhad none, so neither the original bug nor this regression is pinned. The stuck-flag mechanism is subtle enough that a future refactor could silently reintroduce it.Implementation
Testing for the definitively dead state instead of the healthy one is strictly narrower and still catches the reported bug: Engine.IO's read loop ends in
_reset(), which setsstate = 'disconnected', so the stuck state is always exactlyconnected == True and eio.state == 'disconnected'.Plus
tests/test_air.py— three cases against aFakeRelaythat reproduces the two python-socketio behaviours the whole bug rests on:disconnect()leavesconnectedset once the transport is already dead, andconnect()rejects whileconnectedisTrue. Without both, the tests would be vacuous.The fixture collects every relay client the
Airinstance creates, so the tests are agnostic to how the stale client is dealt with — they pass equally against "replace the client" and against "clear the flag in place", and would survive a change of approach.Verification — the full A/B matrix (each test seen to fail on the code that lacks its fix)
nicegui/air.pyunder test…transport_died_silently…while_healthy…while_disconnectingupstream/main— no fixassert 0 == 1fix/air-stale-connected-flag— the in-flight fixassert 1 == 0Row 1 proves the tests actually pin the original bug. Row 2 is the finding — and proves the one-word change is load-bearing rather than cosmetic.
Note the captured
WARNINGin row 2 — the in-flight fix does not merely fail to skip the shutdown, it actively logs that it is replacing the client and reconnects.Local gates on this branch:
Where the finding came from, and the library facts behind it
The
'disconnecting'edge was raised by Codex (GPT-5, a different model lineage) reviewing an earlier, independent attempt at this fix, briefed to assume the change was broken:It was flagged against a private draft; checking it against
fix/air-stale-connected-flagshowed the same guard shape there, and the test above confirmed it empirically rather than by reading.Supporting facts, read from the installed libraries rather than from memory:
engineio/async_client.pysets exactly three state values:'disconnecting'(duringdisconnect()),'disconnected'(_reset(), and the base client's initial value),'connected'.'disconnected': the read loop's tail runs_trigger_event('disconnect')then_reset().socketio.AsyncClientthatdisconnect()leavesconnected == Truewheneio.state != 'connected', and thatconnect()then raisesConnectionError('Already connected')— whichair.pyswallows atdebuglevel. That is the permanent stuck state, and it is why replacing the client (or clearing the flag) is necessary at all.Scope — deliberately not included
remote_urland resetting on a repeated 404 — also covers "the relay forgot the device /readynever arrived", which remains unfixed. It carries its own policy questions (probe cadence, consecutive-404 threshold, and correctly doing nothing when the relay is unreachable, since that is an internet outage) and is implemented downstream inair-link.fix/air-stale-connected-flagas the PR base. Keptmainas the base, so the two commits are visible together.Progress
Opened by an autonomous agent under a fork-only charter — not pushed upstream, and no comment left on the upstream PR. Supersedes the earlier duplicate draft in this fork (
fix/6212-air-reconnect), which was opened before that in-flight PR was discovered.