fix(nats): clean up failed JetStream request subscriptions

[Kuang-xianxin]

Description

JetStream requests leave reply inbox subscriptions behind when they time out, fail to publish, or are cancelled before a response arrives. unsubscribe(limit=1) only removes interest after a message is received, so failed requests accumulate subscriptions for the lifetime of the connection.

Clean up the reply future and subscription in finally, including failures while setting the reply limit. Shield unsubscribe from AnyIO cancellation so the final UNSUB can be sent. Preserve the original request error if the connection has already closed or started draining.

Type of change

• Bug fix

Validation

Python 3.13.14, nats-py 2.14.0, and a real local NATS 2.14.6 server with JetStream:

uv run --no-sync pytest tests/brokers/nats/test_request_cleanup.py tests/brokers/nats/test_requests.py -q -o addopts='' --timeout=15
Original source: 3 failed, 19 passed
Fixed source:    22 passed

Following the repository's testing-patterns skill, the cleanup tests are reduced from nine to four distinct boundaries:

• A real request timeout removes the unused inbox subscription.
• Failure while setting the reply limit still cleans up the subscription.
• Cancellation during publish still sends the final unsubscribe command.
• Connection closure does not replace the original publish error.

The cancellation case also fails when shielding is disabled. The error-preservation case fails when suppression of connection-closed/draining errors is removed. Existing connected and in-memory request tests cover successful broker/publisher responses; duplicate success and failure cases were removed. The connected case uses the existing settings and unique queue fixtures.

Ruff lint/format, codespell, strict mypy for the changed test module, and the configured mypy check over 516 source files pass. Applicable pre-commit file checks, typos, and detect-secrets pass; the just wrapper hooks were skipped, with lint and type checks executed directly. The production code is unchanged by this test-review follow-up.

These checks run natively on Windows. The full multi-broker Docker matrix was not run.

Checklist

• Focused implementation and regression tests reviewed
• Tests demonstrate the original failure and pass with the fix
• Existing real and in-memory NATS request tests pass
• Formatting, type checks, and file checks pass as described above

[CLAassistant]

All committers have signed the CLA.

[IvanKirpichnikov]

Add type hints

[IvanKirpichnikov]

I don’t like your parameterized tests because of the large number of branches.

Let’s write a separate test for each parameter.I don’t like your parameterized tests because of the large number of branches.

Let’s write a separate test for each parameter.

[Kuang-xianxin]

Updated in c7a7fd4: timeout, publish failure, reply-limit failure, and cancellation now have separate tests with fixed expectations. Shared real-server setup is in a typed fixture; test arguments, fixture yields, and callbacks have type hints. The connected tests still repeat each failure three times to check for accumulating subscriptions.

Validation: 27 cleanup/request tests passed with real NATS 2.14.6, and the changed test module passes strict mypy plus Ruff lint/format. The production fix is unchanged.

[Kuang-xianxin]

Reworked in a99dd63 following the repository testing-patterns skill: reduced nine cleanup cases to four distinct boundaries, removed duplicate success/failure coverage and the single-use fixture, used the existing settings/queue fixtures, and consolidated the subscription assertions. Existing request tests cover successful replies.

The named cleanup/request files pass all 22 tests with real NATS 2.14.6; reverting the production fix gives 3 failures. Disabling shielding and removing connection-error suppression each fail their specific retained test. Ruff/format, codespell, strict test typing, and configured mypy over 516 source files pass. Production code is unchanged.

[IvanKirpichnikov]

rewrite tests with using local skills for tests

[Kuang-xianxin]

Hi @IvanKirpichnikov, following up on the test rewrite in a99dd63. It follows the repository testing-patterns skill, uses four distinct cleanup cases and existing fixtures, and removes the duplicate coverage and single-use fixture. The September 9 validation recorded 22 passing cleanup/request tests with real NATS.

The head is unchanged, and Run linters, Run all tests, and CodeQL still await workflow approval. Could you revisit the test layout and approve the pending runs when convenient? Thanks!

[Kuang-xianxin]

The workflow-approval blocker from my previous comment is now cleared: Run all tests, Run linters and CodeQL all succeeded on unchanged head a99dd63 (28 successful checks/statuses, 2 skipped).

Could you re-review the four-case cleanup test revision? It follows the requested local testing skill, uses the existing fixtures, and leaves successful-request coverage to the existing tests.

[IvanKirpichnikov]

Hi, I apologize for the long silence. In principle, I’m ready to merge your changes.

But I have a feeling that you used neural networks. If you did, were the skills applied within the repository? If yes, please let me know; if not, I ask you to apply them to your changes.