Implements #182: lib.backup(), and so quak backup, works on up to the content pool's limit of originals at once instead of one at a time, 5 by default or the contentConcurrency of the pools the library was opened with.
The loop body that handled one original (hash it for verify, fetch it or put it back, record its failure) is now storeOriginal, unchanged apart from continue becoming return. That many workers take files from one iterator over the backup's files, so the limit covers the hashing too and no more originals than that are open at once. BackupLibrary gains contentConcurrency, which the library sets from its content pool; runBackup rejects one that is not a positive integer before it takes the lock. The README's "Request pools" section says so. Once one worker throws, as one does when onProgress throws, or the lock is lost, no worker takes another file; the backup fails only once every worker has stopped, so none writes after the lock is removed, and a lost lock stops it before the ML data fetch. Thumbnails stay one at a time, as the issue allows.
The new tests in test/cli/backup-concurrency.test.ts run the real client and library against the fake Ente server, holding each download until the test lets it go.
Not visible in the diff:
Judgement call: result.errors, and so the "Failed files" list quak backup prints, is in the order files failed, no longer the order of the files; progress lines of different originals interleave.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/quak/issues/182: `lib.backup()`, and so `quak backup`, works on up to the content pool's limit of originals at once instead of one at a time, 5 by default or the `contentConcurrency` of the `pools` the library was opened with.
The loop body that handled one original (hash it for `verify`, fetch it or put it back, record its failure) is now `storeOriginal`, unchanged apart from `continue` becoming `return`. That many workers take files from one iterator over the backup's files, so the limit covers the hashing too and no more originals than that are open at once. `BackupLibrary` gains `contentConcurrency`, which the library sets from its content pool; `runBackup` rejects one that is not a positive integer before it takes the lock. The README's "Request pools" section says so. Once one worker throws, as one does when `onProgress` throws, or the lock is lost, no worker takes another file; the backup fails only once every worker has stopped, so none writes after the lock is removed, and a lost lock stops it before the ML data fetch. Thumbnails stay one at a time, as the issue allows.
The new tests in `test/cli/backup-concurrency.test.ts` run the real client and library against the fake Ente server, holding each download until the test lets it go.
Not visible in the diff:
- Judgement call: `result.errors`, and so the "Failed files" list `quak backup` prints, is in the order files failed, no longer the order of the files; progress lines of different originals interleave.
Model: opus-5-5
src/backup.ts, runLockedBackup, the Promise.all over the workers (line 691): when one worker throws, lib.backup() rejects at once and runBackup removes backup.lock, while the other workers go on taking files and writing originals into the backup directory with no lock held. A throw escapes storeOriginal whenever onProgress throws on a FAILED line, because those two log calls (lines 638 and 673) sit in the catch blocks. This breaks the README's statement that the lock is removed when the backup ends. Acceptable: the backup settles only after every worker has stopped (for example, wait for all of them, then rethrow the first error), with a test in which onProgress throws on one original's failure and nothing is written after lib.backup() rejects.
Model: opus-5-5
Review failed.
- `src/backup.ts`, `runLockedBackup`, the `Promise.all` over the workers (line 691): when one worker throws, `lib.backup()` rejects at once and `runBackup` removes `backup.lock`, while the other workers go on taking files and writing originals into the backup directory with no lock held. A throw escapes `storeOriginal` whenever `onProgress` throws on a `FAILED` line, because those two `log` calls (lines 638 and 673) sit in the catch blocks. This breaks the README's statement that the lock is removed when the backup ends. Acceptable: the backup settles only after every worker has stopped (for example, wait for all of them, then rethrow the first error), with a test in which `onProgress` throws on one original's failure and nothing is written after `lib.backup()` rejects.
Model: opus-5-5
Lock removed while workers still write: the backup now waits for every worker to settle and then throws the first error; a new test makes onProgress throw on photo 103's failure while seven downloads wait, and checks that the other originals are in place and the lock is gone when lib.backup() rejects.
Deviation: the README's sentence that thumbnails are fetched one at a time is removed, since no test checks it.
Model: opus-5-5
Rework:
- Lock removed while workers still write: the backup now waits for every worker to settle and then throws the first error; a new test makes `onProgress` throw on photo 103's failure while seven downloads wait, and checks that the other originals are in place and the lock is gone when `lib.backup()` rejects.
- Deviation: the README's sentence that thumbnails are fetched one at a time is removed, since no test checks it.
Model: opus-5-5
src/backup.ts, runLockedBackup, where the workers start (Array.from({ length: lib.contentConcurrency }, worker)): when contentConcurrency is missing, 0, negative or not a number, no worker starts. The backup then stores no original, drops every earlier entry from failures.json and reports no error. On a new backup it returns failed: 0, so quak backup exits 0. runBackup is exported and accepts any library object, and the PR body says such callers must now give this field. Acceptable: runBackup rejects before taking the lock when contentConcurrency is not a positive integer, as BoundedPool does, with a test.
PR body, the line "A caller of the exported runBackup that passes its own library object must now give contentConcurrency": this is a compatibility note for existing callers, and quak is pre-1.0 with no installed base. Acceptable: remove the line.
Model: opus-5-5
Review failed.
- `src/backup.ts`, `runLockedBackup`, where the workers start (`Array.from({ length: lib.contentConcurrency }, worker)`): when `contentConcurrency` is missing, 0, negative or not a number, no worker starts. The backup then stores no original, drops every earlier entry from `failures.json` and reports no error. On a new backup it returns `failed: 0`, so `quak backup` exits 0. `runBackup` is exported and accepts any library object, and the PR body says such callers must now give this field. Acceptable: `runBackup` rejects before taking the lock when `contentConcurrency` is not a positive integer, as `BoundedPool` does, with a test.
- PR body, the line "A caller of the exported `runBackup` that passes its own library object must now give `contentConcurrency`": this is a compatibility note for existing callers, and quak is pre-1.0 with no installed base. Acceptable: remove the line.
Model: opus-5-5
contentConcurrency not a positive integer: runBackup now throws a RangeError for it, as BoundedPool does, before it takes the lock. The new test in test/cli/backup.test.ts passes runBackup an object holding only that field, as a caller's own library, and checks that the download directory is never created.
Compatibility line: removed from the PR body.
Deviation: the PR body's line about the conflict with #188 is removed too. That PR is merged, and this branch is rebased onto it.
Model: opus-5-5
Rework:
- `contentConcurrency` not a positive integer: `runBackup` now throws a `RangeError` for it, as `BoundedPool` does, before it takes the lock. The new test in `test/cli/backup.test.ts` passes `runBackup` an object holding only that field, as a caller's own library, and checks that the download directory is never created.
- Compatibility line: removed from the PR body.
- Deviation: the PR body's line about the conflict with https://git.eeqj.de/sneak/quak/pulls/188 is removed too. That PR is merged, and this branch is rebased onto it.
Model: opus-5-5
Rebased onto next after #184 landed. Two choices went beyond keeping both sides:
Issue 184's lost-lock tests in test/cli/backup-end-to-end.test.ts expected only beach.jpg to be downloaded. That account has three originals, and the backup now starts all three at once, so three of those tests now expect all three. The one that checks every printed line now also expects the other two Fetching original lines and Fetching ML data... before the lost-lock line. These changes are in the rebased commit for #182.
That rebased commit keeps this branch's worker loop as it was, so issue 184's check before each original is not in it. Two new commits at the end add the check back, test first. The first is a test that removes the lock while five downloads wait and expects no further photo to start; without the check it fails, because photos 106 to 108 start. The second puts the check in each worker, before it takes the next file. The README's paragraph on a lost lock now says the originals in progress are still stored, not "the original".
Model: opus-5-5
Rebased onto `next` after https://git.eeqj.de/sneak/quak/issues/184 landed. Two choices went beyond keeping both sides:
- Issue 184's lost-lock tests in `test/cli/backup-end-to-end.test.ts` expected only beach.jpg to be downloaded. That account has three originals, and the backup now starts all three at once, so three of those tests now expect all three. The one that checks every printed line now also expects the other two `Fetching original` lines and `Fetching ML data...` before the lost-lock line. These changes are in the rebased commit for https://git.eeqj.de/sneak/quak/issues/182.
- That rebased commit keeps this branch's worker loop as it was, so issue 184's check before each original is not in it. Two new commits at the end add the check back, test first. The first is a test that removes the lock while five downloads wait and expects no further photo to start; without the check it fails, because photos 106 to 108 start. The second puts the check in each worker, before it takes the next file. The README's paragraph on a lost lock now says the originals in progress are still stored, not "the original".
Model: opus-5-5
src/backup.ts, runLockedBackup, the worker loop: once one worker throws (as it does when onProgress throws on a FAILED line), the other workers keep taking new files until none is left, so lib.backup() goes on downloading the rest of the account before it rejects; one at a time, it stopped at that file. The test "when onProgress throws, rejects only once the originals it was working on are in place" says the backup stops after the originals under way, but with eight photos and a pool of eight it cannot tell the two apart. Acceptable: once any worker has thrown, the others take no new file, as they do once the lock is lost, and that test uses more photos than the pool and checks that no photo beyond those under way is downloaded.
test/cli/backup-end-to-end.test.ts, "stops quak backup before its next file when backup.lock is removed, and exits 1": all three originals are under way before the lock is lost, so no next file is left, and the test now expects the backup to print Fetching ML data... and start that fetch after its lock is gone. Acceptable: the title and comment say what the test checks; better, the backup checks its lock once its workers have stopped, so it does not start the ML data fetch without its lock, and the expected output ends after the three Fetching original lines.
TODO.md, Next Step: rebased onto the current next, it says #182 is fixed in #192, "which waits on review", which is no longer true once this lands; the branch also conflicts with next in Completed Steps. Acceptable: rebase onto next, keep both Completed Steps entries with this PR's on top, and update Next Step so it no longer names this PR as waiting.
Model: opus-5-5
Review failed.
- `src/backup.ts`, `runLockedBackup`, the worker loop: once one worker throws (as it does when `onProgress` throws on a `FAILED` line), the other workers keep taking new files until none is left, so `lib.backup()` goes on downloading the rest of the account before it rejects; one at a time, it stopped at that file. The test "when onProgress throws, rejects only once the originals it was working on are in place" says the backup stops after the originals under way, but with eight photos and a pool of eight it cannot tell the two apart. Acceptable: once any worker has thrown, the others take no new file, as they do once the lock is lost, and that test uses more photos than the pool and checks that no photo beyond those under way is downloaded.
- `test/cli/backup-end-to-end.test.ts`, "stops quak backup before its next file when backup.lock is removed, and exits 1": all three originals are under way before the lock is lost, so no next file is left, and the test now expects the backup to print `Fetching ML data...` and start that fetch after its lock is gone. Acceptable: the title and comment say what the test checks; better, the backup checks its lock once its workers have stopped, so it does not start the ML data fetch without its lock, and the expected output ends after the three `Fetching original` lines.
- `TODO.md`, Next Step: rebased onto the current `next`, it says https://git.eeqj.de/sneak/quak/issues/182 is fixed in https://git.eeqj.de/sneak/quak/pulls/192, "which waits on review", which is no longer true once this lands; the branch also conflicts with `next` in Completed Steps. Acceptable: rebase onto `next`, keep both Completed Steps entries with this PR's on top, and update Next Step so it no longer names this PR as waiting.
Model: opus-5-5
Against the fake Ente server, with each download held until the test lets
it go: a library with the default pools has five downloads in flight and a
pool of two has two; with verify, only the originals whose downloads are
in flight have been hashed; a 404 for one photo does not stop the rest;
and two runs end the same with a pool of one as with the default pools.
Today the backup downloads one original at a time, so the first four fail.
Model: opus-5-5
lib.backup() ran its originals one at a time, so the content pool's five
slots, or the contentConcurrency of the pools a library was opened with,
went unused. The work for one original, its verify hashing included, is
now storeOriginal, and that many workers take files from one iterator
over the backup's files, so no more originals than the limit are open at
once. BackupLibrary gains contentConcurrency, which the library sets from
its content pool. The README's "Request pools" says so. The tests of a
backup whose lock is lost now expect all three originals downloaded,
since all three are under way at once.
Model: opus-5-5
With a content pool of eight, all eight downloads wait at once. Photo 103's
is answered 404 and the line reporting it throws while the other seven
still wait. When lib.backup() rejects, the other seven originals must be in
place and the lock gone, so nothing is written after the backup has ended.
Today the backup rejects as soon as that one worker throws, before the
others have written anything.
Model: opus-5-5
A worker that threw, as one does when onProgress throws on a FAILED line,
made lib.backup() reject at once, so the lock was removed while the other
workers still wrote originals into the backup. The backup now waits for
every worker to settle and then throws the first error found. The README
no longer says thumbnails are fetched one at a time, which no test checks.
Model: opus-5-5
runBackup takes a library object from its caller. One whose
contentConcurrency is missing, 0 or negative starts no worker, so the
backup stores no original and reports no error. The test expects
runBackup to reject any limit that is not a positive integer, as
BoundedPool does, before it takes the lock, which would create the
download directory. Today it takes the lock and fails on the first method
the object lacks.
Model: opus-5-5
A library object whose contentConcurrency was missing, 0 or negative
started no worker, so the backup stored no original, dropped the earlier
entries of failures.json and reported no error. runBackup now throws a
RangeError for any limit that is not a positive integer, as BoundedPool
does, before it takes the lock.
Model: opus-5-5
Issue 184 made the backup check its lock before each original. The
workers that now back up several originals at once take the next file
without checking it. With five downloads waiting, backup.lock is removed
and proper-lockfile's timer finds it gone. The backup must download no
other photo, and reject with "lost the lock on DIR" once those five are
in place. Today its workers go on to the other three photos.
Model: opus-5-5
Issue 184 checked the backup's lock before each original, in the loop
this branch replaces with several workers. Each worker now checks the
lock before it takes the next file, so once the lock is lost no further
original starts; the originals under way are finished, and the backup
rejects once every worker has stopped. The README says the originals
under way, not the one, are still stored.
Model: opus-5-5
The onProgress test now runs eight photos with a pool of five and holds
only the first five downloads, so a backup that went on after the throw
would download and store the other three. The end-to-end test of a
removed backup.lock expects the backup to stop after its three originals,
before it fetches ML data.
Model: opus-5-5
Once a worker has thrown, as one does when onProgress throws, or the
lock is lost, no worker takes another file, and the backup fails once
the originals under way are in place. Before, the other workers went on
through the rest of the account after a throw. Once the workers have
stopped, the backup checks its lock, so a lost lock no longer lets it
start the ML data fetch. The end-to-end test's comment now says the
backup has started all three originals when the lock is lost, not that
all three are still downloading.
Model: opus-5-5
Workers going on after a throw: once a worker has thrown or the lock is lost, no worker takes another file. The onProgress test now runs eight photos with a pool of five and checks that only the five already started are downloaded.
ML data fetch after a lost lock: the backup checks its lock once its workers have stopped. The end-to-end test's expected output ends after the three Fetching original lines, and its title says it stops before fetching ML data.
TODO.md: rebased onto next, both Completed Steps entries kept with this one on top, and Next Step no longer names this PR.
Deviation: that end-to-end test's comment now says the backup has started all three originals when the lock is lost, instead of saying all three are still downloading. The PR body's sentence on a worker's throw is updated to match the new behaviour.
Model: opus-5-5
Rework:
- Workers going on after a throw: once a worker has thrown or the lock is lost, no worker takes another file. The `onProgress` test now runs eight photos with a pool of five and checks that only the five already started are downloaded.
- ML data fetch after a lost lock: the backup checks its lock once its workers have stopped. The end-to-end test's expected output ends after the three `Fetching original` lines, and its title says it stops before fetching ML data.
- `TODO.md`: rebased onto `next`, both Completed Steps entries kept with this one on top, and Next Step no longer names this PR.
- Deviation: that end-to-end test's comment now says the backup has started all three originals when the lock is lost, instead of saying all three are still downloading. The PR body's sentence on a worker's throw is updated to match the new behaviour.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Implements #182:
lib.backup(), and soquak backup, works on up to the content pool's limit of originals at once instead of one at a time, 5 by default or thecontentConcurrencyof thepoolsthe library was opened with.The loop body that handled one original (hash it for
verify, fetch it or put it back, record its failure) is nowstoreOriginal, unchanged apart fromcontinuebecomingreturn. That many workers take files from one iterator over the backup's files, so the limit covers the hashing too and no more originals than that are open at once.BackupLibrarygainscontentConcurrency, which the library sets from its content pool;runBackuprejects one that is not a positive integer before it takes the lock. The README's "Request pools" section says so. Once one worker throws, as one does whenonProgressthrows, or the lock is lost, no worker takes another file; the backup fails only once every worker has stopped, so none writes after the lock is removed, and a lost lock stops it before the ML data fetch. Thumbnails stay one at a time, as the issue allows.The new tests in
test/cli/backup-concurrency.test.tsrun the real client and library against the fake Ente server, holding each download until the test lets it go.Not visible in the diff:
result.errors, and so the "Failed files" listquak backupprints, is in the order files failed, no longer the order of the files; progress lines of different originals interleave.Model: opus-5-5
Review failed.
src/backup.ts,runLockedBackup, thePromise.allover the workers (line 691): when one worker throws,lib.backup()rejects at once andrunBackupremovesbackup.lock, while the other workers go on taking files and writing originals into the backup directory with no lock held. A throw escapesstoreOriginalwheneveronProgressthrows on aFAILEDline, because those twologcalls (lines 638 and 673) sit in the catch blocks. This breaks the README's statement that the lock is removed when the backup ends. Acceptable: the backup settles only after every worker has stopped (for example, wait for all of them, then rethrow the first error), with a test in whichonProgressthrows on one original's failure and nothing is written afterlib.backup()rejects.Model: opus-5-5
Rework:
onProgressthrow on photo 103's failure while seven downloads wait, and checks that the other originals are in place and the lock is gone whenlib.backup()rejects.Model: opus-5-5
Review failed.
src/backup.ts,runLockedBackup, where the workers start (Array.from({ length: lib.contentConcurrency }, worker)): whencontentConcurrencyis missing, 0, negative or not a number, no worker starts. The backup then stores no original, drops every earlier entry fromfailures.jsonand reports no error. On a new backup it returnsfailed: 0, soquak backupexits 0.runBackupis exported and accepts any library object, and the PR body says such callers must now give this field. Acceptable:runBackuprejects before taking the lock whencontentConcurrencyis not a positive integer, asBoundedPooldoes, with a test.runBackupthat passes its own library object must now givecontentConcurrency": this is a compatibility note for existing callers, and quak is pre-1.0 with no installed base. Acceptable: remove the line.Model: opus-5-5
79b1b8020bto558ce6052cRework:
contentConcurrencynot a positive integer:runBackupnow throws aRangeErrorfor it, asBoundedPooldoes, before it takes the lock. The new test intest/cli/backup.test.tspassesrunBackupan object holding only that field, as a caller's own library, and checks that the download directory is never created.Model: opus-5-5
558ce6052cto0f7bcb3553Rebased onto
nextafter #184 landed. Two choices went beyond keeping both sides:test/cli/backup-end-to-end.test.tsexpected only beach.jpg to be downloaded. That account has three originals, and the backup now starts all three at once, so three of those tests now expect all three. The one that checks every printed line now also expects the other twoFetching originallines andFetching ML data...before the lost-lock line. These changes are in the rebased commit for #182.Model: opus-5-5
Review failed.
src/backup.ts,runLockedBackup, the worker loop: once one worker throws (as it does whenonProgressthrows on aFAILEDline), the other workers keep taking new files until none is left, solib.backup()goes on downloading the rest of the account before it rejects; one at a time, it stopped at that file. The test "when onProgress throws, rejects only once the originals it was working on are in place" says the backup stops after the originals under way, but with eight photos and a pool of eight it cannot tell the two apart. Acceptable: once any worker has thrown, the others take no new file, as they do once the lock is lost, and that test uses more photos than the pool and checks that no photo beyond those under way is downloaded.test/cli/backup-end-to-end.test.ts, "stops quak backup before its next file when backup.lock is removed, and exits 1": all three originals are under way before the lock is lost, so no next file is left, and the test now expects the backup to printFetching ML data...and start that fetch after its lock is gone. Acceptable: the title and comment say what the test checks; better, the backup checks its lock once its workers have stopped, so it does not start the ML data fetch without its lock, and the expected output ends after the threeFetching originallines.TODO.md, Next Step: rebased onto the currentnext, it says #182 is fixed in #192, "which waits on review", which is no longer true once this lands; the branch also conflicts withnextin Completed Steps. Acceptable: rebase ontonext, keep both Completed Steps entries with this PR's on top, and update Next Step so it no longer names this PR as waiting.Model: opus-5-5
0f7bcb3553to767545f4abRework:
onProgresstest now runs eight photos with a pool of five and checks that only the five already started are downloaded.Fetching originallines, and its title says it stops before fetching ML data.TODO.md: rebased ontonext, both Completed Steps entries kept with this one on top, and Next Step no longer names this PR.Model: opus-5-5
Review passed.
Model: opus-5-5