Skip to content

fix(import-audit): preserve JSON reports when writing fails - #4425

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
Lorkydey:fix/import-audit-atomic-json
Oct 14, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
Lorkydey:fix/import-audit-atomic-json

Conversation

@Lorkydey

@Lorkydey Lorkydey commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What

import_audit.py --json audit.json truncates the destination before writing the new report. A write error leaves an incomplete JSON file and destroys the previous audit. For a new destination it leaves a partial file that looks like an available report.

Write the complete report inside a temporary directory beside the resolved destination, then atomically replace the destination. New reports retain the normal 0666 permissions filtered by the process umask, without changing that process-wide setting. Preserve existing report permissions and symlink targets. Keep the caller-supplied output path in the directory-error diagnostic, including Windows short paths. Failed writes or replacements keep the old report, return the existing failure exit code and remove staging files, including Windows read-only temporaries. Input-overwrite protection and import classification stay unchanged.

Eight new test methods exercise the CLI entry point and real temporary files. POSIX permission cases run with isolated subprocess umasks. The disk-full injection writes an actual partial prefix before raising ENOSPC; with main dacd78525, an existing report is truncated and a new report is left behind. A replacement denial is also covered. This is controlled fault injection, not a claim of exhausting the host disk.

Tested

Rebased onto main dacd78525e537a779682032c0ace7f7af120af05, including #4545 and #4606, at exact head d45fee9b69c8f59e11818d3e25fbdfc0b6e296fd:

  • Windows 11, Python 3.12.4: python -m unittest discover -s tools/tests -p 'test_*.py' -q runs 287 tests, 285 pass and the two POSIX-only permission tests skip.
  • WinLibs GCC 15.2.0 r7, Ninja, Release: the relinker-only build succeeds and all 150 CTests pass. The focused import_audit CTest also passes.
  • Exact-source Windows/Linux/macOS validation passes with Python 3.12. Each runs all 287 tools tests, including JSON stops, module imports, atomic replacement, partial writes, symlinks, permissions and input-overwrite protection. Windows skips the two POSIX-only permission tests; Linux/macOS skip the Windows-only read-only test.
  • On all three hosted platforms and local Windows, the partial-write regression fails in both subcases against main's import_audit.py: the old report is truncated, or a partial new report is left behind. Restoring the exact PR source passes. This is controlled fault injection, not actual disk exhaustion.
  • The official Build run also passes all three relinker builds and test suites (Windows, Linux, macOS) for this PR head.
  • Conventions and git diff --check pass. No native runtime change or title run.

Overlap coordination

#4545 and #4606 are now merged. This rebase preserves main's stops construction and JSON field, then passes the complete result to write_json(args.json, result). It also retains the merged hard-link test changes, with one os import and the stat import required for permission tests. Their module-import, stops and hard-link regressions pass alongside the atomic-write tests. #4415 is closed without merging. No outstanding dependency on those PRs remains.

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; no runtime exports or silent stubs changed
  • No notes, investigation files or images added to the repository
  • New third-party code is a submodule built from source; none added
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Unblocks: none
  • AI-assisted: yes (Codex)

@MarcZX10

Copy link
Copy Markdown
Contributor

One filesystem behavior question: when the destination does not exist, NamedTemporaryFile creates the staging file with mode 0600 on POSIX, and that mode is carried through the replace. The old Path.write_text created a new report with the usual 0666 filtered by the process umask; existing destinations are correctly handled separately here. Is the new-report permission tightening intentional? If reports are meant to remain group-readable under a typical umask, the new-destination case may need an explicit mode and regression check.

@MarcZX10

MarcZX10 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

The Windows relinker check has one reproducible failure: import_audit fails test_unreadable_library_and_unwritable_json_exit_two (1/124). The test passes str(work.modules) as --json and expects that path in the diagnostic. write_json() first calls path.resolve(), so on the hosted Windows runner the error reports C:\Users\runneradmin\...\modules while the supplied temp path is C:\Users\RUNNER~1\...\modules. Keeping the caller-supplied path for diagnostics (while using a separately resolved path for the atomic replace if needed) should preserve the existing CLI error contract and make this test pass.

@Lorkydey
Lorkydey force-pushed the fix/import-audit-atomic-json branch 2 times, most recently from f6e0356 to 05d52e3 Compare October 12, 2026 12:30
@Lorkydey

Copy link
Copy Markdown
Contributor Author

@MarcZX100 Thx for catching both issues! Fixed the diagnostic path and the permissions for new reports, with regression tests added. The relinker CI now passes on Windows, Linux and macOS :)

@Lorkydey

Copy link
Copy Markdown
Contributor Author

@MrMha @thuanquang @Nixitov

I checked the overlap using #4425 at 05d52e396, #4415 at 79b039269, #4545 at 4feeef698 and #4606 at 7eae85946, combined on main c952c2b0e. Those PR heads are still unchanged. The changes cover different behavior, but there are textual merge conflicts:

With those resolutions, the combined Windows checkout ran 99 focused Python tests: 96 passed and 3 platform-specific cases were skipped. This was a local integration check on the pinned base above, not a full suite or Linux/macOS run. No dependency commits have been added to #4425.

@Nixitov has offered to rebase #4606, and #4545's description also offers a rebase if #4425 lands first. I propose #4425 first, followed by rebases preserving the changes above. Would that order work for everyone? If a different order is preferred, I can rebase this PR instead.

AI-assisted: yes (Codex).

@Nixitov

Nixitov commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Thanks @Lorkydey — that order works for me.

#4425 first, then I'll rebase #4606 after it lands: keep the stops data in the JSON result and pass it to your write_json(args.json, result), and if #4415 lands before that I'll retain its module-aware reference count and both sets of tests as well.

AI-assisted: yes

@thuanquang

Copy link
Copy Markdown
Contributor

Thanks @Lorkydey, that order works for me: #4425 first. Once it lands I'll rebase #4545 onto main, keeping a single import os and the two os.link changes, and rerun import_audit and stubgen. If #4415 lands before that, I'll keep its imports too.

AI-assisted: yes (Claude Code)

@MrMha

MrMha commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Confirm their proposed merge order works, and explain that your #4415 was closed, so they can proceed without waiting for it.

@github-actions github-actions Bot mentioned this pull request Oct 13, 2026
10 tasks done
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
@thuanquang

Copy link
Copy Markdown
Contributor

#4545 was merged first, so the overlap is now only the import os line that main already has. Dropping yours from the import block should resolve the conflict. Sorry for the reversed order, and thanks for coordinating.

AI-assisted: yes (Claude Code)

@Lorkydey
Lorkydey force-pushed the fix/import-audit-atomic-json branch from 05d52e3 to d45fee9 Compare October 13, 2026 19:46
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
@oneandonlydean
oneandonlydean merged commit 19785b5 into boykopovar:main Oct 14, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants