Skip to content

Load each FwData project once under concurrent requests - #2698

Draft
imnasnainaec wants to merge 7 commits into
developfrom
fix/fwdata-single-flight-load
Draft

imnasnainaec wants to merge 7 commits into
developfrom
fix/fwdata-single-flight-load

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

When several requests asked for the same FwData project at once, each one ran LoadCache, because IMemoryCache.GetOrCreate isn't atomic. The later load's cache Set replaced the earlier entry, and the eviction callback (reason Replaced) disposed that earlier copy. The Platform.Bible lexicon picker fetches writing systems for every local project, so each retry after a timeout queued another full round of loads, and the queue never drained.

FwDataFactory now holds a lock for each project around the whole load, and the memory cache only ever holds a finished LcmCache.

  • Requests for the same project wait on its lock and share one load. Different projects still load in parallel.
  • A failed load caches nothing, so the next request retries it.
  • An entry can't be evicted during a load, because it doesn't exist until the load finishes.
  • CloseProjectAsync takes the same lock. It waits for any load still running, then removes and disposes the cache, so the project's lock file is gone before it returns (FwLinker and FwIntegrationRoutes depend on this before they hand out a FieldWorks link). A request made during a close waits for the close to finish, then loads a new copy.
  • Before a reload or close, any copy of the project that's still open (e.g. an expired entry whose eviction callback hasn't run yet) is disposed under the same lock, and the eviction callback takes that lock too. Otherwise a late dispose could delete the new copy's lock file, which has the same path.
  • Open caches are tracked by instance, and whichever of eviction, close or shutdown untracks a cache disposes it, so each one is disposed exactly once. A late eviction callback can't untrack a newer cache for the same project.
  • The "Evicting project" log line now includes the eviction reason.

Test plan

dotnet test FwDataMiniLcmBridge.Tests --filter "FullyQualifiedName~FwDataFactory": 9 tests, all passing across repeated runs.

  • ConcurrentRequestsForTheSameProjectShareOneLoad: fails against develop (2 loads instead of 1).
  • CloseWaitsForAnInFlightLoadAndDisposesIt: fails against develop.
  • FwDataFactoryOnDiskTests.CloseDuringALoadReleasesTheLockFile: loads a real on-disk project, closes it during the load, and checks that LCM's .fwdata.lock file is gone afterwards. (The XML backend doesn't hold the .fwdata open; that lock file is its lock.) Fails against develop.
  • CloseRightAfterRemovalDisposesTheOldCache and ReloadAfterRemovalDisposesTheOldCacheBeforeLoading: remove the cache key (queuing the eviction callback, as expiry does) and then close or reload; the old copy must be disposed before close returns or the new load starts. Both fail with the stale-copy disposal removed.
  • FailedLoadIsRetriedOnTheNextRequest, RemovingTheCacheKeyDuringALoadDoesNotLoseTheLoad, ShutdownDisposesAProjectRetriedAfterAFailedLoad, RequestDuringACloseGetsANewCacheAfterIt: guard the behaviour above.

FwLiteWeb and FwHeadless build.

Known limitations

  • A close frees the project's files only if no new request for that project arrives during or after it; any such request loads it again. Preventing that is up to the callers.
  • A cache can still be disposed while another caller is using it (the existing todo in the eviction callback).
  • A close, or an eviction callback, that arrives during a load ties up a thread-pool thread on the project lock until the load finishes.
  • At shutdown, a load still running isn't waited for. Nothing unsaved can be lost, and exiting releases its lock file.
  • RequestDuringACloseGetsANewCacheAfterIt can't force its request to arrive while the close is still disposing, so the waiting path isn't exercised every run.

For the team: per-project locks across scopes

FwHeadless registers FwDataFactory as scoped (AddFwDataBridge(ServiceLifetime.Scoped)), but IMemoryCache is a singleton. Each scope therefore gets its own _keyLocks, which never grows much there, but loads of the same project from two scopes at once aren't coordinated; that's no worse than develop. If FwHeadless can open one project from two scopes at once, the locks should be shared, e.g. through a singleton.

Considered and rejected

  • Caching a Lazy<LcmCache> created under a short lock. It also shares one load, but a load in progress then sits in the cache, so a failed load, eviction during a load, and closing during a load each needed extra handling. The per-project lock avoids all three.
  • A caching library that loads each key once (e.g. LazyCache, FusionCache). It adds a dependency and still leaves disposal on close and at shutdown to us.
  • Only fixing the caller. Reducing the picker's repeat requests is worth doing separately, but it would leave the factory's race in place.

🤖 Generated with Claude Code

FwDataFactory.GetProjectServiceCached used IMemoryCache.GetOrCreate with
a factory that ran the slow LoadCache. GetOrCreate isn't atomic, so
concurrent requests for the same project (e.g. the Platform.Bible
extension fetching writing systems for every project, then retrying
after a client timeout) each loaded it. When a later load finished, its
Set replaced the earlier entry, and the eviction callback disposed the
earlier LcmCache (EvictionReason.Replaced). The logs showed "loaded"
immediately followed by "Evicting"/"disposed", and the load queue grew
with every retry.

Cache a Lazy<LcmCache> (ExecutionAndPublication) created under a lock,
so concurrent callers join the one in-flight load. A failed load is
removed from the cache so the next call retries. Eviction, Dispose and
CloseProjectAsync only dispose a Lazy whose value was created. The
eviction log now includes the reason.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

FwDataFactory now coordinates project loads by cache key and tracks loaded caches through eviction, explicit close, and shutdown. New tests cover load concurrency, retries, disposal coordination, reloads, and lock-file cleanup.

Changes

Project cache lifecycle

Layer / File(s) Summary
Serialize project loads
backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs, backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs
FwDataFactory checks for a live cache under a per-key lock and caches completed loads. Tests cover concurrent requests sharing one load, retries after failure, and cache-key removal during a load.
Coordinate cache disposal
backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs, backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs, backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs
Eviction, close, and shutdown untrack caches before disposal. Tests cover close during loading, close and reload, shutdown disposal, and lock-file removal after an on-disk project closes.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: 🟡 Moderate · up to e7292

After cache expiration, reopening a project may fail while the previous cache is still being disposed. Ensure disposal finishes before replacement loading prior to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary change: coordinating concurrent loads so each FwData project loads once.
Description check ✅ Passed The description is detailed and directly explains the loading, locking, caching, disposal, testing, and known limitations in the changeset.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit watched the project load,
While caches shared a careful road.
Close waited till the load was done,
Then freed the cache when work was spun.
The lock file left; the rabbit hopped.

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the 💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related label Sep 25, 2026
@argos-ci

argos-ci Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Sep 25, 2026, 5:51 PM
e2e (Inspect) ✅ No changes detected - Sep 25, 2026, 6:01 PM

imnasnainaec and others added 2 commits September 25, 2026 10:39
- CloseProjectAsync takes the entry out of the cache and waits for a load
  still in flight, then disposes it, so file locks are released before
  it returns.
- Evicting an entry whose load hasn't finished disposes the cache once
  the load completes.
- Track cache entries instead of keys, so a failed load's late eviction
  callback can't untrack its replacement and hide it from shutdown.

Each entry owns a run-once disposal shared by eviction, close and
shutdown. The concurrency test no longer depends on a fixed delay.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@imnasnainaec imnasnainaec self-assigned this Sep 25, 2026
imnasnainaec and others added 3 commits September 25, 2026 11:11
Loads a real on-disk project through a gated loader, closes it while
the load is blocked, and checks LCM's lock file is gone afterwards.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Replace the Lazy-based cache entry with a lock per project held for the
whole load, so the cache only ever holds a finished LcmCache. A failed
load caches nothing, an entry can't be evicted mid-load, and a request
made during a close waits for it to finish disposing. Open caches are
tracked by instance, and whoever untracks one disposes it, so eviction,
close and shutdown dispose each cache exactly once.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
coderabbitai[bot]

This comment was marked as resolved.

An expired entry's eviction callback runs on the thread pool, so a
reload could start LoadCache before the old LcmCache was disposed, and
disposing it afterwards deleted the new copy's lock file. Close could
also return while that callback was still disposing.

Reload and close now dispose any tracked copies of the project under its
lock first, and the eviction callback takes the same lock.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant