Fix shared-build admission and selection precedence

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: mojazayeri

libraries/rush-daemon/src/PhasedRequestRouter.ts

@@ -210,6 +210,7 @@ class PhasedRequestBatchCoordinator {
   #acceptingCurrentBatch: boolean = false;
   #currentBatch: ReadonlyArray<IBatchEntry> | undefined;
   #drainScheduled: boolean = false;
+  #nextGraphLeasePromise: Promise<IRequestLease> | undefined;
   #running: boolean = false;
 
   public constructor(
@@ -265,6 +266,9 @@ class PhasedRequestBatchCoordinator {
       return;
     }
     this.#drainScheduled = true;
+    this.#nextGraphLeasePromise = this.#graphExecutionScheduler.acquireAsync({
+      exclusivityClass: RequestExclusivityClass.Exclusive
+    });
     setImmediate(() => {
       this.#drainScheduled = false;
       void this.#drainAsync();
@@ -277,6 +281,13 @@ class PhasedRequestBatchCoordinator {
     }
     this.#running = true;
     try {
+      if (this.#pending.length === 0) {
+        const unusedGraphLeasePromise: Promise<IRequestLease> | undefined =
+          this.#nextGraphLeasePromise;
+        this.#nextGraphLeasePromise = undefined;
+        (await unusedGraphLeasePromise)?.release();
+        return;
+      }
       while (this.#pending.length > 0) {
         const first: IBatchEntry = this.#pending.shift()!;
         const batch: IBatchEntry[] = [first];
@@ -307,12 +318,14 @@ class PhasedRequestBatchCoordinator {
   }
 
   #canJoinCurrentBatch(request: IPreparedPhasedRequest): boolean {
+    if (request.exclusivityClass !== RequestExclusivityClass.SharedBuild) {
+      return false;
+    }
     if (!this.#running) {
       return true;
     }
     return (
       this.#acceptingCurrentBatch &&
-      request.exclusivityClass === RequestExclusivityClass.SharedBuild &&
       this.#currentBatch?.[0]?.exclusivityClass === RequestExclusivityClass.SharedBuild
     );
   }
@@ -331,9 +344,12 @@ class PhasedRequestBatchCoordinator {
   }
 
   async #executeBatchAsync(batch: IBatchEntry[]): Promise<void> {
-    const graphLeasePromise: Promise<IRequestLease> = this.#graphExecutionScheduler.acquireAsync({
-      exclusivityClass: RequestExclusivityClass.Exclusive
-    });
+    const graphLeasePromise: Promise<IRequestLease> =
+      this.#nextGraphLeasePromise ??
+      this.#graphExecutionScheduler.acquireAsync({
+        exclusivityClass: RequestExclusivityClass.Exclusive
+      });
+    this.#nextGraphLeasePromise = undefined;
     const graphLease: IRequestLease = await graphLeasePromise;
     try {
       if (this.#graph.hasScheduledIteration || this.#graph.status === OperationStatus.Executing) {
@@ -743,23 +759,33 @@ function applySelections(
   graph: IOperationGraph,
   selections: ReadonlyArray<IResolvedSelection>
 ): void {
-  graph.setEnabledStates(graph.operations, false, 'unsafe');
-  graph.setEnabledStates(
-    selections.flatMap(
-      (selection: IResolvedSelection) => selection.ignoreDependencyOperations
-    ),
-    'ignore-dependency-changes',
-    'safe'
+  const enabledOperations: ReadonlyArray<Operation> = selections.flatMap(
+    (selection: IResolvedSelection) => selection.enabledOperations
   );
-  graph.setEnabledStates(
-    selections.flatMap((selection: IResolvedSelection) => selection.enabledOperations),
-    true,
-    'safe'
+  const ignoreDependencyOperations: ReadonlyArray<Operation> = selections.flatMap(
+    (selection: IResolvedSelection) => selection.ignoreDependencyOperations
+  );
+  const enabledClosureBySelection: ReadonlyArray<ReadonlySet<Operation>> = selections.map(
+    (selection: IResolvedSelection) =>
+      new Set(collectSelectionClosure(selection.enabledOperations, []))
   );
+  const effectiveIgnoreDependencyOperations: Operation[] = [];
+  selections.forEach((selection: IResolvedSelection, selectionIndex: number) => {
+    for (const operation of selection.ignoreDependencyOperations) {
+      const requiredByAnotherSelection: boolean = enabledClosureBySelection.some(
+        (enabledClosure: ReadonlySet<Operation>, enabledSelectionIndex: number) =>
+          enabledSelectionIndex !== selectionIndex && enabledClosure.has(operation)
+      );
+      if (!requiredByAnotherSelection) {
+        effectiveIgnoreDependencyOperations.push(operation);
+      }
+    }
+  });
+  graph.setEnabledStates(graph.operations, false, 'unsafe');
+  graph.setEnabledStates(ignoreDependencyOperations, 'ignore-dependency-changes', 'safe');
+  graph.setEnabledStates(enabledOperations, true, 'safe');
   graph.setEnabledStates(
-    selections.flatMap(
-      (selection: IResolvedSelection) => selection.ignoreDependencyOperations
-    ),
+    effectiveIgnoreDependencyOperations,
     'ignore-dependency-changes',
     'unsafe'
   );

libraries/rush-daemon/src/test/PhasedRequestBatching.test.ts

@@ -364,6 +364,63 @@ describe('shared phased request batching', () => {
     expect(scheduleSpy).toHaveBeenCalledTimes(3);
   });
 
+  it('applies graph admission to same-turn shared-read requests', async () => {
+    const operationStarted: IDeferred = createDeferred();
+    const releaseOperation: IDeferred = createDeferred();
+    const fixture: ITestRoutingFixture = createFixture({
+      actionAAsync: async (): Promise<void> => {
+        operationStarted.resolve();
+        await releaseOperation.promise;
+      }
+    });
+    const router: PhasedRequestRouter = new PhasedRequestRouter(fixture.session);
+    const first = router.executeAsync(
+      { ...createRequest('first', OPERATION_A), commandName: 'list' },
+      new TestPhasedRequestClient('one')
+    );
+    const noWait = router.executeAsync(
+      {
+        ...createRequest('no-wait', OPERATION_C),
+        admission: { noWait: true },
+        commandName: 'list'
+      },
+      new TestPhasedRequestClient('two')
+    );
+
+    const noWaitResult: IDaemonPhasedRequestResult = await noWait;
+    expect(noWaitResult).toMatchObject({ admissionErrorCode: 'no-wait', outcome: 'failure' });
+    await operationStarted.promise;
+    releaseOperation.resolve();
+    await first;
+  });
+
+  it('keeps true enabled state dominant across merged selections', async () => {
+    const fixture: ITestRoutingFixture = createFixture();
+    const enabledStates: Array<boolean | 'ignore-dependency-changes' | undefined> = [];
+    fixture.graph.hooks.onIterationScheduled.tap('capture enabled state', () => {
+      enabledStates.push(fixture.operations.get(OPERATION_A)?.enabled);
+    });
+    const router: PhasedRequestRouter = new PhasedRequestRouter(fixture.session);
+
+    await Promise.all([
+      router.executeAsync(
+        {
+          ...createRequest('ignore-dependency', OPERATION_A),
+          operationSelection: [
+            { enabledState: 'ignore-dependency-changes', operationId: OPERATION_A }
+          ]
+        },
+        new TestPhasedRequestClient('one')
+      ),
+      router.executeAsync(
+        createRequest('requires-dependency', OPERATION_B),
+        new TestPhasedRequestClient('two')
+      )
+    ]);
+
+    expect(enabledStates).toEqual([true]);
+  });
+
   it('preserves per-client backpressure and final-result ordering in a merged batch', async () => {
     const fixture: ITestRoutingFixture = createFixture({
       actionAAsync: async (terminal: ITerminal): Promise<void> => {

libraries/rush-daemon/src/test/RequestAdmissionIntegration.test.ts

@@ -116,13 +116,14 @@ function createPhasedRequest(
 }
 
 function createLegacyPhasedRequest(requestId: string): IDaemonPhasedRequest {
-  return {
+  const legacyRequest: Partial<IDaemonPhasedRequest> = {
     commandName: 'build',
     engineShape: TEST_ENGINE_SHAPE,
     environment: {},
     operationSelection: [{ enabledState: true, operationId: TEST_OPERATION }],
     requestId
   };
+  return legacyRequest as IDaemonPhasedRequest;
 }
 
 function createDeferred(): { readonly promise: Promise<void>; readonly resolve: () => void } {