From daaca3f479fc1cec2a2c07f3bdc65b2e243e04ae Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 1 Aug 2026 09:09:54 -0400 Subject: [PATCH] refactor: simplify artifact ZIP download Path safety now lives in one place. The NUL-byte and drive-letter rules move from a server-only helper into the store's own filename validation, so uploads reject those paths at write time instead of only the ZIP read path catching them. The download still re-checks, because artifacts stored before the rule existed can still carry an unsafe path, but it now skips a bad path rather than failing the whole archive. Promote is_boundary_stage to RunProjection and drop the three identical private copies. The ZIP download used a node-name match instead, which would have dropped artifacts from a working node that happened to be named "start". Compress the archive. Entries were Stored while the response was also excluded from transfer compression, so text artifacts moved at full size. async_zip gains the deflate feature; async-compression and flate2 were already in the lock file. Log archive failures unconditionally. The send-succeeded guard meant a client that had already disconnected left no record at all, which is the case where the log is the only evidence. Also: collapse the duplicate 500 arms, drop the dead stage-ID tiebreaker and the cached order in the selection map, name the accessible label after the visible one, share the run URL prefix between the two download href builders, and document the mid-stream truncation behavior in the OpenAPI description. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 2 + Cargo.toml | 2 +- apps/fabro-web/app/lib/api-client.ts | 18 ++- apps/fabro-web/app/routes/run-artifacts.tsx | 3 +- docs/public/api-reference/fabro-api.yaml | 6 +- .../src/server/handler/artifacts.rs | 142 ++++++++---------- .../src/server/handler/billing.rs | 11 +- lib/apps/fabro-server/src/server/tests.rs | 17 +-- .../fabro-store/src/artifact_store.rs | 32 +++- lib/components/fabro-store/src/run_state.rs | 11 +- .../fabro-workflow/src/billing_rollup.rs | 11 +- .../fabro-types/src/run_projection.rs | 14 ++ .../src/api/run-internals-api.ts | 8 +- 13 files changed, 145 insertions(+), 132 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 784dc1357..aa1d75762 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -338,6 +338,7 @@ checksum = "e79b3f8a79cccc2898f31920fc69f304859b3bd567490f75ebf51ae1c792a9ac" dependencies = [ "compression-codecs", "compression-core", + "futures-io", "pin-project-lite", "tokio", ] @@ -408,6 +409,7 @@ version = "0.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0d8c50d65ce1b0e0cb65a785ff615f78860d7754290647d3b983208daa4f85e6" dependencies = [ + "async-compression", "crc32fast", "futures-lite", "pin-project", diff --git a/Cargo.toml b/Cargo.toml index 702b4e77e..53ff423f3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,7 +36,7 @@ dotenvy = "0.15" futures = "0.3" tokio-stream = "0.1" async-trait = "0.1" -async_zip = { version = "0.0.18", features = ["tokio"] } +async_zip = { version = "0.0.18", features = ["tokio", "deflate"] } fs2 = "0.4" base64 = "0.22" bytes = "1" diff --git a/apps/fabro-web/app/lib/api-client.ts b/apps/fabro-web/app/lib/api-client.ts index 4c298fd0c..bb59e3607 100644 --- a/apps/fabro-web/app/lib/api-client.ts +++ b/apps/fabro-web/app/lib/api-client.ts @@ -408,6 +408,13 @@ export function requestSignalOptions(request?: Request): RawAxiosRequestConfig { return request?.signal ? { signal: request.signal } : {}; } +/** Absolute href for a path under a run, for links the browser follows itself. */ +function runApiPath(id: string, suffix: string): string { + return `${generatedApiConfiguration.basePath ?? ""}/api/v1/runs/${ + encodeURIComponent(id) + }${suffix}`; +} + export function stageArtifactDownloadUrl( id: string, stageId: string, @@ -418,13 +425,12 @@ export function stageArtifactDownloadUrl( filename, retry: String(retry), }); - return `${generatedApiConfiguration.basePath ?? ""}/api/v1/runs/${ - encodeURIComponent(id) - }/stages/${encodeURIComponent(stageId)}/artifacts/download?${searchParams}`; + return runApiPath( + id, + `/stages/${encodeURIComponent(stageId)}/artifacts/download?${searchParams}`, + ); } export function runArtifactsDownloadUrl(id: string): string { - return `${generatedApiConfiguration.basePath ?? ""}/api/v1/runs/${ - encodeURIComponent(id) - }/artifacts/download`; + return runApiPath(id, "/artifacts/download"); } diff --git a/apps/fabro-web/app/routes/run-artifacts.tsx b/apps/fabro-web/app/routes/run-artifacts.tsx index c03d68875..0f75d6a2e 100644 --- a/apps/fabro-web/app/routes/run-artifacts.tsx +++ b/apps/fabro-web/app/routes/run-artifacts.tsx @@ -138,8 +138,7 @@ function ArtifactList({ runId, files }: { runId: string; files: readonly Artifac