Skip to content

Commit 1dfdcb5

Browse files
committed
fix(security): late-attach mutation worker so IPC is live before its boot
CI e2e was failing across every Security spec with: Error: page.waitForFunction: Error: Error invoking remote method 'security:get-scan-status': Error: No handler registered for 'security:get-scan-status' Root cause: PR #N introduced `await bootMutationWorker()` between `bootScanWorker` and `registerSecurityIpc`. Both boots have to settle before any ipcMain.handle is wired in. The e2e harness calls firstWindow() and immediately starts polling `security:get-scan-status` via `waitForFunction`; an IPC rejection there propagates the throw and ends polling on the first attempt instead of retrying. The extra mutation-worker spawn latency pushed registration past that polling window on every CI runner. Fix: keep the IPC registration tight against scan-worker readiness (the pre-PR behaviour) and defer mutation-worker plumbing. - `registerSecurityIpc` now returns `{ dispose, attachMutationWorker }` instead of just `dispose`. - The mutation handlers close over a `let currentMutationWorker: MutationWorkerProxy | null = null`, read at call time, so a worker attached AFTER registration is picked up by every subsequent call without re-binding the closure. - The mutation-worker change-event forwarder daemon fiber is forked by `attachMutationWorker(proxy)`, not at registration time. `dispose` interrupts it alongside the scan-worker forwarders. - In `main/index.ts`: `registerSecurityIpc` runs immediately after `bootScanWorker` (same moment as pre-PR). `bootMutationWorker()` fires in the background; on success its proxy is plugged in via `securityIpc.attachMutationWorker(mutationWorker)`. Until that runs the IPC handlers fall back to the in-process SQL path that was already gated behind the `if (mutationWorker)` check — the same correctness, just without the off-main offload for the boot window. Tests: - The default IPC fixture in `security.test.ts` already exercises the in-process fallback (no `attachMutationWorker` call) — what it used to do via `mutationWorker: null` it now does by not attaching. - The two delegation tests call `attachMutationWorker(fakeProxy)` explicitly to pin the worker-delegated path + the EVT_FINDINGS_CHANGED forwarder. - Suite: 355 / 355 (no change in count, three call-sites adjusted).
1 parent 7da19eb commit 1dfdcb5

3 files changed

Lines changed: 105 additions & 48 deletions

File tree

packages/app/src/main/index.ts

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -341,13 +341,18 @@ async function ensureSecurityBooted(): Promise<void> {
341341
setSecurityReadiness?.({ ready: false, reason: 'scanner-unavailable' })
342342
return
343343
}
344-
// Mutation worker is best-effort — a boot failure logs and the IPC
345-
// handlers fall back to in-process SQL on the main thread.
346-
await bootMutationWorker()
347-
disposeSecurityIpc = registerSecurityIpc({
344+
// Register IPC immediately after the scan worker is ready so the
345+
// renderer's `security:get-scan-status` polling on first window
346+
// open finds a handler. Mutation-worker boot is deferred and
347+
// plumbed in via `securityIpc.attachMutationWorker` once it
348+
// reports ready — the handlers fall back to in-process SQL in the
349+
// meantime. Without this split the e2e harness saw the
350+
// first-window polling rejected with "No handler registered for
351+
// security:get-scan-status" because both worker boots were
352+
// awaited sequentially before the IPC was registered.
353+
const securityIpc = registerSecurityIpc({
348354
db,
349355
worker: scanWorker,
350-
mutationWorker,
351356
runPromise: runWithObservability,
352357
getMainWindow: () => mainWindow,
353358
pfCoordinator,
@@ -358,11 +363,23 @@ async function ensureSecurityBooted(): Promise<void> {
358363
})
359364
},
360365
})
366+
disposeSecurityIpc = securityIpc.dispose
361367
setSecurityReadiness?.({ ready: true })
362368
console.log('[security.lifecycle] booted — worker + IPC ready, backfilling')
363369
runWithObservability(scanWorker.backfill()).catch((err) => {
364370
console.error('[security] boot backfill failed:', err)
365371
})
372+
373+
// Mutation worker boots in the background. Until it's ready the
374+
// IPC handlers run their in-process fallback path on the main
375+
// thread — same SQL, same correctness, just no off-main offload.
376+
// On success, attach so subsequent calls route through the worker
377+
// AND start the per-mutation change forwarder.
378+
void bootMutationWorker().then(() => {
379+
if (mutationWorker) {
380+
securityIpc.attachMutationWorker(mutationWorker)
381+
}
382+
})
366383
// If the user enabled PF before this boot, bring the inference window
367384
// up now that the rest of Spool is ready.
368385
if (loadSecurityPreferences().pfEnabled) {

packages/app/src/main/ipc/security.test.ts

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -124,13 +124,13 @@ async function setupFixture(): Promise<Fixture> {
124124
handlers.clear()
125125
sentEvents.length = 0
126126

127-
const dispose = registerSecurityIpc({
127+
// Mutation worker is never attached in the default fixture so the
128+
// existing assertions exercise the in-process fallback path; the
129+
// suite further down attaches a fake proxy and pins the worker-
130+
// delegated path.
131+
const { dispose } = registerSecurityIpc({
128132
db,
129133
worker,
130-
// null mutationWorker forces the in-process fallback path that
131-
// the existing assertions already cover. A separate suite below
132-
// pins the worker-delegated path with a fake proxy.
133-
mutationWorker: null,
134134
runPromise: <A, E>(eff: Effect.Effect<A, E>) => Effect.runPromise(eff as unknown as Effect.Effect<A>),
135135
getMainWindow: () => fakeWindow,
136136
})
@@ -543,13 +543,15 @@ describe('registerSecurityIpc with mutationWorker', () => {
543543
webContents: { send: (channel: string, payload: unknown) => { sentEvents.push({ channel, payload }) } },
544544
} as unknown as import('electron').BrowserWindow
545545

546-
const dispose = registerSecurityIpc({
546+
const { dispose, attachMutationWorker } = registerSecurityIpc({
547547
db,
548548
worker,
549-
mutationWorker: fakeProxy,
550549
runPromise: <A, E>(eff: Effect.Effect<A, E>) => Effect.runPromise(eff as unknown as Effect.Effect<A>),
551550
getMainWindow: () => fakeWindow,
552551
})
552+
// Late-attach the fake proxy — mirrors how production wires the
553+
// mutation worker in after the IPC layer is already live.
554+
attachMutationWorker(fakeProxy)
553555

554556
try {
555557
await invoke('security:purge-finding', 42)
@@ -611,13 +613,13 @@ describe('registerSecurityIpc with mutationWorker', () => {
611613
webContents: { send: (channel: string, payload: unknown) => { sentEvents.push({ channel, payload }) } },
612614
} as unknown as import('electron').BrowserWindow
613615

614-
const dispose = registerSecurityIpc({
616+
const { dispose, attachMutationWorker } = registerSecurityIpc({
615617
db,
616618
worker,
617-
mutationWorker: fakeProxy,
618619
runPromise: <A, E>(eff: Effect.Effect<A, E>) => Effect.runPromise(eff as unknown as Effect.Effect<A>),
619620
getMainWindow: () => fakeWindow,
620621
})
622+
attachMutationWorker(fakeProxy)
621623

622624
try {
623625
// The forwarder fiber is forked via Effect.runPromise which is

packages/app/src/main/ipc/security.ts

Lines changed: 72 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,6 @@ export function registerSecurityReadinessIpc(
158158
export interface SecurityIpcDeps {
159159
db: Database.Database
160160
worker: ScanWorker
161-
/** Writes-only proxy that forwards purge / dismiss commands to the
162-
* mutation worker thread. Falls back to in-process via the same
163-
* core helpers when null (worker boot failed or feature-flagged
164-
* off in a tests path). */
165-
mutationWorker: MutationWorkerProxy | null
166161
/** Mount with `ManagedRuntime.make` so the IPC layer can run Effects
167162
* without each call paying ManagedRuntime construction cost. */
168163
runPromise: <A, E>(eff: Effect.Effect<A, E>) => Promise<A>
@@ -182,11 +177,41 @@ export interface SecurityIpcDeps {
182177
pfRuntime?: PfRuntime | null
183178
}
184179

180+
export interface SecurityIpcHandle {
181+
/** Interrupts all forwarder daemons + removes every ipcMain.handle
182+
* registration. Idempotent — fine to call after a failed attach. */
183+
dispose: () => void
184+
/** Late-bind a mutation worker so the IPC handlers start delegating
185+
* purge / dismiss / undismiss to it, AND fork the change-event
186+
* forwarder onto the worker's `changes` stream. Mutation worker
187+
* boot happens in the background after `registerSecurityIpc`
188+
* returns so the IPC layer is live the moment the scan worker is
189+
* ready — without this split, the e2e harness opens the first
190+
* window before mutation-worker boot completes and `security:
191+
* get-scan-status` rejects with "No handler registered" before
192+
* the worker proxies the call.
193+
*
194+
* Calling twice or with the same proxy is a no-op-ish: the prior
195+
* forwarder is left running (no harm — the previous PubSub
196+
* becomes unreachable and GCs once the proxy reference drops).
197+
* Real-world flow boots one proxy, attaches it, and replaces only
198+
* on teardown / re-boot. */
199+
attachMutationWorker: (proxy: MutationWorkerProxy) => void
200+
}
201+
185202
/** Register every Security Scan ipcMain.handle and start a background
186-
* fiber that forwards worker change events to the main window. The
187-
* returned disposer interrupts the forwarding fiber. */
188-
export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
189-
const { db, worker, mutationWorker, runPromise, getMainWindow, pfCoordinator, pfRuntime: hostRuntime, onPfEnabledChanged } = deps
203+
* fiber that forwards scan-worker change + status events to the main
204+
* window. Mutation-worker boot is intentionally deferred — call
205+
* `attachMutationWorker(proxy)` on the returned handle once the
206+
* worker is ready so the IPC layer becomes live before that boot
207+
* completes. */
208+
export function registerSecurityIpc(deps: SecurityIpcDeps): SecurityIpcHandle {
209+
const { db, worker, runPromise, getMainWindow, pfCoordinator, pfRuntime: hostRuntime, onPfEnabledChanged } = deps
210+
// Closure-local ref read by every mutation handler at call time —
211+
// lets `attachMutationWorker` swap the worker in after IPC has
212+
// already started taking calls. Until it's set the handlers fall
213+
// back to in-process SQL on the main thread.
214+
let currentMutationWorker: MutationWorkerProxy | null = null
190215

191216
ipcMain.handle(SECURITY_IPC_CHANNELS.LIST_FINDINGS, (_e, filter: FindingFilter) =>
192217
listFindings(db, filter),
@@ -224,7 +249,7 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
224249
runPromise(worker.getStatus),
225250
)
226251

227-
// All write paths route through `mutationWorker` when present so
252+
// All write paths route through `currentMutationWorker` when present so
228253
// the main event loop stays free during multi-second bulk
229254
// operations on a large archive. The fallback to in-process is
230255
// kept for the rare worker-boot-failure case (and for the IPC
@@ -233,11 +258,11 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
233258
ipcMain.handle(
234259
SECURITY_IPC_CHANNELS.DISMISS_FINDING,
235260
async (_e, args: { findingId: number; scope: 'session' | 'global' }) => {
236-
if (mutationWorker) {
261+
if (currentMutationWorker) {
237262
// Per-event publish lands via the proxy's `changes` stream
238263
// (subscribed below in the forwarder fiber), so no need to
239264
// webContents.send on success here.
240-
await mutationWorker.dismissFinding(args.findingId, args.scope)
265+
await currentMutationWorker.dismissFinding(args.findingId, args.scope)
241266
return { ok: true }
242267
}
243268
const sessionId = dismissFinding(db, args.findingId, args.scope, true)
@@ -252,8 +277,8 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
252277
ipcMain.handle(
253278
SECURITY_IPC_CHANNELS.DISMISS_FINDINGS,
254279
async (_e, args: { findingIds: number[]; scope: 'session' | 'global' }) => {
255-
if (mutationWorker) {
256-
await mutationWorker.dismissFindings(args.findingIds, args.scope)
280+
if (currentMutationWorker) {
281+
await currentMutationWorker.dismissFindings(args.findingIds, args.scope)
257282
return { ok: true }
258283
}
259284
const sessionIds = dismissFindings(db, args.findingIds, args.scope)
@@ -268,8 +293,8 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
268293
ipcMain.handle(
269294
SECURITY_IPC_CHANNELS.UNDISMISS_FINDING,
270295
async (_e, args: { findingId: number }) => {
271-
if (mutationWorker) {
272-
await mutationWorker.undismissFinding(args.findingId)
296+
if (currentMutationWorker) {
297+
await currentMutationWorker.undismissFinding(args.findingId)
273298
return { ok: true }
274299
}
275300
const sessionId = undismissFinding(db, args.findingId)
@@ -282,7 +307,7 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
282307
},
283308
)
284309
ipcMain.handle(SECURITY_IPC_CHANNELS.PURGE_FINDING, async (_e, findingId: number) => {
285-
if (mutationWorker) return mutationWorker.purgeFinding(findingId)
310+
if (currentMutationWorker) return currentMutationWorker.purgeFinding(findingId)
286311
const publish = (change: Parameters<NonNullable<typeof getMainWindow extends () => infer R ? R : never>['webContents']['send']>[1]) =>
287312
Effect.sync(() => {
288313
getMainWindow()?.webContents.send(SECURITY_IPC_CHANNELS.EVT_FINDINGS_CHANGED, change)
@@ -291,7 +316,7 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
291316
return result
292317
})
293318
ipcMain.handle(SECURITY_IPC_CHANNELS.PURGE_FINDINGS, async (_e, findingIds: number[]) => {
294-
if (mutationWorker) return mutationWorker.purgeFindings(findingIds)
319+
if (currentMutationWorker) return currentMutationWorker.purgeFindings(findingIds)
295320
const publish = (change: unknown) =>
296321
Effect.sync(() => {
297322
getMainWindow()?.webContents.send(SECURITY_IPC_CHANNELS.EVT_FINDINGS_CHANGED, change)
@@ -302,8 +327,8 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
302327
ipcMain.handle(
303328
SECURITY_IPC_CHANNELS.PURGE_EVERYWHERE,
304329
async (_e, args: { kind: SensitiveKind; valueHash: string }) => {
305-
if (mutationWorker) {
306-
const out = await mutationWorker.purgeEverywhere(args.kind, args.valueHash)
330+
if (currentMutationWorker) {
331+
const out = await currentMutationWorker.purgeEverywhere(args.kind, args.valueHash)
307332
return { count: out.results.length, sessionIds: out.sessionIds }
308333
}
309334
const publish = (change: unknown) =>
@@ -464,23 +489,14 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
464489
safeSend(SECURITY_IPC_CHANNELS.EVT_SCAN_STATUS, status),
465490
),
466491
)
467-
// Mutation worker has its own per-mutation FindingsChange stream;
468-
// forward it onto the SAME EVT_FINDINGS_CHANGED renderer channel
469-
// so consumers can't tell whether the publish came from a scan
470-
// (via scan-worker) or a purge / dismiss (via mutation-worker).
471-
// No-op when the worker failed to boot — the per-handler fallback
472-
// sends its own EVT_FINDINGS_CHANGED so events still flow.
473-
if (mutationWorker) {
474-
mutationForwarderFiber = yield* Effect.forkDaemon(
475-
Stream.runForEach(mutationWorker.changes, (change) =>
476-
safeSend(SECURITY_IPC_CHANNELS.EVT_FINDINGS_CHANGED, change),
477-
),
478-
)
479-
}
480492
}),
481493
).catch(() => { /* fork rejected; ignore (cleanup path) */ })
482494

483-
return () => {
495+
// Mutation-worker change forwarder is forked when `attachMutationWorker`
496+
// fires, not at registration time, so the IPC layer is live even
497+
// before the worker has finished booting. Stored here so `dispose`
498+
// can interrupt it alongside the scan-worker forwarders.
499+
const dispose = (): void => {
484500
if (forwarderFiber) {
485501
Effect.runFork(Fiber.interrupt(forwarderFiber))
486502
}
@@ -495,4 +511,26 @@ export function registerSecurityIpc(deps: SecurityIpcDeps): () => void {
495511
ipcMain.removeHandler(ch)
496512
}
497513
}
514+
515+
const attachMutationWorker = (proxy: MutationWorkerProxy): void => {
516+
currentMutationWorker = proxy
517+
// Mutation worker has its own per-mutation FindingsChange stream;
518+
// forward it onto the SAME EVT_FINDINGS_CHANGED renderer channel
519+
// so consumers can't tell whether the publish came from a scan
520+
// or a purge / dismiss. Until this fork runs, the per-handler
521+
// fallback path sends its own EVT_FINDINGS_CHANGED for the same
522+
// mutations — events still flow, they just take the in-process
523+
// route.
524+
Effect.runPromise(
525+
Effect.gen(function* () {
526+
mutationForwarderFiber = yield* Effect.forkDaemon(
527+
Stream.runForEach(proxy.changes, (change) =>
528+
safeSend(SECURITY_IPC_CHANNELS.EVT_FINDINGS_CHANGED, change),
529+
),
530+
)
531+
}),
532+
).catch(() => { /* fork rejected; ignore */ })
533+
}
534+
535+
return { dispose, attachMutationWorker }
498536
}

0 commit comments

Comments
 (0)