Repository navigation
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) <[email protected]>
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.
…set-no-resync # Conflicts: # fluree-db-api/tests/it_graph_source_bm25.rs
`PropertyDeps::from_indexing_query` collected the predicates it could see and said nothing about the ones it could not. A `*` select became the literal dependency `"*"` that never encodes; a nested selection failed `as_str()` and vanished with both of its predicates; a variable predicate returned `None`; a bare term under `@vocab` survived as a string no ledger would ever match. Every one of those left the set silently short, and the incremental sync had no way to tell a short set from a total one. `PropertyDeps::analyze` walks the same query and returns the set together with `incomplete: Option<String>`, set the first time it meets a construct it cannot vouch for. The rule it enforces is the one incremental sync actually needs: every predicate the query observes is in the set, and each sits on the document subject, so that a changed flake can be attributed to the document it changes. That admits the documented shape — object-form `select` over one variable, explicit properties, `where` patterns rooted at that variable, with `optional`/`union`/`filter`/`values`/`bind`/`graph` around them — and declines wildcards, nested projections and patterns, reverse terms and selections, `@path` aliases, variable predicates, anonymous or constant subjects, subqueries, `depth`, result-shaping clauses, and selects that name no document. Keys now expand through the real JSON-LD context (`@vocab`, `@base`, term definitions, array contexts, `@id`/`@type` aliases) via the same `fluree_graph_json_ld` path the parser uses, which is what makes "does not encode at `ledger_t`" mean "the query cannot match it either". `from_indexing_query` is unchanged in signature and now delegates. The verdict is deliberately not persisted. `VectorPropertyDeps` is postcard-serialized, so a new field on `PropertyDeps` would break loading every existing vector blob; computing it from the stored config at sync time also judges an index built by an older extractor by the current one.
… is complete
Advancing the watermark on an empty change set is sound only if the
tracked property set is total. It was not always: a `select {"?x": ["*"]}`
index compiled its dependencies down to `rdf:type`, so a commit that
changed `ex:title` produced an empty change set, the sync recorded the
index as current at that `t`, and every later sync started above it. The
term was never indexed and `is_valid_at` vouched for the stale index.
Both syncs now run `PropertyDeps::analyze` on the stored config before the
commit walk and, when the dependencies are incomplete, log the reason at
`warn` and take the full resync. That is the decline the empty-set branch
always assumed existed, and it is stricter than guarding that branch alone:
an incomplete query is rebuilt on every sync, not only on empty windows.
The incremental path was already silently lossy for such queries whenever a
window mixed tracked and untracked predicates, so the middle ground was
fast and wrong rather than fast and correct. The dependencies used for the
walk come from the same analysis, so the verdict and the predicate set
cannot disagree, and the vector path adds the embedding property as the
builder does.
The compile-against-`ledger_t` closure now says why a silent encode skip is
sound — namespaces are append-only and this snapshot is the newest — so
nobody "fixes" it to the watermark-time db.
Tests, mutation-checked with the gate disabled: BM25 wildcard (the review's
repro), nested projection, and variable predicate; vector empty-window
advance, wildcard, and a nested pattern that changes membership. The
routing-sensitive ones fail without the gate and the empty-window ones
still pass.
Replace "if no affected subjects are detected, it falls back to a full resync" with the two rules that now hold: a query outside the trackable shape is rebuilt on every sync with the reason logged, and inside it an empty change set advances the watermark without touching the index.
bplatz
added
bug
|
Merged Completeness guard (the blocking item). The verdict is computed from the stored config rather than persisted: Tests. Unit tests on the analysis; BM25 integration tests for the wildcard repro, nested projection, and variable predicate; vector twins for the empty-window advance, wildcard, and a nested pattern that changes membership. Mutation-checked: with the gate disabled the five routing-sensitive tests fail and the two empty-window tests still pass. The encode-closure soundness comment is in both syncs, and the docs describe the shape rules. Status. fmt and wasm32 are green on the new head; clippy and the test jobs are still running. |
3 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.