refactor(fs): Improve duplicate step write error

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<string, object>} 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);
 		}

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

packages/fs/lib/internal/stepWriteBuffer.js

@@ -16,29 +16,56 @@
  *
  * The buffer is a <code>Map</code> keyed by resource path (or, for a test fake without
  * <code>getPath</code>, by identity). Each entry has the shape
- * <code>{resource, args, index}</code>: <code>resource</code> is the resource to write, <code>args</code>
- * are the trailing arguments to replay to <code>workspace.write(resource, ...args)</code>, and
- * <code>index</code> is the unit's position in the key order.
+ * <code>{resource, args, index, inputName}</code>: <code>resource</code> is the resource to write,
+ * <code>args</code> are the trailing arguments to replay to <code>workspace.write(resource, ...args)</code>,
+ * <code>index</code> is the unit's position in the key order, and <code>inputName</code> is its string key or
+ * map-key resource path, if available.
  */
 
+function describeInput({index, inputName}) {
+	return inputName === undefined ? `#${index + 1}` : `'${inputName}'`;
+}
+
+/**
+ * Derives the <code>inputName</code> 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 <code>getPath</code>) has no
+ * name, so the buffer entry and the error message fall back to the positional <code>#N</code> 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 <code>undefined</code> 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.
  *
  * A unit writing a path it buffered itself (same <code>index</code>) is allowed, so a unit may overwrite
  * its own earlier write within one run. A different unit (different <code>index</code>) writing the same
  * path throws, because concurrent keys must be independent.
  *
- * @param {Map<string, {resource: @ui5/fs/Resource, args: object[], index: number}>} buffer Shared write
- *   buffer, keyed by resource path
+ * @param {Map<string, {resource: @ui5/fs/Resource, args: object[], index: number,
+ *   inputName: string|undefined}>} 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 <code>index</code>, so the observable write sequence matches
  * the key order regardless of the order the concurrent units finished in.
  *
- * @param {Map<string, {resource: @ui5/fs/Resource, args: object[], index: number}>} buffer Shared write
- *   buffer, keyed by resource path
+ * @param {Map<string, {resource: @ui5/fs/Resource, args: object[], index: number,
+ *   inputName: string|undefined}>} buffer Shared write buffer, keyed by resource path
  * @param {@ui5/fs/AbstractReaderWriter} workspace Workspace the buffered writes are flushed to
  * @returns {Promise<undefined>} Resolves once every buffered write has been flushed
  */

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

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(),

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 {
 	 *   <code>null</code> 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: <code>true</code> (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

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