[ragnorc]

Refs #643.

What & why

Every capture on a branch other than the handle's bound one (merge preparation, branch-source capture) goes through merge_authority_cache, a single-entry cache behind one tokio::sync::Mutex. On a miss, validated_cached_coordinator called open_coordinator_for_branch, a full __manifest open and scan of about 16 requests (#813's figure), while holding that mutex.

Since #783, merges on different branches share the schema gate and run together. They evict each other's entry and then reopen one at a time under this mutex. After #783 that was most of the serialization left between independent merges, and the field's comment still justified the mutex with "the schema serial queue already serializes merges … at capture time".

A miss now opens the coordinator with the mutex released, then re-locks and installs the entry. What does not change:

• the capture still selects and reads the entry under the returned guard;
• the cache keeps one entry, so there is no memory trade-off;
• the hit path's probe and refresh stay under the lock.

Two captures that miss on the same branch open it twice. The entry is a hint, and a non-bound publisher that finds a different branch's view in the slot already falls back to a fresh open.

Measured

N concurrent merges into N independent, diverged targets, on RustFS through toxiproxy at +30 ms per round trip, release builds. The value is the last completion over a single merge on a graph with the same branch count:

N	Before #783 (b14c22c5)	main (c6f24757)	This PR
2	1.99	1.23	1.13
4	3.98	1.86	1.23
8	7.88	2.89	1.27

With this PR, eight independent merges finish in 5.4 s against 4.2 s for one. The harness was a local ignored test, posted in full in #643. The measurement criterion of #643 would be best served by a checked-in concurrent-merges scenario in the bench harness, as a follow-up.

Local verification

• The failpoints (121), branching (48), merge_truth_table, merge_fast_forward (20), warm_read_cost (17), branch_control_cost, merge_cost (4), consistency (23), writes (47) and schema_apply (30) owners pass, with --features failpoints.
• The DST suite passes from crates/omnigraph-dst, with the cost golden unchanged.
• cargo clippy -p omnigraph-engine --all-targets --features failpoints -- -D warnings and cargo fmt --check are clean.

Release note: changelog.d/merges-overlap-across-independent-targets.performance.md (#833). The branch merges current main cleanly; on the merged tree failpoints, branching, merge_cost, merge_fast_forward, merge_truth_table, merge_cleanup, warm_read_cost, branch_control_cost and workspace clippy pass. The checked-in instrument for the numbers above is #835.

[ragnorc]

The checked-in instrument in #835 (--scenario concurrent-merges) reproduces this PR's effect. Same setup as above (RustFS, +30 ms per round trip), --rows 256 --dims 8 --delta-rows 50, both arms on c6f24757. Each value is the batch's last completion over one merge alone; a single merge takes about 4.9 s, and the controls run before and after the batch agree within 2%:

N	main	#825
2	1.20	1.14
4	1.76	1.22
8	2.60	1.28
16		1.40
32		1.68

Every run verified exact row counts on every branch, merged keys read back, and each merge's publication receipt against its target's head.

Memory is the cost of the overlap rather than of the cache: process peak RSS after the batch, over a 70 MB pre-batch peak, is 81 MB at N = 8 on main and 99, 113 and 128 MB at N = 8, 16 and 32 with this PR, because the merges now run at the same time. These merges carry 50 rows each. Whether concurrent merges need an admission limit is the open question on #643.

[ragnorc]

Recommendation: approve. I found no blocking correctness defect in 76fdd1dc5c64f253afa2e9e5785af691887624b1. This is a COMMENT review because the authenticated account is the PR author. The two inline comments are optional improvements.

What this PR does, in plain words

Several users or agents can merge separate branches through one server handle. Each merge first reads the branch's saved graph state. Previously, a cache miss held one shared lock during that storage read. Other merges waited, even when they used different branches. This PR releases the lock during the read, then takes it again to install and capture the result. It removes that unnecessary wait. It does not reduce the work inside each merge or change what a successful merge commits.

Contract and correctness

The useful workload is concurrent merges with disjoint source and target branches, especially when object storage has noticeable latency. Sharing either branch still serializes the merges. The existing branch gates enforce this scope. The deployment boundary remains one mutation-capable writer process unless external ownership provides equivalent protection.

I checked these safety conditions in the actual code:

• Each caller reads its selected coordinator under the returned cache guard. Another opener cannot replace the entry during that capture.
• A coordinator loads table state and lineage from the same pinned manifest. Branch identity surrounds the Lance checkout with checks. See coordinator construction and identity capture.
• Cache replacement can discard useful work. It cannot authorize a commit. The publisher selection checks the branch, lifetime, graph head and manifest version. A mismatch causes a fresh open.
• Every publication attempt reads fresh state and checks the expected graph head. One manifest publication still makes the graph change visible.

I checked Lance 11.0.0 at its packaged source commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its branch checkout returns a separate dataset with a pinned manifest. Its strict overwrite path disables conflict rebasing when retries are zero. OmniGraph still uses that publication boundary. I also read the relevant full Lance format, versioning, branch, storage, read/write and performance guides.

Tradeoffs and long-term liability

The change trades waiting for concurrent work. More opens can run together, use temporary memory, and compete for storage bandwidth. The retained cache still has one entry. That does not imply unchanged peak memory. Cache replacement can also cause another open before publication. Cache-hit probes and refreshes still hold the lock.

The server already limits admitted writes to 64 globally and 16 per actor by default. However, merge admission estimates only 256 input bytes. These limits do not budget the merge's working memory. Embedded callers must bound their own concurrent work. Larger histories and bursty workloads therefore still need resource measurements.

My assessment is lower coordination liability with a small increase in concurrency reasoning and resource pressure. The diff adds 15 net lines, but adds no public API, persistent state, queue, cache map, or new authority. It fixes the lock scope that causes the wait. Five similar changes should retain this ownership pattern and explicit resource limits. They should not create five separate coordination mechanisms.

Evidence and limits

Local checks used the exact head, Rust 1.97.1, --locked, and --features failpoints. All 212 tests passed across branching, failpoints, merge_cost, merge_fast_forward, merge_truth_table, and warm_read_cost. Two subprocess helper entries remain ignored as standalone tests. I made no source edits.

The exact-head CI run passed workspace, RustFS, Azurite, formatting and lint checks. The DST run and GQT run also passed. Those are CI results, not local cloud tests. I did not reproduce the reported latency or RSS measurements. The current tests support correctness, but do not directly prove overlap during a cold coordinator open. The inline test suggestion targets that gap.

[ragnorc]

Optional: narrow the overlap claim to merges whose source and target branch sets are disjoint. branch_merge_impl takes exclusive gates for both branches (exec/merge.rs:5324–5328). Thus source → target_a and source → target_b still serialize, despite different targets. Also, cache-hit probes and refreshes still hold the cache lock. Suggested wording: ‘A cache miss no longer holds the shared cache lock during a coordinator open. This permits overlap between merges with disjoint source and target branches.’

[ragnorc]

Recommendation: approve 3f4a7a05b0991584d01065d5fde290d40fdfb900. I found no new blocking correctness defect after the merges from main. This is a COMMENT review because the authenticated account owns the PR.

This PR lets independent branch merges spend less time waiting for one shared cache lock. A cache miss reads a branch's saved graph state. The code now releases the lock during that storage read, then takes it again to install and capture the result. The useful workload is several merges through one handle, with disjoint source and target branches and noticeable storage latency. The change does not reduce each merge's storage work.

The three merges since the previous review have no manual conflict resolutions. Against current main, the PR still changes only the cache helper and its comments, plus the release note. I traced the helper's current callers and the new history-cache and merge-input code brought in from main.

The design still preserves the required contract:

• Selection and capture use the returned mutex guard. Another opener can replace the cache only after that capture ends. Duplicate opens or a later replacement can waste work, but do not mix two coordinators in one capture. The same helper covers merge inputs, branch-control source capture, and non-bound authority capture.
• Branch opening checks native identity around checkout. Captured lineage copies the head and buffer from that coordinator. The shared history cache supplies immutable settled records; it does not supply the branch's current head.
• Merge admission still takes both branch gates. Preparation pins its inputs and checks their freshness. The publisher reloads authority and checks the expected head on every attempt. Cache reuse must match branch, lifetime, head, and manifest version; otherwise it opens a fresh coordinator. One manifest publication remains the visibility boundary.

This addresses the cause at the shared helper: storage I/O need not hold the lock that protects cache selection. It adds no public API, persistent state, queue, or second authority. I assess it as lower coordination liability, with more concurrent resource use to account for. Five similar changes should keep the existing ownership and publication rules rather than add separate coordination mechanisms.

The tradeoff is waiting versus concurrent work. The retained cache has one entry, but several in-flight opens can consume more temporary memory and storage bandwidth. A replacement can also cause a fresh open at publication. Hit probes and refreshes still hold the lock. The one-writer-process deployment boundary remains. Server admission limits operations; it does not prove a bound on total engine memory. Embedded callers must bound their own concurrency.

Local validation on the exact head passed 224 tests, with two subprocess helper entries ignored as standalone tests:

RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target \
RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
cargo test --locked -p omnigraph-engine --features failpoints \
  --test branching --test failpoints --test merge_cost \
  --test merge_fast_forward --test merge_truth_table \
  --test merge_projection_cache --test warm_read_cost -j3

Documentation checks passed for 218 Markdown files. Instruction links, formatting, and diff whitespace checks passed. No source or test edits remain. I read the relevant upstream guides and checked Lance 11.0.0 checkout, refs, and strict overwrite code. The crate archive matches Cargo.lock; those installed source files match the archive at packaged commit ab6b5bbe46009ed78746b444df8db59a8bc5d842.

Main CI, GQT, and DST passed. Workspace CI checked out a6e711a3ec9ded9aaed632682bff5b54c4ccc601; its tree matches the reviewed head exactly (aa8265216ff82839447cb16d44c7671197ea5d14). RustFS and Azurite success is CI evidence, not a local cloud test.

The earlier optional wording correction and controlled cold-open test remain applicable. I have not duplicated those comments. GQT rows and errors do not distinguish this scheduling change, and no existing seam pauses this coordinator open. The existing Rust merge owner is the right place for that mechanism check. The focused suite supports correctness, but it is not a before/after proof of cold-open overlap. I did not reproduce the latency or peak-memory measurements, and I do not claim a new performance result.

[ragnorc]

Recommendation: approve at f9c98827334017d91ebc4c02ddc35a1dff8d2fd2. No blocking defect found. This is a COMMENT review because the authenticated account is the PR author.

This PR removes a shared wait during merge preparation. A cache miss used to hold the handle's cache mutex while it opened another branch's coordinator from storage. The change releases that mutex during the open, then takes it again to install and capture the result. Independent cold captures can therefore make progress together. It keeps the existing single-entry cache and publication checks.

The update since the last review is a clean merge of main. There is no manual merge resolution. Against base 47dfe7b4a00c1d6405bc076dc6f4f549ae6fc06c, the PR still changes only the cache-miss path, its comments, and the release note.

Correctness and scope:

• The required invariant is one coherent capture of branch identity, head, snapshot, and lineage. The cache helper and merge caller still select and read the entry under the same returned guard. A competing open can replace the hint; it cannot replace the entry halfway through that capture. Later hits still probe freshness. I also traced branch creation, ordinary writes, read-target reuse, branch deletion, and unpublished-commit publication.
• Publication still uses the captured authority. Non-bound publication checks the cached branch, identity, head, and version before reuse. The catalog publisher checks branch identity and exact head against its own attempt. The cache is not commit authority.
• Correction to my earlier review: sharing a source no longer serializes the whole merge. Main now captures and pins the inputs, checks freshness, then releases the source gate. The target gate remains held through publication. The same-source overlap, source-write, and source-delete tests pass. My earlier request to limit overlap to wholly disjoint source/target pairs is superseded. Contending branch captures and cache-hit refreshes can still wait.

The change removes unnecessary coordination at the helper that owns it. It adds no API, persistent state, cache map, or migration. It does add an interleaving to maintain: an open that finishes later can displace a newer hint. Existing freshness and publication checks make that a reuse/cost issue rather than authority. This is a small reduction in long-term liability. Five more callers can use the same helper without five more locks or invalidation protocols.

The tradeoff is concurrent work: simultaneous misses can duplicate opens and increase temporary coordinator/scan memory and storage requests. Only retained cache cardinality stays at one. The benefit should be largest for concurrent cold captures with material storage latency; a warm single-branch workload should gain little. I did not reproduce the PR's timing or peak-memory figures.

I checked the other open PRs. The closest interactions are #962's freshness probe and #818's immutable table-handle cache. Neither requires this hint to become authority. The schema, diff, branch-v2, and group-commit proposals (#963, #964, #677, #785) likewise give no reason to add another cache protocol here. This is design alignment, not a combined-branch test. Current process-local branch-control and deployment boundaries remain in force.

I read the current repository guides and relevant full upstream storage, branch/tag, versioning, and cache documentation. I checked the installed, pinned Lance 11.0.0 source (ab6b5bbe46009ed78746b444df8db59a8bc5d842): checkout creates a branch-qualified manifest view, and zero-retry Overwrite skips automatic rebase. Lance 13's release notes include cheaper latest-manifest discovery. That may change the size of this benefit; it does not remove the lock dependency. I did not qualify Lance 13 with OmniGraph.

Local validation used a clean, isolated checkout of the exact head and compatible cached build outputs. The commands used these environment settings:

export RUSTUP_TOOLCHAIN=1.97.1
export CARGO_TARGET_DIR=/tmp/review-928/target
export RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'
export RUST_MIN_STACK=16777216
export RUSTC_WRAPPER=
cargo test --locked -p omnigraph-engine --features failpoints --test integration -- merge_projection_cache:: merge_fast_forward:: merge_truth_table:: merge_cleanup:: merge_cost:: branching::
cargo test --locked -p omnigraph-engine --features failpoints --test failpoints -- merge

Results: 87 passed, 2 ignored in the integration selection; 33 passed, 0 ignored in the merge failpoint selection. The two ignored cases are heavy wide-row hydration repros. git diff --check 47dfe7b4a00c1d6405bc076dc6f4f549ae6fc06c HEAD passed. No temporary source or test edits remain.

Separately, CI, GQT, and DST passed. Their workspace, ordinary-GQT, and pinned-DST checkout logs identify merge commit 1eac0a472dcf94f7b5b4d47ce2dd44146adcd766. Its tree equals the reviewed head's tree: 50144c77580ab36dab15038a9da1d5c88b056ca8. Cloud suites were CI evidence, not local runs.

Optional coverage remains as previously noted: pause one cold coordinator open and prove another independent capture completes before releasing it. Existing overlap tests pause later, so they do not isolate this cache-miss change. GQT cannot express this internal scheduling boundary; the existing Rust concurrency owner is the right place. No before/after regression proof or measured speedup is claimed for base 47dfe7b4a00c1d6405bc076dc6f4f549ae6fc06c versus head f9c98827334017d91ebc4c02ddc35a1dff8d2fd2. This update adds no query-result bug fix requiring a new GQT case. I have not repeated the existing optional inline comment.