Repository navigation
Conversation
Merges into independent targets overlap: with one merge parked after its table effects and before its manifest commit, a merge between two other branches runs to completion. Merges into one target stay serialized: the second waits for the first's target gate and then merges on top of it.
The merge's counterpart of parked_writer_blocks_schema_apply: a schema apply arriving while a merge is parked before its manifest commit must not answer until the merge publishes, since past the exclusive gate it refuses at once on the merge's source branch. The merge's result is intact afterwards.
Deleting a merge's target or source while the merge is parked before its manifest commit waits on that branch's gate; the merge publishes and the delete succeeds after it. Two merges from one source into two targets both land with the source's row; whether they may overlap is left unpinned.
Merge A fails after its table effects, before its manifest commit, while merge B into another target is parked mid-merge. A's target head and table pin stay where they were, B publishes on release, and the same handle retries A to completion, so the failure released A's gates.
Another process deletes a merge's target and creates a branch of the same name while the merge is parked after its table effects. The merge refuses with ReadSetChanged on the target's branch identifier, not in doubt, the recreated target's head does not move, and it holds only its own writer's rows.
A merge task aborted while parked at the authority-capture seam or after its table effects stops at its first await after the release. The target is untouched or merged whole, the same handle retries the merge and writes to the target, and cleanup succeeds afterwards.
ragnorc
changed the title
…s-issue-643 # Conflicts: # crates/omnigraph/tests/schema_apply.rs
A merge held its source's branch gate and every per-table queue on the source from preparation through publication, so writes, index builds and deletes on the source waited for the whole merge, and merges from one source into several targets ran one after another. Its input is the tagged source snapshot: the merge pins it, proves it was still current after tagging, and the final re-read still refuses a source whose incarnation or schema changed. Hold the source gate only through that capture, and take per-table queues for the target only. The target gate and queues stay through publication, keeping a target delete/recreate out from under the plan and serializing merges into one target.
ragnorc
changed the title
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: request changes for one correctness issue. The source-lock reduction is a useful design change, but the pure-insert path still depends on the live source version.
A merge used to keep its source branch locked for the whole operation. For example, a long merge from main into a feature branch could delay writes to main. This PR keeps that lock only while it captures, tags, and checks the source snapshot. The merge then uses that fixed snapshot. Later source writes belong to a later merge. The target stays locked through publication, so two merges into one target still serialize.
The intended contract is sound: one complete target publication from coherent, retained inputs. The problem is that one optimized path still checks whether the source table is current. A normal source insert during planning can therefore make the merge fail with ReadSetChanged, even though its tagged input remains valid. The added source-write test parks after this check and does not cover that window. See the inline finding.
Tradeoffs and liability:
- The change addresses an unnecessary input lock. It reuses existing tags and snapshot capture, branch gates, and publication checks. It adds no public API, persistent format, or second source of truth. All merge paths must now use the same captured-source rule. The remaining live-pin equality prevents that composition.
- The expected benefit is strongest when frequent writes share a source with long merges into independent targets. Same-target work remains serial. More overlapping planners can increase peak memory and I/O. HTTP admission bounds operation counts, not total RSS. These tests do not establish throughput or memory costs.
- Pinned Lance 11.0.0 tags name exact manifest versions. OmniGraph's collector traces their table pins. Branch retirement retains the physical tree and ancestry archive. Local cleanup also acquires the live target gate. These existing mechanisms support shorter source ownership without a new recovery protocol.
- The deployment boundary remains one mutation-capable writer process unless an external fence proves exclusivity. These process-local gates do not become distributed fencing. Azure keeps its admission wrapper and qualification boundary.
- The PR adds 708 lines and removes 14. Of the additions, 665 are tests. Production merge code grows by 18 net lines. The liability question is which obligations remain. Five changes of this kind should share one lock order and one snapshot identity rule, rather than add mutable-tip checks to each optimization.
After the correction, I expect this design to reduce runtime liability by using an immutable input instead of prolonged source ownership. As written, it adds a refusal that depends on the selected optimization and burdens callers with retries.
Optional contention limitation: a merge whose source sorts before its target holds the source gate while waiting for the target gate. A queued merge can therefore block source writes behind another long merge. This includes main as source. It is remaining contention, not a newly introduced data-integrity defect. Qualify the unconditional “do not wait” wording, and measure this arrival pattern before claiming source latency is independent of merge duration.
Validation at 0ab3af7cd91b4901282e1c5d4f7f13d3278fe4e6:
- Reviewed the complete diff in an isolated checkout. Read the required repository guides and relevant complete upstream documentation. Checked assumptions against OmniGraph and pinned Lance source.
- The nine original
issue_643tests passed locally: eight infailpoints, one inschema_apply. - Temporary extensions of the existing test owners reproduced both timing cases with production code unchanged. The pure-insert probe showed that it used the certified insertion path. Its concurrent source insert succeeded, then the merge returned
ReadSetChangedforbranch_merge_source_dataset:node:Person. - Removing only the live-pin equality made that same probe pass, including captured-row and next-merge assertions. This was a causal check, not a submitted fix. I restored all temporary edits.
- After restoration, all 21 existing
branch_merge_failpoint tests passed. These include captured-source cleanup, source advancement, branch recreation, publication failure, and retained input tags. The checkout is clean. - Documentation checks passed for 166 Markdown files. AGENTS links, formatting, changed-file spelling, and diff whitespace checks passed. Local builds emitted a compact-unwind linker warning.
- CI for this exact head passed, including workspace tests, RustFS S3, and Azurite. The separate DST workflow also passed. I did not rerun cloud tests or measure production throughput, latency, or RSS.
| return Err(error); | ||
| } | ||
| }; | ||
| drop(source_gate); |
There was a problem hiding this comment.
[P2] Allow the captured pure-insert source to advance
Releasing this gate lets a normal source write finish during planning. However, revalidate_proven_pure_insert_source still requires pin_version(live_entry) == proven.source_version. A new source pin therefore rejects an otherwise valid merge of the tagged snapshot. Hot append workloads can repeatedly hit this refusal.
I reproduced this by extending source_write_proceeds_during_merge_issue_643: leave the target unchanged, insert on the source, and park at BRANCH_MERGE_POST_CANDIDATE_VALIDATION. A probe counter recorded certified insertion history. A second source insert completed while parked. On release, the merge failed with ReadSetChanged on branch_merge_source_dataset:node:Person.
Removing only that live-pin equality made the same probe pass. The target received the captured rows, and the next merge received the later insert. I restored all temporary edits.
Use the pinned source for the insertion proof while retaining the schema and incarnation checks. Extend the existing test to cover this earlier window. Its current seam runs after final revalidation and misses the failure.
| let source_gate = queue.acquire_branch(source_branch.as_deref()).await; | ||
| ( | ||
| source_gate, | ||
| queue.acquire_branch(target_branch.as_deref()).await, |
There was a problem hiding this comment.
Optional: document the remaining wait on the source gate
When the source sorts before the target, this await retains the source gate while another merge owns the target. main always sorts first. A queued merge can therefore block source writes for the duration of another merge, before source capture begins.
I extended same_target_merges_serialize_issue_643 to insert on s2 while its merge waited for t. The insert timed out after two seconds while the first merge remained parked. The original test passed before this added assertion.
This is remaining contention, not a new data-integrity regression. Qualify the unconditional “do not wait” wording in the guide and release note. Include queued same-target arrivals in performance measurements. Any later locking change must preserve the global acquisition order.
1 participant
Refs #643.
The fix: a merge no longer holds its source
A merge held its source's branch gate and every per-table queue on the source from preparation through publication. So every write, index build and delete on the source waited for the whole merge. On a server, a merge that syncs a feature branch from
mainstalled every write tomainfor its duration (a probe: an insert on the source did not finish within 3 s while a merge out of it was parked). Merges from one source into several targets also ran one after another.The source gate dates from #343 (RFC-022), whose comment justified the gates only by the target ("prevents a target delete/recreate from reusing the branch name underneath a plan"). It was hidden while every write took the graph-wide schema key exclusively; #783 made that key shared and exposed it.
The merge does not need the source held. Its input is the tagged source snapshot:
So the merge now holds the source's branch gate only through capture, tagging and that proof, and takes per-table queues for the target only. The target gate and queues are unchanged: they stay through publication, keeping a target delete/recreate out from under the plan and serializing merges into one target.
Why not simply drop the source gate: the post-tag "still current" proof would then race concurrent source writes and refuse merges spuriously.
For review (@ the RFC-022 author): the final re-read refuses source delete/recreate (ABA) up to that point. After it, a source delete now proceeds while the merge publishes. Data stays safe, because the merge's tags protect the source snapshot until publication, after which the target's own pins do. The merged parent's lineage stays readable through the retired branch's archive.
merge_target_delete_waits_and_source_delete_does_not_issue_643exercises this with an edge only the source added, so the target adopts the source'sedge:Knowspin. The source is deleted mid-merge, cleanup runs after publication, and the target still reads the edge. If there is a reason the source must stay pinned through publication beyond this, it belongs in that comment.Tests (failpoints, plus one in
schema_apply.rs)Concurrency
independent_target_merges_overlap_issue_643: merge A parks after its table effects; a merge between two other branches completes within 10 s.same_target_merges_serialize_issue_643: a second merge into the same target waits, then merges on top.same_source_merges_overlap_issue_643(changed): with a merge fromsparked, a second merge fromsinto another target completes. Both land.source_write_proceeds_during_merge_issue_643(new): an insert on the source completes while a merge out of it is parked. The merge publishes the source as captured, so the concurrent insert reaches the target only with the next merge.Boundaries
parked_merge_blocks_schema_apply_issue_643: a parked merge holds its shared schema permit; an apply waits, then refuses on the non-main branch.merge_target_delete_waits_and_source_delete_does_not_issue_643(changed): deleting the target waits; deleting the source does not. The merge publishes, and the adopted edge survives cleanup.failed_merge_beside_an_independent_merge_issue_643: a merge failing before its manifest commit leaves its target unmoved while an independent merge is parked; the same handle retries it.merge_refuses_a_target_recreated_by_another_process_issue_643: another process deletes and recreates the target mid-merge. The merge refuses withReadSetChangedonbranch_identifier:t, and the recreated branch keeps only its own rows.cancelled_merge_leaves_the_target_whole_issue_643: aborting a parked merge's task (at authority capture and after the table effects) leaves the target untouched or merged whole; the handle keeps working and cleanup succeeds.Measurement of overlap, latency and RSS lives in #835.
Docs
docs/dev/writes.mdstates which locks a merge holds.changelog.d/merge-releases-its-source-after-capture.changed.md.Local checks, on
main97f293e8mergedAll pass:
failpoints(126),schema_apply(32),branching(48),merge_truth_table,merge_fast_forward(20),merge_cleanup,merge_cost,merge_projection_cache,changes(46),writes(47);detached_commit_matrix, default andOMNIGRAPH_MATRIX=full;-D warnings),cargo fmt --check,typosandcheck-docs.py.