fix: heartbeat the build claim so long builds don't defeat the lock - #26
Merged
Conversation
The build lock used a fixed 5-minute timeout: if a `vite build` ran longer than that, waiting workers gave up (WAIT_TIMEOUT_MS) and started building concurrently into the same output dir — exactly the race the lock exists to prevent. A large SSR app or a constrained CI/edge box can cross that bound. Instead of relying on a timeout large enough for the longest build, the claiming worker now re-stamps its "building" record every 30s while building. A live build stays fresh no matter how long it takes; a crashed builder's claim still goes stale within STALE_MS (now 2m — purely a crash-detection bound) and is reclaimed. Waiting workers no longer have an absolute timeout: they wait as long as the claim keeps heartbeating, skip once it goes "idle" (sibling finished), and take over only if the heartbeat lapses (sibling died). The heartbeat is stopped (awaiting any in-flight re-stamp) before the terminal "idle" write, so no stray beat can revert the record to "building". Adds a unit test using mock timers. Ports the same hardening applied to `@harperfast/nextjs`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a heartbeat mechanism to the build lock system in src/buildLock.ts to keep the 'building' claim fresh during long builds, preventing other workers from prematurely reclaiming the lock. It also updates the waiting logic to wait dynamically as long as the heartbeat is active and adds corresponding unit tests. The feedback recommends replacing setInterval with a recursive setTimeout pattern to avoid race conditions from overlapping concurrent database writes, and updating the unit tests to mock setTimeout accordingly.
Addresses review feedback: setInterval fires on a fixed cadence regardless of whether the previous re-stamp finished, so under DB load two writes could be in flight and a slow earlier "building" write could land after the terminal "idle" write — leaving the claim stuck. Recursive setTimeout schedules the next beat only after the current write settles, so at most one is ever in flight and stopHeartbeat() fully awaits it before the terminal write. Chaining off Promise.resolve().then(() => table.put(...)) also converts a synchronous throw from table.put into a caught rejection instead of an uncaught timer error. Test mocks setTimeout instead of setInterval accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🎉 This PR is included in version 1.1.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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.
Problem
withBuildLockuses a fixed 5-minute timeout (STALE_MS/WAIT_TIMEOUT_MS). If avite buildruns longer than that, waiting workers hitWAIT_TIMEOUT_MS, give up, and start building concurrently into the same output directory — the exact race the lock exists to prevent. A large SSR app, a big monorepo, or a resource-constrained CI/edge box can cross that bound.This is the same latent bug that was flagged on
@harperfast/nextjs's port of this lock; this PR brings the fix back so the two plugins stay in sync.Fix: heartbeat the claim
Instead of relying on a timeout large enough for the longest build:
buildingrecord every 30s (HEARTBEAT_MS) while compiling, so a live build stays fresh no matter how long it takes.STALE_MSis now purely a crash-detection bound (2 min) — it no longer has to exceed the build duration. A crashed builder's claim goes stale within 2 min and gets reclaimed.WAIT_TIMEOUT_MS. A waiter now waits as long as the claim keeps heartbeating; it skips once the claim goesidle(sibling finished), and takes over the build if the heartbeat lapses (sibling died) — previously a crashed builder left the waiter to skip and serve nothing.idlewrite, so no stray beat can revert the record tobuilding.Behavior preserved
Claim → build → release (
idle), rebuild-on-restart (anidlerecord isn't treated as a live claim), skip-while-a-sibling-builds, and release-on-throw all behave as before. The existingwithBuildLocktests pass unchanged; only the stale-claim test's comment was updated.Testing
npm test— 34/34 pass, including a new mock-timer test asserting the claim is re-stamped across heartbeat intervals and thatidleis the last write.npm run format:checkclean.Note
STALE_MS(2 min) must stay comfortably larger thanHEARTBEAT_MS(30 s) so a live-but-busy builder (event loop pinned during a build) isn't misjudged as crashed.vite buildis async/spawns workers, so the main-thread timer fires between phases — 4× headroom covers transient stalls.🤖 Generated with Claude Code