diff --git a/backend/src/db/schema.sql b/backend/src/db/schema.sql index b274c7c..c824d34 100644 --- a/backend/src/db/schema.sql +++ b/backend/src/db/schema.sql @@ -211,8 +211,15 @@ CREATE TABLE IF NOT EXISTS track_genre ( CREATE TABLE IF NOT EXISTS dislikes ( track_id UUID PRIMARY KEY REFERENCES tracks(id) ON DELETE CASCADE, disliked_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + -- When the track entered HIDDEN. The pre-deletion grace period + -- (MUZICK_HARD_DELETE_GRACE_DAYS, default 7 days) is measured from here — + -- separate from, and much longer than, grace_hours below. Recorded + -- explicitly rather than inferred from disliked_at, because an irreversible + -- clock must not rest on 'HIDDEN' merely being the state DEFAULT. + hidden_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, warned_at TIMESTAMP, deleted_at TIMESTAMP, + -- HIDDEN -> WARNED delay (the ntfy warning), NOT the deletion delay. grace_hours INTEGER DEFAULT 48, state dislike_state DEFAULT 'HIDDEN' ); diff --git a/backend/src/services/db.service.ts b/backend/src/services/db.service.ts index 587a7c9..e1b3084 100644 --- a/backend/src/services/db.service.ts +++ b/backend/src/services/db.service.ts @@ -1,4 +1,7 @@ -import { readFile, unlink } from 'fs/promises'; +// No `unlink` here, deliberately: the backend mounts /music `:ro` and must never +// remove a library file. Deletions are queued into pending_file_deletions and +// carried out by the worker's gated cleanup sweep. +import { readFile } from 'fs/promises'; import { fileURLToPath } from 'url'; import { dirname, join } from 'path'; import { Pool, PoolClient } from 'pg'; @@ -673,6 +676,53 @@ const MIGRATIONS: { id: string; sql: string }[] = [ END $mig$; `, }, + { + // Sorts after 20260730_claims_dedup_nulls_not_distinct and + // 20260730_feedback_track_id_set_null (append-only registry; 'h' > 'f' > 'c'). + id: '20260730_hard_delete_audit_trail', + sql: ` + -- Real hard deletion of disliked files (invariant §C). Three pieces: + -- + -- 1. Denormalised identity on the audit row. 20260730_feedback_track_id_set_null + -- made feedback.track_id ON DELETE SET NULL so the 'deleted_permanent' + -- audit row outlives the track — but the surviving row no longer says + -- WHICH track was destroyed. Under real (irreversible) deletion that row + -- is the only forensic record, so copy the identifying fields into it. + ALTER TABLE feedback ADD COLUMN IF NOT EXISTS track_path TEXT; + ALTER TABLE feedback ADD COLUMN IF NOT EXISTS track_title TEXT; + ALTER TABLE feedback ADD COLUMN IF NOT EXISTS track_artist TEXT; + + -- 2. An explicit "entered HIDDEN" timestamp. The pre-deletion grace period + -- (7 days, MUZICK_HARD_DELETE_GRACE_DAYS) is measured from it. Until now + -- the only nearby column was disliked_at, which lines up with HIDDEN + -- entry solely because 'HIDDEN' happens to be the state DEFAULT and + -- restoreDislike() deletes the row outright. That is far too incidental a + -- basis for an irreversible clock, so record it directly. + -- Added without a DEFAULT first: a DEFAULT would backfill existing rows + -- with now(), resetting every in-flight dislike's clock. + ALTER TABLE dislikes ADD COLUMN IF NOT EXISTS hidden_at TIMESTAMP; + UPDATE dislikes SET hidden_at = disliked_at WHERE hidden_at IS NULL; + ALTER TABLE dislikes ALTER COLUMN hidden_at SET DEFAULT CURRENT_TIMESTAMP; + + -- 3. A durable marker for files whose DB row is already gone but whose + -- bytes are not. The sweep commits the transaction FIRST and unlinks + -- afterwards, so this row is what makes the window crash-safe and an + -- unlink failure (EROFS, EACCES, EBUSY) recorded rather than swallowed. + -- Deliberately no FK to tracks: the track row no longer exists. + CREATE TABLE IF NOT EXISTS pending_file_deletions ( + path TEXT PRIMARY KEY, + track_id UUID, + track_title TEXT, + track_artist TEXT, + requested_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + attempts INTEGER NOT NULL DEFAULT 0, + last_attempt_at TIMESTAMP, + last_error TEXT + ); + CREATE INDEX IF NOT EXISTS idx_pending_file_deletions_requested + ON pending_file_deletions(requested_at); + `, + }, ]; export class DbService { @@ -1066,42 +1116,13 @@ export class DbService { return (res.rows[0] as DislikeEntry) || null; } - /** - * Permanently delete a track: remove from DB (cascades to related tables) - * and delete the physical file. Used by the cleanup_sweep worker after the - * full lifecycle (grace period + 24h warning) has elapsed. - */ - async hardDeleteTrack(userId: string, trackId: string, filePath: string): Promise { - const { unlink } = await import('fs/promises'); - - // Log the permanent deletion feedback event first (before the track is gone) - await this.pgClient.query( - "INSERT INTO feedback (user_id, track_id, action) VALUES ($1, $2, 'deleted_permanent')", - [userId, trackId] - ); - - // Write evidence: manual_deleted → negative profile (strongest negative - // signal, spec §B.3). entity_id has no FK to tracks, so the row survives. - await this.recordEvidence({ - user_id: userId, - entity_type: 'track', - entity_id: trackId, - signal: 'manual_deleted', - profile: 'negative', - weight: -0.90, - }); - - // Delete DB record (ON DELETE CASCADE handles track_genre, play_history, - // feedback, track_audio_features, track_lyrics) - await this.pgClient.query('DELETE FROM tracks WHERE id = $1', [trackId]); - - // Delete physical file from disk - try { - await unlink(filePath); - } catch { - // File may already be gone (e.g. missing). Log but don't abort. - } - } + // NOTE: hardDeleteTrack() used to live here as a second, unreferenced deletion + // path with the same insert-then-delete shape as permanentlyDeleteTrack(), but + // with the audit insert outside any transaction and the unlink after the DELETE + // and silently swallowed. It had no callers. Removed rather than fixed twice + // over: there is now exactly one deletion path in the backend + // (permanentlyDeleteTrack, below) and exactly one unlink site in the whole + // system (workers/src/cleanup.service.ts). async recordPlay(userId: string, trackId: string, completed: boolean, batchId?: string): Promise { if (!completed) { @@ -1423,26 +1444,67 @@ export class DbService { } /** - * Permanently delete a disliked track: remove the file from disk, then cascade- - * delete the DB record (which also removes the dislikes row via ON DELETE CASCADE). - * Logs a 'deleted_permanent' feedback event. Atomic: DB deletion only happens after - * the file is gone (or the file was already missing). + * Permanently delete a disliked track, skipping the grace period. Cascade- + * deletes the DB record (which also removes the dislikes row), writes the + * 'deleted_permanent' audit row, and hands the file itself to the worker. + * + * This method does NOT unlink. The backend mounts /music `:ro` on purpose — + * nothing reachable from an HTTP request may write to the library — so the + * unlink is queued as a pending_file_deletions row and performed by the + * cleanup sweep in the worker, the one process with an rw mount, and only when + * MUZICK_ALLOW_HARD_DELETE is on. Until then the row is visible evidence of an + * orphaned file rather than a lost one. + * + * Ordering: everything durable commits in one transaction, and the + * irreversible filesystem act happens strictly afterwards. The previous + * implementation unlinked first, which on this deployment meant EROFS and a + * 500 with the track still present. + * + * `filePath` is accepted for call-site compatibility but the path is re-read + * from the row under the transaction, so the queued deletion can never target + * a stale or caller-supplied path. */ - async permanentlyDeleteTrack(userId: string, trackId: string, filePath: string): Promise { - try { - await unlink(filePath); - } catch (err: any) { - if (err.code !== 'ENOENT') throw err; - } - + async permanentlyDeleteTrack(userId: string, trackId: string, _filePath?: string): Promise { await this.withTransaction(async (client) => { - await client.query( - "INSERT INTO feedback (user_id, track_id, action) VALUES ($1, $2, 'deleted_permanent')", - [userId, trackId] + const trackRes = await client.query( + 'SELECT path, title, artist FROM tracks WHERE id = $1 FOR UPDATE', + [trackId] ); - // CASCADE deletes dislikes, play_history, feedback, etc. + const track = trackRes.rows[0] as + | { path: string; title: string | null; artist: string | null } + | undefined; + if (!track) return; // Already gone; nothing to audit or unlink. + + // Denormalised identity: track_id is ON DELETE SET NULL, so without these + // the surviving audit row would not say which track was destroyed. + await client.query( + `INSERT INTO feedback (user_id, track_id, action, track_path, track_title, track_artist) + VALUES ($1, $2, 'deleted_permanent', $3, $4, $5)`, + [userId, trackId, track.path, track.title, track.artist] + ); + + await client.query( + `INSERT INTO pending_file_deletions (path, track_id, track_title, track_artist) + VALUES ($1, $2, $3, $4) + ON CONFLICT (path) DO UPDATE SET requested_at = NOW()`, + [track.path, trackId, track.title, track.artist] + ); + + // CASCADE deletes dislikes, play_history, track_genre, etc. await client.query('DELETE FROM tracks WHERE id = $1', [trackId]); }); + + // Strongest negative signal (spec §B.3). entity_id has no FK to tracks, so + // the row survives the deletion. Outside the transaction: a failure here + // must not resurrect an already-audited deletion. + await this.recordEvidence({ + user_id: userId, + entity_type: 'track', + entity_id: trackId, + signal: 'manual_deleted', + profile: 'negative', + weight: -0.90, + }); } /** @@ -1526,29 +1588,36 @@ export class DbService { * remaining files from disk, then hard-delete the rest. All inside a * transaction so a partial failure leaves no orphans. * - * File deletion happens BEFORE the transaction (best-effort: we delete the - * file, then commit the DB changes; if the DB commit fails the file is - * already gone but the track stays orphaned in the DB — a subsequent - * integrity sweep will catch it and mark it MISSING). + * Like permanentlyDeleteTrack(), this no longer unlinks: the loser files are + * queued into pending_file_deletions inside the same transaction and removed + * later by the worker's gated cleanup sweep. Previously the unlink ran BEFORE + * the transaction and rethrew anything other than ENOENT, so on this + * deployment (/music mounted `:ro` in the backend) every dedup merge failed + * with EROFS before touching the DB at all. */ async mergeDuplicates(keepId: string, deleteIds: string[]): Promise { - // 1. Fetch paths for the tracks being deleted (before they're gone). - const { rows: losers } = await this.pgClient.query<{ id: string; path: string }>( - `SELECT id, path FROM tracks WHERE id = ANY($1::uuid[])`, + // 1. Fetch paths + identity for the tracks being deleted (before they go). + const { rows: losers } = await this.pgClient.query<{ + id: string; + path: string; + title: string | null; + artist: string | null; + }>( + `SELECT id, path, title, artist FROM tracks WHERE id = ANY($1::uuid[])`, [deleteIds] ); - // 2. Delete files from disk (best-effort — tolerate missing files). - for (const row of losers) { - try { - await unlink(row.path); - } catch (err: any) { - if (err.code !== 'ENOENT') throw err; - } - } - - // 3. DB transaction: re-parent history + delete rows. + // 2. DB transaction: queue the file removals, re-parent history, delete rows. await this.withTransaction(async (client) => { + for (const row of losers) { + await client.query( + `INSERT INTO pending_file_deletions (path, track_id, track_title, track_artist) + VALUES ($1, $2, $3, $4) + ON CONFLICT (path) DO UPDATE SET requested_at = NOW()`, + [row.path, row.id, row.title, row.artist] + ); + } + for (const id of deleteIds) { await client.query( `UPDATE play_history SET track_id = $1 WHERE track_id = $2`, diff --git a/docker-compose.yml b/docker-compose.yml index ac0fe8a..8d21a38 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -69,8 +69,23 @@ services: DISCOGS_TOKEN: ${DISCOGS_TOKEN} SOCKS_PROXY_URL: ${SOCKS_PROXY_URL} MUSIC_DIR: /music + # Hard-deletion gates for the dislike lifecycle (invariant §C). All three + # default to the safe value inside cleanup.service.ts; they are listed here + # as documentation and are intentionally left unset. + # + # MASTER SWITCH — leave unset/false. While off, the cleanup sweep logs + # exactly which files it WOULD delete and changes nothing at all. Set to + # true by hand, only after reviewing a dry-run log: + # MUZICK_ALLOW_HARD_DELETE: "false" + # Blast-radius cap per sweep (default 5): + # MUZICK_HARD_DELETE_MAX_PER_SWEEP: "5" + # Days a track must sit in HIDDEN before its file is eligible (default 7): + # MUZICK_HARD_DELETE_GRACE_DAYS: "7" volumes: - - /mnt/hdd1/media/Music:/music:ro + # READ-WRITE, and the only rw mount of the library in the stack. The worker + # is the sole process permitted to unlink a music file, and only via the + # gated cleanup sweep. The backend keeps `:ro`. + - /mnt/hdd1/media/Music:/music:rw networks: infra-net: diff --git a/docs/architecture/02-invariants-and-risks.md b/docs/architecture/02-invariants-and-risks.md index 6add01d..d57b953 100644 --- a/docs/architecture/02-invariants-and-risks.md +++ b/docs/architecture/02-invariants-and-risks.md @@ -14,7 +14,31 @@ This document outlines the critical rules that MUST be respected to maintain sys ### **C. Filesystem Safety (The "Irreversible Action" Rule)** * **Invariant:** Hard deletion of a file from the filesystem is the final, irreversible step in the `PENDING_REMOVAL` lifecycle. -* **Mechanism:** A track only enters the `DELETE_FILE` state after it has been `warned` for at least 24 hours and the user has not explicitly triggered a `RESTORE`. +* **Invariant:** Exactly **one** process may unlink a library file, and exactly **one** code path may do it: the worker's cleanup sweep (`workers/src/cleanup.service.ts`). Nothing reachable from an HTTP request may write to the library. + +#### Implemented behaviour + +**Mount.** Only the `worker` service binds `/mnt/hdd1/media/Music` read-write. `backend` keeps `:ro`, so an API bug or a compromised request handler physically cannot remove a file. The backend's two deletion entry points (`permanentlyDeleteTrack`, the Quarantine immediate-delete endpoint; and `mergeDuplicates`, the admin dedup merge) therefore do **not** unlink — they commit the DB side and queue the path into `pending_file_deletions` for the worker to carry out under the gates below. + +**Ordering.** The DB transaction commits *first*, the unlink happens *after*: + +1. Select eligible candidates (all gates applied in SQL). +2. In one transaction: write the `deleted_permanent` audit row, insert a `pending_file_deletions` marker, `DELETE FROM tracks`. Commit. +3. `unlink()`. On success, clear the marker. On failure, log at **error** and increment `attempts` / record `last_error` on the marker. + +A marker that outlives its sweep is the durable record of a file whose DB row is gone but whose bytes are not — from a failed unlink or a worker killed mid-sweep. The next sweep retries it. Failures are never swallowed. + +**Audit row.** `feedback.track_id` is `ON DELETE SET NULL`, so the audit row survives its track but loses its identity. Deletion rows therefore carry denormalised `track_path`, `track_title`, `track_artist`. This row is the only forensic record that the file ever existed. + +**Gates.** All three default to the safe value; none is set to an unsafe value anywhere in the repo. + +| Env var | Default | Effect | +|---|---|---| +| `MUZICK_ALLOW_HARD_DELETE` | **off** | Master switch. While off the sweep runs its full selection logic and logs exactly which files it *would* delete, performs no unlink, and makes **no** DB write of any kind — not the audit row, not the terminal-state transition. The owner enables it by hand after reviewing a dry-run log. | +| `MUZICK_HARD_DELETE_MAX_PER_SWEEP` | `5` | Blast-radius cap. On hitting it the sweep stops, logs at error, and leaves the remainder for the next run, so a selection bug cannot empty the library in one pass. | +| `MUZICK_HARD_DELETE_GRACE_DAYS` | `7` | Minimum age since the track entered `HIDDEN`, measured from `dislikes.hidden_at` (an explicit column — an irreversible clock must not be inferred). Separate from, and much longer than, `dislikes.grace_hours` (default 48), which only governs `HIDDEN` → `WARNED`. | + +* **Mechanism:** A track's file is deleted only when *all* of: 24h have passed since `warned_at`; `MUZICK_HARD_DELETE_GRACE_DAYS` have passed since `hidden_at`; the user has not triggered a `RESTORE` (which deletes the dislike row outright); the per-sweep cap has not been reached; and `MUZICK_ALLOW_HARD_DELETE` is on. ## 2. Known Technical Risks diff --git a/workers/src/cleanup.service.ts b/workers/src/cleanup.service.ts index ff45d69..86c7e91 100644 --- a/workers/src/cleanup.service.ts +++ b/workers/src/cleanup.service.ts @@ -1,10 +1,73 @@ import type { Pool } from 'pg'; import { unlink } from 'fs/promises'; +import { withTransaction } from './db.js'; const NTFY_URL = process.env.NTFY_URL || ''; const NTFY_TOPIC = process.env.NTFY_TOPIC || 'muzick'; const SYSTEM_USER_ID = '00000000-0000-0000-0000-000000000000'; +/** + * Hard deletion is IRREVERSIBLE and destroys the owner's music files. Three + * independent gates stand in front of it, all env-configurable, all defaulting + * to the safe value: + * + * - MUZICK_ALLOW_HARD_DELETE (default false) — the master switch. While it is + * off the sweep runs its full selection logic and logs exactly what it WOULD + * delete, but performs no unlink and makes no transition to the terminal + * state. This is the default mode of operation; the owner enables it by hand + * after reading a dry-run log. It is not set to true anywhere in the repo. + * - MUZICK_HARD_DELETE_MAX_PER_SWEEP (default 5) — blast-radius cap. On hitting + * it the sweep stops, logs at error, and leaves the remainder for next time, + * so a selection bug cannot empty the library in one run. + * - MUZICK_HARD_DELETE_GRACE_DAYS (default 7) — minimum age since the track + * entered HIDDEN (dislikes.hidden_at). Separate from, and much longer than, + * dislikes.grace_hours (default 48), which only governs HIDDEN -> WARNED. + * + * Note that only the worker container mounts /music read-write; the backend + * keeps `:ro`, so nothing in the API request path can unlink a library file. + */ +function envFlag(name: string): boolean { + const raw = (process.env[name] || '').trim().toLowerCase(); + return raw === '1' || raw === 'true' || raw === 'yes'; +} + +function envInt(name: string, fallback: number, min: number): number { + const raw = (process.env[name] || '').trim(); + if (!raw) return fallback; + const n = Number(raw); + if (!Number.isFinite(n) || !Number.isInteger(n) || n < min) { + console.error( + `[Cleanup] Ignoring invalid ${name}=${JSON.stringify(raw)}; using ${fallback}` + ); + return fallback; + } + return n; +} + +export const HARD_DELETE_DEFAULTS = { + /** MUZICK_ALLOW_HARD_DELETE */ + allow: false, + /** MUZICK_HARD_DELETE_MAX_PER_SWEEP */ + maxPerSweep: 5, + /** MUZICK_HARD_DELETE_GRACE_DAYS */ + graceDays: 7, +} as const; + +interface HardDeleteConfig { + allow: boolean; + maxPerSweep: number; + graceDays: number; +} + +export function readHardDeleteConfig(): HardDeleteConfig { + return { + // Default OFF. Anything other than an explicit truthy opt-in is a dry run. + allow: envFlag('MUZICK_ALLOW_HARD_DELETE'), + maxPerSweep: envInt('MUZICK_HARD_DELETE_MAX_PER_SWEEP', HARD_DELETE_DEFAULTS.maxPerSweep, 1), + graceDays: envInt('MUZICK_HARD_DELETE_GRACE_DAYS', HARD_DELETE_DEFAULTS.graceDays, 1), + }; +} + async function sendNtfy(title: string, message: string): Promise { if (!NTFY_URL) return; try { @@ -18,20 +81,51 @@ async function sendNtfy(title: string, message: string): Promise { } } -export class CleanupSweepService { - constructor(private pgClient: Pool) {} +interface Candidate { + track_id: string; + track_path: string; + track_title: string | null; + track_artist: string | null; +} - async runSweep(): Promise<{ warned: number; deleted: number }> { +export interface SweepResult { + warned: number; + /** Files actually unlinked and DB rows actually deleted. Always 0 in dry-run. */ + deleted: number; + /** + * Eligible candidates seen. Selection stops at `maxPerSweep + 1` rows, so this + * saturates at cap+1 — read `eligible > maxPerSweep` as "more remain". + */ + eligible: number; + /** True when MUZICK_ALLOW_HARD_DELETE was off, i.e. nothing was destroyed. */ + dryRun: boolean; + /** Unlinks that failed and were left recorded in pending_file_deletions. */ + failed: number; + /** Previously-failed/interrupted unlinks reaped at the start of this sweep. */ + reaped: number; +} + +export class CleanupSweepService { + // A Pool, not a Client: finalizeDeleted() runs a real transaction and must + // check out a dedicated connection for it. The worker runs jobs with + // `concurrency: 10`, so a BEGIN on a shared connection would enrol other + // jobs' queries in this transaction and discard them on ROLLBACK. + constructor(private pool: Pool) {} + + async runSweep(): Promise { + const config = readHardDeleteConfig(); const warned = await this.advanceToWarned(); - const deleted = await this.finalizeDeleted(); - return { warned, deleted }; + const reaped = await this.reapPendingFileDeletions(config); + const finalized = await this.finalizeDeleted(config); + return { warned, reaped, ...finalized }; } /** * Find HIDDEN dislikes past their grace period → send ntfy warning, mark WARNED. + * Not gated: this transition destroys nothing. */ private async advanceToWarned(): Promise { - const res = await this.pgClient.query<{ track_id: string; track_title: string; track_artist: string }>( + const res = await this.pool.query<{ track_id: string; track_title: string; track_artist: string }>( `SELECT d.track_id, t.title AS track_title, t.artist AS track_artist FROM dislikes d JOIN tracks t ON t.id = d.track_id @@ -40,7 +134,7 @@ export class CleanupSweepService { ); for (const row of res.rows) { - await this.pgClient.query( + await this.pool.query( `UPDATE dislikes SET warned_at = NOW(), state = 'WARNED' WHERE track_id = $1`, [row.track_id] ); @@ -55,44 +149,178 @@ export class CleanupSweepService { } /** - * Find WARNED dislikes where 24h has passed since warned_at → delete file + DB record. + * Retry files whose DB row is already gone but whose bytes survive — an unlink + * that failed, or a worker that died between COMMIT and unlink. Subject to the + * same gates as a fresh deletion. */ - private async finalizeDeleted(): Promise { - const res = await this.pgClient.query<{ track_id: string; track_path: string; track_title: string }>( - `SELECT d.track_id, t.path AS track_path, t.title AS track_title + private async reapPendingFileDeletions(config: HardDeleteConfig): Promise { + const res = await this.pool.query<{ path: string; track_id: string | null; attempts: number }>( + `SELECT path, track_id, attempts + FROM pending_file_deletions + ORDER BY requested_at ASC + LIMIT $1`, + [config.maxPerSweep] + ); + if (res.rows.length === 0) return 0; + + if (!config.allow) { + for (const row of res.rows) { + console.warn( + `[Cleanup] DRY RUN (MUZICK_ALLOW_HARD_DELETE off): orphaned file left on disk ` + + `after ${row.attempts} attempt(s): ${row.path}` + ); + } + return 0; + } + + let reaped = 0; + for (const row of res.rows) { + if (await this.unlinkAndSettle(row.path, row.track_id)) reaped++; + } + return reaped; + } + + /** + * Find WARNED dislikes eligible for hard deletion and, unless dry-run, destroy + * them: DB transaction commits FIRST, then the file is unlinked. + * + * The old implementation unlinked before the transaction and `continue`d on + * failure, which is backwards on both counts: the irreversible act happened + * before anything durable recorded it, and a failure left no trace beyond a log + * line. Now the commit records the intent (audit row + pending_file_deletions + * marker) and the unlink either clears the marker or annotates it for retry. + * A crash in that window leaves the marker, which the next sweep reaps. + */ + private async finalizeDeleted( + config: HardDeleteConfig + ): Promise<{ deleted: number; eligible: number; dryRun: boolean; failed: number }> { + // Ask for one more than the cap purely to detect overflow. + const res = await this.pool.query( + `SELECT d.track_id, t.path AS track_path, + t.title AS track_title, t.artist AS track_artist FROM dislikes d JOIN tracks t ON t.id = d.track_id WHERE d.state = 'WARNED' - AND NOW() > d.warned_at + INTERVAL '24 hours'` + AND NOW() > d.warned_at + INTERVAL '24 hours' + AND NOW() > COALESCE(d.hidden_at, d.disliked_at) + ($1 || ' days')::interval + ORDER BY d.warned_at ASC + LIMIT $2`, + [config.graceDays, config.maxPerSweep + 1] ); + const eligible = res.rows.length; + if (eligible === 0) { + return { deleted: 0, eligible: 0, dryRun: !config.allow, failed: 0 }; + } + + let batch = res.rows; + if (batch.length > config.maxPerSweep) { + batch = batch.slice(0, config.maxPerSweep); + console.error( + `[Cleanup] Per-sweep deletion cap reached (${config.maxPerSweep}, ` + + `MUZICK_HARD_DELETE_MAX_PER_SWEEP). Stopping after ${batch.length}; ` + + `remaining eligible tracks are left for the next sweep.` + ); + } + + if (!config.allow) { + // Dry run: log the selection and return. No unlink, no state transition, + // no audit row — this branch performs no writes of any kind. + console.warn( + `[Cleanup] DRY RUN: MUZICK_ALLOW_HARD_DELETE is off. ` + + `${batch.length} file(s) WOULD be permanently deleted ` + + `(grace=${config.graceDays}d, cap=${config.maxPerSweep}). Nothing was changed.` + ); + for (const row of batch) { + console.warn( + `[Cleanup] DRY RUN would delete: ${row.track_path} ` + + `("${row.track_title ?? '?'}" by ${row.track_artist ?? '?'}, track ${row.track_id})` + ); + } + return { deleted: 0, eligible, dryRun: true, failed: 0 }; + } + let deleted = 0; - for (const row of res.rows) { + let failed = 0; + for (const row of batch) { try { - await unlink(row.track_path); - } catch (err: any) { - if (err.code !== 'ENOENT') { - console.error(`[Cleanup] Failed to delete file ${row.track_path}:`, err); - continue; - } + // Step 1: commit the DB side. The audit row carries the denormalised + // path/title/artist because feedback.track_id becomes NULL the moment + // the track is deleted (ON DELETE SET NULL) — this row is the only + // forensic record that this file ever existed. + await withTransaction(this.pool, async (client) => { + await client.query( + `INSERT INTO feedback (user_id, track_id, action, track_path, track_title, track_artist) + VALUES ($1, $2, 'deleted_permanent', $3, $4, $5)`, + [SYSTEM_USER_ID, row.track_id, row.track_path, row.track_title, row.track_artist] + ); + await client.query( + `INSERT INTO pending_file_deletions (path, track_id, track_title, track_artist) + VALUES ($1, $2, $3, $4) + ON CONFLICT (path) DO UPDATE SET requested_at = NOW()`, + [row.track_path, row.track_id, row.track_title, row.track_artist] + ); + await client.query('DELETE FROM tracks WHERE id = $1', [row.track_id]); + }); + } catch (err) { + // withTransaction already rolled back and released the connection. + // Nothing was committed and no file was touched: safe to skip. + console.error(`[Cleanup] DB deletion failed for ${row.track_id}, file untouched:`, err); + failed++; + continue; } - try { - await this.pgClient.query('BEGIN'); - await this.pgClient.query( - `INSERT INTO feedback (user_id, track_id, action) VALUES ($1, $2, 'deleted_permanent')`, - [SYSTEM_USER_ID, row.track_id] - ); - await this.pgClient.query('DELETE FROM tracks WHERE id = $1', [row.track_id]); - await this.pgClient.query('COMMIT'); + // Step 2: the irreversible part, now that it is durably recorded. + if (await this.unlinkAndSettle(row.track_path, row.track_id)) { deleted++; console.log(`[Cleanup] Permanently deleted: ${row.track_title} (${row.track_id})`); - } catch (err) { - await this.pgClient.query('ROLLBACK'); - console.error(`[Cleanup] DB deletion failed for ${row.track_id}:`, err); + } else { + failed++; } } - return deleted; + return { deleted, eligible, dryRun: false, failed }; + } + + /** + * Unlink one file and settle its pending_file_deletions marker: clear it on + * success (or ENOENT — the file is gone either way, which is the desired end + * state), annotate it on any other error so the failure is durable and + * retryable instead of swallowed. Returns true if the file is gone. + * + * Callers must only reach this once the DB side is committed. + */ + private async unlinkAndSettle(path: string, trackId: string | null): Promise { + try { + await unlink(path); + } catch (err: any) { + if (err?.code !== 'ENOENT') { + console.error( + `[Cleanup] Unlink FAILED for ${path} (track ${trackId ?? 'unknown'}); ` + + `DB row already deleted, file left on disk and recorded in ` + + `pending_file_deletions for retry:`, + err + ); + try { + await this.pool.query( + `UPDATE pending_file_deletions + SET attempts = attempts + 1, last_attempt_at = NOW(), last_error = $2 + WHERE path = $1`, + [path, String(err?.message || err?.code || err)] + ); + } catch (markerErr) { + console.error(`[Cleanup] Could not record unlink failure for ${path}:`, markerErr); + } + return false; + } + } + + try { + await this.pool.query('DELETE FROM pending_file_deletions WHERE path = $1', [path]); + } catch (markerErr) { + // The file is gone; a stale marker only causes a harmless ENOENT retry. + console.error(`[Cleanup] Could not clear deletion marker for ${path}:`, markerErr); + } + return true; } } diff --git a/workers/src/index.ts b/workers/src/index.ts index 1a3f61f..1fa917e 100644 --- a/workers/src/index.ts +++ b/workers/src/index.ts @@ -126,7 +126,11 @@ async function initWorker() { console.log('[Cleanup] Starting dislike cleanup sweep'); const cleanupService = new CleanupSweepService(pgPool); const result = await cleanupService.runSweep(); - console.log(`[Cleanup] Sweep complete: warned=${result.warned} deleted=${result.deleted}`); + console.log( + `[Cleanup] Sweep complete: warned=${result.warned} eligible=${result.eligible} ` + + `deleted=${result.deleted} failed=${result.failed} reaped=${result.reaped}` + + (result.dryRun ? ' (DRY RUN — MUZICK_ALLOW_HARD_DELETE off, nothing deleted)' : '') + ); break; } case 'vibe_reap': {