Skip to content

fix(server): report a yamux stream that could not be opened - #6843

Merged
otavio merged 1 commit into
masterfrom
fix/dialer-swallowed-open-error
Aug 2, 2026
Merged

fix(server): report a yamux stream that could not be opened#6843
otavio merged 1 commit into
masterfrom
fix/dialer-swallowed-open-error

Conversation

@otavio

@otavio otavio commented Aug 2, 2026

Copy link
Copy Markdown
Member

What

Manager.Dial now returns the error when opening a yamux stream fails, instead of logging it and returning a nil connection with a nil error.

Why

The V2 branch logged the failed session.Open() and then fell through to the shared success return:

conn, err := session.Open()
if err != nil {
    log.WithFields(...).WithError(err).Error("failed to open yamux stream for reverse connection")
}

return conn, TransportVersion2, nil

conn is a nil *yamux.Stream boxed in a net.Conn, so the interface is non-nil and the caller's err != nil check passes. The first method call against it panics. A session that is shutting down — the ordinary case when a device drops while a request is in flight — becomes a crashed request rather than a failed one.

Found while investigating #6777; unrelated to that fix and based on master, so the two can merge in either order.

Testing

The new test stores a closed yamux session in the manager and asserts Dial reports the failure. Without the fix it returns (nil, TransportVersion2, nil) and the assertions on the error and the version both fail.

Failing to open the stream was logged and then dropped: Dial fell through to its success return,
handing the caller a nil connection with a nil error. The first write against it panics, which
turns a device whose session is shutting down into a crashed request rather than a failed one.
@otavio
otavio requested a review from a team as a code owner August 2, 2026 19:54
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 1m 38s —— View job


Code Review Complete

Reviewed 2 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

The fix at server/ssh/pkg/dialer/manager.go:199 correctly mirrors the existing v1 error path (lines 174–181): instead of falling through to return conn, TransportVersion2, nil with a boxed-nil *yamux.Stream, it now returns nil, TransportVersionUnknown, err. This eliminates the panic-on-first-call for the common device-shutdown-mid-request case.

The regression test is deterministic — session.Close() puts the yamux session into shutdown state, so the subsequent session.Open() returns ErrSessionShutdown synchronously with no network round-trip or timing dependency. The stored *yamux.Session type-asserts correctly into the v2 branch, imports are all used, and the assertions match the corrected return values.


To request another review round, comment /review.

@otavio
otavio enabled auto-merge (rebase) August 2, 2026 20:00
@otavio
otavio merged commit a1fa447 into master Aug 2, 2026
39 checks passed
@otavio
otavio deleted the fix/dialer-swallowed-open-error branch August 2, 2026 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant