Fail loudly instead of storing nothing when HF meets the capture config or a record ring by zaoxing · Pull Request #146 · ProjectDMX/DMI

zaoxing · 2026-09-23T20:17:37Z

Stacked on #143. Merge #143 first; GitHub then retargets this PR to main.

Problem

The audit reproduced two ways HF generation "succeeds" while storing nothing:

Under storage_backend="capture": HF runs its legacy HookPoints on a ring with no sink attached, whose P2P thread drops every capture. Nothing raises.
After create_record_runtime: each step's metadata push throws "legacy metadata cannot be pushed to a record ring". generate_with_monitoring swallowed that with except Exception: pass, while generate_greedy_with_monitoring propagated it.

Changes

The HF entry points refuse the capture backend. attach_model (shared through BackendAdapter), generate_with_monitoring and generate_greedy_with_monitoring raise ConfigurationError under storage_backend="capture". The message names the adapter and the entry point, since capture storage isn't wired to any adapter yet; the HF bridge comes later. vLLM inherits the refusal through BackendAdapter, which is intended, since it is not wired to capture either. Megatron's adapter does not subclass BackendAdapter and drives record runtimes directly, so it is unaffected.
No more swallowed step failures. A failed capture step in the HF prepare wrapper is re-raised in capture or record mode. On the legacy ring it's logged once per adapter at WARNING instead of passing silently, and generation continues as before.
commit_step refuses record mode, whatever ring the adapter holds. An adapter attached before create_record_runtime still holds the stopped legacy ring, which accepts the step silently. That would slip past the native guards on a CUDA-graph replay. The independent review found this attach order.
Native guards. prepare_step, the hook_no_notify* producer entries, reserve_one and submit_cpu_direct throw on a record ring, mirroring the existing metadata guard. Without them a legacy step leaked ring capacity: 65536 → less, never reclaimed. reserve_one and submit_cpu_direct go beyond the plan's list; they are the eager safety net's halves of the same paths.
The v1 contract now documents both refusals (ConfigurationError is not a ValueError), and a test pins the passages.

Evidence

CPU: tests/test_hf_capture_refusal.py uses a small stand-in HF model with a real HookPoint, a spy ring and the real RingTransport, because CI's CPU job doesn't install transformers. It passes 12; before the fix, 8 failed (DID NOT RAISE, silent generate, no warning). The adapter/HF CPU suites pass 251; the CPU tier 2225 passed, 0 skipped.
Native (RTX 4090): tests/native/ring/test_ring_engine failed 8 on the pre-fix binary, all in the new record-ring test, including the capacity leak. Fixed: 116 passed.
GPU: tests/test_record_ring_refuses_legacy_path.py covers both attach orders. Together with test_native_sink_ring_e2e.py and test_native_capture_storage_gpu_e2e.py, 16 passed.
Pre-existing, unrelated: 5 failures in tests/test_producer_chunked_schema.py (eager-ring staging tests) reproduce identically on Native capture storage service: spool to object store to catalog, in-process #143's base without this branch.
Not run: the real-model HF GPU e2e suites, which use legacy rings, where these guards don't fire; and the vLLM/Megatron test suites.

Since the independent review (2026-09-24)

An independent review judged this "ship" and found two minors worth folding in; both are fixed, test-first (a60d884..9419de0):

The capture refusal now also runs in commit_step, the one choke point every legacy step goes through. An adapter that overrides attach_model without calling super(), or a v1 integration driving install_ring_hooks plus commit_step directly, used to reserve and publish steps that stored nothing; it is now refused.
In record mode with capture disabled, before_forward and commit_step returned early on null mode before the record-mode refusal, so the error came from inside the model forward. The refusal now runs first, and the HF prepare wrapper follows suit. On a real engine (GPU) the clear "before_forward(): the engine is in record mode" error now comes first, with ring capacity unchanged.
The driver-failure docstring now says only the first failure is logged, and that a step failing after prepare_step leaks its reservation until the ring is torn down.

Current evidence: tests/test_hf_capture_refusal.py + tests/test_integration_api_v1.py 29 passed; GPU record-ring refusal and sink ring suites 14 passed; CPU tier 2254 passed. CI green at 9419de0.

Copilot

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

…s additions Under storage_backend="none" the adapter refusal was tested only through the private helper. A parametrized test now drives all five entry points -- the HF and base attach_model, attach_config, generate_with_monitoring and generate_greedy_with_monitoring -- and checks each raises "capture is off" before the model runs. It passed first (the behaviour was right, the coverage missing); with "none" taken out of the helper's refusal, all five fail. #146's later commits added a test and a v1 sentence that still spelled the persistent backend "capture"; both now say "persistent". Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

Under storage_backend="capture" the HF adapter still installs legacy HookPoints. They run on the engine's legacy ring, which that backend builds without a host, so its P2P thread drops every capture: generate() returns normally and the catalog stays empty (audit link 6). Nothing wires those hooks to the capture path yet, so the config cannot store what it asks for and is now refused with a ConfigurationError that says so, before anything is armed or wrapped. The check lives in BackendAdapter.attach_model, which every adapter's legacy attach goes through (attach_config included), and is repeated at the top of generate_with_monitoring -- ahead of Phase 1, which rewrites the model's compilation outside the try/finally that restores it -- and in generate_greedy_with_monitoring(monitoring=True). Other storage backends attach exactly as before. D2 replaces the refusal with the real bridge. The forked HookedLlama classes live in the transformers submodule, which the CI CPU job does not install, so the new tests use a minimal model with a real HookPoint and HF's prepare/generate protocol over a spy ring. Tests: tests/test_hf_capture_refusal.py 8 passed (5 of them failed before the change with DID NOT RAISE, or ran the model); adapter/HF CPU suites (13 files) 231 passed. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

generate_with_monitoring's prepare wrapper ran before_forward inside `except Exception: pass`. Once create_record_runtime has put the engine on a record ring, every step's commit_step -> push_all_metas throws "legacy metadata cannot be pushed to a record ring", and the wrapper discarded it: generate() returned as if it had captured, while generate_greedy_with_monitoring, which calls the driver directly, raised the same error (audit link 7). The swallow is deliberate on the legacy ring and stays there: HF calls the wrapper from inside generate(), capture on that path is best-effort, and a failed step disarms its own hooks (before_forward clears capture_step first), so generation can go on. What changes is that the first such failure per attachment is now logged at WARNING with its traceback instead of vanishing. In record mode, or under the capture backend, the failure means nothing the config chose is being stored, so it propagates. Tests: tests/test_hf_capture_refusal.py 11 passed; the three new ones failed first (DID NOT RAISE x2, and no WARNING record logged). Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

RingEnginePy::push_step already refused legacy metadata on a record ring, but the entries around it did not, and after create_record_runtime the active transport points legacy HookPoints at that ring: - prepare_step and reserve_one advanced the CPU payload/task heads with no record publication behind them. The space was never reclaimed, and every later reserve_record reclaim was registered against a task sequence offset by the phantom count. HF's commit_step made exactly this reservation every step before push_step threw, so each failed step leaked its bytes. - hook_no_notify{,_prefix,_chunked} launched producers whose payloads have no record descriptor, so the record consumer paired them with the next record's descriptor, or failed the ring when none was queued. - submit_cpu_direct, the safety net's bypass, did the same from the host. Each now throws std::logic_error("<entry> cannot be used on a record ring") before touching the ring. reserve_one and submit_cpu_direct go beyond the plan's list (prepare_step, hook_no_notify*): they are the eager safety net's halves of the same two paths, and reserve_one runs before the producer, so guarding only the producer would still leak. The alignment test in test_ring_engine.cu built its engine as a record ring with a null sink only incidentally; it now uses a legacy ring. Tests, on GPU 2 (RTX 4090, sm_89, C++20 build of _native_backend): - tests/native/ring test_ring_engine: 116 passed, 0 failed; the pre-fix binary gave 108 passed, 8 failed, including available_capacity dropping after prepare_step/reserve_one on a record ring. - tests/test_record_ring_refuses_legacy_path.py: 7 passed (prepare_step, the three producer strip modes, both safety-net branches, and HF generate after create_record_runtime, with capacity unchanged). - tests/test_native_sink_ring_e2e.py 6 passed, tests/test_native_capture_storage_gpu_e2e.py 2 passed. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

An adapter keeps the ring and transport it took at attach. After attach_config then create_record_runtime, those are the legacy ring that create_record_runtime stopped, and it accepts prepare_step and the metadata push without error. So the prepare wrapper saw no failure, and _handle_driver_failure never ran, even with the engine in record mode. Only the native producer guard caught the step, when a HookPoint fired eagerly on the record ring. A replayed graph would not fire it. commit_step now raises when the engine is in record mode, before reserving or pushing anything. That covers both attach orders, and before_forward, the HF prepare wrapper and the greedy loop, since each of them reaches commit_step. The prepare wrapper re-raises the error in record mode. Tests: a CPU test in test_hf_capture_refusal.py simulates the switch on a spy engine. A GPU test in test_record_ring_refuses_legacy_path.py uses a real engine and create_record_runtime. With the old base.py both failed: the CPU test with DID NOT RAISE, and the GPU test with DID NOT RAISE (the existing link-7 GPU test also failed, because the native message came first). With the fix: test_hf_capture_refusal.py 12 passed. On GPU 2, test_record_ring_refuses_legacy_path.py, test_native_sink_ring_e2e.py and test_native_capture_storage_gpu_e2e.py gave 16 passed. The adapter/HF CPU suites gave 251 passed. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

attach_model now raises ConfigurationError under storage_backend="capture", and vLLM and Megatron inherit that through BackendAdapter. commit_step raises RuntimeError in record mode. Neither was in the v1 contract. Its attach_model section listed ValueError and RuntimeError only, and its storage paragraph promised ValueError. ConfigurationError is not a ValueError, so an integration that followed the document would not have caught it. The contract now names both refusals, and a test pins the three passages. The driver-failure warning's latch is per adapter instance, not per attachment; its docstring and message now say so. Tests: test_integration_api_v1.py + test_hf_capture_refusal.py, 22 passed (the new contract test failed before the doc change). Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

The capture refusal ran in the base attach_model and the HF entry points, but a step need not come through either. attach_config accepts an adapter that overrides attach_model without calling super(), and a v1 integration can arm its hooks with the exported install_ring_hooks and call commit_step directly. Under storage_backend="capture" such a step reserved and published into the legacy ring, which has no host there, so nothing was stored. commit_step now calls _refuse_unwired_capture_storage next to its record-mode check, before reserving anything. Red: a stub adapter whose attach_model skips super() (directly and through attach_config) got StepReservation.RESERVED from commit_step, with one prepare_step(64, 1) and one metadata push on the spy ring. Green: both raise ConfigurationError naming _NoSuperAttachAdapter.commit_step(), with no reservation or push. The capture-mode prepare-wrapper test now sees that refusal instead of the spy's reservation error, and still propagates it. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

With capture disabled (set_capture_enabled(False)), before_forward and commit_step returned on transport.null_offload before the record-mode refusal, and the HF prepare wrapper skipped the driver altogether. The installed HookPoints stay armed in null mode, so on a record ring the first error came from inside the model forward: the native "legacy producer cannot be used on a record ring", naming neither the adapter nor the fix. The v1 contract said commit_step raises before reserving anything in record mode, which was false while null_offload was set. The refusal moves into _refuse_record_mode, which before_forward and commit_step both call ahead of the null-mode return; the HF wrapper no longer skips the driver in record mode. before_forward names itself in the message, so the tests' refusal regex accepts before_forward() or commit_step(). Nothing in the tree relies on the early return in record mode: RecordRuntime reads null_offload on its own path, and before_forward_manual goes through before_forward. Null mode outside record mode still skips. Red: on a spy engine in record mode with null_offload=True, commit_step returned SKIPPED, before_forward returned None, and generate_with_monitoring returned normally (3 failed). Green: all three raise the record-mode RuntimeError with no reservation or metadata push, and a new test pins that null mode on the legacy ring still skips. The v1 doc says both. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

The _handle_driver_failure docstring said a failed legacy step is logged once per adapter and lost, which read as the whole cost. It did not say that every later failure from the adapter is swallowed with no log line, or that a step failing after prepare_step reserved its space leaks that reservation until the ring is torn down (the reviewer measured available_capacity() staying 4096 bytes lower after one such step). The docstring now says both. Documentation only; the behaviour is unchanged. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

…s additions Under storage_backend="none" the adapter refusal was tested only through the private helper. A parametrized test now drives all five entry points -- the HF and base attach_model, attach_config, generate_with_monitoring and generate_greedy_with_monitoring -- and checks each raises "capture is off" before the model runs. It passed first (the behaviour was right, the coverage missing); with "none" taken out of the helper's refusal, all five fail. #146's later commits added a test and a v1 sentence that still spelled the persistent backend "capture"; both now say "persistent". Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y

* Name the storage choices in-memory, persistent and none The user's storage choice now has exactly three values: - "in-memory": records delivered in memory. Until its consumer interface exists this is the ClickHouseRecordSink path, which needs a host engine. - "persistent": the native capture storage path, object store + catalog + ClickHouse. - "none": capture off entirely. The engine allocates no ring, so none of the ring's pinned staging or payload memory. A record runtime, a host engine, an explicit ring_config and adapter attachment are refused, each with a message that names the two choices that capture. dmi.config.USER_STORAGE_CHOICES lists them. "auto" stays the unset default, the inference every caller made before the field existed; the configurator will always emit one of the three. The earlier names still work, with a DeprecationWarning that names the replacement: "native" means in-memory and "capture" means persistent. MonitoringConfig keeps whatever the caller wrote, so an integration that reads the field back still sees its own value. The engine acts on canonical_storage_backend. "none" used to mean capture and transport with no persistence: the ring still ran, and every record then failed at flush_and_wait with "record sink is not configured". Nothing depended on that, and it is now a clear refusal up front. The one test that expected an adapter to attach under "none" now expects the refusal. Docs: the v1 contract lists the three choices, the aliases and what "none" refuses; the capture-storage design doc uses the new names. Tests: a new tests/test_storage_backend_choices.py (13 cpu tests) covers the three choices, auto as the unset default, the aliases and their warning, the unknown-value message, and none allocating no ring and refusing a ring_config, a record runtime, a host engine and attachment. It failed at import before the change. The existing suites moved to the new names. CPU tier: 2238 passed, 1 skipped (no CUDA device), 0 deprecation warnings. GPU (RTX 4090) capture storage, sink ring and record-ring refusal suites: 29 passed with DeprecationWarning as an error, so no test still uses an old name. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y * Refuse a ring under none after construction too; pin the old names in the engine Two findings from an independent review: - storage_backend="none" skipped only the constructor's ring. The public enable_ring_transport(), which the adapter docs point callers at, still allocated one and made it globally active. It now refuses under "none" with the same message as create_record_runtime. - Nothing tested that a deprecated name reaches the engine: reverting the engine's mapping to the raw field left all 85 related tests green. The mapping is now a module-level canonical_storage_backend() the engine applies to whatever config it is given, so a duck-typed config carrying "native" or "capture" is checked too (before, it slipped past every check). New tests drive the engine with each old name: "native" without a host is refused, "capture" with a host is refused, "capture" with a sink config resolves to "persistent", and a duck-typed "native" is refused. With the old raw read put back, 3 of them fail. CPU tier: 2242 passed, 1 skipped (no CUDA device). GPU (RTX 4090) capture storage, sink ring, record-ring refusal and storage-choice suites: 33 passed with DeprecationWarning as an error. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y * Type the canonical storage name, and name the caller in its warnings and errors Three smaller findings from the independent review: - canonical_storage_backend returned a plain str, so every literal comparison in the engine and adapters went unchecked by a type checker. It now returns CanonicalStorageBackend, a Literal of the four names the engine acts on. - The deprecation warning used stacklevel=3, which points at the caller only for direct construction. Through dataclasses.replace() -- which the configurator's _install_schedule uses -- it was attributed to dataclasses.py. The level is now computed by walking past this module, the generated __init__ and dataclasses.py, so both routes name the caller's line. - A refusal quoted the canonical name even when the caller wrote the old one, so "native" produced an error about 'in-memory'. Refusals now read "config.storage_backend='native' (now 'in-memory') ...". Tests: three new ones in test_storage_backend_choices.py, each red first. With the storage, engine and wiring suites, 72 passed. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y * Bring the storage docs in line with the persistent path as it ships The design doc's comparison table still headed its columns "native path" and "capture path", and described the persistent side as the Python reference sink, "explicitly reference-only". Since the native pack writer became the default and the in-process storage service landed, that read as "persistent = Python reference-only". The table now names both paths by their storage choice and describes the native writer and storage service as production, the Python sink as the reference and rollback. The v1 contract's list of refusals left out "none" with a ring_config (ValueError at construction) and did not say that "none" refuses create_record_runtime and enable_ring_transport with RuntimeError and an adaptor's attach_model with ConfigurationError. It now lists each with its exception type. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y * Refuse none at every HF entry point, and finish the rename over #146's additions Under storage_backend="none" the adapter refusal was tested only through the private helper. A parametrized test now drives all five entry points -- the HF and base attach_model, attach_config, generate_with_monitoring and generate_greedy_with_monitoring -- and checks each raises "capture is off" before the model runs. It passed first (the behaviour was right, the coverage missing); with "none" taken out of the helper's refusal, all five fail. #146's later commits added a test and a v1 sentence that still spelled the persistent backend "capture"; both now say "persistent". Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y --------- Co-authored-by: Alan Liu <zaoxing@users.noreply.github.com>

zaoxing requested review from Samfisheryu and a lite review from Copilot September 23, 2026 20:17

Copilot AI reviewed Sep 23, 2026

zaoxing mentioned this pull request Sep 23, 2026

Name the storage choices in-memory, persistent and none #148

Merged

zaoxing force-pushed the fix/capture-fail-loudly branch 2 times, most recently from abf18c2 to 9419de0 Compare September 24, 2026 14:07

zaoxing force-pushed the feat/native-capture-service branch from d14ca30 to b3dfb0d Compare September 24, 2026 14:46

zaoxing added 8 commits September 24, 2026 10:46

zaoxing force-pushed the fix/capture-fail-loudly branch from 9419de0 to a7cc4e3 Compare September 24, 2026 14:47

zaoxing changed the base branch from feat/native-capture-service to main September 24, 2026 14:47

zaoxing merged commit 54186ba into main Sep 24, 2026

zaoxing deleted the fix/capture-fail-loudly branch September 24, 2026 15:37

Navigation Menu

Fail loudly instead of storing nothing when HF meets the capture config or a record ring - #146

Fail loudly instead of storing nothing when HF meets the capture config or a record ring#146
zaoxing merged 8 commits into
mainfrom
fix/capture-fail-loudly

zaoxing commented Sep 23, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

zaoxing commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Evidence

Since the independent review (2026-09-24)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zaoxing commented Sep 23, 2026 •

edited

Loading