From 8b767c658b2fa036916962cac98e394479d0ec48 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 1 Aug 2026 09:20:40 -0400 Subject: [PATCH] fix: make the artifact tie-break match the artifacts page Two artifacts can share a filename, a retry, and an absent stage, in which case the winner was whichever the object store listed first. Break the tie on the serialized stage ID, which is the third key the artifacts page sorts on. Compare the `node@visit` string rather than StageId's own ordering: the page compares the string, so "unknown@2" beats "unknown@10" there and now here too. The spec said captures from the `start` and `exit` nodes are excluded, but the exclusion is by handler type, so a node named `start` that does real work keeps its artifacts. Say that instead. Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/api-reference/fabro-api.yaml | 4 ++-- .../src/server/handler/artifacts.rs | 21 +++++++++++++----- lib/apps/fabro-server/src/server/tests.rs | 22 ++++++++++++++++++- .../src/api/run-internals-api.ts | 8 +++---- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 8f764d069..878095624 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -3289,8 +3289,8 @@ paths: summary: Download Run Artifacts description: | Streams a ZIP archive with the latest captured version of each artifact path. - Stage order and retry number determine the latest version, matching the artifacts page. - Captures from the `start` and `exit` control nodes are excluded. + Stage order, retry number, and then stage ID determine the latest version, matching the artifacts page. + Captures from the graph's boundary nodes are excluded, identified by their `start` and `exit` handler type rather than by node name. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. diff --git a/lib/apps/fabro-server/src/server/handler/artifacts.rs b/lib/apps/fabro-server/src/server/handler/artifacts.rs index 408636699..f426a0007 100644 --- a/lib/apps/fabro-server/src/server/handler/artifacts.rs +++ b/lib/apps/fabro-server/src/server/handler/artifacts.rs @@ -184,9 +184,12 @@ enum ArtifactArchiveError { /// One entry per artifact path, holding the newest capture of that path. /// -/// Newest means latest stage, then latest retry. Stages the projection does not -/// know about sort oldest (`None` < `Some`), which is what the artifacts page -/// does too. Boundary stages are dropped: they run no work, so anything they +/// Newest means latest stage, then latest retry, then highest stage ID. That +/// last tiebreaker only decides between two stages the projection does not know +/// about, which both sort oldest (`None` < `Some`); it is here so the winner +/// does not depend on the order the store happens to list objects in. All three +/// keys mirror the artifacts page, which sorts on the same triple and takes the +/// last entry. Boundary stages are dropped: they run no work, so anything they /// captured was already in the workspace. /// /// Paths are re-checked here rather than trusted: a path stored before a @@ -202,8 +205,16 @@ fn latest_run_artifacts( .enumerate() .map(|(order, (stage_id, _))| (stage_id.clone(), order)) .collect::>(); - let capture_rank = - |artifact: &NodeArtifact| (stage_order.get(&artifact.node).copied(), artifact.retry); + // The stage ID compares as its serialized `node@visit` form, matching the + // string the artifacts page sorts on rather than `StageId`'s own ordering, + // which compares the visit numerically and would disagree. + let capture_rank = |artifact: &NodeArtifact| { + ( + stage_order.get(&artifact.node).copied(), + artifact.retry, + artifact.node.to_string(), + ) + }; let mut latest_by_path: HashMap = HashMap::new(); for artifact in entries { diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 30f49e621..d3f08e7e1 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -10672,6 +10672,21 @@ async fn run_artifacts_download_streams_latest_files_as_zip() { "control-exit.txt", &b"excluded"[..], ), + // Neither stage reached the projection, so both rank equally on stage + // order and retry. The serialized stage ID breaks the tie the same way + // the artifacts page does: "unknown@2" sorts above "unknown@10". + ( + StageId::new("unknown", 10), + 1, + "orphan.txt", + &b"visit ten"[..], + ), + ( + StageId::new("unknown", 2), + 1, + "orphan.txt", + &b"visit two"[..], + ), ] { state .artifact_store @@ -10723,7 +10738,11 @@ async fn run_artifacts_download_streams_latest_files_as_zip() { .iter() .map(|entry| entry.filename().as_str().unwrap().to_string()) .collect::>(); - assert_eq!(names, vec!["logs/run.txt", "reports/result.txt"]); + assert_eq!(names, vec![ + "logs/run.txt", + "orphan.txt", + "reports/result.txt" + ]); let mut contents_by_name = HashMap::new(); for (index, name) in names.into_iter().enumerate() { @@ -10734,6 +10753,7 @@ async fn run_artifacts_download_streams_latest_files_as_zip() { } assert_eq!(contents_by_name["logs/run.txt"], b"build log"); assert_eq!(contents_by_name["reports/result.txt"], b"latest verify"); + assert_eq!(contents_by_name["orphan.txt"], b"visit two"); } #[tokio::test] diff --git a/lib/packages/fabro-api-client/src/api/run-internals-api.ts b/lib/packages/fabro-api-client/src/api/run-internals-api.ts index 7e0fe8f03..403e2eb05 100644 --- a/lib/packages/fabro-api-client/src/api/run-internals-api.ts +++ b/lib/packages/fabro-api-client/src/api/run-internals-api.ts @@ -147,7 +147,7 @@ export const RunInternalsApiAxiosParamCreator = function (configuration?: Config }; }, /** - * Streams a ZIP archive with the latest captured version of each artifact path. Stage order and retry number determine the latest version, matching the artifacts page. Captures from the `start` and `exit` control nodes are excluded. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. + * Streams a ZIP archive with the latest captured version of each artifact path. Stage order, retry number, and then stage ID determine the latest version, matching the artifacts page. Captures from the graph\'s boundary nodes are excluded, identified by their `start` and `exit` handler type rather than by node name. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. * @summary Download Run Artifacts * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option. @@ -987,7 +987,7 @@ export const RunInternalsApiFp = function(configuration?: Configuration) { return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); }, /** - * Streams a ZIP archive with the latest captured version of each artifact path. Stage order and retry number determine the latest version, matching the artifacts page. Captures from the `start` and `exit` control nodes are excluded. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. + * Streams a ZIP archive with the latest captured version of each artifact path. Stage order, retry number, and then stage ID determine the latest version, matching the artifacts page. Captures from the graph\'s boundary nodes are excluded, identified by their `start` and `exit` handler type rather than by node name. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. * @summary Download Run Artifacts * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option. @@ -1264,7 +1264,7 @@ export const RunInternalsApiFactory = function (configuration?: Configuration, b return localVarFp.attachRunEvents(id, sinceSeq, options).then((request) => request(axios, basePath)); }, /** - * Streams a ZIP archive with the latest captured version of each artifact path. Stage order and retry number determine the latest version, matching the artifacts page. Captures from the `start` and `exit` control nodes are excluded. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. + * Streams a ZIP archive with the latest captured version of each artifact path. Stage order, retry number, and then stage ID determine the latest version, matching the artifacts page. Captures from the graph\'s boundary nodes are excluded, identified by their `start` and `exit` handler type rather than by node name. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. * @summary Download Run Artifacts * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option. @@ -1490,7 +1490,7 @@ export class RunInternalsApi extends BaseAPI { } /** - * Streams a ZIP archive with the latest captured version of each artifact path. Stage order and retry number determine the latest version, matching the artifacts page. Captures from the `start` and `exit` control nodes are excluded. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. + * Streams a ZIP archive with the latest captured version of each artifact path. Stage order, retry number, and then stage ID determine the latest version, matching the artifacts page. Captures from the graph\'s boundary nodes are excluded, identified by their `start` and `exit` handler type rather than by node name. The archive streams, so the response status is sent before the first artifact is read. A failure after that point aborts the transfer rather than returning `500`. The ZIP central directory is written last, so a truncated download does not open as a valid archive. * @summary Download Run Artifacts * @param {string} id Unique run identifier (ULID). * @param {*} [options] Override http request option.