maintenance: make VictoriaMetrics flush recovery durable

[zqr10159]

Summary

• retain an unsuccessful VictoriaMetrics batch and retry it before draining later data
• recheck the queue after every immediate flush so writes added during an in-flight flush are scheduled
• when a periodic flush fails, schedule the retained batch on the one-second recovery path while keeping exactly one normal periodic chain
• reschedule the periodic chain from finally, and clear the pending guard when timer scheduling is rejected
• keep buffering bounded by applying producer backpressure during an outage instead of silently discarding data
• stop scheduling during shutdown, make one final synchronous drain, and reject later writes explicitly

Regression proof

The result-oriented contracts were run against the previous implementation first. They demonstrated that:

• only the first of two refilled batches was written
• a failed periodic write using the production-length 3600-second interval was attempted only once during a four-second observation window
• shutdown wrote none of the buffered metrics

The updated implementation proves that all refilled batches reach the HTTP boundary, the failed periodic batch is retried byte-for-byte on the one-second recovery path without creating a second periodic chain, and shutdown drains the queue once before rejecting later writes.

Validation

• ./mvnw -pl hertzbeat-warehouse -Dtest=VictoriaMetricsClusterDataStorageTest#retriesPeriodicFlushFailuresQuicklyWhenTheConfiguredIntervalIsLong test -DskipITs -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false — passed with flushInterval=3600
• ./mvnw -pl hertzbeat-warehouse -Dtest=VictoriaMetricsClusterDataStorageTest test -DskipITs -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false — 3 tests passed
• ./mvnw -pl hertzbeat-warehouse -am test -DskipITs -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false — reactor passed; common-core 584, common-spring 102, warehouse 62 tests
• touched-file CJK scan and git diff --check — passed

Operational impact

No configuration migration is required. A periodic failure no longer waits for the full configured interval: retained data enters the bounded one-second retry path. During a sustained VictoriaMetrics outage, the queue applies backpressure to writers after one failed batch is retained. Shutdown performs a best-effort final flush and logs the retained item count if the destination still rejects it.

AI assistance: used for draft implementation and test iteration.
Human validation: executed the focused long-interval failure/recovery contract and the full warehouse reactor listed above.
Risk notes: recovery remains in-memory; a process crash cannot preserve buffered metrics, and prolonged destination outages can slow warehouse producers through deliberate backpressure.

[zqr10159]

Author remediation update:

Immediate flush completion now clears its guard in finally, rechecks the queue, and schedules the next batch when refill raced with the active flush. Failed HTTP batches remain queued and retry before later data, periodic scheduling is restored from finally while the timer is usable, rejected scheduling cannot leave the pending flag stuck, and shutdown performs a bounded best-effort drain with an observable retained count.

Refill-race, 503 retry and requeue, and shutdown-drain contracts passed; the full warehouse reactor also passed (62 tests). License and label checks are green; backend jobs are still running at the time of this update. Maintainer review remains required.

[zqr10159]

CI follow-up: backend build, Maven E2E, image E2E, license, and label checks have all completed successfully on the current head.

[zqr10159]

Addressed the long-interval periodic retry gap in the latest head (419e0e8658). A failed periodic flush now keeps the normal periodic chain and also schedules the retained batch through the bounded one-second immediate-recovery path. The regression uses flushInterval=3600; it failed on the previous implementation because only one HTTP attempt occurred in four seconds, and now proves a byte-identical second attempt. The focused class (3 tests) and the full warehouse reactor (584 common-core, 102 common-spring, 62 warehouse tests) pass.

[Duansg]

Thanks for this — the problems you're fixing are real, and I confirmed all three against master:

• MetricsFlushTask.run() drains a batch out of the queue and calls doSaveData(), which swallows failures internally, so a failed batch is silently lost;
• the overflow path calls doSaveData(contentList) — the whole list, once per item that failed to be offered — producing duplicate writes;
• the reschedule sits inside the try, so an exception escaping the flush kills the periodic chain permanently.

All worth fixing. But I think the new backpressure model is a blocking regression.

Unbounded backpressure freezes the persistence pipeline

sendVictoriaMetrics() now loops with no exit condition other than success or shutdown:

while (!offered) {
synchronized (metricsFlushLock) { offered = metricsBufferQueue.offer(content); }
if (offered) break;
triggerImmediateFlush();
Thread.sleep(MAX_WAIT_MS); // no bound, no attempt limit
}

saveData() runs on the single warehouse-persistent-data-storage thread (DataStorageDispatch.java:74-91), which also drives:

• calculateMonitorStatus() (line 86) — monitor up/down state
• pluginRunner.pluginExecute(PostCollectPlugin.class, ...) (line 88)
• realTimeDataWriter.saveData() (line 90) — real-time store backing the dashboard

If VictoriaMetrics is rejecting writes and the buffer fills, that thread blocks indefinitely and all four stop. metricsDataToStorageQueue is an unbounded LinkedBlockingQueue (InMemoryCommonDataQueue.java:58), so the backlog grows until the heap is exhausted.

To be precise about the blast radius: threshold alerting is not immediately affected — MetricsRealTimeAlertCalculator.java:132 consumes pollMetricsDataToAlerter(), a separate queue and thread. But once the storage queue exhausts the heap, that goes down with everything else.

The existing availability check does not save you here. checkVictoriaMetricsDatasourceAvailable() probes vmselect (VictoriaMetricsClusterDataStorage.java:147, vmClusterProps.select().url()), while this PR is about vminsert write failures. With vminsert down and vmselect healthy, serverAvailable stays true and execution falls straight into the loop above.

Trading bounded data loss for an unbounded stall is a bad trade for a monitoring system — a history-store outage should degrade history, not freeze status calculation and real-time writes.

Suggestion: either keep a bounded retry (N attempts / a total time budget) and then drop with a counter or metric, or move the VM write path onto its own executor so backpressure cannot propagate back into the shared consumer thread.

The same defects remain in the single-node storage

VictoriaMetricsDataStorage has essentially the same code and is untouched by this PR:

• sendVictoriaMetrics() at lines 572-612 still has the doSaveData(contentList) overflow path that re-sends the entire batch;
• MetricsFlushTask.run() at lines 627-651 still calls triggerIntervalFlushTimer() inside the try, so the chain can still die, and a drained batch is still lost on failure.

Since the single-node deployment is the more common one, fixing only the cluster variant leaves most users exposed. Worth either covering both here or extracting the shared flush logic.

Minor

Replacing log.error("... failed. {}", responseEntity.getBody()) with status-only logging removes the main diagnostic for write rejections (bad label, out of disk, retention policy). Keeping the body at DEBUG would preserve troubleshooting without the noise.

[zqr10159]

Addressed the review findings in commit ab4fc80bd0.

Changes:

• both VictoriaMetrics storage modes retain failed batches and retry them after one second;
• periodic scheduling is restored from finally after failures;
• immediate flushes drain bounded bursts per callback and reschedule while work remains;
• producer backpressure is bounded when a failed retry batch and the queue remain full, with cumulative, rate-limited drop diagnostics rather than an infinite wait;
• timer resolution was reduced to 100 ms so healthy concurrent bursts do not stall behind one-second scheduling gaps.

Human validation:

• the existing 100-thread healthy-write regression passed;
• full VictoriaMetricsDataStorageTest,VictoriaMetricsClusterDataStorageTest suites passed, including failed-batch retry and persistent-failure non-blocking cases;
• git diff --check.

The new-head backend, Maven E2E, license, and label checks are currently queued by GitHub and have not started yet.

AI assistance: used for draft implementation and test iteration.
Risk notes: sustained downstream failure can still exhaust the bounded buffer; this is now an explicit counted drop instead of a warehouse-thread deadlock.

@Duansg, please re-review this head when convenient.

[Duansg]

Two follow-ups: the flush lock doesn't cover the HTTP write, and dropped metrics have no signal outside the log

Both apply to VictoriaMetricsClusterDataStorage and VictoriaMetricsDataStorage — the code is parallel in the two files.

Replacing the unbounded while (!offered) retry with a bounded offer is the right call, and pulling the standalone storage in alongside the cluster one closes the gap the earlier revision left. Two things I'd like to see addressed before this lands.

---

1. retryBatch is not claimed while it is being sent

flushBufferedMetrics() releases metricsFlushLock before the POST, and leaves retryBatch pointing at the batch for the whole duration of the write:

List<VictoriaMetricsContent> batch;
synchronized (metricsFlushLock) {
    if (retryBatch.isEmpty()) {
        List<VictoriaMetricsContent> nextBatch = new ArrayList<>(...);
        metricsBufferQueue.drainTo(nextBatch, ...);
        retryBatch = nextBatch;
    }
    batch = retryBatch;
}                                    // lock released here
if (batch.isEmpty()) return true;
if (!trySaveData(batch)) { ... return false; }   // HTTP POST, unsynchronised
synchronized (metricsFlushLock) {
    if (retryBatch == batch) retryBatch = Collections.emptyList();
}

There is no in-flight marker. A second caller entering this method during the POST sees retryBatch non-empty, takes the same list reference, and sends it again. The success path clears by reference identity, so the loser of the race simply doesn't clear — the samples have already gone twice. The critical section guards the drain and the clear, but not the state that actually matters.

This is latent rather than live today, and I want to be precise about why, because the reason isn't in this file:

• MetricsFlushTask and ImmediateMetricsFlushTask are both scheduled on the single metricsFlushTimer, and HashedWheelTimer invokes task.run(this) inline on its one worker thread (HashedWheelTimer.java:645, reached from expire() at line 722). They serialise on that thread.
• destroy() runs on the shutdown thread, but calls metricsFlushTimer.stop() before its drain loop, and stop() (lines 350-359) repeats workerThread.interrupt(); workerThread.join(100); until the worker is dead. So the shutdown flush cannot overlap an in-flight timer flush either.

So the mutual exclusion protecting retryBatch during the write comes from the timer's single-threaded execution model plus stop()'s join — not from metricsFlushLock. Nothing in flushBufferedMetrics() states or enforces that dependency, and it stops holding as soon as anyone supplies the timer with a task executor, adds a second flush trigger, or calls doSaveData — now public and delegating straight to trySaveData — from another thread.

Suggested fix, either is cheap:

• Claim the batch under the lock: set retryBatch = Collections.emptyList() while holding it, keep the reference locally, and restore it under the lock on failure; or
• Add an AtomicBoolean flushInFlight and return early when a flush is already running, in the same spirit as the existing immediateFlushPending.

Unrelated but worth noting alongside it: the retry path is at-least-once by construction — a POST that times out after VictoriaMetrics accepted the batch will be resent on the next flush. That's inherent to retrying and separate from the concurrency point above; I mention it only so the two aren't conflated when someone looks at duplicate samples.

---

2. Dropped metrics are effectively invisible to operators

After MAX_BUFFER_OFFER_ATTEMPTS (3) offers at MAX_WAIT_MS (500ms) each — roughly 1.5s — the producer gives up on the rest of the batch:

if (!offered) {
    recordDroppedMetrics(contentList.size() - index, "buffer remained full");
    return;
}

That discards every remaining item from index onward, permanently. Bounded loss instead of blocking the warehouse-persistent-data-storage thread is the right trade — that thread also drives calculateMonitorStatus(), realTimeDataWriter.saveData() and PostCollectPlugin, so blocking it was worse. But the loss needs to be visible, and right now it isn't:

• getDroppedMetricCount() is package-private and isn't wired into any metric, health indicator, or UI surface — nothing outside the tests reads it;
• the log itself is throttled to the first occurrence, powers of two, and multiples of 100 (total == count || (total & (total - 1)) == 0 || total % 100 == 0), so sustained loss produces sparse lines rather than steady evidence.

An operator whose VM insert endpoint is degraded will silently lose history with no indication in the product. Exposing the counter — as an actuator/Micrometer gauge, or through the existing storage-status endpoint that the monitor pages already poll — would make the trade-off honest.