Skip to content

fix(state): keep disk I/O out of the intern write lock - #795

Merged
frahlg merged 1 commit into
masterfrom
harvest-intern-lock-no-disk-io
Aug 4, 2026
Merged

fix(state): keep disk I/O out of the intern write lock#795
frahlg merged 1 commit into
masterfrom
harvest-intern-lock-no-disk-io

Conversation

@frahlg

@frahlg frahlg commented Aug 4, 2026

Copy link
Copy Markdown
Member

The hazard

go/internal/state/store_ts.go interns driver and metric names to integer ids
so a sample row stays small. The cache guarded its maps with one sync.RWMutex
and did its disk work while holding that mutex exclusively:

  • driverID and metricID took the write lock, then issued the allocating
    INSERT (and, for a relabelled unit, an UPDATE) with it still held.
  • hydrateIntern held it across two full-table scans.

All three run inside the control tick (main.go:2753:3634
RecordTickWithOptionalHistory). Every read-only surface that answers the API —
MetricsCatalog, MetricNames, DriverNames, LoadSeries,
LoadSeriesBuckets, LatestSample, SamplesBefore — takes the read side of
that same mutex.

So on a slow SD card, the first sample of a new metric parked every API reader
until SQLite committed. That is the shape of the 2026-07-16 prune incident
(blocking disk work inside something the whole system waits on), expressed as a
lock instead of a channel. The hot path is unaffected — a known name has always
answered under the read lock — but a new driver, a new metric, a driver that
starts reporting a unit, and every process boot all hit it, and a busy SQLite
writer stretches the window well past a tick.

The fix

The standard three-step for an intern cache:

  1. read-check under RLock;
  2. do the DB work with no map lock held;
  3. take the write lock only to publish the resulting id.

hydrateIntern scans into local maps and swaps them in under the lock.

A second mutex, allocMu, serializes the disk half. It buys two things the
lock-free version does not:

  • the row and the cached entry cannot disagree when two callers relabel the
    same metric's unit at once (without it, DB and cache can end up with
    different units, and the map's value survives the restart that reloads the
    row's);
  • hydrate's swap cannot discard an id that an allocation published between the
    scan and the swap.

Readers never take allocMu, so a stuck write still cannot reach them. Lock
order is documented on the type: allocMu first, mu second, never the
reverse.

Concurrent allocation of the same new name is handled by the schema:
ts_drivers.name and ts_metrics.name are both UNIQUE, so
INSERT … ON CONFLICT resolves to the existing row instead of failing the whole
sample batch. metricID folds allocate-and-relabel into one upsert whose
COALESCE(NULLIF(excluded.unit, ''), ts_metrics.unit) keeps an empty unit from
erasing a label already stored — the previous behaviour, in one statement.

Tests

go/internal/state/store_ts_intern_test.go:

  • TestInternAllocationDoesNotBlockReaders — the regression. It makes the
    disk write slow the way a loaded Pi does, by holding SQLite's write lock on a
    second connection, then requires the four read surfaces to answer while two
    allocations are stuck on it. Against the old code MetricsCatalog took
    741 ms (bound: 250 ms); with the fix the reads return in microseconds.
  • TestInternConcurrentSameNameYieldsOneID — 24 goroutines interning the
    same new driver / metric agree on one id and leave one row.
  • TestInternUnderConcurrentReadersAndWriters — the post-2026-07-16 rule
    that DB work is volume-tested with a simultaneous writer: four writers stream
    480 overlapping new metric names across six drivers while four readers poll
    the intern cache. No errors, no reader parked, every name on exactly one row.
    Slowest read went from 639 µs to 123 µs on this box; the point is the bound,
    not the number.

make verify clean. -race clean for the new tests.

Note on an unrelated flake

TestPruneLargeBacklogWithConcurrentWriter fails intermittently under -race
on a loaded machine (1 in 3 runs) — on master as well as here. It exercises
history_hot only and never touches the intern path. Not addressed in this PR.

🤖 Generated with Claude Code

The time-series intern cache held its exclusive lock across the
allocating SQLite INSERT, and hydrate held it across two full-table
scans. Both run from the control tick, and every API reader of the
metric catalog, name lists and chart series waits on the same lock.

Allocation now writes with no map lock held and takes the lock only to
publish the id; hydrate scans into local maps and swaps them in. A
second mutex serializes allocators so the row and the cached entry
cannot disagree, and so hydrate's swap cannot drop an id allocated
meanwhile. ON CONFLICT keeps a concurrent duplicate name on one row.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@frahlg
frahlg merged commit 2aa23c3 into master Aug 4, 2026
13 checks passed
@frahlg
frahlg deleted the harvest-intern-lock-no-disk-io branch August 7, 2026 08:18
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.

1 participant