From fb1771b4ded328e7648a5f61fc77c3cfa7c570b5 Mon Sep 17 00:00:00 2001 From: Casper Beyer Date: Wed, 27 May 2026 20:06:06 +0200 Subject: [PATCH 1/5] Add remove_*_callback methods to nats-core Client --- nats-core/src/nats/client/__init__.py | 40 +++++++ nats-core/tests/test_client.py | 148 ++++++++++++++++++++++++++ 2 files changed, 188 insertions(+) diff --git a/nats-core/src/nats/client/__init__.py b/nats-core/src/nats/client/__init__.py index acaa9de5..b9fe87e0 100644 --- a/nats-core/src/nats/client/__init__.py +++ b/nats-core/src/nats/client/__init__.py @@ -1471,6 +1471,16 @@ def add_disconnected_callback(self, callback: Callable[[], None]) -> None: """ self._disconnected_callbacks.append(callback) + def remove_disconnected_callback(self, callback: Callable[[], None]) -> None: + """Remove a previously registered disconnected callback. + + Raises ``ValueError`` if ``callback`` was not registered. + + Args: + callback: Function previously passed to :meth:`add_disconnected_callback`. + """ + self._disconnected_callbacks.remove(callback) + def add_reconnected_callback(self, callback: Callable[[], None]) -> None: """Add a callback to be invoked when the client is reconnected. @@ -1479,6 +1489,16 @@ def add_reconnected_callback(self, callback: Callable[[], None]) -> None: """ self._reconnected_callbacks.append(callback) + def remove_reconnected_callback(self, callback: Callable[[], None]) -> None: + """Remove a previously registered reconnected callback. + + Raises ``ValueError`` if ``callback`` was not registered. + + Args: + callback: Function previously passed to :meth:`add_reconnected_callback`. + """ + self._reconnected_callbacks.remove(callback) + def add_error_callback(self, callback: Callable[[Exception | str], None]) -> None: """Add a callback to be invoked when the client encounters an error. @@ -1487,6 +1507,16 @@ def add_error_callback(self, callback: Callable[[Exception | str], None]) -> Non """ self._error_callbacks.append(callback) + def remove_error_callback(self, callback: Callable[[Exception | str], None]) -> None: + """Remove a previously registered error callback. + + Raises ``ValueError`` if ``callback`` was not registered. + + Args: + callback: Function previously passed to :meth:`add_error_callback`. + """ + self._error_callbacks.remove(callback) + def add_lame_duck_mode_callback(self, callback: Callable[[], None]) -> None: """Add a callback to be invoked when the server enters lame duck mode. @@ -1506,6 +1536,16 @@ def add_lame_duck_mode_callback(self, callback: Callable[[], None]) -> None: """ self._lame_duck_mode_callbacks.append(callback) + def remove_lame_duck_mode_callback(self, callback: Callable[[], None]) -> None: + """Remove a previously registered lame duck mode callback. + + Raises ``ValueError`` if ``callback`` was not registered. + + Args: + callback: Function previously passed to :meth:`add_lame_duck_mode_callback`. + """ + self._lame_duck_mode_callbacks.remove(callback) + def _setup_nkey_auth( nkey: str | Path | tuple[Callable[[], str], Callable[[str], bytes]], diff --git a/nats-core/tests/test_client.py b/nats-core/tests/test_client.py index 12c7f4a8..958ca819 100644 --- a/nats-core/tests/test_client.py +++ b/nats-core/tests/test_client.py @@ -3416,3 +3416,151 @@ async def test_force_reconnect_raises_when_drained(server): with pytest.raises(ConnectionError): await client.force_reconnect() + + +@pytest.mark.asyncio +async def test_remove_disconnected_callback_skips_invocation(): + """A removed disconnected callback is not invoked when the client disconnects.""" + server = await run(port=0) + + client = await connect( + server.client_url, + timeout=1.0, + allow_reconnect=True, + reconnect_time_wait=0.1, + ) + + removed_called = False + kept_called = asyncio.Event() + + def removed_cb(): + nonlocal removed_called + removed_called = True + + def kept_cb(): + kept_called.set() + + client.add_disconnected_callback(removed_cb) + client.add_disconnected_callback(kept_cb) + client.remove_disconnected_callback(removed_cb) + + try: + await server.shutdown() + await asyncio.wait_for(kept_called.wait(), timeout=2.0) + assert removed_called is False + finally: + await client.close() + + +@pytest.mark.asyncio +async def test_remove_reconnected_callback_skips_invocation(): + """A removed reconnected callback is not invoked when the client reconnects.""" + server = await run(port=0) + server_port = server.port + + client = await connect( + server.client_url, + timeout=1.0, + allow_reconnect=True, + reconnect_time_wait=0.1, + ) + + removed_called = False + kept_called = asyncio.Event() + + def removed_cb(): + nonlocal removed_called + removed_called = True + + def kept_cb(): + kept_called.set() + + client.add_reconnected_callback(removed_cb) + client.add_reconnected_callback(kept_cb) + client.remove_reconnected_callback(removed_cb) + + try: + await server.shutdown() + new_server = await run(port=server_port) + try: + await asyncio.wait_for(kept_called.wait(), timeout=5.0) + assert removed_called is False + finally: + await new_server.shutdown() + finally: + await client.close() + + +@pytest.mark.asyncio +async def test_remove_error_callback_skips_invocation(client): + """A removed error callback is not invoked when the client surfaces an error.""" + test_subject = f"test.remove_error_callback.{uuid.uuid4()}" + + removed_called = False + kept_errors: list[Exception | str] = [] + + def removed_cb(_error): + nonlocal removed_called + removed_called = True + + def kept_cb(error): + if isinstance(error, SlowConsumerError): + kept_errors.append(error) + + client.add_error_callback(removed_cb) + client.add_error_callback(kept_cb) + client.remove_error_callback(removed_cb) + + await client.subscribe(test_subject, max_pending_messages=5) + await client.flush() + + for i in range(20): + await client.publish(test_subject, f"message-{i}".encode()) + await client.flush() + await asyncio.sleep(0.2) + + assert len(kept_errors) == 1 + assert removed_called is False + + +@pytest.mark.skipif(sys.platform == "win32", reason="SIGUSR2 is POSIX only") +@pytest.mark.asyncio +async def test_remove_lame_duck_mode_callback_skips_invocation(client, server): + """A removed lame duck mode callback is not invoked when LDM is signalled.""" + removed_called = False + kept_called = asyncio.Event() + + def removed_cb(): + nonlocal removed_called + removed_called = True + + def kept_cb(): + kept_called.set() + + client.add_lame_duck_mode_callback(removed_cb) + client.add_lame_duck_mode_callback(kept_cb) + client.remove_lame_duck_mode_callback(removed_cb) + + server.lame_duck_mode() + await asyncio.wait_for(kept_called.wait(), timeout=5.0) + assert removed_called is False + + +@pytest.mark.asyncio +async def test_remove_callback_raises_when_not_registered(client): + """remove_*_callback raises ValueError when the callback was never registered.""" + + def never_registered(): + pass + + def never_registered_error(_error): + pass + + with pytest.raises(ValueError): + client.remove_disconnected_callback(never_registered) + with pytest.raises(ValueError): + client.remove_reconnected_callback(never_registered) + with pytest.raises(ValueError): + client.remove_error_callback(never_registered_error) + with pytest.raises(ValueError): + client.remove_lame_duck_mode_callback(never_registered) From c03eb731e5a653294ba75a8b68728d5fb39307b2 Mon Sep 17 00:00:00 2001 From: Casper Beyer Date: Sun, 31 May 2026 17:00:22 +0200 Subject: [PATCH 2/5] Deregister heartbeat callbacks when pull consumer stops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pull message streams and batches register disconnect/reconnect callbacks on the client for the heartbeat timer but never removed them, so each created-then-stopped consumer left two callbacks — and the stream object they bound — alive for the connection's lifetime. Remove them from the same cleanup paths that already tear down the subscription. --- .../src/nats/jetstream/consumer/pull.py | 35 ++++++++++--- nats-jetstream/tests/test_consumer.py | 51 +++++++++++++++++++ 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/nats-jetstream/src/nats/jetstream/consumer/pull.py b/nats-jetstream/src/nats/jetstream/consumer/pull.py index e4beb686..765c4c67 100644 --- a/nats-jetstream/src/nats/jetstream/consumer/pull.py +++ b/nats-jetstream/src/nats/jetstream/consumer/pull.py @@ -24,7 +24,7 @@ from nats.jetstream.util import new_inbox if TYPE_CHECKING: - from nats.client import Subscription + from nats.client import Client, Subscription from nats.client.message import Message as ClientMessage from nats.jetstream.stream import Stream @@ -59,6 +59,7 @@ class PullMessageBatch(MessageBatch): _heartbeat_deadline: float | None _heartbeat_paused: bool _heartbeat_remaining: float | None + _client: Client | None def __init__( self, @@ -80,10 +81,11 @@ def __init__( self._heartbeat_remaining = None # Register disconnect/reconnect callbacks for heartbeat timer (ADR-37) + self._client = None if heartbeat is not None: - client = jetstream._client - client.add_disconnected_callback(self._pause_heartbeat_timer) - client.add_reconnected_callback(self._resume_heartbeat_timer) + self._client = jetstream._client + self._client.add_disconnected_callback(self._pause_heartbeat_timer) + self._client.add_reconnected_callback(self._resume_heartbeat_timer) def _pause_heartbeat_timer(self) -> None: """Pause the heartbeat timer on disconnect (ADR-37).""" @@ -98,6 +100,13 @@ def _resume_heartbeat_timer(self) -> None: self._heartbeat_paused = False self._heartbeat_remaining = None + def _deregister_callbacks(self) -> None: + """Remove the heartbeat callbacks registered on the client (ADR-37).""" + if self._client is not None: + self._client.remove_disconnected_callback(self._pause_heartbeat_timer) + self._client.remove_reconnected_callback(self._resume_heartbeat_timer) + self._client = None + @property def error(self) -> Exception | None: return self._error @@ -109,6 +118,7 @@ async def __anext__(self) -> Message: if self._terminated or self._pending_messages <= 0: if not self._terminated: await self._subscription.unsubscribe() + self._deregister_callbacks() self._terminated = True raise StopAsyncIteration @@ -207,6 +217,7 @@ async def __anext__(self) -> Message: except (StopAsyncIteration, asyncio.TimeoutError): if not self._terminated: await self._subscription.unsubscribe() + self._deregister_callbacks() self._terminated = True raise StopAsyncIteration @@ -233,6 +244,7 @@ class PullMessageStream(MessageStream): _heartbeat_deadline: float | None _heartbeat_paused: bool _heartbeat_remaining: float | None + _client: Client | None def __init__( self, @@ -277,10 +289,11 @@ def __init__( self._heartbeat_deadline = time.time() + (heartbeat * 2) if heartbeat is not None else None # Register disconnect/reconnect callbacks for heartbeat timer (ADR-37) + self._client = None if heartbeat is not None: - client = consumer._stream._jetstream._client - client.add_disconnected_callback(self._pause_heartbeat_timer) - client.add_reconnected_callback(self._resume_heartbeat_timer) + self._client = consumer._stream._jetstream._client + self._client.add_disconnected_callback(self._pause_heartbeat_timer) + self._client.add_reconnected_callback(self._resume_heartbeat_timer) def _pause_heartbeat_timer(self) -> None: """Pause the heartbeat timer on disconnect (ADR-37).""" @@ -295,6 +308,13 @@ def _resume_heartbeat_timer(self) -> None: self._heartbeat_paused = False self._heartbeat_remaining = None + def _deregister_callbacks(self) -> None: + """Remove the heartbeat callbacks registered on the client (ADR-37).""" + if self._client is not None: + self._client.remove_disconnected_callback(self._pause_heartbeat_timer) + self._client.remove_reconnected_callback(self._resume_heartbeat_timer) + self._client = None + @property def is_active(self) -> bool: """Check if the message stream is still active.""" @@ -495,6 +515,7 @@ async def _cleanup(self): pass self._heartbeat_task = None + self._deregister_callbacks() await self._subscription.unsubscribe() diff --git a/nats-jetstream/tests/test_consumer.py b/nats-jetstream/tests/test_consumer.py index 7c51d1e8..a7f85377 100644 --- a/nats-jetstream/tests/test_consumer.py +++ b/nats-jetstream/tests/test_consumer.py @@ -377,6 +377,57 @@ async def collect_messages(): await message_stream.stop() +@pytest.mark.asyncio +async def test_messages_deregisters_heartbeat_callbacks_on_stop(jetstream: JetStream): + """Regression for #962: stopping a heartbeat message stream removes the + disconnect/reconnect callbacks it registered, so repeatedly creating and + stopping streams over one connection does not leak callbacks.""" + client = jetstream._client + stream = await jetstream.create_stream(name="hb_leak_stream", subjects=["HBLEAK.*"]) + consumer = await stream.create_consumer(name="hb_leak_consumer") + + disconnected = len(client._disconnected_callbacks) + reconnected = len(client._reconnected_callbacks) + + for _ in range(5): + message_stream = await consumer.messages(max_messages=10, heartbeat=5.0) + # Registration is observable while the stream is active. + assert len(client._disconnected_callbacks) == disconnected + 1 + assert len(client._reconnected_callbacks) == reconnected + 1 + await message_stream.stop() + # Stopping deregisters exactly what it registered, leaving no residue. + assert len(client._disconnected_callbacks) == disconnected + assert len(client._reconnected_callbacks) == reconnected + + # A second stop() is a no-op and does not raise. + await message_stream.stop() + assert len(client._disconnected_callbacks) == disconnected + assert len(client._reconnected_callbacks) == reconnected + + +@pytest.mark.asyncio +async def test_fetch_deregisters_heartbeat_callbacks_on_exhaustion(jetstream: JetStream): + """Regression for #962: a heartbeat fetch batch deregisters its callbacks + once the batch is exhausted (StopAsyncIteration), not just on stream stop().""" + client = jetstream._client + stream = await jetstream.create_stream(name="hb_fetch_stream", subjects=["HBFETCH.*"]) + consumer = await stream.create_consumer(name="hb_fetch_consumer") + + disconnected = len(client._disconnected_callbacks) + reconnected = len(client._reconnected_callbacks) + + # No messages published — the batch ends via timeout, exhausting the iterator. + batch = await consumer.fetch(max_messages=5, max_wait=0.5, heartbeat=1.0) + assert len(client._disconnected_callbacks) == disconnected + 1 + assert len(client._reconnected_callbacks) == reconnected + 1 + + async for _ in batch: + pass + + assert len(client._disconnected_callbacks) == disconnected + assert len(client._reconnected_callbacks) == reconnected + + @pytest.mark.asyncio async def test_messages_rejects_both_max_messages_and_max_bytes(jetstream: JetStream): """Test ADR-37: messages() cannot accept both max_messages and max_bytes simultaneously.""" From 1cd5e304e3386bd934af2407b32c32306458b3fe Mon Sep 17 00:00:00 2001 From: Casper Beyer Date: Wed, 10 Jun 2026 19:04:19 +0200 Subject: [PATCH 3/5] Deregister heartbeat callbacks when fetch iteration is cancelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CancelledError is a BaseException, so the StopAsyncIteration/timeout handler never saw it and a cancelled batch leaked its callbacks — the same leak this branch fixes for normal termination. --- .claude/scheduled_tasks.lock | 1 + .claude/worktrees/agent-a29dec7e825f1ae16 | 1 + .claude/worktrees/agent-a31635715696d38ca | 1 + .claude/worktrees/agent-a38f8a7500b6c8767 | 1 + .claude/worktrees/agent-a57beae899be22836 | 1 + .claude/worktrees/agent-a58cd5db85506526b | 1 + .claude/worktrees/agent-a6c72205b0c1ee128 | 1 + .claude/worktrees/agent-a6f5b4cbfa2e573f2 | 1 + .claude/worktrees/agent-a7c403367d15dd702 | 1 + .claude/worktrees/agent-a80b7c6a926b6121a | 1 + .claude/worktrees/agent-abfb7b5e082a9843c | 1 + .claude/worktrees/agent-ac3a3c260d121e50b | 1 + .claude/worktrees/agent-adc1096074aaf7d96 | 1 + .claude/worktrees/agent-aed46536a524cf101 | 1 + .claude/worktrees/agent-aedda637b1d250552 | 1 + .claude/worktrees/agent-af0610073a71bb997 | 1 + .claude/worktrees/stoic-shannon-d2a62a | 1 + nats-core/AUDIT-FABLE.md | 447 ++++++++++++++++++ nats-core/AUDIT.md | 233 +++++++++ nats-jetstream/AUDIT.md | 235 +++++++++ .../src/nats/jetstream/consumer/pull.py | 9 + nats-jetstream/tests/test_consumer.py | 30 ++ nats-schemas | 1 + 23 files changed, 972 insertions(+) create mode 100644 .claude/scheduled_tasks.lock create mode 160000 .claude/worktrees/agent-a29dec7e825f1ae16 create mode 160000 .claude/worktrees/agent-a31635715696d38ca create mode 160000 .claude/worktrees/agent-a38f8a7500b6c8767 create mode 160000 .claude/worktrees/agent-a57beae899be22836 create mode 160000 .claude/worktrees/agent-a58cd5db85506526b create mode 160000 .claude/worktrees/agent-a6c72205b0c1ee128 create mode 160000 .claude/worktrees/agent-a6f5b4cbfa2e573f2 create mode 160000 .claude/worktrees/agent-a7c403367d15dd702 create mode 160000 .claude/worktrees/agent-a80b7c6a926b6121a create mode 160000 .claude/worktrees/agent-abfb7b5e082a9843c create mode 160000 .claude/worktrees/agent-ac3a3c260d121e50b create mode 160000 .claude/worktrees/agent-adc1096074aaf7d96 create mode 160000 .claude/worktrees/agent-aed46536a524cf101 create mode 160000 .claude/worktrees/agent-aedda637b1d250552 create mode 160000 .claude/worktrees/agent-af0610073a71bb997 create mode 160000 .claude/worktrees/stoic-shannon-d2a62a create mode 100644 nats-core/AUDIT-FABLE.md create mode 100644 nats-core/AUDIT.md create mode 100644 nats-jetstream/AUDIT.md create mode 160000 nats-schemas diff --git a/.claude/scheduled_tasks.lock b/.claude/scheduled_tasks.lock new file mode 100644 index 00000000..f6fdde28 --- /dev/null +++ b/.claude/scheduled_tasks.lock @@ -0,0 +1 @@ +{"sessionId":"bfc143a9-b1ee-4f7a-9269-d7b6c8ecbb82","pid":95970,"procStart":"Tue Jun 9 13:40:29 2026","acquiredAt":1781013766293} \ No newline at end of file diff --git a/.claude/worktrees/agent-a29dec7e825f1ae16 b/.claude/worktrees/agent-a29dec7e825f1ae16 new file mode 160000 index 00000000..1cbc40bd --- /dev/null +++ b/.claude/worktrees/agent-a29dec7e825f1ae16 @@ -0,0 +1 @@ +Subproject commit 1cbc40bd6c68cbff14d6b508fe72e437f12ded4a diff --git a/.claude/worktrees/agent-a31635715696d38ca b/.claude/worktrees/agent-a31635715696d38ca new file mode 160000 index 00000000..eb8b959a --- /dev/null +++ b/.claude/worktrees/agent-a31635715696d38ca @@ -0,0 +1 @@ +Subproject commit eb8b959a99091d4a9ebee44b4dc9b55ed10a6d84 diff --git a/.claude/worktrees/agent-a38f8a7500b6c8767 b/.claude/worktrees/agent-a38f8a7500b6c8767 new file mode 160000 index 00000000..4438f0c9 --- /dev/null +++ b/.claude/worktrees/agent-a38f8a7500b6c8767 @@ -0,0 +1 @@ +Subproject commit 4438f0c9045269ac6fc0df141b54385b2d03dde8 diff --git a/.claude/worktrees/agent-a57beae899be22836 b/.claude/worktrees/agent-a57beae899be22836 new file mode 160000 index 00000000..91c25c98 --- /dev/null +++ b/.claude/worktrees/agent-a57beae899be22836 @@ -0,0 +1 @@ +Subproject commit 91c25c9827c94a4ac38de0bd69f5050bea7bb301 diff --git a/.claude/worktrees/agent-a58cd5db85506526b b/.claude/worktrees/agent-a58cd5db85506526b new file mode 160000 index 00000000..869d7d20 --- /dev/null +++ b/.claude/worktrees/agent-a58cd5db85506526b @@ -0,0 +1 @@ +Subproject commit 869d7d202239057ae248491ded06a558505076b5 diff --git a/.claude/worktrees/agent-a6c72205b0c1ee128 b/.claude/worktrees/agent-a6c72205b0c1ee128 new file mode 160000 index 00000000..e52ac7c6 --- /dev/null +++ b/.claude/worktrees/agent-a6c72205b0c1ee128 @@ -0,0 +1 @@ +Subproject commit e52ac7c6dc146bdf197fcfbdf63db6f700cdb1d6 diff --git a/.claude/worktrees/agent-a6f5b4cbfa2e573f2 b/.claude/worktrees/agent-a6f5b4cbfa2e573f2 new file mode 160000 index 00000000..fa8fb46e --- /dev/null +++ b/.claude/worktrees/agent-a6f5b4cbfa2e573f2 @@ -0,0 +1 @@ +Subproject commit fa8fb46e51730cbcc44da795eb78e28ffed5cab5 diff --git a/.claude/worktrees/agent-a7c403367d15dd702 b/.claude/worktrees/agent-a7c403367d15dd702 new file mode 160000 index 00000000..f086de66 --- /dev/null +++ b/.claude/worktrees/agent-a7c403367d15dd702 @@ -0,0 +1 @@ +Subproject commit f086de66e5f757702eebfafb3436ae4b3c847b0d diff --git a/.claude/worktrees/agent-a80b7c6a926b6121a b/.claude/worktrees/agent-a80b7c6a926b6121a new file mode 160000 index 00000000..138ed1a1 --- /dev/null +++ b/.claude/worktrees/agent-a80b7c6a926b6121a @@ -0,0 +1 @@ +Subproject commit 138ed1a1ce2db202c2511026e45cc326b19fe07d diff --git a/.claude/worktrees/agent-abfb7b5e082a9843c b/.claude/worktrees/agent-abfb7b5e082a9843c new file mode 160000 index 00000000..e4115c6c --- /dev/null +++ b/.claude/worktrees/agent-abfb7b5e082a9843c @@ -0,0 +1 @@ +Subproject commit e4115c6ce5e7ad394e70f144dfe3ebcef145b5fc diff --git a/.claude/worktrees/agent-ac3a3c260d121e50b b/.claude/worktrees/agent-ac3a3c260d121e50b new file mode 160000 index 00000000..b4e522ef --- /dev/null +++ b/.claude/worktrees/agent-ac3a3c260d121e50b @@ -0,0 +1 @@ +Subproject commit b4e522ef38d221b6514abb8fd9c682ceed00bcce diff --git a/.claude/worktrees/agent-adc1096074aaf7d96 b/.claude/worktrees/agent-adc1096074aaf7d96 new file mode 160000 index 00000000..6a53ec97 --- /dev/null +++ b/.claude/worktrees/agent-adc1096074aaf7d96 @@ -0,0 +1 @@ +Subproject commit 6a53ec97a25c545e794632ab1c0782e30ca152eb diff --git a/.claude/worktrees/agent-aed46536a524cf101 b/.claude/worktrees/agent-aed46536a524cf101 new file mode 160000 index 00000000..fb1771b4 --- /dev/null +++ b/.claude/worktrees/agent-aed46536a524cf101 @@ -0,0 +1 @@ +Subproject commit fb1771b4ded328e7648a5f61fc77c3cfa7c570b5 diff --git a/.claude/worktrees/agent-aedda637b1d250552 b/.claude/worktrees/agent-aedda637b1d250552 new file mode 160000 index 00000000..303e58c7 --- /dev/null +++ b/.claude/worktrees/agent-aedda637b1d250552 @@ -0,0 +1 @@ +Subproject commit 303e58c73f1a7f025f99111660007e848bbef0e1 diff --git a/.claude/worktrees/agent-af0610073a71bb997 b/.claude/worktrees/agent-af0610073a71bb997 new file mode 160000 index 00000000..02bf3464 --- /dev/null +++ b/.claude/worktrees/agent-af0610073a71bb997 @@ -0,0 +1 @@ +Subproject commit 02bf34644ee4abdc687287002259b61403b612be diff --git a/.claude/worktrees/stoic-shannon-d2a62a b/.claude/worktrees/stoic-shannon-d2a62a new file mode 160000 index 00000000..c91177f0 --- /dev/null +++ b/.claude/worktrees/stoic-shannon-d2a62a @@ -0,0 +1 @@ +Subproject commit c91177f0deccbae9c55b31d64fa5f139356f1d16 diff --git a/nats-core/AUDIT-FABLE.md b/nats-core/AUDIT-FABLE.md new file mode 100644 index 00000000..86153dfa --- /dev/null +++ b/nats-core/AUDIT-FABLE.md @@ -0,0 +1,447 @@ +# nats-core Audit (Fable) + +Full-source review of `nats-core/src/nats/client/` (~3,500 lines): correctness issues, +parity defects against expected NATS client behavior, and unpythonic patterns. +File references are relative to `nats-core/src/nats/client/`. + +--- + +## Critical + +### C1. Top-level `import nkeys` breaks the package without the optional extra + +`__init__.py:40` imports `nkeys` unconditionally, but `pyproject.toml` declares +`dependencies = []` and ships nkeys only as the `nkeys` extra. A plain +`pip install nats-core` followed by `import nats.client` raises `ImportError`. +The import should be deferred into `_setup_nkey_auth` / `_setup_jwt_auth` (the +only consumers), with a clear error message pointing at `nats-core[nkeys]`, +mirroring how the websocket extra is handled in `connection.py:317-321`. + +### C2. Ping state is not reset on reconnect → reconnect death spiral + +`__init__.py:960-997` (reconnect success path) restores subscriptions and stats +but never resets `_pings_outstanding`, `_last_ping_sent`, or `_last_pong_received`. +If the original disconnect was caused by reaching `max_outstanding_pings` +(`_pings_outstanding == 2`), the value survives into the new connection. The new +`_write_loop` then hits its first idle timeout, sees +`_pings_outstanding >= _max_outstanding_pings` (`__init__.py:602`), and force +disconnects again. Every reconnected connection lives at most one +`ping_interval` after a ping-timeout disconnect, forever. + +### C3. `CancelledError` swallowed inside the reconnect loop + +`__init__.py:1003-1006`: + +```python +except (asyncio.CancelledError, TimeoutError) as e: + logger.error("Failed to connect to %s: %s", server, type(e).__name__) + self._last_server = server + continue +``` + +If `close()` cancels the task while it is inside `establish_connection` for one +server, the cancellation is consumed and the loop *continues* to the next +server. `close()` then blocks on `await self._read_task` until the entire +reconnect schedule (all servers × all attempts × backoff) runs dry. +Cancellation must be re-raised; never `continue` past a `CancelledError`. + +### C4. Initial connect treats EOF (and any non-PONG) as success + +`__init__.py:1849-1872`: after CONNECT+PING, the response is only checked with +`isinstance(response, Err)`. If the server closes the connection without an +`-ERR` (e.g. some auth/TLS-verify failures), `parse()` returns `None` and +`connect()` happily returns a dead `Client`. The reconnect path +(`__init__.py:945-958`) handles all four cases correctly (`None`, `Err`, +non-`Pong`, timeout) — the initial path should do the same. This verification +logic is duplicated in two places and has already diverged; extract it. + +--- + +## High + +### H1. Keepalive PING starves under continuous publish traffic + +`_write_loop` (`__init__.py:583-611`) only sends a PING when +`wait_for(self._flush_waker.wait(), timeout=self._ping_interval)` times out. +Any publish sets the waker, so on a connection with steady outgoing traffic the +timeout branch never fires and no PING is ever sent — a dead server (or a +half-open TCP connection) is never detected as long as the app keeps +publishing. Stale-connection detection must be driven by a timer independent of +write activity (check `now - _last_ping_sent >= ping_interval` on every loop +iteration, not only in the timeout branch). + +### H2. The 5 ms minimum flush interval serializes request/reply to ~200 req/s + +`_DEFAULT_MIN_FLUSH_INTERVAL = 0.005` (`__init__.py:78`) combined with +`_write_loop`'s enforced sleep (`__init__.py:589-592`) means any publish that +lands within 5 ms of the previous flush waits out the remainder. Sequential +`request()` calls each pay this: publish → buffered → flusher sleeps ~5 ms → +flush → response. A local round trip that should take ~100 µs takes ~5 ms, +capping serialized request throughput at roughly 200/s. Reference clients +coalesce at sub-millisecond granularity (flush as soon as the event loop yields, +i.e. coalescing happens naturally via the task scheduling, not a fixed timer). +Consider flushing immediately when the buffer transitions from empty, and using +the interval only as a backpressure coalescing hint. Also note +`_last_flush = current_time` (`__init__.py:596`) stamps the pre-sleep time, so +the interval bookkeeping is wrong by up to the sleep duration. + +### H3. Publish buffer grows unboundedly while reconnecting + +`publish()` (`__init__.py:1141-1148`) appends to `_pending_messages` during +`RECONNECTING` (status check only blocks `CLOSED`/`CLOSING`), and +`_force_flush()` (`__init__.py:1032-1033`) returns early when the connection is +down without clearing the buffer — there is no flusher task alive either. A +busy publisher during a long outage grows the buffer without limit until OOM. +There needs to be a reconnect-buffer cap (cf. legacy `pending_size`, nats.go +`ReconnectBufSize`, default 8 MB) with a defined overflow behavior (raise or +drop). + +### H4. `subscribe()`/`unsubscribe()` are broken while reconnecting + +- `subscribe()` (`__init__.py:1193-1210`) registers the subscription in + `_subscriptions` *before* writing SUB. During `RECONNECTING` the write raises + `ConnectionError`, the caller sees a failure, but the subscription stays + registered and gets silently replayed on reconnect — a retry then creates a + duplicate. +- `Subscription.unsubscribe()` → `_unsubscribe()` (`__init__.py:1230-1241`) + writes UNSUB for any status other than `CLOSED`/`CLOSING`; during + `RECONNECTING` the write raises and the `del self._subscriptions[sid]` never + runs, so the sub is resurrected on reconnect even though the user + unsubscribed. + +Both should tolerate a down connection: mutate local state, skip the wire write, +and rely on the reconnect replay (which already reads `_subscriptions`). + +### H5. WebSocket clients poison the reconnect pool with TCP endpoints + +`connect()` (`__init__.py:1791-1792`) and `_handle_info` +(`__init__.py:762-765`) append `info.connect_urls` — bare `host:port` TCP +endpoints — into `_server_pool` regardless of transport. For a `ws://`/`wss://` +client, the reconnect loop (`__init__.py:856-871`) then parses those as +`nats://host:port` and attempts raw TCP to the cluster's client ports. The +INFO field `ws_connect_urls` (already declared in `protocol/types.py:85`) exists +precisely for this and is ignored. WebSocket clients should populate the pool +from `ws_connect_urls` only. + +### H6. `drain()` does not actually stop publishing (contradicts its own docs) + +`drain()`'s docstring promises "No new messages can be published", but +`publish()` (`__init__.py:1095`) only rejects `CLOSED`/`CLOSING`; `DRAINING` and +`DRAINED` sail through (likewise `subscribe()` at `__init__.py:1179` and +`request()` at `__init__.py:1278`). Reference clients raise a +draining-specific error for all three. Additionally, +`asyncio.wait_for(asyncio.gather(*drain_tasks), ...)` (`__init__.py:1356`) is a +no-op timeout: `Subscription.drain()` returns immediately after +`queue.shutdown(immediate=False)` — nothing waits for consumers to work off the +queues, so the timeout protects nothing. + +### H7. No header validation → CRLF injection into the header block + +`encode_headers` (`protocol/command.py:52-57`) interpolates keys and values +into the wire format with no validation. A value (or key) containing `\r\n` +injects arbitrary header lines; a key containing `:` or whitespace corrupts the +block. Framing stays intact (the block is length-prefixed) so this is not a +command injection like the subject case, but it silently produces malformed or +attacker-shaped headers. Subjects are carefully validated +(`__init__.py:194-226`); headers deserve the same. + +--- + +## Medium + +### M1. Concurrent `_force_disconnect` races: status overwrite and double reconnect + +`_force_disconnect` (`__init__.py:786-826`) sets +`self._status = DISCONNECTED` *before* taking `_reconnect_lock`. A second caller +(read loop EOF + write loop max-pings can both call) overwrites `RECONNECTING` +with `DISCONNECTED` while the first holds the lock through the *entire* +reconnect cycle. When the first cycle succeeds and releases the lock, the +second caller observes `old_status` not in `(CLOSING, CLOSED)`, +`_reconnecting == False`, and starts a second reconnect cycle on top of the +fresh, healthy connection — replacing `self._connection` and leaking the old +one. The status flip and the should-I-reconnect decision need to happen +atomically, and a freshly reestablished connection must not be torn down by a +stale disconnect notification (guard on connection identity). + +### M2. `_force_disconnect` cancels the task it runs on + +The read loop calls `_force_disconnect`, which cancels `_read_task` — the +currently running task (`__init__.py:792-795`). The self-`await` raises and the +pending cancellation happens to be consumed by the surrounding +`contextlib.suppress(asyncio.CancelledError, ...)`, after which the reconnect +loop continues on a task whose `cancelling()` count is permanently 1 (no +`uncancel()`). This works by accident of CPython task-step ordering and will +misbehave inside any future `asyncio.timeout()` scope. Restructure so teardown +cancels *the other* task only, or hand reconnection off to a dedicated task. + +### M3. Inbound `MAX_CONTROL_LINE` of 4096 can kill valid connections + +`protocol/message.py:46,346-348` rejects any server control line over 4096 +bytes. 4096 is the *server's default* limit for client→server lines; +`max_control_line` is configurable upward, and a long subject + reply easily +exceeds 4 KiB on a server configured for it. Other clients don't enforce a +fixed inbound cap (or honor the server's advertised one). Also note the check +runs *after* `readline()`, whose own `StreamReader` 64 KiB limit raises a raw +`ValueError`/`LimitOverrunError` first for truly long lines — a noisy break of +the read loop instead of a clean protocol error. + +### M4. Slow-consumer handling is inconsistent and leaks `QueueShutDown` + +- In `Subscription._enqueue` (`subscription.py:193-207`), the byte-limit check + raises *before* callbacks run, but `put_nowait` raises `QueueFull` *after* + callbacks ran — so whether registered callbacks observe a dropped message + depends on which limit tripped. +- The client catches `(asyncio.QueueFull, ValueError)` (`__init__.py:653,734`) + but not `asyncio.QueueShutDown`. Today a shut-down subscription is always + removed from `_subscriptions` first, so it is unreachable in practice — but + one refactor away from an unhandled exception that kills the read loop. +- Dropped-message accounting uses `len(payload)` while `_enqueue` budgets + `len(message.data)` — same value, but the duplicated logic in `_handle_msg` + and `_handle_hmsg` (see U1) makes such drift likely. + +### M5. Disconnect/teardown paths produce ERROR-level tracebacks for normal events + +A connection dropped mid-payload raises `IncompleteReadError` inside the read +loop's `except Exception` (`__init__.py:543-545`), logging a full traceback via +`logger.exception("Error in read loop")` for an ordinary network event. The +WebSocket transport is worse: EOF in `readline()` propagates +`IncompleteReadError` (`connection.py:280-287`) instead of returning `b""` like +`TcpConnection.readline`, so a clean server close logs as an error instead of +"Connection closed by server". Distinguish expected EOF/reset from genuine +parser/internal errors. + +### M6. Dead/misleading exception handling in `_read_loop` + +`__init__.py:546-548` catches `(asyncio.CancelledError, ParseError)` at the +outer level and returns *without* reconnecting — but `ParseError` is always +caught first by the inner `except Exception` (which breaks into the reconnect +path). The outer `ParseError` arm is unreachable and documents the wrong +behavior. Similarly, `int()` failures in `parse_msg`/`parse_hmsg` +(`protocol/message.py:187-190,228-233`) escape as raw `ValueError` instead of +`ParseError`. + +### M7. `flush()` / `rtt()` misbehave when not connected + +- `flush()` during `RECONNECTING`: `_force_flush` no-ops, then `_ping()` writes + straight to the dead connection and raises `ConnectionError` + (`__init__.py:1040-1046`). Callers get a transport exception for a state the + client is supposed to be managing. +- `flush()` on PONG timeout force-disconnects (`__init__.py:1071-1073`) — + reasonable — but `rtt()` lets the `TimeoutError` escape without any state + handling, and both share `_pong_waker`, so concurrent `flush()`/`rtt()` + calls can complete on each other's PONGs (rtt underreports). +- `_ping()` (`__init__.py:1040`) increments `_pings_outstanding` without the + max-outstanding check that `_queue_ping` has; the two near-identical methods + should be one. + +### M8. Publishing with headers is not gated on server support + +`connect()` always sends `headers: true`, and `publish()` happily encodes HPUB +regardless of `server_info.headers` (`__init__.py:1114-1132`). Against an old +or proxied server that advertises `headers: false`, reference clients raise a +clear "headers not supported" error; this client sends a frame the server will +treat as a protocol error and disconnects. + +### M9. `ServerInfo.from_protocol` crashes the read loop on minimal INFO + +`__init__.py:145-152` indexes `server_id`, `version`, `go`, `host`, `port`, +`headers` directly. Any async INFO (or nonstandard server/proxy) missing one of +these raises `KeyError` inside `_handle_info`, which breaks the read loop and +tears down the connection. Use `.get()` with defaults for everything that isn't +strictly load-bearing. + +### M10. Reconnect pool/server-selection nits + +- `_last_server` skip (`__init__.py:850-851`) skips the last *attempted* + server, and `_last_server` is updated on every failed attempt too + (`__init__.py:1005,1009`) — the net effect is hard to reason about and + differs from the usual "deprioritize the server we just lost" semantics. +- The IPv6 normalization reassigns the loop variable `server` + (`__init__.py:861-869`), so `_last_server` stores the bracketed form while + the pool holds the unbracketed one — the skip comparison never matches for + IPv6 entries. +- `no_randomize=False` shuffles only the tail, pinning `server_pool[0]` + (`__init__.py:845-848`); full-pool shuffle is the conventional behavior. +- `reconnect_max_attempts` counts *passes over the whole pool*, not per-server + attempts, so the effective retry budget scales with cluster size. (The + `0 == unlimited` sentinel is already tracked separately for a follow-up PR.) +- Reconnect verification reads exactly one protocol message and fails on + anything that isn't PONG (`__init__.py:955-958`); a server that interleaves + an async INFO (e.g. entering LDM) before the PONG fails the attempt. Loop + until PONG/ERR. + +### M11. Write-buffer bypass reorders commands + +`subscribe()`, `_unsubscribe()`, `_handle_ping`'s PONG, and `_queue_ping` write +directly to the connection while published messages sit in +`_pending_messages`. A SUB can therefore reach the server *before* a PUB that +the application issued earlier — observable with echo enabled (you receive your +own earlier publish) and surprising for anyone reasoning about ordering. Either +route everything through the buffer or flush the buffer before direct writes. + +--- + +## Parity gaps + +### P1. `connect()` accepts a single URL only + +No way to seed multiple servers (`nats.connect(["nats://a", "nats://b"])` or +`servers=[...]`). The pool only grows via `connect_urls` after the first +connect succeeds — so the bootstrap server is a single point of failure. Every +reference client accepts a server list. + +### P2. Auto-unsubscribe is unreachable + +`encode_unsub` supports `max_msgs` (`protocol/command.py:105-117`) but nothing +exposes it: `Subscription.unsubscribe()` takes no limit and there is no +`max_msgs=` on `subscribe()`. `UNSUB ` / auto-unsubscribe-after-N is +standard across clients (and required by request-many patterns). + +### P3. Callbacks are sync-only and weakly typed + +`add_disconnected_callback` et al. accept `Callable[[], None]` only — an +`async def` callback would produce an un-awaited coroutine and a warning. +Legacy nats-py and most asyncio APIs accept coroutine callbacks. Also +`add_error_callback` takes `Callable[[Exception | str], None]` +(`__init__.py:299,1512`): server `-ERR` strings are passed raw while +slow-consumer errors arrive as exceptions. Wrap protocol errors in an exception +type so the callback signature is just `Callable[[Exception], ...]`. + +### P4. Missing surface + +- No closed/connection-terminal callback (only disconnected/reconnected/error/LDM). +- No `is_connected`-style convenience or `connected_url`/current-server accessor. +- `ClientStatistics` lacks `errors_received` (legacy parity). +- `request()` takes `headers: dict[...]` but not `Headers` + (`__init__.py:1257`), and `subject: str` but not `bytes` — both inconsistent + with `publish()`. +- Subscription pending defaults are 65 536 msgs (`__init__.py:1160`) vs the + conventional 512 × 1024; worth a deliberate decision either way. +- Repeated authorization errors during reconnect retry forever (until + max attempts); reference clients abort a server after repeated auth + failures to avoid hammering. +- `new_inbox()` uses `uuid4().hex` (32 chars + prefix) where other clients use + NUID (22 chars, faster); fine functionally, but inboxes are hot-path. + +### P5. Inbox prefix validation misses whitespace/CRLF + +`__init__.py:410-417` rejects `>`, `*`, and trailing `.` but not spaces or +CRLF. An injected prefix is only caught later when `publish()` validates the +reply subject — surfacing as a confusing per-request `ValueError` instead of a +clear `connect()`-time error. + +--- + +## Unpythonic / code-quality + +### U1. Large-scale duplication + +- `_handle_msg` and `_handle_hmsg` (`__init__.py:626-756`) are ~90% identical + (mux dispatch, slow-consumer handling, error callbacks) — 130 lines that + should be one helper. +- The reconnect CONNECT/verify block (`__init__.py:896-958`) duplicates + `connect()`'s (`__init__.py:1794-1872`), and the two have already diverged + (C4). Extract a shared "send CONNECT, await PONG" helper. +- `subscribe()` inlines the body of `_subscribe()` (`__init__.py:1204-1210` vs + `1214-1228`). +- `close()` cancels `_read_task`/`_write_task` twice, ~30 lines apart + (`__init__.py:1419-1427` and `1451-1462`). +- `_queue_ping` vs `_ping` (M7). + +### U2. `assert` used for runtime control flow + +`__init__.py:632,692` (`assert self._request_prefix is not None` — an +unexpected sid-0 message under `python -O` becomes an `AttributeError`/wrong +behavior, and under normal mode an `AssertionError` that kills the read loop) +and `_setup_jwt_auth`'s `assert isinstance(...)` (`__init__.py:1650-1651`, +should raise `TypeError`). Asserts vanish under `-O`; user-input validation +must not rely on them. + +### U3. `Headers` is a fake dataclass and not a Mapping + +`message.py:8-26` decorates `Headers` with `@dataclass` but hand-writes +`__init__` and `__eq__`, so the decorator generates nothing useful (and the +custom `__eq__` silently sets `__hash__ = None`). More importantly it supports +neither `headers["Key"]`, `"Key" in headers`, `len(headers)`, nor iteration — +`collections.abc.Mapping` is the obvious shape (`get`, `items`, `keys`, +`values`, `__contains__` for free). Also `asdict()` returns a shallow copy +whose lists are shared with internal state — mutating +`headers.asdict()["X"].append(...)` mutates the live headers. + +### U4. Event-loop API inconsistency + +`asyncio.get_event_loop().time()` in `__init__.py:452,457,560,576,589,599` vs +`asyncio.get_running_loop()` at `__init__.py:1045,1053`. Inside coroutines the +modern form is `get_running_loop()` everywhere; `get_event_loop()` is +soft-deprecated and slower. + +### U5. `Subscription.messages` swallows `RuntimeError` to end iteration + +`subscription.py:123-130` breaks the async iterator on *any* `RuntimeError`, +not just "subscription closed". A genuine `RuntimeError` from user code or +asyncio internals silently terminates the message stream. Define a dedicated +`SubscriptionClosedError` (or re-raise unless the subscription is actually +closed). The `next()`-raises-`RuntimeError` contract has the same smell. + +### U6. Protocol-module oddities + +- `ping()`/`pong()`/`parse_info()`/`parse_err()` are `async def` with no + awaits (`protocol/message.py:258-325`); `Ping`/`Pong` could be module-level + singletons returned synchronously. +- `if TYPE_CHECKING: pass` dead block (`protocol/message.py:10-11`). +- `parse` accepts bare `b"ERR"` in addition to `b"-ERR"` + (`protocol/message.py:365`) — the server never sends the former. +- Control-line split is single-space only (`protocol/message.py:350`); + consecutive spaces/tabs (which the Go parser tolerates) produce empty args + and raw `ValueError`s. +- `ConnectInfo` marks every default field `Required[...]` in a `total=True` + TypedDict (`protocol/types.py:14-22`) — redundant noise. + +### U7. Exception-chaining and transport nits + +- `open_tcp_connection` raises `ConnectionError(msg)` without `from e` + (`connection.py:209-211`); the websocket twin chains correctly. +- `TcpConnection.upgrade_to_tls` pokes `self._writer._transport` + (`connection.py:118`) — known asyncio wart, but deserves a comment on why + it's safe w.r.t. `drain()` and the reader protocol. +- `open_websocket_connection` passes `max_size=None` (`connection.py:328`) — + unbounded frame buffering from the server; the parser's own caps never get a + chance to apply. +- `TcpConnection.read()` (and `Connection.read` in the protocol) is dead code — + nothing in the client calls byte-granularity `read`. + +### U8. Docstring defects + +- `Client.server_info` is annotated `-> ServerInfo | None` + (`__init__.py:472-475`) but `_server_info` is always set; the `| None` forces + every caller to narrow for no reason. +- `connect()`'s docstring references type aliases named `Nkey` and `JWT` + (`__init__.py:1720-1724`) — the actual names are `NkeySeed`/`NkeyHandlers` + and `JWTCredentials`/`JWTHandlers`. +- `_validate_subject`, `_validate_queue`, and the `skip_subject_validation` + docs cite nats.go/nats.rs behavior by name + (`__init__.py:202-206,232-234,1729-1730`); per project convention, describe + the behavior directly instead of citing other clients. +- The type aliases (`NkeySeed`, `JWTCredentials`, …) are public-facing but + absent from `__all__` (`__init__.py:1909-1924`). + +--- + +## Summary + +| Severity | Count | Headliners | +|----------|-------|------------| +| Critical | 4 | broken bare install (C1), reconnect ping death spiral (C2), swallowed cancellation (C3), EOF-as-success on connect (C4) | +| High | 7 | keepalive starvation (H1), 5 ms request serialization (H2), unbounded reconnect buffer (H3), sub/unsub during reconnect (H4), ws pool poisoning (H5), drain doesn't block publish (H6), header injection (H7) | +| Medium | 11 | disconnect races (M1/M2), inbound control-line cap (M3), slow-consumer inconsistencies (M4), noisy/dead error paths (M5/M6) | +| Parity | 5 | single-URL connect (P1), no auto-unsubscribe (P2), sync-only callbacks (P3) | +| Quality | 8 | duplication (U1), asserts as control flow (U2), fake-dataclass Headers (U3) | + +The architecture (Protocol-typed transports, NamedTuple wire messages, +`match`-based dispatch, Queue.shutdown-driven subscription lifecycle) is sound +and idiomatic for 3.13. The risk is concentrated in connection lifecycle code: +`_force_disconnect`/reconnect is one 240-line function owning cancellation, +locking, pool management, CONNECT/auth replay, and state transitions — C2, C3, +C4, M1, M2, M6, and M10 all live there. Decomposing it (dedicated reconnect +task, shared CONNECT/verify helper, atomic state transitions) would resolve the +bulk of this audit in one structural change. diff --git a/nats-core/AUDIT.md b/nats-core/AUDIT.md new file mode 100644 index 00000000..418764a6 --- /dev/null +++ b/nats-core/AUDIT.md @@ -0,0 +1,233 @@ +# nats-core audit (2026-05-27) + +Eight specialized agents reviewed `nats-core/src/nats/client/` through different +lenses (code smells, pythonic patterns, ADR compliance, nats.go divergence, +security, asyncio/concurrency, protocol parser, public API). This document is +the deduplicated, severity-ranked synthesis. Items confirmed by multiple lenses +are noted in parentheses. + +## Critical — fix first + +1. **TLS downgrade on reconnect leaks credentials** *(security, ADR-40)* + `__init__.py:872-896, 936, 1844`. On reconnect, TLS upgrade is gated on the + *current* INFO advertising `tls_required`/`tls_available`. A MITM stripping + those flags causes CONNECT — with `auth_token`/`password`/`jwt`/`nkey`/`sig` + — to be written in plaintext. Pin TLS intent at first connect; never consult + server INFO to downgrade. + +2. **Server-driven `connect_urls` is unbounded and unvalidated** *(security; ADR-40 defines the mechanism, not the hardening)* + `__init__.py:698-701, 968-971`. A hostile/compromised server can append + arbitrary hosts to the client's reconnect pool; entries are never pruned, no + allowlist, TLS hostname is not re-pinned for discovered URLs. Combined with + #1 this is a credential-stealing chain. ADR-40 specifies that advertised + URLs are stored and used for reconnect, with discovery **on by default** and + only a boolean opt-*out* ("Ignore advertised servers", default false); it + says nothing about pruning, allowlisting, bounding the pool, or re-pinning + TLS, and its Security Considerations section is an empty stub. So the fix + here is hardening beyond the spec, not ADR conformance: keep ADR-40's + opt-out default (do **not** flip discovery to opt-in — that would diverge + from the spec and every other client), prune on each INFO, and require TLS + for inherited servers when the seed was TLS. + +3. ~~**Concurrent writes to the connection are unsynchronized** *(asyncio)*~~ + **Retracted.** Every call site passes a complete frame (`encode_pub`/ + `encode_ping`/`encode_sub` all return one `bytes`; the buffered path + `b"".join(...)` first). `Connection.write` is `self._writer.write(data); + await self._writer.drain()` — the `writer.write` call is a single C-level + append, atomic under the GIL, and `drain()` flushes in FIFO append order. + Two coroutines calling `connection.write(complete_frame)` land their frames + adjacent on the wire, not interleaved. Would be a real hazard if a frame + were split across two writes with an await between — it isn't. + +4. **`_force_disconnect` invoked from the read task awaits itself** *(asyncio)* + `__init__.py:728-736`. The `RuntimeError("Task cannot await on itself")` is + silently swallowed by `contextlib.suppress`. Disconnect/reconnect proceeds + before the old reader actually terminates. Detect + `asyncio.current_task() is self._read_task` and skip the await, or move + reconnect to a supervisor task. + +5. **Reader loop silently disconnects on `ParseError`** *(asyncio, parser)* + `__init__.py:482`. `except (CancelledError, ParseError): return` skips + `_force_disconnect`, leaving the client in `CONNECTED` with a dead socket. + Re-raise `CancelledError`; route `ParseError` through reconnect. + +## High + +6. **Credentials logged at DEBUG** *(security)* — [PR #955](https://github.com/nats-io/nats.py/pull/955) + `__init__.py:935, 1844`. `logger.debug("->> CONNECT %s", json.dumps(connect_info))` + dumps `auth_token`/`password`/`jwt`/`nkey`/`sig` in cleartext. Redact before + logging. + +7. **CRLF / whitespace injection in subject, reply, queue, headers** *(security, parser)* + `__init__.py:1078-1205`, `protocol/command.py:28-117, 52-57`. Encoders + interpolate caller-supplied bytes verbatim with `b"PUB %b ..."` / + `f"SUB {subject} {sid}\r\n"`. A `\r\n` in a subject or header value forges + arbitrary protocol commands. Validate against the subject grammar; reject + CR/LF in header keys/values. Inbox-prefix validation at `__init__.py:347-356` + also misses CR/LF/whitespace. + +8. **No typed `-ERR` model — Stale-Connection, Auth, Permissions invisible** *(parser, nats.go, ADR-7)* — [PR #956](https://github.com/nats-io/nats.py/pull/956) + `protocol/message.py:304-307`, `errors.py`, `__init__.py:711-720`. `-ERR` + arrives as an opaque string; only `MaxPayloadError` exists locally and only + for client-side checks. Callers can't programmatically react to auth failure + vs transient error vs reconnect-required error. (Abort-reconnect-on-auth is + a follow-up now unblocked.) + +9. **Sequential request tokens within a session are predictable** *(security, nats.go)* + `__init__.py:1283`. `_next_request_id` is a monotonic int under a per-client + UUID prefix. A subscriber to `_INBOX..*` (e.g. a co-tenant in the same + account) can reply-spoof other requests. Use `secrets.token_hex(8)` per + request — nats.go uses NUIDs per request, not sequential ints. + +10. **`_force_flush` clear-after-await drops and double-sends concurrently-buffered messages** *(asyncio)* + `__init__.py:1090-1101, 1204-1211`. Not a data race — the GIL and the + single event loop already serialize bytecode, and the limit-check → + `append`/`+=` tail of `publish()` is await-free, so two callers cannot both + observe under-limit and overrun `_max_pending_bytes`. The hazard is + cooperative interleaving across the one suspension point, `await + self._connection.write(b"".join(self._pending_messages))` in + `_force_flush`, and only with concurrent publishers (e.g. `gather`, or + publishing from multiple tasks). The `join` snapshots the buffer, then + `drain()` yields; another `publish()` can `append` into the same list + before the resuming flush calls `clear()`, so that message is silently + dropped (it was never in the snapshot). Worse, two over-limit publishers + can both pass the `if not self._pending_messages` guard and `write` the + same buffer before either clears, putting those messages on the wire twice. + Swap in a fresh list *before* the await (`batch, self._pending_messages = + self._pending_messages, []; self._pending_bytes = 0; await ...write(b"".join(batch))`), + which closes both the drop and the double-send. + +11. **HMSG/MSG numeric parsing crashes → silent disconnect** *(parser)* + `protocol/message.py:185-194, 226-241`. `int(args[2])` on corrupt/attacker + input raises, the read loop's blanket `except Exception` catches it and + `break`s. Same pattern for non-UTF-8 bytes in headers (`message.py:124`) — + strict `.decode()` plus the silent break means one bad frame disconnects + the client without a typed error. + +12. **`+OK` is not parsed at all** *(parser)* — [PR #949](https://github.com/nats-io/nats.py/pull/949) + `protocol/message.py:354-369` has no `b"+OK"` arm. Any user passing + `verbose=True` in a custom CONNECT silently disconnects on the first server + reply. + +13. **Treating any non-200 status as error breaks 1xx informational responses** *(API, parser)* + `__init__.py:1295`, `errors.py:71`. `request()` raises `StatusError` on + `100 Idle Heartbeat` / `100 Flow Control`. Status code is also a `str`, + not `int`. + +14. **`Client` is a god-class — 1923 lines in `__init__.py`, ~40 attrs, duplicated handshake/CONNECT/TLS logic** *(smells)* — [PR #948](https://github.com/nats-io/nats.py/pull/948) extracts `establish_connection`, killing ~150 lines of the open-socket + read-INFO + maybe-upgrade-TLS dedup. Remaining work: `_build_connect_info` + `_perform_handshake`, then `RequestMultiplexer` / `WriteBuffer` / `ServerPool` extraction. + +15. **`Headers` is not a `Mapping`, keys are case-sensitive** *(API, parser, spec)* — [PR #954](https://github.com/nats-io/nats.py/pull/954) + `message.py:8-109`. Cannot do `msg.headers["trace-id"]`, `in`, `len()`, + `iter()`, `dict(headers)`. ADR-21/HTTP-style is case-insensitive; nats.go + canonicalizes. Inherit `collections.abc.MutableMapping`, store case-preserved + but case-insensitive lookup. + +16. **No `Message.respond()`** *(API)* — [PR #953](https://github.com/nats-io/nats.py/pull/953) + Every reply forces `await client.publish(msg.reply, ...)` with manual + `msg.reply is not None` guards. This is the single most-used convenience in + NATS — add it. + +## Medium + +17. **No offline publish buffer / no PUB replay on reconnect** *(nats.go)* — + `publish` during `RECONNECTING` raises `RuntimeError`; nats.go buffers up to + 8 MB and replays. +18. **Reconnect backoff/jitter semantics diverge from nats.go** *(nats.go)* — + multiplicative jitter + exponential base doubling; Go uses additive + `ReconnectJitter` (100 ms / 1 s TLS) with no exponential. Missing + `ReconnectJitterTLS`, `CustomReconnectDelayCB`, `RetryOnFailedConnect`, + `IgnoreAuthErrorAbort`, `ReconnectBufSize`, `ConnectedCB`/`ClosedCB`, + `DiscoveredServersCB`, `ReconnectErrCB`, `CustomDialer`, `RootCAsCB`. +19. **Per-server reconnect counter / dead-server eviction missing** *(nats.go)* — [PR #960](https://github.com/nats-io/nats.py/pull/960). Every server was retried forever; now `reconnect_max_attempts` is per-server and servers are evicted on exhaustion (behavioral change). +20. **ADR-5 lame-duck mode** *(ADR)* — detected and callback fires, but no + proactive jittered self-disconnect/migration. +21. **ADR-11 multi-IP hostname fallback** *(ADR)* — `asyncio.open_connection` + picks one address; no per-IP retry or randomization. +22. ~~**`_next_sid` and `_next_request_id` non-atomic increment** *(asyncio)*~~ — + **Retracted.** `sid = self._next_sid; self._next_sid += 1` is pure Python + bytecode with no `await` between read and increment. The asyncio loop is + single-threaded and only context-switches at `await`, so the sequence is + atomic for our purposes. Would be a real race in a multi-threaded context; + we're not in one. +23. **`flush()`/`rtt()` share a single `_pong_waker`** *(asyncio)* — keepalive + PONG can wake `flush()` early; two concurrent flushes both wake on a single + PONG. Use a deque of per-call futures. +24. **`_force_disconnect` not idempotent under concurrent callers** *(asyncio)* — + lock only covers the post-cancel block; `close()` racing with reader- + triggered disconnect double-runs. +25. **`subscribe`/`_unsubscribe` bypass the publish buffer** *(smells)* — + head-of-line ordering surprise between buffered PUB and unbuffered SUB to + the same subject. +26. **No `unsubscribe(max_msgs=...)`** *(API, nats.go)* — protocol supports it + (`encode_unsub` even takes the arg), but no API surface. +27. **No shared exception base class** *(API)* — MIGRATION.md admits this. Bare + `RuntimeError("Connection is closed")` in `publish`/`subscribe`/`request`. + Add `NATSError`, `ConnectionClosedError`. +28. **In-flight `_request_futures` are not failed on disconnect** *(asyncio, smells)* — + callers see `TimeoutError` instead of a connection error. +29. **`Subscription._enqueue` raises mixed exceptions** *(smells)* — `ValueError` + for bytes overrun, `QueueFull` for message overrun; callers branch identically. +30. **Pending limits immutable after `subscribe()`** *(nats.go)* — no + `set_pending_limits` on `Subscription`. +31. **ADR-4 header field-name validation missing** *(ADR)* — invalid keys reach + the wire and cause server disconnects. +32. **User-supplied `nkey_signature_handler` bytes are not base64url-encoded** *(ADR-14)* — + `_setup_nkey_auth` correctly encodes when the client signs, but the + `NkeyHandlers` path forwards `.decode()` raw bytes. Document or wrap. +33. **`add_*_callback` methods are Java-flavored**; no `remove_disconnected_callback` *(API)* — [PR #957](https://github.com/nats-io/nats.py/pull/957) adds the four `remove_*_callback` counterparts (`add_*_callback`/`remove_*_callback` matches stdlib `Future.add_done_callback`/`remove_done_callback`, so the "Java-flavored" criticism was overstated). +34. **Reconnect cannot be aborted by `close()` mid-backoff** *(asyncio)* — close + should also set `_reconnect_wake`. +35. **Subscription dict mutated during reconnect's re-SUB iteration** *(asyncio)* — + user `subscribe`/`unsubscribe` during the window races with the bulk re-SUB + write. +36. **`close()` cancels read/write tasks twice and writes UNSUB after the socket is closed** *(asyncio, smells)*. +37. **`_handle_msg` / `_handle_hmsg` are near-identical** *(smells)* — collapse + to one `_dispatch`. +38. **Inbox uses UUID4 (32 hex) instead of NUID (22 chars)** *(nats.go)* — + interop and forensic-tooling deviation. +39. **`ServerInfo` drops `ws_connect_urls`, `git_commit`, `ip`, `client_ip`, `cluster`, `domain`, `xkey`** *(parser, ADR)*. +40. **IPv6 detection by colon-counting** *(smells)* — `__init__.py:797-806`; use + `ipaddress` or require brackets. + +## Low / polish + +41. `from __future__ import annotations` in 8 files — dead weight at 3.13 *(pythonic)*. +42. `asyncio.get_event_loop()` at `__init__.py:388, 393, 496, 512, 525, 535` + while line 1048 correctly uses `get_running_loop()` *(pythonic, asyncio)*. +43. `asyncio.TimeoutError` mixed with `TimeoutError` builtin *(pythonic)* — [PR #950](https://github.com/nats-io/nats.py/pull/950). +44. `@dataclass` missing `slots=True` on `ServerInfo`; `Headers` is `@dataclass` + *and* defines `__init__`/`__eq__` (decorator is dead) *(pythonic, API)*. +45. `Enum` instead of `StrEnum` for `ClientStatus`; eight states with + undocumented transitions *(pythonic, API)*. +46. Reader/writer broad `except Exception` and `except BaseException` swallows + real bugs and `CancelledError` *(pythonic, asyncio)*. +47. WebSocket buffer is O(n²) `bytes += frame` / slicing *(pythonic)*. +48. `protocol/message.py:310-325` `async def ping/pong` with no awaits; + `if TYPE_CHECKING: pass` dead block; unused `TypeVar T` in + `subscription.py:22`. +49. `connection.py:114` pokes `_writer._transport` private attr (justified, + document it). +50. `force_reconnect()` is Go/Java-named — prefer `reconnect()` *(API)*. +51. `return_on_error` is a double-negative kwarg — prefer `raise_for_status` *(API)*. +52. Magic numbers `1*1024*1024`, `1*512`, `0.005` *(smells)* — [PR #951](https://github.com/nats-io/nats.py/pull/951). +53. Verb match is case-sensitive (`PING` vs `Ping`) — spec is case-insensitive *(parser)*. +54. `__all__` exports `MaxPayloadError`/`NoRespondersError` but not + `SlowConsumerError` *(API)*. +55. `Subscription.messages` exists "for legacy compat" — drop it *(API)*. +56. INFO `cast(ServerInfo, data)` skips required-field validation *(parser)*. +57. `Verbose`/`Pedantic` hard-coded in CONNECT — no user override *(nats.go)* — [PR #949](https://github.com/nats-io/nats.py/pull/949). `protocol` deliberately left as a wire-format internal. +58. Inconsistent `logger.exception` vs `logger.error` for similar failure paths *(smells)* — [PR #952](https://github.com/nats-io/nats.py/pull/952). + +## Themes / leverage points + +- **Decomposing `Client` (#14) unlocks unit testability** and removes the + duplicate CONNECT/TLS branches that drove findings #1, #6, the asyncio races, + and several smells. +- **A typed `-ERR` model + a header-spec-compliant `Headers`/`Status`** (#8, #13, + #15) is one refactor that resolves three lenses simultaneously and aligns + with both ADRs and nats.go. +- **Security #1 + #2 (TLS pinning + `connect_urls` validation) are the only + items where a remote attacker can cause direct harm** — these should ship + before the package leaves "in development." +- ~~**One write lock around `_connection.write()`** (#3) is a ~10-line fix with + very high payoff against a real wire-corruption hazard.~~ Retracted — see #3. diff --git a/nats-jetstream/AUDIT.md b/nats-jetstream/AUDIT.md new file mode 100644 index 00000000..2d148533 --- /dev/null +++ b/nats-jetstream/AUDIT.md @@ -0,0 +1,235 @@ +# nats-jetstream audit (2026-05-27) + +Five specialized agents reviewed `nats-jetstream/src/nats/jetstream/` through +different lenses (ADR compliance, nats.go parity, pythonic patterns, code +smells, public API ergonomics). This document is the deduplicated, +severity-ranked synthesis. Items confirmed by multiple lenses are noted in +parentheses. + +Package is ~7.4k LOC across 11 files. Notable concentrations: `api/types.py` +(1774), `stream.py` (1673), `__init__.py` (911), `consumer/pull.py` (862). + +## Critical — correctness / interop bugs + +1. **Hard-coded `$JS.API.DIRECT.GET.{name}` ignores prefix/domain** *(smells, ADR-31)* — [PR #958](https://github.com/nats-io/nats.py/pull/958) + `stream.py:1174, 1179`. Bypasses `self._prefix`. Direct-get fails for users + with a custom prefix or JetStream domain. + +2. **`OrderedConsumer.fetch()` recreates the server-side consumer on every call** *(API, ADR-17)* + `consumer/ordered.py:178-213`. `_prepare_fetch` triggers a full delete + create cycle + between batches. Either make ordered consumers `messages()`-only or make + `fetch()` transparent. The docstring acknowledges the bug. + +3. **`PullMessageBatch`/`PullMessageStream` leak callbacks on the client** *(smells)* + `consumer/pull.py:84-86, 282-283`. Each `fetch()` / `messages()` calls + `add_disconnected_callback` + `add_reconnected_callback` and never removes + them. Cumulative leak per invocation. Cleanup must `remove_*_callback`. + +4. **Pull `messages()` mishandles `Nats-Pending-Messages`/`Nats-Pending-Bytes` on 404** *(parity)* + `consumer/pull.py:335-341`. On 404 No Messages, the spec says the request + still counted against the batch — client decrements pending by the headers. + nats-jetstream zeros out pending unconditionally instead of reading the + headers, causing drift from the server's view on intermittent gaps. + +5. **`_cleanup` in `__anext__` converts errors to `StopAsyncIteration`** *(smells, parity)* + `consumer/pull.py:322`. Callers can't distinguish connection loss from + end-of-batch. `PullMessageBatch` stashes on `self._error`; `PullMessageStream` + doesn't. + +6. ~~**`AckPolicy = Literal["none", "all", "explicit", "flow_control"]`** *(pythonic, ADR)*~~ + **Retracted.** `"flow_control"` is a real server policy + (`AckFlowControl` in `nats-server/server/consumer.go` — "functions like + AckAll, but acks based on responses to flow control"). The server requires + it to be paired with a push consumer (`deliver_subject` set) and + `flow_control=true`, so combined with nats-jetstream being pull-only today + it can't actually be used end-to-end — but the literal value is correct, + not hallucinated. (See related finding on push-only fields exposed on a + pull-only package.) + +## High + +7. **Pinned-client priority groups silently broken** *(ADR-42)* + `consumer/pull.py:330-369`. No `Nats-Pin-Id` capture, no `id` echo in + subsequent pulls, no 423 handling. Config fields (`group`/`priority`/ + `min_pending`/`min_ack_pending`/`priority_policy`) are wired but the + protocol completion is missing; `CONSUMER.UNPIN` admin API absent. + +8. **`publish` lacks first-class JetStream options** *(API, parity, ADR-37)* + `__init__.py:281-359`. No `msg_id`, `expected_stream`, `expected_last_seq`, + `expected_last_subject_seq`, `expected_last_msg_id`, `ttl`. Users hand-craft + `Nats-Msg-Id` / `Nats-Expected-*` / `Nats-TTL` / `Nats-Marker-Reason` + headers. Header-name constants for these missing in `headers.py`. + +9. **No `publish_async` / batched publish** *(parity, ADR-50)* + Only synchronous one-at-a-time publish. nats.go has `PublishAsync` → + `PubAckFuture` with max-pending window. ADR-50 batch headers + (`Nats-Batch-Sequence`, `Nats-Batch-Commit`) and `PublishAck.batch_id`/ + `batch_size` exist (`headers.py:7-23`, `__init__.py:215-228`) but no + orchestration that drives them. + +10. **No `create_or_update_stream`** *(API, parity)* + `__init__.py:436`. Asymmetric with `create_or_update_consumer` + (`__init__.py:618`). Single most common operation in real apps. + +11. **`update_stream` returns `StreamInfo`, not `Stream`** *(API)* + `__init__.py:465-483`. Asymmetric with `create_stream → Stream`. Callers + lose the handle. + +12. **Direct Get is single-message only** *(ADR-31, parity)* + `stream.py:1168-1230`. Missing batch / `max_bytes` / `multi_last` / + `up_to_seq` / `up_to_time` / `next_by_subj` / `start_time`, EOB-204 + handling, `Nats-Num-Pending` / `Nats-Last-Sequence` propagation, 413 + handling. Reduces a 2.11+ feature to its 2.10 capability set. + +13. **Ordered-consumer invariants not enforced** *(ADR-17, parity)* + `consumer/ordered.py:329-360`. Spec requires `ack_policy=none`, + `max_deliver=1`, `mem_storage=true`, `num_replicas=1`, + `flow_control=true`, default `idle_heartbeat≈5s`. Package hard-codes some, + silently overrides others (`inactive_threshold` → 5min if `None`, + `consumer/ordered.py:327`), and ignores user values that ever might be + exposed. Recovery is coarse (any inner-iter exit triggers reset) — no + explicit sequence-gap-vs-heartbeat distinction. + +14. **`Stream` is a god-class with 11 private-attr workarounds** *(smells)* + `stream.py:1085-1673` — ~590 LOC, 19 public methods. 11 sites do + `getattr(self._jetstream, "_api", None)` then `raise RuntimeError("can't + happen")` (`stream.py:1135, 1164, 1317, 1332, 1350, 1403, 1435, 1481, + 1577, 1595, 1635`). `JetStream._api` is always set; the guards exist only + to defeat typing. Make `_api` a real attribute on a typed protocol. + +15. **Pull consumer polls every 100 ms instead of refilling on threshold** *(parity, smells)* + `consumer/pull.py:426-448` (request loop), `:450-473` (heartbeat monitor). + Three places independently track `_heartbeat_deadline`; the monitor + rewrites `_pending_messages`/`_pending_bytes` without coordinating with + in-flight `__anext__` — real race. Replace with `asyncio.Event` / + `wait_for` and own the deadline in one place. + +16. **Sparse typed error catalog** *(parity, API)* + `errors.py:7-41`. Missing common codes: `BAD_REQUEST (10003)`, + `STREAM_WRONG_LAST_SEQUENCE (10071)`, `CONSUMER_NAME_EXISTS (10013)`, + `CONSUMER_ALREADY_EXISTS (10105)`, `DUPLICATE_FILTER_SUBJECTS (10136)`, + `OVERLAPPING_FILTER_SUBJECTS (10138)`, `CONSUMER_EMPTY_FILTER (10139)`. + No `BadRequestError`, `WrongLastSequenceError`, + `ConsumerNameAlreadyExistsError`. `ErrorCode` is a bare class — should be + `IntEnum`. + +17. ~~**`api/types.py` 1774 LOC with massive duplication** *(smells, pythonic)*~~ + **Retracted.** `api/types.py` is generated from JSON schemas by + `nats-jetstream/tools/generate_types.py` (schemas under + `nats-jetstream/schemas/jetstream/api/v1/`). The "3 edits per field change" + cost is paid by the schema source, not by us. The remaining valid concern + is #18 — the hand-rolled `from_response`/`to_request` layer on the + user-facing dataclasses that re-encodes the generated TypedDicts. + +18. **~25 `@dataclass` types with hand-rolled `from_response`/`to_request`** *(smells, pythonic)* + `ConsumerConfig.from_response` ~100 lines; `StreamConfig` `from_response` + + `to_request` ~220 lines. Per project memory, msgspec was meant to back + these. Adding a field is 6 edits and silent on omission. + +19. **Pull batch 408/409 errors silently swallowed** *(parity, smells)* + `consumer/pull.py:151-152, 175-186`. 408 raises `StopAsyncIteration` + without setting `_error`; "exceeded maxrequestbatch/expires/maxbytes/ + maxwaiting" matched by `description.lower()` substring (server text isn't + API-stable) and converted to bare `Exception`. Use error codes; surface as + typed errors. + +20. **`publish` retry loop control-flow bug** *(API, smells)* + `__init__.py:323-359`. The `for` loop returns inside the `try`; a future + edit adding a new exception type could fall through and return `None`. + Add an explicit `raise` after the loop. + +21. **JetStream not `async with`-able, no `close()`** *(API)* + `__init__.py:248-264`. No `__aenter__`/`__aexit__`, no graceful shutdown. + +22. **`JetStream.get_message` / `get_last_message_for_subject` claim to require `allow_direct=true` but use the API path** *(smells)* + `__init__.py:763-849`. Docstring is wrong and the implementation duplicates + `Stream._get_message`'s non-direct branch (~90 lines). Plus there's a + third copy of the base64+headers decode in the same package. + +## Medium + +23. **`Consumer.reset` on the protocol; `OrderedConsumer.reset` raises `NotImplementedError`** *(API)* — LSP violation. Split into `ResettableConsumer`, or move off the protocol. +24. **`Consumer` protocol mismatch with `PullConsumer.next`** *(API)* — protocol declares `(max_wait)`; impl adds `heartbeat`, `min_ack_pending`, `min_pending`, `priority_group`, `priority`. Protocol is a lie. `consumer/__init__.py:553` vs `consumer/pull.py:566`. +25. **Per-message TTL: config wired, headers/publish path missing** *(ADR-37)* — `StreamConfig.allow_msg_ttl` exists; no `Nats-TTL` constant or `publish(ttl=...)` kwarg. +26. **`Stream.pause_consumer(pause_until: float)` takes Unix timestamp** *(API)* — should be `datetime` to match `ConsumerConfig.pause_until: datetime`. `stream.py:1563`. +27. **`PullMessageStream` rejects `max_messages + max_bytes`** *(parity)* — Go allows both; `max_bytes` is a soft cap within the batch. `consumer/pull.py:644-646`. +28. **No `StopAfter` option on `messages()`** *(parity)*. +29. **No `ConsumeErrHandler` callback** *(parity)* — non-terminal errors only logged, no user hook. `consumer/pull.py:119, 458`. +30. **Fixed 5-second timeout on every API call** *(parity)* — `api/client.py:424`. No per-call override; `stream_create` on large mirror sources / `stream_purge` on huge streams will time out. +31. **`api/client.py:155-395` repeats the same try/`error_code`/raise-subclass pattern ~10 times** *(smells)* — a `{ErrorCode.X: ErrorXError, ...}` map + `_remap(e, allowed)` helper eliminates it. +32. **`pause_consumer` returns `None`; nats.go returns `ConsumerPauseResponse`** *(parity)* — loses the actual pause time. +33. **`Message.ack_sync` missing; called `double_ack` instead** *(parity, smells)* — uses `subscription.next` instead of `request`, opening a sub per call (extra round-trip). And five near-identical ack methods (`ack`/`nak`/`nak_with_delay`/`in_progress`/`term`/`term_with_reason`) repeat the same 4-line `_reply`/`_jetstream` validation; extract `_send_ack(payload)`. +34. **`Message.metadata` always populated, with junk defaults** *(API)* — server-pushed messages without a JS reply silently get `stream=""`, `sequence=(0,0)`. Should be `None` or raise. +35. **`ConsumerConfig` exposes push-only fields on a pull-only package** *(API)* — `deliver_subject`, `deliver_group`, `flow_control`, `idle_heartbeat`, `direct` ("internal use"); `stream.py:1376` rejects push but no validation upfront. +36. **Auto-generated consumer name is `consumer-{base64(str(datetime.now(utc)))}`** *(smells)* — magic, not collision-proof, bizarre. Use `uuid.uuid4().hex` or NUID. `stream.py:1338-1380`. +37. **`StreamConfig.name: str | None = None`; `create_stream` validates at runtime** *(API)* — make `name` required positionally, or take it as the primary arg. +38. **Magic `$JS.API` literal in `stream.py:1174, 1179`** *(smells)* — hard-coded direct-get subject ignores `self._prefix` (see #1). +39. **`StreamConfig` missing `metadata` field on the public dataclass** *(ADR-33)* — present in `api/types.py` but not exposed on the user-facing model. Affects ADR-44 versioning too (`NATS_REQUIRED_API_LEVEL` constant exists; nothing sets/reads it). +40. **`MessageBatch.error` is `Exception`, not typed** *(API, parity)* — 409 sub-errors matched by string substring (#19). +41. **Cleanup / `_delete_consumer` silently swallows all exceptions** *(smells, ordered)* — `consumer/ordered.py:407-412`. No `logger.debug`. Combined with fire-and-forget `_reset()` task creation, recreation failures are invisible. +42. **`getattr(self._jetstream, "_api", None)` 11 times** *(smells)* — see #14. +43. **`StreamNameBySubject`, `UnpinConsumer`, push consumer surface, `CleanupPublisher`, `Stream.cached_info()` missing** *(parity)*. +44. **Pull/`messages()` `max_messages` semantics differs from `fetch()`** *(API)* — same parameter name, different meaning (per-batch hint vs total bound). `consumer/pull.py:566-602` vs `:610-670`. +45. **`Stream.get_message` vs `Stream.direct_get_message`** *(API)* — `Stream._get_message` does fallback transparently; `JetStream.get_message` doesn't. Two routes diverge. +46. **`stream_names`/`list_streams`/`consumer_names`/`list_consumers` pagination duplicated 4 times** *(smells)* — extract `_paginate`. +47. **All dataclasses lack `frozen=True`/`slots=True`/`kw_only=True`** *(pythonic)* — conceptually immutable returned-value records (`PublishAck`, `APIStats`, `Tier`, `AccountInfo`, `Metadata`, `StreamMessage`, `ConsumerReset`, etc.). Either dataclass with all three, or msgspec. +48. **`time.time()` used everywhere for deadlines** *(pythonic)* — `consumer/pull.py:76, 78, 91, 97, 118, 130, 142, 277, 288, 294, 328, 457, 468`. Must be `time.monotonic()` (wall-clock-jump immune). +49. **`asyncio.get_event_loop().time()` for publish deadlines** *(pythonic)* — `__init__.py:319, 326, 353`. Deprecated when no loop; use `get_running_loop()` or `time.monotonic()`. +50. **Manual polling in `_request_loop` / `_heartbeat_monitor`** *(pythonic)* — see #15. +51. **`datetime.fromisoformat(s.replace("Z", "+00:00"))` repeated 11 times** *(pythonic)* — Python 3.11+ handles `Z` natively. Drop the `.replace`. +52. **`timedelta(microseconds=ns / 1000)` repeated 9 times** *(pythonic, bug-shape)* — float divide introduces rounding; wrap as `_ns_to_timedelta(ns)` using `ns // 1000`. +53. **Broad `except Exception:` swallowing in header parse** *(pythonic)* — `__init__.py:797, 839`, `stream.py:1264`. Silently sets `headers=None`. At minimum log. +54. **`PullConsumer.get_info` is `async def` but never awaits** *(pythonic, API)* — `consumer/pull.py:521-523`. Docstring claims "refresh from server"; body returns cached `self._info`. Either fix the body or remove `async`. +55. **`OrderedConsumer.create` async factory** *(pythonic)* — instances created via `__init__` directly are half-initialized; every property guards on it. Make `__init__` private or initialize lazily. + +## Low / polish + +56. **`from __future__ import annotations` in all 9 files** *(pythonic)* — dead weight at 3.11+. +57. **`Union[...]` / `Optional[...]` / `Tuple[...]` from typing in `api/types.py`** *(pythonic)* — use `|`. +58. **`AsyncIterator` imported from `typing` in 5 files** *(pythonic)* — should be `collections.abc.AsyncIterator`. Also, `async def` generators return `AsyncGenerator[T, None]`, not `AsyncIterator[T]`. Affects `__init__.py:361, 392, 670, 703`, `stream.py:1021, 1029`. +59. **`StreamMessage.__getitem__`** *(API)* — `stream.py:1078-1082`. `dict`-style attribute access alongside attribute access. Footgun, also lets you read private attrs. Drop. +60. **`ErrorCode` as bare class** *(pythonic)* — should be `IntEnum` for `.name` / `.value` / iteration. +61. **`CONSUMER_ACTION_*` string literals** *(pythonic)* — should be `StrEnum`. Same for `Literal[...]` aliases (`AckPolicy`, `DeliverPolicy`, etc.) where runtime identity is useful. +62. **`MessageBatch.error: Exception | None`** — see #40. +63. **`__all__` doesn't re-export `Headers`, `Metadata`, `SequencePair`, `Message`, `MessageBatch`, `MessageStream`, `PullConsumer`, `OrderedConsumer`, or `headers.NATS_*`** *(API)* — users dig into submodules. +64. **`PublishAck.value` opaque field name** *(API)* — rename `counter_value`. +65. **`Stream.purge(filter=...)` shadows the Python builtin** *(API)* — `stream.py:1123`. Rename `subject` or `subject_filter`. +66. **Local imports inside `publish()`** *(pythonic)* — `__init__.py:314 import asyncio`, `:316 from nats.client.errors import NoRespondersError`. Lift to module level. Also `stream.py:1257`, `stream.py:1574` (which is dead code — already imported at module top). +67. **`JetStream.__init__` positional args** *(API)* — `(client, prefix, domain, strict)`. Make all but `client` keyword-only. +68. **Inconsistent return types: `delete_stream → bool`, `delete_consumer → bool`, `delete_message → None`, `pause_consumer → None`** *(API)* — pick `→ None` (raise on failure) everywhere. +69. **TODO `alternates` field never added to `StreamInfo`** *(smells)* — `stream.py:997`. Real lost data. +70. **`StreamManager` Protocol declares `create_stream(**config)`; impl takes positional `StreamConfig`** *(API)* — `stream.py:1017-1066`. Protocol is wrong. +71. **`consumer/__init__.py:125 priority_timeout: Any | None`** *(pythonic)* — `Any` while every other duration is `timedelta`. +72. **`timezone.utc` → `datetime.UTC` (3.11+)** *(pythonic)* — 6+ sites. +73. **Bare `pass` in retry/error paths with no logging** *(pythonic)* — `consumer/pull.py:444-448, 471-473`, `consumer/ordered.py:411`. +74. **`OrderedConsumer.__aexit__(self, *exc_info)` signature drift** *(pythonic)* — `consumer/ordered.py:144`. Use typed `(exc_type, exc_val, exc_tb)` like `pull.py:557`. +75. **Magic numbers / strings** *(smells)* — heartbeat 2x multiplier (`pull.py:78, 142, 277, 328, 468`), default batch 100, byte-mode batch 1_000_000 (`pull.py:653, 733`), backoff 1.0/10.0/2.0 (`ordered.py`), `$JS.ACK` parsing (`message.py:71-117`), `Nats-Pending-Messages/Bytes` (`pull.py:346, 348`). +76. **`Stream.__init__` accepts `info: StreamInfo | None`; `_info` nullable everywhere** *(API)* — force-fetch on init or split into `Stream` (always has info) vs `StreamRef`. +77. **`api/client.py:73-89 _error_from_response` is private but called from `__init__.py:342`** *(smells)* — public-by-use, private-by-name. Same for `is_error_response`. +78. **`SubjectTransform` typed as `Any` on `StreamSource`/`StreamSourceInfo`** *(ADR-36)* — round-trips OK, loses type safety. `stream.py:181, 257`. +79. **Inconsistent prefix on `nats.jetstream.api` logger** *(smells)* — hard-coded string vs `__name__` elsewhere. +80. **`set()` empties / lambda-style boilerplate in `check_response`** *(API)* — returns 3-tuple `(bool, set, set)` callers re-check field by field. + +## Themes / leverage points + +- **Dataclass marshaling layer (#18)** is the single highest-leverage + cleanup. The generated `api/types.py` TypedDicts already describe the wire + shape; the user-facing `@dataclass` mirrors plus their hand-rolled + `from_response` / `to_request` (~400 lines across `consumer/__init__.py` + and `stream.py`) re-encode the same fields by hand. Migrate the user-facing + layer to `msgspec.Struct` (or extend the generator to emit it), and the + silent-drop bugs and edit-three-places cost go away. +- **Pull consumer rewrite (#15, #4, #19, #5, #44)** is the biggest correctness + cluster: polling → event-driven, fix 404/408 header semantics, surface + errors properly instead of swallowing into `StopAsyncIteration`. +- **`Stream` god-class (#14, #22, #42)** — extracting `_api` to a typed + protocol kills 11 RuntimeError guards. Split into `StreamManagement`, + `StreamMessages`, `StreamConsumers` collaborators. +- **`OrderedConsumer` (#2, #13, #23, #41)** is largely broken or fragile: + `fetch()` recreates per-call, `reset()` raises, invariants not enforced, + exceptions silently swallowed. Worth a focused rewrite. +- **Publish ergonomics (#8, #9, #25, #20)** is the biggest user-facing gap — + `msg_id`, `expected_*`, `ttl`, `publish_async`, batch publish. None of it + exists despite the headers/types being half-wired. +- **Hard-coded `$JS.API` literals (#1, #38)** are real bugs for any + domain/custom-prefix user. Single grep, ~6 sites. diff --git a/nats-jetstream/src/nats/jetstream/consumer/pull.py b/nats-jetstream/src/nats/jetstream/consumer/pull.py index 765c4c67..bab16609 100644 --- a/nats-jetstream/src/nats/jetstream/consumer/pull.py +++ b/nats-jetstream/src/nats/jetstream/consumer/pull.py @@ -220,6 +220,15 @@ async def __anext__(self) -> Message: self._deregister_callbacks() self._terminated = True raise StopAsyncIteration + except asyncio.CancelledError: + # Cancellation must release the heartbeat callbacks too, or they + # leak on the client. Deregister before the await: unsubscribing + # can itself be interrupted by a second cancellation. + if not self._terminated: + self._terminated = True + self._deregister_callbacks() + await self._subscription.unsubscribe() + raise class PullMessageStream(MessageStream): diff --git a/nats-jetstream/tests/test_consumer.py b/nats-jetstream/tests/test_consumer.py index a7f85377..2914a60e 100644 --- a/nats-jetstream/tests/test_consumer.py +++ b/nats-jetstream/tests/test_consumer.py @@ -428,6 +428,36 @@ async def test_fetch_deregisters_heartbeat_callbacks_on_exhaustion(jetstream: Je assert len(client._reconnected_callbacks) == reconnected +@pytest.mark.asyncio +async def test_fetch_deregisters_heartbeat_callbacks_on_cancellation(jetstream: JetStream): + """Regression for #962: cancelling a heartbeat fetch mid-iteration releases + the callbacks too, not just normal exhaustion.""" + client = jetstream._client + stream = await jetstream.create_stream(name="hb_cancel_stream", subjects=["HBCANCEL.*"]) + consumer = await stream.create_consumer(name="hb_cancel_consumer") + + disconnected = len(client._disconnected_callbacks) + reconnected = len(client._reconnected_callbacks) + + # No messages published — iteration blocks until cancelled. + batch = await consumer.fetch(max_messages=5, max_wait=5.0, heartbeat=1.0) + assert len(client._disconnected_callbacks) == disconnected + 1 + assert len(client._reconnected_callbacks) == reconnected + 1 + + async def consume() -> None: + async for _ in batch: + pass + + task = asyncio.create_task(consume()) + await asyncio.sleep(0.2) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert len(client._disconnected_callbacks) == disconnected + assert len(client._reconnected_callbacks) == reconnected + + @pytest.mark.asyncio async def test_messages_rejects_both_max_messages_and_max_bytes(jetstream: JetStream): """Test ADR-37: messages() cannot accept both max_messages and max_bytes simultaneously.""" diff --git a/nats-schemas b/nats-schemas new file mode 160000 index 00000000..4c3313f1 --- /dev/null +++ b/nats-schemas @@ -0,0 +1 @@ +Subproject commit 4c3313f10b45423b7950378b68073dcd910a822f From ea1d125da36db94bd374532320ac7ca6e61d4cad Mon Sep 17 00:00:00 2001 From: Casper Beyer Date: Wed, 10 Jun 2026 19:04:47 +0200 Subject: [PATCH 4/5] Remove accidentally committed local files --- .claude/scheduled_tasks.lock | 1 - .claude/worktrees/agent-a29dec7e825f1ae16 | 1 - .claude/worktrees/agent-a31635715696d38ca | 1 - .claude/worktrees/agent-a38f8a7500b6c8767 | 1 - .claude/worktrees/agent-a57beae899be22836 | 1 - .claude/worktrees/agent-a58cd5db85506526b | 1 - .claude/worktrees/agent-a6c72205b0c1ee128 | 1 - .claude/worktrees/agent-a6f5b4cbfa2e573f2 | 1 - .claude/worktrees/agent-a7c403367d15dd702 | 1 - .claude/worktrees/agent-a80b7c6a926b6121a | 1 - .claude/worktrees/agent-abfb7b5e082a9843c | 1 - .claude/worktrees/agent-ac3a3c260d121e50b | 1 - .claude/worktrees/agent-adc1096074aaf7d96 | 1 - .claude/worktrees/agent-aed46536a524cf101 | 1 - .claude/worktrees/agent-aedda637b1d250552 | 1 - .claude/worktrees/agent-af0610073a71bb997 | 1 - .claude/worktrees/stoic-shannon-d2a62a | 1 - nats-core/AUDIT-FABLE.md | 447 ---------------------- nats-core/AUDIT.md | 233 ----------- nats-jetstream/AUDIT.md | 235 ------------ nats-schemas | 1 - 21 files changed, 933 deletions(-) delete mode 100644 .claude/scheduled_tasks.lock delete mode 160000 .claude/worktrees/agent-a29dec7e825f1ae16 delete mode 160000 .claude/worktrees/agent-a31635715696d38ca delete mode 160000 .claude/worktrees/agent-a38f8a7500b6c8767 delete mode 160000 .claude/worktrees/agent-a57beae899be22836 delete mode 160000 .claude/worktrees/agent-a58cd5db85506526b delete mode 160000 .claude/worktrees/agent-a6c72205b0c1ee128 delete mode 160000 .claude/worktrees/agent-a6f5b4cbfa2e573f2 delete mode 160000 .claude/worktrees/agent-a7c403367d15dd702 delete mode 160000 .claude/worktrees/agent-a80b7c6a926b6121a delete mode 160000 .claude/worktrees/agent-abfb7b5e082a9843c delete mode 160000 .claude/worktrees/agent-ac3a3c260d121e50b delete mode 160000 .claude/worktrees/agent-adc1096074aaf7d96 delete mode 160000 .claude/worktrees/agent-aed46536a524cf101 delete mode 160000 .claude/worktrees/agent-aedda637b1d250552 delete mode 160000 .claude/worktrees/agent-af0610073a71bb997 delete mode 160000 .claude/worktrees/stoic-shannon-d2a62a delete mode 100644 nats-core/AUDIT-FABLE.md delete mode 100644 nats-core/AUDIT.md delete mode 100644 nats-jetstream/AUDIT.md delete mode 160000 nats-schemas diff --git a/.claude/scheduled_tasks.lock b/.claude/scheduled_tasks.lock deleted file mode 100644 index f6fdde28..00000000 --- a/.claude/scheduled_tasks.lock +++ /dev/null @@ -1 +0,0 @@ -{"sessionId":"bfc143a9-b1ee-4f7a-9269-d7b6c8ecbb82","pid":95970,"procStart":"Tue Jun 9 13:40:29 2026","acquiredAt":1781013766293} \ No newline at end of file diff --git a/.claude/worktrees/agent-a29dec7e825f1ae16 b/.claude/worktrees/agent-a29dec7e825f1ae16 deleted file mode 160000 index 1cbc40bd..00000000 --- a/.claude/worktrees/agent-a29dec7e825f1ae16 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 1cbc40bd6c68cbff14d6b508fe72e437f12ded4a diff --git a/.claude/worktrees/agent-a31635715696d38ca b/.claude/worktrees/agent-a31635715696d38ca deleted file mode 160000 index eb8b959a..00000000 --- a/.claude/worktrees/agent-a31635715696d38ca +++ /dev/null @@ -1 +0,0 @@ -Subproject commit eb8b959a99091d4a9ebee44b4dc9b55ed10a6d84 diff --git a/.claude/worktrees/agent-a38f8a7500b6c8767 b/.claude/worktrees/agent-a38f8a7500b6c8767 deleted file mode 160000 index 4438f0c9..00000000 --- a/.claude/worktrees/agent-a38f8a7500b6c8767 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 4438f0c9045269ac6fc0df141b54385b2d03dde8 diff --git a/.claude/worktrees/agent-a57beae899be22836 b/.claude/worktrees/agent-a57beae899be22836 deleted file mode 160000 index 91c25c98..00000000 --- a/.claude/worktrees/agent-a57beae899be22836 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 91c25c9827c94a4ac38de0bd69f5050bea7bb301 diff --git a/.claude/worktrees/agent-a58cd5db85506526b b/.claude/worktrees/agent-a58cd5db85506526b deleted file mode 160000 index 869d7d20..00000000 --- a/.claude/worktrees/agent-a58cd5db85506526b +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 869d7d202239057ae248491ded06a558505076b5 diff --git a/.claude/worktrees/agent-a6c72205b0c1ee128 b/.claude/worktrees/agent-a6c72205b0c1ee128 deleted file mode 160000 index e52ac7c6..00000000 --- a/.claude/worktrees/agent-a6c72205b0c1ee128 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit e52ac7c6dc146bdf197fcfbdf63db6f700cdb1d6 diff --git a/.claude/worktrees/agent-a6f5b4cbfa2e573f2 b/.claude/worktrees/agent-a6f5b4cbfa2e573f2 deleted file mode 160000 index fa8fb46e..00000000 --- a/.claude/worktrees/agent-a6f5b4cbfa2e573f2 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit fa8fb46e51730cbcc44da795eb78e28ffed5cab5 diff --git a/.claude/worktrees/agent-a7c403367d15dd702 b/.claude/worktrees/agent-a7c403367d15dd702 deleted file mode 160000 index f086de66..00000000 --- a/.claude/worktrees/agent-a7c403367d15dd702 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit f086de66e5f757702eebfafb3436ae4b3c847b0d diff --git a/.claude/worktrees/agent-a80b7c6a926b6121a b/.claude/worktrees/agent-a80b7c6a926b6121a deleted file mode 160000 index 138ed1a1..00000000 --- a/.claude/worktrees/agent-a80b7c6a926b6121a +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 138ed1a1ce2db202c2511026e45cc326b19fe07d diff --git a/.claude/worktrees/agent-abfb7b5e082a9843c b/.claude/worktrees/agent-abfb7b5e082a9843c deleted file mode 160000 index e4115c6c..00000000 --- a/.claude/worktrees/agent-abfb7b5e082a9843c +++ /dev/null @@ -1 +0,0 @@ -Subproject commit e4115c6ce5e7ad394e70f144dfe3ebcef145b5fc diff --git a/.claude/worktrees/agent-ac3a3c260d121e50b b/.claude/worktrees/agent-ac3a3c260d121e50b deleted file mode 160000 index b4e522ef..00000000 --- a/.claude/worktrees/agent-ac3a3c260d121e50b +++ /dev/null @@ -1 +0,0 @@ -Subproject commit b4e522ef38d221b6514abb8fd9c682ceed00bcce diff --git a/.claude/worktrees/agent-adc1096074aaf7d96 b/.claude/worktrees/agent-adc1096074aaf7d96 deleted file mode 160000 index 6a53ec97..00000000 --- a/.claude/worktrees/agent-adc1096074aaf7d96 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 6a53ec97a25c545e794632ab1c0782e30ca152eb diff --git a/.claude/worktrees/agent-aed46536a524cf101 b/.claude/worktrees/agent-aed46536a524cf101 deleted file mode 160000 index fb1771b4..00000000 --- a/.claude/worktrees/agent-aed46536a524cf101 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit fb1771b4ded328e7648a5f61fc77c3cfa7c570b5 diff --git a/.claude/worktrees/agent-aedda637b1d250552 b/.claude/worktrees/agent-aedda637b1d250552 deleted file mode 160000 index 303e58c7..00000000 --- a/.claude/worktrees/agent-aedda637b1d250552 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 303e58c73f1a7f025f99111660007e848bbef0e1 diff --git a/.claude/worktrees/agent-af0610073a71bb997 b/.claude/worktrees/agent-af0610073a71bb997 deleted file mode 160000 index 02bf3464..00000000 --- a/.claude/worktrees/agent-af0610073a71bb997 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 02bf34644ee4abdc687287002259b61403b612be diff --git a/.claude/worktrees/stoic-shannon-d2a62a b/.claude/worktrees/stoic-shannon-d2a62a deleted file mode 160000 index c91177f0..00000000 --- a/.claude/worktrees/stoic-shannon-d2a62a +++ /dev/null @@ -1 +0,0 @@ -Subproject commit c91177f0deccbae9c55b31d64fa5f139356f1d16 diff --git a/nats-core/AUDIT-FABLE.md b/nats-core/AUDIT-FABLE.md deleted file mode 100644 index 86153dfa..00000000 --- a/nats-core/AUDIT-FABLE.md +++ /dev/null @@ -1,447 +0,0 @@ -# nats-core Audit (Fable) - -Full-source review of `nats-core/src/nats/client/` (~3,500 lines): correctness issues, -parity defects against expected NATS client behavior, and unpythonic patterns. -File references are relative to `nats-core/src/nats/client/`. - ---- - -## Critical - -### C1. Top-level `import nkeys` breaks the package without the optional extra - -`__init__.py:40` imports `nkeys` unconditionally, but `pyproject.toml` declares -`dependencies = []` and ships nkeys only as the `nkeys` extra. A plain -`pip install nats-core` followed by `import nats.client` raises `ImportError`. -The import should be deferred into `_setup_nkey_auth` / `_setup_jwt_auth` (the -only consumers), with a clear error message pointing at `nats-core[nkeys]`, -mirroring how the websocket extra is handled in `connection.py:317-321`. - -### C2. Ping state is not reset on reconnect → reconnect death spiral - -`__init__.py:960-997` (reconnect success path) restores subscriptions and stats -but never resets `_pings_outstanding`, `_last_ping_sent`, or `_last_pong_received`. -If the original disconnect was caused by reaching `max_outstanding_pings` -(`_pings_outstanding == 2`), the value survives into the new connection. The new -`_write_loop` then hits its first idle timeout, sees -`_pings_outstanding >= _max_outstanding_pings` (`__init__.py:602`), and force -disconnects again. Every reconnected connection lives at most one -`ping_interval` after a ping-timeout disconnect, forever. - -### C3. `CancelledError` swallowed inside the reconnect loop - -`__init__.py:1003-1006`: - -```python -except (asyncio.CancelledError, TimeoutError) as e: - logger.error("Failed to connect to %s: %s", server, type(e).__name__) - self._last_server = server - continue -``` - -If `close()` cancels the task while it is inside `establish_connection` for one -server, the cancellation is consumed and the loop *continues* to the next -server. `close()` then blocks on `await self._read_task` until the entire -reconnect schedule (all servers × all attempts × backoff) runs dry. -Cancellation must be re-raised; never `continue` past a `CancelledError`. - -### C4. Initial connect treats EOF (and any non-PONG) as success - -`__init__.py:1849-1872`: after CONNECT+PING, the response is only checked with -`isinstance(response, Err)`. If the server closes the connection without an -`-ERR` (e.g. some auth/TLS-verify failures), `parse()` returns `None` and -`connect()` happily returns a dead `Client`. The reconnect path -(`__init__.py:945-958`) handles all four cases correctly (`None`, `Err`, -non-`Pong`, timeout) — the initial path should do the same. This verification -logic is duplicated in two places and has already diverged; extract it. - ---- - -## High - -### H1. Keepalive PING starves under continuous publish traffic - -`_write_loop` (`__init__.py:583-611`) only sends a PING when -`wait_for(self._flush_waker.wait(), timeout=self._ping_interval)` times out. -Any publish sets the waker, so on a connection with steady outgoing traffic the -timeout branch never fires and no PING is ever sent — a dead server (or a -half-open TCP connection) is never detected as long as the app keeps -publishing. Stale-connection detection must be driven by a timer independent of -write activity (check `now - _last_ping_sent >= ping_interval` on every loop -iteration, not only in the timeout branch). - -### H2. The 5 ms minimum flush interval serializes request/reply to ~200 req/s - -`_DEFAULT_MIN_FLUSH_INTERVAL = 0.005` (`__init__.py:78`) combined with -`_write_loop`'s enforced sleep (`__init__.py:589-592`) means any publish that -lands within 5 ms of the previous flush waits out the remainder. Sequential -`request()` calls each pay this: publish → buffered → flusher sleeps ~5 ms → -flush → response. A local round trip that should take ~100 µs takes ~5 ms, -capping serialized request throughput at roughly 200/s. Reference clients -coalesce at sub-millisecond granularity (flush as soon as the event loop yields, -i.e. coalescing happens naturally via the task scheduling, not a fixed timer). -Consider flushing immediately when the buffer transitions from empty, and using -the interval only as a backpressure coalescing hint. Also note -`_last_flush = current_time` (`__init__.py:596`) stamps the pre-sleep time, so -the interval bookkeeping is wrong by up to the sleep duration. - -### H3. Publish buffer grows unboundedly while reconnecting - -`publish()` (`__init__.py:1141-1148`) appends to `_pending_messages` during -`RECONNECTING` (status check only blocks `CLOSED`/`CLOSING`), and -`_force_flush()` (`__init__.py:1032-1033`) returns early when the connection is -down without clearing the buffer — there is no flusher task alive either. A -busy publisher during a long outage grows the buffer without limit until OOM. -There needs to be a reconnect-buffer cap (cf. legacy `pending_size`, nats.go -`ReconnectBufSize`, default 8 MB) with a defined overflow behavior (raise or -drop). - -### H4. `subscribe()`/`unsubscribe()` are broken while reconnecting - -- `subscribe()` (`__init__.py:1193-1210`) registers the subscription in - `_subscriptions` *before* writing SUB. During `RECONNECTING` the write raises - `ConnectionError`, the caller sees a failure, but the subscription stays - registered and gets silently replayed on reconnect — a retry then creates a - duplicate. -- `Subscription.unsubscribe()` → `_unsubscribe()` (`__init__.py:1230-1241`) - writes UNSUB for any status other than `CLOSED`/`CLOSING`; during - `RECONNECTING` the write raises and the `del self._subscriptions[sid]` never - runs, so the sub is resurrected on reconnect even though the user - unsubscribed. - -Both should tolerate a down connection: mutate local state, skip the wire write, -and rely on the reconnect replay (which already reads `_subscriptions`). - -### H5. WebSocket clients poison the reconnect pool with TCP endpoints - -`connect()` (`__init__.py:1791-1792`) and `_handle_info` -(`__init__.py:762-765`) append `info.connect_urls` — bare `host:port` TCP -endpoints — into `_server_pool` regardless of transport. For a `ws://`/`wss://` -client, the reconnect loop (`__init__.py:856-871`) then parses those as -`nats://host:port` and attempts raw TCP to the cluster's client ports. The -INFO field `ws_connect_urls` (already declared in `protocol/types.py:85`) exists -precisely for this and is ignored. WebSocket clients should populate the pool -from `ws_connect_urls` only. - -### H6. `drain()` does not actually stop publishing (contradicts its own docs) - -`drain()`'s docstring promises "No new messages can be published", but -`publish()` (`__init__.py:1095`) only rejects `CLOSED`/`CLOSING`; `DRAINING` and -`DRAINED` sail through (likewise `subscribe()` at `__init__.py:1179` and -`request()` at `__init__.py:1278`). Reference clients raise a -draining-specific error for all three. Additionally, -`asyncio.wait_for(asyncio.gather(*drain_tasks), ...)` (`__init__.py:1356`) is a -no-op timeout: `Subscription.drain()` returns immediately after -`queue.shutdown(immediate=False)` — nothing waits for consumers to work off the -queues, so the timeout protects nothing. - -### H7. No header validation → CRLF injection into the header block - -`encode_headers` (`protocol/command.py:52-57`) interpolates keys and values -into the wire format with no validation. A value (or key) containing `\r\n` -injects arbitrary header lines; a key containing `:` or whitespace corrupts the -block. Framing stays intact (the block is length-prefixed) so this is not a -command injection like the subject case, but it silently produces malformed or -attacker-shaped headers. Subjects are carefully validated -(`__init__.py:194-226`); headers deserve the same. - ---- - -## Medium - -### M1. Concurrent `_force_disconnect` races: status overwrite and double reconnect - -`_force_disconnect` (`__init__.py:786-826`) sets -`self._status = DISCONNECTED` *before* taking `_reconnect_lock`. A second caller -(read loop EOF + write loop max-pings can both call) overwrites `RECONNECTING` -with `DISCONNECTED` while the first holds the lock through the *entire* -reconnect cycle. When the first cycle succeeds and releases the lock, the -second caller observes `old_status` not in `(CLOSING, CLOSED)`, -`_reconnecting == False`, and starts a second reconnect cycle on top of the -fresh, healthy connection — replacing `self._connection` and leaking the old -one. The status flip and the should-I-reconnect decision need to happen -atomically, and a freshly reestablished connection must not be torn down by a -stale disconnect notification (guard on connection identity). - -### M2. `_force_disconnect` cancels the task it runs on - -The read loop calls `_force_disconnect`, which cancels `_read_task` — the -currently running task (`__init__.py:792-795`). The self-`await` raises and the -pending cancellation happens to be consumed by the surrounding -`contextlib.suppress(asyncio.CancelledError, ...)`, after which the reconnect -loop continues on a task whose `cancelling()` count is permanently 1 (no -`uncancel()`). This works by accident of CPython task-step ordering and will -misbehave inside any future `asyncio.timeout()` scope. Restructure so teardown -cancels *the other* task only, or hand reconnection off to a dedicated task. - -### M3. Inbound `MAX_CONTROL_LINE` of 4096 can kill valid connections - -`protocol/message.py:46,346-348` rejects any server control line over 4096 -bytes. 4096 is the *server's default* limit for client→server lines; -`max_control_line` is configurable upward, and a long subject + reply easily -exceeds 4 KiB on a server configured for it. Other clients don't enforce a -fixed inbound cap (or honor the server's advertised one). Also note the check -runs *after* `readline()`, whose own `StreamReader` 64 KiB limit raises a raw -`ValueError`/`LimitOverrunError` first for truly long lines — a noisy break of -the read loop instead of a clean protocol error. - -### M4. Slow-consumer handling is inconsistent and leaks `QueueShutDown` - -- In `Subscription._enqueue` (`subscription.py:193-207`), the byte-limit check - raises *before* callbacks run, but `put_nowait` raises `QueueFull` *after* - callbacks ran — so whether registered callbacks observe a dropped message - depends on which limit tripped. -- The client catches `(asyncio.QueueFull, ValueError)` (`__init__.py:653,734`) - but not `asyncio.QueueShutDown`. Today a shut-down subscription is always - removed from `_subscriptions` first, so it is unreachable in practice — but - one refactor away from an unhandled exception that kills the read loop. -- Dropped-message accounting uses `len(payload)` while `_enqueue` budgets - `len(message.data)` — same value, but the duplicated logic in `_handle_msg` - and `_handle_hmsg` (see U1) makes such drift likely. - -### M5. Disconnect/teardown paths produce ERROR-level tracebacks for normal events - -A connection dropped mid-payload raises `IncompleteReadError` inside the read -loop's `except Exception` (`__init__.py:543-545`), logging a full traceback via -`logger.exception("Error in read loop")` for an ordinary network event. The -WebSocket transport is worse: EOF in `readline()` propagates -`IncompleteReadError` (`connection.py:280-287`) instead of returning `b""` like -`TcpConnection.readline`, so a clean server close logs as an error instead of -"Connection closed by server". Distinguish expected EOF/reset from genuine -parser/internal errors. - -### M6. Dead/misleading exception handling in `_read_loop` - -`__init__.py:546-548` catches `(asyncio.CancelledError, ParseError)` at the -outer level and returns *without* reconnecting — but `ParseError` is always -caught first by the inner `except Exception` (which breaks into the reconnect -path). The outer `ParseError` arm is unreachable and documents the wrong -behavior. Similarly, `int()` failures in `parse_msg`/`parse_hmsg` -(`protocol/message.py:187-190,228-233`) escape as raw `ValueError` instead of -`ParseError`. - -### M7. `flush()` / `rtt()` misbehave when not connected - -- `flush()` during `RECONNECTING`: `_force_flush` no-ops, then `_ping()` writes - straight to the dead connection and raises `ConnectionError` - (`__init__.py:1040-1046`). Callers get a transport exception for a state the - client is supposed to be managing. -- `flush()` on PONG timeout force-disconnects (`__init__.py:1071-1073`) — - reasonable — but `rtt()` lets the `TimeoutError` escape without any state - handling, and both share `_pong_waker`, so concurrent `flush()`/`rtt()` - calls can complete on each other's PONGs (rtt underreports). -- `_ping()` (`__init__.py:1040`) increments `_pings_outstanding` without the - max-outstanding check that `_queue_ping` has; the two near-identical methods - should be one. - -### M8. Publishing with headers is not gated on server support - -`connect()` always sends `headers: true`, and `publish()` happily encodes HPUB -regardless of `server_info.headers` (`__init__.py:1114-1132`). Against an old -or proxied server that advertises `headers: false`, reference clients raise a -clear "headers not supported" error; this client sends a frame the server will -treat as a protocol error and disconnects. - -### M9. `ServerInfo.from_protocol` crashes the read loop on minimal INFO - -`__init__.py:145-152` indexes `server_id`, `version`, `go`, `host`, `port`, -`headers` directly. Any async INFO (or nonstandard server/proxy) missing one of -these raises `KeyError` inside `_handle_info`, which breaks the read loop and -tears down the connection. Use `.get()` with defaults for everything that isn't -strictly load-bearing. - -### M10. Reconnect pool/server-selection nits - -- `_last_server` skip (`__init__.py:850-851`) skips the last *attempted* - server, and `_last_server` is updated on every failed attempt too - (`__init__.py:1005,1009`) — the net effect is hard to reason about and - differs from the usual "deprioritize the server we just lost" semantics. -- The IPv6 normalization reassigns the loop variable `server` - (`__init__.py:861-869`), so `_last_server` stores the bracketed form while - the pool holds the unbracketed one — the skip comparison never matches for - IPv6 entries. -- `no_randomize=False` shuffles only the tail, pinning `server_pool[0]` - (`__init__.py:845-848`); full-pool shuffle is the conventional behavior. -- `reconnect_max_attempts` counts *passes over the whole pool*, not per-server - attempts, so the effective retry budget scales with cluster size. (The - `0 == unlimited` sentinel is already tracked separately for a follow-up PR.) -- Reconnect verification reads exactly one protocol message and fails on - anything that isn't PONG (`__init__.py:955-958`); a server that interleaves - an async INFO (e.g. entering LDM) before the PONG fails the attempt. Loop - until PONG/ERR. - -### M11. Write-buffer bypass reorders commands - -`subscribe()`, `_unsubscribe()`, `_handle_ping`'s PONG, and `_queue_ping` write -directly to the connection while published messages sit in -`_pending_messages`. A SUB can therefore reach the server *before* a PUB that -the application issued earlier — observable with echo enabled (you receive your -own earlier publish) and surprising for anyone reasoning about ordering. Either -route everything through the buffer or flush the buffer before direct writes. - ---- - -## Parity gaps - -### P1. `connect()` accepts a single URL only - -No way to seed multiple servers (`nats.connect(["nats://a", "nats://b"])` or -`servers=[...]`). The pool only grows via `connect_urls` after the first -connect succeeds — so the bootstrap server is a single point of failure. Every -reference client accepts a server list. - -### P2. Auto-unsubscribe is unreachable - -`encode_unsub` supports `max_msgs` (`protocol/command.py:105-117`) but nothing -exposes it: `Subscription.unsubscribe()` takes no limit and there is no -`max_msgs=` on `subscribe()`. `UNSUB ` / auto-unsubscribe-after-N is -standard across clients (and required by request-many patterns). - -### P3. Callbacks are sync-only and weakly typed - -`add_disconnected_callback` et al. accept `Callable[[], None]` only — an -`async def` callback would produce an un-awaited coroutine and a warning. -Legacy nats-py and most asyncio APIs accept coroutine callbacks. Also -`add_error_callback` takes `Callable[[Exception | str], None]` -(`__init__.py:299,1512`): server `-ERR` strings are passed raw while -slow-consumer errors arrive as exceptions. Wrap protocol errors in an exception -type so the callback signature is just `Callable[[Exception], ...]`. - -### P4. Missing surface - -- No closed/connection-terminal callback (only disconnected/reconnected/error/LDM). -- No `is_connected`-style convenience or `connected_url`/current-server accessor. -- `ClientStatistics` lacks `errors_received` (legacy parity). -- `request()` takes `headers: dict[...]` but not `Headers` - (`__init__.py:1257`), and `subject: str` but not `bytes` — both inconsistent - with `publish()`. -- Subscription pending defaults are 65 536 msgs (`__init__.py:1160`) vs the - conventional 512 × 1024; worth a deliberate decision either way. -- Repeated authorization errors during reconnect retry forever (until - max attempts); reference clients abort a server after repeated auth - failures to avoid hammering. -- `new_inbox()` uses `uuid4().hex` (32 chars + prefix) where other clients use - NUID (22 chars, faster); fine functionally, but inboxes are hot-path. - -### P5. Inbox prefix validation misses whitespace/CRLF - -`__init__.py:410-417` rejects `>`, `*`, and trailing `.` but not spaces or -CRLF. An injected prefix is only caught later when `publish()` validates the -reply subject — surfacing as a confusing per-request `ValueError` instead of a -clear `connect()`-time error. - ---- - -## Unpythonic / code-quality - -### U1. Large-scale duplication - -- `_handle_msg` and `_handle_hmsg` (`__init__.py:626-756`) are ~90% identical - (mux dispatch, slow-consumer handling, error callbacks) — 130 lines that - should be one helper. -- The reconnect CONNECT/verify block (`__init__.py:896-958`) duplicates - `connect()`'s (`__init__.py:1794-1872`), and the two have already diverged - (C4). Extract a shared "send CONNECT, await PONG" helper. -- `subscribe()` inlines the body of `_subscribe()` (`__init__.py:1204-1210` vs - `1214-1228`). -- `close()` cancels `_read_task`/`_write_task` twice, ~30 lines apart - (`__init__.py:1419-1427` and `1451-1462`). -- `_queue_ping` vs `_ping` (M7). - -### U2. `assert` used for runtime control flow - -`__init__.py:632,692` (`assert self._request_prefix is not None` — an -unexpected sid-0 message under `python -O` becomes an `AttributeError`/wrong -behavior, and under normal mode an `AssertionError` that kills the read loop) -and `_setup_jwt_auth`'s `assert isinstance(...)` (`__init__.py:1650-1651`, -should raise `TypeError`). Asserts vanish under `-O`; user-input validation -must not rely on them. - -### U3. `Headers` is a fake dataclass and not a Mapping - -`message.py:8-26` decorates `Headers` with `@dataclass` but hand-writes -`__init__` and `__eq__`, so the decorator generates nothing useful (and the -custom `__eq__` silently sets `__hash__ = None`). More importantly it supports -neither `headers["Key"]`, `"Key" in headers`, `len(headers)`, nor iteration — -`collections.abc.Mapping` is the obvious shape (`get`, `items`, `keys`, -`values`, `__contains__` for free). Also `asdict()` returns a shallow copy -whose lists are shared with internal state — mutating -`headers.asdict()["X"].append(...)` mutates the live headers. - -### U4. Event-loop API inconsistency - -`asyncio.get_event_loop().time()` in `__init__.py:452,457,560,576,589,599` vs -`asyncio.get_running_loop()` at `__init__.py:1045,1053`. Inside coroutines the -modern form is `get_running_loop()` everywhere; `get_event_loop()` is -soft-deprecated and slower. - -### U5. `Subscription.messages` swallows `RuntimeError` to end iteration - -`subscription.py:123-130` breaks the async iterator on *any* `RuntimeError`, -not just "subscription closed". A genuine `RuntimeError` from user code or -asyncio internals silently terminates the message stream. Define a dedicated -`SubscriptionClosedError` (or re-raise unless the subscription is actually -closed). The `next()`-raises-`RuntimeError` contract has the same smell. - -### U6. Protocol-module oddities - -- `ping()`/`pong()`/`parse_info()`/`parse_err()` are `async def` with no - awaits (`protocol/message.py:258-325`); `Ping`/`Pong` could be module-level - singletons returned synchronously. -- `if TYPE_CHECKING: pass` dead block (`protocol/message.py:10-11`). -- `parse` accepts bare `b"ERR"` in addition to `b"-ERR"` - (`protocol/message.py:365`) — the server never sends the former. -- Control-line split is single-space only (`protocol/message.py:350`); - consecutive spaces/tabs (which the Go parser tolerates) produce empty args - and raw `ValueError`s. -- `ConnectInfo` marks every default field `Required[...]` in a `total=True` - TypedDict (`protocol/types.py:14-22`) — redundant noise. - -### U7. Exception-chaining and transport nits - -- `open_tcp_connection` raises `ConnectionError(msg)` without `from e` - (`connection.py:209-211`); the websocket twin chains correctly. -- `TcpConnection.upgrade_to_tls` pokes `self._writer._transport` - (`connection.py:118`) — known asyncio wart, but deserves a comment on why - it's safe w.r.t. `drain()` and the reader protocol. -- `open_websocket_connection` passes `max_size=None` (`connection.py:328`) — - unbounded frame buffering from the server; the parser's own caps never get a - chance to apply. -- `TcpConnection.read()` (and `Connection.read` in the protocol) is dead code — - nothing in the client calls byte-granularity `read`. - -### U8. Docstring defects - -- `Client.server_info` is annotated `-> ServerInfo | None` - (`__init__.py:472-475`) but `_server_info` is always set; the `| None` forces - every caller to narrow for no reason. -- `connect()`'s docstring references type aliases named `Nkey` and `JWT` - (`__init__.py:1720-1724`) — the actual names are `NkeySeed`/`NkeyHandlers` - and `JWTCredentials`/`JWTHandlers`. -- `_validate_subject`, `_validate_queue`, and the `skip_subject_validation` - docs cite nats.go/nats.rs behavior by name - (`__init__.py:202-206,232-234,1729-1730`); per project convention, describe - the behavior directly instead of citing other clients. -- The type aliases (`NkeySeed`, `JWTCredentials`, …) are public-facing but - absent from `__all__` (`__init__.py:1909-1924`). - ---- - -## Summary - -| Severity | Count | Headliners | -|----------|-------|------------| -| Critical | 4 | broken bare install (C1), reconnect ping death spiral (C2), swallowed cancellation (C3), EOF-as-success on connect (C4) | -| High | 7 | keepalive starvation (H1), 5 ms request serialization (H2), unbounded reconnect buffer (H3), sub/unsub during reconnect (H4), ws pool poisoning (H5), drain doesn't block publish (H6), header injection (H7) | -| Medium | 11 | disconnect races (M1/M2), inbound control-line cap (M3), slow-consumer inconsistencies (M4), noisy/dead error paths (M5/M6) | -| Parity | 5 | single-URL connect (P1), no auto-unsubscribe (P2), sync-only callbacks (P3) | -| Quality | 8 | duplication (U1), asserts as control flow (U2), fake-dataclass Headers (U3) | - -The architecture (Protocol-typed transports, NamedTuple wire messages, -`match`-based dispatch, Queue.shutdown-driven subscription lifecycle) is sound -and idiomatic for 3.13. The risk is concentrated in connection lifecycle code: -`_force_disconnect`/reconnect is one 240-line function owning cancellation, -locking, pool management, CONNECT/auth replay, and state transitions — C2, C3, -C4, M1, M2, M6, and M10 all live there. Decomposing it (dedicated reconnect -task, shared CONNECT/verify helper, atomic state transitions) would resolve the -bulk of this audit in one structural change. diff --git a/nats-core/AUDIT.md b/nats-core/AUDIT.md deleted file mode 100644 index 418764a6..00000000 --- a/nats-core/AUDIT.md +++ /dev/null @@ -1,233 +0,0 @@ -# nats-core audit (2026-05-27) - -Eight specialized agents reviewed `nats-core/src/nats/client/` through different -lenses (code smells, pythonic patterns, ADR compliance, nats.go divergence, -security, asyncio/concurrency, protocol parser, public API). This document is -the deduplicated, severity-ranked synthesis. Items confirmed by multiple lenses -are noted in parentheses. - -## Critical — fix first - -1. **TLS downgrade on reconnect leaks credentials** *(security, ADR-40)* - `__init__.py:872-896, 936, 1844`. On reconnect, TLS upgrade is gated on the - *current* INFO advertising `tls_required`/`tls_available`. A MITM stripping - those flags causes CONNECT — with `auth_token`/`password`/`jwt`/`nkey`/`sig` - — to be written in plaintext. Pin TLS intent at first connect; never consult - server INFO to downgrade. - -2. **Server-driven `connect_urls` is unbounded and unvalidated** *(security; ADR-40 defines the mechanism, not the hardening)* - `__init__.py:698-701, 968-971`. A hostile/compromised server can append - arbitrary hosts to the client's reconnect pool; entries are never pruned, no - allowlist, TLS hostname is not re-pinned for discovered URLs. Combined with - #1 this is a credential-stealing chain. ADR-40 specifies that advertised - URLs are stored and used for reconnect, with discovery **on by default** and - only a boolean opt-*out* ("Ignore advertised servers", default false); it - says nothing about pruning, allowlisting, bounding the pool, or re-pinning - TLS, and its Security Considerations section is an empty stub. So the fix - here is hardening beyond the spec, not ADR conformance: keep ADR-40's - opt-out default (do **not** flip discovery to opt-in — that would diverge - from the spec and every other client), prune on each INFO, and require TLS - for inherited servers when the seed was TLS. - -3. ~~**Concurrent writes to the connection are unsynchronized** *(asyncio)*~~ - **Retracted.** Every call site passes a complete frame (`encode_pub`/ - `encode_ping`/`encode_sub` all return one `bytes`; the buffered path - `b"".join(...)` first). `Connection.write` is `self._writer.write(data); - await self._writer.drain()` — the `writer.write` call is a single C-level - append, atomic under the GIL, and `drain()` flushes in FIFO append order. - Two coroutines calling `connection.write(complete_frame)` land their frames - adjacent on the wire, not interleaved. Would be a real hazard if a frame - were split across two writes with an await between — it isn't. - -4. **`_force_disconnect` invoked from the read task awaits itself** *(asyncio)* - `__init__.py:728-736`. The `RuntimeError("Task cannot await on itself")` is - silently swallowed by `contextlib.suppress`. Disconnect/reconnect proceeds - before the old reader actually terminates. Detect - `asyncio.current_task() is self._read_task` and skip the await, or move - reconnect to a supervisor task. - -5. **Reader loop silently disconnects on `ParseError`** *(asyncio, parser)* - `__init__.py:482`. `except (CancelledError, ParseError): return` skips - `_force_disconnect`, leaving the client in `CONNECTED` with a dead socket. - Re-raise `CancelledError`; route `ParseError` through reconnect. - -## High - -6. **Credentials logged at DEBUG** *(security)* — [PR #955](https://github.com/nats-io/nats.py/pull/955) - `__init__.py:935, 1844`. `logger.debug("->> CONNECT %s", json.dumps(connect_info))` - dumps `auth_token`/`password`/`jwt`/`nkey`/`sig` in cleartext. Redact before - logging. - -7. **CRLF / whitespace injection in subject, reply, queue, headers** *(security, parser)* - `__init__.py:1078-1205`, `protocol/command.py:28-117, 52-57`. Encoders - interpolate caller-supplied bytes verbatim with `b"PUB %b ..."` / - `f"SUB {subject} {sid}\r\n"`. A `\r\n` in a subject or header value forges - arbitrary protocol commands. Validate against the subject grammar; reject - CR/LF in header keys/values. Inbox-prefix validation at `__init__.py:347-356` - also misses CR/LF/whitespace. - -8. **No typed `-ERR` model — Stale-Connection, Auth, Permissions invisible** *(parser, nats.go, ADR-7)* — [PR #956](https://github.com/nats-io/nats.py/pull/956) - `protocol/message.py:304-307`, `errors.py`, `__init__.py:711-720`. `-ERR` - arrives as an opaque string; only `MaxPayloadError` exists locally and only - for client-side checks. Callers can't programmatically react to auth failure - vs transient error vs reconnect-required error. (Abort-reconnect-on-auth is - a follow-up now unblocked.) - -9. **Sequential request tokens within a session are predictable** *(security, nats.go)* - `__init__.py:1283`. `_next_request_id` is a monotonic int under a per-client - UUID prefix. A subscriber to `_INBOX..*` (e.g. a co-tenant in the same - account) can reply-spoof other requests. Use `secrets.token_hex(8)` per - request — nats.go uses NUIDs per request, not sequential ints. - -10. **`_force_flush` clear-after-await drops and double-sends concurrently-buffered messages** *(asyncio)* - `__init__.py:1090-1101, 1204-1211`. Not a data race — the GIL and the - single event loop already serialize bytecode, and the limit-check → - `append`/`+=` tail of `publish()` is await-free, so two callers cannot both - observe under-limit and overrun `_max_pending_bytes`. The hazard is - cooperative interleaving across the one suspension point, `await - self._connection.write(b"".join(self._pending_messages))` in - `_force_flush`, and only with concurrent publishers (e.g. `gather`, or - publishing from multiple tasks). The `join` snapshots the buffer, then - `drain()` yields; another `publish()` can `append` into the same list - before the resuming flush calls `clear()`, so that message is silently - dropped (it was never in the snapshot). Worse, two over-limit publishers - can both pass the `if not self._pending_messages` guard and `write` the - same buffer before either clears, putting those messages on the wire twice. - Swap in a fresh list *before* the await (`batch, self._pending_messages = - self._pending_messages, []; self._pending_bytes = 0; await ...write(b"".join(batch))`), - which closes both the drop and the double-send. - -11. **HMSG/MSG numeric parsing crashes → silent disconnect** *(parser)* - `protocol/message.py:185-194, 226-241`. `int(args[2])` on corrupt/attacker - input raises, the read loop's blanket `except Exception` catches it and - `break`s. Same pattern for non-UTF-8 bytes in headers (`message.py:124`) — - strict `.decode()` plus the silent break means one bad frame disconnects - the client without a typed error. - -12. **`+OK` is not parsed at all** *(parser)* — [PR #949](https://github.com/nats-io/nats.py/pull/949) - `protocol/message.py:354-369` has no `b"+OK"` arm. Any user passing - `verbose=True` in a custom CONNECT silently disconnects on the first server - reply. - -13. **Treating any non-200 status as error breaks 1xx informational responses** *(API, parser)* - `__init__.py:1295`, `errors.py:71`. `request()` raises `StatusError` on - `100 Idle Heartbeat` / `100 Flow Control`. Status code is also a `str`, - not `int`. - -14. **`Client` is a god-class — 1923 lines in `__init__.py`, ~40 attrs, duplicated handshake/CONNECT/TLS logic** *(smells)* — [PR #948](https://github.com/nats-io/nats.py/pull/948) extracts `establish_connection`, killing ~150 lines of the open-socket + read-INFO + maybe-upgrade-TLS dedup. Remaining work: `_build_connect_info` + `_perform_handshake`, then `RequestMultiplexer` / `WriteBuffer` / `ServerPool` extraction. - -15. **`Headers` is not a `Mapping`, keys are case-sensitive** *(API, parser, spec)* — [PR #954](https://github.com/nats-io/nats.py/pull/954) - `message.py:8-109`. Cannot do `msg.headers["trace-id"]`, `in`, `len()`, - `iter()`, `dict(headers)`. ADR-21/HTTP-style is case-insensitive; nats.go - canonicalizes. Inherit `collections.abc.MutableMapping`, store case-preserved - but case-insensitive lookup. - -16. **No `Message.respond()`** *(API)* — [PR #953](https://github.com/nats-io/nats.py/pull/953) - Every reply forces `await client.publish(msg.reply, ...)` with manual - `msg.reply is not None` guards. This is the single most-used convenience in - NATS — add it. - -## Medium - -17. **No offline publish buffer / no PUB replay on reconnect** *(nats.go)* — - `publish` during `RECONNECTING` raises `RuntimeError`; nats.go buffers up to - 8 MB and replays. -18. **Reconnect backoff/jitter semantics diverge from nats.go** *(nats.go)* — - multiplicative jitter + exponential base doubling; Go uses additive - `ReconnectJitter` (100 ms / 1 s TLS) with no exponential. Missing - `ReconnectJitterTLS`, `CustomReconnectDelayCB`, `RetryOnFailedConnect`, - `IgnoreAuthErrorAbort`, `ReconnectBufSize`, `ConnectedCB`/`ClosedCB`, - `DiscoveredServersCB`, `ReconnectErrCB`, `CustomDialer`, `RootCAsCB`. -19. **Per-server reconnect counter / dead-server eviction missing** *(nats.go)* — [PR #960](https://github.com/nats-io/nats.py/pull/960). Every server was retried forever; now `reconnect_max_attempts` is per-server and servers are evicted on exhaustion (behavioral change). -20. **ADR-5 lame-duck mode** *(ADR)* — detected and callback fires, but no - proactive jittered self-disconnect/migration. -21. **ADR-11 multi-IP hostname fallback** *(ADR)* — `asyncio.open_connection` - picks one address; no per-IP retry or randomization. -22. ~~**`_next_sid` and `_next_request_id` non-atomic increment** *(asyncio)*~~ — - **Retracted.** `sid = self._next_sid; self._next_sid += 1` is pure Python - bytecode with no `await` between read and increment. The asyncio loop is - single-threaded and only context-switches at `await`, so the sequence is - atomic for our purposes. Would be a real race in a multi-threaded context; - we're not in one. -23. **`flush()`/`rtt()` share a single `_pong_waker`** *(asyncio)* — keepalive - PONG can wake `flush()` early; two concurrent flushes both wake on a single - PONG. Use a deque of per-call futures. -24. **`_force_disconnect` not idempotent under concurrent callers** *(asyncio)* — - lock only covers the post-cancel block; `close()` racing with reader- - triggered disconnect double-runs. -25. **`subscribe`/`_unsubscribe` bypass the publish buffer** *(smells)* — - head-of-line ordering surprise between buffered PUB and unbuffered SUB to - the same subject. -26. **No `unsubscribe(max_msgs=...)`** *(API, nats.go)* — protocol supports it - (`encode_unsub` even takes the arg), but no API surface. -27. **No shared exception base class** *(API)* — MIGRATION.md admits this. Bare - `RuntimeError("Connection is closed")` in `publish`/`subscribe`/`request`. - Add `NATSError`, `ConnectionClosedError`. -28. **In-flight `_request_futures` are not failed on disconnect** *(asyncio, smells)* — - callers see `TimeoutError` instead of a connection error. -29. **`Subscription._enqueue` raises mixed exceptions** *(smells)* — `ValueError` - for bytes overrun, `QueueFull` for message overrun; callers branch identically. -30. **Pending limits immutable after `subscribe()`** *(nats.go)* — no - `set_pending_limits` on `Subscription`. -31. **ADR-4 header field-name validation missing** *(ADR)* — invalid keys reach - the wire and cause server disconnects. -32. **User-supplied `nkey_signature_handler` bytes are not base64url-encoded** *(ADR-14)* — - `_setup_nkey_auth` correctly encodes when the client signs, but the - `NkeyHandlers` path forwards `.decode()` raw bytes. Document or wrap. -33. **`add_*_callback` methods are Java-flavored**; no `remove_disconnected_callback` *(API)* — [PR #957](https://github.com/nats-io/nats.py/pull/957) adds the four `remove_*_callback` counterparts (`add_*_callback`/`remove_*_callback` matches stdlib `Future.add_done_callback`/`remove_done_callback`, so the "Java-flavored" criticism was overstated). -34. **Reconnect cannot be aborted by `close()` mid-backoff** *(asyncio)* — close - should also set `_reconnect_wake`. -35. **Subscription dict mutated during reconnect's re-SUB iteration** *(asyncio)* — - user `subscribe`/`unsubscribe` during the window races with the bulk re-SUB - write. -36. **`close()` cancels read/write tasks twice and writes UNSUB after the socket is closed** *(asyncio, smells)*. -37. **`_handle_msg` / `_handle_hmsg` are near-identical** *(smells)* — collapse - to one `_dispatch`. -38. **Inbox uses UUID4 (32 hex) instead of NUID (22 chars)** *(nats.go)* — - interop and forensic-tooling deviation. -39. **`ServerInfo` drops `ws_connect_urls`, `git_commit`, `ip`, `client_ip`, `cluster`, `domain`, `xkey`** *(parser, ADR)*. -40. **IPv6 detection by colon-counting** *(smells)* — `__init__.py:797-806`; use - `ipaddress` or require brackets. - -## Low / polish - -41. `from __future__ import annotations` in 8 files — dead weight at 3.13 *(pythonic)*. -42. `asyncio.get_event_loop()` at `__init__.py:388, 393, 496, 512, 525, 535` - while line 1048 correctly uses `get_running_loop()` *(pythonic, asyncio)*. -43. `asyncio.TimeoutError` mixed with `TimeoutError` builtin *(pythonic)* — [PR #950](https://github.com/nats-io/nats.py/pull/950). -44. `@dataclass` missing `slots=True` on `ServerInfo`; `Headers` is `@dataclass` - *and* defines `__init__`/`__eq__` (decorator is dead) *(pythonic, API)*. -45. `Enum` instead of `StrEnum` for `ClientStatus`; eight states with - undocumented transitions *(pythonic, API)*. -46. Reader/writer broad `except Exception` and `except BaseException` swallows - real bugs and `CancelledError` *(pythonic, asyncio)*. -47. WebSocket buffer is O(n²) `bytes += frame` / slicing *(pythonic)*. -48. `protocol/message.py:310-325` `async def ping/pong` with no awaits; - `if TYPE_CHECKING: pass` dead block; unused `TypeVar T` in - `subscription.py:22`. -49. `connection.py:114` pokes `_writer._transport` private attr (justified, - document it). -50. `force_reconnect()` is Go/Java-named — prefer `reconnect()` *(API)*. -51. `return_on_error` is a double-negative kwarg — prefer `raise_for_status` *(API)*. -52. Magic numbers `1*1024*1024`, `1*512`, `0.005` *(smells)* — [PR #951](https://github.com/nats-io/nats.py/pull/951). -53. Verb match is case-sensitive (`PING` vs `Ping`) — spec is case-insensitive *(parser)*. -54. `__all__` exports `MaxPayloadError`/`NoRespondersError` but not - `SlowConsumerError` *(API)*. -55. `Subscription.messages` exists "for legacy compat" — drop it *(API)*. -56. INFO `cast(ServerInfo, data)` skips required-field validation *(parser)*. -57. `Verbose`/`Pedantic` hard-coded in CONNECT — no user override *(nats.go)* — [PR #949](https://github.com/nats-io/nats.py/pull/949). `protocol` deliberately left as a wire-format internal. -58. Inconsistent `logger.exception` vs `logger.error` for similar failure paths *(smells)* — [PR #952](https://github.com/nats-io/nats.py/pull/952). - -## Themes / leverage points - -- **Decomposing `Client` (#14) unlocks unit testability** and removes the - duplicate CONNECT/TLS branches that drove findings #1, #6, the asyncio races, - and several smells. -- **A typed `-ERR` model + a header-spec-compliant `Headers`/`Status`** (#8, #13, - #15) is one refactor that resolves three lenses simultaneously and aligns - with both ADRs and nats.go. -- **Security #1 + #2 (TLS pinning + `connect_urls` validation) are the only - items where a remote attacker can cause direct harm** — these should ship - before the package leaves "in development." -- ~~**One write lock around `_connection.write()`** (#3) is a ~10-line fix with - very high payoff against a real wire-corruption hazard.~~ Retracted — see #3. diff --git a/nats-jetstream/AUDIT.md b/nats-jetstream/AUDIT.md deleted file mode 100644 index 2d148533..00000000 --- a/nats-jetstream/AUDIT.md +++ /dev/null @@ -1,235 +0,0 @@ -# nats-jetstream audit (2026-05-27) - -Five specialized agents reviewed `nats-jetstream/src/nats/jetstream/` through -different lenses (ADR compliance, nats.go parity, pythonic patterns, code -smells, public API ergonomics). This document is the deduplicated, -severity-ranked synthesis. Items confirmed by multiple lenses are noted in -parentheses. - -Package is ~7.4k LOC across 11 files. Notable concentrations: `api/types.py` -(1774), `stream.py` (1673), `__init__.py` (911), `consumer/pull.py` (862). - -## Critical — correctness / interop bugs - -1. **Hard-coded `$JS.API.DIRECT.GET.{name}` ignores prefix/domain** *(smells, ADR-31)* — [PR #958](https://github.com/nats-io/nats.py/pull/958) - `stream.py:1174, 1179`. Bypasses `self._prefix`. Direct-get fails for users - with a custom prefix or JetStream domain. - -2. **`OrderedConsumer.fetch()` recreates the server-side consumer on every call** *(API, ADR-17)* - `consumer/ordered.py:178-213`. `_prepare_fetch` triggers a full delete + create cycle - between batches. Either make ordered consumers `messages()`-only or make - `fetch()` transparent. The docstring acknowledges the bug. - -3. **`PullMessageBatch`/`PullMessageStream` leak callbacks on the client** *(smells)* - `consumer/pull.py:84-86, 282-283`. Each `fetch()` / `messages()` calls - `add_disconnected_callback` + `add_reconnected_callback` and never removes - them. Cumulative leak per invocation. Cleanup must `remove_*_callback`. - -4. **Pull `messages()` mishandles `Nats-Pending-Messages`/`Nats-Pending-Bytes` on 404** *(parity)* - `consumer/pull.py:335-341`. On 404 No Messages, the spec says the request - still counted against the batch — client decrements pending by the headers. - nats-jetstream zeros out pending unconditionally instead of reading the - headers, causing drift from the server's view on intermittent gaps. - -5. **`_cleanup` in `__anext__` converts errors to `StopAsyncIteration`** *(smells, parity)* - `consumer/pull.py:322`. Callers can't distinguish connection loss from - end-of-batch. `PullMessageBatch` stashes on `self._error`; `PullMessageStream` - doesn't. - -6. ~~**`AckPolicy = Literal["none", "all", "explicit", "flow_control"]`** *(pythonic, ADR)*~~ - **Retracted.** `"flow_control"` is a real server policy - (`AckFlowControl` in `nats-server/server/consumer.go` — "functions like - AckAll, but acks based on responses to flow control"). The server requires - it to be paired with a push consumer (`deliver_subject` set) and - `flow_control=true`, so combined with nats-jetstream being pull-only today - it can't actually be used end-to-end — but the literal value is correct, - not hallucinated. (See related finding on push-only fields exposed on a - pull-only package.) - -## High - -7. **Pinned-client priority groups silently broken** *(ADR-42)* - `consumer/pull.py:330-369`. No `Nats-Pin-Id` capture, no `id` echo in - subsequent pulls, no 423 handling. Config fields (`group`/`priority`/ - `min_pending`/`min_ack_pending`/`priority_policy`) are wired but the - protocol completion is missing; `CONSUMER.UNPIN` admin API absent. - -8. **`publish` lacks first-class JetStream options** *(API, parity, ADR-37)* - `__init__.py:281-359`. No `msg_id`, `expected_stream`, `expected_last_seq`, - `expected_last_subject_seq`, `expected_last_msg_id`, `ttl`. Users hand-craft - `Nats-Msg-Id` / `Nats-Expected-*` / `Nats-TTL` / `Nats-Marker-Reason` - headers. Header-name constants for these missing in `headers.py`. - -9. **No `publish_async` / batched publish** *(parity, ADR-50)* - Only synchronous one-at-a-time publish. nats.go has `PublishAsync` → - `PubAckFuture` with max-pending window. ADR-50 batch headers - (`Nats-Batch-Sequence`, `Nats-Batch-Commit`) and `PublishAck.batch_id`/ - `batch_size` exist (`headers.py:7-23`, `__init__.py:215-228`) but no - orchestration that drives them. - -10. **No `create_or_update_stream`** *(API, parity)* - `__init__.py:436`. Asymmetric with `create_or_update_consumer` - (`__init__.py:618`). Single most common operation in real apps. - -11. **`update_stream` returns `StreamInfo`, not `Stream`** *(API)* - `__init__.py:465-483`. Asymmetric with `create_stream → Stream`. Callers - lose the handle. - -12. **Direct Get is single-message only** *(ADR-31, parity)* - `stream.py:1168-1230`. Missing batch / `max_bytes` / `multi_last` / - `up_to_seq` / `up_to_time` / `next_by_subj` / `start_time`, EOB-204 - handling, `Nats-Num-Pending` / `Nats-Last-Sequence` propagation, 413 - handling. Reduces a 2.11+ feature to its 2.10 capability set. - -13. **Ordered-consumer invariants not enforced** *(ADR-17, parity)* - `consumer/ordered.py:329-360`. Spec requires `ack_policy=none`, - `max_deliver=1`, `mem_storage=true`, `num_replicas=1`, - `flow_control=true`, default `idle_heartbeat≈5s`. Package hard-codes some, - silently overrides others (`inactive_threshold` → 5min if `None`, - `consumer/ordered.py:327`), and ignores user values that ever might be - exposed. Recovery is coarse (any inner-iter exit triggers reset) — no - explicit sequence-gap-vs-heartbeat distinction. - -14. **`Stream` is a god-class with 11 private-attr workarounds** *(smells)* - `stream.py:1085-1673` — ~590 LOC, 19 public methods. 11 sites do - `getattr(self._jetstream, "_api", None)` then `raise RuntimeError("can't - happen")` (`stream.py:1135, 1164, 1317, 1332, 1350, 1403, 1435, 1481, - 1577, 1595, 1635`). `JetStream._api` is always set; the guards exist only - to defeat typing. Make `_api` a real attribute on a typed protocol. - -15. **Pull consumer polls every 100 ms instead of refilling on threshold** *(parity, smells)* - `consumer/pull.py:426-448` (request loop), `:450-473` (heartbeat monitor). - Three places independently track `_heartbeat_deadline`; the monitor - rewrites `_pending_messages`/`_pending_bytes` without coordinating with - in-flight `__anext__` — real race. Replace with `asyncio.Event` / - `wait_for` and own the deadline in one place. - -16. **Sparse typed error catalog** *(parity, API)* - `errors.py:7-41`. Missing common codes: `BAD_REQUEST (10003)`, - `STREAM_WRONG_LAST_SEQUENCE (10071)`, `CONSUMER_NAME_EXISTS (10013)`, - `CONSUMER_ALREADY_EXISTS (10105)`, `DUPLICATE_FILTER_SUBJECTS (10136)`, - `OVERLAPPING_FILTER_SUBJECTS (10138)`, `CONSUMER_EMPTY_FILTER (10139)`. - No `BadRequestError`, `WrongLastSequenceError`, - `ConsumerNameAlreadyExistsError`. `ErrorCode` is a bare class — should be - `IntEnum`. - -17. ~~**`api/types.py` 1774 LOC with massive duplication** *(smells, pythonic)*~~ - **Retracted.** `api/types.py` is generated from JSON schemas by - `nats-jetstream/tools/generate_types.py` (schemas under - `nats-jetstream/schemas/jetstream/api/v1/`). The "3 edits per field change" - cost is paid by the schema source, not by us. The remaining valid concern - is #18 — the hand-rolled `from_response`/`to_request` layer on the - user-facing dataclasses that re-encodes the generated TypedDicts. - -18. **~25 `@dataclass` types with hand-rolled `from_response`/`to_request`** *(smells, pythonic)* - `ConsumerConfig.from_response` ~100 lines; `StreamConfig` `from_response` + - `to_request` ~220 lines. Per project memory, msgspec was meant to back - these. Adding a field is 6 edits and silent on omission. - -19. **Pull batch 408/409 errors silently swallowed** *(parity, smells)* - `consumer/pull.py:151-152, 175-186`. 408 raises `StopAsyncIteration` - without setting `_error`; "exceeded maxrequestbatch/expires/maxbytes/ - maxwaiting" matched by `description.lower()` substring (server text isn't - API-stable) and converted to bare `Exception`. Use error codes; surface as - typed errors. - -20. **`publish` retry loop control-flow bug** *(API, smells)* - `__init__.py:323-359`. The `for` loop returns inside the `try`; a future - edit adding a new exception type could fall through and return `None`. - Add an explicit `raise` after the loop. - -21. **JetStream not `async with`-able, no `close()`** *(API)* - `__init__.py:248-264`. No `__aenter__`/`__aexit__`, no graceful shutdown. - -22. **`JetStream.get_message` / `get_last_message_for_subject` claim to require `allow_direct=true` but use the API path** *(smells)* - `__init__.py:763-849`. Docstring is wrong and the implementation duplicates - `Stream._get_message`'s non-direct branch (~90 lines). Plus there's a - third copy of the base64+headers decode in the same package. - -## Medium - -23. **`Consumer.reset` on the protocol; `OrderedConsumer.reset` raises `NotImplementedError`** *(API)* — LSP violation. Split into `ResettableConsumer`, or move off the protocol. -24. **`Consumer` protocol mismatch with `PullConsumer.next`** *(API)* — protocol declares `(max_wait)`; impl adds `heartbeat`, `min_ack_pending`, `min_pending`, `priority_group`, `priority`. Protocol is a lie. `consumer/__init__.py:553` vs `consumer/pull.py:566`. -25. **Per-message TTL: config wired, headers/publish path missing** *(ADR-37)* — `StreamConfig.allow_msg_ttl` exists; no `Nats-TTL` constant or `publish(ttl=...)` kwarg. -26. **`Stream.pause_consumer(pause_until: float)` takes Unix timestamp** *(API)* — should be `datetime` to match `ConsumerConfig.pause_until: datetime`. `stream.py:1563`. -27. **`PullMessageStream` rejects `max_messages + max_bytes`** *(parity)* — Go allows both; `max_bytes` is a soft cap within the batch. `consumer/pull.py:644-646`. -28. **No `StopAfter` option on `messages()`** *(parity)*. -29. **No `ConsumeErrHandler` callback** *(parity)* — non-terminal errors only logged, no user hook. `consumer/pull.py:119, 458`. -30. **Fixed 5-second timeout on every API call** *(parity)* — `api/client.py:424`. No per-call override; `stream_create` on large mirror sources / `stream_purge` on huge streams will time out. -31. **`api/client.py:155-395` repeats the same try/`error_code`/raise-subclass pattern ~10 times** *(smells)* — a `{ErrorCode.X: ErrorXError, ...}` map + `_remap(e, allowed)` helper eliminates it. -32. **`pause_consumer` returns `None`; nats.go returns `ConsumerPauseResponse`** *(parity)* — loses the actual pause time. -33. **`Message.ack_sync` missing; called `double_ack` instead** *(parity, smells)* — uses `subscription.next` instead of `request`, opening a sub per call (extra round-trip). And five near-identical ack methods (`ack`/`nak`/`nak_with_delay`/`in_progress`/`term`/`term_with_reason`) repeat the same 4-line `_reply`/`_jetstream` validation; extract `_send_ack(payload)`. -34. **`Message.metadata` always populated, with junk defaults** *(API)* — server-pushed messages without a JS reply silently get `stream=""`, `sequence=(0,0)`. Should be `None` or raise. -35. **`ConsumerConfig` exposes push-only fields on a pull-only package** *(API)* — `deliver_subject`, `deliver_group`, `flow_control`, `idle_heartbeat`, `direct` ("internal use"); `stream.py:1376` rejects push but no validation upfront. -36. **Auto-generated consumer name is `consumer-{base64(str(datetime.now(utc)))}`** *(smells)* — magic, not collision-proof, bizarre. Use `uuid.uuid4().hex` or NUID. `stream.py:1338-1380`. -37. **`StreamConfig.name: str | None = None`; `create_stream` validates at runtime** *(API)* — make `name` required positionally, or take it as the primary arg. -38. **Magic `$JS.API` literal in `stream.py:1174, 1179`** *(smells)* — hard-coded direct-get subject ignores `self._prefix` (see #1). -39. **`StreamConfig` missing `metadata` field on the public dataclass** *(ADR-33)* — present in `api/types.py` but not exposed on the user-facing model. Affects ADR-44 versioning too (`NATS_REQUIRED_API_LEVEL` constant exists; nothing sets/reads it). -40. **`MessageBatch.error` is `Exception`, not typed** *(API, parity)* — 409 sub-errors matched by string substring (#19). -41. **Cleanup / `_delete_consumer` silently swallows all exceptions** *(smells, ordered)* — `consumer/ordered.py:407-412`. No `logger.debug`. Combined with fire-and-forget `_reset()` task creation, recreation failures are invisible. -42. **`getattr(self._jetstream, "_api", None)` 11 times** *(smells)* — see #14. -43. **`StreamNameBySubject`, `UnpinConsumer`, push consumer surface, `CleanupPublisher`, `Stream.cached_info()` missing** *(parity)*. -44. **Pull/`messages()` `max_messages` semantics differs from `fetch()`** *(API)* — same parameter name, different meaning (per-batch hint vs total bound). `consumer/pull.py:566-602` vs `:610-670`. -45. **`Stream.get_message` vs `Stream.direct_get_message`** *(API)* — `Stream._get_message` does fallback transparently; `JetStream.get_message` doesn't. Two routes diverge. -46. **`stream_names`/`list_streams`/`consumer_names`/`list_consumers` pagination duplicated 4 times** *(smells)* — extract `_paginate`. -47. **All dataclasses lack `frozen=True`/`slots=True`/`kw_only=True`** *(pythonic)* — conceptually immutable returned-value records (`PublishAck`, `APIStats`, `Tier`, `AccountInfo`, `Metadata`, `StreamMessage`, `ConsumerReset`, etc.). Either dataclass with all three, or msgspec. -48. **`time.time()` used everywhere for deadlines** *(pythonic)* — `consumer/pull.py:76, 78, 91, 97, 118, 130, 142, 277, 288, 294, 328, 457, 468`. Must be `time.monotonic()` (wall-clock-jump immune). -49. **`asyncio.get_event_loop().time()` for publish deadlines** *(pythonic)* — `__init__.py:319, 326, 353`. Deprecated when no loop; use `get_running_loop()` or `time.monotonic()`. -50. **Manual polling in `_request_loop` / `_heartbeat_monitor`** *(pythonic)* — see #15. -51. **`datetime.fromisoformat(s.replace("Z", "+00:00"))` repeated 11 times** *(pythonic)* — Python 3.11+ handles `Z` natively. Drop the `.replace`. -52. **`timedelta(microseconds=ns / 1000)` repeated 9 times** *(pythonic, bug-shape)* — float divide introduces rounding; wrap as `_ns_to_timedelta(ns)` using `ns // 1000`. -53. **Broad `except Exception:` swallowing in header parse** *(pythonic)* — `__init__.py:797, 839`, `stream.py:1264`. Silently sets `headers=None`. At minimum log. -54. **`PullConsumer.get_info` is `async def` but never awaits** *(pythonic, API)* — `consumer/pull.py:521-523`. Docstring claims "refresh from server"; body returns cached `self._info`. Either fix the body or remove `async`. -55. **`OrderedConsumer.create` async factory** *(pythonic)* — instances created via `__init__` directly are half-initialized; every property guards on it. Make `__init__` private or initialize lazily. - -## Low / polish - -56. **`from __future__ import annotations` in all 9 files** *(pythonic)* — dead weight at 3.11+. -57. **`Union[...]` / `Optional[...]` / `Tuple[...]` from typing in `api/types.py`** *(pythonic)* — use `|`. -58. **`AsyncIterator` imported from `typing` in 5 files** *(pythonic)* — should be `collections.abc.AsyncIterator`. Also, `async def` generators return `AsyncGenerator[T, None]`, not `AsyncIterator[T]`. Affects `__init__.py:361, 392, 670, 703`, `stream.py:1021, 1029`. -59. **`StreamMessage.__getitem__`** *(API)* — `stream.py:1078-1082`. `dict`-style attribute access alongside attribute access. Footgun, also lets you read private attrs. Drop. -60. **`ErrorCode` as bare class** *(pythonic)* — should be `IntEnum` for `.name` / `.value` / iteration. -61. **`CONSUMER_ACTION_*` string literals** *(pythonic)* — should be `StrEnum`. Same for `Literal[...]` aliases (`AckPolicy`, `DeliverPolicy`, etc.) where runtime identity is useful. -62. **`MessageBatch.error: Exception | None`** — see #40. -63. **`__all__` doesn't re-export `Headers`, `Metadata`, `SequencePair`, `Message`, `MessageBatch`, `MessageStream`, `PullConsumer`, `OrderedConsumer`, or `headers.NATS_*`** *(API)* — users dig into submodules. -64. **`PublishAck.value` opaque field name** *(API)* — rename `counter_value`. -65. **`Stream.purge(filter=...)` shadows the Python builtin** *(API)* — `stream.py:1123`. Rename `subject` or `subject_filter`. -66. **Local imports inside `publish()`** *(pythonic)* — `__init__.py:314 import asyncio`, `:316 from nats.client.errors import NoRespondersError`. Lift to module level. Also `stream.py:1257`, `stream.py:1574` (which is dead code — already imported at module top). -67. **`JetStream.__init__` positional args** *(API)* — `(client, prefix, domain, strict)`. Make all but `client` keyword-only. -68. **Inconsistent return types: `delete_stream → bool`, `delete_consumer → bool`, `delete_message → None`, `pause_consumer → None`** *(API)* — pick `→ None` (raise on failure) everywhere. -69. **TODO `alternates` field never added to `StreamInfo`** *(smells)* — `stream.py:997`. Real lost data. -70. **`StreamManager` Protocol declares `create_stream(**config)`; impl takes positional `StreamConfig`** *(API)* — `stream.py:1017-1066`. Protocol is wrong. -71. **`consumer/__init__.py:125 priority_timeout: Any | None`** *(pythonic)* — `Any` while every other duration is `timedelta`. -72. **`timezone.utc` → `datetime.UTC` (3.11+)** *(pythonic)* — 6+ sites. -73. **Bare `pass` in retry/error paths with no logging** *(pythonic)* — `consumer/pull.py:444-448, 471-473`, `consumer/ordered.py:411`. -74. **`OrderedConsumer.__aexit__(self, *exc_info)` signature drift** *(pythonic)* — `consumer/ordered.py:144`. Use typed `(exc_type, exc_val, exc_tb)` like `pull.py:557`. -75. **Magic numbers / strings** *(smells)* — heartbeat 2x multiplier (`pull.py:78, 142, 277, 328, 468`), default batch 100, byte-mode batch 1_000_000 (`pull.py:653, 733`), backoff 1.0/10.0/2.0 (`ordered.py`), `$JS.ACK` parsing (`message.py:71-117`), `Nats-Pending-Messages/Bytes` (`pull.py:346, 348`). -76. **`Stream.__init__` accepts `info: StreamInfo | None`; `_info` nullable everywhere** *(API)* — force-fetch on init or split into `Stream` (always has info) vs `StreamRef`. -77. **`api/client.py:73-89 _error_from_response` is private but called from `__init__.py:342`** *(smells)* — public-by-use, private-by-name. Same for `is_error_response`. -78. **`SubjectTransform` typed as `Any` on `StreamSource`/`StreamSourceInfo`** *(ADR-36)* — round-trips OK, loses type safety. `stream.py:181, 257`. -79. **Inconsistent prefix on `nats.jetstream.api` logger** *(smells)* — hard-coded string vs `__name__` elsewhere. -80. **`set()` empties / lambda-style boilerplate in `check_response`** *(API)* — returns 3-tuple `(bool, set, set)` callers re-check field by field. - -## Themes / leverage points - -- **Dataclass marshaling layer (#18)** is the single highest-leverage - cleanup. The generated `api/types.py` TypedDicts already describe the wire - shape; the user-facing `@dataclass` mirrors plus their hand-rolled - `from_response` / `to_request` (~400 lines across `consumer/__init__.py` - and `stream.py`) re-encode the same fields by hand. Migrate the user-facing - layer to `msgspec.Struct` (or extend the generator to emit it), and the - silent-drop bugs and edit-three-places cost go away. -- **Pull consumer rewrite (#15, #4, #19, #5, #44)** is the biggest correctness - cluster: polling → event-driven, fix 404/408 header semantics, surface - errors properly instead of swallowing into `StopAsyncIteration`. -- **`Stream` god-class (#14, #22, #42)** — extracting `_api` to a typed - protocol kills 11 RuntimeError guards. Split into `StreamManagement`, - `StreamMessages`, `StreamConsumers` collaborators. -- **`OrderedConsumer` (#2, #13, #23, #41)** is largely broken or fragile: - `fetch()` recreates per-call, `reset()` raises, invariants not enforced, - exceptions silently swallowed. Worth a focused rewrite. -- **Publish ergonomics (#8, #9, #25, #20)** is the biggest user-facing gap — - `msg_id`, `expected_*`, `ttl`, `publish_async`, batch publish. None of it - exists despite the headers/types being half-wired. -- **Hard-coded `$JS.API` literals (#1, #38)** are real bugs for any - domain/custom-prefix user. Single grep, ~6 sites. diff --git a/nats-schemas b/nats-schemas deleted file mode 160000 index 4c3313f1..00000000 --- a/nats-schemas +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 4c3313f10b45423b7950378b68073dcd910a822f From 6e8a88078632499dec225f92b2f7e47475fbd85c Mon Sep 17 00:00:00 2001 From: Casper Beyer Date: Mon, 13 Jul 2026 06:13:47 +0200 Subject: [PATCH 5/5] Run batch cleanup in a finally block A dedicated except clause for CancelledError still left gaps: any other unexpected exception escaping the iteration skipped both the unsubscribe and the heartbeat callback deregistration, leaking the callbacks on the client. Move the cleanup into a finally guarded by a delivering flag so every exit except a successful message delivery tears down the batch, and deregister before the awaited unsubscribe since that await can itself be interrupted by cancellation. Apply the same ordering to the exhausted-batch path at the top of __anext__. --- .../src/nats/jetstream/consumer/pull.py | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/nats-jetstream/src/nats/jetstream/consumer/pull.py b/nats-jetstream/src/nats/jetstream/consumer/pull.py index bab16609..5a70fa50 100644 --- a/nats-jetstream/src/nats/jetstream/consumer/pull.py +++ b/nats-jetstream/src/nats/jetstream/consumer/pull.py @@ -117,11 +117,12 @@ def __aiter__(self) -> AsyncIterator[Message]: async def __anext__(self) -> Message: if self._terminated or self._pending_messages <= 0: if not self._terminated: - await self._subscription.unsubscribe() - self._deregister_callbacks() self._terminated = True + self._deregister_callbacks() + await self._subscription.unsubscribe() raise StopAsyncIteration + delivering = False try: while True: # Check heartbeat timeout (ADR-37: warn at 2x idle_heartbeat) @@ -213,22 +214,21 @@ async def __anext__(self) -> Message: ) self._pending_messages -= 1 + delivering = True return js_msg except (StopAsyncIteration, asyncio.TimeoutError): - if not self._terminated: - await self._subscription.unsubscribe() - self._deregister_callbacks() - self._terminated = True raise StopAsyncIteration - except asyncio.CancelledError: - # Cancellation must release the heartbeat callbacks too, or they - # leak on the client. Deregister before the await: unsubscribing - # can itself be interrupted by a second cancellation. - if not self._terminated: + finally: + # Any exit other than delivering a message terminates the batch: + # exhaustion, timeout, cancellation, or an unexpected error. All + # of them must release the subscription and the heartbeat + # callbacks, or the callbacks leak on the client. Deregister + # before the await: unsubscribing can itself be interrupted by a + # (second) cancellation. + if not delivering and not self._terminated: self._terminated = True self._deregister_callbacks() await self._subscription.unsubscribe() - raise class PullMessageStream(MessageStream):