Skip to content

Honor explicit inactive_threshold of 0 in KV watch - #991

Draft
caspervonb wants to merge 2 commits into
mainfrom
fix-580
Draft

Honor explicit inactive_threshold of 0 in KV watch#991
caspervonb wants to merge 2 commits into
mainfrom
fix-580

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

watch() used a truthiness check on inactive_threshold, so an explicit 0 (disable the inactivity cleanup) was indistinguishable from None and got overridden with the 5 minute default. Check against None.

Fixes #580.

watch() treated inactive_threshold with a truthiness check, so an
explicit 0 (disable the inactivity cleanup) was indistinguishable from
None and got overridden with the 5 minute default. Check against None so
0 is passed through to the consumer instead of being clobbered.
@caspervonb caspervonb added the nats-legacy Legacy asyncio NATS client label Jul 14, 2026
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 14, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand the request
  • Review PR changes (gh pr diff 991)
  • Examine relevant source files for context
  • Post review findings

Summary

The fix in kv.py is correct and minimal. The root cause was a truthiness check (if not inactive_threshold:) treating 0 and None identically; replacing it with if inactive_threshold is None: resolves that cleanly.

Findings

1. Related unfixed bug in client.py:432 (not in diff, but surfaced by this change)

# nats/src/nats/js/client.py:432
if inactive_threshold:
    config.inactive_threshold = inactive_threshold

This is the same truthiness-check pattern. If a caller uses js.subscribe(inactive_threshold=0) directly, this silently drops the value. For the specific kv.watch(inactive_threshold=0) path fixed here it doesn't cause a regression — because _to_nanoseconds(None) and _to_nanoseconds(0) both produce 0 on the wire — but it's an inconsistency. Should be:

if inactive_threshold is not None:
    config.inactive_threshold = inactive_threshold

2. Weak test assertion

# test_js.py
assert info.config.inactive_threshold < 5 * 60

This verifies the old bug (forced 300s) is gone, but not what value the server actually applied. Given the comment says "An explicit 0 disables the inactivity cleanup so the watch runs forever", asserting < 5 * 60 is the right check for a regression guard, but the comment in code is slightly misleading — passing 0 means "let the server decide its minimum", not literally forever. Minor documentation nit.

3. No correctness or security issues. The change is safe and the test covers the regression case.

Mirror the KV watch fix in the ordered/watch consumer config path: the
truthiness check silently dropped an explicit 0, so a caller asking to
forward 0 was ignored. Assert against the config the client sends rather
than the server round-trip, since the server rewrites 0 to its own
minimum for ordered consumers. Also reword the KV comment to describe the
forwarding accurately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nats-legacy Legacy asyncio NATS client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Impossible to watch forever

1 participant