fix(tests): the two sanitizer defects actually red on main (#395, #396) - #399
Conversation
Both lanes of `sanitize-cpu` are red on `main` at `0eb049f7`, and neither failure is one of the six #274 lists -- those were #301 and #321 and both landed. A full local run of the CI recipe at that SHA finds exactly one failure per lane: address,undefined: 1/384 test_punica_cpu thread: 1/384 test_lmcache_client #395 -- `test_punica_cpu` heap-buffer-overflow, ASan: READ of size 4 ... 0 bytes after 144-byte region #0 RefShrink tests/vllm/lora/test_punica_cpu.cpp:63 #1 DOCTEST_ANON_FUNC_13 tests/vllm/lora/test_punica_cpu.cpp:312 The case feeds `idx = {1, -1, 0, num_slots}` on purpose, to prove the kernel skips an out-of-range slot. Production `AddShrink` does exactly that (`punica_cpu.cpp:128`, `slot < 0 || slot >= num_slots`). The test's own reference helper only guarded `s < 0`, so it read `a[36..53]` out of a 36-element adapter while computing a value the checking loop then discards. Guard the reference the same way the kernel guards, deriving `num_slots` from `a`'s extent since it is not a parameter. New since the last green main sanitizer run: `03b615eb` (#278) added the case and is not an ancestor of `dbd0d51c`, which was green. #396 -- `MockLmcacheServer` data race, TSan: Write of size 4 by main thread: ~MockLmcacheServer ...:76 Previous read of size 4 by T1: Run ...:206 `stop_` was made atomic; `listen_fd_` was not. The destructor closes and reassigns it BEFORE joining the accept thread, so the write races that thread's read of the same plain `int`. Intermittent by construction, which is why CI sees it on the connector on one PR and the client on another -- the mock is duplicated in both TUs and both carry the defect. Publish the descriptor once the socket is ready, hand it over with `exchange(-1)`, and read it through `load()`; fixed at both sites. RED evidence, at `0eb049f7`: - full ASan/UBSan ctest, CI recipe: `99% tests passed, 1 tests failed out of 384` -- `test_punica_cpu`, report byte-identical to hosted job 93821547189 - full TSan ctest: `99% tests passed, 1 tests failed out of 384` -- `test_lmcache_client`, same race the hosted job 93616755110 reported against the connector GREEN after: - ASan/UBSan focused: `test_punica_cpu`, `test_lmcache_client`, `test_lmcache_connector` -- 3/3 - TSan focused, 10 consecutive runs: 3/3 each time - `-Wall -Wextra` clean in both lanes, so the plain lane's `-Werror` holds Mutation, to prove the punica case still catches what it exists to catch: dropping `slot >= num_slots` from production `AddShrink` moves the ASan report from the test helper to `punica_cpu.cpp:129` and the binary aborts. Tree restored byte-for-byte. No suppression file, no sanitizer flag change, no expected-to-fail marking. Refs #274. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode]
FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode]
…epted fd residual, and #400 Fresh review at `da543d83` returned PASS with three low, non-blocking findings. Two of them are rules this branch was actually breaking, so they are answered here rather than deferred; the third is a code comment. 1. AGENTS.md wants an issue linked in three places that agree -- the roadmap issue table, the PR body, and the ROW'S SPEC. #395 and #396 had the first two and not the third. Added to `specs/lora-adapter.md` and `specs/lmcache-cpp-client-connector.md`, each stating what the bug actually was, which is what `.agents/bugfixing.md` asks the spec to carry. 2. Mutating `BgmvShrink`'s `s >= num_slots` guard away leaves `test_punica_cpu` GREEN -- 8 cases / 149 assertions / rc=0. So does the same guard in `BgmvExpandSlice`. The `bgmv_shrink` case feeds `idx = {0,1,2,-1,1,0}` with `num_slots = 3`: no index is ever out of range, so the upper half of those guards has no test at all. PRE-EXISTING since `1d37f152`, not introduced here, and a different fixture change from this branch's scope -- filed as #400 with the one-line close, and recorded in the roadmap table. 3. The reviewer asked whether `exchange(-1)` then `close(fd)` can race a stale `load()` in `Run()`. It cannot reach a reused descriptor in this mock: only two threads exist, the destructor opens nothing and joins before returning, so the worst case is `EBADF`, which is the loop's intended exit. Named as an accepted residual in a comment at both sites, including why the obvious alternative is wrong -- deferring the `close()` until after the join does not wake `accept()` on Darwin, which these files explicitly target. Re-verified after the edits: ASan/UBSan 2/2, TSan 5 consecutive runs 2/2 each, zero `-Wall -Wextra` warnings in both lanes, `agent-preflight.sh --staged` all gates green. Refs #274. Refs #395. Refs #396. Refs #400. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode]
|
Fresh review at Reviewer evidence worth keeping in the PR record (their runs, not mine):
Findings and what
Re-verified after the edits: ASan/UBSan 2/2, TSan 5 consecutive runs 2/2, zero warnings, |
|
Hosted CI green at Everything else on the run is green too: That closes the loop on #274 finding #1: the lane was red on Not merging — merge authority is not recorded in developer preferences and this is a helper claim. Ready for an operator to rerun the gate and land. |
Brings in #407 (Marlin gencode), #399 (sanitizer defects), #391 (CPU decode barrier) and the SPEC-DSPARK correction. No overlap with this branch: none of them touch .github/workflows/, scripts/main-baseline.py or scripts/agent-preflight.sh. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode]
Summary
Triage of the sanitizer half of #274, plus the two defects that triage actually
found. Both sanitizer lanes are red on
main; neither failure is one of the six#274 lists.
The six in #274 were #301 (five misaligned BF16 reads from borrowed
safetensors mmap) and #321 (
AsyncLLMoutput thread using a freedPromRegistry). Both landed in PR #344 (ea4deb20). A full local run of the CIrecipe at
main0eb049f7confirms all six are green and finds exactly onefailure per lane instead:
0eb049f7address,undefined99% tests passed, 1 tests failed out of 384—test_punica_cputhread99% tests passed, 1 tests failed out of 384—test_lmcache_clientCloses #395.
Closes #396.
#395 —
test_punica_cpureads pasta_stackedThe case
add_shrink leaves a -1 slot's buffer row UNTOUCHED, not zeroedfeedsidx = {1, -1, 0, num_slots}deliberately, to prove the kernel skips anout-of-range slot. Production
AddShrinkdoes exactly that(
src/vllm/lora/punica_cpu.cpp:128). The test's own reference helper guardedonly
s < 0, so it reada[36..53]out of a 36-element adapter to compute avalue the checking loop then discards. The reference now skips what the kernel
skips.
New since the last green
mainsanitizer run:03b615eb(#278) added the caseand is not an ancestor of
dbd0d51c, which was green (run 31493332021).#396 —
MockLmcacheServerraces its own accept threadstop_was made atomic;listen_fd_was not. The destructor closes andreassigns it before joining, so that write races the accept thread's read of
the same plain
int. The mock is duplicated intest_lmcache_client.cppandtest_lmcache_connector.cppand both carry it — which is why CI reported theconnector on #311 and the local suite reported the client. Both fixed: publish
the descriptor once the socket is ready, hand it over with
exchange(-1), readit through
load().Verification
RED, at
0eb049f7, CI recipe (-DVLLM_CPP_BUILD_TESTS=ON -DVLLM_CPP_CUDA=OFF -DVLLM_CPP_SANITIZE=…,VT_POOL_BYPASS=1, serialctest):test_punica_cpu, report byte-identicalto hosted job 93821547189
test_lmcache_client, same race hosted job93616755110 reported against the connector
GREEN, re-verified after merging
origin/main0b7fbff7:-Wall -Wextraclean in both lanes, so the plain lane's-Werrorholdsscripts/agent-preflight.shand--staged: all gates greenMutation, to show the punica case still catches its defect: dropping
slot >= num_slotsfrom productionAddShrinkmoves the ASan report from thetest helper to
punica_cpu.cpp:129and the binary aborts. Tree restoredbyte-for-byte.
No suppression file, no sanitizer flag change, no expected-to-fail marking.
Notes for #274
.github/workflows/deliberately untouched — the CI half of main is not verified by its own CI: every run is cancelled, and 5 tests fail under ASan/UBSan #274 is beingworked concurrently.
PromRegistry::SeriesForreturning a reference into a reallocatingvector) is still OPEN but its fix
2efb5680is an ancestor ofmain;test_llm_engineis green on both lanes here. It looks closeable.