fix(qwen36): three CUDA expert-tier bugs (#1339, #1340, #1341)

[crichalchemist]

Summary

Three pre-existing bugs in the Qwen3.6 CUDA expert tier, found while porting the tier to Vulkan (#1338) and filed as #1339, #1340, #1341. One commit per issue, each with a test that fails on the tree before its fix. The tests link qwen36_tier.c against a fake CUDA backend (tests/qwen36_fake_cuda.h, factored out of the existing test_qwen36_tier_int8.c), so they run in the plain CPU build and in CI.

• qwen36 CUDA tier: qt_issue overruns G.is_x with two or more GPUs (stride 8*D into a 32*D buffer) #1339 qt_issue strides each device's input block by 8*D floats into a buffer of 32*D floats. With two or more GPUs, a full 32-row issue on device 1 writes past the end of the allocation. Fix: allocate ndev*32*D floats and stride by 32*D. The new struct field is_x_floats exists so the test can assert against the real allocation rather than restate the arithmetic.
• qwen36 CUDA tier: qt_shutdown never signals cv_take, so pthread_join can hang if a group is open #1340 qt_shutdown signals cv but never cv_take, so the uploader parked on the LFRU victim wait (and qt_note_*/qt_fill_wait waiters) never notice th_stop and pthread_join hangs when a group is still open. Fix: broadcast cv_take too; the uploader abandons a swap it wakes into during shutdown instead of freeing a victim tensor an in-flight group may still reference.
• qwen36 CUDA tier: int8 containers free the weights the tier still points at (regression from #1334) #1341 After fix(qwen36): the VRAM tier now promotes int8 experts instead of reserving for nothing #1334, the warmstart hands the tier the live int8 weights and then frees them, assuming eviction rematerialises from g4. That only holds for int4; int8 has no second copy, so the tier's pointer dangles and the CPU fallback dereferences NULL. Fix: free the int8 copy only for int4 containers. The warmstart is extracted into tier_warmstart() so the engine-level test can drive it without main.

Notes for review: with int8 weights kept, the tier's slot pointer aliases a live engine slot; that is safe because qt_init requires cap == n_experts, so a planned slot is never recycled. The cv_take broadcast makes a benign post-shutdown enqueue reachable if a caller races qt_note_* against qt_shutdown (previously that caller blocked forever); left as is to keep the diff surgical. qt_shutdown still frees none of G's allocations (pre-existing, untouched).

Validation

• make -C c check (OK, skipped=35) and make -C c test-asan (clean under ASan+UBSan)
• CUDA changes were tested with make -C c cuda-test (if applicable) — on a Colab Tesla T4 (driver 580.82.07, CUDA 12.8), commit d937ad2: make -C c cuda-test CUDA_ARCH=native passes; the four tier tests pass; make qwen36 CUDA=1 CUDA_ARCH=native builds with 0 warnings; the CI tiny fixture (--ebits 8, int8 experts) decodes 16/16 tokens against the torch reference with the tier off, on (64/64 experts resident, 100 % VRAM hits), and on with a starved budget (CUDA_EXPERT_GB=0.00002: 1/64 resident, 317 CPU misses through the int8 fallback that qwen36 CUDA tier: int8 containers free the weights the tier still points at (regression from #1334) #1341 fixes), tier-on output identical to tier-off. One warning appeared in that run from Colab's older gcc in the engine-test build, qwen36.c:1450 -Waggressive-loop-optimizations, in the pre-existing nibble-unpack tail loop that this PR does not touch; gcc 13 (CI) is clean.
• Performance claims include hardware, commands, and repeatable measurements (no performance claims)

RED evidence, each against the tree without its fix:

• qwen36 CUDA tier: qt_issue overruns G.is_x with two or more GPUs (stride 8*D into a 32*D buffer) #1339: test_qwen36_tier_multidev under ASan with the old stride:

  =================================================================
  ==1336655==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x525000002100 at pc 0x7b42016fb303 bp 0x7ffeb9de4710 sp 0x7ffeb9de3eb8
  WRITE of size 256 at 0x525000002100 thread T0
      #2 0x5d36d942a1fc in qt_issue tests/../qwen36_tier.c:450
      #3 0x5d36d942c61f in main tests/test_qwen36_tier_multidev.c:76
  0x525000002100 is located 0 bytes after 8192-byte region [0x525000000100,0x525000002100)
  SUMMARY: AddressSanitizer: heap-buffer-overflow ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors_memintrinsics.inc:115 in memcpy
  

• qwen36 CUDA tier: qt_shutdown never signals cv_take, so pthread_join can hang if a group is open #1340: test_qwen36_tier_shutdown hangs in qt_shutdown; the test's 10 s watchdog fires (FAIL: qt_shutdown hung). With the broadcast but without the drop path, the post-shutdown state checks fail (victim tensor freed, incoming expert uploaded after shutdown began).
• qwen36 CUDA tier: int8 containers free the weights the tier still points at (regression from #1334) #1341: test_qwen36_tier_int8_engine against the tree with the extraction but the old free condition:

  int8 container
    ok   every planned int8 expert reached VRAM
    ok   three uploads per planned expert (gate, up, down)
    FAIL a planned int8 expert still has weights for the CPU fallback
    FAIL those weights are the bytes the loader wrote
    FAIL the pointer the tier kept still targets the live int8 block
  int4 container
    ok   every planned int4 expert reached VRAM
    ok   int4 still drops the int8 copy right after staging (peak RSS)
    ok   slot_ensure_int8 rebuilds it from the packed g4/u4/d4
  FAILED 3
  

Build: 0 warnings under the Makefile's flags for the tests and make qwen36.

Compatibility

• The default CPU build remains dependency-free
• No model files, generated binaries, or benchmark artifacts are included

Fixes #1339, fixes #1340, fixes #1341.

🤖 Generated with Claude Code

https://claude.ai/code/session_019BacNGNxAJ1M57UdYE2M3N

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[JustVugg]

Reviewed, and this supersedes the two PRs I had opened — I closed them. Yours came 16 hours first, covers all three, and does the one thing I had written off as impossible.

On that last point specifically. I claimed in my own PR that the caller-side violation of #1341 could not be caught without a GPU, because it lives in the warmstart rather than in the tier. test_qwen36_tier_int8_engine.c catches it: include qwen36.c, build an in-memory int8 model with no container behind it so expert_get takes its hit path, drive tier_warmstart() against the fake backend. That is the right answer and I had stopped one step short of it. Factoring qwen36_fake_cuda.h out so all three tier tests share it is the move that made it possible.

Three commits, one per issue, each with a test that fails before its fix: that is the shape we want.

One suggestion, on #1341's condition. You have:

if (!keep8 && expert_is_int4 && e->g) { free(e->g); ... }

Correct today. What it encodes, though, is the current correspondence between weight format and memory ownership. Whoever adds a third format that also aliases e->g — an fp8 container, say — has no reason to read expert_is_int4 as a statement about ownership, and would reintroduce this exact bug without noticing.

The invariant itself does not mention formats:

if (!keep8 && e->g && wg != (const uint8_t *)e->g) { free(e->g); ... }

Do not free what you just handed over. A future aliasing format is then correct without anyone remembering; a future format with its own separate copy still frees as before. I verified it across the current two plus two hypothetical formats before suggesting it.

Take it or leave it — the PR is good either way and I am not blocking on a one-line preference. If you would rather not touch it, say so and I will send it as a follow-up with your name on the original.

CI approved and running. Reviewing the rest of the diff now.

[JustVugg]

CI is red on Windows UCRT64, and it is the test rather than the fix:

tests/test_qwen36_tier_shutdown.c:115: error: 'SIGALRM' undeclared
tests/test_qwen36_tier_shutdown.c:116: error: implicit declaration of function 'alarm'

alarm/SIGALRM are POSIX and MinGW has neither. Everything else is green, and the tier code itself builds there.

I hit exactly this two days ago on #1336 with pipe/dup2, so the shape is familiar. A watchdog thread is the portable equivalent and keeps the test meaningful on Windows instead of skipping it there:

static void *deadline(void *unused) {
    (void)unused;
    struct timespec limit = {5, 0};
    nanosleep(&limit, NULL);
    fprintf(stderr, "FAIL: qt_shutdown did not return within 5s (#1340)\n");
    _exit(1);
}
/* start it detached before qt_shutdown(), let the process exit normally if we get there */

Worth keeping it running on Windows rather than #ifndef _WIN32: on #1336 the Windows job — once the test could actually build there — found a real semantic difference in my own code that Linux could not see. A skipped test on the platform you are least able to check locally is the one you most want running.

Say if you would rather I push that as a commit on a branch here and you pull it, same as I did for #1316. The three fixes read well and I would like them landed.

[kreuzzelg]

#1339 and #1340 are mine — the tier landed in #713 with both. Thank you for finding them, and more for the shape of the fix: a fake backend that lets the tier run in the CPU build is exactly what my own tests lacked, which is why these two survived. I read the three commits against current dev rather than against the description; notes below, two confirmations and three things I would still change.

Confirmations

#1340, the abandon path holds up against the tier's accounting. An LFRU swap is enqueued with v_eid >= 0, so enqueue_locked never reserves budget for it (!reserved && v_eid<0 is false) — abandoning it needs no G.used correction, and the patch correctly makes none. The two flags it restores are the two that qt_lfru_tick_locked and enqueue_locked changed: the victim's resident (cleared before enqueue) and the newcomer's queued. The victim still holds its tensors, so resident=1 is the truthful state. And the race the test cannot pin — whether the uploader has reached the cv_take wait when shutdown fires — is handled in both orders: if it is parked, the broadcast wakes it into the abandon branch; if it has not arrived yet, !G.th_stop fails and it skips the wait into the same branch. Nothing depends on the poll in the test having caught it parked.

The other three cv_take waiters exit cleanly too. qt_note_planned and qt_note_block wake into enqueue_locked, which returns 0 on a full queue — qt_note_planned then returns its reservation via the existing s->planned branch. qt_fill_wait simply returns with qn > 0, which at shutdown is what we want.

#1339 sizing: G.ndev is final at the allocation (the have < ndev clamp is thirty lines above it), and home() never returns a device index at or beyond ndev, so ndev * 32 * D is the whole address space qt_issue can touch.

Three things I would still change

1. Name the row limit once. After this patch the literal 32 stands in seven places that must agree: is_k[QT_MAX_DEV][32], K>32 in qt_issue, tg/tu/td[QT_MAX_DEV][32], rows[32], the is_x_floats computation and the stride. #1339 was three of those disagreeing. A QT_MAX_ROWS next to QT_MAX_DEV, used in all seven, plus one line in the multidev test that ties the stride to the array the rows are indexed by —

check(sizeof G.is_k[0] / sizeof G.is_k[0][0] == 32 &&
      G.is_x_floats == (size_t)G.ndev * (sizeof G.is_k[0] / sizeof G.is_k[0][0]) * G.D,
      "replica block stride equals the per-device row capacity");

— makes the next drift fail here instead of in a driver.

2. The warmstart line and the docs now say something false on int8. "[qtier] warmstart (parallel): all %d experts in RAM (int8 only for non-residents)" was true because the int8 copy of every resident was freed. With #1341 that free is int4-only, so on an int8 container every resident keeps its weights and the parenthesis is wrong — and so is the RSS saving docs/qwen36-cuda-tier.md quotes (40 → 29 GB), which is an int4 number. Both should say so: the tier's RSS advantage is a property of packed containers, an int8 container pays full residency and gets VRAM speed only. Better a line that says "int8 container: all experts stay in RAM" than one that promises a saving the reader will not see.

3. One cheap assertion for the mixed batch. The multidev test proves the two blocks are disjoint and inside the buffer. It could also pin where they are — device 0's at G.is_x, device 1's at G.is_x + 32*D — which is the property the stride literal encodes and the one assertion that would have failed on the old code even with a single row on device 1.

On the test infrastructure

Because TEST_RULES collects every tests/test_* rule, all four land in make test-c without a list edit, and test-asan rebuilds that suite under ASan/UBSan in the Sanitizers job. So the multidev test is also an ASan detector for #1339 from the day it merges, and the tier finally has a memory-safety gate that does not need a GPU. I will build the placement work I proposed in #1040 on qwen36_fake_cuda.h rather than on a second harness — thank you for factoring it out.

With the watchdog in place of alarm this is good to go from where I sit.

[JustVugg]

Pushed the MinGW fix to your branch as one commit on top of yours, so nothing of yours moved: test_qwen36_tier_shutdown.c now bounds qt_shutdown() with a detached watchdog thread instead of alarm()/SIGALRM, which MinGW does not have. The test already used pthread and nanosleep, so this adds no dependency. Same 10 s, same failure line, same _exit(1).

Verified here: the test passes on Linux, and a probe that never sets the flag exits 1 after exactly 10 s, so the watchdog still catches a hang. CI will say whether Windows agrees.

Reason for the hurry, stated plainly: #1334 is on dev since 1.10.1 and this PR is the fix for the use-after-free it introduced (#1341). The next release must not ship one without the other, and this test was the only thing standing between them. If you would rather carry the change yourself, revert my commit and push yours; either is fine.

[mfethe1]

All three changes are in, plus the watchdog. I don't have push access to
qwen36-tier-fixes, so they're in a cross-fork PR against that branch rather than
pushed directly: crichalchemist#1 (ef3ac1e). @crichalchemist — merge,
cherry-pick, or apply by hand, whichever suits. Two of your requests turned out to be
measurably complementary rather than overlapping, which is worth writing down.

1. QT_MAX_ROWS, and why your check #1 and your check #3 are both needed

QT_MAX_ROWS now stands in all seven places (is_k, the K> clamp, tg/tu/td,
rows[], the topk clamp message, is_x_floats, the qt_issue stride), and the
multidev test carries your sizeof assertion plus a placement pin.

I built both and then deliberately broke the tree in each of the two drift directions,
three repeats each, to check neither assertion was decoration:

injected drift	sizeof check	placement pin	in-bounds	disjoint
stride reverted to 8*D (the #1339 shape)	fires	fires	fires	fires
is_k[..][64], stride still 32	fires	passes	passes	passes

The second row is the one I did not expect. If a later edit widens the row array
without touching the stride, the placement pin is silent — device blocks really are
at is_x + di*32*D and really are disjoint; the buffer is just too small for a group
the array now permits. Your sizeof check is the only thing that catches it. And in
the first row the placement pin fires on a single-row device-1 issue where in-bounds
and disjointness both pass. So they detect different things and I kept both, rather
than folding one into the other as I had first intended.

Related: the PR's own in-bounds assertion passes under a too-small stride in the
mixed batch, because undersized blocks stay inside the allocation while overlapping
each other. In-bounds is not a sufficient invariant here — that is the argument for
the placement pin being an assertion rather than a comment.

2. The warmstart line and the docs

You are right, and the docs were worse than the log line. Both now say which container
they are talking about:

• the log line branches on expert_is_int4 — "int8 copy dropped for residents, kept for non-residents" vs "int8 container: all experts keep their weights in RAM";
• docs/qwen36-cuda-tier.md says the RSS saving is a property of packed containers,
  that an int8 container frees nothing since qwen36 CUDA tier: int8 containers free the weights the tier still points at (regression from #1334) #1341 and pays full residency for VRAM
  speed, and that the 40 → 29 GB row is an int4 measurement with no int8 analogue.

3. The watchdog

alarm/SIGALRM are gone; a detached thread sleeps 10 s and _exit(1)s with the same
FAIL: qt_shutdown hung message. Same contract, and the intent is that the test runs
on UCRT64 rather than being compiled out — but see the caveat below: I have not built it
there, so that is the goal of the change, not something I have measured.

I checked it is a real oracle before proposing it. Fixed and unfixed trees, alarm and
watchdog harnesses, five runs per cell: identical verdicts in all four cells, 20/20, no
within-cell splits — passing cells 16–150 ms, hanging cells 10046–10270 ms, about two
and a half orders of magnitude apart and non-overlapping. And reverting just the
pthread_cond_broadcast(&G.cv_take) in qt_shutdown on the current tree reproduces the
hang 3/3 under the watchdog at 10.13–10.22 s, so the deadline is load-bearing and not
merely green.

Caveat I want stated plainly: all of the above is macOS/arm64 + Apple clang against
the fake backend. I have not compiled or run this on MinGW/UCRT64 — I removed the
non-portable API, I did not measure the Windows build. Until someone builds it there (I
intend to), the CI job is the only thing that will actually prove the port. Nothing here
is a performance claim either; these are liveness and memory-safety gates only.

Thanks for the review — the #1040 placement work landing on qwen36_fake_cuda.h is
exactly why I pulled it out into a shared header.

[mfethe1]

Verified on merged dev (e9505363): all four commits landed and all three fixes are intact in the tree.

Built and ran the tier suite against the fake-CUDA harness on darwin/arm64:

test_qwen36_tier_int8          ok
test_qwen36_tier_multidev      ok   (2 devices, per-device budgets)
test_qwen36_tier_shutdown      ok   (thread watchdog, no hang)
test_qwen36_tier_int8_engine   OK   (3/3: planned int4 reach VRAM,
                                     int8 copy dropped after staging,
                                     slot_ensure_int8 rebuilds from packed g4/u4/d4)

@JustVugg your SIGALRM→thread swap in fa8142d7 is the right call — the watchdog fires correctly here and the test is now portable to MinGW without the signal dependency. Thanks for pushing it onto the branch rather than bouncing the PR back.

@kreuzzelg the #1340 shutdown fix reads well in final form — pthread_cond_broadcast(&G.cv_take) alongside the existing signal(&G.cv) at qwen36_tier.c:514, so the qt_note_planned/qt_fill_wait waiters on the shared condvar all observe th_stop and pthread_join can't hang.

One note on what this does and doesn't establish: this is the fake-CUDA harness on CPU, so it validates the tier's control flow — per-device replica sizing, shutdown wakeup, int8 slot retention — but not real CUDA behaviour. The multidev test proves the tier keeps per-device state separate; it doesn't prove correctness against actual multi-GPU hardware, which I can't test here. Worth someone running it on a real 2-GPU box before relying on #1339 being fully closed.

Nothing outstanding from my side. Thanks both — clean collaboration.