dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Borrow the library to index it, and give it back

Browse Source 
The passes that need every photograph's bytes — thumbnails, face
indexing — now borrow each one and release it at the end. On a
placeholder library that is the difference between peak disk being the
working set and being the whole library.

Including on cancellation, which was nearly missed: the face sweep
returns mid-loop when the user presses Stop, and without releasing there
the disk is spent and nothing is delivered for it.

`materialise` now answers whether *it* fetched the content. The pool used
to work that out by listing a file's parent directory — one listing per
file across a library — when the backend already had to `stat` it to
decide whether to ask. One syscall instead of a directory walk, and it
removes the bug class the tests found earlier: a file at the library root
has no `parent()`, so every one of them read as already-downloaded.

**Pinning is the retention control**, and it drives the model the catalog
already had rather than a second one. `tier_desired` is what the user
asked to keep hydrated, `pending_pins` is the resumable work list, and a
pinned collection is never dehydrated for the same reason it was never
evicted. It was in fact *broken* here before: `get` on a stub failed, and
the pin worker logged "one unreadable file must not abandon the whole
pin" and silently did nothing.

Pinned originals on such a library are recorded with `path = NULL`
(`Cache::record_in_place`) rather than copied under `originals/`. Two
reasons, and the second is the important one. A copy would hold every
pinned photograph twice, with the budget able to evict the half that was
not costing the disk. And `release` deletes the file a row names — so a
row that names none cannot delete anything, which puts the one
catastrophic operation out of reach by construction rather than by
remembering not to call it. Deleting a materialised file inside a synced
tree removes the photograph from the server and every other device.

Handing disk back is `spawn_dehydrate`, which asks the client.

Two gaps written down rather than papered over (docs/storage.md §7): a
hydrating pass cannot yet quote its cost, because a stub reports no size;
and the two sweeps hold separate pools, so a library indexed for both
fetches twice.
This commit is contained in:
dtourolle
2026-08-29 09:57:53 +02:00
parent c102ba9df2
commit 5768100816
9 changed files with 406 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-catalog/src/cache.rs
+50
View File
@@ -222,6 +222,56 @@ impl Cache {
Ok(())
}
/// TRACES: FR-NC-6c | FR-NC-6a
/// Record an original this cache does **not** own the bytes of.
///
/// The virtual-filesystem case. On a library kept by a sync client the
/// original is materialised *in the library folder itself*, so copying it
/// under `originals/` would hold two copies of every pinned photograph —
/// and the copy would be the one the budget could evict while the real
/// disk cost stayed.
///
/// So the bytes are left where they are and only the bookkeeping is kept.
/// `path` is deliberately `NULL`, which is what makes this safe:
/// [`release`](Self::release) deletes the file a row names, and a row that
/// names none deletes nothing. **That matters more than it sounds.**
/// Deleting a materialised file inside a synced folder does not free a
/// cache — it deletes the photograph, and the client propagates that to
/// the server and to every other device. Handing the disk back is the
/// backend's job (`RemoteBackend::dematerialise`), not this one's.
///
/// `bytes` is what the original occupies where it lies, for the budget and
/// for reporting; pass 0 where it is not known.
pub fn record_in_place(
&self,
conn: &Connection,
image: ImageId,
bytes: u64,
pinned: bool,
now: i64,
) -> Result<(), CatalogError> {
conn.execute(
"INSERT INTO image_cache
(image_id, tier_actual, tier_desired, bytes, last_used, pinned, path)
VALUES (?1, ?2, ?2, ?3, ?4, ?5, NULL)
ON CONFLICT(image_id) DO UPDATE SET
tier_actual = ?2,
tier_desired = max(tier_desired, ?2),
bytes = ?3,
last_used = ?4,
pinned = max(pinned, ?5),
path = NULL",
rusqlite::params![
image.0 as i64,
Tier::Original.stored(),
bytes as i64,
now,
i64::from(pinned),
],
)?;
Ok(())
}
/// Read a cached original back, if it is here.
///
/// Touches `last_used`, which is what makes the eviction order reflect
core/dr-sync-folder/src/borrow.rs
+8 -65
View File
@@ -85,27 +85,6 @@ impl BorrowPool {
&'p self,
backend: &dyn RemoteBackend,
path: &RemotePath,
) -> Result<Borrowed<'p>, RemoteError> {
self.borrow_known(backend, path, None).await
}
/// [`borrow`](Self::borrow) where the caller already knows whether the
/// content is here.
///
/// A sweep reads availability out of the catalog on its way to building a
/// work list, so making the pool re-derive it costs a directory listing
/// per file for an answer already in hand.
///
/// **`None` means "find out", and being wrong is not symmetric.** Claiming
/// a file was already local leaves it downloaded — disk, and nothing else.
/// Claiming it was not releases a file the user may have pinned. So an
/// uncertain caller passes `Some(true)` or `None`, never a guess at
/// `false`.
pub async fn borrow_known<'p>(
&'p self,
backend: &dyn RemoteBackend,
path: &RemotePath,
already_local: Option<bool>,
) -> Result<Borrowed<'p>, RemoteError> {
if !backend.capabilities().materialisation.can_materialise() {
return Ok(Borrowed {
@@ -129,32 +108,19 @@ impl BorrowPool {
}
}
let id = RemoteId::Path(path.clone());
// The backend answers whether *it* fetched the content, because it had
// to look before deciding. Determining that here instead would cost a
// directory listing per file, and getting it wrong in the wrong
// direction releases a file the user pinned.
let ours = backend.materialise(&RemoteId::Path(path.clone())).await?;
// What was here *before* we asked. The whole contract rests on this
// being read first: after `materialise` there is no way to tell what
// we brought from what was already there.
let was_local = match already_local {
Some(known) => known,
None => is_materialised(backend, path).await,
};
if !was_local {
backend.materialise(&id).await?;
}
self.lock().insert(
path.clone(),
Held {
borrowers: 1,
ours: !was_local,
},
);
self.lock()
.insert(path.clone(), Held { borrowers: 1, ours });
Ok(Borrowed {
pool: Some(self),
path: path.clone(),
hydrated: !was_local,
hydrated: ours,
})
}
@@ -237,28 +203,5 @@ impl Drop for Borrowed<'_> {
}
}
/// Whether the backend currently holds this file's content.
///
/// Asked by listing its parent, because the trait has no "stat one object" —
/// and adding one for this would put a method on every backend to serve a case
/// only one of them has.
async fn is_materialised(backend: &dyn RemoteBackend, path: &RemotePath) -> bool {
// `parent()` is `None` for a file directly under the library root, whose
// parent *is* the root — not "no parent". Treating the two alike reported
// every top-level photograph as already downloaded, so nothing was ever
// hydrated and nothing was ever released.
let parent = path.parent().unwrap_or_else(RemotePath::root);
match backend.list(&parent, None).await {
Ok(entries) => entries
.iter()
.find(|e| &e.path == path)
.map(|e| e.materialised)
// Not listed at all: nothing to hydrate, and the caller's own read
// will report the miss properly.
.unwrap_or(true),
Err(_) => true,
}
}
#[cfg(test)]
mod tests;
core/dr-sync-folder/src/borrow/tests.rs
+38 -14
View File
@@ -221,22 +221,46 @@ async fn borrowing_against_a_plain_folder_does_nothing_at_all() {
}
#[tokio::test]
async fn an_uncertain_caller_keeps_the_file_rather_than_releasing_it() {
// The asymmetry stated on `borrow_known`: claiming "already local" costs
// disk, claiming "not local" can release a pin.
let t = Tmp::new("uncertain");
t.real("a.CR2");
async fn the_backend_is_what_decides_whether_a_file_was_ours() {
// Not the pool, and not the caller. The backend had to look before
// deciding whether to ask, so it can answer for the cost of that same
// `stat`; a borrower working it out separately would pay a directory
// listing per file and could get it wrong in the direction that releases
// a file the user pinned.
let t = Tmp::new("who-decides");
t.real("had.CR2").stub("wanted.CR2");
let b = t.backend(FakeClient::new());
assert!(
!b.materialise(&RemoteId::Path(RemotePath::new("had.CR2")))
.await
.unwrap(),
"already here, so not ours to release"
);
assert!(
b.materialise(&RemoteId::Path(RemotePath::new("wanted.CR2")))
.await
.unwrap(),
"this call fetched it"
);
}
#[tokio::test]
async fn a_file_borrowed_twice_in_one_pass_is_fetched_once_and_released_once() {
// Thumbnailing and face indexing visit the same photograph. Fetching it
// per stage doubles the transfer over the whole library.
let t = Tmp::new("sequential");
t.stub("a.CR2");
let client = FakeClient::new();
let b = t.backend(client.clone());
let pool = BorrowPool::new();
let path = RemotePath::new("a.CR2");
{
let _held = pool
.borrow_known(&b, &RemotePath::new("a.CR2"), Some(true))
.await
.unwrap();
}
pool.release_all(&b).await;
assert!(t.has("a.CR2"), "kept");
assert_eq!(client.dehydrations.load(Ordering::SeqCst), 0);
// Sequential borrows, as two passes over one work list would make.
drop(pool.borrow(&b, &path).await.unwrap());
drop(pool.borrow(&b, &path).await.unwrap());
assert_eq!(client.hydrations.load(Ordering::SeqCst), 1, "paid once");
let stats = pool.release_all(&b).await;
assert_eq!(stats.released, 1, "given back once");
}
core/dr-sync-folder/src/lib.rs
+5 -5
View File
@@ -773,12 +773,12 @@ impl RemoteBackend for FolderBackend {
/// Suffix-mode VFS *renames* on hydration, so completion is the
/// materialised path appearing — not the stub changing size. Polling the
/// original would wait forever.
async fn materialise(&self, id: &RemoteId) -> Result<(), RemoteError> {
async fn materialise(&self, id: &RemoteId) -> Result<bool, RemoteError> {
let (local, materialised) = self.locate_id(id)?;
if materialised {
// Already here. Not an error, and not a reason to ask again: the
// borrow pool relies on this being idempotent.
return Ok(());
// Already here. Not an error, and not a reason to ask again — and
// `false` is what tells a borrower to leave it alone afterwards.
return Ok(false);
}
let vfs = self.vfs.clone();
let target = self.resolve_id(id)?;
@@ -793,7 +793,7 @@ impl RemoteBackend for FolderBackend {
let deadline = std::time::Instant::now() + MATERIALISE_TIMEOUT;
while std::time::Instant::now() < deadline {
if target.is_file() {
return Ok(());
return Ok(true);
}
std::thread::sleep(POLL);
}
core/dr-sync/src/lib.rs
+10 -3
View File
@@ -170,9 +170,16 @@ pub trait RemoteBackend: Send + Sync {
/// agreed to. **Never for filling a grid**: doing so downloads the entire
/// library to produce thumbnails.
///
/// Returns once the content is readable. Callers that borrowed it should
/// hand it back with [`dematerialise`](Self::dematerialise).
async fn materialise(&self, _id: &RemoteId) -> Result<(), RemoteError> {
/// Returns once the content is readable, and **whether this call is what
/// brought it here** — `false` meaning it was already local.
///
/// That boolean is the whole basis of borrowing. A caller releasing what
/// it fetched must not release what the user already had, and after the
/// fact the two are indistinguishable; the backend knows because it had to
/// look before deciding whether to ask. Answering it here costs the `stat`
/// the implementation performs anyway, where a caller determining it
/// separately would pay a directory listing per file.
async fn materialise(&self, _id: &RemoteId) -> Result<bool, RemoteError> {
Err(RemoteError::Unsupported(
"this backend has no placeholders to materialise",
))