fix(transact): give staged views dictionaries that cover their own flakes - #1791
fix(transact): give staged views dictionaries that cover their own flakes#1791bplatz wants to merge 3 commits into
Conversation
…akes
A transaction's SHACL and f:postState policy probes read a StagedLedger on
the binary lane once the ledger has a persisted index. The lane resolves
every overlay flake through the persisted dictionary plus DictNovelty, both
committed-state artefacts, so the subjects and strings the transaction is
introducing resolve in neither: every probe failed to translate exactly the
transaction's own flakes ("subject not found in persisted or novelty
dict"), logged a WARN, and merged them raw — after re-walking and
re-translating the whole graph novelty, because the staged view reported
no content version and so was never cached.
`attach_staged_dicts` clones the base dictionaries, extends them
persisted-first with the staged flakes, and attaches a provider over them
to the staged view before SHACL validation and post-state policy
evaluation. The policy executor reads post-state conditions through that
snapshot. Commit still rebuilds the provider from the ledger's canonical
dictionaries, so the view-local ids never reach a committed state.
StagedLedger now reports a process-unique content version, drawn from a
core allocator shared with Novelty, and the query engine's cross-query
translation cache is keyed on it instead of the overlay epoch: a staged
view reports the very epoch and to_t the committed novelty reports right
after its flakes commit, so an epoch key would serve the staged product
(view-local ids) for the committed state. Overlays without a content
version are no longer cached across queries.
Two paths that mutated dict_novelty in place with the provider still
attached — sequential multi-operation SPARQL staging and commit-transfer
apply — now detach and reattach it, so reads through the state resolve the
subjects those operations introduce instead of the pre-mutation copies.
The new it_staged_view_dict binary pins the translation outcome through a
tracing probe (SHACL and post-state policy over an indexed ledger) and the
post-commit read; each assertion was watched failing without the fix.
`notify` reads the nameservice record before taking the ledger's state lock, and the raft commit worker holds that lock's write side across the next chunk's stage + publish + install. A reconciliation queued behind it wakes to a local state one commit newer than the record it fetched, and `UpdatePlan::plan` logged that as an unexpected "local t ahead" WARN on every sequential publish. The record is merely stale and the plan was already a no-op; the branch now logs at debug with the ordering spelled out. The two `mut` view bindings that only the SHACL pass needs are now gated on the feature so a default-feature clippy stays clean.
aaj3f
left a comment
There was a problem hiding this comment.
@bplatz this is a fix for a real & serious problem and I had Claude help me validate the seriousness of the problem pre-existing this PR. The design pattern to extend the dictionary view w/ a provider over it also seems right to me (i.e. instead of minting ids into novelty during staging for a commit that could still fail, etc). Claude found a few significant issues you'll likely want to address, including an accidental scenario in which a useful cache is accidentally made invalid, so I'll leave Claude's review verbatim below:
This closes a real production wound, @bplatz, and the transaction half is built the way I'd want it: a view-local dictionary extension over a clone so the canonical dictionaries are never touched by a transaction that may still fail, an attach that runs only before SHACL validation and post-state policy evaluation (verified at all three call sites — a transaction with neither pays one atomic increment), commit rebuilding the provider from the canonical dictionaries so view-local ids never reach a committed state (traced through commit_txn, every into_parts caller, the sequential stager, commit-transfer apply and the catch-up loop), and the two detach/reattach brackets fixing pre-existing bugs on the way. Dropping the three attach calls turns three of the four new tests red, so it is load-bearing. Every gate I could run locally is green.
What I have to block on is the other half. Re-keying the cross-query translation cache on content_version is right in principle, but content_version? in warm_whole_product (and the insert path) means an overlay that returns the trait default None is never cached across queries — and at HEAD the only implementors are Novelty and StagedLedger. ReasoningOverlay, SchemaBundleOverlay, OverlayRef, DerivedFactsOverlay, CompositeOverlay and HistoricalLedgerView all return None, so every reasoning-enabled query, every query on a ledger with f:schemaSource or an inline ontology, and every datalog rule iteration now re-translates the whole overlay per execution — the multi-second floor the global cache was added to remove. At base their composed epochs (base.epoch() * 1_000_003 + derived.instance_id() and friends) were content-stable, which is why it worked. The fix is mostly one-liners (forward in OverlayRef/HistoricalLedgerView; allocate a stamp at construction for the immutable DerivedFactsOverlay/SchemaBundleFlakes; intern a stamp from part versions for the composites) plus a unit test that a ReasoningOverlay over the same parts reports a stable Some. Second: the epoch collision this PR fixes has no test — restoring Some(epoch) as the key leaves all four new tests green, because the SHACL probes are predicate-bound and below the 256-match guard, so no whole-graph product is ever cached during staging; the caching half of the fix is unpinned the same way. A unit test with two overlays reporting equal epoch()/to_t and different content, or an unbounded probe followed by the post-commit read, gives it teeth.
One item I'd name as follow-up-acceptable, because it needs a new dictionary shape (though by all means consider it now if you'd prefer it in): the attach deep-clones the committed DictNovelty (O(subjects + strings since the last reindex) per SHACL/policy transaction, ~100 MB transient at a million novel subjects) and commit_txn repeats its persisted-dictionary probes; a layered dictionary makes attach O(staged flakes). Still a clear win over base as is. Optional: the WARN → debug downgrade is unconditional where the race is exactly a gap of one; a config-enabled-but-shapeless graph still attaches and validates; staged one-shot entries evict committed readers from the 4-entry global cache. Also worth knowing: nothing in this diff depends on #1789 (content_version, Novelty::content_version and the range-cache key all predate it), so this production fix could be retargeted to main and get CI rather than wait on the stats review.
Adherence to repo commitments:
- Patterns/abstractions: ✔ extends
BinaryRangeProvider/DictNovelty/ theOverlayProvidercontract rather than a parallel path; commit's own extension untouched;⚠️ thecontent_versioncontract is now enforced by the cache but only two overlay types honour it. - Performance (speed first, memory second): ✖ CRITICAL — reasoning / schema-bundle / datalog overlays lose cross-query translation caching (
binary_scan.rs:2428); ✔ plain transactions unchanged, SHACL/policy transactions a net win;⚠️ O(|dict novelty|) clone per attach. - Testing: ✔ four integration tests in a declared
[[test]]with a tracing probe, mutation-red on the attach; ✖ the epoch collision and the caching half are unpinned (key →Some(epoch)andcontent_version → Noneboth stay green);⚠️ no CI on this head (stacked). - Conventions: ✔ three focused commits with the mechanism; the caveat test points at #1790;
shacl-gatedmutbindings keep default-feature clippy clean; fmt/clippy clean locally.
Verified locally at branch HEAD c4bce6131: it_staged_view_dict 4/4, grp_transact 197 (+3 ignored), fluree-db-transact 295 + 46, fluree-db-query binary_scan 13, fluree-db-novelty 95, fluree-db-ledger 37 + 1, fluree-db-core overlay 5; cargo fmt --all -- --check and clippy -D warnings --all-targets --no-deps on the five touched crates clean; mutations A (attach no-op'd → 3 red), B (key → Some(epoch) → green), C (content_version → None → green), all restored; worktree clean.
Just be sure the composite overlays get a stamp and the collision gets a pin before this merges — and I'd rather see the shapeless-config guard and the gap-of-one warn folded in than in the backlog.
| // translation (ids from a view-local dictionary) for the | ||
| // committed state. An overlay that cannot vouch for a content | ||
| // version is not cached across queries at all. | ||
| let content_version = ctx.overlay().content_version(); |
There was a problem hiding this comment.
CRITICAL (performance on the committed read path — blocking). Keying the cross-query layer on content_version is right in principle, but content_version? here (and on the insert path at :2480/:2514) means an overlay that returns the trait default None (fluree-db-core/src/overlay.rs:109) is never cached across queries — and at HEAD the only implementors are Novelty and StagedLedger. ReasoningOverlay (reasoning.rs), SchemaBundleOverlay (schema_bundle.rs), OverlayRef (datalog_rules.rs), DerivedFactsOverlay (fluree-db-reasoner), CompositeOverlay and HistoricalLedgerView all return None.
At base the key was overlay_epoch: epoch, and those overlays compose a content-stable epoch (base.epoch() * 1_000_003 + derived.instance_id(); base.epoch() * K + bundle.epoch()), which is why the global cache worked for them — it exists precisely for reasoning materializations ("a flat multi-second floor per query once the overlay holds millions of derived facts", :2913). At HEAD every reasoning-enabled query, every query on a ledger with f:schemaSource or an inline ontology, and every datalog rule iteration re-translates the whole overlay on every unbounded scan and every bounded scan that trips the selectivity guard (the hot rdf:type/class scans). Scenario: indexed ledger, OWL2-RL, 2 M derived facts in global_reasoning_cache — base: query 2..N pay ~0 translate; HEAD: multi-second re-translate per query.
Fix: forward content_version() in OverlayRef and HistoricalLedgerView (SizedOverlayRef already does); allocate next_overlay_content_version() at construction for the immutable DerivedFactsOverlay and SchemaBundleFlakes; for the composites, intern a stamp from the tuple of part versions in a small process-global map so "no two differing overlays share a version" holds without hashing. Pin it with a unit test that a ReasoningOverlay over the same (novelty, derived) pair reports a stable Some across constructions and a different one when either part changes.
| @@ -0,0 +1,449 @@ | |||
| //! Staged-view dictionary coverage. | |||
There was a problem hiding this comment.
Should-fix (the collision this PR fixes has no test). Restoring let content_version = Some(epoch); in binary_scan.rs:2428 leaves all four tests here green: the SHACL pass over 40 seeded persons runs predicate-bound probes below the 256-match guard floor, so no whole-graph product is ever inserted into global_translation_cache during staging and the post-commit ?s ?p ?o scan has nothing to collide with. Restoring StagedLedger::content_version → None is also green, so the caching half of the fix (the per-probe re-walk) is unpinned — these tests pin the WARN outcome only, which the body says.
The collision is the silent-wrong-results item (view-local ids served for the committed state), so it deserves its own pin: a binary_scan.rs unit test with two overlays that report equal epoch()/to_t and different content, asserting the second execution does not get the first's TranslatedOverlayOps; or an integration test whose probe is unbounded (an ASK { ?s ?p ?o }-shaped sh:sparql, or ≥ 256 matches so the guard trips) followed by the post-commit read.
| return Ok(()); | ||
| }; | ||
| if !brp.dict_novelty().is_initialized() { | ||
| return Ok(()); |
There was a problem hiding this comment.
Should-fix (performance — naming this as the one item a follow-up is acceptable for, because it needs a new dictionary shape). (**brp.dict_novelty()).clone() is a deep clone of DictNovelty — the reverse: HashMap<Box<[u8]>, u64> is one heap allocation per novel subject, plus the entries and string-dict copies — so the attach costs O(subjects + strings introduced since the last reindex) per transaction that reaches it, not O(staged flakes); ≈ 60–100 B per novel subject, ~100 MB transient at 1 M subjects since reindex, per in-flight SHACL transaction. Then populate_dict_novelty_safe probes the persisted dictionary per staged subject/ref/string, and commit_txn repeats exactly those probes on the canonical dictionary (commit.rs:1004-1012), so SHACL/policy transactions pay them twice; and when a transaction has both shapes and a post-state condition the attach runs on two different views (stage.rs:1990 then :3243 / tx.rs:1245) — two clones, two probe passes.
Versus base this is still a clear win (one clone replaces O(probes × novelty) re-translation and the WARN storm), which is why it isn't blocking. The shape that removes the cost: a layered dictionary — the base Arc<DictNovelty> untouched plus a small staged delta consulted after it — makes attach O(staged flakes) with no copy of the base; and letting commit_txn adopt the staged extension when Arc::ptr_eq(view.base().dict_novelty, canonical) still holds runs the probes once.
| @@ -1970,12 +1970,18 @@ impl UpdatePlan { | |||
| // Large gap — full reload | |||
| UpdatePlan::Reload | |||
| } else { | |||
There was a problem hiding this comment.
Optional. The WARN → debug downgrade is unconditional: any ns.commit_t < local_t now logs at debug. The plan was already Noop, so behaviour is unchanged — but the described race is exactly local_t − ns.commit_t == 1 (one in-flight raft chunk), and a larger gap (a rolled-back record, a branch reset, two managers on one record) is still a genuine divergence that is now invisible. if local_t - ns.commit_t == 1 { debug!() } else { warn!() } keeps the signal; the test can stay as is.
| @@ -1228,6 +1237,14 @@ pub(crate) async fn apply_shacl_policy_to_staged_view( | |||
| return Ok(()); | |||
There was a problem hiding this comment.
Optional. This early return fires only when !has_config && shacl_cache.is_empty(), and the one at :1010-1013 only when has_config && per_graph_policy.is_none(), so a ledger whose config enables SHACL for a graph but has zero shapes falls through to attach_staged_dicts + validate_view_with_shacl on every transaction — paying the dictionary clone for nothing. if shacl_cache.is_empty() { return Ok(()) } regardless of config. Related, small: with StagedLedger now vouching, every SHACL/policy transaction inserts one-shot entries keyed by its own stamp into the 4-entry global_translation_cache (binary_scan.rs:2921) and the other capacity-8 LRUs; a transaction touching two graphs × two index orders evicts every committed-state entry from the global cache, so readers between stage and commit re-translate. Bounded and harmless, but a capacity of 8 (or a separate staged slot) would keep committed readers warm.
| @@ -190,11 +190,20 @@ impl SequentialStager { | |||
| } | |||
There was a problem hiding this comment.
Praise. The detach/reattach bracket here — and its twin in commit_transfer.rs:1240-1277 — fixes real pre-existing bugs, not just this PR's: sequential SPARQL staging could not translate the previous operation's subjects in the next operation's WHERE, and push-apply left the installed state's provider reading pre-mutation dictionaries. Both verified by reading apply_staged_flakes_for_sequential_staging and apply_pushed_commits_to_state. I also looked for a third path that mutates dict_novelty with a provider attached and found only apply_single_commit, whose sole caller (the catch-up loop) was already bracketed.
| reverse_graph, | ||
| )?) | ||
| let mut view = StagedLedger::new(ledger.clone(), flakes.to_vec(), reverse_graph)?; | ||
| crate::staged_dicts::attach_staged_dicts(&mut view)?; |
There was a problem hiding this comment.
Praise. The gating is exact: stage_with_shacl returns before the attach when the shape cache is empty, this call sits under has_post_state_conditions(), and the API path returns on no-config/no-shapes — so a transaction with neither shapes nor a post-state condition pays one relaxed atomic increment for its StagedLedger stamp and nothing else. And commit never sees the view-local ids: commit_txn detaches whatever provider is on the view's snapshot, extends the canonical dict_novelty (the attach only ever wrapped a clone), and rebuilds the provider from it. Dropping the three attach calls turns three of the four new tests red, so the transaction-side fix is load-bearing.
| /// | ||
| /// Uses a push-based API to avoid `Box<dyn Iterator>` allocations in hot path. | ||
| /// Process-wide counter behind [`next_overlay_content_version`]. Starts at 1 | ||
| /// so `0` can mean "empty since construction" for overlays that want it. |
There was a problem hiding this comment.
Praise, and a note for whoever wires the composites. A single AtomicU64 fetch_add(1, Relaxed) is exactly enough for process-unique stamps (an RMW on one location is totally ordered; wrap-around is never), Novelty::new reporting 0 for "empty since construction" is a legitimate shared stamp, and Novelty refreshes on every public mutator and never on a read, so committed-state cache entries are invalidated exactly when they should be. The trait doc's promise — None means "do not cache" — is what makes the composite-overlay item above a regression rather than a bug: the callers now honour it, so every overlay type has to vouch or lose the cache.
Problem
A transaction's SHACL and
f:postStatepolicy probes read aStagedLedger(committed state plus the transaction's own staged flakes). Once the ledger has a persisted index those probes run on the binary lane, which resolves every overlay flake through the persisted dictionary plusDictNovelty. Both are committed-state artefacts, so the subjects and strings the transaction is introducing resolve in neither.The effect on every publish that creates new IRIs, observed on a production ledger as ~230k warnings in 83 minutes and multi-second commits:
subject not found in persisted or novelty dict), logged a WARN, and merged them as raw flakes;content_version, so the range-provider translation cache never served it and every probe re-walked and re-translated the whole graph novelty — O(probes × novelty) per transaction.There was also a latent cache-key collision: a staged view reports the very overlay epoch and
to_tthe committed novelty reports right after its flakes commit, and the query engine's cross-query translation cache was keyed on the epoch, so a product built during staging could be served for the committed state.Change
fluree-db-transact/src/staged_dicts.rs:attach_staged_dictsclones the base dictionaries, extends them persisted-first with the staged flakes, and attaches aBinaryRangeProviderover them to the staged view. Runs before SHACL validation (stage_with_shacl,apply_shacl_policy_to_staged_view) and post-state policy evaluation only, so transactions that never read their own staged state pay nothing. Commit still rebuilds the provider from the ledger's canonical dictionaries, so the view-local ids never reach a committed state.QueryPolicyExecutor::with_post_state_snapshotpairs post-state conditions with that snapshot.StagedLedgerreports a process-uniquecontent_version, drawn from a new core allocator (next_overlay_content_version) thatNoveltynow shares.GlobalTranslationKeyis keyed on it instead of the epoch; overlays without a content version are no longer cached across queries.dict_noveltyin place with the provider still attached — sequential multi-operation SPARQL staging and commit-transfer apply — now detach and reattach it (detach_binary_provider/attach_binary_provider), so reads through the state resolve the subjects those operations introduce rather than the pre-mutation copies.UpdatePlan::plan: the "local t ahead of nameservice" branch logs at debug instead of WARN. It fires routinely under the raft commit worker (notifyreads the record before taking the ledger state lock, which the worker holds across the next chunk's publish and install) and the plan was already a no-op.Tests
New standalone bin
it_staged_view_dict(cargo test -p fluree-db-api --features shacl --test it_staged_view_dict) pins the translation outcome through a tracing probe over an indexed ledger for the SHACL pass, the post-state policy pass, and a post-commit whole-graph scan, plus a rejection control proving the staged view is still read. Each assertion was watched failing with the fix reverted.StagedLedgergains a unit test for content-version uniqueness.Sweep run locally: unit suites for novelty, ledger, transact, query; API groups
grp_policy,grp_transact,grp_misc,grp_query,grp_ledger;it_cached_handle_cow. Clippy clean under default andshaclfeatures.Notes
feat/stats-kernel(feat(stats): fluree-db-stats kernel and on-demand column profiling #1789) because that is the branch the work started from; the diff does not touch the stats crate.Follow-up: #1790 — a post-state ASK that joins on a string value the same transaction introduces is denied on the staged view both before and after this change; the new test deliberately joins on an indexed value.