enhance: add error-handling ratchet gate (abort/throw baseline burn-down) - #596
Conversation
…own) Library code must report failures as arrow::Status/arrow::Result (tagged with ExtendStatusDetail where classification matters), not by aborting the process (ValueOrDie on an error path) or throwing exceptions across the library boundary. Past sweeps fixed sites individually (milvus-io#575 removed four unguarded ValueOrDie aborts) but nothing prevented regressions, and new instances kept appearing in later work. This adds a ratchet: cpp/scripts/error_handling_ratchet.sh counts abort (ValueOrDie/ValueUnsafe/MoveValueUnsafe) and throw sites per git-tracked production file (cpp/src + cpp/include, no test code) and diffs the result against a checked-in baseline. CI fails on ANY divergence: - count went up: the new code must return Status instead; the baseline is not to be raised. - count went down: the burn-down is recorded by regenerating the baseline (update-error-ratchet target) in the same PR, so the slack cannot grow back later. Baseline at introduction: abort=143, throw=59. Wired as a standalone lightweight workflow (no toolchain deps) plus check-error-ratchet / update-error-ratchet targets. Verified locally: check passes on the pristine tree; injecting a throw into cpp/src/properties.cpp fails the check with a per-file diff. issue: milvus-io/milvus#50903 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: czs007 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
==========================================
- Coverage 75.95% 75.95% -0.01%
==========================================
Files 168 168
Lines 16672 16672
Branches 2510 2510
==========================================
- Hits 12664 12663 -1
- Misses 4008 4009 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Adversarial review of this CI ratchet (script + workflow + baseline TSV) surfaced one high-severity gap in how the gate enforces its own promise, plus several medium/low issues around counting semantics and CI wiring — none block merge, but the PR description should be adjusted to match what the gate actually enforces. High
Medium
Low
|
…e branch Adopts the adversarial-review findings on milvus-io#596: - High (self-referential gate): 'check' now takes an optional base-branch baseline; in CI the pull_request job fetches the base ref's baseline and fails if any per-category TOTAL increased versus it. Regenerating a raised baseline inside the same PR keeps the exact-match layer green but fails this layer, so the no-new-sites direction is enforced by the gate itself, not by reviewers noticing a .tsv diff. Totals rather than per-file so moving grandfathered code between files stays neutral. - Medium (paths-filter + required-check trap): dropped the paths filter; the script runs in seconds and now reports on every PR, with a comment explaining why. - Medium x2 (comment counting can mask an equal-count swap; grep -c counts lines not call sites): disclosed both in the script header as known counting limits of the text-level gate. - Low: git -c core.quotePath=false ls-files -z + read -d '' for special filenames; permissions: contents: read on the workflow. Verified locally: plain check green; check against the current baseline as base green; the attack path (add a throw, regenerate the baseline, check against the pre-attack base) fails with the totals message; a burn-down (one throw removed, baseline regenerated) passes against the old base. Tree restored; committed baseline unchanged (abort=143, throw=59). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
|
Thanks for the adversarial pass — the High finding is real and is now fixed in 0832462; all six findings addressed:
Baseline content unchanged (abort=143, throw=59). |
|
Adversarial review verified 7 non-blocking issues (3 medium, 4 low) in the error-handling ratchet CI gate — all relate to CI reliability, known scope trade-offs, and edge-case correctness rather than defects in the core mechanism, which was independently confirmed to work. Medium
Low
|
- Anchor layer 2 to the PR's recorded base commit (github.event.pull_request.base.sha) instead of the branch name, so both sides of the comparison agree even when the base tip just moved (timing-skew false reds/misses). - Distinguish bootstrap from real failures: the baseline's absence from the base commit is verified with git cat-file before skipping layer 2 (a renamed path or transient git failure no longer silently disables the self-reference protection); a fetch failure degrades to layer 1 with a visible ::warning instead of blocking unrelated PRs. - Document the two standing assumptions in the workflow: layer 2 trusts the committed base baseline (kept in sync by the push trigger), and the checker runs from the PR checkout (in-repo lint-gate trust model: defense target is accidental regression, not a malicious PR). Script and baseline unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
|
Round-2 findings addressed — workflow hardening in 838ab6a, wording fixes in the PR description (just edited):
Script and baseline unchanged; the core mechanism was independently confirmed working in this round. |
| throw cpp/include/milvus-storage/common/fiu_local.h 2 | ||
| throw cpp/src/common/metadata.cpp 4 | ||
| throw cpp/src/ffi/ffi_fiu_c.cpp 1 | ||
| throw cpp/src/ffi/reader_c.cpp 1 |
There was a problem hiding this comment.
Why are ValueOrDie / ValueUnsafe / MoveValueUnsafe considered technical debt? In extern "C" functions, we need logic like this to convert Arrow errors into FFI errors:
auto fs_result = FilesystemCache::getInstance().get();
RETURN_ARROW_ERROR_IF(fs_result.status(), LOON_ARROW_ERROR,
"Failed to obtain filesystem: ",
fs_result.status().ToString());
auto fs = fs_result.ValueOrDie();
I think this is reasonable because the Result has already been checked before accessing its value.
There was a problem hiding this comment.
You're right that a guarded ValueOrDie after RETURN_ARROW_ERROR_IF is not a bug — the FFI files follow that pattern consistently (the July audit reached the same conclusion: all FFI-layer sites are guarded and safe). The baseline entry doesn't claim otherwise; being in the baseline means grandfathered, never blocks anything as-is.
Why they're still counted, three practical reasons:
- A text-level gate can't tell guarded from unguarded, and the unguarded flavor is exactly the P0 abort class: enhance: classify packed extend status codes into segcore error codes #575 removed four, and enhance: add PackedRecordBatchReader::Make factory; fix abort and code-destroying paths in packed #598 just found a fifth (
file_reader.cpp, data-dependent process abort) that had been sitting next to the guarded idiom for a year. Counting all of them means any newValueOrDiegets one reviewer glance at one diff line — that's the entire cost, since existing sites never trip the gate. - The burn-down direction isn't "guarded sites are wrong", it's "prefer constructs that are safe by construction":
ARROW_ASSIGN_OR_RAISEorstd::move(*result)after the same.ok()check express the identical logic with no abort path to hand-verify. Where the current macro pattern is the clearest option (these FFI functions), staying at the baseline forever is a perfectly fine steady state. - Keeping one category rather than a guarded/unguarded split keeps the gate a 30-line grep instead of a clang-query dependency.
That said, this is maintainer's call: if you'd prefer the FFI layer excluded from the abort category (e.g. drop cpp/src/ffi/ from the scope, where the guarded macro pattern is the house style), I'll make that change — the gate keeps its value for the library core either way.
There was a problem hiding this comment.
As u said A text-level gate can't tell guarded from unguarded. So, I don't think the current PR can solve any problems. :)
There was a problem hiding this comment.
Fair — for the abort category you've convinced me: since the gate can't distinguish your (legitimate, house-style) guarded idiom from an unguarded abort, all it does there is nag every FFI addition without ever identifying a dangerous one. Dropped in d3e77b2; the baseline is now throw-only. If we ever want abort coverage back it should be a clang-query check that actually understands the guard, not a grep.
The throw category doesn't have this problem, which is why the PR still stands: Ring-1 forbids the library from leaking exceptions at all, so in library code any throw is a violation — no guarded/unguarded distinction exists to blur, and textual counting is the semantic judgment. Concretely: #597 just burned down 23 throw sites (lance/iceberg bridges); this gate is the only thing that keeps them from creeping back one convenience-throw at a time, which is exactly how the previous 59 accumulated.
So the narrowed claim for this PR: it doesn't catch unguarded aborts (yours to review, as today), it does mechanically prevent exception-leak regressions. If that narrowed scope is acceptable, it's ready for another look.
The abort category (ValueOrDie/ValueUnsafe) is dropped: a text-level gate cannot tell the guarded FFI idiom (.ok() check + macro return + ValueOrDie, the house style in cpp/src/ffi/) from an unguarded abort path, so counting them nagged every legitimate addition without distinguishing dangerous ones. Unguarded aborts remain a review concern; a clang-query-based check could reintroduce the category with real semantic discrimination. The throw category stays: Ring-1 forbids the library from leaking exceptions, so in library code ANY throw is a violation -- textual counting IS the semantic judgment there, and it is what prevents the 23 sites burned down in milvus-io#597 from creeping back. Baseline regenerated: throw=59 only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
| throw cpp/src/ffi/reader_c.cpp 1 | ||
| throw cpp/src/ffi/v2_column_groups_builder.cpp 5 | ||
| throw cpp/src/filesystem/azure/azurefs.cc 4 | ||
| throw cpp/src/filesystem/gcp/gcp_filesystem_producer.cpp 1 |
There was a problem hiding this comment.
The throw in cpp/src/filesystem/gcp/gcp_filesystem_producer.cpp is not the real throw execption.
// Register cleanup on exit. atexit handlers must not throw, so log on failure.
| throw cpp/src/ffi/ffi_fiu_c.cpp 1 | ||
| throw cpp/src/ffi/reader_c.cpp 1 | ||
| throw cpp/src/ffi/v2_column_groups_builder.cpp 5 | ||
| throw cpp/src/filesystem/azure/azurefs.cc 4 |
There was a problem hiding this comment.
Same as the gcp_filesystem_producer.cpp, not the real throw exception.
Can u import a tools which like clang-tidy which can detech the real throw case i guess...
There was a problem hiding this comment.
Both confirmed — comment-only mentions, and there were more (10 of the 59). Fixed in the commit just pushed: the collector now strips comments via gcc -fpreprocessed -dD -E -P (removes comments without expanding includes/macros, so the job stays toolchain-free — the runner's stock gcc suffices). Baseline drops 59 → 49, eliminating the whole comment false-positive class: azurefs.cc ×4, fiu_local.h ×2, the two *_filesystem_producer.cpp you flagged, reader_c.cpp, jni_utils.cpp — all gone; the remaining 49 are real throw expressions.
On clang-tidy/clang-query: the exact check is match cxxThrowExpr(), and I'd like to get there — but it needs a compile_commands.json, i.e. a fully configured conan toolchain in the CI job, which turns a 5-second gate into a heavy build job. The pragmatic split I'd propose: this comment-stripped textual gate as the fast always-on ratchet, and a clang-query pass wired into the existing check-tidy infrastructure (which already has the compile database locally) as a local/nightly deep check — happy to add that as a follow-up if you want it. The baseline flow is reusable either way.
Both flagged baseline entries (gcp_filesystem_producer.cpp, s3_filesystem_producer.cpp) were the word 'throw' inside comments. The collector now pipes each file through gcc -fpreprocessed -dD -E -P, which removes comments without expanding includes or macros, keeping the gate toolchain-free while eliminating the comment false-positive class entirely: the baseline drops 59 -> 49, removing 10 comment-only entries (azurefs.cc x4, fiu_local.h x2, gcp/s3 producers, reader_c, jni_utils). A clang-query cxxThrowExpr() check would be exact but requires a compile_commands.json and thus a configured-toolchain CI job; noted in the header as the upgrade path, reusing the same baseline flow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
|
/lgtm |
…us-io#606) ## Problem `main` currently fails its own error-handling ratchet, and every open PR inherits the failure once CI merges it with main. milvus-io#598 (`352d545`) removed the last `throw` from `cpp/src/packed/column_group.cpp` — `ColumnGroup::Table()` now returns `arrow::Result` — but did not regenerate `cpp/scripts/error_handling_baseline.tsv`. The committed baseline still claims one throw there, and the ratchet's first layer is an exact match: ``` -throw cpp/src/packed/column_group.cpp 1 ``` ## Why it got through milvus-io#598 branched from before milvus-io#596 added the gate, so there was no baseline at its base commit and the workflow took its documented bootstrap path: ``` ::notice::error-handling-ratchet: no baseline at base commit (bootstrap); base-totals layer skipped ``` The exact-match layer had nothing to compare against on that PR, so the divergence only became visible after the merge. milvus-io#597 is in the same position (branched pre-milvus-io#596) — it regenerated its baseline on rebase, so it is fine, but the bootstrap hole is worth knowing about: a PR that predates the gate can merge a stale baseline. ## Change Deletes the one stale line. Content taken from the diff CI printed on milvus-io#604, not from a local `update` run. **Warning for whoever touches this next on macOS:** do not run `cpp/scripts/error_handling_ratchet.sh update` there. The scan uses `gcc -fpreprocessed -dD -E -P`, which Apple clang rejects with `unknown argument`, so every file silently counts zero — `check` reports the entire baseline as removed, and `update` would erase the file. Worth a guard in the script (probe the preprocessor once and fail loudly, and refuse to write an empty baseline); I can follow up with that if wanted. Unblocks: milvus-io#597, milvus-io#603, milvus-io#604, and anything else merged with main after `352d545`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 participants
What
Adds a CI ratchet for error-handling discipline: a checked-in per-file baseline of
abortsites (ValueOrDie/ValueUnsafe/MoveValueUnsafe) andthrowsites in production code (cpp/src+cpp/include, tests excluded), enforced by a lightweight two-layer check:make -C cpp update-error-ratchet) in the same PR, so freed slack cannot silently grow back.Baseline at introduction: abort = 143, throw = 59 (git-tracked files only, so vendored/generated trees are excluded automatically).
Why
Ring-1 discipline for this library is that failures are reported as
arrow::Statuswith structured classification, not by aborting the process or leaking exceptions to consumers (milvus links this C++ directly). Individual sweeps keep fixing sites — #575 removed four unguardedValueOrDieabort paths — but nothing prevents regressions, and the pattern keeps reappearing:ValueOrDie(format/parquet/file_reader.cpp, reachable when the file schema lacks field-id metadata) predates enhance: classify packed extend status codes into segcore error codes #575's sweep and was never on any list (fixed in enhance: add PackedRecordBatchReader::Make factory; fix abort and code-destroying paths in packed #598);packed/reader.cppstill throws from a constructor; the lance/iceberg bridges threw across the library boundary until enhance: classify lance/iceberg bridge errors and stop leaking exceptions #597 — the ratchet makes that burn-down visible and permanent in the baseline diff.How to work with it
The failure output is a unified diff against the baseline plus instructions for both directions; the base-totals failure names the category and both totals.
Verification
checkpasses on the pristine tree; category totals reported (abort=143, throw=59).throw std::runtime_errorfails layer 1 with a per-file diff (verified locally, then reverted).throw: 59 (base) -> 60 (this PR); a genuine burn-down against the same base passes (both verified locally).github.event.pull_request.base.shaand distinguishes bootstrap (no baseline on base, verified viagit cat-file) from real failures.Not covered (honest scope) — this is a text-level ratchet, not a semantic linter:
throw, add a real one in the same file — or, for layer 2, in another file) is not caught;grep -ccounts matching lines, not call sites: two sites on one line count once, and a site added to an already-matching line does not trip the gate;issue: milvus-io/milvus#50903
🤖 Generated with Claude Code