From b0efcce886b074967c3321cb4f579afb8cd89926 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Thu, 13 Aug 2026 14:07:37 +0900 Subject: [PATCH] cpython-ext: raise the concrete OSError subclass for I/O errors translate_io_error() built errors with `PyErr::new::(py, args)`, which stores `OSError` as the exception type and the constructor arguments as the exception value. CPython turns that pair into an instance when the error is normalized, but normalization does not replace the type, so the type stays `OSError` even though `OSError.__new__` selects a subclass from errno. CPython 3.10 and older match `except` clauses against the exception type rather than the instance, so `except FileNotFoundError` never caught an ENOENT error coming from Rust. localrepo.transaction() probes for a leftover journal with exactly that pattern: try: self.svfs.stat("journal") except FileNotFoundError: pass else: self.recover() The Windows builds embed CPython 3.10, so every locked transaction aborted there with "The system cannot find the file specified.: journal" - clone, pull and rebase all failed, while lock-free paths such as commit and goto kept working. The macOS and Linux builds use CPython 3.12+, which normalizes exceptions when they are raised, and were unaffected. vfs.lexists() relies on the same pattern. Instantiate `OSError` so the exception type is the concrete subclass on every version. Also report the Win32 error code as `OSError.winerror` on Windows. `raw_os_error()` is a Win32 error code there, not an errno, and now that errno selects the exception type a wrong errno means a wrong type: ERROR_ACCESS_DENIED (5) was reported as EIO and stayed a plain `OSError` instead of becoming `PermissionError`, and ERROR_PATH_NOT_FOUND (3) would be reported as ESRCH. CPython derives the matching errno from winerror, the same way it reports its own Windows I/O errors. The Rust tests assert on the exception type, so they fail without the fix regardless of the Python version. The Python test uses `except`, so it only reproduces the original bug on CPython 3.10 and older. --- eden/scm/lib/cpython-ext/src/io_error.rs | 128 ++++++++++++++++++++--- eden/scm/tests/test-rust-io-errors.py | 64 ++++++++++++ 2 files changed, 179 insertions(+), 13 deletions(-) create mode 100644 eden/scm/tests/test-rust-io-errors.py diff --git a/eden/scm/lib/cpython-ext/src/io_error.rs b/eden/scm/lib/cpython-ext/src/io_error.rs index 578fc682d939d..2eaf9d646deee 100644 --- a/eden/scm/lib/cpython-ext/src/io_error.rs +++ b/eden/scm/lib/cpython-ext/src/io_error.rs @@ -5,26 +5,50 @@ * LICENSE file in the root directory of this source tree. */ +use cpython::ObjectProtocol; use cpython::Python; +use cpython::PythonObject; +use cpython::ToPyObject; use cpython::exc; use util::path_error_details; pub fn translate_io_error(py: Python, e: &std::io::Error) -> cpython::PyErr { - if let Some(details) = path_error_details(e) { - let e = details.original_io_error; - let errno = io_error_errno(e); - return cpython::PyErr::new::( - py, - ( - errno, - io_error_strerror(e, errno), - details.path.display().to_string(), - ), - ); - } + let (e, filename) = match path_error_details(e) { + Some(details) => ( + details.original_io_error, + Some(details.path.display().to_string()), + ), + None => (e, None), + }; let errno = io_error_errno(e); - cpython::PyErr::new::(py, (errno, io_error_strerror(e, errno))) + let strerror = io_error_strerror(e, errno); + + // On Windows `raw_os_error` is a Win32 error code, not an errno. Report it + // as `OSError.winerror` so CPython replaces the errno above with the + // matching one, like it does for its own Windows errors in + // `PyErr_SetExcFromWindowsErrWithFilenameObjects`. Without this, + // ERROR_PATH_NOT_FOUND (3) would be taken as ESRCH, and ERROR_ACCESS_DENIED + // (5) as EIO, picking the wrong exception type below. + #[cfg(windows)] + let args = (errno, strerror, filename, e.raw_os_error()).to_py_object(py); + #[cfg(not(windows))] + let args = (errno, strerror, filename).to_py_object(py); + + // Instantiate the exception instead of using + // `PyErr::new::(py, args)`, which would keep `OSError` as + // the exception type and only build the instance when the error gets + // normalized. Normalization does not adjust the type, so the type stays + // `OSError` even though `OSError.__new__` picks a subclass based on errno. + // Interpreters matching `except` clauses against the type instead of the + // instance (CPython 3.10 and older) then fail to run `except + // FileNotFoundError` for an ENOENT error raised from Rust. + let os_error_type = py.get_type::().into_object(); + match os_error_type.call(py, args, None) { + Ok(instance) => cpython::PyErr::from_instance(py, instance), + // Building the exception failed. Report that failure instead. + Err(err) => err, + } } fn io_error_strerror(e: &std::io::Error, errno: Option) -> String { @@ -51,6 +75,11 @@ fn io_error_strerror(e: &std::io::Error, errno: Option) -> String { message } +/// The raw OS error code, or an errno inferred from the error type. +/// +/// On Windows `raw_os_error` is a Win32 error code rather than an errno. It is +/// still the code Rust puts in the error message, and it is passed to Python as +/// `OSError.winerror` so that CPython derives the real errno from it. fn io_error_errno(e: &std::io::Error) -> Option { if let Some(errno) = e.raw_os_error() { return Some(errno); @@ -84,3 +113,76 @@ fn io_error_errno(e: &std::io::Error) -> Option { _ => None, } } + +#[cfg(test)] +mod tests { + use std::io; + + use cpython::PythonObjectWithTypeObject; + + use super::*; + + #[test] + fn test_missing_file_is_a_file_not_found_error() { + let gil = Python::acquire_gil(); + let py = gil.python(); + + let io_error = std::fs::metadata("this-path-does-not-exist").unwrap_err(); + let err = translate_io_error(py, &io_error); + + // `PyErr::matches` is what an `except FileNotFoundError` clause does on + // CPython 3.10 and older: it compares against the exception type rather + // than the exception instance. Newer CPython compares against the + // instance and hides the difference when the error is actually raised, + // so this checks the exception type directly to stay meaningful on + // every Python version. + assert!(err.matches(py, py.get_type::())); + } + + #[cfg(unix)] + #[test] + fn test_errno_selects_the_exception_type() { + let gil = Python::acquire_gil(); + let py = gil.python(); + + assert_error::(py, libc::ENOENT, libc::ENOENT); + assert_error::(py, libc::EACCES, libc::EACCES); + } + + #[cfg(windows)] + #[test] + fn test_win32_error_code_selects_the_exception_type() { + let gil = Python::acquire_gil(); + let py = gil.python(); + + // Win32 error codes are not errno values. Used as errno, + // ERROR_PATH_NOT_FOUND would be ESRCH and ERROR_ACCESS_DENIED would be + // EIO. + const ERROR_FILE_NOT_FOUND: i32 = 2; + const ERROR_PATH_NOT_FOUND: i32 = 3; + const ERROR_ACCESS_DENIED: i32 = 5; + const ERROR_ALREADY_EXISTS: i32 = 183; + + assert_error::(py, ERROR_FILE_NOT_FOUND, libc::ENOENT); + assert_error::(py, ERROR_PATH_NOT_FOUND, libc::ENOENT); + assert_error::(py, ERROR_ACCESS_DENIED, libc::EACCES); + assert_error::(py, ERROR_ALREADY_EXISTS, libc::EEXIST); + } + + /// Check that an `io::Error` with `raw_os_error` becomes an exception of + /// type `T` carrying `errno`. + fn assert_error(py: Python, raw_os_error: i32, errno: i32) { + let mut err = translate_io_error(py, &io::Error::from_raw_os_error(raw_os_error)); + assert!( + err.matches(py, py.get_type::()), + "os error {raw_os_error} has an unexpected exception type" + ); + let actual = err + .instance(py) + .getattr(py, "errno") + .unwrap() + .extract::(py) + .unwrap(); + assert_eq!(actual, errno, "os error {raw_os_error} has a wrong errno"); + } +} diff --git a/eden/scm/tests/test-rust-io-errors.py b/eden/scm/tests/test-rust-io-errors.py new file mode 100644 index 0000000000000..ce2b986158132 --- /dev/null +++ b/eden/scm/tests/test-rust-io-errors.py @@ -0,0 +1,64 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This software may be used and distributed according to the terms of the +# GNU General Public License version 2. + +"""I/O errors raised by Rust are catchable as the matching OSError subclass. + +Rust reports I/O errors as `OSError` plus its constructor arguments, and +CPython only builds the concrete exception when the error is normalized. +Normalization keeps `OSError` as the exception type even though +`OSError.__new__` picks a subclass from errno, so on CPython 3.10 and older - +which matches `except` clauses against the type rather than the instance - +`except FileNotFoundError` did not catch a missing file reported by Rust. +`localrepo.transaction()` relies on exactly that, which used to make every +locked transaction (clone, pull, rebase) abort on Windows builds, where the +embedded interpreter is CPython 3.10. + +CPython 3.12 and newer normalize exceptions when they are raised, so these +tests pass with or without the fix there. The check that is meaningful on +every Python version lives in the Rust unit tests of +eden/scm/lib/cpython-ext/src/io_error.rs. +""" + +import os +import tempfile +import unittest + +import silenttestrunner +from sapling import vfs as vfsmod + + +class testrustioerrors(unittest.TestCase): + def setUp(self): + self.vfs = vfsmod.vfs(tempfile.mkdtemp(dir=os.getcwd()), audit=False) + + def teststat(self): + with self.assertRaises(FileNotFoundError): + self.vfs.stat("missing") + + def testlstat(self): + with self.assertRaises(FileNotFoundError): + self.vfs.lstat("missing") + + def testread(self): + with self.assertRaises(FileNotFoundError): + self.vfs.read("missing") + + def testlistdir(self): + with self.assertRaises(FileNotFoundError): + self.vfs.listdir("missing") + + def testunlink(self): + with self.assertRaises(FileNotFoundError): + self.vfs.unlink("missing") + + def testlexistsofmissingvfs(self): + # lexists() swallows FileNotFoundError to report a vfs whose own base + # directory is gone. + missing = vfsmod.vfs(os.path.join(self.vfs.base, "missing"), audit=False) + self.assertFalse(missing.lexists("anything")) + + +if __name__ == "__main__": + silenttestrunner.main(__name__)