fix(core): surface corrupt and permission errors in publish-state I/O#4269
Open
calebcgates wants to merge 1 commit into
Open
Conversation
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.
Summarize your changes:
packages/api/core/src/util/publish-state.tsis the persistence layer behindelectron-forge publish --dry-run/--from-dry-run. The class persistsmakeresults to<outDir>/publish-dry-run/<sha256>/<hash>.forge.publishso users can resume publishing without re-runningmake(which can take 10+ minutes on a real Electron app).Today, three fs-extra calls —
fs.readJsoninload(),fs.mkdirs+fs.writeJsoninsaveToDisk()— have no error handling. Three failure modes hit users with opaque errors that hide the actual cause:load()on a corrupt state file — if a previousmakewas killed mid-write (Ctrl-C, OOM, kill -9, NFS write that didn't fsync),readJsonrejects withSyntaxError: Unexpected end of JSON input. The user sees this in the listr task UI with no path, no recovery hint.load()on a missing state file — if the user manually cleanedout/between--dry-runand--from-dry-run,readJsonrejects withENOENTand the entire resume crashes — no graceful skip across the other state files in the same dir.saveToDisk()on a non-writableout/— corporate-managed macOS profile, full-disk-encrypted volume locked, Windows AppData on a network share that disconnected, sandbox--readonly.fs.mkdirsrejects withEACCES/EPERM/EROFSand the user sees an opaque rejection with no hint that it's theirout/directory.This PR:
load()with try/catch that translatesENOENTandSyntaxErrorinto clear, actionable error messages (with the offending path and a recovery hint). The original error is attached as.causeviaObject.assign(matching the existingtarget: ES2021lib intsconfig.base.json— no Error 2-arg constructor / ES2022 dependency). Other errors re-throw unchanged.fs.mkdirs(dirname) + fs.writeJson(file, ...)insaveToDisk()with the documented fs-extra equivalentfs.outputJson(file, ...)— see outputJson.md: "Almost the same as writeJson, except that if the directory does not exist, it's created." This collapses two file-system calls into one atomic call (no race window wheremkdirssucceeds butwriteJsonfails), and wraps it in try/catch that surfacesEACCES/EPERM/EROFSwith the path included.packages/api/core/spec/fast/util/publish-state.spec.ts(8 tests) covering happy path + ENOENT + corrupt-JSON + EACCES + EROFS + ENOSPC re-throw for both methods. This is the first dedicated spec forpublish-state.ts; it follows the existing pattern frominstall-dependencies.spec.ts(vi.mock(import('fs-extra'), async (importOriginal) => ...)). No new test infra introduced — thespec/fast/workspace already globs this path viavitest.workspace.mts.No behaviour change on the happy path. The dry-run state files are still written to and read from the same paths in the same JSON format. The only observable change is what surfaces when the file system fails.
Verification
yarn build— clean (the pre-existing@aws-sdk/typesCredentialserror inPublisherS3.tsis on main; not introduced here).yarn lint:js— clean (no new warnings).yarn vitest run --project fast packages/api/core— 17 files, 123 tests, all passing (including the 8 new tests).Scope
The forge codebase has additional fs-extra calls without error handling in WebpackPlugin / VitePlugin, the template scaffolders, and the Maker base class. Those each have different trade-offs (high blast radius, shared-helper refactor, intent-by-omission swallowing) and are out of scope for this PR. This change addresses the smallest, most surgical fix to a user-reproducible failure mode in a critical publish-resume UX feature.