Merge pull request #2589 from DeusData/fix/cli-flake-diagnostics

fix(cli): failed updates and the scope fixture name their cause

Author: DeusData

src/cli/activation_transaction.c

@@ -138,6 +138,16 @@ void cbm_activation_transaction_note_refusal_for_testing(const char *predicate,
         activation_note_refusal(predicate, os_error);
     }
 }
+
+/* Test seam: the next `count` staging writes fail with errno = os_error, so a
+ * caller's report of a full disk is testable without filling one. */
+static unsigned int activation_write_failures_for_testing;
+static int activation_write_failure_errno_for_testing;
+
+void cbm_activation_transaction_write_failures_set_for_testing(unsigned int count, int os_error) {
+    activation_write_failures_for_testing = count;
+    activation_write_failure_errno_for_testing = os_error;
+}
 #endif
 
 #ifdef _WIN32
@@ -1134,6 +1144,13 @@ static bool activation_native_sync(activation_native_file_t file) {
 
 static bool activation_native_write_all(activation_native_file_t file, const void *data,
                                         size_t length) {
+#ifdef CBM_ENABLE_TEST_SEAMS
+    if (activation_write_failures_for_testing > 0) {
+        activation_write_failures_for_testing--;
+        errno = activation_write_failure_errno_for_testing;
+        return false;
+    }
+#endif
     const unsigned char *cursor = data;
     while (length > 0) {
 #ifdef _WIN32
@@ -1618,6 +1635,16 @@ static void activation_failed_stage_cleanup(cbm_activation_transaction_t *transa
     }
 }
 
+/* A failed stage write reports IO with errno restored, after the cleanup, to
+ * the error of the call that failed (0 when none applies), so the caller can
+ * name it. */
+static cbm_activation_transaction_status_t activation_stage_io_failure(
+    cbm_activation_transaction_t *transaction, int os_error) {
+    activation_failed_stage_cleanup(transaction);
+    errno = os_error;
+    return CBM_ACTIVATION_TRANSACTION_IO;
+}
+
 cbm_activation_transaction_status_t cbm_activation_transaction_stage_bytes(
     const char *target_path, const void *candidate, size_t candidate_size,
     cbm_activation_transaction_t **transaction_out) {
@@ -1643,11 +1670,20 @@ cbm_activation_transaction_status_t cbm_activation_transaction_stage_bytes(
     }
     transaction->staged_exists = true;
     bool written = activation_native_write_all(staged, candidate, candidate_size);
+    int os_error = written ? 0 : errno;
     bool durable = written && activation_native_sync(staged);
+    if (written && !durable) {
+        os_error = errno;
+    }
     bool closed = activation_native_close(staged);
-    if (!written || !durable || !closed || !activation_sync_directory(transaction)) {
-        activation_failed_stage_cleanup(transaction);
-        return CBM_ACTIVATION_TRANSACTION_IO;
+    if (!closed && os_error == 0) {
+        os_error = errno;
+    }
+    if (!written || !durable || !closed) {
+        return activation_stage_io_failure(transaction, os_error);
+    }
+    if (!activation_sync_directory(transaction)) {
+        return activation_stage_io_failure(transaction, errno);
     }
     *transaction_out = transaction;
     return CBM_ACTIVATION_TRANSACTION_OK;
@@ -1797,28 +1833,45 @@ cbm_activation_transaction_status_t cbm_activation_transaction_stage_file(
     unsigned char buffer[64U * 1024U];
     size_t total = 0;
     bool copied = true;
+    int os_error = 0;
     for (;;) {
         size_t amount = 0;
         if (!activation_native_read(source, buffer, sizeof(buffer), &amount)) {
             copied = false;
+            os_error = errno;
             break;
         }
         if (amount == 0) {
             break;
         }
-        if (SIZE_MAX - total < amount || !activation_native_write_all(staged, buffer, amount)) {
+        if (SIZE_MAX - total < amount) {
             copied = false;
             break;
         }
+        if (!activation_native_write_all(staged, buffer, amount)) {
+            copied = false;
+            os_error = errno;
+            break;
+        }
         total += amount;
     }
     bool durable = copied && total > 0 && activation_native_sync(staged);
+    if (copied && total > 0 && !durable) {
+        os_error = errno;
+    }
     bool source_closed = activation_native_close(source);
+    if (!source_closed && os_error == 0) {
+        os_error = errno;
+    }
     bool staged_closed = activation_native_close(staged);
-    if (!copied || total == 0 || !durable || !source_closed || !staged_closed ||
-        !activation_sync_directory(transaction)) {
-        activation_failed_stage_cleanup(transaction);
-        return CBM_ACTIVATION_TRANSACTION_IO;
+    if (!staged_closed && os_error == 0) {
+        os_error = errno;
+    }
+    if (!copied || total == 0 || !durable || !source_closed || !staged_closed) {
+        return activation_stage_io_failure(transaction, os_error);
+    }
+    if (!activation_sync_directory(transaction)) {
+        return activation_stage_io_failure(transaction, errno);
     }
     *transaction_out = transaction;
     return CBM_ACTIVATION_TRANSACTION_OK;

src/cli/activation_transaction.h

@@ -45,7 +45,11 @@ void cbm_activation_transaction_set_before_absent_publish_for_test(
 void cbm_activation_transaction_rename_failures_set_for_test(unsigned int count);
 
 /* Stage a candidate beside target_path (therefore on the same filesystem).
- * The staged file is private to the current account and executable. */
+ * The staged file is private to the current account and executable.
+ * On POSIX, an IO result leaves errno at the error of the failing system call
+ * (0 when none applies, e.g. a short read), through the stage cleanup, so a
+ * caller can name it: a full disk reads "No space left on device". The same
+ * holds for stage_file below. */
 cbm_activation_transaction_status_t cbm_activation_transaction_stage_bytes(
     const char *target_path, const void *candidate, size_t candidate_size,
     cbm_activation_transaction_t **transaction_out);
@@ -100,6 +104,7 @@ const char *cbm_activation_transaction_refusal_note(void);
 #ifdef CBM_ENABLE_TEST_SEAMS
 void cbm_activation_transaction_note_refusal_for_testing(const char *predicate,
                                                          unsigned long os_error);
+void cbm_activation_transaction_write_failures_set_for_testing(unsigned int count, int os_error);
 #endif
 
 #endif /* CBM_ACTIVATION_TRANSACTION_H */

src/cli/cli.c

@@ -13202,6 +13202,45 @@ static int cli_update_activate_binary(void *opaque) {
     return CLI_OK;
 }
 
+#ifdef __APPLE__
+/* The first step of the signed-candidate preparation that failed. One boolean
+ * used to cover all of them, so a full disk read exactly like a broken
+ * codesign: "candidate preparation failed", and nothing else. */
+typedef struct {
+    const char *step;
+    int os_error;       /* errno left by an OS-level step; 0 when none applies */
+    const char *detail; /* the reason when no OS error applies, or NULL */
+} cli_update_prep_failure_t;
+
+/* Records the first failing step. An OS-level step keeps the errno it left; a
+ * step that succeeds clears errno so the next one starts clean. Returns ok, so
+ * the steps chain with &&. */
+static bool cli_update_prep_step(bool ok, const char *step, bool os_level, const char *detail,
+                                 cli_update_prep_failure_t *failure) {
+    if (ok) {
+        errno = 0;
+        return true;
+    }
+    if (!failure->step) {
+        failure->step = step;
+        failure->os_error = os_level ? errno : 0;
+        failure->detail = detail;
+    }
+    return false;
+}
+
+static void cli_update_prep_report(const cli_update_prep_failure_t *failure) {
+    const char *step = failure->step ? failure->step : "an unrecorded step";
+    const char *reason = failure->os_error != 0 ? strerror(failure->os_error) : failure->detail;
+    if (reason) {
+        (void)fprintf(stderr, "error: signed update candidate preparation failed while %s: %s\n",
+                      step, reason);
+    } else {
+        (void)fprintf(stderr, "error: signed update candidate preparation failed while %s\n", step);
+    }
+}
+#endif
+
 static int extract_and_install_binary(extract_install_args_t args) {
     const char *tmp_archive = args.tmp_archive;
     const char *ext = args.ext;
@@ -13259,42 +13298,79 @@ static int extract_and_install_binary(extract_install_args_t args) {
 #ifdef __APPLE__
     /* codesign replaces the signed file on current macOS releases. Publish and
      * sign a disposable private copy first, then stage the resulting immutable
-     * bytes into the final transaction. */
+     * bytes into the final transaction. Every step records why it failed, so a
+     * full disk is reported as one and not as a signing failure. */
+    cli_update_prep_failure_t failure = {0};
     char prepared_dir[CLI_BUF_1K];
     char prepared_candidate[CLI_BUF_1K];
     int prepared_dir_length =
         snprintf(prepared_dir, sizeof(prepared_dir), "%s/cbm-update-sign-XXXXXX", cbm_tmpdir());
-    bool prepared = prepared_dir_length > 0 && (size_t)prepared_dir_length < sizeof(prepared_dir) &&
-                    cbm_mkdtemp(prepared_dir) != NULL;
+    errno = 0;
+    bool prepared = cli_update_prep_step(prepared_dir_length > 0 &&
+                                             (size_t)prepared_dir_length < sizeof(prepared_dir) &&
+                                             cbm_mkdtemp(prepared_dir) != NULL,
+                                         "creating its private directory", true, NULL, &failure);
     int prepared_candidate_length = prepared
                                         ? snprintf(prepared_candidate, sizeof(prepared_candidate),
                                                    "%s/codebase-memory-mcp", prepared_dir)
                                         : CLI_ERR;
-    prepared = prepared && prepared_candidate_length > 0 &&
-               (size_t)prepared_candidate_length < sizeof(prepared_candidate);
+    prepared = prepared && cli_update_prep_step(prepared_candidate_length > 0 &&
+                                                    (size_t)prepared_candidate_length <
+                                                        sizeof(prepared_candidate),
+                                                "naming the candidate file", false, NULL, &failure);
     cbm_activation_transaction_t *preparation = NULL;
     stage_status = prepared ? cbm_activation_transaction_stage_bytes(prepared_candidate, bin_data,
                                                                      (size_t)bin_len, &preparation)
                             : CBM_ACTIVATION_TRANSACTION_IO;
+    prepared = prepared &&
+               cli_update_prep_step(
+                   stage_status == CBM_ACTIVATION_TRANSACTION_OK && preparation,
+                   "writing the unsigned candidate", stage_status == CBM_ACTIVATION_TRANSACTION_IO,
+                   cbm_activation_transaction_status_message(stage_status), &failure);
     free(bin_data);
     cli_binary_validator_t unsigned_validator = {{0}};
-    prepared = stage_status == CBM_ACTIVATION_TRANSACTION_OK && preparation &&
-               cli_activation_transaction_expected_build(preparation, &unsigned_validator) &&
-               cli_activation_transaction_commit_validated(preparation, &unsigned_validator,
-                                                           CLI_OCTAL_PERM) == CLI_OK &&
-               cli_activation_transaction_finalize_close(&preparation) == CLI_OK &&
-               cbm_macos_adhoc_sign(prepared_candidate) == CLI_OK &&
-               cbm_daemon_build_fingerprint_file(prepared_candidate, validator.fingerprint);
+    prepared = prepared &&
+               cli_update_prep_step(
+                   cli_activation_transaction_expected_build(preparation, &unsigned_validator),
+                   "fingerprinting the unsigned candidate", true, NULL, &failure) &&
+               cli_update_prep_step(cli_activation_transaction_commit_validated(
+                                        preparation, &unsigned_validator, CLI_OCTAL_PERM) == CLI_OK,
+                                    "publishing the unsigned candidate", false, NULL, &failure) &&
+               cli_update_prep_step(
+                   cli_activation_transaction_finalize_close(&preparation) == CLI_OK,
+                   "closing the unsigned candidate's transaction", false, NULL, &failure) &&
+               cli_update_prep_step(cbm_macos_adhoc_sign(prepared_candidate) == CLI_OK,
+                                    "signing the candidate ad hoc", false, NULL, &failure) &&
+               cli_update_prep_step(
+                   cbm_daemon_build_fingerprint_file(prepared_candidate, validator.fingerprint),
+                   "fingerprinting the signed candidate", true, NULL, &failure);
     const char *prepared_argv[] = {prepared_candidate, "--version", NULL};
-    prepared = prepared && cbm_exec_no_shell(prepared_argv) == CLI_OK;
+    int version_rc = prepared ? cbm_exec_no_shell(prepared_argv) : CLI_OK;
+    char version_detail[80];
+    if (version_rc == CBM_NOT_FOUND) {
+        snprintf(version_detail, sizeof(version_detail), "it could not run or was killed");
+    } else {
+        snprintf(version_detail, sizeof(version_detail), "--version exited with status %d",
+                 version_rc);
+    }
+    prepared =
+        prepared && cli_update_prep_step(version_rc == CLI_OK, "running the signed candidate",
+                                         false, version_detail, &failure);
     if (prepared) {
         stage_status = cbm_activation_transaction_stage_file(bin_dest, prepared_candidate,
                                                              &binary_transaction);
         cli_binary_validator_t staged_validator = {{0}};
         prepared =
-            stage_status == CBM_ACTIVATION_TRANSACTION_OK && binary_transaction &&
-            cli_activation_transaction_expected_build(binary_transaction, &staged_validator) &&
-            strcmp(staged_validator.fingerprint, validator.fingerprint) == 0;
+            cli_update_prep_step(
+                stage_status == CBM_ACTIVATION_TRANSACTION_OK && binary_transaction,
+                "staging the signed candidate", stage_status == CBM_ACTIVATION_TRANSACTION_IO,
+                cbm_activation_transaction_status_message(stage_status), &failure) &&
+            cli_update_prep_step(
+                cli_activation_transaction_expected_build(binary_transaction, &staged_validator),
+                "fingerprinting the staged candidate", true, NULL, &failure) &&
+            cli_update_prep_step(strcmp(staged_validator.fingerprint, validator.fingerprint) == 0,
+                                 "comparing the staged and signed candidates", false,
+                                 "their fingerprints differ", &failure);
         if (prepared) {
             validator = staged_validator;
         }
@@ -13308,18 +13384,35 @@ static int extract_and_install_binary(extract_install_args_t args) {
         (void)cbm_rmdir(prepared_dir);
     }
     if (!prepared) {
-        (void)fprintf(stderr, "error: signed update candidate preparation failed\n");
+        cli_update_prep_report(&failure);
         (void)cli_activation_transaction_abort(&binary_transaction);
         return CLI_TRUE;
     }
 #else
+    errno = 0;
     stage_status = cbm_activation_transaction_stage_bytes(bin_dest, bin_data, (size_t)bin_len,
                                                           &binary_transaction);
+    /* POSIX only: Windows staging reports through GetLastError, not errno. */
+#ifdef _WIN32
+    int stage_error = 0;
+#else
+    int stage_error = stage_status == CBM_ACTIVATION_TRANSACTION_IO ? errno : 0;
+#endif
     free(bin_data);
-    if (stage_status != CBM_ACTIVATION_TRANSACTION_OK || !binary_transaction ||
-        !cli_activation_transaction_expected_build(binary_transaction, &validator)) {
-        (void)fprintf(stderr, "error: failed to stage verified update: %s\n",
-                      cbm_activation_transaction_status_message(stage_status));
+    if (stage_status != CBM_ACTIVATION_TRANSACTION_OK || !binary_transaction) {
+        if (stage_error != 0) {
+            (void)fprintf(stderr, "error: failed to stage verified update: %s: %s\n",
+                          cbm_activation_transaction_status_message(stage_status),
+                          strerror(stage_error));
+        } else {
+            (void)fprintf(stderr, "error: failed to stage verified update: %s\n",
+                          cbm_activation_transaction_status_message(stage_status));
+        }
+        (void)cli_activation_transaction_abort(&binary_transaction);
+        return CLI_TRUE;
+    }
+    if (!cli_activation_transaction_expected_build(binary_transaction, &validator)) {
+        (void)fprintf(stderr, "error: failed to fingerprint the staged update\n");
         (void)cli_activation_transaction_abort(&binary_transaction);
         return CLI_TRUE;
     }