Skip to content

mcp(test): cancel and join click workers on unwind so a panic cannot leak the browser - #216

Merged
suchintan merged 1 commit into
mainfrom
repo-sync/rustwright-cloud-198
Aug 5, 2026
Merged

mcp(test): cancel and join click workers on unwind so a panic cannot leak the browser#216
suchintan merged 1 commit into
mainfrom
repo-sync/rustwright-cloud-198

Conversation

@suchintan

Copy link
Copy Markdown
Member

@suchintan suchintan added the sync Automated cross-repository sync label Aug 5, 2026
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Summary: Small, well-targeted test-only fix. Previously, if a panic (e.g. a failed assert_eq!) occurred between spawning the background click thread and the explicit click.join() call, the plain thread::JoinHandle would just be dropped -- silently detaching the thread so it kept running click_with_cancel against a page/browser that unwinding then tears down (BrowserInner::drop kills the Chromium process). WorkerGuard's Drop impl now cancels the token and joins the worker on unwind, closing that race. Drop-order reasoning (guard declared after browser, so it drops -- and therefore joins -- before browser's Drop fires) is correct per Rust's LIFO drop semantics, and the guard swallows join errors (let _ = worker.join()) so a worker-thread panic during unwind can't trigger a double-panic/abort. No production code (src/lib.rs, python/, node/) is touched -- this is confined to mcp/src/actor.rs test helpers, so risk is low.

Suggestions (1)
  • Two other real-browser background-thread patterns in the same file don't hold a live Browser/Page across the spawn (worker at ~L5572, ~L5950 use synthetic actor state, not a real browser), so they are not affected -- but it's worth double-checking there are no other thread::spawn sites elsewhere in mcp/ (outside actor.rs) that hold a live browser/page across a fallible assertion before .join(), since this same leak pattern could exist there too.
Minor / Style (1)
  • WorkerGuard construction and the declared-after-browser comment are duplicated verbatim across the two call sites. Fine for two uses, but if a third shows up, consider a small helper such as spawn_guarded(cancel, f) to avoid copy-pasted construction.

@suchintan
suchintan merged commit ba4bc96 into main Aug 5, 2026
12 of 16 checks passed
@suchintan
suchintan deleted the repo-sync/rustwright-cloud-198 branch August 5, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sync Automated cross-repository sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant