From 0a22fb14dfaebaeefa47bd8750c6a25bcf3f8fd4 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 28 Jul 2026 22:49:22 +0200 Subject: [PATCH] fix(windows): absorb transient file locks when retiring a running image Release run 30385988475 lost three Windows smoke jobs to one phenomenon on two different surfaces. A Windows file that was just written or just executed can refuse deletion and rename for a moment while a scanner reads it or while the OS finishes reaping a child. Both conditions clear on their own within moments. It reads as nondeterminism because it is timing: windows-2025 passed both amd64 variants while windows-latest -- the SAME image, windows-2025-vs2026 -- failed both, and windows-11-arm failed too. PRODUCT: uninstall abandoned a live installation. With one binary per platform, uninstall must remove the very image it is executing, and on Windows a running image can only be renamed, never overwritten. That rename got a single attempt, so a transient sharing violation surfaced to the user as error: failed to remove ...; completed configuration/index cleanup may remain with the install left half-torn-down. install.ps1 already retries this exact operation ten times for this exact reason; the C path now does the same, and only for sharing/access/lock violations, so a file that is genuinely held still fails closed instead of spinning. Proven with a failure-injection seam rather than by racing a real scanner: 3 injected failures must still succeed, 64 must still fail AND leave the target in place. Deterministic, so it does not need CI to learn whether it works. HARNESS: the smoke killed itself silently. Phases that install a binary then `rm -rf` the directory hit the same lock. Under `set -euo pipefail` that returns non-zero and ends the run WITHOUT printing anything -- the logs jump straight from "OK 13h" to job cleanup with no FAIL line, which is what made this expensive to read. None of those 16 removals is an assertion; they are fixture cleanup, and _smoke.yml already states the principle ("a temp dir on an ephemeral runner must never fail the job") that smoke-test.sh did not honour. They now go through smoke_rmtree, which retries so the disk is actually reclaimed -- these fixtures hold ~300 MB binaries and runners are disk-tight -- then warns and continues. Verified: Windows VM full smoke ALL PASSED (previously died after 13h), macOS full smoke ALL PASSED, activation_transaction + cli 259 passed on Windows, all 10 shell contracts pass, lint-ci clean. Signed-off-by: Martin Vogel --- scripts/smoke-test.sh | 59 ++++++++++++++++++++--------- src/cli/activation_transaction.c | 54 ++++++++++++++++++++++++-- src/cli/activation_transaction.h | 5 +++ tests/test_activation_transaction.c | 50 ++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 20 deletions(-) diff --git a/scripts/smoke-test.sh b/scripts/smoke-test.sh index ef89d2f56..f082fc26a 100755 --- a/scripts/smoke-test.sh +++ b/scripts/smoke-test.sh @@ -68,6 +68,31 @@ smoke_file_sha256() { fi } +# Fixture cleanup, never an assertion. On Windows a directory holding an +# executable that was just written or just run can refuse deletion for a moment +# while a scanner or an unreaped child still holds it. Under `set -e` a plain +# `rm -rf` then kills the run WITHOUT printing anything — three release smoke +# jobs died exactly that way, silently, immediately after "OK 13h". +# +# Retry so the disk actually gets reclaimed (these fixtures hold ~300 MB +# binaries and runners are disk-tight), then warn and continue: an ephemeral +# temp dir must never decide the verdict. +smoke_rmtree() { + local target + for target in "$@"; do + [ -n "$target" ] || continue + local attempt + for attempt in 1 2 3 4 5 6 7 8 9 10; do + rm -rf "$target" 2>/dev/null && break + sleep 0.5 + done + if [ -e "$target" ]; then + echo "warn: could not remove smoke fixture $target (leaving it to the runner)" + fi + done + return 0 +} + # Every platform ships ONE binary, Windows included: a fixture copy is complete # with nothing beside it. copy_smoke_binary() { @@ -123,7 +148,7 @@ DRYRUN_HOME="" if command -v cygpath &>/dev/null; then TMPDIR=$(cygpath -m "$TMPDIR") fi -trap 'rm -rf "$TMPDIR" "${DRYRUN_HOME:-}"' EXIT +trap 'smoke_rmtree "$TMPDIR" "${DRYRUN_HOME:-}"' EXIT CLI_STDERR=$(smoke_mktemp_file) cli() { "$BINARY" cli "$@" 2>"$CLI_STDERR"; } @@ -970,7 +995,7 @@ chmod 755 "$INSTALL_DIR/codebase-memory-mcp" INSTALLED_VER=$("$INSTALL_DIR/codebase-memory-mcp" --version 2>&1) if ! echo "$INSTALLED_VER" | grep -qE 'v?[0-9]+\.[0-9]+|dev'; then echo "FAIL: installed binary --version failed: $INSTALLED_VER" - rm -rf "$REPLACE_DIR" + smoke_rmtree "$REPLACE_DIR" exit 1 fi @@ -986,7 +1011,7 @@ chmod 755 "$INSTALL_DIR/codebase-memory-mcp" REPLACED_VER=$("$INSTALL_DIR/codebase-memory-mcp" --version 2>&1) if ! echo "$REPLACED_VER" | grep -qE 'v?[0-9]+\.[0-9]+|dev'; then echo "FAIL: replaced binary --version failed: $REPLACED_VER" - rm -rf "$REPLACE_DIR" + smoke_rmtree "$REPLACE_DIR" exit 1 fi echo "OK: binary replacement succeeded (version: $REPLACED_VER)" @@ -1000,12 +1025,12 @@ chmod 755 "$INSTALL_DIR/codebase-memory-mcp" READONLY_VER=$("$INSTALL_DIR/codebase-memory-mcp" --version 2>&1) if ! echo "$READONLY_VER" | grep -qE 'v?[0-9]+\.[0-9]+|dev'; then echo "FAIL: read-only replacement --version failed: $READONLY_VER" - rm -rf "$REPLACE_DIR" + smoke_rmtree "$REPLACE_DIR" exit 1 fi echo "OK: read-only binary replacement succeeded" -rm -rf "$REPLACE_DIR" +smoke_rmtree "$REPLACE_DIR" echo "" echo "=== Phase 7: MCP advanced tool calls ===" @@ -2671,7 +2696,7 @@ if ! echo "$INSTALL_OUT" | grep -qi 'detected agents'; then fi echo "OK 9b-1: install with minimal agents exits cleanly" retire_account_daemon "9b-1-cleanup" -rm -rf "$EMPTY_HOME" +smoke_rmtree "$EMPTY_HOME" # 9b-2: Install twice (idempotent) IDEM_HOME=$(smoke_mktemp_dir) @@ -2695,7 +2720,7 @@ if [ "$COUNT" != "1" ]; then fi echo "OK 9b-2: double install is idempotent" retire_account_daemon "9b-2-cleanup" -rm -rf "$IDEM_HOME" +smoke_rmtree "$IDEM_HOME" # 9b-3: Uninstall without prior install CLEAN_HOME=$(smoke_mktemp_dir) @@ -2708,7 +2733,7 @@ if [ "$UNINSTALL_RC" -ge 128 ]; then fi echo "OK 9b-3: uninstall without install doesn't crash" retire_account_daemon "9b-3-cleanup" -rm -rf "$CLEAN_HOME" +smoke_rmtree "$CLEAN_HOME" # 9b-4: Install over corrupt JSON CORRUPT_HOME=$(smoke_mktemp_dir) @@ -2719,7 +2744,7 @@ run_no_crash 9b-4 env HOME="$CORRUPT_HOME" "$BINARY" install -y # Should either fix it or handle gracefully — not crash echo "OK 9b-4: install over corrupt JSON doesn't crash" retire_account_daemon "9b-4-cleanup" -rm -rf "$CORRUPT_HOME" +smoke_rmtree "$CORRUPT_HOME" # 9b-8: Double uninstall DBL_HOME=$(smoke_mktemp_dir) @@ -2734,7 +2759,7 @@ run_no_crash 9b-8-first env HOME="$DBL_HOME" "$DBL_UNINSTALLER" uninstall -y -n run_no_crash 9b-8-second env HOME="$DBL_HOME" "$BINARY" uninstall -y -n echo "OK 9b-8: double uninstall doesn't crash" retire_account_daemon "9b-8-cleanup" -rm -rf "$DBL_HOME" +smoke_rmtree "$DBL_HOME" # 9b-9: Non-interactive update without --standard/--ui should fail cleanly (not hang) if [ "$(uname -s)" != "MINGW64_NT" ] 2>/dev/null; then @@ -2748,7 +2773,7 @@ if [ "$(uname -s)" != "MINGW64_NT" ] 2>/dev/null; then fi retire_account_daemon "9-cleanup" -rm -rf "$FAKE_HOME" "$EMPTY_HOME" +smoke_rmtree "$FAKE_HOME" "$EMPTY_HOME" if [ "$SMOKE_MODE" = "--agent-config-only" ]; then echo "" @@ -2848,7 +2873,7 @@ else fi fi -rm -rf "$SECURITY_DIR" +smoke_rmtree "$SECURITY_DIR" echo "" echo "=== Phase 11: process kill E2E ===" @@ -3063,7 +3088,7 @@ sys.exit(0) exit 1 fi - rm -rf "$UPDATE_HOME" + smoke_rmtree "$UPDATE_HOME" else # Local mode: basic binary replacement test (no download) @@ -3083,7 +3108,7 @@ else exit 1 fi echo "OK 14: binary replacement + verify (local mode)" - rm -rf "$UPDATE_DIR" + smoke_rmtree "$UPDATE_DIR" fi # ── Phase 12 + 13: Download E2E + install script E2E (CI only) ── @@ -3234,7 +3259,7 @@ else echo "OK 12f: binary runs without signing ($DL_OS)" fi -rm -rf "$DL_DIR" +smoke_rmtree "$DL_DIR" echo "" echo "=== Phase 13: install script E2E ===" @@ -3301,7 +3326,7 @@ if [ "$DL_OS" != "windows" ] && [ -f "$REPO_ROOT/install.sh" ]; then echo "OK 13f: PATH setup (rc file may not have been modified if already present)" fi - rm -rf "$INSTALL_TEST_HOME" "$INSTALL_TEST_DIR" + smoke_rmtree "$INSTALL_TEST_HOME" "$INSTALL_TEST_DIR" elif [ -f "$REPO_ROOT/install.ps1" ] && command -v powershell.exe &>/dev/null; then echo "--- Phase 13: install.ps1 E2E (Windows) ---" @@ -3356,7 +3381,7 @@ elif [ -f "$REPO_ROOT/install.ps1" ] && command -v powershell.exe &>/dev/null; t exit 1 fi - rm -rf "$PS1_TEST_HOME" "$PS1_TEST_DIR" + smoke_rmtree "$PS1_TEST_HOME" "$PS1_TEST_DIR" else echo "SKIP Phase 13: no install script available for this platform" fi diff --git a/src/cli/activation_transaction.c b/src/cli/activation_transaction.c index 2445703f6..90cda8159 100644 --- a/src/cli/activation_transaction.c +++ b/src/cli/activation_transaction.c @@ -1118,6 +1118,40 @@ static bool activation_sync_directory(const cbm_activation_transaction_t *transa #endif } +#ifdef _WIN32 +/* Renaming is how this transaction both publishes and retires -- and on Windows + * it is the ONLY mutation permitted on a running image, which is what lets a + * single binary replace or remove itself at all. It can still lose to a handle + * that is about to go away: an antivirus scan of a just-written executable, or + * a child process the OS has not finished reaping. Both surface as a sharing or + * access violation and both clear within moments. + * + * install.ps1 already retries this exact operation for this exact reason. The C + * path giving up on the first attempt is what let `uninstall` intermittently + * abandon a live installation with "cleanup may remain" -- observed on one + * runner while an identical job on the same image passed. + * + * Bounded on purpose: a file that is genuinely held must still fail, and fail + * closed, rather than spin. */ +#define ACTIVATION_RENAME_ATTEMPTS 10U +#define ACTIVATION_RENAME_RETRY_MS 500U + +static unsigned int activation_rename_failures_for_test; + +static bool activation_rename_error_is_transient(DWORD error) { + return error == ERROR_SHARING_VIOLATION || error == ERROR_ACCESS_DENIED || + error == ERROR_LOCK_VIOLATION; +} +#endif + +void cbm_activation_transaction_rename_failures_set_for_test(unsigned int count) { +#ifdef _WIN32 + activation_rename_failures_for_test = count; +#else + (void)count; +#endif +} + static bool activation_rename(const cbm_activation_transaction_t *transaction, const char *source, const char *source_name, const char *destination, const char *destination_name, bool replace_destination) { @@ -1128,9 +1162,23 @@ static bool activation_rename(const cbm_activation_transaction_t *transaction, c wchar_t *wide_source = activation_utf8_to_wide(source); wchar_t *wide_destination = activation_utf8_to_wide(destination); DWORD flags = MOVEFILE_WRITE_THROUGH | (replace_destination ? MOVEFILE_REPLACE_EXISTING : 0); - bool ok = wide_source && wide_destination && - MoveFileExW(wide_source, wide_destination, flags) != 0 && - activation_directory_still_valid(transaction); + bool moved = false; + for (unsigned int attempt = 0; + wide_source && wide_destination && !moved && attempt < ACTIVATION_RENAME_ATTEMPTS; + attempt++) { + if (activation_rename_failures_for_test > 0) { + activation_rename_failures_for_test--; + } else if (MoveFileExW(wide_source, wide_destination, flags) != 0) { + moved = true; + break; + } else if (!activation_rename_error_is_transient(GetLastError())) { + break; + } + if (attempt + 1U < ACTIVATION_RENAME_ATTEMPTS) { + Sleep(ACTIVATION_RENAME_RETRY_MS); + } + } + bool ok = moved && activation_directory_still_valid(transaction); free(wide_source); free(wide_destination); return ok; diff --git a/src/cli/activation_transaction.h b/src/cli/activation_transaction.h index ac67a34be..e3847ea46 100644 --- a/src/cli/activation_transaction.h +++ b/src/cli/activation_transaction.h @@ -39,6 +39,11 @@ typedef void (*cbm_activation_transaction_before_absent_publish_for_test_fn)( void cbm_activation_transaction_set_before_absent_publish_for_test( cbm_activation_transaction_before_absent_publish_for_test_fn hook, void *context); +/* Test-only seam: make the next `count` Windows rename attempts fail as though + * another handle held the file, so the transient-lock retry can be proven + * without racing a real scanner. Inert on POSIX and when count is 0. */ +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. */ cbm_activation_transaction_status_t cbm_activation_transaction_stage_bytes( diff --git a/tests/test_activation_transaction.c b/tests/test_activation_transaction.c index 39e68fe0b..c73ae3b8a 100644 --- a/tests/test_activation_transaction.c +++ b/tests/test_activation_transaction.c @@ -543,6 +543,53 @@ TEST(activation_transaction_removal_can_rollback_or_finalize) { PASS(); } +#ifdef _WIN32 +/* A running Windows image can be renamed but never overwritten, so retiring it + * is a rename -- and that rename can lose to a handle that is about to go away + * (an antivirus scan of a fresh executable, a child the OS has not reaped). + * Both clear on their own, so the retry must absorb them rather than abandon a + * live installation. The failure seam substitutes for that timing instead of + * waiting on a real scanner, which no test could schedule. */ +TEST(activation_transaction_removal_survives_transient_rename_locks) { + char directory[ACTIVATION_TEST_PATH_CAP]; + char target[ACTIVATION_TEST_PATH_CAP]; + ASSERT_TRUE(activation_test_fixture(directory)); + ASSERT_TRUE(activation_test_path(target, directory, "cbm")); + ASSERT_TRUE(activation_test_write(target, "old")); + activation_test_validation_t absent = { + .expect_absent = true, + .expected_contents = NULL, + }; + + cbm_activation_transaction_t *transaction = NULL; + ASSERT_EQ(cbm_activation_transaction_stage_removal(target, &transaction), + CBM_ACTIVATION_TRANSACTION_OK); + /* Strictly inside the budget: the retire must still succeed. */ + cbm_activation_transaction_rename_failures_set_for_test(3U); + ASSERT_EQ(cbm_activation_transaction_commit(transaction, activation_test_validate, &absent), + CBM_ACTIVATION_TRANSACTION_OK); + cbm_activation_transaction_rename_failures_set_for_test(0U); + ASSERT_FALSE(activation_test_exists(target)); + ASSERT_EQ(cbm_activation_transaction_finalize(transaction), CBM_ACTIVATION_TRANSACTION_OK); + ASSERT_EQ(cbm_activation_transaction_close(&transaction), CBM_ACTIVATION_TRANSACTION_OK); + + /* Past the budget it must still FAIL, and leave the target in place: a + * genuinely held file is not something to spin on or to report as removed. */ + ASSERT_TRUE(activation_test_write(target, "old")); + ASSERT_EQ(cbm_activation_transaction_stage_removal(target, &transaction), + CBM_ACTIVATION_TRANSACTION_OK); + cbm_activation_transaction_rename_failures_set_for_test(64U); + ASSERT_EQ(cbm_activation_transaction_commit(transaction, activation_test_validate, &absent), + CBM_ACTIVATION_TRANSACTION_IO); + cbm_activation_transaction_rename_failures_set_for_test(0U); + ASSERT_TRUE(activation_test_exists(target)); + (void)cbm_activation_transaction_rollback(transaction); + ASSERT_EQ(cbm_activation_transaction_close(&transaction), CBM_ACTIVATION_TRANSACTION_OK); + ASSERT_EQ(th_rmtree(directory), 0); + PASS(); +} +#endif + TEST(activation_transaction_rejects_cross_account_writable_target_directory) { #ifndef _WIN32 char directory[ACTIVATION_TEST_PATH_CAP]; @@ -972,6 +1019,9 @@ SUITE(activation_transaction) { RUN_TEST(activation_transaction_stage_file_installs_new_target); RUN_TEST(activation_transaction_stage_file_survives_long_target_path); RUN_TEST(activation_transaction_removal_can_rollback_or_finalize); +#ifdef _WIN32 + RUN_TEST(activation_transaction_removal_survives_transient_rename_locks); +#endif RUN_TEST(activation_transaction_rejects_cross_account_writable_target_directory); RUN_TEST(activation_transaction_rejects_windows_callback_allow_directory_ace); RUN_TEST(activation_transaction_rejects_symlink_candidate_target_and_parent);