[dfederm]

Scheduler.GetExecutingRequestByNode read the node's executing request twice:

if (!_schedulingData.IsNodeWorking(nodeId))
{
    return null;
}

SchedulableRequest request = _schedulingData.GetExecutingRequestByNode(nodeId);
return request.BuildRequest;

When a request leaves the Executing state the scheduler stores an explicit null against its node (SchedulingData.cs:315), so those two reads can disagree. Every other caller of the scheduler holds the BuildManager lock; FileAccessManager does not. With /reportfileaccesses, file accesses are reported on the thread draining the sandbox's report pipe, which races the scheduler freely, and the resulting NullReferenceException is unhandled on a threadpool thread, so it takes the whole build down. Reported as microsoft/MSBuildCache#160.

The fix looks the request up once and lets it be null. FileAccessManager.ReportFileAccess and ReportProcess already guard with if (buildRequest != null) and drop the access when the node has no executing request, which is the documented expectation there ("If the node isn't executing anything it may be accessing binaries required to run"). So the only behavior change is crash-to-drop, and that drop is the same outcome the race would have produced had it resolved a moment either side. Nothing is dropped that wasn't already. The other two test-then-fetch reads of that map, the Assumed in UpdateFromState and DumpSchedulerState, are folded into single lookups as well, so the map has no read left that can throw.

_executingRequestByNode also becomes a ConcurrentDictionary. That part is independent of the null: new Dictionary<>(32) resizes once a build has more than 32 nodes, and reading a resizing Dictionary from another thread gives an IndexOutOfRangeException or a corrupt walk rather than a stale value.

On cost, the only operation that gets slower is the write: +15ns on net472 and +12ns on net10.0, with no allocation on either (microbenchmark, 2M ops, median of 11 runs). Writes happen only in SchedulingData.UpdateFromState, once entering and once leaving Executing, so they are bounded by scheduler state transitions, on the order of tens of thousands for a large build and well under a millisecond in total. ChangeToState already spends more than that per transition on its DateTime.UtcNow (33ns on net472, 17ns on net10.0). The high-volume operation here is the read, since GetExecutingRequestByNode runs once per file access when /reportfileaccesses is on, and reads measured unchanged on net472 and about 30% faster on net10.0.

Testing: GetExecutingRequestByNodeToleratesConcurrentRequestCompletion is the regression guard. It reproduces the reported NullReferenceException at the same frame against unfixed binaries, and asserts that the reader observed both a live request and a cleared node so it cannot go vacuous. GetExecutingRequestByNodeReflectsTheNodesCurrentRequest pins the contract but passes at baseline, so it does not catch the bug. Full unit test suite on net472 and net11.0: one test flips fail-to-pass, no regressions.

I could not reproduce the original crash end to end (43 runs of a 2000-project build against unfixed binaries, zero hits), so the link to the field report is the matching stack frame plus the source analysis above rather than a customer repro.

FileAccessManager has two more reads of this shape that I left alone: _tempDirectory at FileAccessManager.cs:71 and _scheduler at :164/:167, both nulled by ShutdownComponent. Same failure mode, much narrower window. I can fold them in if you'd prefer.

[Copilot]

Pull request overview

Fixes a race in scheduler state lookup that could crash builds when /reportfileaccesses is enabled by making “executing request by node” retrieval tolerant of concurrent request completion, and adding regression coverage for the scenario.

Changes:

• Make Scheduler.GetExecutingRequestByNode a single lookup and return null when no request is executing.
• Replace _executingRequestByNode storage with a ConcurrentDictionary and update call sites to tolerate missing/cleared entries.
• Add unit tests to validate both the steady-state contract and concurrent request completion tolerance.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/Build/BackEnd/Components/Scheduler/SchedulingData.cs	Switches executing-request map to ConcurrentDictionary and changes executing-request lookup to TryGetValue-based null-tolerant behavior.
src/Build/BackEnd/Components/Scheduler/Scheduler.cs	Removes check-then-fetch race; returns request?.BuildRequest and updates scheduler state dump to avoid double reads.
src/Build/BackEnd/Components/Scheduler/IScheduler.cs	Updates contract docs to allow null when a node isn’t executing a request.
src/Build.UnitTests/BackEnd/Scheduler_Tests.cs	Adds regression tests for null-tolerant and concurrent completion behavior.

Suppressed comments (1)

src/Build.UnitTests/BackEnd/Scheduler_Tests.cs:779

• These test comments say completing a request "stores a null against the node". With this PR, _executingRequestByNode is a ConcurrentDictionary, which cannot store null values, so the implementation will need to clear the entry by removing the key (or using a sentinel). The comment should be updated to match the actual clearing mechanism to avoid misleading future readers.

                {
                    // Cycle node 1 between executing a request and executing nothing. Completing a request stores a
                    // null against the node, which is the value the reader must not dereference.
                    int configId = DefaultConfigId + 1 + i;

[ViktorHofer]

/review

[github-actions]

✅ Expert Code Review (command) completed successfully!

[github-actions]

Code Review: Fix NullReferenceException reporting file accesses for detoured nodes

This is a well-targeted fix for a real concurrency bug. The change is correct and the approach is sound.

✅ Correctness

The root cause is a classic TOCTOU race:

• Old code in Scheduler.GetExecutingRequestByNode: called IsNodeWorking(nodeId) (check), then GetExecutingRequestByNode(nodeId).BuildRequest (use) — another thread could clear the executing request between these two calls, yielding a NRE.
• New code: calls GetExecutingRequestByNode once and uses request?.BuildRequest — eliminates the race.

The fix in SchedulingData.GetExecutingRequestByNode switching from _executingRequestByNode[nodeId] (throws KeyNotFoundException for unknown nodes) to TryGetValue is correct and necessary.

✅ ConcurrentDictionary with concurrencyLevel: 1

Using concurrencyLevel: 1 is well-justified: all writes come from the scheduler thread (protected by BuildManager lock) and reads from the file-access pipe drain thread. ConcurrentDictionary provides the necessary memory-visibility guarantees for cross-thread reads. This is the minimal-overhead correct solution.

✅ Backward Compatibility

GetExecutingRequestByNode is on IScheduler, an internal interface. Returning null instead of throwing was already the contract for the IsNodeWorking == false branch. The change broadens when null is returned (unknown nodes previously threw KeyNotFoundException from SchedulingData). All callers are internal and this is a bug fix path, so this is not a breaking change.

✅ IsNodeWorking Refactoring in WriteSummary

Calling GetExecutingRequestByNode once rather than IsNodeWorking + GetExecutingRequestByNode separately is a clean improvement that also eliminates a secondary TOCTOU in the summary writer.

✅ Test Quality

• GetExecutingRequestByNodeReflectsTheNodesCurrentRequest: solid sequential state-machine test covering null-before-first-request and null-after-last-request states.
• GetExecutingRequestByNodeToleratesConcurrentRequestCompletion: good stress test with a spinning reader thread. The sawExecutingRequest/sawIdleNode guard assertions prevent the test from silently going vacuous — a nice defensive technique.

Minor Observation (non-blocking)

The IsNodeWorking method in SchedulingData (line 504) now reads from ConcurrentDictionary and its logic is equivalent to GetExecutingRequestByNode(nodeId) is not null. It could be simplified to delegate to GetExecutingRequestByNode in a follow-up for consistency, but this is not required.

Summary

The fix is minimal, correct, and well-tested. The ConcurrentDictionary change is the right tool. The new tests cover both sequential and concurrent scenarios. No blocking issues found.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Expert Code Review (command) for #14602 · sonnet46 · 42.4 AIC · ⌖ 5.13 AIC · ⊞ 4.7K
_{Comment /review to run again}

[ViktorHofer]

@dfederm minor feedback from above:

The IsNodeWorking method in SchedulingData (line 504) now reads from ConcurrentDictionary and its logic is equivalent to GetExecutingRequestByNode(nodeId) is not null. It could be simplified to delegate to GetExecutingRequestByNode in a follow-up for consistency, but this is not required.

@rainersigwald would you mind taking a quick look? I don't know anything about detoured nodes but the overall change looks correct to me.