Native capture decoder parity, and the tests that hid the gaps by zaoxing · Pull Request #134 · ProjectDMX/DMI

zaoxing · 2026-09-11T14:57:56Z

What

Nine fixes found by re-reviewing the native capture path after #127 merged.
Three are correctness defects; six are tests that passed without proving what
they claimed. Every correctness fix went RED first, and every test
strengthening was proven by mutation — broken the corresponding source, shown
the old assertion passing and the new one failing.

None of this is visible to CI. main is green and carries all nine defects:
two tests pass with their fix compiled out, one character never appears in a
parametrize list, one case needs two Spool objects no test constructs.

The correctness fixes

The two escape decoders disagreed, so a legal capture could not be read
back. unquote_sql (footer side) decodes eight escapes; parse_tsv_tuple
(catalog side) handled four and passed the rest through literally, so \b
decoded to the letter b. A hook_name containing 0x08 is legal — Python's
_validate_text checks only non-empty UTF-8 within 512 bytes — so the pack
stages, uploads and indexes, and then hydrate throws catalog descriptor does not match the pack footer: field 12. The oracle reads over the native
protocol with no TSV layer and hydrates it fine.

The escape set was measured, not assumed: SELECT tuple(concat('block', char(N), 'resid')) for N in 0..127 against a live server. ClickHouse escapes
exactly eight bytes inside a tuple:

0x00 \0   0x08 \b   0x09 \t   0x0A \n
0x0C \f   0x0D \r   0x27 \'   0x5C \\

0x07 and 0x0B travel raw even though sql_quote rewrites them as \a/\v,
so copying all eight cases from the other decoder would have introduced new
asymmetry. Only \b and \f were added; the deliberate default: passthrough
that keeps \' and \\ from double-decoding is untouched.

A lone surrogate was persisted as CESU-8. The surrogate-pair fix on main
handles full pairs, but \ud83d alone still emitted ED A0 BD, and
ValidText checks byte shape rather than surrogate range, so it was packed
and inserted. Python raises UnicodeEncodeError and the record never lands.
Same lambda: \uZZZZ decoded to F0 A5 95 93 where json.loads raises.
The decoder now emits U+FFFD and clears an optional ok flag — the
replacement character alone ends the CESU-8-in-a-pack class regardless of the
caller, while the flag lets both metadata parsers refuse as the oracle does.

The spool byte cap could be exceeded. Stage's retry and EEXIST-loser
paths added nothing to the committed account, on the reasoning that the file
was counted by whoever created it — false for a ready file created by a second
Spool object. Measured at max_bytes=1500: 2000 ready bytes on disk where
Python raises SpoolFullError and stops at 1000.

This one is a reconciliation, not a port. main independently reworked
the same code (Scan/ListPending, plus an inflight_temps_ skip that fixes
a separate hazard). This keeps main's structure wholesale and adds the one
thing it lacks — a path-level ledger answering "has this object counted this
path", which main's local byte accumulator cannot. Remove now uncounts by
path, which is part of the same root cause rather than a drive-by: with a path
ledger, decrementing by size alone would leave a dead path charged and make the
next stage of that pack a no-op. main's
TestRepeatedRemovalDoesNotReleaseAnotherPacksCapacity still passes unmodified.

The tests that proved nothing

Two were vacuous — they pass with the surrogate fix compiled out. The cause is
worth recording: the pack footer writer decodes each UTF-8 sequence and
re-emits \uXXXX, so broken CESU-8 is decoded back to U+D83D + U+DE00 and
rewritten, which json.loads recombines. An exact inverse of the bug. The fix
was pinned only by a manual/clickhouse test — nothing on the CPU gate.

They now assert observables the footer serializer cannot undo: the pack-sink
test echoes the parsed hook_name bytes back through a new session-less
parse_metadata op; the torch test, which cannot reach that op, uses a
hook_name of exactly 512 UTF-8 bytes — CESU-8 is two bytes longer per non-BMP
character, so at the _validate_text limit the broken decoder refuses a record
Python admits.

The rest:

test	what it failed to prove
`test_spool_reservations.cpp`	claimed to cover reservation release; the refusal path returns before taking a reservation, so a leaking `unreserve()` still passed. Now drives the EEXIST loser and asserts a later stage under the cap is admitted
`test_native_sink_ring_e2e.py`	asserted only `persisted_records == 3`, identical whichever transport each record took. Now asserts the `StepReservation` sequence `[RESERVED, OVERSIZED, RESERVED]`
`test_native_adapter_torch.py`	`match="metadata"` matched every metadata refusal and an unrelated one. Now compares the whole message, parametrized
`test_engine_runtime_api.py`	`assert load() is not None` could not fail. Now asserts the module the loader returns
`test_native_hydration_footer_fields.py`	hard-coded width 32; add a column and it keeps passing. Now derived from `CAPTURE_COLUMNS`
`test_native_pack_sink.py`	`assert a.encode() == b.encode()` restated the line above it

Checks

make -C native cpu-goals        0 errors, 0 warnings
pytest -m cpu                   1476 passed, 0 failed, 0 skipped
pytest -m "clickhouse and manual and not garage"
                                202 passed, 1 failed

The one live failure is test_a_role_that_cannot_see_one_object_is_told_to_grant_it_not_to_rebuild,
which needs CREATE USER on the server; the local standalone ClickHouse has no
access management and it passes in CI.

Known, not fixed

tests/test_native_sink_ring_e2e.py never runs in CI. There is no GPU
job in python-checks.yml — only cpu-and-package-layout and
clickhouse-live — and the file is gated on require_cuda() with no cpu
marker, so -m cpu cannot select it and the live job only globs
tests/*_live.py. The strengthening closes the gap for whoever runs it by
hand; it is documentation until a GPU job exists.
hydration.cpp's literal fields.size() != 32 is now the one place
still carrying the hard-coded footer width. Changing it is a production edit
outside these findings.
ValidText still checks byte shape, not surrogate range. The root cause
was the decoder, and no surrogate-range sequence can reach the validator now;
adding a second check there would change behaviour on paths outside this
scope.

hydrate binds every catalog descriptor field against the pack footer, and the two sides run different decoders: the catalog side parses ClickHouse's rendered tuple, the footer side runs unquote_sql over sql_quote's map. The tuple parser covered only \n \t \r \0 and let everything else through on its deliberate `\'`/`\\` passthrough, so \b decoded to the letter "b" and \f to "f" while the footer side decoded them to 0x08 and 0x0C. A hook_name carrying either byte is legal -- _validate_text asks only for non-empty UTF-8 under 512 bytes -- so it staged, uploaded and indexed, and then hydrate refused it with "catalog descriptor does not match the pack footer: field 12". Python reads over the native protocol with no TSV layer and hydrates it fine. The set is measured, not copied from the other switch: `SELECT tuple(concat('block', char(N), 'resid')) FORMAT TSV` driven against a live server over the whole control range shows ClickHouse escapes exactly \0 \b \t \n \f \r \' \\ inside a tuple and leaves every other byte raw -- notably 0x07 and 0x0B, which sql_quote DOES rewrite (\a, \v). So only \b and \f are added; the passthrough that keeps `\'` and `\\` from being double-decoded stays as it is. Pinned on the CPU gate by a new parametrized test that decodes each measured rendering through `tuple_fields` and binds it against `footer_row_fields`, and on the live gate by extending the hydrate parametrization to the full measured set. Before the fix the CPU test failed on bytes 8 and 12 and the live one on the same two with "field 12"; a full 1..127 sweep comparing both decoders now reports no disagreement.

…undo test_a_non_bmp_identifier_survives_the_metadata_decoder and test_a_non_bmp_hook_name_round_trips were vacuous: both still passed with the surrogate-pair combine branch compiled out (`if (false && ...)`, rebuilt -- 2 passed and 26 passed). Both read hook_name back off the staged pack, and the pack footer writer decodes every UTF-8 sequence and re-emits \uXXXX, so the broken CESU-8 the decoder wrote (ED A0 BD ED B8 80) came back out of the footer as exactly the 😀 a correct decoder produces, and json.loads recombined it either way. The staged bytes were traced: b':"block\\ud83d\\ude0', hook_name 'block😀', with the fix OFF. The surrogate work was pinned only by a clickhouse/manual live test -- nothing on the CPU gate. So both now assert an observable the footer serializer cannot reach: * the pack-sink test echoes the parsed RecordMetadata.hook_name BYTES back through a new session-less `parse_metadata` op on conformance_sink (the same shape conformance_catalog gained footer_row_fields/tuple_fields for), as hex, and compares against hook_name.encode("utf-8"); * the torch test gains a hook_name of exactly 512 UTF-8 bytes -- the limit _validate_text and ValidText both apply. CESU-8 is two bytes longer per non-BMP character, so at the limit the broken decoder is refused outright ("invalid capture metadata") where Python admits the record. The oracle assertion is now in the test, so the parity claim is explicit. Measured with the combine branch disabled and rebuilt: the pack-sink test fails with 626c6f636beda0bdedb880 != 626c6f636bf09f9880 and the torch test with RuntimeError "invalid capture metadata"; both pass with it on. Also drops tests/test_native_pack_sink.py:1137, which restated line 1136: two equal `str`s necessarily have equal encodings, so `staged.hook_name.encode("utf-8") == hook_name.encode("utf-8")` could not fail. The real byte-level check is now the parse_metadata assertion above.

…cle does The surrogate-pair combine handles a full pair, but a surrogate with NO partner kept the three-byte form: \ud83d decoded to ED A0 BD and \udcff to ED B3 BF. That is CESU-8, not UTF-8, and ValidText admits it -- it checks lead/continuation byte SHAPE, not the surrogate range -- so the record was packed and inserted. The oracle never gets that far: _validate_text calls .encode("utf-8"), which raises UnicodeEncodeError ("surrogates not allowed"), and no record lands. Measured through the driver, before: hook_name_hex 626c6f636beda0bd / 626c6f636bedb3bf, ok:true. The same lambda's hex4 did not validate its digits -- `h <= '9' ? h - '0' : (h | 0x20) - 'a' + 10` accepts any byte -- so \uZZZZ decoded to U+25553 and was packed as F0 A5 95 93 (626c6f636bf0a59593, ok:true), a name nothing in the request ever spelled. json.loads raises "Invalid \uXXXX escape". Where the refusal lives, and why: the decoder is the only place that can SEE either fault (by the time the bytes exist the information is gone), but it has no error channel and most of its callers read fields that nothing validates, so it must stay total. It therefore does both: it emits U+FFFD, so that whatever a caller does next no sequence that is not valid UTF-8 ever leaves it -- that alone ends the CESU-8-in-a-pack class -- and it CLEARS an optional `bool* ok` so callers that do have an error channel can refuse. The flag is only ever cleared, so one flag latches a whole object, and an absent key leaves it alone. Both defaults are nullptr, so no existing caller changes behaviour. The two metadata parsers pass it and refuse: ParseMetadataJson (record_row.cpp -- the production path, torch adapter through SubmitRow) with "capture metadata text is not encodable UTF-8", and the driver's ParseMetadata, which is the native side of CaptureMetadata.from_mapping, on the same latch the out-of-range integer refusal already uses. dtype is left on the plain decoder: a replacement character there simply is not one of the fourteen dtype names and DtypeSupported already refuses it. Red before green, against the unfixed binaries: all three new cases answered ok:true from parse_metadata and were admitted by submit_row.

The docstring claimed records flowing through the Ring and a comment labelled the second record CPU-direct, but persisted_records == 3, failures == 0 and payload equality are identical whether one record took the Ring or all three went CPU-direct. RecordRuntime.emit_output already returns the answer -- StepReservation.OVERSIZED exactly on the CPU-direct branch -- and the hook was discarding it. Keep it and pin the sequence [RESERVED, OVERSIZED, RESERVED]: measured, and a records.py forced down the CPU-direct branch for every record now fails the test where it used to pass.

match="metadata" is satisfied by every RowStatusName(kBadMetadata) message -- "missing an integer field", "not an integer", "out of range", "failed validation" -- and by the unrelated "descriptor requires metadata JSON followed by one payload slice". Parametrise the reason and compare the whole message, the way tests/test_native_pack_sink.py already does. step_number=-1 must land on OUT OF RANGE specifically: with the kOutOfRange answer folded into not-an-integer the old test still passed and the new one does not.

…-None _load_native_sink_extension either raises or returns _load_named_extension's result, so `is not None` held by construction and would have held for any other object. Check RING_TYPES_ARE_STANDINS the way the two tests above it do: a loader patched to hand back a different object passes the old assertion and fails the new one.

32 is len(CAPTURE_COLUMNS) - 1, right today and silent tomorrow: adding a capture column left the hand-built row at 32 tokens, the assertion at 32, and hydration.cpp's matching `fields.size() != 32` guard drifting with nothing to catch it. Assemble the row per column name and take the width and the five spot-checked positions from the oracle. Proved on a copy with a 33rd column: the derived test names the unrendered column, the hard-coded one still passed.

The retry and EEXIST-loser paths added nothing to the committed account, on the reasoning that "the file was counted by the stage (or process start) that created it". That holds only when the creator was THIS Spool object. A ready file created by a SECOND Spool object on the same root after this one's Open() has never been counted here, so adding nothing judged the cap against 0. main's Recover/Scan split did not close this: the reconciliation scan only runs when the capacity check has already failed, and the retry path returns before any capacity check. Executed, max_bytes=1500, two Spool objects on one root: before: other.Stage(pack1,1000)=ok; writer.Stage(pack1,1000) retry=ok, writer.Snapshot().bytes=0; writer.Stage(pack2,1000)=ok -> 2000 ready bytes on disk against a 1500 cap oracle: after the retry snapshot.bytes=1000, then SpoolFullError "2000 > 1500", 1000 bytes on disk after: snapshot.bytes=1000, then kFull "2000 > 1500", 1000 on disk Python makes the distinction with a SET: stage() runs both paths through _account_ready_locked (spool.py:124 and :159), which is a no-op only when the path is already in _accounted_ready. The aggregate alone cannot decide it, so this ports that set -- accounted_ready_, path -> bytes -- and counts each ready path at most once. It is rebuilt wholesale wherever the committed aggregate it describes is: Open, the reconciliation scan in Stage, and Scan (Recover/ListPending), so the two can never disagree. Remove moves with it, and has to. Uncounting by staged.object_bytes while the path stayed in the ledger would leave the ledger claiming a file that is gone, and the next stage of that pack would then be treated as already accounted and add nothing. Uncounting by PATH drops exactly the bytes this object recorded, keeps main's property that a removal RETRY releases no capacity (the path is already out of the ledger, so the retry is free), and additionally matches the oracle, which unaccounts a path whose file has vanished (spool.py:314) instead of leaving it charged. What is deliberately unchanged: the reservation account, and main's Scan/ListPending structure including its `inflight_temps_` skip. Only committed_* and the new ledger move here. Re-verified by execution after the change -- every Stage failure exit (kFull, kBadArgument, kIo, the EEXIST loser's kConflict) still leaves reserved_* at zero and the cap's room intact; a Recover across a held reservation rebuilds only the committed half; Snapshot() is still committed + reserved; the concurrent-admission case (A reserved and paused, B refused, 1000 on disk, then 500 more admitted) still holds; 20 concurrent 2MiB stages against 1830 Recover() passes lost no in-flight temp (the same harness fails 20/20 with the inflight_temps_ skip removed). peak_bytes_ now also moves on the retry path, which is what _account_ready_locked does. Red first: the two new C++ cases -- a retry of another object's ready file, and an EEXIST loser against another object's winner -- fail 8 checks against main's spool.cpp, with "second pack was admitted: ok", "loser admitted a second pack: ok" and 2000 bytes on disk. The existing case at test_spool_reservations.cpp:211 could not see this: it retries a file the same object staged, which is the half that was already right.

…over TestARefusedStageLeavesNoReservationBehind named the release of a reservation, but a refused stage returns kFull before reserved_bytes_ moves -- there is nothing to release, so replacing the whole unreserve lambda with a no-op left that case printing ok. Split it: the refusal and retry half keeps its (sound) idempotency claim under an honest name and now also shows the refusal left room, and a new case drives the EEXIST loser, the reachable path that really does reserve and then give it back. Its observable is that a 500-byte stage under the 1500 cap is ADMITTED after the loser released 1000, which the leak mutant refuses with "2500 > 1500".

Copilot

🟡 Changes recommended

Unresolved spool capacity and removal-race issues, plus inaccurate malformed-Unicode diagnostics, require fixes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves native capture parity by fixing Unicode/SQL decoding, spool accounting, and regression-test coverage.

Changes:

Corrects escape and surrogate handling.
Adds path-aware spool capacity accounting.
Strengthens transport, metadata, schema, and loader tests.

File summaries

File	Reviewed changes and final notes
`tests/test_native_sink_ring_e2e.py`	Verifies per-record reservation sequences.
`tests/test_native_reader_parity_live.py`	Covers live SQL escape parity.
`tests/test_native_pack_sink.py`	Strengthens Unicode and metadata tests.
`tests/test_native_hydration_footer_fields.py`	Derives footer width from `CAPTURE_COLUMNS`. Nit (1 vote): fix the docstring’s “bar” wording.
`tests/test_native_adapter_torch.py`	Checks exact refusal reasons and byte-limit behavior.
`tests/test_engine_runtime_api.py`	Verifies the loader’s returned module.
`tests/native/test_spool_reservations.cpp`	Adds spool reservation race coverage.
`native/csrc/store/spool.h`	Declares path-accounting state and helpers.
`native/csrc/store/spool.cpp`	Implements path-level accounting. Critical (1 vote): retry/EEXIST accounting must include in-flight reservations. Moderate (2 votes): handle the `ENOENT` removal race by unaccounting the path.
`native/csrc/sink/record_row.cpp`	Rejects malformed metadata text. Moderate (1 vote): report invalid Unicode escapes distinctly from encoding failures.
`native/csrc/sink/conformance_sink.cpp`	Adds metadata parsing diagnostics. Moderate (1 vote): align malformed-escape diagnostics with the parser’s actual failure.
`native/csrc/common/json.h`	Extends decoder error reporting.
`native/csrc/common/json.cpp`	Validates Unicode escapes and surrogate handling.
`native/csrc/catalog/reader.cpp`	Adds tuple escape parity.

Review details

Suppressed comments (3)

native/csrc/sink/conformance_sink.cpp:183

The conformance driver's new diagnostic has the same mismatch as the production row path: \uZZZZ is an invalid escape that gets replaced by valid U+FFFD, so calling it “not encodable UTF-8” misidentifies the input and does not match the parser's actual failure. Keep this driver diagnostic aligned with the production error when the decoder carries a distinct reason.

  const auto refuse_bad_text = [] {
    std::string out = "{\"ok\":false,\"what\":";
    jc::EscapeJson("capture metadata text is not encodable UTF-8", &out);
    std::cout << out << "}\n";

native/csrc/sink/record_row.cpp:70

For a malformed escape such as \uZZZZ, this path returns capture metadata text is not encodable UTF-8, even though the decoder has replaced the invalid escape with U+FFFD (valid UTF-8); the actual problem is an invalid Unicode escape, not an encoding failure. Distinguishing the escape/surrogate failure reason, or using a message that covers both accurately, would make the refusal actionable.

  if (!text_ok) {
    return fail("capture metadata text is not encodable UTF-8");

tests/test_native_hydration_footer_fields.py:6

The updated module docstring reads “one per CAPTURE_COLUMNS entry bar index_version”; “bar” is not grammatical in this construction. Please change it to “barring index_version” (or “except index_version”).

fields of the footer's rendered VALUES row -- one per CAPTURE_COLUMNS entry
bar index_version (pack_index's renderer, SQL
escaping and all) -- against the catalog row the reader returns (TSV escapes

Files reviewed: 14/14 changed files
Comments generated: 2
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+  auto account_existing = [&] {
+    std::lock_guard<std::mutex> lock(mutex_);
+    if (AccountReadyLocked(ready, n)) ++generation_;
+  };


+    // Uncount by PATH: the bytes that leave the aggregate are the ones this
+    // object recorded for it, and a path this object never counted (removed
+    // on behalf of another Spool object) costs nothing, exactly as
+    // _unaccount_ready_locked does. Uncounting by anything else would let a
+    // path stay in the ledger after its file is gone, and the next stage of
+    // the same pack would then be treated as already accounted.
+    UnaccountReadyLocked(staged.path);


zaoxing added 9 commits September 11, 2026 10:18

Copilot AI lite review requested due to automatic review settings September 11, 2026 14:57

Copilot started reviewing on behalf of zaoxing September 11, 2026 14:58 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

zaoxing mentioned this pull request Sep 11, 2026

Dynamic-dim shape inference, upload conflict reporting, and libcurl lifetime #135

Open

Navigation Menu

Uh oh!

Native capture decoder parity, and the tests that hid the gaps - #134

Native capture decoder parity, and the tests that hid the gaps#134
zaoxing wants to merge 9 commits into
mainfrom
fix/native-decoder-parity

zaoxing commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

zaoxing commented Sep 11, 2026

What

The correctness fixes

The tests that proved nothing

Checks

Known, not fixed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants