fix(minimald,minimal): keepalives instead of the inactivity reaper by norrietaylor · Pull Request #798 · gominimal/minimal

norrietaylor · 2026-07-17T00:02:18Z

Summary

Fixes the reproduced mechanism behind #788: russh's inherited inactivity_timeout: Some(600s) reaps healthy-but-quiet attaches; a lost close across the vsock (#588) then leaves the client — no keepalives, no timeout — hung forever. Full evidence chain and on-demand reproduction in the issue thread.

minimald: inactivity_timeout: None (explicit), keepalive_interval: 20s, keepalive_max: 3, with named constants documenting the policy
min client: mirror keepalive config — any silent transport death becomes a visible error within ~60s

Out of scope (tracked in #788): binding teardown on connection death (owned separately), bounded attach() for the establishment race, #588 upstream.

Test plan

cargo check -p minimal (macOS) — client config compiles
CI full matrix
Field validation: idle an attached session >10 min on the new build — expect no reap, no wedge; kill the daemon under a live attach — expect client error within ~60s

Refs: #788

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved SSH connection resilience during extended inactivity, including laptop sleep and wake cycles, by refining keepalive behavior.
- Enhanced server-side keepalive handling to better distinguish real disconnects from idle periods without unintended session drops.
Chores
- Increased CI Linux KVM build timeout to allow more time for the build phase.

russh's default server config ships a 600s inactivity_timeout that garbage-collects any connection with no inbound traffic — including a healthy attached session whose user simply stops typing for ten minutes. When the resulting close was lost across the vsock, the client (which defaults to no keepalives and no timeout of its own) hung forever: the "established attach died silently" field report, reproduced on demand from nothing but idle time. Invert the liveness policy on both sides: probe quiet connections every 20s, tolerate 3 unanswered probes (~60s bounded detection of dead peers, visible as an error), and switch the wall-clock reaper off explicitly — an idle connection is not a dead one. Binding teardown when a connection dies out from under it is handled separately. Refs: #788 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

coderabbitai · 2026-07-17T00:02:27Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f8d972c9-8b98-4450-97f3-4427da185ff1

📥 Commits

Reviewing files that changed from the base of the PR and between dd08d8b and cb66dfc.

📒 Files selected for processing (1)

.github/workflows/ci-linux-kvm.yml

📝 Walkthrough

Walkthrough

The server now uses explicit SSH keepalive settings instead of inactivity reaping defaults. The client documents its intentional absence of keepalives, and the Linux KVM build timeout is increased.

Changes

SSH keepalive behavior

Layer / File(s)	Summary
Keepalive policy and client behavior `crates/minimald/src/server.rs`, `crates/minimal/src/client.rs`	The server disables inactivity reaping and configures keepalive interval and maximum unanswered probes. The client documents its no-keepalive configuration and sleep/wake behavior.

CI build timeout

Layer / File(s)	Summary
Linux KVM build timeout `.github/workflows/ci-linux-kvm.yml`	The `build-linux` job timeout increases from 45 to 60 minutes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

I’m a rabbit with packets that hop through the night,
The server sends probes to keep links just right.
The client stays quiet through slumber and dew,
While builds get more time before morning light.
Keepalive carrots for all in the queue!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: replacing the inactivity reaper with keepalive-based liveness handling in minimald/minimal.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

twitchyliquid64

I suggest:

keepalive interval in the 10 mins to 1 hr range
No client keepalives, or have them be in the 10mins or more range

The 20s keepalive interval was too aggressive: it wakes the radio every 20s, draining the battery — a real cost once these connections run remote. And the client keepalive broke the "close the laptop for an hour, reopen, still works" case, which works precisely when the client does not send keepalives. With the wall-clock inactivity reaper already off, keepalives only need to be a slow liveness backstop, not fast-detection: no idle-but-alive session is ever spuriously reaped, so we do not need to notice a dead peer quickly. So: - minimald: KEEPALIVE_INTERVAL 20s -> 30min (KEEPALIVE_MAX stays 3). ~30min x 3 ~= 90min to reap a truly-dead peer, which is fine. - minimal: drop client keepalives entirely; back to russh::client::Config::default(). A laptop closed for an hour now reconnects transparently on wake, and the server's long-interval keepalive is the only liveness mechanism. Refs: #788 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/minimal/src/client.rs`:
- Around line 84-88: Reword the comment describing the client’s keepalive
behavior to avoid promising transparent or automatic reconnection. Clarify that
this layer maintains a single SSH session and disabling client keepalives only
prevents client-side timeout during sleep; state that higher layers must
recreate the session after wake if recovery is needed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3b85e3c1-653d-4446-9541-80dcffa7bf8d

📥 Commits

Reviewing files that changed from the base of the PR and between 9e9f15e and dd08d8b.

📒 Files selected for processing (2)

crates/minimal/src/client.rs
crates/minimald/src/server.rs

A cold cargo build takes just over 45 minutes, so once the cache entry is evicted the job times out before its main-only save step can run and the cache never repopulates — every subsequent run on main and on PRs builds cold and times out again. 60 minutes lets a cold build finish. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

twitchyliquid64 reviewed Jul 17, 2026

View reviewed changes

Comment thread crates/minimal/src/client.rs Outdated

twitchyliquid64 reviewed Jul 17, 2026

View reviewed changes

Comment thread crates/minimald/src/server.rs Outdated

twitchyliquid64 approved these changes Jul 17, 2026

View reviewed changes

norrietaylor marked this pull request as ready for review July 17, 2026 00:36

norrietaylor and others added 2 commits July 16, 2026 17:36

Merge branch 'main' into worktree-fix-788-inactivity-keepalive

008182a

norrietaylor enabled auto-merge (squash) July 17, 2026 04:01

This was referenced Jul 17, 2026

feat(minimal): add min bug diagnostic bundle command #784

Closed

Diagnostics subsystem: min bug support bundles #801

Closed

docs(minimald): trim keepalive constant doc comments

dd08d8b

norrietaylor force-pushed the worktree-fix-788-inactivity-keepalive branch from 6d666a7 to dd08d8b Compare July 17, 2026 05:07

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread crates/minimal/src/client.rs

norrietaylor merged commit 8d2f695 into main Jul 17, 2026
28 checks passed

norrietaylor deleted the worktree-fix-788-inactivity-keepalive branch July 17, 2026 07:03

Navigation Menu

Uh oh!

fix(minimald,minimal): keepalives instead of the inactivity reaper - #798

fix(minimald,minimal): keepalives instead of the inactivity reaper#798
norrietaylor merged 5 commits into
mainfrom
worktree-fix-788-inactivity-keepalive

norrietaylor commented Jul 17, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

Uh oh!

Uh oh!

twitchyliquid64 left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

norrietaylor commented Jul 17, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Uh oh!

Uh oh!

twitchyliquid64 left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

norrietaylor commented Jul 17, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading