Push lock contention is reported as "remote refs changed; pull and retry", and never retried
- 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