fix(scripts): self-heal corrupt Mailpit install and extract atomically#259
fix(scripts): self-heal corrupt Mailpit install and extract atomically#259rlorenzo wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #259 +/- ##
=======================================
Coverage 45.29% 45.29%
=======================================
Files 897 897
Lines 51928 51928
Branches 4878 4878
=======================================
Hits 23523 23523
Misses 27803 27803
Partials 602 602
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Bundle ReportBundle size has no change ✅ |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughMailpit ZIP extraction now finalizes files atomically, startup handles process errors, and management reinstalls binaries whose installed version cannot be determined. ChangesMailpit management reliability
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/manage-mailpit.js (2)
476-484: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear the timeout on process spawn failure.
The
setTimeoutis not cleared if the spawned process immediately errors out. This leaves a dangling timer that will execute seconds after the promise has already rejected, leading to redundant checks and conflicting success/failure logs.Store the timer ID and clear it in the error handler.
🛠️ Proposed fix
+ let startupTimer; + process.on("error", (err) => { + clearTimeout(startupTimer); logError(`Failed to start Mailpit: ${err.message}`) resolve(false) }) process.unref() // Wait a moment and check if it started successfully - setTimeout(async () => { + startupTimer = setTimeout(async () => { const isRunning = await checkMailpitWeb()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/manage-mailpit.js` around lines 476 - 484, Update the process startup flow around the error handler and delayed startup check to store the setTimeout identifier, then clear that timer inside the process "error" handler before resolving failure. Preserve the existing delayed success check when no spawn error occurs.
249-273: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winPrevent finalizing corrupt files and background leaks on stream errors.
The
closeevent fires even if the write stream encounters an error or is destroyed. When an error occurs, the promise correctly rejects, but theclosehandler still fires, renaming the broken.partialfile into place and callingzipfile.readEntry(). This defeats the atomicity fix and continues extracting files in the background after returning a failure.Track the error state to skip finalization and ensure
writeStreamis destroyed on read errors.🛠️ Proposed fix
const partialPath = `${filePath}.partial` const writeStream = fs.createWriteStream(partialPath) + let hasErrored = false; writeStream.on("close", () => { + if (hasErrored) return; + try { fs.renameSync(partialPath, filePath) } catch (renameErr) { logError(`Error finalizing ${filePath}: ${renameErr.message}`) reject(renameErr) return } extractedFiles.push(entry.fileName) logInfo(`Extracted: ${entry.fileName}`) zipfile.readEntry() }) readStream.on("error", (readErr) => { + hasErrored = true; + writeStream.destroy(); logError(`Error reading ${entry.fileName}: ${readErr.message}`) reject(readErr) }) writeStream.on("error", (writeErr) => { + hasErrored = true; logError(`Error writing ${filePath}: ${writeErr.message}`) reject(writeErr) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/manage-mailpit.js` around lines 249 - 273, Update the extraction stream handling around the writeStream close/error listeners to track whether a read or write error occurred; have the close handler return without renaming, logging success, or calling zipfile.readEntry() when errored. In the readStream error handler, mark the stream as failed, reject the promise, and destroy writeStream to prevent background writes; apply the same failed-state guard in the writeStream error handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/manage-mailpit.js`:
- Around line 476-484: Update the process startup flow around the error handler
and delayed startup check to store the setTimeout identifier, then clear that
timer inside the process "error" handler before resolving failure. Preserve the
existing delayed success check when no spawn error occurs.
- Around line 249-273: Update the extraction stream handling around the
writeStream close/error listeners to track whether a read or write error
occurred; have the close handler return without renaming, logging success, or
calling zipfile.readEntry() when errored. In the readStream error handler, mark
the stream as failed, reject the promise, and destroy writeStream to prevent
background writes; apply the same failed-state guard in the writeStream error
handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1535648f-93eb-4b42-9923-3903385ff7f5
📒 Files selected for processing (1)
scripts/manage-mailpit.js
- Reinstall when the installed binary cannot report its version - Extract zip entries to .partial files, renaming into place only after the write is fully flushed - Log async spawn failures instead of crashing
a755a3c to
008d1d0
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/manage-mailpit.js`:
- Around line 246-277: Update the extraction failure handling around the
writeStream close/error callbacks to remove partialPath whenever reading,
writing, or finalizing fails. Ensure cleanup occurs before rejecting, while
preserving the existing successful rename and extraction flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5233fe5-7485-4cac-8d20-ae88b92d35ae
📒 Files selected for processing (1)
scripts/manage-mailpit.js
2 participants
Problem
An interrupted Mailpit install (e.g. Ctrl+C during extraction) could leave a truncated
mailpit.exe. The manage script then got permanently stuck: the version check on the broken binary returned null, which short-circuited the update/reinstall logic, and every subsequentdev/dev:buildfailed withError managing Mailpit: spawn EFTYPE(Windows ERROR_BAD_EXE_FORMAT) with no way to recover short of manually deleting the install folder.Changes
.partialfile and rename it into place only after the write is fully flushed, so an interrupted install can never leave a truncated file at the final path.Testing
C:\Tools\mailpit, rannpm run mailpit:start, verified download, hash check, atomic extraction, and startup.npm run lint scripts/manage-mailpit.jsclean; pre-commit lint/test/build all passed.