test: extend td_init coverage + guard td_free(NULL) (follow-up to #41) by fcostaoliveira · Pull Request #44 · RedisBloom/t-digest-c

fcostaoliveira · 2026-07-27T16:25:58Z

Builds on #41 (targets its branch so the diff here is only the delta; retarget to master once #41 merges).

Two follow-ups the review of #41 called out, kept out of #41 to keep it small:

Guard td_free(NULL). Guard TDigest capacity calculation before integer narrowing #41 makes td_new() return NULL for invalid compression (non-finite / <= 0 / capacity > INT_MAX), so the idiomatic td_free(td_new(bad)) cleanup would dereference NULL. Adds the standard if (!histogram) return;.
Extend the unit tests (the ones added in Guard TDigest capacity calculation before integer narrowing #41 only asserted the reject side):
- td_free(NULL) / td_free(td_new(bad)) don't crash
- td_new() rejects the same inputs as td_init() (NaN/INF/-1/0/overflow → NULL)
- td_init() leaves *result untouched on failure (sentinel, stronger than "== NULL")
- cap == 6*compression + 10 and determinism at safe sizes; pins current sub-1/fractional behavior
- a large valid digest (compression 100000) actually works end to end

Verified locally

25 tests / 5.01M assertions / 0 failures; clean under ASan + UBSan + LSan; clang-format OK.
(With #43's CI in place, these run automatically under sanitizers.)

🤖 Generated with Claude Code

Note

Low Risk
Defensive cleanup and test/refactor around existing validation; no changes to digest merge/quantile logic.

Overview
Follow-up to compression validation in #41: td_free(NULL) is now a no-op so td_free(td_new(bad)) is safe when td_new returns NULL for invalid compression.

Compression sizing is refactored into allocation-free capacity_from_compression() (same rules as before); td_init delegates to it. Public docs in tdigest.h spell out NULL returns, failure leaving *result untouched, and safe td_free(td_new(...)).

Tests: new td_capacity_test hits the largest accepted compression boundary without ~34 GiB allocations; td_test adds NULL-free paths, td_new parity with td_init, sentinel *result on failure, cap formula/determinism, fractional compression behavior, and a large-but-usable digest (compression 100000). The old td_init(100000000) case is dropped in favor of the capacity test.

^{Reviewed by Cursor Bugbot for commit caadf02. Bugbot is set up for automated code reviews on this repo. Configure here.}

gabsow

One test assertion issue to address.

gabsow · 2026-07-27T18:19:33Z

+    mu_assert_double_eq(10000.0, td_max(t));
+    mu_assert_long_eq(10000, td_size(t));
+    mu_assert(td_centroid_count(t) <= t->cap, "centroid count must stay within cap");
+    mu_assert_double_eq_epsilon(5000.5, td_quantile(t, 0.5), 50.0);


mu_assert_double_eq_epsilon does not fail for NAN: fabs(expected - NAN) is NAN, and NAN > epsilon is false. As written, this end-to-end test can pass even if td_quantile() returns NAN. Please store the result and assert isfinite(median) before the epsilon comparison.

Fixed in f2beaa2. The end-to-end test now stores the result and asserts isfinite(median) before the epsilon compare, with a comment explaining that mu_assert_double_eq_epsilon does not fail for NaN.

gabsow

Blocking merge-target issue: #41 has already been squash-merged into master, but this PR still targets codex/td-init-capacity-guard. The current head has diverged from master (3 commits ahead, 1 behind). Please rebase/cherry-pick only 9428653 onto current master, update the PR branch, and retarget the PR to master; retargeting alone would expose the duplicate #41 history/diff.

gabsow · 2026-07-27T18:23:36Z

+// just allocate but actually work end to end.
+MU_TEST(test_td_init_large_success_is_usable) {
+    td_histogram_t *t = NULL;
+    mu_assert_long_eq(0, td_init(100000, &t));


This adds a useful ~9.6 MB end-to-end case, but it does not complete the follow-up called out on #41: the exact accepted side, (INT_MAX - 10) / 6, remains untested, while the existing td_init(100000000) expectation (~9.6 GB) is still overcommit-dependent. Please replace that fragile allocation test and cover the accepted boundary through a factored/testable capacity helper or allocator hook that avoids allocating tens of GiB.

Fixed in f2beaa2. Factored the validation + capacity math out of td_init() into an allocation-free helper capacity_from_compression(), and added td_capacity_test (includes the TU) that drives it directly at the exact boundary: floor((INT_MAX-10)/6)=357913939 is accepted (cap=2147483644 <= INT_MAX) and +1 is rejected with *capacity left untouched — no allocation. Dropped the overcommit-dependent td_init(100000000) (~9.6 GB) case.

gabsow · 2026-07-27T18:23:36Z

+    // This guard is required by the capacity validation added in this PR: td_new()
+    // now returns NULL for invalid compression (non-finite / <= 0 / cap > INT_MAX),
+    // so the idiomatic td_free(td_new(bad)) cleanup would otherwise dereference NULL.
+    if (!histogram) {


Since this makes NULL tolerance part of the public API, please update tdigest.h alongside it: td_new() can return NULL for invalid compression or allocation failure; td_init() can return 1 for either case and leaves *result untouched on rejection; and td_free(NULL) is supported. Also, the nearby comment should not say the validation was added “in this PR”—that landed in #41.

Fixed in f2beaa2. tdigest.h now documents that td_new() returns NULL for invalid compression or allocation failure, td_init() returns 1 and leaves *result untouched on rejection, and td_free(NULL) is a no-op. Also corrected the td_free comment — it no longer claims the validation was added in this PR (it landed in #41).

… docs Follow-up to gabsow's review on RedisBloom#44: - Factor the compression validation + capacity computation out of td_init() into capacity_from_compression() (no allocation). Add td_capacity_test that drives it directly to cover the exact accepted/rejected boundary floor((INT_MAX-10)/6) without allocating the ~17 GB that compression needs, and drop the overcommit-dependent td_init(100000000) (~9.6 GB) case. - test_td_init_large_success_is_usable: store td_quantile() and assert isfinite() before the epsilon compare; mu_assert_double_eq_epsilon does not fail for NaN, so the check alone could pass on a NaN median. - Document the NULL/rejection contract in tdigest.h (td_new returns NULL, td_init returns 1 and leaves *result untouched, td_free(NULL) is a no-op), and correct the td_free comment: td_new()'s validation landed in RedisBloom#41. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

fcostaoliveira · 2026-07-27T20:09:52Z

Pushed f2beaa2 addressing all three review comments:

NaN-safe assert — the large end-to-end test now stores td_quantile() and asserts isfinite(median) before the epsilon compare (mu_assert_double_eq_epsilon doesn't fail for NaN).
Accepted-boundary coverage without huge allocations — extracted the validation + capacity math from td_init() into an allocation-free helper capacity_from_compression(), and added td_capacity_test that drives it directly: floor((INT_MAX-10)/6)=357913939 accepted (cap 2147483644 <= INT_MAX), +1 rejected with *capacity untouched. Dropped the overcommit-dependent td_init(100000000) (~9.6 GB) case.
Public-API docs — tdigest.h now documents the NULL/return 1/untouched-*result/td_free(NULL) contract; corrected the td_free comment (validation landed in Guard TDigest capacity calculation before integer narrowing #41, not here).

Verified locally under ASan+UBSan: td_test 25 tests / 5,012,049 assertions / 0 failures, td_capacity_test OK, clang-format clean.

gabsow

Two follow-up items remain outside the inline findings:

The merge target is still codex/td-init-capacity-guard. This head is now diverged from current master (4 commits ahead / 1 behind). Please replay both PR commits (9428653 and f2beaa2), or squash them, onto master and retarget the PR.
#43 is still open, and its sanitizer job manually compiles/runs only td_test; it will not execute the new td_capacity_test. Please include that target in the sanitizer workflow (or run a sanitizer-enabled CMake/CTest build) before claiming these tests run automatically under sanitizers.

gabsow · 2026-07-27T21:50:28Z

+
+int main(void) {
+    /* The largest integer compression whose capacity still fits in an int. */
+    const long long max_ok = (long long)((INT_MAX - 10) / 6);


This assumes INT_MAX is always the binding capacity limit, but production also checks SIZE_MAX / sizeof(double) and SIZE_MAX / sizeof(long long). On a 32-bit size_t target, this produces cap_ok = 2147483644 while the per-array element limit is only 536870911, so capacity_from_compression() correctly rejects max_ok and this new test fails. Please derive max_capacity = min(INT_MAX, SIZE_MAX / sizeof(double), SIZE_MAX / sizeof(long long)), then compute max_ok = (max_capacity - 10) / 6 and test that boundary plus one. Also, the nearby memory estimate is low by 2×: two 8-byte arrays at cap ≈ INT_MAX total about 34.4 GB (~32 GiB), not ~17 GB.

Fixed in 2fb6b5a. The test now derives max_capacity = min(INT_MAX, SIZE_MAX/sizeof(double), SIZE_MAX/sizeof(long long)) and takes max_ok = (max_capacity - 10) / 6 from that, so it stays correct on a 32-bit size_t target where the SIZE_MAX/8 element limit binds instead of INT_MAX. On 64-bit size_t, INT_MAX still binds so the tested boundary is unchanged (357913939 -> cap 2147483644). Also corrected the memory estimate to ~34 GB (~32 GiB) here and in the related comments.

gabsow · 2026-07-27T21:50:28Z

+    target_include_directories(td_capacity_test PRIVATE ${CMAKE_SOURCE_DIR}/src)
+    target_link_libraries(td_capacity_test m)
+    add_test(td_capacity_test td_capacity_test)
 endif()


CMAKE_SOURCE_DIR refers to the outermost project, so this include path is wrong when t-digest-c is consumed via add_subdirectory() (and could even select an unrelated parent src/tdigest.c). Please use ${PROJECT_SOURCE_DIR}/src or, more robustly here, ${CMAKE_CURRENT_LIST_DIR}/../src.

Fixed in 2fb6b5a — switched the include path to ${CMAKE_CURRENT_LIST_DIR}/../src so it resolves correctly when t-digest-c is consumed via add_subdirectory().

…estimate Follow-up to gabsow's second review on RedisBloom#44: - td_capacity_test assumed INT_MAX is always the binding capacity limit, but the helper also rejects on SIZE_MAX/sizeof(double) and SIZE_MAX/sizeof(long long). On a 32-bit size_t target the per-array element limit (SIZE_MAX/8) is smaller than INT_MAX, so max_ok would be rejected and the test would fail. Derive max_capacity = min(INT_MAX, SIZE_MAX/sizeof(double), SIZE_MAX/sizeof(long long)) and take the boundary from that. On 64-bit size_t INT_MAX still binds, so the tested boundary (357913939 -> cap 2147483644) is unchanged. - Fix the tests/CMakeLists.txt include path: CMAKE_SOURCE_DIR points at the outermost project and breaks when t-digest-c is used via add_subdirectory(); use CMAKE_CURRENT_LIST_DIR/../src instead. - Correct the memory estimate: two 8-byte node arrays at cap ~ INT_MAX total ~34 GB (~32 GiB), not ~17 GB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

gabsow · 2026-07-27T22:04:16Z

Thanks — I rechecked 2fb6b5a, and the two latest inline findings are fixed correctly. Two blockers remain:

This PR still targets codex/td-init-capacity-guard, although Guard TDigest capacity calculation before integer narrowing #41 has already been squash-merged. Please replay or squash the three test: extend td_init coverage + guard td_free(NULL) (follow-up to #41) #44 commits onto current master and retarget the PR; simply changing the base would expose duplicate Guard TDigest capacity calculation before integer narrowing #41 history in the diff.
ci: add GitHub Actions build/test + ASan/UBSan (fix cmake test build) #43 is still open, and its sanitizer job compiles and runs only td_test, so the new td_capacity_test is not covered by ASan/UBSan. Please add that target to the sanitizer job, or use a sanitizer-enabled CMake/CTest build, before claiming automatic sanitizer coverage.

gabsow

Thanks — I rechecked 2fb6b5a, and the two latest inline findings are fixed correctly. Two blockers remain:

This PR still targets codex/td-init-capacity-guard, although #41 has already been squash-merged. Please replay or squash the three #44 commits onto current master and retarget the PR; simply changing the base would expose duplicate #41 history in the diff.
#43 is still open, and its sanitizer job compiles and runs only td_test, so the new td_capacity_test is not covered by ASan/UBSan. Please add that target to the sanitizer job, or use a sanitizer-enabled CMake/CTest build, before claiming automatic sanitizer coverage.

This PR makes td_new() return NULL for invalid compression (non-finite, <= 0, or capacity > INT_MAX), so the idiomatic td_free(td_new(bad)) cleanup would dereference NULL. Add the standard `if (!histogram) return;` guard to td_free. Extend the unit tests to cover the accept side and the new NULL paths (the added tests previously only asserted rejection): - td_free(NULL) and td_free(td_new(bad)) do not crash - td_new() rejects the same inputs as td_init() (NaN/INF/-1/0/overflow -> NULL) - td_init() leaves *result untouched on failure (sentinel, not just == NULL) - cap == 6*compression + 10 and determinism at safe sizes; pins the current sub-1/fractional-compression behavior (accepted, floored to cap 10) - a large valid digest (compression 100000) actually works end to end Verified: 25 tests / 5.01M assertions pass; clean under ASan+UBSan+LSan; clang-format OK. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… docs Follow-up to gabsow's review on RedisBloom#44: - Factor the compression validation + capacity computation out of td_init() into capacity_from_compression() (no allocation). Add td_capacity_test that drives it directly to cover the exact accepted/rejected boundary floor((INT_MAX-10)/6) without allocating the ~17 GB that compression needs, and drop the overcommit-dependent td_init(100000000) (~9.6 GB) case. - test_td_init_large_success_is_usable: store td_quantile() and assert isfinite() before the epsilon compare; mu_assert_double_eq_epsilon does not fail for NaN, so the check alone could pass on a NaN median. - Document the NULL/rejection contract in tdigest.h (td_new returns NULL, td_init returns 1 and leaves *result untouched, td_free(NULL) is a no-op), and correct the td_free comment: td_new()'s validation landed in RedisBloom#41. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…estimate Follow-up to gabsow's second review on RedisBloom#44: - td_capacity_test assumed INT_MAX is always the binding capacity limit, but the helper also rejects on SIZE_MAX/sizeof(double) and SIZE_MAX/sizeof(long long). On a 32-bit size_t target the per-array element limit (SIZE_MAX/8) is smaller than INT_MAX, so max_ok would be rejected and the test would fail. Derive max_capacity = min(INT_MAX, SIZE_MAX/sizeof(double), SIZE_MAX/sizeof(long long)) and take the boundary from that. On 64-bit size_t INT_MAX still binds, so the tested boundary (357913939 -> cap 2147483644) is unchanged. - Fix the tests/CMakeLists.txt include path: CMAKE_SOURCE_DIR points at the outermost project and breaks when t-digest-c is used via add_subdirectory(); use CMAKE_CURRENT_LIST_DIR/../src instead. - Correct the memory estimate: two 8-byte node arrays at cap ~ INT_MAX total ~34 GB (~32 GiB), not ~17 GB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

fcostaoliveira · 2026-07-28T21:38:20Z

Both blockers from the last review are addressed:

Stale base / duplicate Guard TDigest capacity calculation before integer narrowing #41 history. Rebased test: extend td_init coverage + guard td_free(NULL) (follow-up to #41) #44's three commits directly onto current master (which now contains the squash-merged Guard TDigest capacity calculation before integer narrowing #41), dropping the duplicated Guard TDigest capacity calculation before integer narrowing #41 commits, and retargeted this PR to base: master. The diff is now exactly the three follow-up commits (5 files: td_free(NULL) guard + docs, the td_init/capacity tests, and td_capacity_test) — no Guard TDigest capacity calculation before integer narrowing #41 history.
Sanitizer coverage of td_capacity_test. Fixed on ci: add GitHub Actions build/test + ASan/UBSan (fix cmake test build) #43: its ASan/UBSan job no longer compiles a single hardcoded td_test. It now does a CMake Debug build of the whole suite and runs it via CTest, so every registered test target — including td_capacity_test — is covered automatically. (Debug is explicit because the project defaults to Release, whose -std=c99 hides POSIX clock_gettime() in minunit.) Verified locally: ctest under ASan+UBSan builds and passes both td_test and td_capacity_test on this branch.

sonarqubecloud · 2026-07-29T08:19:55Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

gabsow

Verified the latest fixes: capacity validation and exact boundary coverage are correct, td_free(NULL) is guarded, the full sanitizer suite passes, and all CI checks are green.

fcostaoliveira requested a review from gabsow July 27, 2026 16:31

gabsow reviewed Jul 27, 2026

View reviewed changes

fcostaoliveira requested a review from gabsow July 27, 2026 20:09

gabsow reviewed Jul 27, 2026

View reviewed changes

fcostaoliveira requested a review from gabsow July 27, 2026 21:55

fcostaoliveira mentioned this pull request Jul 27, 2026

Fix O(n²) compress on duplicate/low-cardinality input (MOD-17228) #42

Closed

gabsow reviewed Jul 28, 2026

View reviewed changes

fcostaoliveira and others added 3 commits July 28, 2026 22:33

fcostaoliveira force-pushed the td-init-tests-and-free-null branch from 2fb6b5a to 53f9e69 Compare July 28, 2026 21:37

fcostaoliveira changed the base branch from codex/td-init-capacity-guard to master July 28, 2026 21:37

fcostaoliveira requested a review from gabsow July 28, 2026 21:38

Merge remote-tracking branch 'upstream/master' into pr44-work

caadf02

gabsow approved these changes Jul 29, 2026

View reviewed changes

gabsow merged commit fddc36e into RedisBloom:master Jul 29, 2026
7 checks passed

gabsow mentioned this pull request Jul 29, 2026

MOD-17228: Fix O(n²) t-digest compress on duplicate input #45

Merged

Uh oh!

Conversation

fcostaoliveira commented Jul 27, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verified locally

Uh oh!

gabsow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabsow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcostaoliveira commented Jul 27, 2026

Uh oh!

gabsow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabsow commented Jul 27, 2026

Uh oh!

gabsow left a comment

Choose a reason for hiding this comment

Uh oh!

fcostaoliveira commented Jul 28, 2026

Uh oh!

sonarqubecloud Bot commented Jul 29, 2026

Quality Gate passed

Uh oh!

gabsow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fcostaoliveira commented Jul 27, 2026 •

edited by cursor Bot

Loading