Run the test suite under ASan+UBSan, and fix the clean that hid it

[ZacharyZcR]

Depends on #835. Verifying this PR turned up a tracked build artifact — c/tests/bench_omp_grain, a 16,880-byte ELF committed by my own #808. Once clean.py correctly removes extensionless binaries, make clean would delete a tracked file and leave every contributor with a dirty tree. #835 untracks it and closes the bench_*/fuzz_* gap in .gitignore. Merge that first; this is harmless after it.

make clean removes nothing on Linux and macOS

$ ls tests/test_* | grep -v '\.' | wc -l
40
$ make clean
clean: removed 0 files/dirs

tools/clean.py globs "tests/test_*.exe". Its own comment says it means "(.exe on Windows, no extension on Unix)" — only the Windows half was ever implemented. The engine binaries (colibri, inkling, kimi_k3) are not in the removal list at all either; it still lists glm, the pre-rename name.

This silently invalidates verification. Change a compile flag, run make clean && make test-c, and the stale binaries built with the old flags are re-run and reported as passing. CONTRIBUTING's make check opens with exactly that sequence.

It is also how this PR nearly shipped as a lie. My first sanitizer run came back clean — because clean.py had left the previous, unsanitized binaries in place and make had nothing to rebuild. nm tests/test_topp | grep asan was empty. The "zero findings" was the old binaries passing again.

Nothing in this repo has ever been sanitized in CI

fuzz-rans builds with -fsanitize=address,undefined and has since it was written — but it is not wired to any job, and it covers rans.h alone. The engine's 9k lines of pointer arithmetic, its mmap/pread paths and its worker threads have never been run under ASan here.

What this adds

• tools/clean.py removes Unix test binaries and the four engines. KEEP_EXT is the safety rail — a file with a source extension can never match, directories (tests/fixtures/) are skipped — because the failure mode of getting this wrong is deleting tracked sources. Rebased on dev after Add DeepSeek V4 target-only CPU inference #165: ARTIFACT_GLOBS now carries COLI_V4_UNIT_*.o alongside the test globs, and BINARY_EXTENSIONS is gone — .o is absent from KEEP_EXT, so the existing rail already permits removing it. One allowlist, not two facing opposite directions.
• EXTRA_CFLAGS / EXTRA_LDFLAGS, appended to every platform's flags. The obvious way to build with sanitizers, make CFLAGS=..., replaces the platform's flags and quietly drops its OpenMP and -march settings — a second way to end up with a run that is not what it claims to be. This is also the supported way to pass any one-off flag: make colibri EXTRA_CFLAGS=-DFOO.
• make -C c test-asan — clean, rebuild the suite with -fsanitize=address,undefined -O1 -g, run it.
• CI sanitizers job — that, plus make fuzz-rans.

On leak detection being off

It is off deliberately, and this is the one judgement call worth challenging.

The engines allocate the tensor index, the parsed config and the weight slabs once and use them until exit. st.h states the intent directly: "intentionally leaked ... one-time startup parsing". LeakSanitizer reports 9 such sites (json_parse, st_init, load_cfg, qalloc). Reporting them would bury the findings that matter — a heap-buffer-overflow, a use-after-free — under noise nobody intends to fix, and a noisy gate gets ignored.

ASan's memory-error checks and all of UBSan stay on. Those are what catch defects. Flip ASAN_OPTIONS in the test-asan target if you want the leak reports.

Verification, both directions

A sanitizer job that cannot fail is worse than no job, so I checked it can. With a deliberate 4-int buffer written at index 9 in tests/test_topp.c:

	result
make test-asan	FAILED: tests/test_topp
make test-c	passes, silently

And with the tree as-is:

• make test-asan — suite clean under ASan + UBSan (binaries confirmed linked against libasan, not assumed)
• make fuzz-rans — parsed_ok=1334 refused=3573 decoded_ok=2662 decode_refused=1331
• make clean — 43 artifacts removed, 0 tracked sources touched, tests/fixtures/ intact
• Normal builds unaffected: four engines warning-free, make test-c green

Cost

The sanitizer job is a full rebuild at -O1 plus a sanitized run of the suite — a few minutes, on its own job so it does not hold up the fast feedback.

Not covered

GPU paths (CUDA/Metal/Vulkan) are compile-checked only; sanitizing them needs real hardware, which CI does not have. test-python is not run under sanitizers here — it drives the engine binaries, so it would work, but it is a much longer run and belongs in a separate decision about CI budget.

[JustVugg]

Holding this one, and it needs a decision from you rather than a rebase from me.

Why it is held: your description opens with Draft. The GitHub draft flag is off, so the PR reads as ready from the outside — I nearly took it. If Draft. in the body is your signal, please also set the flag, because that is the one the merge queue and I both look at.

Why it now conflicts: #165 (DeepSeek V4) landed on dev and it also rewrote tools/clean.py. I resolved the additive conflicts from that merge on several other PRs myself, but not this one, because the two changes are not additive — they are two different designs for the same function:

# this PR
TEST_GLOBS = ["tests/test_*", "tests/bench_*", "tests/fuzz_*"]
KEEP_EXT   = (".c", ".h", ".cc", ".cpp", ".cu", ".mm", ".py", ...)   # allowlist of what to KEEP

# dev, after #165
ARTIFACT_GLOBS    = ["tests/test_*", "COLI_V4_UNIT_*.o"]
BINARY_EXTENSIONS = {"", ".exe", ".o"}                               # allowlist of what to REMOVE

Picking either side loses something real: yours drops the COLI_V4_UNIT_*.o cleanup, dev's drops bench_*/fuzz_* and the KEEP_EXT rail. I am not going to guess which safety property you want on top — you wrote the argument for the rail and it is a good one.

For what it is worth, the synthesis looks clean: keep your structure and add COLI_V4_UNIT_*.o to TEST_GLOBS. .o is not in KEEP_EXT, so the rail already permits removing it. But that is your call to make, not mine.

Separately, on the finding itself: the sanitizer story here is the part I would not want lost. That your first ASan run came back clean because clean.py had left the unsanitized binaries in place is the strongest argument in the PR, and it is an argument about verification, not tidiness. make check opening with make clean && make test-c means every contributor has been in a position to report stale results. Worth keeping in the description when you rework this.

One more thing you should know: #804 and #806 also opened with Draft., and I merged both before I noticed the convention. I judged them on the merits and I stand behind both — #806 in particular is the best-validated change in the queue after @ThefloorMiner ran it on GLM-5.2 744B across three runs per side with byte-identical output, and #804 is twenty lines pointing the tests at the engine's real name. dev is green with both in. But if either was genuinely unfinished, say so and I will revert it; that is my mistake to undo, not yours to work around.

[ZacharyZcR]

Answering the revert question first, because you should not have to hold that open.

#804 and #806 are finished. Please do not revert either.

Both were complete when I opened them, and I stand behind them as merged. Draft. in those bodies was my sloppiness with a word, not a signal about the code — and #806 in particular had @ThefloorMiner's 744B validation on it before you took it, which is more evidence than most of my PRs carry.

The convention is yours and I will follow it: GitHub's draft flag is the only signal, and I will stop writing Draft. in bodies entirely. It was doing nothing except creating exactly this ambiguity. My apologies for the hour you spent worrying about two merges that were fine.

clean.py — taking your synthesis, and it collapses further than you suggested

You were right that the two sides are different designs, and right about which way it resolves. Pushed, rebased on current dev:

ARTIFACT_GLOBS = ["tests/test_*", "tests/bench_*", "tests/fuzz_*",
                  "COLI_V4_UNIT_*.o"]
KEEP_EXT = (".c", ".h", ".cc", ".cpp", ".cu", ".mm", ".py", ".txt", ".json",
            ".md", ".bin", ".sh", ".toml", ".yml", ".yaml")

Your read of the rail was exactly right: .o is not in KEEP_EXT, so COLI_V4_UNIT_*.o needs no second mechanism — it falls out of the rule already there. BINARY_EXTENSIONS disappears rather than coexisting with KEEP_EXT, which I think is the better outcome: one allowlist, not two facing opposite directions. FILES and DIRS are the union of both sides — dev gained the hip/deepseek_v4/native_quant entries and build/ownership, mine had inkling/kimi_k3. The glob list is renamed ARTIFACT_GLOBS since it is no longer only tests.

Verified rather than asserted: 83 source files under c/tests/ survive clean.py, and planted tests/test_*, tests/bench_* and COLI_V4_UNIT_*.o binaries are all removed. .github/workflows/ci.yml and c/Makefile also conflicted (both additive — your docker job and bench-omp-grain alongside my sanitizers job and test-asan); both sides kept, and test-asan got its own .PHONY line per the convention dev now uses.

Verifying it turned up something, and it is mine

Running the fixed clean.py against a clean checkout deleted a tracked file:

D c/tests/bench_omp_grain

c/tests/bench_omp_grain is a 16,880-byte ELF executable, mode 100755, added by 00129d2 — #808, mine. It is the only tracked build artifact under c/tests/. .gitignore guards c/tests/test_* with source exceptions but that pattern was never extended to bench_* or fuzz_*, which is the gap it walked through.

This is a blocker for this PR and I have filed it separately as #835 (untrack the binary, close the bench_*/fuzz_* gap in .gitignore). If #801 lands while that binary is tracked, the first make clean hands every contributor a dirty tree and the cause looks like it is here. #835 should go first; this one is harmless after it.

It is worth noting what happened here, since it is the same argument as the PR: the reason nobody caught the committed binary is that make clean never removed extensionless binaries, so it never showed up as deleted. A broken clean does not just hide stale test results — it hid this too.

On the finding itself

Thank you for saying the sanitizer argument is the part worth keeping. It is the part I care about: the first ASan run came back clean because clean.py had left unsanitized binaries in place, and make check opens with make clean && make test-c, so every contributor has been in a position to report results from binaries built with the wrong flags. I have rewritten the description to lead with that rather than with the tidiness framing — and without the word Draft. at the top.

[ZacharyZcR]

The blocker on this one is gone — you merged it yourself.

#835 landed in 2e45ad3, so c/tests/bench_omp_grain is no longer tracked:

c/tests/bench_omp_grain | Bin 16880 -> 0 bytes

That was the only reason this PR was unsafe to merge. With the binary untracked, the fixed clean.py removes build artifacts and nothing else — no contributor ends up with a deleted tracked file after make clean. Flagging it because you held this for a reason that has since been resolved by a different PR, and that is easy to lose track of across a queue this size.

Current state: rebased on dev, 17/17 checks green, MERGEABLE. The clean.py synthesis is the one you proposed — ARTIFACT_GLOBS carrying COLI_V4_UNIT_*.o alongside the test globs, BINARY_EXTENSIONS dropped since .o is absent from KEEP_EXT and the existing rail already permits removing it. FILES and DIRS are the union of both sides.

Verified rather than asserted: 83 source files under c/tests/ survive, planted tests/test_*, tests/bench_* and COLI_V4_UNIT_*.o binaries are all removed, make test-c green.

No rush from my side — just did not want it sitting on a condition that no longer holds.