[PR #412] fix(healthcheck): guard target status fields against data race #8992

New Issue

GiteaMirror · 2026-07-14T21:07:48-05:00

GiteaMirror commented

2026-07-14 21:07:48 -05:00

📋 Pull Request Information

Original PR: https://github.com/fosrl/newt/pull/412
Author: @lafoush
Created: 7/14/2026
Status: 🔄 Open

Base: main ← Head: fix/healthcheck-status-data-race

📝 Commits (1)

a4017be fix(healthcheck): guard target status fields against data race

📊 Changes

2 files changed (+117 additions, -4 deletions)

View changed files

📝 healthcheck/healthcheck.go (+34 -4)
➕ healthcheck/healthcheck_test.go (+83 -0)

📄 Description

Problem

monitorTarget / performHealthCheck mutate a Target's Status, LastError, LastCheck, CheckCount, and consecutive-success/failure counters from the target's monitor goroutine. Meanwhile getAllTargetsUnsafe (called by GetTargets, used for status reporting to the server) copied the whole struct with targetCopy := *target.

The monitor mutex protects the targets map, not the contents of each Target, so these accesses overlap without synchronization. The race detector flags it on any concurrent monitor + GetTargets workload — e.g. the reconnect path, where the status reporter snapshots targets while their monitor goroutines are updating status.

WARNING: DATA RACE
Read at ... by goroutine N:
  healthcheck.(*Monitor).getAllTargetsUnsafe()   // targetCopy := *target
Previous write at ... by goroutine M:
  healthcheck.(*Monitor).monitorTarget()         // target.timer / target.Status

Fix

Add a per-Target sync.Mutex guarding the mutable status fields.
Replace the whole-struct copy with a snapshot() taken under that lock, copying only the reported fields (Config, Status, LastCheck, LastError, CheckCount). Runtime-only fields (timer/ctx/cancel/client) are intentionally omitted — callers of GetTargets only read the reported fields, and this also avoids copying a sync.Mutex.
The lock is not held across the health-check network request, so status snapshots are never blocked for the check timeout.
DisableTarget's Status write is guarded too.

Test

Adds healthcheck_test.go with a regression test that exercises concurrent monitoring + snapshotting. It fails under -race without the fix and passes with it:

# without the fix
--- FAIL: TestConcurrentStatusSnapshotNoRace (race detected)
# with the fix
ok  github.com/fosrl/newt/healthcheck

go build, go vet ./healthcheck/, and gofmt are clean.

No functional/behavioral change to health checking — this is purely a concurrency-safety fix.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/fosrl/newt/pull/412 **Author:** [@lafoush](https://github.com/lafoush) **Created:** 7/14/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/healthcheck-status-data-race` --- ### 📝 Commits (1) - [`a4017be`](https://github.com/fosrl/newt/commit/a4017be6431c6a5db5500f6e33c8c1d366236681) fix(healthcheck): guard target status fields against data race ### 📊 Changes **2 files changed** (+117 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `healthcheck/healthcheck.go` (+34 -4) ➕ `healthcheck/healthcheck_test.go` (+83 -0) </details> ### 📄 Description ## Problem `monitorTarget` / `performHealthCheck` mutate a `Target`'s `Status`, `LastError`, `LastCheck`, `CheckCount`, and consecutive-success/failure counters from the target's monitor goroutine. Meanwhile `getAllTargetsUnsafe` (called by `GetTargets`, used for status reporting to the server) copied the whole struct with `targetCopy := *target`. The monitor mutex protects the `targets` **map**, not the contents of each `Target`, so these accesses overlap without synchronization. The race detector flags it on any concurrent monitor + `GetTargets` workload — e.g. the reconnect path, where the status reporter snapshots targets while their monitor goroutines are updating status. ``` WARNING: DATA RACE Read at ... by goroutine N: healthcheck.(*Monitor).getAllTargetsUnsafe() // targetCopy := *target Previous write at ... by goroutine M: healthcheck.(*Monitor).monitorTarget() // target.timer / target.Status ``` ## Fix - Add a per-`Target` `sync.Mutex` guarding the mutable status fields. - Replace the whole-struct copy with a `snapshot()` taken under that lock, copying only the reported fields (`Config`, `Status`, `LastCheck`, `LastError`, `CheckCount`). Runtime-only fields (`timer`/`ctx`/`cancel`/`client`) are intentionally omitted — callers of `GetTargets` only read the reported fields, and this also avoids copying a `sync.Mutex`. - The lock is **not** held across the health-check network request, so status snapshots are never blocked for the check timeout. - `DisableTarget`'s `Status` write is guarded too. ## Test Adds `healthcheck_test.go` with a regression test that exercises concurrent monitoring + snapshotting. It **fails under `-race` without the fix and passes with it**: ``` # without the fix --- FAIL: TestConcurrentStatusSnapshotNoRace (race detected) # with the fix ok github.com/fosrl/newt/healthcheck ``` `go build`, `go vet ./healthcheck/`, and `gofmt` are clean. No functional/behavioral change to health checking — this is purely a concurrency-safety fix. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

GiteaMirror added the pull-request label 2026-07-14 21:07:48 -05:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/newt#8992