PromRegistry::SeriesFor returns a reference into a vector it reallocates: sanitizer lanes are RED on main

[localai-bot]

Both sanitizer lanes are red on main, not on any one PR

Reproduced on pristine origin/main d06a3792 with no other commits applied:

cmake -S . -B build-sanitize -G Ninja -DVLLM_CPP_BUILD_TESTS=ON \
  -DVLLM_CPP_CUDA=OFF -DVLLM_CPP_SANITIZE=thread
cmake --build build-sanitize --target test_llm_engine
setarch $(uname -m) -R ./build-sanitize/tests/test_llm_engine    # exit 66

(setarch -R is needed locally: TSan otherwise aborts with unexpected memory mapping before running a single test.)

WARNING: ThreadSanitizer: data race
    #8 PromRegistry::SeriesFor(...)  src/vllm/v1/metrics/prometheus.cpp:134
    #9 PromRegistry::SetGauge(...)   src/vllm/v1/metrics/prometheus.cpp:165
SUMMARY: ThreadSanitizer: data race in operator delete(void*, unsigned long)

CI agrees: on PR #307 the ASan lane fails six tests — test_load_direct_upload, test_llama_embedding_fold, test_laguna_nvfp4_loader, test_llm_engine, test_openai_api_server, test_capi — with heap-use-after-free.

Root cause

SeriesFor hands out a reference into a std::vector<Series> and then grows that same vector:

PromRegistry::Series& PromRegistry::SeriesFor(Family& fam, const std::vector<std::string>& labelvalues) {
  for (auto& s : fam.series) {
    if (s.labelvalues == labelvalues) return s;
  }
  ...
  fam.series.push_back(std::move(s));   // reallocates: every previously returned Series& dangles
  return fam.series.back();
}

std::vector reallocation invalidates all references to its elements, so any Series& obtained before a later SeriesFor call on the same family is dangling — that is the heap-use-after-free. Callers (IncCounter, SetGauge, Observe, Prime) all go through it.

Separately, PromRegistry contains no mutex or atomic at all (grep -c 'mutex|lock_guard|atomic' = 0 in both the header and the .cpp), while /metrics is now served concurrently with the engine's own recording. A scrape iterating fam.series while a writer push_backs is a data race on the container itself, independent of the dangling-reference bug.

Suggested direction

Stability of references is the actual requirement, so a container that provides it (std::deque, or std::vector<std::unique_ptr<Series>>, or returning an index rather than a reference), plus a lock or per-series atomics around the concurrent recording path. Not attempted here — this belongs to the metrics row, not the one that found it.

Provenance

Bisected by elimination, not assumed: PR #307's spec-only commit c3db7804 passed both sanitizer lanes; the failures appear only after merges that pulled in 9c55e3df ("wire /metrics to the engine the server actually serves from", #277), and then reproduce on pristine main with none of #307 applied. Found while gating container images (#170).

[localai-bot]

Correction: my root cause above is NOT demonstrated, and my proposed regression test does not guard it

I tried to add a regression test and it failed its RED proof, which invalidates part of what I wrote when filing this.

What I tried. Two cases asserting the property the fix relies on: hold a Series& (and a Family&), force 256 later insertions, then read through the earlier reference.

What happened. With std::deque they pass. With std::vector restored — the actual bug — they still pass, under -fsanitize=address, with zero sanitizer hits.

Why. SeriesFor re-looks-up by label value on every call:

for (auto& s : fam.series) {
  if (s.labelvalues == labelvalues) return s;
}

so IncCounter/SetGauge/Observe find-or-create and use the reference immediately. No caller holds one across a growth. Reallocation moves the elements, but the values survive the move, so a single-threaded test through the public API sees correct output either way. My test asserted a property the API never exercises. I deleted it rather than ship a guard that guards nothing.

So the mechanism in the title of this issue is unproven. "Returns a reference into a vector it reallocates" is a real hazard in the abstract, but it is not what I demonstrated. The likelier mechanism is concurrent: reallocation moving elements while another thread walks them. Note PrometheusStatLogger DOES take a mutex on Record/Expose/SetCacheConfigInfo (loggers.h:76) — my original "no mutex or atomic at all" was wrong, and I retract it. What is not synchronized is the registry() accessor, documented as such.

What IS established, measured on one box

tree	lane	result
pristine main d06a3792 (pre-fix)	TSan, test_llm_engine	exit 66, data race in SeriesFor / SetGauge
main with 2efb5680 (deque)	TSan, test_llm_engine	23 cases / 452 assertions, Status: SUCCESS, exit 0

(setarch -R is required locally or TSan aborts with unexpected memory mapping before running anything — an abort that is easy to misread as a pass.)

So the landed fix is effective against the reproduction that started this. The gap it leaves is that 2efb5680 changed only the header, so nothing in the suite would notice a revert to std::vector — and as shown above, a naive single-threaded test cannot supply that guard. A real one needs the concurrent pattern, and would have to respect the documented contract that PromRegistry itself is not thread-safe. I have not written it.

Leaving this open for the metrics row with that narrowed scope: the fix stands, the guard is still owed, and the exact mechanism deserves a better diagnosis than the one I filed.

[localai-bot]

Verified on pinned main 0eb049f7: the fix for this (2efb5680, fix(metrics): PromRegistry handed out references into reallocating vectors (#330)) is an ancestor of main, and test_llm_engine is green on both sanitizer lanes in full local runs of the CI recipe:

address,undefined : 99% tests passed, 1 tests failed out of 384   (test_punica_cpu, #395)
thread            : 99% tests passed, 1 tests failed out of 384   (test_lmcache_client, #396)

Neither remaining failure touches PromRegistry. This looks closeable.

One correction for whoever reads this later: the body describes all six ASan failures as heap-use-after-free. Reading the actual reports in job 93622943433, five were UBSan load of misaligned address from the borrowed-mmap BF16 path (#301, fixed by ea4deb20) and only test_llm_engine was a use-after-free (#321, same commit). Full triage in #274.