fix(macos): share notification dismissal polling - #388
Conversation
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
|
Native development acceptance update for These are human-observed results coordinated by the agent, not an independent agent-observed UI run. Accumulated-card CPU/scroll sampling, packaged identity, and account/access-fence acceptance remain unverified. PR remains draft; no broader readiness claim. Carl, an automated agent, commenting via Wes’s GitHub account. |
wesbillman
left a comment
There was a problem hiding this comment.
On Wes’s behalf — Brain: Initial source review at 9fe7acf6bd1b2e9ee21b320f398e643409295070 found no concrete blocking defect in the shared polling implementation. Compared the vendored files against the checksum-matching 0.6.15 crate, traced the actual native caller and response lifecycle, and inspected the native regression harness. No local execution; independent maintenance review remains pending. This is not an approval or native performance acceptance.
Required CI 36502979222, attempt 1 is not merely an unexplained cancellation. Chromium 1/3 job 109197899350 exceeded the 15-minute job limit after four recorded failures. The uploaded traces show all four are 15-second page.goto load timeouts, at messages.spec.mjs:44 (case at 78), messages.spec.mjs:596 (case at 589), app-style-order.spec.mjs:6, and avatar-edit.spec.mjs:15—before the later media/style/avatar assertions. The log contains 99 completed passing cases before cancellation. This evidence does not establish the timeout cause or justify raising timeouts/automatic reruns.
The tested checkout is merge 1267e3f184fb9ff8cf7daaa040ac24564aae9b51 (this head into 423f7215). JavaScript, Rust/tool integration, measurements and the other five functional shards passed; Linux CI does not compile/run the macOS Objective-C harness. Please retain the outstanding CI diagnosis, patched accumulated-card performance sampling, packaged identity and account/access acceptance gates. The existing human click/dismissal report is useful evidence but does not close those distinct gaps. No code edits, reruns, readiness changes or thread resolutions by this review.
|
|
||
| [patch.crates-io] | ||
| # Query Notification Center once per poll, not once per retained card. | ||
| mac-notification-sys = { path = "vendor/mac-notification-sys" } |
There was a problem hiding this comment.
On Wes’s behalf — Brain: P2 — guard the vendored polling fix against routine dependency updates.
Follow-up to my initial review at 9fe7acf6bd1b2e9ee21b320f398e643409295070: independent review identified a delivery gap that I corroborated. src-tauri/Cargo.toml requires mac-notification-sys = "=0.6.15", but renovate.json does not constrain its updates. If Renovate bumps that requirement to a newer release, the 0.6.15 path patch no longer matches: Cargo can use the registry crate and merely warn that this patch is unused. The reviewed one-query-per-tick fix would then no longer ship. Commit 115a5b7834777017d6e0828c51fd8987960f444b demonstrates that this repository’s non-major Renovate group already updates exact notification-crate requirements.
Please add a narrow Cargo packageRules entry for mac-notification-sys, using allowedVersions: "=0.6.15" or enabled: false, with a rationale requiring review of the vendored fix before upgrading. A bare 0.6.15 is not an exact Cargo constraint. This needs no polling redesign.
The current CI has no macOS lane, and the native harness directly includes the vendored source rather than proving which dependency production links, so those checks do not catch this future bypass. This supersedes my earlier no-blocking-finding conclusion; the polling implementation itself remains sound in source review. No local execution performed.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 I don't see any blockers at 9fe7acf6. Thanks for splitting the patch provenance out into BUZZ_PATCH.md, which made this easy to check.
I diffed the vendored tree against the crates.io 0.6.15 tarball. The checksum is fd604973…beca and the vcs sha1 is deb55968, both matching BUZZ_PATCH.md. The only behavioral delta is objc/notify.m. The other three .rs diffs are rustfmt import ordering plus one assert wrap. The shared timer lifecycle looks right to me. Add and remove are serialized on main, and the main-thread path pumps the run loop instead of doing a dispatch_sync to itself. The timer is only torn down when the live dictionary is empty, and releasing it inside its own callout is safe because CFRunLoop retains it across the fire. Under MRC, the blocks retain waitIdentifier before the sender releases it. First-terminal-wins still holds through the is_done check in resolveAutoDismiss and the Rust result mutex. notifications.rs is byte-identical, so the 128 cap and the one-worker-per-wait behavior don't change.
On macOS (headless, no real notifications) the native harness passes. buzz-foundation builds with --locked, and cargo tree -i confirms the vendor path is what gets linked. The vendored crate's own suite passes (17 unit, 3 integration, 14 doctests). Restoring per-card deliveredNotifications queries fails the harness at notification_polling.m:96, and dropping stop-on-empty fails it at :106.
A few non-blocking things:
- I'd take the Renovate guard from the inline thread on
Cargo.toml. 0.6.15 is still the latest release, so nothing is pending right now. But if a bump to=0.6.16lands, the path patch silently stops applying (the only signal is a[[patch.unused]]entry inCargo.lock).notification_polling.m#imports the vendored file directly, so the harness would stay green while the registry crate ships. Arenovate.jsonrule like the existingvirtuaone closes that. - The fake
finish()innotification_polling.m:11-16dedupes before the assertions see anything, so a mutation that firesrust_notification_auto_dismissed()twice still passes. That isn't a user-visible bug, since Rust is first-wins anyway. Still, counting callback attempts before the dedup would let the harness actually back the at-most-one claim. The harness also drivesaddDismissalWait/pollDismissalsdirectly, so the backgroundsendNotificationenqueue/cleanup path isn't covered. host_command::tests::passes_exact_args_without_shell_interpretation(host_command.rs:318) failed 1 of 3 full-suite runs on head and 0 of 3 on base. That file is unchanged here, and the same test flaked in parallel on #308, so I'm not attributing it to this PR.rejects_large_output_without_waiting_for_the_processpassed 3/3 on both.- The standalone command in
BUZZ_PATCH.mdfails from a nested.worktrees/checkout, because Cargo walks up to the parent checkout's workspace, which doesn't exclude that path. It works from a normal clone.
The red CI required is the chromium 1/3 shard hitting the 15m limit after four page.goto timeouts. This PR has no web changes, and Linux CI never compiles notify.m, so I don't think it's from this diff. I didn't cover real XPC ordering, the ~100-card CPU sampling, or packaged identity. Those are still your acceptance gates.
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
|
Merged main The previous failure was Chromium shard 1/3 exceeding the 15-minute job limit, not a failing notification assertion. This brings in #370’s four-way browser sharding/test changes and #389’s cold-runner Rust provisioning improvement. The PR patch is unchanged relative to its new base (identical stable patch ID). Commit/push hooks passed, the worktree is clean, and locked macOS dependency resolution still selects the vendored Carl, an automated agent, commenting via Wes’s GitHub account. |
Summary
Fix progressive macOS UI stalls caused by notification cards accumulating in
Notification Center. The pinned dependency previously installed one main-thread
0.5-second timer per waiting notification, each synchronously querying the full
OS delivered-notification list.
timer and one delivered-list query per tick. Stop polling when waits empty.
first-terminal-response semantics, and the existing 128-active-notification cap.
OS/response boundaries. No frontend, Linux or Windows behavior changes.
The apparent diff size is mostly the dependency snapshot and licenses. Production
changes are confined to its polling implementation and Cargo wiring; provenance,
format-only upstream deviations and removal criteria are in
vendor/mac-notification-sys/BUZZ_PATCH.md. No upstream fix submission is claimed.Diagnostic evidence
Before intervention, about 100 notification waits remained active and two native
samples spent ~83–84% of main-thread samples in notification polling/IPC. Manually
dismissing the cards without restarting reduced native CPU from ~44–50% to 4.7%,
left ~95% of main-thread samples in the ordinary idle run loop, and reduced waits
to one. The reporter confirmed scrolling recovered. This establishes the original
bottleneck; it is not a benchmark of the patch or a renderer-memory diagnosis.
Validation
Checked head:
9fe7acf6bd1b2e9ee21b320f398e643409295070.per tick, no delivered-card expiry, removal, completion, delivery grace,
callback ordering and stop/restart. A mutation restoring per-card queries fails
its query-count assertion; the candidate passes.
before mandatory import/assertion formatting (no behavior edits afterward).
buzz-foundationsuite passed during development: 131 unit tests plusthe native harness, four existing ignored tests. A final-head rerun hit the
unchanged
host_command::tests::rejects_large_output_without_waiting_for_the_processone-second wall-clock assertion (130 passed, one failed, four ignored).
This timing failure remains unresolved; it is not hidden by a retry or timeout edit.
Linux CI cannot establish Objective-C compilation or native click acceptance.
Draft acceptance gates
minimized app, two distinct targets, immediate click, banner fade followed by
Notification Center click, dismissal without navigation, account/access fences.
verify cost no longer scales as one OS query per card per tick.
The existing running app was not modified or restarted by this work. A native
restart onto this branch is required. Human testing is being coordinated separately.
One synchronous OS query per 0.5-second tick and one blocked worker per pending
notification remain; the existing cap/rejection policy is deliberately unchanged.
Carl, an automated agent, authored this change and opened the PR via Wes’s GitHub account.