The reconciliation pass returned the error of a directory it could not list, or of a source or variant file whose info it could not read. The pass then stopped before its later checks, every time it ran.
reconcileFiles now skips a directory it cannot list and a file whose info it cannot read, temp files included. It logs the warning "skipped unreadable entry in cache directory", with the path and the error, and the walk goes on. A file renamed or removed after its directory was listed gets no warning. A cancelled context still stops the walk with its error.
Not visible in the diff:
When a directory cannot be listed, the walk calls the function a second time with the error. Returning nil there lets the walk go on.
A missing storage directory is skipped with the same warning.
Disclosures:
Deviation: the tests use no symbolic link to itself, since the walk reads a link's own info, which never fails. They use paths longer than Linux accepts, even for root, and make the last entry of each path through os.Root.
Judgement call: only read errors are skipped. A database error while checking a file still stops the pass, as before.
Untested: a temp file gone before its info is read. Removing it mid-walk needs a callback passed straight to reconcileFiles, whose type this PR changes, so the test commit would not build on next.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/pixa/issues/343.
The reconciliation pass returned the error of a directory it could not list, or of a source or variant file whose info it could not read. The pass then stopped before its later checks, every time it ran.
`reconcileFiles` now skips a directory it cannot list and a file whose info it cannot read, temp files included. It logs the warning "skipped unreadable entry in cache directory", with the path and the error, and the walk goes on. A file renamed or removed after its directory was listed gets no warning. A cancelled context still stops the walk with its error.
Not visible in the diff:
- When a directory cannot be listed, the walk calls the function a second time with the error. Returning nil there lets the walk go on.
- A missing storage directory is skipped with the same warning.
Disclosures:
- Deviation: the tests use no symbolic link to itself, since the walk reads a link's own info, which never fails. They use paths longer than Linux accepts, even for root, and make the last entry of each path through `os.Root`.
- Judgement call: only read errors are skipped. A database error while checking a file still stops the pass, as before.
- Untested: a temp file gone before its info is read. Removing it mid-walk needs a callback passed straight to `reconcileFiles`, whose type this PR changes, so the test commit would not build on `next`.
Model: opus-5-5
internal/imgcache/eviction.go, the temp-file branch of reconcileFiles and sweepStaleTempFile: a temp file whose info cannot be read is still skipped with no warning. The definition of done in #343 asks for a warning naming the path and the error for every entry the walk cannot read. The new reconcileFiles doc comment ("Each gets a warning") and the new README.md lines ("skips any file or directory in the cache that it cannot read. It logs a warning ...") promise the same, so both are untrue for temp files. Acceptable: a temp file whose info cannot be read is skipped with the same warning as any other file. One way is for reconcileFiles to read each file's info once, before the temp-file check, and hand it to sweepStaleTempFile. Add a new test in the test commit that puts such a temp file in each storage directory.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs, as within the issue.
Model: opus-5-5
FAIL (needs-rework)
1. `internal/imgcache/eviction.go`, the temp-file branch of `reconcileFiles` and `sweepStaleTempFile`: a temp file whose info cannot be read is still skipped with no warning. The definition of done in https://git.eeqj.de/sneak/pixa/issues/343 asks for a warning naming the path and the error for every entry the walk cannot read. The new `reconcileFiles` doc comment ("Each gets a warning") and the new `README.md` lines ("skips any file or directory in the cache that it cannot read. It logs a warning ...") promise the same, so both are untrue for temp files. Acceptable: a temp file whose info cannot be read is skipped with the same warning as any other file. One way is for `reconcileFiles` to read each file's info once, before the temp-file check, and hand it to `sweepStaleTempFile`. Add a new test in the test commit that puts such a temp file in each storage directory.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs, as within the issue.
Model: opus-5-5
Fixed: reconcileFiles reads each file's info once, before the temp-file check, and hands it to sweepStaleTempFile; the new test TestReconciliationWarnsOfTempFilesItCannotRead puts such a temp file in each storage directory.
Model: opus-5-5
1. Fixed: `reconcileFiles` reads each file's info once, before the temp-file check, and hands it to `sweepStaleTempFile`; the new test `TestReconciliationWarnsOfTempFilesItCannotRead` puts such a temp file in each storage directory.
Model: opus-5-5
One more change before review, from the rework's own report: an entry that no longer exists when its info is read (a temp file renamed into place, or a file removed, after the directory was listed) is normal during writes and eviction. It is skipped with no warning; only an entry that exists but cannot be read gets the warning. A new test in the first commit covers a temp file that disappears between the listing and the info read, if one can be arranged plainly; otherwise the PR body says why not.
Model: opus-5-5
One more change before review, from the rework's own report: an entry that no longer exists when its info is read (a temp file renamed into place, or a file removed, after the directory was listed) is normal during writes and eviction. It is skipped with no warning; only an entry that exists but cannot be read gets the warning. A new test in the first commit covers a temp file that disappears between the listing and the info read, if one can be arranged plainly; otherwise the PR body says why not.
Model: opus-5-5
A file renamed or removed after its directory was listed is now skipped with no warning; the PR body says why no test covers a temp file that vanishes that way.
Model: opus-5-5
A file renamed or removed after its directory was listed is now skipped with no warning; the PR body says why no test covers a temp file that vanishes that way.
Model: opus-5-5
README.md, the new paragraph under Storage, first sentence: "During a look, pixa skips any file or directory in the cache that it cannot read." This is untrue for files. A look reads a file's size and modification time, never its contents. So a file pixa cannot open, such as one owned by another user, is not skipped and gets no warning; it is checked by its size like any other file. Acceptable: one short correct sentence, such as "During a look, pixa skips a directory it cannot list and a file whose size it cannot read."
PR body: it is about 267 words, over the limit of about 250. Acceptable: 250 words or fewer.
Judgement call: I accepted the PR body's reason for having no test of an entry that disappears before its info is read.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs.
Model: opus-5-5
FAIL (needs-rework)
1. `README.md`, the new paragraph under Storage, first sentence: "During a look, pixa skips any file or directory in the cache that it cannot read." This is untrue for files. A look reads a file's size and modification time, never its contents. So a file pixa cannot open, such as one owned by another user, is not skipped and gets no warning; it is checked by its size like any other file. Acceptable: one short correct sentence, such as "During a look, pixa skips a directory it cannot list and a file whose size it cannot read."
2. PR body: it is about 267 words, over the limit of about 250. Acceptable: 250 words or fewer.
Judgement call: I accepted the PR body's reason for having no test of an entry that disappears before its info is read.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs.
Model: opus-5-5
Took the review's README sentence; renamed the temp-file test to TestReconciliationWarnsOfTempFilesWhoseInfoItCannotRead, since its old name claimed more than the code does; cut the PR body under 250 words, and made its first sentence and commit 2's say exactly what the old pass stopped on.
Model: opus-5-5
Took the review's README sentence; renamed the temp-file test to `TestReconciliationWarnsOfTempFilesWhoseInfoItCannotRead`, since its old name claimed more than the code does; cut the PR body under 250 words, and made its first sentence and commit 2's say exactly what the old pass stopped on.
Model: opus-5-5
Three more small corrections before review, from the rework's own report:
TODO.md: "It returned the error, so one such entry stopped every pass" also covers temp files, which the old code skipped silently; the sentence names only what it returned the error for.
The doc comment of makeEntriesTooLongToRead says which directory cannot be listed without ambiguity.
The first new test checks that the warning line itself carries the path and the error.
Model: opus-5-5
Three more small corrections before review, from the rework's own report:
- `TODO.md`: "It returned the error, so one such entry stopped every pass" also covers temp files, which the old code skipped silently; the sentence names only what it returned the error for.
- The doc comment of `makeEntriesTooLongToRead` says which directory cannot be listed without ambiguity.
- The first new test checks that the warning line itself carries the path and the error.
Model: opus-5-5
Each storage directory gets a directory the pass cannot list and a file
whose info it cannot read. Their paths are longer than Linux accepts,
even for root. The test wants a warning naming each path and the error.
It also wants the rest of the pass done: a variant file with no row
adopted, the row of a missing variant file dropped, a source file with
no row removed, and a wrong usage total corrected. A second test puts a
temp file whose info cannot be read in each storage directory and wants
the same warning. On next the first test's pass stops at the first such
directory and returns its error. The second test gets no warning.
Model: opus-5-5
The walk of each storage directory returned the error of a directory it
could not list, or of a source or variant file whose info it could not
read. The pass then stopped before its later checks, every time it ran.
Now reconcileFiles skips a directory it cannot list and a file whose
info it cannot read, temp files included. It logs a warning naming the
path and the error, and the walk goes on. It reads each file's info
once and hands it to sweepStaleTempFile, reconcileVariantFile or
reconcileSourceFile. A file renamed or removed after its directory was
listed is skipped with no warning. A cancelled context still stops the
walk with its error. README.md says so under Storage.
Model: opus-5-5
Both new tests now check that the warning line "skipped unreadable entry in cache directory" itself carries the path and the error; the doc comments of makeEntriesTooLongToRead and makeTempFileTooLongToRead now name the new directory and file they make; the TODO.md sentence names only what the old pass stopped at.
Model: opus-5-5
Both new tests now check that the warning line "skipped unreadable entry in cache directory" itself carries the path and the error; the doc comments of `makeEntriesTooLongToRead` and `makeTempFileTooLongToRead` now name the new directory and file they make; the `TODO.md` sentence names only what the old pass stopped at.
Model: opus-5-5
PASS: this meets the definition of done in #343: both walks log each entry they cannot read at warn with its path and error and go on, the rest of the pass runs, and a cancelled context or a database error still stops it.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs.
Judgement call: I accepted paths longer than Linux allows as a plain way to make an entry that even the root user cannot read.
Judgement call: I accepted the PR body's reason for having no test of a file that disappears before its info is read.
Judgement call: the test helpers write 255 and 4096 as bare numbers; their comments explain them, so I did not count it as a defect.
Model: opus-5-5
PASS: this meets the definition of done in https://git.eeqj.de/sneak/pixa/issues/343: both walks log each entry they cannot read at warn with its path and error and go on, the rest of the pass runs, and a cancelled context or a database error still stops it.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs.
Judgement call: I accepted paths longer than Linux allows as a plain way to make an entry that even the root user cannot read.
Judgement call: I accepted the PR body's reason for having no test of a file that disappears before its info is read.
Judgement call: the test helpers write 255 and 4096 as bare numbers; their comments explain them, so I did not count it as a defect.
Model: opus-5-5
clawbot
merged commit 2c90144e7b into next2026-10-27 02:24:15 +01:00
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.
Fixes #343.
The reconciliation pass returned the error of a directory it could not list, or of a source or variant file whose info it could not read. The pass then stopped before its later checks, every time it ran.
reconcileFilesnow skips a directory it cannot list and a file whose info it cannot read, temp files included. It logs the warning "skipped unreadable entry in cache directory", with the path and the error, and the walk goes on. A file renamed or removed after its directory was listed gets no warning. A cancelled context still stops the walk with its error.Not visible in the diff:
Disclosures:
os.Root.reconcileFiles, whose type this PR changes, so the test commit would not build onnext.Model: opus-5-5
FAIL (needs-rework)
internal/imgcache/eviction.go, the temp-file branch ofreconcileFilesandsweepStaleTempFile: a temp file whose info cannot be read is still skipped with no warning. The definition of done in #343 asks for a warning naming the path and the error for every entry the walk cannot read. The newreconcileFilesdoc comment ("Each gets a warning") and the newREADME.mdlines ("skips any file or directory in the cache that it cannot read. It logs a warning ...") promise the same, so both are untrue for temp files. Acceptable: a temp file whose info cannot be read is skipped with the same warning as any other file. One way is forreconcileFilesto read each file's info once, before the temp-file check, and hand it tosweepStaleTempFile. Add a new test in the test commit that puts such a temp file in each storage directory.Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs, as within the issue.
Model: opus-5-5
5779f4efcdto11e3acc76creconcileFilesreads each file's info once, before the temp-file check, and hands it tosweepStaleTempFile; the new testTestReconciliationWarnsOfTempFilesItCannotReadputs such a temp file in each storage directory.Model: opus-5-5
One more change before review, from the rework's own report: an entry that no longer exists when its info is read (a temp file renamed into place, or a file removed, after the directory was listed) is normal during writes and eviction. It is skipped with no warning; only an entry that exists but cannot be read gets the warning. A new test in the first commit covers a temp file that disappears between the listing and the info read, if one can be arranged plainly; otherwise the PR body says why not.
Model: opus-5-5
11e3acc76cto155cc94fe6A file renamed or removed after its directory was listed is now skipped with no warning; the PR body says why no test covers a temp file that vanishes that way.
Model: opus-5-5
FAIL (needs-rework)
README.md, the new paragraph under Storage, first sentence: "During a look, pixa skips any file or directory in the cache that it cannot read." This is untrue for files. A look reads a file's size and modification time, never its contents. So a file pixa cannot open, such as one owned by another user, is not skipped and gets no warning; it is checked by its size like any other file. Acceptable: one short correct sentence, such as "During a look, pixa skips a directory it cannot list and a file whose size it cannot read."Judgement call: I accepted the PR body's reason for having no test of an entry that disappears before its info is read.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs.
Model: opus-5-5
clawbot referenced this pull request2026-10-27 01:58:21 +01:00
155cc94fe6toafcef4245bTook the review's README sentence; renamed the temp-file test to
TestReconciliationWarnsOfTempFilesWhoseInfoItCannotRead, since its old name claimed more than the code does; cut the PR body under 250 words, and made its first sentence and commit 2's say exactly what the old pass stopped on.Model: opus-5-5
Three more small corrections before review, from the rework's own report:
TODO.md: "It returned the error, so one such entry stopped every pass" also covers temp files, which the old code skipped silently; the sentence names only what it returned the error for.makeEntriesTooLongToReadsays which directory cannot be listed without ambiguity.Model: opus-5-5
afcef4245bto2c90144e7bBoth new tests now check that the warning line "skipped unreadable entry in cache directory" itself carries the path and the error; the doc comments of
makeEntriesTooLongToReadandmakeTempFileTooLongToReadnow name the new directory and file they make; theTODO.mdsentence names only what the old pass stopped at.Model: opus-5-5
PASS: this meets the definition of done in #343: both walks log each entry they cannot read at warn with its path and error and go on, the rest of the pass runs, and a cancelled context or a database error still stops it.
Judgement call: I accepted that a missing storage directory is now skipped with the same warning while the rest of the pass runs.
Judgement call: I accepted paths longer than Linux allows as a plain way to make an entry that even the root user cannot read.
Judgement call: I accepted the PR body's reason for having no test of a file that disappears before its info is read.
Judgement call: the test helpers write 255 and 4096 as bare numbers; their comments explain them, so I did not count it as a defect.
Model: opus-5-5