SF-3532 Dispose realtime docs when no longer in use

[Nateowami]

This is still work in progress, but I want to put it out here as it appears to be working.

---

This change is 

---

[codecov]

Codecov Report

❌ Patch coverage is 70.92199% with 123 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.29%. Comparing base (5740054) to head (2b25245).
⚠️ Report is 3 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...re/ClientApp/src/xforge-common/realtime.service.ts	54.34%	21 Missing ⚠️
...boarding-requests/onboarding-requests.component.ts	0.00%	14 Missing ⚠️
...diagnostic-overlay/diagnostic-overlay.component.ts	0.00%	14 Missing ⚠️
...nx-insights-panel/lynx-insights-panel.component.ts	7.69%	12 Missing ⚠️
...e/ClientApp/src/app/shared/project-router.guard.ts	56.52%	10 Missing ⚠️
...App/src/xforge-common/activated-project.service.ts	76.66%	7 Missing ⚠️
...ure/ClientApp/src/xforge-common/project.service.ts	22.22%	7 Missing ⚠️
...al-administration/serval-administration.service.ts	25.00%	6 Missing ⚠️
...pture/ClientApp/src/app/core/sf-project.service.ts	78.94%	4 Missing ⚠️
...ClientApp/src/xforge-common/models/realtime-doc.ts	82.60%	4 Missing ⚠️
... and 15 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
+ Coverage   81.26%   81.29%   +0.02%     
==========================================
  Files         624      625       +1     
  Lines       39656    39835     +179     
  Branches     6417     6432      +15     
==========================================
+ Hits        32228    32385     +157     
- Misses       6434     6453      +19     
- Partials      994      997       +3     

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

[marksvc]

Hello @Nateowami ,

Thank you for your work on this!

Here are some comments on the code.

I find positive names to be easier to understand than negative names, when using boolean logic. I suggest to consider renaming DocSubscription.isUnsubscribed$ to isSubscribed$.

Can you explain more about the use of DocSubscription.unsubscribe() instead of using a destroyRef argument to DocSubscription.constructor? It looks like DestroyRef is for some angular classes, and so if we want to use a DocSubscription in a different situation, we might not have a DestroyRef available, and so we would use DocSubscription.unsubscribe?

Rather than provide DocSubscription.unsubscribe for situations where a DestroyRef|Observable was not provided to DocSubscription.constructor, do you think we could always require clients to do one of

• Give a DestroyRef that they have.
• inject a DestroyRef, and pass it to DocSubscription.constructor, and later the DestroyRef gets destroyed when.. when the "corresponding injector is destroyed"; or
• Pass an Observable, and emit from the Observable when the client wants to unsubscribe.
  I'm thinking this would reduce the complexity and variation of the DocSubscription class. (DocSubscription.unsubscribe could be removed.)
  I might have to have a look at what that looks like to decide if I like it better or worse :).

It looks like if we removed DocSubscription.unsubscribe, and instead had clients pass an Observable, that might look something like

// New client field
private readonly bye$ = new Subject<void>();
...
// Pass in constructor
new DocSubscription('DraftGenerationStepsComponent', this.bye$.asObservable())
...
// New client method
wave(): void {
  this.bye$.next();
  this.bye$.complete();
}

I want to mention that we could further reduce the complexity of DocSubscription by changing the constructor destroyRef argument from DestroyRef | Observable<void> to just Observable<void>. That would give some simplification.
Client code might change from

new DocSubscription('DraftGenerationStepsComponent', this.destroyRef)

to something like

new DocSubscription('DraftGenerationStepsComponent', new Observable(subscriber => this.destroyRef.onDestroy(() => subscriber.next())))

That makes the client code more complex just to simplify DocSubscription.constructor by a couple lines, so I'm not too inclined toward it. But I wanted to mention that idea in case it inspires other ideas.

Sometimes when working in TypeScript it seems like it could be useful to have a standard disposable system. In C#, there is an IDisposable interface, and implementing classes have a dispose() method that is called when the class is going away. In this dispose method you can let go of resources that you were holding onto.
If we had any desire to apply a generic disposal system across the frontend TypeScript application, this might be a good place to implement and do it.
AI suggested some disposal patterns that could be applied, which are

• Implement our own Disposable interface with a dispose method like C#.
• Using RxJS Subscription, where we add in each held resource and then in ngOnDestroy or another "dispose" method we let go of the resources.
• Using Angular DestroyRef. I wonder if we can use this with arbitrary classes, not just with Components and Directives.
• And of interest, in the future, using might come to TypeScript.

In C#, the IDisposal.dispose method is automatically called if you are using an object and it goes out of scope. (using someResource = foo()) Unfortunately, only the Angular DestroyRef and in-the-future using patterns above would be as automated (at least where we could not rely on ngOnDestroy). Hmm.
Well, do you have any comments on generic disposal ability in the frontend application?

[marksvc]

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.spec.ts line 1 at r12 (raw file):

// import { NgZone } from '@angular/core';

I am working on writing a test suite for the new CacheService.

[Nateowami]

Reviewable status: 50 of 136 files reviewed, 8 unresolved discussions (waiting on @marksvc)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 51 at r12 (raw file):

Previously, marksvc wrote…

Is this line here in case the active/current project changes while loadAllChapters is running? Okay, I see that matches functionality that was present before but implemented in a different way. Why do you also check if this.currentProject.projectId != null as part of it? Based on the code in the contsructor, I am expecting it is because when a project changes, currentProject.projectId changes from "previousProjectId" to undefined to "newProjectId". Is that so + why?

currentProject should be renamed to activatedProject for clarity. If activatedProject.id == null, it would indicate no project is currently active (such as when on the my projects page). We wouldn't want to stop caching until a new project is activated.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 55 at r12 (raw file):

Previously, marksvc wrote…

Previously we were not keeping a reference to the TextDoc (such as by storing it in subscribedTexts). Was this causing it to not be adequately cached?

I don't think we will need to keep the reference to the TextDoc in order to prevent it from being destroyed, since our docSubscription is still hanging around until we .unsubscribe() from it.

Can you explain a bit more about what the purpose is of recording the TextDoc objects in this.subscribedTexts?

Thanks for noticing this. I think there was a point where I was keeping track of them so we could dispose them. This PR then progressed well beyond that point, to where docs clean up themselves. Looking at uses of subscribedTexts in this file:

• It's defined
• It's reset to []
• It has elements pushed to it

At no point do we read elements out of it. Which means it can be removed. That was a good catch.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.spec.ts line 1 at r12 (raw file):

Previously, marksvc wrote…

I am working on writing a test suite for the new CacheService.

Thanks. I had forgotten I'd commented these out.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 112 at r12 (raw file):

Previously, marksvc wrote…

Hmmm. I wonder if any requestors will have a reference to the SFProjectProfileDoc, and use it for some good reason after the current/active project changes, but then the SFProjectProfileDoc is destroyed out from under them. Perhaps never.

I think you're misreading the situation.

The ActivatedProjectService is always the subscriber that holds on to the SFProjectProfileDoc. The service will live for the life of the application, so they'll never get disposed. Which is OK. Users of the ActivatedProjectService basically "borrow" the project doc from the service, and don't have to "buy" the doc themselves, since the ActivatedProjectService owns it permanently.

If the service didn't take ownership of it, when you go from one page to another, the page you're leaving would release its hold on the document milliseconds before the next page requested it, leading to destroying and recreating docs. This trashing in some cases can happen thousands of times per second, in the worst cases (such as when calling a permission check on each chapter, if that check loads and destroys a doc each time).

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-doc.ts line 253 at r6 (raw file):

Previously, marksvc wrote…

The comment I added to this and the method below sounded pretty silly (since if there is a doc subscription, presumably it is subscribed :)

Hmm, some alternate terminology for DocSubscription might be:

DocHandle, or
DocRequest, or
DocHold

And instead of isUnsubscribed, it could be

active, or
connected, or
complete, or
ended, or
terminated

Then we'd be seeing things like

if (docHandle.active)
  count++

if (!docHandle.active)
  this.dispose()

Or comments like

/** Number of doc handles that are still active. */

DocSubscription isn't a bad name. And it could be paired with active instead of isUnsubscribed. So we'd have a comment like

/** Number of doc subscriptions that are still active. */

Or the field could be complete to be symmetric with isUnsubscribed.

Does any of this alternate terminology sound preferable?

We can probably get rid of docSubscriptionsCount now, since we now clean up docs instantly when they're no longer in use. This was added temporarily when I had implemented logic to track what was using documents, but was still experimenting with how they are cleaned up (for example, I had a garbage collection approach where it would look for documents to be cleaned up every few seconds, which was one approach to avoid repeatedly destroying and recreating documents).

I don't think I really like the idea of changing the name, even though I've never liked DocSubscription as a name. I think it's important to convey the fact that we're working with a subscription here; i.e. the type of thing that one ought to unsubscribe from when it's no longer needed. (Also, it wouldn't be hard to do a rename in the future if we come up with a better description)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 112 at r13 (raw file):

    if (doc != null) {
      // Handle documents that currently exist but are in the process of being disposed.
      const docBeingDisposed: Subject<void> | undefined = this.docsBeingDisposed.get(doc.id);

This variable seems to me to be misnamed. It's not getting a doc. It's getting a Subject<void> that will complete when the doc has been fully cleaned up.

Also, we usually end observables with $ to indicate their type. I think this convention makes sense, since while a variable name should usually give you some kind of hint about its type (for example, count is probably a number, doc is probably a realtime doc), an observable for a count being named count would make it appear to be a number. Also, usually after you observe the thing, you get the value inside. So if you have an observable for a count named count, you can't subscribe to it, get the emitted value, and call that count. So the pattern of count$.subscribe(count => console.log(count)) works pretty well. Otherwise you write count.subscribe(helpWhatDoICallThisVariable => console.log(helpWhatDoICallThisVariable))

[marksvc]

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 55 at r12 (raw file):

Previously, Nateowami wrote…

Thanks for noticing this. I think there was a point where I was keeping track of them so we could dispose them. This PR then progressed well beyond that point, to where docs clean up themselves. Looking at uses of subscribedTexts in this file:

• It's defined
• It's reset to []
• It has elements pushed to it

At no point do we read elements out of it. Which means it can be removed. That was a good catch.

Okay. Working on that.

[marksvc]

Reviewable status: 50 of 136 files reviewed, 7 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.spec.ts line 1 at r12 (raw file):

Previously, Nateowami wrote…

Thanks. I had forgotten I'd commented these out.

Here is a test suite.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 112 at r12 (raw file):

Previously, Nateowami wrote…

I think you're misreading the situation.

The ActivatedProjectService is always the subscriber that holds on to the SFProjectProfileDoc. The service will live for the life of the application, so they'll never get disposed. Which is OK. Users of the ActivatedProjectService basically "borrow" the project doc from the service, and don't have to "buy" the doc themselves, since the ActivatedProjectService owns it permanently.

If the service didn't take ownership of it, when you go from one page to another, the page you're leaving would release its hold on the document milliseconds before the next page requested it, leading to destroying and recreating docs. This trashing in some cases can happen thousands of times per second, in the worst cases (such as when calling a permission check on each chapter, if that check loads and destroys a doc each time).

Thank you. Right, the DocSubscription is using ActivatedProjectService's this.destroyRef. Thank you.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 112 at r13 (raw file):

Previously, Nateowami wrote…

This variable seems to me to be misnamed. It's not getting a doc. It's getting a Subject<void> that will complete when the doc has been fully cleaned up.

Also, we usually end observables with $ to indicate their type. I think this convention makes sense, since while a variable name should usually give you some kind of hint about its type (for example, count is probably a number, doc is probably a realtime doc), an observable for a count being named count would make it appear to be a number. Also, usually after you observe the thing, you get the value inside. So if you have an observable for a count named count, you can't subscribe to it, get the emitted value, and call that count. So the pattern of count$.subscribe(count => console.log(count)) works pretty well. Otherwise you write count.subscribe(helpWhatDoICallThisVariable => console.log(helpWhatDoICallThisVariable))

Thank you.
Yes, thank you for the reminder and reasoning about using $ for Observables.
I have adjusted it.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 11 at r12 (raw file):

// In production this should be true, but when testing doc cleanup it may be useful to set to false an observe behavior
const KEEP_PRIOR_PROJECT_CACHED_UNTIL_NEW_PROJECT_ACTIVATED = true;

I'm removing this to clean the file, but we can get similar functionality by inserting this.uncache() at the top of the constructor's subscribe handler.

[Nateowami]

Reviewable status: 49 of 136 files reviewed, 5 unresolved discussions (waiting on @marksvc)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/tabs/editor-tab-persistence.service.ts line 65 at r6 (raw file):

Previously, marksvc wrote…

I imagine that one kind of bug we might start to encounter is that of having the wrong component hold onto the DocSubscription, such that it's let go of before the actual user of the data is done with it. I had to look around for a bit to understand if calling code should be the one to hold onto the DocSubscription here.

Perhaps. I think the more common thing is going to be documents not being released like they should, which is a big part of why the developer diagnostics have had doc cleanup added to them.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 257 at r6 (raw file):

Previously, Nateowami wrote…

That is a very good question, and I'm still trying to figure it out.

I'm pretty sure this is a bug, but I need to look at it in more detail to be sure.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-query.ts line 15 at r7 (raw file):

Previously, marksvc wrote…

I was adding a comment to unsubscribe$ saying

/** The object is being disposed. */

But when I looked further at how it is being used I decided really I should go ahead and rename the field. I'm mentioning it because maybe I wasn't thinking about the right aspects or nuances of how this should be named.

I'm not convinced this is an improvement. The name you picked seems to suggest that this is a BehaviorSubject that tracks whether or not the "being disposed" state is active or not. In reality, it's a simple subject that's acting like an event emitter. It emits and completes upon unsubscribe. It doesn't describe a state, but an event.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-query.ts line 15 at r14 (raw file):

export class RealtimeQuery<T extends RealtimeDoc = RealtimeDoc> {
  private _docs: T[] = [];
  private beingDisposed = new Subject<void>();

This should end with $

[marksvc]

I squashed, rebased, and added more commits. Which I am pushing now.

[devin-ai-integration]

Devin Review found 7 new potential issues.

View 10 additional findings in Devin Review.

[devin-ai-integration]

🚩 SFUserProjectsService calls projectDoc.dispose() directly, bypassing the new DocSubscription system

At user-projects.service.ts:59, when a user is removed from a project, projectDoc.dispose() is called directly. This bypasses the DocSubscription auto-disposal mechanism. The doc was fetched with a DocSubscription('SFUserProjectsService', this.destroyRef) at line 68, but that subscription is never explicitly unsubscribed before dispose() is called. After dispose(), the stale DocSubscription remains in the doc's docSubscriptions set, and when the DocSubscription eventually tries to unsubscribe (on service destroy, which never happens for root services), it would try to interact with an already-disposed doc. Since SFUserProjectsService is a root service, this is harmless in practice, but it's an inconsistency with the new subscription model.

(Refers to line 59)

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🚩 DraftSourcesService accumulates DocSubscriptions for root-scoped service

In draft-sources.service.ts:80, getSourceFromConfig creates a new DocSubscription('DraftSources', this.destroyRef) each time it's called. Since DraftSourcesService is providedIn: 'root', the destroyRef will never fire, so these subscriptions never auto-unsubscribe. Each call to getSourceFromConfig (triggered by project/config changes via the observable pipeline) adds a new active subscriber to the project profile doc. Over a long session with many project switches, this accumulates subscribers. It doesn't cause incorrect behavior, but it prevents project profile docs from ever being auto-disposed via the subscriber count mechanism.

(Refers to lines 80-89)

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🚩 DraftHistoryEntryComponent creates many DocSubscriptions per entry set

In draft-history-entry.component.ts:106-190, the entry setter creates multiple DocSubscription('DraftHistoryEntry', this.destroyRef) instances each time a new entry is set. Each call to projectService.getProfile() creates a new subscription. When the entry input changes (e.g., switching between history entries), the old subscriptions remain active until the component is destroyed. For long-lived components that cycle through many entries, this could accumulate many subscriptions on project profile docs. Consider creating a single DocSubscription per entry update cycle and unsubscribing the previous one.

(Refers to lines 106-190)

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🚩 addSubscriber immediately disposes doc if passed an already-unsubscribed DocSubscription

When addSubscriber receives a DocSubscription whose isUnsubscribed$ BehaviorSubject already has value true (e.g., because the DestroyRef passed to the constructor was already destroyed, triggering the NG0911 path at realtime-doc.ts:46), the filter(true).subscribe() callback fires synchronously. This deletes the subscription from the set and checks activeDocSubscriptionsCount === 0. For a newly created doc (first subscriber), this would trigger void this.dispose(), causing the doc to be disposed before the caller even receives it from realtimeService.get(). In practice, the NG0911 case only arises when a component is already destroyed, so the returned doc wouldn't normally be used. But it's a latent issue where a doc could be created and immediately destroyed, wasting resources.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🚩 CacheService uses a single DocSubscription for all texts in a project, tying their lifecycle together

At cache.service.ts:31-33, a single DocSubscription is created and then reused for every getText call in loadAllChapters. This means all cached text documents for a project share the same subscription. When uncache() is called at line 37, it unsubscribes from this single DocSubscription, which will signal all associated documents to check their subscriber count. If the CacheService was the only subscriber, all of those documents will be disposed. This is likely the intended behavior — batch subscribe and batch unsubscribe — but it means a user navigating between chapters won't benefit from cached text docs of other chapters if no other component is also subscribing to them. Previously, the CacheService used loadTextDocs which subscribed without any disposal mechanism, keeping docs alive indefinitely.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[marksvc]

Converting to Draft so it's clear that it's not something to review right now.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 14 additional findings in Devin Review.

[devin-ai-integration]

🚩 CacheService has a narrow race window between canAccessTextAsync and getText during project switching

In loadAllChapters, the interrupt check at line 48 (this.activatedProjectService.projectDoc !== project) runs at the top of the loop iteration, before canAccessTextAsync and getText. If a project switch occurs DURING the await canAccessTextAsync() call (line 52), the docSubscription parameter will have been unsubscribed by the new subscription callback's uncache() call. When getText (line 53) then runs, it passes an already-unsubscribed subscription. The text doc would be fetched, the subscriber added, then immediately the unsubscribe callback fires. If the doc has no other subscribers, it gets disposed (including offline data deletion). This is a very narrow race window and the impact is limited since the doc was just fetched (not previously cached), and the user is switching away from this project anyway. The next loop iteration's interrupt check would catch the change and return.

(Refers to lines 44-63)

---

Was this helpful? React with 👍 or 👎 to provide feedback.