dolthub / dolthub/doltlite

Push lock contention is reported as "remote refs changed; pull and retry", and never retried

Closed
#2,906 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

Found in a full-repo review at `0ba280f06f`.

Every `SQLITE_BUSY` from the remote server is reported to the client as a ref race. The dominant source of BUSY is graph-lock contention, which has nothing to do with refs, so the client is told to pull — advice that is a no-op at best and can trigger a needless merge.

With a peer simply holding the served database's graph lock, and nothing touching refs:

```
POST /r/chunks -> HTTP/1.1 409 Conflict
{"code":"refs_changed","sqlite":5,"message":"remote refs changed; pull and retry"}

dolt_push -> Error: remote refs changed; pull and retry

(after the lock is released, the same push) -> 0
```

`POST /chunks` does not involve refs at all. A plain `dolt_gc()` on the served database made 11 of 12 concurrent HTTP pushes fail this way. The `file://` path says the same thing (`push failed (remote refs changed)`, `sqlite3_errcode()==5`).

## Cause

`sendSqliteError` maps the code unconditionally — `src/doltlite_remotesrv.c:162-165`:

```c
case SQLITE_BUSY:
sendStructuredError(fd, 409, "Conflict", "refs_changed", rc,
"remote refs changed; pull and retry");
return;
```

The BUSY actually comes from `remoteDbAcquire()` → `remoteSrvLockAndForceRefresh()` (`src/doltlite_remotesrv.c:1075`, also `:787`, `:804`). The client mirrors the message at `src/doltlite_http_remote.c:182`, and the `file://` path does the same at `src/doltlite_remote_sql.c:88`.

There is no busy handler and no retry anywhere in `doltlite_remote.c`, `doltlite_http_remote.c` or `doltlite_remotesrv.c`.

## Fix

Distinguish lock contention from a ref race — they need different codes, different messages and different client behaviour. Contention should carry a busy/locked diagnosis that names the lock (which is what the `writer.vc_command_busy` row of `test/concurrency_contract.tsv` already says the message should do) and should be retried with a bounded backoff rather than surfaced to the user. A genuine non-fast-forward already has its own code (`non_ff`), so `refs_changed` should be reserved for an actual ref change detected under the lock.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at sendSqliteError in src/doltlite_remotesrv.c and trace remoteDbAcquire() through remoteSrvLockAndForceRefresh(), then compare client handling in src/doltlite_http_remote.c, src/doltlite_remote_sql.c, and doltlite_remote.c. Read test/concurrency_contract.tsv, especially writer.vc_command_busy, and run the relevant concurrency tests. Done means lock contention has its own diagnosis and bounded retry behavior, while genuine ref races retain refs_changed.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, sqlite
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.