grenadeandClaude Opus 5
3790f2589c
The lock added alongside compare-and-set removed its map entry inside the critical section. A queued waiter still held that mutex, so the next arrival found no entry, created a fresh one, and the two then ran concurrently -- defeating the lock exactly when there was contention, which is the only time it does anything. I previously reported this fix as working on the strength of ten clean release-mode runs. That was wrong: in debug, where the timing differs, the guarding test fails 3 times in 12. Retiring the entry only after dropping our own reference, under the same shard lock `entry` takes, gives 0 failures in 25 -- and restoring the old behaviour reproduces 3 in 12, so the test really does catch it. The consequence was the one the test names: a file served to a client but never recorded, and so invisible the moment the instance is sealed. Real against the local filesystem store, which has no conditional put and relies on the verify-after-write retry; MinIO's compare-and-set covers the same race, which is why the deployed path never showed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZG2i4AmfSqE97EJGBVb64
89 lines
4.6 KiB
Markdown
89 lines
4.6 KiB
Markdown
# rustingface — notes for agents
|
|
|
|
Read [`doc/spec.md`](doc/spec.md) first; it is the contract, not a sketch. Then
|
|
read `~/git/architecture/generic.md` for the conventions this repo inherits
|
|
(workspace layout, deployment, systemd, firewalld, SELinux, commits) and
|
|
`port-allocations.md` for why the port is `20482`.
|
|
|
|
## What must not regress
|
|
|
|
The five guarantees in `doc/spec.md` §1 are contractual. In particular:
|
|
|
|
- **Never normalise a replayed header.** `ETag`, `X-Repo-Commit`,
|
|
`X-Linked-Etag` and `X-Linked-Size` are recorded as upstream sent them and
|
|
emitted unchanged, quotes included. Client cache layouts are keyed on these.
|
|
Do not compute a git blob SHA-1, derive an etag from content, or decide what
|
|
a commit "should" be.
|
|
- **Never let a redirect eat the Hub's headers.** The Hub answers a resolve
|
|
request with a redirect and puts the replayable metadata on *that* response;
|
|
the CDN it points at has none of it. `rustingface-data`'s upstream client
|
|
follows redirects by hand for this reason. If you ever re-enable reqwest's
|
|
own redirect policy, every recorded etag silently becomes wrong.
|
|
- **Never set `Content-Encoding` on the resolve path.** The client sends
|
|
`Accept-Encoding: identity` precisely so the sizes it learns are real.
|
|
- **Never write a manifest entry before its blob upload has completed.** The
|
|
ordering in `rustingface-core/src/fetch.rs` is deliberate: a crash may leak
|
|
an orphan blob, which `gc` reclaims, but a manifest entry pointing at
|
|
incomplete bytes is a silent correctness failure.
|
|
- **Never complete a client response before the manifest write lands.** The tee
|
|
runs one chunk behind for this reason. A file that was served but not
|
|
recorded is invisible the moment the instance is sealed.
|
|
|
|
## Testing
|
|
|
|
`cargo test --workspace` covers it, including `tests/sovereignty.rs`, which
|
|
runs the real service against a fake Hub speaking the real header protocol.
|
|
Add a case there for any behaviour a client depends on.
|
|
|
|
The conformance suite (`test/conformance/`) needs network and a pinned
|
|
`huggingface_hub`. Run it before claiming a change is safe for clients — most
|
|
of the real bugs in this repo's history were found by it and not by the unit
|
|
tests. It has also been run against the live MinIO on caveman; do that for
|
|
anything touching `rustingface-data`, because the local filesystem store hides
|
|
whole classes of problem (it has no multipart, no conditional update, and no
|
|
presigning).
|
|
|
|
## The web UI
|
|
|
|
`web/` is a Vite + React + SWC + TS app (`architecture/generic.md` §4), served
|
|
statically from the nginx host and reading the `/v1/` API.
|
|
|
|
- **Keep UI routes out of `/api/`.** That surface is the Hub's, replayed
|
|
verbatim. Anything rustingface invents goes under `/v1/`.
|
|
- **Model cards are untrusted third-party content.** `components/Markdown.tsx`
|
|
parses their HTML and then sanitises it; the plugin order (`rehype-raw`, then
|
|
`rehype-sanitize`) is not interchangeable, and reversing it would reintroduce
|
|
the raw HTML unchecked. Card images are deliberately not loaded.
|
|
- **Any new route must not shadow a repository id.** rustingface owns the whole
|
|
URL root, which is the Hub's namespace, so every prefix collides with some
|
|
repo. Where it is ambiguous, disambiguate in the handler the way
|
|
`v1_repo_detail` does — and test it against `/resolve/` in a *directory*
|
|
name, not just in a repo name.
|
|
|
|
## Auth, and what actually bounds the bucket
|
|
|
|
Two credentials that are easy to confuse, pointing in opposite directions:
|
|
|
|
- `upstream.token_file` (`hf-token`) authenticates **rustingface to the Hub**.
|
|
Outbound only, never echoed downstream, never logged. Needed for gated repos.
|
|
- `auth.token_file` (`client-tokens`) authenticates **clients to rustingface**.
|
|
|
|
A client's token is never forwarded upstream, and rustingface's token is never
|
|
shown to a client. Do not let these merge.
|
|
|
|
`auth.mode = "bearer"` controls *who*, not *what* — a token holder can still
|
|
cause any repository to be fetched. **`policy.allow_new_repos = false` is what
|
|
bounds the bucket**, and the deployed configuration relies on it.
|
|
|
|
`auth.anonymous = "catalog"` opens `/v1/` to unauthenticated requests. That is
|
|
only safe while every handler under `/v1/` reads the bucket and never consults
|
|
upstream. **If you add a route there that can fetch, you have made the public
|
|
face able to fill the bucket** — there is a test asserting the whole surface
|
|
causes zero upstream requests, and it should stay that way.
|
|
|
|
## Where things live
|
|
|
|
Types in `entities`, logic in `core`, I/O in `data`, HTTP in `api`, wiring in
|
|
the binary. `core` defines the `Store` and `Upstream` ports and must not gain a
|
|
direct dependency on `object_store` or `reqwest`.
|