Verify directory hashes, --paranoid, chunk retry, companions — and fixes from the first live offload - #2
Verify directory hashes, --paranoid, chunk retry, companions — and fixes from the first live offload#2owenpkent wants to merge 19 commits into
Conversation
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed all four commits and ran the CLI end to end. The chunk-retry work is the strongest part of this — Exhausted closing the outer whole-file retry, non-transient errors still propagating raw, and _BadSector logging read offsets so a test can assert log.count(0) == 1 is a genuinely good way to prove "did not restart the file". UnstableRead being transient by construction, and warning rather than claiming the guarantee when the cache can't be evicted, are both right.
One blocker though: the directory-hash verification fails on a tree that nothing has touched. Details inline on verify.py. The short version is that <name>_Reports/ is folded into the recomputed root hash but was never in the recorded one, so offloader verify exits non-zero straight after a successful offload — the default --report pdf path.
| listed = {p.resolve() for p, _, _, _ in entries} | ||
| for candidate in _described_root(manifest, listed).rglob("*"): | ||
| patterns = _ignore_patterns(ascmhl_root) if ascmhl_root is not None else [] | ||
| scan_root = (managed_root if ascmhl_root is not None |
There was a problem hiding this comment.
Blocker — this fails on a tree nothing has touched.
Reproduced with a plain CLI run: offload with --report ascmhl,csv, then verify the manifest it just wrote.
2 checked: 2 ok; 1 of 2 directory hashes differ
DIR . CHANGED
DIR Clips OK
passed: False
unlisted: ['JobReport.csv']
Nothing was renamed, moved or corrupted. scan_root is managed_root — the destination root — so every unlisted file underneath gets hashed into the recomputed root content hash, including <name>_Reports/JobReport.csv and the PDF, HTML and thumbs/ beside it. Those were written after the manifest and were never in the recorded hashes: write_manifest defaults ignore_patterns to [ASCMHL_DIRNAME] only (ascmhl.py:324), so the report directory is unignored on the read side and absent on the write side.
Combined with passed now including directory_failures, offloader verify exits non-zero — and the README says a format script can gate on that exit code. On main the same tree passes, so this is a regression, and it hits the default path since --report defaults to pdf.
Both halves of the fix look worth doing: record the report directory in the manifest's ignore patterns at write time, and skip a sibling *_Reports directory here regardless — manifests already written won't carry the pattern.
| return next(v for v in report.directories if v.relative == relative) | ||
|
|
||
|
|
||
| def test_verify_rechecks_the_recorded_directory_hashes(history): |
There was a problem hiding this comment.
This is where the verify.py blocker slipped through: every new directory-hash test builds its tree from the history fixture, which has no <name>_Reports/ directory, so none of them exercise the layout a real offload actually produces.
A test that runs cli.main(["offload", ..., "--report", "ascmhl,pdf"]) and then verifies would have caught it, and is worth having permanently — the report directory sitting inside the managed root is exactly the kind of thing that gets re-broken.
| line = f"{self.checked} checked: " + ", ".join(parts) | ||
| if self.directory_failures: | ||
| line += (f"; {len(self.directory_failures)} of " | ||
| f"{len(self.directories)} directory hashes differ") |
There was a problem hiding this comment.
This composes into 2 checked: 2 ok; 1 of 2 directory hashes differ — the first thing a reader's eye lands on is "2 ok", on a report that did not pass. Worth leading with the failure, the way the file-level branch does.
| # The directory pass needs the unexpected files hashed, so it scans even | ||
| # when the caller did not ask for them to be reported. | ||
| if find_unlisted or ascmhl_root is not None: | ||
| listed = {p.resolve() for p, _, _, _ in entries} |
There was a problem hiding this comment.
Worth calling out in docs/data-safety.md: the directory pass hashes every unlisted file under the destination root, with cache eviction, and does it even when the caller passed find_unlisted=False. On a destination that holds anything besides this job that's unbounded extra I/O, and --allow-cache only removes the eviction, not the hashing.
| # Recovered without restarting the file, which is why the copy | ||
| # succeeded at all — but the sector that needed it is real. | ||
| job.warnings.append( | ||
| f"{source.name}: recovered a failed read at byte {offset} " |
There was a problem hiding this comment.
One warning per recovered chunk means one line per 8 MiB across a bad region, not one per file. A card failing over a contiguous stretch could bury the rest of job.warnings — worth collapsing to a count and a range per file.
| return Path(path).suffix.lower() in COMPANION_SUFFIXES | ||
|
|
||
|
|
||
| def in_proxy_directory(path: Path) -> bool: |
There was a problem hiding this comment.
This matches on directory name alone, so a card that happens to file originals under a folder named Proxy gets every one of them treated as a companion and reported as belonging to something else. Given how carefully group() refuses to guess on an ambiguous stem, this is the looser half of the same judgement — maybe require the suffix to be one of PROXY_SUFFIXES as well.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Follow-up: the full suite on this branch finished clean here — 433 passed, 1 skipped, no failures. The skip is test_a_firmlinked_system_volume_is_not_a_card (macOS-only), so the collected count of 434 in the docs is right.
The wall-clock figure isn't, at least not on this machine — see inline. Worth noting a green suite is also consistent with the verify.py blocker above: the directory-hash tests all build from the history fixture, which has no <name>_Reports/ directory, so nothing in the suite exercises the layout that breaks.
| ```sh | ||
| pip install -e ".[dev]" | ||
| pytest # 400 tests, ~33s | ||
| pytest # 434 tests, ~50s |
There was a problem hiding this comment.
The run took 12:51 here, not ~50s. I can't pin that on this branch — Windows, Qt GUI tests and real file I/O, and #1's 44-test subset was slow on the same box too — so it may well be environmental rather than a regression. But the figure is stated as fact in three places (here, line 304, and CONTRIBUTING.md:40) and it's the number someone uses to decide whether the suite has hung, so it's worth re-timing on the machine you'd want it to describe.
The test count checks out: 433 passed + 1 macOS-only skip = the 434 claimed.
I didn't measure coverage, so no comment on the 86% on line 304.
They were written from the first release and never read back. A rename or a moved file leaves every individual file hashing exactly as recorded, so no file-level check can object to it; the structure hash exists precisely to catch that. Content matching while structure does not is now reported as RENAMED, which is a far stronger statement than the "not in manifest" line it used to produce. Recomputing means hashing the files the manifest does not list — that is what proves a rename is only a rename — while honouring the manifest's own ignore patterns, or a deliberately ignored file would fail every directory above it. A directory whose mismatch is already accounted for by a file that failed on its own hash says so, rather than repeating itself once per level up to the root; a directory that gained an unexpected file never counts as accounted for, because no file verdict can report an arrival. The hashing itself is the writer's own, exposed as `ascmhl.directory_hashes` and pinned to the reference implementation's published values.
Three engine changes that share the copy loop, so they land together. --paranoid reads every source file a second time and compares. The gap it closes is a read that returns wrong bytes without raising: the checksum is taken from whatever came back, so the destination faithfully matches a corrupted source and verifies clean at every level. A disagreement is not adjudicated — there is no basis for deciding which read was true — so it is retried, and a source that will not read the same twice fails that file and leaves nothing behind. The page cache is dropped first, and the job says so when it could not be, since a second read served from memory compares the first read against itself. It is opt-in because it costs a full extra pass. A transient read failure is now retried at the chunk that failed rather than by restarting the file: recovering a bad sector near the end of a 79 GB clip cost 79 GB and now costs 8 MiB. This needed no hasher rewind after all — a chunk is only hashed once it has arrived whole, so a failed read has produced no state to unwind. The source is reopened and sought back to the offset, because a reader that dropped off the bus needs its handle re-established. Writes still restart the whole file: a partial write leaves the destination at a length the copy loop does not know. retry.Exhausted stops the outer retry from repeating the same attempts against the same fault. Sidecars and proxies are matched to their clip by stem. A .sidecar carries a BRAW's grade; delivered without its clip it is nothing, and the clip delivered without it has silently lost the grade. An ambiguous stem — two takes of the same name in different folders — is left unlinked rather than guessed at. A clip that copies while a file belonging to it does not is now a job warning instead of two rows twenty lines apart. Also fixes a deadlock introduced while doing the above: closing the source moved out of a `with` block into a `finally`, and a close that raised skipped the end-of-file sentinel and hung the consumer forever. Closing now swallows everything, and there is a regression test for a source whose close() raises.
The editor had sixteen fields in one flat column, four of them checkboxes sitting on blank labels. The two or three that bear on any given change were never next to each other. They are now three sections — Preset, Copying, Reports — with the checkboxes stacked under a single Options label. "Job name" is now "Job name template", because Simple mode has a field of the same name that takes a literal one. "Skip files already present at matching size" gained a tooltip saying what it does not compare, since it is the one place the tool takes something on trust. --paranoid was reachable from the CLI and from Preset.to_options but exposed nowhere in the interface, which makes it a field nobody can use. It is now a checkbox in both the preset editor and Simple mode, worded and explained the same way in each, and it persists with the preset.
`offloader verify` prints directory-hash failures alongside the file ones, and `offloader offload` takes --paranoid. The run summary says when a second source read was done, kept out of the PDF's header string because that is pinned to the reference report's wording. The roadmap's Next section is emptied by this branch, so three gaps already documented in docs/ are promoted into it rather than new ideas invented: --skip-existing by checksum, previousPath, and a lock file between instances. docs/data-safety.md had a paragraph stating that a retry restarts the whole file "because a partial read leaves the running checksum meaningless" — the opposite of what the copy loop now does for reads. Corrected, and the limit it listed for unrepeatable source reads now points at --paranoid. 434 tests, 86% line coverage.
Verifying the directory hashes recomputes them from what is on disk, which means hashing the files the manifest does not list — that is what proves a rename is only a rename. But the job's own reports are written into the destination after the manifest, so they are on disk when a verifier recomputes and were never in what it recomputes against. A card that had just been copied reported its own JobReport.pdf as a change to the tree, and `offloader verify` exited non-zero on the path the README says a format script can gate on. The format already has the mechanism: `ignore`, which the verifier honoured and the writer only ever used for `ascmhl`. The report directory goes in the same list for the same reason — neither is managed data. The path is recorded rather than the conventional name, because `--report-dir` moves it, and both are recorded because they can differ: thumbnails land in `<name>_Reports` wherever the PDF goes. A history written before this says nothing about its reports, so `*_Reports` is allowed for when recomputing, keyed off the absence of any recorded pattern but `ascmhl` — it stops applying the moment a manifest describes its own layout. Deliberately not applied to the unlisted list, which still names those files, and scoped to that one directory: a stray file anywhere else still moves the hash it belongs to. The tests that missed this built their destination with `write_manifest` directly, so it never had a reports folder in it. The new ones go through the CLI, which is what writes the reports.
The app section described a drive panel, a preset list and a queue without showing any of them. Three screenshots: preset mode with a job running, simple mode, and the preset editor. Generated rather than captured, because a screenshot taken by hand is wrong the next time the interface moves and nobody notices. `tools/screenshots.py` drives the real app — nothing is mocked but the two things that would otherwise leak this machine into a public README: the config directory is a throwaway, so real presets, settings and history are neither read nor written, and the drive panel is fed invented volumes rather than whatever is mounted. The queue items are built directly instead of enqueued, so no job runs and nothing is copied. The window is opened wider than it starts and the queue splitter pushed down: the default split leaves the queue a row and a half tall, which is the one part of that screen a reader needs to see.
Two things the directory-hash and chunk-retry work reported badly. A report where every file matched and a directory hash did not opened "3 checked: 3 ok", which reads as a pass to anyone scanning — on the one verdict where the file hashes agreeing is the point rather than the reassurance. That case now leads with the directories and says what the combination means. A report with file failures is unchanged: it already opened with them. Recovered reads were one warning each, so a card failing over a contiguous stretch produced one line per 8 MiB and buried every other warning in the job. Now one line per file: a single bad sector still names its offset exactly, because there the byte is the useful fact; a run of them is bounded by first and last, because there it is not. Also documents what recomputing directory hashes actually reads. Proving a rename is only a rename means hashing what the manifest does not list, so a destination root holding anything besides this job is read too.
Neither branch could see this on its own. Companion grouping matches a sidecar to its clip by stem, which is the only relationship a camera records. The data profile's whole claim is that nothing is treated as a clip — so on a dataset it would announce that `capture.xmp` belongs to `capture.h5` on no evidence beyond a shared name, and warn when one copied without the other. Gated on the same `probes_media` the CLI summary already uses, at both call sites: `run` and `rescan`.
The data profile and this branch landed independently, so three things were describing a tool that no longer exists in that shape. `CONTRIBUTING.md` still claimed 434 tests; the merged suite collects 453. Both files now say so, and neither claims a wall-clock figure any more — two runs of identical code here differed by three and a half minutes, so the number was measuring the machine rather than the suite. The changelog gained the two reporting changes it was missing, and the sidecar entry now says the grouping is media-profile only. The README's generic-transfer section claimed nothing is treated as a clip while companion grouping was still doing exactly that; it now says what that means for a dataset.
fa36bb1 to
9677448
Compare
owenpkent
changed the title
1 participant
Clears the roadmap's
Nextsection: the three items it listed, plus thedirectory-hash verification that was top of it, plus a sweep of the interface
that followed from exposing the new option.
Four commits, each reviewable on its own.
Verify the ASC MHL directory hashes
They were written from the first release and never read back. A rename leaves
every individual file hashing exactly as recorded, so no file-level check can
object to it — the structure hash exists precisely to catch that.
RENAMED— bytes intact, a name changed or a file movedCHANGEDRecomputing hashes the files the manifest does not list — that is what proves
a rename is only a rename — while honouring the manifest's own
ignorepatterns. Without that, one stray
.DS_Storefails every directory above it;there is a test that says so.
--paranoidCloses the one gap nothing else here can see: a read that returns wrong bytes
without raising. The checksum is taken from whatever came back, so the
destination faithfully matches a corrupted source and verifies clean at every
level, including the directory hashes above.
The test worth reading is
test_without_paranoid_the_same_source_verifies_clean— it asserts that the same simulated bad source passes today at
--verify full.That is the gap, written as a passing test.
Three decisions:
VerificationMode. The existing modesare a ladder; you want paranoid and full, and a rung cannot say that.
which read was true, so
UnstableReadis transient by construction. A sourcethat will not read the same twice fails the file and leaves nothing behind.
page cache and proves nothing. Where the platform cannot evict, the job says
so rather than claiming the guarantee.
Chunk-level retry
The roadmap expected this to need the hasher state rewound to a chunk boundary.
It did not — a chunk is only hashed once it has been delivered whole, so a
failed read has produced no state to unwind. The retry lives in the reader
thread: reopen, seek to the failed offset, read again. A bad sector near the end
of a 79 GB clip cost 79 GB and now costs 8 MiB.
Writes still restart the whole file (a partial write leaves the destination at a
length the copy loop does not know), and
retry.Exhaustedstops the outer retryrepeating the same attempts against the same fault.
A deadlock was introduced here and is fixed in the same commit. Moving the
source's close out of a
withblock into afinallymeant a close that raisedskipped the end-of-file sentinel and hung the consumer forever — the same
failure an existing regression test was written for. There is now a test for a
source whose
close()raises.Companion grouping
Sidecars (
.sidecar,.rmd,.xmp) and proxies are matched to their clip bystem. An ambiguous stem — two takes of the same name in different folders — is
left unlinked rather than guessed at: naming the wrong take is worse than saying
nothing when the only value of the link is that it can be trusted.
A clip that copies while a file belonging to it does not is now a job warning. A
BRAW delivered without its
.sidecarhas silently lost its grade, and oneVerified row and one Failed row twenty lines apart is not how anyone finds that
out. HTML shows them together; CSV gains a
Companion Ofcolumn, appended soexisting index-based readers are unaffected.
Interface sweep
The preset editor was sixteen fields in one flat column with four blank labels.
Now three sections — Preset / Copying / Reports — with checkboxes stacked under
one
Optionslabel.Job namebecameJob name template(Simple mode has afield of the same name taking a literal one), and
Skip files already present at matching sizegained a tooltip saying what it does not compare.This also caught a gap:
paranoidreachedPreset.to_options()but was exposednowhere in the interface. A preset field the UI never shows is a field nobody
can use. It is now in both modes, worded identically, and persists.
Notes
ROADMAP.md'sNextis emptied by this branch, so three gaps alreadydocumented in
docs/are promoted rather than new ideas invented:--skip-existingby checksum,previousPath, and a lock file betweeninstances.
docs/data-safety.mdhad a paragraph stating that a retry restarts the wholefile "because a partial read leaves the running checksum meaningless" — the
opposite of what the copy loop now does for reads. Corrected.
wording, so the second read is said in the HTML summary and the CLI instead.
434 tests (+25), 86% line coverage, ruff clean.
Fixes from the first live card offload
A first real offload — 512 GB exFAT card, BRAW, one destination — then surfaced
a cluster of problems in how the app reports itself and what it does by
default. Nine further commits, one per fix.
What a real job exposed
bytes / total elapsedfolded the card scanand every between-file stall into the number forever: the queue read
3.5 MB/s while clips were demonstrably flying past, and the ETA was wrong the
same way. Now a 5-second trailing window that visibly decays during a stall
and survives the copy→verify counter reset.
hardware it is ~40x slower, and the engine hashes every byte on the copy
path, so the choice caps copy speed. Every picker — the GUI combos,
--hashhelp,
offloader info— now says what each algorithm costs.and batched. Now they run concurrently, local drives are delivered before
network shares, known share rows persist while re-probing, and Refresh shows
a busy state.
(stage, current file, percent, live rate, ETA), a progress bar that is
readable on the auto-selected row and carries a percent label, fixed-width
numeric columns, and a once-a-second repaint so a stall shows as a falling
rate rather than a frozen one. Simple-mode form rows no longer clip at 125%
display scale.
now says "Add to queue", and the ready line says the job runs after the
current one.
Defaults and paperwork
proves what is on the destination device. Cost: one extra read of each copy
at the destination's read speed — measured 73 MB/s cold on the offload
target used for testing, roughly doubling that job; a fast SSD destination
adds a few percent.
source-onlystays one flag or one dropdown away.Stock ffmpeg cannot decode BRAW; the first clip of a suffix that produces no
frames marks it dead for the rest of the job. Camera proxies are unaffected.
E:\has no folder name, so the job and all its paperwork were called"Offload". Now they are called what the operator calls the card —
A003—in the engine, the
{card}naming token, and Simple mode's placeholder.reports can be told apart in a file manager.
467 tests (+33 from this round), ruff clean. The queue readout, drive panel,
form layout and button states were also verified against the live desktop app
on Windows at 125% scale.