diff --git a/cli/tui/text.py b/cli/tui/text.py index c7712b170..c4738df4a 100644 --- a/cli/tui/text.py +++ b/cli/tui/text.py @@ -81,5 +81,8 @@ def workspace_path(path: str, workspace: str | None) -> str: except ValueError: pass else: - return str(relative) or "." + # Display form: forward slashes on every platform, so a path reads + # the same on Windows as it does in the transcript examples (and in + # what the user types: ``tools/foo.py``). + return relative.as_posix() or "." return short_path(candidate) diff --git a/core/persistence/database.py b/core/persistence/database.py index 2b1d12af2..576cdf441 100644 --- a/core/persistence/database.py +++ b/core/persistence/database.py @@ -30,6 +30,11 @@ def default_database_path() -> Path: return deepcode_home() / "state" / "deepcode.sqlite3" +# Database files whose ACLs this process already repaired — see +# Database._harden_files for why re-hardening on every open is wrong. +_hardened_files: set[str] = set() + + class Database: """Connection factory; no process-global mutable connection is retained.""" @@ -114,8 +119,38 @@ def _connect(self) -> sqlite3.Connection: return connection def _harden_files(self) -> None: + """Repair the database files' permissions at most once per process. + + ``ensure_private_file`` re-applies the Windows ACL (3 ``icacls`` spawns + per file), and this runs on every connect, read and transaction: the + suite spawned tens of thousands of ``icacls`` processes, which is both + slow and — because ``CreateProcess`` can stall for minutes under that + much spawn pressure — a way to wedge the process. It also contradicts + the "restrict at creation, not per open" rule this module already + follows (``ensure_private_directory`` restricts only what it created) + and the ACL tests pin (``tests/test_private_storage_acl_once.py``). + + Keyed by path, deliberately without re-checking identity: a sibling that + SQLite deletes and recreates (``-wal``/``-shm``) is created inside the + already restricted directory, so it inherits the restricted ACL — + re-running ``icacls`` on every recreation is exactly the per-open + re-hardening this avoids. The parent directory is restricted first, in + ``_connect``/``initialize``, which is what makes that inheritance hold. + """ + for suffix in ("", "-wal", "-shm", "-journal"): - ensure_private_file(Path(f"{self.path}{suffix}")) + path = Path(f"{self.path}{suffix}") + key = os.fspath(path) + if key in _hardened_files: + continue + try: + path.lstat() + except OSError: + # Not created yet: it will be seen (and repaired) the first time + # it exists, not on every open before that. + continue + ensure_private_file(path) + _hardened_files.add(key) def _migration_lock_path(self) -> Path: return self.path.with_name(f"{self.path.name}.migration.lock") diff --git a/core/private_storage.py b/core/private_storage.py index 2ab496b99..e1d615bb4 100644 --- a/core/private_storage.py +++ b/core/private_storage.py @@ -16,6 +16,7 @@ import os import stat import subprocess +import threading from functools import lru_cache from pathlib import Path @@ -90,26 +91,50 @@ def _windows_icacls() -> str | None: return None +_ICACLS_TIMEOUT_SECONDS = 15.0 +# How long the caller waits for the whole spawn before giving up and leaving the +# ACLs as they are. See _run_icacls for why that wait has to exist. +_ICACLS_ABANDON_AFTER_SECONDS = _ICACLS_TIMEOUT_SECONDS + 5.0 + + def _run_icacls(executable: str, path: Path, *arguments: str) -> bool: - """Run one bounded icacls operation and report whether it succeeded.""" + """Run one bounded icacls operation and report whether it succeeded. - try: - subprocess.run( - [executable, os.fspath(path), *arguments], - capture_output=True, - text=True, - encoding="mbcs", - errors="replace", - timeout=15, - check=True, - # A detached service has no console to inherit. Avoid allocating - # a new console for every ACL helper it launches. - creationflags=subprocess.CREATE_NO_WINDOW if os.name == "nt" else 0, - stdin=subprocess.DEVNULL, - ) - except (OSError, subprocess.SubprocessError): + Bounded end to end: both the child's run *and* its creation are covered, because + the caller must never be able to hang here. ``subprocess``'s own ``timeout`` + only starts once the process exists, and ``CreateProcess`` itself is unbounded, + so the spawn runs on a daemon worker thread that may be abandoned mid-flight. + """ + + completed = threading.Event() + succeeded: list[bool] = [] + + def _attempt() -> None: + try: + subprocess.run( + [executable, os.fspath(path), *arguments], + capture_output=True, + text=True, + encoding="mbcs", + errors="replace", + timeout=_ICACLS_TIMEOUT_SECONDS, + check=True, + # A detached service has no console to inherit. Avoid allocating + # a new console for every ACL helper it launches. + creationflags=subprocess.CREATE_NO_WINDOW if os.name == "nt" else 0, + stdin=subprocess.DEVNULL, + ) + succeeded.append(True) + except (OSError, subprocess.SubprocessError): + succeeded.append(False) + finally: + completed.set() + + worker = threading.Thread(target=_attempt, name="deepcode-icacls", daemon=True) + worker.start() + if not completed.wait(_ICACLS_ABANDON_AFTER_SECONDS): return False - return True + return bool(succeeded and succeeded[0]) def _restrict_windows_acl(path: Path) -> None: diff --git a/pyproject.toml b/pyproject.toml index bf1e690c1..9bd0c3c6b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,6 +7,10 @@ testpaths = ["tests"] pythonpath = ["."] # Emit thread stacks for a stalled test without changing its pass/fail budget. faulthandler_timeout = 60 +# A stalled test must stop the run instead of sitting there all night: on +# 2026-09-13 a TUI test blocked inside CreateProcess (ACL hardening, see +# core/private_storage.py) and the suite produced no output until morning. +faulthandler_exit_on_timeout = true filterwarnings = [ "error::pytest.PytestUnhandledThreadExceptionWarning", "error::pytest.PytestUnraisableExceptionWarning", diff --git a/tests/persistence/test_database.py b/tests/persistence/test_database.py index 81c8f36e4..fed83fd20 100644 --- a/tests/persistence/test_database.py +++ b/tests/persistence/test_database.py @@ -375,3 +375,42 @@ def test_database_rejects_cross_thread_execution_records(tmp_path: Path) -> None with pytest.raises(sqlite3.IntegrityError): with database.transaction() as connection: ItemRepository(connection).add(cross_thread_item) + + +def test_an_existing_database_is_not_restricted_again_on_every_open( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Opening the database must not re-apply the ACL (3 ``icacls`` spawns/file). + + ``_harden_files`` runs on every connect, read and transaction. Re-applying + the restriction there contradicts the "restrict at creation, not per open" + contract (tests/test_private_storage_acl_once.py) and spawned tens of + thousands of ``icacls`` processes per suite run — and under that much + process-creation pressure ``CreateProcess`` was observed to block for + minutes, which is how a TUI test (and the overnight suite) wedged on + 2026-09-14. New ``-wal``/``-shm`` files inherit the restricted directory + ACL, so one repair per file identity is enough. + """ + + import core.persistence.database as database_module + + hardened: list[str] = [] + monkeypatch.setattr( + database_module, + "ensure_private_file", + lambda path: hardened.append(Path(path).name), + ) + monkeypatch.setattr(database_module, "_hardened_files", set()) + + database = Database(tmp_path / "state" / "deepcode.sqlite3") + database.initialize() + + assert hardened.count("deepcode.sqlite3") == 1 + + for _ in range(5): + with database.read(): + pass + with database.transaction(): + pass + + assert hardened.count("deepcode.sqlite3") == 1, "an existing file is repaired once" diff --git a/tests/test_private_storage_acl_once.py b/tests/test_private_storage_acl_once.py index 2ea778d9a..92883b029 100644 --- a/tests/test_private_storage_acl_once.py +++ b/tests/test_private_storage_acl_once.py @@ -171,3 +171,44 @@ def fail_strip(executable, path, *arguments): ("/grant:r", "DOMAIN\\user:F"), ("/inheritance:r",), ] + + +def test_icacls_is_abandoned_when_the_spawn_itself_blocks( + monkeypatch, tmp_path: Path +) -> None: + """A stuck ``CreateProcess`` must not be able to wedge the caller. + + ``subprocess.run(timeout=...)`` starts counting only once the child exists: + ``_winapi.CreateProcess`` is unbounded, and under sustained spawn pressure + (a Windows security product filtering every process creation) it blocked for + minutes *past* the declared 15s timeout — the stall that hung the whole test + suite on 2026-09-14. The spawn therefore runs on a worker thread the module + is willing to abandon; a stuck creation degrades to "ACLs left as they are". + """ + + import threading + import time + + import core.private_storage as ps + + started = threading.Event() + release = threading.Event() + + def blocking_spawn(*_args, **_kwargs): + started.set() + release.wait(30) # never released in the failing case: creation never returns + raise ps.subprocess.TimeoutExpired("icacls", 15) + + monkeypatch.setattr(ps.subprocess, "run", blocking_spawn) + monkeypatch.setattr(ps, "_ICACLS_ABANDON_AFTER_SECONDS", 0.2) + + began = time.monotonic() + try: + result = ps._run_icacls( + "trusted-icacls.exe", tmp_path / "state.jsonl", "/grant:r", "u:F" + ) + assert time.monotonic() - began < 5, "the caller must return promptly" + assert result is False, "fail safe: report failure instead of hanging" + assert started.wait(5) is True, "the spawn was actually attempted" + finally: + release.set()