[PiyushXCoder]

What this changes

The store used to pay for durability on every completed piece: sync the payload,
write the bitfield claim, sync the claim. Two device flushes per piece, which
measured 2.82 ms on ext4 and capped the whole disk path at 25 MB/s.

That cost is now gone. Durability moved to a trailing flag byte whose clean bit
is set only by finalize on a tidy exit. A store found without it was left by a
crash, so every piece its bitfield claims is re-hashed on the way back in and
any claim that outlived its data is dropped. A claim is corrected on resume
rather than prevented on write.

The sense of the bit is deliberate: a byte that was zeroed, truncated or never
written reads as not clean, so damage costs a re-verification rather than a
bitfield taken on trust.

Measured effect

Component costs, release build, store on ext4, medians over 100 samples:

	before	after
set_bitfield	2.82 ms	8.46 µs
write_whole_piece	10.32 ms — 25 MB/s	268 µs — 977 MB/s

End to end, scripts/bench-seeders.sh on ext4, 128 MiB, median of three:

seeders	before	after
1	4.6 MB/s	6.1 MB/s
4	14.6 MB/s	33.2 MB/s
8	19.5 MB/s	44.5 MB/s
32	20.5 MB/s	152 MB/s

The old table flattened at 20.5 MB/s from eight peers on, and the README
described that as the write rate of the disk. It was the flush rate. The ceiling
was three cells wide and read as hardware.

tmpfs and ext4 now agree within a few percent, so the two benchmark tables in
the README collapse into one. The store no longer has to live in a RAM disk for
the client's own shape to be visible.

Breaking: existing .dhaar stores reset

The trailer grew by one byte, so a store written by an earlier build is now the
wrong size. initialize reads wrong-size as "not resumable" and lays it out
fresh, zeroing the bitfield. The payload survives — set_len extends, it
never truncates — but the download restarts from zero.

This was hit for real on a 6.5 GB store mid-session. Recovery is to set every
bit in the bitfield and leave the flag byte clear: the repair pass then re-hashes
and prunes whatever does not verify. There is no automatic migration, and adding
one needs a format version the trailer does not carry yet.

Also in here

• Geometry consolidation. piece_length was a per-call parameter on
  initialize, read and write that every caller passed the same value to,
  which let the benchmark open a store at one stride and write it at another. It
  moves to the constructor; bitfield_length becomes derivable rather than
  supplied. StoreLayout owns the region offsets, which were open-coded nine
  times across four functions.
• DownloadState::Finalizing — every piece verified but the payload still
  being written out. Milliseconds on a small torrent, minutes on a large one,
  and previously invisible.
• Instrumentation. request_window and serve_latency trace targets, off
  unless asked for, emitting samples rather than pre-aggregated averages so the
  analysis can change without another run.
• scripts/test-repair.sh — two arms over the same deliberately damaged
  store, differing only in the flag byte, asserting the bitfield on disk rather
  than the reported piece count.
• scripts/console.sh plus a profiling profile, for tokio-console.
• Fixes: blocks dropped after an endgame cancel are now counted as wasted;
  test-inbound.sh waits for the extracted payload rather than a sampled log
  line; the pre-commit hook checks the whole workspace, which is why the GUI
  compiled against a stale enum for two commits.

Known gaps

• test-inbound.sh is intermittently flaky (~1 in 10). Not a download bug —
  the payload verifies every time. Completion lands between 1 Hz status samples
  and the process exits before Seeding 100.0% is ever published, so the test
  waits for a line that never comes. Needs an edge, not a poll.
• A store whose info hash does not match is never re-laid-out. It returns no
  bitfield without rewriting the identity on disk, so that store rediscards its
  progress on every start, silently and permanently.
• The request window still cannot span pieces. At 256 KiB that caps a peer
  at sixteen blocks in flight, and measured mean depth is ~2.6 of that. This is
  now the binding constraint on per-peer throughput, and the trade it protects —
  a dead connection strands at most one partial piece — is unchanged here.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved critical durability-contract findings and moderate resume, accounting, benchmark, test, and validation findings block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR defers .dhaar durability to resume-time verification and adds finalization tracking, geometry consolidation, instrumentation, repair tests, and tooling updates.

Changes:

• Replaces per-piece flushes with a clean trailer flag and resume repair.
• Adds Finalizing status, telemetry, and consolidated store layout.
• Updates tests, benchmarks, documentation, dependencies, and developer tooling.

File summaries

File	Summary
src/status.rs	Adds extraction progress and Finalizing state.
src/piece_manager/piece_writer.rs	Implements the new store layout and repair logic. Critical (3 votes): stale durability-barrier documentation at lines 329/373. Critical (2 votes): clean-flag writes require synchronization at lines 270/365. Moderate (1 vote): validate deserialized zero piece_length.
src/piece_manager/mod.rs	Integrates resume verification and finalization. Moderate (3 votes): recovered pieces do not restore counters or complete finalization.
src/peer_explorer/tracker/tracker_client_messages.rs	Updates compact-peer parsing.
src/peer_connection/request_manager.rs	Adds request timing and wasted-byte accounting. Three moderate findings (1 vote each): wasted blocks are not counted as downloaded at lines 619, 630, and 650.
src/peer_connection/mod.rs	Simplifies address initialization.
src/main.rs	Adds optional console tracing.
src/lib.rs	Propagates geometry and finalizing status. Moderate (1 vote): resumed complete stores can skip Finalizing and Seeding.
scripts/test-repair.sh	Adds crash-repair integration coverage.
scripts/test-inbound.sh	Updates asynchronous completion checks. Moderate (2 votes): the flaky status-log assertion remains.
scripts/console.sh	Adds a tokio-console runner.
scripts/bench-seeders.sh	Adds multi-seeder benchmarking. Moderate (3 votes): timing excludes extraction completion.
README.md	Updates performance and request-window documentation.
crates/dhaar-gui/src/main.rs	Displays finalization state.
Cargo.toml	Adds console and profiling configuration.
Cargo.lock	Locks new dependencies.
benches/piece_writer.rs	Updates writer benchmarks. Nit (1 vote): benchmark comments still describe removed durability barriers.
assets/benchmarks.svg	Updates benchmark visualization.
.husky/pre-commit	Checks the complete workspace.

Review details

Suppressed comments (9)

benches/piece_writer.rs:84

• These updated benchmarks no longer execute a durability barrier in set_bitfield, but the surrounding benchmark documentation still calls it a durability barrier and describes write_whole_piece as including one. The reported microsecond timings can therefore be interpreted as flush costs even though they measure unsynced writes; update the comments to match the new protocol.

            runtime().block_on(w.write(next_piece(), 0, data)).unwrap()

src/lib.rs:304

• This new state check uses DownloadStats, but initialization restores completed pieces only into PieceProgress; DownloadStats::completed_pieces is incremented only by piece_verified and remains zero on resume. A complete resumed store therefore skips Finalizing and is reported as Downloading until a piece is verified again. Base this branch on the restored progress counts (or seed the stats from the bitfield).

        } else if stats.is_complete() {
            DownloadState::Finalizing

src/peer_connection/request_manager.rs:619

• This received block is counted as wasted but not as downloaded. DownloadStats::add_downloaded is documented as counting every payload byte received, including worthless copies, so this early return underreports tracker downloaded and measured network traffic. Increment downloaded before wasted here.

            self.stats.add_wasted(block.len() as u64);

src/peer_connection/request_manager.rs:630

• This received block is counted as wasted but not as downloaded. DownloadStats::add_downloaded is documented as counting every payload byte received, including worthless copies, so this early return underreports tracker downloaded and measured network traffic. Increment downloaded before wasted here.

            self.stats.add_wasted(block.len() as u64);

src/peer_connection/request_manager.rs:650

• This late endgame copy crossed the network and is counted as wasted, but not as downloaded. That violates DownloadStats's documented byte accounting and makes tracker downloaded and measured traffic too low whenever a cancelled copy arrives. Increment downloaded before wasted here.

            self.stats.add_wasted(block.len() as u64);

src/piece_manager/mod.rs:145

• When an existing store returns Some(bitfield), this marks pieces complete but leaves DownloadStats.completed_pieces/verified_bytes at zero and never calls finalize for an already-complete store. As a result sample_status never reaches Finalizing/Seeding, the tracker continues to announce the full left value, and a missing output file is not recreated after restart. Initialize the counters from recovered pieces and trigger (or otherwise complete) finalization before publishing startup progress.

        if let Some(bitfield) = bitfield {
            for (index, piece) in self.pieces.iter_mut().enumerate() {
                piece.complete = bitfield.has_piece(index as u32);
            }

src/piece_manager/piece_writer.rs:373

• The clean bit is written before the match below creates and copies the extracted files. A crash or copy error after this write leaves the store marked tidy even though the output is partial or missing; on the next start the bitfield is trusted and the current startup path never retries extraction. Set the clean bit only after the extraction succeeds, and handle a complete resumed store on startup.

            file.write_all_at(&[flags.to_byte()], flags_at)

src/piece_manager/piece_writer.rs:368

• Because payload and bitfield writes are intentionally unsynced, this unsynced clean-flag write does not establish the ordering promised by the format. A power loss can persist the flag before dirty payload/bitfield pages, after which initialize trusts a claim whose data was not durable. Flush the store once before publishing the clean bit; this preserves the per-piece performance win while making the flag a valid durability marker.

        // Marking the store clean. Deliberately unsynced: if this byte is lost
        // to a crash the store reads as still open, which costs one needless
        // re-verification on the way back in and nothing else. The polarity is
        // what makes that safe — a lost write cannot fake a tidy exit.

src/piece_manager/piece_writer.rs:172

• piece_length is deserialized as a raw u64 with no validation, while the public torrent-construction path returns a Result. A torrent advertising zero now triggers this assertion during construction and panics instead of returning an input error. Reject or validate the geometry before constructing the writer, or make this constructor fallible.

        assert!(piece_length > 0, "piece length must be non-zero");

• Files reviewed: 17/19 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.