chadtmiller.com/knotless
git that lives in your Atmosphere account
A private repository's readers sync it once its syncer has not lately
MergedThis adds a last_synced map, set by the sweep and by reader syncs. A read of a private repository whose space hasn't synced in 60 seconds now syncs it with the reader's credential before answering. I couldn't run the test, since it needs the spaces alpha.
A failed stale sync now fails the read. Before this change, a space that was already synced and had a syncer was never synced on read, so a read could not fail because of a sync. Now keep_read and keep return AppError::upstream when the reader's own sync fails on a space that was already synced. That happens when the space's authority is briefly unreachable or the reader's grant is poor. last_synced is only set on success, so every read retries, with no backoff. Readers of a healthy repository then get 502s where they used to get the slightly old index. When synced was already true, a failed stale sync should be logged and the read served from the index. Only the never-synced case should return the error.
Doc comment misplaced. In lib.rs the new synced_now is inserted between the doc comment of moved and moved itself. The two doc blocks merge onto synced_now, and moved loses its doc. Put the new functions before that comment.
The docs update and the test are fine. Unlike keep_read, keep already syncs as the reader, since follow_space makes them the syncer. That is consistent with what the pull describes.
crates/knotless-serve/src/private.rsline 281Outdated
if let Some(read) = &read { | |
self.read_now(uri, read); | |
} | |
read.transpose().map(drop).map_err(AppError::upstream) |
When synced was already true and the stale sync fails, this returns an upstream error for a read that used to succeed from the index. Serve the index and log instead; keep the error for the never-synced case. The same applies in keep at the crate::spaces::sync call.
crates/knotless-serve/src/lib.rsline 505Outdated
pub(crate) fn sync_stale(&self, uri: &str) -> bool { | |
let last = self.last_synced.lock().unwrap(); | |
last.get(uri).is_none_or(|at| at.elapsed() >= spaces::STALE) | |
} |
The doc comment of moved ("Records that the space uri was written to…") now sits above synced_now, so moved has none. Move the new functions above that comment.
Both fixed in the latest push. A reader's sync now goes through read_now: on a space already synced, a failure is logged (space not synced) and the read is answered from the index as it was, and the failed attempt counts as a sync, so it is tried again only after STALE, not at each read. Only a space never synced fails the read, as before. synced_now and sync_stale now sit above moved's doc comment. docs/design/access.md says the same.
Both points from my earlier review are done. I couldn't run the test, since it needs the spaces alpha.
- Failed stale sync:
read_nowinprivate.rsmarks the space as synced even when the sync failed, if it had been synced before. It logs the failure and serves the read from the index. Only a space that was never synced fails the read with an upstream error. This also gives a retry backoff of oneSTALEinterval, so failed syncs no longer retry on every read. - Doc comment:
synced_nowandsync_stalenow sit before the doc comment ofmoved, somovedkeeps its doc.
The docs in access.md match the new behavior. The test covers the lapsed-syncer case.
The code is unchanged in substance since my last approval. read_now still serves a stale read from the index when the reader's sync fails on a space that was synced before, and it counts that failed sync as made, so it isn't retried until the next STALE interval. A never-synced space still fails the read. The new commit adds a takedown case to the test. A member whose repo can't be read makes the stale sync fail. The test checks that the read still answers from the old index, that last_synced is set, and that the issue shows up once the member is restored. The docs in access.md describe this. I couldn't run the test, since it needs the spaces alpha.
9 passed, 1 skipped
What changes
A private repository's index no longer stays behind when the member it is synced as can no longer sync it. knotless-serve remembers when each space was last synced, by any path, and a member reading it (logged in to the pages, or with a delegation token) syncs it with their own credential before the answer when that was more than a minute ago, two of the 30-second sweeps, or not since the server started.
Why
A space is synced as one member's session here, its syncer: the owner's, for a repository made with
knotless create --private. A read synced it itself only if it was never synced (keep), or, with a delegation token, if no session synced it (keep_read). So when the syncer's sync failed without being refused (its session lapsed, or its grant leaves the space out),spaces::followloggedspace not syncedevery sweep and nothing else took it up. Every write since was missing for every reader until the syncer logged in again.Seen on knotless.dev with chadtmiller.com/group-host: asked as a member with a delegation token,
getRepoanswers the repository withopenPulls: 0andlistItemsno pulls, while its space holds three open pulls, which the CLI lists and the runner checked. Asking again changed nothing.Syncing on read only once the syncer has evidently stopped, rather than on every read, keeps a healthy repository's reads as cheap as before: a syncer that works syncs it every 30 seconds, and its readers never do.
How it was checked
private_repositories_synced_by_readers_once_their_syncer_lapsesintests/gateway.rs: the owner reads the repository on the pages and becomes its syncer, their session is then removed while they stay the syncer, a member opens an issue, and once the space counts as not synced lately, the member's delegatedlistItemslists it. Before this change the read takes nothing in, and the list is empty. Built here; the tests are left to the runner, as they need the spaces alpha.Shortcomings
A lapsed syncer stays the syncer: each sweep still fails and logs
space not synceduntil they log in again or someone else reads it on the pages. Moving the syncer to a reader whose session works would stop that, and is a larger change. A read of a stale space waits for its sync; one whose sync fails is answered from the index as it was, the failure logged, and tried again a minute on.Review
keepandkeep_readincrates/knotless-serve/src/private.rs, andSTALEinspaces.rs.