From 506551615d58b78867b565537693bcef090d204a Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:28:32 -0400 Subject: [PATCH 1/2] Name the file a download could not write, and the album an upload could not fill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two seams the error-reporting work of #21, #22 and #26 created the conditions to close and did not. `Failure::message()` led with `id.or(path)`, but a download sets both and `imogen_sdk::Error::Io` wraps a bare `std::io::Error` that carries no path of its own. A full disk therefore said `warning: asset-7: io: No space left on device` — which photograph, and never which file or directory to go and look at. The record keeps both names, so the fix is which one the sentence leads with: a failure that happened here is about the file, one the server sent is about the asset. The marker is not serialized, because the record's shape is the API's own and both names are in it either way. `upload` still rendered a refused album fill with `{error}` onto stderr — the last place in a batch path that dropped the `details` map. Under `--json` it was absent from the document, `addedToAlbums` was silently short, and the run exited 0: a run whose every fill was refused looked successful. Filing what was uploaded is part of the run, so a refused fill is now a `Failure` like every other, named by the album's id where a file's is named by its path, and it participates in the exit rule #27 settled rather than inventing a second one. That leaves a batch holding two kinds of item, and an album is not a file, so a run that sent one file no longer closes with "1 of 2 files failed". The noun widens to the README's own word for a batch's members, and only for a run that had one of the other kind. Both tests were written first and fail on main: the download names the asset, and the upload exits 0. Closes #34 Co-Authored-By: Claude Opus 5 --- src/commands/upload.rs | 19 +++++- src/errors.rs | 42 +++++++++++- tests/batch_exit_status.rs | 132 ++++++++++++++++++++++++++++++++++++- 3 files changed, 189 insertions(+), 4 deletions(-) diff --git a/src/commands/upload.rs b/src/commands/upload.rs index 5602c65..0e2fe50 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -180,6 +180,7 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { } } + let files_refused = failures.len(); let mut added = 0u64; for (album_id, asset_ids) in &by_album { // Album membership goes on in chunks: an imported album can hold thousands. @@ -191,7 +192,12 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { .await { Ok(result) => added += result.added, - Err(error) => ctx.out.warn(format!("Could not fill an album: {error}")), + // Filing what was uploaded is part of the run, so a refused fill is one + // of the run's failures: it carries the fields the server named, it is in + // the one document `--json` writes, and it counts towards `$?`. Named by + // the album's id where a file's failure is named by its path, which is + // also what tells the two apart in the list. + Err(error) => failures.push(Failure::for_id(album_id, &error)), } } } @@ -230,8 +236,17 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { } } + // A run that files into albums has two kinds of item in it, and an album is not a + // file: counting a refused fill among the files would close a run that sent one file + // with "1 of 2 files failed". "Item" is what the README already calls a batch's + // members, and it is only reached for by a run that had one of the other kind. + let noun = if failures.len() > files_refused { + "item" + } else { + "file" + }; // A file the server already had is not a file that failed, so it counts as got through. - crate::commands::finish_batch(ctx, summary, failures.len(), uploaded.len(), "file") + crate::commands::finish_batch(ctx, summary, failures.len(), uploaded.len(), noun) } /// One line of the `--report` JSONL: what became of one file, in the API's own field diff --git a/src/errors.rs b/src/errors.rs index 954ff48..023fb5e 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -46,6 +46,11 @@ pub struct Failure { pub error: String, #[serde(skip_serializing_if = "Option::is_none")] pub details: Option
, + /// Whether the failure happened here rather than at the server, which decides only + /// which of the two names the sentence leads with. Not serialized: the record is the + /// API's own shape, and both names are in it either way. + #[serde(skip)] + local: bool, } impl Failure { @@ -57,6 +62,10 @@ impl Failure { path: None, error: error.to_string(), details: error.details().cloned(), + // `io::Error` carries no path of its own — "No space left on device" is the + // whole of what it knows — so the path this program was writing to is the + // only thing that says where the trouble is. + local: matches!(error, imogen_sdk::Error::Io(_)), } } @@ -83,8 +92,18 @@ impl Failure { /// What a person is told: whichever of the two names the command was working from, /// the server's sentence, and under it the fields it refused. Told only that three /// files failed, nobody can tell which field of which file was wrong. + /// + /// A download holds both names, and which one to lead with is the question the + /// failure answers: the server refused *this asset*, but the filesystem refused + /// *this file*, and being told the asset when the disk filled up leaves the person + /// without the directory to go and look at. pub fn message(&self) -> String { - let head = match self.id.as_deref().or(self.path.as_deref()) { + let (first, second) = if self.local { + (self.path.as_deref(), self.id.as_deref()) + } else { + (self.id.as_deref(), self.path.as_deref()) + }; + let head = match first.or(second) { Some(subject) => format!("{subject}: {}", self.error), None => self.error.clone(), }; @@ -321,6 +340,27 @@ mod tests { assert_eq!(serde_json::json!(failure)["path"], "/out/harbour.jpg"); } + /// The other half of the same record: a download that could not be written failed on + /// this machine, where the file is the subject. `io::Error` carries no path of its + /// own, so naming the asset leaves `warning: asset-7: io: No space left on device` — + /// the one thing the person needs, and the one thing it does not say. + #[test] + fn a_download_that_could_not_be_written_names_the_file() { + let error = imogen_sdk::Error::Io(std::io::Error::other("No space left on device")); + let failure = + Failure::new(Path::new("/out/2024/06/harbour.jpg"), &error).with_id("asset-7"); + let message = failure.message(); + assert!( + message.starts_with("/out/2024/06/harbour.jpg: "), + "{message} names the asset and never the file" + ); + // Both are still recorded: which asset is as much a part of the answer as which + // file, and only the sentence has to choose. + let record = serde_json::json!(&failure); + assert_eq!(record["id"], "asset-7"); + assert_eq!(record["path"], "/out/2024/06/harbour.jpg"); + } + /// An edit has no file to name, so the record carries the id alone and the person is /// told the field the server refused: ergofobe/imogen-cli#23. #[test] diff --git a/tests/batch_exit_status.rs b/tests/batch_exit_status.rs index de92239..ffdcedb 100644 --- a/tests/batch_exit_status.rs +++ b/tests/batch_exit_status.rs @@ -1,5 +1,6 @@ //! What a script sees when a batch partly fails: the exit status, and the one document -//! `--json` promises. ergofobe/imogen-cli#27. +//! `--json` promises. ergofobe/imogen-cli#27, and the two places that did not reach: +//! ergofobe/imogen-cli#34. //! //! These run the real program against a stub that answers the few requests a batch makes, //! because the two things under test — `$?` and the bytes on stdout — only exist once @@ -144,6 +145,24 @@ fn asset(id: &str) -> String { ) } +/// A library holding one album, which is what `upload --album` looks it up in. +fn album_called(name: &str) -> String { + format!( + r#"{{"items":[{{"id":"album-1","ownerId":"owner-1","name":"{name}","description":null, + "coverAssetId":null,"assetCount":0,"createdAt":"2024-01-01T00:00:00.000Z", + "updatedAt":"2024-01-01T00:00:00.000Z","shareSlug":null}}]}}"# + ) +} + +/// The album refusing what was uploaded, with the fields it faulted. +fn full_album() -> (u16, String) { + ( + 400, + r#"{"error":{"code":"validation_failed","message":"The request did not match what this endpoint expects","details":{"assetIds":["That album will not take any more"]}}}"# + .to_string(), + ) +} + fn uploaded(id: &str) -> (u16, String) { ( 201, @@ -296,6 +315,117 @@ fn a_download_reports_the_same_failure_to_both_audiences() { assert_eq!(documents[0]["failed"], 1); } +/// Filing what was uploaded is the other half of `upload --album`, and a server that +/// refused it used to be a sentence on stderr: absent from the `--json` document, +/// `addedToAlbums` silently short, exit 0. ergofobe/imogen-cli#34. +#[test] +fn an_album_that_could_not_be_filled_is_a_failure_like_any_other() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|method, path| match (method, path) { + ("GET", "/api/v1/albums") => (200, album_called("Trip")), + ("POST", "/api/v1/albums/album-1/assets") => full_album(), + _ => uploaded("asset-1"), + }); + + let only = photograph(home.path(), "one.jpg"); + let prose = imogen( + &server.base, + home.path(), + &["upload", &only, "--album", "Trip"], + ); + // The file did get through, so this is the partial status rather than the one that + // says nothing worked. + assert_eq!(prose.status.code(), Some(EXIT_PARTIAL)); + let commentary = String::from_utf8_lossy(&prose.stderr); + assert!(commentary.contains("album-1"), "{commentary}"); + assert!( + commentary.contains("assetIds: That album will not take any more"), + "the fields the server named are the point: {commentary}" + ); + + let json = imogen( + &server.base, + home.path(), + &["--json", "upload", &only, "--album", "Trip"], + ); + assert_eq!(json.status.code(), Some(EXIT_PARTIAL)); + let documents = documents(&json); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["uploaded"], 1); + assert_eq!(documents[0]["addedToAlbums"], 0); + assert_eq!( + documents[0]["failed"], 1, + "the count and the list have to agree: {}", + documents[0] + ); + assert_eq!(documents[0]["failures"][0]["id"], "album-1"); + assert_eq!( + documents[0]["failures"][0]["details"]["assetIds"][0], + "That album will not take any more" + ); + // One file was sent, and it arrived. Closing with "1 of 2 files failed" would say two + // were, so a run holding both kinds of item says so. + assert_eq!(documents[0]["error"], "1 of 2 items failed"); + // A file's failure carries the path it was sent from; an album is not a file, which + // is what tells the two apart in one list. + assert!( + documents[0]["failures"][0].get("path").is_none(), + "{}", + documents[0] + ); +} + +/// A download that could not be written failed here, not at the server, and +/// `io::Error` carries no path of its own. Named by the asset, the person is told which +/// photograph and never which file or directory to go and look at. +/// ergofobe/imogen-cli#34. +#[test] +fn a_download_that_could_not_be_written_names_the_file() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|_method, _path| (200, asset("asset-1"))); + + // A file where the output directory should be: every destination under it fails to + // be created, which is this machine's version of the disk being full. + let blocked = home.path().join("out"); + std::fs::write(&blocked, b"not a directory").expect("a file"); + + let prose = imogen( + &server.base, + home.path(), + &["download", "asset-1", "-o", &blocked.display().to_string()], + ); + assert_eq!(prose.status.code(), Some(EXIT_FAILED)); + let commentary = String::from_utf8_lossy(&prose.stderr); + assert!( + commentary.contains(&blocked.display().to_string()), + "the asset is named and the file is not: {commentary}" + ); + + // The record keeps both: which asset is as much a part of the answer as which file. + let json = imogen( + &server.base, + home.path(), + &[ + "--json", + "download", + "asset-1", + "-o", + &blocked.display().to_string(), + ], + ); + let documents = documents(&json); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["failures"][0]["id"], "asset-1"); + assert!( + documents[0]["failures"][0]["path"] + .as_str() + .expect("the record names the file") + .starts_with(&blocked.display().to_string()), + "{}", + documents[0] + ); +} + /// The rule only earns its keep if a run that worked still says so. #[test] fn a_batch_that_worked_still_exits_zero() { From 2998c615e71ed17fcc685ba8b05e5848b4b77fb1 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:36:28 -0400 Subject: [PATCH 2/2] Count the fills that worked, and say what for_id is for now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First review round. The total the run closes with counted only the files that got through, while the failures counted both kinds: a manifest filing into two albums where one fill is refused did four things and three of them worked, and the sentence called it "1 of 3 items failed", dropping the fill that succeeded. The noun and the total have to move together, because the noun names what is being counted — `download` has summed the files it wrote with the ones it skipped under one noun since #27, and this is the same rule with a second kind of item. Two doc comments in `errors.rs` were left describing a world with one caller. `with_id`'s said "an upload only has the file", which the album fill it now names by id alone contradicts outright, and `for_id`'s named `assets edit` as though it were the only way in. Co-Authored-By: Claude Opus 5 --- src/commands/upload.rs | 25 ++++++++++++--- src/errors.rs | 6 ++-- tests/batch_exit_status.rs | 63 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/src/commands/upload.rs b/src/commands/upload.rs index 0e2fe50..058318a 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -182,6 +182,7 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { let files_refused = failures.len(); let mut added = 0u64; + let mut fills_done = 0usize; for (album_id, asset_ids) in &by_album { // Album membership goes on in chunks: an imported album can hold thousands. for chunk in asset_ids.chunks(500) { @@ -191,7 +192,10 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { .add_assets(album_id, &AssetSelection::ids(chunk)) .await { - Ok(result) => added += result.added, + Ok(result) => { + added += result.added; + fills_done += 1; + } // Filing what was uploaded is part of the run, so a refused fill is one // of the run's failures: it carries the fields the server named, it is in // the one document `--json` writes, and it counts towards `$?`. Named by @@ -240,13 +244,24 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { // file: counting a refused fill among the files would close a run that sent one file // with "1 of 2 files failed". "Item" is what the README already calls a batch's // members, and it is only reached for by a run that had one of the other kind. - let noun = if failures.len() > files_refused { - "item" + // + // The noun and what the total counts move together, because the noun names what is + // being counted: once fills are on the failed side they are on the succeeded side + // too, the way `download` counts a file it skipped alongside one it wrote. Counting + // them under "file" would be the same miscount the other way round. + let (noun, fills_through) = if failures.len() > files_refused { + ("item", fills_done) } else { - "file" + ("file", 0) }; // A file the server already had is not a file that failed, so it counts as got through. - crate::commands::finish_batch(ctx, summary, failures.len(), uploaded.len(), noun) + crate::commands::finish_batch( + ctx, + summary, + failures.len(), + uploaded.len() + fills_through, + noun, + ) } /// One line of the `--report` JSONL: what became of one file, in the API's own field diff --git a/src/errors.rs b/src/errors.rs index 023fb5e..a9c89aa 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -78,12 +78,14 @@ impl Failure { } } - /// `assets edit` works from ids and never touches a file. + /// A failure with no file in it at all: `assets edit` works from ids, and so does the + /// album an upload could not fill. pub fn for_id(id: impl Into, error: &imogen_sdk::Error) -> Self { Self::of(error).with_id(id) } - /// A download knows which asset it was fetching; an upload only has the file. + /// The second name, for the one command that has both: a download knows the asset it + /// was fetching as well as the file it was writing. pub fn with_id(mut self, id: impl Into) -> Self { self.id = Some(id.into()); self diff --git a/tests/batch_exit_status.rs b/tests/batch_exit_status.rs index ffdcedb..4f95541 100644 --- a/tests/batch_exit_status.rs +++ b/tests/batch_exit_status.rs @@ -154,6 +154,18 @@ fn album_called(name: &str) -> String { ) } +/// Two albums, which is what a manifest naming one per file files into. +fn two_albums() -> String { + r#"{"items":[ + {"id":"album-1","ownerId":"owner-1","name":"Trip","description":null, + "coverAssetId":null,"assetCount":0,"createdAt":"2024-01-01T00:00:00.000Z", + "updatedAt":"2024-01-01T00:00:00.000Z","shareSlug":null}, + {"id":"album-2","ownerId":"owner-1","name":"Harbour","description":null, + "coverAssetId":null,"assetCount":0,"createdAt":"2024-01-01T00:00:00.000Z", + "updatedAt":"2024-01-01T00:00:00.000Z","shareSlug":null}]}"# + .to_string() +} + /// The album refusing what was uploaded, with the fields it faulted. fn full_album() -> (u16, String) { ( @@ -375,6 +387,57 @@ fn an_album_that_could_not_be_filled_is_a_failure_like_any_other() { ); } +/// The total has to count every item of the kind the noun names, on both sides of it. +/// A run filing into two albums where one fill is refused did four things, three of them +/// successfully — counting only the files would call it three, and drop the fill that +/// worked. `download` has summed its two kinds of success under one noun since #27. +#[test] +fn a_fill_that_worked_is_counted_beside_the_one_that_did_not() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|method, path| match (method, path) { + ("GET", "/api/v1/albums") => (200, two_albums()), + ("POST", "/api/v1/albums/album-1/assets") => { + (200, r#"{"added":1,"skipped":0,"assetCount":1}"#.to_string()) + } + ("POST", "/api/v1/albums/album-2/assets") => full_album(), + _ => uploaded("asset-1"), + }); + + // One file for each album, which is what a manifest is for. + let first = photograph(home.path(), "one.jpg"); + let second = photograph(home.path(), "two.jpg"); + let manifest = home.path().join("manifest.jsonl"); + std::fs::write( + &manifest, + format!( + "{{\"path\":\"{first}\",\"album\":\"Trip\"}}\n{{\"path\":\"{second}\",\"album\":\"Harbour\"}}\n" + ), + ) + .expect("a manifest"); + + let output = imogen( + &server.base, + home.path(), + &[ + "--json", + "upload", + "--manifest", + &manifest.display().to_string(), + ], + ); + assert_eq!(output.status.code(), Some(EXIT_PARTIAL)); + let documents = documents(&output); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["uploaded"], 2); + assert_eq!(documents[0]["addedToAlbums"], 1); + assert_eq!(documents[0]["failed"], 1); + assert_eq!( + documents[0]["error"], "1 of 4 items failed", + "two files and two fills were attempted: {}", + documents[0] + ); +} + /// A download that could not be written failed here, not at the server, and /// `io::Error` carries no path of its own. Named by the asset, the person is told which /// photograph and never which file or directory to go and look at.