Merge and commit swallow persist, restore, and CV-clear failures
- Dominant language
- C
- Stars
- 268
- Forks
- 18
- Avg merge
- 2h 27m
- Merged PRs (30d)
- 447
Description
The CAS design is careful (`doltliteRefreshAndConfirmHead` reads the on-disk tip; `CompareAndAdvanceBranch` keeps the lock across persist). Several call sites then ignore the result of the work that CAS is supposed to protect.
**`--force` commit drops CVs without checking the clear** (`src/doltlite_commit_cmd.c`):
```c
if( doltliteSessionHasConstraintViolations(db) ){
doltliteClearAllConstraintViolations(db);
}
```
`doltliteClearAllConstraintViolations` persists the empty CV catalog. If that persist fails, HEAD can still advance and the new working set can still carry CVs.
**Merge-commit create failure voids restore** (`src/doltlite_merge_cmd.c`):
```c
(void)doltliteRestoreTxnStateOnFailure(db, pSaved, rc);
sqlite3_result_error(context, "failed to create merge commit", -1);
```
The merged catalog is already live. If restore fails, the session sits on the merged tree with no merge commit.
**Same family**
- `dolt_conflicts_resolve` across multiple tables is not wrapped in a savepoint (`src/doltlite_conflicts.c`). Table `a` can be resolved, then `b` fails.
- Branch move treats a working-set read failure as empty and still deletes the source (`src/doltlite_branch.c`).
- Merge holds the graph lock across `REINDEX` / `ALTER` / `ANALYZE`, then unlocks before CV detection. Nested SQL can `chunkStoreCommit` at `lockDepth>0` with no HEAD CAS.
**Fix**
- Check `doltliteClearAllConstraintViolations`; do not CAS if it fails.
- Never `(void)` restore after a catalog switch; if restore fails, return that code and keep merge metadata consistent.
- One write savepoint around multi-table `dolt_conflicts_resolve`.
- Fail the branch move if `chunkStoreGetBranchWorkingSet` fails for any reason other than a documented empty set.
- Unlock after confirm, do install/CV work, re-confirm immediately before ref advance (the `--no-commit` path already does this). Add a two-connection test: merge that forces `ALTER`+`ANALYZE` must not block a peer `dolt_commit` for the whole install, and a peer commit in that window must yield `BUSY`, not a clobber.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the listed call sites in src/doltlite_commit_cmd.c, src/doltlite_merge_cmd.c, src/doltlite_conflicts.c, and src/doltlite_branch.c, then compare them with the existing --no-commit path. Trace the restore, constraint-violation, working-set, lock, and CAS results before running the relevant test suite. Done means each failure propagates safely, multi-table resolution is atomic, and the two-connection ALTER/ANALYZE merge test reports BUSY without clobbering a peer commit.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, sqlite
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100