Skip to content

fix(export): cancel PDF work when requesting document retires - #34907

Merged
OrcaWin merged 2 commits into
mainfrom
OrcaWin/pdf-export-document-lifetime
Oct 28, 2026
Merged

OrcaWin merged 2 commits into
mainfrom
OrcaWin/pdf-export-document-lifetime

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​417 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​417
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​52 $\color{#cf222e}{\Huge{\mathbf{−}}}$​9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​43

ELI5

A Markdown PDF export should stop when its requesting window document crashes or reloads, before it asks the user where to save. A file the user already chose to save still finishes writing.

What Changed

Bind the interactive export to the existing renderer-document lifetime and pass its AbortSignal into the converter. Retirement destroys the hidden renderer, removes its temporary document and listeners, suppresses late image/print continuations, and returns cancellation. Node receives the signal for best-effort initial-write cancellation; cleanup waits for that write to settle so a late-created file is still removed. Load-file and load-event rejections are observed together.

Captured exports continue through ordinary tab/file selection and same-document navigation. Concurrent requesters and a new document on the same surviving sender remain independent. Retirement while a save dialog is already open rejects its eventual selection before writing; a valid file write that already started finishes independently of later retirement. The existing print/security settings, exclusive temporary-file ownership, timeout and output bytes remain covered.

Why

A PDF export started from Markdown can outlive a crashed, reloaded, or replaced requesting document. The hidden export renderer and temporary HTML stay alive until conversion settles or its 60-second render timeout; a late conversion then opens a save dialog for an obsolete request. The export path was introduced by 3a58a829f94 (#882).

Linked Issue

Found during the authorized document-lifetime/resource audit; no pre-existing issue.

Visual Proof

Not captured. This draft changes obsolete native-dialog behavior, but no app/native window or save dialog was launched under the required background-launch policy. The production-seam before/after counts below are automated evidence; physical native-dialog and focus verification remains pending.

Testing

  • Manually tested in a native Electron app
  • Automated tests added/updated

Validation:

  • Actual IPC handler plus actual converter, with only Electron/fs/dialog boundaries mocked: unchanged main retains one temporary document and an undestroyed hidden window after each crash/navigation/destruction during image readiness; late images trigger one print and one save dialog. Fixed behavior destroys the window and removes the temporary document before late settlement, with zero late print or dialog calls.
  • Final regression on pinned main 7cdfebdc1becc12ae969bd34ea98186b7f3947ff: 19 new cases fail, 30 existing cases pass. Fixed focused run: 4 suites, 49 tests pass. Covers pending initial write, load-file/load-event, images and print; late resolution/rejection; replacement and overlapping requesters; stale save choice; accepted-write completion; and 10 alternating cycles (5 asserted successful saves with one print and the written PDF each, 5 retirements with no print, dialog or write) with no remaining listeners, timers or temporary documents.
  • Rebased onto main 14ab7f0c5ec; ORCA_BACKGROUND_LAUNCH=1 pnpm check:changed --maxWorkers=2: all 20 checks pass (833.9s).

No app or native save dialog was launched. Deterministic boundary counts establish production cleanup/admission and stale-result behavior; physical Electron print cancellation, native-dialog presentation, filesystem interruption timing, CPU/heap impact and native platform coverage remain unmeasured. An already-open save dialog stays under native ownership until it settles. This change adds no remote/mobile wire contract or workspace-path assumption.

Review

Self-reviewed lifetime boundaries, late promise handling, temporary-file ownership, successor/independent requesters and accepted-write completion. Coordinator owns independent review.

Agent skill upstream boundary

  • Not applicable; no upstream skill-installer material copied or translated.

Notes

Uses existing platform-neutral renderer events, Node AbortSignal and path handling. Remote/mobile contracts, git worktrees and folder workspaces gain no assumptions. Native Linux/macOS/Windows verification remains pending.

Checklist

  • Small and focused
  • User-facing before/after and mechanism explained
  • Native visual proof attached (pending; see limits above)
  • Self-reviewed correctness, security and resource cleanup
  • Cross-platform, remote/mobile and workspace behavior considered
  • Required changed-file gate: all 20 checks pass; complete build/native validation remains for CI and isolated desktop checks

@OrcaWin
OrcaWin force-pushed the OrcaWin/pdf-export-document-lifetime branch from ffa4b8d to 12fd28f Compare October 28, 2026 09:45
@OrcaWin
OrcaWin marked this pull request as ready for review October 28, 2026 09:45
@coderabbitai

coderabbitai Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 33a395a7-64b8-481a-b677-0af2f55da90a
Commits

Reviewing files that changed from the base of the PR and between b050ac2 and 12fd28f.

Files selected for processing: 4
  • src/main/ipc/export.test.ts
  • src/main/ipc/export.ts
  • src/main/lib/html-to-pdf.test.ts
  • src/main/lib/html-to-pdf.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@OrcaWin
OrcaWin merged commit 8405982 into main Oct 28, 2026
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant