Repository navigation
fix(artifacts): make extraction output tree work on Windows - #753
Merged
Merged
Conversation
SafeOutputTree.create() called fchmod(0o700) on a directory handle and
commit() fsynced a directory handle. Both are POSIX-only: Windows rejects
them with EPERM ("operation not permitted, fchmod"/"fsync"), so every
extract_artifact call failed with a generic io error and produced no output.
Skip the redundant directory chmod and directory fsync on Windows. mkdir
already applies the requested mode, and file contents are synced in write().
Add a Windows regression test and run it, plus the archive-safety test, in
the curated Windows CI lane so the behavior is covered.
Owner
|
Thanks, @DeryFerd, for your work on “fix(artifacts): make extraction output tree work on Windows.” I appreciate your contribution to REA. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
extract_artifactfails on every Windows host.SafeOutputTree.create()opens the freshly created output directory and callschmod(0o700)on the directory handle, andcommit()callssync()on a directory handle. Both are POSIX-only operations, and Windows rejects each withEPERM: operation not permitted. The provider catches the failure and reports it as a genericioreason, so the caller seesartifact_operation_failedwith no output directory and no hint about the cause. This change skips the two directory-only operations on Windows, where they have no effect to begin with, and adds a regression test plus Windows CI coverage so the behavior cannot silently break again.Problem and expected behavior
The smallest trigger is a single extraction on Windows. Given any extractable archive target (
zip,ipa,apk,msix,appx, or amachoslice set),ArtifactProvider.execute("extract_artifact", { output_root })callsextractArtifact, which callsSafeOutputTree.create(output_root). On Windows that call throwsEPERM: operation not permitted, fchmodbefore any file is written,extractArtifactrolls back the unsealed tree, and the provider returns{ ok: false, reason: "io" }. The tool is advertised as available on Windows, and the operation is a first-class workflow, so the effect is that a supported capability is entirely broken rather than partially degraded.The violated invariant is narrower than it looks. The directory
chmodwas meant to force0o700on the new output root, butmkdir(canonicalOutput, { mode: 0o700 })on the line above already applies the requested mode, so the handlechmodonly re-asserts a mode that is already set. The directorysyncincommit()was meant to make the tree durable, but each file's bytes are already flushed and read back insidewrite(), and Windows has no directoryfsyncat all. Neither operation contributes anything on Windows, and both are the reason the whole operation aborts there.The expected behavior is that extraction produces the same files with the same digests on Windows as it does on Linux and macOS, and that the POSIX-only durability steps remain in place on the hosts that support them. That is what this change does.
Change and scope
SafeOutputTreenow takes the platform as a constructor value with aprocess.platformdefault, matching the seam already used elsewhere in the artifact layer (for exampleArtifactProvider(platform = process.platform)).create()only opens the directory handle and applies0o700when the platform is notwin32; on Windows it relies on themkdirmode, which is what actually sets the mode on that host.commit()marks the tree published and returns immediately on Windows, because there is no directoryfsyncthere and the file-level durability guarantee is already established inwrite(). The POSIX path is byte-for-byte unchanged, including the descriptor close on the failure path and the parent/output sync ordering.Two regression surfaces are added.
tests/boundary/filesystem/safeOutputTree.test.tsgains a case that builds and commits a tree through thewin32seam and asserts the written file is present with the expected content, which is exactly the path that used to throw. The curated Windows CI lane in.github/workflows/ci.ymlnow runssafeOutputTree.test.tsandartifactProvider.archiveSafety.test.ts, so the Windows-only behavior is exercised on a Windows runner instead of only on developer machines.Intentionally not included: no change to extraction containment, path normalization, digest verification, rollback, or the public tool contract; no new capability flag or approval step; and no attempt to give Windows a directory-level durability guarantee it does not have. The Windows behavior is now "the same result, minus the two no-op POSIX steps", not "a reduced guarantee that used to exist".
Contract and boundary impact
src/artifacts/SafeOutputTree.ts), reached fromsrc/application/ArtifactExtraction.tsandsrc/firmware/FirmwarePublication.ts.extract_artifactinput, output, and result schemas are unchanged.O_EXCL | O_NOFOLLOW, symlink-resistant parent creation, and readback verification are untouched.docs/product-catalog.json), package, or installation impact: none.Evidence and regression coverage
tests/boundary/filesystem/safeOutputTree.test.ts, and the Windows CI lane now includessafeOutputTree.test.tsandartifactProvider.archiveSafety.test.ts.main,safeOutputTree.test.tsfails three cases withEPERM: operation not permitted, fchmod, andartifactProvider.archiveSafety.test.tsfails the twomsix/appxbundleextraction cases withexpected false to be true. The new case fails on base with the samefchmoderror and passes after the fix, which is the red/green check that the test actually covers the change.For evidence-bearing changes:
Validation performed
npx tsc --noEmit— passed, exit 0.npm run build:cached— passed, 1 task successful.npx vitest run tests/boundary/filesystem/safeOutputTree.test.ts tests/boundary/providers/native/artifactProvider.archiveSafety.test.ts tests/boundary/providers/native/artifactProvider.extraction.test.ts tests/process-global/curatedWindowsLane.test.ts— 10 passed, 1 skipped (4 files).npx oxlint src/artifacts/SafeOutputTree.ts tests/boundary/filesystem/safeOutputTree.test.ts— 0 warnings, 0 errors.npx oxfmt --checkon the changed files — clean (LF, committed form).The focused runs above were executed on Windows x64 with Node.js 24, which is the affected host. The Windows CI lane will re-run the two boundary files on a hosted Windows runner.
Compatibility, safety, and release
mkdirand still writes each file withO_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOWunder a symlink-checked parent. The only thing skipped on Windows is a redundant mode re-assertion and a directory sync that Windows does not implement.Review checklist
type(scope): outcome.