From 4dc4dfa3b8e3ecf16591f2d9ee4704cbcb234048 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Wed, 30 Sep 2026 15:24:54 +0200 Subject: [PATCH] test(project): Reproduce abort/retry delta over-write minimally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a minimal reproduction of the build-server abort/retry over-write. A delta rebuild only re-processes the files that actually changed. When such a rebuild is aborted mid-flight and restarted, the retry re-processes ALL files the task handles instead of just the changed one. The served output stays correct (the retry writes the same bytes), so this is an optimization gap — redundant writes of unchanged files — rather than a correctness fault. The replaceCopyright task substitutes ${copyright} in every file that contains it; the library.d fixture has two such files (some.js and .library). The test runs three builds: 1. Build once so the caches are warm. 2. A clean delta rebuild that changes only some.js. replaceCopyright writes only some.js. This already passes today. 3. The same delta, but aborted mid-build and retried. The retry should again write only some.js, but today it also re-writes .library. Steps 2 and 3 use the same skipped-task set and expect the same written delta, so the aborted build differs from the clean one only in the replaceCopyright written set. That isolates the abort as the sole cause of the over-write. Both are asserted via the FixtureTester writtenResources assertion. To drive the mid-build abort, requestResource gains an onBuildStatus hook that is attached for the duration of the request, so tests no longer hand-roll the process.on/off dance around byPath. _assertBuild dedupes the skipped-task list because an abort+retry emits one task-skip event per attempt. Marked test.serial.failing until the abort/retry delta is optimized to re-process only the changed file. JIRA: CPOUI5FOUNDATION-1363 --- .../BuildServer.abortRetry.integration.js | 150 +++++++++++++++--- .../__helper__/BuildServerFixtureTester.js | 22 ++- 2 files changed, 146 insertions(+), 26 deletions(-) diff --git a/packages/project/test/lib/build/BuildServer.abortRetry.integration.js b/packages/project/test/lib/build/BuildServer.abortRetry.integration.js index 82027865a17..9571d402a5e 100644 --- a/packages/project/test/lib/build/BuildServer.abortRetry.integration.js +++ b/packages/project/test/lib/build/BuildServer.abortRetry.integration.js @@ -52,14 +52,12 @@ test.serial("Aborted initial build must not leak in-memory StageCache to retry", project, "/resources/library/d/some.js", false); } }; - process.on("ui5.project-build-status", abortHandler); - try { - // byPath returns once the retry succeeds, so all events for both attempts are captured. - await fixtureTester._reader.byPath("/resources/library/d/some.js"); - } finally { - process.off("ui5.project-build-status", abortHandler); - } + // requestResource returns once the retry succeeds, so all events for both attempts are captured. + await fixtureTester.requestResource({ + resource: "/resources/library/d/some.js", + onBuildStatus: abortHandler, + }); t.true(aborted, "Test setup precondition: abort trigger should have fired"); @@ -99,13 +97,11 @@ test.serial( project, "/resources/library/d/some.js", false); } }; - process.on("ui5.project-build-status", abortHandler); - try { - await fixtureTester._reader.byPath("/resources/library/d/some.js"); - } finally { - process.off("ui5.project-build-status", abortHandler); - } + await fixtureTester.requestResource({ + resource: "/resources/library/d/some.js", + onBuildStatus: abortHandler, + }); t.true(aborted, "Test setup precondition: abort trigger should have fired"); @@ -175,16 +171,13 @@ test.serial("Source change during second build retries cleanly without no_cache appendFileSync(changedFilePath, "\n// mid-build-2 change\n"); } }; - process.on("ui5.project-build-status", handler); - - let resource; - try { - // Without the fix this rejects with - // "Unexpected result cache state after restoring dependency indices for project XYZ: no_cache". - resource = await fixtureTester._reader.byPath("/resources/library/d/some.js"); - } finally { - process.off("ui5.project-build-status", handler); - } + + // Without the fix this rejects with + // "Unexpected result cache state after restoring dependency indices for project XYZ: no_cache". + const resource = await fixtureTester.requestResource({ + resource: "/resources/library/d/some.js", + onBuildStatus: handler, + }); t.true(triggered, "Test setup precondition: source change handler fired during build 2"); @@ -194,3 +187,114 @@ test.serial("Source change during second build retries cleanly without no_cache t.true(servedContent.includes("mid-build-2 change"), "Retry served content reflecting the mid-build-2 change"); }); + +// CPOUI5FOUNDATION-1363 (abort/retry delta over-write — OPTIMIZATION, not a correctness bug): +// A delta rebuild only re-processes the files that actually changed. When such a rebuild is aborted +// mid-flight and restarted, the retry re-processes ALL files the task handles instead of just the +// changed one. The served output is still correct — the retry writes the same bytes — so this only +// wastes work. Closing the gap would let the retry write the same reduced delta as the aborted +// attempt. +// +// The `replaceCopyright` task substitutes the `${copyright}` token in every file that contains it. +// The library.d fixture has two such files: `some.js` and `.library`. In a delta rebuild that +// changed only `some.js`, the task writes only `some.js`. This test first proves that a *clean* +// (non-aborted) delta rebuild already writes only `some.js` today, then drives an aborted+retried +// delta rebuild and checks the retry still writes only `some.js` — today it writes `.library` too, +// because the reused in-memory state loses the delta and re-globs all files. The clean delta phase +// isolates the abort as the sole cause of the over-write. +// +// Recipe: +// 1. Build once so the caches are warm. +// 2. Change only `some.js`. This clean delta rebuild writes only `some.js` for replaceCopyright +// (asserted — passes today). +// 3. Change only `some.js` again, and change it a THIRD time mid-build so the build aborts and +// restarts. The retry should again write only `some.js`, but today it also re-writes `.library` +// (asserted — fails today). +// +// Marked test.serial.failing: AVA reports it as a pass while it fails and as an error once it +// starts passing, so it keeps CI green today and turns into a signal once the retry is optimized +// (then drop the `.failing`). The clean delta assertion in step 2 already passes; the test as a +// whole still fails on step 3 until the retry is optimized. +test.serial.failing( + "Aborted delta build should re-process only the changed file on retry (optimization)", async (t) => { + const fixtureTester = t.context.fixtureTester = await FixtureTester.create(t, "library.d"); + + await fixtureTester.serveProject({config: {excludedTasks: ["minify"]}}); + const project = fixtureTester.graph.getProject("library.d"); + const somePath = `${fixtureTester.fixturePath}/main/src/library/d/some.js`; + const originalSome = await fs.readFile(somePath, {encoding: "utf8"}); + + // #1 initial build populates the caches and records each task's inputs. + await fixtureTester.requestResource({resource: "/resources/library/d/some.js"}); + + // #2 clean delta rebuild (no abort). Change ONLY some.js and notify the watcher. This is the + // baseline the aborted retry must match: replaceCopyright writes only some.js, never .library. + // This assertion already succeeds in the current codebase. + await fs.writeFile(somePath, `${originalSome}\n// v2\n`); + await fixtureTester.fireWatcherEvent("update", somePath); + await fixtureTester.requestResource({ + resource: "/resources/library/d/some.js", + assertions: { + projects: { + "library.d": { + skippedTasks: [ + "buildThemes", "enhanceManifest", "escapeNonAsciiCharacters", "replaceBuildtime", + ], + writtenResources: { + replaceCopyright: ["/resources/library/d/some.js"], + }, + }, + }, + }, + }); + + // #3 aborted delta rebuild. Change ONLY some.js and notify the watcher, then abort mid-build. + await fs.writeFile(somePath, `${originalSome}\n// v3\n`); + await fixtureTester.fireWatcherEvent("update", somePath); + + // One-shot abort trigger: when `replaceCopyright` ends during the delta rebuild, change + // `some.js` AGAIN on disk and route the change through _projectResourceChanged. This + // invalidates the project, aborts the running build at the next signal check, and + // re-enqueues it against the newest source. + let aborted = false; + const abortHandler = (event) => { + if ( + !aborted && + event.projectName === "library.d" && + event.status === "task-end" && + event.taskName === "replaceCopyright" + ) { + aborted = true; + fs.writeFile(somePath, `${originalSome}\n// v4\n`); + fixtureTester.buildServer._projectResourceChanged( + project, "/resources/library/d/some.js", false); + } + }; + + // requestResource returns once the retry succeeds, so all events for both attempts are + // captured. The retry should write only the changed file's delta for replaceCopyright: + // only some.js changed, so the delta must not include the unchanged .library file. The + // writtenResources assertion checks the LAST task-end for the task (i.e. the retry's). + // The skippedTasks set is identical to the clean delta #2, so this build differs from the + // passing baseline ONLY in the replaceCopyright written set — isolating the abort as the + // sole cause. This is the assertion that fails today. + await fixtureTester.requestResource({ + resource: "/resources/library/d/some.js", + onBuildStatus: abortHandler, + assertions: { + projects: { + "library.d": { + skippedTasks: [ + "buildThemes", "enhanceManifest", "escapeNonAsciiCharacters", "replaceBuildtime", + ], + writtenResources: { + replaceCopyright: ["/resources/library/d/some.js"], + }, + }, + }, + }, + }); + + t.true(aborted, "Test setup precondition: abort trigger fired mid-build"); + } +); diff --git a/packages/project/test/lib/build/__helper__/BuildServerFixtureTester.js b/packages/project/test/lib/build/__helper__/BuildServerFixtureTester.js index 0e9c87ad857..7d9d8b25312 100644 --- a/packages/project/test/lib/build/__helper__/BuildServerFixtureTester.js +++ b/packages/project/test/lib/build/__helper__/BuildServerFixtureTester.js @@ -223,9 +223,23 @@ class FixtureTester { this._reader = this.buildServer.getReader(); } - async requestResource({resource, notFound = false, assertions}) { + // `onBuildStatus`, when provided, is attached to the `ui5.project-build-status` process event for + // the duration of the `byPath` call and detached in a `finally`. It lets a test inject a change + // mid-build (e.g. to drive an abort/retry) without hand-rolling the process.on/off dance around + // the request. The handler is registered AFTER resetHistory so it never sees stale events. + async requestResource({resource, notFound = false, assertions, onBuildStatus}) { this._sinon.resetHistory(); - const res = await this._reader.byPath(resource); + if (onBuildStatus) { + process.on("ui5.project-build-status", onBuildStatus); + } + let res; + try { + res = await this._reader.byPath(resource); + } finally { + if (onBuildStatus) { + process.off("ui5.project-build-status", onBuildStatus); + } + } if (notFound) { this._t.is(res, null, `Resource '${resource}' must not be served`); } else { @@ -335,7 +349,9 @@ class FixtureTester { // Assert skipped tasks and written resources per project for (const [projectName, expected] of Object.entries(projects)) { const skippedTasks = expected.skippedTasks || []; - const actualSkipped = (tasksByProject[projectName]?.skipped || []).sort(); + // Dedupe: an abort+retry within a single request window emits a task-skip event per + // attempt, so the same task can appear twice. "Skipped" is a set — assert it as one. + const actualSkipped = [...new Set(tasksByProject[projectName]?.skipped || [])].sort(); const expectedArray = skippedTasks.sort(); this._t.deepEqual(actualSkipped, expectedArray);