Skip to content

fix(hub): stop background goroutines from crashing the hub - #2169

Open
milanobrtlik wants to merge 1 commit into
henrygd:mainfrom
milanobrtlik:fix/2154-background-goroutine-panics
Open

fix(hub): stop background goroutines from crashing the hub#2169
milanobrtlik wants to merge 1 commit into
henrygd:mainfrom
milanobrtlik:fix/2154-background-goroutine-panics

Conversation

@milanobrtlik

Copy link
Copy Markdown

📃 Description

Fixes #2154, where the hub panicked with a nil pointer dereference in upsertSmartDeviceRecord and exited, taking every monitored system down with it.

What happens

SMART data is fetched in a detached goroutine (system.go:163). That fetch can still be in flight when the app shuts down or restarts. ResetBootstrapState nils the PocketBase database handles, so the next query calls a method on a nil dbx.Builder and panics — and a panic in a detached goroutine cannot be recovered by its parent, so it takes the process with it.

This matches the reported trace: RecordQuery at core/record_query.go:38 is app.ConcurrentDB(), and addr=0xb0 is an itab offset, i.e. a nil interface rather than a nil *BaseApp.

The same shape exists in six other places — system updates, the realtime worker, staggered startup and alert delivery all run detached and touch the database — so this fixes the class rather than the one reported call site.

It also lines up with the note already in systems_production.go: "Background SMART fetching can outlive teardown and crash in PocketBase internals (nil DB)." That workaround only disables the fetch under tests; production was left exposed.

Approach

Two layers, because neither alone is enough:

  1. utils.SafeGo recovers and logs instead of aborting the process. Background metric collection is not worth a crash. It reports through slog rather than app.Logger() on purpose — PocketBase writes its logs to the database, which is exactly what is going away when these panics happen, so stderr is the only sink that still works.
  2. SystemManager.Shutdown() cancels every system context and refuses new ones, bound to OnTerminate. PocketBase triggers OnTerminate before ResetBootstrapState (pocketbase.go:212), so this stops the work rather than racing the teardown. Layer 1 stays as the backstop, since a restart can still land between a context check and a query.

Shutdown also closes hbStop, which honors the contract heartbeat.Start already documents ("runs until the provided stop channel is closed") but that nothing was fulfilling.

On verification

TestSaveSmartDevicesWithoutHub is verified both ways: with the nil guard removed it fails on the panic, and passes with it.

I could not reproduce the original production crash. Flipping backgroundSmartFetchEnabled() to true in the test helpers did not surface it — the suite passes. Under -count=5 the systems package does fail, but on an unrelated pre-existing issue (fatal error: send on synctest channel from outside bubble, from a DNS lookup inside a synctest bubble) that reproduces on an unmodified checkout with the flag off. I left the test-side workaround in place, since I have nothing to show that removing it is safe.

So the case here rests on the stack trace, the ResetBootstrapState → concurrentDB = nil path, and the unit tests — not on a reproduction of the race itself. Happy to adjust if you would rather scope this down to just the SMART path.

Note: make lint does not run for me — the golangci-lint I have is built against Go 1.25 while the module targets 1.26.3. I ran go vet -tags=testing ./internal/... and gofmt instead. gofmt -l also flags internal/entities/smart/smart.go, which is untouched here and already flagged on main.

🪵 Changelog

➕ Added

  • utils.SafeGo, which runs a function in a goroutine and recovers from panics so background work cannot terminate the hub
  • SystemManager.Shutdown, bound to OnTerminate, cancelling all system contexts and rejecting new systems during teardown

🔧 Fixed

)

The hub panicked with a nil pointer dereference inside
upsertSmartDeviceRecord and exited, taking every monitored system with it.

SMART data is fetched in a detached goroutine, which can still be in
flight when the app shuts down or restarts. PocketBase nils its database
handles in ResetBootstrapState, so the next query panics on a nil
dbx.Builder, and a panic in a detached goroutine cannot be recovered by
its parent.

The same shape appears in six other places: system updates, the realtime
worker, staggered startup and alert delivery all run detached and touch
the database.

Add utils.SafeGo, which recovers and logs instead of aborting the
process, and use it for background work that reaches the database. It
reports through slog rather than app.Logger() because PocketBase logs to
the database, which is exactly what is going away when these panics
happen.

Also stop racing the teardown in the first place. SystemManager.Shutdown
cancels every system context and refuses new ones, bound to OnTerminate,
which PocketBase triggers before ResetBootstrapState. This also closes
hbStop, honoring the contract heartbeat.Start already documents.
@milanobrtlik
milanobrtlik requested a review from henrygd as a code owner July 30, 2026 18:24
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 crashes with nil pointer dereference in SMART handling (upsertSmartDeviceRecord)

1 participant