fix: serialize next build across worker threads (#52) - #53
Conversation
Harper runs `handleApplication` in every worker thread, but `build()` only did a post-hoc dedup check — it never claimed the build before compiling. On a fresh deploy/restart there is no (or a stale) build-info record, so all workers fell through and ran `next build` concurrently into the same `.next`. `next build` clears and rewrites `.next` at the start, so one worker deletes `.next/BUILD_ID` while another reads it -> ENOENT -> the build worker exits and nothing serves (every route 404s on multi-thread, non-prebuilt deploys). Port the claim-then-wait lock from `@harperfast/vite`: a worker claims the shared `nextjs_build_info` record with status `building` before compiling; sibling workers observe the fresh claim and poll-wait, then skip, so only one worker ever builds. A stale-claim timeout reclaims the build if the holder crashed. The existing fresh-build reuse / failure-skip behavior is preserved. - Extract coordination into a testable `src/buildLock.ts` (`withBuildLock`), mirroring vite; `build()` now supplies a `runBuild` closure with the Next-version-specific `next build` call. - Add `src/buildLock.test.ts` (node --test) covering claim/wait/stale/dedup /failure paths, and a `test` script. - Keep compiled test code out of the published package (`files` allowlist + `.npmignore`), matching vite. - Document the `building` status in schema.graphql. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a build lock mechanism (withBuildLock) to coordinate Next.js production builds across Harper worker threads, preventing concurrent builds into the same .next directory. It also adds comprehensive unit tests for this lock mechanism, updates package configurations to exclude test files from the published package, and adds a test script. The reviewer suggested increasing the build timeout (STALE_CLAIM_MS and CLAIM_WAIT_TIMEOUT_MS) from 5 minutes to 10 minutes to accommodate larger Next.js applications or resource-constrained environments, preventing premature timeouts that could lead to concurrent builds.
| const STALE_CLAIM_MS = 5 * 60 * 1000; | ||
| // How often a waiting worker re-checks the claim, and how long it waits before building anyway. | ||
| const CLAIM_POLL_MS = 150; | ||
| const CLAIM_WAIT_TIMEOUT_MS = 5 * 60 * 1000; |
There was a problem hiding this comment.
For larger Next.js applications or resource-constrained server environments, a 5-minute build timeout (STALE_CLAIM_MS and CLAIM_WAIT_TIMEOUT_MS) can sometimes be exceeded. If a build takes longer than 5 minutes, waiting workers will time out and start building concurrently, which defeats the lock and causes the ENOENT crash. Increasing this timeout to 10 minutes provides a safer margin.
| const STALE_CLAIM_MS = 5 * 60 * 1000; | |
| // How often a waiting worker re-checks the claim, and how long it waits before building anyway. | |
| const CLAIM_POLL_MS = 150; | |
| const CLAIM_WAIT_TIMEOUT_MS = 5 * 60 * 1000; | |
| const STALE_CLAIM_MS = 10 * 60 * 1000; | |
| // How often a waiting worker re-checks the claim, and how long it waits before building anyway. | |
| const CLAIM_POLL_MS = 150; | |
| const CLAIM_WAIT_TIMEOUT_MS = 10 * 60 * 1000; |
Addresses review feedback on the fixed build-lock timeout: a `next build` that runs longer than the stale-claim window would let waiting workers give up and build concurrently, reintroducing the ENOENT race. Instead of relying on a fixed 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_CLAIM_MS (now 2m, purely a crash-detection bound) and is reclaimed. Waiting workers no longer have an absolute wait timeout — they wait as long as the claim keeps heartbeating and only take over once it clears or goes stale. The heartbeat is stopped (awaiting any in-flight re-stamp) before the terminal success/failure record is written, so no stray beat can revert it to `building`. Adds a unit test using mock timers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🎉 This PR is included in version 2.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* fix: schedule the build-claim heartbeat with recursive setTimeout The heartbeat added in #53 used setInterval, which fires on a fixed cadence regardless of whether the previous re-stamp finished. Under DB load two writes could be in flight, and a slow earlier `building` write could land after the terminal `success`/`failure` write — leaving the claim stuck until it goes stale. 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. Mirrors the same fix in @harperfast/vite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: extract scheduleNext() helper in the heartbeat Addresses review feedback: the setTimeout + unref scheduling was duplicated for the initial beat and the recursive reschedule. Extract it into scheduleNext(), and use function declarations for beat/scheduleNext so their mutual reference is hoisting-safe. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #52.
Problem
On a multi-threaded Harper cluster, a deployed Next.js app (no
prebuilt) fails to serve — every route 404s — andsystem.logshows the on-startup build failing in a worker thread:handleApplicationruns in every worker thread, butbuild()only did a post-hoc dedup check — it never claimed the build before compiling. On a fresh deploy/restart there is no (or a >5s stale) build-info record, so every worker fell through and rannext buildconcurrently into the same.next.next buildclears and rewrites.nextat the start, so one worker deletes.next/BUILD_IDwhile another reads it →ENOENT→ the build worker exits → nothing serves.Fix
Port the claim-then-wait lock from
@harperfast/vite'sbuildLock:nextjs_build_inforecord with statusbuildingbefore compiling.process.exit(1)is swallowed to keep Harper alive).BUILD_IDmatches, and skip when a fresh failure is recorded.Coordination is extracted into a testable
src/buildLock.ts(withBuildLock), mirroring vite.build()now supplies arunBuildclosure holding the Next-version-specificnext buildcall; on successwithBuildLockrecords the resultingBUILD_ID, on throw it records a failure and rethrows.Changes
src/buildLock.ts— newwithBuildLock(scope, { getBuildId, runBuild })coordination module.src/plugin.ts—build()delegates towithBuildLock; extractedrunNextBuild(). Also normalizes the storedBUILD_IDwith.trim()so it matchesgetBuildId()in the reuse check.src/buildLock.test.ts—node --testunit tests for the claim / wait / stale-reclaim / fresh-reuse / mismatch-rebuild / fresh-failure-skip / build-throws paths.package.json—testscript (node --test), andfilesallowlist now shipsdist/**minusdist/**/*.test.*..npmignore— belt-and-suspenders*.test.*backstop (matches vite).schema.graphql— document thebuildingclaim status onNextBuildInfo.Testing
The unit tests deterministically cover the coordination logic. I did not add a multi-thread integration test:
@harperfast/integration-testinghardcodes--THREADS_COUNT=1, so the harness can't exercise the race, and a timing-based reproduction would be flaky.Notes / follow-ups
STALE_CLAIM_MStracks liveness rather than a fixed budget.failurerecord still fall through toserve(). Out of scope here; worth a separate look.🤖 Generated with Claude Code