Eager safety net: flush when the task ring is full, not only the payload

[claude]

Requested by Alan Liu · Slack thread

Before: In the legacy ring, a step that fires more hooks than the task ring has entries could quietly lose or mislabel captured tensors. The eager fallback only checked that there was enough payload space. It never checked for a free task slot, so the hook after the ring filled wrote over a slot the drain had not read yet. flush_and_wait still reported success.

After: The eager fallback flushes the ring when either payload space or task slots run out, so every hook gets a free slot. reserve_one itself now refuses (raises) instead of overwriting when no task slot is free. Record-mode rings were never affected and behave the same.

How. Found by TLA+ model checking of the payload ring (CapacityRespected and then NoSlotOverwrite fail in 12–15 steps). The failure path, confirmed against main @ a987dfe:

1. prepare_step sees num_hooks > task_cap, flushes, and returns STEP_OVERSIZED. The adapter sets force_eager.
2. Each hook's safety net in HookPoint.forward checks only available_capacity() (payload bytes) and calls reserve_one. That calls DrainThread::reserve(n, 1), which advances the task head with no task-capacity check.
3. The producer for sequence task_cap release-stores its READY|size word into slot 0 while hook 0's word is still unread (producers never read the tails). The drain then either reads the wrong size, shifting the TensorMeta pairing by one, or clears the new word in flush_state_update and waits at that sequence forever.

The fix adds RingEnginePy::available_task_slots(), computed and bound the same way as available_capacity(). The safety net takes the no-flush branch only when the bytes fit and at least one task entry is free. Otherwise it flushes and then reserves, and a flush frees both. As a second guard, reserve_one throws std::logic_error and reserves nothing when no task entry is free. The ring's only other reserve_one caller is this safety net, and it now checks first, so no correct path hits the throw. The estimate.py message that said the over-task-cap case "falls back to eager CPU-direct dispatch" is corrected, and the check-and-reserve comments in ring_engine_py.cu now cover task slots.

Tests

• New CPU regression test tests/test_hook_point_eager_task_slots.py: a fake legacy ring with 4 task entries and plenty of payload fires 5 hooks through the eager safety net. It asserts that no more than 4 tasks are ever outstanding and that the 5th hook flushes before it reserves. Fails on main (assert 5 <= 4), passes with the fix.
• New native case test_reserve_one_refuses_without_a_free_task_slot in tests/native/ring/test_ring_engine.cu.
• The existing GPU fakes in test_hook_point_eager_cap_cache.py and test_producer_chunked_schema.py gain available_task_slots().
• python -m pytest -m cpu -q: 1944 passed, 324 skipped (the skips are native binaries not built here). ruff check --select F821 src/dmi is clean.
• Couldn't run here: there is no CUDA device or nvcc, so the native ring test and the GPU-marked tests did not run. bindings.cpp passed g++ -fsyntax-only against the torch/pybind headers. A host-side g++ parse of ring_engine_py.cu and test_ring_engine.cu shows the same errors as main (only __CUDACC__-gated launcher declarations) and none in the touched code. CI's native compile is the real build check.

---

[XbzOnGit]

Reviewed revision 6c48a66. No blocking correctness issues found.

The eager path checks both aligned payload capacity and a free task slot before reserving, and the native reserve_one guard refuses overflow before advancing either head. The check-and-reserve sequence is sound under the existing single-producer contract.

Built and tested this exact revision in a separate worktree using the fresh Conda environment and an RTX 4090:

• 43 focused Python/GPU tests passed, including independent regressions with 1, 2, and 4 task slots, repeated wraparound, zero-byte eager routing, full payload reclamation, and record-mode refusal.
• 34 CPU adapter/planning tests passed.
• All six native test executables passed: 633 checks.
• An additional native delivery regression passed 27 checks, including byte-for-byte payload and metadata ordering for nine hooks through a two-slot ring.

Nonblocking recommendation: retain the real-GPU wraparound/delivery regression in the repository. The additional regressions above were run locally against this revision and are not currently included in this PR.

Approved.

[claude]

@XbzOnGit thanks for running it on a GPU. Agreed on keeping the wraparound/delivery regression — I don't have that test here, so if you push it to this branch (or paste it) I'll wire it into the native test target and keep CI green.

---

Generated by Claude Code

[Copilot]

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

[zaoxing]

Superseded by #160: the same change, re-landed on current main and authored by Alan Liu, plus the GPU delivery regression XbzOnGit asked for and review fixes. Merged as 20b58b4.