Repository navigation
Conversation
com6056
left a comment
There was a problem hiding this comment.
Good catch, and it matches how the local-promotion path already handles this. A couple of things.
It could use a test. Here are three that slot into packages/aionanit/tests/test_camera.py (they use its existing imports and _make_camera). The first fails on main and passes here, the second passes either way, and the third still fails here, which is the window inline.
Tests
def _emit_connect(cam: NanitCamera) -> AsyncMock:
"""Mock connect that emits the transitions WsTransport._async_connect does.
The real transport closes the old socket, then reports CONNECTING, then
CONNECTED, so the mock has to as well.
"""
async def connect(*_a: object, **_k: object) -> None:
cam._on_connection_change(ConnectionState.CONNECTING, TransportKind.CLOUD, None)
cam._on_connection_change(ConnectionState.CONNECTED, TransportKind.CLOUD, None)
return AsyncMock(side_effect=connect)
def _stop_loops(cam: NanitCamera) -> None:
cam._cancel_sensor_poll()
cam._cancel_playback_poll()
cam._cancel_token_refresh()
class TestInlineReconnectFailsPending:
async def test_perform_reconnect_fails_pending_requests(self) -> None:
"""A request on the replaced socket fails fast instead of timing out."""
cam, *_ = _make_camera()
cam._transport = MagicMock()
cam._transport.connected = True
cam._transport.idle_seconds = 0.0
cam._transport.transport_kind = TransportKind.CLOUD
cam._transport.async_connect_cloud = _emit_connect(cam)
cam._async_enable_sensor_push = AsyncMock()
future = cam._pending.track(cam._pending.next_id())
try:
await cam._perform_reconnect(force=True)
finally:
_stop_loops(cam)
cam._transport.async_connect_cloud.assert_awaited_once()
assert future.done()
with pytest.raises(NanitTransportError):
future.result()
async def test_skipped_reconnect_leaves_pending_requests_alone(self) -> None:
"""The freshness guard returns before anything is failed."""
cam, *_ = _make_camera()
cam._transport = MagicMock()
cam._transport.connected = True
cam._transport.idle_seconds = 0.0
cam._transport.async_connect_cloud = AsyncMock()
future = cam._pending.track(cam._pending.next_id())
await cam._perform_reconnect(force=False)
cam._transport.async_connect_cloud.assert_not_awaited()
assert not future.done()
async def test_request_sent_during_token_fetch_is_failed(self) -> None:
"""A request tracked while the reconnect awaits its token is failed too.
The old socket is still open during the token fetch, so a concurrent
caller can send on it. The transport reports CONNECTING only after it
has closed that socket, which is where pending requests get failed.
"""
cam, tm, _ = _make_camera()
gate = asyncio.Event()
late: list[asyncio.Future[Response]] = []
async def slow_token(*_a: object, **_k: object) -> str:
late.append(cam._pending.track(cam._pending.next_id()))
await gate.wait()
return "tok"
tm.async_get_access_token = AsyncMock(side_effect=slow_token)
cam._transport = MagicMock()
cam._transport.connected = True
cam._transport.idle_seconds = 0.0
cam._transport.transport_kind = TransportKind.CLOUD
cam._transport.async_connect_cloud = _emit_connect(cam)
cam._async_enable_sensor_push = AsyncMock()
task = asyncio.create_task(cam._perform_reconnect(force=True))
await asyncio.sleep(0)
gate.set()
try:
await task
finally:
_stop_loops(cam)
assert late and late[0].done()
with pytest.raises(NanitTransportError):
late[0].result()Could you also drop the Co-Authored-By trailer from the commit (AGENTS.md asks for no AI co-author trailers) and the Claude Code footer from the PR body?
| # Fail them now (as the local-promotion path does) so callers retry | ||
| # on the new session instead of each waiting out a full timeout and | ||
| # then forcing yet another reconnect. | ||
| self._pending.cancel_all(NanitTransportError("Reconnecting")) |
There was a problem hiding this comment.
This fails pending requests a bit too early. The old socket stays connected until _async_connect closes it, and between here and there the reconnect awaits the token (a full REST refresh when it needs one). A request sent in that window (the sensor and playback polls keep running through a reconnect) still gets tracked on the dying socket and waits its full 10s. Failing pending requests on ConnectionState.CONNECTING in _on_connection_change would cover it, since the transport only emits that after closing the old socket. Then this call can go:
- if state in (ConnectionState.DISCONNECTED, ConnectionState.RECONNECTING):
+ # CONNECTING fires once the transport has closed the previous socket,
+ # so nothing sent on it can still be answered.
+ if state in (
+ ConnectionState.CONNECTING,
+ ConnectionState.DISCONNECTED,
+ ConnectionState.RECONNECTING,
+ ):
reason = (
- "Connection reconnecting"
- if state == ConnectionState.RECONNECTING
- else "Connection lost"
+ "Connection lost"
+ if state == ConnectionState.DISCONNECTED
+ else "Connection reconnecting"
)The other paths that reach CONNECTING are safe (the first connect has nothing pending, local promotion does its own cancel_all, and the local probe's throwaway transport doesn't report to the camera). With that change the full suite plus ruff and mypy pass, and the third test in the summary passes too.
An inline reconnect replaced the socket without failing the requests already sent on it, so each one waited out its full 10 s timeout and then asked for another reconnect. Fail pending requests when the transport reports CONNECTING. It reports that only after closing the previous socket, so this also covers requests sent while the reconnect awaits its token (the sensor and playback polls keep running through a reconnect). The other paths that reach CONNECTING have nothing pending (first connect) or cancel on their own (local promotion). Tests written by @com6056 in review.
ebf3b28 to
22f17da
Compare
|
Applied the |
com6056
left a comment
There was a problem hiding this comment.
Looks good, thanks for taking the change and the tests as is! One optional extra if you feel like it. cancel_all now runs on every inline reconnect, and its if not future.done() check in ws/pending.py is the only thing that stops an InvalidStateError when a caller was cancelled mid-request and left its future behind (a stop wrapped in asyncio.timeout does exactly that). Removing that check keeps the whole suite green, so a small test that tracks a future, cancels it, emits CONNECTING and checks pending_count == 0 would pin it.
What does this do
An inline reconnect (
_perform_reconnect) replaced the socket without failing the requests already sent on it, so each one waited out its full 10 s timeout and then asked for another reconnect. During an incident on my install, a request sent on the old socket did exactly that.As suggested in review, pending requests are now failed when the transport reports
CONNECTING. It reports that only after closing the previous socket, so this also covers a request sent while the reconnect awaits its token (the sensor and playback polls keep running through a reconnect). The other paths that reachCONNECTINGare safe: the first connect has nothing pending, local promotion cancels on its own, and the local probe's throwaway transport doesn't report to the camera.Changelog
How I tested this
packages/aionanit/tests/test_camera.py: without the change the first and third fail, and the second passes either way; with it all three pass.ruff check,ruff format --check,mypyand both test suites pass locally (Raspberry Pi 4, the locked toolchain viauv sync). One library test also fails on unmodifiedmainon this machine and is untouched here:TestRealDemuxer::test_pyav_reads_cold_start_and_late_join(PyAV "Immediate exit requested").main.Checklist
just checkpasses (lint, types, tests), apart from the machine-specific failure abovejust dev): not done, the behaviour is covered by the tests aboveSuggested label:
release:patch.