Buffer subscription changes when in repo mode by Enkidu93 · Pull Request #1052 · sillsdev/serval

Enkidu93 · 2026-09-30T19:08:16Z

I decided not to add a just-poll-the-pending-builds backstop. It seemed to me that we should get the subscription working properly and that something beyond that could be a bit hacky/overly complicated. But I'm open to adding it if you guys would prefer.

This change is

codecov-commenter · 2026-09-30T19:10:14Z

Codecov Report

❌ Patch coverage is 70.96774% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.85%. Comparing base (c467e51) to head (df554cd).

Files with missing lines	Patch %	Lines
...DataAccess/src/SIL.DataAccess/MongoSubscription.cs	29.41%	8 Missing and 4 partials ⚠️
...ataAccess/src/SIL.DataAccess/MemorySubscription.cs	81.81%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1052      +/-   ##
==========================================
+ Coverage   72.83%   72.85%   +0.01%     
==========================================
  Files         391      391              
  Lines       21745    21793      +48     
  Branches     2826     2836      +10     
==========================================
+ Hits        15838    15877      +39     
- Misses       4785     4787       +2     
- Partials     1122     1129       +7

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pmachapman

@pmachapman reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ddaspit and Enkidu93).

src/DataAccess/src/SIL.DataAccess/MemoryRepository.cs line 360 at r1 (raw file):

                if (kvp.Value(entity))
                    allSubscriptions.Add(kvp.Key);
            }

NIT: You could combine these two conditions

Code quote:

            if (kvp.Key.Mode == SubscriptionMode.Repository)
            {
                if (kvp.Value(entity))
                    allSubscriptions.Add(kvp.Key);
            }
            else if (kvp.Key.Change.Entity is null)
            {
                if (kvp.Value(entity))
                    allSubscriptions.Add(kvp.Key);
            }

src/DataAccess/src/SIL.DataAccess/MemorySubscription.cs line 23 at r1 (raw file):

        {
            await TaskTimeout(_changeEvent.WaitAsync, timeout ?? Timeout.InfiniteTimeSpan, cancellationToken)
                .ConfigureAwait(false);

NIT: You could remove this before the if statement then remove the else block.

Code quote:

            await TaskTimeout(_changeEvent.WaitAsync, timeout ?? Timeout.InfiniteTimeSpan, cancellationToken)
                .ConfigureAwait(false);

src/DataAccess/src/SIL.DataAccess/MongoSubscription.cs line 15 at r1 (raw file):

    private BsonDocument? _resumeToken;
    private readonly SubscriptionMode _mode;
    private readonly Queue<EntityChange<T>> _changes;

You use a ConcurrentQueue in the MemorySubscription, but just a normal Queue here. I would have thought both classes would have the same thread safety requirements?

Code quote:

private readonly Queue<EntityChange<T>> _changes;

ddaspit · 2026-10-01T17:26:49Z

    {
-        await TaskTimeout(_changeEvent.WaitAsync, timeout ?? Timeout.InfiniteTimeSpan, cancellationToken)
-            .ConfigureAwait(false);
+        if (Mode == SubscriptionMode.Repository)


I think the same change can be returned twice. If HandleChange enqueues a change while the waiter is checking _changes.IsEmpty, both of them set the event. The next wait then returns with an empty queue, and Change still holds the previous change. Would it be possible to loop on the wait until a change is actually dequeued?

Good catch. I think this fixes it. Let me know if I'm not following.

The loop looks good. Timeout.InfiniteTimeSpan is -1 ms, so remainingTimeout is always negative when no timeout is passed. This causes WaitForChangeAsync() to return immediately without waiting. It affects callers like GetNextFinishedBuild and LocalBuildJobRunner. Would it be possible to only compute the remaining time when a timeout is specified?

ddaspit · 2026-10-01T17:26:49Z

        foreach (KeyValuePair<MemorySubscription<T>, Func<T, bool>> kvp in _subscriptions)
        {
-            if (kvp.Key.Change.Entity is null)
+            if (kvp.Key.Mode == SubscriptionMode.Repository)


This sends deletes to repository mode subscriptions. MongoSubscription only watches inserts, updates and replaces in repository mode, so tests would see events that never happen in production. Can you skip deletes for repository mode subscriptions here?

That makes sense. Done.

This checks the type of the last change the subscription received, not the type of the change being sent, so deletes are still sent. It also means that once a repository mode subscription receives a delete, it will only receive changes for that entity. Would it be possible to pass the change type to GetSubscriptions and skip deletes for repository mode subscriptions?

Oh, right. I think this the first time I've ever really worked in this code. Let me know if this looks right now.

Enkidu93

@Enkidu93 made 5 comments and resolved 2 discussions.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on ddaspit and pmachapman).

src/DataAccess/src/SIL.DataAccess/MemoryRepository.cs line 360 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: You could combine these two conditions

Done.

src/DataAccess/src/SIL.DataAccess/MemorySubscription.cs line 23 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: You could remove this before the if statement then remove the else block.

Done.

src/DataAccess/src/SIL.DataAccess/MongoSubscription.cs line 15 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

You use a ConcurrentQueue in the MemorySubscription, but just a normal Queue here. I would have thought both classes would have the same thread safety requirements?

Done. You're right.

Enkidu93 · 2026-10-01T19:14:49Z

        foreach (KeyValuePair<MemorySubscription<T>, Func<T, bool>> kvp in _subscriptions)
        {
-            if (kvp.Key.Change.Entity is null)
+            if (kvp.Key.Mode == SubscriptionMode.Repository)


That makes sense. Done.

Enkidu93 · 2026-10-01T19:14:50Z

    {
-        await TaskTimeout(_changeEvent.WaitAsync, timeout ?? Timeout.InfiniteTimeSpan, cancellationToken)
-            .ConfigureAwait(false);
+        if (Mode == SubscriptionMode.Repository)


Good catch. I think this fixes it. Let me know if I'm not following.

Enkidu93

@Enkidu93 made 2 comments.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on ddaspit and pmachapman).

Enkidu93 · 2026-10-05T15:42:09Z

        foreach (KeyValuePair<MemorySubscription<T>, Func<T, bool>> kvp in _subscriptions)
        {
-            if (kvp.Key.Change.Entity is null)
+            if (kvp.Key.Mode == SubscriptionMode.Repository)


Oh, right. I think this the first time I've ever really worked in this code. Let me know if this looks right now.

Enkidu93 · 2026-10-05T15:42:09Z

    {
-        await TaskTimeout(_changeEvent.WaitAsync, timeout ?? Timeout.InfiniteTimeSpan, cancellationToken)
-            .ConfigureAwait(false);
+        if (Mode == SubscriptionMode.Repository)


ddaspit

@ddaspit reviewed 4 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93 and pmachapman).

src/DataAccess/src/SIL.DataAccess/MemorySubscription.cs line 24 at r3 (raw file):

        {
            TimeSpan remainingTimeout = Timeout.InfiniteTimeSpan;
            if (timeout != null)

You should also check that timeout is not equal to Timeout.InfiniteTimeSpan.

Enkidu93

@Enkidu93 made 1 comment.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on ddaspit and pmachapman).

src/DataAccess/src/SIL.DataAccess/MemorySubscription.cs line 24 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You should also check that timeout is not equal to Timeout.InfiniteTimeSpan.

Good point, done.

ddaspit

@ddaspit reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).

Enkidu93

@Enkidu93 resolved 1 discussion and dismissed @pmachapman from a discussion.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on pmachapman).

pmachapman

@pmachapman reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Enkidu93).

Enkidu93 requested review from ddaspit and pmachapman September 30, 2026 19:08

pmachapman requested changes Sep 30, 2026

View reviewed changes

ddaspit reviewed Oct 1, 2026

View reviewed changes

Enkidu93 commented Oct 1, 2026

View reviewed changes

Enkidu93 commented Oct 5, 2026

View reviewed changes

ddaspit reviewed Oct 5, 2026

View reviewed changes

Enkidu93 commented Oct 5, 2026

View reviewed changes

ddaspit approved these changes Oct 5, 2026

View reviewed changes

Enkidu93 force-pushed the buffer-changes-in-repo-mode branch from d100dfb to b744f70 Compare October 5, 2026 19:41

Enkidu93 requested a review from pmachapman October 5, 2026 22:01

Enkidu93 added 4 commits October 5, 2026 18:29

Buffer subscription changes when in repo mode

b9433fd

Address reviewer comments

4b40749

Address reviewer comments

d38f4fc

Address reviewer comment

df554cd

Enkidu93 force-pushed the buffer-changes-in-repo-mode branch from b744f70 to df554cd Compare October 5, 2026 22:29

Enkidu93 commented Oct 5, 2026

View reviewed changes

pmachapman approved these changes Oct 5, 2026

View reviewed changes

Enkidu93 merged commit 8f8a895 into main Oct 5, 2026
2 checks passed

Enkidu93 deleted the buffer-changes-in-repo-mode branch October 5, 2026 22:45

Conversation

Enkidu93 commented Sep 30, 2026 • edited by ddaspit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Enkidu93 commented Sep 30, 2026 •

edited by ddaspit

Loading

codecov-commenter commented Sep 30, 2026 •

edited

Loading