Try once again to fix the element already registered bug (#290795)

Fixes microsoft/vscode-pull-request-github#8073

extensions/vscode-api-tests/src/singlefolder-tests/tree.test.ts

@@ -104,6 +104,114 @@ suite('vscode API - tree', () => {
 		}
 	});
 
+	test('TreeView - element already registered after rapid root refresh', async function () {
+		this.timeout(60_000);
+
+		// This test reproduces a race condition where rapid concurrent getChildren calls
+		// return different element object instances that have the same ID in their TreeItem,
+		// causing "Element with id ... is already registered" error.
+		//
+		// The bug: When _addChildrenToClear(undefined) is called, it clears _childrenFetchTokens.
+		// If two fetches are pending, both may reset the requestId counter to 1, so both think
+		// they are the current request. When both try to register elements with the same ID
+		// but different object instances, the error is thrown.
+
+		type TreeElement = { readonly kind: 'leaf'; readonly instance: number };
+
+		class RapidRefreshTreeDataProvider implements vscode.TreeDataProvider<TreeElement> {
+			private readonly changeEmitter = new vscode.EventEmitter<TreeElement | undefined>();
+			private readonly requestEmitter = new vscode.EventEmitter<number>();
+			private readonly pendingRequests: DeferredPromise<TreeElement[]>[] = [];
+			// Return different element instance each time
+			private element1: TreeElement = { kind: 'leaf', instance: 1 };
+			private element2: TreeElement = { kind: 'leaf', instance: 2 };
+
+			readonly onDidChangeTreeData = this.changeEmitter.event;
+
+			getChildren(element?: TreeElement): Thenable<TreeElement[]> {
+				if (!element) {
+					const deferred = new DeferredPromise<TreeElement[]>();
+					this.pendingRequests.push(deferred);
+					this.requestEmitter.fire(this.pendingRequests.length);
+					return deferred.p;
+				}
+				return Promise.resolve([]);
+			}
+
+			getTreeItem(): vscode.TreeItem {
+				// Both element instances return the same id
+				const item = new vscode.TreeItem('test element', vscode.TreeItemCollapsibleState.None);
+				item.id = 'same-id-each-time';
+				return item;
+			}
+
+			getParent(): TreeElement | undefined {
+				return undefined;
+			}
+
+			getElement1(): TreeElement {
+				return this.element1;
+			}
+
+			getElement2(): TreeElement {
+				return this.element2;
+			}
+
+			async waitForRequestCount(count: number): Promise<void> {
+				while (this.pendingRequests.length < count) {
+					await asPromise(this.requestEmitter.event);
+				}
+			}
+
+			resolveRequestWithElement(index: number, element: TreeElement): void {
+				const request = this.pendingRequests[index];
+				if (request) {
+					request.complete([element]);
+				}
+			}
+
+			dispose(): void {
+				this.changeEmitter.dispose();
+				this.requestEmitter.dispose();
+				while (this.pendingRequests.length) {
+					this.pendingRequests.shift()!.complete([]);
+				}
+			}
+		}
+
+		const provider = new RapidRefreshTreeDataProvider();
+		disposables.push(provider);
+
+		const treeView = vscode.window.createTreeView('test.treeRapidRefresh', { treeDataProvider: provider });
+		disposables.push(treeView);
+
+		// Start two concurrent reveal operations - this should trigger two getChildren calls
+		// Similar to the first test
+		const firstReveal = (treeView.reveal(provider.getElement1(), { expand: true })
+			.then(() => ({ error: undefined as Error | undefined })) as Promise<{ error: Error | undefined }>)
+			.catch(error => ({ error }));
+
+		const secondReveal = (treeView.reveal(provider.getElement2(), { expand: true })
+			.then(() => ({ error: undefined as Error | undefined })) as Promise<{ error: Error | undefined }>)
+			.catch(error => ({ error }));
+
+		// Wait for both getChildren calls to be pending
+		await provider.waitForRequestCount(2);
+
+		// Resolve requests returning DIFFERENT element instances with SAME id
+		// First request returns element1, second returns element2
+		// Both elements have the same id 'same-id-each-time' in getTreeItem
+		provider.resolveRequestWithElement(0, provider.getElement1());
+		await delay(0);
+		provider.resolveRequestWithElement(1, provider.getElement2());
+
+		const [firstResult, secondResult] = await Promise.all([firstReveal, secondReveal]);
+		const error = firstResult.error ?? secondResult.error;
+		if (error && /Element with id .+ is already registered/.test(error.message)) {
+			assert.fail(error.message);
+		}
+	});
+
 	test('TreeView - element already registered after refresh', async function () {
 		this.timeout(60_000);
 

src/vs/workbench/api/common/extHostTreeViews.ts

@@ -325,6 +325,10 @@ class ExtHostTreeView<T> extends Disposable {
 	// Track the latest child-fetch per element so that refresh-triggered cache clears ignore stale results.
 	// Without these tokens, an earlier getChildren promise resolving after refresh would re-register handles and hit the duplicate-id guard.
 	private readonly _childrenFetchTokens = new Map<T | typeof ExtHostTreeView.ROOT_FETCH_KEY, number>();
+	// Global counter for fetch tokens. Using a monotonically increasing counter ensures that even after
+	// _childrenFetchTokens.clear() during a root refresh, old in-flight fetches will have requestIds that
+	// can never match new fetches (e.g., old fetch has id=5, after clear new fetches get 6, 7, 8...).
+	private _globalFetchTokenCounter = 0;
 
 	private _visible: boolean = false;
 	get visible(): boolean { return this._visible; }
@@ -737,8 +741,7 @@ class ExtHostTreeView<T> extends Disposable {
 		// clear children cache
 		this._addChildrenToClear(parentElement);
 		const fetchKey = this._getFetchKey(parentElement);
-		let requestId = this._childrenFetchTokens.get(fetchKey) ?? 0;
-		requestId++;
+		const requestId = ++this._globalFetchTokenCounter;
 		this._childrenFetchTokens.set(fetchKey, requestId);
 
 		const cts = new CancellationTokenSource(this._refreshCancellationSource.token);