refactor(project, server): Improve error handling and recovery#1452
Draft
RandomByte wants to merge 18 commits into
Draft
refactor(project, server): Improve error handling and recovery#1452RandomByte wants to merge 18 commits into
RandomByte wants to merge 18 commits into
Conversation
Classify caught task errors into three branches: abort, transient (concurrent source change), and normal. Transient errors re-queue the affected projects and keep held reader requests open so they resolve on the retry. Normal errors are reported via ServeLogger. The 'error' event stays reserved for fatal failures like a watcher crash, so the process still exits on those.
A failed build latches its projects into a new ERRORED state and short-circuits #getReaderForProject with the captured error. An unchanged rebuild would reproduce the same failure, so hold the gate until real signal arrives. Three signals lift the gate: - Source change on the project itself: invalidate() already clears #lastError. - Source change on a dependent: _projectResourceChanged invalidate()s each dependent, lifting ERRORED as a side effect. - Source change on a transitive dependency, or any successful build cycle: both call the new ProjectBuildStatus.clearError() (ERRORED -> INVALIDATED, drops #lastError without touching the reader or aborting a build). Without this a request resolving through a gated dependency would replay the captured error forever. Correct the banner for the error path: _projectResourceChanged now also transitions ERROR -> STALE, #processBuildRequests re-signals BUILDING per iteration so retry progress replaces the red state, #getStaleProjectNames excludes ERRORED projects, and the post-cycle transition preserves ERROR for projects that stay gated. Add an integration test for the gate plus four unit tests: a dependent's change lifts its dependency's gate, a successful cycle lifts unrelated gates, no signal keeps the gate closed, and a change on the ERRORED project lifts its own gate.
Every next(err) in the middleware chain now flows through a custom 4-argument handler that responds with HTTP 500, text/plain content type, and the error stack (falling back to the message or String(err)) as the body. Registered last so it catches errors from every earlier middleware, including third-party custom middleware where we cannot wrap a try/catch. If the response has already started, delegate to Express's default handler which closes the connection. Console-log routing (differentiating gated build errors from unexpected ones) stays out of this handler for now — that lands in a follow-up.
On a delta signature transition whose changedProjectResourcePaths lists a path the delta task did not re-emit, the merge loop in recordTaskResult replayed the previous stage's copy of that path into the new stage writer. When a source file is deleted between builds, the delta task emits nothing for it and the merge resurrects the pre-deletion output, so downstream readers keep serving the file after its source is gone. Affects every differential task (replaceCopyright, replaceVersion, replaceBuildtime, minify). Filter the merge by cacheInfo.changedProjectResourcePaths: if a path was flagged changed and the task did not write it, drop the stale copy instead of replaying it. Paths outside the delta merge as before. Track the dropped count alongside merged count in the perf log. Add four fail-then-succeed test buckets covering invariants across a failed-attempt / retry boundary on one ProjectBuildCache instance: 1. #writtenResultResourcePaths accumulates across failed attempts, because allTasksCompleted (which clears it) never runs on a thrown build. The worst outcome is redundant I/O on the retry, not wrong output: the retry re-hashes each leaked path against its fresh reader, so unchanged content yields the same signature and changed content invalidates correctly. The test pins this behavior. Clearing the set cleanly needs a separate fail/abort hook, since #initSourceIndex legitimately seeds the array with delta paths on the first build after loading from persistent cache. 2. #currentStageSignatures reflect the retry's stages after a full retry. 3. The delta merge does not resurrect a deleted-source resource across the boundary. 4. #frozenSourceReader is nulled by the retry's initStages call before further task inputs are computed.
MonitoredReader records every byPath/byGlob call regardless of result, so a task probing an optional file or using a glob that matches nothing still contributes those requests. On #addRequestSet, when findExactMatch misses and findBestParent picks a proper-subset parent, the delta can contain only requests that resolve to zero resources. That threw "Unexpected empty added resources for request set ID ...", reproduced by branch-switching in OpenUI5. These reads are not benign: whether a probed file exists can change task output, so collapsing an empty-delta node onto its parent's signature would serve stale results after the file's existence changes. Replace the throw with a marker scheme: - On an empty-resolving delta, derive an empty tree from the parent (deriveTree([])) and record the added-request keys on metadata.unresolvedKeys. For an all-empty root recording, keep the empty ResourceIndex and populate unresolvedKeys with every request. Partially-resolving deltas record only the unresolved subset. - Route getIndexSignatures() and #addRequestSet's returned signature through a new #computeNodeSignature: the pure tree hash when unresolvedKeys is empty, otherwise sha256(treeHash \0 sorted keys). - In updateIndices, drop keys from unresolvedKeys as their request resolves to a real resource. Once drained, the signature collapses to the pure tree hash — matching a same-shape recording with the resource present. - Persist unresolvedKeys on root and delta entries through toCacheObject/fromCache. SharedHashTree is untouched: deriveTree([]) already produces a distinct ResourceIndex whose tree hashes to the parent's, so the tree-identity map keys stay distinct across nodes. Rewrite the three repro tests to assert no-throw plus signature distinctness; add convergence, cache round-trip, empty-root discrimination, and BuildTaskCache-shape coverage.
…estManager #collectUnresolvedKeys and #drainUnresolvedKeys both branched on request.type === 'path' vs 'patterns', with the path branch scanning the path array via Array.includes. Extract the check into #requestResolvesAgainstPaths and back the path lookup with a Set; the patterns branch still gets the array form it needs for micromatch.
@parcel/watcher can report that OS-level FS events were dropped and its
event cache is unreliable ("Events were dropped by the FSEvents client.
File system must be re-scanned."). The build cache derives what changed
solely from watcher events fed through resourcesChanged(), so dropped
events leave the change signal incomplete and cached results can be served
stale. Any WatchHandler error was previously terminal — the server went to
ERROR with no recovery.
Treat a watcher error as recoverable: recreate the watch subscriptions,
force a full source re-scan of every project, and return the server to
STALE so the next reader request rebuilds against a re-indexed tree.
- Extract ProjectBuildCache.resetForFullRescan() from the reset block
allTasksCompleted() already used for build-end source drift. It re-arms
initSourceIndex() to re-glob and diff against the persisted index,
catching changes the watcher never reported.
- Add ProjectBuilder.forceFullRescan() (guarded by #buildIsRunning),
delegating through BuildContext / ProjectBuildContext.
- BuildServer.#recoverWatcher() orchestrates recovery: guarded against
re-entrant error storms, quiesces validation and the active build,
recreates the watcher, forces the re-scan, invalidates all projects, and
drains parked reader requests. #triggerRequestQueue and
#scheduleBackgroundValidation are suppressed during recovery so nothing
races the builder lock. A sliding-window counter escalates to terminal
ERROR if recovery keeps failing, since dropped events arrive via the
subscription callback rather than a watch() rejection and would
otherwise loop forever.
emit("error") stays reserved for the terminal fallback; a successful
recovery emits sourcesChanged so clients reload.
A build-task failure (e.g. a LESS syntax error from buildThemes) left the reused in-memory build state corrupted, so `ui5 serve` kept returning the same error after the source was fixed, until a server restart. When a task throws, runTasks rejects before allTasksCompleted() runs. The project's ProjectResources keeps the partial stage writers from the broken source, and #currentResultSignature keeps the last successful build's value. On the next build the recovered source signature matches the retained #currentResultSignature, so #findResultCache short-circuits, marks the project usesCache, and serves the stale broken stages. Dependent theme libraries then recompile against that and fail the same way. Discard the failing project's in-memory state on a genuine (non-abort) failure so a later rebuild re-imports clean stages from the persistent cache. ProjectBuilder#build tracks the in-flight context and calls resetForFullRescan() on it; abort and SourceChangedDuringBuildError are excluded via isAbortError. Harden resetForFullRescan() against the same corruption: also clear #currentResultSignature, #cachedResultSignature, #currentStageSignatures, and #writtenResultResourcePaths, and reset the stage pipeline via the new ProjectResources.reset(). Otherwise the #findResultCache early return would still fire and skip re-importing the cached stages after a reset.
…settle sourcesChanged drives live-reload, so a lone edit should reach clients fast. On small projects the triggered build finishes well under 100 ms, so the previous 100 ms trailing debounce dominated edit-to-reload latency. Emit on the leading edge: the first change of a quiet period notifies immediately, and a trailing settle window coalesces the rest of a burst into one further emit. A single edit now reaches clients at the watcher's own latency floor. Raise the window to 550 ms, above @parcel/watcher's 500 ms coalescing cap. The watcher delivers a continuous operation like `git checkout` as batches up to 500 ms apart; a window below the cap would emit per batch, while one above it lets each batch reset the window so the whole operation collapses to one leading plus one trailing emit. With leading-edge emission the window size no longer affects single-edit latency. Rename SOURCES_CHANGED_DEBOUNCE_MS to SOURCES_CHANGED_SETTLE_MS.
When a source change lands mid-build, the build is aborted and its projects re-queued. The restart used the same 10 ms debounce as a reader request, so a burst of changes restarted a build per batch, each aborted by the next. @parcel/watcher caps its coalescing at 500 ms, so a continuous operation (git checkout, save-all, a bundler writing many files) arrives as batches up to 500 ms apart, producing a build -> abort -> build cycle for the whole operation. Hold the post-abort restart until changes are quiet for 550 ms (above the watcher's cap), resetting the window on each further change via #_projectResourceChanged, so the burst collapses into a single rebuild. The delay is scoped to the speculative restart: a reader request still enqueues on BUILD_REQUEST_DEBOUNCE_MS and supersedes a pending restart, so serving a request is unaffected. Extract the inline 10 ms request-queue debounce into BUILD_REQUEST_DEBOUNCE_MS and give #triggerRequestQueue a delay parameter.
During a `git checkout` the serve status could stay on `building` or flip to red `error` while nothing ran, until the checkout finished. The banner claimed a build was in progress when #activeBuild was already null. The deferred post-abort restart had no state to report: during its settle window the server is waiting for changes to stop, but SERVER_STATES had no value meaning that, so it showed whatever the last transition left. Add a SETTLING lifecycle state — changes seen, rebuild pending, holding until changes go quiet. It sits between STALE and BUILDING with no active build (#activeBuild === null); the deferred timer moves it to BUILDING when the window elapses. Route both deferral sites through it: - Post-abort restart reports SETTLING instead of leaving the banner on `building`. - Failure-with-pending-changes (the signal.aborted || #resourceChangeQueue.size > 0 branch) now defers and reports like the abort path instead of parking on the failed build's state. This fixes the `git checkout` case: the doomed first build reports SETTLING and retries once the tree is quiet instead of a spurious serve-error. Genuine non-transient failures still latch ERRORED. BUILDING -> SETTLING skips the buildDone emission (like BUILDING -> ERROR), and #reconcileServerState bails on SETTLING so the deferred-restart timer owns the SETTLING -> BUILDING transition. ServeLogger gains settling(pendingProjects) emitting serve-settling, and the interactive banner renders a spinning "settling · waiting for changes to settle" line.
The first build after a quiet period fired on BUILD_REQUEST_DEBOUNCE_MS (10 ms). On a multi-file operation the tree is half-written when the first watcher event lands, so the build fires into an incomplete tree and fails transiently. Give the first speculative build a 100 ms settle window (FIRST_BUILD_SETTLE_MS) instead. This absorbs an editor's multi-file save fan-out; 100 ms sits well below @parcel/watcher's 500 ms cap and near its 50 ms floor, so single-edit latency is barely affected. The window reports SETTLING. Applies only when a build is already pending (a reader request queued but not started); laziness is preserved, since with nothing queued a source change still waits for a reader request. An explicit reader request supersedes the window by re-arming the queue at the snappy debounce, so serving a request is never delayed. Kept separate from the SETTLING reporting change so a regression in single-edit latency can be bisected cleanly.
transitionTo() in state/build.js resets spinFrame to 0 on every transition, and no other serve-* branch sets it at the call site. Drop the redundant assignment and rely on the helper.
…chines architecture.md had fallen behind BuildServer.js. Rewrite the affected sections to match the implementation: - Replace the 3-state per-project sketch (INITIAL / INVALIDATED / FRESH) with the actual 6-state ProjectBuildStatus diagram, adding VALIDATING, BUILDING, and ERRORED, and note which transitions each state accepts. - Add a Server lifecycle section for SERVER_STATES (IDLE / STALE / BUILDING / VALIDATING / ERROR) driven by #setState and #reconcileServerState, including SETTLING between STALE and BUILDING and the BUILDING -> SETTLING edge. - Document background cache validation (#scheduleBackgroundValidation / #runBackgroundValidation) promoting INITIAL projects to FRESH, and the reader-request wait-on-validation path in #getReaderForProject. - Add a Startup section for BuildServer.create() awaiting WatchHandler readiness (the Windows ReadDirectoryChangesW race). - Expand Build Request Flow with the ERRORED short-circuit, the VALIDATING wait, #stopActiveValidation, markBuilding, the transient-vs-non-transient failure split, and FIRST_BUILD_SETTLE_MS. - Expand File Watch and Abort with fileAddedOrRemoved reader eviction, leading-edge sourcesChanged (SOURCES_CHANGED_SETTLE_MS), the deferred post-abort restart (ABORTED_BUILD_RESTART_SETTLE_MS), the first-build 100 ms window, and error clearing on invalidate. - Correct the WatchHandler entry from chokidar to @parcel/watcher and record its 50 ms / 500 ms coalescing wait behind the debounce values. - Add an Error gating section for ERRORED, #lastError, and the transient-error carve-out. - Point the BuildServer row in the component table at ProjectBuildStatus and SERVER_STATES.
…igation
While the build server is errored, a top-level HTML navigation now renders
an error page with the live-reload client attached instead of a plain-text
stack trace, so the tab reloads itself once the source is fixed. The
terminal error handler previously answered every failure with plain text,
losing the live-reload script serveResources.js injects, so a fixed source
emitted sourcesChanged but the tab had no client listening.
Content-negotiate in createErrorHandler: for a top-level HTML navigation,
render errorPage.html with the error message and stack HTML-escaped, and
embed INJECT_SCRIPT_TAG when liveReload.active. Non-HTML requests keep the
text/plain stack response. The script tag reuses the constant
serveResources.js injects, so there is one source of truth. On the next
sourcesChanged the client calls location.reload(), landing on the
recovered build or a fresh error page.
Negotiate on Sec-Fetch-Dest and an explicit text/html Accept rather than
req.accepts("html"): a wildcard Accept returns "html", which would serve
the HTML page to every failing subresource load and leave a <script> tag
to run the error page as JavaScript. Sec-Fetch-Dest ("document" only for
top-level navigations) wins when present; an explicit text/html Accept is
the fallback, so an older prefetch sending Accept: text/html for a
subresource still gets plain text. isDocumentNavigation is extracted to
helper/ so the errorHandler and the gate below share this logic. The
template is loaded once at module load via readFileSync, matching
serveIndex/directory.html.
Surface the page on any navigation while errored, not only when the
requested path maps to the failed project. Previously, navigating to the
app's index.html while a dependency library build was broken served a
normal 200. Add a serveBuildError gate middleware before serveResources:
for document navigations it consults the server-level error and diverts to
the errorHandler via next(err); subresource loads pass through and keep
their per-project behavior. The gate is a 3-argument middleware, since the
4-argument errorHandler is only reached once something calls next(err),
which a would-be-200 navigation never does.
- BuildServer: capture the error on entry to ERROR, clear it on every
other transition, expose it via getServeError(). Transient/aborted
builds report SETTLING, so getServeError() returns null during that
window.
- Thread getServeError from server.js through MiddlewareManager options.
The error handler hand-rolled a five-character escape map. @ui5/server already depends on escape-html and uses it in serveIndex.cjs; it escapes the same ["'&<>] set to identical strings. Import it and drop the local HTML_ESCAPES map and escapeHtml function.
The delta merge in recordTaskResult scanned writtenResourcePaths with Array.includes once per previous-stage resource — O(n*m) string comparisons on every incremental serve rebuild, scaling with a project's total written-resource count. Build a Set from the paths for the membership check; the array stays for the ordered downstream uses.
#setState already defaults the STALE and SETTLING project lists to #getStaleProjectNames(). Three call sites passed exactly that default explicitly. Drop the argument and rely on the ?? fallback inside #setState. The post-build STALE call keeps its argument — it forwards an already-computed set alongside hrtime.
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
2 participants
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.
No description provided.