fix: treat negative keepalive replies as liveness

[skevetter]

Summary

Fixes #27.

OpenSSH-style keepalive requests use the arrival of a reply as the liveness signal. A protocol-level negative reply (ok=false, err=nil) still proves the peer received and processed the request and must therefore reset the keepalive deadline.

This PR:

• treats both positive and negative SSH replies as proof of liveness when err == nil;
• falls back from channel keepalive to global keepalive only on a transport-level channel error;
• updates keepalive comments/logging to describe missing replies rather than negative acknowledgements;
• replaces the previous negative-reply teardown regressions with tests proving negative global and channel replies keep the connection alive beyond the configured timeout window;
• verifies negative channel replies do not trigger unnecessary global fallback;
• preserves teardown behavior for peers that do not reply at all.

Root cause

The current implementation requires:

err == nil && ok

before resetting keepalive state. For keepalive@openssh.com, OpenSSH commonly responds with an SSH failure reply because the request is intentionally unsupported. The failure reply is still a valid response and proves transport liveness.

With Devsy's default 15s * 8 policy, this caused healthy OpenSSH/VS Code Remote-SSH connections to be closed after roughly 120 seconds.

Validation

The updated tests cover:

• negative global replies keep the connection alive;
• negative channel replies keep the connection alive;
• negative channel replies do not fall back to global probes;
• fully non-responsive peers are still torn down by the existing stalled-connection tests.

CI results will be attached to this draft PR.

Summary by CodeRabbit

• Bug Fixes

  • Improved SSH connection keep-alive handling so protocol-level negative replies correctly confirm that the peer is reachable.
  • Connections remain active when peers respond negatively to keep-alive probes, preventing unnecessary timeouts and disconnects.
  • Improved channel-based keep-alive behavior to avoid sending unnecessary fallback probes when channel communication has already established peer liveness.

• Tests

  • Updated coverage to verify connections remain alive after negative keep-alive responses.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 91e7803e-1e78-40d6-a860-84bd645c70fc

📥 Commits

Reviewing files that changed from the base of the PR and between bf7198c and bef60cc.

📒 Files selected for processing (2)

• server.go
• server_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The keepalive implementation now treats negative SSH replies as liveness evidence. Channel probes fall back to global requests only on transport errors. Regression tests verify continued connection liveness and prevent unnecessary global fallback.

Changes

Keepalive liveness handling

Layer / File(s)	Summary
Update keepalive probe handling server.go	The keepalive timer resets when a channel or global request receives any reply. Channel removal and global fallback occur only after transport errors.
Validate negative-reply behavior server_test.go	Global and channel tests now expect negative replies to keep the connection alive. The channel test also verifies that no global fallback requests occur.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to bef60

Keepalive replies now preserve live SSH connections even when the protocol reply is negative, while unresponsive peers remain eligible for teardown. No current merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	❓ Inconclusive	The implementation and regression tests address negative global and channel replies, liveness resets, and channel fallback behavior. The provided summary does not confirm the required positive-reply, …	Confirm or add tests for positive replies, negative replies, transport errors, and non-responsive peers for both channel and global keepalives. Include evidence that transport errors trigger global fallback while negative replies do not tri…

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: treating negative keepalive replies as proof of liveness.
Out of Scope Changes check	✅ Passed	The changes are limited to keepalive handling and its tests. They align with the linked issue and do not modify unrelated transport, forwarding, or configuration behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.

Full details: Linked Issues check

Explanation

The implementation and regression tests address negative global and channel replies, liveness resets, and channel fallback behavior. The provided summary does not confirm the required positive-reply, transport-error, and non-responsive-peer test coverage for both request paths.

Resolution

Confirm or add tests for positive replies, negative replies, transport errors, and non-responsive peers for both channel and global keepalives. Include evidence that transport errors trigger global fallback while negative replies do not trigger fallback.

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/openssh-negative-keepalive-liveness

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git rebase --exec 'git commit -S --amend --no-edit -n' @{upstream}) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

1. Generate a GPG key
2. Add the GPG key to your GitHub account
3. Configure git to use your GPG key for commit signing

Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

1. Generate an SSH key and add it to ssh-agent
2. Add the SSH key to your GitHub account
3. Configure git to use your SSH key for commit signing

Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.