diff --git a/api/rollback.py b/api/rollback.py index 004c743c..d89dee76 100644 --- a/api/rollback.py +++ b/api/rollback.py @@ -169,22 +169,39 @@ def _read_workspace_text(workspace_root: Path, rel_path: str) -> str | None: target = safe_resolve_ws(workspace_root, rel_path) except (ValueError, OSError): return None + # Pre-check the leaf type WITHOUT opening it: open_anchored_fd opens with + # blocking O_RDONLY, which would HANG on a FIFO/special file swapped in at a + # regular-file path. lstat on the already-symlink-resolved target tells us the + # type without an open, so we never block on a non-regular leaf. (The old + # Path.is_file() guard likewise never opened a FIFO.) + try: + if not stat.S_ISREG(os.lstat(target).st_mode): + return None + except OSError: + return None try: fd = open_anchored_fd(workspace_root, target, want_dir=False) except (FileNotFoundError, ValueError, OSError): return None + # From here the fd is owned; make sure every path closes it exactly once. try: st = os.fstat(fd) if not stat.S_ISREG(st.st_mode): + os.close(fd) return None - with os.fdopen(fd, "rb") as fh: - return fh.read().decode(errors="replace") + fh = os.fdopen(fd, "rb") except OSError: try: os.close(fd) except OSError: pass return None + # os.fdopen succeeded → the file object owns the fd and will close it. + try: + with fh: + return fh.read().decode(errors="replace") + except OSError: + return None # ── Public API functions (called from routes.py) ──────────────────────────── diff --git a/tests/test_rollback_diff_symlink_disclosure.py b/tests/test_rollback_diff_symlink_disclosure.py index 4206801b..c5bab2ad 100644 --- a/tests/test_rollback_diff_symlink_disclosure.py +++ b/tests/test_rollback_diff_symlink_disclosure.py @@ -125,3 +125,31 @@ def test_checkpoint_diff_renders_large_workspace_file_as_modified_not_deleted(tm "(the capped read_file_content path regressed this): got " f"{statuses.get('big.txt')!r}" ) + + +def test_checkpoint_diff_skips_workspace_fifo_without_hanging(tmp_path, monkeypatch): + """Regression (Codex gate): a workspace FIFO/special file at a checkpoint- + tracked regular-file path must NOT hang the diff (open_anchored_fd opens + blocking O_RDONLY, which blocks forever on a FIFO with no writer) and must + not leak an fd — the leaf type is pre-checked via lstat before any open. + """ + import threading + + workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch) + _commit_checkpoint_file(ckpt_dir, "pipe.txt", "checkpoint content\n") + # Replace the workspace-side file with a FIFO (no writer → blocking open hangs). + os.mkfifo(workspace / "pipe.txt") + + result_box = {} + + def _run(): + result_box["r"] = rollback.get_checkpoint_diff(str(workspace), checkpoint) + + t = threading.Thread(target=_run, daemon=True) + t.start() + t.join(timeout=10) + assert not t.is_alive(), "get_checkpoint_diff hung on a workspace FIFO" + # FIFO workspace side is treated as absent → checkpoint file renders as deleted, + # never blocks and never leaks the secret/hangs. + statuses = {f["file"]: f.get("status") for f in result_box["r"]["files_changed"]} + assert statuses.get("pipe.txt") == "deleted"