Don't negotiate leafnode compression over WebSockets

[neilalexander]

WebSockets already have compression, so this would be wasteful and likely counter-productive to do so twice.

Signed-off-by: Neil Twigg neil@nats.io

[synadia-claude-reviewer]

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

---

PR Review

• Gathering context
• Reading changed files
• Reviewing code

---

Summary

The change is correct and well-motivated. WebSocket already negotiates compression at the framing layer (permessage-deflate), so layering S2 compression on top would waste CPU without benefit. The fix is minimal and focused.

Control flow (server/leafnode.go):

• negotiateLeafCompression is called only when needsCompression(co.Mode) is true (i.e. the server is configured with a real compression mode like s2_fast). The early return for WebSocket connections correctly short-circuits before any compression handshake and sets c.leaf.compression = CompressionOff.
• The caller re-acquires c.mu after the function returns false, nil, so the lock state is consistent with the non-WebSocket path.

One issue to address (see inline comment):

c.isWebsocket() is called without holding c.mu, violating its documented locking contract ("Lock held on entry"). Functionally safe since c.ws is set once at creation and never mutated, but worth fixing for consistency. The simplest fix is to inline c.ws != nil directly (no lock requirement) rather than calling isWebsocket().

Test (server/leafnode_test.go): Covers the case where s1 has compression: s2_fast and s2 connects via ws://. Correctly asserts CompressionOff on both sides. Locking in getLeafCompMode is correct (RLock on server, then Lock on the leaf client).

No other issues found.

[synadia-claude-reviewer]

isWebsocket() is documented as "Lock held on entry" but is called here without c.mu (the lock is released before negotiateLeafCompression is invoked). In practice this is safe since c.ws is set once at connection creation and never mutated, but it violates the stated locking contract. Consider either acquiring the lock first, or simply checking c.ws != nil directly (inlining the check without the locking requirement).

[MauriceVanVeen]

LGTM

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3dd37dc7f8

ℹ️ 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".

[chatgpt-codex-connector]

Preserve leaf compression when WS per-message deflate is off

This unconditional c.isWebsocket() fast-path disables leafnode compression for all WebSocket leaf links, but WS compression is only negotiated when the remote explicitly asks for ws_compression (server/leafnode.go:3277-3328) and the accept side has websocket.compression enabled (server/websocket.go:840-843; server/opts.go:590-593). Leafnode compression still defaults to s2_auto for both listeners and remotes (server/opts.go:5915-5937), so a previously supported setup like url: "ws://..." with compression: s2_fast but no WS compression now becomes completely uncompressed. That is a real regression for existing WS leaf deployments that relied on leaf compression without per-message deflate.

Useful? React with 👍 / 👎.

[kozlovic]

You should lock before c.isWebsocket() although I do believe that it is immutable. But, as also pointed out by codex, it is not necessary that web socket connection uses their own compression (it can be disabled or negotiated as off). So I wonder, should it be instead more something like:

c.mu.Lock()
if c.isWebsocket() && c.ws.compress {
   c.leaf.compression = CompressionOff
   c.mu.Unlock()
   return false, nil
}
c.mu.Unlock()

[kozlovic]

If you accept the changes in negotiateLeafCompression(), then you would likely need a multiple-case test where you try with compression: true here and in the remote ws_compression: true and then verify that there is no s2 compression in that case. However, if either one is not set (or set to false), then s2 compression should be used.