Dynamic-dim shape inference, upload conflict reporting, and libcurl lifetime by zaoxing · Pull Request #135 · ProjectDMX/DMI

zaoxing · 2026-09-11T17:09:48Z

What

Six defects found by reviewing the native capture path against its own
reference implementation. The first breaks every production capture that uses
a dynamic tensor dimension with a dtype wider than one byte.

Each correctness fix went RED first; the two that are scoping decisions rather
than mechanical fixes are justified below with the evidence that settled them.

Independent of #134 — disjoint files, same base.

`ResolveShape` inferred a dynamic dim in bytes, not elements

native_pack_sink.cpp computed length_bytes / fixed, where fixed is the
product of the non-dynamic dims. The element size was never divided out. The
reference (clickhouse_record_sink.cpp:60-92) does it in two steps, with two
guards this path lacked:

if (length_bytes % element_bytes != 0) invalid("… not divisible by dtype size");
const std::uint64_t elements = length_bytes / element_bytes;
const std::uint64_t inferred = elements / fixed;
if (inferred > int64_max) invalid("inferred tensor dimension exceeds int64");

A float32 slice declared shape=[-1, 4] with 32 payload bytes infers 8
instead of 2. SubmitRow then rejects on shape mismatch and submit() throws.

Why 1,462 tests missed it: bindings_sink.cpp hardcoded
inferred_dynamic_dim = -1, so the conformance driver could not reach the
branch at all — only the production path (bindings.cpp:196) sets a real
dynamic dim. The driver now accepts the field (absent → -1, so every
existing row dict is unchanged), which is what made a red test possible.

The element width comes from the sink's own dtype table, which SubmitRow
already multiplies by for its size check — so the two agree by construction
rather than becoming a second source of truth. The reference's non-dynamic
guard (checked_product(shape, -1) != elements) was deliberately NOT copied:
SubmitRow's existing check is strictly stronger, since it also binds the
resolved shape to the metadata shape.

The one non-retryable upload failure reported an empty string

uploader.cpp builds the pack-conflict diagnostic — "the object store already
holds a different object at <key> … do not overwrite the existing object" —
into last_error, then returns early, skipping the *error = last_error
assignment. UploadPending recorded {pack_id, key, attempts: 0, error: ""}.

Now assigns both, with attempts = 1 (the HEAD that found the conflict),
matching the retry-exhausted exit rather than the byte-gate refusal's 0,
where nothing was attempted.

An oversized record could block forever

Submit screened max_pack_bytes but not max_queue_bytes. With the defaults
(16 MiB queue, 128 MiB pack) a 32 MiB record passes admission, and
queue_bytes_ + n <= max_queue_bytes can never be satisfied even on an empty
queue — so under kBlock with a negative timeout it blocks forever, and under
the other policies it answers kDropped/kTimedOut where the oracle says
TOO_LARGE. Now screened before the wait loop, in the oracle's order.

A pre-existing test was pinning this defect. test_block_with_timeout_counts_timeouts
drove a 64-byte payload under a 63-byte cap and asserted timed_out — which is
the divergence. It was not deleted or skipped: the function was re-pointed at
the boundary the new screen must not overshoot (a record of exactly the cap is
still admitted) and renamed, with the history in its docstring.

Cost, stated plainly: timed_out is no longer reachable from the conformance
driver. Reaching it needs a queue held full by an undrained consumer; the
oracle pins it by stalling its sink with a fault double, and the native driver
has no equivalent — the packer pops under the same lock, so every alternative
is a race. Adding a fault hook would put test-only behaviour in production
pack_sink.cpp. timed_out_records therefore now has native coverage only
through the Python oracle's suite. Worth a maintainer's view.

libcurl was torn down per object

~ClickHouseClient called curl_global_cleanup() — process-global teardown
libcurl documents as unsafe while another thread is using the library.
S3Client never called curl_global_init at all (its only entry is
curl_easy_init inside Exchange), and SpoolUploader::UploadPending runs it
from max_workers threads. Destroying a catalog client while an upload batch
is in flight was undefined behaviour.

Now a shared dmi_common::EnsureCurlGlobalInit() (std::call_once, no
teardown) in common/, which both link lines already carry — so
conformance_store gains no catalog source. Both clients call it from their
constructors; ~ClickHouseClient is = default. Neither public API moves.

No deterministic red test exists for this, and none was fabricated.
Reproducing it needs a destructor racing curl_easy_perform; single-threaded,
libcurl's refcounting makes the sequence benign, so any "reproduction" would be
a flake. What is pinned instead: a source invariant (no native source calls
curl_global_cleanup; curl_global_init appears only in curl_init.cpp
behind std::call_once) — which IS red on the pre-fix tree — and a lifetime
guard driving three client constructions and destructions in one process.

A configured capture sink was silently ignored

capture_sink_config was only read when storage_backend == "capture", and the
backend defaults to "auto". A caller setting it and leaving the default passed
the type check and then wrote no packs at all.

This is a deliberate behaviour change, so it was established rather than
assumed:

Does "auto" ever resolve to "capture"? No. _reject_a_sink_the_config_did_not_ask_for
returns early on "auto" and the default-writer branch compares the literal;
there is no resolution step in src/. Under "auto" the sink config is not
merely unused, it is unreachable — so the straightforward check is the
correct one.
Does anything in-repo pair them? No. The only two call sites set
storage_backend="capture" or bypass MonitoringConfig entirely.

So the raise turns a silent no-op into a startup failure and breaks nothing in
the repo. It names both fields and the offending backend.

The live CI job could silently collect nothing

The job selected tests/*_live.py by filename. A new manual/clickhouse
test outside that convention was silently uncollected — and the "fail if any
live test was skipped" gate below it cannot catch that, because uncollected is
not skipped. That is the failure mode the rest of that CI work exists to close.

Now selected by marker. Verified: the marker expression alone selects 199
tests across 9 files, all already *_live.py — identical to the glob, so
nothing expensive or service-dependent is pulled in. A temporary probe file
outside the convention was collected by the marker (200) and missed by the
glob (199), then removed.

Checks

make -C native cpu-goals        0 errors, 0 warnings
pytest -m cpu                   1475 passed, 0 failed, 0 skipped   (main: 1462)
pytest -m "clickhouse and manual and not garage"
                                198 passed, 1 failed

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

Known, not fixed

timed_out_records native coverage, above — the honest cost of fixing the
admission bug, and a maintainer call.
docs/capture-storage-design.md documents storage_backend and could
reasonably note the new refusal; left alone as outside this scope.

ResolveShape divided the raw payload-slice byte count by the product of the fixed dims, so the inferred dimension came out too large by exactly the dtype width: a float32 [-1, 4] slice of 32 bytes resolved to [8, 4] where the reference (clickhouse_record_sink.cpp resolve_shape) says [2, 4]. SubmitRow then compared that against the row's own metadata, answered kShapeMismatch, and submit() threw -- so every production capture with a dynamic dim and a dtype wider than one byte failed hard. The element size comes from DtypeWidth(dtype_name), which submit() already computes and proves non-zero one line above the call, and which SubmitRow uses for the same purpose. The reference's two other guards come with it: a slice length that is not a whole number of elements, and an inferred dimension past int64. The non-dynamic branch needs no equivalent of the reference's 'fixed tensor shape does not match payload-slice bytes': SubmitRow already refuses that row as kSizeMismatch against elements * DtypeWidth. None of this was reachable from a test, because bindings_sink.cpp pinned inferred_dynamic_dim to -1 while production (bindings.cpp) sets it from the -1 in a PayloadSlice's shape. The driver now takes the field, and the new tests drive the float32 case the reference specifies with a uint8 control that passed before.

The pack-conflict branch composed its diagnostic into last_error and then returned straight out of the retry loop, skipping the `*error = last_error` at the end of UploadOne. UploadPending recorded {pack_id, key, attempts=0, error=""} -- an empty string for the ONE failure that means 'the object store already holds someone else's immutable object at this key, do not overwrite it'. An operator saw a pack that had stopped uploading with no stated reason. The exit now assigns both outputs, like the other two: attempts carries the HEAD that found the conflict (1), matching the retry-exhausted exit rather than the byte-gate refusal's 0, which is recorded before any attempt is made.

Submit screened max_pack_bytes and nothing else, so a record between max_queue_bytes and max_pack_bytes passed admission and reached a wait loop whose condition (queue_bytes_ + n <= max_queue_bytes) cannot hold even on an empty queue. Under kDropNewest it came back dropped, with an admission timeout timed out, and under kBlock with a negative timeout it blocked forever -- all three reachable from the conformance driver, and with the shipped defaults (16 MiB queue, 128 MiB pack) that is every record over 16 MiB. The oracle, _BoundedQueue.put (pipeline.py), answers TOO_LARGE for that record before entering its wait loop, and HostCapturePipeline.submit counts it oversized -- the same answer and the same counter as the max_pack_bytes screen it applies first. Both screens now sit before the loop, in that order. test_block_with_timeout_counts_timeouts asserted the defect: it drove a 64-byte payload under a 63-byte cap and expected timed_out, which the oracle calls too_large. Its refusal moved to the new parity test, and what remains under that name is the boundary the new screen must not overshoot (a record of exactly the cap is still admitted). The timeout path needs a queue held full by an undrained consumer, which the oracle pins by stalling its sink (BlockingPackSink) and this driver has no equivalent for.

ClickHouseClient called curl_global_init in its constructor and curl_global_cleanup in its DESTRUCTOR. Both are process-global, and libcurl documents them as unsafe to call while another thread is inside the library -- SpoolUploader::UploadPending runs up to max_workers concurrent curl_easy_perform calls, so destroying a client on the main thread during an upload batch tore the library down underneath them, and the workers' next curl_easy_init raced on re-initialisation. S3Client made this worse by calling no init at all: it leaned on the implicit one inside curl_easy_init, which carries the same caveat. One process-lifetime init, behind a std::once_flag, in the shared common/ helper both clients already link (json.cpp is there, and the store's link line stays free of the catalog). No teardown: there is no correct moment for a library-wide cleanup in a process that still has threads, and the OS reclaims the allocations at exit. Neither client's public API changes -- ~ClickHouseClient stays declared, and is now defaulted. No deterministic red test exists for the race itself; a test that reproduces it sometimes is a flake. Pinned instead: the source invariant (no per-object teardown anywhere, one init in one place), which fails on the pre-fix tree, and the lifetime behaviour through the catalog driver (three clients constructed and destroyed in one process, the third still reaching libcurl).

capture_sink_config is consumed in exactly one branch of create_record_runtime, guarded by `storage_backend == "capture"`, and storage_backend defaults to "auto". So a caller who set the sink config and left the backend alone passed the engine's type check at the boundary and then fell through with record_sink=None: no packs written at all, and nothing said so anywhere. Checked before changing it: nothing RESOLVES "auto" into "capture". "auto" is the pre-field behaviour -- _reject_a_sink_the_config_did_not _ask_for returns early on it and the default-writer branch compares the literal -- so the sink config is not merely unused under "auto", it is unreachable. And no test, example or doc in the tree passes capture_sink_config with a non-capture backend: the two tests that use it either set storage_backend="capture" or assign engine. _capture_sink_config directly, past MonitoringConfig entirely. So the naive check is the correct one, in __post_init__ where the field's own design note ("a mismatch becomes an error instead of a silent choice") already lives. This is a deliberate behaviour change: the combination now fails at config construction instead of silently capturing nothing. Narrow -- only a sink config that was actually passed is refused, and only under a backend that cannot read it.

The live job ran `pytest tests/*_live.py -m "clickhouse and manual and not garage"`, so the naming convention was load-bearing: a new `manual`/`clickhouse` test written under any other filename is silently NOT COLLECTED, and the "Fail if any live test was skipped" gate below cannot catch it -- uncollected is not skipped, so the job stays green over coverage that never ran. That is the same silent-green the gate exists to end. Measured before widening, as the risk is pulling in files that are expensive or need another service: the marker expression alone over `tests/` selects 199 tests in 9 files, identical to what the glob selected, and every one of those files is already `*_live.py`. Collection of the whole tree takes ~2s with no errors, and the cpu job already collects it the same way (`pytest -m cpu`, no path) on the same image and dependency set. Verified with a temporary `manual`/`clickhouse` test named outside the convention (`tests/test_..._e2e.py`): the glob collected 199 and missed it; marker selection collected 200 and named it. Temporary file removed.

Copilot

🟡 Changes recommended

Five unresolved findings remain, including one critical linkage issue and four moderate correctness or coverage issues.

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

Pull request overview

This PR fixes native capture inference and queue admission, upload diagnostics, libcurl lifetime management, configuration validation, and live-test collection.

Changes:

Corrects dynamic-dimension inference and oversized-record handling.
Preserves upload conflict diagnostics and centralizes libcurl initialization.
Rejects ignored capture configuration and selects live tests by marker.

File summaries

File	Summary	Review comment
`tests/test_native_uploader.py`	Tests upload conflict reporting.	—
`tests/test_native_pack_sink.py`	Tests queue-admission boundaries.	Moderate (1 vote): Replacement drops native `BLOCK` wait/deadline coverage; add deterministic native coverage or document the gap.
`tests/test_native_curl_global_lifetime.py`	Tests libcurl lifetime invariants.	Moderate (1 vote): Source scanning is not token-aware; use a stateful C++ comment/string scanner.
`tests/test_native_adapter_torch.py`	Tests dynamic-shape inference.	—
`tests/test_engine_runtime_api.py`	Tests configuration validation.	—
`src/dmi/config.py`	Rejects ignored capture configuration.	—
`native/Makefile`	Updates native build recipes.	—
`native/csrc/store/uploader.cpp`	Reports conflict errors and attempts.	—
`native/csrc/store/s3_client.cpp`	Initializes libcurl globally.	—
`native/csrc/sink/pack_sink.cpp`	Screens queue-size violations.	—
`native/csrc/sink/native_pack_sink.cpp`	Resolves dynamic dimensions in elements.	Moderate (2 votes): Check fixed-dimension multiplication for overflow and negative values before inference.
`native/csrc/sink/bindings_sink.cpp`	Exposes inferred-dimension metadata.	—
`native/csrc/common/curl_init.h`	Declares shared curl initialization.	—
`native/csrc/common/curl_init.cpp`	Implements once-only initialization.	Moderate (1 vote): Check and propagate the `curl_global_init` result.
`native/csrc/catalog/clickhouse_client.cpp`	Uses shared curl lifetime management.	Critical (1 vote): Add `curl_init.cpp` and its header dependency to both normal extension source lists.
`.github/workflows/python-checks.yml`	Selects live tests by marker.	—

Review details

Suppressed comments (3)

native/csrc/common/curl_init.cpp:11

curl_global_init returns a CURLcode, but this result is discarded. If process-global initialization fails, std::call_once still marks the helper complete and every later client proceeds as though libcurl were initialized, turning the original failure into misleading downstream handle/transport errors. Check the return code and propagate the failure instead of recording a successful initialization.

  std::call_once(once, [] { curl_global_init(CURL_GLOBAL_DEFAULT); });

tests/test_native_curl_global_lifetime.py:55

This source scan cannot reliably enforce the invariant it claims to pin: the regex treats // or /* inside C++ string literals as comments. The native tree already contains http:// in a string (native/csrc/store/s3_client.cpp:453), so a forbidden call placed later on that line could be stripped and the test would pass. Use a stateful C++ comment/string scanner or another token-aware check so a future per-object teardown cannot evade this guard.

_COMMENT = re.compile(r"//[^\n]*|/\*.*?\*/", re.DOTALL)

tests/test_native_pack_sink.py:257

Replacing this test removes native-driver coverage for the BLOCK wait/deadline path: the new assertions only exercise immediate admission, while BlockingPackSink in the Python oracle drives a different implementation. A regression in PackSink::Submit that returns the wrong timeout admission or timed_out_records count can now pass the native CPU suite. Please add a deterministic native-level stall/unit seam, or explicitly retain this as a documented native coverage gap rather than presenting the boundary test as timeout coverage.

    assert snapshot["timed_out_records"] == 0

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

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

+  // to run in the destructor below, which tore libcurl down for the WHOLE
+  // process while the uploader's worker threads were inside
+  // curl_easy_perform. See common/curl_init.h.
+  dmi_common::EnsureCurlGlobalInit();


  uint64_t fixed = 1;
  for (size_t i = 0; i < shape_out->size(); ++i) {
    if (static_cast<int>(i) == dim) continue;
    fixed *= static_cast<uint64_t>((*shape_out)[i]);
  }


…t build 41313f3 widened the live job's selection from `tests/*_live.py` to the whole `tests` tree so a `manual`/`clickhouse` test named outside the convention could not hide. The goal was right, the mechanism was not: walking the tree makes pytest IMPORT every module, and this job cannot import all of them. It builds only the four conformance drivers. It does NOT build build/_dmi_native_sink, which the cpu job gets from `make -C native cpu-goals` (CPU_ONLY_GOALS includes it). tests/test_native_adapter_torch.py and tests/test_native_rollback.py import that .so at module scope and so take a module-level `pytest.skip` when it is absent -- they are the two permanent exceptions tests/test_ci_guard_skips.py already records as unable to convert to a per-test skipif. A collection skip is written to the JUnit as `<skipped/>`, and the "Fail if any live test was skipped" gate then failed the job over two files this suite never wanted: 201 collected, 2 skipped, across 1 suite(s) skipped: tests.test_native_adapter_torch: collection skipped skipped: tests.test_native_rollback: collection skipped The gate behaved exactly as designed and is untouched here -- byte for byte what it was before 41313f3. What changes is that the RUN goes back to importing only the files it needs. Note the local measurement that cleared 41313f3 was not wrong about torch: BOTH jobs install torch. The difference is the native build step, which is why collecting on a dev box with the .so present showed 199/199 and no skips. Reproduced here by hiding _dmi_native_sink*.so with torch fully installed: the tree walk emits both skips at tests/..._torch.py:34 and tests/..._rollback.py:33, the glob emits none, and the glob's JUnit contains no "collection skipped" at all. The hole 41313f3 set out to close is real and stays closed -- by a separate explicit check, in the next commit, rather than by widening this run.

The previous commit put the live run back on `tests/*_live.py`, which leaves the naming convention load-bearing: a `manual`/`clickhouse` test written as `test_..._e2e.py` is never collected by that job, and its skip gate cannot see the gap, because uncollected is not skipped. That is the real hole 41313f3 found. This closes it without making the live job import anything new. The check collects the marker expression over the whole `tests` tree and compares the collected node ids' FILES against what `tests/*_live.py` matches. Anything in the first set and not the second fails the job and names the file. It runs in the CPU job, not the live job, and the placement is the whole point. The tree walk needs an environment that can import the tree, and only this job has one: `make -C native cpu-goals` builds CPU_ONLY_GOALS, including build/_dmi_native_sink, so the two suites that import that .so at module scope collect normally here. The live job builds four conformance drivers and cannot. This job also already collects the same tree for `make check` (`pytest -m cpu`, no path), so the walk adds no import that is not already happening. Comparing FILES, not node ids, is what makes an unimportable module a no-op: it contributes no node ids, so it simply is not in the set. Nothing special-cases "collection skipped", and nothing here can be widened into ignoring a skip. An unimportable module in this job is still a failure -- the cpu skip gate above catches it -- so the two checks compose rather than overlap. Collection errors are handled deliberately rather than with `|| true`, which would silently disable the gate. `--collect-only` exits 2 on a collection error and 5 on an empty selection; both are failures that name themselves. The gate also refuses to pass on an empty collected set, so a marker expression that stops matching cannot make every comparison vacuously true. Verified both directions on the current tree: - with a temporary `manual`/`clickhouse` test added as tests/test_capture_catalog_probe_e2e.py, the check exits 1 with "outside the glob: tests/test_capture_catalog_probe_e2e.py", while the live job's glob collects 199 and never sees the file. Temporary file removed. - with the tree as it stands, the check exits 0: "9 file(s) hold 'clickhouse and manual and not garage' tests; 9 matched by tests/*_live.py". - with _dmi_native_sink*.so hidden to mimic the live job, the check still exits 0 and still reports 9/9 -- the two collection-skipped modules are invisible to it, as designed. - with a deliberately unimportable module in tests/, the check exits 1 with "collecting ... exited 2; this gate cannot run, which is indistinguishable from it passing". The live job's skip gate is unchanged and byte-identical to pre-41313f3.

zaoxing added 6 commits September 11, 2026 12:24

Copilot AI lite review requested due to automatic review settings September 11, 2026 17:09

Copilot started reviewing on behalf of zaoxing September 11, 2026 17:10 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

zaoxing added 2 commits September 11, 2026 13:23

Navigation Menu

Uh oh!

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

Dynamic-dim shape inference, upload conflict reporting, and libcurl lifetime#135
zaoxing wants to merge 8 commits into
mainfrom
fix/native-shape-and-lifecycle

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

ResolveShape inferred a dynamic dim in bytes, not elements

The one non-retryable upload failure reported an empty string

An oversized record could block forever

libcurl was torn down per object

A configured capture sink was silently ignored

The live CI job could silently collect 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

`ResolveShape` inferred a dynamic dim in bytes, not elements