Skip to content

Validate handshake completion on initial connect#975

Draft
caspervonb wants to merge 1 commit into
nats-core-connect-verbose-pedantic-protocol-overridesfrom
nats-core-handshake-completion
Draft

Validate handshake completion on initial connect#975
caspervonb wants to merge 1 commit into
nats-core-connect-verbose-pedantic-protocol-overridesfrom
nats-core-handshake-completion

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

Initial connect only checked for -ERR after sending CONNECT+PING; a server that closed the socket mid-handshake or answered with an unexpected frame fell through silently and connect() returned a CONNECTED client wrapping a dead connection. The reconnect path already validated all three outcomes — this closes the asymmetry flagged in review on #949 by extracting the shared sequence into complete_handshake alongside establish_connection (#948), so the two paths cannot drift again.

Stacked on #949; will retarget to main once that merges.

The initial-connect path only checked for -ERR after CONNECT+PING; a
server that closed the socket or answered with an unexpected frame
fell through silently and produced a CONNECTED client on a dead
connection. The reconnect path already validated all three outcomes.

Move the shared CONNECT+PING+PONG sequence into complete_handshake
next to establish_connection so both paths get the same checks, and
both now special-case authorization failures in the error message.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 10, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 3m 35s —— View job


PR Review

  • Gather context (PR diff, changed files)
  • Review connection.pycomplete_handshake implementation
  • Review __init__.py — initial connect path
  • Review test_client.py — new test coverage
  • Post inline comments and summary

The extraction is clean and the core goal is achieved: initial connect now validates None (EOF mid-handshake) and unexpected frames, closing the gap that was left in the old path. A few things worth knowing:

response potentially unbound (inline comment) — Both except handlers always re-raise, so response is provably assigned at line 514 at runtime. But a type checker that doesn't track "all paths raise" (mypy without narrowing) will flag it as potentially unbound. Initialising response: Message | None = None before the try block costs nothing and future-proofs against a type-checking pass being added to nats-core.

Reconnect now gets auth-aware error messages — The old reconnect path raised ConnectionError("Connection error: ...") for every -ERR. The new shared path checks for "authorization" in the error text and raises ConnectionError("Authorization failed: ...") accordingly. This is a positive improvement, but it's a subtle behaviour change for any caller matching on reconnect error strings. Worth calling out in the PR description if it isn't already clear.

ConnectionError from parse() is now wrapped — The original initial-connect try block had except ConnectionError: raise to pass through errors from the underlying transport unchanged. complete_handshake drops that guard; a ConnectionError raised inside parse() (e.g. from a bad readline call) will now be re-wrapped as ConnectionError("Failed to verify connection: ..."). In practice the message is preserved via from e, so debuggability is fine — just worth knowing the contract changed slightly.

Tests — The two new tests cover exactly the previously-missing scenarios. @pytest.mark.asyncio is redundant with asyncio_mode = "auto" but consistent with the existing test style and harmless.

msg = f"Failed to verify connection: {e}"
raise ConnectionError(msg) from e

if response is None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both except handlers unconditionally re-raise, so response is always assigned when execution reaches here — this is correct at runtime. However, a static type checker that doesn't track that both handlers always raise (e.g. mypy without control-flow narrowing) may flag response as potentially unbound here.

Initialising before the loop makes the intent explicit and silences that class of warning:

Suggested change
if response is None:
response: Message | None = None
try:
while True:
response = await asyncio.wait_for(parse(connection), timeout=timeout)
if not isinstance(response, Ok):
break
except TimeoutError:
await connection.close()
msg = "Server did not respond to PING"
raise ConnectionError(msg)
except Exception as e:
await connection.close()
msg = f"Failed to verify connection: {e}"
raise ConnectionError(msg) from e
if response is None:

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