Skip to content

fix(viewer): actively cancel superseded PDF.js work - #347

Draft
seonghobae wants to merge 3 commits into
fix/viewer-generation-safe-renderingfrom
fix/viewer-active-pdfjs-cancellation-after-323-20260810
Draft

fix(viewer): actively cancel superseded PDF.js work#347
seonghobae wants to merge 3 commits into
fix/viewer-generation-safe-renderingfrom
fix/viewer-active-pdfjs-cancellation-after-323-20260810

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Objective

Advance the remaining resource-cancellation acceptance in issue #322 without moving parent #323's exact reviewed/verified identity. A superseded viewer operation must not only suppress stale DOM publication; where PDF.js exposes cancellation/destruction APIs, it must actively cancel the current RenderTask and destroy an in-flight PDFDocumentLoadingTask.

Fresh exact stack state — 2026-08-10

  • exact child head: 29438e42327a1bd5b888422fbfde713f7fca9b5a;
  • exact parent fix(viewer): suppress stale PDF render publication #323 head: 8914580339c644f2cfdfef6a1634dfd5cff53693;
  • parent fix(viewer): suppress stale PDF render publication #323 remains directly based on protected main 55d7ae8647208e301f282350f076eeddaba61d11;
  • child is intentionally stacked on fix(viewer): suppress stale PDF render publication #323 because both slices own viewer.js and this run did not move the parent exact identity;
  • exact-head CI 31406253421: success;
  • exact-head fuzz 31406253440: success;
  • CI checked out the exact child SHA, ran 496 Maven tests with 0 failures, 0 errors and 0 skips, passed all JaCoCo coverage checks and public Javadocs, passed buyer-readiness script tests, and passed synthetic parent-merge compatibility;
  • no formal reviews or unresolved review threads at the latest direct refetch.

Because this stacked PR targets the parent branch rather than main, no main-targeted Security Scan/SAST result is synthesized as passing evidence. After #323 integrates and this child is rebuilt/reconciled onto the new exact protected main, all base-sensitive security/review evidence must be regenerated before final merge.

Test-first / RCA evidence

RED

Test-only head 5e7cbd2c6a868332ea8c1da371ce73cc46253d5d added a real Node runtime regression around PDF.js resources before production changes. Exact-head CI 31405521868 failed Buyer-readiness scripts exactly at the first intended boundary: aborting a viewer operation while the first-page RenderTask.promise was held pending produced cancelCalls = 0, proving parent #323 suppressed stale publication but did not actively cancel the renderer.

First GREEN attempt exposed a harness race

Production head 36bb461ed7d02febea446d3a1bce3c2fff163b99 added active cancellation/destruction, but exact-head script CI still reported cancelCalls = 0. RCA showed the regression used a fixed count of resolved microtasks before aborting; that did not prove page.render() had actually started in the Node VM. The failure was therefore timing-sensitive test synchronization, not proof that the new cancel listener was absent. The test was corrected to expose explicit renderStarted / loadingStarted synchronization boundaries instead of sleeping/yielding an assumed number of microtasks.

GREEN

Current renderPdfInline(...) now:

  • retains the current PDF.js RenderTask and calls cancel() exactly once when the operation aborts;
  • destroys a still-loading PDFDocumentLoadingTask on abort when PDF.js exposes destroy();
  • guards cancellation with a one-shot state flag so abort observation cannot double-cancel resources;
  • treats cancellation/destruction as best-effort resource cleanup while existing abort checks remain authoritative against stale publication;
  • suppresses cancellation errors only when the caller's abort signal is actually set and continues to surface non-cancellation failures;
  • removes the abort listener in finally and still destroys a resolved PDFDocumentProxy.

The synchronized Node regression proves both active render-task cancellation and loading-task destruction. The unchanged exact head is GREEN across buyer-readiness runtime scripts, full Maven verify/coverage/Javadocs, synthetic parent-merge compatibility and fuzz.

Scope / remaining issue contract

This child does not duplicate #323 stale-publication suppression, issue #317 identity/bootstrap work, #264 demo.js/dom-utils.js, signed-token authority, terminal focus ownership, or rapid three-generation/error-state coverage. Signed-token mode still invokes renderPdfInline without a generation signal; parity remains open under issue #322.

Do not close issue #322 when this child eventually integrates. Remaining acceptance includes rapid multi-generation/error-state coverage, signed-token lifecycle parity, terminal status/focus ownership, and proof that cancellation cannot surface an unhandled rejection in the supported PDF.js runtime.

Dependency gate

Keep this PR Draft while parent #323 is open. Merging the child into the parent branch would move #323's exact reviewed/verified identity and collapse two independently reviewable semantics. After #323 reaches protected main, rebuild/reconcile this unique delta onto the new exact protected base, regenerate all main-targeted CI/security/review evidence, and only then move the child toward final merge.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 24e463f9-d642-4fae-bb7d-098b2b0d43f2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

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