Conversation
…tocol
Resolves critical mutex timeout error during local workspace initialization.
## Problem
- Renderer made RPC calls before background window finished loading
- Electron queued IPC messages internally for ~30 seconds
- Actual work took only 4-5s, but total time was 33+ seconds
- Mutex timeout (10s) fired, causing workspace load failures
- 100% reproduction rate on cold start
## Root Cause
Electron's undocumented IPC message queueing behavior:
1. Renderer makes RPC call 1s after app start
2. Main forwards to background window immediately
3. Background hasn't finished loading yet
4. Electron queues message for ~30s
5. Background finishes loading at 4s
6. Message delivered 30s later (!)
7. Work completes in 4.6s
8. Total: 33+ seconds
## Solution: Background Readiness Protocol
Three-part coordination mechanism:
1. Background signals readiness after RPC handlers registered
- src/renderer/index.js: sends 'background-process-ready' IPC event
2. Main process buffers calls until ready
- src/main/actions/setupIPCForwarding.js:
- Tracks background ready state
- Buffers incoming calls in pendingCalls[]
- Flushes immediately when ready signal received
3. Unique reply channels per call
- src/renderer/lib/RPCServiceOverIPC.ts:
- Extracts replyChannel from message payload
- Prevents race conditions with concurrent calls
## Additional Improvements
- Enhanced retry logic for 'No handler registered' errors
- Comprehensive logging at every layer ([RPC], [IPC-MAIN], [IPC-HANDLER], [PERF])
- Moved background active flag to after 'did-finish-load'
- Added performance tracking throughout build process
## Results
- Reduced initialization time: 33s → 9.6s (71% improvement)
- Eliminated IPC queue delays (30s → 0s)
- No false mutex timeouts
- Maintains resilience with retry logic
## Files Changed
- src/main/actions/setupIPCForwarding.js (buffering logic)
- src/main/actions/startBackgroundProcess.js (lifecycle timing)
- src/renderer/index.js (ready signal)
- src/renderer/lib/RPCServiceOverIPC.ts (unique reply channels)
- src/renderer/actions/local-sync/fs-manager-builder.rpc-service.ts (perf logging)
- src/renderer/actions/local-sync/fs-manager.rpc-service.ts (perf logging)
## Testing
Verified on cold start with multiple concurrent workspace loads:
- All calls buffered correctly during background initialization
- Flushed immediately on background ready signal
- No mutex timeouts
- Total time: 9-10s consistently
Related: ENGG-5325
WalkthroughAdds a background readiness protocol and per-call reply-channel handling for IPC. setupIPCForwarding.js now tracks background readiness, buffers webapp→background calls in pendingQueues, assigns per-call replyChannel and callId, enforces per-call timeouts, and flushes queued calls when the background signals ready. renderer/index.js signals the main process that the renderer's background service is ready after initializing the service. RPCServiceOverIPC.ts accepts two envelope formats (legacy payload-only and new { payload, replyChannel }), normalizes arguments, and routes replies to the supplied reply channel while preserving backward compatibility. Public function signatures are unchanged. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/lib/RPCServiceOverIPC.ts (1)
23-23:⚠️ Potential issue | 🟡 MinorRemove debug artifact.
"DBG-1: method name"is a leftover debug log that will be noisy in production.Proposed fix
- console.log("DBG-1: method name", method.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/RPCServiceOverIPC.ts` at line 23, Remove the leftover debug console.log that prints "DBG-1: method name" and method.name in RPCServiceOverIPC.ts; locate the statement referencing console.log("DBG-1: method name", method.name) (near the RPCServiceOverIPC handling logic / method invocation code) and delete it or replace it with a non-verbose logger call if intentional (e.g., use existing logging utilities instead of console.log) so no debug artifact remains in production.
🧹 Nitpick comments (4)
src/renderer/index.js (1)
44-45: Consider usingloggerinstead ofconsole.logfor consistency.
loggeris imported on line 3 but these initialization/readiness messages useconsole.log. The error handlers above (lines 13-19) uselogger.error. Consider usinglogger.logorlogger.infohere for consistent log routing.Also applies to: 63-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/index.js` around lines 44 - 45, Replace the raw console.log calls used for initialization/readiness with the existing logger to ensure consistent routing: locate the console.log("[BACKGROUND] Starting initialization...") call and the other console.log usages around the 63-69 region in src/renderer/index.js and change them to use logger.info (or logger.log if your project prefers) while keeping the original message text; ensure imports/usage reference the already-imported logger symbol and that log level matches surrounding error handlers (which use logger.error).src/renderer/actions/local-sync/fs-manager.rpc-service.ts (2)
174-191: ThewaitForInit()hack adds a fixed 800ms to every workspace initialization.Now that the background readiness protocol properly buffers calls until handlers are registered, is this arbitrary 800ms delay still needed? It was presumably a workaround for the same timing issue this PR solves. Removing it could shave ~800ms off initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/actions/local-sync/fs-manager.rpc-service.ts` around lines 174 - 191, The code contains an unnecessary fixed 800ms wait in FsManagerRPCService.init() using waitForInit() that artificially inflates startup time; remove the arbitrary delay and its PERF logs (the console.log about 800ms, waitStart/waitEnd timing, and related totalDuration checks), and rely on the existing background readiness protocol which already buffers calls until handlers are registered; update any remaining references to fsMgrInitStart/totalDuration only if they become unused to avoid dead variables.
26-40: Consider gating or removing the extensive[PERF]logging before merging to production.There are ~15
console.log/warn/errorcalls added acrossinit()for performance tracing. These are valuable during debugging but add noise in production. Consider wrapping them behind a debug flag, using a proper log-level system, or planning to strip them in a follow-up.Also applies to: 49-51, 170-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/actions/local-sync/fs-manager.rpc-service.ts` around lines 26 - 40, The added verbose `[PERF]` console logging in FsManagerRPCService.init (e.g., the fsMgrInitStart/fsMgrInitEnd timing code and console.log/console.error calls around this.fsManager.init()) should be gated behind a runtime debug/logging flag or replaced with the project's logger at an appropriate level; update the init method to check a boolean (e.g., `this.debugPerf` or use the existing logger) before emitting these messages, and apply the same gating to the other PERF prints referenced (lines near 49-51 and 170-191) so they are no-ops in production unless explicit debugging is enabled.src/renderer/lib/RPCServiceOverIPC.ts (1)
39-67: Verbose performance logging — consider gating behind a debug flag or log level.The
[IPC-HANDLER]logs on every call (receipt, method completion timing, reply send timing) will be noisy in production. Consider gating these behind a debug/verbose flag or using a proper log level so they can be silenced outside of development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/lib/RPCServiceOverIPC.ts` around lines 39 - 67, The IPC handler is emitting verbose console logs on every call (callId, timing, reply) which should be gated for production; wrap or replace the console.log calls around callId, method timing, send timing and the initial receipt log with a debug/verbose check (e.g., isDebug or logLevel) or route them to a proper logger.debug method; update the block that uses callId, startTime, method, actualReplyChannel and ipcRenderer.send so logs only run when the debug flag is true (or use logger.debug) to avoid noisy production output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 16-30: The buffered pending call flush drops the original
startTime because the destructuring in the pendingCalls loop omits startTime, so
forwardToBackground falls back to performance.now() and skews metrics; update
the destructuring when shifting from pendingCalls to include startTime and pass
that startTime through to forwardToBackground (preserving its existing parameter
name) so the original timing is used instead of recomputing it inside
forwardToBackground.
- Around line 88-108: Add a timeout guard to the reply listener so promises
can't hang: when you register ipcMain.once(replyChannel, ...) capture the
timeout ID and also set a setTimeout (e.g., 5–30s) that will remove the
listener, log a timeout including eventName and callId, and resolve the awaiting
promise with an error/timeout payload; clear the timeout when the reply arrives.
Apply the same pattern to the webapp-direction listener (the other ipcMain.once
registration) so both replyChannel handlers clean up their listeners, clear
timers on success, and resolve with an explicit error on timeout to avoid
resource leaks.
- Around line 3-5: The module-level flags isBackgroundReady and pendingCalls are
not reset when the background window is destroyed/recreated, causing
setupIPCForwardingToBackground to skip reinitialization; modify
setupIPCForwardingToBackground to reset isBackgroundReady = false and
pendingCalls.length = 0 (or reassign a new empty array) at its start, or
alternatively scope these variables to the backgroundWindow instance and clear
them in the backgroundWindow 'closed' handler (ensure any
ipcMain.once("background-process-ready") behavior aligns with the reset so the
new ready event can fire and queued calls will be replayed).
In `@src/renderer/lib/RPCServiceOverIPC.ts`:
- Around line 33-37: The fallback currently sets actualArgs = args ||
incomingData which incorrectly treats a present-but-falsy payload (e.g.,
payload: null) as missing and forwards the whole envelope; update the
ipcRenderer.on handler (the async callback that destructures { payload: args,
replyChannel }) to detect presence of the payload key (use "payload" in
incomingData or incomingData.hasOwnProperty("payload")) and only use
incomingData as the old-format fallback when the payload property is absent;
similarly determine actualReplyChannel by checking for replyChannel presence
before falling back to `reply-${channelName}` so replyChannel values that are
falsy but intentionally present are preserved (see variables actualArgs and
actualReplyChannel and the setupIPCForwarding-related envelope).
---
Outside diff comments:
In `@src/renderer/lib/RPCServiceOverIPC.ts`:
- Line 23: Remove the leftover debug console.log that prints "DBG-1: method
name" and method.name in RPCServiceOverIPC.ts; locate the statement referencing
console.log("DBG-1: method name", method.name) (near the RPCServiceOverIPC
handling logic / method invocation code) and delete it or replace it with a
non-verbose logger call if intentional (e.g., use existing logging utilities
instead of console.log) so no debug artifact remains in production.
---
Nitpick comments:
In `@src/renderer/actions/local-sync/fs-manager.rpc-service.ts`:
- Around line 174-191: The code contains an unnecessary fixed 800ms wait in
FsManagerRPCService.init() using waitForInit() that artificially inflates
startup time; remove the arbitrary delay and its PERF logs (the console.log
about 800ms, waitStart/waitEnd timing, and related totalDuration checks), and
rely on the existing background readiness protocol which already buffers calls
until handlers are registered; update any remaining references to
fsMgrInitStart/totalDuration only if they become unused to avoid dead variables.
- Around line 26-40: The added verbose `[PERF]` console logging in
FsManagerRPCService.init (e.g., the fsMgrInitStart/fsMgrInitEnd timing code and
console.log/console.error calls around this.fsManager.init()) should be gated
behind a runtime debug/logging flag or replaced with the project's logger at an
appropriate level; update the init method to check a boolean (e.g.,
`this.debugPerf` or use the existing logger) before emitting these messages, and
apply the same gating to the other PERF prints referenced (lines near 49-51 and
170-191) so they are no-ops in production unless explicit debugging is enabled.
In `@src/renderer/index.js`:
- Around line 44-45: Replace the raw console.log calls used for
initialization/readiness with the existing logger to ensure consistent routing:
locate the console.log("[BACKGROUND] Starting initialization...") call and the
other console.log usages around the 63-69 region in src/renderer/index.js and
change them to use logger.info (or logger.log if your project prefers) while
keeping the original message text; ensure imports/usage reference the
already-imported logger symbol and that log level matches surrounding error
handlers (which use logger.error).
In `@src/renderer/lib/RPCServiceOverIPC.ts`:
- Around line 39-67: The IPC handler is emitting verbose console logs on every
call (callId, timing, reply) which should be gated for production; wrap or
replace the console.log calls around callId, method timing, send timing and the
initial receipt log with a debug/verbose check (e.g., isDebug or logLevel) or
route them to a proper logger.debug method; update the block that uses callId,
startTime, method, actualReplyChannel and ipcRenderer.send so logs only run when
the debug flag is true (or use logger.debug) to avoid noisy production output.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/actions/setupIPCForwarding.js (1)
127-139:⚠️ Potential issue | 🟠 Major
setupIPCForwardingToWebAppuses non-unique reply channels — concurrent calls to the sameeventNamewill collide.Line 133 uses
\reply-${eventName}`as the reply channel. If two concurrent calls share the sameeventName, the secondipcMain.oncereplaces the first listener (sinceonce` registers a new one, and the first reply resolves whichever listener fires). This causes one call to get the other's response and the other to hang.The same per-call unique
replyChannelpattern used in the background direction should be applied here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/setupIPCForwarding.js` around lines 127 - 139, The current setupIPCForwardingToWebApp handler registers a non-unique listener on `ipcMain.once('reply-${eventName}', ...)` which can collide for concurrent calls; change it to generate a per-call unique reply channel (e.g., `const replyChannel = \`reply-${eventName}-${uniqueId}\`` using a UUID or timestamp+random), include that `replyChannel` in the payload sent via `webAppWindow.webContents.send(eventName, { actualPayload, replyChannel })`, and listen on `ipcMain.once(replyChannel, (responseEvent, responsePayload) => resolve(responsePayload))` so each invocation of `setupIPCForwardingToWebApp` is isolated; ensure the uniqueId generation and `replyChannel` usage replaces the existing `reply-${eventName}` occurrences in `setupIPCForwardingToWebApp`.
🧹 Nitpick comments (1)
src/main/actions/setupIPCForwarding.js (1)
62-64:callIdgeneration usessubstring(7)which can yield an empty or very short suffix.
Math.random().toString(36)produces"0."followed by a variable-length base-36 string (often 10-11 chars total).substring(7)skips the first 7 characters and returns whatever remains — which could be as few as 3-4 chars, reducing collision resistance.Consider
substring(2)(skip"0.") or usecrypto.randomUUID()for guaranteed uniqueness.Proposed fix
- const callId = `${Date.now()}-${Math.random() - .toString(36) - .substring(7)}`; + const callId = `${Date.now()}-${Math.random() + .toString(36) + .substring(2)}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/setupIPCForwarding.js` around lines 62 - 64, The callId generation (variable callId in setupIPCForwarding.js) uses Math.random().toString(36).substring(7) which can produce very short suffixes; replace it with a stronger, consistent ID strategy — prefer using crypto.randomUUID() for a guaranteed unique id, or if unavailable use Date.now() plus Math.random().toString(36).substring(2) (skip "0.") or a crypto-based hex (e.g., crypto.randomBytes) to build the suffix; update the code that assigns callId to use one of these approaches and keep the same template string pattern so downstream consumers of callId are unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 14-24: The timeout handler using readyTimeoutId currently drains
pendingCalls once but leaves isBackgroundReady false so future incoming calls
pushed into pendingCalls hang indefinitely; add a boolean flag (e.g.,
isBackgroundFailed) and set it to true inside the BACKGROUND_READY_TIMEOUT_MS
timeout branch after draining pendingCalls, then update the code path that
currently pushes new calls into pendingCalls (the function handling incoming IPC
calls around where pendingCalls is modified) to check isBackgroundFailed and
immediately resolve/reject with the same timeout error instead of buffering when
isBackgroundFailed is true; keep existing behavior for the original drain and
ensure any ready signal still toggles isBackgroundReady appropriately without
clearing the failed flag unless you explicitly want to allow retries.
---
Outside diff comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 127-139: The current setupIPCForwardingToWebApp handler registers
a non-unique listener on `ipcMain.once('reply-${eventName}', ...)` which can
collide for concurrent calls; change it to generate a per-call unique reply
channel (e.g., `const replyChannel = \`reply-${eventName}-${uniqueId}\`` using a
UUID or timestamp+random), include that `replyChannel` in the payload sent via
`webAppWindow.webContents.send(eventName, { actualPayload, replyChannel })`, and
listen on `ipcMain.once(replyChannel, (responseEvent, responsePayload) =>
resolve(responsePayload))` so each invocation of `setupIPCForwardingToWebApp` is
isolated; ensure the uniqueId generation and `replyChannel` usage replaces the
existing `reply-${eventName}` occurrences in `setupIPCForwardingToWebApp`.
---
Duplicate comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 3-9: The module-level flags isBackgroundReady and pendingCalls are
never reset, so modify setupIPCForwardingToBackground to reset isBackgroundReady
= false and pendingCalls.length = 0 at the start of the function; ensure any
existing ipcMain.once("background-process-ready") listener behavior still works
by re-attaching the once handler inside setupIPCForwardingToBackground after
resetting state, and confirm the rest of the logic that consumes pendingCalls
and flips isBackgroundReady runs against the new background window instance.
- Around line 127-139: The ipcMain.once reply listener in
setupIPCForwardingToWebApp (inside setupIPCForwardingToWebApp -> ipcMain.handle
handler) can hang forever; add a per-call timeout like the forwardToBackground
pattern: when creating the Promise, start a timer (e.g., setTimeout) that
rejects or resolves with a timeout error/value after the desired delay, and
clear that timer inside the ipcMain.once callback when a reply arrives; ensure
you also remove the ipcMain.once listener or clear any resources on timeout so
the handler (and webAppWindow.webContents.send(eventName, actualPayload)) cannot
leave dangling listeners.
---
Nitpick comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 62-64: The callId generation (variable callId in
setupIPCForwarding.js) uses Math.random().toString(36).substring(7) which can
produce very short suffixes; replace it with a stronger, consistent ID strategy
— prefer using crypto.randomUUID() for a guaranteed unique id, or if unavailable
use Date.now() plus Math.random().toString(36).substring(2) (skip "0.") or a
crypto-based hex (e.g., crypto.randomBytes) to build the suffix; update the code
that assigns callId to use one of these approaches and keep the same template
string pattern so downstream consumers of callId are unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 69-71: The callId construction (variable callId) uses
Math.random().toString(36).substring(7) which can return an empty suffix; update
the random suffix generation so it strips the leading "0." (use substring(2) or
slice(2) on the toString(36) result) instead of starting at index 7 and
optionally limit the suffix length (e.g., take a fixed number of characters
after removing "0.") to ensure a non-empty, bounded random suffix.
- Around line 11-61: Hoist a mutable module-scoped readyTimeoutId and a
module-scoped backgroundReadyHandler reference and, at the top of
setupIPCForwardingToBackground, if readyTimeoutId exists call
clearTimeout(readyTimeoutId) and if backgroundReadyHandler exists call
ipcMain.removeListener("background-process-ready", backgroundReadyHandler) to
remove the stale listener; then create a new handler function (assign it to
backgroundReadyHandler) and assign its timeout id to the module-scoped
readyTimeoutId so subsequent calls can clear the previous timer and listener;
keep existing logic that updates isBackgroundReady/isBackgroundFailed and
flushes pendingCalls inside the new backgroundReadyHandler.
- Around line 63-108: Before calling ipcMain.handle(...) for the
"forward-event-from-webapp-to-background-and-await-reply" channel, call
ipcMain.removeHandler("forward-event-from-webapp-to-background-and-await-reply")
to unregister any previous handler; this ensures repeated invocations of the
setup (e.g., after background restart) won't throw "Attempted to register a
second handler" and won't keep the old closure referencing the destroyed
backgroundWindow. Update the setup that registers the handler (the
ipcMain.handle(...) block in setupIPCForwarding.js) to remove the handler first,
then register the new handler; keep the existing channel string and handler
function signature (event, incomingData) so other logic (pendingCalls,
forwardToBackground, isBackgroundReady/isBackgroundFailed) continues to work
unchanged.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/actions/setupIPCForwarding.js (2)
161-165:⚠️ Potential issue | 🟡 Minor
ipcMain.onaccumulates stale listeners on each re-invocation, risking crashes and duplicate deliveries.Unlike
ipcMain.handle,ipcMain.onsilently stacks listeners. On a second call tosetupIPCForwardingToWebApp, the old handler (closing over the destroyedwebAppWindow) stays registered alongside the new one. When a message arrives, both fire: the stale handler throws ondestroyedWindow.webContents.send(...)and the message is delivered twice to the live window.🔧 Proposed fix
+ ipcMain.removeAllListeners("send-from-background-to-webapp"); + ipcMain.on("send-from-background-to-webapp", (event, incomingData) => { const { payload, channel } = incomingData; console.log("Sending to webapp", channel, payload); webAppWindow.webContents.send(channel, payload); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/setupIPCForwarding.js` around lines 161 - 165, The current ipcMain.on handler for "send-from-background-to-webapp" in setupIPCForwardingToWebApp accumulates listeners and can close over a destroyed webAppWindow; fix by removing any existing listener before adding a new one (or register a single persistent handler): deregister the old handler for "send-from-background-to-webapp" (via ipcMain.removeListener or ipcMain.removeAllListeners for that channel) or keep a named handler reference and call ipcMain.removeListener(handler) prior to ipcMain.on, then re-register the handler that calls webAppWindow.webContents.send so you don't stack stale closures referencing a destroyed webAppWindow.
147-159:⚠️ Potential issue | 🟠 MajorTwo issues in
setupIPCForwardingToWebApp: missingremoveHandlerguard and shared reply-channel race condition.1. Missing
ipcMain.removeHandler(crash on re-invocation)
setupIPCForwardingToBackgroundalready hasipcMain.removeHandler(IPC_FORWARD_CHANNEL)at line 27 for exactly this reason.setupIPCForwardingToWebAppis missing the equivalent. On macOS,app.on("activate")can triggercreateWindow()when the dock icon is clicked with no windows open, causingsetupIPCForwardingToWebAppto be invoked a second time. This throws"Attempted to register a second handler for 'forward-event-from-background-to-webapp-and-await-reply'", leaving the old handler bound to the destroyed window.2. Shared reply channel — concurrent same-event calls corrupt each other
ipcMain.once(reply-${eventName})at line 153 has no per-callcallId. Node'sEventEmitterfires all registered listeners when the channel fires, so two concurrent calls to the sameeventNamewill both resolve with the first reply and the second actual reply is silently dropped. The background→webapp direction correctly avoids this by generating a uniquereplyChannelper call (line 83); the reverse direction should do the same.🔧 Proposed fix
export const setupIPCForwardingToWebApp = (webAppWindow) => { + ipcMain.removeHandler("forward-event-from-background-to-webapp-and-await-reply"); + ipcMain.handle( "forward-event-from-background-to-webapp-and-await-reply", async (event, incomingData) => { return new Promise((resolve) => { const { actualPayload, eventName } = incomingData; - ipcMain.once(`reply-${eventName}`, (responseEvent, responsePayload) => { + const callId = `${Date.now()}-${Math.random().toString(36).substring(2)}`; + const replyChannel = `reply-${eventName}-${callId}`; + ipcMain.once(replyChannel, (responseEvent, responsePayload) => { resolve(responsePayload); }); - webAppWindow.webContents.send(eventName, actualPayload); + webAppWindow.webContents.send(eventName, { payload: actualPayload, replyChannel }); }); } );Note: The webapp-side handler for these events will also need updating to read
replyChannelfrom the envelope and reply to it, analogous to howRPCServiceOverIPC.tshandles the background direction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/setupIPCForwarding.js` around lines 147 - 159, The handler setup in setupIPCForwardingToWebApp registers ipcMain.handle for "forward-event-from-background-to-webapp-and-await-reply" without first calling ipcMain.removeHandler and uses ipcMain.once(`reply-${eventName}`) which causes race conditions for concurrent calls; fix by first calling ipcMain.removeHandler("forward-event-from-background-to-webapp-and-await-reply") before ipcMain.handle registration, and change the reply channel to a per-call unique channel (e.g., generate a callId/nonce inside the async handler), send the event to webAppWindow.webContents with an envelope that includes that replyChannel, and listen for ipcMain.once on that unique reply channel (e.g., `reply-${callId}-${eventName}`) so each concurrent call resolves its own promise.
🧹 Nitpick comments (1)
src/main/actions/setupIPCForwarding.js (1)
125-133: Optional: preferremoveListenerwith a named handler reference overremoveAllListeners.
ipcMain.removeAllListeners(replyChannel)works correctly here becausereplyChannelis unique per call, but it's semantically over-broad. Extracting the handler allowsremoveListenerto be scoped precisely and avoids any surprise if a second listener were ever added to that channel.♻️ Proposed refactor
- const callTimeoutId = setTimeout(() => { - ipcMain.removeAllListeners(replyChannel); - resolve({ success: false, data: `IPC call timed out: ${eventName}` }); - }, PER_CALL_TIMEOUT_MS); - - ipcMain.once(replyChannel, (responseEvent, responsePayload) => { - clearTimeout(callTimeoutId); - resolve(responsePayload); - }); + let callTimeoutId; + const onReply = (responseEvent, responsePayload) => { + clearTimeout(callTimeoutId); + resolve(responsePayload); + }; + ipcMain.once(replyChannel, onReply); + + callTimeoutId = setTimeout(() => { + ipcMain.removeListener(replyChannel, onReply); + resolve({ success: false, data: `IPC call timed out: ${eventName}` }); + }, PER_CALL_TIMEOUT_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/actions/setupIPCForwarding.js` around lines 125 - 133, The current timeout uses ipcMain.removeAllListeners(replyChannel) which is too broad; instead define the response handler as a named function (e.g., responseHandler) and register it with ipcMain.once(replyChannel, responseHandler), then in the timeout callback call ipcMain.removeListener(replyChannel, responseHandler) before resolving; likewise inside responseHandler clear the timeout (clearTimeout(callTimeoutId)) and resolve the payload — this scopes removal to the exact listener and avoids removing any other listeners on replyChannel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 161-165: The current ipcMain.on handler for
"send-from-background-to-webapp" in setupIPCForwardingToWebApp accumulates
listeners and can close over a destroyed webAppWindow; fix by removing any
existing listener before adding a new one (or register a single persistent
handler): deregister the old handler for "send-from-background-to-webapp" (via
ipcMain.removeListener or ipcMain.removeAllListeners for that channel) or keep a
named handler reference and call ipcMain.removeListener(handler) prior to
ipcMain.on, then re-register the handler that calls
webAppWindow.webContents.send so you don't stack stale closures referencing a
destroyed webAppWindow.
- Around line 147-159: The handler setup in setupIPCForwardingToWebApp registers
ipcMain.handle for "forward-event-from-background-to-webapp-and-await-reply"
without first calling ipcMain.removeHandler and uses
ipcMain.once(`reply-${eventName}`) which causes race conditions for concurrent
calls; fix by first calling
ipcMain.removeHandler("forward-event-from-background-to-webapp-and-await-reply")
before ipcMain.handle registration, and change the reply channel to a per-call
unique channel (e.g., generate a callId/nonce inside the async handler), send
the event to webAppWindow.webContents with an envelope that includes that
replyChannel, and listen for ipcMain.once on that unique reply channel (e.g.,
`reply-${callId}-${eventName}`) so each concurrent call resolves its own
promise.
---
Duplicate comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 151-157: The promise returned in the ipc forwarding handler can
hang because ipcMain.once(`reply-${eventName}`) may never fire; update the
handler in setupIPCForwarding.js to use a timeout: when creating the Promise for
incomingData (using actualPayload and eventName), start a timer (e.g.,
configurable constant) that will remove the ipcMain listener
(ipcMain.removeListener or use a named callback) and reject the promise with a
timeout error if no reply arrives in time; when the reply does arrive in the
ipcMain listener, clear the timeout and resolve with responsePayload as before.
Ensure the cleanup removes the listener and the timer in both success and
timeout paths to avoid leaks (references: ipcMain.once / removeListener,
webAppWindow.webContents.send, incomingData/eventName/actualPayload).
---
Nitpick comments:
In `@src/main/actions/setupIPCForwarding.js`:
- Around line 125-133: The current timeout uses
ipcMain.removeAllListeners(replyChannel) which is too broad; instead define the
response handler as a named function (e.g., responseHandler) and register it
with ipcMain.once(replyChannel, responseHandler), then in the timeout callback
call ipcMain.removeListener(replyChannel, responseHandler) before resolving;
likewise inside responseHandler clear the timeout (clearTimeout(callTimeoutId))
and resolve the payload — this scopes removal to the exact listener and avoids
removing any other listeners on replyChannel.
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.
…tocol
Resolves critical mutex timeout error during local workspace initialization.
Problem
Root Cause
Electron's undocumented IPC message queueing behavior:
Solution: Background Readiness Protocol
Three-part coordination mechanism:
Background signals readiness after RPC handlers registered
Main process buffers calls until ready
Unique reply channels per call
Additional Improvements
Results
Files Changed
Testing
Verified on cold start with multiple concurrent workspace loads:
Related: ENGG-5325
Summary by CodeRabbit
Refactor
Bug Fixes