[FIXED] Nil account panic in processLeafNodeConnect#7991
[FIXED] Nil account panic in processLeafNodeConnect#7991umut-polat wants to merge 1 commit intonats-io:mainfrom
Conversation
MauriceVanVeen
left a comment
MauriceVanVeen
left a comment
There was a problem hiding this comment.
LGTM, but would it be possible to add a test for this?
umut-polat
force-pushed
the
fix/leafnode-nil-account
branch
from
590acb6 to
59d6d38
Compare
|
added a test that races a CONNECT with a very short auth timeout (1ms) to exercise the nil account guard. the exact race from #7989 is hard to reproduce deterministically since it depends on goroutine scheduling, but the test covers the defensive path. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59d6d381de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // timeout while still being processed), bail out to avoid a panic. | ||
| if acc == nil { | ||
| c.closeConnection(MissingAccount) | ||
| return ErrMissingAccount |
There was a problem hiding this comment.
In processLeafNodeConnect, the new nil-account branch closes the connection and then returns ErrMissingAccount. The parser read loop treats non-whitelisted errors as protocol errors (client.go read loop around parse()), so this path logs an error and attempts a ProtocolViolation close even when this is just the auth-timeout race you are handling. That creates noisy/misclassified errors compared with adjacent leaf handlers that close and return nil for equivalent pre-CONNECT races.
Useful? React with 👍 / 👎.
|
Looks OK but could you please fix sign-offs? https://github.com/nats-io/nats-server/blob/main/CONTRIBUTING.md#sign-off |
When a leafnode connection is closed during authentication (e.g. auth timeout), the account may not have been set on the client yet. If processLeafNodeConnect continues to run after the connection was closed, it dereferences c.acc in registerLeafNodeCluster, causing a nil pointer panic. Add a nil check for the account before proceeding, consistent with other code paths in the same file (initLeafNodeSmapAndSendSubs, checkJetStreamExportReconcile) that already guard against this. Add a test that races a CONNECT with an expired auth timeout to exercise the guard. Resolves nats-io#7989 Signed-off-by: umut-polat <[email protected]>
umut-polat
force-pushed
the
fix/leafnode-nil-account
branch
from
59d6d38 to
8353a69
Compare
|
done — added sign-off. |
3 participants
What
Fixes a nil pointer panic in
processLeafNodeConnectwhenc.accis nil.Stack trace from #7989
The logs show
Authentication Timeoutfollowed byconnection closedright before the panic. The account was never set on the client because the connection was closed during auth.Fix
Add a nil guard for
accafter readingc.acc, consistent with the same pattern used in other code paths within the same file (lines 2259, 2758, 2890).Resolves #7989