Stop Win32Exception from reporting success, and keep cleanup failures inspectable

[Borega]

Win32Exception can say the operation succeeded

Win32Exception snapshots GetLastError() at construction, and MemLib constructs it wherever an API returns falsy — without checking that an error was actually set. When GetLastError() is 0, FormatMessageW cheerfully produces:

ctypes.windll.kernel32.SetLastError(0)
raise Win32Exception()
# Win32Exception: The operation completed successfully. (0x00000000)

A raised exception claiming success is unactionable: the caller cannot tell whether the operation worked. In practice consumers end up string-matching the message to survive teardown, which is what prompted this PR.

__format_message now special-cases code 0 and states what actually happened:

No error reported by Windows (GetLastError() == 0); the call failed without setting an error code (0x00000000)

Real error codes are unaffected — Win32Exception(5) still formats "Access is denied."

Two related robustness fixes in the same class:

• args was empty. RuntimeError.__init__ was never called, so error.args == (). Logging handlers, repr(), and re-raise paths that inspect args saw nothing. Now (message, code).
• Pickling re-read the last error. Added __reduce__, so a pickled exception round-trips the captured code rather than whatever the thread-local last error happens to be at unpickle time.

SharedMemory cleanup errors could only be grepped

SharedMemory.destroy() and close_shared_memory_connection() collected failures into a list[Win32Exception], then flattened them into a string and raised a bare Exception:

raise Exception(f"Caught {len(errors)} Win32Exception:\n" + "\n-> ".join(fmt_error))

The structured information was discarded at the raise site, so a caller wanting to ignore, say, ERROR_ACCESS_DENIED from an already-dead target had no option but substring matching. Both now raise SharedMemoryCleanupError, which keeps the failures on .errors and exposes .codes:

try:
    shm.destroy()
except SharedMemoryCleanupError as exc:
    if exc.codes == [ERROR_ACCESS_DENIED]:
        pass   # target already exited
    else:
        raise

The formatted message is byte-identical to before, and it subclasses Exception, so existing except Exception handlers keep working. Exported from MemLib/__init__.py.

Compatibility

Additive apart from the message for code 0, which was misinformation. Full suite passes (85 passed, 2 skipped) including 11 new tests in tests/test_win32_exception.py.

[Copilot]

🟡 Not ready to approve

The new tests hard-code localized Windows error text and use a potentially non-deterministic handle value, which can make CI and user test runs flaky across environments.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR improves error reporting and robustness around Windows API failures by making Win32Exception accurately represent “no error set” scenarios, and by preserving structured cleanup failure details for shared-memory teardown so callers can branch on error codes instead of grepping strings.

Changes:

• Special-case Win32Exception error code 0 to avoid misleading “completed successfully” messaging; also populate args and add __reduce__ to preserve the captured error across pickling.
• Introduce SharedMemoryCleanupError to raise aggregated cleanup failures while keeping individual Win32Exception instances and codes inspectable.
• Add focused tests for the new behaviors and export SharedMemoryCleanupError from the package API.

File summaries

File	Description
tests/test_win32_exception.py	Adds tests covering the new Win32Exception formatting/pickling behavior and the new cleanup aggregation exception.
MemLib/windows.py	Updates Win32Exception to avoid “success” messages on code 0 and to preserve captured state via args and pickling support.
MemLib/SharedMemory.py	Adds SharedMemoryCleanupError and switches shared-memory cleanup paths to raise it with structured errors.
MemLib/init.py	Exports SharedMemoryCleanupError as part of the public package surface.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Borega]

Fixed in c247f48. Now asserts ctypes.FormatError(5).strip() in error.message instead of the English string.

[Borega]

Not applying this one -- the suggested -1 would break the test. -1 is the pseudo-handle for "current process", and CloseHandle(-1) succeeds:

CloseHandle(1)  -> ret=0 err=6   (ERROR_INVALID_HANDLE)
CloseHandle(-1) -> ret=1         (success)

The test needs both cleanup steps to fail so it can exercise the aggregation path, so 0x1 is the correct value. Kept it and added a comment in c247f48 recording why, along with what UnmapViewOfFile(0x1) returns (ERROR_INVALID_ADDRESS), so nobody "fixes" it to -1 later.

[Zvendson]

I understand the intention and I know my code here is far from perfect. This is still not a real fix to the main issue. The main issue is, that we would actually need to properly handle every win32 function call here.
Instead of appending every Win32Exception (which can be indeed none), we should figure out which edge case is reporting a 0 win error that causes a successful call with a wrong result

[Borega]

You're right, and I found the specific call: NtUnmapViewOfSection in destroy() is the edge case that reports 0.

NTSTATUS routines report failure in their return value and never call SetLastError. windows.py:1097 collapses that to a bool, so the status is discarded and the Win32Exception() on SharedMemory.py:287 reads whatever the previous call left in the thread-local slot. Measured:

NtUnmapViewOfSection(bad addr):        NTSTATUS=0xC0000019  GetLastError=0
after SetLastError(1234), same call:   NTSTATUS=0xC0000019  GetLastError=1234   (stale)
CloseHandle(pseudo) succeeds:                               GetLastError=1234   (success never clears)

In destroy(), UnmapViewOfFile and CloseHandle run first. If they succeed on a fresh thread, the slot is 0 when NtUnmapViewOfSection fails → "The operation completed successfully." If they failed, step three reports their code instead — wrong but plausible, which is worse than the obviously-bogus message.

So the aggregation in this PR is exactly what you called it: not a fix. SharedMemoryCleanupError.codes faithfully collects codes that were never set, which is your "which can be indeed none".

Fixed properly in #13, via RtlNtStatusToDosError at the wrapper: 0xC0000019 → 487 "Attempt to access invalid address." regardless of what ran before. Applied to all five NTSTATUS wrappers. Three of the five new tests fail without it.

What I'd like to do with this PR: rebase it onto #13 and drop the framing that the zero-guard is the fix — with #13 merged it's just a backstop for any call site still doing it wrong. What remains is independent of the root cause:

• Win32Exception.args was empty, so except Win32Exception as e: log(*e.args) saw nothing.
• __reduce__, so pickling doesn't re-read an unrelated last error in the receiving process.
• SharedMemoryCleanupError, so cleanup failures are branchable by code instead of by grepping the message.

Happy to drop the zero-guard entirely if you'd rather not have a backstop there — it's genuinely dead code once #13 lands, unless a future call site regresses. Your call.