dolthub / dolthub/doltlite

Multi-step VC ops can't hold the graph lock across sub-operations (btree owns it per-statement)

Open
#1,098 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.