chadtmiller.com/knotless
git that lives in your Atmosphere account
A read of a repository no longer waits on the sync a change started
MergedThis pull serves a public repository's last synced mirror, without taking the sync's lock, while the firehose is watched. It does so only for reads that follow no write from the pages. I found one problem: as written, it mostly won't serve a stale mirror in the case the pull is meant to fix.
served takes age from entry.synced, which is when the last sync finished, not when the stale event arrived. STALE_FOR is documented, in the PR body and in docs/design/serve.md, as ten seconds from the event, while the sync it started runs. Take a repository that was last synced minutes ago, which is the usual case. A push event arrives, and the background heard_of sync takes the lock. A read then finds quiet == false and age in minutes, so serves_last is false and the read queues on the lock as before. The stale path only applies to a repository that was also synced within the last 10 s. The unit test passes age in directly, so it can't catch this.
To fix it, record when the mirror first became stale (for example, the instant of the first event not yet settled), or the instant the in-flight sync began, and measure age from that. Then add a test that goes through served (a repository synced long ago, an event, then a read during the sync), not only through serves_last.
Smaller points:
- The new
served,rememberandadmitbranches need coverage to pass the 100% gate. The PR body says only the quiet read ofttl_and_firehose_freshnessexercises the new path, so check that the stale branch and the removal of a missing repository are reached. - The docs addition says "ten seconds" without saying from when. Update it to match whichever fix you make.
crates/knotless-serve/src/lib.rsline 587Outdated
let last = self.last.lock().unwrap(); | |
let entry = last.get(&(did.to_string(), name.to_string()))?; | |
let quiet = entry.events == self.stamp(did, entry.key(did).as_deref()); | |
let age = entry.synced.map(|at| at.elapsed()).unwrap_or(STALE_FOR); |
age is the time since the last sync finished, not since the mirror went stale. A repository last synced more than 10 s ago, then hit by an event, is !quiet with age >= STALE_FOR, so it is not served and the read waits on the lock as before. Measure from the first unseen event or the start of the in-flight sync.
The earlier request is done. heard_of now records stale_since on the kept entries of the owner's repositories when the first event arrives. serves measures the ten seconds from that instant, not from the last sync. A repository synced long ago, then hit by an event, is therefore served stale while the sync runs.
A new sync stores an entry with stale_since of None. If an event arrives mid-sync and the entry stays unsettled, reads wait on the lock again. That is conservative, so it is safe. A write from the pages still bumps writes, so the writer's reads wait.
The unit test now covers the quiet, stale-without-a-note, stale-too-long and fresh-stale cases. It still tests serves directly, not the whole of served/public. I'd like the 100% coverage gate to confirm that the served early-return and the remember removal branch are reached, but CI enforces that and I'm not withholding approval for it. The docs now say "from the first change".
The change since the approved commit is small and doesn't break what I approved. public now takes an owner flag, true when the signed-in viewer is the repository's owner. The owner skips the stale-served path and waits for the sync as before, so a push or command-line write they just made is seen. Other readers still get the last synced mirror during the sync. The docs say so. The change is conservative, since it only narrows when stale serving applies. I'd want a test that an owner read waits while a non-owner read is served stale, but CI's coverage gate will show whether that branch is reached, and I'm not withholding approval for it.
The change since the approved commit stands. The background refresh a change starts used to call mirror. With stale serving in place, that call could be served the stale mirror and return without syncing. The refresh now goes through located_with(.., true), and wait is folded into owner, so public skips the stale-served path and does the sync. That fixes a real gap. located and mirror behave as before for every other caller. Private repositories take the same path as before, since wait only affects the owner flag passed to public. I'd want a test that the refresh syncs while a non-owner read is served stale, but CI's coverage gate will show whether that branch is reached, and I'm not withholding approval for it.
The change since the approved commit stands. It flips the default. located now always waits for the sync, so anything that derives and keeps state from the mirror is safe by default. The new located_for_reader allows stale serving. Only the web Repo::open and Repo::find use it, and those answer page requests. Every other caller of located now waits as it did before this pull, so the change only narrows where stale serving applies. I have no new findings. CI's coverage gate will show whether located_for_reader is reached, and I'm not withholding approval for that.
The change since the approved commit stands. The feed's push events show the mirror next to what the index holds of the change. They now open the repository with Repo::open_synced, which uses located and waits for the sync, so a pushed commit the feed lists can be found. Other page requests keep the stale-served path. This only narrows where stale serving applies, and I have no new findings. CI's coverage gate will show whether open_synced is reached, and I'm not withholding approval for that.
The change since the approved commit stands. Stale serving now applies only to the five XRPC code reads (getRepo, getTree, getLastCommits, listBranches, listTags), through the new found_for_reader and Repo::find_for_reader. Repo::open and Repo::find go back to located, so they wait for the sync. open_synced is removed. This only narrows where stale serving applies, so it is safe. The docs match.
One consequence to know about: the page's own HTML request (Repo::open) now waits on the sync again. The PR body names that request as one of the slow ones, so the fix covers fewer of the stalls than the body says. Either narrow the body or add a reader variant of open for pages that show only the mirror. I'm not withholding approval for that. CI's coverage gate will show whether found_for_reader and find_for_reader are reached.
The change since the approved commit is one line. It qualifies the Located parameter type of Repo::moved as crate::Located. That is a type-path fix with no behavior change, so what I approved stands. I have no new findings.
1 passed, 2 skipped
What changes
A read of a repository does not wait on the sync a change started. While the firehose or Jetstream is watched, a public repository's mirror that events have made stale is served as last synced for up to ten seconds, without taking the lock the sync holds. A write from the pages still waits for its sync: reads after it block as before, so whoever wrote sees it.
Why
Measured on knotless.dev (
knotless admin logs --message "mirror sync", 500 syncs over six hours): median 270 ms, p90 1020 ms, max 3.5 s, 233 s in all, and 220 ms or more of each is reading the repository's records. Each sync holds the repository's lock, so every read behind it waits, and a repository page sends its own request and five XRPC calls at once. Page requests that took a sync's time were common: warm repos at 300 to 500 ms, one request in the sample at 1.1 s for a 25 ms sync of its own because it queued, and cold ones after a deploy at 4 to 6 s. 97 of the 500 syncs were run by a read itself on a warm repository (average 358 ms), and the rest by the background refresh a change starts, which reads queue behind.Other options weighed: reading only the changed records would cut each sync but needs the PDS's record diff and a new reconciliation of the mirror, a larger change that this one does not rule out. Skipping the wait when the index already has the write does not help, since the mirror is what is stale, not the index. Serving the last synced state is per repository, keeps no global lock, and a write seen by the pages keeps read-your-writes.
How it was checked
A unit test of the rule (
a_stale_mirror_is_served_while_it_syncs). The watched, quiet read ofttl_and_firehose_freshnessgoes through the new path. The rest is left to CI. After it deploys the same log query is run again for the after numbers.Shortcomings
A write made elsewhere, such as a push, was seen by the first read after its event. It is now seen after the sync that event starts finishes, up to the sync's length later. Before the event it was not seen either. A sync that fails leaves a stale mirror served for ten seconds, then reads wait again.
🤖 Generated with Claude Code