DecodingUs Grid — the Navigator edge - #61
Merged
Merged
Conversation
First piece of the Navigator edge. A node receives a finished manifest with its
lease — URLs, md5s and sizes, curated by the AppView — and this module fetches
exactly what that manifest names. It asks ENA to discover nothing, which is what
keeps a fleet of any size off the archive. Design §7.1.
Design §7.1 said to mirror `refgenome::download`. It cannot be mirrored, on three
counts, and each matters more here than for a reference genome:
- it sends no `Range` header, so an interrupted transfer restarts at zero. A
reference genome is ~900 MB on a developer's connection; an ENA run file is
10-30 GB on a volunteer's, and resume is what decides whether a unit ever
completes;
- it hashes SHA-256 where ENA publishes md5;
- its one retry is blind, repeating the whole transfer on errors that repeating
cannot fix.
What is worth copying is copied: `.part` then atomic rename, so a partial file can
never be mistaken for a complete one.
Resume means the bytes on disk were hashed by a process that is gone, so a resumed
transfer re-reads its own prefix to rebuild the md5 state before asking for the
remainder. That costs one sequential read of what we already have — far less than
fetching it twice — and it overlaps with waiting on the server. Hashing the whole
file at the end costs the same read but cannot start until the transfer finishes.
Four decisions that are about not corrupting a volunteer's disk or their trust:
- an oversized `.part` restarts rather than being truncated to fit. It is evidence
the file on disk is not the file the manifest describes, and trimming it would
pass a size check and fail an md5 one after another full download;
- a server that ignores `Range` and answers 200 is detected, not trusted — its
body is a whole file, and appending it would give the right size by accident;
- a checksum failure deletes the `.part` before retrying, because resuming from
known-bad bytes only re-confirms them;
- a cancel leaves the `.part` alone, so cancelling does not throw away hours of
someone's bandwidth.
Tests run against a real HTTP/1.1 server on loopback rather than a mock — about
forty lines, no new dependency, and it can be made to misbehave. That paid for
itself immediately: the first version of the stub matched `Range:` case-sensitively,
reqwest sends header names lowercased, and so the "resume" test was silently
exercising the ignores-Range path instead. The code was right; the stub was not.
`free_space`/`has_room` become pub(crate) rather than being reimplemented — one
place should answer "is there room", including its considered choice that an
unmeasurable disk permits the attempt.
Comments are Simplified Technical English; the workspace is back to zero violations.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
…ame bytes
Second piece of the Navigator edge: register, claim, heartbeat, release, submit,
and the caller's own standing. Same device-key path the exchange and recruitment
clients already use, over the shared `appview_post` / `appview_get_signed`
transport. Design §4.4 and §7.1.
The canonical strings live in `navigator-sync::grid::messages`, mirroring
`du_db::grid::messages` byte for byte — the convention `navigator-sync::recruitment`
already set. A mirrored contract is a contract that can drift, and drift here
surfaces as a 403 with no explanation against a released desktop build, so both
sides carry tests pinning the literals.
I also diffed the two implementations mechanically rather than trusting that I had
copied them correctly: all six format strings are identical across the repos, and
so are the two `canonical_sha256_b64` bodies. Worth doing, because "I wrote both
sides" is exactly the confidence that lets a one-character difference through.
Three things the signatures had to get right:
- a mutating call signs `{ts}\n{base}` via `DeviceKey::sign_fresh`, which already
existed and already mirrors `du_web::sig::fresh_message`. One signature binds
the operation and the time, and the AppView burns it against replay;
- `claim` normalizes data kinds (upper, dedup, sort) *before* signing and sends
the normalized list, so the bytes on the wire are the bytes the signature
covers. It signs what the node asked for, not what the server will clamp it to
— a node cannot know our bounds;
- `submit` carries two signatures for two purposes. The request signature proves
who is calling now; the digest signature persists in the row so a later audit
can prove which node produced a result after the AppView pools it across
contributors.
`grid_heartbeat` returns whether the lease is still ours, so a node that lost one
stops rather than spending hours on a unit it will not be credited for.
`GridStanding::rank` is `Option<i64>` because the AppView sends null for an
uncredited contributor — decoding that as a number would reintroduce the "you are
last" reading the AppView side deliberately removed.
Comments are Simplified Technical English; the workspace stays at zero. Writing in
STE from the start cost 6 violations to clean up against 100 for the previous
module, which is an argument for not treating it as a post-pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
claim -> fetch -> import -> analyze -> ancestry -> digest -> submit -> clean. Every
step is a method that already existed; this module orders them, reports stages for
the heartbeat, and guarantees the lease ends. Design §7.2.
Two invariants it exists to hold:
- **A lease always ends.** Every exit path gives it back — success closes it
inside the submit transaction, failure sends a release. The AppView's reaper
exists for a node that vanished; a node that is still alive must not need it.
- **Scratch is always removed**, whatever the outcome. Unit files are 10-30 GB,
and a node running for a week otherwise fills its owner's disk.
**FASTQ is not advertised, on purpose.** `supported_data_kinds()` returns `["CRAM"]`
only, and it is the single place that decides — the AppView filters offers by it, so
a node is never handed work this module cannot do. That is what the capability
filter is for.
The reason is not that FASTQ is hard. `realign_job` already has exactly the stages a
FASTQ unit needs, and its stage A already writes FASTQ that stage B consumes — an
ENA FASTQ unit is that pipeline with a different source for stage A. But making
stage A accept external reads changes a module that shipped in alpha.17 and that
phase 5 validated end to end on a whole genome. That belongs in its own commit,
where a reviewer can weigh it against the validated behaviour, rather than buried
inside the first version of a driver. `map_reads` is the seam, documented and
returning an error it should never reach.
An absent ancestry estimate is not a unit failure. A fresh ENA sample may have no
autosomal consensus yet; the digest then carries no ancestry value, and two results
that both lack one still agree. Failing the unit there would discard hours of
completed analysis over a field the agreement test tolerates.
Absent digest values are omitted rather than sent as null, matching what the
AppView's `Comparable` projection expects: two results that both lack a Y call
agree, and one that has a Y call never agrees with one that does not.
The digest has no `mt_terminal`, and there is a test asserting the string does not
appear anywhere in it — the analysis path declines to assign mtDNA on CHM13, and the
Grid analyses against CHM13.
Comments are Simplified Technical English; the workspace stays at zero.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
`navigator contribute` announces the node, claims a small batch, runs each unit, and claims again — until Ctrl-C, until the catalogue has nothing this node can do, or until `--max-units`. It ends by printing the contributor's standing. **Ctrl-C must not cost a lease.** The signal sets the cancel token rather than exiting: the unit in flight stops at its next stage and releases, and every unit claimed-but-not-started is released in the same loop. A node that merely exited would hold its whole batch until the leases lapsed, and no one else could take that work. The AppView's reaper exists for a node that vanished — a node being shut down politely must not need it. `CLAIM_BATCH` is 4, for two reasons worth separating: a node that stops has few leases to hand back, and a new node cannot take a large slice of the catalogue before it has proven anything. `--dry-run` prints what the node would offer and stops before claiming, so someone can see what they are volunteering for without volunteering. Reading the standing at the end cannot change the exit code — it is a courtesy after the work, and a failed read must not turn a successful run into a failure. **What is and is not verified.** It compiles, clippy is clean at `-D warnings`, and `contribute --help` renders correctly. The `--dry-run` path is NOT exercised: the binary hangs before printing anything on this machine. That is not this command — `navigator subjects --db <fresh path>` hangs identically, so `App::open` does not complete here for any CLI subcommand. Worth knowing before someone debugs this command for it. Also fixed a mistake this made on the way in: the two new constants were first inserted between `cli_try!`'s doc comment and the macro, silently orphaning that documentation. The STE checker caught it by reporting the two blocks as merged — a nonsense-looking violation that turned out to be a real defect. `tokio`'s `signal` feature is now on for `navigator-ui`, which the Ctrl-C handling needs. Comments are Simplified Technical English; the workspace stays at zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
`supported_data_kinds()` now advertises FASTQ, so a node maps reads itself: reference -> index -> map -> sort -> mark duplicates -> compress to CRAM. The result goes through the same import and analysis path a passthrough unit takes. **I said this would be a focused commit against `realign_job`. Having read that module, it should not be.** Its stages are not separable from the machinery that resumes a stopped job — `Resumed`, `ScratchState`, and the rules about which file each stage may delete. A comment in that file records a fault in exactly those rules that destroyed a 59 GB file and about four hours of work. Extracting through that line, with no way to re-run the whole-genome validation that alpha.17 passed, is a bad trade for the benefit. And the benefit turned out to be small, because a Grid unit wants none of that machinery. It has no source alignment, so there is no revert stage. It never continues a stopped job — a failed unit releases its lease and another node starts it clean, and the scratch is deleted either way. It registers no alignment against a source row. What the two genuinely share is four operations that are already public functions, so this calls them directly. The realignment module is untouched. `git diff` against it is empty, which is the property worth having: what phase 5 validated on a whole genome is still exactly what ships. Mate files are split on `_1.` and `_2.` — the dot matters, because a run accession can contain `_1` and matching on that alone sends a file to the wrong mate. There is a test for `ERR1_1_1.fastq.gz`. Preset follows the read layout: short-read for a pair, HiFi for an unmated set. A long-read set mapped under a short-read preset does not fail; it produces alignments that look right and are wrong. Each intermediate is deleted as soon as the next stage has consumed it — reads after mapping, mapped after sorting, sorted after marking. A whole-genome unit is tens of GB at each step, and this runs on a volunteer's disk. **Not verified:** no FASTQ unit has been run end to end. That needs a live AppView, a real ENA sample, and hours of compute. What is verified is that it compiles, clippy is clean, the mate-splitting has tests, and the realignment path is byte-for- byte unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
`grid_submit` no longer sends an empty `record_refs`. A finished unit now queues its biosample anchor and coverage record, each carrying the `Provenance` block that du-domain gained earlier, and hands the submission their `at://` addresses. Provenance is what makes a record *about* a public sample nobody owns while being *made by* the contributor who computed it — the subject/author split §5.1 describes. The anchor carries the ENA accession as an external id, which is the other half of that split. **The block is attached after the builder, not threaded through it.** The record builders in `publish.rs` serve the ordinary path, where someone publishes about their own genome, and they have thirteen call sites. An extra argument on each builder would put a `None` at every one of those sites for a value only the Grid ever supplies. So the Grid adds the block afterwards, from the typed `Provenance`, so shape and field names still come from the shared contract. That leaves exactly one string — the key. Two tests remove the assumption: one builds a record through the typed `with_provenance` and asserts the key comes back, the other asserts the block this module adds is byte-identical to the block the type writes. A rename in du-domain now fails here rather than producing a record the AppView reads and silently does not understand. **Addresses are known before the write**, because these records use fixed rkeys. That is what lets the submission carry them in the same run instead of waiting for the outbox to drain. Publishing goes through that outbox rather than a direct write for the obvious reason: a volunteer's machine goes offline, and the queue retries where a direct write would simply lose the record. **A publish failure does not fail the unit.** The analysis is done and the digest is what the quorum reads; the records are the detail behind it, and the outbox will send them later. Failing the unit over a network call would discard hours of completed compute — the same reasoning as the ancestry step above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
`grid_heartbeat` had no caller. The CLI reset an interval timer and sent nothing, so
no node ever told the AppView it was alive, and the `held: false` answer — the only
way a node learns it lost its lease — was unreachable. I built the client method and
the timer scaffolding two commits apart and never joined them.
The cause is worth recording because it will recur: `report` is a synchronous
`FnMut` and `grid_heartbeat` is async, so the beat cannot be sent from inside the
progress callback. I wrote the timer next to the callback, where it looked wired,
and moved on.
The fix puts the beat where it belongs — beside the work, in the driver, so every
caller gets it and no CLI has to remember. Both futures borrow `&self`, so neither
can be a spawned task; `tokio::select!` drives them together without needing a
'static value. The current stage passes between them through a small shared cell, so
the AppView shows what the node is doing now rather than what it started with.
Two judgements in the beat loop:
- `held: false` cancels the token and stops the unit. A node that lost its lease
earns nothing for continuing, and could otherwise spend hours on a unit another
node already finished.
- A beat that fails to send does NOT stop the unit. A volunteer's network drops;
that is not evidence the lease is gone. The lease has its own bound and the
AppView reclaims it if this node really did stop.
Also declared tokio's `time` feature explicitly on navigator-app. `ena`'s retry
delay and this interval both compiled already through feature unification from
another crate, which is not a property to rely on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
Found while auditing the branch for the same class of defect as the heartbeat. `grid_unit_inner` creates a subject and imports the downloaded CRAM against it, because `analyze_biosample` works on a subject. `run_grid_unit` then deletes the unit's scratch directory — which is where that CRAM lives. The subject survived, permanently, with an alignment row naming a file that no longer exists. A node contributing for a week would leave some thousands of those among its owner's real subjects, each holding nothing usable and each hard to distinguish from a real one. That is worse than the heartbeat bug: it damages the user's own workspace rather than failing to report. A Grid unit is work, not the user's data. The subject now goes away with the files. It is recorded the moment it is created, before the import, so every later failure path still removes it — and the removal happens before the directory, since the subject names files inside it. `delete_biosample` refuses while a subject holds data, so this removes the sequence runs first. A failure there is silent: the unit is already complete, its digest is with the AppView and its records are in the publish queue. A leftover subject is a fault to fix, not a reason to report finished work as failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
A `/code-review high` over the branch returned fifteen findings. These are the five that break correctness; the rest are triaged and follow. **A lost lease killed the node for the whole run.** `contribute` used one `CancelToken` for the session, and `beat_while_working` cancels it when the AppView reports another node holds the lease. `CancelToken` is deliberately one-way — `cancel.rs` says a token covers exactly one run — so one reclaimed lease latched it, released every other claimed unit, and ended the session. A lost lease is a normal event and must cost one unit. Each unit now gets its own token, with a small task copying the session token into it so Ctrl-C still stops work within moments. **The cleanup destroyed exactly what `ena.rs` exists to preserve.** The scratch directory was removed on every exit path, so the `.part` files and md5-prefix machinery could never be used: Ctrl-C 25 GB into a 30 GB transfer discarded all of it. The scratch now survives a cancel, and `sweep_old_scratch` removes directories older than a week at startup so a run never continued does not leak disk. **The published record carried no ENA accession** — while a comment right beside it claimed it did. `BiosampleRecord` deliberately has no accession field and reads `external_ids` from a table the grid path never wrote, so records reached the PDS with an empty `externalIds` and nothing could tie them to the public sample. That is the subject half of §5.1's subject/author split, and it was simply missing. **Two failed analyses could reach a quorum.** `analyze_biosample` records per-step failures in `errors` and still returns `Ok` — correct for a batch over a user's own subjects, wrong here. A failed step leaves its value out of the digest, absent compares equal to absent, so two independently broken nodes agreed on nothing and were paid. The unit now fails and another node does the work. The AppView gets the same guard independently (`decodingus@dcd4484`), because a node is untrusted by construction. **`ContributeArgs` split `ArchaicArgs` from its doc comment and its derive** — stealing `#[derive(Args)]` and leaving `ArchaicArgs` with the `#[derive(Parser)]` meant for the new struct. This is the second time in this file that inserting by string anchor landed on the wrong side of an attribute; the first orphaned `cli_try!`'s documentation. It compiles either way, which is what makes it worth naming. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
The other ten from the branch review. Grouped by what they cost. **Wasted a volunteer's bandwidth and reported the wrong answer.** A multi-run sample had every run's files downloaded and exactly one of them used — the first mate pair for FASTQ, the first CRAM otherwise — and the coverage from that fraction was submitted as the coverage of the whole sample. The node now refuses such a unit *before fetching anything*: the manifest arrives with the claim, so the count is free. Merging runs needs a merge stage that does not exist; until it does, another node should not be handed a wrong answer either. **The preflight was sized for the wrong pipeline.** `SPACE_MULTIPLE = 3` on manifest bytes, but a FASTQ unit's manifest names *compressed* reads and the node then holds `mapped.bam` and `sorted.bam` at once plus the sort spill. Thirty GB of reads peaks well past 90 GB. Split into `SPACE_MULTIPLE_ALIGNED = 3` and `SPACE_MULTIPLE_READS = 10`, so the check catches the case it exists for instead of failing hours in. **One bad record dropped every record ref, including the anchor.** `coverage_record` errors on an alignment with no cached coverage and on a Y-scoped file labelled WGS — both real on ENA samples. A `?` turned either into an empty list, so the submission named no records at all, not even the biosample anchor already queued for publishing. Failures are now per-record; whatever was gathered survives. **The release reason leaked local paths.** It was `e.to_string()`, and `ena::io_err` formats the file path into I/O errors — so a full disk sent a volunteer's home directory to a public service, stored and signed. Now a short class: `disk`, `checksum`, `stopped`, `unsupported`, `analysis`, `error`. Two tests, one of which asserts no `/` reaches the server. Full detail stays local in `outcome.error`. **The scratch wipe raced a live blocking task.** When the beat won the `select!`, the work future was dropped mid-await — but the heavy stages run in `spawn_blocking`, and dropping a JoinHandle does not stop the task. The sort kept writing while the directory was deleted under it. The beat now sets the token and the code *waits* for the work to unwind before touching the directory. **Three smaller ones.** The `.crai` in the manifest was parsed and never fetched, so every passthrough unit re-derived an index with a full extra pass over a 10-30 GB file; it is now fetched, best-effort, since a failure only costs that time back. Capabilities were hardcoded to `disk_budget: 0, memory_bytes: 0`, which becomes invisible starvation the day the server filters on them — now measured, with `--disk-gb` to override. And node-level liveness only updated at startup, since only `register_node` writes `last_heartbeat` and the per-lease beat writes a different row; the node now re-registers before each claim batch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
…nce build A second `/code-review high` after the last round of fixes returned twelve findings. One (the multi-run guard counting files) I had found and fixed independently while it ran; these are the rest. **The digest reported a build the analysis never ran against.** Every unit filed its result under `params.reference_build` — the CLI default, `chm13v2.0`. But a CRAM unit is a *passthrough*: the submitter chose that file's build, which across ENA is usually GRCh37 or GRCh38, and nothing realigns it. Since the AppView only compares digests whose builds agree, those results joined the pool of genuine CHM13 results and had their coverage and Y calls compared as though they measured the same thing. The header probe already records the truth on the alignment row during import, so the digest and the provenance now read it from there. **A lost lease left 10-30 GB on disk.** The cleanup keeps the scratch when the token is cancelled, so a `.part` can be resumed — but the beat cancels that same token when another node takes the lease, and this node will never see that unit again. The beat now records *why* it cancelled, and only a stop by the user preserves the files. **A won submission could be thrown away.** When the beat won the `select!`, the work future was awaited but its result discarded. If the work had reached `grid_submit` and succeeded inside that window, the node reported failure, lost the credit, and called release on a lease submit had already closed. The work's `Ok` now wins. **The accession built a path that is later recursively deleted**, unvalidated. The AppView is not an attacker, but a value from outside with `remove_dir_all` on the other side of it deserves a check. Restricted to `[A-Za-z0-9_.-]`, which refuses nothing any real archive emits. **`sex` went on the wire as a `Debug` rendering.** Renaming a variant of `InferredSex` would silently change a quorum-compared value, and two Navigator versions would score each other `DIVERGENT` with no visible cause. Explicit mapping now, and `Unknown` yields no value at all rather than the string "Unknown" — two nodes that both could not tell agree, and one that could does not agree with one that could not. **One leaked task per unit.** The session→unit cancel bridge looped until the *unit* token cancelled, which never happens on a normal completion — so every finished unit left a task waking four times a second forever. It now takes a oneshot that drops when the unit returns. **No coordinate index was ever built.** `ena.rs` claims "the node makes the index itself", but nothing called `ensure_alignment_index`. When ENA publishes no `.crai`, every region query failed, which populated `analyze_biosample`'s `errors`, which now fails the unit — after the multi-hour whole-file walk had already succeeded. Built before analysis instead. **A 416 was unrecoverable.** With no size in the manifest — always true for the index sidecar — `resume_from` asks for a range past the end of a complete `.part`. The server answers 416, `error_for_status` makes it an error, and all five attempts repeat the identical request. The `.part` is now discarded so the next try starts clean. Still open and recorded, not fixed here: the Y terminal is taken from the per-alignment walk rather than the genome-level consensus `build_y_profile` has just computed; a female sample still gets a full chrY placement because the unit's biosample carries `sex: None`; and ancestry is always absent because nothing builds an autosomal consensus, so the `Ancestry` stage is a no-op today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
The three findings held back from the last pass. Each was the difference between a unit contributing a genuine answer and contributing sex-plus-coverage. **Ancestry was absent from every digest.** `estimate_ancestry_from_consensus` requires an autosomal consensus, and nothing in the Grid path ever built one — so the `Ancestry` stage printed a label and did nothing, for every unit ever. It now builds the profile first, which genotypes the alignment at the full panel: a second whole-genome pass, and a real cost for a volunteer. That cost is not optional, and the reason is worth stating because it constrains every future choice here: **the setup of a node must never change the content of a digest.** A `--with-ancestry` flag would make one honest node send a key another honest node omits, and the AppView — which compares absent against absent as equal — would read two correct results as a disagreement. Uniform, or the unit fails. **The Y value was the wrong one.** It came from `haplogroup_calls(...).first()`, the per-alignment walk ordered by row id. `analyze_biosample` had just built the Y profile, whose `consensus_label` is this application's actual answer for the sample, and which is frequently deeper in the tree. The Grid was publishing a shallower placement than Navigator itself would give for the same data. **A female sample got a chrY placement anyway.** The unit creates its biosample with `sex: None`, so `subject_has_y_dna` returns true and the female short-circuit never fires — the walk places noise, and either fails the unit or publishes a spurious Y branch into the quorum. The measured sex is available by that point and now gates it. Four tests, including one that pins the exact key set a finished unit sends. That set is the contract two nodes have to agree on, and it should fail loudly if it ever becomes conditional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
`consensus_profile::get(..., "Y")` was correct — `DnaType::Y.as_str()` is exactly "Y" — but it is the third stringly-typed key in this module today, after `PROVENANCE_KEY` and the `Debug` rendering of `InferredSex`. The first two each turned out to be a way for a rename elsewhere to change behaviour here silently. This one now reads the enum, so it cannot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
…'s data
Eight findings. Two are serious, and both were introduced by fixes from earlier
rounds — which is the honest headline.
**`sweep_old_scratch` could destroy a user's files.** I added it two rounds ago so a
cancelled transfer's `.part` would not leak disk forever. It iterated `--scratch` and
`remove_dir_all`'d every subdirectory older than a week. `--scratch` is a free-form
path, so `navigator contribute --scratch ~/genomes` would delete every directory in
`~/genomes` untouched for a week — at startup, before doing any work. Two guards now:
the name must be one this node would generate, and the directory must carry a marker
file this node wrote. A test creates a user directory and a node directory side by
side and asserts only the node's is removed.
**A cancelled analysis read as a successful one.** `analyze_biosample` returns `Ok`
with an *empty* `errors` on cancellation — reasonably, since a stop is not a fault of
the sample. My `errors.is_empty()` guard from the previous round therefore passed
after Ctrl-C, and the driver continued: a second whole-genome pass for the autosomal
consensus (which takes no cancel token), publish, and submit — sending a digest
holding whichever steps had finished. Two nodes stopped at the same step would agree
on it. It also meant Ctrl-C did not stop the node for hours, and a lost lease still
submitted. Now checked explicitly.
The rest:
- `held` defaulted to `false` on a missing or renamed field, so an AppView that
answered `200 {}` would abort hours of work. Only an explicit `false` now means
a lost lease; anything ambiguous keeps working, because the lease has its own
expiry and the worst case is duplicated compute.
- The fetch progress callback fired once per HTTP chunk — hundreds of thousands of
console lines and mutex takes for a 30 GB file. Throttled to whole-percent changes.
- A headless node never drained the outbox: only the GUI timer calls it, so every
submission named records that sat in local SQLite forever while it grew by two
rows per unit. `contribute` drains after each unit.
- The multi-run guard counted runs but not files, so a single run with two unmated
FASTQs still had one analysed and the other deleted — the same "coverage from one
part reported as the whole" the guard exists to prevent.
- Node liveness re-registered once per claim batch, which with 4-unit batches of
multi-hour units is every 10–20 hours. It has its own 5-minute task now.
Left open: the minimap2 preset is chosen from mate count, so a single-end Illumina
run is mapped as HiFi. Fixing it properly needs the instrument on the manifest, which
is an AppView curation change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
The last finding from the third review. `paired = r1.is_some() && r2.is_some()` picked ShortRead, and everything else got HiFi — so a single-end Illumina run, or one where ENA published only `_1`, was short-read data mapped under a long-read preset. The comment sitting directly above that rule already stated the consequence: a map under the wrong preset does not fail, it gives alignments that look correct and are wrong. It was true in both directions and the code only guarded one. The manifest now carries ENA's instrument model (`decodingus` side, same branch name), and `Preset::infer` turns it into a preset. That function **errors** on an instrument it does not know rather than guessing, and this passes the error on: the unit goes back and another node takes it, instead of a result of unknown quality reaching the quorum. Duplicate marking now keys off the preset too, not the mate count. A single-end short-read run still wants duplicates marked; a long-read run still does not, because two long reads rarely share end points and marking them removes real coverage. Pairing at the map step still comes from whether both mates are present, which is the right question for that step and a different one from the preset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The volunteer side of the Grid: fetch a
public-ENA work unit, analyse it, publish the records, submit a signed digest. Pairs with
decodingus#33.
Completes P1 for both data kinds. 29 unit tests + 5 integration tests; fmt, clippy
-D warnings, andste-checkall clean.What is here
ena.rsnavigator-sync::griddu_db::grid::messagesgrid.rsgrid_job.rscontributeRead this before the diff
Nothing here has run end to end. No unit has been claimed, fetched, analysed or
submitted. The pieces are unit-tested and the downloader is tested against a real
HTTP server, but the loop has never executed. Separately,
App::openhangs on thismachine for every CLI subcommand (
navigator subjectstoo), so even--dry-runisunexercised — worth knowing before anyone debugs
contributefor it.Three review rounds found 35 defects, and two rounds introduced regressions in the
previous round's fixes — including a
sweep_old_scratchthat wouldremove_dir_allevery subdirectory under
--scratcholder than a week, at startup.navigator contribute --scratch ~/genomeswould have deleted the user's data before doing anywork. It now requires both a node-generated name and a marker file this node wrote,
with a test that puts a user directory and a node directory side by side.
Every defect was a seam between two individually-correct pieces — the heartbeat
client and its timer; the scratch cleanup and the resume machinery it silently
defeated; a comment claiming an ENA external id and the code that never wrote one.
None was caught by the compiler, the tests, clippy, or the STE gate. That is the case
for a careful human read of this before it merges.
Decisions worth a look
refgenome::downloadcould not be reused. §7.1 said to mirror it; it cannotresume (no
Range), hashes SHA-256 where ENA publishes md5, and retries blindly. On a10–30 GB file over a volunteer's connection, resume is what decides whether a unit
ever completes. A resumed transfer re-reads its own
.partprefix to rebuild md5state before asking for the remainder.
The realignment module is untouched.
git diffagainstrealign_job.rsis empty.Its stages are inseparable from the
Resumed/ScratchStatemachinery — a comment inthat file records a fault in exactly those rules that destroyed a 59 GB file and four
hours of work — and a Grid unit wants none of it: no revert, no resume, no source row.
So
grid_jobcalls the four public primitives directly, and what phase 5 validated ona whole genome still ships unchanged.
The setup of a node must never change a digest's contents. This constrains
everything here. A
--with-ancestryflag would make one honest node send a key anotheromits, and the AppView compares absent-against-absent as equal — so it would read two
correct results as a disagreement. Ancestry is therefore unconditional, which means a
second whole-genome pass per unit. That is a real cost increase for volunteers, and
the alternative is dropping the field from the digest on both sides, not making it
optional. A test pins the exact five-key set a finished unit sends.
FASTQ was not deferred, and not merged into
realign_jobeither. The seam is fourpublic functions; the mapper preset comes from ENA's instrument (via the new manifest
field), because
paired = both mates presentmapped every single-end Illumina run asHiFi — and
Preset::infererrors rather than guesses on an unknown instrument.A unit leaves nothing behind. It creates a subject because
analyze_biosampleneeds one, then removes it — an earlier version left a subject per unit with an
alignment pointing into a deleted scratch directory, so a week of contributing would
have littered the owner's real workspace with thousands of broken subjects.
Still open
is wasted); merging runs needs a stage that does not exist.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SequryftjcFkLnjQRMKCx3