From c528aef343f514663a783b81c6a31cd98d2c9c29 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Fri, 16 Oct 2026 10:29:56 +0200 Subject: [PATCH] refactor(fs): Improve duplicate step write error When two concurrent map-step invocations write to the same path, name the step and task, both colliding invocations, and the shared path, so the message points at which inputs collided rather than only stating the rule. An invocation is named by its input (string key or resource path), or by position when the key has no name. See: https://github.com/UI5/cli/pull/1659 --- packages/builder/lib/tasks/runSteps.js | 23 +++++++-- packages/builder/test/lib/tasks/runSteps.js | 8 ++-- packages/fs/lib/internal/stepWriteBuffer.js | 47 +++++++++++++++---- .../fs/test/lib/internal/stepWriteBuffer.js | 23 +++++++-- packages/project/lib/build/TaskRunner.js | 2 + .../project/lib/build/helpers/StepRunner.js | 31 +++++++++--- .../test/lib/build/helpers/StepRunner.js | 10 ++-- 7 files changed, 112 insertions(+), 32 deletions(-) diff --git a/packages/builder/lib/tasks/runSteps.js b/packages/builder/lib/tasks/runSteps.js index 8f08c537b35..fd7134eedf0 100644 --- a/packages/builder/lib/tasks/runSteps.js +++ b/packages/builder/lib/tasks/runSteps.js @@ -1,5 +1,5 @@ import AbstractReaderWriter from "@ui5/fs/AbstractReaderWriter"; -import {assertDistinctWrite, flushWriteBuffer} from "@ui5/fs/internal/stepWriteBuffer"; +import {assertDistinctWrite, flushWriteBuffer, getInputName} from "@ui5/fs/internal/stepWriteBuffer"; /** * Buffers the writes of one concurrent map-step key and delegates reads to the underlying workspace, so @@ -12,17 +12,26 @@ class BufferedWriter extends AbstractReaderWriter { #workspace; #buffer; #index; + #inputName; + #stepName; + #taskName; /** * @param {@ui5/fs/AbstractReaderWriter} workspace Underlying workspace * @param {Map} buffer Shared write buffer, keyed by resource path * @param {number} index Key position, used to flush in key order and detect same-path writes + * @param {string|undefined} inputName String map key or resource path, if available + * @param {string} stepName Name of the map step + * @param {string|undefined} taskName Name of the task, if available */ - constructor(workspace, buffer, index) { + constructor(workspace, buffer, index, inputName, stepName, taskName) { super(typeof workspace.getName === "function" ? workspace.getName() : "workspace"); this.#workspace = workspace; this.#buffer = buffer; this.#index = index; + this.#inputName = inputName; + this.#stepName = stepName; + this.#taskName = taskName; } _byGlob(virPattern, options) { @@ -41,8 +50,8 @@ class BufferedWriter extends AbstractReaderWriter { // Real resources are keyed and deduplicated by their virtual path; a value without getPath // (a test fake) is keyed by identity so it still buffers and flushes in insertion order. const key = typeof resource.getPath === "function" ? resource.getPath() : resource; - assertDistinctWrite(this.#buffer, key, this.#index); - this.#buffer.set(key, {resource, args, index: this.#index}); + assertDistinctWrite(this.#buffer, key, this.#index, this.#inputName, this.#stepName, this.#taskName); + this.#buffer.set(key, {resource, args, index: this.#index, inputName: this.#inputName}); } } @@ -108,7 +117,11 @@ export default async function runSteps(build, {workspace, dependencies, taskUtil // Concurrent keys: buffer writes and flush them in key order once all keys finish. const buffer = new Map(); const results = await Promise.all(keys.map((key, index) => - step.each(key, {...context, workspace: new BufferedWriter(workspace, buffer, index)}))); + step.each(key, { + ...context, + workspace: new BufferedWriter( + workspace, buffer, index, getInputName(key), step.name, options?.taskName), + }))); await flushWriteBuffer(buffer, workspace); returns.set(step.name, results); } diff --git a/packages/builder/test/lib/tasks/runSteps.js b/packages/builder/test/lib/tasks/runSteps.js index 9f3f05bf482..1c0859fb2ee 100644 --- a/packages/builder/test/lib/tasks/runSteps.js +++ b/packages/builder/test/lib/tasks/runSteps.js @@ -109,15 +109,17 @@ test("A sequential map step makes an earlier key's write visible to a later key" test("Concurrent map-step keys writing the same path throw", async (t) => { const workspace = createWorkspace(); + const inputA = createResource("/resources/first.js"); + const inputB = createResource("/resources/second.js"); const err = await t.throwsAsync(runSteps(() => [ - {name: "m", keys: async () => ["a", "b"], each: async (key, {workspace}) => { + {name: "m", keys: async () => [inputA, inputB], each: async (key, {workspace}) => { await workspace.write(createResource("/same")); }}, ], {workspace})); // The exact user-visible message, shared with the cached runner through @ui5/fs/internal/stepWriteBuffer. t.is(err.message, - "Concurrent map-step keys must not write the same resource path /same. " + - "Pass {sequential: true} if a later key must build on an earlier key's writes.", + "Concurrent map step 'm' tried to write '/same' for invocations '/resources/first.js' and " + + "'/resources/second.js'. No two invocations may write to the same path.", "The same-path guard surfaces the shared message verbatim"); }); diff --git a/packages/fs/lib/internal/stepWriteBuffer.js b/packages/fs/lib/internal/stepWriteBuffer.js index c8a4d4a7437..304c8db16f6 100644 --- a/packages/fs/lib/internal/stepWriteBuffer.js +++ b/packages/fs/lib/internal/stepWriteBuffer.js @@ -16,11 +16,33 @@ * * The buffer is a Map keyed by resource path (or, for a test fake without * getPath, by identity). Each entry has the shape - * {resource, args, index}: resource is the resource to write, args - * are the trailing arguments to replay to workspace.write(resource, ...args), and - * index is the unit's position in the key order. + * {resource, args, index, inputName}: resource is the resource to write, + * args are the trailing arguments to replay to workspace.write(resource, ...args), + * index is the unit's position in the key order, and inputName is its string key or + * map-key resource path, if available. */ +function describeInput({index, inputName}) { + return inputName === undefined ? `#${index + 1}` : `'${inputName}'`; +} + +/** + * Derives the inputName for a map key: the key itself when it is a string, or its resource + * path when it is a resource. Any other key (for example a test fake without getPath) has no + * name, so the buffer entry and the error message fall back to the positional #N form. Both + * runners feed this to {@link assertDistinctWrite} and the buffer entry, so it lives here with the rest of + * the write-buffer contract to keep the two runners from drifting. + * + * @param {*} value Map key of the writing unit + * @returns {string|undefined} String key, resource path, or undefined if the key has no name + */ +export function getInputName(value) { + if (typeof value === "string") { + return value; + } + return value && typeof value.getPath === "function" ? value.getPath() : undefined; +} + /** * Rejects a second concurrent key writing a path another key already buffered. * @@ -28,17 +50,22 @@ * its own earlier write within one run. A different unit (different index) writing the same * path throws, because concurrent keys must be independent. * - * @param {Map} buffer Shared write - * buffer, keyed by resource path + * @param {Map} buffer Shared write buffer, keyed by resource path * @param {string} path Resource path being written, used as the buffer key * @param {number} index Position of the writing unit in the key order + * @param {string|undefined} inputName String map key or resource path of the writing unit, if available + * @param {string} stepName Name of the map step + * @param {string|undefined} taskName Name of the task, if available */ -export function assertDistinctWrite(buffer, path, index) { +export function assertDistinctWrite(buffer, path, index, inputName, stepName, taskName) { const existing = buffer.get(path); if (existing && existing.index !== index) { + const taskContext = taskName ? ` in task '${taskName}'` : ""; throw new Error( - `Concurrent map-step keys must not write the same resource path ${path}. ` + - `Pass {sequential: true} if a later key must build on an earlier key's writes.`); + `Concurrent map step '${stepName}'${taskContext} tried to write '${path}' for invocations ` + + `${describeInput(existing)} and ${describeInput({index, inputName})}. ` + + `No two invocations may write to the same path.`); } } @@ -48,8 +75,8 @@ export function assertDistinctWrite(buffer, path, index) { * The buffered entries are replayed sorted by index, so the observable write sequence matches * the key order regardless of the order the concurrent units finished in. * - * @param {Map} buffer Shared write - * buffer, keyed by resource path + * @param {Map} buffer Shared write buffer, keyed by resource path * @param {@ui5/fs/AbstractReaderWriter} workspace Workspace the buffered writes are flushed to * @returns {Promise} Resolves once every buffered write has been flushed */ diff --git a/packages/fs/test/lib/internal/stepWriteBuffer.js b/packages/fs/test/lib/internal/stepWriteBuffer.js index 5081dc9ab15..607ab423eb8 100644 --- a/packages/fs/test/lib/internal/stepWriteBuffer.js +++ b/packages/fs/test/lib/internal/stepWriteBuffer.js @@ -4,8 +4,9 @@ import {assertDistinctWrite, flushWriteBuffer} from "../../../lib/internal/stepW // The exact user-visible message. Both step runners surface this verbatim, so pin it here as the single // definition: a change to the shared message must update this assertion deliberately. const SAME_PATH_MESSAGE = - "Concurrent map-step keys must not write the same resource path /a.out. " + - "Pass {sequential: true} if a later key must build on an earlier key's writes."; + "Concurrent map step 'compile' in task 'buildResources' tried to write '/a.out' for invocations " + + "'/resources/first.js' and " + + "'/resources/second.js'. No two invocations may write to the same path."; test("assertDistinctWrite passes for an empty buffer", (t) => { t.notThrows(() => assertDistinctWrite(new Map(), "/a.out", 0)); @@ -18,11 +19,25 @@ test("assertDistinctWrite passes when the same unit overwrites its own buffered }); test("assertDistinctWrite throws the exact message when a different unit writes the same path", (t) => { - const buffer = new Map([["/a.out", {resource: {}, args: [], index: 0}]]); - const err = t.throws(() => assertDistinctWrite(buffer, "/a.out", 1)); + const buffer = new Map([["/a.out", { + resource: {}, + args: [], + index: 0, + inputName: "/resources/first.js", + }]]); + const err = t.throws(() => assertDistinctWrite( + buffer, "/a.out", 1, "/resources/second.js", "compile", "buildResources")); t.is(err.message, SAME_PATH_MESSAGE, "The user-visible message has one definition"); }); +test("assertDistinctWrite identifies non-resource invocations by position", (t) => { + const buffer = new Map([["/a.out", {resource: {}, args: [], index: 0}]]); + const err = t.throws(() => assertDistinctWrite(buffer, "/a.out", 1, undefined, "compile")); + t.is(err.message, + "Concurrent map step 'compile' tried to write '/a.out' for invocations #1 and #2. " + + "No two invocations may write to the same path."); +}); + test("flushWriteBuffer replays writes in key order regardless of insertion order", async (t) => { const written = []; const workspace = {write: async (resource, ...args) => written.push({path: resource.getPath(), args})}; diff --git a/packages/project/lib/build/TaskRunner.js b/packages/project/lib/build/TaskRunner.js index 8c6121865d6..0524647c5c7 100644 --- a/packages/project/lib/build/TaskRunner.js +++ b/packages/project/lib/build/TaskRunner.js @@ -349,6 +349,7 @@ class TaskRunner { const taskReport = this.#createTaskExecutionReport(taskName); const stepDriver = new StepRunner({ steps, + taskName, options, ...this.#createStepStageHooks(taskName, requiresDependencies), returnValueStore: this._buildCache.getStepReturnValueStore(), @@ -666,6 +667,7 @@ class TaskRunner { const taskReport = this.#createTaskExecutionReport(taskName); const stepDriver = new StepRunner({ steps, + taskName, options: factoryOptions, ...this.#createStepStageHooks(taskName, provideDependenciesReader, taskUtilInterface), returnValueStore: this._buildCache.getStepReturnValueStore(), diff --git a/packages/project/lib/build/helpers/StepRunner.js b/packages/project/lib/build/helpers/StepRunner.js index 0734ed922f8..d5149bdd831 100644 --- a/packages/project/lib/build/helpers/StepRunner.js +++ b/packages/project/lib/build/helpers/StepRunner.js @@ -1,7 +1,7 @@ import crypto from "node:crypto"; import AbstractReader from "@ui5/fs/AbstractReader"; import AbstractReaderWriter from "@ui5/fs/AbstractReaderWriter"; -import {assertDistinctWrite, flushWriteBuffer} from "@ui5/fs/internal/stepWriteBuffer"; +import {assertDistinctWrite, flushWriteBuffer, getInputName} from "@ui5/fs/internal/stepWriteBuffer"; import {getLogger} from "@ui5/logger"; import MonitoredTaskUtil from "./MonitoredTaskUtil.js"; @@ -181,6 +181,9 @@ class RecordingReaderWriter extends AbstractReaderWriter { #recorder; #writeBuffer; #invocationIndex; + #inputName; + #stepName; + #taskName; /** * @param {@ui5/fs/AbstractReaderWriter} workspace Task-level monitored workspace @@ -189,13 +192,19 @@ class RecordingReaderWriter extends AbstractReaderWriter { * null to write through immediately (sequential mode) * @param {number} invocationIndex Position of this invocation in the key order, used to flush buffered writes * deterministically and to detect two invocations writing the same path + * @param {string|undefined} inputName String map key or resource path, if available + * @param {string} stepName Name of the map step + * @param {string|undefined} taskName Name of the task, if available */ - constructor(workspace, recorder, writeBuffer, invocationIndex) { + constructor(workspace, recorder, writeBuffer, invocationIndex, inputName, stepName, taskName) { super(workspace.getName()); this.#workspace = workspace; this.#recorder = recorder; this.#writeBuffer = writeBuffer; this.#invocationIndex = invocationIndex; + this.#inputName = inputName; + this.#stepName = stepName; + this.#taskName = taskName; } async _byGlob(virPattern, options) { @@ -225,8 +234,14 @@ class RecordingReaderWriter extends AbstractReaderWriter { // Concurrent mode: buffer and flush in key order once all keys finish. Concurrent keys are // required to be independent, so two keys writing the same path is a contract violation // rather than a last-wins race. - assertDistinctWrite(this.#writeBuffer, resourcePath, this.#invocationIndex); - this.#writeBuffer.set(resourcePath, {resource, args: [options], index: this.#invocationIndex}); + assertDistinctWrite(this.#writeBuffer, resourcePath, this.#invocationIndex, this.#inputName, + this.#stepName, this.#taskName); + this.#writeBuffer.set(resourcePath, { + resource, + args: [options], + index: this.#invocationIndex, + inputName: this.#inputName, + }); return; } // Sequential mode: persist immediately so a later key reads what this key wrote. @@ -284,6 +299,7 @@ class RecordingReaderWriter extends AbstractReaderWriter { */ export default class StepRunner { #steps; + #taskName; #options; #prepareStage; #reopenStage; @@ -311,6 +327,7 @@ export default class StepRunner { * * @param {object} parameters * @param {object[]} [parameters.steps] Ordered step list from the task factory (factory path) + * @param {string} [parameters.taskName] Name of the task that owns the steps * @param {object} [parameters.options] Task options, passed through to each step's context * @param {function(string): Promise<(object|boolean)>} [parameters.prepareStage] Switches the project to * the named step's stage and returns its cache verdict: true (fully cached, do not run), @@ -357,10 +374,11 @@ export default class StepRunner { * @param {AbortSignal} [parameters.signal] Build abort signal, checked between invocations */ constructor({ - steps, options, prepareStage, recordStage, createStageContext, getPreviousInvocationData, + steps, taskName, options, prepareStage, recordStage, createStageContext, getPreviousInvocationData, returnValueStore, resolveInputValue, applyTagOperations, notifyStepExecution, signal, reopenStage }) { this.#steps = steps; + this.#taskName = taskName; this.#options = options; this.#prepareStage = prepareStage; this.#reopenStage = reopenStage; @@ -1124,7 +1142,8 @@ export default class StepRunner { const runInvocation = async ({key, keyId, index}) => { this.#signal?.throwIfAborted(); const recorder = new InvocationRecorder(); - const workspace = new RecordingReaderWriter(ctx.workspace, recorder, writeBuffer, index); + const workspace = new RecordingReaderWriter(ctx.workspace, recorder, writeBuffer, index, + getInputName(key), group, this.#taskName); const dependencies = ctx.dependencies ? new RecordingReader(ctx.dependencies, recorder) : undefined; // A per-invocation MonitoredTaskUtil wrapping the stage's taskUtil: it attributes the invocation's diff --git a/packages/project/test/lib/build/helpers/StepRunner.js b/packages/project/test/lib/build/helpers/StepRunner.js index 26b74b0cd0b..988618066d0 100644 --- a/packages/project/test/lib/build/helpers/StepRunner.js +++ b/packages/project/test/lib/build/helpers/StepRunner.js @@ -106,11 +106,12 @@ function createReturnValueStore() { function makeDriver({ steps, options, workspace = createWorkspace(), dependencies, taskUtil = {}, returnValueStore, resolveInputValue, applyTagOperations, signal, cacheVerdicts = {}, previousData = new Map(), - notifyStepExecution, + notifyStepExecution, taskName = "test-task", }) { const recorded = new Map(); const runner = new StepRunner({ steps, + taskName, options, returnValueStore, resolveInputValue, @@ -179,7 +180,7 @@ test("runSteps runs a map step's each once per enumerated key", async (t) => { test("Concurrent map-step keys writing the same path throw the shared message", async (t) => { const {runner} = makeDriver({ steps: [ - {name: "m", keys: async () => ["a", "b"], each: async (key, {workspace}) => { + {name: "m", keys: async () => ["first", "second"], each: async (key, {workspace}) => { await workspace.write(createResource("/same")); }}, ], @@ -188,8 +189,9 @@ test("Concurrent map-step keys writing the same path throw the shared message", const err = await t.throwsAsync(runner.runSteps()); // The exact user-visible message, shared with the uncached runner through @ui5/fs/internal/stepWriteBuffer. t.is(err.message, - "Concurrent map-step keys must not write the same resource path /same. " + - "Pass {sequential: true} if a later key must build on an earlier key's writes.", + "Concurrent map step 'm' in task 'test-task' tried to write '/same' " + + "for invocations 'first' and 'second'. " + + "No two invocations may write to the same path.", "The same-path guard surfaces the shared message verbatim"); });