fix(database): normalize virtual health paths - #836
Conversation
javi11
left a comment
javi11
left a comment
There was a problem hiding this comment.
Nice win on the join change — dropping TRIM() from the join predicates makes them sargable so idx_file_health_path actually gets used. That part I like.
Migration 035 is the blocker though. It does DELETE FROM file_health + full reinsert, so it rewrites the whole table and all 8 indexes regardless of how many rows actually need fixing. I benchmarked it on SQLite/WAL, NVMe:
| rows | dirty | 035 |
|---|---|---|
| 50k | 0 | 1.6s |
| 200k | 0 | 9.1s |
| 200k | 1/1000 | 9.6s |
Cost scales with library size, not with rows needing repair — a clean 200k table pays the same 9.1s. That's a fast dev machine; most people run AltMount on a NAS or Pi, so 500k–1M rows is minutes of silent hang at startup with no logging. Users will assume it's stuck and kill it mid-migration.
Doing only the dirty rows is 75ms at 200k (~120x), and the merge then runs over 200 rows instead of 200k:
UPDATE file_health SET file_path = trim(replace(file_path, char(92), '/'), '/')
WHERE file_path <> trim(replace(file_path, char(92), '/'), '/')
AND NOT EXISTS (SELECT 1 FROM file_health c
WHERE c.file_path = trim(replace(file_health.file_path, char(92), '/'), '/'));Whatever is still non-canonical after that is exactly the collision set — run the existing merge over just those.
Two more things on the migration: it materializes two full copies of the table in temp storage, and in Docker SQLite's temp store often lands on a small /tmp or a RAM-backed tmpfs, so a big library can hit database or disk is full or OOM the container. And on Postgres the delete+reinsert leaves 100% dead tuples — table and indexes stay at double size until autovacuum catches up, and you can't VACUUM inside the goose transaction.
Two things outside the diff that belong in this PR:
internal/database/health_repository.go:1996 (GetFilesByPaths) — this one got missed, still strings.TrimPrefix(path, "/"), the same idiom you replaced in DeleteHealthRecordsBulk just above. No backslash conversion, no trailing trim, no double-slash. Matters more now that the joins lost their TRIM() — that was what was covering for it. Caller is POST /health/regenerate-symlinks, which takes file_paths straight from the request, so \movies\x.mkv silently matches nothing and it reports 0 files.
health_repository.go:1731 — while you're on that line, the prefix is + "%" but every other prefix query in the file uses + "/%" (569, 605, 635). So tv/Show also matches tv/ShowOther/..., and this is a DELETE. Pre-existing, but TrimPrefix → Trim now also strips a trailing separator a caller might have passed as the bound.
| SET virtual_path = trim(replace(virtual_path, char(92), '/'), '/') | ||
| WHERE virtual_path <> trim(replace(virtual_path, char(92), '/'), '/'); | ||
|
|
||
| DROP TABLE IF EXISTS file_health_path_normalized; |
There was a problem hiding this comment.
This would drop a real table of that name if one ever existed. Doesn't buy anything either — temp tables die with the session.
| WHEN 'degraded' THEN 4 | ||
| WHEN 'checking' THEN 3 | ||
| WHEN 'pending' THEN 2 | ||
| WHEN 'partial' THEN 2 |
There was a problem hiding this comment.
Dead code — partial was dropped from the CHECK constraint back in 004, so no row can hold it. Same on line 76. Reads like live logic.
| WITH ranked AS ( | ||
| SELECT h.*, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE status | ||
| WHEN 'corrupted' THEN 6 | ||
| WHEN 'repair_triggered' THEN 5 | ||
| WHEN 'degraded' THEN 4 | ||
| WHEN 'checking' THEN 3 | ||
| WHEN 'pending' THEN 2 | ||
| WHEN 'partial' THEN 2 | ||
| WHEN 'healthy' THEN 1 | ||
| ELSE 0 | ||
| END DESC, updated_at DESC, id DESC) AS status_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN NULLIF(library_path, '') IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS library_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN NULLIF(last_error, '') IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS last_error_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN NULLIF(source_nzb_path, '') IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS source_nzb_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN NULLIF(error_details, '') IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS error_details_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN release_date IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS release_date_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN NULLIF(metadata, '') IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS metadata_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN NULLIF(indexer, '') IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS indexer_rank, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY canonical_path | ||
| ORDER BY CASE WHEN NULLIF(download_id, '') IS NOT NULL THEN 0 ELSE 1 END, | ||
| updated_at DESC, id DESC) AS download_id_rank | ||
| FROM file_health_path_normalized h | ||
| ) | ||
| SELECT | ||
| COALESCE(MIN(CASE WHEN file_path = canonical_path THEN id END), MIN(id)) AS id, | ||
| canonical_path AS file_path, | ||
| MAX(CASE WHEN library_rank = 1 AND NULLIF(library_path, '') IS NOT NULL THEN library_path END) AS library_path, | ||
| MAX(CASE WHEN status_rank = 1 THEN CASE status WHEN 'partial' THEN 'corrupted' ELSE status END END) AS status, |
There was a problem hiding this comment.
The merge is column-wise across 9 independent rank windows, so the survivor is a composite that existed in no prior state — status from one row, counters from another, timestamps from a third. MAX(retry_count) in particular trips the repair gate at worker.go:523 a cycle early.
Not strictly wrong, "worst-of wins" is a fair reading of not discarding evidence, and it only hits real collisions. But picking one winner row and backfilling only its NULL/empty columns would be easier to reason about. At minimum worth a comment saying this is deliberate.
|
Thanks for the detailed review. The branch is now at
The remaining action is maintainer review/merge; I’m not claiming merge authority. |
|
Nice work on this — the 1. Removing the
|
| Scenario | 500k rows | 1M rows |
|---|---|---|
| Clean catalog (no dirty paths) | 0.79s | — |
Every path dirty (legacy leading /) |
21.5s | 44.1s |
| Every path dirty (Windows backslashes) | 21.4s | — |
| 500k rows, 250k collision pairs | 16.5s | — |
Scaling is linear (2x rows -> 2.05x time). The clean fast path is excellent and matches your 100k/61ms test.
The gap is that the clean case is the only one benchmarked, and a legacy leading-slash catalog is exactly the population this migration exists for — so "every row dirty" is the expected case for affected users, not a pathological one. That's 21.5s at 500k and 44s at 1M of blocked startup, since goose.Up runs before the app serves.
Where the time goes at 500k all-dirty:
| Stage | Time | Share |
|---|---|---|
file_health_path_collisions GROUP BY scan |
0.15-0.6s | 3% |
file_health_path_affected build |
0.33-0.54s | 2% |
file_health_path_merged (9 window functions) |
10.6s | 49% |
DELETE FROM file_health |
3.0s | 14% |
INSERT (rebuilds 5 indexes) |
7.9s | 37% |
The merge CTE dominates: SQLite computes nine separate ROW_NUMBER() partitions, each needing its own sort of the affected set. And for a non-colliding dirty row the group size is 1, so all nine ranks are trivially 1 — the migration pays nine sorts plus a DELETE+INSERT with full index rebuild for rows that need nothing but a file_path rewrite.
Suggested split
A non-colliding dirty row has a unique canonical target, so a plain in-place UPDATE is safe against UNIQUE(file_path). Reserve the window-function merge for actual collision groups, which are normally a handful of rows rather than the whole catalog:
-- collisions temp table + index: unchanged
DROP TRIGGER update_file_health_timestamp;
UPDATE file_health
SET file_path = trim(replace(file_path, char(92), '/'), '/')
WHERE file_path <> trim(replace(file_path, char(92), '/'), '/')
AND NOT EXISTS (
SELECT 1 FROM file_health_path_collisions c
WHERE c.canonical_path = trim(replace(file_health.file_path, char(92), '/'), '/'));
CREATE TRIGGER update_file_health_timestamp
AFTER UPDATE ON file_health
BEGIN
UPDATE file_health SET updated_at = CURRENT_TIMESTAMP WHERE id = NEW.id;
END;
-- then run the existing affected/merged/DELETE/INSERT path, but scoped to
-- collision groups onlyMeasured on the same 500k all-dirty dataset:
| Approach | Time |
|---|---|
| Current (DELETE + INSERT for all affected) | 21.5s |
UPDATE for non-colliding + merge only collision groups |
5.5s |
~4x faster and it skips the index rebuild entirely. The trigger drop/recreate is what preserves updated_at — I confirmed it stays at 2026-08-22 10:00:00 across the run, so you keep the property that motivated DELETE+INSERT in the first place.
Two smaller notes on cost: the temp tables materialize full row copies of the affected set, so at 1M dirty rows that's roughly two extra copies of the table's row data spilling to SQLite's temp file — worth a thought on containers with a small /tmp. And the PostgreSQL cost is entirely unmeasured; a whole-table DELETE+INSERT there also leaves table and index bloat pending autovacuum on top of the same window-function work.
2 participants
Summary
Why
Legacy leading separators and Windows separators can make logically identical paths miss health/history joins or collide under the file_health unique key. Normalizing only new writes leaves existing rows inconsistent, so the migration and repository behavior land together.
This is separate from #667: that PR fixed failed-NZB filesystem namespacing; this change repairs persisted virtual-path identity.
Validation
The migration is intentionally non-reversible because collision merging cannot be losslessly undone.