Ilya (@iSazonov) Could you re-add the backport consider labels? Or it does not matter.

Kris Borowinski (@kborowinski) Thanks for your contribution!

@PowerShell/powershell-maintainers triage decision:
For 7.4, I don't think a fix that doesn't affect the actual script results makes the bar for an LTS. The regression risk would be higher than the potential gains here.

Travis Plunk (@TravisEz13)

I understand the decision not to backport this PR to the LTS version, but I would like to clarify that the actual script results are affected in PS 7.4.X and up.

I first reported this issue on the 7Zip4PowerShell repo, where the unpacking process was crashing mid-task.
I proposed a PR that was merged to implement a workaround that swallows NREs coming from progress pane:

try {
    worker.Progress.StatusDescription = "Finished";
    worker.Progress.RecordType = ProgressRecordType.Completed;
    WriteProgress(worker.Progress);
} catch (NullReferenceException) {
    // Possible bug in PowerShell 7.4.0 leading to a null reference exception being thrown on ProgressPane completion
    // This is not happening on PowerShell 5.1
}

This issue affects all scripts that write to the console from multiple threads, leading to potential crashes in the LTS version of PowerShell. Affected users are forced to disable the progress bar either globally or per script.

Kris Borowinski (@kborowinski) I had the bot open PRs for the backports and added your comments. These still need to be triaged, but we are moving over to a new process that helps us track all the information we need.

1. Kris Borowinski (@kborowinski) It would be good if you know exactly when it was introduced, that will help us with our decision.

Travis Plunk (@TravisEz13) In comment on the original issue Ilya (@iSazonov) pinpointed it to PR #13758 PowerShell 7.2.0-preview.2 and #15864 PowerShell 7.2.0-preview.9 when PSAnsiRendering was made stable.

Root Cause

I debugged the code and found that the race condition was caused by ConsoleHostUserInterface.PreWrite calls _progPane?.Hide() without locking the _instanceLock.

The NRE happens because _savedRegion gets set to null while the Hide() method is running. However, _savedRegion is set to null only in the Hide() method, and that means at least two Hide() are running in parallel, which is unexpected.

Looking closely, Hide() has 5 call sites, and 3 of them are guarded by lock (_instanceLock). The other 2 un-guarded call sites are ConsoleHostUserInterface.PreWrite() and ConsoleHostUserInterface.PreRead(). The PreWrite() method gets called in WriteToCosnole method, which is the most low-level method called by all Write-* commands, including Write-Verbose.

The root cause becomes clear give the above -- calling Write-Verbose in the middle of on-going Write-Progress potentially causes the Hide() method to be called in parallel, which corrupts the state of the ProgressPane.

So, the potential problem still exists in CosnoleHostUserInterface.PreRead(), ConsoleHostUserInterface.PostWrite(), and ConsoleHostUserInterface.PostRead(). The latter 2 call _progPane?.Show() without locking _instanceLock.

Thoughts on this fix

The safest and complete fix would be adding lock (_instanceLock) to PreWrite(), PreRead(), PostWrite(), and PostRead(), so as to make all calls to Hide and Show guarded by the same lock.

However, after discussing with Patrick Meinecke (@SeeminglyScience) and thinking about this more, we decide to not make further changes to the original fix. Here is why:

1. By looking into the code more closely, it seems all Write-* actions (including $host.UI.Write*) that can be triggered by a user go through WriteImpl, which has been guarded by the original fix. Other calls to WriteToCosnole outside of WriteImpl are for reading input, prompting user with choices, and etc., not related to a Write-* action from user. So, it feels to me that guard WriteImpl is sufficient for the cases where a race condition could happen.

2. Comparatively, even though adding locks to PreWrite and PostWrite would be a safer and more complete fix, it's not really needed practically in cases where WriteToConsole is called because of reading input or prompting users. Furthermore, it will result in acquiring locks twice in the WriteToConsole method, which will make it less performant in general.

3. Regarding PreRead and PostRead, they are used only when reading input, and it's unlikely to happen while progress rendering is ongoing. So, again, even though adding locks there would be a complete fix, it seems unnecessary practically and will degrade the performance for sure.

The safest and complete fix would be adding lock (_instanceLock) to PreWrite(), PreRead(), PostWrite(), and PostRead(), so as to make call calls to Hide and Show guarded by the same lock.

Dongbo Wang (@daxian-dbw) Thank you for your investigations! The reason I didn't dare to do this was for fear of a deadlock. There are too many combinations. Even a change in one place in the future could potentially open up scenarios impossible today.
Steve and I had a short exchange about this a few years ago with the conclusion to wait for feedback. Now that such report has been received, we have corrected the specific scenario. If there are more complex scenarios with problems, then it may be more correct not to saturate the code with locks, but to change the approach more fundamentally.

Patrick Meinecke (@SeeminglyScience) did look into the feasibility to make the progress rendering lock-less, using atomic primitives. But it turned out to be a too big effort and too low priority for the team to commit on.