Fix infinite route-loader spinner from per-navigation root cache inva…

[naomigassler]

PR: Fix infinite route-loader spinner on navigation (stale root-endpoint cache deadlock)

Target branch: DSpace/dspace-angular:main
Relates to: #3584, #3697
Fixes #5855

Description

BrowserInitService invalidates the root API endpoint cache on every
NavigationStart. On a subsequent request, HALEndpointService.getEndpointMapAt
(hit on essentially every request via getEndpoint()) discards the now-stale
root /server/api entry and triggers a re-fetch that can deadlock — the
route resolver never completes, NavigationEnd never fires, and the
ds-base-root route-loader spinner hangs indefinitely on a frozen store.

The root endpoint map is effectively static between navigations, so
re-invalidating it on each NavigationStart is unnecessary. Backend-down
detection still happens at init and through normal request-failure handling.

Root cause

• BrowserInitService → invalidateRootCache() on every NavigationStart.
• Marks the root /server/api endpoint cache stale.
• Next getEndpoint() → getEndpointMapAt discards the stale root → re-fetch
  deadlocks → resolver never resolves → NavigationEnd never fires → spinner
  hangs forever.
• Higher REST latency makes it fire on nearly every revisit (why it looks
  intermittent).

Steps to reproduce

1. Open any listing (collection items, MyDSpace, Browse, Search — even
   Communities & Collections).
2. Navigate listing → listing → listing.
3. The UI hangs on the route-loader spinner; a full page reload clears it.

Reproducible on a small instance (~7k items) — not load- or scale-dependent.
Higher REST/proxy latency increases the frequency.

Fix

Remove the per-NavigationStart invalidateRootCache() call in
browser-init.service.ts; keep the one-time invalidation at init. No behavior
change to backend-down detection.

How to test

1. Before the fix: reproduce the hang via the steps above.
2. After the fix: the same navigation sequence no longer hangs; the spinner
   always resolves.
3. Backend-down handling still works: stop the REST backend and confirm the
   app still detects/handles it (init-time + request-failure paths).

Tests

Added a spec asserting the root endpoint cache is invalidated once at init
and not on subsequent NavigationStart events.

Checklist notes

• One-file logic fix; no new dependencies; no user-facing strings (i18n N/A).
• Passes yarn lint and yarn check-circ-deps.
• TypeDoc added/updated on any modified public method.

[tinsch]

I was able to reproduce the issue with the DSpace sample data. The steps to reproduce it locally were:

1. login as admin
2. start a simple search on the whole repo (hit search on front page)
3. click on DCAT Journal Publications
4. go to "MyDSpace" listing
5. observe infinite loading on the list

I tested this PR, and the issue did not occur anymore 🎉. But then again, I tried to reproduce the original issue on main, just to double check - and could not successfully reproduce it anymore. So I would suggest:

• more people should try to reproduce the original issue and confirm that this PR fixes it
• a frontend dev could review the code and based on that we can decide to merge this PR, even if the original bug is not reproducible every time

Thanks for the PR! This is a great contribution in my opinion.

[jlipka]

Hey @tinsch and others in here.
Thanks for listing your click sequence—unfortunately, I wasn't able to reproduce the effect using that, but I've included a similar attempt below.

Starting point:
The code in the main branch without any changes from this MR!

I see the infinite route loader appear when I double-click very quickly on a navigation item.
You can also reproduce this in the DSpace Sandbox.

1. Open https://sandbox.dspace.org
2. Hover over “All of DSpace” and double-click on “By title”
3. As a result, you’ll see the loading spinner centered in the middle (it no longer disappears).
4. If that didn’t work right away, double-click again on, for example, “By author”
   (You should see the effect after a few attempts at most.)

Screencast on the DSpace sandbox page
(on the second attempt the spinner appears)

The issue can apparently be reproduced this way—unless I’m missing something.
I used Firefox running on an Ubuntu/Linux.

If I now apply the fix from the merge request, the error no longer occurs. Therefore, in my opinion, the fix makes perfect sense—especially since the logic in the underlying code apparently isn’t needed at all.

---

So far, so good.

But now I’m wondering if this really solves the problem. Even if we probably don’t actually need to manually invalidate the /server/api API endpoint on every NavigationStart event, invalidating requests is still a common pattern in DSpace (at this point, I’d just like to loosely refer to the msToLive property in the configuration). Other requests will also become invalid or stale over the course of the application’s lifecycle, and the app shouldn’t freeze up in such cases either.

So, another approach or at least a try could be to swap the order of the following two lines:
Original code in findbyHref in base-data.service.ts

dspace-angular/src/app/core/data/base/base-data.service.ts

Lines 312 to 314 in a52ba24

	skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)),
	this.reRequestStaleRemoteData(reRequestOnStale, () =>
	this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),

We could move the skipWhile operator below the reRequestStaleRemoteData sideffect, as its a regular tap operator wrapped inside another function.

• Could this be a valid (additional) solution?
• Does skipWhile still serve its purpose, or has the logic been modified in an unintended way?

No matter what sequence of clicks I used after making this change, I could no longer trigger the state where the loading spinner is displayed continuously.

Long story short:

• I think the change in this MR makes sense, and I can reproduce it myself using the sequence of clicks I described above
• At the same time, in my opinion, the fix does not resolve a potential issue in BaseDataService that might be blocking streams unintentionally—because if the skipWhile mechanism is moved, the freezes won’t occur, regardless of whether we use the fix in this merge request or not. However, it hasn't yet been determined whether this would cause problems elsewhere in the app.
• I’m aware that the calculated Boolean for the LoadingIndicator is retrieved in app.component.ts. We could certainly make adjustments here as well, but this again raises the question for me of whether this might be masking a potential (!—I’m not sure) problem elsewhere.

I’d be very interested in hearing your thoughts on these points, and of course whether you can reproduce the behavior.

[jlipka]

After further investigation, I might have found a better, more logic solution, which leaves my previous idea seeming obsolete.

---

Original code:

dspace-angular/src/app/core/data/request.service.ts

Lines 155 to 176 in a52ba24

	isValid = (entry: RequestEntry): boolean => {
	if (hasNoValue(entry)) {
	// undefined entries are invalid
	return false;
	} else {
	if (isLoading(entry.state)) {
	// entries that are still loading are always valid
	return true;
	} else {
	if (isStale(entry.state)) {
	// entries that are stale are always invalid
	return false;
	} else {
	// check whether it should be stale
	const timeOutdated = entry.response.timeCompleted + (entry.request.responseMsToLive ?? this.defaultResponseMsToLive);
	const now = new Date().getTime();
	const isOutDated = now > timeOutdated;
	return !isOutDated;
	}
	}
	}
	};

My (partial) code changes:

const isValid = (entry: RequestEntry): boolean => {
  if (hasNoValue(entry)) {
    // undefined entries are invalid
    return false;
  } else {
    if (isStale(entry.state)) {
      return false;
    } else if (isLoading(entry.state)) {
      return true;
    } else {
      // check whether it should be stale
      const timeOutdated = entry.response.timeCompleted + entry.request.responseMsToLive;
      const now = new Date().getTime();
      const isOutDated = now > timeOutdated;
      return !isOutDated;
    }
  }
};

This change targets to check isStale first (no matter if the loading state is currently true or false), before to check the loading state, and last to check the TTL.

---

Another, additional check I added in the shouldDispatchRequest method in the same file (request.service.ts), starting around Line 493:

Original code:

dspace-angular/src/app/core/data/request.service.ts

Lines 431 to 436 in a52ba24

	} else {
	// if we are, check the request cache
	const urlWithoutEmbedParams = getUrlWithoutEmbedParams(request.href);
	if (this.hasByHref(urlWithoutEmbedParams) === true) {
	return false;
	} else {

My (partial) code changes:

    } else {
      // if we are, check the request cache
      const urlWithoutEmbedParams = getUrlWithoutEmbedParams(request.href);
      if (this.hasByHref(urlWithoutEmbedParams) === true) {
        return false;
      } else if (this.hasByHref(urlWithoutEmbedParams, false) === true) {
        // an entry exists but is stale/outdated -> always fetch fresh data,
        // regardless of whether the object is still present in the object cache
        return true;
      } else {

This change ensures that a request entry that exists in the request cache but is stale or expired is always re-fetched from the server—regardless of whether the associated object is still present in the object cache. Previously, in this case, the system would incorrectly fall back on the object cache and suppress the request, even though the data was marked as stale.

---

As before, the fix in this merge request makes perfect sense. My additional changes could make the app even more robust.

I have to do some more testing about these changes (especially in our customized instance)... but wanted to let you know about my current investigation.

[tinsch]

Thanks @jlipka for your investigation. I can reproduce the spinner issue on sandbox with your click sequence, but not locally.

I tried my click sequence again to reproduce the issue, and it happened reliably on main and was fixed on the PR branch. However, I looked into the PR code and tested it further: The problem is, we now don't invalidate the backend route anymore on NavigationStart. This means that if the backend is down (simulated locally by turning off the backend container) the frontend won't notice. I can still click around in the UI and get some errors on certain pages, but this will be confusing for end users who don't know what's happening when the UI still looks somewhat fine. It can lead to data loss on the users side, and a lot of questions to the repository owners should the backend be down. Unfortunately I think we cannot use the fix for this reason.

@naomigassler could you look into this please? Maybe there is a way to prevent the problem I described and still fix the spinner issues by tweaking the code a bit.
@jlipka would you be willing to open an additional PR that includes your fix, so we could discuss it separately?

To help with further investigations:
When reproducing the spinner issue on the workspace page, I saw a 400 response from the backend every time the spinner issue occurred.

The route was http://localhost:8080/server/api/discover/search/objects?sort=lastModified,DESC&page=0&size=10&configuration=workspace&embed=thumbnail&embed=item%2Fthumbnail and the errors in the backend indicate that the frontend does not properly send the login cookie on that request:

2026-08-20 12:45:03,898 ERROR unknown 66fe317a-003e-44f0-a1f5-1841e2c99fe7 org.dspace.discovery.utils.DiscoverQueryBuilder @ anonymous::Error in Discovery while setting up date facet range:date facet\colon; org.dspace.discovery.configuration.DiscoverySearchFilterFacet@38816a6c
org.dspace.discovery.SearchServiceException: An anonymous user cannot perform a workspace or workflow search

[naomigassler]

Hello @tinsch and @jlipka,

We measured every candidate on a test box against two scenarios: A the
deadlock, B the backend stopped. Full write-up and raw data available if
useful.

variant	Scenario A (deadlock)	Scenario B (backend down)	cost
baseline	hangs 3/3	/500 every navigation, 338 ms	—
this PR	fixed 0/3	no /500; inline "Error fetching …" at 353 ms; /mydspace blank, no error	none
our published alternative	hangs 3/3	= baseline	—
@jlipka's isValid reorder	wedges the browser tab	rejected	—
decoupled liveness probe	fixed 0/3	= baseline (/500 every navigation)	1 root request per navigation

Measured on our DSpace 9.3 deployment with added REST latency, not on main. The
/mydspace blank page in row 2 is reachable only with this PR applied — see below.

A correction to our own issue report. The alternative we suggested there —
filter(rd => !rd.isStale || rd.hasCompleted) in getEndpointMapAt — does not
work. It hangs 3/3 with a frozen-store signature byte-identical to the unfixed
baseline: the stale-but-completed RemoteData never reaches that filter, so the
deadlock is upstream of it. Please disregard it, and apologies to anyone who spent
time on it.

On the objection. "Won't notice the backend is down" isn't quite what we
measured — the outage surfaces on the first navigation at 353 ms, as inline errors
on most routes. But there is no /500, the root stays Success/200 for its full
6 h TTL, and /mydspace renders an empty main area with no error at all. There a
user cannot distinguish an outage from an empty repository, and that is exactly
where the data-loss concern lives, because submissions are on that page.

So the concern was sound even where the mechanism wasn't, and chasing it turned up
a pre-existing bug: <ds-search> renders nothing — no results, no empty state, no
error — when its search-configuration request fails from a cold cache. It backs 6
routes, is reproducible on main today, and is filed as #6111.

But on unmodified main you cannot reach it by stopping the backend, because
ServerCheckGuard redirects to /500 first. This PR's deletion removes that
blanket, so it converts an unreachable blank page into a reachable one on
/mydspace. That is a fair reading of the objection and we would rather state it
than have it found in review. The consequence is a merge order rather than a
rejection: with #6111 in, those 6 routes report the outage honestly and this PR has
no silent hole on search-backed pages. #6111 stands on its own either way.

What the actual decision is. Today's /500 is a categorical guarantee: every
route reports an outage. This PR replaces it with per-route handling — better where
implemented, worse where it isn't. Two coherent positions, and we don't think our
measurements settle which is right:

1. Per-route errors. This PR + Fix <ds-search> rendering an empty page when the search configuration request fails #6111 + finish the remaining routes. Smallest
   changes, better messages, but inherently whack-a-mole: /community-list still
   sits on "Loading…" indefinitely with the backend down, on its own data path.
2. Keep a categorical signal. Make liveness an explicit HttpClient probe that
   never enters the ngrx store, keeping this PR's deletion. Scenario B came out
   bit-for-bit identical to baseline, which is what proves the decoupling. Cost:
   one root request per navigation (settle 293–847 ms vs this PR's 162–656 ms;
   baseline pays the same), a new service plus a guard rewrite instead of deleting
   five lines, and a de-duplication TTL we picked rather than derived.

They are compatible. Happy to open the probe separately if there is appetite.

On the isValid reorder, tested as its own variant: the page stops responding
to any script about 9 s in and never recovers — worse than the original deadlock,
which at least leaves the shell responsive. Requests freeze rather than climb, so
it isn't a request storm, and controls through the same harness stay clean, so it
isn't the harness. The accompanying shouldDispatchRequest change also looks
redundant: with isValid corrected, the object-cache fallback calls
hasByUUID(..., true) and runs the same corrected check; the two were identical in
our runs.

Caveat. Our reproduction amplifies the production race with added REST latency,
so we have shown these mechanisms are sufficient to produce the hang, not that
they are the only cause.