Fix shutdown race when restoring alert information from the database - #22448
Merged
Conversation
Shutdown does not actively wait for HEALTH to leave sqlite3_step: service_wait_exit(SERVICE_HEALTH) is bounded to 3s, cancel_main_threads only joins threads already marked EXITED, and nd_thread_join_threads reaps the exited list rather than blocking on live threads. If HEALTH is still iterating SQL_LOAD_HEALTH_LOG when sqlite_close_databases() runs, sqlite3_close_v2() tears down the page cache underneath it and the next pcache1Unpin faults at offset 0x30. Bail the row loop on !service_running(SERVICE_HEALTH), matching the existing pattern at sqlite_health.c:567. Also move rw_spinlock_init() before sql_health_alarm_log_load() so the lock the load already takes is initialized first.
Contributor
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant HEALTH as HEALTH Service
participant INIT as health_initialize_rrdhost()
participant LOAD as sql_health_alarm_log_load()
participant DB as SQLite Database
participant SPIN as health_log.spinlock
Note over HEALTH,DB: HEALTH startup flow (current)
INIT->>SPIN: rw_spinlock_init(&spinlock)
Note over INIT,SPIN: (CHANGED order: now before load)
INIT->>LOAD: sql_health_alarm_log_load(host)
LOAD->>SPIN: rw_spinlock_read_lock()
LOAD->>DB: sqlite3_step_monitored(res)
DB-->>LOAD: SQLITE_ROW (row data)
loop For each row
LOAD->>HEALTH: service_running(SERVICE_HEALTH)?
alt HEALTH is still running
LOAD->>LOAD: Process row, create ALARM_ENTRY
LOAD->>DB: sqlite3_step_monitored(res) for next row
DB-->>LOAD: SQLITE_ROW or SQLITE_DONE
else HEALTH signaled to stop
LOAD->>LOAD: Exit loop early
Note over LOAD: (prevents access after DB close)
end
end
LOAD->>SPIN: rw_spinlock_read_unlock()
LOAD-->>INIT: Return
Note over HEALTH,DB: HEALTH shutdown sequence (race fixed)
HEALTH->>HEALTH: service_signal_exit(SERVICE_HEALTH)
HEALTH->>DB: sqlite_close_databases()
Note over HEALTH,DB: LOAD loop already exited, no dangling step()
stelfrag
marked this pull request as ready for review
May 7, 2026 19:32
stelfrag
marked this pull request as draft
May 7, 2026 19:32
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a shutdown-time race in the HEALTH service where SQLite could be closed while alert log restoration is still iterating rows, and ensures the health log spinlock is initialized before it’s used during load.
Changes:
- Initialize
host->health_log.spinlockbefore callingsql_health_alarm_log_load()during host health initialization. - Stop iterating SQLite result rows in
sql_health_alarm_log_load()whenSERVICE_HEALTHis no longer running.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/health/health_event_loop.c |
Moves health log spinlock initialization earlier so the DB restore path can safely take the lock. |
src/database/sqlite/sqlite_health.c |
Adds service_running(SERVICE_HEALTH) to the row-iteration loop condition to exit early during shutdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
stelfrag
marked this pull request as ready for review
May 8, 2026 10:43
thiagoftsm
approved these changes
May 8, 2026
thiagoftsm
left a comment
Contributor
There was a problem hiding this comment.
No issues happen during shutdown and runtime. LGTM!
Merged
Ferroin
pushed a commit
that referenced
this pull request
Jul 15, 2026
…22448) * fix(health): close shutdown race in sql_health_alarm_log_load Shutdown does not actively wait for HEALTH to leave sqlite3_step: service_wait_exit(SERVICE_HEALTH) is bounded to 3s, cancel_main_threads only joins threads already marked EXITED, and nd_thread_join_threads reaps the exited list rather than blocking on live threads. If HEALTH is still iterating SQL_LOAD_HEALTH_LOG when sqlite_close_databases() runs, sqlite3_close_v2() tears down the page cache underneath it and the next pcache1Unpin faults at offset 0x30. Bail the row loop on !service_running(SERVICE_HEALTH), matching the existing pattern at sqlite_health.c:567. Also move rw_spinlock_init() before sql_health_alarm_log_load() so the lock the load already takes is initialized first. * fix(health): change spinlock to write lock for health log access (cherry picked from commit 5c96c3e)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pcache1Unpin(fault at0x30) whensqlite_close_databases()ran whilesql_health_alarm_log_load()was still insidesqlite3_step.!service_running(SERVICE_HEALTH)so the earlyservice_signal_exit(SERVICE_HEALTH)at shutdown is honored before sqlite is closed.host->health_log.spinlockbeforesql_health_alarm_log_load()instead of after — the load already takes that lock.Summary by cubic
Fixes a shutdown race in the HEALTH service that could crash while restoring alert logs if SQLite closes mid-iteration. Also switches health log access to a write lock during load to avoid concurrent writes.
service_running(SERVICE_HEALTH) && sqlite3_step_monitored(res) == SQLITE_ROW.host->health_log.spinlockbeforesql_health_alarm_log_load(host)and use a write lock (rw_spinlock_write_lock/unlock) during load.Written for commit 371b50e. Summary will update on new commits.