From 9b1d378d2fe8051f9138b908239071d09baf6a33 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 05:11:54 +0200 Subject: [PATCH] fix: never truncate a copy another run renames On Windows a verified partial copy is closed before its rename. A second run that claimed it in that moment truncated it and the first rename failed with a sharing violation, reported as an unwritable folder. A complete partial is now left whole there, the rename waits for a sharing violation to clear, and a sharing violation no longer reads as a refused folder. --- install.ps1 | 2 +- install.py | 90 +++++++++++++++++++++++++++++++++++++++-- install.sh | 2 +- tests/test_install.py | 94 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 183 insertions(+), 5 deletions(-) diff --git a/install.ps1 b/install.ps1 index fa2ce047..2165680f 100644 --- a/install.ps1 +++ b/install.ps1 @@ -12,7 +12,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "973d64627af05b239555ab5a83a8902747b7a742bc1176f1b994cf7ac74774f9" + $defaultInstallSha256 = "007fffece97716aa9e1ed0606a8453f2da8a1262711aa68ca910673956b2b341" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 4e18bb17..406d7f8d 100644 --- a/install.py +++ b/install.py @@ -2576,6 +2576,7 @@ def _download_one( resumable = expected_size >= RESUME_FROM_SIZE reason = "download failed" refused = False + full_partial = False for attempt in range(1, MAX_RETRIES + 1): tmp_path: Path | None = None counted = 0 @@ -2594,7 +2595,17 @@ def _download_one( tmp_path = None else: offset = os.fstat(partial.fileno()).st_size - if offset >= expected_size: + if offset >= expected_size and _NO_RENAME_WHILE_OPEN: + # The run that verified this copy releases its lock + # to rename it: truncating it there destroyed that + # copy and failed the rename. It is left alone and + # this run writes a private file. + partial.close() + partial = None + tmp_path = None + offset = 0 + full_partial = True + elif offset >= expected_size: partial.truncate(0) offset = 0 request = urllib.request.Request(url) @@ -2665,11 +2676,15 @@ def _download_one( # written with sudo has to stay readable by the emulator's user. os.chmod(tmp_path, (0o777 if f.get("executable") else 0o666) & ~_UMASK) _clear_read_only(dest) - if partial is not None and os.name == "nt": + if partial is not None and _NO_RENAME_WHILE_OPEN: # Windows renames no open file. Elsewhere the lock is held # until the copy has its final name. partial.close() - os.replace(tmp_path, dest) + _replace_released(tmp_path, dest, expected) + else: + os.replace(tmp_path, dest) + if full_partial: + _drop_full_partial(dest, expected_size) return f["dest"], True, "" except _Interrupted: @@ -2739,6 +2754,71 @@ def _download_one( return f["dest"], False, reason +# Windows renames no file another handle holds open, and Python opens files +# without FILE_SHARE_DELETE: a locked partial copy is closed before its rename. +_NO_RENAME_WHILE_OPEN = os.name == "nt" +# Seconds a closed partial copy waits for a sharing violation to clear: a +# second run closing it again, or a scanner reading the new file. +RELEASE_WAIT = 30.0 +_SHARING_VIOLATIONS = (32, 33) + + +def _replace_released(source: Path, dest: Path, expected: dict[str, str]) -> None: + """Rename a verified copy whose lock was just released. + + Another run may open the copy in that moment, which refuses the rename + with a sharing violation until it lets go. When the copy is gone, that + run placed the same file and removed it: the destination is checked + against the manifest's hashes before it counts as installed. + """ + deadline = time.monotonic() + RELEASE_WAIT + while True: + try: + os.replace(source, dest) + return + except FileNotFoundError: + if expected and dest.is_file() and _digest_file(dest, tuple(expected)) == expected: + return + raise + except PermissionError as exc: + if (getattr(exc, "winerror", None) not in _SHARING_VIOLATIONS + or time.monotonic() >= deadline): + raise + time.sleep(0.2) + + +def _drop_full_partial(dest: Path, expected_size: int) -> None: + """Remove a complete partial copy this run left alone. + + A copy at full size that nothing holds is what a run stopped between its + check and its rename leaves behind. The destination now holds the + verified file, so the copy is only clutter. One another run holds is + left to it. + """ + path = dest.parent / f".{dest.name}.part" + if not path.is_file(): + return + try: + handle = _claim_partial(path) + except OSError: + return + if handle is None: + return + with handle: + complete = os.fstat(handle.fileno()).st_size >= expected_size + if complete: + try: + path.unlink() + except OSError: + pass # another run opened it meanwhile and finishes with it + else: + try: + if path.stat().st_size == 0: + path.unlink() + except OSError: + pass # created by the claim and taken since by another run + + def _partial_size(path: Path) -> int: """Bytes already held by a partial copy. Anything but a plain file is dropped.""" try: @@ -2823,6 +2903,10 @@ NOT_WRITABLE = "folder not writable" def _write_refused(exc: OSError) -> bool: + # A sharing violation is a file another process holds open, which a + # later attempt can find released: no refusal of the folder. + if getattr(exc, "winerror", None) in _SHARING_VIOLATIONS: + return False return isinstance(exc, PermissionError) or exc.errno == errno.EROFS diff --git a/install.sh b/install.sh index fa86e185..00463b67 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ esac TEMP_INSTALLER="" TEMP_DIRECTORY="" DEFAULT_INSTALL_URL="https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -DEFAULT_INSTALL_SHA256="973d64627af05b239555ab5a83a8902747b7a742bc1176f1b994cf7ac74774f9" +DEFAULT_INSTALL_SHA256="007fffece97716aa9e1ed0606a8453f2da8a1262711aa68ca910673956b2b341" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/tests/test_install.py b/tests/test_install.py index 3ae1f221..a70ef8da 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -3586,5 +3586,99 @@ class DestAloneFollowsWhatIsFoundHere(unittest.TestCase): self.assertIn("Which platform's files go to /media/card?", out) +class _FakeResponse: + """What urlopen returns, serving one payload from its start.""" + + def __init__(self, payload: bytes) -> None: + self.status = 200 + self.headers = {"Content-Length": str(len(payload))} + self._body = io.BytesIO(payload) + + def __enter__(self) -> _FakeResponse: + return self + + def __exit__(self, *_exc: object) -> None: + return None + + def read(self, size: int = -1) -> bytes: + return self._body.read(size) + + +def _sharing_violation() -> PermissionError: + exc = PermissionError(13, "The process cannot access the file") + exc.winerror = 32 + return exc + + +class ACompletePartialIsHandedOffOnWindows(unittest.TestCase): + """Windows renames no open file, so the run that verified a large copy + released its lock before the rename. A second run claimed the copy in + that moment, found it complete and truncated it, and the first run's + rename failed with a sharing violation reported as "folder not + writable".""" + + PAYLOAD = bytes(range(256)) * 200 + ENTRY = { + "dest": "big.bin", "repo_path": "bios/big.bin", "size": len(PAYLOAD), + "sha256": hashlib.sha256(PAYLOAD).hexdigest(), + } + + def _run(self, tmp: Path, seen: list[int]): + part = tmp / ".big.bin.part" + + def urlopen(request, timeout=None): + seen.append(part.stat().st_size if part.exists() else -1) + return _FakeResponse(self.PAYLOAD) + + with unittest.mock.patch.object(install, "RESUME_FROM_SIZE", 1024), \ + unittest.mock.patch.object(install, "_NO_RENAME_WHILE_OPEN", True), \ + unittest.mock.patch.object(install.urllib.request, "urlopen", urlopen): + return install._download_one(dict(self.ENTRY), tmp) + + def test_the_other_runs_copy_is_left_whole(self): + tmp = Path(_tempdir(self)) + (tmp / ".big.bin.part").write_bytes(self.PAYLOAD) + seen: list[int] = [] + self.assertEqual(self._run(tmp, seen), ("big.bin", True, "")) + # The download ran while the complete copy was still whole. + self.assertEqual(seen, [len(self.PAYLOAD)]) + self.assertEqual((tmp / "big.bin").read_bytes(), self.PAYLOAD) + # Nothing held it afterwards: it was a leftover, now removed. + self.assertFalse((tmp / ".big.bin.part").exists()) + + def test_the_rename_waits_for_the_other_run_to_let_go(self): + tmp = Path(_tempdir(self)) + source = tmp / "copy" + source.write_bytes(b"x") + real_replace = os.replace + calls = [] + + def replace(src, dst): + calls.append(src) + if len(calls) < 3: + raise _sharing_violation() + return real_replace(src, dst) + + with unittest.mock.patch.object(install.os, "replace", replace), \ + unittest.mock.patch.object(install.time, "sleep"): + install._replace_released(source, tmp / "dest", {}) + self.assertEqual(len(calls), 3) + self.assertEqual((tmp / "dest").read_bytes(), b"x") + + def test_a_copy_the_other_run_placed_counts_once_verified(self): + tmp = Path(_tempdir(self)) + dest = tmp / "dest" + dest.write_bytes(self.PAYLOAD) + expected = {"sha256": self.ENTRY["sha256"]} + install._replace_released(tmp / "gone", dest, expected) + dest.write_bytes(b"other bytes") + with self.assertRaises(FileNotFoundError): + install._replace_released(tmp / "gone", dest, expected) + + def test_a_sharing_violation_is_no_refused_folder(self): + self.assertFalse(install._write_refused(_sharing_violation())) + self.assertTrue(install._write_refused(PermissionError(13, "denied"))) + + if __name__ == "__main__": unittest.main()