Multi-step VC ops can't hold the graph lock across sub-operations (btree owns it per-statement)
- Dominant language
- C
- Stars
- 268
- Forks
- 18
- Avg merge
- 2h 26m
- Merged PRs (30d)
- 454
Description
## 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](https://claude.com/claude-code)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by tracing doltliteRebaseFunc, chunkStoreLockAndRefresh, chunkStoreUnlock, and chunkStoreForceRefresh through the btree/SQL path, including doltliteEnsureWriteTxnAndSavepoints and the sqlite3_exec plan-table work. Compare the behavior described in #1097. Done means a multi-step VC operation can preserve a consistent lock or snapshot across sub-operations without regressing non-clobbering writes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, sqlite
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 28/100