fix: sort posting list when spilling memtable (root-cause MS MARCO bucket-8 hang)

[tjgreen42]

Summary

Root-cause and fix the MS MARCO bucket-8 hang in production benchmarks (nightly run 25648758099), plus close the watchdog gap that allowed the underlying corruption to persist undetected.

Root cause: unsorted block_postings in spilled segments

tp_write_segment (memtable → segment spill path) iterates a term's posting-list entries in array order and writes them as block_postings[] without sorting. This is fine under single-writer COPY/CREATE INDEX, because entries are appended in CTID order, which matches doc_id order after tp_docmap_finalize. Under concurrent inserts, multiple backends interleave appends to the same posting list in arbitrary thread-scheduling order:

// src/memtable/posting.c:191
posting_list->is_sorted = false;  /* New entry may break sort order */

That is_sorted flag is set on every insert but never set back to true anywhere — there's no sort step on the read side. The spilled segment's block_postings then violate the sorted-block invariant the entire BMW machinery relies on (binary search on block_last_doc_ids, seek_term_to_doc finding "first doc ≥ target", find_wand_pivot's smallest-first walk), and merges propagate the corruption to higher levels.

Diagnostic evidence

A temporary debug function (bm25_check_segment_consistency, removed in the final commit once root cause was fixed) walked every segment / term / block and compared cached skip-data against actual on-disk block_postings. Run against the failing benchmark's MS MARCO corpus before the fix:

Checked 2 segments, 4,080,947 terms, 5,776,365 blocks.
Total inconsistencies: 904,174

All examples were "not strictly ascending within block", with delta magnitudes up to ~200,000 doc_ids — genuine out-of-order data, not a one-byte glitch. (Check #1 — skip.last_doc_id ≠ postings[doc_count-1].doc_id — fired zero times, so the skip metadata was always consistent with what got written; what got written was the bug.)

Fix

1. Sort posting list by doc_id when spilling memtable to segment

src/segment/segment.c — qsort(block_postings, doc_count, ..., cmp_by_doc_id) after building it in tp_write_segment, before splitting into TP_BLOCK_SIZE blocks. doc_id is monotonic with CTID after tp_docmap_finalize, so this is equivalent to sorting by CTID — the invariant the segment format documents and the merge / scoring code assumes. build_context.c is unaffected (EXPULL streams already-sorted entries).

2. Harden seek_term_to_doc to actually scan multiple blocks (defense-in-depth)

src/scoring/bmw.c — The old code had two unverified fall-through paths that loaded one "next" block and returned true based purely on !iter.finished, without verifying that the newly loaded block's first doc actually reached target_doc_id. Under correct invariants those fall-throughs are unreachable, but with the segment-sort bug they fired and seek_to_pivot's i--; continue; re-entry spun forever (the gdb-observed bucket-8 hang at bmw.c:1329).

Restructured so the fast path and binary-search path both just position the iterator at a candidate starting block, then a single block-advancing scan loop keeps advancing until it finds a posting >= target (returns true) or exhausts the iterator. The post-condition cur_doc_id >= target_doc_id is now guaranteed by control flow.

Even with the segment-sort root cause fixed, this is a worthwhile correctness improvement — the original seek_term_to_doc was latently incorrect for any skip-data inconsistency.

3. CHECK_FOR_INTERRUPTS in BMW hot loops

src/scoring/bmw.c — Added in two places:

• Inside the new for(;;) block-advancing loop in seek_term_to_doc (per-iter disk I/O + decompression should be cancelable).
• Inside seek_to_pivot's for loop (the prior PR BMW scan hangs uninterruptibly on bucket-7 MS MARCO queries (Benchmarks workflow timing out for weeks) #355 added CFI only to the outer WAND main loop; if seek_to_pivot itself spins, we never return there).

4. Validation watchdog actually validates

The corruption above existed for as long as concurrent inserts have, and we had a validation step (validate_queries.sql against ground_truth.tsv) explicitly designed to catch this class of correctness regression. It never fired. Two latent bugs:

• validate_queries.sql had an ambiguous-column reference (WHERE query_id = p_query_id where query_id shadows a plpgsql variable). psql errored out partway through with ON_ERROR_STOP.
• Every Validate ... workflow step pipes psql through tee (no set -o pipefail), then grep -q "VALIDATION FAILED" to decide pass/fail. When the SQL errors before reaching the FAILED marker, psql's non-zero exit is swallowed by tee, the grep finds nothing, and the step claims success.

Fixed in benchmarks/datasets/msmarco/validate_queries.sql (qualify as ground_truth.query_id) and in .github/workflows/benchmark.yml (6 affected Validate ... steps): each now sets set -o pipefail and requires an explicit VALIDATION PASSED marker — absence of which fails the step. Future SQL-level errors in validation will fail the run loudly instead of silently passing. (Cranfield has no validation step in the workflow at all; out of scope.)

Verification

• ✅ Built clean on PG 17 / PG 18 (-O2 -g, no new warnings)
• ✅ All 61 regression tests pass via pg_regress
• ✅ make format-check clean
• ✅ Local stress repro: 8-thread pgbench concurrent inserts of 4000 docs over 8 terms → spill → consistency check (during development). Before this fix: thousands of inconsistencies. After: 0.
• ✅ First successful end-to-end benchmark run 25689967666:
  • Run MS MARCO insert benchmark completed in seconds (was 32m+ hang) — bucket-8 p99=59ms, n=100, results=1000
  • Run MS MARCO concurrent insert (8-thread pgbench, 8.8M passages) ran the full query suite + validation — first time these steps have ever reached completion
• 🟡 Validated run with hardened watchdog: in progress at 25696335265. The previous green run's "VALIDATION PASSED" was actually a spurious-pass — the hardened watchdog (commit 95d064c7) will give us a truthful signal.

Migration

Pre-existing corrupt segments in production indexes will need REINDEX after upgrading — they cannot be repaired by future merges (which propagate, not heal, the sort violation).

Refs: #355 / #357 (related but distinct BMW infinite-loop site: block_max_skip_advance).

[codecov]

Codecov Report

❌ Patch coverage is 67.44186% with 14 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/scoring/bmw.c	61.11%	14 Missing ⚠️

📢 Thoughts on this report? Let us know!