fix(cli): persist remove-agent + session rename before responding#1055
Conversation
📝 WalkthroughWalkthroughTwo session remote event handlers in the renderer now await immediate disk persistence before responding to CLI clients, closing persistence race conditions where the CLI received success responses before the renderer's debounced storage flush could complete. Delete and rename operations both synchronously flush their mutations via ChangesSession Remote Event Persistence
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR extends the synchronous
Confidence Score: 3/5The rename handler sends a success response unconditionally even when the disk flush fails, meaning the exact race the PR is fixing can still occur on any setMany error path. The rename handler's unconditional sendRemoteRenameSessionResponse(responseChannel, true) call undermines the core guarantee: if setMany throws, the CLI receives a success signal but the disk still has the old name, leaving a follow-up show agent reading stale data - the identical symptom the PR set out to eliminate. The delete handler and the overall approach are sound; it is specifically the rename error path that breaks the contract. src/renderer/hooks/remote/useAppRemoteEventListeners.ts - the rename handler's post-flush response logic needs a second look. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI
participant WebServer as Web Server Factory
participant Renderer as Renderer (IPC handler)
participant ReactState as React State (setSessions)
participant Disk as Disk (setMany)
Note over CLI,Disk: maestro:remoteRenameSession (after this PR)
CLI->>WebServer: rename session
WebServer->>Renderer: dispatch CustomEvent
Renderer->>ReactState: setSessions (schedules update)
Renderer->>Disk: "await setMany([{...session, name}], [])"
alt setMany succeeds
Disk-->>Renderer: ok
Renderer->>CLI: sendRemoteRenameSessionResponse(true)
else setMany throws
Disk-->>Renderer: error
Renderer->>Renderer: logger.error (swallowed)
Renderer->>CLI: sendRemoteRenameSessionResponse(true) - bug: still signals success
end
Note over CLI,Disk: maestro:remoteDeleteSession (after this PR)
CLI->>WebServer: remove-agent (fire-and-forget)
WebServer->>Renderer: dispatch CustomEvent
Renderer->>ReactState: setSessions (schedules removal)
Renderer->>Disk: await setMany([], [sessionId])
Disk-->>Renderer: ok / error (logged, no response channel)
Reviews (1): Last reviewed commit: "fix(cli): persist remove-agent and sessi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| await window.maestro.sessions.setMany([{ ...session, name: newName } as any], []); | ||
| } catch (persistErr) { | ||
| logger.error('[Remote] Failed to persist session rename:', undefined, persistErr); | ||
| } | ||
|
|
||
| window.maestro.process.sendRemoteRenameSessionResponse(responseChannel, true); |
There was a problem hiding this comment.
The sendRemoteRenameSessionResponse(responseChannel, true) call is unconditional - it fires after the catch block regardless of whether setMany succeeded or threw. This defeats the PR's central guarantee: a CLI consumer that calls rename followed immediately by show agent will still see the stale name if setMany throws, because the success signal arrived but the disk was never updated. The fix should either send false on persistence failure or re-throw and let the caller handle it.
| try { | ||
| await window.maestro.sessions.setMany([], [sessionId]); | ||
| } catch (persistErr) { | ||
| logger.error('[Remote] Failed to persist session removal:', undefined, persistErr); | ||
| } |
There was a problem hiding this comment.
setMany failures are logged via logger.error but are never forwarded to captureException, so any production disk-write failures will be invisible in the error-tracking dashboard. If persistence failure is considered recoverable, the catch block should at minimum call captureException(persistErr) alongside the log.
| try { | |
| await window.maestro.sessions.setMany([], [sessionId]); | |
| } catch (persistErr) { | |
| logger.error('[Remote] Failed to persist session removal:', undefined, persistErr); | |
| } | |
| try { | |
| await window.maestro.sessions.setMany([], [sessionId]); | |
| } catch (persistErr) { | |
| logger.error('[Remote] Failed to persist session removal:', undefined, persistErr); | |
| captureException(persistErr, { extra: { sessionId, operation: 'remoteDeleteSession' } }); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/hooks/remote/useAppRemoteEventListeners.ts (1)
1072-1076: ⚡ Quick winAdd
captureExceptionfor persistence failures to align with codebase patterns.Both persistence error handlers log but don't report to Sentry. Other error paths in this file consistently use
captureException(e.g., lines 286, 525, 749). Per coding guidelines, unexpected errors should be captured.Proposed fix
// Flush the removal to disk synchronously... try { await window.maestro.sessions.setMany([], [sessionId]); } catch (persistErr) { + captureException(persistErr, { extra: { event: 'maestro:remoteDeleteSession', sessionId } }); logger.error('[Remote] Failed to persist session removal:', undefined, persistErr); }// Flush the rename to disk before signaling success... try { await window.maestro.sessions.setMany([{ ...session, name: newName } as any], []); } catch (persistErr) { + captureException(persistErr, { extra: { event: 'maestro:remoteRenameSession', sessionId, newName } }); logger.error('[Remote] Failed to persist session rename:', undefined, persistErr); }As per coding guidelines: "Use Sentry utilities (
captureException,captureMessage) fromsrc/utils/sentry.tsfor explicit error reporting with context."Also applies to: 1145-1149
🤖 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 `@src/renderer/hooks/remote/useAppRemoteEventListeners.ts` around lines 1072 - 1076, The persistence error handlers call logger.error but don't report to Sentry — import and use captureException from the Sentry utilities and call it with the caught error and a short context message; specifically, in the catch blocks surrounding window.maestro.sessions.setMany (the persistErr variable) replace or augment logger.error('[Remote] Failed to persist session removal:', undefined, persistErr) with a captureException(persistErr, { level: 'error', extra: { action: 'persistSessionRemoval', sessionId } }) (and do the same for the other identical block around the second setMany call) so the errors are recorded in Sentry along with contextual info.
🤖 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.
Nitpick comments:
In `@src/renderer/hooks/remote/useAppRemoteEventListeners.ts`:
- Around line 1072-1076: The persistence error handlers call logger.error but
don't report to Sentry — import and use captureException from the Sentry
utilities and call it with the caught error and a short context message;
specifically, in the catch blocks surrounding window.maestro.sessions.setMany
(the persistErr variable) replace or augment logger.error('[Remote] Failed to
persist session removal:', undefined, persistErr) with a
captureException(persistErr, { level: 'error', extra: { action:
'persistSessionRemoval', sessionId } }) (and do the same for the other identical
block around the second setMany call) so the errors are recorded in Sentry along
with contextual info.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2140e4e-1902-4b21-95ee-d9cc179b56a4
📒 Files selected for processing (1)
src/renderer/hooks/remote/useAppRemoteEventListeners.ts
1 participant
Summary
Extends the synchronous
sessions:setManyflush pattern introduced in #1014 to the two remaining CLI-driven IPC handlers that bypass the renderer's 2s debounced persistence path:maestro:remoteDeleteSession— was relying onuseDebouncedPersistenceto eventually persist the removal. A CLIremove-agentimmediately followed bylist agentscould read the stale on-disk state until the debounce fired.maestro:remoteRenameSession— same race for the renamed name;show agentwould see the old name for up to 2s.Both handlers now await
window.maestro.sessions.setMany(updates, removeIds)before signaling success. The subsequent debounced flush is idempotent (it just rewrites the same merged set), so there's no churn.Scope notes
The issue also called out that the web-server-factory's
setDeleteSessionCallbackis fire-and-forget (returnstruebefore the renderer even sees the event), which is technically still a tiny race even with the disk flush in place. I left that pattern alone for now — the reporter rated it as low real-world impact, and the user-observable reproduction (CLI follow-up reads stale disk state) is fully closed by the renderer-side flush. Happy to follow up with a request-response refactor for the delete callback (matching rename / create / updateCwd) if desired.Closes #1054
Refs #1013 #1014
Test plan
npm run lintpassesnpx vitest rununit tests pass (1570 in the related suites)maestro-cli remove-agent <id>→list agentsimmediately, confirm agent is gonemaestro-cli session rename <id> "New Name"→show agent <id>immediately, confirm new name surfaces~/Library/Application Support/Maestro/maestro-sessions.jsonbetween the CLI call and the 2s debounce window to confirm disk is up-to-dateSummary by CodeRabbit