From b8a0be3e1fbe75619f3cb140046a6192ab0b6e8e Mon Sep 17 00:00:00 2001 From: raymondginger Date: Mon, 14 Sep 2026 22:24:38 +0800 Subject: [PATCH 1/2] fix: do not re-apply the private-storage ACL on every database open `Database._harden_files` repaired the ACL of every database file on every connect, read and transaction. On Windows that is three `icacls` spawns per file, so a single test-suite run created tens of thousands of processes. Under that much process-creation pressure `_winapi.CreateProcess` was observed to block for minutes -- and it is not covered by `subprocess.run`'s own `timeout`, which only starts counting once the child exists -- so the caller could hang indefinitely. On 2026-09-13 that is what left the suite producing no output until morning. Two changes, both fail-safe: * `_harden_files` repairs each database file at most once per process. New `-wal`/`-shm` siblings are created inside the already restricted directory and inherit its ACL, which is the same "restrict at creation, not per open" rule `ensure_private_directory()` already follows and `tests/test_private_storage_acl_once.py` pins. * `_run_icacls` now runs the spawn on a daemon worker thread that the module is willing to abandon after `_ICACLS_TIMEOUT_SECONDS + 5`, so a stuck creation degrades to "ACLs left as they are" -- the fail-safe the rest of the module already promises -- instead of wedging the caller. `[tool.pytest.ini_options]` gains `faulthandler_exit_on_timeout = true` so a future stall stops the run with thread stacks rather than hanging. `cli/tui/text.py` renders workspace paths with `as_posix()`, so a path reads the same on Windows as it does in the transcript examples and in what the user types (`tools/foo.py`). --- cli/tui/text.py | 5 ++- core/persistence/database.py | 37 +++++++++++++++- core/private_storage.py | 59 ++++++++++++++++++-------- pyproject.toml | 4 ++ tests/persistence/test_database.py | 39 +++++++++++++++++ tests/test_private_storage_acl_once.py | 37 ++++++++++++++++ 6 files changed, 162 insertions(+), 19 deletions(-) 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..f6b2d19b9 100644 --- a/tests/test_private_storage_acl_once.py +++ b/tests/test_private_storage_acl_once.py @@ -171,3 +171,40 @@ 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() From d09c68b0d79295fa66661fd76d127790f4b9bf91 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Mon, 14 Sep 2026 23:27:44 +0800 Subject: [PATCH 2/2] style(test): apply ruff format to the icacls abandonment test The lint-and-format CI job runs ruff-format with the version pinned in .pre-commit-config.yaml (astral-sh/ruff-pre-commit v0.15.21). Two lines in tests/test_private_storage_acl_once.py exceeded the 88-column budget: * the signature of test_icacls_is_abandoned_when_the_spawn_itself_blocks * the ps._run_icacls("trusted-icacls.exe", ..., "/grant:r", "u:F") call ruff check did not flag them because E501 is not in its default select set, so the formatter was never run locally. Formatting with the pinned version reproduces the CI diff byte for byte; the change is line-wrapping only (8 passed before and after). --- tests/test_private_storage_acl_once.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_private_storage_acl_once.py b/tests/test_private_storage_acl_once.py index f6b2d19b9..92883b029 100644 --- a/tests/test_private_storage_acl_once.py +++ b/tests/test_private_storage_acl_once.py @@ -173,7 +173,9 @@ def fail_strip(executable, path, *arguments): ] -def test_icacls_is_abandoned_when_the_spawn_itself_blocks(monkeypatch, tmp_path: Path) -> None: +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: @@ -202,7 +204,9 @@ def blocking_spawn(*_args, **_kwargs): began = time.monotonic() try: - result = ps._run_icacls("trusted-icacls.exe", tmp_path / "state.jsonl", "/grant:r", "u:F") + 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"