Skip to content

Reset keepalive state on reconnect#980

Draft
caspervonb wants to merge 1 commit into
mainfrom
reset-ping-state-on-reconnect
Draft

Reset keepalive state on reconnect#980
caspervonb wants to merge 1 commit into
mainfrom
reset-ping-state-on-reconnect

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

A reconnect caused by reaching max_outstanding_pings inherited the maxed _pings_outstanding counter: the reconnect success path restored the connection and subscriptions but never reset keepalive state. The new write loop saw the stale counter on its first idle timeout and force-disconnected again one ping_interval later — reconnecting forever.

Reset _pings_outstanding / _last_ping_sent / _last_pong_received on reconnect, mirroring initial-connect setup. Covered by a regression test that puts the client in the ping-exhausted state, forces a reconnect, and asserts the counter is cleared and no spiral follows.

The reconnect success path restored the connection and subscriptions but
never reset _pings_outstanding, so a reconnect caused by reaching
max_outstanding_pings inherited the maxed counter. The new write loop
re-tripped it on its first idle timeout and force-disconnected again one
ping_interval later, reconnecting forever. Reset the keepalive counters
on reconnect as initial connect already does.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 22, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 1m 34s —— View job


PR Review: Reset keepalive state on reconnect

  • Gather context (PR diff, changed files)
  • Review implementation changes in nats-core/src/nats/client/__init__.py
  • Review test coverage in nats-core/tests/test_client.py
  • Post inline comments and summary

Fix is correct and the root cause analysis is sound. The three-field reset (_pings_outstanding, _last_pong_received, _last_ping_sent) exactly mirrors __init__'s setup and is placed at the right point in the reconnect path — after the new connection is confirmed with a PONG, before the new read/write tasks start. Without the fix, _write_loop would see _pings_outstanding >= _max_outstanding_pings on its first idle timeout and immediately call _force_disconnect() again.

_pong_waker doesn't need resetting_ping() calls self._pong_waker.clear() before every use, so any stale "set" state from the prior connection is harmless.

Two minor notes left as inline comments:

  1. get_event_loop() vs get_running_loop() — the reconnect path correctly uses asyncio.get_running_loop().time() but __init__ still uses the deprecated asyncio.get_event_loop().time() pattern. Not introduced by this PR, but worth aligning.
  2. Comment length — the 4-line block comment is more verbose than the one-line-max style in CLAUDE.md; the why can be captured in one line.

Test is solid — directly setting _pings_outstanding = _max_outstanding_pings gives a deterministic repro without needing a mock server, and the 3-interval sleep (ping_interval=0.2, sleep=0.6) is long enough to catch a spiral without flaking. The post-reconnect pub/sub round-trip confirms the connection stays usable.

# ping_interval later (mirrors initial-connect setup).
self._pings_outstanding = 0
self._last_pong_received = asyncio.get_running_loop().time()
self._last_ping_sent = self._last_pong_received

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor inconsistency: __init__ timestamps these fields with asyncio.get_event_loop().time() (lines 457–458), while the reconnect path uses asyncio.get_running_loop().time(). Both return the same loop time in an async context, and get_running_loop() is actually the more correct call here — but the inconsistency could confuse a future reader auditing timer semantics. Worth aligning __init__ to get_running_loop() in a follow-up (or here).

Comment on lines +985 to +988
# Reset keepalive state so a reconnect caused by ping
# exhaustion doesn't inherit the stale counter and have
# the new write loop force-disconnect again one
# ping_interval later (mirrors initial-connect setup).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is accurate and the bug it describes is non-obvious, so a comment is warranted. CLAUDE.md asks for one-line max though — something like # Reset keepalive state; a ping-exhaustion reconnect must not inherit a stale counter. would capture the why without the four lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant