chadtmiller.com/knotless

Switch repository

Loading…

What changes

A space renews what lets it in. A Space holds its credential, or its owner's session, behind a lock its clones share, and each read takes it through Space::auth or Space::for_repo, now async. If it ends within a minute, the read renews it first, the way session::ensure renews a session: from the credential kept on disk if another read renewed it already, otherwise with a fresh delegation token and exchange from the reader's session. An owner's session is refreshed through session::ensure. All of a space's clones, and every Space::at of it, share that one renewal.

Space::owner takes the Ctx it renews under, and Space::kept takes the authority's PDS. Space::member and the public auth field are gone.

Why

knotless-run on 178.105.255.120 watches the private chadtmiller.com/hetzner-infra as infra.chadtmiller.com, whose space is on a pds.js worker. About ten minutes after it starts, every pass fails with JwtExpired (401): token expired from com.atproto.space.getRecord, until it restarts. The runner opens each watched repository once (Runner::named_watch, Remote::open) and keeps that Remote in its Watch for as long as it runs. Remote::open joins the space then, so the Space it keeps holds the credential from that moment, and Remote::read and get_blob sent it on every read after it expired. The paths that call space::joined each time (the reporter's mine, space_moved, register) kept renewing the credential on disk, so the disk held a fresh one while the passes used the old one. On the box, the credential file was minted at 21:04:54 and valid until 21:14:54, but passes were failing from 21:06. A restart at 21:04 had picked up a credential cached at about 20:56, which still had more than its minute left.

runner.knotless.dev does not show this because the repositories it watches are public: Remote::open finds their records in the public repo, and no Space is involved. A runner that owns the private repository it watches would have hit the same bug once its session's access token expired, because Space::owner froze session.auth(). Both cases are covered.

Renewing inside Space fixes every long-lived holder at once: the watch, the runs it spawns, and the forks a pass opens. The alternative, re-opening the Remote in the runner at each pass, would leave runs and their readers holding the old credential.

How it was checked

Unit tests in space.rs run against an axum stand-in for the authority. It hands out ten-minute credentials, counts exchanges, and answers listRepos with JwtExpired when a credential or session has ended, as the alpha and pds.js do:

• a_credential_held_past_its_end_is_renewed: a space whose credential ended is renewed with one exchange. Its clones, its at, and a second space of the same reader pick up that renewal, the second from the disk cache. With no session left, the read fails with "not logged in".
• an_owner_session_held_past_its_end_is_renewed: an owner's ended session is replaced by the one on disk, with no exchange.
• signature_bases is rewritten for the new API.

The rest is left to CI.

Shortcomings

A space read with a credential that was handed over as a delegation token, with no session of the reader's (the appview's Remote::delegated and the no-session branch of Remote::open), cannot mint another. Once its credential runs out it fails with "not logged in", where before it failed with JwtExpired from the authority. The deployed runner at 178.105.255.120 needs this release before the errors stop.

Review

Start at Space::held and Renew::renewed in crates/knotless/src/space.rs.

🤖 Generated with Claude Code

This puts a space's credential, or its owner's session, behind a mutex that all clones share, and renews it when it ends within a minute. It looks for a fresh credential on disk first, and otherwise exchanges a new one from the reader's session. The design is sound and fits the root cause in the body: a long-lived Remote kept the credential from open and never renewed it. I found nothing that must change.

• Renewal path: Held::ending and Renew::renewed match what session::ensure does. The lock is held across the renewal, so concurrent readers make one exchange. A failed renewal leaves the old value in place, so the next read tries again.
• Call sites: no Space::member call and no use of the auth field remain. Space::owner has the new ctx argument at every call site, and space.auth() is awaited in remote.rs and space_sync.rs.
• Housekeeping: the pull includes the changeset and the access.md update that AGENTS.md asks for.
• Tests: they cover the cases the body names. These are a credential renewed once across clones, at and another reader via the disk cache, "not logged in" with no session, and an owner session picked up from disk.
• Checks: the earlier head (10c2d7d) failed check (clippy), test and e2e (build), so I assume the current head fixes the compile and lint errors. At 9cd5504, check, test, e2e and docs are still queued, so I can't confirm that. Wait for check and test, which include the 100% coverage gate, before queueing it. The renewal error paths and Space::kept are the likeliest places for an uncovered line.
• Known limit: as the body says, a space joined from a delegation token alone now fails with "not logged in" once its credential ends. That is accepted.

Since my last review, the only change is f73ccd6. It boxes the Renew::renewed future in Space::held (space.rs) so the large renewal future isn't embedded in every read's future. The behavior is unchanged: the lock is still held across the renewal, and a failed renewal still leaves the old value in place. I found nothing that must change.

check and test are still queued at this head, so I can't confirm they pass. Don't merge before they do, since test includes the coverage gate.

Since my last review there is one new commit, 05a92c5. It makes the test in a_credential_held_past_its_end_is_renewed call listRepos directly with the ended credential and assert the authority answers JwtExpired. I read it as a fix for the test failure at f73ccd6. That run failed the 100% coverage gate, and the only uncovered lines were space.rs:825-827, the stand-in's expired branch, which no test reached. The new call reaches that branch, and it also shows the stand-in refuses an ended credential, so the test proves more than before. I found nothing that must change in the code.

I haven't confirmed the fix. check and test at 05a92c5 were still running when I looked. Before it merges, make sure test passes, in particular that the coverage report no longer lists space.rs lines. The earlier report also listed space.rs:228-229 (the closure in Space::held) among its region-level misses. That is not a failing line, but look at it if the gate still complains.