diff --git a/packages/bruno-app/src/providers/App/useIpcEvents.js b/packages/bruno-app/src/providers/App/useIpcEvents.js index e14fc2caf..2b34b8877 100644 --- a/packages/bruno-app/src/providers/App/useIpcEvents.js +++ b/packages/bruno-app/src/providers/App/useIpcEvents.js @@ -209,7 +209,7 @@ const useIpcEvents = () => { const removeScriptEnvUpdateListener = ipcRenderer.on('main:script-environment-update', (val) => { dispatch(scriptEnvironmentUpdateEvent(val)); if (val.collectionUid) { - dispatch(persistActiveEnvironment(val.collectionUid, val.requestUid)); + dispatch(persistActiveEnvironment(val.collectionUid)); } }); diff --git a/packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js b/packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js index 16b7f2c0a..31e9089a3 100644 --- a/packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js +++ b/packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js @@ -505,8 +505,6 @@ export const wsConnectOnly = (item, collectionUid) => (dispatch, getState) => { const environment = findEnvironmentInCollection(collectionCopy, collectionCopy.activeEnvironmentUid); // WS connect does not run user scripts — no baseline to clear. - // Wiping baselines here would also wipe collection._scriptRequestUid, opening - // a window where a late HTTP post-response could pass the stale-update gate. connectWS(itemCopy, collectionCopy, environment, collectionCopy.runtimeVariables, { connectOnly: true }) .then(resolve) @@ -2412,15 +2410,11 @@ export const clearScriptVariableBaselines = (collectionUid) => (dispatch) => { dispatch(_clearScriptGlobalEnvBaseline()); }; -export const persistActiveEnvironment = (collectionUid, requestUid) => (dispatch, getState) => { +export const persistActiveEnvironment = (collectionUid) => (dispatch, getState) => { const state = getState(); const collection = findCollectionByUid(state.collections.collections, collectionUid); if (!collection) return; - // Ignore stale updates from superseded requests so an in-flight pre/post - // from request N-1 can't trigger a disk write for request N. - if (requestUid && collection._scriptRequestUid && requestUid !== collection._scriptRequestUid) return; - const environment = findEnvironmentInCollection(collection, collection.activeEnvironmentUid); if (!environment) return; @@ -2441,18 +2435,13 @@ export const persistActiveEnvironment = (collectionUid, requestUid) => (dispatch .catch((err) => console.error('Failed to persist environment during script execution:', err)); }; -export const collectionVariablesUpdateEvent = ({ collectionVariables, collectionUid, requestUid }) => (dispatch, getState) => { +export const collectionVariablesUpdateEvent = ({ collectionVariables, collectionUid }) => (dispatch, getState) => { if (!collectionVariables || !collectionUid) return; const state = getState(); const collection = findCollectionByUid(state.collections.collections, collectionUid); if (!collection) return; - // Ignore stale updates from superseded requests. - if (requestUid && collection._scriptRequestUid && requestUid !== collection._scriptRequestUid) { - return; - } - const savedVars = get(collection, 'root.request.vars.req', []); const draftVars = collection.draft?.root ? get(collection, 'draft.root.request.vars.req', null) diff --git a/packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js b/packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js index fcfa08278..ffd05245d 100644 --- a/packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js +++ b/packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js @@ -459,12 +459,6 @@ export const collectionsSlice = createSlice({ const collection = findCollectionByUid(state.collections, collectionUid); if (collection) { - // Ignore stale updates from superseded requests so an in-flight pre/post - // from request N-1 can't clobber state for request N. - if (requestUid && collection._scriptRequestUid && requestUid !== collection._scriptRequestUid) { - return; - } - const activeEnvironment = findEnvironmentInCollection(collection, collection.activeEnvironmentUid); if (activeEnvironment) { @@ -503,12 +497,9 @@ export const collectionsSlice = createSlice({ } }, runtimeVariablesUpdateEvent: (state, action) => { - const { collectionUid, runtimeVariables, requestUid } = action.payload; + const { collectionUid, runtimeVariables } = action.payload; const collection = findCollectionByUid(state.collections, collectionUid); if (collection) { - if (requestUid && collection._scriptRequestUid && requestUid !== collection._scriptRequestUid) { - return; - } collection.runtimeVariables = runtimeVariables; } }, @@ -2782,9 +2773,6 @@ export const collectionsSlice = createSlice({ if (!collection) return; delete collection._scriptEnvBaseline; delete collection._scriptCollVarBaseline; - // Also drop the inflight request UID so updates from WS/OAuth2 paths (which - // don't dispatch initRunRequestEvent) aren't gated out by a stale HTTP UID. - delete collection._scriptRequestUid; }, collectionAddFileEvent: (state, action) => { const file = action.payload.file; @@ -3099,7 +3087,6 @@ export const collectionsSlice = createSlice({ delete collection._scriptEnvBaseline; delete collection._scriptCollVarBaseline; - collection._scriptRequestUid = requestUid; const item = findItemInCollection(collection, itemUid); if (!item) return; @@ -3258,7 +3245,6 @@ export const collectionsSlice = createSlice({ // pre-flush snapshot. delete collection._scriptEnvBaseline; delete collection._scriptCollVarBaseline; - collection._scriptRequestUid = action.payload.requestUid || null; collection.runnerResult.items.push({ uid: request.uid, @@ -3307,8 +3293,10 @@ export const collectionsSlice = createSlice({ if (type === 'runner-request-skipped') { const item = collection.runnerResult.items.findLast((i) => i.uid === request.uid); - item.status = 'skipped'; - item.responseReceived = action.payload.responseReceived; + if (item) { + item.status = 'skipped'; + item.responseReceived = action.payload.responseReceived; + } } if (type === 'post-response-script-execution') { diff --git a/packages/bruno-app/src/providers/ReduxStore/slices/global-environments.js b/packages/bruno-app/src/providers/ReduxStore/slices/global-environments.js index a024c414e..b451bbcea 100644 --- a/packages/bruno-app/src/providers/ReduxStore/slices/global-environments.js +++ b/packages/bruno-app/src/providers/ReduxStore/slices/global-environments.js @@ -284,19 +284,11 @@ export const deleteGlobalEnvironment = ({ environmentUid }) => (dispatch, getSta }); }; -export const globalEnvironmentsUpdateEvent = ({ globalEnvironmentVariables, collectionUid, requestUid }) => (dispatch, getState) => { +export const globalEnvironmentsUpdateEvent = ({ globalEnvironmentVariables }) => (dispatch, getState) => { if (!globalEnvironmentVariables) return; const state = getState(); - // Ignore stale updates from superseded requests on the originating collection. - if (collectionUid && requestUid) { - const sourceCollection = state?.collections?.collections?.find((c) => c.uid === collectionUid); - if (sourceCollection?._scriptRequestUid && requestUid !== sourceCollection._scriptRequestUid) { - return; - } - } - const globalEnvironments = state?.globalEnvironments?.globalEnvironments || []; const environmentUid = state?.globalEnvironments?.activeGlobalEnvironmentUid; const environment = globalEnvironments?.find((env) => env?.uid == environmentUid); diff --git a/packages/bruno-electron/src/ipc/collection.js b/packages/bruno-electron/src/ipc/collection.js index bc2b759c6..ae683462d 100644 --- a/packages/bruno-electron/src/ipc/collection.js +++ b/packages/bruno-electron/src/ipc/collection.js @@ -58,7 +58,8 @@ const { isBrunoConfigFile, isBruEnvironmentConfig, isCollectionRootBruFile, - scanForBrunoFiles + scanForBrunoFiles, + withFileLock } = require('../utils/filesystem'); const { getCollectionConfigFile, openCollection, openCollectionDialog, openCollectionsByPathname, registerScratchCollectionPath } = require('../app/collections'); const { generateUidBasedOnHash, stringifyJson, safeStringifyJSON, safeParseJSON } = require('../utils/common'); @@ -764,14 +765,21 @@ const registerRendererEventHandlers = (mainWindow, watcher) => { throw new Error(`environment: ${envFilePath} does not exist`); } - if (envHasSecrets(environment)) { - environmentSecretsStore.storeEnvSecrets(collectionPathname, environment); - } + // Serialize concurrent saves to the same env file. Without the lock the + // read-then-write pattern below can interleave: writer A reads pre-A state, + // writer B reads pre-A state, B writes B-content, A writes A-content — + // dropping B's update. Rapid scripted `bru.setEnvVar(..., {persist:true})` + // calls (e.g. across folder-run requests) hit this without serialization. + await withFileLock(envFilePath, async () => { + if (envHasSecrets(environment)) { + environmentSecretsStore.storeEnvSecrets(collectionPathname, environment); + } - const content = await stringifyEnvironment(environment, { format }); - const existing = fs.readFileSync(envFilePath, 'utf8'); - if (content === existing) return; // skip write if content unchanged - await writeFile(envFilePath, content); + const content = await stringifyEnvironment(environment, { format }); + const existing = fs.readFileSync(envFilePath, 'utf8'); + if (content === existing) return; // skip write if content unchanged + await writeFile(envFilePath, content); + }); } catch (error) { return Promise.reject(error); } @@ -909,12 +917,13 @@ const registerRendererEventHandlers = (mainWindow, watcher) => { throw new Error(`environment: ${envFilePath} does not exist`); } - // Read, update color, and write back to file - const fileContent = fs.readFileSync(envFilePath, 'utf8'); - const environment = parseEnvironment(fileContent, { format }); - environment.color = color; - const updatedContent = stringifyEnvironment(environment, { format }); - fs.writeFileSync(envFilePath, updatedContent, 'utf8'); + await withFileLock(envFilePath, async () => { + const fileContent = fs.readFileSync(envFilePath, 'utf8'); + const environment = parseEnvironment(fileContent, { format }); + environment.color = color; + const updatedContent = stringifyEnvironment(environment, { format }); + await writeFile(envFilePath, updatedContent); + }); } catch (error) { return Promise.reject(error); } diff --git a/packages/bruno-electron/src/ipc/network/apply-collection-vars.js b/packages/bruno-electron/src/ipc/network/apply-collection-vars.js new file mode 100644 index 000000000..c8cb92ce4 --- /dev/null +++ b/packages/bruno-electron/src/ipc/network/apply-collection-vars.js @@ -0,0 +1,46 @@ +const { uuid } = require('../../utils/common'); + +// Mutates `collection.root.request.vars.req` (and draft.root.request.vars.req if +// present) to reflect a script's collection-variable changes, so the next request +// in a folder/collection run's iteration picks them up through mergeVars(). +// Without this, bru.setCollectionVar() inside request N would be invisible to +// request N+1 in the same iteration (envVars/globalEnvironmentVariables already +// propagate because they're mutated in place by reference). +// +// TODO: this is a write-back patch, not the structural fix. The root cause +// is that mergeVars (utils/collection.js) rebuilds `collectionVariables` fresh +// from `collection.root.request.vars.req` on every iteration, while envVars is +// built once at runner scope and passed by reference. May need a re-wire. +const applyCollectionVarsToCollectionRoot = (collection, collectionVariables) => { + if (!collectionVariables || typeof collectionVariables !== 'object') return; + + const writeBack = (root) => { + if (!root) return; + if (!root.request) root.request = {}; + if (!root.request.vars) root.request.vars = {}; + const existing = Array.isArray(root.request.vars.req) ? root.request.vars.req : []; + + const disabled = existing.filter((v) => !v.enabled); + const enabledByName = new Map(existing.filter((v) => v.enabled).map((v) => [v.name, v])); + const scriptNames = Object.keys(collectionVariables); + + // Rebuild the enabled slice from the script's output. Keys present here are + // kept (with updated value); previously-enabled keys missing from the script + // output are treated as `bru.deleteCollectionVar` and dropped. Disabled vars + // (user-disabled in the UI) are preserved untouched. + const updatedEnabled = scriptNames.map((name) => { + const existingVar = enabledByName.get(name); + if (existingVar) { + return { ...existingVar, value: collectionVariables[name] }; + } + return { uid: uuid(), name, value: collectionVariables[name], type: 'text', enabled: true }; + }); + + root.request.vars.req = [...updatedEnabled, ...disabled]; + }; + + writeBack(collection.root); + if (collection.draft?.root) writeBack(collection.draft.root); +}; + +module.exports = { applyCollectionVarsToCollectionRoot }; diff --git a/packages/bruno-electron/src/ipc/network/apply-collection-vars.spec.js b/packages/bruno-electron/src/ipc/network/apply-collection-vars.spec.js new file mode 100644 index 000000000..d16859e3c --- /dev/null +++ b/packages/bruno-electron/src/ipc/network/apply-collection-vars.spec.js @@ -0,0 +1,160 @@ +const { applyCollectionVarsToCollectionRoot } = require('./apply-collection-vars'); + +describe('applyCollectionVarsToCollectionRoot', () => { + const makeCollection = (req = []) => ({ + root: { request: { vars: { req } } } + }); + + test('no-op when collectionVariables is null/undefined/non-object', () => { + const before = [{ uid: 'u1', name: 'a', value: '1', type: 'text', enabled: true }]; + const collection = makeCollection([...before]); + + applyCollectionVarsToCollectionRoot(collection, null); + expect(collection.root.request.vars.req).toEqual(before); + + applyCollectionVarsToCollectionRoot(collection, undefined); + expect(collection.root.request.vars.req).toEqual(before); + + applyCollectionVarsToCollectionRoot(collection, 'not-an-object'); + expect(collection.root.request.vars.req).toEqual(before); + }); + + test('updates value on existing enabled var (preserves uid/type/enabled flag)', () => { + const collection = makeCollection([ + { uid: 'u1', name: 'counter', value: '0', type: 'text', enabled: true } + ]); + + applyCollectionVarsToCollectionRoot(collection, { counter: '1' }); + + expect(collection.root.request.vars.req).toEqual([ + { uid: 'u1', name: 'counter', value: '1', type: 'text', enabled: true } + ]); + }); + + test('adds a new enabled var with generated uid for keys the script introduces', () => { + const collection = makeCollection([]); + + applyCollectionVarsToCollectionRoot(collection, { fresh: 'value' }); + + const req = collection.root.request.vars.req; + expect(req).toHaveLength(1); + expect(req[0].name).toBe('fresh'); + expect(req[0].value).toBe('value'); + expect(req[0].type).toBe('text'); + expect(req[0].enabled).toBe(true); + expect(typeof req[0].uid).toBe('string'); + expect(req[0].uid.length).toBeGreaterThan(0); + }); + + test('drops previously-enabled vars missing from the script output (delete semantics)', () => { + const collection = makeCollection([ + { uid: 'u1', name: 'keep', value: '1', type: 'text', enabled: true }, + { uid: 'u2', name: 'drop', value: '1', type: 'text', enabled: true } + ]); + + applyCollectionVarsToCollectionRoot(collection, { keep: '2' }); + + expect(collection.root.request.vars.req).toEqual([ + { uid: 'u1', name: 'keep', value: '2', type: 'text', enabled: true } + ]); + }); + + test('disabled vars are preserved untouched and not subject to delete semantics', () => { + const collection = makeCollection([ + { uid: 'u1', name: 'enabledKey', value: '1', type: 'text', enabled: true }, + { uid: 'u2', name: 'disabledKey', value: 'kept', type: 'text', enabled: false } + ]); + + // Script doesn't touch disabledKey or enabledKey; only writes a new one. + applyCollectionVarsToCollectionRoot(collection, { newKey: 'newVal' }); + + const req = collection.root.request.vars.req; + expect(req).toContainEqual({ uid: 'u2', name: 'disabledKey', value: 'kept', type: 'text', enabled: false }); + expect(req.find((v) => v.name === 'newKey')).toMatchObject({ value: 'newVal', enabled: true }); + expect(req.find((v) => v.name === 'enabledKey')).toBeUndefined(); + }); + + test('mirrors writes onto collection.draft.root when a draft exists', () => { + const collection = { + root: { request: { vars: { req: [{ uid: 'u1', name: 'a', value: '0', type: 'text', enabled: true }] } } }, + draft: { + root: { request: { vars: { req: [{ uid: 'u1', name: 'a', value: '0', type: 'text', enabled: true }] } } } + } + }; + + applyCollectionVarsToCollectionRoot(collection, { a: 'updated' }); + + expect(collection.root.request.vars.req[0].value).toBe('updated'); + expect(collection.draft.root.request.vars.req[0].value).toBe('updated'); + }); + + test('does not crash when collection.draft is absent', () => { + const collection = makeCollection([{ uid: 'u1', name: 'a', value: '0', type: 'text', enabled: true }]); + expect(() => applyCollectionVarsToCollectionRoot(collection, { a: '1' })).not.toThrow(); + }); + + test('initializes root.request.vars.req when collection.root.request.vars is missing', () => { + const collection = { root: {} }; + applyCollectionVarsToCollectionRoot(collection, { a: '1' }); + + expect(collection.root.request.vars.req).toHaveLength(1); + expect(collection.root.request.vars.req[0]).toMatchObject({ name: 'a', value: '1', enabled: true }); + }); + + test('empty collectionVariables clears all previously-enabled vars (delete-by-omission)', () => { + // The function treats `result.collectionVariables` as a full snapshot of the + // script's view of enabled vars — so an empty object means "delete every + // enabled var." This is correct ONLY if the bruno-js runtime always emits + // the full enabled snapshot (not a delta). If the runtime ever changes to + // emit `{}` when a script doesn't touch collection vars, this lock-in test + // will start surfacing data loss in folder runs. Disabled vars are still + // preserved either way. + const collection = makeCollection([ + { uid: 'u1', name: 'a', value: '1', type: 'text', enabled: true }, + { uid: 'u2', name: 'b', value: '2', type: 'text', enabled: true }, + { uid: 'u3', name: 'c', value: '3', type: 'text', enabled: false } + ]); + + applyCollectionVarsToCollectionRoot(collection, {}); + + expect(collection.root.request.vars.req).toEqual([ + { uid: 'u3', name: 'c', value: '3', type: 'text', enabled: false } + ]); + }); + + test('normalizes a non-array root.request.vars.req without crashing', () => { + const collection = { root: { request: { vars: { req: 'oops-not-an-array' } } } }; + + expect(() => applyCollectionVarsToCollectionRoot(collection, { a: '1' })).not.toThrow(); + + const req = collection.root.request.vars.req; + expect(Array.isArray(req)).toBe(true); + expect(req).toHaveLength(1); + expect(req[0]).toMatchObject({ name: 'a', value: '1', enabled: true }); + }); + + test('silently no-ops when collection.root is absent', () => { + const collection = {}; + expect(() => applyCollectionVarsToCollectionRoot(collection, { a: '1' })).not.toThrow(); + expect(collection.root).toBeUndefined(); + }); + + test('current behavior: when a script writes a name that also exists as disabled, both entries are present', () => { + // Documents the duplicate-name edge case surfaced in PR review. The script + // sees only the enabled namespace, so it can't "see" a disabled var of the + // same name — but the rebuild here doesn't dedupe by name across the two + // slices. If this becomes a real problem, the fix is to filter `disabled` + // by `!scriptNames.includes(v.name)` before concatenation. Until then, + // lock the behavior so a future change is explicit. + const collection = makeCollection([ + { uid: 'u-disabled', name: 'shared', value: 'old-disabled', type: 'text', enabled: false } + ]); + + applyCollectionVarsToCollectionRoot(collection, { shared: 'from-script' }); + + const req = collection.root.request.vars.req; + expect(req).toHaveLength(2); + expect(req).toContainEqual(expect.objectContaining({ name: 'shared', value: 'from-script', enabled: true })); + expect(req).toContainEqual({ uid: 'u-disabled', name: 'shared', value: 'old-disabled', type: 'text', enabled: false }); + }); +}); diff --git a/packages/bruno-electron/src/ipc/network/index.js b/packages/bruno-electron/src/ipc/network/index.js index 3aeb44099..8edd5a1dc 100644 --- a/packages/bruno-electron/src/ipc/network/index.js +++ b/packages/bruno-electron/src/ipc/network/index.js @@ -19,6 +19,7 @@ const { addDigestInterceptor, addEdgeGridInterceptor } = require('@usebruno/requ const prepareGqlIntrospectionRequest = require('./prepare-gql-introspection-request'); const { prepareRequest } = require('./prepare-request'); const interpolateVars = require('./interpolate-vars'); +const { applyCollectionVarsToCollectionRoot } = require('./apply-collection-vars'); const { makeAxiosInstance } = require('./axios-instance'); const { resolveInheritedSettings } = require('../../utils/collection'); const { cancelTokens, saveCancelToken, deleteCancelToken } = require('../../utils/cancel-token'); @@ -545,6 +546,7 @@ const registerNetworkIpc = (mainWindow) => { requestUid, collectionUid }); + applyCollectionVarsToCollectionRoot(collection, result.collectionVariables); } }; diff --git a/packages/bruno-electron/src/store/workspace-environments.js b/packages/bruno-electron/src/store/workspace-environments.js index 53684966e..ea94d5aee 100644 --- a/packages/bruno-electron/src/store/workspace-environments.js +++ b/packages/bruno-electron/src/store/workspace-environments.js @@ -3,7 +3,7 @@ const path = require('path'); const _ = require('lodash'); const { parseEnvironment, stringifyEnvironment } = require('@usebruno/filestore'); const { parseValueByDataType } = require('@usebruno/common/utils'); -const { writeFile, createDirectory } = require('../utils/filesystem'); +const { writeFile, createDirectory, withFileLock } = require('../utils/filesystem'); const { generateUidBasedOnHash, uuid } = require('../utils/common'); const { decryptStringSafe } = require('../utils/encryption'); const EnvironmentSecretsStore = require('./env-secrets'); @@ -183,12 +183,17 @@ class GlobalEnvironmentsManager { environment.color = color; } - if (this.envHasSecrets(environment)) { - environmentSecretsStore.storeEnvSecrets(workspacePath, environment); - } + // Serialize concurrent writes per env file. Two rapid scripted + // bru.setGlobalEnvVar() persist calls can otherwise overlap and the + // second writer's stringify+write can land before the first, dropping it. + await withFileLock(envFile.filePath, async () => { + if (this.envHasSecrets(environment)) { + environmentSecretsStore.storeEnvSecrets(workspacePath, environment); + } - const content = await stringifyEnvironment(environment, { format: 'yml' }); - await writeFile(envFile.filePath, content); + const content = await stringifyEnvironment(environment, { format: 'yml' }); + await writeFile(envFile.filePath, content); + }); return true; } catch (error) { @@ -274,11 +279,13 @@ class GlobalEnvironmentsManager { throw new Error(`Environment file not found for uid: ${environmentUid}`); } - const environment = await this.parseEnvironmentFile(envFile.filePath, workspacePath); - environment.color = color; + await withFileLock(envFile.filePath, async () => { + const environment = await this.parseEnvironmentFile(envFile.filePath, workspacePath); + environment.color = color; - const content = stringifyEnvironment(environment, { format: 'yml' }); - await writeFile(envFile.filePath, content); + const content = stringifyEnvironment(environment, { format: 'yml' }); + await writeFile(envFile.filePath, content); + }); return true; } catch (error) { diff --git a/packages/bruno-electron/src/utils/filesystem.js b/packages/bruno-electron/src/utils/filesystem.js index 1a1983f27..7fb560440 100644 --- a/packages/bruno-electron/src/utils/filesystem.js +++ b/packages/bruno-electron/src/utils/filesystem.js @@ -104,6 +104,34 @@ const writeFile = async (pathname, content, isBinary = false) => { } }; +// Per-path async mutex for file writes. Callers that perform read-modify-write +// against the same file from multiple async chains (e.g. two scripted variable +// writes racing on the same environment file) wrap the whole critical section in +// withFileLock(absPath, ...) so the second writer reads the post-first-write state. +// Without this, `fs.readFileSync` outside the lock can capture pre-A state while +// A is still flushing, and B then overwrites A. Lock entries are cleaned up once +// the queue for a path drains. +const _pathLocks = new Map(); +const withFileLock = async (pathname, fn) => { + // Canonicalize so callers passing `/foo/./bar.env` and `/foo/bar.env` (or + // trailing slashes / `..` segments) share a single lock. Does NOT normalize + // symlinks or filesystem case-insensitivity — callers passing semantically + // equivalent but different strings beyond that get separate locks. + const key = path.resolve(pathname); + const prior = _pathLocks.get(key) || Promise.resolve(); + // Errors from a prior writer must not block subsequent writers; the next caller + // gets its own try/catch. + const next = prior.catch(() => {}).then(() => fn()); + _pathLocks.set(key, next); + try { + return await next; + } finally { + if (_pathLocks.get(key) === next) { + _pathLocks.delete(key); + } + } +}; + const hasJsonExtension = (filename) => { if (!filename || typeof filename !== 'string') return false; return ['json'].some((ext) => filename.toLowerCase().endsWith(`.${ext}`)); @@ -547,6 +575,7 @@ module.exports = { isWSLPath, normalizeWSLPath, writeFile, + withFileLock, hasJsonExtension, hasBruExtension, hasRequestExtension, diff --git a/packages/bruno-electron/tests/utils/filesystem.spec.js b/packages/bruno-electron/tests/utils/filesystem.spec.js new file mode 100644 index 000000000..2c869e1d2 --- /dev/null +++ b/packages/bruno-electron/tests/utils/filesystem.spec.js @@ -0,0 +1,182 @@ +const { withFileLock } = require('../../src/utils/filesystem'); + +// Manual-gate helper: returns a Promise + a `resolve` function. Tests use this +// to deterministically interleave two async operations through withFileLock — +// without it, racing setTimeouts make the test flaky. +const deferred = () => { + let resolve, reject; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +}; + +// Drain the entire microtask queue. withFileLock chains `.catch().then(() => fn())`, +// so fn() starts two microtask hops after the synchronous call — a single +// `await Promise.resolve()` isn't enough. setImmediate runs after all queued +// microtasks resolve, giving us a clean barrier. +const flush = () => new Promise((resolve) => setImmediate(resolve)); + +describe('withFileLock', () => { + test('serializes two concurrent ops on the same path in queued order', async () => { + const events = []; + const gateA = deferred(); + const gateB = deferred(); + + const opA = withFileLock('/p/one', async () => { + events.push('A:start'); + await gateA.promise; + events.push('A:end'); + return 'A'; + }); + + // Start B while A is still inside its critical section. B must NOT start + // until A resolves — that's the whole point of the lock. + const opB = withFileLock('/p/one', async () => { + events.push('B:start'); + await gateB.promise; + events.push('B:end'); + return 'B'; + }); + + // Let microtasks settle. A should have started but B must still be queued. + await flush(); + expect(events).toEqual(['A:start']); + + // Open A's gate first; B's start should follow. + gateA.resolve(); + await flush(); + expect(events).toEqual(['A:start', 'A:end', 'B:start']); + + gateB.resolve(); + + const [a, b] = await Promise.all([opA, opB]); + expect(a).toBe('A'); + expect(b).toBe('B'); + expect(events).toEqual(['A:start', 'A:end', 'B:start', 'B:end']); + }); + + test('different paths run concurrently (lock is per-path)', async () => { + const events = []; + const gateA = deferred(); + const gateB = deferred(); + + const opA = withFileLock('/p/one', async () => { + events.push('A:start'); + await gateA.promise; + events.push('A:end'); + }); + + const opB = withFileLock('/p/two', async () => { + events.push('B:start'); + await gateB.promise; + events.push('B:end'); + }); + + // Both should be inside their critical sections concurrently. + await flush(); + expect(events.sort()).toEqual(['A:start', 'B:start']); + + // Resolve in reverse order to demonstrate true independence — B finishes + // before A despite starting in a separate concurrent path. + gateB.resolve(); + await opB; + expect(events).toContain('B:end'); + expect(events).not.toContain('A:end'); + + gateA.resolve(); + await opA; + expect(events).toContain('A:end'); + }); + + test('an error in op A does NOT block op B on the same path', async () => { + // Pre-fix: if `prior` rejected, `prior.then(...)` would also reject, leaking + // an unhandled rejection AND propagating B's chain to a rejected state before + // its own fn ran. The `.catch(() => {})` on `prior` is what makes B independent. + const opA = withFileLock('/p/lock-err', async () => { + throw new Error('boom'); + }); + + const opB = withFileLock('/p/lock-err', async () => { + return 'B-completed'; + }); + + await expect(opA).rejects.toThrow('boom'); + await expect(opB).resolves.toBe('B-completed'); + }); + + test('subsequent ops on a path after the queue drains start a fresh chain', async () => { + // Regression guard: the cleanup `if (_pathLocks.get(pathname) === next) delete` + // must remove the entry when the queue drains, so a later op on the same path + // doesn't chain off a stale (already-settled) promise. If it did, the new op + // would still run — but the lock would leak one Map entry per saved file. + const order = []; + + await withFileLock('/p/drained', async () => { + order.push('first'); + }); + await withFileLock('/p/drained', async () => { + order.push('second'); + }); + + expect(order).toEqual(['first', 'second']); + }); + + test('three concurrent ops on the same path execute strictly in queued order', async () => { + // Smoke test for the rapid-fire scripted-write scenario: a folder run + // emitting bru.setEnvVar(..., persist:true) on each of 3 back-to-back requests. + const events = []; + const gates = [deferred(), deferred(), deferred()]; + + const ops = gates.map((g, i) => + withFileLock('/p/triple', async () => { + events.push(`op-${i}:start`); + await g.promise; + events.push(`op-${i}:end`); + }) + ); + + await flush(); + expect(events).toEqual(['op-0:start']); + + gates[0].resolve(); + await flush(); + expect(events).toEqual(['op-0:start', 'op-0:end', 'op-1:start']); + + gates[1].resolve(); + await flush(); + expect(events).toEqual(['op-0:start', 'op-0:end', 'op-1:start', 'op-1:end', 'op-2:start']); + + gates[2].resolve(); + await Promise.all(ops); + expect(events).toEqual([ + 'op-0:start', 'op-0:end', + 'op-1:start', 'op-1:end', + 'op-2:start', 'op-2:end' + ]); + }); + + test('the inner fn sees post-prior-write state (read-modify-write safety)', async () => { + // The reason the lock exists. Simulate the env-save read-then-write pattern: + // two writers both read the "file," compute new content, then write. Without + // the lock the second writer's read would capture pre-first-write state and + // overwrite the first writer's update. + let onDisk = '0'; + + const write = (newContent) => + withFileLock('/p/rmw', async () => { + const existing = onDisk; // "read" + const merged = `${existing}+${newContent}`; + // Simulate a small async cost between read and write. + await new Promise((r) => setImmediate(r)); + onDisk = merged; // "write" + }); + + // Fire both concurrently; without the lock the second read would see '0' + // and overwrite '0+A' with '0+B'. With the lock the second read sees '0+A'. + await Promise.all([write('A'), write('B')]); + + expect(onDisk).toBe('0+A+B'); + }); +}); diff --git a/packages/bruno-js/tests/runtime.spec.js b/packages/bruno-js/tests/runtime.spec.js index 2a6068836..28210dbfa 100644 --- a/packages/bruno-js/tests/runtime.spec.js +++ b/packages/bruno-js/tests/runtime.spec.js @@ -2,8 +2,6 @@ const { describe, it, expect, beforeAll } = require('@jest/globals'); const TestRuntime = require('../src/runtime/test-runtime'); const ScriptRuntime = require('../src/runtime/script-runtime'); const AssertRuntime = require('../src/runtime/assert-runtime'); -const Bru = require('../src/bru'); -const VarsRuntime = require('../src/runtime/vars-runtime'); const { loader: quickJsLoader } = require('../src/sandbox/quickjs'); describe('runtime', () => { @@ -252,15 +250,6 @@ describe('runtime', () => { expect(result.globalEnvironmentVariables).toBeNull(); }); - it('should not include persistentEnvVariables in result', async () => { - const script = `bru.setEnvVar('key', 'val');`; - const runtime = new ScriptRuntime({ runtime: 'nodevm' }); - - const result = await runtime.runRequestScript(script, {}, {}, {}, '.', null, process.env); - - expect(result).not.toHaveProperty('persistentEnvVariables'); - }); - it('should include collectionVariables in result', async () => { const script = `bru.setCollectionVar('myVar', 'myValue');`; const runtime = new ScriptRuntime({ runtime: 'nodevm' }); diff --git a/tests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.bru b/tests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.bru index 59e5c1b12..6db8838d7 100644 --- a/tests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.bru +++ b/tests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.bru @@ -1,4 +1,3 @@ vars { host: https://testbench-sanity.usebruno.com } - diff --git a/tests/scripting/bru-api/collection-var-multi-request/collection-var-multi-request.spec.ts b/tests/scripting/bru-api/collection-var-multi-request/collection-var-multi-request.spec.ts new file mode 100644 index 000000000..e5dd7a06b --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/collection-var-multi-request.spec.ts @@ -0,0 +1,24 @@ +import { test } from '../../../../playwright'; +import { openCollection, selectEnvironment } from '../../../utils/page'; +import { runCollection, validateRunnerResults } from '../../../utils/page/runner'; + +test.describe('Script collection-variable propagation across requests in one runner invocation', () => { + test('request 2 observes request 1\'s setCollectionVar write', async ({ + pageWithUserData: page + }) => { + await openCollection(page, 'collection-var-multi-request-test'); + await selectEnvironment(page, 'Test'); + await runCollection(page, 'collection-var-multi-request-test'); + + // Both requests should pass: req-1 sets counter="1"; req-2 reads counter + // (must see "1"), records it in seenInReq2, then sets counter="2". + // If applyCollectionVarsToCollectionRoot didn't propagate the change to + // collection.root.request.vars.req, req-2's mergeVars would rebuild + // collectionVariables from the pre-run "0" and req-2's first test would fail. + await validateRunnerResults(page, { + totalRequests: 2, + passed: 2, + failed: 0 + }); + }); +}); diff --git a/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/bruno.json b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/bruno.json new file mode 100644 index 000000000..a87d556f0 --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/bruno.json @@ -0,0 +1,9 @@ +{ + "version": "1", + "name": "collection-var-multi-request-test", + "type": "collection", + "ignore": [ + "node_modules", + ".git" + ] +} diff --git a/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/collection.bru b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/collection.bru new file mode 100644 index 000000000..acf19e8c9 --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/collection.bru @@ -0,0 +1,7 @@ +meta { + name: collection +} + +vars:pre-request { + counter: 0 +} diff --git a/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/environments/Test.bru b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/environments/Test.bru new file mode 100644 index 000000000..6db8838d7 --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/environments/Test.bru @@ -0,0 +1,3 @@ +vars { + host: https://testbench-sanity.usebruno.com +} diff --git a/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-1-write.bru b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-1-write.bru new file mode 100644 index 000000000..78c612816 --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-1-write.bru @@ -0,0 +1,21 @@ +meta { + name: req-1-write + type: http + seq: 1 +} + +get { + url: {{host}}/ping + body: none + auth: none +} + +script:post-response { + bru.setCollectionVar("counter", "1"); +} + +tests { + test("request 1 wrote counter=1", function() { + expect(bru.getCollectionVar("counter")).to.equal("1"); + }); +} diff --git a/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-2-read-and-write.bru b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-2-read-and-write.bru new file mode 100644 index 000000000..6aed4b0e5 --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/fixtures/collections/collection-var-multi-request-test/req-2-read-and-write.bru @@ -0,0 +1,31 @@ +meta { + name: req-2-read-and-write + type: http + seq: 2 +} + +get { + url: {{host}}/ping + body: none + auth: none +} + +script:post-response { + // Request 2 must observe request 1's setCollectionVar write. The electron-side + // applyCollectionVarsToCollectionRoot in sendVariableUpdates is what makes this + // visible — without it, mergeVars rebuilds request 2's collectionVariables from + // the original collection.root and req-2's read returns "0". + const prev = bru.getCollectionVar("counter"); + bru.setCollectionVar("seenInReq2", prev); + bru.setCollectionVar("counter", "2"); +} + +tests { + test("request 2 observed request 1's collection-var write", function() { + expect(bru.getCollectionVar("seenInReq2")).to.equal("1"); + }); + + test("request 2 wrote counter=2", function() { + expect(bru.getCollectionVar("counter")).to.equal("2"); + }); +} diff --git a/tests/scripting/bru-api/collection-var-multi-request/init-user-data/collection-security.json b/tests/scripting/bru-api/collection-var-multi-request/init-user-data/collection-security.json new file mode 100644 index 000000000..9079a45f6 --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/init-user-data/collection-security.json @@ -0,0 +1,10 @@ +{ + "collections": [ + { + "path": "{{collectionPath}}/collection-var-multi-request-test", + "securityConfig": { + "jsSandboxMode": "developer" + } + } + ] +} diff --git a/tests/scripting/bru-api/collection-var-multi-request/init-user-data/preferences.json b/tests/scripting/bru-api/collection-var-multi-request/init-user-data/preferences.json new file mode 100644 index 000000000..9bf51f716 --- /dev/null +++ b/tests/scripting/bru-api/collection-var-multi-request/init-user-data/preferences.json @@ -0,0 +1,5 @@ +{ + "lastOpenedCollections": [ + "{{collectionPath}}/collection-var-multi-request-test" + ] +}