From de93ebd044d0b00a8647a9f47f86939a6b18b9aa Mon Sep 17 00:00:00 2001 From: Mihir Srivastava Date: Thu, 22 Oct 2026 08:59:23 -0700 Subject: [PATCH] Handle Windows EINVAL in Git batch fallback --- .../research/windows-git-batch-pipe-errors.md | 130 ++++++++++++++ scripts/research_package.py | 17 +- tests/test_research_git_pipe_errors.py | 159 ++++++++++++++++++ 3 files changed, 303 insertions(+), 3 deletions(-) create mode 100644 docs/research/windows-git-batch-pipe-errors.md create mode 100644 tests/test_research_git_pipe_errors.py diff --git a/docs/research/windows-git-batch-pipe-errors.md b/docs/research/windows-git-batch-pipe-errors.md new file mode 100644 index 0000000000..c6ecaae81f --- /dev/null +++ b/docs/research/windows-git-batch-pipe-errors.md @@ -0,0 +1,130 @@ +# Windows closed-stdin errors in source-pin verification + +The Git blob reader now recognizes Windows `EINVAL` from a closed child stdin +at both request writing and cleanup. This lets the existing older-Git fallback +run when its unsupported-option probe exits before the first request. The retry +still requires the first NUL-mode request, an absent response and exit status +129. Other OS errors, non-129 exits and failures after a valid response still +reject; a failing legacy reader cannot start a third process. + +This is a shared-tool portability repair. It changes no source hashes, frozen +research packages, supplier claims or selected frontier. + +## Reproduced failure and repair + +Base: canonical main `b58ad9ef4d4e861b0eaa5dba41742bdc072dca51`. +The base reader's Git blob is `ad2f94e3d38c3167feb66b07cbe6c52192284d39`; +the unchanged compatibility tests' blob is +`4e6a4c66a4945a718446c34db387a013e98e780e`. + +On native Windows with CPython 3.12.0 and Git 2.47.1.windows.1: + +```sh +python -W error -B tests/test_research_git_compatibility.py -v +``` + +The original `test_option_rejection_before_first_write_handles_broken_pipe` +raises `OSError: [Errno 22] Invalid argument` at `process.stdin.flush()`, then +again at `process.stdin.close()`. The module reports eight passes, one error +and one existing CR/LF-path skip. Its fixture waits for an actual process to +exit 129, then attempts the write; real Git serves the fallback objects. + +The reader caught only `BrokenPipeError`. CPython itself handles this Windows +closed-pipe behavior in +[`Popen._stdin_write`](https://github.com/python/cpython/blob/v3.12.0/Lib/subprocess.py). +The new classifier accepts `BrokenPipeError` everywhere and `errno.EINVAL` +only when `os.name == "nt"`. It classifies a pipe error, not a successful +verification or permission to retry. The existing exit/response gate, +one-pass input iterator, exact object and byte checks, and cleanup remain. + +## Verification + +The six new tests in `tests/test_research_git_pipe_errors.py` inject errors at +write, flush and close around real exited subprocesses. Actual Git supplies +every successful fallback payload. They check binary/empty/duplicate objects, +independent blob IDs, iterator consumption, process counts, closed streams and +reaped children. Controls cover EPIPE on both platform branches, POSIX EINVAL, +unrelated EIO, exit statuses 0/7, a failure after a valid blob and a failed +legacy reader. Only the module's platform view is injected; the host's OS and +subprocess module retain their actual behavior. + +The final six-test suite against an exact byte copy of the base reader fails with +eight error subcases. With the repair they pass normally and under `-O`. + +| Environment | Modules | Result in each of normal and optimized runs | +|---|---|---| +| Native Windows, CPython 3.12.0, Git 2.47.1.windows.1 | Six modules below | 51 passed, one existing CR/LF-path skip | +| Ubuntu under WSL, CPython 3.12.3, Git 2.43.0 | First three modules below | 23 passed, no skips | + +Every run used `-W error -B`. From the repository root: + +```sh +python3 -W error -B tests/test_research_git_batch.py -v +python3 -W error -B tests/test_research_git_compatibility.py -v +python3 -W error -B tests/test_research_git_pipe_errors.py -v +python3 -W error -B tests/test_research_git_replacements.py -v +python3 -W error -B tests/test_research_commit_bytes.py -v +python3 -W error -B tests/test_research_source_history.py -v +``` + +Repeat with `-O` to reproduce the optimized checks. The latter three modules +retain replacement-ref, source-history and committed-byte rejection coverage. +`git diff --check` also passes. These are local results; hosted CI is separate. + +The unchanged registered `research/git-batch-compatibility` hook passes on +Ubuntu under WSL: metadata/source pins and all 38 currently discovered +Git-reader tests, including the new tests. Reproduce with: + +```sh +python3 -W error -B scripts/research_package.py --package research/git-batch-compatibility +``` + +The repository planner selects all 2,480 registered packages because this is +a shared runner change. That selection was checked; those scientific replays +were not run locally. The planner's coverage policy is unchanged. + +An old Git binary was not installed for this repair. Unsupported-option exit +129 is simulated, as in the predecessor tests; native Windows pipe behavior +and successful Git blob reads are real. The original CR/LF fixture remains +skipped on Windows and passes under Linux. An initial broader Windows onboarding +run encountered unrelated existing fixture errors and was stopped; no complete +onboarding or selected scientific replay pass is claimed. The sparse selected +baseline also stopped on an absent input, before verifying its closure. + +## Scope, overlap and credit + +This follows the `portable-replay` card from verifier-engineering picker seed +`f65c1c7a783342c01e342be2f39f11a8`, priority revision 7. Scope is **changed** +from canonical regenerated-payload comparison to an observed Windows failure +in immutable-source verification. The bounded question is whether the existing +compatibility retry can recover without broadening its admission conditions. +The stop rule is that boundary's repair and regression evidence, not a wider +runner rewrite. Coordination is in +[Discussion #4681](https://github.com/CrocSwap/integer-mult-bounds/discussions/4681). + +The canonical main refresh before submission remained at the base above. +A refreshed overlap scan covered 450 open PRs and 3,612 Discussion records +with complete remote pagination. The local follow-up searched all 5,860 tracked +in-scope notes (package README/TASK files and the PR ledger). Exact error/path +searches found this claim and no competing repair. Arbitrary diffs, closed remote PRs and unpublished +work are not covered. The adjacent #4579/#4679 work repairs exporter fixtures; +the active isolated-runner Windows lifetime task in #1472 owns a different +launcher. None is claimed here. + +Nick Petalas's [PR2752](https://github.com/CrocSwap/integer-mult-bounds/pull/2752), +head `73406fd73ebaf8361b51674fad5e0b02a26db075`, supplies the fallback and its +original tests. Its retained package explicitly left Windows untested. +Douglas Colkitt supplied the runner's contracts, Arham Tahir the NUL pin +batching, and Woklerx the streamed-reader/cleanup lineage. Maxime Fleury's +reproducibility distinction and the original P104/Abhinav Ramachandran reports +retain their predecessor credit. Prepared for Mihir Srivastava with substantial +OpenAI Codex assistance, one agent. Apache-2.0. + +## Open next questions + +An actual older Git for Windows run would complement the controlled rejection +test. First put that binary on the PATH of a disposable local fixture and run +the three Git-reader modules; the missing prerequisite here is that binary. +This repair does not address synchronous pipe-read deadlines or establish +compatibility with every Git/Python version. A separate reported failure is +needed before expanding the work. diff --git a/scripts/research_package.py b/scripts/research_package.py index ad2f94e3d3..be41785cf8 100644 --- a/scripts/research_package.py +++ b/scripts/research_package.py @@ -4,6 +4,7 @@ Douglas Colkitt, with OpenAI Codex assistance. Apache-2.0. """ import argparse +import errno from contextlib import closing from itertools import chain import hashlib @@ -139,6 +140,13 @@ class _UnsupportedNulBatch(Exception): """Git rejected the initial NUL-mode request before yielding any source.""" +def _closed_stdin_error(error): + # Windows can report EINVAL instead of EPIPE when a child closes stdin. + # This classifies the pipe error only; the retry still requires exit 129. + return (isinstance(error, BrokenPipeError) + or (os.name == "nt" and error.errno == errno.EINVAL)) + + def _git_blob_batch(root, specs, nul): argv = ["git", "cat-file", "--batch"] + (["-z"] if nul else []) # Usage diagnostics from the unsupported-option probe are expected. Avoid @@ -156,7 +164,9 @@ def _git_blob_batch(root, specs, nul): try: process.stdin.write(request.encode("utf-8") + (b"\0" if nul else b"\n")) process.stdin.flush() - except BrokenPipeError: + except OSError as exc: + if not _closed_stdin_error(exc): + raise header = b"" else: header = process.stdout.readline() @@ -177,8 +187,9 @@ def _git_blob_batch(root, specs, nul): # Option rejection can close stdin before our first write/flush. try: process.stdin.close() - except BrokenPipeError: - pass + except OSError as exc: + if not _closed_stdin_error(exc): + raise process.stdout.close() if process.poll() is None: try: diff --git a/tests/test_research_git_pipe_errors.py b/tests/test_research_git_pipe_errors.py new file mode 100644 index 0000000000..8f1709e1fe --- /dev/null +++ b/tests/test_research_git_pipe_errors.py @@ -0,0 +1,159 @@ +"""Closed-stdin errors must retain the exact Git compatibility retry gate. + +Builds on Nick Petalas's PR2752 and Douglas Colkitt's package runner. +Mihir Srivastava, with substantial OpenAI Codex assistance. Apache-2.0. +""" +from contextlib import contextmanager +import errno +import hashlib +import importlib.util +import io +import os +from pathlib import Path +import subprocess +import sys +import tempfile +from types import SimpleNamespace +import unittest +from unittest.mock import patch + +ROOT = Path(__file__).resolve().parents[1] +SPEC = importlib.util.spec_from_file_location("git_pipe_package", ROOT / "scripts/research_package.py") +PACKAGE = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(PACKAGE) + + +class ErrorPipe(io.BytesIO): + """Inject one specified OS error per operation, while retaining close state.""" + def __init__(self, operations, number, request=1): + super().__init__() + self.operations = set(operations) + self.number = number + self.request = request + self.writes = 0 + + def fail(self, operation): + if operation in self.operations and self.writes >= self.request: + self.operations.remove(operation) + raise OSError(self.number, "injected stdin " + operation) + + def write(self, data): + self.writes += 1 + self.fail("write") + return super().write(data) + + def flush(self): + self.fail("flush") + return super().flush() + + def close(self): + super().close() + self.fail("close") + + +class GitPipeErrorTests(unittest.TestCase): + def setUp(self): + temporary = tempfile.TemporaryDirectory() + self.addCleanup(temporary.cleanup) + self.root = Path(temporary.name) + PACKAGE.git(self.root, "init", "-q", "--object-format=sha1") + self.payloads = [b"", b"\x00\xff\r\n" * 4097, b""] + self.objects = [] + for index, data in enumerate(self.payloads): + name = "input-" + str(index) + (self.root / name).write_bytes(data) + oid = PACKAGE.git(self.root, "hash-object", "-w", "--", name).decode().strip() + expected = hashlib.sha1(b"blob " + str(len(data)).encode() + b"\0" + data).hexdigest() + self.assertEqual(oid, expected) + self.objects.append(oid) + + @contextmanager + def rejected_probe(self, operations, number=errno.EINVAL, status=129, + platform="nt", response=b"", request=1, fail_legacy=False): + """Model the probe only; real Git supplies every successful fallback blob.""" + real_popen = subprocess.Popen + calls, children, pipes = [], [], [] + + def start(argv, **kwargs): + calls.append(argv) + probe = (argv == ["git", "cat-file", "--batch", "-z"] + or (fail_legacy and argv == ["git", "cat-file", "--batch"])) + if probe: + code = f"import sys; sys.stdout.buffer.write({response!r}); sys.exit({status})" + child = real_popen([sys.executable, "-c", code], **kwargs) + child.wait(timeout=10) + child.stdin.close() + child.stdin = ErrorPipe(operations, number, request) + pipes.append(child.stdin) + else: + child = real_popen(argv, **kwargs) + children.append(child) + return child + + try: + with (patch.object(PACKAGE, "os", SimpleNamespace(name=platform, environ=os.environ)), + patch.object(PACKAGE.subprocess, "Popen", side_effect=start)): + yield calls, pipes + finally: + self.assertTrue(all(child.poll() is not None for child in children)) + self.assertTrue(all(child.stdin.closed and child.stdout.closed for child in children)) + + def test_windows_einval_retries_only_the_probe_and_preserves_every_blob(self): + for operations in (("write",), ("flush",), ("close",), ("flush", "close")): + with self.subTest(operations=operations): + consumed = [] + def requests(): + for oid in self.objects: + consumed.append(oid) + yield oid + with self.rejected_probe(operations) as (calls, _): + actual = list(PACKAGE.git_blobs(self.root, requests())) + self.assertEqual(actual, list(zip(self.objects, self.payloads))) + self.assertEqual(consumed, self.objects) + self.assertEqual(calls, [["git", "cat-file", "--batch", "-z"], + ["git", "cat-file", "--batch"]]) + + def test_broken_pipe_retains_the_same_fallback_on_both_platforms(self): + for platform in ("nt", "posix"): + with self.subTest(platform=platform), self.rejected_probe( + ("write", "close"), number=errno.EPIPE, platform=platform): + self.assertEqual(list(PACKAGE.git_blobs(self.root, self.objects)), + list(zip(self.objects, self.payloads))) + + def test_unrelated_errors_and_posix_einval_propagate_without_retry(self): + for platform, number in (("nt", errno.EIO), ("posix", errno.EINVAL)): + for operation in ("write", "flush", "close"): + with self.subTest(platform=platform, operation=operation): + with self.rejected_probe((operation,), number=number, platform=platform) as (calls, _): + with self.assertRaises(OSError) as caught: + list(PACKAGE.git_blobs(self.root, self.objects)) + self.assertEqual(caught.exception.errno, number) + self.assertEqual(len(calls), 1) + + def test_windows_einval_with_non_usage_exit_does_not_retry(self): + for status in (0, 7): + with self.subTest(status=status), self.rejected_probe( + ("flush", "close"), status=status) as (calls, _): + with self.assertRaisesRegex(ValueError, "missing or non-blob"): + list(PACKAGE.git_blobs(self.root, self.objects)) + self.assertEqual(len(calls), 1) + + def test_windows_einval_after_a_valid_response_does_not_retry(self): + response = self.objects[0].encode() + b" blob 0\n\n" + with self.rejected_probe(("write", "close"), response=response, request=2) as (calls, _): + rows = PACKAGE.git_blobs(self.root, self.objects) + self.assertEqual(next(rows), (self.objects[0], b"")) + with self.assertRaisesRegex(ValueError, "missing or non-blob"): + next(rows) + self.assertEqual(len(calls), 1) + + def test_windows_einval_in_the_legacy_reader_does_not_start_a_third_process(self): + with self.rejected_probe(("flush", "close"), fail_legacy=True) as (calls, _): + with self.assertRaisesRegex(ValueError, "missing or non-blob"): + list(PACKAGE.git_blobs(self.root, self.objects)) + self.assertEqual(calls, [["git", "cat-file", "--batch", "-z"], + ["git", "cat-file", "--batch"]]) + + +if __name__ == "__main__": + unittest.main()