Skip to content

fix(hub): don't read the SSH client after it is closed - #2277

Merged
henrygd merged 1 commit into
henrygd:mainfrom
justadityaraj:fix/ssh-session-client-race
Aug 30, 2026
Merged

henrygd merged 1 commit into
henrygd:mainfrom
justadityaraj:fix/ssh-session-client-race

Conversation

@justadityaraj

Copy link
Copy Markdown
Contributor

📃 Description

Closes #2157.

createSessionWithTimeout checks sys.client for nil, then dereferences it a second time inside the goroutine that calls NewSession. Those two reads are not the same read. update() runs the SMART fetch in its own goroutine (system.go:166), so the updater goroutine can call closeSSHConnection and clear the field in between, and the session goroutine dereferences a nil client. That panic is not recovered anywhere, so it takes down the whole hub process.

The client field is now an atomic.Pointer[ssh.Client], loaded once before the goroutine starts and cleared with Swap, matching the atomic.Bool fields already on the struct. NewSession on an already-closed client returns an error, which runSSHOperation already retries. createSSHClient still stores the dial result unconditionally, so a failed dial clears the field exactly as before.

🪵 Changelog

🔧 Fixed

  • Hub no longer panics with a nil pointer dereference when an SSH connection is closed while a session is being created.

Verification

TestCreateSessionDuringClose runs session creation against closeSSHConnection and reproduces the reported crash on main with the same frames as the issue:

panic: runtime error: invalid memory address or nil pointer dereference
golang.org/x/crypto/ssh.(*Client).NewSession(0x0?)
	golang.org/x/[email protected]/ssh/client.go:161 +0xe
github.com/henrygd/beszel/internal/hub/systems.(*System).createSessionWithTimeout.func1()
	internal/hub/systems/system.go:776 +0x45
created by ...createSessionWithTimeout in goroutine 9

It panicked on 3 of 3 runs before the change and passes after it.

  • go test -tags testing ./internal/hub/...: config, expirymap, heartbeat, systems, transport and ws all pass. internal/hub itself does not build on this Windows host, before or after the change, because agent/sensors_windows.go embeds a LibreHardwareMonitor binary that is not checked in.
  • go vet -tags testing ./internal/hub/systems/... and gofmt -l clean.
  • I could not run -race here (no cgo toolchain on this machine), which is part of why the field moved to an atomic rather than a plain captured pointer.

createSessionWithTimeout checked sys.client for nil and then dereferenced
it again inside the goroutine that calls NewSession. update() runs the
SMART fetch in its own goroutine, so closeSSHConnection can clear the
field between those two reads and the goroutine dereferences a nil
client, panicking the whole hub process.

Make client an atomic.Pointer, load it once before starting the
goroutine, and clear it with Swap so a concurrent close cannot be
observed mid-session-creation. NewSession on an already-closed client
returns an error, which the existing retry path already handles.

Closes henrygd#2157

@henrygd henrygd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks like a good solution, thanks for your work 👍

@henrygd
henrygd merged commit 3af6512 into henrygd:main Aug 30, 2026
JuliusFreudenberger pushed a commit to JuliusFreudenberger/nix-config that referenced this pull request Sep 3, 2026
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [henrygd/beszel](https://github.com/henrygd/beszel) | minor | `0.18.8` → `0.19.0` |

---

> ⚠️ **Warning**
>
> Some dependencies could not be looked up. Check the [Dependency Dashboard](issues/12) for more information.

---

### Release Notes

<details>
<summary>henrygd/beszel (henrygd/beszel)</summary>

### [`v0.19.0`](https://github.com/henrygd/beszel/releases/tag/v0.19.0)

[Compare Source](henrygd/beszel@v0.18.8...v0.19.0)

:warning:  **Potential breaking change:** Agents now verify HTTPS certificates. If an agent connects to a hub using a self-signed or otherwise untrusted certificate, configure `CA_CERT_FILE` with the appropriate CA certificate or the connection will be rejected.

#### What's Changed

- Add ZFS pool and dataset monitoring ([#&#8203;2209](henrygd/beszel#2209))
- Add container health alerts with log excerpts in notifications ([#&#8203;2225](henrygd/beszel#2225))
- Add alerts for failed systemd services ([#&#8203;2173](henrygd/beszel#2173))
- Add alerts for CPU I/O wait and steal time ([#&#8203;2249](henrygd/beszel#2249))
- Add cumulative disk read and write totals to the Disk I/O sheet ([#&#8203;2179](henrygd/beszel#2179))
- Add Greek and Uzbek translations ([#&#8203;2034](henrygd/beszel#2034))
- Add custom root disk names and sort disks alphabetically ([#&#8203;2006](henrygd/beszel#2006))
- Improve alert security by enforcing system ownership and blocking additional internal notification URL ranges
- Improve ATA SMART warnings for critical attributes ([#&#8203;2275](henrygd/beszel#2275))
- Fix a hub crash caused by concurrent SSH client shutdown ([#&#8203;2277](henrygd/beszel#2277))
- Fix host CPU and memory totals when Docker runs in a VM on macOS or Windows ([#&#8203;2272](henrygd/beszel#2272))
- Fix multi-line `TOKEN_FILE` loading ([#&#8203;2276](henrygd/beszel#2276))
- Fix QNAP MD RAID arrays being incorrectly reported as failed ([#&#8203;2065](henrygd/beszel#2065))
- Fix Intel GPU power data disappearing between samples ([#&#8203;2256](henrygd/beszel#2256))
- Fix swap usage calculations to include cached swap ([#&#8203;2267](henrygd/beszel#2267))
- Fix agent payload failures caused by invalid UTF-8 in battery names ([#&#8203;2241](henrygd/beszel#2241))
- Fix uptime reporting inside Linux containers ([#&#8203;2180](henrygd/beszel#2180))
- Fix fan discovery on legacy hwmon devices ([#&#8203;2238](henrygd/beszel#2238))
- Fix CPU model detection on MIPS systems ([#&#8203;2138](henrygd/beszel#2138))
- Fix machine ID generation on FreeBSD and pfSense installations ([#&#8203;2274](henrygd/beszel#2274))
- Fix agent environment variable updates during reinstall ([#&#8203;2107](henrygd/beszel#2107))
- Fix the install script reporting the wrong listen address after upgrades ([#&#8203;2243](henrygd/beszel#2243))
- Fix load average precision on macOS, BSD, and Windows ([#&#8203;2245](henrygd/beszel#2245))
- Update the Go version and Go dependencies

#### New Contributors

- [@&#8203;IlyaMur](https://github.com/IlyaMur) made their first contribution in [#&#8203;2238](henrygd/beszel#2238)
- [@&#8203;alecrubin](https://github.com/alecrubin) made their first contribution in [#&#8203;2180](henrygd/beszel#2180)
- [@&#8203;toomore](https://github.com/toomore) made their first contribution in [#&#8203;2241](henrygd/beszel#2241)
- [@&#8203;mews-se](https://github.com/mews-se) made their first contribution in [#&#8203;2243](henrygd/beszel#2243)
- [@&#8203;justadityaraj](https://github.com/justadityaraj) made their first contribution in [#&#8203;2245](henrygd/beszel#2245)
- [@&#8203;wassupluke](https://github.com/wassupluke) made their first contribution in [#&#8203;2256](henrygd/beszel#2256)
- [@&#8203;ErkinjonYusupov](https://github.com/ErkinjonYusupov) made their first contribution in [#&#8203;2034](henrygd/beszel#2034)
- [@&#8203;snh](https://github.com/snh) made their first contribution in [#&#8203;2279](henrygd/beszel#2279)
- [@&#8203;spatiumstas](https://github.com/spatiumstas) made their first contribution in [#&#8203;2249](henrygd/beszel#2249)
- [@&#8203;martinstenrose](https://github.com/martinstenrose) made their first contribution in [#&#8203;2173](henrygd/beszel#2173)
- [@&#8203;mleczakm](https://github.com/mleczakm) made their first contribution in [#&#8203;2225](henrygd/beszel#2225)
- [@&#8203;santhiprakash](https://github.com/santhiprakash) made their first contribution in [#&#8203;2269](henrygd/beszel#2269)

**Full Changelog**: <henrygd/beszel@v0.18.8...v0.19.0>

</details>

---

### Configuration

📅 **Schedule**: (UTC)

- Branch creation
  - At any time (no schedule defined)
- Automerge
  - At any time (no schedule defined)

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xNTAuMSIsInVwZGF0ZWRJblZlciI6IjQzLjE1MC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Reviewed-on: https://git.jfreudenberger.de/JuliusFreudenberger/nix-config/pulls/43
xiaoran7 pushed a commit to xiaoran7/beszel that referenced this pull request Oct 17, 2026
createSessionWithTimeout checked sys.client for nil and then dereferenced
it again inside the goroutine that calls NewSession. update() runs the
SMART fetch in its own goroutine, so closeSSHConnection can clear the
field between those two reads and the goroutine dereferences a nil
client, panicking the whole hub process.

Make client an atomic.Pointer, load it once before starting the
goroutine, and clear it with Swap so a concurrent close cannot be
observed mid-session-creation. NewSession on an already-closed client
returns an error, which the existing retry path already handles.

Closes henrygd#2157
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.

[Bug]: Hub can panic in createSessionWithTimeout during concurrent SSH client close

2 participants