From 8ee41da7433374803373ba7f66d6740a89757b0f Mon Sep 17 00:00:00 2001 From: Enrico Date: Wed, 30 Sep 2026 11:10:33 +0200 Subject: [PATCH 1/2] fix(jsonl): make write_private_atomic work on Windows On Windows every write failed. os.fchmod() is Unix-only, so on Python <= 3.12 it raised AttributeError just after mkstemp() left the descriptor open; the cleanup unlink() then hit WinError 32 ("the file is being used by another process") and that secondary error replaced the real one, so users saw "cannot write target session ...: " pointing at a target file that never existed. On Python 3.13, where os.fchmod exists, writes instead died in _fsync_directory(): Windows cannot os.open() a directory. - skip os.fchmod() where it is unavailable; Windows keeps the creating user's ACLs - skip the directory fsync on Windows; NTFS journaling keeps directory entries durable without it - make the temporary-file unlink in the error path best-effort so a cleanup failure cannot mask the original error being reported Add regression tests for both failure modes. Verified on Windows with Python 3.11 and 3.13: a 371-record Codex-to-Claude transfer now completes, and write, collision refusal, and rollback behave as on POSIX. Note: some existing tests remain POSIX-specific (permission-bit assertions, pty imports, CRLF-sensitive fixture bytes) and cannot pass on Windows regardless of this change; the Ubuntu CI matrix is unaffected. --- CHANGELOG.md | 6 ++++++ src/session_migrate/jsonl.py | 13 +++++++++++-- tests/test_jsonl.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d75a84..7f349f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ here. Native format compatibility is documented separately in ## Unreleased +- Fix atomic writes on Windows in `write_private_atomic`: skip the Unix-only + `os.fchmod` call and the directory `fsync` where the platform cannot + perform them (Windows keeps the creating user's ACLs and NTFS journaling + covers directory durability), and make temporary-file cleanup best-effort + so a cleanup failure cannot mask the original error. + ## 0.11.0 - 2026-09-11 - Read root Codex rollouts written with `history_mode: "paginated"` by using diff --git a/src/session_migrate/jsonl.py b/src/session_migrate/jsonl.py index c86c932..164782b 100644 --- a/src/session_migrate/jsonl.py +++ b/src/session_migrate/jsonl.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import hashlib import json import os @@ -160,7 +161,9 @@ def write_private_atomic(path: Path, data: bytes) -> tuple[int, int]: temporary_path = Path(temporary_name) published_identity: tuple[int, int] | None = None try: - os.fchmod(descriptor, 0o600) + # os.fchmod is Unix-only; Windows keeps the creating user's ACLs. + if hasattr(os, "fchmod"): + os.fchmod(descriptor, 0o600) with os.fdopen(descriptor, "wb") as stream: stream.write(data) stream.flush() @@ -178,7 +181,9 @@ def write_private_atomic(path: Path, data: bytes) -> tuple[int, int]: _fsync_directory(path.parent) return published_identity except BaseException: - temporary_path.unlink(missing_ok=True) + # A cleanup failure must not replace the error being reported. + with contextlib.suppress(OSError): + temporary_path.unlink(missing_ok=True) if published_identity is not None: _unlink_if_same_file(path, published_identity) raise @@ -205,6 +210,10 @@ def _mkdir_private(path: Path) -> None: def _fsync_directory(path: Path) -> None: + # Windows cannot os.open() a directory, and NTFS journaling keeps + # directory entries durable without an explicit fsync. + if os.name == "nt": + return descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_DIRECTORY", 0)) try: os.fsync(descriptor) diff --git a/tests/test_jsonl.py b/tests/test_jsonl.py index 2141881..3f1ee89 100644 --- a/tests/test_jsonl.py +++ b/tests/test_jsonl.py @@ -99,3 +99,34 @@ def racing_link(source: object, target: object) -> None: write_private_atomic(path, b"migrator output\n") assert path.read_bytes() == b"racing winner" + + +def test_atomic_write_when_fchmod_is_unavailable( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Windows and WASI have no os.fchmod; writes must still succeed there. + monkeypatch.delattr(os, "fchmod", raising=False) + path = tmp_path / "session.jsonl" + + write_private_atomic(path, b"{}\n") + + assert path.read_bytes() == b"{}\n" + assert not list(tmp_path.glob(".*.tmp")) + + +def test_atomic_write_cleanup_does_not_mask_original_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + path = tmp_path / "session.jsonl" + + def failing_fchmod(descriptor: int, mode: int) -> None: + raise OSError("fchmod failed") + + def failing_unlink(target: object, **kwargs: object) -> None: + raise OSError("cleanup unlink failed") + + monkeypatch.setattr(os, "fchmod", failing_fchmod) + monkeypatch.setattr(os, "unlink", failing_unlink) + + with pytest.raises(JsonlError, match="fchmod failed"): + write_private_atomic(path, b"{}\n") From a94add69869b05bd6511cd7a00486ed0fef28521 Mon Sep 17 00:00:00 2001 From: Enrico Date: Fri, 9 Oct 2026 18:31:13 +0200 Subject: [PATCH 2/2] fix(jsonl): state Windows privacy/durability boundary and test nt fsync branch Correct the docstring, comments, and changelog: skipping os.fchmod does not make the file owner-only on Windows (it inherits the destination directory ACL), and directory-entry crash durability is left to the platform rather than guaranteed by NTFS journaling. Add a test that selects the nt branch and asserts directory os.open and os.fsync are not called. --- CHANGELOG.md | 8 +++++--- src/session_migrate/jsonl.py | 17 +++++++++++++---- tests/test_jsonl.py | 30 +++++++++++++++++++++++++++++- 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f349f6..fdaa499 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,11 @@ here. Native format compatibility is documented separately in - Fix atomic writes on Windows in `write_private_atomic`: skip the Unix-only `os.fchmod` call and the directory `fsync` where the platform cannot - perform them (Windows keeps the creating user's ACLs and NTFS journaling - covers directory durability), and make temporary-file cleanup best-effort - so a cleanup failure cannot mask the original error. + perform them, and make temporary-file cleanup best-effort so a cleanup + failure cannot mask the original error. On Windows this means file data is + flushed but directory-entry crash durability is left to the platform, and + the file inherits the destination directory's ACL rather than receiving a + POSIX mode-0600 equivalent, so confidentiality depends on that ACL. ## 0.11.0 - 2026-09-11 diff --git a/src/session_migrate/jsonl.py b/src/session_migrate/jsonl.py index 164782b..1ab7a6c 100644 --- a/src/session_migrate/jsonl.py +++ b/src/session_migrate/jsonl.py @@ -148,7 +148,13 @@ def encode_jsonl(records: Iterable[Mapping[str, Any]]) -> bytes: def write_private_atomic(path: Path, data: bytes) -> tuple[int, int]: - """Write mode-0600 bytes atomically without silently replacing a session.""" + """Write bytes atomically without silently replacing a session. + + On POSIX the file is mode 0600 and the parent directory is fsynced. On + Windows neither is enforced: file data is flushed, but the file inherits + the destination directory's ACL and directory-entry durability after a + crash is left to the platform. + """ path = Path(os.path.abspath(path.expanduser())) if os.path.lexists(path): @@ -161,7 +167,9 @@ def write_private_atomic(path: Path, data: bytes) -> tuple[int, int]: temporary_path = Path(temporary_name) published_identity: tuple[int, int] | None = None try: - # os.fchmod is Unix-only; Windows keeps the creating user's ACLs. + # os.fchmod is Unix-only. Skipping it does not make the file + # owner-only on Windows: the file inherits the ACL of the + # destination directory, so confidentiality depends on that ACL. if hasattr(os, "fchmod"): os.fchmod(descriptor, 0o600) with os.fdopen(descriptor, "wb") as stream: @@ -210,8 +218,9 @@ def _mkdir_private(path: Path) -> None: def _fsync_directory(path: Path) -> None: - # Windows cannot os.open() a directory, and NTFS journaling keeps - # directory entries durable without an explicit fsync. + # Windows cannot os.open() a directory, so no directory flush is attempted + # there. File data is still fsynced; crash durability of the new directory + # entry is left to the platform (Windows caches filesystem metadata). if os.name == "nt": return descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_DIRECTORY", 0)) diff --git a/tests/test_jsonl.py b/tests/test_jsonl.py index 3f1ee89..632e937 100644 --- a/tests/test_jsonl.py +++ b/tests/test_jsonl.py @@ -4,7 +4,13 @@ import pytest from session_migrate.errors import JsonlError -from session_migrate.jsonl import encode_jsonl, file_sha256, iter_jsonl, write_private_atomic +from session_migrate.jsonl import ( + _fsync_directory, + encode_jsonl, + file_sha256, + iter_jsonl, + write_private_atomic, +) def test_jsonl_round_trip(tmp_path: Path) -> None: @@ -114,6 +120,28 @@ def test_atomic_write_when_fchmod_is_unavailable( assert not list(tmp_path.glob(".*.tmp")) +def test_directory_fsync_is_skipped_on_windows( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + calls: list[str] = [] + + def fail_open(*args: object, **kwargs: object) -> int: + calls.append("open") + raise AssertionError("directory os.open must not be called on nt") + + def fail_fsync(descriptor: int) -> None: + calls.append("fsync") + raise AssertionError("os.fsync must not be called on nt") + + monkeypatch.setattr(os, "name", "nt") + monkeypatch.setattr(os, "open", fail_open) + monkeypatch.setattr(os, "fsync", fail_fsync) + + _fsync_directory(tmp_path) + + assert calls == [] + + def test_atomic_write_cleanup_does_not_mask_original_error( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: