fix(bm25,vector): advance the watermark on an empty change set, don't rebuild - #1661
Conversation
… rebuild
The incremental sync traced the commits in `(watermark, ledger_t]`, asked which
subjects they touched, and treated an EMPTY answer as a reason to rebuild the
whole index from a full ledger re-query.
An empty answer there is determinate, not a failure to determine. The walk
completed, and `affected_subjects` reported that no commit touched a property the
index depends on. Queries whose shape cannot be narrowed at all never reach that
point — they decline earlier, and declining is what correctly routes those to a
full resync. So an empty set means the index is already correct as of `ledger_t`,
and the only work owed is to record it.
On an append-heavy ledger whose writes mostly miss the indexed properties, this
made the cheapest possible window trigger the most expensive possible operation,
once per commit. Measured on a deployment indexing as:content / as:name /
as:summary / as:preferredUsername while the write volume was observations
carrying only sosa: properties:
- 23 full index rebuilds in 10 minutes across 4 ledgers
- one of them from a ONE-commit window (old_watermark=15194 ledger_t=15195)
over a 15,000-commit ledger
- resident memory reached 23.5 GiB of non-reclaimable anon against a 6 GiB
index-cache bound, and the process was OOM-killed three times
It also feeds back: a rebuild is slow, the ledger advances hundreds of commits
while it runs, so the next window is wider and — still containing no indexed
change — rebuilds again. The staleness gap grew monotonically under observation.
The watermark is advanced and PERSISTED rather than left in place. Leaving it is
cheaper per pass but never converges: every later sync re-walks a commit range
that grows without bound, which on a busy ledger becomes thousands of commit
reads per sync, a slower version of the same problem. Persisting costs one index
serialization — O(index), bounded — and skips exactly the expensive half of a
resync, the full ledger re-query and rebuild.
Appending a manifest entry that points at the EXISTING snapshot id would avoid
even that serialization, since Bm25SnapshotEntry already carries index_t. Not
done: `manifest.trim()` plus `delete_old_snapshots(removed)` could then delete a
blob a newer entry still references.
The vector graph source had the identical fallback and is fixed the same way. Its
rebuilds are dearer still, because a resync re-embeds every document.
A note on the test, because the first version of it was useless. `was_full_resync`
is the only observable that separates the two paths — doc count and watermark are
identical either way, since a resync arrives at the same correct index by
rebuilding it, which is why this went unnoticed. And the obvious "unrelated write"
does not exercise the path at all: `affected_subjects` filters flakes by
PREDICATE, and a query matching on `@type` makes rdf:type a dependent predicate,
so any typed insert counts as an indexed change. The test passed against the
unfixed code until the fixture dropped `@type` entirely. Mutation-checked:
restoring the old fallback fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
@christophediprima this is a good catch and I like the mechanism here. I validated that the the O(corpus)-rebuild-per-empty-window pathology is real, the persist-vs-leave-the-watermark tradeoff is reasoned correctly, and the manifest-entry alternative was rejected for exactly the right blob-lifetime reason.
With that in mind, I want this PR to land, but I'd like to raise a few concerns first.
The premise the PR stands on--"an empty affected set is determinate"--is only true when property-dep extraction is total, but this isn't always true: wildcard selects, nested select projections, and variable predicates all produce deps that silently under-track, and the resync fallback this PR deletes was the correctness net over that gap.
I had Claude build and run the repro both ways: a select {"?x": ["*"]} index goes permanently, silently stale at this HEAD (was_full_resync=false, upserted=0, watermark advanced past the change) but did stay correct at the pre-PR merge base. Silent staleness in an index that is_valid_at then vouches for is wrong-query-results territory, so I have to block on it. The fix, though, is likely small: track a completeness bit in PropertyDeps::from_indexing_query and keep the resync fallback when deps are incomplete or empty (details and a suggested shape in the inline comment).
With that guard in, everything else here is a clear approve.
Adherence to repo commitments:
- Patterns/abstractions: ✔ extends the existing sync/manifest/retention machinery rather than inventing a parallel path; the empty branch's persist is byte-for-byte the normal tail.
- Performance (speed first, memory second): ✔ background sync path, not the query engine; converts O(corpus) rebuilds into one bounded O(index) persist; no per-row allocations added. No performance-degradation risk.
- Testing:
⚠️ the new test is excellent, runs ingrp_graphsource(verified by name), and is mutation-checked (verified) — but the un-narrowable query shapes have no coverage, the vector path has no test, and the repro above shows the missing case is the one that bites. - Conventions: ✔ thorough multi-line commit body; fmt/clippy clean (default features locally; all-features green in CI); self-describing title.
Verified locally at branch HEAD: grp_graphsource 142/142 green; author's mutation check reproduced (old fallback → new test fails); adversarial wildcard-select repro fails at HEAD and passes at base; cargo fmt --check and cargo clippy -p fluree-db-api --all-targets -- -D warnings clean.
Just be sure to get the completeness guard (or an equivalent decline for un-narrowable deps) into both bm25.rs and vector.rs before this merges — with that in place I'm enthusiastically on board, and happy to re-review quickly.
| // problem. Persisting costs one index serialization, which is O(index) and | ||
| // bounded, and skips exactly the expensive half of a resync: the full | ||
| // ledger re-query and rebuild. | ||
| if affected_sids.is_empty() { |
There was a problem hiding this comment.
🔴 Blocking — the "empty is determinate" premise fails for legal query shapes, converting an expensive-but-correct rebuild into permanent silent staleness.
The whole fix rests on "an empty affected_subjects means no commit touched a property the index depends on." That's only true when PropertyDeps::from_indexing_query extracts every predicate the query can observe, and it doesn't: a select wildcard ({"?x": ["*"]}) becomes a literal dep IRI "*" that never encodes and is silently dropped by CompiledPropertyDeps::compile (index.rs:535-537); an object-valued select entry ({"ex:author": ["ex:name"]}) fails prop.as_str() in extract_select_properties (index.rs:487-495) and is dropped with no recursion, losing both ex:author and ex:name; a variable predicate key returns None. And there is no completeness or is_empty guard anywhere in the sync path — the commit's "queries whose shape cannot be narrowed decline earlier" describes a decline that does not exist in the code (the only pre-walk resync is index_id.is_none() at bm25.rs:867-869).
Consequence, Claude-verified with an executable repro run both ways: create a BM25 index with where {"@id":"?x","@type":"ex:Doc"} / select {"?x": ["*"]} (deps compile down to rdf:type alone), then update ex:title on an existing doc. At this PR's HEAD the sync returns was_full_resync=false, upserted=0, advances the watermark past the change, and the new term is never indexed — later syncs start above it, so the staleness is permanent, and is_valid_at happily serves the stale index as current. With the pre-PR fallback restored the same test passes (was_full_resync=true, upserted=1, term present). This is the worst failure class — silently wrong query results — and it lands on exactly the "index everything on these subjects" configs users write when they don't want to enumerate properties.
To be fair about what's pre-existing: for these query shapes the incremental path was already silently lossy before this PR whenever a window mixed tracked and untracked predicates (the empty fallback only rescued windows that touched no tracked predicate at all). Your change didn't create the extractor's gaps — but the empty→resync fallback was the last correctness net over them, and on low-write-rate ledgers (one-commit windows, the case this PR's own commit message leads with) it was catching most of the damage.
Fix path, and I think it's small: make determinacy a checked property instead of an assumed one. Give PropertyDeps a complete: bool (or an Unnarrowable marker) that from_indexing_query clears whenever it meets "*", a non-string select entry, a variable predicate key, or any construct it doesn't positively recognize — then keep the old resync fallback in both syncs when !deps.complete || deps.is_empty(), and take the new watermark-advance branch only for provably-total deps. That preserves 100% of the win for the enumerated-property configs the PR was tuned on. A unit test asserting a wildcard-select config yields complete=false would pin it. The same guard needs to land in vector.rs:508 — and while you're in there, a vector-side copy of the sync test would be welcome since that path currently ships with no test at all — the feature gating makes it more work, but I'd rather see it land here than in a follow-up issue we all forget about.
(Commenting here because the extractor's gaps live in fluree-db-query/src/bm25/index.rs:394-509, which this diff doesn't touch.)
| } | ||
|
|
||
| // If no subjects affected, fall back to full resync | ||
| // Nothing the index covers changed in this window. |
There was a problem hiding this comment.
Praise, and a suggestion. The compile-against-ledger_t choice is what makes the silent encode-skip sound (namespaces are append-only, so a predicate with flakes in the window always encodes). That argument is load-bearing and non-obvious — a one-line comment on the encode_iri closure saying "sound because namespaces are append-only and this ledger is at ledger_t" would keep someone from 'fixing' it against the watermark-time db later.
| /// append-heavy ledger whose writes mostly miss the indexed properties, that made | ||
| /// the *cheapest* possible window trigger the *most expensive* possible operation, | ||
| /// once per commit. In production it produced 23 full rebuilds in 10 minutes and | ||
| /// OOM-killed the process three times. |
There was a problem hiding this comment.
Praise. The test's design notes (why was_full_resync is the only separating observable; why the obvious @type-carrying fixture silently passes against unfixed code) are exactly the kind of trap-documentation that stops a future regression from being written with a green suite. Mutation claim reproduced: restoring the old fallback fails it.
| // because a resync re-embeds every document. | ||
| if affected_sids.is_empty() { | ||
| warn!( | ||
| index.watermark.update(&source_ledger_alias, ledger_t); |
There was a problem hiding this comment.
Observation. The empty-branch persist matches the vector sync's own normal tail (blob write + publisher head), and neither path GCs old vector blobs — pre-existing parity, not introduced here, just noting I checked it.
2 participants
The incremental sync traces the commits in
(watermark, ledger_t], asks which subjects they touched, and treats an empty answer as a reason to rebuild the whole index from a full ledger re-query.An empty answer there is determinate, not a failure to determine. The walk completed, and
affected_subjectsreported that no commit touched a property the index depends on. Queries whose shape cannot be narrowed at all never reach that point — they decline earlier, and declining is what correctly routes those to a full resync. So an empty set means the index is already correct as ofledger_t, and the only work owed is to record it.Why this is worth fixing rather than tidying
Whether it matters at all depends entirely on how often a commit misses the indexed properties, and that is a property of the data, not of the index.
An index over a handful of text properties, on a ledger whose write volume is dominated by subjects that carry none of them, hits the empty case on most commits. Each one then rebuilds the entire index — so the cost per sync jumps from
O(delta)toO(corpus), and it does so precisely in the situation where there was nothing to do at all.It also feeds back. A rebuild is slow, the ledger keeps advancing while it runs, so the next window is wider — and, still containing no indexed change, rebuilds again. The index falls further behind the more often it syncs, and the transient state of a rebuild is allocated outside the index cache, so resident memory follows. We hit this on a deployment with exactly that shape: repeated whole-index rebuilds a few seconds apart, a staleness gap that grew monotonically rather than converging, and eventually the process being OOM-killed.
The fix
When the affected set is empty, set the index watermark to
ledger_t, persist, and return a no-op result.The watermark is advanced and persisted rather than left in place. Leaving it is cheaper per pass but never converges: every later sync re-walks a commit range that grows without bound, which on a busy ledger becomes thousands of commit reads per sync — a slower version of the same problem. Persisting costs one index serialization, which is
O(index)and bounded, and skips exactly the expensive half of a resync: the full ledger re-query and rebuild.An alternative would be to append a manifest entry pointing at the existing snapshot id, avoiding even that serialization since
Bm25SnapshotEntryalready carriesindex_t. I did not do that:manifest.trim()plusdelete_old_snapshots(removed)could then delete a blob a newer entry still references.The vector graph source has the identical fallback
vector.rscarried the same code and is fixed the same way. Its rebuilds are dearer still, because a resync re-embeds every document.Testing
fmtandclippy --all --all-features --all-targets -- -D warningsclean.fluree-db-api --liband thegrp_graphsourcesuite both pass.One new test, and two things about it worth flagging, because both made this easy to miss.
was_full_resyncis the only observable that separates the two paths. Document count and watermark are identical either way, since a resync reaches the same correct index by rebuilding it — so every assertion you would naturally reach for passes against the unfixed code.And the obvious fixture does not exercise the branch.
affected_subjectsfilters flakes by predicate, so a query that matches on@typemakesrdf:typea dependent predicate, and inserting an unrelated typed subject still counts as touching an indexed predicate. The fixture has to touch no dependent predicate at all. Mutation-checked: restoring the old fallback fails the test.