fix: WAL replay hang when flush doesn't progress

[paul1r]

What this PR does / why we need it:
During WAL replay, the ingester uses a backpressure loop to throttle recovery when in-memory bytes exceed 90% of ReplayMemoryCeiling. If the flush never reduces memory — because the storage backend is slow, unavailable, or chunks haven't been marked as flushed yet — the loop had no exit condition and would spin forever. This caused ingester pods to hang at startup with no error logged and no corruption detected; the only visible signal was loki_ingester_wal_replay_active remaining stuck at 1.

replay_controller.go — WithBackPressure

• Guard against ReplayMemoryCeiling == 0: previously, a zero ceiling computed a threshold of 0, causing the loop to fire immediately on any non-zero byte count. Now backpressure is skipped entirely when the ceiling is unset.
• Detect no-progress flushes: track a monotonically increasing totalSubtracted counter in Sub(). Before each flush, snapshot the counter; if it hasn't increased after the flush completes, the flusher did no work and recovery returns a descriptive error:

WAL replay flush made no progress: 4.0 GB in use, ceiling 3.6 GB; cannot recover

Using totalSubtracted rather than a before/after comparison of currentBytes avoids a false positive: replay runs GOMAXPROCS workers concurrently, and other workers can legitimately call Add() while a flush is in progress, pushing currentBytes back above the pre-flush snapshot even when the flush genuinely freed memory.

Four new test cases:

TestReplayControllerZeroCeiling — verifies backpressure is bypassed and the flusher is never called when ReplayMemoryCeiling is zero
TestReplayControllerNoProgressFlush — verifies an error is returned when the flusher is a complete no-op
TestReplayControllerPartialProgressFlush — verifies an error is returned when the flusher makes partial progress on the first call but stalls on subsequent calls, which is the realistic scenario when a degraded backend drains some chunks but cannot keep up
TestReplayControllerNoSpuriousErrorOnConcurrentAdd — verifies that a concurrent Add() racing with a genuine flush does not cause a false no-progress error

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[DylanGuedes]

@claude

[claude]

Claude finished @DylanGuedes's task in 3m 41s —— View job

---

Code Review

• Read changed files
• Analyze code changes
• Post review feedback

---

The fix is well-reasoned and correctly addresses the hang. The totalSubtracted approach for progress detection is sound. Here are my observations:

replay_controller.go

Zero-ceiling guard (replay_controller.go:113-116):

ceiling := int(c.cfg.ReplayMemoryCeiling) * 9 / 10
if ceiling <= 0 {
    return fn()
}

The pre-computation is a nice cleanup (avoids recomputing every loop iteration). One subtlety: ReplayMemoryCeiling is flagext.ByteSize (a uint64). The int(...) cast was already pre-existing, but ceiling <= 0 is effectively only ceiling == 0 here (since a uint64 cast to int can't come back negative unless it overflows). If someone passes a value large enough to overflow int64 → int, ceiling would be negative, and backpressure would be silently skipped. This is an edge case but the comment could note it.

No-progress detection (replay_controller.go:119-127):

subtractedBefore := c.totalSubtracted.Load()
c.Flush()
if c.totalSubtracted.Load() == subtractedBefore {
    return fmt.Errorf("WAL replay flush made no progress: ...")
}

Design is correct. Using a monotonically increasing counter avoids the false positive from concurrent Add() calls pushing bytes back up after a genuine flush.

One thing worth noting: since replayController.Sub() is called from flush.go:395 (inside removeFlushedChunks, which is called from replayFlusher.Flush()), and Flush() is the only code path that calls Sub(), the "concurrent Sub masks a no-progress flush" concern doesn't apply in practice here. The counter accurately reflects flush progress.

Error message — minor UX nit: the error shows ceiling (which is 90% of the configured value) rather than the configured ReplayMemoryCeiling. For example, if a user sets --ingester.wal-replay-memory-ceiling=4GB, they'd see ceiling 3.6 GB in the error, which could be confusing when cross-referencing their config. Consider including the configured value:

return fmt.Errorf("WAL replay flush made no progress: %s in use, threshold %s (ceiling %s); cannot recover",
    humanize.Bytes(uint64(c.currentBytes.Load())),
    humanize.Bytes(uint64(ceiling)),
    humanize.Bytes(uint64(c.cfg.ReplayMemoryCeiling)),
)

No logging at detection point: When the no-progress condition fires, the function just returns an error without logging. This is fine if the caller (recovery.go:Series / recovery.go:Push) propagates it to the WAL replay machinery which logs it. Based on the PR description ("no error logged" was a symptom of the original bug), it's worth verifying that errors from Series() / Push() are visibly logged by the WAL replay engine, not silently dropped.

totalSubtracted field placement — The alignment comment at lines 44-49 says "the first word must be 64-bit aligned." Adding totalSubtracted as the second field is at offset 8 (after 8-byte currentBytes), which is always 8-byte aligned. Since both fields use go.uber.org/atomic.Int64 which handles 32-bit alignment internally, there's no issue, but the comment could mention totalSubtracted also relies on this guarantee.

replay_controller_test.go

All four new tests are well-structured and cover the key scenarios:

• TestReplayControllerZeroCeiling — clear and simple ✓
• TestReplayControllerNoProgressFlush — clean no-op flusher test ✓
• TestReplayControllerPartialProgressFlush — correctly models the realistic degraded backend case ✓
• TestReplayControllerNoSpuriousErrorOnConcurrentAdd — the most important regression test; the channel-based synchronization is correct ✓

One observation on TestReplayControllerNoSpuriousErrorOnConcurrentAdd: the test asserts require.NoError which holds regardless of whether the concurrent rc.Add(85) races with the no-progress check (because totalSubtracted is already > 0 from the genuine Sub(80) call). The comment description of step 4 correctly captures this. The test is not flaky. ✓

Summary

Overall this is a solid, targeted fix. The two most actionable items are:

1. Verify errors from Series()/Push() are visibly logged upstream (so the descriptive error message actually surfaces to operators)
2. (Optional) Make the error message include the configured ceiling value, not just the 90% threshold

[DylanGuedes]

lgtm!