From d03aa42b849880022314892f079f39443e39174d Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Tue, 21 Jul 2026 14:09:22 +0200 Subject: [PATCH 1/2] refactor(project): Drop resurrected resources from delta merge 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 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 not flagged changed merge as before. Track the dropped count alongside the merged count in the perf log. Add four fail-then-succeed tests 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. Clearing the set cleanly needs a separate fail/abort hook, since #initSourceIndex seeds the array with delta paths on the first build after loading persistent cache. 2. #currentStageSignatures reflect only the retry's stages. 3. The delta merge does not resurrect a deleted-source resource. 4. #frozenSourceReader is nulled by the retry's initStages call before task inputs are computed. --- .../lib/build/cache/ProjectBuildCache.js | 22 +- .../test/lib/build/cache/ProjectBuildCache.js | 219 ++++++++++++++++++ 2 files changed, 237 insertions(+), 4 deletions(-) diff --git a/packages/project/lib/build/cache/ProjectBuildCache.js b/packages/project/lib/build/cache/ProjectBuildCache.js index b8d9eb2c44b..0b05f8b36e0 100644 --- a/packages/project/lib/build/cache/ProjectBuildCache.js +++ b/packages/project/lib/build/cache/ProjectBuildCache.js @@ -896,21 +896,35 @@ export default class ProjectBuildCache { reader = cacheInfo.previousStageCache.stage.getWriter() ?? cacheInfo.previousStageCache.stage.getCachedWriter(); } + // Paths flagged changed but not re-emitted by the delta task: their source + // is gone or excluded, so replaying the previous stage's copy would + // resurrect content that no longer belongs in the output. + const changedProjectResourcePaths = new Set(cacheInfo.changedProjectResourcePaths ?? []); const mergeStart = performance.now(); const previousWrittenResources = await reader.byGlob("/**/*"); let mergedCount = 0; + let droppedCount = 0; for (const res of previousWrittenResources) { - if (!writtenResourcePaths.includes(res.getOriginalPath())) { - await stageWriter.write(res); - mergedCount++; + const path = res.getOriginalPath(); + if (writtenResourcePaths.includes(path)) { + continue; // Delta re-emitted this path; skip } + if (changedProjectResourcePaths.has(path)) { + // Flagged changed but not written back by the delta task. + // Drop the stale copy from the merge. + droppedCount++; + continue; + } + await stageWriter.write(res); + mergedCount++; } if (log.isLevelEnabled("perf")) { log.perf( `recordTaskResult delta merge for task ${taskName} ` + `in project ${this.#project.getName()} completed in ` + `${(performance.now() - mergeStart).toFixed(2)} ms ` + - `(${previousWrittenResources.length} previous, ${mergedCount} merged)`); + `(${previousWrittenResources.length} previous, ${mergedCount} merged, ` + + `${droppedCount} dropped)`); } } else { // Calculate signature for executed task diff --git a/packages/project/test/lib/build/cache/ProjectBuildCache.js b/packages/project/test/lib/build/cache/ProjectBuildCache.js index 2487c062419..ce598b057eb 100644 --- a/packages/project/test/lib/build/cache/ProjectBuildCache.js +++ b/packages/project/test/lib/build/cache/ProjectBuildCache.js @@ -1952,3 +1952,222 @@ test("validateCache: Cache.Force throws when source changes are detected", async ); t.regex(err.message, /Force.*mode.*stale/i, "Error message mentions Force mode and stale cache"); }); + +// ===== FAIL-THEN-SUCCEED (BuildServer error-recovery) REPRO TESTS ===== +// +// State that persists across build attempts on one ProjectBuildCache instance, +// the shape a long-running `ui5 serve` hits when a task throws mid-build (e.g. a +// LESS syntax error) and a later source edit fixes it. Reported symptom: "server +// serves corrupted resources from the failed build after fix". `StageCache#discardPending` +// (called from `validateCache({prepareForBuild:true})`) already scrubs the pending +// stage cache entries the failed attempt added. The remaining risk is state that +// outlives that scrub: `#writtenResultResourcePaths`, `#currentStageSignatures` and +// the delta-merge input. +// +// Each test drives a successful build, a second attempt where task B "throws" +// (modeled by omitting its `recordTaskResult` call), and a retry. Assertions +// inspect the arguments passed to the taskCache / projectResources mocks on the retry. + +// Points the project's getStage mock at a stage with the given id whose writer +// returns `written` from byGlob. Pass `write` to capture merge writes. +function stubStage(project, stageId, {written = [], write} = {}) { + const writer = {byGlob: sinon.stub().resolves(written)}; + if (write) { + writer.write = write; + } + project.getProjectResources().getStage.returns({ + getId: () => stageId, + getWriter: sinon.stub().returns(writer), + }); +} + +// Records a task result with empty project/dependency request sets. +function recordEmptyResult(cache, taskName, cacheInfo = null, isDelta = false) { + return cache.recordTaskResult( + taskName, {paths: new Set(), patterns: new Set()}, + {paths: new Set(), patterns: new Set()}, cacheInfo, isDelta); +} + +test("Fail-then-succeed: #writtenResultResourcePaths accumulates across failed attempts (documented behavior)", + async (t) => { + // After taskA records in a failed build and taskB throws, /a.js is left in + // #writtenResultResourcePaths. On retry, prepareTaskExecutionAndValidateCache + // calls taskCache.updateProjectIndices(reader, #writtenResultResourcePaths). + // + // Benign in practice: updateProjectIndices re-fetches each path through the + // retry's fresh reader and re-hashes. Unchanged content yields the same + // signature and no invalidation; changed content invalidates correctly. The + // impact is redundant I/O on the retry, not wrong output. This test pins the + // behavior so a later refactor that removes the leak can update it. + const project = createMockProject(); + const cacheManager = createMockCacheManager(); + + const initialA = createMockResource("/a.js", "hash-a", 1000, 100, 1); + const initialB = createMockResource("/b.js", "hash-b", 1000, 100, 2); + project.getSourceReader.callsFake(() => ({ + byGlob: sinon.stub().resolves([initialA, initialB]), + byPath: sinon.stub().callsFake((p) => { + return Promise.resolve(p === "/a.js" ? initialA : p === "/b.js" ? initialB : null); + }) + })); + + const cache = await ProjectBuildCache.create(project, "sig", cacheManager); + await cache.initSourceIndex(); + + const mockDependencyReader = { + byGlob: sinon.stub().resolves([]), + byPath: sinon.stub().resolves(null), + }; + + // Failed build attempt: taskA runs successfully and writes /a.js. + cache.setTasks(["taskA", "taskB"]); + await cache.prepareTaskExecutionAndValidateCache("taskA"); + + const writtenA = createMockResource("/a.js", "hash-a-built", 2000, 200, 1); + stubStage(project, "task/taskA", {written: [writtenA]}); + await recordEmptyResult(cache, "taskA"); + + // taskB "throws": no recordTaskResult call. The failed build leaves + // #writtenResultResourcePaths containing ["/a.js"] since allTasksCompleted + // (which would clear it) never runs. + + // Retry: BuildServer calls validateCache({prepareForBuild:true}) again. + await cache.validateCache(mockDependencyReader, {prepareForBuild: true}); + + // The retry claims taskA again. Currently /a.js is passed as a "changed" + // path to updateProjectIndices even though it did not change on disk. + cache.setTasks(["taskA", "taskB"]); + const updateProjectIndicesStub = sinon.stub( + cache.getTaskCache("taskA"), "updateProjectIndices").resolves(); + + stubStage(project, "task/taskA"); + await cache.prepareTaskExecutionAndValidateCache("taskA"); + + t.true(updateProjectIndicesStub.called, + "updateProjectIndices is called on retry with the leaked paths"); + const changedPaths = updateProjectIndicesStub.firstCall.args[1]; + t.true(changedPaths.includes("/a.js"), + `Documented behavior: retry passes /a.js to updateProjectIndices even ` + + `though it did not change between attempts (changed paths: ${JSON.stringify(changedPaths)}). ` + + `Re-hashing through the retry's fresh reader means this does not produce ` + + `incorrect output, only redundant work.`); + }); + +test("Fail-then-succeed: #currentStageSignatures from failed attempt does not linger", + async (t) => { + // After a failed build, #currentStageSignatures contains the entry the + // completed task wrote. On retry, if the source signature happens to match + // what the failed build indexed at that stage, #findResultCache / + // #getResultStageSignature could combine the stale entry with the fresh one + // and either short-circuit unnecessarily or produce a spurious cache hit. + // Assert #getResultStageSignature (via allTasksCompleted -> cache write) + // reflects only what the *successful* retry recorded. + const project = createMockProject(); + const cacheManager = createMockCacheManager(); + + const src = createMockResource("/test.js", "hash-src", 1000, 100, 1); + project.getSourceReader.callsFake(() => ({ + byGlob: sinon.stub().resolves([src]), + byPath: sinon.stub().resolves(src) + })); + + const cache = await ProjectBuildCache.create(project, "sig", cacheManager); + await cache.initSourceIndex(); + + const mockDependencyReader = { + byGlob: sinon.stub().resolves([]), + byPath: sinon.stub().resolves(null), + }; + await cache.validateCache(mockDependencyReader, {prepareForBuild: true}); + + // Failed attempt: taskA records with a distinctive signature. + cache.setTasks(["taskA", "taskB"]); + await cache.prepareTaskExecutionAndValidateCache("taskA"); + stubStage(project, "task/taskA"); + await recordEmptyResult(cache, "taskA"); + + // taskB "throws": build fails. + + // Retry. + await cache.validateCache(mockDependencyReader, {prepareForBuild: true}); + cache.setTasks(["taskA", "taskB"]); + + await cache.prepareTaskExecutionAndValidateCache("taskA"); + stubStage(project, "task/taskA"); + await recordEmptyResult(cache, "taskA"); + + await cache.prepareTaskExecutionAndValidateCache("taskB"); + stubStage(project, "task/taskB"); + await recordEmptyResult(cache, "taskB"); + + await cache.allTasksCompleted(); + await cache.writeCache(); + + // Inspect the result metadata that was written. It must reference exactly + // the two stages the successful retry recorded, no more and no less. + const resultMetadataCalls = cacheManager.writeResultMetadata.getCalls(); + t.is(resultMetadataCalls.length, 1, "One result metadata write"); + const {stageSignatures} = resultMetadataCalls[0].args[3]; + const stageNames = Object.keys(stageSignatures); + t.deepEqual(stageNames.sort(), ["task/taskA", "task/taskB"], + "Result metadata references exactly the two retry stages, no lingering entries"); + }); + +test("Fail-then-succeed: delta merge does not resurrect resources from a stage a failed attempt wrote", + async (t) => { + // In build 1, taskA records a stage that contains /a.js and /b.js. + // Build 1 completes; result cache is persisted. + // Build 2 (failed): source /b.js is deleted; taskA runs, taskB throws. + // Build 3 (retry): source /b.js is still gone. taskA runs in delta mode + // with cacheInfo.previousStageCache pointing at build 1's stage entry. + // The delta merge at recordTaskResult (line 900-907) reads previousStageCache + // and writes every resource not overlaid by the current delta. If it merges + // the stale /b.js, /b.js becomes visible in the retry's output even though + // the source file no longer exists. + const project = createMockProject(); + const cacheManager = createMockCacheManager(); + const cache = await ProjectBuildCache.create(project, "sig", cacheManager); + await cache.initSourceIndex(); + + cache.setTasks(["deltaTask"]); + await cache.prepareTaskExecutionAndValidateCache("deltaTask"); + + // The retry's delta task writes only the changed /a.js. + const retryA = createMockResource("/a.js", "hash-a-new", 3000, 300, 1); + const writeStub = sinon.stub().resolves(); + stubStage(project, "task/deltaTask", {written: [retryA], write: writeStub}); + + // previousStageCache reflects the state before the failed build: /a.js AND /b.js. + // The retry drops /b.js (source deleted). If the delta merge blindly + // replays every previous-stage resource, /b.js survives in the retry output. + const prevA = createMockResource("/a.js", "hash-a-old", 1000, 100, 1); + const prevB = createMockResource("/b.js", "hash-b-old", 1000, 100, 2); + const cacheInfo = { + previousStageCache: { + signature: "prev-proj-prev-dep", + stage: { + byGlob: sinon.stub().resolves([prevA, prevB]), + }, + writtenResourcePaths: ["/a.js", "/b.js"], + projectTagOperations: undefined, + buildTagOperations: undefined, + }, + newSignature: "new-proj-new-dep", + // changedProjectResourcePaths tells the delta task which paths changed. + // /b.js was deleted, so it changed, but the delta task didn't write + // its replacement (it can't; the file is gone). + changedProjectResourcePaths: ["/a.js", "/b.js"], + changedDependencyResourcePaths: [], + }; + + await recordEmptyResult(cache, "deltaTask", cacheInfo, true); + + // Without the fix, the merge writes every previous resource whose path is not + // in the delta's writtenResourcePaths. /b.js is not overlaid, so it gets + // written: the deleted file is resurrected, the bug shape reported by the user. + const writtenPaths = writeStub.getCalls().map((c) => c.args[0].getOriginalPath()); + t.false(writtenPaths.includes("/b.js"), + `LEAK: /b.js was merged into the retry stage from the previous stage cache ` + + `even though its source is gone. Written paths: ${JSON.stringify(writtenPaths)}`); + }); + From 0843b5ca9f02386ba7956916d10551290805db34 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Tue, 21 Jul 2026 14:13:55 +0200 Subject: [PATCH 2/2] refactor(project): Use a Set for the delta-merge membership check 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 for the membership check; the array stays for the ordered downstream uses. --- packages/project/lib/build/cache/ProjectBuildCache.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/project/lib/build/cache/ProjectBuildCache.js b/packages/project/lib/build/cache/ProjectBuildCache.js index 0b05f8b36e0..a1ae8ea523f 100644 --- a/packages/project/lib/build/cache/ProjectBuildCache.js +++ b/packages/project/lib/build/cache/ProjectBuildCache.js @@ -900,13 +900,16 @@ export default class ProjectBuildCache { // is gone or excluded, so replaying the previous stage's copy would // resurrect content that no longer belongs in the output. const changedProjectResourcePaths = new Set(cacheInfo.changedProjectResourcePaths ?? []); + // Set form for the membership check below; the array is retained for the + // ordered downstream uses (recordStageCache, verbose counts). + const writtenResourcePathSet = new Set(writtenResourcePaths); const mergeStart = performance.now(); const previousWrittenResources = await reader.byGlob("/**/*"); let mergedCount = 0; let droppedCount = 0; for (const res of previousWrittenResources) { const path = res.getOriginalPath(); - if (writtenResourcePaths.includes(path)) { + if (writtenResourcePathSet.has(path)) { continue; // Delta re-emitted this path; skip } if (changedProjectResourcePaths.has(path)) {