Skip to content

Don't negotiate leafnode compression over WebSockets #7969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
neilalexander wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Don't negotiate leafnode compression over WebSockets #7969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions server/leafnode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1646,6 +1646,14 @@ func (c *client) processLeafnodeInfo(info *Info) {
}

func (s *Server) negotiateLeafCompression(c *client, didSolicit bool, infoCompression string, co *CompressionOpts) (bool, error) {
// WebSockets already have compression available so shouldn't negotiate
Copy link
Copy Markdown
Member

@kozlovic kozlovic Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

// a second layer of it.
if c.isWebsocket() {
Copy link
Copy Markdown

@synadia-claude-reviewer synadia-claude-reviewer bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

c.mu.Lock()
c.leaf.compression = CompressionOff
c.mu.Unlock()
return false, nil
Comment on lines +1651 to +1655
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

}
// Negotiate the appropriate compression mode (or no compression)
cm, err := selectCompressionMode(co.Mode, infoCompression)
if err != nil {
Expand Down
49 changes: 49 additions & 0 deletions server/leafnode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7652,6 +7652,55 @@ func TestLeafNodeCompressionWithOlderServer(t *testing.T) {
}
}

func TestLeafNodeNoCompressionWithWebsocket(t *testing.T) {
conf1 := createConfFile(t, []byte(`
port: -1
server_name: "A"
websocket {
listen: "127.0.0.1:-1"
no_tls: true
}
Copy link
Copy Markdown
Member

@kozlovic kozlovic Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

leafnodes {
port: -1
compression: "s2_fast"
}
`))
s1, o1 := RunServerWithConfig(conf1)
defer s1.Shutdown()

conf2 := createConfFile(t, []byte(fmt.Sprintf(`
port: -1
server_name: "B"
leafnodes {
remotes [
{url: "ws://127.0.0.1:%d"}
]
}
`, o1.Websocket.Port)))
s2, _ := RunServerWithConfig(conf2)
defer s2.Shutdown()

checkLeafNodeConnected(t, s2)

getLeafCompMode := func(s *Server) string {
var cm string
s.mu.RLock()
defer s.mu.RUnlock()
for _, l := range s.leafs {
l.mu.Lock()
cm = l.leaf.compression
l.mu.Unlock()
return cm
}
return _EMPTY_
}
for _, s := range []*Server{s1, s2} {
if cm := getLeafCompMode(s); cm != CompressionOff {
t.Fatalf("Expected websocket leaf compression to be %q, got %q", CompressionOff, cm)
}
}
}

func TestLeafNodeCompressionAuto(t *testing.T) {
for _, test := range []struct {
name string
Expand Down
Loading