diff --git a/scripts/install/install.sh b/scripts/install/install.sh index e40cb02f43..fff99229a5 100755 --- a/scripts/install/install.sh +++ b/scripts/install/install.sh @@ -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="" diff --git a/scripts/install/test_install_sh.py b/scripts/install/test_install_sh.py index ad69ac4b78..7cd18deb44 100644 --- a/scripts/install/test_install_sh.py +++ b/scripts/install/test_install_sh.py @@ -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])