dev server: survive watch-mode rebuild failures - #8
Conversation
The content watcher fires on transient states — a half-swapped chapter set, a _meta.json entry whose file isn't on disk yet — and the strict validators (navigation completeness, frontmatter titles) throw on them. rebuild() had no error handling: the voided promise turned the throw into an unhandled rejection and killed the dev server mid-session (reproduced during a multi-file chapter swap on a real project), and the held mutex would have deadlocked any later rebuild even if the process had survived. rebuild() now wraps its body in try/catch/finally: on failure it reports the error, keeps serving the last good build, consumes this batch's change events (the paths stay recorded in changesSinceLastBuild, so the next change event retries them), and always releases the build mutex. Verified live: broke the demo nav mid-serve (moved a chapter file away) — the server logged 'Rebuild failed — still serving the last good build', kept answering with HTTP 200, and recovered with a clean rebuild when the file came back. Co-Authored-By: Shannon's Claude <257597027+shannonshen49@users.noreply.github.com>
|
Do you want Sepo to review this PR? Add the |
![sepo-agent-app[bot]](https://avatars.githubusercontent.com/in/3527007?s=60&v=4){kind=small}
| `Emitted ${emittedFiles} files to \`${argv.output}\` in ${perf.timeSince("rebuild")}`, | ||
| ) | ||
| console.log(styleText("green", `Done rebuilding in ${perf.timeSince()}`)) | ||
| changes.splice(0, numChangesInBuild) |
There was a problem hiding this comment.
changes.splice(0, numChangesInBuild) also runs in the catch below. If clientRefresh() throws after this success-path splice, control enters the catch and splices another batch from changes, which can drop queued change events. Consider moving the shared splice into finally next to release() so the queue is consumed exactly once on both success and failure.
This comment has been minimized.
This comment has been minimized.
|
Re regression coverage for the recovery path: agreed as follow-up — it needs a small integration harness (spawn the dev server, mutate content, assert liveness), which doesn't fit the unit suite. The manual reproduction procedure is documented in the PR body; happy to add the harness in a separate PR. |
AI Review Synthesis
Summary of PR/IssuePR #8 makes the dev server survive watch-mode rebuild failures instead of crashing on transient invalid content states. The change wraps ReviewThe core fix is correct and narrowly scoped. Both reviewer artifacts agree on one concrete remaining warning: the watcher queue can be over-consumed if
Progress
Issue DetailsQueue events can be over-consumed if clientRefresh throwsCause: The success path splices the current batch at Candidate solutions: Move the shared Comments: This is low probability and still better than the previous crash, but it is concrete branch-change work and already has inline feedback. Recovery guarantee has non-blocking limitsCause: One reviewer noted that parser worker errors can still exit the process, and another noted that emitter failures may happen after partial output writes. The new catch covers the reported docs-navigation validation path, but not every possible rebuild failure mode. Candidate solutions: Treat these as follow-up hardening only if maintainers want the guarantee to cover arbitrary parser and emitter failures. Comments: This is informational and should not drive automated fix-pr work for this PR. Failed-rebuild recovery lacks automated regression coverageCause: The behavior spans watcher batching, retained Candidate solutions: Add a future integration harness that spawns the dev server, mutates content into an invalid state, asserts liveness, then restores content and checks recovery. Comments: The author already agreed this belongs in a separate follow-up PR. Recommended Next Step
Final Verdict
Action Items
|
…hrows Review finding: changes.splice(0, numChangesInBuild) ran on the success path and again in catch, so a clientRefresh() throw after a successful rebuild consumed the batch twice — dropping unrelated queued watcher events. The catch-path splice is now guarded by a batchConsumed flag. Co-Authored-By: Shannon's Claude <257597027+shannonshen49@users.noreply.github.com>
Rubrics UpdateNo changes were committed to no rubric changes I inspected PR #8’s body, issue comments, review comments, reviews, linked follow-up issue #15, collaborator/owner signals, and existing rubric locations. There is no existing
|
1 participant
Summary
Fifth PR in the migration-fix series (independent of the others). The dev server died whenever a watch-mode rebuild hit a validation error: the content watcher fires on transient states (half-swapped chapter sets, a
_meta.jsonentry whose file isn't written yet), strict validation throws, andrebuild()'s voided promise turned that into an unhandled rejection that killed the process — reproduced during a multi-file chapter swap on a real project. The held build mutex would also have deadlocked later rebuilds.rebuild()now wraps its body in try/catch/finally: report, keep serving the last good build, consume the batch's change events (paths stay inchangesSinceLastBuild, so the next change event retries), always release the mutex.Verification
npm run checkpasses.Rebuild failed — still serving the last good build, the site kept answering HTTP 200, and restoring the file produced a clean recovery rebuild — process alive throughout. Same scenario killed the unpatched server.🤖 Generated with Claude Code