Stage 4 fixes: nested pumps drain, timed out Send is abandoned

The Mac and X11 hosts guard their idle drain against reentry, and
CaptureScreenshot's recovery spin runs underneath a queued action - so its
drains were no-ops. With the context installed a genuinely suspending capture
continuation is Posted to that same queue, which nothing would then run: a
silent screenshot failure. The spins now call the new
UiThread.DrainForNestedPump, which skips the guard on purpose and is safe to
nest because InvokePendingActions works from a private copy of the queue. The
guard keeps protecting ordinary reentry (modal dialogs); the two RunEventLoop
call sites are the outermost loop, never nested, so they still drain normally.

A Send that times out now marks its queued work abandoned before it throws, so
the work does not apply late over whatever the caller did instead, and a throw
from work that slipped through goes to UiThread.ReportUnhandledException rather
than a field no caller will ever read. Post and Send reject a null callback at
the call site, per the BCL contract.

Comments corrected where they now asserted the opposite of reality, plus a note
on Post latency: a resumption costs up to one idle tick, so an N deep chain of
suspending awaits can cost N.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: larsbrubaker

Gui/MainLoopSynchronizationContext.cs

@@ -44,6 +44,10 @@ namespace MatterHackers.Agg.UI
 	/// <see cref="UiThread.RunOnIdle(Action)"/>, so posted continuations and hand written RunOnIdle work
 	/// share a single FIFO. A continuation posted while the pump is running the current batch executes on
 	/// the NEXT pump, never re-entrantly inside the current one.</para>
+	/// <para>That costs latency: a resumption waits for the next pump - up to one idle tick, about 10ms on
+	/// Windows Forms - so an N deep chain of genuinely suspending awaits can take up to N ticks to unwind.
+	/// Predictable ordering is worth that, but work that cannot afford it should not be hopping the loop
+	/// once per await.</para>
 	/// <para>On WebAssembly nothing installs this - Blazor supplies its own single threaded dispatcher and
 	/// that is already the model this context imitates.</para>
 	/// <para><see cref="SynchronizationContext.OperationStarted"/> and
@@ -128,15 +132,36 @@ public void Dispose()
 		/// </summary>
 		public override void Post(SendOrPostCallback d, object state)
 		{
+			// Checked here rather than left to blow up on the pump: the BCL contract is an
+			// ArgumentNullException, and a NullReferenceException raised one tick later on the main loop
+			// names neither the caller nor the mistake.
+			if (d == null)
+			{
+				throw new ArgumentNullException(nameof(d));
+			}
+
 			UiThread.RunOnIdle(() => d(state));
 		}
 
 		/// <summary>
 		/// Runs work on the main loop and waits for it. Inline when already on the main loop; from any
 		/// other thread this queues and blocks, bounded by <see cref="SendFromOtherThreadTimeout"/>.
 		/// </summary>
+		/// <remarks>
+		/// A Send that times out does NOT run its work afterwards. The queued item stays in the queue - it
+		/// cannot be pulled back out - but it checks on the way in whether the caller has already given up,
+		/// and if so does nothing. Otherwise a caller that timed out and retried, or fell back to another
+		/// path, would have the abandoned work applied a second time whenever the loop recovered. Work that
+		/// had already passed that check when the wait gave up still runs to completion - the check closes
+		/// the window that lasts as long as the timeout, not the instant at the end of it.
+		/// </remarks>
 		public override void Send(SendOrPostCallback d, object state)
 		{
+			if (d == null)
+			{
+				throw new ArgumentNullException(nameof(d));
+			}
+
 			if (UiThread.IsUiThread)
 			{
 				d(state);
@@ -148,15 +173,34 @@ public override void Send(SendOrPostCallback d, object state)
 			ExceptionDispatchInfo failure = null;
 			var completed = new ManualResetEventSlim(false);
 
+			// Volatile because it is written by this thread and read by the pump thread. Set before the
+			// TimeoutException below is thrown, so the throw and the abandonment are one decision.
+			bool abandoned = false;
+
 			UiThread.RunOnIdle(() =>
 			{
 				try
 				{
+					if (Volatile.Read(ref abandoned))
+					{
+						return;
+					}
+
 					d(state);
 				}
 				catch (Exception sentWorkException)
 				{
-					failure = ExceptionDispatchInfo.Capture(sentWorkException);
+					if (Volatile.Read(ref abandoned))
+					{
+						// The caller is long gone and its `failure` will never be read, so reporting it
+						// through the field would swallow the exception entirely. This is the same channel
+						// any other throw out of a queued action takes.
+						UiThread.ReportUnhandledException(sentWorkException);
+					}
+					else
+					{
+						failure = ExceptionDispatchInfo.Capture(sentWorkException);
+					}
 				}
 				finally
 				{
@@ -166,6 +210,8 @@ public override void Send(SendOrPostCallback d, object state)
 
 			if (!completed.Wait(SendFromOtherThreadTimeout))
 			{
+				Volatile.Write(ref abandoned, true);
+
 				// Deliberately not disposed: the work is still queued and will call Set on the pump thread
 				// if the loop ever recovers. Disposing here would turn that into an ObjectDisposedException
 				// on the UI thread, which is a far worse failure than leaking one event.

Gui/UiThread.cs

@@ -248,6 +248,25 @@ public static void InvokePendingActions()
 			}
 		}
 
+		/// <summary>
+		/// Drains the queue from inside a nested pump - a loop that is itself running underneath a queued
+		/// action and cannot finish until an awaited continuation makes progress.
+		/// </summary>
+		/// <remarks>
+		/// The platform hosts guard their ordinary idle drain with a re-entrancy flag, which is the right
+		/// protection for an idle action that runs a message loop of its own and the wrong one for a loop
+		/// that is spinning precisely because it is waiting on work this queue now owns: a suspended await
+		/// resumes by posting through <see cref="MainLoopSynchronizationContext"/> into this very queue, so
+		/// a guarded (no-op) drain would leave such a loop waiting forever for the one thing that could
+		/// release it. Those loops call this instead. Nesting is safe because
+		/// <see cref="InvokePendingActions"/> runs from a private copy of the queue rather than the live
+		/// list.
+		/// </remarks>
+		public static void DrainForNestedPump()
+		{
+			InvokePendingActions();
+		}
+
 		public class DeferredAction
 		{
 			protected Action action;

PlatformLinux/linux/X11SystemWindow.cs

@@ -842,11 +842,17 @@ public void CaptureScreenshot(string path)
 				// The native read-back completes inside the paint (wgpu's buffer map is polled to
 				// completion there), so this is normally already set. It is only not set if the await in
 				// CaptureThenPresent genuinely suspended, in which case its continuation is queued to the
-				// idle pump - hence pumping rather than blocking, which would deadlock.
+				// idle pump by MainLoopSynchronizationContext - hence pumping rather than blocking, which
+				// would deadlock.
+				//
+				// DrainForNestedPump, not InvokeIdleActions: this loop very often runs underneath an idle
+				// action already (the off-thread branch above marshals through RunOnIdle), and the guarded
+				// drain is a no-op while that is true - which would leave this spinning for a continuation
+				// only a drain can run.
 				for (int spin = 0; spin < ScreenshotPumpSpins && !this.screenshotComplete.IsSet; spin++)
 				{
 					PumpEvents();
-					InvokeIdleActions();
+					UiThread.DrainForNestedPump();
 				}
 			}
 			finally
@@ -1000,7 +1006,9 @@ private static unsafe void InstallErrorHandlers()
 
 		/// <summary>
 		/// Drains the RunOnIdle queue. Guarded because an idle action can run a nested loop (a modal
-		/// dialog, or <see cref="CaptureScreenshot"/>'s pump) and re-enter this.
+		/// dialog) and re-enter this. A nested loop that must instead let awaited continuations run -
+		/// <see cref="CaptureScreenshot"/>'s spin - calls <see cref="UiThread.DrainForNestedPump"/>, which
+		/// deliberately skips this guard.
 		/// </summary>
 		private static void InvokeIdleActions()
 		{

PlatformMac/mac/MacSystemWindow.cs

@@ -822,15 +822,18 @@ public void CaptureScreenshot(string path)
 
 				// The native read-back completes inside the paint (wgpu's buffer map is polled to
 				// completion there), so this is normally already set. It is only not set if the await in
-				// CaptureThenPresent genuinely suspended. This host installs no SynchronizationContext, so
-				// that continuation resumes on a thread-pool thread rather than back here - but the work it
-				// is waiting on is driven by this window's frames and idle queue, so pumping (rather than
-				// blocking the UI thread, which would stop the very frames it needs) is still what lets it
-				// finish.
+				// CaptureThenPresent genuinely suspended, in which case its continuation is queued to the
+				// idle pump by MainLoopSynchronizationContext - hence pumping rather than blocking the UI
+				// thread, which would stop the very frames and continuations it is waiting on.
+				//
+				// DrainForNestedPump, not InvokeIdleActions: this loop very often runs underneath an idle
+				// action already (the off-thread branch above marshals through RunOnIdle), and the guarded
+				// drain is a no-op while that is true - which would leave this spinning for a continuation
+				// only a drain can run.
 				for (int spin = 0; spin < ScreenshotPumpSpins && !completed.IsSet; spin++)
 				{
 					PumpEvents();
-					InvokeIdleActions();
+					UiThread.DrainForNestedPump();
 				}
 			}
 			finally
@@ -944,9 +947,10 @@ public async Task CaptureScreenshotAsync(string path)
 			}
 			finally
 			{
-				// Only clear what still belongs to this request. There is no SynchronizationContext in this
-				// host, so this cleanup can run on a thread-pool thread well after the request was given up
-				// on, by which point the fields may already have been claimed by the next request.
+				// Only clear what still belongs to this request. The continuation that gets here resumes on
+				// the main loop (MainLoopSynchronizationContext), but on a LATER pump - so this cleanup can
+				// still run well after the request was given up on, by which point the fields may already
+				// have been claimed by the next request.
 				if (ReferenceEquals(this.screenshotCompletion, completion))
 				{
 					this.pendingScreenshotPath = null;
@@ -1194,7 +1198,9 @@ private static void OnIdleTimer(IntPtr self, IntPtr cmd, IntPtr timer)
 
 		/// <summary>
 		/// Drains the RunOnIdle queue. Guarded because an idle action can run a nested loop (a modal
-		/// dialog, or <see cref="CaptureScreenshot"/>'s pump) and re-enter this.
+		/// dialog) and re-enter this. A nested loop that must instead let awaited continuations run -
+		/// <see cref="CaptureScreenshot"/>'s spin - calls <see cref="UiThread.DrainForNestedPump"/>, which
+		/// deliberately skips this guard.
 		/// </summary>
 		private static void InvokeIdleActions()
 		{