RUWIO/dev_docs/07_electron-rendering-fix-plan.md

Electron Rendering — Fix Plan (Second-Review Findings #7–#13)

Date: 2026-06-06

This document is a follow-on to 05_electron-rendering-integration-plan_2.md. That plan tracked findings #1–#6 (all resolved). A second review of the uncommitted desktop-render changes surfaced a new set of issues, numbered #7–#13 here to avoid collision. Two of them (#7, #8) are user-facing data loss / crash bugs and should be fixed before any GUI/packaging sign-off.

Status (2026-06-06): all of #7–#13 are now IMPLEMENTED and verified. #10 and #11 took the interim-mitigation path (per-frame resize back to canvas size + fail-fast on canvases larger than the display), not the offscreen rewrite; #13 pads odd dimensions up to even on both render paths. All six test:render-desktop checks pass, as do the server smoke + e2e tests; #7, #8, and #13 were additionally verified with throwaway harnesses (overwrite safety; early-ffmpeg-exit surfaces an error without crash or hang; a 101×101 canvas encodes to a clean 102×102 MP4). See the per-finding "Status" notes below.

Note: plan_2's "What is already correct" section claims "Partial output file is removed on failure/cancel" as a safety property. That is precisely the mechanism behind finding #7 — the worker writes ffmpeg output straight to the user's chosen path, so "removing the partial file" deletes a pre-existing file the user chose to overwrite. The note should be corrected when #7 lands.

Summary table

#	Sev	File	One-line
7	CRITICAL	electron/renderWorker.cts	Direct-to-target ffmpeg write + fs.rm destroys a pre-existing file on cancel/ffmpeg failure (data loss).
8	HIGH	electron/renderWorker.cts	No error listener on ffmpeg.stdin; an early ffmpeg exit makes the next write() throw an uncaught EPIPE that crashes the main process.
9	HIGH (trivial)	package.json	"packageManager": "traversal@1.0.0" is a bogus value; breaks corepack-driven tooling.
10	MEDIUM	electron/renderWorker.cts	capturePage returns frames at the display scale factor → HiDPI output is N× the requested resolution.
11	MEDIUM	electron/renderWorker.cts	A non-offscreen render window can't be captured larger than the display → canvases > screen are truncated.
12	MEDIUM (perf)	electron/renderWorker.cts + server/render.html	Hidden-window rAF is throttled, so __animatrSeek hits its 1 s fallback every frame → renders crawl at ~1 fps.
13	LOW (optional)	electron/renderWorker.cts + server/server.js	Odd canvas width/height fails yuv420p/libx264 (shared latent bug).

Recommended sequencing: #9 (1 line) → #7, #8 (correctness) → #12 (one line, big perf win) → decide the #10/#11 strategy (offscreen vs. mitigations) → #13 (optional hardening).

---

#7 — CRITICAL: desktop export destroys an existing file on cancel/failure

Status: IMPLEMENTED. Worker renders to a sibling .<name>.<jobId>.partial file and promotes it with rename (rm-then-rename fallback on Windows) only after ffmpegDone resolves; the failure/cancel path only ever removes the temp file. One extra change was required: ffmpeg infers its muxer from the output extension, and the temp file has none, so the spawn now passes an explicit -f mp4 (this also hardens a final path the user typed without a .mp4 extension). Verified: cancel mid-render leaves a pre-existing sentinel file byte-identical with no leftover .partial.

Problem

renderMp4Job spawns ffmpeg writing directly to the user-chosen path:

const ffmpeg = spawn(ffmpegPath, [ '-y', ..., input.outputPath ])

and on any error/cancel does:

await fs.rm(input.outputPath, { force: true }).catch(() => {})

ffmpeg -y <outputPath> opens and truncates the target the instant it starts. If the user chose to overwrite an existing file (the native save dialog explicitly offers this), then any of:

• user clicks Cancel mid-render,
• the inactivity watchdog fires,
• ffmpeg exits non-zero,

leaves the original file already truncated, and the catch block then deletes it. Net result: the user's pre-existing file is gone and there is no replacement. The standalone server never has this problem because it renders into a private mkdtemp workDir and only exposes the finished file.

Fix (render to a sibling temp file, promote on success)

Write to a temp file in the same directory as the target (same filesystem ⇒ rename is atomic and can't hit EXDEV), and only touch the real path once the render has fully succeeded:

const finalPath = input.outputPath
const tmpPath = path.join(
  path.dirname(finalPath),
  `.${path.basename(finalPath)}.${input.jobId}.partial`,
)
// spawn ffmpeg with tmpPath instead of finalPath

// success path, after `await ffmpegDone`:
await fs.rm(finalPath, { force: true }) // Windows rename won't replace an existing dest
await fs.rename(tmpPath, finalPath)
return { path: finalPath }

// catch path:
await fs.rm(tmpPath, { force: true }).catch(() => {}) // NEVER finalPath

Notes:

• The rm(finalPath) before rename only runs after a successful render (tmp file is complete), so the old file is replaced, never lost.
• Keep '-movflags', '+faststart' — ffmpeg still finalizes into tmpPath.
• Optionally sweep stale .*.partial files in the target dir on worker start (best-effort), to cover hard crashes.

Verification

• New scripts/verify-overwrite.cjs: pre-create a sentinel file at the target path, start an export to it, cancel mid-render, assert the sentinel bytes are unchanged and no .partial file remains.
• Re-run scripts/verify-cancel.cjs (already passing) — must stay green.

---

#8 — HIGH: an early ffmpeg exit crashes/hangs the worker

Status: IMPLEMENTED. The investigation found the EPIPE was only one of three failure modes when ffmpeg exits mid-render; all three are now fixed:

1. EPIPE crash — added ffmpeg.stdin.on('error', …) so a write to the dead pipe can't become an uncaughtException.
2. Unhandled-rejection crash — ffmpegDone rejects mid-loop, long before the post-loop await attaches a handler; under Node's default --unhandled-rejections=throw that terminated the process. Now a no-op handler is attached at creation (void ffmpegDone.catch(() => {})); real handling still flows through await ffmpegDone. (This is why the original cancel test passed but the early-exit path died — the cancel path attaches its handler within Node's grace window, the early-exit path does not.)
3. Backpressure hang — when ffmpeg is dead, stdin.write returns false and the old await stdin.once('drain') waited forever (no drain ever comes, and it wasn't abortable). The wait now races ffmpegDone and raceAbort(signal).

Also added if (settled) break at the top of the frame loop so a dead ffmpeg stops being fed. Verified: a stub ffmpeg that exits non-zero mid-stream makes renderMp4Job reject with the ffmpeg error, and the process survives and exits 0 (no UNCAUGHT_EXCEPTION / UNHANDLED_REJECTION, no hang).

Problem

The only error listeners are on the ffmpeg ChildProcess (spawn/error/ exit) and ffmpeg.stderr. There is no listener on ffmpeg.stdin. If ffmpeg exits early (malformed frame, disk full, codec/resource error), the next loop iteration runs:

if (!ffmpeg.stdin.write(png)) { ... }

Writing to a pipe whose read end is closed emits an asynchronous 'error' (EPIPE) on the stdin stream. Because it is async, the surrounding try/catch does not catch it; with no 'error' listener it becomes an uncaughtException and takes down the entire Electron app (not just the export). The standalone server has the same write pattern, but there it only crashes an isolated, restartable server process — in the desktop app it kills the user's editor.

Fix

1. Swallow the stdin error (the real failure is already surfaced via the exit/ffmpegDone path):

   ffmpeg.stdin.on('error', () => { /* EPIPE after early exit; surfaced via ffmpegDone */ })
   

2. Stop feeding a dead ffmpeg so the real error surfaces promptly. settled is already set in the exit handler — check it at the top of the loop:

   for (let i = 0; i < input.options.totalFrames; i += 1) {
     if (signal.aborted) throw abortReason(signal)
     if (settled) break            // ffmpeg already exited; quit feeding it
     ...
     ffmpeg.stdin.write(png)
   }
   ffmpeg.stdin.end()
   await ffmpegDone                 // rejects with "ffmpeg exited with code N: <stderr>"
   

Verification

• New scripts/verify-ffmpeg-early-exit.cjs: induce an early ffmpeg exit (e.g. point FFMPEG_PATH at a stub that reads a few bytes then exit(1), or kill the ffmpeg child mid-stream) and assert renderMp4Job rejects with a surfaced ffmpeg error and the process does not crash (the test harness itself surviving is the assertion).

---

#9 — HIGH (trivial): bogus packageManager field

Status: IMPLEMENTED. Line removed from package.json; valid JSON, no references remain.

Problem

package.json:6:

"packageManager": "traversal@1.0.0",

traversal is not a package manager. It is referenced nowhere and is absent from package-lock.json — almost certainly an accidental insertion. In any corepack-enabled environment (Node ships corepack; CI or contributors may have run corepack enable), invoking npm/yarn/pnpm reads this field and errors (Unsupported package manager 'traversal'), blocking installs and builds.

Fix

Delete the line. (If a manager really must be pinned, use a real value like "npm@10.x".)

Verification

npm install / npm ci and corepack npm -v succeed in a clean checkout.

---

#10 / #11 / #12 — Capture fidelity & performance

These three share a root cause: the render window is a hidden, non-offscreen BrowserWindow, and capturePage is coupled to the physical display (scale factor and size) and to Chromium's background throttling. The strategic fix is offscreen rendering (already floated in plan_2 finding #1); pragmatic per-issue mitigations are listed if offscreen is deferred.

#10 — MEDIUM: HiDPI doubles the output resolution

Status: IMPLEMENTED (interim mitigation). The frame loop now calls frame.getSize() and, when the captured NativeImage doesn't match the requested canvas, frame.resize({width, height}) back to the canvas dims before toPNG(). No-op on scale-factor-1 displays. The offscreen "deep fix" below was not taken. (Can't be reproduced on this scale-1 box; should be sanity-checked once on a HiDPI machine.)

capturePage(captureRect) takes a rect in DIPs but returns a NativeImage at the display's device scale factor. On a 2× display a 1920×1080 canvas yields a 3840×2160 PNG, so the MP4 is double the requested resolution and ~4× the size — diverging from the server, which pins deviceScaleFactor: 1. Likely unnoticed so far because the verify harness runs at scale factor 1.

Interim mitigation (normalize every frame to the requested size):

const frame = await raceAbort(renderWindow.webContents.capturePage(captureRect), signal)
const { width, height } = frame.getSize()
const png = (width === captureRect.width && height === captureRect.height
  ? frame
  : frame.resize({ width: captureRect.width, height: captureRect.height })
).toPNG()

(Do not use app.commandLine.appendSwitch('force-device-scale-factor','1') — it is global and would degrade the editor UI on HiDPI.)

#11 — MEDIUM: canvases larger than the display are truncated

Status: IMPLEMENTED (interim mitigation). main.cts now compares the normalized canvas against screen.getPrimaryDisplay().size before starting and throws a clear error ("Canvas WxH is larger than this display …") rather than silently truncating. This is desktop-only: the web/server path keeps using a headless viewport and still accepts up to 4096². The offscreen "deep fix" below (which would let the desktop path render oversize canvases too) was not taken.

A non-offscreen window can't be composited/captured beyond the display bounds, so a canvas larger than the screen (validation allows up to 4096²) produces a clipped frame. The server avoids this with a headless Chrome viewport.

Interim mitigation: in main.cts, before starting, compare the (normalized) canvas against screen.getPrimaryDisplay().workAreaSize and fail fast with a clear error rather than silently truncating; or document a desktop max-canvas limit. (The real fix is offscreen.)

#12 — MEDIUM (perf): hidden-window rAF throttling ⇒ ~1 fps

Status: IMPLEMENTED. backgroundThrottling: false added to the render window's webPreferences. (#10 and #11 remain open; the offscreen-rendering "deep fix" below was not taken.)

__animatrSeek now resolves after a double requestAnimationFrame, with a setTimeout(finish, 1000) anti-hang fallback. But the render window is show: false with no backgroundThrottling: false, so Chromium throttles/ pauses rAF for the hidden document — the rAFs don't fire promptly and the 1 s fallback becomes the per-frame path. A 10 s @ 30 fps render then takes ~5 min. (The verify harness checks correctness, not throughput, so this slipped through.)

Fix (one line, large win):

webPreferences: {
  ...
  backgroundThrottling: false, // keep rAF firing while the window is hidden
}

Deep fix (recommended) — offscreen rendering, subsumes #10/#11/#12

Switch the render window to webPreferences: { offscreen: true }:

• renders to an in-memory bitmap independent of the physical display (fixes #11 oversize) at a controllable scale (fixes #10 HiDPI),
• isn't background-throttled and pairs with webContents.setFrameRate(...) (fixes #12),
• capture via either the paint event / beginFrameSubscription or capturePage (still pair with the existing rAF settle to avoid the off-by-one from plan_2 #2).

Cost/risk: changes the capture mechanism, so the entire test:render-desktop suite (all 6 checks) must be re-run, plus new HiDPI + large-canvas checks, plus a GPU-vs-software-rendering sanity pass on each platform. If offscreen lands, #10/#11/#12 close together and the interim mitigations above are unnecessary.

Verification (either path)

• New scripts/verify-resolution.cjs: render and ffprobe the output; assert width/height exactly equal the canvas dims. (Won't reproduce #10 on a scale-factor-1 CI box; note that, and run it once on a HiDPI machine.)
• New scripts/verify-large-canvas.cjs: render a canvas larger than the test display; assert full-size, non-truncated frames.
• New throughput assertion (extend verify-export.cjs): time N frames and fail if per-frame wall-clock is near the 1 s fallback — guards the #12 regression.

---

#13 — LOW (optional): odd canvas dimensions fail yuv420p

Status: IMPLEMENTED (pad to even). Both ffmpeg spawns (desktop worker and standalone server) now carry -vf pad=ceil(iw/2)*2:ceil(ih/2)*2, so an odd canvas encodes (e.g. 101×101 → 102×102) instead of crashing with "height not divisible by 2". Validation was left permissive (no reject). Verified by a throwaway harness rendering a 101×101 scene to a clean 102×102 H.264 MP4, and the server e2e test still passes.

Problem

libx264 with -pix_fmt yuv420p requires even width/height. Validation allows any integer 1..4096, so an odd-dimension canvas (e.g. 101×101) makes ffmpeg exit with a confusing "height not divisible by 2" error. This is shared with the standalone server (pre-existing), but the new desktop path is a fresh surface for it. On HiDPI it's accidentally masked because the 2× capture is always even — fixing #10 (normalize to canvas size) re-exposes it.

Fix (pick one, apply to both desktop worker and server)

• Pad to even in the ffmpeg args (least surprising, adds ≤1 px): '-vf', 'pad=ceil(iw/2)*2:ceil(ih/2)*2', or
• Reject odd dimensions in shared/renderValidation.cjs with a clear message.

Verification

Add an odd-dimension case (e.g. 101×101) to shared/renderValidation.test.cjs and/or a desktop check asserting a clean result instead of an ffmpeg crash.

---

Out of scope / already correct (do not redo)

• Validation parity, the abort/watchdog plumbing, the cancel-before-jobId race, ENOENT friendly message, packaging wiring, and the __animatrSeek paint settle (plan_2 #2) are all sound and unchanged by this plan.
• Image assets are imported as data: URLs, so the render window's *://*/* network block (plan_2 #5) does not drop them — no action needed.

Suggested doc bookkeeping

When these land, update 05_electron-rendering-integration-plan_2.md:

• correct the "Partial output file is removed on failure/cancel" note (#7),
• and 06_electron-rendering-manual-checks.md with the new verify scripts.