fix(context): preserve packages when replacement fails (#142) - #147
TheRealBecks wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 9b1e885 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
moshest
left a comment
moshest
left a comment
There was a problem hiding this comment.
The fix works, and I confirmed the bug is real rather than taking the issue's word for it. I wrote a throwaway test against 149f112 that builds a package and then injects a failure into the rebuild. Two distinct losses: failing at openDatabase leaves the directory completely empty (test-lib@1.0.0.db is gone), and failing later — I used the CREATE VIRTUAL TABLE step — leaves a same-size file that readPackageInfo rejects with "Invalid package: missing name or version in meta table". The same test against this branch passes both cases with the original intact and still readable. Full validation on Linux: lint and build clean, 306 tests pass (257 context + 49 registry), matching your number.
The thing I'd want changed before merge is the CI matrix. Run 289 added windows-latest and it failed — but every failure was in @neuledge/registry (definition.test.ts ENOENT on @apollo\client.yaml, two publish-all summary assertions), all pre-existing and unrelated to this change. Turbo then aborted, so @neuledge/context never finished its suite there. Commit 9b1e885 dropped Windows instead. Net effect: the rename path this PR is entirely about has still never executed on Windows, and package-write.test.ts's sharing-violation test is dead code on Linux and macOS since the renameSync EPERM/EBUSY cases are mocked. Meanwhile macos-latest doubles test cost on a platform with the same rename semantics as Linux. I'd keep windows-latest and skip the three known-bad registry tests there with a linked follow-up issue, and drop macOS. At minimum the PR description needs fixing — it still claims "Extends CI testing to Linux, Windows, and macOS."
Smaller things. A SIGKILL mid-build leaves a .context-XXXXXX directory in ~/.context/packages and nothing ever removes it; I confirmed three of them survive a subsequent successful build. loadPackages correctly ignores them so this is cosmetic, but a sweep of stale .context-* and legacy .downloading-* in ensureDataDir would close it out. Also, download.test.ts's "uses independent staging files for simultaneous downloads" still stubs Date.now — uniqueness comes from mkdtempSync now, so the stub does nothing and the test no longer exercises what its name says.
Exporting loadPackages from cli.ts for tests isn't a correctness problem — I checked, the isRunDirectly argv guard holds under vitest and importing the module parses nothing — but it pulls commander, the server and git into two test files. store.ts seems like the natural home.
Checked and fine: BuildResult does have path, and { ...result, path: outputPath } is correct since writePackage would otherwise return the staging path. buildPackage into a non-existent dirname(outputPath) throws on both main and this branch, so no regression — only the message degrades from "Cannot open database because the directory does not exist" to a raw mkdtemp ENOENT, which is worth a nicer error someday. I ran the built CLI with add --save ./brand/new/dir against a scratch HOME and it created the tree and saved correctly, no staging left behind, because savePackageCopy already mkdirs before copyPackageFile. loadPackages survives a directory named *.db in the packages folder. And last-writer-wins on concurrent builds is safe here — each build stages in its own mkdtemp and rename is atomic, so the destination is always one complete database.
Generated by Claude Code
|
Filed #153 for the three Windows registry failures that caused the matrix removal, so the skip has something to link to. Restoring Generated by Claude Code |
2 participants
Fixes #142.
Rebuilds and installs previously removed or overwrote the working package before its replacement was ready. This change stages replacements in unique temporary directories on the destination filesystem, closes and validates them, then renames them into place.
Includes documentation and a patch changeset.
Validation on Linux: frozen dependency install, lint, build, and all 306 tests pass. Windows and macOS validation will run through the PR’s CI matrix.