diff --git a/packages/project/lib/build/cache/ProjectBuildCache.js b/packages/project/lib/build/cache/ProjectBuildCache.js index b8d9eb2c44b..a1ae8ea523f 100644 --- a/packages/project/lib/build/cache/ProjectBuildCache.js +++ b/packages/project/lib/build/cache/ProjectBuildCache.js @@ -896,21 +896,38 @@ 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 ?? []); + // 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) { - if (!writtenResourcePaths.includes(res.getOriginalPath())) { - await stageWriter.write(res); - mergedCount++; + const path = res.getOriginalPath(); + if (writtenResourcePathSet.has(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)}`); + }); +