jetstream: surface ErrMaxBytesExceeded to ConsumeErrHandler

[Deln0r]

Closes #1718.

Problem

When a single message in the stream is larger than the configured PullMaxBytes, the server responds with 409 Message Size Exceeds MaxBytes on every MSG.NEXT. handleStatusMsg already detects the status but returns (nil, nil), so neither the terminal error path nor the user's ConsumeErrHandler ever sees the condition. The consumer keeps retrying silently and burns CPU on both client and server until either the offending message is removed or PullMaxBytes is raised.

Maintainer discussion in the issue thread confirmed this is a client-side concern.

Change

Return ErrMaxBytesExceeded as the non-terminal notification error from handleStatusMsg. The error then flows through the existing Consume notification path and is delivered to the user's ConsumeErrHandler. ErrTimeout and ErrBatchCompleted continue to fall through silently — those are internal pull-flow signals, not user-facing errors.

The retry behaviour itself is intentionally unchanged. Whether the loop should stop, back off, or auto-raise PullMaxBytes is a separate design question (see #1718 discussion); this change just makes the situation observable.

Test plan

A new subtest TestPullConsumerConsume/max_bytes_exceeded_surfaces_to_ErrHandler in jetstream/test/pull_test.go publishes a single 1 KiB message, starts Consume with PullMaxBytes(100), and asserts that the registered ConsumeErrHandler receives an ErrMaxBytesExceeded within 3 s.

$ go test -run "TestPullConsumerConsume/max_bytes_exceeded_surfaces_to_ErrHandler" -v ./jetstream/test/
=== RUN   TestPullConsumerConsume
=== RUN   TestPullConsumerConsume/max_bytes_exceeded_surfaces_to_ErrHandler
--- PASS: TestPullConsumerConsume (0.04s)
    --- PASS: TestPullConsumerConsume/max_bytes_exceeded_surfaces_to_ErrHandler (0.04s)
PASS

[synadia-claude-reviewer]

Claude finished @Deln0r's task in 3m 7s —— View job

---

PR Review: jetstream: surface ErrMaxBytesExceeded to ConsumeErrHandler

Tasks:

• Gather context and understand request
• Examine PR diff
• Read relevant source files
• Analyze changes for correctness, race conditions, error handling, API compatibility
• Post inline comments and review summary

---

Summary

The fix is correct and minimal. The core change in handleStatusMsg is logically sound and threads through the existing error-notification path cleanly.

Analysis

handleStatusMsg change (pull.go:757-759)

The return signature is (terminalErr, notifyErr error). Previously, ErrMaxBytesExceeded fell through to return nil, nil, meaning neither error was ever delivered to the caller. Now it returns (nil, ErrMaxBytesExceeded) — which is exactly what the Consume path at lines 261-271 needs:

termErr, notifyErr := sub.handleStatusMsg(msg, msgErr)
if termErr == nil {
    sub.checkPending()   // ✅ still fires — consumer keeps retrying
    ...
}
sub.Unlock()
if sub.consumeOpts.ErrHandler != nil && notifyErr != nil {
    sub.consumeOpts.ErrHandler(sub, notifyErr)   // ✅ now reached
}

termErr == nil, so checkPending() still fires and the consumer continues retrying — intentional per the PR description. No goroutine leak, no lock held across the ErrHandler call, no race introduced.

ErrHandler call rate

Because checkPending() fires immediately after each ErrMaxBytesExceeded, the consumer issues a new pull request right away. The server rejects it immediately (no Expires wait for a 409), so the ErrHandler will be called in a tight loop at the rate of a round-trip to the NATS server. This is pre-existing behavior (the consumer was already spinning silently), but now user error handler code also runs in that loop. The test's default branch in the ErrHandler channel send signals the author is aware of this. Worth calling out in any user-facing documentation or follow-up (see also #1718 discussion).

Messages()/Next() path is unaffected

At pull.go:665, notifyErr is discarded:

if termErr, _ := s.handleStatusMsg(msg, msgErr); termErr != nil { ... }

This is fine in practice because ConsumeErrHandler does not implement configureMessages, so Messages() consumers can never register an ErrHandler. There's no handler to call and no gap introduced by this PR.

Fetch() path is unaffected — ErrMaxBytesExceeded is already explicitly excluded from the fetch error result at line 955.

Test (pull_test.go)

Well-structured. Buffered error channel with a default branch to safely discard repeated errors from the tight loop. Uses errors.Is for assertion. defer l.Stop() cleans up. The 3-second assertion timeout is generous for what amounts to a sub-millisecond round-trip.

No blocking issues

No correctness bugs, race conditions, API compatibility breaks, or security concerns introduced. The change does what it says.

[coveralls]

coverage: 85.415% (+0.04%) from 85.372% — Deln0r:fix/issue-1718-maxbytes-errhandler into nats-io:main