feat: process state persistence and renderer reconnection after reload#389
Open
openasocket wants to merge 8 commits intoRunMaestro:mainfrom
Open
feat: process state persistence and renderer reconnection after reload#389openasocket wants to merge 8 commits intoRunMaestro:mainfrom
openasocket wants to merge 8 commits intoRunMaestro:mainfrom
Conversation
Collaborator
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds disk-backed persistence for batch runs and live process snapshots, IPC/preload APIs for save/load/clear/flush, a process reconciliation IPC for renderer reloads, debounce/flush lifecycle controls, output buffering/capping for reconnect, and renderer hooks to recover and replay in-flight state after reloads. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Preload
participant Main as Main Process
participant ProcMgr as Process Manager
participant Disk as Disk
Note over Renderer,Disk: Save / Flush Flow
Renderer->>Preload: batchState.save(activeBatches)
Preload->>Main: IPC `batch-state:save`
Main->>Main: store pending snapshot + debounce (3000ms)
alt debounce expires
Main->>Disk: write `batch-run-state.json`
end
Note over Renderer,Disk: Flush
Renderer->>Preload: batchState.flush()
Preload->>Main: IPC `batch-state:flush`
Main->>Disk: immediate write `batch-run-state.json` (cancel debounce)
Note over Renderer,ProcMgr: Reload Reconciliation
Renderer->>Preload: process.reconcileAfterReload()
Preload->>Main: IPC `process:reconcileAfterReload`
Main->>ProcMgr: gather running processes (+ recentOutput capped)
ProcMgr-->>Main: processes[]
Main-->>Preload: processes[]
Preload-->>Renderer: processes[] (Promise resolved)
Renderer->>Renderer: useProcessReconciliation loads batchState, replays output, updates sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 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)
Comment |
Collaborator
|
@greptile review |
|
✅ Actions performedReview triggered.
|
Greptile SummaryImplements robust process state persistence and renderer reconnection to survive F5 reloads, HMR, and dev restarts. When the Electron renderer reloads, child processes (AI agents, terminals) survive in the main process but the renderer loses all in-memory session state. This PR adds: Process State Persistence (Main Process)
Reconnection IPC
Renderer Reconciliation
Batch State Persistence
Post-Review Fixes
Key Insight: The root cause of previous reconnection failures was a session ID mismatch — main process stores composite IDs while renderer uses bare UUIDs. This PR correctly parses composite IDs to extract the base session UUID for matching. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Renderer as Renderer (React)
participant Main as Main Process
participant PM as ProcessManager
participant Disk as Disk Storage
Note over Renderer,Disk: Normal Operation
Renderer->>Main: Spawn process
Main->>PM: spawn(config)
PM->>PM: emit('spawn')
PM-->>Main: Process created
Main->>Disk: Save snapshot (debounced 2s)
PM->>PM: Capture output in streamedText (50KB)
Note over Renderer,Disk: Renderer Reload (F5/HMR)
Renderer->>Renderer: Mount useProcessReconciliation
Renderer->>Main: reconcileAfterReload()
Main->>PM: getAll()
PM-->>Main: Running processes + 10KB output buffer
Main-->>Renderer: Process metadata + output
Renderer->>Renderer: Parse composite session IDs
Renderer->>Renderer: Restore busy states
Renderer->>Renderer: Replay output buffers
Renderer->>Renderer: Show reconnection toast
Note over Renderer,Disk: Batch Recovery
Renderer->>Main: batchState.load()
Main->>Disk: Read snapshot (reject if >10min)
Disk-->>Main: Batch state snapshot
Main-->>Renderer: Persisted batch state
Renderer->>Main: reconcileAfterReload()
Main-->>Renderer: Running processes
Renderer->>Renderer: Check process still alive
alt Process still running
Renderer->>Renderer: Restore batch + resume
else Process dead
Renderer->>Renderer: Mark batch paused with error
end
Note over Renderer,Disk: Clean Shutdown
Main->>PM: will-quit
Main->>Disk: Clear snapshot file
Last reviewed commit: fce93d0 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/main/index.ts
Outdated
Comment on lines
722
to
724
| // Clear snapshot on clean shutdown so stale data isn't used on next launch | ||
| app.on('will-quit', () => { | ||
| processStateStore.clear().catch(() => {}); |
There was a problem hiding this comment.
Consider flushing processStateStore before clearing on shutdown to ensure pending writes complete
Suggested change
| // Clear snapshot on clean shutdown so stale data isn't used on next launch | |
| app.on('will-quit', () => { | |
| processStateStore.clear().catch(() => {}); | |
| app.on('will-quit', () => { | |
| processStateStore.flush().catch(() => {}); | |
| processStateStore.clear().catch(() => {}); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/renderer/hooks/batch/useBatchProcessor.ts (2)
302-304: Verify the filter logic excludes the just-completed session.The check at lines 302-304 filters for
state.isRunning, but afterCOMPLETE_BATCHis dispatched,newStates[action.sessionId]should no longer haveisRunning: true. However, the comment at line 301 says "OTHER active batches", yet the filter doesn't explicitly exclude the currentaction.sessionId.This works correctly because
COMPLETE_BATCHshould setisRunning: falsein the reducer, so the filter naturally excludes it. But for clarity, consider making this explicit:♻️ Proposed clarification
// Check if there are any OTHER active batches still running - const remainingActive = Object.entries(newStates).some( - ([, state]) => state.isRunning + const remainingActive = Object.entries(newStates).some( + ([sessionId, state]) => sessionId !== action.sessionId && state.isRunning );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 302 - 304, The check computing remainingActive should explicitly exclude the just-completed session to make intent clear: when handling COMPLETE_BATCH ensure the predicate that computes remainingActive uses both state.isRunning and that the key !== action.sessionId (i.e., filter entries where state.isRunning && sessionId !== action.sessionId) so the "OTHER active batches" comment matches the logic; reference the newStates object, remainingActive calculation, action.sessionId and the COMPLETE_BATCH handling in the reducer/handler.
475-519: Consider extracting duplicated agentSessionId restoration logic.Lines 475-492 and 501-519 contain nearly identical code for restoring
agentSessionIdon the active tab. Consider extracting this into a helper function:♻️ Example extraction
// Helper to restore agentSessionId on the active tab function restoreAgentSessionId(sessionId: string, agentSessionId: string) { const sessions = useSessionStore.getState().sessions; const session = sessions.find((s) => s.id === sessionId); if (!session) return; const activeTab = session.aiTabs?.find((t) => t.id === session.activeTabId) || session.aiTabs?.[0]; if (!activeTab) return; useSessionStore.getState().setSessions((prev) => prev.map((s) => { if (s.id !== sessionId) return s; return { ...s, aiTabs: s.aiTabs.map((tab) => { if (tab.id !== activeTab.id) return tab; return { ...tab, agentSessionId: agentSessionId ?? null }; }), }; }) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 475 - 519, The duplicated logic that restores agentSessionId on the active tab (seen around the agentSessionId checks in useBatchProcessor.ts) should be extracted into a single helper (e.g., restoreAgentSessionId(sessionId: string, agentSessionId: string | null)) that locates the session via useSessionStore.getState().sessions, finds the active tab via session.aiTabs?.find(t => t.id === session.activeTabId) || session.aiTabs?.[0], and calls useSessionStore.getState().setSessions(prev => ...) to update that tab's agentSessionId; replace both inline blocks that reference batch.agentSessionId, session.aiTabs, session.activeTabId, and useSessionStore.getState().setSessions with calls to this new helper using batch.sessionId and batch.agentSessionId.src/main/preload/batchState.ts (1)
17-42: Consider sharing types between main and preload to avoid drift.The
PersistedBatchRunStateandPersistedBatchSnapshotinterfaces are duplicated in bothsrc/main/ipc/handlers/batch-state.tsandsrc/main/preload/batchState.ts. If the main process types change, the preload types could drift out of sync.Consider extracting these types to a shared location (e.g.,
src/shared/types/batchState.ts) that both files import. This is a minor concern for now, but worth noting for future maintenance.Also applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/batchState.ts` around lines 17 - 42, The PersistedBatchRunState and PersistedBatchSnapshot interfaces are duplicated and risk drifting between main and preload; extract these interfaces into a single shared module (e.g., create src/shared/types/batchState.ts) and update both locations to import PersistedBatchRunState and PersistedBatchSnapshot from that shared module so both preload and main use the same type definitions; ensure exported names match the existing interfaces and update any relative imports in files that previously declared these interfaces.src/main/ipc/handlers/batch-state.ts (1)
165-172: Consider canceling pending timer inbatch-state:clear.If the renderer calls
save()followed immediately byclear()(e.g., batch completes and clears state), the debounce timer fromsave()is still pending. When it fires, it will checkpendingSnapshotwhich may no longer be relevant.While the current code handles this (line 125 checks
if (pendingSnapshot)), explicitly canceling the timer inclear()would be cleaner and prevent unnecessary timer callbacks:♻️ Proposed improvement
ipcMain.handle('batch-state:clear', async () => { + // Cancel any pending debounced write + if (writeTimer) { + clearTimeout(writeTimer); + writeTimer = null; + } + pendingSnapshot = null; + try { await fs.unlink(getSnapshotPath()); logger.debug('Cleared batch state snapshot', LOG_CONTEXT); } catch { // File may not exist — that's fine } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/batch-state.ts` around lines 165 - 172, The 'batch-state:clear' IPC handler should cancel any pending debounce timer and clear the in-memory pending snapshot before deleting the file; update the handler for 'batch-state:clear' to check for and call clearTimeout on the debounce timer variable (e.g., pendingSnapshotTimer), set that timer variable to null/undefined and set pendingSnapshot = null (or falsey) before calling fs.unlink(getSnapshotPath()), then proceed with the unlink and logging so the pending save callback won't run afterward.src/renderer/hooks/agent/useProcessReconciliation.ts (2)
153-153: Adddepsto the dependency array.The effect uses
deps.batchedUpdateranddeps.addToastRefbut they're not in the dependency array. While these are likely stable references, ESLint'sreact-hooks/exhaustive-depsrule would flag this. Since this is a mount-only effect, consider either:
- Adding deps to the array (safe if refs are stable)
- Explicitly disabling the lint rule with a comment explaining the intentional mount-only behavior
♻️ Option 1: Add deps to array
- }, []); + }, [deps.batchedUpdater, deps.addToastRef]);♻️ Option 2: Disable lint with explanation
- }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps -- Mount-only effect; deps are stable refs + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useProcessReconciliation.ts` at line 153, The useEffect in useProcessReconciliation uses deps.batchedUpdater and deps.addToastRef but the dependency array is empty; update the effect's dependency array to include the actual refs used (e.g. deps, or at minimum deps.batchedUpdater and deps.addToastRef) so react-hooks/exhaustive-deps is satisfied, or if you intend mount-only behavior, add an explicit eslint-disable-next-line comment with a short justification; locate the effect in useProcessReconciliation and modify its dependency array or add the lint-disable and explanation accordingly.
87-88: Consider handling multiple process types per session.Line 87 takes
procs[0]as the "primary process" to determinebusySource. If a session has both an AI tab process and a terminal process running, the first one found determines the busy source, which could be misleading.Consider setting
busySourcebased on whether any AI process exists (prioritize AI over terminal):♻️ Proposed improvement
- const proc = procs[0]; // Primary process for this session + // Determine busy source: prioritize AI over terminal + const hasAiProcess = procs.some((p) => !p.isTerminal); + const busySource = hasAiProcess ? 'ai' : 'terminal'; + const proc = procs[0]; // Use first process for timing info // ... return { ...session, state: 'busy' as SessionState, - busySource: proc.isTerminal ? 'terminal' : 'ai', + busySource, thinkingStartTime: proc.startTime, aiTabs: updatedAiTabs, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useProcessReconciliation.ts` around lines 87 - 88, The code currently picks the primary process with const proc = procs[0] and uses it to set busySource, which misreports when multiple process types exist; update the logic in useProcessReconciliation (where procs and busySource are handled) to check the procs array for an AI-type process first (e.g., Array.prototype.find to locate a process whose type indicates an AI/tab) and if found set busySource to AI, otherwise fall back to a terminal-type process or default; replace the single procs[0] usage with this prioritized selection so AI activity is reported over terminal activity when both are present.src/main/process-manager/handlers/StdoutHandler.ts (1)
242-246: Remove DEBUG console.log statements before merging.Lines 242-246 and 264-267 contain
console.logstatements prefixed with[StdoutHandler]that appear to be debug artifacts. These should be removed or converted tologger.debug()with appropriate gating to avoid polluting production logs.♻️ Proposed fix to use logger instead
- // DEBUG: Log usage extracted from parser - console.log('[StdoutHandler] Usage from parser (line 255 path)', { - sessionId, - toolType: managedProcess.toolType, - parsedUsage: usage, - }); + logger.debug('[StdoutHandler] Usage from parser', 'ProcessManager', { + sessionId, + toolType: managedProcess.toolType, + parsedUsage: usage, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 242 - 246, Remove the stray debug console.log calls in StdoutHandler and replace them with the class logger: locate the console.log statements (the ones printing '[StdoutHandler] Usage from parser...' and the other '[StdoutHandler]' debug) inside the StdoutHandler methods, delete console.log(...) and call this.logger.debug(...) (or the existing logger instance used by StdoutHandler) with the same message and metadata; ensure these calls rely on the logger's level so they won’t appear in production logs.src/__tests__/main/ipc/handlers/batch-state.test.ts (1)
73-73: Consider using a more specific type thanFunction.Using
Functionas a type is discouraged because it accepts any function signature. Consider using a more specific type:♻️ Proposed fix
- let handlers: Map<string, Function>; + let handlers: Map<string, (...args: unknown[]) => unknown>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/batch-state.test.ts` at line 73, The declaration let handlers: Map<string, Function>; uses the broad Function type—replace it with a specific handler signature used by your IPC code (e.g., declare a type like type IpcHandler = (...args: unknown[]) => Promise<unknown> | unknown and then change the variable to let handlers: Map<string, IpcHandler>;); update any related places that set or call handlers to match the new IpcHandler signature (for example functions that return Promises should conform to Promise<unknown> return) so the Map enforces the proper argument/return contract.
🤖 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/index.ts`:
- Around line 699-725: The will-quit handler currently swallows errors from
processStateStore.clear() which hides IO failures; change it to properly
surface/report errors by importing and using the Sentry helper
(captureException) from src/utils/sentry and invoking it if clear() rejects (or
await clear() inside try/catch and call captureException(err, { extra: { phase:
'shutdown', action: 'clearProcessSnapshot' } })). Update the app.on('will-quit',
...) block to await/try-catch processStateStore.clear() and call
captureException with the caught error and contextual metadata instead of an
empty catch.
In `@src/main/process-manager/ProcessStateStore.ts`:
- Around line 65-80: The loadSnapshot, clear, and writeSnapshotToDisk functions
currently swallow all errors; change them to explicitly handle expected ENOENT
(file-not-found) by treating it as a missing snapshot and continue, but for any
other unexpected IO or JSON parse errors call the Sentry utilities
(captureException or captureMessage) from src/utils/sentry with contextual info
(function name, this.snapshotPath, and LOG_CONTEXT), log the error via
logger.error, and then either return null (for loadSnapshot) or
rethrow/propagate the error as appropriate (for clear and writeSnapshotToDisk)
instead of silently returning; update the try/catch blocks in loadSnapshot,
clear, and writeSnapshotToDisk to branch on (err.code === 'ENOENT') vs other
errors and include references to loadSnapshot, clear, writeSnapshotToDisk,
logger, LOG_CONTEXT, and the Sentry helpers to locate changes.
---
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/batch-state.test.ts`:
- Line 73: The declaration let handlers: Map<string, Function>; uses the broad
Function type—replace it with a specific handler signature used by your IPC code
(e.g., declare a type like type IpcHandler = (...args: unknown[]) =>
Promise<unknown> | unknown and then change the variable to let handlers:
Map<string, IpcHandler>;); update any related places that set or call handlers
to match the new IpcHandler signature (for example functions that return
Promises should conform to Promise<unknown> return) so the Map enforces the
proper argument/return contract.
In `@src/main/ipc/handlers/batch-state.ts`:
- Around line 165-172: The 'batch-state:clear' IPC handler should cancel any
pending debounce timer and clear the in-memory pending snapshot before deleting
the file; update the handler for 'batch-state:clear' to check for and call
clearTimeout on the debounce timer variable (e.g., pendingSnapshotTimer), set
that timer variable to null/undefined and set pendingSnapshot = null (or falsey)
before calling fs.unlink(getSnapshotPath()), then proceed with the unlink and
logging so the pending save callback won't run afterward.
In `@src/main/preload/batchState.ts`:
- Around line 17-42: The PersistedBatchRunState and PersistedBatchSnapshot
interfaces are duplicated and risk drifting between main and preload; extract
these interfaces into a single shared module (e.g., create
src/shared/types/batchState.ts) and update both locations to import
PersistedBatchRunState and PersistedBatchSnapshot from that shared module so
both preload and main use the same type definitions; ensure exported names match
the existing interfaces and update any relative imports in files that previously
declared these interfaces.
In `@src/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 242-246: Remove the stray debug console.log calls in StdoutHandler
and replace them with the class logger: locate the console.log statements (the
ones printing '[StdoutHandler] Usage from parser...' and the other
'[StdoutHandler]' debug) inside the StdoutHandler methods, delete
console.log(...) and call this.logger.debug(...) (or the existing logger
instance used by StdoutHandler) with the same message and metadata; ensure these
calls rely on the logger's level so they won’t appear in production logs.
In `@src/renderer/hooks/agent/useProcessReconciliation.ts`:
- Line 153: The useEffect in useProcessReconciliation uses deps.batchedUpdater
and deps.addToastRef but the dependency array is empty; update the effect's
dependency array to include the actual refs used (e.g. deps, or at minimum
deps.batchedUpdater and deps.addToastRef) so react-hooks/exhaustive-deps is
satisfied, or if you intend mount-only behavior, add an explicit
eslint-disable-next-line comment with a short justification; locate the effect
in useProcessReconciliation and modify its dependency array or add the
lint-disable and explanation accordingly.
- Around line 87-88: The code currently picks the primary process with const
proc = procs[0] and uses it to set busySource, which misreports when multiple
process types exist; update the logic in useProcessReconciliation (where procs
and busySource are handled) to check the procs array for an AI-type process
first (e.g., Array.prototype.find to locate a process whose type indicates an
AI/tab) and if found set busySource to AI, otherwise fall back to a
terminal-type process or default; replace the single procs[0] usage with this
prioritized selection so AI activity is reported over terminal activity when
both are present.
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 302-304: The check computing remainingActive should explicitly
exclude the just-completed session to make intent clear: when handling
COMPLETE_BATCH ensure the predicate that computes remainingActive uses both
state.isRunning and that the key !== action.sessionId (i.e., filter entries
where state.isRunning && sessionId !== action.sessionId) so the "OTHER active
batches" comment matches the logic; reference the newStates object,
remainingActive calculation, action.sessionId and the COMPLETE_BATCH handling in
the reducer/handler.
- Around line 475-519: The duplicated logic that restores agentSessionId on the
active tab (seen around the agentSessionId checks in useBatchProcessor.ts)
should be extracted into a single helper (e.g., restoreAgentSessionId(sessionId:
string, agentSessionId: string | null)) that locates the session via
useSessionStore.getState().sessions, finds the active tab via
session.aiTabs?.find(t => t.id === session.activeTabId) || session.aiTabs?.[0],
and calls useSessionStore.getState().setSessions(prev => ...) to update that
tab's agentSessionId; replace both inline blocks that reference
batch.agentSessionId, session.aiTabs, session.activeTabId, and
useSessionStore.getState().setSessions with calls to this new helper using
batch.sessionId and batch.agentSessionId.
Comment on lines
65
to
80
| async loadSnapshot(): Promise<ProcessStateSnapshot | null> { | ||
| try { | ||
| const content = await fs.readFile(this.snapshotPath, 'utf-8'); | ||
| const snapshot: ProcessStateSnapshot = JSON.parse(content); | ||
|
|
||
| // Reject snapshots older than 5 minutes — processes are likely dead | ||
| const age = Date.now() - snapshot.timestamp; | ||
| if (age > 5 * 60 * 1000) { | ||
| logger.info('Process snapshot too old, ignoring', LOG_CONTEXT, { ageMs: age }); | ||
| return null; | ||
| } | ||
|
|
||
| return snapshot; | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Handle snapshot IO errors explicitly and report via Sentry.
loadSnapshot, clear, and writeSnapshotToDisk currently swallow errors (or only warn), which can hide corrupted snapshots or permission failures. Handle ENOENT explicitly and report unexpected errors with Sentry.
🛠️ Suggested fix
-import { logger } from '../utils/logger';
+import { logger } from '../utils/logger';
+import { captureException } from '../utils/sentry';
@@
- } catch {
- return null;
- }
+ } catch (err) {
+ if ((err as NodeJS.ErrnoException).code === 'ENOENT') {
+ return null;
+ }
+ if (err instanceof SyntaxError) {
+ captureException(err, { tags: { context: 'ProcessStateStore.loadSnapshot' } });
+ return null;
+ }
+ captureException(err, { tags: { context: 'ProcessStateStore.loadSnapshot' } });
+ throw err;
+ }
@@
- } catch {
- // File may not exist
- }
+ } catch (err) {
+ if ((err as NodeJS.ErrnoException).code === 'ENOENT') return;
+ captureException(err, { tags: { context: 'ProcessStateStore.clear' } });
+ throw err;
+ }
@@
- } catch (err) {
- logger.warn('Failed to save process state snapshot', LOG_CONTEXT, { error: String(err) });
- }
+ } catch (err) {
+ captureException(err, { tags: { context: 'ProcessStateStore.writeSnapshotToDisk' } });
+ logger.warn('Failed to save process state snapshot', LOG_CONTEXT, { error: String(err) });
+ }As per coding guidelines: Do NOT silently swallow exceptions with try-catch-console.error blocks. Handle only expected/recoverable errors explicitly with specific error codes. Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context.
Also applies to: 86-96, 109-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/process-manager/ProcessStateStore.ts` around lines 65 - 80, The
loadSnapshot, clear, and writeSnapshotToDisk functions currently swallow all
errors; change them to explicitly handle expected ENOENT (file-not-found) by
treating it as a missing snapshot and continue, but for any other unexpected IO
or JSON parse errors call the Sentry utilities (captureException or
captureMessage) from src/utils/sentry with contextual info (function name,
this.snapshotPath, and LOG_CONTEXT), log the error via logger.error, and then
either return null (for loadSnapshot) or rethrow/propagate the error as
appropriate (for clear and writeSnapshotToDisk) instead of silently returning;
update the try/catch blocks in loadSnapshot, clear, and writeSnapshotToDisk to
branch on (err.code === 'ENOENT') vs other errors and include references to
loadSnapshot, clear, writeSnapshotToDisk, logger, LOG_CONTEXT, and the Sentry
helpers to locate changes.
Collaborator
|
@reachraza will you review this PR please. |
…ross reloads Introduces a disk-backed snapshot store that enables the renderer to detect running processes after a reload/restart. Debounced writes (2s) prevent excessive I/O; snapshots older than 5 minutes are rejected as stale. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion after reload When the Electron renderer reloads (F5, HMR, dev restart), child processes survive in the main process but the renderer loses all session state. This adds a reconciliation mechanism so sessions are restored to match reality: - ProcessStateStore: persists active process snapshots to disk (debounced) - ProcessManager emits 'spawn' event for snapshot tracking - process:reconcileAfterReload IPC handler returns running processes with recent output buffer (capped at 50KB per process) - DataBufferManager retains output in streamedText for replay after reload - useProcessReconciliation hook restores session/tab busy states on mount and replays buffered output into terminal views - Shows reconnection toast when processes are recovered Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssReconciliation The react-hooks/exhaustive-deps rule doesn't flag the empty deps array, so the disable comment was unnecessary and produced an ESLint warning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…reload recovery Add batch-state:save, batch-state:load, batch-state:clear, and batch-state:flush IPC handlers that persist Auto Run batch state to disk. This enables recovery of batch run progress after renderer reloads. - Save debounced to 3s to avoid excessive disk I/O - Load rejects snapshots older than 10 minutes - Flush forces immediate write for clean shutdown - Registered in handler index via registerBatchStateHandlers() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…te persistence Expose batch-state IPC handlers (save/load/clear/flush) to the renderer via window.maestro.batchState, enabling Auto Run state recovery after reload. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…reload survival Implements batch state snapshot emission on every progress update (via dispatch wrapper), recovery on renderer mount by loading persisted state from the main process, and edge case handling for dead processes, worktree validation, and power management re-acquisition. Key changes: - persistBatchState() sends active batch snapshots to main process after every state-modifying dispatch (START_BATCH, UPDATE_PROGRESS, SET_ERROR, etc.) - COMPLETE_BATCH and killBatchRun clear the snapshot file - On mount, loads snapshot and restores Zustand state so UI shows correct progress - Dead processes are marked as PAUSED_ERROR with recoverable agent_crashed error - Living processes get power management locks re-acquired - agentSessionId is restored on active tabs for --resume on next spawn - All batchState IPC calls are guarded with optional chaining for test safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…w fixes
Root cause: useProcessReconciliation compared composite process session IDs
(e.g. `{uuid}-ai-{tabId}`, `{uuid}-terminal`) against bare Session.id UUIDs,
causing zero matches and silent reconnection failure after renderer reload.
Fixes:
- Parse composite session IDs via parseSessionId() + terminal suffix handling
- Add 'spawn' event to ProcessManagerEvents type interface
- ProcessStateStore: use getter callback for live state at write time, fix
flush() to actually write to disk, clear() cancels pending timers
- Prevent streamedText double-append via skipBufferRetention flag in
DataBufferManager when re-emitting already-captured result text
- Remove dead agentSessionId from ProcessSnapshot, retain processType for VIBES
- Fix appendLog to pass base UUID instead of composite process ID
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openasocket
force-pushed
the
feat/process-reconnect
branch
from
fce93d0 to
ff9899e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/process-manager/handlers/DataBufferManager.ts (1)
29-31: Flag mechanism could lose data if interleaved calls occur before flush.Setting
skipNextRetentionon the process object means if anotheremitDataBufferedcall (withoutskipBufferRetention) arrives before the flush timeout fires, both data chunks share the same flag state. The second chunk would incorrectly skip retention.In practice this appears safe since the flag is used during exit processing when no new stdout should arrive, but the coupling between
skipBufferRetentionbeing set and the eventual flush is implicit.Consider documenting this invariant or, for robustness, tracking the skip state per-buffered-chunk:
💡 Alternative: track skip offset instead of boolean flag
emitDataBuffered(sessionId: string, data: string, skipBufferRetention = false): void { const managedProcess = this.processes.get(sessionId); if (!managedProcess) { this.emitter.emit('data', sessionId, data); return; } - if (skipBufferRetention) { - managedProcess.skipNextRetention = true; - } + const bufferLenBefore = (managedProcess.dataBuffer || '').length; + if (skipBufferRetention) { + // Mark bytes starting at current buffer length as "skip retention" + managedProcess.skipRetentionFrom ??= bufferLenBefore; + } managedProcess.dataBuffer = (managedProcess.dataBuffer || '') + data;Then in
flushDataBuffer, only append the portion beforeskipRetentionFrom(if set) tostreamedText.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/DataBufferManager.ts` around lines 29 - 31, The current use of managedProcess.skipNextRetention in emitDataBuffered can misapply to subsequent buffered chunks if interleaved calls happen; instead record the skip state per buffered chunk (e.g., add a skipRetentionFrom/skipOffset property to the buffer item created in emitDataBuffered when skipBufferRetention is true) and then update flushDataBuffer to honor that property by only appending the portion of that specific chunk before skipRetentionFrom to streamedText; update/remove managedProcess.skipNextRetention accordingly (or keep as fallback) and/or add a short comment documenting the invariant if you prefer the simpler approach.src/renderer/hooks/batch/useBatchProcessor.ts (1)
479-490: Consider extracting the repeated agentSessionId restoration logic.The same pattern for restoring
agentSessionIdon the active tab appears twice (lines 479-490 for dead processes and 506-517 for running processes). While functionally correct, this could be extracted to reduce duplication.💡 Optional: Extract helper for agentSessionId restoration
// Inside recoverBatchState, before the loop: const restoreAgentSessionId = (sessionId: string, agentSessionId: string | undefined) => { if (!agentSessionId) return; const session = useSessionStore.getState().sessions.find((s) => s.id === sessionId); const activeTab = session?.aiTabs?.find((t) => t.id === session.activeTabId) || session?.aiTabs?.[0]; if (!activeTab) return; useSessionStore.getState().setSessions((prev) => prev.map((s) => { if (s.id !== sessionId) return s; return { ...s, aiTabs: s.aiTabs.map((tab) => { if (tab.id !== activeTab.id) return tab; return { ...tab, agentSessionId: agentSessionId ?? null }; }), }; }) ); }; // Then use: restoreAgentSessionId(batch.sessionId, batch.agentSessionId);Also applies to: 506-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 479 - 490, Duplicate logic for restoring agentSessionId appears in recoverBatchState; extract it into a helper (e.g., restoreAgentSessionId) and call it from both places. The helper should accept sessionId and agentSessionId, find the session via useSessionStore.getState().sessions, determine the active tab (session.activeTabId or first aiTabs entry), and call useSessionStore.getState().setSessions to map the session and update only the matching aiTabs entry to set agentSessionId (or null). Replace both in-place blocks with a single call to restoreAgentSessionId(batch.sessionId, batch.agentSessionId).src/main/ipc/handlers/batch-state.ts (1)
157-159: Consider logging or differentiating error types in load handler.The catch block silently returns
nullfor any error (file not found, JSON parse error, permission denied). While this is safe, it may hide issues during debugging.💡 Optional: Add debug logging for load failures
} catch { + // File may not exist (first run) or be corrupted — both are acceptable return null; }Or for more visibility:
- } catch { + } catch (err) { + logger.debug('Failed to load batch state snapshot', LOG_CONTEXT, { error: String(err) }); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/batch-state.ts` around lines 157 - 159, The current catch block in the batch-state load handler swallows all errors and returns null; change it to catch the error as a variable (e.g., `err`) and handle or log by type: if the error is file-not-found (ENOENT) return null, if it's a JSON parse error or permission/IO error log a debug/warn via the module logger (or console) with the error details and return null, and for unexpected/critical errors rethrow or return a distinct failure so callers can differentiate; update the catch in the load function inside batch-state.ts to inspect err.code/err.name and emit a clear log message including the error and context (function/load operation) instead of silently returning null.src/main/preload/batchState.ts (1)
17-50: Type duplication between main and preload - consider shared types.
PersistedBatchRunStateandPersistedBatchSnapshotare defined identically in bothsrc/main/ipc/handlers/batch-state.tsand here. This creates a maintenance risk where changes to one may not be reflected in the other.Consider extracting these interfaces to a shared location (e.g.,
src/shared/types/batchState.ts) and importing in both files:// src/shared/types/batchState.ts export interface PersistedBatchRunState { ... } export interface PersistedBatchSnapshot { ... }This follows the existing pattern where
src/shared/typescontains cross-boundary type definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/batchState.ts` around lines 17 - 50, PersistedBatchRunState and PersistedBatchSnapshot are duplicated between modules — extract these interfaces into a single shared types module (e.g., shared/types/batchState) and export PersistedBatchRunState and PersistedBatchSnapshot there, then replace the local definitions in both the preload and main handlers with imports from that shared module; keep the exported interface names identical, update import statements where the duplicates existed (references to PersistedBatchRunState and PersistedBatchSnapshot), and run type-check to ensure no breakage.
🤖 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/renderer/App.tsx`:
- Around line 3658-3663: The reconciliation hook can run before sessions are
restored; update useProcessReconciliation to accept an enabled boolean and
ensure it runs only once after enabled flips true: add an enabled parameter to
the UseProcessReconciliationDeps, add a hasReconciledRef useRef flag (e.g.,
hasReconciledRef) and wrap the existing reconciliation logic in a useEffect that
returns early if !deps.enabled or hasReconciledRef.current, set
hasReconciledRef.current = true before running the reconcile logic, and update
the call site (useProcessReconciliation({...})) to pass enabled tied to the
session-restored state so reconciliation is gated until sessions are loaded.
---
Nitpick comments:
In `@src/main/ipc/handlers/batch-state.ts`:
- Around line 157-159: The current catch block in the batch-state load handler
swallows all errors and returns null; change it to catch the error as a variable
(e.g., `err`) and handle or log by type: if the error is file-not-found (ENOENT)
return null, if it's a JSON parse error or permission/IO error log a debug/warn
via the module logger (or console) with the error details and return null, and
for unexpected/critical errors rethrow or return a distinct failure so callers
can differentiate; update the catch in the load function inside batch-state.ts
to inspect err.code/err.name and emit a clear log message including the error
and context (function/load operation) instead of silently returning null.
In `@src/main/preload/batchState.ts`:
- Around line 17-50: PersistedBatchRunState and PersistedBatchSnapshot are
duplicated between modules — extract these interfaces into a single shared types
module (e.g., shared/types/batchState) and export PersistedBatchRunState and
PersistedBatchSnapshot there, then replace the local definitions in both the
preload and main handlers with imports from that shared module; keep the
exported interface names identical, update import statements where the
duplicates existed (references to PersistedBatchRunState and
PersistedBatchSnapshot), and run type-check to ensure no breakage.
In `@src/main/process-manager/handlers/DataBufferManager.ts`:
- Around line 29-31: The current use of managedProcess.skipNextRetention in
emitDataBuffered can misapply to subsequent buffered chunks if interleaved calls
happen; instead record the skip state per buffered chunk (e.g., add a
skipRetentionFrom/skipOffset property to the buffer item created in
emitDataBuffered when skipBufferRetention is true) and then update
flushDataBuffer to honor that property by only appending the portion of that
specific chunk before skipRetentionFrom to streamedText; update/remove
managedProcess.skipNextRetention accordingly (or keep as fallback) and/or add a
short comment documenting the invariant if you prefer the simpler approach.
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 479-490: Duplicate logic for restoring agentSessionId appears in
recoverBatchState; extract it into a helper (e.g., restoreAgentSessionId) and
call it from both places. The helper should accept sessionId and agentSessionId,
find the session via useSessionStore.getState().sessions, determine the active
tab (session.activeTabId or first aiTabs entry), and call
useSessionStore.getState().setSessions to map the session and update only the
matching aiTabs entry to set agentSessionId (or null). Replace both in-place
blocks with a single call to restoreAgentSessionId(batch.sessionId,
batch.agentSessionId).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/__tests__/main/ipc/handlers/batch-state.test.tssrc/__tests__/main/ipc/handlers/process.test.tssrc/main/index.tssrc/main/ipc/handlers/batch-state.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/process.tssrc/main/preload/batchState.tssrc/main/preload/index.tssrc/main/preload/process.tssrc/main/process-manager/ProcessManager.tssrc/main/process-manager/ProcessStateStore.tssrc/main/process-manager/constants.tssrc/main/process-manager/handlers/DataBufferManager.tssrc/main/process-manager/handlers/ExitHandler.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/index.tssrc/main/process-manager/types.tssrc/renderer/App.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/index.tssrc/renderer/hooks/agent/useProcessReconciliation.tssrc/renderer/hooks/batch/useBatchProcessor.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/main/process-manager/constants.ts
- src/main/process-manager/types.ts
- src/tests/main/ipc/handlers/batch-state.test.ts
- src/renderer/hooks/agent/useProcessReconciliation.ts
- src/main/process-manager/ProcessManager.ts
- src/main/process-manager/ProcessStateStore.ts
- src/main/process-manager/index.ts
- src/main/preload/process.ts
- src/main/process-manager/handlers/StdoutHandler.ts
- src/tests/main/ipc/handlers/process.test.ts
- src/main/ipc/handlers/index.ts
- src/main/index.ts
Comment on lines
+3658
to
+3663
| // --- PROCESS RECONNECTION AFTER RELOAD --- | ||
| // Reconcile session states with still-running processes after renderer reload (F5, HMR) | ||
| useProcessReconciliation({ | ||
| batchedUpdater, | ||
| addToastRef, | ||
| }); |
There was a problem hiding this comment.
Race condition: reconciliation can run before sessions are restored.
Because reconciliation is mounted immediately here, it can execute before the async session restore completes (see Line 1379). In that case, reconciliation may no-op against an empty session list and never rerun, so busy state/output recovery is missed after reload.
💡 Proposed fix (gate reconciliation until sessions are loaded)
- useProcessReconciliation({
- batchedUpdater,
- addToastRef,
- });
+ useProcessReconciliation({
+ batchedUpdater,
+ addToastRef,
+ enabled: sessionsLoaded,
+ });And in src/renderer/hooks/agent/useProcessReconciliation.ts, add an enabled guard and run once after it flips true:
type UseProcessReconciliationDeps = {
batchedUpdater: /* existing type */;
addToastRef: /* existing type */;
enabled: boolean;
};
const hasReconciledRef = useRef(false);
useEffect(() => {
if (!deps.enabled || hasReconciledRef.current) return;
hasReconciledRef.current = true;
// existing reconcile logic
}, [deps.enabled]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/App.tsx` around lines 3658 - 3663, The reconciliation hook can
run before sessions are restored; update useProcessReconciliation to accept an
enabled boolean and ensure it runs only once after enabled flips true: add an
enabled parameter to the UseProcessReconciliationDeps, add a hasReconciledRef
useRef flag (e.g., hasReconciledRef) and wrap the existing reconciliation logic
in a useEffect that returns early if !deps.enabled or hasReconciledRef.current,
set hasReconciledRef.current = true before running the reconcile logic, and
update the call site (useProcessReconciliation({...})) to pass enabled tied to
the session-restored state so reconciliation is gated until sessions are loaded.
…orting, type dedup, and code quality fixes
- Replace empty .catch(() => {}) in will-quit handler with captureException reporting
- Add ENOENT-aware error handling in ProcessStateStore (loadSnapshot, clear, writeSnapshotToDisk)
- Flush processStateStore before clearing on shutdown to prevent data loss
- Remove debug console.log statements from StdoutHandler
- Extract duplicated agentSessionId restoration logic into restoreAgentSessionId() helper
- Move PersistedBatchRunState types to src/shared/batch-state-types.ts (single source of truth)
- Cancel pending debounce timer in batch-state:clear handler to prevent stale writes
- Fix useEffect dependency array in useProcessReconciliation (add batchedUpdater, addToastRef)
- Prioritize AI processes over terminal in busySource detection
- Replace Function type with specific callable signature in batch-state tests
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 2015-2017: The code currently calls
window.maestro?.batchState?.clear(), which wipes the entire persisted batch
snapshot and breaks recoverability for other sessions; instead remove only the
state for the current session by deleting the session-specific key (e.g., use
batchState.remove(currentSessionId) or batchState.update/patch to delete only
currentSessionId) — locate the call to batchState.clear() in useBatchProcessor
(the block labeled "8. Clear persisted batch state snapshot") and replace it
with logic that reads the current session identifier (currentSessionId /
batchSessionId) and deletes only that entry, leaving the rest of the persisted
snapshot intact.
- Around line 398-423: The effect sets hasRecoveredRef.current = true before
confirming sessions are loaded, which permanently prevents retries when sessions
hydrate later; in the useEffect/recoverBatchState logic (the useEffect block,
hasRecoveredRef and recoverBatchState), defer setting hasRecoveredRef.current =
true until after you verify sessions are present (e.g., after reading const
sessions = useSessionStore.getState().sessions and confirming it is non-empty)
or add an explicit guard to return and leave hasRecoveredRef.current false when
sessions are empty so the recovery will retry once sessions hydrate; ensure this
change is applied around the hasRecoveredRef assignment and the session check to
avoid skipping future recovery attempts.
- Around line 435-438: The running-process check in useBatchProcessor.ts
(variable hasRunningProcess using runningProcesses.some) only treats exact
matches or IDs starting with batch.sessionId + '-ai-', missing other composite
IDs and causing false agent_crashed recoveries; update the predicate to treat
any composite session IDs that begin with the batch.sessionId and a hyphen
(e.g., change the second condition to use startsWith(batch.sessionId + '-')
rather than '-ai-') so hasRunningProcess correctly detects all
composite/terminal child sessions tied to batch.sessionId.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/__tests__/main/ipc/handlers/batch-state.test.tssrc/main/index.tssrc/main/ipc/handlers/batch-state.tssrc/main/preload/batchState.tssrc/main/process-manager/ProcessStateStore.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/renderer/hooks/agent/useProcessReconciliation.tssrc/renderer/hooks/batch/useBatchProcessor.tssrc/shared/batch-state-types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/index.ts
- src/main/ipc/handlers/batch-state.ts
- src/main/process-manager/ProcessStateStore.ts
- src/main/preload/batchState.ts
Comment on lines
+398
to
+423
| const hasRecoveredRef = useRef(false); | ||
| useEffect(() => { | ||
| if (hasRecoveredRef.current) return; | ||
| hasRecoveredRef.current = true; | ||
|
|
||
| let cancelled = false; | ||
|
|
||
| async function recoverBatchState() { | ||
| try { | ||
| if (!window.maestro?.batchState?.load) return; | ||
| const snapshot = await window.maestro.batchState.load(); | ||
| if (cancelled || !snapshot || snapshot.activeBatches.length === 0) return; | ||
|
|
||
| // Check which processes are still running (PROC-RECONNECT-01) | ||
| let runningProcesses: Array<{ | ||
| sessionId: string; | ||
| toolType: string; | ||
| tabId?: string; | ||
| }> = []; | ||
| try { | ||
| runningProcesses = await window.maestro.process.reconcileAfterReload(); | ||
| } catch { | ||
| // If reconciliation fails, treat all processes as dead | ||
| } | ||
|
|
||
| const sessions = useSessionStore.getState().sessions; |
There was a problem hiding this comment.
Recovery can be skipped permanently if sessions are not loaded yet.
hasRecoveredRef.current is set before confirming sessions are available, so an early run with empty sessions prevents any later retry after sessions hydrate.
💡 Suggested fix
const hasRecoveredRef = useRef(false);
useEffect(() => {
if (hasRecoveredRef.current) return;
- hasRecoveredRef.current = true;
+ if (sessions.length === 0) return;
let cancelled = false;
async function recoverBatchState() {
try {
if (!window.maestro?.batchState?.load) return;
const snapshot = await window.maestro.batchState.load();
if (cancelled || !snapshot || snapshot.activeBatches.length === 0) return;
+ hasRecoveredRef.current = true;
// ...
} catch (err) {
console.error('[BatchRecovery] Batch state recovery failed:', err);
}
}
recoverBatchState();
return () => {
cancelled = true;
};
-}, [dispatch]);
+}, [dispatch, sessions]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 398 - 423, The
effect sets hasRecoveredRef.current = true before confirming sessions are
loaded, which permanently prevents retries when sessions hydrate later; in the
useEffect/recoverBatchState logic (the useEffect block, hasRecoveredRef and
recoverBatchState), defer setting hasRecoveredRef.current = true until after you
verify sessions are present (e.g., after reading const sessions =
useSessionStore.getState().sessions and confirming it is non-empty) or add an
explicit guard to return and leave hasRecoveredRef.current false when sessions
are empty so the recovery will retry once sessions hydrate; ensure this change
is applied around the hasRecoveredRef assignment and the session check to avoid
skipping future recovery attempts.
Comment on lines
+435
to
+438
| const hasRunningProcess = runningProcesses.some( | ||
| (p) => | ||
| p.sessionId === batch.sessionId || p.sessionId.startsWith(batch.sessionId + '-ai-') | ||
| ); |
There was a problem hiding this comment.
Running-process check misses non--ai- composite IDs.
Line 435 only treats exact IDs or -ai- IDs as alive. Batch/terminal composite IDs can be incorrectly marked dead, which triggers false agent_crashed recovery errors.
💡 Suggested fix
+import { parseSessionId } from '../../utils/sessionIdParser';
+
// ...
-const hasRunningProcess = runningProcesses.some(
- (p) =>
- p.sessionId === batch.sessionId || p.sessionId.startsWith(batch.sessionId + '-ai-')
-);
+const hasRunningProcess = runningProcesses.some((p) => {
+ if (p.sessionId === batch.sessionId) return true;
+ const baseId = p.sessionId.endsWith('-terminal')
+ ? p.sessionId.slice(0, -'-terminal'.length)
+ : parseSessionId(p.sessionId).baseSessionId;
+ return baseId === batch.sessionId;
+});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 435 - 438, The
running-process check in useBatchProcessor.ts (variable hasRunningProcess using
runningProcesses.some) only treats exact matches or IDs starting with
batch.sessionId + '-ai-', missing other composite IDs and causing false
agent_crashed recoveries; update the predicate to treat any composite session
IDs that begin with the batch.sessionId and a hyphen (e.g., change the second
condition to use startsWith(batch.sessionId + '-') rather than '-ai-') so
hasRunningProcess correctly detects all composite/terminal child sessions tied
to batch.sessionId.
Comment on lines
+2015
to
+2017
| // 8. Clear persisted batch state snapshot | ||
| await window.maestro?.batchState?.clear(); | ||
|
|
There was a problem hiding this comment.
Do not clear the entire persisted snapshot on single-session kill.
Line 2016 clears the whole persisted batch snapshot even if other sessions are still running, which can drop recoverability for those sessions after reload.
💡 Suggested fix
-// 8. Clear persisted batch state snapshot
-await window.maestro?.batchState?.clear();
+// 8. Persistence is already handled by COMPLETE_BATCH dispatch:
+// - clears when no active batches remain
+// - otherwise re-saves remaining active batches📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 8. Clear persisted batch state snapshot | |
| await window.maestro?.batchState?.clear(); | |
| // 8. Persistence is already handled by COMPLETE_BATCH dispatch: | |
| // - clears when no active batches remain | |
| // - otherwise re-saves remaining active batches | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 2015 - 2017, The
code currently calls window.maestro?.batchState?.clear(), which wipes the entire
persisted batch snapshot and breaks recoverability for other sessions; instead
remove only the state for the current session by deleting the session-specific
key (e.g., use batchState.remove(currentSessionId) or batchState.update/patch to
delete only currentSessionId) — locate the call to batchState.clear() in
useBatchProcessor (the block labeled "8. Clear persisted batch state snapshot")
and replace it with logic that reads the current session identifier
(currentSessionId / batchSessionId) and deletes only that entry, leaving the
rest of the persisted snapshot intact.
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
When the Electron renderer reloads, all child processes (AI agents, terminals) survive in the main process — but the renderer loses all in-memory session state. Sessions reset to
idle, terminal buffers are cleared, and the user sees agents as disconnected even though they're still running. Auto Run batch progress is also lost entirely.Approach
This was developed iteratively through a structured playbook process (PROC-RECONNECT-01). During post-implementation review, we identified and fixed several issues including the root cause that prevented reconnection from working at all.
Process State Persistence (main process)
ProcessStateStore.ts): Debounced disk-backed snapshot of active processes. Uses a getter callback pattern so snapshots always reflect live state at write time (not stale captured data). Snapshots older than 5 minutes are rejected as stale.index.ts):spawnandexitevents trigger snapshot updates.will-quitclears the snapshot file.DataBufferManager.ts,StdoutHandler.ts): Retains the last 50KB of process output inmanagedProcess.streamedTextso it can be replayed after reload. AddedskipBufferRetentionflag to prevent double-appending when result emission falls back to already-captured streamed text.Reconnection IPC (
process.ts,preload/process.ts)process:reconcileAfterReloadhandler returns running processes with metadata and recent output buffer (capped at 10KB per process for IPC transfer).Renderer Reconciliation (
useProcessReconciliation.ts){uuid}-ai-{tabId},{uuid}-terminal) to extract the base session UUID for matching against rendererSession.id— this was the root cause of the original failureBatchedUpdater.appendLog()Batch State Persistence (
batch-state.ts,useBatchProcessor.ts)batch-state:save/load/clear/flushIPC handlers with 3-second debounced writes and 10-minute stale rejectionuseBatchProcessorhook saves state on every batch state change and loads persisted state on mountPost-Review Fixes
spawnevent toProcessManagerEventstype interface for type safetyProcessStateStore.flush()to actually write to disk (was silently discarding)agentSessionIdfield fromProcessSnapshot; retainedprocessTypefor future VIBES attributionstreamedTextdouble-appending viaskipBufferRetentioninDataBufferManagerFiles Changed (22 files, +1403 -12)
ProcessStateStore.ts,ProcessManager.ts,index.ts,types.ts,constants.tsDataBufferManager.ts,StdoutHandler.ts,ExitHandler.tsprocess.ts,batch-state.ts,handlers/index.tspreload/process.ts,preload/batchState.ts,preload/index.tsuseProcessReconciliation.ts,useBatchProcessor.ts,App.tsx,hooks/agent/index.tsglobal.d.ts,process-manager/index.tsbatch-state.test.ts,process.test.tsTest plan
npm run lint— TypeScript type checking (3 configs)npm run lint:eslint— ESLint code qualitynpm run test— Full test suite (457 files, 19,429 tests, all pass)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests