Terminal env snapshot concurrency and atomicity fixes

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #54098 fix(terminal): atomic env-snapshot writes keyed on $BASHPID to prevent PATH corruption (#38249)

  merged to main

  files: infographic/atomic-env-snapshot-38249/infographic.png, tests/tools/test_base_environment.py, tools/environments/base.py, tools/environments/local.py

  Show the diff

  • tests/tools/test_base_environment.py

    @@ -90,6 +90,166 @@ def test_cd_failure_exit_126(self):
             assert "exit 126" in wrapped
     
     
    +class TestAtomicSnapshotWrite:
    +    """Regression for #38249: concurrent terminal calls in one session both
    +    source AND rewrite the shared env snapshot. A non-atomic ``export -p >
    +    snap`` truncates-then-writes in place, so a concurrent ``source snap`` can
    +    read a half-written file and embed ``declare -x``/``export`` fragments into
    +    PATH, breaking ``ls``/``git``/``tr`` with command-not-found. The write must
    +    assemble in a temp file and ``mv -f`` it into place (mv is atomic on POSIX
    +    same-fs), so a reader sees the old-or-new complete file, never a torn one.
    +    """
    +
    +    def test_wrap_command_uses_atomic_temp_then_mv(self):
    +        env = _TestableEnv()
    +        env._snapshot_ready = True
    +        wrapped = env._wrap_command("echo hi", "/tmp")
    +        # Env dump goes to a temp file, not directly over the live snapshot.
    +        assert "export -p > " in wrapped
    +        assert ".tmp." in wrapped
    +        # Then an atomic rename onto the real snapshot path.
    +        assert "mv -f " in wrapped
    +        # The env-dump must NOT write the live snapshot in place (the bug).
    +        snap = env._snapshot_path
    +        assert f"export -p > {snap} " not in wrapped
    +        assert f"export -p > '{snap}'" not in wrapped
    +
    +    def test_temp_path_uses_bashpid_not_dollardollar(self):
    +        """The temp name MUST use ``$BASHPID`` (the real subshell PID), not
    +        ``$$``.  In ``&``-launched concurrent subshells ``$$`` stays the parent
    +        shell's PID, so two writers would pick the same temp name, clobber each
    +        other mid-write, and mv would publish a torn file — the corruption is
    +        only narrowed, not closed.  This is the bug shared by every prior PR in
    +        the #38249 cluster."""
    +        env = _TestableEnv()
    +        env._snapshot_ready = True
    +        wrapped = env._wrap_command("echo hi", "/tmp")
    +        assert "$BASHPID" in wrapped
    +        # The bare $$ temp form must be gone.
    +        assert ".tmp.$$" not in wrapped
    +
    +    def test_temp_path_static_part_is_quoted_bashpid_outside(self):
    +        """The static path portion must be shlex-quoted (Windows/Git-Bash
    +        ``C:/Users/...`` or spaces) while ``$BASHPID`` stays OUTSIDE the quotes
    +        so it still expands."""
    +        env = _TestableEnv()
    +        env._snapshot_ready = True
    +        env._snapshot_path = "/tmp/has space/hermes-snap-x.sh"
    +        wrapped = env._wrap_command("echo hi", "/tmp")
    +        # The static path (with its space) is shlex-quoted as a single word, with
    +        # $BASHPID appended OUTSIDE the quotes so it still expands at runtime.
    +        assert "'/tmp/has space/hermes-snap-x.sh.tmp.'$BASHPID" in wrapped
    +        # The space must never appear bare/unquoted in the temp token (that would
    +        # word-split into two args and break the redirect/mv).
    +        assert " space/hermes-snap-x.sh.tmp.$BASHPID" not in wrapped
    +
    +    def test_wrap_command_mv_chained_on_export_success(self):
    +        """A failed/partial ``export -p`` must NOT mv a torn temp over a good
    +        snapshot.  The mv is chained with ``&&`` on the export, and the temp is
    +        removed on failure."""
    +        env = _TestableEnv()
    +        env._snapshot_ready = True
    +        wrapped = env._wrap_command("echo hi", "/tmp")
    +        assert "export -p > " in wrapped and "&& mv -f " in wrapped
    +        assert "rm -f " in wrapped  # temp cleanup on failure
    +
    +    def test_init_session_bootstrap_also_atomic_and_bashpid(self):
    +        """The init_session bootstrap (first snapshot write) is the same shared
    +        file a concurrent command could source — it must be atomic and use
    +        ``$BASHPID`` too."""
    +        env = _TestableEnv()
    +        captured = {}
    +
    +        def fake_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
    +            captured["cmd"] = cmd_string
    +            raise RuntimeError("stop after capture")  # we only need the script
    +
    +        env._run_bash = fake_run_bash  # type: ignore[assignment]
    +        try:
    +            env.init_session()
    +        except Exception:
    +            pass
    +        boot = captured.get("cmd", "")
    +        assert ".tmp." in boot and "mv -f " in boot, boot
    +        assert "$BASHPID" in boot
    +        assert ".tmp.$$" not in boot
    +
    +
    +class TestAtomicSnapshotConcurrencyBehavioral:
    +    """Behavioral regression for #38249 — actually EXECUTES the generated
    +    snapshot write/read concurrently and asserts the file never tears.
    +
    +    The string-inspection tests prove the right script is emitted; this proves
    +    the emitted script's guarantee holds under real concurrency: N concurrent
    +    writers + readers, and the snapshot is ALWAYS a complete, parseable env
    +    dump — never truncated mid-line with a ``declare -x`` / ``export`` fragment
    +    that would corrupt PATH.  Crucially it uses ``$BASHPID`` (per-subshell
    +    unique), which is what closes the race; ``$$`` would still tear here.
    +    """
    +
    +    def _run(self, script):
    +        import subprocess
    +        return subprocess.run(["/bin/bash", "-c", script], capture_output=True, text=True)
    +
    +    def test_concurrent_writes_never_tear_the_snapshot(self, tmp_path):
    +        import shutil
    +        if not shutil.which("bash"):
    +            import pytest
    +            pytest.skip("bash required")
    +        import shlex
    +        snap = str(tmp_path / "hermes-snap-x.sh")
    +        _q = shlex.quote
    +        _snap_tmp = _q(snap + ".tmp.") + "$BASHPID"
    +        # One writer iteration = the exact atomic sequence _wrap_command emits.
    +        writer = (
    +            "for i in $(seq 1 80); do "
    +            "export BIG_$i=$(head -c 600 /dev/zero | tr '\\0' x); "
    +            f"{{ export -p > {_snap_tmp} && mv -f {_snap_tmp} {_q(snap)}; }} "
    +            f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true; "
    +            "done"
    +        )
    +        # Reader: repeatedly source the snapshot and check PATH never absorbs
    +        # an `export `/`declare -x` fragment (the corruption signature).
    +        reader = (
    +            "export PATH=/usr/bin:/bin; "
    +            "for i in $(seq 1 160); do "
    +            f"( source {_q(snap)} >/dev/null 2>&1 || true; "
    +            "case \"$PATH\" in *'declare -x'*|*'export '*) echo CORRUPT;; esac ); "
    +            "done"
    +        )
    +        self._run(f"export -p > {_q(snap)}")  # seed a valid snapshot
    +        # 4 concurrent writers + 4 readers, repeated.
    +        w = " & ".join([writer] * 4)
    +        r = " & ".join([reader] * 4)
    +        procs = [self._run(f"{w} & {r} & wait") for _ in range(3)]
    +        corrupt = any("CORRUPT" in p.stdout for p in procs)
    +        assert not corrupt, "snapshot tore — PATH absorbed a declare-x/export fragment"
    +        final = self._run(f"source {_q(snap)} >/dev/null 2>&1 && echo OK || echo BROKEN")
    +        assert "OK" in final.stdout, f"final snapshot not sourceable: {final.stdout} {final.stderr}"
    +
    +    def test_failed_export_does_not_destroy_good_snapshot(self, tmp_path):
    +        """If ``export -p`` fails, the ``&&``-chained mv must NOT clobber the
    +        existing good snapshot."""
    +        import shutil
    +        if not shutil.which("bash"):
    +            import pytest
    +            pytest.skip("bash required")
    +        import shlex
    +        snap = str(tmp_path / "snap.sh")
    +        _q = shlex.quote
    +        self._run(f"echo 'export GOOD=1' > {_q(snap)}")  # seed good snapshot
    +        # Redirect export into an unwritable dir so the export side fails; mv
    +        # must then NOT run (&&) and not clobber snap.
    +        bad_tmp = _q("/nonexistent-dir/snap.tmp.") + "$BASHPID"
    +        script = (
    +            f"{{ export -p > {bad_tmp} && mv -f {bad_tmp} {_q(snap)}; }} "
    +            f"2>/dev/null || rm -f {bad_tmp} 2>/dev/null || true"
    +        )
    +        self._run(script)
    +        out = self._run(f"cat {_q(snap)}")
    +        assert "export GOOD=1" in out.stdout, "good snapshot was destroyed by a failed export"
    +
    +
     class TestExtractCwdFromOutput:
         def test_happy_path(self):
             env = _TestableEnv()

  • tools/environments/base.py

    @@ -371,13 +371,34 @@ def init_session(self):
             # backends) into every terminal-tool response.
             _quoted_snap = shlex.quote(self._snapshot_path)
             _quoted_cwd_file = shlex.quote(self._cwd_file)
    +        # Use atomic file replacement: assemble the snapshot in a temp file,
    +        # then mv it over the final path.  This prevents concurrent source()
    +        # calls from reading a half-written snapshot when another terminal
    +        # command finishes and rewrites the env vars (issue #38249).  `mv` is
    +        # atomic on POSIX when src and dest are on the same filesystem, so
    +        # source() either sees the old complete snapshot or the new complete
    +        # one — never a partial/truncated file.
    +        #
    +        # The temp name MUST be unique per concurrent writer.  ``$$`` is the
    +        # bash PID, but in ``&``-launched subshells (how concurrent terminal
    +        # calls run) ``$$`` stays the *parent* shell's PID — so two concurrent
    +        # writers would pick the SAME temp name, clobber each other's temp
    +        # mid-write, and mv would then publish a torn file (the corruption is
    +        # only narrowed, not closed).  ``$BASHPID`` is the actual subshell PID
    +        # and is genuinely unique per writer, which closes the race.  The
    +        # static path is shlex-quoted (Windows/Git-Bash drive letters, spaces)
    +        # with ``$BASHPID`` left outside the quotes so it still expands.
    +        _snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$BASHPID"
             bootstrap = (
    -            f"export -p > {_quoted_snap}\n"
    -            f"declare -f | grep -vE '^_[^_]' >> {_quoted_snap}\n"
    -            f"alias -p >> {_quoted_snap}\n"
    -            f"echo 'shopt -s expand_aliases' >> {_quoted_snap}\n"
    -            f"echo 'set +e' >> {_quoted_snap}\n"
    -            f"echo 'set +u' >> {_quoted_snap}\n"
    +            f"export -p > {_snap_tmp}\n"
    +            f"declare -f | grep -vE '^_[^_]' >> {_snap_tmp}\n"
    +            f"alias -p >> {_snap_tmp}\n"
    +            f"echo 'shopt -s expand_aliases' >> {_snap_tmp}\n"
    +            f"echo 'set +e' >> {_snap_tmp}\n"
    +            f"echo 'set +u' >> {_snap_tmp}\n"
    +            # Publish atomically only if assembly succeeded; otherwise drop the
    +            # partial temp rather than leave it to be sourced or orphaned.
    +            f"mv -f {_snap_tmp} {_quoted_snap} || rm -f {_snap_tmp}\n"
                 f"builtin cd {_quoted_cwd} 2>/dev/null || true\n"
                 f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true\n"
                 f"printf '\\n{self._cwd_marker}%s{self._cwd_marker}\\n' \"$(pwd -P)\"\n"
    @@ -427,6 +448,14 @@ def _wrap_command(self, command: str, cwd: str) -> str:
             # :meth:`init_session` for the same fix on the bootstrap block.
             _quoted_snap = shlex.quote(self._snapshot_path)
             _quoted_cwd_file = shlex.quote(self._cwd_file)
    +        # Use atomic file replacement for env snapshot updates (issue #38249).
    +        # Assemble into a per-writer-unique temp file, then mv to atomically
    +        # replace the snapshot so concurrent source() calls never read a
    +        # truncated/half-written file.  ``$BASHPID`` (not ``$$``) is the actual
    +        # subshell PID — unique per concurrent ``&``-launched writer — so two
    +        # writers never share a temp name and clobber each other before the mv.
    +        # Static path shlex-quoted (Windows/spaces); ``$BASHPID`` left to expand.
    +        _snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$BASHPID"
     
             parts = []
     
    @@ -451,9 +480,15 @@ def _wrap_command(self, command: str, cwd: str) -> str:
             parts.append(f"eval '{escaped}'")
             parts.append("__hermes_ec=$?")
     
    -        # Re-dump env vars to snapshot (last-writer-wins for concurrent calls)
    +        # Re-dump env vars to snapshot (atomic replacement to avoid races).
    +        # Chain mv on the export succeeding so a failed/partial dump never
    +        # replaces a good snapshot; drop the temp on failure so it isn't
    +        # orphaned (cleaned up wholesale in LocalEnvironment.cleanup too).
             if self._snapshot_ready:
    -            parts.append(f"export -p > {_quoted_snap} 2>/dev/null || true")
    +            parts.append(
    +                f"{{ export -p > {_snap_tmp} && mv -f {_snap_tmp} {_quoted_snap}; }} "
    +                f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true"
    +            )
     
             # Write CWD to file (local reads this) and stdout marker (remote parses this)
             parts.append(f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true")

  • tools/environments/local.py

    @@ -987,3 +987,14 @@ def cleanup(self):
                     os.unlink(f)
                 except OSError:
                     pass
    +        # Remove any orphaned atomic-write temp snapshots (snap.tmp.<bashpid>)
    +        # a failed/interrupted mv could have left behind (#38249).
    +        try:
    +            import glob
    +            for tmp in glob.glob(f"{self._snapshot_path}.tmp.*"):
    +                try:
    +                    os.unlink(tmp)
    +                except OSError:
    +                    pass
    +        except Exception:
    +            pass