-
Notifications
You must be signed in to change notification settings - Fork 616
fix: add LRU eviction for session storage to prevent QuotaExceededError #8101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
78e2d70
fix: add LRU eviction for session storage to prevent QuotaExceededError
christian-byrne add7a4f
refactor: simplify session storage service
christian-byrne fdba5d2
test: remove redundant mock restoration calls
christian-byrne 5b6cc87
fix: use startsWith for prefix matching in protected keys
christian-byrne f44c06f
fix: restore mock cleanup in tests
christian-byrne 3d88497
merge: resolve conflict with main - adopt draft store system
christian-byrne aec1a55
fix: use pointer instead of payload in sessionStorage to prevent Quot…
christian-byrne File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
238 changes: 238 additions & 0 deletions
238
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,238 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { | ||
| getStorageStats, | ||
| getWithLruTracking, | ||
| removeFromStorage, | ||
| setWithLruEviction | ||
| } from './sessionStorageLruService' | ||
|
|
||
| describe('sessionStorageLruService', () => { | ||
| beforeEach(() => { | ||
| sessionStorage.clear() | ||
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| describe('setWithLruEviction', () => { | ||
| it('stores data with LRU metadata wrapper', () => { | ||
| const data = { nodes: [], links: [] } | ||
| const result = setWithLruEviction('workflow:test', data) | ||
|
|
||
| expect(result).toBe(true) | ||
|
|
||
| const stored = sessionStorage.getItem('workflow:test') | ||
| expect(stored).toBeTruthy() | ||
|
|
||
| const parsed = JSON.parse(stored!) | ||
| expect(parsed).toHaveProperty('accessedAt') | ||
| expect(parsed).toHaveProperty('data') | ||
| expect(parsed.data).toEqual(data) | ||
| expect(typeof parsed.accessedAt).toBe('number') | ||
| }) | ||
|
|
||
| it('returns true on successful storage', () => { | ||
| const result = setWithLruEviction('key', { test: 'data' }) | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| it('evicts LRU entries when quota is exceeded', () => { | ||
| const oldEntry = { accessedAt: 1000, data: { old: 'data' } } | ||
| const newEntry = { accessedAt: 2000, data: { new: 'data' } } | ||
|
|
||
| sessionStorage.setItem('workflow:old', JSON.stringify(oldEntry)) | ||
| sessionStorage.setItem('workflow:new', JSON.stringify(newEntry)) | ||
|
|
||
| let callCount = 0 | ||
| const originalSetItem = sessionStorage.setItem.bind(sessionStorage) | ||
| vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => { | ||
| callCount++ | ||
| if (key === 'workflow:current' && callCount === 1) { | ||
| const error = new DOMException('Quota exceeded', 'QuotaExceededError') | ||
| throw error | ||
| } | ||
| return originalSetItem(key, value) | ||
| }) | ||
|
|
||
| const result = setWithLruEviction( | ||
| 'workflow:current', | ||
| { current: 'data' }, | ||
| /^workflow:/ | ||
| ) | ||
|
|
||
| expect(result).toBe(true) | ||
| expect(sessionStorage.getItem('workflow:old')).toBeNull() | ||
| expect(sessionStorage.getItem('workflow:new')).toBeTruthy() | ||
| }) | ||
|
|
||
| it('returns false after max eviction attempts', () => { | ||
| const spy = vi.spyOn(sessionStorage, 'setItem').mockImplementation(() => { | ||
| throw new DOMException('Quota exceeded', 'QuotaExceededError') | ||
| }) | ||
|
|
||
| const result = setWithLruEviction('key', { test: 'data' }) | ||
| expect(result).toBe(false) | ||
|
|
||
| spy.mockRestore() | ||
| }) | ||
| }) | ||
|
|
||
| describe('getWithLruTracking', () => { | ||
| it('returns null for non-existent key', () => { | ||
| const result = getWithLruTracking('nonexistent') | ||
| expect(result).toBeNull() | ||
| }) | ||
|
|
||
| it('returns unwrapped data for new format entries', () => { | ||
| const entry = { accessedAt: Date.now(), data: { nodes: [], links: [] } } | ||
| sessionStorage.setItem('workflow:test', JSON.stringify(entry)) | ||
|
|
||
| const result = getWithLruTracking('workflow:test') | ||
| expect(result).toEqual({ nodes: [], links: [] }) | ||
| }) | ||
|
|
||
| it('handles legacy format (unwrapped data) with accessedAt: 0', () => { | ||
| const legacyData = { nodes: [1, 2, 3], links: [] } | ||
| sessionStorage.setItem('workflow:legacy', JSON.stringify(legacyData)) | ||
|
|
||
| const result = getWithLruTracking('workflow:legacy') | ||
| expect(result).toEqual(legacyData) | ||
| }) | ||
|
|
||
| it('updates accessedAt when reading', () => { | ||
| const oldTime = 1000 | ||
| const entry = { accessedAt: oldTime, data: { test: 'data' } } | ||
| sessionStorage.setItem('workflow:test', JSON.stringify(entry)) | ||
|
|
||
| getWithLruTracking('workflow:test', true) | ||
|
|
||
| const stored = JSON.parse(sessionStorage.getItem('workflow:test')!) | ||
| expect(stored.accessedAt).toBeGreaterThan(oldTime) | ||
| }) | ||
|
|
||
| it('does not update accessedAt when updateAccessTime is false', () => { | ||
| const oldTime = 1000 | ||
| const entry = { accessedAt: oldTime, data: { test: 'data' } } | ||
| sessionStorage.setItem('workflow:test', JSON.stringify(entry)) | ||
|
|
||
| getWithLruTracking('workflow:test', false) | ||
|
|
||
| const stored = JSON.parse(sessionStorage.getItem('workflow:test')!) | ||
| expect(stored.accessedAt).toBe(oldTime) | ||
| }) | ||
| }) | ||
|
|
||
| describe('removeFromStorage', () => { | ||
| it('removes item from session storage', () => { | ||
| sessionStorage.setItem('key', 'value') | ||
| expect(sessionStorage.getItem('key')).toBe('value') | ||
|
|
||
| removeFromStorage('key') | ||
| expect(sessionStorage.getItem('key')).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| describe('getStorageStats', () => { | ||
| it('returns stats for all entries', () => { | ||
| const entry1 = { accessedAt: 1000, data: { a: 1 } } | ||
| const entry2 = { accessedAt: 2000, data: { b: 2 } } | ||
| sessionStorage.setItem('workflow:a', JSON.stringify(entry1)) | ||
| sessionStorage.setItem('workflow:b', JSON.stringify(entry2)) | ||
| sessionStorage.setItem('other:c', 'value') | ||
|
|
||
| const stats = getStorageStats() | ||
|
|
||
| expect(stats.totalItems).toBe(3) | ||
| expect(stats.matchingItems).toBe(3) | ||
| }) | ||
|
|
||
| it('filters entries by pattern', () => { | ||
| const entry1 = { accessedAt: 1000, data: { a: 1 } } | ||
| const entry2 = { accessedAt: 2000, data: { b: 2 } } | ||
| sessionStorage.setItem('workflow:a', JSON.stringify(entry1)) | ||
| sessionStorage.setItem('workflow:b', JSON.stringify(entry2)) | ||
| sessionStorage.setItem('other:c', 'value') | ||
|
|
||
| const stats = getStorageStats(/^workflow:/) | ||
|
|
||
| expect(stats.totalItems).toBe(3) | ||
| expect(stats.matchingItems).toBe(2) | ||
| expect(stats.entries).toHaveLength(2) | ||
| }) | ||
|
|
||
| it('sorts entries by accessedAt (oldest first)', () => { | ||
| const entry1 = { accessedAt: 2000, data: { newer: true } } | ||
| const entry2 = { accessedAt: 1000, data: { older: true } } | ||
| sessionStorage.setItem('workflow:newer', JSON.stringify(entry1)) | ||
| sessionStorage.setItem('workflow:older', JSON.stringify(entry2)) | ||
|
|
||
| const stats = getStorageStats(/^workflow:/) | ||
|
|
||
| expect(stats.entries[0].key).toBe('workflow:older') | ||
| expect(stats.entries[1].key).toBe('workflow:newer') | ||
| }) | ||
|
|
||
| it('treats legacy entries as accessedAt: 0', () => { | ||
| const legacyData = { nodes: [], links: [] } | ||
| const newEntry = { accessedAt: 1000, data: { test: true } } | ||
| sessionStorage.setItem('workflow:legacy', JSON.stringify(legacyData)) | ||
| sessionStorage.setItem('workflow:new', JSON.stringify(newEntry)) | ||
|
|
||
| const stats = getStorageStats(/^workflow:/) | ||
|
|
||
| expect(stats.entries[0].key).toBe('workflow:legacy') | ||
| expect(stats.entries[0].accessedAt).toBe(0) | ||
| }) | ||
| }) | ||
|
|
||
| describe('LRU eviction order', () => { | ||
| it('evicts oldest entries first (legacy before new)', () => { | ||
| const legacyData = { nodes: [], links: [] } | ||
| const newEntry = { accessedAt: Date.now(), data: { new: true } } | ||
|
|
||
| sessionStorage.setItem('workflow:legacy', JSON.stringify(legacyData)) | ||
| sessionStorage.setItem('workflow:new', JSON.stringify(newEntry)) | ||
|
|
||
| let callCount = 0 | ||
| const originalSetItem = sessionStorage.setItem.bind(sessionStorage) | ||
| vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => { | ||
| callCount++ | ||
| if (key === 'workflow:current' && callCount === 1) { | ||
| throw new DOMException('Quota exceeded', 'QuotaExceededError') | ||
| } | ||
| return originalSetItem(key, value) | ||
| }) | ||
|
|
||
| setWithLruEviction('workflow:current', { current: true }, /^workflow:/) | ||
|
|
||
| expect(sessionStorage.getItem('workflow:legacy')).toBeNull() | ||
| expect(sessionStorage.getItem('workflow:new')).toBeTruthy() | ||
| }) | ||
|
|
||
| it('evicts oldest new-format entries when no legacy entries exist', () => { | ||
| const oldEntry = { accessedAt: 1000, data: { old: true } } | ||
| const newEntry = { accessedAt: 2000, data: { new: true } } | ||
|
|
||
| sessionStorage.setItem('workflow:old', JSON.stringify(oldEntry)) | ||
| sessionStorage.setItem('workflow:new', JSON.stringify(newEntry)) | ||
|
|
||
| let callCount = 0 | ||
| const originalSetItem = sessionStorage.setItem.bind(sessionStorage) | ||
| vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => { | ||
| callCount++ | ||
| if (key === 'workflow:current' && callCount === 1) { | ||
| throw new DOMException('Quota exceeded', 'QuotaExceededError') | ||
| } | ||
| return originalSetItem(key, value) | ||
| }) | ||
|
|
||
| setWithLruEviction('workflow:current', { current: true }, /^workflow:/) | ||
|
|
||
| expect(sessionStorage.getItem('workflow:old')).toBeNull() | ||
| expect(sessionStorage.getItem('workflow:new')).toBeTruthy() | ||
| }) | ||
| }) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 143
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 290
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 48392
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 8193
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2216
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1538
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1029
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 961
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2577
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1903
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1165
Consider validating parsed localStorage workflow data with Zod schema.
The
as ComfyWorkflowJSONcast on line 99 accepts any parsed JSON without validation. WhileloadGraphDataincludes optional schema validation (ifComfy.Validation.Workflowsis enabled), corrupted or invalid data could slip through if validation is disabled. Following the codebase pattern, usevalidateComfyWorkflow()instead:This validates data at the entry point and aligns with existing patterns in the codebase (e.g.,
src/platform/remote/comfyui/jobs/fetchJobs.ts).🤖 Prompt for AI Agents