Repository navigation
Conversation
|
excellent!! |
|
While investigating CI failures in unrelated PRs, I reproduced two regressions on current main: open_then_run loses its requested command after asynchronous preparation, and startup bytes can be consumed while a local password retry is still pending. Replacing fixed-frame waits with condition-based waits exposed both as actual behavior failures rather than only timing-sensitive tests. I see this PR already covers those continuation/ordering paths, so I am holding a separate narrow fix to avoid overlapping production changes. Useful regression cases are: tool execution exactly once after plain/encrypted opening; no tool execution on wrong password or cancellation; successful/failed text/image conversion; and startup bytes remaining queued while local password preparation is held behind a deterministic barrier. The six affected integration binaries also need completion-aware waits if they exercise the asynchronous mode. Our independent narrow patch plus those waits passed 2,996 tests (13 existing ignored) and the workspace gates on base fff985f. That is evidence for the regression cases, not validation of this PR’s full integration. Current native CI in #1672 stops at the open-then-tool test before reaching the unrelated extent regression. |
|
should be ok now |
FredApps
left a comment
There was a problem hiding this comment.
Re-reviewed 6d1f5c2. The two continuation/ordering concerns are addressed: deferred tools follow the correct open request, and startup bytes stay queued through pending password preparation. I also checked lazy-index invalidation, the independent COS inspection view and pre-validation signed-document guards.
Independent Linux headless validation at this exact head: 18 open-worker tests, 36 picker tests, both deferred-tool/recent-open command regressions, and 9 engine/COS/render regressions passed (65 total). Hosted Windows/Linux/WASM checks were also green at this head.
No further blocker found in the reviewed scope. This does not claim a cold-cache large-file first-frame benchmark or native interactive validation; the documented conversion and M1 scope limits remain.
|
Rechecked the upstream-sync delta at exact head Fresh independent Linux validation at this head passed 28 worker tests, 2 file-watcher regressions, all 36 picker tests, and the deferred-tool/recent-file command regressions: 68 passed, zero failures or ignored tests. The checkouts remained clean and assertions were unchanged. My earlier source-isolation, lazy-index and signed-file review still applies; these fresh tests do not repeat the full workspace or establish cold-cache first-render performance. |
|
CI follow-up: the full hosted run has now failed on both Ubuntu and Windows in the two encrypted recovery tests, outside the targeted batch above: I am checking whether the synchronous recovery path makes that predicate true before the password modal is drawn. This is not yet evidence of an encrypted recovery behavior failure, but CI is not clear until the distinction is verified. |
|
@LabDennis Confirmed the cause at Keeping every original assertion and waiting for Please use a bounded condition-based wait for that interactive field at the two query sites, rather than adding a fixed frame count. The existing Cancel-button wait in the same recovery test file follows this pattern. No production recovery change was needed in this diagnostic, and the encrypted bytes, dirty state, page counts, metadata and saved-path assertions all remained intact. Diagnostic changes have been restored; this still needs the test adjustment and a fresh CI run. |
|
Rechecked the recovery-test adjustment at exact head f87b2c7. The two sites now wait for a present, enabled PasswordInput with a 30-second bound, and the original behavioral assertions remain intact. This addresses my timing finding without changing production recovery code. The fresh hosted CI run now passes Ubuntu, Windows, WASM and dependency checks (fuzz skipped): https://github.com/storytold/pdfcraft/actions/runs/38523965264 . My recovery-test CI blocker is cleared. This update is source review plus verification of hosted checks; I did not rerun a separate local suite on this new head. The previously documented M1/performance scope limits remain. |
…aration Consolidate the reviewed change and its follow-ups on current upstream. Original change notes: M1: Defer document-open scans and share inspection state Integrate deferred marker/link indexes, on-demand signature details and shared inspection parsing to reduce large-document open work. Preserve signed-file rewrite guards independently of signature validation. Keep inspection caches independent, retain stricter stream limits, and fall back to source parsing after object, trailer or encryption edits. Add regression coverage for source isolation, lazy cache invalidation, pre-validation rewrite refusal and signature panel behavior. Based on @cyfung1031's proposals: - storytold#1434 (a4cdc2d) - storytold#1465 (b9cd81f) - storytold#1527 (eaa0c27) Refs storytold#307. Background desktop opening and compact xref storage remain separate; this does not claim to fix cold-cache first rendering. M1: Keep desktop responsive throughout document opening Build on @cyfung1031's background-open proposal storytold#1493 at 355d4c5. Split engine preparation from registration and keep CLI/MCP opening synchronous. Explicitly enable the native worker before startup files, carry tool and navigation continuations through password retries, serialize modal completion, and preserve pending session paths on quit. Reprepare stale security-policy snapshots before registration. Keep the bounded worker and UI completion state machine separate. Keep startup view options attached to their original request through retries, failures and cancellation. Add deterministic responsiveness and opening-flow regressions. Seven regressions were observed failing before their fixes. Pin the existing reload test's English labels to an explicit English test locale. Refs storytold#307 Based-on: storytold#1493 M1: Keep startup bytes queued during password preparation Preserve the local password interaction until background preparation finishes. Add a deterministic barrier regression and exercise deferred tools, wrong passwords, cancellation, and text/image conversion success and failure. Cover the public open-then-tool flow in both synchronous and background hosts. test(ui): wait for interactive recovery password fields (storytold#1634) M1: apply backpressure to native document preparation Keep queued paths unread until the UI can consume their result, so a password or tool dialog does not retain every prepared PDF in a batch. Use the UI pending order for both password retries and stale-policy repreparation instead of independently ordering the worker queue. Add regressions for held results, newly queued files behind a password prompt, and security repreparation ahead of later reads. All three fail on the preceding PR head; 1,130 UI tests pass after the change. Refs storytold#307
49a6538 to
7cc67ec
Compare
|
Rechecked the squashed/upstream-sync head 7cc67ec against the previously reviewed f87b2c tree. The actual file-tree delta is the queue backpressure follow-up, its tests/docs and the upstream version bump; the earlier integration is retained. I found no new blocker in the reviewed request-ordering/backpressure code. Fresh independent Linux validation at this exact head passed 101 tests: 31 open-worker, 11 recovery, 36 picker and 23 command tests, with zero failures or ignored tests. This covers held-result backpressure, paths staying unread behind a password modal, security-policy re-preparation priority, and open-then-tool continuations. The bounded recovery waits and existing behavioral assertions remain intact; the checkout stayed clean. Hosted CI is still pending, so this does not claim all gates are green. This is scoped headless validation, not a new full-workspace run, native desktop benchmark, or cold-cache/first-render measurement. The documented M1 limits still apply. |
|
CI follow-up: the dependency gate now fails because Cargo.lock contains rustls 0.23.45, flagged by today's RUSTSEC-2026-0346 through 0349. The audit requires >=0.23.46; bans, licences and sources pass. This is inherited from current main 9931329, which locks the same version/checksum, rather than a change introduced by this PR. Existing PRs #2181 and #2216 already contain the minimal rustls 0.23.46 version/checksum update. Once one is merged, please update this branch from main and run fresh CI rather than suppressing the advisories or duplicating the fix. The 101 focused tests reported above remain valid for the reviewed head, but do not clear this separate dependency gate. Failed job: https://github.com/storytold/pdfcraft/actions/runs/38604055308/job/115990282408 |
…aration Consolidate the reviewed change and its follow-ups on current upstream. Original change notes: M1: Defer document-open scans and share inspection state Integrate deferred marker/link indexes, on-demand signature details and shared inspection parsing to reduce large-document open work. Preserve signed-file rewrite guards independently of signature validation. Keep inspection caches independent, retain stricter stream limits, and fall back to source parsing after object, trailer or encryption edits. Add regression coverage for source isolation, lazy cache invalidation, pre-validation rewrite refusal and signature panel behavior. Based on @cyfung1031's proposals: - storytold#1434 (a4cdc2d) - storytold#1465 (b9cd81f) - storytold#1527 (eaa0c27) Refs storytold#307. Background desktop opening and compact xref storage remain separate; this does not claim to fix cold-cache first rendering. M1: Keep desktop responsive throughout document opening Build on @cyfung1031's background-open proposal storytold#1493 at 355d4c5. Split engine preparation from registration and keep CLI/MCP opening synchronous. Explicitly enable the native worker before startup files, carry tool and navigation continuations through password retries, serialize modal completion, and preserve pending session paths on quit. Reprepare stale security-policy snapshots before registration. Keep the bounded worker and UI completion state machine separate. Keep startup view options attached to their original request through retries, failures and cancellation. Add deterministic responsiveness and opening-flow regressions. Seven regressions were observed failing before their fixes. Pin the existing reload test's English labels to an explicit English test locale. Refs storytold#307 Based-on: storytold#1493 M1: Keep startup bytes queued during password preparation Preserve the local password interaction until background preparation finishes. Add a deterministic barrier regression and exercise deferred tools, wrong passwords, cancellation, and text/image conversion success and failure. Cover the public open-then-tool flow in both synchronous and background hosts. test(ui): wait for interactive recovery password fields (storytold#1634) M1: apply backpressure to native document preparation Keep queued paths unread until the UI can consume their result, so a password or tool dialog does not retain every prepared PDF in a batch. Use the UI pending order for both password retries and stale-policy repreparation instead of independently ordering the worker queue. Add regressions for held results, newly queued files behind a password prompt, and security repreparation ahead of later reads. All three fail on the preceding PR head; 1,130 UI tests pass after the change. Refs storytold#307
7cc67ec to
b42a850
Compare
Reuse the editable COS parse for inspection only while it represents the same unmodified source. Keep independent caches and existing stream limits. Extend native background opening with UI-driven admission: later paths remain unread while a prepared result, password prompt, picker or tool interaction waits. Preserve request-owned startup options, session pages, recovery and tool continuations; reprepare stale security-policy snapshots. Retain upstream's eager signature validation, rendered verdicts, trust refresh and signed snapshots. Keep exact structural rewrite guards for adopted signature fields and standalone document timestamps. Build on cyfung1031's shared-parsing proposal storytold#1527 (eaa0c27), marker/link index proposal storytold#1434 (a4cdc2d), and background preparation storytold#1493 (355d4c5). The latter two are already upstream; preserve their implementations. Refs storytold#307. Cold-cache first rendering, global memory budgets and interrupting an active parse remain outside this change.
b42a850 to
c348bf4
Compare
|
Updated onto f0c9901 while retaining upstream #1895's eager signature validation, rendered verdicts, trust refresh and signed snapshots. Removed the obsolete deferred-signature behavior and corresponding claims from the PR description. The remaining change shares source inspection and adds UI-driven admission/backpressure and request-owned opening continuations. The rebase keeps the current file-picker admission guard: an already-prepared document cannot replace the recipient of an open picker. The upstream picker tests reproduced the regression before that guard was restored. Queue-bound fixtures now use distinct paths to respect upstream deduplication; the recovery test releases its store clones before checking that the instance-lock directory is empty. Windows validation against upstream f0c9901: 276 engine unit tests, 448 UI unit tests and 71 integration tests across picker, recovery, reload, signing and signer-trust flows passed. The engine and UI unit suites each retain one existing ignored test. Scoped all-target Clippy ( The PR remains one focused commit. Earlier measurements were not rerun on this tree; the documented M1/global-memory/cold-cache scope limits remain. Roadmaps are unchanged. |
FredApps
left a comment
There was a problem hiding this comment.
Scoped revalidation at c348bf4 after the upstream refresh. I reviewed the current production/test delta and found no new blocking issue in the signature-image loading and admission paths.
Independent checks passed: engine library276/1 ignored; UI library456/1 ignored; pickers36, recovery12, reload11, signing UI11, signer trust1; COS2 and render2 regressions; signing crate117/1 ignored. Formatting, affected strict Clippy, UI/web WASM, assets and layers passed. Seven WASM dead-code warnings remain. The initial unchanged font test failed with the validation wrapper setting PDFCRAFT_SYSTEM_FONTS=0; after unsetting that variable only for child processes, the focused test and complete UI suite passed. No code or assertion changes were made; all1429 tracked hashes matched and final Git status was clean. Engine was not rerun during that second pass. Hosted CI38877148112 is successful, fuzz skipped.
These are scoped correctness checks, not fresh cold-cache performance, native macOS/M1 testing or proof that every global memory-budget concern is solved.
What this changes
Refs #307. Reuse the unmodified source parse during document inspection and bound background PDF preparation to the next result the UI can accept. Later paths stay unread while a prepared result, password prompt or other modal interaction waits. Up to 64 requests remain accepted.
The desktop enables background opening before startup files and session restoration. Request-owned continuations preserve startup view options, restored pages and focus, requested tools, remote destinations, recovery metadata and password retries. Failed opens cannot run a tool on a different tab. Results prepared under stale JavaScript or certificate-trust settings are prepared again before registration.
Inspection only shares the COS parse for the same unmodified source allocation; edited objects, trailers or encryption force independent source inspection. Inspection caches remain independent and existing stream limits are not raised. Exact structural signed-file guards include adopted fields and standalone timestamps.
The current branch retains upstream #1895's eager signature validation, rendered verdicts, status banner, trust refresh and signed snapshots. It does not defer signature details or require opening the Signatures panel to validate them. Source and UI completion logic are separated into small modules; CLI/MCP opening remains synchronous.
Source work and remaining scope
Credit to @cyfung1031's source proposals: #1527 (
eaa0c275c31effe8cc0a5e1423045ae809fdeb77) for shared parsing, #1434 (a4cdc2d4bb1423cb00560b5092be4f1f14d04537) for deferred marker/link indexes, and #1493 (355d4c5be83bf208565b9a7aaf295a3735ceed95) for background preparation. The latter two have landed upstream; this branch preserves them and adds the remaining integration and admission control. #1465's deferred-signature proposal is no longer included because current upstream needs those verdicts during rendering.This is a partial improvement for #307, not a claim to finish M1. Cold-cache opening and first rendered frame remain unmeasured on this rebased tree. Admission control is not a global byte/RSS budget: an active parse is not interrupted, and already-open documents and caller-owned buffers are outside the bound. Image/text conversion still runs on the UI thread after reading. Compact xref storage remains separate work.
Earlier benchmark measurements were made on older implementations and have not been repeated on this tree; they are not performance claims for the current head.
Validation
Windows validation against upstream f0c9901: 276 engine unit tests, 448 UI unit tests and 71 integration tests across picker, recovery, reload, signing and signer-trust flows passed. The engine and UI unit suites each retain one existing ignored test. Scoped all-target Clippy (
--no-deps, warnings denied), UI WASM, desktop Cargo check, formatting, worktree-specific assets and layering checks passed. Hosted CI must complete independently.No new assets, fonts or dependencies. Control/MCP remain opt-in; source confinement is unchanged. Roadmaps and local agent instructions are unchanged.