feat(platform): D2: hold and scope artifacts by group - #552
Open
dannash100 wants to merge 55 commits into
Open
dannash100 wants to merge 55 commits into
dannash100 wants to merge 55 commits into
Conversation
1 task
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary Below consensus threshold (8 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
|
🦸 Review Hero Summary Below consensus threshold (9 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent) |
|
|
||
| ALTER TABLE artifacts DROP CONSTRAINT artifacts_type_platform_version_id; | ||
|
|
||
| CREATE UNIQUE INDEX artifacts_identity |
There was a problem hiding this comment.
[Bugs & Correctness] critical
CREATE UNIQUE INDEX artifacts_identity ... NULLS NOT DISTINCT is created with no dedup step, but the schema it replaces permitted exactly the duplicates it now forbids. The old constraint was UNIQUE (artifact_type, platform, version_id), and the range-registration path in public-server::artifacts::create was a plain INSERT with version_id = NULL — so every repeat registration of a range artifact (e.g. a releaser publishing 2.60.x/installer/windows twice) inserted another row, and the migration's own comment acknowledges range rows had "no uniqueness at all". On any database that has ever taken two registrations of the same range/type/platform, this CREATE UNIQUE INDEX fails and the whole migration (and deploy) aborts, with only a hand-fix on the box as recourse. Add a dedup before the index, keeping the newest row per identity, e.g. DELETE FROM artifacts a USING artifacts b WHERE a.artifact_type = b.artifact_type AND a.platform = b.platform AND a.version_id IS NOT DISTINCT FROM b.version_id AND a.version_range_pattern IS NOT DISTINCT FROM b.version_range_pattern AND a.group_id IS NOT DISTINCT FROM b.group_id AND (a.created_at, a.id) < (b.created_at, b.id); (or a row_number() variant), and consider a test that seeds duplicate range rows before running up.sql.
| // Try to parse as a specific version first | ||
| if let Ok(semver) = SemverVersion::parse(&version) { | ||
| // It's a specific version (e.g., "1.0.5") | ||
| let (version_id, version_range_pattern) = if let Ok(semver) = SemverVersion::parse(&version) { |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
The draft version is created before the body is validated as a location. resting() (inside ArtifactRow::register, line 199) is what refuses a blank/whitespace body, but by then the exact-version branch has already inserted a draft versions row for a version that got no artifact. So POST /artifacts/9.9.9/installer/windows with an empty body answers 400 while leaving a stray draft 9.9.9 in the version list, and repeating the mistake for other versions litters it further. The existing test doesn't catch this because it seeds 2.60.0 first. Validate the body up front (the same location()-style trim/emptiness check, alongside the parse_sri_opt call above) so the refusal happens before anything is written.
| artifact_type: type, | ||
| platform, | ||
| group_id: groupId, | ||
| digest: await digestOf(file), |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
await digestOf(file) runs inside the try whose catch is empty with the comment /* surfaced via action.error */ (line 714) — but if digestOf throws, no API hook was ever called, so upload.error is null and pending never set. The operator clicks Create and absolutely nothing happens: no error, no spinner, no row. This is reachable in practice: File.arrayBuffer() rejects when the picked file has since been moved or truncated on disk, and crypto.subtle is undefined outside a secure context, so an operator reaching the SPA over plain http:// on a tailnet host gets a silent TypeError (the e2e suite runs on http://localhost, which is a secure context, so it can't catch this). Catch the digest failure separately and surface it, e.g. set fileError to the thrown message before the upload call.
| ))); | ||
| } | ||
|
|
||
| let mut conn = state.db.get().await?; |
There was a problem hiding this comment.
[Performance] suggestion
upload_artifact takes a pool connection before it does any of the expensive/rejectable work: the size check (767), the SRI parse, and digest_of(&body) (775) — a SHA-256 over up to 32 MiB — all run while holding it. The write pool defaults to max_open = 5 (crates/database/src/lib.rs:115), so a handful of concurrent uploads can pin the whole pool for tens of milliseconds each of pure CPU, and an oversized upload burns a connection only to be refused. Hash and validate first, then state.db.get() immediately before Artifact::register.
| } | ||
|
|
||
| let claimed = parse_sri(&named.digest)?; | ||
| let digest = digest_of(&body); |
There was a problem hiding this comment.
[Performance] suggestion
digest_of(&body) hashes up to 32 MiB inline on the async worker, with no await in it — exactly the case the download path deliberately moves off the runtime (crates/public-server/src/versions.rs:689, "tens of milliseconds with no await in it"). Uploads should use the same tokio::task::spawn_blocking treatment, otherwise a few concurrent uploads stall unrelated requests on the same worker threads.
| // the same type and platform is registered. An artifact this caller may not | ||
| // see is missing in exactly the way one that never existed is. | ||
| // spec: ART#who-is-offered-a-group-scoped-artifact | ||
| let artifacts = ArtifactRow::get_for_version_all_matches(&mut db, version.id, scope).await?; |
There was a problem hiding this comment.
[Performance] suggestion
download_artifact issues two queries to fetch one artifact. get_for_version_all_matches selects version_id = X OR version_range_pattern IS NOT NULL, so it pulls every range artifact in the table regardless of version, node_semver-parses each pattern in Rust, sorts the whole set by specificity, and all of that just to confirm one UUID is visible — then content_for re-queries the same row by id for the bytes. This is on the hot download path that every fleet machine hits, and the first query's cost grows with the total number of range artifacts ever registered, not with the version. A single filter(id.eq(artifact_uuid)) query selecting the scope predicate plus (version_id, version_range_pattern, content, content_type, digest), with the range check applied to that one row, would be one round trip and O(1) rows scanned.
| let all_artifacts: Vec<Self> = table.select(Self::as_select()).load(db).await?; | ||
| let public_api_ids: std::collections::HashSet<Uuid> = scopes | ||
| .into_iter() | ||
| .flat_map(|scope| Self::offered_ids(&matching_artifacts, scope)) |
There was a problem hiding this comment.
[Performance] suggestion
For Scope::Fleet the offered-id computation is O(artifacts × distinct groups): offered_ids walks the entire matching_artifacts slice and allocates a fresh HashSet for every scope, and the scope list is one entry per distinct group present. Since each group contributing a scope also contributes at least one artifact, this is quadratic in the number of group-scoped artifacts on a version — a fleet with a few hundred groups each holding a reporting schema turns an operator listing into hundreds of full passes. A single pass over the sorted set inserting into a HashSet<(&str, &str, Option<Uuid>)> keyed by (type, platform, owning-scope) gives the same answer in linear time, since the sort already puts each scope's most specific artifact first.
| version_range_pattern, | ||
| group_id, | ||
| )) | ||
| .do_update() |
There was a problem hiding this comment.
[Security] suggestion
register's upsert changes the trust properties of the public releaser endpoint. Before this change the unique constraint on (artifact_type, platform, version_id) made re-registering an existing exact artifact a hard error; now do_update overwrites download_url, digest, device_id and run_id of whatever row is already there. So any device holding a releaser credential can silently repoint the download URL of an already-published release artifact registered by a different device, and — because digest.eq(&input.digest) is unconditional — a registration that simply omits ?digest= clears a previously recorded digest, turning off verification for every client that would have checked it, with no error and no trace (the original registrant's device_id is overwritten too). Note the private Artifact::update path deliberately only drops the digest when the location actually moved; the register path has no equivalent guard. If replacement is intended, consider at minimum keeping the recorded digest when the incoming registration names no digest and the URL is unchanged, and recording that a replacement happened so the provenance of the original registration is not lost.
| /// Base URL for absolute links Canopy emits about itself. Prefers the | ||
| /// configured `PUBLIC_URL`; otherwise reconstructs the origin from the | ||
| /// request's forwarded scheme and `Host` header so local and test runs still | ||
| /// emit well-formed links. |
There was a problem hiding this comment.
[Security] suggestion
public_base_url falls back to the unvalidated Host and x-forwarded-proto request headers when PUBLIC_URL is unset. That fallback used to feed only RSS <link> elements (feed_base_url); this change makes it build the download_url that fleet machines actually fetch artifacts from (Artifact::offered). A poisoned Host — via an upstream cache, or any proxy that forwards the client's value — turns the listing into a set of attacker-hosted download locations, and held artifacts carry no digest in the public response for the client to check against. Since these links point at Canopy itself, prefer emitting a path-relative URL, or require PUBLIC_URL (or an allowlist of known hosts) for the artifact URLs rather than reconstructing the origin from request headers.
|
🦸 Review Hero Summary (round 5) Below consensus threshold (4 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent) |
passcod
requested changes
Sep 14, 2026
Contributor
Author
|
🤖 Follow-up: moving held artifact content out of Postgres and into an S3 bucket, per the note on |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some artifacts are derived from one group's data and are wrong for anyone else, but every artifact is fleet-wide and every read is unauthenticated.
download_urlstays a required string. Nullable would break the published client, so a held artifact is offered Canopy's own download endpoint.CANOPY_ARTIFACT_BUCKETon both pods before deploy, or uploads and held downloads fail.🦸 Review Hero