diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b52969e..a6f5eb0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,9 +29,18 @@ jobs: no-dependencies: # The zero-dependency claim is a promise to users, so it gets its own job: install - # the package alone and prove the core still imports and passes. - runs-on: ubuntu-latest - timeout-minutes: 10 + # the package alone and prove the core still imports and passes. The matrix is + # where Windows gets covered (#7): every platform primitive the package touches + # lives in pyvdisk/compat.py, and the emulation tests force those paths on Linux, + # but only a real Windows runner exercises msvcrt.locking, the directory-fsync + # no-op and drive-letter paths against the actual OS. PR #5 proved the port + # works; this job keeps proving it on every push. + runs-on: ${{ matrix.os }} + timeout-minutes: 15 + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 @@ -47,3 +56,5 @@ jobs: python -c "import pyvdisk; print('pyvdisk', pyvdisk.__version__, 'imported with the standard library only')" - name: Tests (accelerator-dependent cases skip) run: python -m pytest -q --no-header -p no:cacheprovider + - name: Quickstart smoke (stdlib only, end to end) + run: python examples/quickstart.py diff --git a/.github/workflows/pylint.yml b/.github/workflows/pylint.yml index 157709b..5461c8c 100644 --- a/.github/workflows/pylint.yml +++ b/.github/workflows/pylint.yml @@ -26,3 +26,24 @@ jobs: # declared 3.9 floor still parses is proven by the CI test matrix, not here. - name: Lint (errors only) run: python -m pylint -E --score=n pyvdisk tests + + errors-windows: + # Same errors-only gate on Windows (#7). pylint -E resolves imports against + # the running interpreter, so an unguarded `import fcntl` -- the exact bug + # that made PR #5 necessary -- becomes an import-error here instead of a + # runtime surprise on a user's machine. The msvcrt half of compat.py is + # likewise only *real* on Windows, so attribute checks see it bound. + runs-on: windows-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Install + run: | + python -m pip install --upgrade pip + python -m pip install -e .[dev] + python -m pip install "pylint==4.0.9" + - name: Lint (errors only) + run: python -m pylint -E --score=n pyvdisk tests diff --git a/pyvdisk/compat.py b/pyvdisk/compat.py index a096313..06829e9 100644 --- a/pyvdisk/compat.py +++ b/pyvdisk/compat.py @@ -16,8 +16,12 @@ Why a lock on the emulated positional IO: ``os.pread``/``os.pwrite`` are atomic, ``lseek`` + ``read`` is not. Two threads sharing one handle could interleave and read the wrong block, so the emulation serialises through a -module-level lock. That costs throughput on Windows only, and only while a -block IO is in flight; it is not a global lock over the whole disk. +**per-descriptor** lock: one handle queues behind itself, different handles +-- and therefore different disks -- never wait on each other. Stale entries +after a descriptor is closed are harmless: an fd number reused by a new file +at worst shares a lock with its predecessor and picks up spurious +contention, never incorrectness. That costs throughput on Windows only, and +only while a block IO on the *same* handle is in flight. The Windows byte-range lock covers ``[0, LOCK_BYTES)`` -- the same "whole file" scope ``flock`` gives us, because every handle in this package locks from @@ -62,7 +66,23 @@ #: ``pread``/``pwrite``. _HAS_POSITIONAL = hasattr(os, "pread") and hasattr(os, "pwrite") -_POSITIONAL_LOCK = threading.RLock() +#: One lock per descriptor, created on demand (issue #8: the original +#: module-level lock serialised every open disk behind one RLock). Entries +#: are intentionally not pruned: an fd number reused after close shares its +#: predecessor's lock, which can only add contention, never break correctness, +#: and pruning would need a close hook this layer does not own. +_PIO_LOCKS: dict = {} +_PIO_GUARD = threading.Lock() + + +def _pio_lock(fd: int) -> threading.RLock: + """The emulation lock for this descriptor -- same fd, same lock, always.""" + with _PIO_GUARD: + lock = _PIO_LOCKS.get(fd) + if lock is None: + lock = threading.RLock() + _PIO_LOCKS[fd] = lock + return lock #: Errnos that mean "the lock is held by someone else" (or "retry the syscall"): #: ``flock(LOCK_NB)`` raises EAGAIN/EWOULDBLOCK (and EACCES on some systems), @@ -134,8 +154,10 @@ def pread(fd: int, length: int, offset: int) -> bytes: Returns fewer bytes only at end of file, like ``os.pread``. """ if _HAS_POSITIONAL and implementation() == "posix": - return os.pread(fd, length, offset) - with _POSITIONAL_LOCK: + # The guard above is invisible to pylint on Windows, where os has no + # pread/pwrite to infer from -- hence the scoped disable. + return os.pread(fd, length, offset) # pylint: disable=no-member + with _pio_lock(fd): os.lseek(fd, offset, os.SEEK_SET) return os.read(fd, length) @@ -146,8 +168,8 @@ def pwrite(fd: int, data: bytes, offset: int) -> int: Returns the number of bytes written, like ``os.pwrite``. """ if _HAS_POSITIONAL and implementation() == "posix": - return os.pwrite(fd, data, offset) - with _POSITIONAL_LOCK: + return os.pwrite(fd, data, offset) # pylint: disable=no-member + with _pio_lock(fd): os.lseek(fd, offset, os.SEEK_SET) return os.write(fd, data) diff --git a/pyvdisk/sandbox.py b/pyvdisk/sandbox.py index c2ace50..ac17eac 100644 --- a/pyvdisk/sandbox.py +++ b/pyvdisk/sandbox.py @@ -77,15 +77,40 @@ class _SandboxFS: rules, and ``fs.listdir(sandbox, "/")`` would hand the model the very paths ``list_files`` refuses to show it. Paths are translated here and the calls are forwarded, so the capability-scoped namespace underneath still applies. + + The door carries exactly the verbs defined below (plus the path-less + diagnostics the scoped view itself provides, such as ``df`` and ``fsck``). + The VScript stdlib advertises the full disk's fs API -- ``fs.symlink``, + ``fs.chmod`` and friends -- so an agent script that reaches for one of the + verbs the sandbox does not carry must be told so *in words*, not with a + bare AttributeError from the missing scoped method (#9). """ + #: The verbs this door actually carries, for the refusal message. + VERBS = ( + "append_file", "df", "du", "exists", "fsck", "isdir", "isfile", + "listdir", "listdir_with_stat", "mkdir", "makedirs", "open", + "read_file", "remove", "rename", "rmtree", "stat", "statfs", + "walk", "write_file", + ) + def __init__(self, target: Any, translate: Any): self._target = target self._translate = translate def __getattr__(self, name: str): - """Everything without a path (``df``, ``fsck``, ...) is passed straight on.""" - return getattr(object.__getattribute__(self, "_target"), name) + """Path-less diagnostics (``df``, ``fsck``, ``statfs``) pass straight on. + + Anything else the scoped view does not implement is refused with the + carried-verb list, so a script (and the model reading the result) + learns what the sandbox *does* offer instead of guessing. + """ + try: + return getattr(object.__getattribute__(self, "_target"), name) + except AttributeError: + raise SandboxError( + f"fs.{name} 不在沙箱内可用；可用动词: {', '.join(self.VERBS)}" + ) from None @staticmethod def _keep(top: str, name: str) -> bool: @@ -134,8 +159,9 @@ def isdir(self, path: str) -> bool: target = self._translate(path) return False if _hidden(target) else self._target.isdir(target) - def stat(self, path: str) -> Any: - return self._forward("stat", path) + def stat(self, path: str, *args: Any, **kwargs: Any) -> Any: + # follow= passes through: stdlib fs_chmod/fs_chown stat with follow. + return self._forward("stat", path, *args, **kwargs) def read_file(self, path: str) -> bytes: return self._forward("read_file", path) @@ -161,17 +187,11 @@ def rmtree(self, path: str, *args: Any, **kwargs: Any) -> Any: def rename(self, source: str, destination: str, *args: Any, **kwargs: Any) -> Any: return self._target.rename(self._translate(source), self._translate(destination), *args, **kwargs) - def move(self, source: str, destination: str, *args: Any, **kwargs: Any) -> Any: - return self._target.move(self._translate(source), self._translate(destination), *args, **kwargs) - - def copy(self, source: str, destination: str, *args: Any, **kwargs: Any) -> Any: - return self._target.copy(self._translate(source), self._translate(destination), *args, **kwargs) - - def link(self, source: str, destination: str, *args: Any, **kwargs: Any) -> Any: - return self._target.link(self._translate(source), self._translate(destination), *args, **kwargs) - - def symlink(self, source: str, destination: str, *args: Any, **kwargs: Any) -> Any: - return self._target.symlink(self._translate(source), self._translate(destination), *args, **kwargs) + # move/copy/link/symlink/readlink/truncate/chmod/chown/utime are deliberately + # NOT carried: the scoped view implements none of them, and the Python tool + # door does not offer them either. A script that calls one now gets the + # refusal from __getattr__ above, with the carried-verb list, instead of a + # bare AttributeError that reads like a bug in the sandbox (#9). def open(self, path: str, *args: Any, **kwargs: Any) -> Any: return self._forward("open", path, *args, **kwargs) @@ -179,21 +199,6 @@ def open(self, path: str, *args: Any, **kwargs: Any) -> Any: def du(self, path: str = "/") -> Any: return self._forward("du", path) - def readlink(self, path: str) -> Any: - return self._forward("readlink", path) - - def truncate(self, path: str, *args: Any, **kwargs: Any) -> Any: - return self._forward("truncate", path, *args, **kwargs) - - def chmod(self, path: str, *args: Any, **kwargs: Any) -> Any: - return self._forward("chmod", path, *args, **kwargs) - - def chown(self, path: str, *args: Any, **kwargs: Any) -> Any: - return self._forward("chown", path, *args, **kwargs) - - def utime(self, path: str, *args: Any, **kwargs: Any) -> Any: - return self._forward("utime", path, *args, **kwargs) - class SandboxError(Exception): """The sandbox refused an operation before it reached the disk.""" diff --git a/tests/test_agent_sandbox.py b/tests/test_agent_sandbox.py index dd957cb..48a9392 100644 --- a/tests/test_agent_sandbox.py +++ b/tests/test_agent_sandbox.py @@ -196,6 +196,24 @@ def test_scripts_cannot_touch_container_bookkeeping(box): assert not result.ok and "bookkeeping" in result.content, source +def test_fs_verbs_outside_the_sandbox_door_refuse_with_guidance(box): + """The stdlib advertises the full disk's fs API; the sandbox carries a subset. + + A script reaching for a verb the door does not carry (fs.symlink, fs.chmod, + ...) used to die with a bare AttributeError from the missing scoped method. + Now it is refused in words, and the words list what IS available (#9). + """ + # symlink refuses immediately; chmod/truncate first stat/read the target, + # so each source creates its file before reaching the refusal. + for source in ('import std.fs as fs; fs.symlink(sandbox, "/a", "/b");', + 'import std.fs as fs; fs.write(sandbox, "/a.txt", "x"); fs.chmod(sandbox, "/a.txt", 420);', + 'import std.fs as fs; fs.write(sandbox, "/b.txt", "x"); fs.truncate(sandbox, "/b.txt", 0);'): + result = box.call("run_script", {"source": source}) + assert not result.ok, source + assert "不在沙箱内可用" in result.content, source + assert "write_file" in result.content and "read_file" in result.content, source + + def test_the_script_and_python_doors_share_one_workspace(box): """A file written by a script is an ordinary file for every other verb.""" box.dispatch("run_script", {"source": 'import std.fs as fs; fs.write(sandbox, "/shared.txt", "from script");'}) @@ -217,8 +235,11 @@ def test_host_roots_are_opt_in(tmp_path): allowed.mkdir() (allowed / "input.txt").write_text("from the host", encoding="utf-8") with AgentSandbox.create(str(tmp_path / "host.vdisk"), host_read_roots=[str(allowed)]) as box: + # as_posix(): backslashes cannot appear in a plain VScript string + # (the lexer refuses \U etc.), the same rule PR #5 taught the host + # tests; the policy layer accepts the mixed separators on Windows. result = box.call("run_script", { - "source": f'import std.host as host; print(host.read("{allowed}/input.txt"));', + "source": f'import std.host as host; print(host.read("{allowed.as_posix()}/input.txt"));', }) assert result.ok and "from the host" in result.content diff --git a/tests/test_compat.py b/tests/test_compat.py index b20484b..91c6057 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -19,6 +19,7 @@ import os import re import sys +import threading import time from pathlib import Path @@ -98,8 +99,9 @@ def test_uid_and_gid_are_integers(): assert isinstance(compat.uid(), int) assert isinstance(compat.gid(), int) if os.name == "posix": - assert compat.uid() == os.getuid() - assert compat.gid() == os.getgid() + # pylint on Windows cannot see this guard -- os has no getuid there. + assert compat.uid() == os.getuid() # pylint: disable=no-member + assert compat.gid() == os.getgid() # pylint: disable=no-member else: assert (compat.uid(), compat.gid()) == (0, 0) @@ -232,6 +234,48 @@ def test_emulated_positional_io_matches_the_native_one(tmp_path, windows_emulati os.close(fd) +def test_positional_io_lock_is_per_descriptor(windows_emulation): + """Same fd shares one lock; different fds get different locks (#8). + + The pre-#8 module-level lock returned the same RLock for every handle, + which serialised separate disks against each other on Windows. The + assertion only needs descriptors, not files -- a pipe pair provides them. + """ + r, w = os.pipe() + try: + assert compat._pio_lock(w) is compat._pio_lock(w), "same fd, same lock" + assert compat._pio_lock(w) is not compat._pio_lock(r), "different fds must not share a lock" + finally: + os.close(r) + os.close(w) + + +def test_pio_on_one_descriptor_does_not_block_another(tmp_path, windows_emulation): + """While handle A is mid-pwrite, handle B must still make progress (#8). + + Regression for the module-level lock: under it, the worker thread would + wait for A's lock and never finish. This holds a lock on one descriptor + and proves IO on another descriptor completes regardless. The descriptors + must be seekable files: the emulated pwrite is lseek + write, and pipes + would fail with ESPIPE regardless of locking. + """ + a = os.open(str(tmp_path / "a.bin"), os.O_CREAT | os.O_RDWR) + b = os.open(str(tmp_path / "b.bin"), os.O_CREAT | os.O_RDWR) + done = [] + worker = threading.Thread( + target=lambda: (compat.pwrite(b, b"ok", 0), done.append(True)) + ) + with compat._pio_lock(a): # simulate disk A with a block IO in flight + worker.start() + worker.join(timeout=5) + try: + assert done == [1], "IO on descriptor b must not wait for descriptor a's lock" + assert compat.pread(b, 2, 0) == b"ok" + finally: + os.close(a) + os.close(b) + + def test_emulated_locking_contends_and_times_out(tmp_path, windows_emulation): first, second = _lock_file(tmp_path), _lock_file(tmp_path) try: