fix: handle race conditions in collection-watcher during collection deletion#7484
Open
chirag-bruno wants to merge 2 commits intousebruno:mainfrom
Open
fix: handle race conditions in collection-watcher during collection deletion#7484chirag-bruno wants to merge 2 commits intousebruno:mainfrom
chirag-bruno wants to merge 2 commits intousebruno:mainfrom
Conversation
…eletion When deleting a collection, the config file (bruno.json/opencollection.yml) may be deleted before other files, causing getCollectionFormat to throw and crash the app. Changes: - Add format caching in getCollectionFormat to preserve format after config deletion - Wrap all getCollectionFormat calls in try-catch blocks throughout collection-watcher - Add defensive error handling to isFolderRootFile, addDirectory, add, change, unlink, and unlinkDir handlers - Add clearCollectionFormatCache to clean up when collection root is deleted - Add Playwright tests to verify race condition handling
chirag-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughAdds defensive error handling around collection watcher file events, introduces collection-format cache invalidation, and adds Playwright tests to ensure the app remains responsive when collection files or folders are deleted. (46 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ion-watcher-unlink-race-condition
![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: 2
🧹 Nitpick comments (2)
tests/collection/delete-race-condition/delete-race-condition.spec.ts (2)
24-41: Consider replacingwaitForTimeoutwith a more deterministic wait.The
page.waitForTimeout(1000)at line 30 relies on timing assumptions. Per coding guidelines, this should be avoided unless absolutely necessary. If there's an observable state change (e.g., collection disappears, error indicator appears), waiting for that would be more reliable.If no UI indicator exists for this scenario, consider adding a brief comment explaining why the timeout is necessary.
Suggested improvement
await fs.promises.unlink(openCollectionYmlPath); - // Wait for watcher to process the deletion - await page.waitForTimeout(1000); + // Wait for watcher to process the deletion + // Note: No observable UI change occurs when config file is deleted, + // so a timeout is necessary to allow watcher processing + await page.waitForTimeout(1000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collection/delete-race-condition/delete-race-condition.spec.ts` around lines 24 - 41, Replace the brittle page.waitForTimeout(1000) with a deterministic wait for an observable UI/state change: after unlinking openCollectionYmlPath, wait for a specific indicator that the watcher processed the deletion (for example use Playwright's expect or page.waitForSelector to assert a collection row is gone, a "no collections" message appears, or a watcher error/toast becomes visible) instead of timing, referencing the test 'Should handle collection config file deletion without crashing' and the current use of page.waitForTimeout; if no UI change exists, add a brief comment next to the unlink and page.waitForTimeout explaining why a timeout is unavoidable.
6-60: Addtest.stepfor better report readability.Per coding guidelines,
test.stepshould be used to improve generated report clarity. Consider wrapping logical sections of each test.Example refactor for first test
test('Should handle collection config file deletion without crashing', async ({ page }) => { - const openCollectionYmlPath = path.join(fullCollectionPath, 'opencollection.yml'); - - await fs.promises.unlink(openCollectionYmlPath); - - // Wait for watcher to process the deletion - await page.waitForTimeout(1000); - - // The app should still be responsive even after the config file is deleted - await expect(page.getByTestId('collections-header-add-menu')).toBeVisible(); - await page.getByTestId('collections-header-add-menu').click(); - - // Verify dropdown appears (app is responsive) - await expect(page.locator('.tippy-box .dropdown-item').filter({ hasText: 'Create collection' })).toBeVisible(); - - // Close dropdown - await page.keyboard.press('Escape'); + await test.step('Delete collection config file', async () => { + const openCollectionYmlPath = path.join(fullCollectionPath, 'opencollection.yml'); + await fs.promises.unlink(openCollectionYmlPath); + await page.waitForTimeout(1000); + }); + + await test.step('Verify app remains responsive', async () => { + await expect(page.getByTestId('collections-header-add-menu')).toBeVisible(); + await page.getByTestId('collections-header-add-menu').click(); + await expect(page.locator('.tippy-box .dropdown-item').filter({ hasText: 'Create collection' })).toBeVisible(); + await page.keyboard.press('Escape'); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collection/delete-race-condition/delete-race-condition.spec.ts` around lines 6 - 60, Wrap logical actions in each test with Playwright's test.step to improve report readability: inside the test titled "Should handle collection config file deletion without crashing" wrap the filesystem deletion (unlink openCollectionYmlPath) and the subsequent wait/assert/click checks into descriptive test.step blocks (e.g., "Delete config file" and "Verify UI remains responsive"), and likewise in the test titled "Should handle collection folder deletion without crashing" wrap the rm call and the wait/assert/click sequence into descriptive test.step blocks (e.g., "Delete collection folder" and "Verify UI remains responsive"). Ensure you call test.step around the async operations so the steps await correctly and keep existing assertions and waits intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-electron/src/app/collection-watcher.js`:
- Around line 569-598: The unlink handler in function unlink should invalidate
the cached collection format when the collection config file itself is deleted
(e.g., basename 'opencollection.yml' or 'bruno.json' in the collection root) so
subsequent calls to getCollectionFormat don't return stale data; update unlink
(near getCollectionFormat and the early-return check for opencollection.yml) to
call a cache-clear function (e.g., clearCollectionFormatCache(collectionPath) or
invalidateCollectionFormat(collectionPath)) when you detect the config filename
in the collection root (before returning) and ensure that same invalidation
occurs for 'bruno.json' deletions as well.
In `@packages/bruno-electron/src/utils/filesystem.js`:
- Around line 238-258: The getCollectionFormat function normalizes
collectionPath for the cache but still constructs ocYmlPath and brunoJsonPath
from the raw collectionPath, causing inconsistency; update the function to use
normalizedPath when joining filenames (i.e., build ocYmlPath and brunoJsonPath
from normalizedPath) so fs.existsSync checks and cache keys use the same
canonical path (references: getCollectionFormat, collectionFormatCache,
ocYmlPath, brunoJsonPath).
---
Nitpick comments:
In `@tests/collection/delete-race-condition/delete-race-condition.spec.ts`:
- Around line 24-41: Replace the brittle page.waitForTimeout(1000) with a
deterministic wait for an observable UI/state change: after unlinking
openCollectionYmlPath, wait for a specific indicator that the watcher processed
the deletion (for example use Playwright's expect or page.waitForSelector to
assert a collection row is gone, a "no collections" message appears, or a
watcher error/toast becomes visible) instead of timing, referencing the test
'Should handle collection config file deletion without crashing' and the current
use of page.waitForTimeout; if no UI change exists, add a brief comment next to
the unlink and page.waitForTimeout explaining why a timeout is unavoidable.
- Around line 6-60: Wrap logical actions in each test with Playwright's
test.step to improve report readability: inside the test titled "Should handle
collection config file deletion without crashing" wrap the filesystem deletion
(unlink openCollectionYmlPath) and the subsequent wait/assert/click checks into
descriptive test.step blocks (e.g., "Delete config file" and "Verify UI remains
responsive"), and likewise in the test titled "Should handle collection folder
deletion without crashing" wrap the rm call and the wait/assert/click sequence
into descriptive test.step blocks (e.g., "Delete collection folder" and "Verify
UI remains responsive"). Ensure you call test.step around the async operations
so the steps await correctly and keep existing assertions and waits intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 021de5cf-0162-4d92-9997-90d1031246b5
📒 Files selected for processing (3)
packages/bruno-electron/src/app/collection-watcher.jspackages/bruno-electron/src/utils/filesystem.jstests/collection/delete-race-condition/delete-race-condition.spec.ts
Comment on lines
569
to
598
| const unlink = (win, pathname, collectionUid, collectionPath) => { | ||
| try { | ||
| if (!fs.existsSync(collectionPath)) { | ||
| return; | ||
| } | ||
| console.log(`watcher unlink: ${pathname}`); | ||
| console.log(`watcher unlink: ${pathname}`); | ||
|
|
||
| try { | ||
| if (isEnvironmentsFolder(pathname, collectionPath)) { | ||
| return unlinkEnvironmentFile(win, pathname, collectionUid); | ||
| } | ||
|
|
||
| let format; | ||
| try { | ||
| format = getCollectionFormat(collectionPath); | ||
| } catch (error) { | ||
| console.error(`Error getting collection format for: ${collectionPath}`, error); | ||
| return; | ||
| } | ||
| const format = getCollectionFormat(collectionPath); | ||
| if (hasRequestExtension(pathname, format)) { | ||
| const basename = path.basename(pathname); | ||
| const dirname = path.dirname(pathname); | ||
|
|
||
| if (basename === 'opencollection.yml' && path.normalize(dirname) === path.normalize(collectionPath)) { | ||
| return; | ||
| } | ||
|
|
||
| const file = { | ||
| meta: { | ||
| collectionUid, | ||
| pathname, | ||
| name: basename | ||
| } | ||
| }; | ||
| win.webContents.send('main:collection-tree-updated', 'unlink', file); | ||
| } | ||
| } catch (err) { | ||
| console.error(`Error processing unlink event for: ${pathname}`, err); | ||
| } catch (error) { | ||
| console.error(`Error in unlink handler for ${pathname}:`, error); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Missing cache invalidation when config file is unlinked.
When opencollection.yml or bruno.json is deleted via unlink (not unlinkDir), getCollectionFormat at line 577 returns the cached value, but the cache is never cleared. Subsequent calls from other handlers (e.g., loadBrunoConfig in openapi-sync.js, IPC handlers in collection.js) will use stale cached format and attempt to read a non-existent file.
Consider clearing the cache when the config file itself is deleted.
Proposed fix
const unlink = (win, pathname, collectionUid, collectionPath) => {
console.log(`watcher unlink: ${pathname}`);
try {
if (isEnvironmentsFolder(pathname, collectionPath)) {
return unlinkEnvironmentFile(win, pathname, collectionUid);
}
+ const basename = path.basename(pathname);
+ const dirname = path.dirname(pathname);
+
+ // Clear cache if config file is deleted
+ if (path.normalize(dirname) === path.normalize(collectionPath)) {
+ if (basename === 'opencollection.yml' || basename === 'bruno.json') {
+ clearCollectionFormatCache(collectionPath);
+ return;
+ }
+ }
+
const format = getCollectionFormat(collectionPath);
if (hasRequestExtension(pathname, format)) {
- const basename = path.basename(pathname);
- const dirname = path.dirname(pathname);
-
- if (basename === 'opencollection.yml' && path.normalize(dirname) === path.normalize(collectionPath)) {
- return;
- }
-
const file = {
meta: {
collectionUid,
pathname,
name: basename
}
};
win.webContents.send('main:collection-tree-updated', 'unlink', file);
}
} catch (error) {
console.error(`Error in unlink handler for ${pathname}:`, error);
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/app/collection-watcher.js` around lines 569 -
598, The unlink handler in function unlink should invalidate the cached
collection format when the collection config file itself is deleted (e.g.,
basename 'opencollection.yml' or 'bruno.json' in the collection root) so
subsequent calls to getCollectionFormat don't return stale data; update unlink
(near getCollectionFormat and the early-return check for opencollection.yml) to
call a cache-clear function (e.g., clearCollectionFormatCache(collectionPath) or
invalidateCollectionFormat(collectionPath)) when you detect the config filename
in the collection root (before returning) and ensure that same invalidation
occurs for 'bruno.json' deletions as well.
Comment on lines
238
to
258
| const getCollectionFormat = (collectionPath) => { | ||
| const normalizedPath = path.normalize(collectionPath); | ||
|
|
||
| if (collectionFormatCache.has(normalizedPath)) { | ||
| return collectionFormatCache.get(normalizedPath); | ||
| } | ||
|
|
||
| const ocYmlPath = path.join(collectionPath, 'opencollection.yml'); | ||
| if (fs.existsSync(ocYmlPath)) { | ||
| collectionFormatCache.set(normalizedPath, 'yml'); | ||
| return 'yml'; | ||
| } | ||
|
|
||
| const brunoJsonPath = path.join(collectionPath, 'bruno.json'); | ||
| if (fs.existsSync(brunoJsonPath)) { | ||
| collectionFormatCache.set(normalizedPath, 'bru'); | ||
| return 'bru'; | ||
| } | ||
|
|
||
| throw new Error(`No collection configuration found at: ${collectionPath}`); | ||
| }; |
Contributor
There was a problem hiding this comment.
Path normalization inconsistency in getCollectionFormat.
The function normalizes collectionPath for cache operations (lines 239, 241-242, 247, 253) but uses the raw collectionPath for fs.existsSync checks (lines 245, 251-252). If input paths vary in format (e.g., trailing slashes, mixed separators), this could cause cache misses or incorrect lookups.
Proposed fix
const getCollectionFormat = (collectionPath) => {
const normalizedPath = path.normalize(collectionPath);
if (collectionFormatCache.has(normalizedPath)) {
return collectionFormatCache.get(normalizedPath);
}
- const ocYmlPath = path.join(collectionPath, 'opencollection.yml');
+ const ocYmlPath = path.join(normalizedPath, 'opencollection.yml');
if (fs.existsSync(ocYmlPath)) {
collectionFormatCache.set(normalizedPath, 'yml');
return 'yml';
}
- const brunoJsonPath = path.join(collectionPath, 'bruno.json');
+ const brunoJsonPath = path.join(normalizedPath, 'bruno.json');
if (fs.existsSync(brunoJsonPath)) {
collectionFormatCache.set(normalizedPath, 'bru');
return 'bru';
}
- throw new Error(`No collection configuration found at: ${collectionPath}`);
+ throw new Error(`No collection configuration found at: ${normalizedPath}`);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/utils/filesystem.js` around lines 238 - 258, The
getCollectionFormat function normalizes collectionPath for the cache but still
constructs ocYmlPath and brunoJsonPath from the raw collectionPath, causing
inconsistency; update the function to use normalizedPath when joining filenames
(i.e., build ocYmlPath and brunoJsonPath from normalizedPath) so fs.existsSync
checks and cache keys use the same canonical path (references:
getCollectionFormat, collectionFormatCache, ocYmlPath, brunoJsonPath).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bruno-electron/src/app/collection-watcher.js (1)
572-585:⚠️ Potential issue | 🟠 MajorInvalidate format cache when root config files are deleted in
unlink.At Line 583,
opencollection.ymlunlinks return early without clearing cache, andbruno.jsonunlinks are not handled. This leaves stalegetCollectionFormat()state until (or unless) root directory deletion happens.Proposed fix
const unlink = (win, pathname, collectionUid, collectionPath) => { console.log(`watcher unlink: ${pathname}`); try { if (isEnvironmentsFolder(pathname, collectionPath)) { return unlinkEnvironmentFile(win, pathname, collectionUid); } - console.log(`watcher unlink: ${pathname}`); + const basename = path.basename(pathname); + const dirname = path.dirname(pathname); + const isCollectionRootPath = path.normalize(dirname) === path.normalize(collectionPath); + + if (isCollectionRootPath && (basename === 'opencollection.yml' || basename === 'bruno.json')) { + clearCollectionFormatCache(collectionPath); + return; + } const format = getCollectionFormat(collectionPath); if (hasRequestExtension(pathname, format)) { - const basename = path.basename(pathname); - const dirname = path.dirname(pathname); - - if (basename === 'opencollection.yml' && path.normalize(dirname) === path.normalize(collectionPath)) { - return; - } - const file = { meta: { collectionUid, pathname, name: basename } }; win.webContents.send('main:collection-tree-updated', 'unlink', file); } } catch (error) { console.error(`Error in unlink handler for ${pathname}:`, error); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/collection-watcher.js` around lines 572 - 585, The unlink handler returns early for root config files (e.g., opencollection.yml) and doesn't invalidate the collection format cache, and it also doesn't handle bruno.json deletions; update the unlink branch in collection-watcher.js (the block using isEnvironmentsFolder, unlinkEnvironmentFile, getCollectionFormat, hasRequestExtension) so that when a root config file is unlinked (basename === 'opencollection.yml' at the collection root or basename === 'bruno.json' at the collection root) you explicitly invalidate the cached format for that collection (e.g., call a new or existing cache invalidation helper like clearCollectionFormatCache(collectionPath) or remove the entry used by getCollectionFormat) before returning; ensure both opencollection.yml and bruno.json cases are covered.
🧹 Nitpick comments (1)
packages/bruno-electron/src/app/collection-watcher.js (1)
570-577: Remove duplicate unlink logging.Line 570 and Line 576 log the same message; keeping one avoids noisy watcher logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/collection-watcher.js` around lines 570 - 577, The watcher currently logs the same "watcher unlink: ${pathname}" message twice; remove the duplicate console.log so only one log remains before the conditional. In the collection-watcher.js block around the console.log, keep the initial console.log(`watcher unlink: ${pathname}`) (or the later one if preferred) and delete the second identical call immediately after the isEnvironmentsFolder/unlinkEnvironmentFile check to avoid noisy duplicate logs while preserving the isEnvironmentsFolder(pathname, collectionPath) and unlinkEnvironmentFile(win, pathname, collectionUid) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-electron/src/app/collection-watcher.js`:
- Around line 572-585: The unlink handler returns early for root config files
(e.g., opencollection.yml) and doesn't invalidate the collection format cache,
and it also doesn't handle bruno.json deletions; update the unlink branch in
collection-watcher.js (the block using isEnvironmentsFolder,
unlinkEnvironmentFile, getCollectionFormat, hasRequestExtension) so that when a
root config file is unlinked (basename === 'opencollection.yml' at the
collection root or basename === 'bruno.json' at the collection root) you
explicitly invalidate the cached format for that collection (e.g., call a new or
existing cache invalidation helper like
clearCollectionFormatCache(collectionPath) or remove the entry used by
getCollectionFormat) before returning; ensure both opencollection.yml and
bruno.json cases are covered.
---
Nitpick comments:
In `@packages/bruno-electron/src/app/collection-watcher.js`:
- Around line 570-577: The watcher currently logs the same "watcher unlink:
${pathname}" message twice; remove the duplicate console.log so only one log
remains before the conditional. In the collection-watcher.js block around the
console.log, keep the initial console.log(`watcher unlink: ${pathname}`) (or the
later one if preferred) and delete the second identical call immediately after
the isEnvironmentsFolder/unlinkEnvironmentFile check to avoid noisy duplicate
logs while preserving the isEnvironmentsFolder(pathname, collectionPath) and
unlinkEnvironmentFile(win, pathname, collectionUid) behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b936f1c-31bb-4d06-a61f-2ad05b434f20
📒 Files selected for processing (1)
packages/bruno-electron/src/app/collection-watcher.js
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.
Description
When deleting a collection, the config file (bruno.json/opencollection.yml) may be deleted before other files, causing getCollectionFormat to throw and crash the app.
Changes:
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Bug Fixes
Tests