Fix macOS standalone release pruning (#57503)

## Why macOS `lsof +D` can return status 1 even when it finds open files, causing release cleanup to be skipped. It can also escape non-ASCII installation paths, preventing full-path matching from recognizing releases in use. ## What changed Accept `lsof` status 1 while preserving matches, and continue to reject other failures or any stderr. Match the installer-owned ASCII path suffix on macOS and recognize processes whose working directory is the release root. Apply these checks before and after moving a release for deletion. ## Testing Add coverage for `lsof` exit statuses, output, and errors. Exercise release retention with a non-ASCII installation path and, on macOS, a process holding the release directory open. GitOrigin-RevId: 06181467c7d1bb256b6781241a444976b890d7c9
2026-10-28 18:58:46 +00:00 · 2026-10-27 16:21:43 +00:00
parent f40f3dfeb9
commit 615a8c98d6
2 changed files with 71 additions and 6 deletions
@@ -807,13 +807,26 @@ record_standalone_releases_in_use() {
    if /usr/sbin/lsof -nP -F n +D "$releases" > "$tmp_dir/in-use" 2> "$tmp_dir/lsof-error"; then
      :
    else
-      # lsof returns 1 for an empty result as well as for errors.
-      [ ! -s "$tmp_dir/in-use" ] && [ ! -s "$tmp_dir/lsof-error" ] || return 1
+      # macOS lsof +D can return 1 with matches when some files are not open.
+      # Keep those matches, but reject other failures and any stderr below.
+      [ "$?" -eq 1 ] || return 1
    fi
    [ ! -s "$tmp_dir/lsof-error" ] || return 1
  fi
 }

+release_is_unused() {
+  if [ "$os" = "linux" ]; then
+    path_field="n$1"
+  else
+    # lsof +D scopes the scan to this tree, but may escape bytes in CODEX_HOME.
+    # Match the installer-owned ASCII path, including the temporary retirement name.
+    path_field="/packages/${STANDALONE_ROOT##*/}/releases/${1#"$releases"/}"
+  fi
+  # Appending / also matches a process whose cwd is the release root itself.
+  LC_ALL=C RELEASE_PATH="$path_field/" awk 'index($0 "/", ENVIRON["RELEASE_PATH"]) { exit 1 }' "$tmp_dir/in-use"
+}
+
 # Run under the install lock, after the visible command has been verified.
 # Only retire ordinary, manifest-owned packages; never retry a retired tree.
 prune_standalone_releases() (
@@ -847,7 +860,7 @@ prune_standalone_releases() (
  for candidate in "$releases"/*; do
    [ -d "$candidate" ] && [ ! -L "$candidate" ] &&
      [ "$candidate" != "$selected" ] && [ "${candidate##*/}" != "$previous" ] || continue
-    grep -F "n$candidate/" "$tmp_dir/in-use" >/dev/null && continue
+    release_is_unused "$candidate" || continue
    [ -f "$candidate/codex-package.json" ] && [ ! -L "$candidate/codex-package.json" ] || continue
    metadata="$(parse_release_metadata < "$candidate/codex-package.json")" || continue
    version="$(printf '%s\n' "$metadata" | awk -F '\t' '$1 == "version" {print $2}')"
@@ -893,7 +906,7 @@ prune_standalone_releases() (
      # Block new launches at the original path before the final use check.
      # Some systems report the old executable path, so check both names.
      if ! record_standalone_releases_in_use ||
-        grep -F -e "n$candidate/" -e "n$retired/release/" "$tmp_dir/in-use" >/dev/null; then
+        ! release_is_unused "$candidate" || ! release_is_unused "$retired/release"; then
        [ ! -e "$candidate" ] && [ ! -L "$candidate" ] || return 1
        mv "$retired/release" "$candidate" && rmdir "$retired" || return 1
        restore_candidate=""
@@ -321,12 +321,55 @@ class InstallShTest(unittest.TestCase):
            self.assertEqual(guarded.returncode, 0, guarded.stderr)
            self.assertTrue(managed.exists())

+    def test_macos_release_inspection_preserves_lsof_matches(self) -> None:
+        # Exercise the macOS branch on every platform, including lsof's absolute path.
+        function = (
+            INSTALL_SCRIPT.read_text()
+            .split("record_standalone_releases_in_use() {", 1)[1]
+            .split("\n}\n", 1)[0]
+        )
+        function = "record_standalone_releases_in_use() {" + function + "\n}\n"
+        function = function.replace("/usr/sbin/lsof ", "mock_lsof ")
+        matches = "p123\nn/releases/active/bin/codex\n"
+        for status, output, error, expected in (
+            (0, matches, "", 0),
+            (1, "", "", 0),
+            (1, matches, "", 0),
+            (0, matches, "inspection failed\n", 1),
+            (1, matches, "inspection failed\n", 1),
+            (2, "", "", 1),
+            (2, matches, "", 1),
+        ):
+            with self.subTest(status=status, output=output, error=error):
+                with tempfile.TemporaryDirectory() as temp_dir:
+                    script = (
+                        "set -eu\n"
+                        "os=macos\n"
+                        "releases=/releases\n"
+                        f"tmp_dir={shlex.quote(temp_dir)}\n"
+                        # Avoid depending on other processes on the test host.
+                        'ps() { printf "%s codex\\n" "$(id -u)"; }\n'
+                        "mock_lsof() {\n"
+                        f"  printf '%s' {shlex.quote(output)}\n"
+                        f"  printf '%s' {shlex.quote(error)} >&2\n"
+                        f"  return {status}\n"
+                        "}\n" + function + "record_standalone_releases_in_use\n"
+                    )
+                    result = subprocess.run(
+                        ["sh"], input=script, text=True, capture_output=True
+                    )
+                    self.assertEqual(
+                        (result.returncode, result.stdout, result.stderr),
+                        (expected, "", ""),
+                    )
+                    self.assertEqual((Path(temp_dir) / "in-use").read_text(), output)
+
    def test_standalone_retains_previous_and_running_releases_across_installs(
        self,
    ) -> None:
        with tempfile.TemporaryDirectory() as temp_dir:
-            # Exercise real process inspection, including paths containing spaces.
-            root = Path(temp_dir).resolve() / "codex install"
+            # macOS lsof can escape the non-ASCII bytes in the installation path.
+            root = Path(temp_dir).resolve() / "André codex install"
            root.mkdir()
            system = os.uname()
            if system.sysname == "Linux":
@@ -517,6 +560,15 @@ class InstallShTest(unittest.TestCase):
                os.close(resume_fd)
                fake_mv.unlink()

+            if system.sysname == "Darwin":
+                # An external process can hold just the release directory open.
+                with subprocess.Popen(["/bin/cat"], stdin=subprocess.PIPE, cwd=oldest):
+                    held, _ = run_installer_in(
+                        root, "latest", **install_options[VERSION]
+                    )
+                    self.assertEqual(held.returncode, 0, held.stderr)
+                    self.assertTrue(oldest.is_dir(), held.stderr)
+
            # A no-op latest install (also the codex update path) cleans up the
            # now-unused release without rotating previous to current.
            repeated, _ = run_installer_in(root, "latest", **install_options[VERSION])