fix(routerlicious-driver): Properly dispose caches when document service is disposed#26196
Conversation
7111f40 to
32833c7
Compare
…ice is disposed Previously, `DocumentService.dispose()` was empty, causing memory leaks: - Factory-level caches retained all blobs indefinitely - `blobsShaCache` grew unbounded with each blob read - `PrefetchDocumentStorageService` cache was never cleared This change: - Adds `removeByPrefix()` to ICache for document-specific cleanup - Implements `dispose()` in DocumentService to propagate to storage - Adds `dispose()` to storage services to clean document-specific entries - Adds expiration to blobCache as a safety net - Clears blobsShaCache and prefetchCache on dispose This ensures calling `dispose()` properly releases memory regardless of when it's called (immediately or after the "saved" event). Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
32833c7 to
7da42b1
Compare
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes a memory leak where DocumentService.dispose() was empty and did nothing, causing blob caches to retain data indefinitely even after containers were disposed. The fix implements a proper disposal chain and adds cache cleanup methods.
Changes:
- Added
clear()method toICacheinterface and its implementations - Implemented disposal chain:
DocumentService→DocumentStorageService→ internal storage services → cache cleanup - Added 5-day expiration to
blobCacheas a safety net against memory leaks
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
cache.ts |
Adds clear() method to ICache interface and implements it in InMemoryCache and NullCache |
documentService.ts |
Implements dispose() to call documentStorageService?.dispose() |
documentStorageService.ts |
Adds dispose() to propagate disposal to internal storage service |
prefetchDocumentStorageService.ts |
Implements dispose() to clear prefetchCache and propagate to internal service |
shreddedSummaryDocumentStorageService.ts |
Implements dispose() to clear blobCache, snapshotTreeCache, and blobsShaCache |
wholeSummaryDocumentStorageService.ts |
Implements dispose() to clear blobCache and snapshotTreeCache |
documentServiceFactory.ts |
Adds 5-day expiration to blobCache for additional protection |
cache.spec.ts |
Adds unit test for clear() functionality |
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
…river-memory-disposal
packages/drivers/routerlicious-driver/src/shreddedSummaryDocumentStorageService.ts
Outdated
Show resolved
Hide resolved
packages/drivers/routerlicious-driver/src/wholeSummaryDocumentStorageService.ts
Outdated
Show resolved
Hide resolved
The blobCache and snapshotTreeCache are shared across all documents via the factory, so clearing them in a per-document dispose() would wipe cached data for all active documents. Only clear the per-instance blobsShaCache. The MapWithExpiration-based expiration handles cleanup of the shared caches over time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>
This reverts commit 09ccaec.
|
/azp run Build - api-markdown-documenter, Build - benchmark-tool, Build - build-common, Build - build-tools, Build - client packages, Build - common-utils, Build - eslint-config-fluid, Build - eslint-plugin-fluid, Build - protocol-definitions, Build - test-tools |
|
/azp run repo-policy-check, server-gitrest, server-gitssh, server-historian, server-routerlicious |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
The blobCache and snapshotTreeCache are factory-level shared caches.
Using clear() in dispose() wiped cached data for ALL active documents,
not just the one being disposed. This adds removeByPrefix() to the
ICache interface and uses it in dispose() to only remove entries keyed
with this document's ID prefix (`${this.id}:`), leaving other
documents' cache entries intact.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: anthony-murphy <anthony.murphy@microsoft.com>
…:anthony-murphy-agent/FluidFramework into fix/routerlicious-driver-memory-disposal
|
Azure Pipelines commands (copy and post separately, limit of 10 at a time): |
|
Azure Pipelines need to be triggered for this fork PR. Please run the following: |
Summary
DocumentService.dispose()was empty and did nothingclear()method toICacheinterface for cache cleanupDocumentService→DocumentStorageService→ internal storage servicesblobsShaCacheandprefetchCacheon disposeblobCacheas a safety net against leaksBackground
Customer reported memory issues when using Fluid Framework with SharedTree and Routerlicious driver. Their heap snapshot showed a retention chain:
The
readAndParseBlobclosure captures the storage service, which holds the driver's blob cache:The root cause:
DocumentService.dispose()was completely empty, so even when containers were disposed, the driver caches retained all blob data indefinitely.Fix
When
dispose()is called:DocumentService.dispose()→DocumentStorageService.dispose()blobCache.clear(),snapshotTreeCache.clear(), etc.)Even if timer callbacks still hold the
readAndParseBlobclosure reference, the actual blob data in the cache is now freed on dispose.Changes
cache.tsclear()toICacheinterface and implementationsdocumentService.tsdispose()to call storage service disposedocumentStorageService.tsdispose()to propagate to internal serviceshreddedSummaryDocumentStorageService.tsdispose()to clear cacheswholeSummaryDocumentStorageService.tsdispose()to clear cachesprefetchDocumentStorageService.tsdispose()to clearprefetchCachedocumentServiceFactory.tsblobCachecache.spec.tsclear()Test plan
clear()added🤖 Generated with Claude Code