The lock that you're using is the same lock the DQ uses. However, doing the Resume while holding the lock will cause the changes to be emitted while holding the lock. Let me see if I can suggest an alternative.

You're right, they shouldn't diverge. I went back and looked at this again and I think I was patching the symptom rather than the cause.

The actual problem is that ResumeNotifications() takes the lock twice. The using block grabs it to decrement the count and queue up the pending changes, releases it, and then the resume signal grabs it again in a second acquisition. Everything is under a lock, so nothing looks wrong at a glance, but the gap between those two acquisitions is the race window. A suspend that lands in there sees a zero count and skips emitting true, and then the resume comes along and emits its false on top of it. My re-check just made the second acquisition notice it had been beaten, which is exactly the "state corruption is cool if we guard against it" thing you called out.

So rather than re-checking, don't have a second acquisition:

     private void ResumeNotifications()
     {
-        bool emitResume;
+        using var notifications = _notifications.AcquireLock();

-        using (var notifications = _notifications.AcquireLock())
-        {
-            Debug.Assert(_suspensionTracker.IsValueCreated, "Should not be Resuming Notifications without Suspend Notifications instance");
+        Debug.Assert(_suspensionTracker.IsValueCreated, "Should not be Resuming Notifications without Suspend Notifications instance");

-            (var changes, emitResume) = _suspensionTracker.Value.ResumeNotifications();
-            if (changes is not null)
-            {
-                notifications.EnqueueNext(new CacheUpdate(changes, _readerWriter.Count, ++_currentVersion));
-            }
+        var (changes, emitResume) = _suspensionTracker.Value.ResumeNotifications();
+        if (changes is not null)
+        {
+            notifications.EnqueueNext(new CacheUpdate(changes, _readerWriter.Count, ++_currentVersion));
         }

-        // Emit the resume signal after releasing the delivery scope so that
-        // accumulated changes are delivered first. Re-check the suspend count
-        // under the lock: a concurrent SuspendNotifications() may have run in the
-        // window since the count was decremented, and its state must win. Emitting
-        // the signal only when still unsuspended keeps _notifySuspendCount and the
-        // suspended-notification subject from diverging.
+        // Emit the resume signal in the same lock acquisition that cleared the suspend
+        // count, so the count and the signal can never disagree: there is no window for a
+        // concurrent SuspendNotifications() to observe one without the other. The accumulated
+        // changes are still delivered off the lock, when this scope exits. A deferred
+        // subscriber that activates on this signal snapshots the current state and skips the
+        // still-pending delivery via the version guard in CreateConnectObservable.
         if (emitResume)
         {
-            using var readLock = _notifications.AcquireReadLock();
-            if (!_suspensionTracker.Value.AreNotificationsSuspended)
-            {
-                _suspensionTracker.Value.EmitResumeNotification();
-            }
+            _suspensionTracker.Value.EmitResumeNotification();
         }
     }

The thing that makes this work, and the reason I originally thought we couldn't do it, is that ScopedAccess.Dispose() calls ExitLockAndDeliver(), which releases the lock and then drains the queue. So the enqueue and the signal both happen in one critical section, and the accumulated changes still go out with nothing held. ResumeDeliversPendingChangesWithoutHoldingTheLock still passes, so we're not trading the race for the deadlock risk that your original lock(cache._locker) version would have introduced.

One consequence worth noting: the signal now fires before the pending changes are delivered instead of after. That turns out to be fine, because a deferred subscriber that activates on the signal takes its snapshot from GetInitialUpdates() and then skips the queued delivery it already has via the HasPending / _currentDeliveryVersion guard in CreateConnectObservable. That guard is there for precisely this case.

Nice side effect: with the window actually closed instead of papered over, ResumeSignalUnderLockPreventsStaleSnapshotFromReSuspend can come back. I dropped it back into the integration fixture and it passes right alongside your StaleResumeSignalIsSuppressedByConcurrentReSuspend. I'd much rather have both than delete one that was asserting real behavior. 2443 passing locally on net9.0, and I ran the suspend fixture 8 times back to back to make sure the integration tests weren't flaky, all green at 5s.

Two small things folded into the diff above:

• EmitResumeNotification() needs an IsDisposed check. SuspensionTracker.Dispose() runs out of the CacheUpdateObserver completion/error callbacks, and those execute off the lock during delivery, so the subject can get torn down underneath a resume on another thread. ResumeNotifications() already guards for this, that path didn't.
• Went back to ++_notifySuspendCount == 1 for the suspend gate. Once the count and the signal can't disagree, reading the subject's .Value isn't buying anything (and it takes the subject's own internal lock on every call), plus it lets the SuspendSubject should be false for the first suspend call assert come back.

On Interlocked for the counter, I don't think it helps here. Every mutation of it already happens inside _locker, so the increment itself was never the problem. The problem was the count and the subject getting updated in two different critical sections, and making the counter atomic on its own wouldn't have closed that.

Let me know what you think.

false can never be emitted while the count is above zero, because the re-check and the OnNext are both inside the same _locker acquisition and no other thread can mutate the count without that lock.

That's just blatantly untrue. ++_notifySuspendCount on line 486 and --_notifySuspendCount on line 508 happen underneath two separate and unsynchronized locks. The increment is inside lock(_locker) from line 166, and the decrement is inside _notifications.AcquireLock() from line 343. This was basically my point in my prior comment.

SuspendNotificationsFixture.UnitTests.cs ... uses a block-scoped namespace. Every other file in the repo uses file-scoped, including the sibling SuspendNotificationsFixture.IntegrationTests.cs added in this same PR, and the SuspendNotificationsFixture.cs it was split out of.

Fixed.

SuspensionsAreThreadSafe stays in the parallelized UnitTests. It is benign (no interleaving requirement, so it cannot go intermittent the way the moved test did), but the PR's stated rationale about other concurrency tests would apply.

Fixed.

And your new comment just popped up, while I was working this, so I'll just leave it at these two fixes.

Pushed this as bf07da5, since you said go ahead.

Kept it to the two files: the ResumeNotifications() / SuspendNotifications() / EmitResumeNotification() change described above, plus dropping ResumeSignalUnderLockPreventsStaleSnapshotFromReSuspend back into the integration fixture. I left the rest of your nits alone since you'd already handled the styling and moved SuspensionsAreThreadSafe over.

2444 passing on net9.0, and I ran the suspend fixture 5 more times against the real branch to be sure the integration tests are solid: 26 tests, 5s, green every time.

They're the same lock, though. Both ObservableCache constructors hand _locker to the DeliveryQueue as its gate:

_notifications = new DeliveryQueue<CacheUpdate>(_locker, new CacheUpdateObserver(this));

So lock (_locker) on 166 and _notifications.AcquireLock() on 343 are taking the same object. That's what I was getting at in my first comment on this PR about the DQ using the same lock, which I should have been clearer about at the time.

Doesn't really change anything about the fix, though, and your instinct about it was right regardless. Two acquisitions of the same lock still leave a gap between them, and that gap is where this bit us: resume drops the count to zero and exits the scope, a suspend grabs the lock and bumps it back to one and emits a second true, then resume re-enters and puts false on top of it. Same lock, still a window.

The thing that makes this work, and the reason I originally thought we couldn't do it, is that ScopedAccess.Dispose() calls ExitLockAndDeliver(), which releases the lock and then drains the queue. So the enqueue and the signal both happen in one critical section, and the accumulated changes still go out with nothing held.

I find this pretty misleading then, so I'd definitely like to improve that aspect of the DeliveryQueue at some point. Even if it's just as simple as putting "ReleaseLockAnd" in the name of that resume method. But I can live with it, for now.

Let me know what you think.

I think the entire concept of buffering/batching/queueing notifications within SourceCache<> needs a re-design. We have 2 competing systems trying to do the same thing, and stepping on each others' toes: the SuspendNotifications() system and the .Edit() system. But that's definitely out-of-scope. This implementation, while not ideal, looks sound.

They're the same lock, though. Both ObservableCache constructors hand _locker to the DeliveryQueue as its gate:

I actually had the thought to wonder if that was the case, and looked for it. Somehow, I missed it, anyway.

On Interlocked for the counter, I don't think it helps here. Every mutation of it already happens inside _locker, so the increment itself was never the problem.

I meant, like, in terms of the possibility that we could implement the entire suspension system without any locking at all (except for the actual delivery). If the whole system hinges on that one counter, it should be possible, in theory.

Won't let me approve because I pushed a commit, but I'd approve if I could.

Won't let me approve because I pushed a commit, but I'd approve if I could.

Oh. Fun. A deadlock.

Well, I was able to adjust the branch protection rules to allow Maintainers to bypass them, for now. It requires an explicit additional confirmation to do so, so I'm good with leaving it that way, as long as you and @RolandPheasant are as well.