Merge pull request #1322 from DeusData/fix/windows-uninstall-rename-retry

fix(windows): absorb transient file locks when retiring a running image

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

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;

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(

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);