Skip to main content

max / audiofiles

storage: close orphan-GC unlink race and make similarity a true metric (ultra-fuzz) The orphan blob unlink ran after the row-delete transaction committed, so a concurrent import on a separate connection could re-adopt the hash (re-insert the row, reuse the still-present blob) in the gap, after which the late unlink deleted a live blob, leaving a row pointing at a missing file. Move the unlink inside the BEGIN IMMEDIATE transaction so the write lock is held across it; the re-adoption can no longer commit until the blob is gone. The post-commit loop is deleted. feature_distance divided by the count of overlapping dimensions, a per-pair denominator that broke the triangle inequality the VP-tree index relies on for pruning. Impute missing dims to the normalized midpoint and divide by the constant total weight, making it a genuine weighted-Euclidean metric. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-19 20:46 UTC
Commit: 71716cc6a9f3e8bcfa8f78b0708f959cb11b4e48
Parent: b7ef117
2 files changed, +69 insertions, -27 deletions
M crates/audiofiles-core/src/similarity.rs +45 -12
@@ -111,12 +111,22 @@ fn normalize(fv: &FeatureVector, ranges: &NormRanges) -> FeatureVector {
111 111 }
112 112 }
113 113
114 + /// Midpoint of the normalized `[0, 1]` feature range, used to impute a missing
115 + /// dimension so every vector maps to a complete point in the same space.
116 + const FEATURE_IMPUTE: f64 = 0.5;
117 +
114 118 /// Compute weighted Euclidean distance between two feature vectors.
115 - /// Skips dimensions where either vector has `None`.
119 + ///
120 + /// Missing dimensions (`None` on either side) are imputed to the neutral
121 + /// normalized midpoint and the sum is divided by the constant total weight, so
122 + /// the function is a true weighted-Euclidean metric over a fixed 12-dimensional
123 + /// basis. This matters because the VP-tree index prunes using the triangle
124 + /// inequality: the previous form divided by the *count of overlapping dims*, a
125 + /// per-pair denominator that varied with which dimensions happened to be
126 + /// present, broke the triangle inequality, and could prune a genuine nearest
127 + /// neighbour. Analysis populates all DSP columns together, so imputation is
128 + /// rare in practice; it exists to keep the metric well-defined when it isn't.
116 129 pub fn feature_distance(a: &FeatureVector, b: &FeatureVector, weights: &FeatureWeights) -> f64 {
117 - let mut sum = 0.0;
118 - let mut dims = 0.0;
119 -
120 130 let pairs: [(Option<f64>, Option<f64>, f64); 12] = [
121 131 (a.bpm, b.bpm, weights.bpm),
122 132 (a.duration, b.duration, weights.duration),
@@ -132,17 +142,19 @@ pub fn feature_distance(a: &FeatureVector, b: &FeatureVector, weights: &FeatureW
132 142 (a.attack_time, b.attack_time, weights.attack_time),
133 143 ];
134 144
145 + let mut sum = 0.0;
146 + let mut total_weight = 0.0;
135 147 for (va, vb, w) in &pairs {
136 - if let (Some(va), Some(vb)) = (va, vb) {
137 - let diff = va - vb;
138 - sum += w * diff * diff;
139 - dims += 1.0;
140 - }
148 + let va = va.unwrap_or(FEATURE_IMPUTE);
149 + let vb = vb.unwrap_or(FEATURE_IMPUTE);
150 + let diff = va - vb;
151 + sum += w * diff * diff;
152 + total_weight += w;
141 153 }
142 154
143 - // Return a large finite distance when no dimensions overlap, preserving
144 - // the VP-tree triangle inequality contract (INFINITY violates it).
145 - if dims == 0.0 { 1e10 } else { (sum / dims).sqrt() }
155 + // Constant scale factor (sum of all weights) keeps this a metric. Only zero
156 + // when every weight is zero, in which case all distances are zero.
157 + if total_weight == 0.0 { 0.0 } else { (sum / total_weight).sqrt() }
146 158 }
147 159
148 160 /// Load the feature vector for a sample by hash.
@@ -551,6 +563,27 @@ mod tests {
551 563 }
552 564
553 565 #[test]
566 + fn distance_satisfies_triangle_inequality_with_missing_dims() {
567 + // The VP-tree index prunes on the triangle inequality, so the distance
568 + // must satisfy d(a,c) <= d(a,b) + d(b,c) even when vectors have
569 + // different sets of missing dimensions (the case the old per-pair
570 + // denominator broke).
571 + let w = FeatureWeights::default();
572 + let a = FeatureVector { bpm: Some(0.1), duration: Some(0.9), ..Default::default() };
573 + let b = FeatureVector { bpm: Some(0.8), lufs: Some(0.2), ..Default::default() };
574 + let c = FeatureVector { duration: Some(0.1), spectral_centroid: Some(0.7), ..Default::default() };
575 +
576 + let ab = feature_distance(&a, &b, &w);
577 + let bc = feature_distance(&b, &c, &w);
578 + let ac = feature_distance(&a, &c, &w);
579 + assert!(
580 + ac <= ab + bc + 1e-9,
581 + "triangle inequality violated: d(a,c)={ac} > d(a,b)+d(b,c)={}",
582 + ab + bc
583 + );
584 + }
585 +
586 + #[test]
554 587 fn ranking_correctness() {
555 588 let db = Database::open_in_memory().unwrap();
556 589 insert_with_features(&db, "ref", 120.0, 1.0);
M crates/audiofiles-core/src/store.rs +24 -15
@@ -310,16 +310,26 @@ impl SampleStore {
310 310 // Re-check orphan status inside the DELETE so we don't lose a race with
311 311 // a concurrent import that linked the sample after the query above —
312 312 // and so we only unlink blobs for rows we actually removed (a live
313 - // re-link must keep both row and blob). The blob unlink happens after
314 - // commit: a failed unlink leaves a blob with no row, but the store is
315 - // content-addressed, so a re-import of the same bytes re-adopts it (and
316 - // a verify sweep can reclaim it) — disk waste, never data loss.
317 - let deleted: Vec<(String, String)> = db.transaction(|| {
313 + // re-link must keep both row and blob).
314 + //
315 + // The blob unlink runs INSIDE the transaction, while `BEGIN IMMEDIATE`
316 + // still holds the write lock. This is load-bearing: cleanup and import
317 + // hold separate connections (serialized only by SQLite's write lock,
318 + // not the in-process Database mutex), so an unlink performed after
319 + // COMMIT could delete a blob that a concurrent import had already
320 + // re-adopted in the gap (import sees the blob present, re-inserts the
321 + // row, leaves the blob) — leaving a live row pointing at a missing
322 + // file. Holding the write lock across the unlink makes that re-adoption
323 + // unable to commit until the blob is already gone, so the re-import
324 + // re-writes the blob instead. A failed unlink only leaves an orphan
325 + // blob (disk waste, reclaimed by a later re-import or verify sweep) and
326 + // must not roll back the committed row delete, so its error is ignored.
327 + let deleted = db.transaction(|| {
318 328 db.conn().execute(
319 329 "UPDATE sync_state SET value = '1' WHERE key = 'applying_remote'",
320 330 [],
321 331 )?;
322 - let mut done = Vec::with_capacity(orphans.len());
332 + let mut count = 0usize;
323 333 for (hash, ext) in &orphans {
324 334 let n = db.conn().execute(
325 335 "DELETE FROM samples WHERE hash = ?1 \
@@ -328,23 +338,22 @@ impl SampleStore {
328 338 [hash],
329 339 )?;
330 340 if n > 0 {
331 - done.push((hash.clone(), ext.clone()));
341 + if let Ok(path) = self.sample_path(hash, ext)
342 + && path.exists()
343 + {
344 + let _ = fs::remove_file(&path);
345 + }
346 + count += 1;
332 347 }
333 348 }
334 349 db.conn().execute(
335 350 "UPDATE sync_state SET value = '0' WHERE key = 'applying_remote'",
336 351 [],
337 352 )?;
338 - Ok(done)
353 + Ok(count)
339 354 })?;
340 355
341 - for (hash, ext) in &deleted {
342 - if let Ok(path) = self.sample_path(hash, ext)
343 - && path.exists() {
344 - let _ = fs::remove_file(&path);
345 - }
346 - }
347 - Ok(deleted.len())
356 + Ok(deleted)
348 357 }
349 358
350 359 /// Get the store root directory.