Name the file a download could not write, and the album an upload could not fill

src/commands/upload.rs

@@ -180,7 +180,9 @@ 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) {
@@ -190,8 +192,16 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> {
                 .add_assets(album_id, &AssetSelection::ids(chunk))
                 .await
             {
-                Ok(result) => added += result.added,
-                Err(error) => ctx.out.warn(format!("Could not fill an album: {error}")),
+                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
+                // 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 +240,28 @@ 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.
+    //
+    // 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", 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(), "file")
+    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

src/errors.rs

@@ -46,6 +46,11 @@ pub struct Failure {
     pub error: String,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub details: Option<Details>,
+    /// 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(_)),
         }
     }
 
@@ -69,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<String>, 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<String>) -> Self {
         self.id = Some(id.into());
         self
@@ -83,8 +94,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 +342,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]

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,36 @@ 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}}]}}"#
+    )
+}
+
+/// 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) {
+    (
+        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 +327,168 @@ 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]
+    );
+}
+
+/// 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.
+/// 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() {