unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
prometheus/prometheus
senior9/10
concurrencydata-structureserror-handling

tsdb: don't lose samples appended to a series removed by head GC

Project: prometheus/prometheus | Language: Go | Concepts: concurrency, data-structures, error-handling | Difficulty: senior
Link: https://github.com/prometheus/prometheus/pull/19272

The problem

When Prometheus's TSDB garbage collection (GC) removes a series from the in-memory index, an appender that already holds a pointer to that series can still append samples to it. These samples are silently lost: Append() and Commit() report success, but the samples are not reachable from the head, and thus are never persisted or queryable.

Why it's non-trivial

A naive fix might try to widen locking or add checks after the fact, but:

  • The race happens between looking up a series and locking it for append. GC can remove the series in this window.
  • Widening the lock scope is unsafe: appenders may never call Commit()/Rollback(), so holding the lock would block GC indefinitely.
  • The fix must not introduce new deadlocks or break the performance-sensitive hot path.

The approach

1. Mark series as garbage-collected (gced) when GC unlinks them.

diff --git a/tsdb/head.go b/tsdb/head.go
@@ -2770,6 +2775,11 @@ type memSeries struct {
 	pendingCommit                    bool  // Whether there are samples waiting to be committed to this series.
+	// Whether garbage collection has removed this series from the head index. Such a
+	// series is unreachable, so appending to it would silently lose the samples;
+	// appenders that looked it up before it was collected have to get a fresh one
+	// instead. See headAppenderBase.lockForAppend.
+	gced bool

Annotation:
A new gced boolean flag is added to the memSeries struct. This flag is set to true when the series is unlinked by GC, indicating that it is no longer reachable.

2. Set gced in both series GC paths, under the series lock.

diff --git a/tsdb/head.go b/tsdb/head.go
@@ -2375,6 +2378,7 @@ func (s *stripeSeries) gc(mint int64, minOOOMmapRef chunks.ChunkDiskMapperRef) (
 		deleted[storage.SeriesRef(series.ref)] = struct{}{}
+		series.gced = true
 		series.lset.Range(func(l labels.Label) { affected[l] = struct{}{} })
 		s.hashes[hashShard].del(hash, series.ref)
 		delete(s.series[stripe], series.ref)
@@ -2573,6 +2577,7 @@ func (s *stripeSeries) gcSeries(seriesRefs []storage.SeriesRef, maxt int64, shou
 		deleted[storage.SeriesRef(series.ref)] = struct{}{}
+		series.gced = true
 		stale, isHist, buckets := series.sampleState()

Annotation:
Whenever a series is deleted by GC (in both bulk and selected-series paths), its gced flag is set while holding the series lock. This ensures that any concurrent appender will see the flag if it races with GC.

3. Add a helper to lock a series for append, retrying if it was GCed.

+// lockForAppend locks s and returns it, ready to be marked as pending commit.
+//
+// Looking a series up only holds the stripe lock for the duration of the lookup, so
+// garbage collection can remove the series from the head index before the appender gets
+// to lock it and mark it as pending commit. Appending to such a series succeeds but the
+// samples are unreachable from the head, i.e. they are silently lost. When that
+// happened, a live series for the same labels is created and returned locked instead,
+// which is the same outcome as garbage collection winning the race outright.
+func (a *headAppenderBase) lockForAppend(s *memSeries) (*memSeries, error) {
+	for {
+		s.Lock()
+		if !s.gced {
+			return s, nil
+		}
+		lset := s.lset // Safe to access; the series is locked.
+		s.Unlock()
+
+		var err error
+		if s, _, err = a.getOrCreate(lset); err != nil {
+			return nil, err
+		}
+	}
+}

Annotation:
This method locks the series, checks if it was GCed, and if so, unlocks and gets (or creates) a fresh live series for the same label set. This is retried until a non-GCed series is found, ensuring appends never go to an unreachable series.

4. Use the helper in all append paths.

For example, in Append:

-	s.Lock()
+	s, err := a.lockForAppend(s)
+	if err != nil {
+		return 0, err
+	}

Annotation:
All code paths that previously did s.Lock() before appending now use lockForAppend, ensuring they never append to a GCed series.

5. Add a test that reliably triggers the race.

+// TestCompactSelectedSeries_RetriesAfterSeriesEvicted verifies that an appender
+// which resolved a series just before eviction unlinked it retries against a live
+// series instead of appending into the evicted one.

Annotation:
The new test uses a hook to pause the appender after it looks up the series but before it locks it, then triggers GC, and ensures that the append is retried against a live series and not lost.

From the review thread

bboreham: Looks like this Prombench run maybe got forgotten so I canceled it.
Broadly the CPU difference was small enough to ignore, although it did bounce to a 20%+ penalty at times, including a steady period over the last 30 hours.

locker95: yeah, sorry about leaving that bench running. good to hear the cpu delta isn't a blocker — I'll use -short locally next time.

Takeaways

  • Never trust a pointer to a mutable data structure across concurrent GC or mutation without re-validation.
    If a data structure can be concurrently removed, always check its liveness before acting on it.

  • Use minimal, targeted flags and checks to resolve concurrency races instead of widening locks.
    Avoid holding locks across user-controlled or long-running operations; instead, use state flags to detect races.

  • Write targeted tests that reliably trigger concurrency races using hooks or synchronization points.
    Testing concurrency bugs often requires explicit hooks or controlled pauses to reproduce rare races.