Entire

I’ll pin PR #4’s current base/head SHAs, inspect the diff and surrounding contracts, then run focused validation. I’ll keep code findings separate from any still-pending GitHub checks.

The local checkout is clean and exactly matches PR head 0e17ee7; GitHub reports base f28a4ca and no CI checks. I’m splitting review by package while I inspect cross-package behavior, dependency posture, and run the repository validation.

The standard build, lint, and race suite are green. I’m now checking two lifecycle boundaries the happy-path tests do not exercise: concurrent/manual stop versus context cancellation, and whether long-handler heartbeats protect messages that Consume has prefetched but not yet dispatched.

The lifecycle review has surfaced a real concurrency defect in Runner.Stop, plus a shutdown error-classification mismatch. I’m reproducing the separate prefetch/heartbeat risk before deciding whether it is a finding or only an API caveat.

The focused diagnostic is hitting an environment-only bind/start issue outside the repo, so I’m not treating that unconfirmed prefetch concern as a review finding. The repository’s own uncached race suite remains green.

The PR head moved during review (from 0e17ee7 to 2d1955e), so the earlier snapshot is stale. I’m re-pinning the new diff and retesting the findings against the updated head before giving a verdict.

The updated head fixes the MaxDeliver zero-value mismatch, but the remaining lifecycle findings persist. I’m doing one final head/check/worktree verification so the review is anchored to the exact commit I report.

Verdict: request changes on head 2d1955e (base f28a4ca). Three merge-blocking lifecycle issues remain.

Findings

P1 — Prefetch defeats KeepInProgress. jsconsumer.go:157 calls Consume with its 500-message default buffer, while callbacks run serially. Only the active message gets heartbeats at jsconsumer.go:212; messages waiting behind a long handler can exhaust AckWait and redeliver concurrently. Use PullMaxMessages(1) for heartbeat-enabled consumers or expose a bounded prefetch.
P1 — Runner.Stop is neither concurrency-safe nor a completion boundary. jsconsumer.go:123 mutates the interface field r.cc without synchronization, while context cancellation independently calls it at jsconsumer.go:167. Explicit stop plus cancellation causes a data race. It also returns before cc.Closed(), allowing active handlers to race connection/database teardown.
P1 — A transient heartbeat failure permanently disables heartbeats. keepinprogress.go:52 exits on every InProgress error. Errors such as nats.ErrReconnectBufExceeded can recover after reconnection, but the helper never retries or reports the failure, allowing unexpected redelivery during a still-running handler.
P2 — Clean shutdowns can still log warnings. jsconsumer.go:158 uses nuts.IsShutdownFetchErr, but ConsumeErrHandler emits the distinct jetstream.ErrConnectionClosed; the classifier only recognizes core nats errors.
P2 — Empty Durable silently creates an ephemeral consumer. jsconsumer.go:146 does not validate the promised durable name. NATS accepts this and generates a fresh consumer, losing stable resume/update behavior.
P3 — Multiple W3C baggage headers are truncated. tracecontext.go:28 lacks OTel’s ValuesGetter, so only the first baggage header value is extracted.
P3 — Plain Nak() is unobservable in the advertised test double. fakemsg.go:55 records nothing, so tests cannot distinguish a NAK from no disposition.

The newly added EffectiveMaxDeliver correctly fixes the earlier zero-value mismatch.

Validation passed: uncached go test -race -count=1 ./..., lint, build, go mod tidy -diff, and git diff --check. GitHub’s only reported check, Cursor Bugbot, passes; the worktree is clean and exactly matches 2d1955e.