Telemetry: Await pending operations in getLastEvents to prevent race conditions #33285
Conversation
valentinpalkovic
added
bug
telemetry
ci:normal
patch:yes
|
View your CI Pipeline Execution ↗ for commit b7d3251
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughgetLastEvents now awaits the global operation promise before reading cached lastEvents, ensuring any in-flight cache.set completes prior to retrieval. A test suite was added to verify this race-condition prevention. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/telemetry/event-cache.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/telemetry/event-cache.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/telemetry/event-cache.test.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/telemetry/event-cache.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/telemetry/event-cache.test.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/telemetry/event-cache.test.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/telemetry/event-cache.test.ts
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/core/src/telemetry/event-cache.test.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/telemetry/event-cache.test.ts
🧠 Learnings (13)
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest
Applied to files:
code/core/src/telemetry/event-cache.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/telemetry/event-cache.test.ts
🧬 Code graph analysis (1)
code/core/src/telemetry/event-cache.test.ts (2)
scripts/eslint-plugin-local-rules/storybook-monorepo-imports.js (1)
cache(5-5)code/core/src/telemetry/event-cache.ts (1)
getLastEvents(36-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/telemetry/event-cache.test.ts (1)
1-11: Top-level Vitest imports, cache mocking, and serializer look consistentThe vitest imports,
vi.mock('storybook/internal/common', { spy: true }), and use ofMockInstance/vi.mockedalign with the repo’s spy-mocking rules, and theprint: (val: unknown)annotation is a nice strict-typing touch. No changes needed here.As per coding guidelines, this follows the expected vi.mock spy pattern.
| describe('race condition prevention', () => { | ||
| let cacheGetMock: MockInstance; | ||
| let cacheSetMock: MockInstance; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| cacheGetMock = vi.mocked(cache.get); | ||
| cacheSetMock = vi.mocked(cache.set); | ||
| }); | ||
|
|
||
| it('getLastEvents waits for pending set operations to complete', async () => { | ||
| const initialData = { | ||
| init: { timestamp: 1, body: { eventType: 'init', eventId: 'init-1' } }, | ||
| }; | ||
| const updatedData = { | ||
| init: { timestamp: 1, body: { eventType: 'init', eventId: 'init-1' } }, | ||
| upgrade: { timestamp: 2, body: { eventType: 'upgrade', eventId: 'upgrade-1' } }, | ||
| }; | ||
|
|
||
| // Use a simple delay to simulate async operations | ||
| let setGetResolved = false; | ||
| let setSetResolved = false; | ||
|
|
||
| cacheGetMock.mockImplementationOnce(async () => { | ||
| while (!setGetResolved) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
| return initialData; | ||
| }); | ||
|
|
||
| cacheSetMock.mockImplementationOnce(async () => { | ||
| while (!setSetResolved) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
| }); | ||
|
|
||
| // Mock cache.get to return updated data after set completes | ||
| cacheGetMock.mockResolvedValueOnce(updatedData); | ||
|
|
||
| // Start a set operation (this will be pending) | ||
| const setPromiseResult = set('upgrade', { eventType: 'upgrade', eventId: 'upgrade-1' }); | ||
|
|
||
| // Immediately call getLastEvents() - it should wait for set() to complete | ||
| const getPromise = getLastEvents(); | ||
|
|
||
| // Verify that getLastEvents hasn't resolved yet (it's waiting) | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
|
|
||
| // Resolve the set operations | ||
| setGetResolved = true; | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| setSetResolved = true; | ||
| await setPromiseResult; | ||
|
|
||
| // Now getLastEvents should complete and return the updated data | ||
| const result = await getPromise; | ||
|
|
||
| // Verify that getLastEvents waited for set to complete and got the updated data | ||
| expect(result).toEqual(updatedData); | ||
| expect(cacheGetMock).toHaveBeenCalledTimes(2); // Once in setHelper, once in getLastEvents | ||
| expect(cacheSetMock).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/core/src/telemetry/event-cache.test.ts | head -250 | tail -100Repository: storybookjs/storybook
Length of output: 4044
🏁 Script executed:
cat -n code/core/src/telemetry/event-cache.test.ts | sed -n '170,260p'Repository: storybookjs/storybook
Length of output: 3234
🏁 Script executed:
cat -n code/core/src/telemetry/event-cache.ts | head -150Repository: storybookjs/storybook
Length of output: 2992
🏁 Script executed:
ast-grep --pattern 'export const getLastEvents = $_' code/core/src/telemetry/Repository: storybookjs/storybook
Length of output: 703
🏁 Script executed:
rg -n "getLastEvents|^export|^const set" code/core/src/telemetry/event-cache.ts | head -40Repository: storybookjs/storybook
Length of output: 615
🏁 Script executed:
cat -n code/core/src/telemetry/event-cache.test.ts | sed -n '170,250p'Repository: storybookjs/storybook
Length of output: 3234
🏁 Script executed:
wc -l code/core/src/telemetry/event-cache.ts code/core/src/telemetry/event-cache.test.tsRepository: storybookjs/storybook
Length of output: 171
🏁 Script executed:
sed -n '170,250p' code/core/src/telemetry/event-cache.test.tsRepository: storybookjs/storybook
Length of output: 2695
🏁 Script executed:
cat code/core/src/telemetry/event-cache.ts | head -150Repository: storybookjs/storybook
Length of output: 2453
🏁 Script executed:
find code/core/src/telemetry -name "*event*cache*" -type fRepository: storybookjs/storybook
Length of output: 147
🏁 Script executed:
find code/core/src -name "*cache*" -type f 2>/dev/nullRepository: storybookjs/storybook
Length of output: 279
🏁 Script executed:
fd "event.*cache|cache.*event" code/core/src 2>/dev/null | head -20Repository: storybookjs/storybook
Length of output: 147
🏁 Script executed:
ls -la code/core/src/ 2>/dev/null | head -30Repository: storybookjs/storybook
Length of output: 1835
🏁 Script executed:
git ls-files | grep -i "telemetry\|event-cache" | head -20Repository: storybookjs/storybook
Length of output: 1001
🏁 Script executed:
git ls-files | grep test.ts | head -20Repository: storybookjs/storybook
Length of output: 1047
🏁 Script executed:
git log --name-only --pretty=format: --all -- "*event-cache*" 2>/dev/null | grep -v "^$" | head -10Repository: storybookjs/storybook
Length of output: 230
🏁 Script executed:
find code/core -maxdepth 2 -type d | sortRepository: storybookjs/storybook
Length of output: 1137
🏁 Script executed:
git status --short | grep event-cache || echo "No event-cache in status"Repository: storybookjs/storybook
Length of output: 89
🏁 Script executed:
git show HEAD:code/core/src/telemetry/event-cache.test.ts 2>/dev/null | head -50 || echo "File not found in HEAD"Repository: storybookjs/storybook
Length of output: 1770
🏁 Script executed:
find . -name "*spy*mocking*" -o -name "*.cursorrules" 2>/dev/nullRepository: storybookjs/storybook
Length of output: 111
🏁 Script executed:
ls -la .cursor* .cursorrules* 2>/dev/null || echo "No cursor rule files found"Repository: storybookjs/storybook
Length of output: 488
🏁 Script executed:
echo "Analysis complete"Repository: storybookjs/storybook
Length of output: 82
Race-condition test doesn't actually prove that getLastEvents waits for set() to finish, and mock setup violates Vitest guidelines
The test has two issues:
-
Test lacks verification of non-resolution: The second
cache.getis pre-stubbed withmockResolvedValueOnce(updatedData), allowinggetLastEventsto resolve immediately even without awaiting the pendingsetoperation. The test should track resolution state and assert thatgetPromiseremains pending before releasing the gates:+ let getResolved = false; const getPromise = getLastEvents().then((value) => { + getResolved = true; return value; }); // Verify that getLastEvents hasn't resolved yet (it's waiting) await new Promise((resolve) => setTimeout(resolve, 50)); + expect(getResolved).toBe(false);
-
Mock implementations must be in
beforeEachblocks: ThemockImplementationOncecalls at lines 206 and 213, andmockResolvedValueOnceat line 220 are inline within the test. Per spy-mocking guidelines, move all mock behavior configuration to the existingbeforeEachblock to ensure consistent mocking patterns and prevent regressions.
Committable suggestion skipped: line range outside the PR's diff.
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.