Surface native host-sink failure instead of silently truncating captured activations by Copilot · Pull Request #101 · ProjectDMX/DMI

Copilot · 2026-08-24T19:55:45Z

A transient ClickHouse insert failure could permanently kill the native host sink while generation continued normally, leaving activation capture silently truncated. This change makes sink death observable at teardown and adds lightweight accounting for suppressed submit failures and queue drops.

Host-sink failures now surface at teardown
- MonitoringEngine.close() no longer swallows native host-engine failures.
- After close_input() / stop(), teardown checks raise_if_failed() and raises a Python error when the sink died.
- The raised error includes sink telemetry when available, so callers get a direct signal instead of a truncated dataset.
Native sink accounting is now exposed
- DMXHostEngine now enables engine stats collection.
- Bound host-engine profiling snapshots to Python so queue-level counters are accessible from teardown.
- Exposed queue counters relevant to loss diagnosis (dropped, full_errors, retries).
Suppressed submit failures are counted, not just logged once
- Kept the existing once_flag warning behavior in the p2p thread.
- Added a monotonically increasing counter for every suppressed submit exception after the sink aborts.
- Exposed that counter through RingEngine so teardown can report how many slices were suppressed after the first failure.
Lazy internal reads are stricter by default when request metadata exists
- LazyInternal token-range validation now defaults on when request IDs and token ranges are present.
- match_token_ranges=False remains available as an explicit opt-out.
- Updated the docs to reflect the stricter default and the opt-out path.
Regression coverage
- Added a teardown-focused test that asserts host-engine failure is raised and includes sink-loss telemetry.
- Added a lazy-internal test that verifies incomplete token coverage raises by default when per-request metadata is available.

Example of the new failure mode at teardown:

try:
    engine.close()
except RuntimeError as exc:
    print(exc)
    # DMX host sink failed during teardown
    # (queue_stats=dropped=3 full_errors=2 retries=1; suppressed_submit_failures=7)

Fixes Silent data loss #100

Co-authored-by: zaoxing <[email protected]>

Copilot

Pull request overview

This PR makes native host-sink failures observable from Python at teardown (instead of silently suppressing them) and adds lightweight telemetry to diagnose activation-loss scenarios, while also tightening LazyInternal token-range validation defaults when per-request metadata is available.

Changes:

Surface host-engine failure during MonitoringEngine.close() and include sink telemetry (queue stats + suppressed submit failures) in the raised error.
Add native accounting for suppressed submit failures (ring p2p thread) and expose host-engine profiling snapshots/stats to Python via pybind.
Default token-range validation to “on” when request metadata exists, and update docs/tests to reflect the stricter behavior and opt-out path.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/test_storage_internals.py	Adds regression test for default token-range validation when request metadata exists.
tests/test_engine_runtime_api.py	Adds teardown-focused test ensuring host-sink failure is raised and includes sink telemetry.
src/dmi/storage/internals.py	Flips token-range validation default and adjusts validation gating logic.
src/dmi/engine.py	Adds teardown error formatting with telemetry and stops swallowing teardown exceptions.
native/csrc/ring/ring_engine.h	Exposes suppressed submit failure counter from the ring engine.
native/csrc/ring/ring_engine_py.h	Declares Python-facing `suppressed_submit_failures()` API.
native/csrc/ring/ring_engine_py.cu	Implements Python-facing `suppressed_submit_failures()` API.
native/csrc/ring/p2p_thread.h	Adds atomic counter for suppressed submit failures.
native/csrc/ring/p2p_thread.cpp	Increments suppressed-submit counter on every suppressed submit exception.
native/csrc/dmx_host_engine.h	Enables engine stats collection for `DMXHostEngine`.
native/csrc/bindings.cpp	Binds profiling/stats snapshot types and exposes `profiling()`/`reset_metrics()` to Python.
docs/integration-api-v1.md	Updates v1 docs for stricter token-range validation defaults and opt-out.
docs/huggingface.md	Updates HF docs to describe stricter lazy-read validation defaults and opt-out guidance.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

    def _validate_token_ranges(
        self,
        field: str,
        rows: list,
        requirement: InternalRequirement | None,
    ) -> None:
        if (
-            requirement is None
-            or not requirement.match_token_ranges
+            (requirement is not None and not requirement.match_token_ranges)
            or not self._request_ids
            or not self._token_ranges
        ):


@@ -290,27 +338,41 @@ def close(self) -> None:
                except Exception:
                    pass
            try:
-                ring_engine = getattr(self, "_ring_engine", None)
                if ring_engine is not None:
                    ring_engine.stop()
-            except Exception:
-                pass
+            except Exception as exc:
+                teardown_error = teardown_error or exc
            try:
                _rt = _ring_module()
                _rt.deactivate()
-            except Exception:
-                pass
+            except Exception as exc:
+                teardown_error = teardown_error or exc


Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

PipelinedEngine declared its profiling storage with elaborated-type-specifiers in its own base-clause: private ebo_storage<std::conditional_t<EnableEngineProfiling, std::optional<struct EngineProfilingConfig_>, ...>, 1>, A member type is not in scope in a base-clause, so `struct EngineProfilingConfig_` there declares a new dmx_host::EngineProfilingConfig_ at namespace scope that is never defined -- not the nested struct declared later in the class body. While EnableEngineProfiling was false the std::conditional_t branch was never selected and the incomplete type stayed harmless. DMXHostEngine now passes true, which selects it, and std::optional over an incomplete type is ill-formed: error: incomplete type 'dmx_host::EngineProfilingConfig_' used in type trait expression `make host` fails with 7 errors and builds no objects, and `make test-host` fails outright. CI misses it because `make check` is compileall plus pytest and never builds the native extension. Hoist EngineProfilingConfig_, IngestStats, QueueStats and StageStats to namespace scope so the base-clause names complete types, and re-export them as class aliases -- bindings.cpp refers to them as DMXHostEngine::QueueStats. EngineProfile_ and StatsSnapshot had identical members, so they collapse into one EngineStats_<NumStages> instead of two shapes kept in sync. Verified with a standalone TU instantiating the template with EnableEngineProfiling both false and true: it fails against the old header and compiles against this one, with static_asserts confirming the public type identities are unchanged. `make host` now builds and links both objects, and `make test-host` passes. Co-Authored-By: Claude Opus 5 <[email protected]>

Turning match_token_ranges on by default made two existing weaknesses in the validator user-visible. final_logits rows are re-based natively to the trailing positions of each step (p2p_thread.cpp: db_start = end - logits_count) because HF generate passes logits_to_keep=1 and vLLM computes one logit per request. Comparing full ranges therefore fails on every prefill with a prompt longer than one token, so out.dmi_internal.logits raised IncompleteInternalError on every real generate call -- and retry=True could never clear it, because the data was already complete. Validate that act on range ends instead, which still detects a missing step. A test parses p2p_thread.cpp and fails if a second act is ever narrowed, so the exemption cannot silently rot. The per-layer check used the highest *present* layer as its reference, so the reference followed the damage: a layer that lost rows simply lowered the bar. Check every layer instead, grouped in one pass over rows already held, and name the offending layer in the error. Measured over 300 randomised generate scenarios (batch size, prompt length, decode count, EOS strip and logits_to_keep varied) driven through a port of the native write path: false alarms on complete captures : 144/300 -> 0 single-row losses detected : 58.8% -> 98.8% cost : 0.31ms -> 0.44ms per read of 1536 rows The remaining 1.2% are one shape: a layer that wrote no rows at all leaves nothing to compare against, so only a declared count catches it. That limit is documented and pinned by a test rather than left as a surprise. Pipeline parallelism was checked separately: a lagging rank was already caught before this change, is still caught, and clears under retry=True. _actual_ranges_for_request is left unused by the new grouping helper and is removed here. Co-Authored-By: Claude Opus 5 <[email protected]>

close() read the drop and suppressed-submit counters only inside the raise_if_failed() except-handler, so loss without a worker crash was silent. That is the common case: the native P2P thread's submit-failure warning fires at most once per process (std::call_once), so the counters were the only quantitative signal and they were gated behind an unrelated failure. Measured against a live ClickHouse with the CPU-only host build: 3000 rows submitted into a queue capped at 1 with OnFullPolicy.DROP, worker healthy. The counters reported 2998 dropped, and ClickHouse held exactly 2 rows -- the counter corresponds to real loss, not just to a number moving. Before this change close() returned None and reported nothing. Add a SinkStats snapshot and MonitoringEngine.sink_stats(), readable mid-run and cached across teardown. close() now raises when suppressed submits are nonzero -- an exception the P2P thread swallowed is never a configured behaviour -- and warns when rows were dropped, since OnFullPolicy.DROP is a caller's deliberate choice. A healthy run stays silent: 50 rows submitted, 50 stored, no raise and no warning. Only the close() that actually held engines reads and reports the counters, so a repeat call neither overwrites the snapshot with zeros nor re-reports it. close() stays idempotent and sink_stats() keeps returning the first snapshot. When a teardown failure and a sink failure coincide the sink failure is raised and the earlier error is attached as __context__ rather than discarded. Note that close() can now raise where it previously could not, and every in-repo caller invokes it from a bare finally:. The hazard is documented; the call sites are deliberately left for their owners, since whether each should swallow, log or propagate depends on what that script is for. Co-Authored-By: Claude Opus 5 <[email protected]>

profiling() and reset_metrics() both take the engine's profiling mutex, which the C++ insert workers also hold. Every other blocking method on this class is bound with py::call_guard<py::gil_scoped_release>(); these two were not, so they held the GIL while blocking on a mutex the ingest path needs. No deadlock is possible today because the insert workers never acquire the GIL, so this is consistency rather than a live bug. Co-Authored-By: Claude Opus 5 <[email protected]>

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

+                # Prefer the sink failure -- it is the more actionable one --
+                # but keep an earlier teardown error visible in the traceback
+                # instead of discarding it.
+                sink_error.__context__ = teardown_error


+
+| Field | Meaning |
+| --- | --- |
+| `dropped` | Rows the insert queue discarded, via a configured `OnFullPolicy.DROP` or because the engine was already stopping. |


Initial plan

3971e78

Copilot AI assigned Copilot and zaoxing Aug 24, 2026

Copilot AI linked an issue Aug 24, 2026 that may be closed by this pull request

Silent data loss #100

Open

Copilot started work on behalf of zaoxing August 24, 2026 19:55 View session

Surface host sink failures loudly

cc6683c

Co-authored-by: zaoxing <[email protected]>

Copilot AI changed the title ~~[WIP] Fix silent data loss in native host sink~~ Surface native host-sink failure instead of silently truncating captured activations Aug 24, 2026

Copilot finished work on behalf of zaoxing August 24, 2026 20:03

Copilot AI requested a review from zaoxing August 24, 2026 20:03

zaoxing marked this pull request as ready for review August 24, 2026 22:52

Copilot AI lite review requested due to automatic review settings August 24, 2026 22:52

zaoxing marked this pull request as draft August 24, 2026 22:52

Copilot started reviewing on behalf of zaoxing August 24, 2026 22:53 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

zaoxing requested a balanced review from Copilot August 24, 2026 23:07

Copilot started reviewing on behalf of zaoxing August 24, 2026 23:08 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

zaoxing and others added 4 commits August 24, 2026 22:27

zaoxing requested a balanced review from Copilot August 25, 2026 13:01

Copilot started reviewing on behalf of zaoxing August 25, 2026 13:02 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Navigation Menu

Uh oh!

Surface native host-sink failure instead of silently truncating captured activations - #101

Surface native host-sink failure instead of silently truncating captured activations#101
zaoxing with Copilot wants to merge 6 commits into
mainfrom
copilot/fix-silent-data-loss

Copilot AI commented Aug 24, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Copilot AI commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Aug 24, 2026 •

edited

Loading