From 459af04cb6624e7903e944b409ce4940707d441f Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 11 Oct 2026 09:00:23 +0200 Subject: [PATCH] fix(cli): name the failing update step and its OS error On macOS the signed-candidate preparation folded every step into one boolean, so each failure printed the same line: "signed update candidate preparation failed". A full /tmp read exactly like a broken codesign. That is what a cli test hit on 2026-10-09, while a multi-GB index was filling a nearly full disk. Each step now records why it failed, and the update prints, for example: error: signed update candidate preparation failed while writing the unsigned candidate: No space left on device The staging functions in activation_transaction.c keep the errno of the failing write (or sync, close, read) through their cleanup, so a caller can name it. The non-macOS path adds the OS error to its existing "failed to stage verified update" line (POSIX only; Windows reports through GetLastError). A failed fingerprint of the staged file now gets its own message instead of "...: activation transaction completed". A test seam makes the next staging write fail with a chosen errno. The new test cli_update_names_the_failing_staging_step_and_os_error fails without the cli.c change and passes with it, and checks that the old executable stays in place. Also checked on real input: with /tmp on a small volume that fills between two updates, the update prints the line above. Signed-off-by: Martin Vogel --- src/cli/activation_transaction.c | 69 ++++++++++++-- src/cli/activation_transaction.h | 7 +- src/cli/cli.c | 135 +++++++++++++++++++++++----- tests/test_cli.c | 148 +++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+), 30 deletions(-) diff --git a/src/cli/activation_transaction.c b/src/cli/activation_transaction.c index a56052775..18ed6dd14 100644 --- a/src/cli/activation_transaction.c +++ b/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; diff --git a/src/cli/activation_transaction.h b/src/cli/activation_transaction.h index c0dd89bfc..3aa37e1e4 100644 --- a/src/cli/activation_transaction.h +++ b/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 */ diff --git a/src/cli/cli.c b/src/cli/cli.c index c99034847..172657edd 100644 --- a/src/cli/cli.c +++ b/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; } diff --git a/tests/test_cli.c b/tests/test_cli.c index a3fda1d5f..33340328c 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -2969,6 +2969,153 @@ TEST(cli_update_agent_configs_finish_before_guard_release) { #endif } +/* 2026-10-09: /tmp filled up mid-update, and the update said only "signed + * update candidate preparation failed", although the cause was a full disk. + * The failing staging write must be reported by step and OS error, through the + * staging cleanup, and the old executable must stay. */ +TEST(cli_update_names_the_failing_staging_step_and_os_error) { +#ifdef _WIN32 + SKIP_PLATFORM("Windows staging reports through GetLastError, not errno"); +#else + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-update-enospc-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) { + FAIL("cbm_mkdtemp failed"); + } + char *old_home = NULL; + char *old_cache = NULL; + cli_activation_save_env(&old_home, &old_cache); + const char *download_env = getenv("CBM_DOWNLOAD_URL"); + char *old_download = download_env ? strdup(download_env) : NULL; + cbm_setenv("HOME", tmpdir, 1); + char cache_dir[512]; + snprintf(cache_dir, sizeof(cache_dir), "%s/cache", tmpdir); + cbm_setenv("CBM_CACHE_DIR", cache_dir, 1); + char release_dir[512]; + char bin_dir[512]; + char bin_target[640]; + snprintf(release_dir, sizeof(release_dir), "%s/release", tmpdir); + snprintf(bin_dir, sizeof(bin_dir), "%s/.local/bin", tmpdir); + snprintf(bin_target, sizeof(bin_target), "%s/codebase-memory-mcp", bin_dir); + test_mkdirp(release_dir); + test_mkdirp(bin_dir); + static const char old_binary[] = "old binary before a failed update"; + write_test_file(bin_target, old_binary); + +#if defined(__APPLE__) +#if defined(__aarch64__) || defined(__arm64__) + const char *asset_name = "codebase-memory-mcp-darwin-arm64.tar.gz"; +#else + const char *asset_name = "codebase-memory-mcp-darwin-amd64.tar.gz"; +#endif + char native_fixture_path[CBM_SZ_4K] = {0}; + uint32_t native_fixture_size = (uint32_t)sizeof(native_fixture_path); + ASSERT_EQ(_NSGetExecutablePath(native_fixture_path, &native_fixture_size), 0); + const char *native_fixture = native_fixture_path; +#else +#if defined(__aarch64__) + const char *asset_name = "codebase-memory-mcp-linux-arm64-portable.tar.gz"; +#else + const char *asset_name = "codebase-memory-mcp-linux-amd64-portable.tar.gz"; +#endif + const char *native_fixture = "/bin/true"; +#endif + FILE *native_file = fopen(native_fixture, "rb"); + ASSERT_NOT_NULL(native_file); + ASSERT_EQ(fseek(native_file, 0, SEEK_END), 0); + long native_size = ftell(native_file); + ASSERT_GT(native_size, 0); + ASSERT_LTE(native_size, INT_MAX); + ASSERT_EQ(fseek(native_file, 0, SEEK_SET), 0); + unsigned char *replacement = malloc((size_t)native_size); + ASSERT_NOT_NULL(replacement); + ASSERT_EQ(fread(replacement, 1, (size_t)native_size, native_file), (size_t)native_size); + ASSERT_EQ(fclose(native_file), 0); + int archive_len = 0; + unsigned char *archive = + create_test_targz("codebase-memory-mcp", replacement, (int)native_size, &archive_len); + free(replacement); + ASSERT_NOT_NULL(archive); + char archive_path[768]; + snprintf(archive_path, sizeof(archive_path), "%s/%s", release_dir, asset_name); + FILE *archive_file = fopen(archive_path, "wb"); + ASSERT_NOT_NULL(archive_file); + ASSERT_EQ(fwrite(archive, 1, (size_t)archive_len, archive_file), (size_t)archive_len); + ASSERT_EQ(fclose(archive_file), 0); + free(archive); + char digest[65]; + ASSERT_EQ(cbm_cli_sha256_file(archive_path, digest, sizeof(digest)), 0); + char checksum_path[640]; + snprintf(checksum_path, sizeof(checksum_path), "%s/checksums.txt", release_dir); + FILE *checksum_file = fopen(checksum_path, "w"); + ASSERT_NOT_NULL(checksum_file); + ASSERT_TRUE(fprintf(checksum_file, "%s %s\n", digest, asset_name) > 0); + ASSERT_EQ(fclose(checksum_file), 0); + char download_url[640]; + snprintf(download_url, sizeof(download_url), "file://%s", release_dir); + cbm_setenv("CBM_DOWNLOAD_URL", download_url, 1); + + cli_activation_fake_t fake = { + .mutation_reserve_result = 1, + }; + cbm_cli_activation_ops_t ops = cli_activation_fake_ops(&fake); + cbm_cli_set_activation_ops_for_test(&ops); + FILE *capture = tmpfile(); + int saved_stdout = capture ? dup(STDOUT_FILENO) : -1; + int saved_stderr = capture ? dup(STDERR_FILENO) : -1; + int rc = -1; + if (capture && saved_stdout >= 0 && saved_stderr >= 0) { + fflush(NULL); + if (dup2(fileno(capture), STDOUT_FILENO) >= 0 && + dup2(fileno(capture), STDERR_FILENO) >= 0) { + /* The first staging write of the update fails as on a full disk. */ + cbm_activation_transaction_write_failures_set_for_testing(1U, ENOSPC); + char *argv[] = {"--force"}; + rc = cli_test_cmd_update(1, argv); + cbm_activation_transaction_write_failures_set_for_testing(0U, 0); + } + fflush(NULL); + (void)dup2(saved_stdout, STDOUT_FILENO); + (void)dup2(saved_stderr, STDERR_FILENO); + } + if (saved_stdout >= 0) { + close(saved_stdout); + } + if (saved_stderr >= 0) { + close(saved_stderr); + } + cbm_cli_set_activation_ops_for_test(NULL); + char output[16384] = {0}; + if (capture) { + rewind(capture); + size_t count = fread(output, 1, sizeof(output) - 1U, capture); + output[count] = '\0'; + fclose(capture); + } + const char *kept = read_test_file(bin_target); + bool old_kept = kept && strcmp(kept, old_binary) == 0; + + if (old_download) { + cbm_setenv("CBM_DOWNLOAD_URL", old_download, 1); + } else { + cbm_unsetenv("CBM_DOWNLOAD_URL"); + } + free(old_download); + cli_activation_restore_env(old_home, old_cache); + test_rmdir_r(tmpdir); + + ASSERT_NEQ(rc, 0); + ASSERT_NOT_NULL(strstr(output, strerror(ENOSPC))); +#ifdef __APPLE__ + ASSERT_NOT_NULL(strstr(output, "failed while writing the unsigned candidate")); +#else + ASSERT_NOT_NULL(strstr(output, "failed to stage verified update")); +#endif + ASSERT_TRUE(old_kept); + PASS(); +#endif +} + TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index) { char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-daemon-uninstall-XXXXXX"); @@ -17776,6 +17923,7 @@ SUITE(cli) { RUN_TEST(cli_clients_selector_filters_registry_installs_issue1798); RUN_TEST(cli_update_accepts_retired_variant_flags_issue1544); RUN_TEST(cli_update_agent_configs_finish_before_guard_release); + RUN_TEST(cli_update_names_the_failing_staging_step_and_os_error); RUN_TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index); RUN_TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain); RUN_TEST(cli_uninstall_auto_answers_keep_indexes_without_delete_indexes);