Repository navigation
Core: Clean stale runtime instance records - #35023
Merged
Merged
Conversation
kasperpeulen
added
maintenance
kasperpeulen
marked this pull request as ready for review
June 2, 2026 06:57
Contributor
📝 WalkthroughWalkthroughThis PR adds automatic cleanup of stale and malformed runtime instance records from disk. The implementation introduces cleanup decision logic that determines whether registry entries should be kept or removed based on file age, timestamp validity, and process liveness, integrates this cleanup into the write path before persisting new records, and extends tests to validate cleanup behavior and write robustness. ChangesRegistry cleanup mechanism
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/core-server/utils/runtime-instance-registry.test.ts (1)
205-308: ⚡ Quick winAdd exact-threshold cases for the 24h and 7d edges.
The table currently checks values just inside/outside each window, but not the exact boundaries that the helper implements with strict comparisons. Please pin at least
24hfor temp files and7dfor malformed/record entries so a future>/>=regression does not slip through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/core-server/utils/runtime-instance-registry.test.ts` around lines 205 - 308, Add tests that pin the exact threshold boundaries used by getRuntimeInstanceRegistryCleanupDecision: add cases where fileModifiedAtMs equals NOW - 24 hours (use hoursAgo(24).getTime()) for temp-file/record scenarios and where fileModifiedAtMs equals NOW - 7 days (use daysAgo(7).getTime()) for malformed-json and record scenarios; assert the expected actions that match the helper's strict comparisons (keep vs remove or check-pid) and reuse existing helpers/symbols (hoursAgo, daysAgo, NOW, recentRecord, getRuntimeInstanceRegistryCleanupDecision) so future >/>= regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/core-server/utils/runtime-instance-registry.test.ts`:
- Around line 374-445: The test is dynamically mocking 'node:fs/promises' inline
with vi.resetModules()/vi.doMock() and re-imports (creating flaky, non-top-level
mocks); instead hoist the mock to the top of runtime-instance-registry.test.ts
using vi.mock('node:fs/promises', () => ({ ...actual... }), { spy: true }),
remove vi.resetModules()/vi.doMock()/vi.doUnmock()/dynamic imports, and move
per-test behavior into beforeEach/afterEach where you call
vi.mocked(fsPromises).rm.mockImplementationOnce(...) or similar to throw EBUSY
for the lockedRecordPath; update references to use the normal static import
names (and the existing createRuntimeInstanceRecord/writeRuntimeInstanceRecord
symbols) so tests no longer re-import modules under mocked fs.
---
Nitpick comments:
In `@code/core/src/core-server/utils/runtime-instance-registry.test.ts`:
- Around line 205-308: Add tests that pin the exact threshold boundaries used by
getRuntimeInstanceRegistryCleanupDecision: add cases where fileModifiedAtMs
equals NOW - 24 hours (use hoursAgo(24).getTime()) for temp-file/record
scenarios and where fileModifiedAtMs equals NOW - 7 days (use
daysAgo(7).getTime()) for malformed-json and record scenarios; assert the
expected actions that match the helper's strict comparisons (keep vs remove or
check-pid) and reuse existing helpers/symbols (hoursAgo, daysAgo, NOW,
recentRecord, getRuntimeInstanceRegistryCleanupDecision) so future >/>=
regressions are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eaf64e6a-cccc-4bcf-86b4-9cedebd313b8
📒 Files selected for processing (2)
code/core/src/core-server/utils/runtime-instance-registry.test.tscode/core/src/core-server/utils/runtime-instance-registry.ts
Comment on lines
+374
to
+445
| const killSpy = vi.spyOn(process, 'kill').mockImplementation((pid) => { | ||
| if (pid === 202) { | ||
| throw Object.assign(new Error('missing process'), { code: 'ESRCH' }); | ||
| } | ||
|
|
||
| if (pid === 404) { | ||
| throw Object.assign(new Error('permission denied'), { code: 'EPERM' }); | ||
| } | ||
|
|
||
| return true; | ||
| }); | ||
|
|
||
| await writeCurrentRecord(registryDir); | ||
|
|
||
| expect(readdirSync(registryDir).sort()).toEqual([ | ||
| 'current-instance.json', | ||
| 'recent-malformed.json', | ||
| 'recent-valid.json', | ||
| 'recent.tmp', | ||
| 'stale-active.json', | ||
| 'stale-ambiguous.json', | ||
| ]); | ||
| expect(killSpy.mock.calls.map(([pid]) => pid).sort()).toEqual([202, 303, 404]); | ||
| }); | ||
|
|
||
| it('does not block writing the current record when one stale file cannot be removed', async () => { | ||
| vi.useFakeTimers(); | ||
| vi.setSystemTime(NOW); | ||
|
|
||
| const registryDir = makeTempDir(); | ||
| const lockedRecordPath = writeRecordFile({ | ||
| instanceId: 'locked-instance', | ||
| registryDir, | ||
| updatedAt: daysAgo(8).toISOString(), | ||
| }); | ||
| const actualFsPromises = | ||
| await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises'); | ||
|
|
||
| vi.resetModules(); | ||
| vi.doMock('node:fs/promises', () => ({ | ||
| ...actualFsPromises, | ||
| rm: vi.fn(async (path, options) => { | ||
| if (path === lockedRecordPath) { | ||
| throw Object.assign(new Error('locked'), { code: 'EBUSY' }); | ||
| } | ||
|
|
||
| return actualFsPromises.rm(path, options); | ||
| }), | ||
| })); | ||
|
|
||
| try { | ||
| const { | ||
| createRuntimeInstanceRecord: createRuntimeInstanceRecordWithMockedFs, | ||
| writeRuntimeInstanceRecord: writeRuntimeInstanceRecordWithMockedFs, | ||
| } = await import('./runtime-instance-registry.ts'); | ||
| const currentRecordPath = await writeRuntimeInstanceRecordWithMockedFs( | ||
| createRuntimeInstanceRecordWithMockedFs({ | ||
| address: 'http://localhost:7007/', | ||
| instanceId: 'current-instance', | ||
| now: NOW, | ||
| pid: 7007, | ||
| port: 7007, | ||
| storybookVersion: '10.5.0-alpha.0', | ||
| }), | ||
| registryDir | ||
| ); | ||
|
|
||
| expect(existsSync(lockedRecordPath)).toBe(true); | ||
| expect(existsSync(currentRecordPath)).toBe(true); | ||
| } finally { | ||
| vi.doUnmock('node:fs/promises'); | ||
| vi.resetModules(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the test file and inspect relevant sections
FILE="code/core/src/core-server/utils/runtime-instance-registry.test.ts"
echo "=== File exists? ==="
ls -l "$FILE"
echo "=== Relevant section (approx lines 330-520) ==="
nl -ba "$FILE" | sed -n '330,520p'
# 2) Search for vi.mock/vi.doMock/vi.spyOn/vi.resetModules in that file
echo "=== Mocking usage in file ==="
rg -n "vi\.(mock|doMock|resetModules|unmock|useFakeTimers|setSystemTime|mocked|spyOn)" "$FILE"
# 3) Read spy-mocking rules
RULES=".cursor/rules/spy-mocking.mdc"
echo "=== Reading spy-mocking rules ($RULES) ==="
ls -l "$RULES"
nl -ba "$RULES" | head -n 200Repository: storybookjs/storybook
Length of output: 312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="code/core/src/core-server/utils/runtime-instance-registry.test.ts"
RULES=".cursor/rules/spy-mocking.mdc"
echo "=== File exists? ==="
ls -l "$FILE"
echo "=== Relevant section (approx lines 330-520) ==="
# Print with line numbers using awk (no nl in sandbox)
awk 'NR>=330 && NR<=520 {printf "%6d %s\n", NR, $0}' "$FILE"
echo "=== Mocking usage in file ==="
rg -n "vi\.(mock|doMock|resetModules|unmock|useFakeTimers|setSystemTime|mocked|spyOn)" "$FILE" || true
echo "=== Reading spy-mocking rules ($RULES) ==="
ls -l "$RULES"
awk 'NR<=220 {printf "%6d %s\n", NR, $0}' "$RULES"Repository: storybookjs/storybook
Length of output: 10201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="code/core/src/core-server/utils/runtime-instance-registry.test.ts"
echo "=== vi.mock / vi.doMock occurrences in whole file ==="
rg -n "vi\.(mock|doMock)" "$FILE" || true
echo "=== Other module-reset/dynamic import related patterns ==="
rg -n "vi\.resetModules\(|vi\.import\(\'.*runtime-instance-registry\.ts\'\)|vi\.doUnmock\(" "$FILE" || true
echo "=== Is there any top-level vi.mock (outside tests)? ==="
# Best-effort: print first 120 lines where top-level declarations would usually be.
awk 'NR<=160 {print NR ":" $0}' "$FILE"Repository: storybookjs/storybook
Length of output: 4959
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="code/core/src/core-server/utils/runtime-instance-registry.test.ts"
echo "=== beforeEach / afterEach occurrences ==="
rg -n "beforeEach\(|afterEach\(" "$FILE" || true
echo "=== Mocks-related occurrences ==="
rg -n "vi\.(mocked|spyOn|mockImplementation|mockReturnValue|useFakeTimers|setSystemTime|resetModules|doMock|doUnmock)" "$FILE" || true
echo "=== Context around killSpy test (360-410) ==="
awk 'NR>=350 && NR<=410 {printf "%6d %s\n", NR, $0}' "$FILE"
echo "=== Context around node:fs/promises mock tests (400-510) ==="
awk 'NR>=395 && NR<=510 {printf "%6d %s\n", NR, $0}' "$FILE"Repository: storybookjs/storybook
Length of output: 8033
Hoist the node:fs/promises mock to top-level and use the repo’s vi.mock(..., { spy: true }) pattern
runtime-instance-registry.test.ts mocks node:fs/promises inline via vi.resetModules() + vi.doMock() (lines 412-446 and 463-496), which breaks the spy-mocking.mdc rules (top-level mocks, spy: true, configure behavior in beforeEach, and access via vi.mocked() instead of dynamic re-imports).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/core/src/core-server/utils/runtime-instance-registry.test.ts` around
lines 374 - 445, The test is dynamically mocking 'node:fs/promises' inline with
vi.resetModules()/vi.doMock() and re-imports (creating flaky, non-top-level
mocks); instead hoist the mock to the top of runtime-instance-registry.test.ts
using vi.mock('node:fs/promises', () => ({ ...actual... }), { spy: true }),
remove vi.resetModules()/vi.doMock()/vi.doUnmock()/dynamic imports, and move
per-test behavior into beforeEach/afterEach where you call
vi.mocked(fsPromises).rm.mockImplementationOnce(...) or similar to throw EBUSY
for the lockedRecordPath; update references to use the normal static import
names (and the existing createRuntimeInstanceRecord/writeRuntimeInstanceRecord
symbols) so tests no longer re-import modules under mocked fs.
huang-julien
approved these changes
Jun 2, 2026
This was referenced Jun 2, 2026
This was referenced Jul 10, 2026
Merged
This was referenced Jul 13, 2026
Merged
Open
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes #35022
What I did
process.kill(pid, 0)definitely reportsESRCH.tmpfiles only when file mtime is older than 24hcursor-team-kit:thermo-nuclear-code-quality-reviewsubagent review. The review findings were addressed by extracting the pure cleanup decision helper, converting the policy coverage to compact table tests, and fixing the unreadable-file vs malformed-JSON boundary. The final re-review found no remaining actionable maintainability issues.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Automated verification run locally:
Manual QA was performed by running real Storybook dev servers with temporary
HOMEdirectories so the runtime registry was isolated at$HOME/.storybook/instances.Commands used:
Observed:
Storybook ready!onhttp://localhost:61234/andhttp://localhost:61235/.curl -IreturnedHTTP/1.1 200 OKfor both ports while Storybook was running.SIGTERMto Storybook removed its current runtime instance record..tmpstayed.tmpwas removed{ "registry": "/tmp/storybook-dev-registry-qa-active-FTSbc7/.storybook/instances", "remaining": [ "368519e1-baca-4248-ba5b-e1d9ba7c74e2.json", "recent-valid.json", "recent.tmp", "stale-active.json" ], "expectedPresent": [ "recent-valid.json", "recent.tmp", "stale-active.json" ], "expectedAbsent": [ "stale-inactive.json", "old-valid-active-pid.json", "old.tmp" ], "pass": true }Documentation
MIGRATION.MD
No docs update: this is internal runtime registry maintenance behavior.
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
Release Notes
Bug Fixes
Tests