perf(transact): take the cached ledger state for the commit window — stop per-commit DictNovelty deep-clones for serial writers - #1718
Conversation
Every commit calls Arc::make_mut on the ledger's subject and string dictionaries, which extends them in place only while the commit uniquely owns them. Any other live holder turns each call into a deep clone costing O(dictionary entries accumulated since the last index) -- invisible in a short run, quadratic in a long one. The commit path already publishes its ownership count on the fluree::cow_probe target. This harness captures it alongside per-commit latency and an Arc::strong_count census taken from outside the commit, so the copy can be priced rather than merely detected. Three modes: `cached` commits through a cached LedgerHandle, the shape every server transact route uses; `owned` threads a LedgerState through stage_owned and never involves the ledger cache, as a control; `clonebench` grows the dictionaries and times an explicit deep clone of exactly what make_mut copies. Set FLUREE_STORAGE_FSYNC=off for the timed modes -- with fsync on, per-commit disk latency buries the copy.
A commit through a cached LedgerHandle stages against a CLONE of the cached
LedgerState -- clone_state() on the lock-held path, snapshot() plus
to_ledger_state() on the optimistic path -- so for the whole commit the cache
co-holds an Arc clone of everything the commit is about to Arc::make_mut. Two
holders were structural:
1. The cached LedgerState itself. Present with or without an index, so
dict_novelty's strong_count was 2 from the very first commit.
2. Once an index is installed, the BinaryRangeProvider attached to the cached
snapshot, which holds its own clones of both dictionaries: strong_count 3.
The existing detach in finalize_state_with_base cannot release that one --
base.snapshot is an Arc shared with the cached state, so
Arc::make_mut(&mut snapshot).range_provider = None deep-copies the snapshot
and nulls the private copy while the cache's snapshot keeps its provider.
So every cached-handle commit deep-cloned the dictionaries instead of extending
them: roughly 12ns per accumulated entry -- 1.45ms to clone 124k of them, one heap allocation per subject in
NsVecBiDict.reverse), O(entries since the last index) per commit, and O(commits
squared) across a window where indexing lags. Novelty itself was never the
problem -- its segments are Arc-shared, so cloning it copies pointers.
commit_and_finalize now empties the cache slot for the commit window: it takes
the cached state out from under the already-held write guard, leaving a genesis
placeholder, and drops it. The commit's base is then uniquely owned, which also
makes the existing detach effective at last, since base.snapshot is finally
unique.
Dropping rather than parking is the point -- keeping the old state anywhere
holds the refcount above 1 and changes nothing -- so the failure paths reload
from durable storage instead of restoring. A failed commit reloads under the
still-held guard, so the placeholder is never observable; eviction is the
fallback for when storage itself is unreachable. Reload is the primary repair
because callers already holding the handle keep using it, and dropping the
manager entry would leave them reading the placeholder, which would in
particular break the optimistic path's reconcile-and-retry loop. An unwind
releases the lock and spawns a reload out of band.
finalize_commit is split into install_committed_state plus
trigger_reindex_if_needed so the slot can stay held across the install and
repair the cache if the install fails. Its five other callers keep identical
behaviour, including releasing the lock before the reindex trigger.
Measured with examples/cow_probe_repro.rs, release, FLUREE_STORAGE_FSYNC=off.
1500 commits of 20 nodes with indexing off: strong_count 2 -> 1 on every
commit, and the per-commit median goes from 0.79ms rising to 2.46ms, to a flat
0.72ms; total 2.41s -> 1.12s, against 1.02s for the never-cached control path.
1200 commits of 50 nodes with background indexing on: strong_count
{3: 1184, 2: 15} -> 1 on all 1200, total 3.62s -> 1.64s. The owned path is
unchanged at 1.02s -> 0.98s.
Both paths in commit_with_handle are covered, optimistic and lock-held. Five
other cached-handle commit flows still clone from the cache the same way and
still deep-clone every commit: merge, rebase, revert, cypher_txn, and the Raft
commit worker. Each needs the same detach at the top of its own flow, since the
detach must precede the commit while finalize_commit runs after.
Tests. it_cached_handle_cow.rs gets its own binary (it installs a process-global
tracing subscriber) and asserts strong_count == 1 for both dictionaries across
five regimes: before any index, the first commit after an index install,
post-index steady state, and the lock-held SPARQL UPDATE path. It hard-asserts
that the index really reached the cached handle and that the post-install commit
saw a provider attached, so those phases cannot silently degrade into
re-testing the pre-index case. it_cached_handle_cow_recovery.rs pins the failure
path: a commit rejected inside the commit path leaves the handle at the
committed head, still serving data, with the next commit succeeding. Its trigger
is a novelty ceiling one byte above current novelty, which passes staging's
"already at the ceiling" check and fails the commit path's "this delta would
cross it" check; the obvious cheaper trigger is rejected during staging, before
the slot is ever emptied, and made the test vacuous. Both tests were confirmed
to fail against a deliberately broken fix.
The commit window is cancellable, not just panickable, and cancellation is where the genesis placeholder escapes the lock. Found in internal review of #1718. LocalCommitter::transact awaits the commit inline inside the HTTP request future, and axum drops handler futures when a client disconnects; an embedded caller wrapping execute() in tokio::time::timeout gets there too. A future-drop between the detach and the refill runs DetachedCacheSlot's Drop, which releases the write lock FIRST and only then spawns the reload -- so every reader parked behind the commit, the normal condition under load, acquires the instant the lock releases and clones the placeholder: silently empty results at t = 0 for the duration of a full reload (nameservice lookup, commit replay, index attach). The pre-#1718 code under the same cancellation left the cache stale at worst, never empty, so this was a regression on a reachable path in the silent-wrong-results class. The Drop comment claiming "only reachable on an unwind" was simply wrong. The commit tail now runs on its own task (commit_shielded) whose JoinHandle the caller awaits. A cancelled caller abandons the WAIT, never the commit: the slot machinery always runs to completion and the cache is always refilled. Fluree is Clone and Arc-backed and the write guard is an OwnedRwLockWriteGuard, so the whole span moves into the task; IndexConfig is two integers to clone and everything else was already owned. Task-spawn overhead measures ~0.03ms per commit against a commit's storage writes and nameservice publish, and the per-commit cost stays flat in accumulated novelty: 1500 commits x 20 nodes, indexing off, is 1.15s shielded vs 1.12s unshielded vs 2.41s before #1718. Drop's spawned reload is now a true last resort for panics rather than a routine client-disconnect path. recover() also disarmed the drop guard before its own longest await, the fluree.ledger() reload, so a cancellation mid-recovery left the lock released, the placeholder cached, and NO repair scheduled -- and single-node has no head watermark, so get_or_load would keep serving that placeholder until some unrelated write happened to reconcile it. Reads alone never healed it. The disarm now happens after the match, in both arms; Drop already tolerates guard == None. The shield subsumes this, but recover is now correct in isolation. Tests: it_cached_handle_cow_cancel.rs parks a reader behind an in-flight commit, aborts the caller, and asserts the reader never observes t = 0, that the commit still lands, and that the handle keeps reading and writing afterwards. Waiting on the write lock from another task is a precise window signal: the commit path goes from taking the lock to emptying the slot with no await in between, so the first moment another task can run, the slot is already empty. Mutation-verified -- against an unshielded build (commit_shielded awaited inline) it fails with exactly the reviewed symptom, a parked reader reading t = 0. Also documents two consciously accepted residuals that read as missed otherwise: recover()'s storage-unreachable branch leaves handle-holders on the placeholder until they re-fetch (a double failure, with no good state left to install), and the ephemeral-handle Drop has no cache entry to reload into. And corrects Fluree::ledger's docstring, which claimed to use the connection-wide cache; it is a direct load, which is precisely why recover() can call it under the held state write lock without deadlocking. That docstring is now load-bearing, so it says so.
A bare tokio::spawn orphans the commit tail's log events from the request's transact span, degrading request-id linkage for commit-side logs. Instrument the spawned future with the caller's current span so parentage survives the shield. Found by internal review of the cancellation-shield commit.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
Approving. The concurrency reasoning holds up and the tests genuinely pin it — I ran two mutants rather than reading the writeup:
- Shield removed (awaited
commit_shieldedinline instead of spawning) →cancelled_commit_never_exposes_the_empty_cache_slotFAILS with "a reader parked behind a cancelled commit observed the empty cache slot at t = 0". - Detach disabled (
mem::replacemade a no-op) →cached_handle_commits_uniquely_own_the_dictionariesFAILS.
Both fail for exactly the reason they exist. The cancellation test is the strongest thing here — that's a bug that would otherwise only ever show up as intermittent empty reads under client disconnects.
I also checked the two claims the whole design rests on, independently:
- "Every state reader routes through that one lock" — holds. Every read path goes through
inner.state.read().awaitandlock_for_write()takes the same RwLock, so readers park rather than see the placeholder.config_cacheis the one thing read without the state lock, but its key isNovelty::config_write_t, which needs the state lock to obtain — unreachable during the window, and a poisoned key-0 entry would self-heal on the next miss anyway. - "A t=0 placeholder always loses the monotonic-swap guard" — holds, and slightly better than claimed.
ledger_manager.rs:1560isnew_state.t() >= write_guard.state().t(). The>=rather than>means even a reload of a genuinely-empty ledger at t=0 lands, so there's no first-commit-panic edge case.
grp_transact 190/190, the three cow tests green, CI green.
Three inline notes below to look at before merging — none of them structural.
Two on the writeup rather than the code:
The title claims more than ships. "stop per-commit DictNovelty deep-clones" is true for serial writers only; your own sweep says 2 submitters leaves 94% of commits still cloning, 4 leaves 98%. The body is scrupulous about this, but the title is what lands in the changelog and the merge commit. Worth scoping it — "for serial writers", or similar.
No Follow-up: #N for any of the deferrals. The body is Closes #1707 only, but this defers the five sibling commit flows (merge, rebase, revert, cypher_txn, Raft commit_worker — with cypher and the Raft worker carrying real write volume by your own note), the concurrent-writer case, and the in-place-commit design conversation. Per CLAUDE.md that's exactly what the marker exists for, so a deferral isn't indistinguishable from a fix. Right now this PR body is the only record of all of it.
Minor, on the numbers: the 2.1x is measured with fsync off, which you disclose and which is the right call for isolating the window — but it means the ratio shouldn't be quoted as a production figure, since the absolute ~1.7 ms/commit saving at the tail stays constant while the denominator gets much larger with fsync on. Also unexplained: after-fix still sits ~0.09-0.15 ms/commit above the never-cached control, of which the shield accounts for 0.02-0.03. It's flat across bands so it isn't the quadratic returning, but it's a third of the remaining gap to the floor.
| /// it does not read, populate, or lock the connection-wide ledger cache. | ||
| /// The ledger state combines the indexed database with any uncommitted novelty transactions. | ||
| /// | ||
| /// Keep it manager-free: the cached-handle commit path calls this from |
There was a problem hiding this comment.
This property is now load-bearing rather than incidental: DetachedCacheSlot::recover calls this while holding the ledger's state write lock, so routing it through LedgerManager later would deadlock the transact path — and the failure mode is a hang, not a red test.
The corrected doc is good (the old one said the opposite, which is its own small alarm). But a comment is thin protection for an invariant whose violation hangs the write path. Worth either a debug_assert, or a name that carries the constraint — load_ledger_uncached or similar — so someone who doesn't read the comment still can't get it wrong.
There was a problem hiding this comment.
Agreed the comment was thin protection for a hangs-not-fails invariant. Renaming the public method would break embedders, so the shape that landed (35fd8cc3d): the direct-load body moved to pub(crate) fn load_ledger_uncached() — the constraint now lives in the name — with ledger() as a thin public wrapper, and recover() calls the crate-private name. So the caller that depends on the invariant can't reach the wrong thing without reading past a name that says exactly what it guarantees, and the doc carries the deadlock rationale ("recover() calls this under a ledger's state write lock; routing through LedgerManager would deadlock").
While verifying the surrounding safety story we also tightened one corner of it: the "poisoned key-0 entry self-heals on the next miss" reasoning doesn't hold on the panic path (reload never touched config_cache, and a config-quiet reloaded state legitimately carries marker 0 — so the entry would hit, not miss). config_cache_clear() now runs at reload's swap point (f2b226e45). It takes panic + racing reader + config predating index_t to matter, but fixed beats argued.
| %error, | ||
| "could not reload the ledger cache after a failed commit; evicting" | ||
| ); | ||
| drop(guard); |
There was a problem hiding this comment.
Small gap between the documented residual and the code: the comment above covers callers already holding this handle, but between this drop(guard) and the disconnect below, a new lookup through the manager gets the cached entry with the placeholder still in it.
Double-failure path and a tiny window, so I wouldn't restructure for it — just widen the comment so the next reader doesn't have to re-derive that the two cases differ.
There was a problem hiding this comment.
Widened as suggested (801fbf8b2 carries it): the comment now covers the new-lookup-through-the-manager window between drop(guard) and disconnect as its own case, distinct from callers already holding the handle — and the struct-level "never observable" claim is tempered accordingly for the eviction repairs. Agreed it wasn't worth restructuring for; the double-failure framing stands.
| /// lands — handing the genesis placeholder to every reader parked behind | ||
| /// the commit, i.e. silently empty results at `t = 0` until the reload | ||
| /// completes. Spawning means a cancelled caller abandons the *wait* for the | ||
| /// commit, never the commit itself: the slot always runs to completion and |
There was a problem hiding this comment.
This is the right trade, but it's a real change in externally-visible behaviour that lives only in a code comment: a client that disconnects mid-transact now still has its data written, where before the outcome was undefined.
That's the kind of thing an operator discovers from a support ticket rather than from source. Worth a line wherever transact durability is documented.
There was a problem hiding this comment.
Right — that line deserved better than a code comment. docs/transactions/overview.md's durability section now states it plainly (d19c40344): a client that disconnects mid-transact still has its data committed (previously the outcome was undefined), and timed-out clients should re-query or retry idempotently rather than assume the write was lost. That's the operator-facing sentence the support ticket would have needed.
…hed's name Fluree::ledger()'s direct-from-storage property became load-bearing when DetachedCacheSlot::recover started calling it under the ledger's state write lock: routing it through the LedgerManager would deadlock the transact path, and the failure mode is a hang, not a red test. A doc comment is thin protection for that, and renaming the public method would break embedders — so the body moves to a crate-private load_ledger_uncached() whose name and doc state the constraint, and ledger() stays as a thin public wrapper.
The config cache's marker (Novelty::config_write_t) only protects against staleness it can see advance. A reader racing the unwind gap of a detached commit — the lock is released with the genesis placeholder still cached until the out-of-band reload lands — can resolve config against that empty placeholder and cache the result under marker 0. A reloaded state whose config graph was last written at or below index_t legitimately carries marker 0 too (novelty is rebuilt from post-index commits only), so the poisoned entry would HIT, silently serving "no config" for a configured ledger until the next config write: there is no miss to self-heal on. Clearing the entry at the reload swap point closes the persistence. What remains needs a panic plus two racing windows: a resolve that started against the placeholder but whose cache put lands after the swap. The swap holds state -> config_cache briefly, so the lock-order note now records that one-way order as the only permitted one.
merge and revert stage against a clone_state() of the locked cache and
then run the same finalize_state_with_base dictionary make_muts as a
transact commit, so the cache's co-held Arcs deep-cloned the
dictionaries once per operation, O(entries since the last index). A
shared apply_staged_detached helper now applies the DetachedCacheSlot
take -> apply -> install -> refill pattern under each flow's existing
write guard (held continuously from the build's lock_or_load through
the apply — verified per flow), spawned-and-awaited like
commit_shielded because both flows run inside HTTP request futures
(LocalCommitter::{merge,revert}) that axum drops on client disconnect.
Failure repair is a new DetachedCacheSlot::evict, not recover: both
callers roll the nameservice head back (reset_head) AFTER the apply
returns Err, so recover's reload would cache the head the rollback is
about to withdraw. Eviction leaves the next access loading whatever
head the rollback settles on; the residuals match recover's
storage-down fallback and are documented in place.
The other two sibling flows are commented, not converted, because their
structure does not have the cache as a commit-window co-holder:
- rebase replays from a direct storage load (self.ledger(&source_id)),
never a cache clone; its final install performs no dictionary work.
- cypher interactive transactions run their make_muts per statement at
stage time, outside any lock, while the cache must stay readable for
the transaction's lifetime; the publish window does no dictionary
work. Its remaining per-statement clone (base shared with txn.state
itself, plus the cache for statement one) needs owned state threading
through the stage path, not a commit-window detach.
New own-binary test mirrors it_cached_handle_cow: a general merge and a
single-commit revert must both observe strong_count == 1 on
dict_novelty and runtime_small_dicts at commit, and the cached handle
must serve the committed head afterwards. Mutation-verified: with the
detach disabled the merge probe reports a second holder and the test
fails; restored, it passes.
The cancellation shield made this externally visible behavior definite: a client that drops the connection mid-transact now still has its data committed (previously the outcome was undefined). That semantic lived only in a code comment; operators discover it from support tickets. Record it where transaction durability is documented, with the practical consequence: a timed-out client should re-query or retry idempotently rather than assume the write was lost.
…tic path Attributes and removes the previously-unexplained ~0.09-0.15 ms/commit that after-fix cached mode sat above the never-cached control. The optimistic path stages from ledger.snapshot() + to_ledger_state(), and LedgerView had no head_temporal field — to_ledger_state() hardcoded None — so every optimistic commit re-fired ensure_head_temporal: a store.get of the head commit blob plus a full commit decode, inside the locked (and now detached) commit window. HeadTemporal exists precisely so the event-time monotonicity guard and sticky dual-stamp decision are in-memory integer checks; the view round-trip silently defeated that on the hottest transact path, while the owned/threaded path and the lock-held clone_state() path both carried the value forward. The field is Copy; from_state copies it and to_ledger_state carries it, so views stay coherent with the head they were taken from and the lazy resolve now fires only for states that never observed it (e.g. index == head at load, where novelty replay saw no commit). Measured (cow_probe_repro, fsync off, paired base-vs-fixed binaries interleaved on a loaded box): NODES=20 delta ~0.08 ms/commit; NODES=200 delta ~0.55 ms/commit in the uncontaminated pair — scaling with head commit size, the signature of the blob read + decode. After the carry, cached mode sits ~0.01-0.03 ms/commit above the owned control, i.e. approximately the spawn shield alone. The owned path is unchanged (control pair identical). Also spares the cypher interactive path the same read at its first statement.
aaj3f
changed the title
|
All of it is in, and your review directly produced two findings beyond its own asks — the second one closes a question you raised. Title rescoped to "for serial writers" as you suggested, and the deferrals now have their markers: Follow-up: #1742 (Raft worker), #1743 (the in-place-commit design conversation, which your concurrent-writer point is the strongest argument for), plus #1745 (a pre-existing config-cache poisoning found during this pass). On the fold-in front we went further than the markers: merge and revert now take the detach too (spawn-shielded, with an evict-not-reload failure repair because both flows roll the head back after an apply error — reload-first would cache the head the rollback is about to withdraw; Your "unexplained ~0.09–0.15 ms above the never-cached control" — that was the thread worth pulling, and it wasn't partial: Head is |
2 participants
Closes #1707.
The cached-handle transact path — what every server transact route uses — could never satisfy the commit path's unique-ownership precondition: the LedgerManager's cached
LedgerStatewas always a second holder of the dict Arcs (and the snapshot's range provider a third once an index attached), soArc::make_mutdeep-clonedDictNoveltyon every commit, O(entries-since-last-index) per commit and O(commits²) across an unindexed window. #1707 has the full mechanism; the fix follows the direction verified there.What this does.
commit_and_finalizenow empties the cache slot for the commit window via a privateDetachedCacheSlot: it takes the cached state out from under the already-held write guard (leaving a cheap placeholder), which makes the commit's base uniquely owned — and, as a bonus, makes the pre-existing range-provider detach incommit.rseffective for the first time, becausebase.snapshotis finally unique.finalize_commitis split intoinstall_committed_state+trigger_reindex_if_neededso the slot stays held across the install and can repair the cache if the install fails; its five other callers keep byte-identical behavior, including releasing the lock before the reindex trigger.The placeholder is unobservable — including under cancellation. The guard is held with zero release points across detach → install → refill, and every state reader routes through that one lock, so no reader can see the placeholder on any normal path. The subtler case is future cancellation: the commit runs inside the HTTP request future, and axum drops handler futures on client disconnect — an unshielded version of this change would have let a mid-window cancel release the lock and briefly serve the empty placeholder to parked readers (internal review caught exactly this, with a reproducing test). So the entire detach→refill span — including both failure-repair paths — runs as its own spawned task (
commit_shielded) whoseJoinHandlethe caller awaits: a cancelled request abandons the wait, never the commit, and the cache is always refilled. The spawned future carries the caller's tracing span, so request-id linkage on commit-side logs survives the shield. Cost of the shield: ~0.02–0.03 ms/commit (one spawn + a handful of Arc bumps), re-measured below.Error and unwind paths. On commit failure the primary repair is reload-in-place under the still-held guard (reflecting the durable head whether the commit died before or after publishing), with
disconnectas the fallback when storage itself is unreachable — plain eviction would be wrong as the primary, since callers already holding theLedgerHandlewould keep reading the placeholder. A drop guard remains as the genuine last resort for panics: releases the lock, logs, and spawns an out-of-band reload (a t=0 placeholder always loses the manager's monotonic-swap guard, so the repair reliably lands). The two consciously-accepted double-failure residuals (storage down during repair; ephemeral handles with caching disabled) are documented in place.Both write paths are covered —
commit_and_finalizeis shared by the optimistic and lock-held (policy/SPARQL/pre-built) paths, and the ownership test includes a SPARQL-UPDATE phase that goes throughclone_state()to prove the second one.Numbers (release build, fsync off to open the window; Claude ran these and I'm pasting as-is). 1,500 commits × 20 nodes, indexing off — median ms/commit by band:
The growth is gone, not reduced — per-commit cost is flat in accumulated novelty, matching the never-cached path (~2.1× at 1,500 commits and still widening with window size before the fix). With background indexing on (1,200 × 50): ownership probe went
{3: 1184, 2: 15, 1: 1}→{1: 1200}, total 3.62 s → 1.88 s. The owned/threaded control path is unregressed.Tests. Three new tests, each verified to fail against a deliberately broken build before being accepted: an ownership harness asserting
dict_novelty/runtime_small_dictsstrong-count == 1 at commit time on the cached path (pre-index, post-install, immediately-after-install — the guard that didn't exist, which is how 34592f3 quietly covered only the threaded path); a recovery test asserting a failed commit leaves the handle serving the committed head (its ceiling is set one byte above current novelty so the failure fires in the commit path — the obvious 1-byte-ceiling version was vacuous, rejected at staging); and a cancellation test that parks a reader behind a mid-window abort and asserts it never observes t=0 and the commit still lands (deterministic: there is no await between lock acquisition and the detach, sois_locked()observed from outside implies the window is already open — a property worth knowing before anyone adds an await in that gap).Gates.
cargo test -p fluree-db-api3,262 passed / 0 failed;-p fluree-db-consensus53/53 (afinalize_commitcaller); clippy-D warningsclean; fmt clean. One honest caveat: workspace-widecargo check --all-targetswas run with--exclude fluree-search-httpd --exclude fluree-search-serviceon my box — those two hit a local C++ toolchain issue (usearch/cxx cannot compile here at all, unrelated to this change); CI's unrestricted run is the authoritative check for them.Scope limitation, measured — concurrent writers to the same ledger still see the clone. Removing the cache's holder makes the base uniquely owned when one transaction is in flight. It does not when several are: the optimistic path takes its base via
ledger.snapshot()+to_ledger_state()before acquiring the write guard, so every concurrently-staging transaction holds its own Arc clones of the same dictionaries, andmake_mutcopies again. A controlled sweep on this branch (identical workload, only the submitter count varying) shows it cleanly — 1 submitter: 0 ownership violations in 3,060 commits; 2 submitters: 3,126 of 3,329 (94%); 4 submitters: 3,640 of 3,720 (98%). So the honest claim for this PR is: the per-commit clone is eliminated for serial writers to a ledger, and reduced-but-not-eliminated under concurrent ones. That is still worth having (the serial case is the whole single-node ingest story, and the O(commits²) blowup it removes is real), but nobody should read the benchmark above as "the clone is gone under load." The general answer is the same one the reader case points at — committing into the cached state in place, asapply_single_commitalready does — and it wants its own design discussion rather than being smuggled into this fix.Deliberately not in this PR (each needs the same detach at the top of its own flow, and the failure-path story above should settle once before being replicated): five sibling cached-handle commit flows still clone from the cache —
merge.rs,rebase.rs,revert.rs,cypher_txn.rs, and the Raftcommit_worker— with cypher and the Raft worker carrying real write volume. Also unchanged by design: a concurrent reader holding aLedgerViewacross the commit window still pins the Arcs for that window — the durable answer there is the in-place commit shapeapply_single_commitalready uses, which is a design conversation, not a bugfix. If we'd rather fold any of the five in here, happy to talk through the ordering.Post-review addendum (commits
35fd8cc3d…afcd3cb51, headafcd3cb51):Scope grew where the maintainers' fold-it-in preference said it should, and shrank where verification said the premise was wrong. Of the five deferred sibling flows: merge and revert are now folded in (same detach via a shared
apply_staged_detached, spawn-shielded because both run inside request futures — with one deliberate difference: their failure repair is evict, not reload, because both flows roll the nameservice head back after an apply error, and reload-first would cache the very head the rollback is about to withdraw; pinned byit_branch_op_cow, mutation-verified). Rebase turned out not to need it — its replay base is a direct storage load, never a cache clone; a detach there would be pure risk for zero benefit (comment added saying so). Cypher's clone has a different mechanism entirely — per-statementmake_muts at stage time against state shared with the transaction itself, outside any lock — so a commit-window detach saves nothing; it belongs with the Raft worker's residual (Follow-up: #1742), and relatedly its private-state policy resolution can poison the shared config cache (filed separately as #1745).The reviewer's "unexplained third of the gap to the floor" is answered, and it was the whole gap:
LedgerView::to_ledger_state()hardcodedhead_temporal: None, so every optimistic-path commit re-read and re-decoded the head commit blob from storage inside the locked window (ensure_head_temporal— whose own doc says its purpose is "no per-commit storage reads"). Fixed by carrying theCopyfield through (afcd3cb51). Verified by paired interleaved binaries with an owned-mode negative control, and the delta scales linearly with head-commit size (~0.08 ms/commit at 20-node commits → ~0.55 at 200-node) — the signature of a per-commit blob read, not lock or spawn overhead. Post-fix, cached mode sits ~0.01–0.03 ms/commit above the never-cached control — approximately the spawn shield alone. (Measured on a loaded box: direction and scaling are solid; absolutes are approximate.)One corner of the review's own safety argument tightened: "a poisoned key-0 config entry would self-heal on the next miss" doesn't hold on the panic path — the out-of-band reload never touched
config_cache, and a config-quiet reloaded state legitimately carries marker 0 too, so the poisoned entry would hit. Nowconfig_cache_clear()at reload's swap point (f2b226e45); needs panic + racing reader + config predatingindex_tto matter, but it's fixed rather than argued.Also per review:
ledger()'s manager-free invariant now lives in a name at the call site that matters (pub(crate) load_ledger_uncached, withledger()as the thin public wrapper — no public API break); the recover-Err comment covers the new-lookup-through-the-manager window as a distinct case; anddocs/transactions/overview.mdnow documents the externally-visible change plainly: a client that disconnects mid-transact still has its data committed.Follow-up: #1742 (Raft commit worker + the cypher stage-time mechanism), #1743 (in-place commit design — the durable end of this class), #1745 (config-cache poisoning, pre-existing, found during this pass).