Skip to content

fix(tests): retry the Windows working directory read until the child has written it - #10424

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
Oscar45397:fix/windows-working-directory-unwritten-peb
Oct 25, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
Oscar45397:fix/windows-working-directory-unwritten-peb

Conversation

@Oscar45397

Copy link
Copy Markdown
Contributor

What

windows_working_directory reads the child's current directory from its PEB (RTL_USER_PROCESS_PARAMETERS.CurrentDirectory) in a loop until it names the executable's directory. The first read comes right after Popen returns, and on this machine the child's buffer is then still all zeros while its length is already final: the read returns 76 NUL characters. Path(...).resolve() on that raises ValueError: stat: embedded null character in path, which the loop does not retry (it retries only OSError), so the test fails on its first read every time:

  File "core\relinker\relinker\tests\test_windows_working_directory.py", line 96, in check
    if seen.resolve() == case.resolve():
ValueError: stat: embedded null character in path

A read that contains a NUL character is now reported as an OSError, like a failed ReadProcessMemory, so the loop reads again until the directory is written. The later reads return ...\console\elsewhere and then ...\console\app, as before. The check itself is unchanged.

Tested

Windows 11 Home 10.0.26200, AMD Ryzen 9 5900X, WinLibs GCC 15.2.0 (15.2.0posix-14.0.0-ucrt-r7, as in CI), CMake 4.3.1, Ninja 1.13.2, Python 3.11.9, Release, -DBUILD_TESTING=ON.

  • main 2ace103 (and d1fc163 before it): windows_working_directory fails 40 of 40 runs, each with the ValueError above. The full suite on main ca7bf03: 2070 of 2074 passed; this test and the three sample order tests test(agc): read the sample order tests' depth through a sampled copy #9056 covers failed.
  • With this change on 2ace103: 40 of 40 runs pass. All 33 windows_* tests pass.
  • The raw value of the first read, printed from a copy of the test: '\x00' * 76; the next reads: C:\Users\...\anyps5-working-directory-...\console\elsewhere, then ...\console\app.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

…has written it

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@harisibrar1 harisibrar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked against main 7c2400d and the test file on main:

  • Looks right to me. The new OSError goes to the existing except OSError retry, which is bounded by the 10 s deadline in check, so a child that never writes its directory still fails with the existing assertion instead of hanging.
  • A torn read where the length and buffer are updated separately can't produce a false pass either: a truncated or zero-padded string either contains NUL or doesn't equal case, and the loop just reads again. I couldn't run this on Windows; this is from reading the loop.

Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.

@oneandonlydean oneandonlydean self-assigned this Oct 25, 2026

@roboterspinne roboterspinne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 13ebd7d merged onto upstream/main 4017310 (2026-10-25); the merge is clean. It is also clean onto the current main e05ba6f, whose newer commits don't touch this test.

Environment: Windows 11 Home 10.0.26200, Intel i9-12900H, WinLibs GCC 15.2.0 UCRT, CMake 4.3.1, Ninja 1.13.2, -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON, Release; Python 3.13.9, 3.12.14 and 3.11.6.

Results

  • Full relinker-only suite on the merged tree: 292 tests, 290 pass, long_paths skipped. The one failure is patch_advisor_unit. It fails the same way on plain main on this machine, because the temp path contains a space (#10667), and it is unrelated. windows_working_directory passes.
  • The race does not show up on this Intel machine. main's test passed 40/40 on Python 3.13 and 20/20 on 3.11, and the PR's passed 40/40.
  • To exercise the change anyway, I wrapped ReadProcessMemory so that the first read of the CurrentDirectory text comes back as zeros, then ran the console check:
    • Python 3.11.6 and 3.12.14: main fails with ValueError: stat: embedded null character in path, your trace. This branch retries and passes.
    • Python 3.13.9: both pass. On 3.13, Path.resolve() no longer runs the extra stat() (3.11's pathlib.py:1001-1005), which is the call that raises the ValueError.
    • Zeroing every read: this branch fails after the 10 s deadline with current directory is None. A buffer that never fills therefore still ends in a bounded assertion, although the message no longer shows that the reads came back as NULs.
  • A recent Windows CI run (40173750634, windows job) logs Found Python3: .../Python/3.15.1/..., so CI on that image cannot hit this. The fix matters for local runs on Python 3.12 or older.
  • check_conventions.py --base upstream/main passes with this title and body.

Not checked: an AMD machine like yours, where the all-zero window occurs.

I found nothing to change: the retry covers the window you saw, and the check itself is unchanged. For disclosure: none of my open PRs touches this test.

Written with AI assistance (Claude Code). I ran the suite, the repeated runs and the fault injection above on my Windows 11 machine.

@oneandonlydean
oneandonlydean merged commit 64a318c into boykopovar:main Oct 25, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants