fix(variables): correct scripted-variable persistence races and cross-request propagation#8406
Open
sanish-bruno wants to merge 6 commits into
Open
Conversation
…o manage stale updates - Introduced a bounded sliding window mechanism for tracking recent script request UIDs, allowing for better handling of stale updates from superseded or cancelled requests. - Updated relevant functions to utilize the new `isStaleScriptRequest` utility for improved clarity and maintainability. - Enhanced the persistence logic to ensure that updates from retired requests do not interfere with newer requests, improving data integrity during concurrent operations. - Added comprehensive tests to validate the behavior of the new UID management system and its impact on variable propagation across requests.
…untime tests - Eliminated the test case that checked for the inclusion of persistentEnvVariables in the result, as it is no longer relevant. - Cleaned up the test suite to enhance clarity and maintainability, focusing on relevant assertions for variable management.
…ove stale request handling - Updated the `persistActiveEnvironment` and related functions to eliminate the requestUid parameter, streamlining the logic for environment persistence. - Removed outdated checks for stale updates from superseded requests, enhancing clarity and maintainability. - Deleted the `_scriptRequestUids` management code and associated tests, as they are no longer necessary for the current implementation. - Improved the handling of environment updates to ensure accurate state management without stale data interference.
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
sanish-bruno
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRemoves request-based stale-update gating from scripted environment and collection-variable updates, serializes environment file writes per path, and propagates script-returned collection variables back into the in-memory collection root. Adds tests and fixtures for cross-request collection-variable propagation. ChangesCollection Variable Propagation & File Lock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
- Deleted the `hasJsExtension` utility function from the filesystem module as it was no longer needed. - Cleaned up the exports to enhance clarity and maintainability of the codebase.
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (1)
2480-2486: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize collection-root variable saves as well.
With request UID gating removed, rapid script collection-var updates can issue multiple
renderer:save-collection-rootcalls. That IPC handler still does an unlocked read/compare/write, so an older root save can finish last and drop newer script-written collection variables.Suggested follow-up in `renderer:save-collection-root`
const filePath = path.join(collectionPathname, filename); - const existing = fs.existsSync(filePath) ? fs.readFileSync(filePath, 'utf8') : null; - if (content === existing) return; // skip write if content unchanged - await writeFile(filePath, content); + await withFileLock(filePath, async () => { + const existing = fs.existsSync(filePath) ? fs.readFileSync(filePath, 'utf8') : null; + if (content === existing) return; // skip write if content unchanged + await writeFile(filePath, content); + });🤖 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 `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js` around lines 2480 - 2486, The collection-root save path in the collection actions is still firing concurrent renderer:save-collection-root IPC calls, which can let an older write overwrite newer script updates. Serialize these saves in the save flow around findCollectionByUid, transformCollectionRootToSave, and the ipcRenderer.invoke call, or add the same single-flight/queueing protection inside renderer:save-collection-root so root variable writes are processed in order and cannot race.
🤖 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 `@packages/bruno-electron/src/ipc/collection.js`:
- Around line 773-782: The environment file save path in the collection IPC
handler already uses withFileLock, but renderer:update-environment-color still
updates the same file without that lock, which can cause races and lost changes.
Update the renderer:update-environment-color flow to use the same withFileLock
protection around its read-modify-write sequence on the environment file, and
keep the write path consistent with the existing collection save logic in
collection.js so all environment-file writers serialize through the same lock.
In `@packages/bruno-electron/src/store/workspace-environments.js`:
- Around line 189-196: The global environment save path still has an unprotected
write in updateGlobalEnvironmentColor, so it can race with the existing
withFileLock block around envFile.filePath. Update updateGlobalEnvironmentColor
to use the same file lock when persisting the color change, and ensure any write
to the environment file goes through the locked section so it cannot overwrite
concurrent variable saves.
In `@packages/bruno-electron/src/utils/filesystem.js`:
- Line 140: The filename check in isBrunoJsonFile is too loose because
endsWith('bruno.json') matches unrelated names like mybruno.json; update this
helper to verify the exact basename is bruno.json, using the existing
isBrunoJsonFile symbol in filesystem.js, so only the intended config file is
recognized.
- Around line 115-124: The lock key in withFileLock still uses path.resolve(),
which does not collapse symlink targets or case-only variants into the same
queue. Update the keying logic in withFileLock to use a filesystem-canonical
path (for example via a realpath-based helper) so all references to the same
file serialize through one lock and the _pathLocks map no longer splits
equivalent env file paths.
In `@packages/bruno-electron/tests/utils/filesystem.spec.js`:
- Around line 27-178: The file-lock tests are using POSIX-only hardcoded path
literals, which makes them brittle across platforms. Update the
`withFileLock(...)` test calls to use a shared path helper such as
`lockPath(...)` built with `path.join()` or `path.resolve()` instead of `/p/...`
strings. Keep the changes localized to the `filesystem.spec.js` cases that
exercise `withFileLock` so the locking behavior is still validated without
relying on platform-specific separators.
In
`@tests/scripting/bru-api/collection-var-multi-request/init-user-data/collection-security.json`:
- Line 4: The templated collection path is hardcoding a forward slash, which
makes the bootstrap path OS-specific and can break lookups on Windows. Update
the init-user-data entries in collection-security.json and preferences.json to
build the collection path using a fully joined, OS-agnostic path rather than
concatenating with “/”. Keep the same collectionPath placeholder, but ensure the
stored path is assembled through the existing path-joining mechanism used
elsewhere in the bootstrap data.
---
Outside diff comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`:
- Around line 2480-2486: The collection-root save path in the collection actions
is still firing concurrent renderer:save-collection-root IPC calls, which can
let an older write overwrite newer script updates. Serialize these saves in the
save flow around findCollectionByUid, transformCollectionRootToSave, and the
ipcRenderer.invoke call, or add the same single-flight/queueing protection
inside renderer:save-collection-root so root variable writes are processed in
order and cannot race.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a24115cd-fe55-4980-8470-5ae187b7df21
📒 Files selected for processing (19)
packages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/providers/ReduxStore/slices/global-environments.jspackages/bruno-electron/src/ipc/collection.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/store/workspace-environments.jspackages/bruno-electron/src/utils/filesystem.jspackages/bruno-electron/tests/utils/filesystem.spec.jspackages/bruno-js/tests/runtime.spec.jstests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.brutests/scripting/bru-api/collection-var-multi-request/collection-var-multi-request.spec.tstests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/bruno.jsontests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/collection.brutests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/environments/Test.brutests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-1-write.brutests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-2-read-and-write.brutests/scripting/bru-api/collection-var-multi-request/init-user-data/collection-security.jsontests/scripting/bru-api/collection-var-multi-request/init-user-data/preferences.json
💤 Files with no reviewable changes (2)
- tests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.bru
- packages/bruno-js/tests/runtime.spec.js
…pdates - Updated the environment persistence logic in both `collection.js` and `workspace-environments.js` to utilize file locking during read and write operations. - This change ensures that concurrent access to environment files is managed safely, preventing potential data corruption and enhancing reliability during updates.
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
1 participant
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
Three fixes to scripted variable persistence (
bru.setEnvVar/setCollectionVar/setGlobalEnvVar):1. Per-path file lock for env writes (
withFileLockinfilesystem.js)Wraps the read-then-write critical sections in
renderer:save-environmentand the global-envsavepath. Without serialization, two rapid scripted persists on the same env file interleave and drop one update. Per-path async mutex; different files still write in parallel.2. Collection-var propagation across requests in one run (
applyCollectionVarsToCollectionRootinnetwork/index.js)mergeVarsrebuildsrequest.collectionVariablesfromcollection.rootevery iteration, so a script'ssetCollectionVarwas invisible to the next request unless we wrote it back to root. Helper rebuilds the enabled slice from script output (missing keys → deleted), preserves disabled vars, writes tocollection.rootandcollection.draft?.root.3. Removes the
_scriptRequestUidstale-update gate#8315 added a gate that dropped script-emitted updates from "superseded" requests. Removing it because:
bru.runRequest, dropping legitimate in-flight updates.Deletes the field, the 6 call sites, the
deletein_clearScriptCollectionBaselines, and therequestUidthread-through inpersistActiveEnvironment/collectionVariablesUpdateEvent/globalEnvironmentsUpdateEvent.Behavior change: if a user cancels a request after its script has called
setEnvVar(..., {persist:true})but before the update reaches the renderer, the update now lands — same as if the script had errored, or the request had taken 50ms longer. Worth a changelog note.Scope / non-goals
Parallel folder-runner iterations (
runInParallel: true) share one env state and onecollection.rootacross iterations. The file lock fixes the disk-write race on the shared env file, but cross-iteration variable visibility (iteration A'ssetEnvVarbecoming visible inside iteration B's script) is accepted behavior and intentionally not addressed here. Isolating per-iteration state would be a separate architectural change.Tests
filesystem.spec.js—withFileLocksemantics (serialization, per-path independence, error isolation, queue cleanup, read-modify-write).tests/scripting/bru-api/collection-var-multi-request— Playwright e2e: req-1 writescounter='1', req-2 must read it.runtime.spec.jstest asserting absence ofpersistentEnvVariables(meaningless since persistence is default).Contribution Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests