Skip to content

fix(similarity): skip empty fingerprint sort - #1368

Merged
DeusData merged 2 commits into
DeusData:mainfrom
WarGloom:agent/fix-empty-similarity-sort
Aug 4, 2026
Merged

fix(similarity): skip empty fingerprint sort#1368
DeusData merged 2 commits into
DeusData:mainfrom
WarGloom:agent/fix-empty-similarity-sort

Conversation

@WarGloom

Copy link
Copy Markdown
Contributor

What changed

collect_fp_entries() now calls qsort only when at least two fingerprint
entries were collected.

Why

The empty path leaves entries == NULL. Calling qsort(NULL, 0, ...) relies on
library behavior outside the portable array-pointer contract, while the
one-entry path needs no sorting at all. Guarding with count > 1 avoids both
unnecessary cases without changing deterministic ordering.

Fixes #1367.

Validation

  • scripts/build.sh — passed
  • scripts/test.sh --suites simhash — passed, 24 sanitizer-backed tests
  • git diff --check — passed

Checklist

  • Commit is signed off (DCO)
  • Change is focused on one tracked bug
  • Existing focused tests pass

Signed-off-by: wargloom <wargloom@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Thank you for this — and doubly so for reporting the issue and bringing the fix.

This is exactly the shape a fix should have: one file, one guard, and a comment that explains the why rather than restating the code. The distinction you drew in #1367 is the right one — zero elements suppress comparator calls, but that has never made a null base a valid array pointer, and count > 1 also skips the one-entry sort that could never have changed anything.

I checked the caller before merging: entry_count < MIN_FP_ENTRIES returns early via free(entries), and free(NULL) is well-defined, so the empty path stays correct end to end. Determinism at two or more entries is untouched.

Merging. Thanks again for the care you put into both the report and the patch.

@DeusData

Copy link
Copy Markdown
Owner

One thing before I merge: this is still marked as a draft, so I have held off rather than promote it myself.

If that is just left over from opening it, mark it ready (or say the word here) and it goes in as-is — the review above is complete and CI is 28/28 green. If instead you are still polishing something, take the time you need; there is no rush from our side and I would rather merge the version you consider finished.

@WarGloom
WarGloom marked this pull request as ready for review July 31, 2026 12:00
@WarGloom
WarGloom requested a review from DeusData as a code owner July 31, 2026 12:00
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Aug 3, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Aug 3, 2026
@DeusData

DeusData commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Thank you for the focused empty-fingerprint guard and for marking the PR ready. I have routed it at high priority in 0.9.1-rc because it removes undefined behavior from the indexing pipeline. It is back in the maintainer review queue, which is currently full, so the final re-review may take a little time.

@DeusData

DeusData commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Thank you for this one — a textbook fix: exact scope, a clear rationale comment right at the call site, and an honest write-up of why the empty path violates the qsort contract even though every libc happens to tolerate it. The determinism contract for count >= 2 is untouched, so this is behavior-identical where it matters and strictly more correct where it doesn't. Much appreciated, as always!

One note: we verified that no C-level assertion can bind this fix (the existing simhash suite already drives the empty path, but only a trap-on-UBSan build would ever fail without the guard). We're taking a follow-up on our side to bind it in the sanitizer lane so it can never silently regress.

@DeusData
DeusData merged commit b6fe834 into DeusData:main Aug 4, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pass_similarity calls qsort with a null base for an empty fingerprint set

2 participants