Finding
Discovered while fixing the concurrent-merge lost update (#1094, PR #1097).
The chunk-store graph lock (CS_GRAPH_LOCK / chunkStoreLockAndRefresh / chunkStoreUnlock, with cs->lockDepth reentrancy) is acquired and released by the btree / SQL layer per write statement/transaction. A higher-level VC operation therefore cannot hold the lock across its own sub-operations — the btree releases it at statement/transaction boundaries.
Evidence
While fixing #1094, the VC write CAS now reloads persisted refs at the outermost lock (chunkStoreForceRefresh runs only when cs->lockDepth == 1). To make interactive rebase keep its in-flight in-memory state, I tried wrapping the whole rebase op in a single outer lock at the dispatcher (doltliteRebaseFunc): acquire chunkStoreLockAndRefresh + force-refresh once at the top, expecting inner sub-ops to run reentrant (lockDepth > 1) and skip their own reload.
It didn't stick. Instrumenting chunkStoreForceRefresh to print cs->lockDepth showed every call — including rebase's internal sub-ops — ran at lockDepth == 1. The dispatcher's hold was gone by the time the sub-ops ran, because the intervening btree/SQL work (doltliteEnsureWriteTxnAndSavepoints, sqlite3_exec of the plan table, etc.) cycles the graph lock through its own per-statement acquire/release and doesn't preserve a VC-level outer hold.
Implication
After #1097, VC write ops are non-clobbering — each reads fresh persisted refs at its CAS, so a fast-forward/branch-delete can't overwrite a peer's advance. But multi-step VC ops (dolt_rebase('-i') / ('--continue'), and conceptually any op that spans multiple statements/sub-ops) are not atomic against concurrent peers: a peer commit can interleave between sub-operations. There is no "this VC op sees one consistent snapshot for its entire duration" guarantee for multi-step ops.
#1097 makes rebase correct anyway by persisting its intermediate state (the working branch) durably as it goes, rather than relying on holding a snapshot. So this is not a correctness bug today — it's a missing atomicity guarantee and an architectural constraint worth recording.
What a real fix would entail
Rework how the btree and VC layer share the graph lock so a VC op can hold it across statements — e.g.:
- a VC-level lock distinct from the btree's per-statement lock, held for the duration of a multi-step VC op; or
- making the btree's lock handling reentrant-aware of a VC-held outer lock that survives transaction boundaries (so
lockDepth isn't dropped to 0 by statement-end).
Severity
Low / design. Current behavior is correct (non-clobbering writes + durable rebase). File this so the constraint is known before anyone adds a multi-step VC op that assumes whole-op atomicity.
Context: #1094, PR #1097.
🤖 Generated with Claude Code
Finding
Discovered while fixing the concurrent-merge lost update (#1094, PR #1097).
The chunk-store graph lock (
CS_GRAPH_LOCK/chunkStoreLockAndRefresh/chunkStoreUnlock, withcs->lockDepthreentrancy) is acquired and released by the btree / SQL layer per write statement/transaction. A higher-level VC operation therefore cannot hold the lock across its own sub-operations — the btree releases it at statement/transaction boundaries.Evidence
While fixing #1094, the VC write CAS now reloads persisted refs at the outermost lock (
chunkStoreForceRefreshruns only whencs->lockDepth == 1). To make interactive rebase keep its in-flight in-memory state, I tried wrapping the whole rebase op in a single outer lock at the dispatcher (doltliteRebaseFunc): acquirechunkStoreLockAndRefresh+ force-refresh once at the top, expecting inner sub-ops to run reentrant (lockDepth > 1) and skip their own reload.It didn't stick. Instrumenting
chunkStoreForceRefreshto printcs->lockDepthshowed every call — including rebase's internal sub-ops — ran atlockDepth == 1. The dispatcher's hold was gone by the time the sub-ops ran, because the intervening btree/SQL work (doltliteEnsureWriteTxnAndSavepoints,sqlite3_execof the plan table, etc.) cycles the graph lock through its own per-statement acquire/release and doesn't preserve a VC-level outer hold.Implication
After #1097, VC write ops are non-clobbering — each reads fresh persisted refs at its CAS, so a fast-forward/branch-delete can't overwrite a peer's advance. But multi-step VC ops (
dolt_rebase('-i')/('--continue'), and conceptually any op that spans multiple statements/sub-ops) are not atomic against concurrent peers: a peer commit can interleave between sub-operations. There is no "this VC op sees one consistent snapshot for its entire duration" guarantee for multi-step ops.#1097 makes rebase correct anyway by persisting its intermediate state (the working branch) durably as it goes, rather than relying on holding a snapshot. So this is not a correctness bug today — it's a missing atomicity guarantee and an architectural constraint worth recording.
What a real fix would entail
Rework how the btree and VC layer share the graph lock so a VC op can hold it across statements — e.g.:
lockDepthisn't dropped to 0 by statement-end).Severity
Low / design. Current behavior is correct (non-clobbering writes + durable rebase). File this so the constraint is known before anyone adds a multi-step VC op that assumes whole-op atomicity.
Context: #1094, PR #1097.
🤖 Generated with Claude Code