dolthub / dolthub/doltlite

Merge and commit swallow persist, restore, and CV-clear failures

Closed
#2,897 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.