Add origin-backed lazy clones - #2552
Open
eric-richardson1 wants to merge 4 commits into
Open
Conversation
Implement an owned built-in source on the existing callback seam, with host callback precedence, 256-hash remote batches, the existing credential and TLS client, and read-only file remotes. Keep activation connection-local and inert until an origin remote exists. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Teach dolt_clone --lazy to install refs, origin, and tracking refs without transferring chunks. Enable origin-backed misses per connection with file:...?lazy_origin=1, including bootstrap and query in one connection, while leaving the on-disk format unchanged. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Update origin tracking refs without bulk chunk transfer when lazy_origin is active. Fast-forward pulls fault data on demand, while divergent pulls return a clear materialization requirement instead of entering a graph-locked merge. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Cover file and HTTP refs-only clones, same-connection activation, reopen behavior, refresh, offline cached and uncached reads, host precedence, amalgamation, and compile-out errors. Document URI activation, transport lifecycle, failure semantics, and deferred follow-ups. Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
SummaryThe run broadly covered lazy and normal data access, remote synchronization, branch and revision consistency, offline recovery, integrity validation, authentication, and failure handling across local and network-backed sources. These happy-path, edge-case, and adversarial scenarios were generally healthy, but concurrent access exposed a user-visible reliability problem. Merge with caution — the PR introduces a medium-severity concurrency failure in origin-backed reads, where overlapping reads can fail and leave data unavailable offline after an interrupted fetch. The remaining covered behaviors passed, so this is not a broad functional failure, but the attributable read-reliability issue warrants resolution before treating the change as fully safe. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
There was a problem hiding this comment.
Concurrent reads fail with a locked database
What failed: The row read returned the expected value, but the overlapping table scan stopped with a database-locked error. Once the origin was unavailable, the scan could not recover the chunk whose write had been interrupted.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: A broad read can fail when users read data at the same time, even though a single-row read works. If a fetch is interrupted, a later offline read may also be unable to load that chunk until the source is available again.
- Steps to Reproduce:
- Create a writable lazy clone with a table whose chunks are still at the origin.
- Start one process reading a single row and another process scanning the table so both processes fetch overlapping missing chunks.
- Observe that the single-row read succeeds while the broad scan reports that the database is locked.
- Remove the origin and repeat the broad scan; observe the missing-origin-chunk error for data that was not persisted after the lock failure.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR changes src/doltlite_chunk_source.c to add the origin-backed source and to use the source persistence flow for fetched origin bytes. In the current implementation, chunkStoreSourceGet at src/doltlite_chunk_source.c:502-527 verifies the fetched bytes and calls csSourcePersistMany before putting them in the connection cache; chunkStoreSourcePrefetchMany at lines 587-627 does the same for a broad scan and returns immediately when persistence fails. csSourcePersistMany at lines 404-435 opens the writer and calls chunkStoreLockAndRefresh at line 406, but has no retry or wait when that call returns SQLITE_BUSY. The shared lock implementation in src/chunk_store_lock.c:272-308 uses sqlite3_mutex_try at lines 277-279 and nonblocking csFileLockNB at lines 288-296, so a second connection contending for the graph lock receives SQLITE_BUSY immediately. That error is returned by csSourcePersistMany, then by prefetch, and finally reaches the scan as the observed database-locked failure. Because cache publication occurs only after csSourcePersistMany succeeds, the failed fetch is not safely available to a later offline read, which explains the subsequent hash-specific missing-origin error. The smallest practical fix is to coordinate or retry the shared lock for this lazy persistence operation, with bounded retry handling that preserves rollback and only reports SQLITE_BUSY after the contention cannot be resolved.
- Why this is likely a bug: This is not only a browser or setup failure: the recorded result contains a successful demand value, a failed concurrent scan, and a concrete database-locked error, and the source has a matching error path. The PR's new origin-backed lazy reads can therefore turn ordinary concurrent reads into a user-visible failed query. The lock is intentionally nonblocking, but the new persistence caller does not absorb or retry contention, so temporary overlap becomes a hard read failure; a bounded retry or equivalent coordination at this persistence boundary is the targeted remediation.
Relevant code
src/doltlite_chunk_source.c:404-435
rc = csSourceOpenWriter(cs, p, 0);
if( rc!=SQLITE_OK || !p->writerOpen ) return rc;
rc = chunkStoreLockAndRefresh(&p->writer);
...
if( rc!=SQLITE_OK ) chunkStoreRollback(&p->writer);
chunkStoreUnlock(&p->writer);
return rc;src/doltlite_chunk_source.c:621-637
rc = csSourcePersistMany(cs, p, aMissing, apData, anData, nMissing);
if( rc!=SQLITE_OK ) goto prefetch_done;
for(i=0; i<nMissing; i++){
if( !apData[i] ) continue;
rc = csSourceCachePut(p, &aMissing[i], apData[i], anData[i]);
if( rc!=SQLITE_OK ) goto prefetch_done;
}
...
return rc;src/chunk_store_lock.c:272-298
if( cs->pLockMutex && sqlite3_mutex_try(cs->pLockMutex)!=SQLITE_OK ){
return SQLITE_BUSY;
}
...
rc = csFileLockNB(cs->file.pVfs, cs->file.zFilename,
&CS_GRAPH_LOCK(cs), &cs->pGraphLockName);
if( rc!=SQLITE_OK ){
if( cs->pLockMutex ) sqlite3_mutex_leave(cs->pLockMutex);
return rc;
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Concurrent reads fail with a locked database**
**What failed:** The row read returned the expected value, but the overlapping table scan stopped with a database-locked error. Once the origin was unavailable, the scan could not recover the chunk whose write had been interrupted.
- **Impact:** A broad read can fail when users read data at the same time, even though a single-row read works. If a fetch is interrupted, a later offline read may also be unable to load that chunk until the source is available again.
- **Steps to reproduce:**
1. Create a writable lazy clone with a table whose chunks are still at the origin.
2. Start one process reading a single row and another process scanning the table so both processes fetch overlapping missing chunks.
3. Observe that the single-row read succeeds while the broad scan reports that the database is locked.
4. Remove the origin and repeat the broad scan; observe the missing-origin-chunk error for data that was not persisted after the lock failure.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR changes src/doltlite_chunk_source.c to add the origin-backed source and to use the source persistence flow for fetched origin bytes. In the current implementation, chunkStoreSourceGet at src/doltlite_chunk_source.c:502-527 verifies the fetched bytes and calls csSourcePersistMany before putting them in the connection cache; chunkStoreSourcePrefetchMany at lines 587-627 does the same for a broad scan and returns immediately when persistence fails. csSourcePersistMany at lines 404-435 opens the writer and calls chunkStoreLockAndRefresh at line 406, but has no retry or wait when that call returns SQLITE_BUSY. The shared lock implementation in src/chunk_store_lock.c:272-308 uses sqlite3_mutex_try at lines 277-279 and nonblocking csFileLockNB at lines 288-296, so a second connection contending for the graph lock receives SQLITE_BUSY immediately. That error is returned by csSourcePersistMany, then by prefetch, and finally reaches the scan as the observed database-locked failure. Because cache publication occurs only after csSourcePersistMany succeeds, the failed fetch is not safely available to a later offline read, which explains the subsequent hash-specific missing-origin error. The smallest practical fix is to coordinate or retry the shared lock for this lazy persistence operation, with bounded retry handling that preserves rollback and only reports SQLITE_BUSY after the contention cannot be resolved.
- **Why this is likely a bug:** This is not only a browser or setup failure: the recorded result contains a successful demand value, a failed concurrent scan, and a concrete database-locked error, and the source has a matching error path. The PR's new origin-backed lazy reads can therefore turn ordinary concurrent reads into a user-visible failed query. The lock is intentionally nonblocking, but the new persistence caller does not absorb or retry contention, so temporary overlap becomes a hard read failure; a bounded retry or equivalent coordination at this persistence boundary is the targeted remediation.
**Relevant code:**
`src/doltlite_chunk_source.c:404-435`
~~~c
rc = csSourceOpenWriter(cs, p, 0);
if( rc!=SQLITE_OK || !p->writerOpen ) return rc;
rc = chunkStoreLockAndRefresh(&p->writer);
...
if( rc!=SQLITE_OK ) chunkStoreRollback(&p->writer);
chunkStoreUnlock(&p->writer);
return rc;
~~~
`src/doltlite_chunk_source.c:621-637`
~~~c
rc = csSourcePersistMany(cs, p, aMissing, apData, anData, nMissing);
if( rc!=SQLITE_OK ) goto prefetch_done;
for(i=0; i<nMissing; i++){
if( !apData[i] ) continue;
rc = csSourceCachePut(p, &aMissing[i], apData[i], anData[i]);
if( rc!=SQLITE_OK ) goto prefetch_done;
}
...
return rc;
~~~
`src/chunk_store_lock.c:272-298`
~~~c
if( cs->pLockMutex && sqlite3_mutex_try(cs->pLockMutex)!=SQLITE_OK ){
return SQLITE_BUSY;
}
...
rc = csFileLockNB(cs->file.pVfs, cs->file.zFilename,
&CS_GRAPH_LOCK(cs), &cs->pGraphLockName);
if( rc!=SQLITE_OK ){
if( cs->pLockMutex ) sqlite3_mutex_leave(cs->pLockMutex);
return rc;
}
~~~
Contributor
DoltLite performance vs PR base
blobpk details
compositepk details
int details
textpk details
vc details
All relative performance gates passed. |
Contributor
DoltLite source coverage
Merged 202 pooled raw profiles from the distributed Linux correctness jobs. Per-file coverage (99 files)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #2551 and uses its chunk-source seam as the engine-owned executor.
Summary
originremote, reusing the existing file and HTTP(S) remote clientsdolt_clone('--lazy', url)to install refs, origin, and tracking refs without transferring the reachable chunk graphfile:...?lazy_origin=1, including bootstrap and query in one connectionPhase 0 / design
lazy_origin=1is inert until an origin exists, so it activates immediately afterdolt_clone('--lazy', ...)records origin. This makes no file-format, manifest, WAL, or version change.xGetChunksbatches, credential resolution, and TLS behavior. Version 1 opens a client per callback; file remotes are opened read-only.dolt_fetch('origin')updates tracking refs without bulk transfer. Fast-forward pull remains lazy. A divergent pull fails with a materialization requirement because merge would otherwise need source reads while graph-locked.lazy_origin=1, cached chunks remain readable and an uncached miss reportsNOTFOUNDwith the requested hash and activation remedy. With activation, unreachable-origin transport reports a hash-named chunk-source I/O error rather than corruption.Validation
./build/chunk_source_test: 286/286DOLTLITE_ENABLE_CHUNK_SOURCE=0buildDeferred follow-ups
Co-Authored-By: OpenAI Codex noreply@openai.com