scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-08-28 05:27:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix: make the artifact tie-break match the artifacts page

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-08-01 09:20:40 -04:00
parent b834e0bda3
commit 8b767c658b
No known key found for this signature in database
4 changed files with 43 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
docs/public/api-reference/fabro-api.yaml
View file

@ -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`.

21
lib/apps/fabro-server/src/server/handler/artifacts.rs
View file

@ -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::<HashMap<_, _>>();
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<String, NodeArtifact> = HashMap::new();
for artifact in entries {

22
lib/apps/fabro-server/src/server/tests.rs
View file

@ -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::<Vec<_>>();
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]

8
lib/packages/fabro-api-client/src/api/run-internals-api.ts generated
View file

@ -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.