chadtmiller.com/knotless

Switch repository

Loading…

What changes

Oplog values are checked. A private repository's oplog read takes each record as its last write left it (ReadOn::changes), and checks the value's DRISL encoding (the new drisl::encode, with base32::decode for links) against that write's CID. Before, values went into the index unchecked: the set hash vouched for the CIDs, not what was stored under them. A last write with no value, which was silently skipped while its rev and set hash moved on, leaving the index stale for good, now fails the read, and the repo is read whole.

One repo no longer stops a space. sync_with reads every repo listed even when one fails. The error is still returned, and the space's cursor stays put, so the failing repo is listed and read again next time. A repo the listing names at the rev it was synced to already is skipped, so a held cursor costs little. follow marks the space moved after a failed sync too, since its other repos may have been read.

One sync of a space at a time. The follow loop, discover, keep/keep_read and organizations' resync could sync one space at once, and the slower could write older records and an older rev back over the newer. Index::syncing gives each space a lock held across its sync.

An oplog that repeats a page is refused rather than read forever.

Why

The oplog read trusted what it was given beyond the set hash: values went into the index unchecked, a missing last value left the index stale for good, one repo's failure stopped its whole space, and concurrent syncs could write older records back over newer. Fetching a missing value with getRecord was left out: with the reference PDS a value only goes missing in a race, and reading whole covers it without a path no test reaches.

How it was checked

Unit tests for base32::decode, drisl::encode (round trip through drisl::record, key order, refusals) and ReadOn::changes (last write wins, deletions, unwanted collections, missing, wrong and unencodable values), run locally; clippy over both crates' libraries. Every oplog value in the gateway's private-repository tests now goes through the CID check, which CI runs against the spaces alpha.

Shortcomings

An op's prev is still not checked against what was synced before (knotless keeps no record CIDs), so only the final set of records is vouched for, not each event between. A repo that keeps failing holds its space's cursor, and the listing from there grows until it reads again.

Changeset: none

This pull checks private-repo oplog values against their CIDs. It also lets a sync read past a failing repo, takes a per-space lock across sync_with, and refuses an oplog that repeats a page. The design reads sound to me: changes takes last-write-wins per record, drisl::encode sorts keys in DRISL order, and the sync_with callers I found don't nest the lock.

It can't merge yet. Every check failed at once because the head conflicts with main at 851c24511195 in crates/knotless-serve/src/spaces.rs. So none of the new code, including the 100% coverage gate, has run in CI. The PR body says CI runs the CID check, but it hasn't. Merge main into the branch, resolve the conflict, and re-run the checks. I'll look again once they've passed.

One thing to confirm when you do: the new fresh filter and the Err path in sync_with need tests that reach them. The coverage gate will fail the pull if they aren't covered.

The merge conflict is resolved, and check (fmt and clippy) passes. The test check fails, though. Every test passes, but the 100% coverage gate reports one uncovered function: crates/knotless/src/space_sync.rs:268, the is_some_and(|c| !seen.insert(c.clone())) closure in read_on. No test reaches the "oplog repeats a page" refusal. Add one that serves a cursor twice and asserts the error. If read_on can't be driven in a unit test, extract the repeat check into a small function that can. Once test is green I expect to approve; the rest of the change looked sound last round. e2e and docs were still queued when I looked.

The coverage gap from last round is closed. At 5b23fc6 every check passes, including test. That run reports 100% function and line coverage and has no uncovered function, so the "oplog repeats a page" refusal is now exercised. check, e2e and docs pass too, and the other workflows were skipped because their paths weren't touched. The CID checking, the sync that carries on past a failing repo, and the per-space lock all looked sound last round. Nothing further to ask for.

Carried forward: the change this pull makes at 0ac8fafa642b417cb61c9b87b944bda7b0441bbc is the one reviewed at 5b23fc6f3ee040a385e8fca004c9fbdac06b56eb, so that review stands. What is new comes from its base.