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"); });