fix(core): the per-key manifest lock did not actually exclude
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
This commit is contained in:
@@ -6,7 +6,7 @@
|
||||
|
||||
use std::sync::Arc;
|
||||
|
||||
use rustingface_entities::config::AuthMode;
|
||||
use rustingface_entities::config::{Anonymous, AuthMode};
|
||||
use rustingface_entities::error::{Error, Result};
|
||||
use subtle::ConstantTimeEq;
|
||||
|
||||
@@ -15,6 +15,7 @@ use subtle::ConstantTimeEq;
|
||||
pub struct Tokens {
|
||||
mode: AuthMode,
|
||||
accepted: Vec<String>,
|
||||
anonymous: Anonymous,
|
||||
}
|
||||
|
||||
impl Tokens {
|
||||
@@ -23,6 +24,15 @@ impl Tokens {
|
||||
/// One token per line; blank lines and `#` comments are ignored so an
|
||||
/// operator can annotate who each token belongs to.
|
||||
pub fn new(mode: AuthMode, token_file_contents: Option<&str>) -> Result<Self> {
|
||||
Self::with_anonymous(mode, token_file_contents, Anonymous::Nothing)
|
||||
}
|
||||
|
||||
/// As [`Tokens::new`], stating what an unauthenticated request may reach.
|
||||
pub fn with_anonymous(
|
||||
mode: AuthMode,
|
||||
token_file_contents: Option<&str>,
|
||||
anonymous: Anonymous,
|
||||
) -> Result<Self> {
|
||||
let accepted: Vec<String> = token_file_contents
|
||||
.unwrap_or_default()
|
||||
.lines()
|
||||
@@ -38,7 +48,16 @@ impl Tokens {
|
||||
.into(),
|
||||
));
|
||||
}
|
||||
Ok(Self { mode, accepted })
|
||||
Ok(Self {
|
||||
mode,
|
||||
accepted,
|
||||
anonymous,
|
||||
})
|
||||
}
|
||||
|
||||
/// Whether a request with no valid token may read the catalogue.
|
||||
pub fn anonymous_may_browse(&self) -> bool {
|
||||
self.anonymous == Anonymous::Catalog
|
||||
}
|
||||
|
||||
/// Whether any authentication is required.
|
||||
@@ -81,6 +100,7 @@ impl Default for Tokens {
|
||||
Self {
|
||||
mode: AuthMode::None,
|
||||
accepted: Vec::new(),
|
||||
anonymous: Anonymous::Nothing,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -163,6 +163,56 @@ pub async fn detail(State(state): State<AppState>, rest: String) -> Result<Respo
|
||||
.into_response())
|
||||
}
|
||||
|
||||
/// `GET /v1/repos/{type}/{repo_id}/card`
|
||||
///
|
||||
/// The model card, as markdown, read straight out of the bucket.
|
||||
///
|
||||
/// The card is catalogue material, so it lives on the catalogue surface
|
||||
/// rather than being read through `/resolve/` like any other file. That is
|
||||
/// what lets an anonymous visitor read a card without also being able to
|
||||
/// download the weights beside it — and it means browsing never causes a
|
||||
/// fetch, because this only ever reads what is already stored.
|
||||
pub async fn card_markdown(
|
||||
state: &AppState,
|
||||
repo_type: RepoType,
|
||||
repo_id: &RepoId,
|
||||
) -> Result<Response, ApiError> {
|
||||
let commit = match state.registry.load_ref(repo_type, repo_id, "main").await? {
|
||||
Some(record) => Some(record.commit),
|
||||
None => state
|
||||
.registry
|
||||
.list_refs(repo_type, repo_id)
|
||||
.await?
|
||||
.first()
|
||||
.map(|r| r.commit.clone()),
|
||||
};
|
||||
let Some(commit) = commit else {
|
||||
return Err(ApiError(Error::RevisionNotFound(format!(
|
||||
"{repo_id} has no pinned revision"
|
||||
))));
|
||||
};
|
||||
|
||||
let manifest = state
|
||||
.registry
|
||||
.load_manifest(repo_type, repo_id, &commit)
|
||||
.await?
|
||||
.ok_or_else(|| Error::RevisionNotFound(format!("no manifest for {repo_id}@{commit}")))?;
|
||||
|
||||
// A card that was never fetched is a 404, not a fetch. The registry
|
||||
// records what it has actually seen, and browsing must not change that.
|
||||
let entry = manifest.entry("README.md").ok_or_else(|| {
|
||||
Error::EntryNotFound(format!("no README.md is stored for {repo_id}@{commit}"))
|
||||
})?;
|
||||
|
||||
let markdown = state.registry.read_stored_text(entry).await?;
|
||||
telemetry::request("v1_card", 200);
|
||||
Ok((
|
||||
[(http::header::CONTENT_TYPE, "text/markdown; charset=utf-8")],
|
||||
markdown,
|
||||
)
|
||||
.into_response())
|
||||
}
|
||||
|
||||
/// `GET /v1/status`
|
||||
///
|
||||
/// What this instance is, for a viewer deciding whether it can be trusted to
|
||||
|
||||
@@ -65,12 +65,23 @@ async fn authenticate(State(state): State<AppState>, request: Request, next: Nex
|
||||
if path == "/healthz" || path == "/metrics" {
|
||||
return next.run(request).await;
|
||||
}
|
||||
|
||||
let presented = request
|
||||
.headers()
|
||||
.get(http::header::AUTHORIZATION)
|
||||
.and_then(|v| v.to_str().ok());
|
||||
|
||||
if let Err(err) = state.tokens.check(presented) {
|
||||
return ApiError(err).into_response();
|
||||
// `anonymous = "catalog"` opens the `/v1/` surface to a request with
|
||||
// no valid token. It is safe to open precisely because every handler
|
||||
// beneath it reads the bucket and nothing else: an anonymous visitor
|
||||
// can see what is stored but cannot cause anything to be stored, and
|
||||
// cannot download a weight — which is what would make serving this
|
||||
// publicly an act of redistribution.
|
||||
let catalogue = path == "/v1" || path.starts_with("/v1/");
|
||||
if !(catalogue && state.tokens.anonymous_may_browse()) {
|
||||
return ApiError(err).into_response();
|
||||
}
|
||||
}
|
||||
next.run(request).await
|
||||
}
|
||||
@@ -103,6 +114,16 @@ async fn v1_repo_detail(
|
||||
)
|
||||
.await;
|
||||
}
|
||||
// `…/card` is the only suffix this surface takes; anything else is the
|
||||
// repository detail.
|
||||
if let Some(head) = rest.trim_end_matches('/').strip_suffix("/card") {
|
||||
let segments: Vec<&str> = head.trim_matches('/').split('/').collect();
|
||||
if segments.len() >= 2 {
|
||||
let repo_type: RepoType = segments[0].parse()?;
|
||||
let repo_id: rustingface_entities::repo::RepoId = segments[1..].join("/").parse()?;
|
||||
return handlers::inventory::card_markdown(&state, repo_type, &repo_id).await;
|
||||
}
|
||||
}
|
||||
handlers::inventory::detail(state, rest).await
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user