Fix locked readable stream in composable cache #939
Conversation
🦋 Changeset detectedLatest commit: 6a34a21 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull Request Overview
Fixes a locked readable stream issue in the composable cache by improving how pending cache entries are handled. The solution prevents stream locking by ensuring that ReadableStreams are properly converted to strings before storing and recreated when retrieving from the pending write map.
- Updates pending write promise mapping to handle string values instead of streams
- Improves stream handling during cache get operations for pending entries
- Adds comprehensive test coverage for the composable cache functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/open-next/src/adapters/composable-cache.ts | Fixes stream locking by converting streams to strings in pending writes and recreating streams on retrieval |
| packages/tests-unit/tests/adapters/composable-cache.test.ts | Adds comprehensive test suite covering all cache operations including concurrent access scenarios |
| .changeset/wet-emus-brake.md | Documents the fix for locked readable stream issue |
Comments suppressed due to low confidence (2)
packages/open-next/src/adapters/composable-cache.ts:7
- [nitpick] The variable name
pendingWritePromiseMapcould be more concise. Consider renaming topendingWritesorpendingEntriesfor better readability.
const pendingWritePromiseMap = new Map<
packages/tests-unit/tests/adapters/composable-cache.test.ts:404
- The variable name
writtenTagsshadows the global variable declared at line 50. Consider renaming towrittenTagsCalloractualWrittenTagsto avoid confusion.
const writtenTags = tagCache.writeTags.mock.calls[0][0];
| invalidatePaths: vi.fn(), | ||
| }; | ||
| globalThis.cdnInvalidationHandler = invalidateCdnHandler; | ||
| let writtenTags = new Set(); |
There was a problem hiding this comment.
The writtenTags variable is declared with let but could be const since it's reassigned in beforeEach blocks rather than mutated directly.
| let writtenTags = new Set(); | |
| const writtenTags = new Set(); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
It stole mine as well. It wrote a lot of the unit test 😄
| // If we do, we return the pending promise instead of fetching the cache | ||
| if (pendingWritePromiseMap.has(cacheKey)) { | ||
| return pendingWritePromiseMap.get(cacheKey); | ||
| const stored = pendingWritePromiseMap.get(cacheKey)!; |
There was a problem hiding this comment.
Is there a scenario where set doesn't get called and this returns undefined?
There was a problem hiding this comment.
I can't think of any, but I can add a check just in case. And it will stop typescript complaining
|
Thanks for the review! |
Update handling of pending writes to prevent locked readable streams in the composable cache. This change improves the management of cache entries and ensures proper stream handling.
I want to make some test first, as this one will increase memory usage, which could be an issue in cloudflareSeems to be working fine