Skip to content

fix(runtime): persist mobile notification dismissals without blocking Windows - #26754

Closed
nwparker wants to merge 7 commits into
mainfrom
nwparker/win-issues-blocking-local-1008
Closed

nwparker wants to merge 7 commits into
mainfrom
nwparker/win-issues-blocking-local-1008

Conversation

@nwparker

@nwparker nwparker commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 15 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​628 $\color{#cf222e}{\Huge{\mathbf{−}}}$​93 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​535
Prod 11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​319 $\color{#cf222e}{\Huge{\mathbf{−}}}$​78 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​241

ELI5

Windows notification saves now run in the background, so permission tools do not hold up unrelated Orca work. Restarting from settings also uses the existing bounded shutdown path so pending dismissal history can finish saving before the process exits.

What Changed

The shared asynchronous secure writer serializes each destination, checks temporary-file permissions before rename, verifies the published file, preserves optional durability, and removes temporary files after thrown failures. Notification dismissal snapshots coalesce changes and report write/permission failures; unreadable history is preserved. The first asynchronous Windows user-SID lookup shares successful results with the existing synchronous lane.

app:relaunch now uses quitProcess(), as administrative restart and profile switching already do. Its existing auth cleanup still finishes first. The existing quit pipeline stops structured-session and RPC producers, drains notification persistence, and enforces its existing 20-second teardown deadline; tray cleanup runs there too. Recovery fixtures, including the standalone relay retry case, now drain pending snapshots before opening replacement controllers or reading disk, and stop their producers before draining/removing test state.

Why

This preserves the existing permission checks and retry policy while moving notification writes off the main thread. Reusing the quit pipeline fixes a captured relaunch loss without a second shutdown system or an unbounded flush. Existing synchronous APIs and consumers remain available; the writer and notification consumer already occupy separate modules, so another PR split would not remove their shutdown dependency.

Adapted bounded portions of LDH1103's #24029 (a1a2438, 47e0fa2) and llvm-x86's #20498 (8d44d96). Published follow-ups preserve ancestry and the exact coauthor trailers LDH1103 <ldh517525@gmail.com> and Loom <loom@local>.

Linked Issue

Fixes #26027. Refs #20497.

This removes the reproduced notification write blocking mechanism; it does not establish that the reported multi-second agent-prompt workload freeze or #11363 is fully resolved.

Visual Proof

Windows relaunch boundary, before at 1296bc1e44f and after at exact final head 17cf9ab1ea7f0169aba8f7726322c8ce2c18a506:

Before: restart loses the queued dismissal

After: restart retains the original delivery dismissal

A hidden offscreen Electron harness uses the actual renderer restart preparation, IPC handler, quit pipeline, dismissal store, secure writer, ACL/SID code, and auth-preservation orchestrator. Playwright CDP clicks its rendered Relaunch button; Electron exits and actually launches its replacement. Injected 750ms rename delay and a final producer dismissal reproduce the boundary loss. The after transcript shows the producer's final dismissal, persistence drain, then exit. Auth/profile services use synthetic file-backed markers; these images are diagnostic results, not a full settings UI or live account test. No window was revealed or activated.

Earlier Windows write-path timing evidence remains:

Native Windows timing and injected slow-spawn stress

Prior exact-source probes measured native record() at 79–117ms before versus 0.08–0.29ms after; the hidden Electron probe measured 117.37ms versus 0.17ms. The separately labeled 250ms-per-spawn injection measured 1,411ms versus 0.07ms. These establish the blocking mechanism, not the user's complete agent-prompt workload.

Testing

  • Exact final head 17cf9ab1ea7f0169aba8f7726322c8ce2c18a506 tested on real Windows (awin, Node v24.18.0, pinned Bun 1.4.2), isolated clone C:/opr54risk: 22 files, 196 passed / 16 platform skips in one explicit run; full pnpm tc and full PR changed-code quality pass.
  • Windows coverage exercises real filesystem ACL repair/verification, durability, concurrent publishers, failed writer recovery/temp cleanup, SID failure/backoff/races, unreadable history, host restart, and bounded quit. The shutdown boundary holds session teardown, RPC teardown and persistence independently to prove final producer changes are included before exit.
  • Final macOS notification/read/restart/profile/auth/veto contracts: 15 files / 133 tests pass. Writer/SID/store/fault/concurrency/quit focused checks: 7 files / 80 tests pass with unchanged writer/SID production source. Full pnpm tc and original-base changed-code quality pass, zero new findings across 26 PR-changed files.
  • Completed CI run 37890983176 had nine introduced fixture failures across shards 1–3: one queued-frame disk read, one replacement-controller startup, and seven immediate relay snapshot reads. All were reproduced before correction; explicit persistence drains retain the original identity, unseen-prompt and completion assertions. The verify failure only aggregates the failed test jobs. Static/mobile/package/Windows-package/relay-integration jobs and shards 4–5 passed on that older head.
  • Every test/app used ORCA_BACKGROUND_LAUNCH=1; hidden renderer screenshots used Playwright CDP. Local generated diagnostics were excluded from quality scans; every committed production/test file was scanned. Initial Windows missing-Bun and shallow-merge-base setup failures were corrected before passing runs.

Full local repository test/lint/build, Linux/SSH end-to-end, live account credentials, WSL auth promotion and native focus were not exercised. New-head CI must still run.

Review

Reviewed temporary/published permission ordering, false/error outcomes, serialization cleanup, consumer recovery boundaries, producer-before-drain ordering, renderer capture/veto contracts, profile/auth sequencing and shutdown deadlines. The relaunch follow-up changes one production handler and seven test/fixture files, preserving original authorship and existing runtime APIs.

Agent skill upstream boundary

  • Not applicable: no upstream skill-installer material copied.

Notes

Persistence remains best-effort. Crashes, OS termination, other direct forced-exit paths, write/ACL failures and expired shutdown deadlines can lose queued history. ACL failures preserve the existing publish-and-return-false contract. The writer requires one path spelling and one write lane per destination; path aliases and mixed sync/async writers are not coordinated. Other synchronous IPC/startup paths remain outside this change.

Notification identity and folder workspace recovery assertions remain intact. Persistence belongs to the execution host; RPC/notification fields are unchanged for mixed-version peers. Renderer preflight already requests editor hot-exit backup and a durable checkpoint; normal quit now also uses the existing duplicate-capture and veto guards. Degraded startup retains its hydration gate so failed restore cannot overwrite saved sessions.

Checklist

  • Focused notification persistence change
  • User behavior, mechanism and choice explained
  • Windows before/after boundary screenshots attached
  • Reviewed security, cross-platform, host ownership, folder workspaces and wire compatibility
  • Full typecheck and PR changed-code quality pass on macOS and Windows; focused tests pass
  • Full repository lint/test/build and new-head CI

nwparker and others added 3 commits October 8, 2026 22:27
Await staged and published ACL checks through asynchronous process execution, including the first SID lookup. Serialize and coalesce dismissal snapshots, preserve permission failure reporting, and drain pending writes through the existing bounded quit barrier.

Co-authored-by: LDH1103 <ldh517525@gmail.com>

Co-authored-by: Loom <loom@local>
Use a checked errno guard for both durability lanes and keep the unsupported-code set private. Exercise concurrent publication, failed writer recovery, fsync ordering, and cleanup on Windows as well as POSIX.
Co-authored-by: LDH1103 <ldh517525@gmail.com>
Co-authored-by: Loom <loom@local>
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3440ee74-a581-46a5-adac-e783a5afcc8c
📥 Commits

Reviewing files that changed from the base of the PR and between 9b790b9 and 17cf9ab.

📒 Files selected for processing (1)
  • tests/e2e/structured-attention-read-retirement.unit.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The change adds asynchronous secure-file writers with per-path serialization and optional durable syncing. Windows ACL hardening now resolves the current user SID asynchronously. Mobile notification dismissal records persist asynchronously, expose flush methods through the runtime, and are flushed within the bounded application quit teardown. The relaunch path now uses the normal quit pipeline after relaunch cleanup.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 17cf9

The test now waits for persistence before deleting its temporary state, so a persistence failure is less likely to be hidden. No merge-blocking risk is identified in this scoped change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 26 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Directly linked issue #26027 requires removing synchronous Windows secure-write blocking from the notification dismissal path. MobileNotificationDismissalStore.record() now schedules asynchronous se…
Out of Scope Changes check Passed The async writer, SID lookup, dismissal persistence flush, quit-pipeline integration, relaunch routing, and fixture updates support issue #26027. They prevent notification-write blocking and preserve …
Title check Passed The title clearly and concisely describes the primary change: asynchronous mobile notification dismissal persistence that avoids blocking Windows.
Description check Passed The description covers the required sections, explains the user impact and implementation, links the issue, provides visual proof, documents testing and limitations, and includes the checklist. Full r…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • 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.

@nwparker
nwparker force-pushed the nwparker/win-issues-blocking-local-1008 branch from a22500b to 79c96ae Compare October 9, 2026 05:40
nwparker and others added 4 commits October 8, 2026 22:55
Await the existing shutdown flush in the shared host fixture and its mobile consumer, and replace restore-test polling with explicit write drains.

Co-authored-by: LDH1103 <ldh517525@gmail.com>
Co-authored-by: Loom <loom@local>
Reuse the existing bounded quit pipeline for renderer relaunch, retain pre-relaunch auth cleanup, and make relay recovery assertions await the asynchronous snapshot.

Co-authored-by: LDH1103 <ldh517525@gmail.com>
Co-authored-by: Loom <loom@local>
Await persistence before recovery reads and replacement-controller startup, and drain fixture hosts after their producers stop before removing test state. Retain original delivery fences and unseen-prompt assertions.

Co-authored-by: LDH1103 <ldh517525@gmail.com>
Co-authored-by: Loom <loom@local>
Avoid swallowing a missing-path persistence failure while the read-retirement retry test removes its independent relay directory.

Co-authored-by: LDH1103 <ldh517525@gmail.com>
Co-authored-by: Loom <loom@local>
@OrcaWin

OrcaWin commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Closing as superseded by current main (77130982faf44abe3e4fb02354e79b61cdad5358), after reviewing the complete production/test diff at 17cf9ab1ea7f0169aba8f7726322c8ce2c18a506 and the current notification, relaunch, quit, and Windows permission paths.

The core notification fix is already present: MobileNotificationDismissalStore uses the existing asynchronous, coalescing AtomicSnapshotWriter, preserves unreadable history and in-memory dismissal state on write failure, and exposes a persistence close/drain. It no longer sends each notification through the synchronous secure writer. Main's Windows ACL implementation also uses the native security addon; the whoami/icacls subprocess implementation this PR extends has been replaced.

The dismissal-loss boundary is covered too. Normal quit waits for both structured-session teardown and RPC stop before closing dismissal persistence. Relaunch explicitly awaits closeNotificationDismissalsBeforeRelaunch before app.exit, with a two-second bound; once close starts, later dismissal records write synchronously and supersede older queued snapshots. Current tests cover late records, coalescing, recovery, teardown ordering, and the relaunch bound. I ran the current dismissal-store, read-failure, and atomic-snapshot-writer suites: all 17 tests passed. Those source/test files match the main revision above; this is Linux verification, not a new Windows measurement.

This PR instead switches relaunch to the normal quit pipeline and introduces another general secure-writer/serialization lane. That is a different lifecycle choice, but its reported dismissal-preservation requirement is already handled. The original Windows before/after measurements do not establish a remaining gain over today's implementation. Under the performance bar in #27143, the extra writer and obsolete subprocess machinery have no demonstrated remaining benefit that justifies carrying this PR forward.

This closure is based on the implemented replacement, not merge conflicts. Preserving the branch and its Windows evidence for reference.

@OrcaWin OrcaWin closed this Oct 27, 2026
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.

[Bug]: Windows: UI freezes for seconds on every agent prompt — secure file writes run icacls synchronously on the main thread

2 participants