perf(capture): end-to-end native write path, opt-in via NativePackSink - #127
Merged
Conversation
…ling Baseline (5955WX, 10k x 64KiB, median of 5): pipeline 0.235 spool / 0.232 direct GiB/s. Stage attribution: writer alone 0.359, spool stage 1.15, store put 0.199 (fsync-bound) — the ~35% pipeline-to-writer gap is GIL + queue handoff; inside the writer 29% is asdict+deepcopy metadata dict building and 24% is crc32. Native kernel ceiling (g++ 11.4 -O3 -march=native): CRC32C 8-chain 13.7 GiB/s (6.5x zlib), sha256 2.09 GiB/s (parity), append memcpy 2.6 GiB/s, footer 247 GiB/s, spool 0.88 GiB/s fsync-bound. Modeled native ceiling ~1.5-2.0 GiB/s/instance (~8x Python) — clears the 1.1 GiB/s 3x4090 requirement. Two invalid early harness readings (cache-resident corpus, collapsed timing window) caught, reverted, and logged so they stay dead. Decision: proceed with the full native port. Durable state for the loop lives in tasks/todo.md; harnesses checked in under native/csrc/pack/ and benchmarks/profile_pipeline_stages.py.
PackBuilder (native/csrc/pack/): dmi-pack-v1 writer with canonical JSON (sort_keys, ensure_ascii escaping), slice-by-16 IEEE CRC32, trailer offsets verified against the reference's no-padding struct layout, and the one-pass body||trailer SHA-256 scheme. 7 conformance tests compare native bytes to PackWriter output directly (golden corpus, escaped text, alignment, empty/multi-dim shapes, rejection parity, zlib CRC parity): all green. Throughput: 0.63 GiB/s best-of-5 vs 0.359 Python (+75%). zlib substitution measured and reverted (table 3x faster than system zlib). Conformance runs dependency-free: make -C native build/conformance_main (no CUDA, no torch); gate bench via build/bench_builder.
…ocore s3_sign: dependency-free SigV4 (canonical request, HMAC chain, URI encoding), 11 differential tests vs botocore — identical Authorization headers across methods, subresources, nasty keys, tokens, regions. s3_client: libcurl PUT (single + multipart), GET range, HEAD, DELETE, ListV2, botocore-standard retry taxonomy (5xx/429 + transport, never 4xx), short/oversized range refusal. 8 tests vs a fake S3 that re-signs every request server-side with botocore. libcurl headers come from a local sysroot (CURL_INCDIR/CURL_LIBDIR); runtime libcurl.so.4 ships with the OS. Makefile CPU-only goals build without CUDA.
spool.{h,cpp}: ready naming, hash-then-link staging, idempotent retry,
conflict/capacity errors, recovery with quarantine, removal — the same
on-disk contract, so either implementation drains the other's spools.
7 conformance tests incl. both cross directions (Python PackReader over
native-written entries; native Recover over Python-written ones).
pack_sink: bounded queue (block/drop-newest), linger/size/record/session sealing, flush barriers, terminal close, latched failures, counter snapshot — mirroring HostCapturePipeline. object_key: byte-identical key derivation incl. digest-prefix and UTF-8 rules. 9 conformance tests; the golden corpus flows C++ sink -> spool -> Python PackReader with ids, checksums, and tenant binding intact. CPU suite green (1155). Also: the 4 stdin/stdout drivers now share csrc/conformance/json_scan (~600 lines of parsers deduplicated); fixed a Close/Snapshot self-deadlock the migration exposed.
uploader.{h,cpp}: recover, byte-gated admission over N workers, per-pack
retry with backoff, remove-after-commit, outcomes by position. Same
three integrity gates as the Python put(): preflight HEAD + re-hash
blessing, upload-stream hash check, post-upload visibility.
6 uploader tests against the signature-verifying fake S3: full e2e with
Python PackReader read-back, preflight idempotence (PUT-count proof),
parallel batch, layered retry (transport vs uploader attempts split),
corrupt-staged refusal, byte-gate fast failure.
Packer/stager pairs with scope-hash routing (FNV-1a + fmix64): packers seal into bounded stage queues, stagers drain to the spool, flush barriers travel both stages so flush still means staged-to-spool. Per-worker/stage condition variables replace broadcast notify_all; spool mutex covers accounting only (byte reservation keeps the cap exact); PackRecord carries metadata by pointer (one fewer copy per record, honest streaming-metas bench). NVMe spool, 8 scopes: 0.42 -> 0.52 GiB/s monotonic to N=8 (+121% over the 0.235 Python baseline); tmpfs to 0.65. 11 sink conformance tests incl. N=4 scope isolation and cross-worker flush coverage.
NativePackSink (ring::RecordSink): envelope validation, ATen dtype mapping, slicing, and durable submission with no Python on the capture path. Ships as a standalone torch-CPU module (_dmi_native_sink) with a synthetic-envelope entry point: 19 adapter tests, all ten dtypes. Shared csrc/common/json scanner for all five stdin/stdout drivers (promoted from the conformance-only helper the adapter also needs). Selection: dmi.storage.capture.native_sink.create_native_pack_sink, sharing the reference wire layout. Rollback proven: native-staged packs index through the Python CatalogIndexer with byte-identical descriptors. Checkpoint A: PASS — 74 native tests green, CPU suite 1187 green, N-scaling monotonic to N=8 (+121%), rollback demonstrated.
…line - Native golden-corpus build reproduces the recorded manifest digest. - Same 200-record corpus through HostCapturePipeline and the native sink: equal counts and identical descriptor unions. - Same staged pack through SpoolUploader (boto3) and the native uploader: identical object bytes, DMI metadata, and refs. - Fake S3 fix: no duplicate Content-Length on HEAD (object size stays authoritative with the empty body).
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed correctness issue in the native uploader’s UploadPending result ordering and at least one conformance test that currently asserts nothing (always passes), which weakens the regression gate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an opt-in, end-to-end native (C++) capture write path—pack building, spool staging, and S3 upload—while keeping the existing Python pipeline as the conformance oracle and rollback path. It adds conformance/driver binaries and a substantial test suite to prove byte-identical behavior vs the Python reference.
Changes:
- Add native pack/spool/signing/sink/uploader implementations plus stdin/stdout conformance drivers and benchmarks.
- Add Python tests covering pack byte-equality, SigV4 parity vs botocore, spool interoperability (native↔Python), torch adapter validation, and rollback indexing through the Python indexer.
- Add build plumbing and documentation/ledger updates to support CPU-only CI drivers and performance tracking.
File summaries
| File | Description |
|---|---|
| tests/test_native_spool.py | Cross-implementation spool contract tests (native stages ↔ Python recovers and vice versa). |
| tests/test_native_s3_sign.py | Differential SigV4 signer conformance vs botocore with frozen time. |
| tests/test_native_rollback.py | Opt-in sink selection + rollback proof (native-written packs indexed by Python). |
| tests/test_native_pack_conformance.py | Byte-identical pack conformance tests vs Python PackWriter + golden manifest digest check. |
| tests/test_native_adapter_torch.py | Torch CPU adapter tests for envelope validation, dtype mapping, slicing, and staging. |
| tasks/todo.md | Phase checklist and verification log for native pipeline work. |
| tasks/plan.md | Implementation plan and architecture decisions for the native pipeline. |
| src/dmi/storage/capture/native_sink.py | Opt-in factory/config for selecting the native pack sink; lazy extension loading. |
| native/Makefile | Build targets for conformance drivers/benches and CPU-only goal handling. |
| native/csrc/store/uploader.h | Native bounded parallel uploader interface and data model. |
| native/csrc/store/uploader.cpp | Native uploader implementation (recover → upload → remove) with retry/backoff. |
| native/csrc/store/spool.h | Native spool contract interface mirroring Python DurablePackSpool. |
| native/csrc/store/s3_sign.h | Dependency-light SigV4 signing API matching botocore behavior. |
| native/csrc/store/s3_sign.cpp | SigV4 canonicalization/signing implementation. |
| native/csrc/store/s3_client.h | libcurl-based S3 client interface (HEAD/GET-range/PUT/multipart/LIST/DELETE). |
| native/csrc/store/conformance_store.cpp | JSON protocol driver for store/uploader conformance tests. |
| native/csrc/store/conformance_spool.cpp | JSON protocol driver for spool conformance tests. |
| native/csrc/store/conformance_sign.cpp | JSON protocol driver for signing conformance tests. |
| native/csrc/sink/record_row.h | Row validation/submission API (metadata JSON + sliced payload → PackSink). |
| native/csrc/sink/record_row.cpp | Row metadata parsing and validation implementation. |
| native/csrc/sink/pack_sink.h | Core native sink pipeline (queue → packer(s) → stage queue(s) → spool). |
| native/csrc/sink/object_key.h | Native object-key derivation API matching Python pipeline rules. |
| native/csrc/sink/object_key.cpp | Object-key derivation implementation (quoting + date segment). |
| native/csrc/sink/native_pack_sink.h | ring::RecordSink adapter interface for native envelope ingestion. |
| native/csrc/sink/native_pack_sink.cpp | ring envelope validation + slice resolution + row submission into PackSink. |
| native/csrc/sink/conformance_sink.cpp | JSON protocol driver for sink conformance tests and helpers. |
| native/csrc/sink/bindings_sink.cpp | Pybind/torch extension exposing NativePackSink for pytest adapter tests. |
| native/csrc/sink/bench_sink.cpp | Throughput benchmark harness for PackSink at various worker counts. |
| native/csrc/pack/pack_builder.h | Native pack writer public API + format constants and metadata model. |
| native/csrc/pack/conformance_main.cpp | JSON protocol driver for pack build and CRC32 conformance. |
| native/csrc/pack/bench_sha256.cpp | SHA-256 micro-benchmark harness. |
| native/csrc/pack/bench_kernels.cpp | Kernel ceiling micro-bench harness (CRC/SHA/memcpy/footer/spool write). |
| native/csrc/pack/bench_crc32c.cpp | CRC32C micro-benchmark harness. |
| native/csrc/pack/bench_builder.cpp | PackBuilder append+seal throughput benchmark harness. |
| native/csrc/common/json.h | Shared lightweight JSON scanning/escape/base64 helpers for drivers. |
| native/csrc/common/json.cpp | Implementation of shared JSON helpers. |
| docs/benchmarks.md | Performance ledger documenting baselines, experiments, and results. |
| benchmarks/profile_pipeline_stages.py | Python attribution harness for pipeline stage breakdown. |
| benchmarks/data/native-pipeline/baseline-spool.json | Recorded baseline artifact for spool-mode Python pipeline. |
| benchmarks/data/native-pipeline/baseline-direct.json | Recorded baseline artifact for direct-mode Python pipeline. |
Review details
- Files reviewed: 47/47 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zaoxing
changed the title
…cpu CI Module-level `pytest.skip(allow_module_level=True)` reported each missing-driver module as a collection skip: tests=0 in the JUnit file, no per-test names. A `skipif` mark on `pytestmark` evaluates at collection and skips each test individually, so a CI skip guard can name exactly what did not run. Six modules converted; rollback and adapter_torch keep the module-level skip because they import the built _dmi_native_sink.so at module scope, which must not execute at collection when the build is absent. Collection now imports botocore unconditionally in those modules (the old guard used to fire first and mask it), so the cpu job installs botocore explicitly — `pip install --no-deps -e .` never pulled it in, and a missing botocore turned into 3 collection errors (exit 2) on the pull-request gate instead of a clean skip. tests/test_ci_guard_skips.py pins the behavior: with the drivers moved aside, the six modules must collect (>0 tests) and skip per test with no "collection skipped" entries in the JUnit file. It is cpu-marked so `make check` runs it. The clickhouse-live guard's selection is untouched; this change is reporting hygiene for the cpu gate, not a fix for that job.
Co-authored-by: zaoxing <2923149+zaoxing@users.noreply.github.com>
Co-authored-by: zaoxing <2923149+zaoxing@users.noreply.github.com>
UploadPending documented "refs[i] pairs with failures[i]: exactly one
is set, in recover() order", but oversized packs were emplaced/pushed
sequentially (positions 0..k-1) while workers wrote outcomes by pending
index. In a mixed batch an oversized refusal was overwritten by a
success placeholder and its position ended with both slots empty;
snapshot counts still said failed_packs=N, so the loss was silent. The
single-oversized-pack tests could not see it (N == k aligns by
construction). The fix sizes both vectors to the recover order up
front and writes refusals into their own slots; workers fill the rest
by index. Reproduced first by
test_mixed_batch_reports_oversized_pack_at_its_position, which failed
with all-empty failures before the fix.
Also: complete test_native_and_reference_agree_on_multi_dim_shape,
which built records and exited without asserting (its sibling calls
_assert_packs_match); teeth verified by mutation. conformance_main no
longer accumulates every payload for the process lifetime in a static
vector — Append copies synchronously, so a per-record local suffices.
Fix the dmi_common namespace closing comments in common/json.{h,cpp}
and _golden_pack's return annotation (it returns a 3-tuple).
bench_sink.cpp is the A5a N-scaling throughput gate cited by the
ledger and tasks/todo.md ("Gate: make -C native build/bench_sink"),
but the target never existed: the citation was unreproducible, and the
stray goal also broke CPU-only goal detection (bench_sink is not in
CPU_ONLY_GOALS, so the goal filter left a residual and demanded CUDA
resolution). Add the rule with the same link set as conformance_sink
and include it in CPU_ONLY_GOALS.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new native storage pipeline spanning C++, Python integration, build tooling, and CI behavior, which warrants final human review despite strong conformance test coverage.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_native_spool.py:114
- Nit:
FilesystemPackStoreis imported inside the test but never used, which adds noise and can trigger unused-import tooling.
This issue also appears on line 121 of the same file.
tests/test_native_spool.py:123
- Nit:
stagedis assigned but never used; this can be simplified to avoid an unused local.
- Files reviewed: 49/49 changed files
- Comments generated: 1
- Review effort level: Lite
zaoxing
force-pushed
the
feat/native-capture-pipeline
branch
from
September 6, 2026 05:59
78ad7bf to
6473871
Compare
zaoxing
added a commit
that referenced
this pull request
Sep 6, 2026
…d it Checkpoint B reviewed against PR #127 and #128 with every criterion re-verified at this tip rather than read off earlier commits: 44/44 ported live x2, CPU 1202, Python oracles 63, clean rebuild zero warnings, rollback and reader-oracle green, and the quorum verifier 12/12 on BOTH legs x2 against a real Keeper-backed two-replica cluster. That last one PR #128's body still calls "not runnable in this environment" -- it was runnable: the harness keeper was already live, so only quorum_harness/server.xml needed starting. Verdict: PASS. Two gaps are now written down instead of implied. The #125 client-side semantics are half-ported. The quarantine half is real and tested; the serialisation and process-binding halves are not, and their three concurrency tests are absent. The header used to argue the driver's one-op-at-a-time loop "is the same serialisation", which holds for the shipped path and not for the class: a host calling publish_snapshot from two threads, or across a fork, has nothing holding it back. catalog_writer.h now states that limit where it applies, B2b carries the status, and Checkpoint B lists it as a deferred follow-up. The byte-identity gate compares `release` and `fence` only, so the risk table's "Textual SQL identity" overstated it: the parameterized statements (committed_pack_ids IN, the manifest arrayJoin INSERT) are semantically equivalent, not textually identical -- native wraps the rendered list in its own parens and omits the driver's comma-space. The risk table now says which is which, and closing the gap properly (a gate over those statements) is recorded beside it. Also still open and named at the checkpoint: the quiet-host re-measure and reference-host re-baseline, which is the only thing between the recorded numbers and a published one.
zaoxing
added a commit
that referenced
this pull request
Sep 6, 2026
The clickhouse-live job got as far as building the sink driver and then failed to compile it: pack_sink.cpp throws std::runtime_error in four places and never included <stdexcept>. g++ 11 on 20.04 reaches it through another header, so the omission was invisible on this host and on clang 21 as well -- neither local toolchain reproduces it. The CI image's libstdc++ does not, and said so plainly: "'runtime_error' is not a member of 'std'". Audited the rest of native/csrc for the same class rather than fixing only the file CI happened to reach, since a transitive include hides it until some other job asks for the target. schema.h and indexer.h declared std::optional members without <optional>; both fixed. bindings.cpp and dmx_host_engine.h carry six more (std::invalid_argument, std::logic_error, std::shared_ptr, std::optional) and are left alone here: they are Phase A / CUDA-path files that this job never compiles, so the fix belongs to #127's surface rather than widening this diff. Verified: the three CI targets rebuild clean (0 errors, 0 warnings), the job's own command gives 107 passed / 4 deselected, CPU gate 1202. The compile fix itself cannot be proven locally -- no local compiler reproduces the failure -- so CI is the verifier for that one line.
…with a default All seventeen FindInt sites migrated. -1 is a legal answer for none of these fields, and every site read it as one: the timeouts, max_attempts and max_workers test "> 0" for "was it given" and fell back to 5s / 120s / 4 / 4, the multipart sizes and the spool's max_bytes kept their defaults, and a range offset, a length, max_keys or a StagedPack counter cast it to 18446744073709551615 -- a list op answered ok:true with next_token "-1", and a staged counter of 2**64 + 1 was blamed on the checksum. Routed through the ok:false/what shape the file header already documents for every error, naming the field. The get, list, upload_one and upload_pending parses are hoisted ahead of their calls so the refusal lands before any request goes out. INT64_MAX and 2**64 - 1 still reach the transport and are refused (or not) by the client's own checks.
All 46 FindInt sites migrated to a checked field_int that throws CatalogError::kValue -- the ValueError shape the file's own captured_bound lambda already refuses an unrepresentable captured bound with, and which the existing try/catch renders. No new response shape. -1 was legal-looking at every one of them. lease_ttl_ns of 2**64 + 1 opened a writer with a TTL of 18446744073709551615 and reported ok:true; the reader's max_rows_to_read/max_bytes_to_read/max_execution_time_s were LIFTED to UINT64_MAX, which is the opposite of a bound, and the statement went to the server that way; and a uint/int query parameter rendered 18446744073709551615 or -1 straight into a statement -- the same defect test_native_catalog_sql_substitute.py pins for the scanner, in the values rather than the walk. New cpu gate: tests/test_native_catalog_integer_bounds.py, which needs no server (substitute is session-less, open never dials, and the reader-bound case points the client at a closed port so a ValueError is distinguishable from a connection error). It pins the union too -- a uint parameter of 2**64 - 1 must still render as 18446744073709551615.
Comments only, no behaviour change. The two commits above described max_bytes / spool_max_bytes as falling back to the 1 TiB default on an out-of-range literal. They do not: that fallback is keyed on ZERO, and -1 casts to 18446744073709551615, so the capacity limit became no limit at all. Only the fields that test "> 0" (the timeouts, attempt and worker counts, the multipart sizes) take a default. The refusals themselves and their tests are unchanged -- this corrects the reason recorded next to them.
Every one of its 89 call sites now uses FindIntChecked, so the wrapper only existed to return -1 for both kAbsent and kOutOfRange -- the conflation the migration was undertaken to remove. Keeping it exported leaves a way back to that bug. HasKey's comment referred to it; it now refers to the -1 that FindIntChecked's callers use for kAbsent, which is the convention that actually collides with a legal layer_number of -1. The past-tense comments elsewhere that explain what FindInt used to report are left alone: they document why the -1 convention exists and that is still true.
The comment claimed "same rule as the Python uploader's up-front refusal" and the header claimed packs over the limit are "refused up front". Neither is true. ParallelSpoolUploader.upload_pending raises before it starts any work; this code records the oversized pack at its own position and uploads the rest, returning no batch-level error. Measured head to head on three small packs plus one oversized: Python raised with 0 objects uploaded and all 4 packs still staged, while this code returned ok with 3 uploaded and 1 staged. Every observable differs. Behaviour is unchanged -- which policy is right is a maintainer call, and two purpose-named tests plus a docs/benchmarks.md entry pin the current one. Only the false parity claim is removed, in both places, with the divergence named so the next reader is not misled into believing it is already at parity.
The tracked copy stopped mid-run, at the int64 order-statistics finding. It was committed by a concurrent session's `git commit -a` rather than deliberately, so it captured whatever happened to be on disk at that moment and then went stale while the run continued. This is the complete record: all 40 findings with their verdicts, the 3 refuted by execution and the 6 confirmed-but-downgraded with the reason each was not queued, the two findings I reported to the human more strongly than the evidence supported and had to correct, and the methodological problem that distorted three findings -- verifiers reasoning against the combined tree while builders reasoned against one branch of the stack. Kept in the repo because the "deliberately not fixed" list is only useful if a later reviewer can find it and see the evidence, rather than re-finding the same six items and not knowing they were judged.
Consolidating the stacked branches, I reassembled this file programmatically from two whole-file versions and used branch b1's copy as the skeleton. b1 predated the #125 section, so everything the merge base had that b1 lacked was dropped -- silently, from a region 300 lines OUTSIDE the conflict block I was resolving, and with no test failing to signal it. Lost: `# --- #125: the publish protocol's client-side half ---` plus test_two_concurrent_publishes_on_one_writer_are_serialised, test_a_publish_that_fails_releases_the_writer_for_the_next_one, and test_a_writer_used_from_a_forked_child_refuses_to_publish. That left the writer's publish serialisation (the recursive mutex) and its process binding (require_owned_by_this_process) with no coverage on either gate, while tasks/todo.md still recorded the task CLOSED with those tests as its evidence. It also orphaned the two conformance ops that exist only to drive them, publish_concurrent and publish_from_forked_child, which have callers again now. A name-set diff of the merge base against the tip confirms these three were the only casualties. All three pass unmodified against the current tree, which is the point: the production guards were never touched, only the proof that they work. Also repairs the two cosmetic scars the reassembly left (a missing and a tripled blank line at the seams), and stops five comments explaining live behaviour by naming the FindInt wrapper that 8a228ef deleted -- the present-tense ones now name the -1 convention itself, and the historical ones say the wrapper is gone so a reader chasing the name knows why it is not there.
test_a_reader_bound_wider_than_64_bits_is_refused had no accepting counterpart, so it could not tell "refuses the literal it cannot represent" from "refuses this key at all". The file's own docstring promises [-2**63, 2**64 - 1] is legal and round-trips for the reader's read bounds, and nothing asserted it: max_bytes_to_read and max_execution_time_s had no positive test anywhere, and max_rows_to_read only had one at 4242424, far under INT64_MAX. A driver narrowed to INT64_MAX passed the whole file. test_a_reader_bound_keeps_the_whole_union now drives all three bounds at 2**63 and 2**64 - 1 through the dead-server session and asserts error == "ClickHouseError" -- the discriminator that helper's docstring already names, i.e. the value was accepted and the driver went on to dial. The session driving is factored into _watermark_with so both halves use one path. Proof: all six new parametrizations pass on this tree and fail on a copy with json.cpp's ScanInt unsigned limit narrowed to 2**63 - 1, where they report "ValueError: <field> does not fit a 64-bit integer".
test_object_key_keeps_the_64_bit_boundaries_exact looped over captured_at_ns in (0, 2**63 - 1, 2**63, 2**64 - 1) and then asserted "rank=4294967295" in the key -- which comes from producer_rank, fixed at 2**32 - 1 for every iteration. The loop body was identical for all four values apart from `ok`, so the value being parametrized was never observed: a clamp or a wrong wide-value computation of captured_at_ns passed. captured_at_ns does have an observable: object_key_for derives the `date=` segment from it. The test now asserts the WHOLE key against the oracle, the way the sibling test_object_key_parity_with_python does, with the four dates computed through object_key_for rather than hard-coded. That oracle construction is factored into _python_object_key and both tests use it. The docstring also claimed "-0 ... still reach[es] the key builder" while `-0` was untested: session.call goes through json.dumps, and json.dumps(-0) emits `0`. SinkSession grows call_line so the raw literal can be put on the wire by string substitution, the way test_minus_zero_is_still_zero and test_submit_row_parses_the_int64_limits_before_validating_them do it. Proof, both against copies of this tree: - object_key.cpp's DateSegment made signed (a clamp at INT64_MAX): the old assertions all PASS (measured: rank=4294967295 present for every value, dates 1970-01-01 / 2262-04-11 / 1677-09-21 / 1970-01-01) while the strengthened assertion FAILS on the date segment, 2262-04-11 expected against 1677-09-21. - json.cpp's ScanInt made to refuse a negative zero: the strengthened test FAILS on the raw -0 request while its `captured=0` iteration passes.
test_a_query_parameter_wider_than_64_bits_is_refused asserted
`arm in response["message"]` under a comment claiming "the refusal names
the variant arm". Because "int" is a substring of "uint", the `int` arm's
assertion passed against BOTH messages, so for half the parametrization
the check could not fail and a driver that read or named the wrong variant
arm went unnoticed.
The whole message is now pinned by equality, which subsumes the separate
"does not fit" containment check.
Proof: against a copy of this tree whose field_int reports "uint" for the
"int" key -- exactly the wrong-arm scenario -- all five `int`
parametrizations FAIL on the strengthened assertion, while the old
assertions both hold ("int" and "does not fit" are each present in
"uint does not fit a 64-bit integer", measured). All 38 tests in the file
pass on the real tree.
test_search_refusal_parity_on_filter_text closed with an "accepted edge" control that filtered on run_id = "x" * 512 and hook_name = "y" * 512 -- values nothing staged carried. Both readers therefore returned [], the parity assertion was [] == [], and the only assertion with any weight was native["ok"]. The sibling control in test_search_refusal_parity_on_filter_bounds really does select all four rows; this one proved nothing about the 512-byte boundary being handled, only about it not being refused. The staged descriptors now carry the 512-byte run_id, and the 512-byte hook_name on the even rows, so the accepted filter is at the boundary AND selects rows. The control asserts both pages have exactly 2 of the 4 staged rows before comparing them, which also pins that the filter really filters rather than passing everything. Proof: against a copy of this tree whose reader.cpp truncates a hook_name filter to 255 bytes -- an over-tightened text bound, the failure this control exists to catch -- the strengthened test FAILS (assert 0 == 2, native items empty) while the pre-fix version of the same test PASSES on that same build.
FindIntChecked accepts the union [-2^63, 2^64-1] and returns the
two's-complement bit pattern, which is exactly right for the UInt64
counters -- step_number, token_start/end and captured_at_ns are UInt64 in
the catalog and CaptureMetadata admits their whole range. But the union was
applied to every field, including the one field here the oracle types as
SIGNED. record_row.cpp reads layer_number's int64 straight, so the literal
18446744073709551615 arrived as the bit pattern -1, which is that field's
legal "no layer" sentinel, and ValidateMetadata -- which only refuses < -1
or > 2^31 - 1 -- admitted the row. Executed before the fix: submit_row with
that literal returned {"ok":true} and the spooled pack read back through the
oracle's PackReader as layer_number = -1, where CaptureMetadata raises
"layer_number must be an integer in [-1, 2^31 - 1]".
2**64 - 1 is the SINGLE aliasing input: 2**64 - 2 lands on -2 and 2**63 on
INT64_MIN, both already refused by validation. That is why the existing
out-of-range parametrisation (2**64 + 3, -(2**63) - 1, a 40-digit run) never
saw it -- every case it carries is OUTSIDE the union, and this one is inside.
Shape of the fix: the caller declares which half of the union it will accept
(jc::IntDomain::kUnion, the default, or kSigned), rather than range-checking
after the call. A caller-side check cannot work: once FindIntChecked has
returned, the sign of the input is gone, and -1 from "18446744073709551615"
is bit-for-bit the same int64_t as -1 from "-1". The information only exists
while the literal is still text, so the domain has to travel into the scan.
The union is NOT narrowed globally -- kUnion stays the default and the UInt64
counters keep the whole [2^63, 2^64-1] range, which
test_native_pack_conformance.py pins at step_number = 2**64 - 1.
… are
The integer-bounds migration moved the seven scalar metadata fields onto the
checked parse and left the shape dimensions three lines below them on a bare
`v = v * 10 + digit` over a uint32_t. So a dimension over 2**32 wrapped INTO
range and was packed rather than refused. Executed before the fix: metadata
shape [4294967297] with envelope shape [1], dtype uint8 and a 1-byte payload
returned {"ok":true} and packed as shape=(1,); [4294967299] packed as (3,).
The oracle raises "shape dimensions must be integers in [0, 2^31 - 1]".
[2**31] was already refused correctly, by ValidateMetadata, so only WRAPPED
dimensions slipped through -- which is why the existing dimension coverage
never saw it.
The bound is the ACCUMULATOR's, not the field's: it refuses a literal with no
uint32 to hold it, and leaves 0 .. 2**31 - 1 to ValidateMetadata. That is the
same division of labour the scalars have (FindIntChecked bounds at 64 bits,
ValidateMetadata bounds the field), and it keeps the reason reported for
2**31 as validation rather than as an unrepresentable literal -- pinned by
the boundary test added alongside.
On sharing pack_index.cpp's validated shape parser instead: read it, and the
two boundaries genuinely differ. parse_shape/parse_integer re-read footer
bytes written by ANOTHER process at query time, so they carry a JsonInteger
that keeps sign and saturation separate in order to reproduce
CaptureMetadata's exact refusal ORDER and messages, and they report by
throwing CatalogError. This is local admission of a row this process is about
to write, reporting through a bool and an error string, with the pack layer's
own ValidateMetadata as the field validator. Sharing would mean lifting
JsonInteger and the catalog error taxonomy into common/ and pulling the
catalog's message ordering into the sink's admission path -- across a PR
boundary, for two callers that want different answers. What is shared is the
technique: test the bound before the multiply, which all three sites now do.
…drivers
The same aliasing the row path carried, at the three driver decodes that
never went through SubmitRow's parse. All three executed before the fix:
conformance_catalog `write_descriptors` -- render_descriptor_row rendered
the bit pattern -1 into the Int32 layer_number column; live, the insert
succeeded and SELECT read the row back as ('cap-...', -1).
conformance_sink `submit` -- ParseMetadata admitted the record:
{"ok":true,"admission":"accepted"}, persisted_records 1.
conformance_main `build` -- returned a SEALED pack, {"ok":true} with a
record_count of 1, for metadata CaptureMetadata refuses to construct.
Fixed with the same shape as the row path: the caller declares the domain
(jc::IntDomain::kSigned) at the parse, because the sign of the literal is
gone by the time the value is returned. The catalog driver's refusal says
"signed 64-bit integer" for that domain so the message is not misleading --
18446744073709551615 does fit 64 bits, just not a signed field.
NOT changed, deliberately: the UInt64 columns. The reviewer verified
ClickHouse accepts -1 for a UInt64 column and stores 18446744073709551615,
so the signed rendering of step_number/captured_at_ns/object_bytes
round-trips exactly. Those stay on IntDomain::kUnion.
Also NOT changed: read_params' "int" arm in conformance_catalog.cpp. It is a
harness variant SELECTOR rather than a field with an oracle type, and
test_a_query_parameter_wider_than_64_bits_is_refused pins its refusal
message by equality. Noted in the report rather than folded in here.
`filters.limit = static_cast<int>(field_int(line, "limit"))` -- at both the
`select` and `search` sites -- reopened the bound it feeds. The reader
refuses limit < 1 or > 10000, but the cast truncates first, so a value above
2**32 that is not a multiple of 2**32 lands back INSIDE the band. Live before
the fix: limit=4294967297 returned {"ok":true,"items":[]} with the statement
executed at row_limit 2, and limit=4294968296 likewise. CaptureQuery raises
"limit must be between 1 and 10000" for both. 4294967296 and 2**64 - 1
truncate to 0 and -1, which the band already refuses -- which is what hid it.
The width is now checked before the cast and refused with the bound's OWN
message, since that is the message the oracle raises for every value outside
[1, 10000]. No aliasing is left: any literal in [2**63, 2**64 - 1] has a
negative bit pattern and any in (INT_MAX, 2**63) is outside the int, so
nothing above INT_MAX can reach the band.
Second half of the same line, and yes it is genuinely the same line's
problem rather than a separate behaviour question: the assignment was
UNCONDITIONAL, so an absent `limit` overwrote SearchFilters' initialiser of
1000 with field_int's kAbsent -1 and every limit-less query was refused,
where CaptureQuery defaults limit to 1000. The key is presence-guarded now,
exactly as reader_config already guards its three read bounds. Both halves
are one decode of one field, so they are one commit.
field_int stays on IntDomain::kUnion here on purpose. Under kSigned,
limit=2**64 - 1 would report "does not fit a signed 64-bit integer"; with the
union plus the width check it reports the band message, which is what the
oracle raises. The signed domain buys nothing a field whose legal range
excludes every negative can use.
`return found == jc::IntFind::kOk ? value : -1;` sat directly under a HasKey
presence loop, so by the time the scan reported kAbsent the key WAS there and
simply was not an integer -- and every such value took the historical -1. For
layer_number, -1 is the legal "no layer" sentinel, so it was admitted:
executed before the fix, `"layer_number": null` and `"layer_number": "abc"`
both returned {"ok":true} and packed layer_number = -1, where the oracle
raises PackFormatError "invalid capture metadata: layer_number must be an
integer in [-1, 2^31 - 1]". Twenty of the thirty-five (field, literal) pairs
the new test drives were admitted; the other fifteen were refused only
because static_cast<uint64_t>(-1) happened to fail a UInt32 bound or the
token_end >= token_start check.
The original integer-bounds migration deferred this as "a separate parity gap
and not this change's business". It is this change's business now: deleting
the FindInt wrapper made this -1 the ONLY sentinel left in the parse, so
there is nothing else for the convention to be consistent with. Refusing it
retires the sentinel entirely -- all three outcomes of the scan are now
either a value or a refusal -- and the fallback is 0 rather than -1 so no
reader can mistake it for a meaning.
Ordering: out-of-range is reported before not-an-integer, because it is the
more specific answer for input that IS an integer literal, and one row can
only carry one refusal.
`parse_u64_field` is the catalog's one shared decimal-field parse. It threw
`ClickHouseError` on a non-digit and had no bound at all on the accumulator,
so a field too wide for UInt64 wrapped modulo 2**64 and answered a
plausible-looking number for a value it could not represent.
What it parses, per call site: `watermark` (max(index_version)), `version`,
`schema version`, `index_version`, `count`, the four lease fields and the
server clock, and hydration's `offset`/`stored`/`decoded`/`bytes`/`records`
and shape dimensions. All of those but one are UInt64/UInt32 columns coming
back from ClickHouse, and a UInt64 renders in at most 20 digits with
18446744073709551615 as its maximum -- so from a well-behaved server the
new arm is UNREACHABLE, and for those callers the bound is defence in depth.
The exception is what makes this a live defect: `get_by_ids` hands this
function a CALLER's watermark string, gated only on "non-empty and all
digits". Executed before the fix, against the live catalog with head 7:
watermark "18446744073709551616" (2**64) returned
{"ok": true, "items": []} -- it wrapped to 0 and resolved against a snapshot
admitting nothing -- and 2**64 + 7 wrapped to 7 and returned the FULL result
of the published snapshot. Both slid under the published-head guard, which is
the only thing standing between caller data and the snapshot it reads.
`_parse_watermark` raises ValueError("watermark must fit UInt64") on both.
A wrap that lands ABOVE the head is masked by the head guard's own refusal,
which is why the witnesses are chosen to land at or below it.
Two changes, because the two callers want different answers:
- clickhouse_client.cpp bounds the accumulator and refuses through the
ClickHouseError it already raises for a non-digit, with the bound tested
BEFORE the multiply the way json.cpp, record_row.cpp and reader.cpp all
do it. After the multiply the literal is already gone.
- reader.cpp adds the oracle's OWN width refusal ahead of the parse, so the
caller-data path answers in the reader's ValueError taxonomy with
_parse_watermark's wording rather than surfacing a transport-layer
ClickHouseError. Leading zeros are stripped before the width is judged,
because they are insignificant to `int()`: "0000" + str(2**64 - 1) is 24
digits and the oracle accepts it. 2**64 - 1 still reaches the head guard,
and "0007" still resolves at head 7 -- both pinned, so the check cannot
pass by refusing everything wide.
Two of the four remaining sites in this class, both in conformance_sink.cpp. `record_row.cpp` was fixed for exactly this at f3d1868; these follow it, so the shape parsers agree rather than each inventing a bound. The oracle is CaptureMetadata: "shape dimensions must be integers in [0, 2^31 - 1]". ParseMetadata (the `submit` mapping path). The seven scalars go through the checked parse and the shape dimensions seven lines below them accumulated into a bare uint32_t. Executed before the fix, against a fresh sink: shape [4294967297] returned {"ok": true, "admission": "accepted"} with persisted_records 1, and [4294967299] the same -- admitted and packed as (1,) and (3,). submit_row's ENVELOPE shape. A FOURTH accumulation of the same class, over an int64_t, that no reported site named; it was found by grepping the class rather than by chasing the reports. It wraps modulo 2**64, so the 32-bit witnesses do NOT reach it: 4294967297 fits an int64 and correctly answers "envelope shape != metadata shape". What aliases is a literal past 2**63 - 1, and it aliases ONTO the metadata's own dimension -- which is precisely what carried it past SubmitRow's envelope/metadata agreement check and into a pack. Executed before the fix: envelope [18446744073709551617] against metadata shape [1] returned {"ok": true} with persisted_records 1; [18446744073709551619] against [3] and [18446744073709551616000007] against [7] the same. Signed overflow is undefined behaviour besides, which is the second reason the bound is tested before the multiply and not after. Both bounds are the ACCUMULATOR's, not the field's, as record_row.cpp's is: they refuse a literal with no uint32 (resp. int64) to hold it and leave the narrower field range to the layer that owns it. So 2**31 .. 2**32 - 1 still answers with the pack layer's validation, and 2**31 .. 2**63 - 1 still answers "envelope shape != metadata shape" -- pinned by two boundary tests alongside, so neither op can pass by refusing every wide dimension. Both report through the driver's existing out-of-range latch, so the refusal lands BEFORE its side effect: ParseMetadata's is already checked ahead of `submit`'s Submit, and submit_row grows the same check ahead of SubmitRow.
The fourth and last unbounded site in this class. `meta_shape` accumulated each dimension into a bare uint32_t while the scalars beside it went through the checked parse, so a dimension over 2**32 wrapped INTO range and the build SEALED a pack. Executed before the fix: shape [4294967297] returned ok=True record_count=1 and packed as (1,), and [4294967299] as (3,). CaptureMetadata raises "shape dimensions must be integers in [0, 2^31 - 1]". Same shape as the fix at f3d1868 and the two in conformance_sink.cpp: the bound is tested BEFORE the multiply, and it is the ACCUMULATOR's rather than the field's. So a literal with no uint32 to hold it is refused as unrepresentable, and 2**31 .. 2**32 - 1 still answers with the builder's own validation status -- pinned by the boundary test alongside, together with 2**31 - 1, the widest dimension CaptureMetadata admits, which must still seal a pack so the op cannot pass by refusing everything wide. The report rides the existing out-of-range latch, which is already tested after the metadata decode and before builder.Append, so the refusal lands before its side effect and no pack exists.
Samfisheryu
reviewed
Sep 8, 2026
Samfisheryu
left a comment
Samfisheryu
left a comment
Collaborator
There was a problem hiding this comment.
Re-review of the complete #127 stack at 8748b91 (now including #128/#129).
Confirmed fixed: cross-parameter SQL substitution, cursor timestamp/filter bounds, and the original shape/dtype/conflict/limit regressions. Verification: 1,427 CPU tests, 129 live-ClickHouse tests, and 10 legacy reference/ClickHouse single-GPU Ring E2E tests passed.
Not ready to approve yet: the explicit native sink still fails the real Ring RecordSink check (follow-up on the existing inline), and the inline findings below reproduce capacity, hydration and metadata correctness gaps. Synthetic-envelope tests do not establish live Ring E2E. S3 tests used the signature-checking fake, not live Garage.
One scope clarification: the PR body says explicit record_sink only, but engine.py still auto-creates the native sink when storage_backend="capture" and capture_sink_config is provided. The legacy ClickHouse path is not replaced; its E2E tests passed. Please align the description with the intended selection behavior.
The documented INT64_MIN abs_max_int saturation is an intentional contract choice, not counted here as an unacknowledged bug.
Eight findings, each with a regression test:
* sink bindings: NativePackSink now derives from the MAIN backend's
ring::RecordSink registration (pybind11 shares one registry across the
two extensions) instead of registering its own -- globally it collided
("RecordSinkLease is already registered"), module-locally it imported
but was not a `_native_backend.RecordSink` and had no `_acquire_engine`,
so create_record_runtime refused it. The Python loader loads the main
backend first; the module falls back to module-local stand-ins only on
hosts without it. GPU test attaches the sink to a real Ring.
* spool: committed bytes and in-flight reservations are two accounts; the
reconciling directory scan overwrites only the first, so a concurrent
stager's reservation survives it (A reserves 1000, B's 1000 trips the
scan against a 1500 limit -> B refused, 1000 on disk). The serial
upload/remove reconciliation is kept. C++ test with a stage seam.
* hydration budget: each footer range (trailer, then the footer at the
trailer's declared length) is charged against the byte and request
budgets BEFORE it is fetched, through a charge hook on
read_pack_descriptor_rows -- byte_limit=16/request_limit=3 refuses.
* footer binding: the footer row is decoded into typed fields (sql_quote's
inverse, NULL as a kind, toUUID unwrapped) before comparison, so
backslash/quote/tab hook names bind, and the string "NULL" no longer
matches a catalog SQL NULL. Session-less driver op pins the decoder.
* scalars: shape=[] is one element in the sink parser and in
summarize_core's shape_product; a missing shape is still refused.
* integers: FindIntChecked requires a complete integer token (0.5 and
123.5 are refused, not truncated) and IntDomain::kUnsigned refuses a
negative literal for the six unsigned counters instead of wrapping.
* JSON: a 😀 surrogate pair decodes to one four-byte code point.
The previous commit made the FOOTER side of hydrate's binding null-aware but left the catalog side reporting the text "NULL" for a SQL NULL, so every capture with adapter_revision=None mismatched on field 10 (13 live failures), and the string-vs-NULL case still bound. The distinction only exists while the quotes are in the text, so it is kept in the reader's resolved-tuple parser: an unquoted NULL token becomes the empty string, a quoted 'NULL' stays the four characters. Empty is an unambiguous sentinel -- adapter_revision is the schema's one Nullable column and both validators require non-empty text -- and it puts the two readers on one representation, since Python already maps the column to None. The parity helper no longer tolerates "NULL" for that reason. parse_tsv_tuple is declared in reader.h and reachable through a new session-less `tuple_fields` driver op, so this representation is pinned on the CPU gate instead of live-only: that is where the defect was. Also: * native_sink.py reaches _load_extension through getattr -- a test stands a stub module in for dmi.transport.native, which has no such attribute (AttributeError, 1 cpu failure); * the sink base-class test asserts the stand-in branch instead of skipping on it, since the cpu gate fails any skip that is not absent hardware.
The sink extension binds its RecordSink base when it initialises and the
choice is final for the process. Measured against two extensions built from
one pybind11, in all three orders:
main then sink -> isinstance True, base is the main RecordSink, lease
inherited, sink registers no RecordSink of its own
sink alone -> stand-in branch, lease works, no crash
sink then main -> NO registration collision, isinstance False
The third is the trap: create_record_runtime refuses with "record_sink must
be a native RecordSink", which says nothing about import order, and the
extension cannot be re-bound in that process. The loader now detects the
combination -- stand-ins bound while the main backend IS reachable -- and
refuses with that sentence instead. Unreachable through this loader, which
loads the main backend first; it costs one comparison to say so.
Tested in test_engine_runtime_api.py: the mismatch is refused, and both
legitimate states (stand-ins with no backend, a real base with one) still
load, alongside the stub-module tolerance the earlier fix needed.
Samfisheryu
approved these changes
Sep 10, 2026
Samfisheryu
left a comment
Samfisheryu
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at 878a6a4; no remaining blocking findings in this review.
Pushed two small, reproduced spool fixes: upload polling no longer deletes an active writer’s .open file, and repeated removal no longer releases another pack’s capacity. Both regression tests failed before the fixes and pass now. Crash cleanup remains explicit, with other writers stopped.
Validation: 1,462 CPU tests, 136 live-ClickHouse tests, and 16 single-GPU E2E tests passed. The GPU checks include 288 CUDA Graph replays with byte-for-byte tensor readback and the legacy reference/ClickHouse paths. No GPU Ring or legacy ClickHouse implementation changes.
S3 validation used the signature-checking fake, not live Garage. Hosted CI for this commit is still running.
zaoxing
added a commit
that referenced
this pull request
Sep 10, 2026
Two conflicts, both where main's #127 (native capture write path) and this branch added to the same block; both resolved as the union, since the two sides configure unrelated things: - .github/workflows/python-checks.yml: main added the botocore/boto3 pip lines the native capture suites need at collection time; this branch added pyyaml and the [ui] extra (fastapi/uvicorn/httpx). Both jobs need both sets. Keeping only one side would trip main's own new gate, which fails the build when a cpu test skips for anything but absent hardware -- the configurator's API contract suite skips itself without fastapi/httpx, and the native suites error at collection without botocore. - Makefile: main added PYTEST_ARGS (so CI can pass --junit-xml to the gate); this branch added MODEL for the `make ui` target. Independent variables in the same header block. No src/ file was touched by both sides, so nothing else conflicted.
zaoxing
added a commit
that referenced
this pull request
Sep 11, 2026
The /polish loop's working files -- polish-seen.md and polish-state.md -- rode into main with #127. They are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale the moment the branch they describe lands. Remove both and ignore .loop/ alongside .claude/ so a later run cannot commit them again. Co-authored-by: Alan Liu <zaoxing@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replaces the Python capture-path on the hot side with a C++ implementation
that is byte-identical to the reference. The Python pipeline stays as the
permanent conformance oracle and the rollback target.
Why
Per-instance: a 3×4090 host needs 1.1 GiB/s; 1.5–2.0 GiB/s modeled ceiling
clears that with 2–3 instances. The design doc's adopted 0.235 target is
beaten with margin in every reading.
TDD evidence (every fix went RED first)
Whole-stack CPU suite is 1228 passed / 0 failed / 0 skipped, and the
live suite 188 passed with one pre-existing environmental failure (see
"CI story").
Per-module map — every line in the table has a test in the cited file
that was failing before the implementation went in:
pack_builder.cpp(dmi-pack-v1writer)test_native_pack_conformance.pyPackWriteron 4 corpora incl. the recordedtests/data/capture_golden_manifest.jsondigest53a087…; CRC32 vs zlib; shape/empty/rejection parityspool.cpp(.open/.ready/quarantine)test_native_spool.py.openremoval, accounting, component-wise key validation, symlinked-ancestor containmentpack_sink.cpp(queue → assembler → spool)test_native_pack_sink.pyHostCapturePipelines3_client.cpp+s3_sign.cpptest_native_s3_client.py+test_native_s3_sign.pyuploader.cpp(parallel + preflight)test_native_uploader.pynative_pack_sink.cpp(torch adapter)test_native_adapter_torch.pynative_sink.py(selection)test_native_rollback.pyLive-catalog re-run on rebased origin/main (per the TDD verification
checklist — rebase is a code change, the suite re-runs):
This includes the two new tests that drove #125 (per-writer publish
serialization, process binding) — Phase A's native write path never
touches the catalog, but the rollback test (above) uses the Python
CatalogIndexer, so the catalog fix lands cleanly underneath Phase Aand Phase B (the native catalog port) inherits the new contract.
Full suite after rebase:
Design choices, in ledger
docs/benchmarks.mdrecords every kept idea and every dead one:"scaling" — a real bug); fmix64 finalizer is the fix
notify_allon every submit was a 180k-wakeup per-trialthundering herd at N=8; per-worker + per-stage CVs fixed scaling
Spool::Stagecovers accounting only; byte reservationkeeps the byte cap exact
.dmi-pack.readysuffix check was 15 chars when it should be 16 →every ready file invisible to recovery; the test caught it
Close()callingSnapshot()under the held lock self-deadlocked(the test caught it on the first
native-build conformance_sinkrun)Content-Lengthon HEAD → boto3 head-to-head test caught it; S3-faithfulness improved for every client test
".."was tested as a substring of the whole object key, not as apath component, so the legal key
tenant=a..bwas refused — and therefusal fails the stager thread, which closes the sink for every
later submit. Python stages that key fine. Now a component-wise walk
against Python's
_KEY_COMPONENT, which also narrows native forkeys carrying other illegal characters
pack be written outside the spool root and acknowledged as durably
staged — then
recover()could not see it, becauserecursive_directory_iteratordoes not follow directory symlinks.The pack was never uploaded and its bytes stayed charged against the
budget forever. Now
weakly_canonicalcontainment before the lock,matching the oracle's
resolve(strict=False)Post-review polish round (the last 24 commits)
A four-lens review of
native/csrc/(correctness, consistency,test-coverage, simplification) reported 40 findings across the stack.
Each was handed to an independent adversarial verifier whose job was to
refute it; only verifier-confirmed
correctness/requirementfindings were fixed. 3 were refuted outright and 6 more were confirmed
but downgraded to
optional— those are listed below rather thanfixed, so a reviewer can tell "judged" from "missed".
Because #128 and #129 merged mid-review, these fixes were written
against the three separate branches and then replayed onto this one.
The conflicts were resolved to keep BOTH sides wherever both carried a
real check — see the
test_the_inline_identity_list_is_byte_identical_too/
test_the_parameterized_statements_are_byte_identical_toopair, whichwere two same-named tests covering different paths (the inline member
list vs the
%(name)sparameter path); both survive, renamed.What landed, by area:
spool.cppa..bcomponent check (a legal key latched the whole sink);weakly_canonicalcontainment (a symlinked ancestor wrote outside the root and lied about durability)common/json.cppcatalog_writer.cpp/clickhouse_client.cppCURLOPT_POSTFIELDSIZE_LARGE(a NUL truncated the statement on the wire); single-pass%(name)ssubstitution (a value spelling a placeholder was rewritten by a later parameter's pass)pack_index.cpppack_idvalidated as a UUID instead of interpolated rawschema.cppindexer.cppreader.cppcaptured_at_ns,vandwall bounded (each wrapped modulo 2^64); leading zeros refused; the four missingCaptureQuerybounds and the oracle's text validation portedhydration.cppEvery correctness fix went RED first. As one worked example, I re-proved
the two
spool.cppfixes independently of their author by revertingonly that file to
92c2225, rebuilding the drivers, and re-running:The test that passes in both states is the point:
test_object_key_parity_with_pythononly ever compared the keystring and never staged, so a defect that latches the whole sink was
invisible to the test named for object-key parity. It now stages, and
asserts a later submit is still accepted.
Confirmed on this PR's files but deliberately NOT fixed
Recorded so a reviewer does not re-find them. Each was confirmed by
execution and then judged not worth a behavior change here:
s3_sign.cpp:75sortsSignedHeadersby the raw map key while lowercasing only the emitted textX-Amz-Meta-Zeddiverges from botocore,Content-MD5does notpack_builder.cpp:353discardsParseUuid's resultNewPackId(), andSinkConfighas nopack_idfield, so there is no injection surface. Python's reader also rejects such bytes on readspool.cpp:34IsUuidaccepts uppercase hex where Python's ready-name grammar is lowercase-onlypack_sink.cpp:64validates onlynum_workersnative_sink.py, whose docstring says so. 3 of the 5 bounds diverge, not "every other bound"uploader.cpp:221records an oversized pack per-pack where the oracle refuses the whole batch up frontok:truewith 3 uploaded and 1 staged. Two purpose-named tests pin the current behavior anddocs/benchmarks.md:483records a related decision, so choosing the oracle's all-or-nothing policy is a maintainer call. What is unambiguously wrong either way is the comment atuploader.cpp:222asserting the parity it violatesspool.cpp:277conflict check outside the write lock::linkreturnsEEXISTrather than overwriting, and the loser'sunreserve()keeps accounting exact. Do not "fix" this by moving the write under the mutex — it would serialise every stager on disk I/O and still not close the cross-process caseWhat is not in this PR (honest scope)
create_native_pack_sinkis opt-in viaMonitoringEngine.create_record_runtime(record_sink=...). The Pythonpath is unchanged for everyone else.
Phase B (native catalog/indexer) is the next step— no longertrue. feat(catalog): Phase C — native reader at parity with the Python CaptureReader #129 merged into feat(catalog): Phase B Stack over Phase A on Native Capture #128 and feat(catalog): Phase B Stack over Phase A on Native Capture #128 merged into this branch, so
this PR now carries all three phases: the native write path (A), the
native catalog and indexer (B), and the native reader and hydration
(C). Read the diff as the whole stack, not as Phase A alone.
A real
RingEnginepath requires the full backend (submodule-initblocked here). Rollback proven via the Python-side indexer instead.
fake, not a real endpoint. Boto3 head-to-head runs against the fake
too (URL-rewritten via the test's proxy slot).
CI story
make test(CPU): 1228 passed.make test-all(full): 7 pre-existing failures unrelated to thiswork — missing build artifacts (gitignored
.sofor_native_backend/
_host_backend, submodules uninitialized here). Same 7 with orwithout this branch.
absent; the torch adapter needs the venv torch CI installs
(
torch>=2.8,<3is inpython-checks.yml).Files (89 changed, +24 436)
native/csrc/{common,pack,store,sink}/**— the implementationnative/csrc/conformance/— moved intocommon/tests/test_native_{pack_conformance,s3_sign,s3_client,spool,pack_sink,uploader,adapter_torch,rollback}.py— TDD tests, all RED firstbenchmarks/profile_pipeline_stages.py,benchmarks/data/native-pipeline/{baseline-*,}.json— attributiondocs/benchmarks.md— perf ledger (every kept and reverted decision)tasks/{plan,todo}.md— plan and durable state (re-read at every cycle)Reviewer's quick path
make test PYTHON=<venv>/bin/python— 1228 passed, 0 failed.pytest -m 'clickhouse and manual and not garage' PYTHON=<venv>/bin/python— live-catalog pass (11, includes Catalog: one writer cannot publish twice at once -- serialise per writer, bind to its process #125's two new tests).
native/build/conformance_{main,sign,store,spool,sink}(CPU-onlydrivers) cover the byte-compatibility, signing, and fault paths
without needing a GPU or ClickHouse.
native/build/bench_sink singleandnative/build/bench_sink interleaved spooldir=/dev/shm/...reproducethe scaling numbers.