Repository navigation
fix(context): preserve packages when replacement fails (#142) - #147
Conversation
🦋 Changeset detectedLatest commit: 3633612 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 |
|
Heads up: #159 fixed the three Windows failures from #153, so main's CI now runs tests on ubuntu and windows with nothing skipped. That conflicts with this branch's ci.yml change. Merging main and keeping main's matrix (you can add a macos-latest entry to it if you want mac coverage) should clear it, and this PR's rename path gets real Windows coverage for free. Generated by Claude Code |
|
Thanks for the review! Updated in 3633612: merged main, retained Linux/Windows CI without skips, moved Crash-leftover cleanup remains a follow-up: safely removing abandoned staging directories needs ownership tracking so active builds are preserved. |
![human0-bot[bot]](https://avatars.githubusercontent.com/in/3310509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved — the current head preserves installed packages across the replacement failures reviewed.
Data & state — high risk, comment: A killed process can leave excluded staging files on disk; safe cleanup remains a follow-up.
This codebase is managed by Human0.
| export function createPackageTempFile(directory: string) { | ||
| // Keep staging on the destination filesystem for rename, and outside *.db | ||
| // discovery. A private directory also contains any SQLite journal files. | ||
| const tempDir = mkdtempSync(join(directory, ".context-")); |
There was a problem hiding this comment.
Data & state · comment: If the process is killed after this temporary directory is created, its partial database and SQLite artifacts remain because cleanup only runs on handled exits. The directory is excluded from discovery, so this is disk cruft rather than package corruption; track safe cleanup with ownership/locking.
This codebase is managed by Human0.
There was a problem hiding this comment.
Approved — failed builds, downloads, and copies now preserve the installed package, and the Windows and concurrent-download issues from the earlier review are addressed.
Data & state — high risk, comment: forced termination can still leave unused staging directories behind.
This codebase is managed by Human0.
| return { ...info, path: destination }; | ||
| }, | ||
| cleanup() { | ||
| rmSync(tempDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
Data & state · comment: If the process is killed before cleanup runs, its .context-* staging directory remains in the package directory. Discovery ignores it, but repeated interrupted builds can accumulate disk usage; please track the ownership-safe cleanup follow-up so these leftovers can eventually be reaped.
This codebase is managed by Human0.
Releases @neuledge/context 1.2.10 -> 1.2.11 (patch). Consumes four changesets: #147 (keep installed docs when a rebuild fails), #175 (keep formatted and linked text in section titles), #180 (keep code language fences in HTML pre/code blocks) and #185 (clean up abandoned staging directories). @neuledge/registry 0.0.23 -> 0.0.24 is the automatic dependent bump for the private workspace package. Verified before merging: npm dist-tags.latest is 1.2.10 and 1.2.11 is not published yet. The code reviewer approved the current head (eb13c24), and CI on main is green at ef14e81.
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.
store.ts.mainand retains its Linux and Windows CI matrix without test skips.Includes documentation and a patch changeset.
Validation on Linux: frozen dependency install, lint, build, and all 406 tests pass (303 context + 103 registry). CI lint, build, and the full Linux and Windows test suites also pass on commit
3633612.Automatic cleanup of staging directories left by a killed process is deferred: safely distinguishing abandoned directories from another process's active build needs ownership tracking. These directories remain excluded from package discovery.