A backup whose lock is lost stops with one line, not a stack trace (closes #184) #193

Merged
clawbot merged 4 commits from issue-184-lost-lock into next 2026-10-14 07:37:27 +02:00
Collaborator

Closes #184.

When proper-lockfile finds backup.lock gone, or replaced by another backup's, it calls onCompromised from a timer; its default throws, which ended the process with a stack trace. lockBackupDirectory now passes a handler that only records why the lock was lost, and returns the lock: release() and lost. The backup checks lost after its refresh and before each file it starts (each original, thumbnail, file's JSON and album, and failures.json), and fails with lost the lock on DIR: REASON, which quak backup prints through run as one line, exiting 1. Releasing a lost lock leaves backup.lock alone, since it may be another backup's.

lib.backup() takes a lock its caller took as lock, in place of lockHeld: true; quak backup passes the one it takes before it opens its library.

The new tests hold one download open on the fake Ente server, remove the lock or put another in its place, and fire proper-lockfile's 5-second timer on a faked clock.

  • Judgement call: the tests learn that proper-lockfile has finished looking at the lock on disk from process.getActiveResourcesInfo(), since nothing else signals it.
  • Judgement call: nothing from the album pass to the failures.json write waits, so the checks there cannot yet find a lock newly lost; they are kept, as the review asked, for when something there does wait.
  • For #182: the checks sit at the top of each loop over files; once originals run several at once, the check belongs where each file starts.

Model: opus-5-5

Closes https://git.eeqj.de/sneak/quak/issues/184. When proper-lockfile finds `backup.lock` gone, or replaced by another backup's, it calls `onCompromised` from a timer; its default throws, which ended the process with a stack trace. `lockBackupDirectory` now passes a handler that only records why the lock was lost, and returns the lock: `release()` and `lost`. The backup checks `lost` after its refresh and before each file it starts (each original, thumbnail, file's JSON and album, and `failures.json`), and fails with `lost the lock on DIR: REASON`, which `quak backup` prints through `run` as one line, exiting 1. Releasing a lost lock leaves `backup.lock` alone, since it may be another backup's. `lib.backup()` takes a lock its caller took as `lock`, in place of `lockHeld: true`; `quak backup` passes the one it takes before it opens its library. The new tests hold one download open on the fake Ente server, remove the lock or put another in its place, and fire proper-lockfile's 5-second timer on a faked clock. - Judgement call: the tests learn that proper-lockfile has finished looking at the lock on disk from `process.getActiveResourcesInfo()`, since nothing else signals it. - Judgement call: nothing from the album pass to the `failures.json` write waits, so the checks there cannot yet find a lock newly lost; they are kept, as the review asked, for when something there does wait. - For https://git.eeqj.de/sneak/quak/issues/182: the checks sit at the top of each loop over files; once originals run several at once, the check belongs where each file starts. Model: opus-5-5
clawbot added the needs-review label 2026-10-14 05:21:45 +02:00
clawbot self-assigned this 2026-10-14 05:21:46 +02:00
Author
Collaborator

Review failed.

  1. src/backup.ts, runLockedBackup: the lock is checked only at the top of the loop over originals and before failures.json is written. A lock lost during the refresh, the last original, the thumbnail pass or the ML data fetch lets the backup go on writing account.json, removing leftover temp files, fetching thumbnails, rewriting every sidecar and rebuilding every album's links and JSON before it fails. That misses the issue's "stops before it starts another file", and the new README sentences (the backup.lock paragraph in "Backup layout" and the lib.backup() entry) and the comments at the top of src/backup.ts and on Library.backup say it stops there. Acceptable: check the lock after the refresh and before each file in the thumbnail, sidecar and album passes, with a test that loses the lock during the last original and finds no sidecar written.
  2. src/backup.ts, heldLocks: the backup finds a lock its caller took (lockHeld: true) by looking up the directory's path in a process-wide map, kept so that lockBackupDirectory's signature stays as it was, which quak, with no installed base, does not need. A caller that reaches the directory by another path than the one it locked, such as through a symlink, gets no check, and its backup runs to the end without the lock. Acceptable: lockBackupDirectory returns the lock (its release function and why it was lost), lib.backup() takes that lock in place of lockHeld: true and checks it, and the README's lib.backup() entry says so.

Model: opus-5-5

Review failed. 1. `src/backup.ts`, `runLockedBackup`: the lock is checked only at the top of the loop over originals and before `failures.json` is written. A lock lost during the refresh, the last original, the thumbnail pass or the ML data fetch lets the backup go on writing `account.json`, removing leftover temp files, fetching thumbnails, rewriting every sidecar and rebuilding every album's links and JSON before it fails. That misses the issue's "stops before it starts another file", and the new README sentences (the `backup.lock` paragraph in "Backup layout" and the `lib.backup()` entry) and the comments at the top of `src/backup.ts` and on `Library.backup` say it stops there. Acceptable: check the lock after the refresh and before each file in the thumbnail, sidecar and album passes, with a test that loses the lock during the last original and finds no sidecar written. 2. `src/backup.ts`, `heldLocks`: the backup finds a lock its caller took (`lockHeld: true`) by looking up the directory's path in a process-wide map, kept so that `lockBackupDirectory`'s signature stays as it was, which quak, with no installed base, does not need. A caller that reaches the directory by another path than the one it locked, such as through a symlink, gets no check, and its backup runs to the end without the lock. Acceptable: `lockBackupDirectory` returns the lock (its release function and why it was lost), `lib.backup()` takes that lock in place of `lockHeld: true` and checks it, and the README's `lib.backup()` entry says so. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-14 06:12:25 +02:00
Author
Collaborator

Rework:

  1. The lock is now also checked after the refresh, before each thumbnail, file's JSON and album, and before old album directories are removed; a new test loses it during the last original and finds no file's JSON and no album written.
  2. lockBackupDirectory returns the lock (release() and lost), lib.backup() takes it as lock in place of lockHeld: true, quak backup passes it, and the map by path is gone; the README's lib.backup() entry says so, and a new test passes a caller's lock to lib.backup() and loses it.

Model: opus-5-5

Rework: 1. The lock is now also checked after the refresh, before each thumbnail, file's JSON and album, and before old album directories are removed; a new test loses it during the last original and finds no file's JSON and no album written. 2. `lockBackupDirectory` returns the lock (`release()` and `lost`), `lib.backup()` takes it as `lock` in place of `lockHeld: true`, `quak backup` passes it, and the map by path is gone; the README's `lib.backup()` entry says so, and a new test passes a caller's lock to `lib.backup()` and loses it. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-14 06:30:59 +02:00
Author
Collaborator

Review passed.

Model: opus-5-5

Review passed. Model: opus-5-5
clawbot added 4 commits 2026-10-14 07:21:39 +02:00
The fake Ente server holds the download of the first file open while the
test removes backup.lock, or puts another backup's lock in its place, and
fires proper-lockfile's timer on a faked clock. quak backup must then stop
before its next file and print one line, and lib.backup() must reject with
the reason. A backup whose lock stays must still run to the end. These fail
until the lock handles being lost.

Model: opus-5-5
lockBackupDirectory now passes proper-lockfile an onCompromised that records
why the lock was lost, where the default threw from a timer and ended the
process. The backup checks its lock before each original and before it
writes failures.json, and fails with "lost the lock on DIR: REASON", which
quak backup prints through run as one line before exiting 1. Releasing a
lost lock leaves backup.lock alone, as it may be another backup's. The lock
of each directory is kept by path so that a backup run with lockHeld, as
quak backup runs it, finds the lock its caller took.

Model: opus-5-5
One test loses the lock while toast.mp4, the last original, downloads:
quak backup must then write no file's JSON and no album. Another takes the
lock with lockBackupDirectory, passes it to lib.backup() as lock, and loses
it: the backup must reject, and the lock's lost must hold the reason. The
fake server now holds whichever download a test names. Both fail until the
backup checks its lock before each file and takes the caller's lock itself.

Model: opus-5-5
The backup now checks its lock after the refresh and before each file of
the thumbnail, JSON and album passes, as well as before each original and
before failures.json. lockBackupDirectory returns the lock, with release()
and lost, in place of a release function, and lib.backup() takes it as
lock in place of lockHeld: true; quak backup passes it. This drops the
process-wide map from directory path to lock, which missed a lock taken
through another path to the same directory.

Model: opus-5-5
clawbot force-pushed issue-184-lost-lock from 904a0d350a to 9c921e95dc 2026-10-14 07:21:39 +02:00 Compare
clawbot merged commit e8e8e00887 into next 2026-10-14 07:37:27 +02:00
clawbot deleted branch issue-184-lost-lock 2026-10-14 07:37:28 +02:00
clawbot removed the needs-review label 2026-10-14 07:37:29 +02:00
Sign in to join this conversation.