fix: treat negative OpenSSH keepalive replies as proof of liveness

[skevetter]

Summary

Healthy OpenSSH clients are being disconnected by the server-side keepalive logic after approximately ClientAliveInterval * ClientAliveCountMax even though the peer is responding to keepalive probes.

In Devsy this currently manifests as VS Code Remote-SSH losing its dynamic forwarding tunnel after roughly 120 seconds with the default configuration (15s * 8), followed by:

Failed to set up socket for dynamic port forward ... ECONNREFUSED

The forwarding error is downstream. The local OpenSSH process has already exited because the Devsy SSH server closed the underlying transport.

Root cause

The current keepalive implementation treats:

ok == false && err == nil

as a failed liveness check and does not reset the keepalive deadline.

That is incorrect for OpenSSH-style keepalive requests.

keepalive@openssh.com is intentionally an unsupported/bogus request whose purpose is to elicit a reply. A negative SSH reply (CHANNEL_FAILURE / REQUEST_FAILURE) still proves the peer received and processed the request and is therefore alive.

The health signal is:

Did a reply arrive successfully?

not:

Did the peer positively acknowledge the request semantics?

The regression was introduced when keepalive handling changed from err == nil to err == nil && ok.

Affected code

Primary file:

server.go

Current behavior is conceptually:

ok, err := ch.SendRequest(keepAliveRequestType, true, nil)
...
if err == nil && ok {
    keepAlive.Reset()
}

and similarly for the global request path.

When the client returns ok=false, err=nil repeatedly, lastReceived is never refreshed. Eventually TimeIsUp() becomes true and the server explicitly closes sshConn.

Required implementation

1. Treat any successfully received reply as liveness

For channel requests:

_, err := ch.SendRequest(keepAliveRequestType, true, nil)

For global requests:

_, _, err := sshConn.SendRequest(keepAliveRequestType, true, nil)

Reset the keepalive state whenever:

err == nil

regardless of whether the protocol reply is positive or negative.

Expected logic:

ch := openChans.any()
if ch != nil {
    _, err = ch.SendRequest(keepAliveRequestType, true, nil)
    if err != nil {
        openChans.remove(ch)
        ch = nil
    }
}

if ch == nil {
    _, _, err = sshConn.SendRequest(keepAliveRequestType, true, nil)
}

if err == nil {
    keepAlive.Reset()
} else {
    log.Printf("ssh: keepalive request failed: err=%v", err)
}

2. Do not fall back solely because ok == false

A negative channel reply already proves the peer is alive.

Fallback from channel keepalive to global keepalive should occur only when the channel request itself fails at the transport level (err != nil), for example because the channel closed while the probe was in flight.

3. Correct comments and terminology

Update comments such as:

no successful reply

to wording such as:

no reply received

The implementation and documentation should clearly distinguish:

• SSH request result: success/failure reply (ok)
• transport liveness: whether a reply was received (err == nil)

Tests

Unit tests — channel keepalive

Add/modify tests covering:

ok=true,  err=nil  -> reset liveness deadline
ok=false, err=nil  -> reset liveness deadline
err!=nil           -> do not reset; transport remains eligible for timeout

Unit tests — global keepalive

Cover the same matrix:

ok=true,  err=nil  -> alive
ok=false, err=nil  -> alive
err!=nil           -> not confirmed alive

Regression test for the prior behavior

Replace tests that currently assert negative replies should not postpone teardown.

The new regression should prove that repeated negative replies keep an otherwise idle connection alive beyond:

ClientAliveInterval * ClientAliveCountMax

OpenSSH integration test

Add a real OpenSSH compatibility test if practical.

Suggested setup:

1. start the Devsy SSH server with a short keepalive interval/count, e.g. 100ms * 3;
2. connect with the system ssh binary using a no-PTY session and/or dynamic forwarding;
3. keep the connection alive for substantially longer than the 300ms timeout window;
4. assert the OpenSSH process remains connected;
5. separately verify a truly non-responsive peer is eventually disconnected.

This test is important because the prior unit tests encoded an incorrect assumption about OpenSSH's keepalive semantics.

Devsy validation

After releasing the fixed devsy-org/ssh version, update devsy-org/devsy to consume it and validate VS Code Remote-SSH.

The known failure pattern to eliminate is:

SSH connects successfully
VS Code forwarding server starts
connection remains healthy for ~120s
ssh child dies
VS Code reports ECONNREFUSED to its local SOCKS port

With the default Devsy configuration (15s, count 8), the connection must remain healthy well beyond 120 seconds while the peer continues replying to keepalives.

Acceptance criteria

• ok=false, err=nil is treated as proof of liveness for channel keepalive requests
• ok=false, err=nil is treated as proof of liveness for global keepalive requests
• channel-to-global fallback occurs only on transport error, not negative acknowledgment
• keepalive timeout comments/documentation describe missing replies rather than negative replies
• unit tests cover positive reply, negative reply, and transport error for both channel and global requests
• regression test proves repeated negative replies keep the connection alive beyond the timeout window
• truly non-responsive transports are still closed after ClientAliveCountMax missed replies
• Devsy is updated to the fixed ssh release
• VS Code Remote-SSH dynamic forwarding remains connected beyond the previous ~120-second failure point

Out of scope

• redesigning the managed SSH transport architecture
• changing PipeBridge or ManagedConn
• changing VS Code Remote-SSH settings
• addressing unrelated GPG agent/public-key errors observed during reconnect
• broad changes to activity tracking (notePeerActivity) beyond what is necessary for this keepalive fix

Notes

The managed-transport work is not implicated by the failure signature. The transport is established and functional; the server-side keepalive policy is what terminates it on schedule.