diff --git a/CHANGELOG.md b/CHANGELOG.md index 59285eb..ac91e79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ # Changelog ## Unreleased +- Fixed a task tab with a "Leave site?" prompt stalling the extension. Chrome's `tabs.remove` waits until the tab is gone, and a page's beforeunload prompt, which a background tab shows only once activated, can hold the tab open indefinitely. `browser_finish` and task close waited on that removal inside the ownership queue, so every later ownership change, `browser_open`, `browser_tabs`, tab event, and native reconnect stalled behind it until the extension was reloaded. Task cleanup still removes ownership before closing tabs, now waits at most two seconds for Chrome to close them, and leaves a tab still waiting on its prompt out of both `closed_tab_ids` and `retained_tab_ids`; the tab closes if the user confirms leaving. A `browser_act` `close` waits the same two seconds and then returns `close_pending: true` instead of holding its request open. +- No single extension operation can stall the operations queued behind it any more. An ownership change or attention-notice change still running after 10 seconds, or a background tab or alarm event after 30 seconds, keeps running and is logged, but the next operation starts. Ten seconds covers the two-second tab removal wait with room for normal Chrome calls and stays well under the host's 30-second command deadline, so a command queued behind a stuck operation can still answer. +- `browser_finish` and task close now report an extension that does not answer in time as `extension_timeout`, telling the agent that the task is still open and to reload the extension if it stays unresponsive, then finish or close again. They previously reported `state_store_error` and suggested repairing `~/.agenttab` or freeing disk space. When the extension refuses cleanup, such as for a `keep_tab_ids` entry the task does not own, the error now carries the extension's own code and message instead of `native_protocol_error`. Disconnects and protocol or transport failures during cleanup keep their own error codes too. - `scripts/reload_unpacked_extension.sh` now reaches the Chrome instance and profile that have the extension. It previously handed the wake page to whichever same-named Chrome instance macOS picked, so a second Chrome (such as a headless test browser) could receive it, nothing reloaded, and `dev:deploy` still reported success. The script now finds the profiles whose preferences list the extension in the application's default user data directory, opens the wake page in each through Chrome's own instance handoff, and exits with an error when no profile has the extension. Use `--user-data-dir` or `--profile-directory` (or `AGENTTAB_CHROME_USER_DATA_DIR` and `AGENTTAB_CHROME_PROFILE_DIRECTORY`) to override the detection. - `context_click` no longer opens Chrome's native context menu. On macOS that menu stalled every AgentTab request until someone dismissed it. Page `contextmenu` handlers still run, though they see the event as already handled. The suppression applies only to that one right click and expires on its own after five seconds. - Snapshot refs now survive a service worker restart or extension reload. The extension stores each snapshot's refs before returning it, drops them when their tab closes or navigates to a newer page revision, and clears them when Chrome starts. A ref still resolves only while its page document is the same one the snapshot saw. A ref the extension never stored fails with `stale_ref`, saying it is unknown or expired and asking for a new snapshot. diff --git a/docs/mcp.md b/docs/mcp.md index e5edce1..8772cc7 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -64,14 +64,14 @@ For MCP, the capability store namespace is `mcp`; OMP uses `omp`; Pi uses `pi`. |---|---| | `browser_open` | `mode: "create"` optionally accepts an `http`, `https`, or `about` URL, `background`, and `placement`. The default `placement: "task"` creates a tab in the task's existing window when possible. `placement: "new_window"` creates the first tab of an otherwise empty task in a separate unfocused normal window and rejects `background: false`. `mode: "adopt_active"` explicitly adopts only the currently active tab. The result includes task, tab, window, page-revision, and `automation_route` identifiers. It returns after the tab's first load completes, up to 10 seconds, so its `page_revision` and `url` describe the loaded page and can be passed straight to `browser_act`. | | `browser_snapshot` | Requires `tab_id`. Modes are `accessibility`, `text`, `html`, and `screenshot`; each may scope to an opaque CDP `frame_id`. Only accessibility snapshots return revisioned node references, each bound to its originating frame; full-tree accessibility snapshots also append `dom_fallback` textbox nodes for editable elements that never entered Chrome's accessibility tree, so those refs still address the existing typed actions. Screenshot requests may select `png`, `jpeg`, or `webp`, set JPEG/WebP `quality`, bound dimensions with `max_width`/`max_height`, and cap compressed image size with `max_bytes`. Snapshots require the `full` automation route. | -| `browser_act` | Requires `tab_id`, `expected_page_revision`, and one to 64 typed actions. Actions are click, hover, double-click, context-click, press, type, fill, select, scroll, drag, navigate, history movement, reload, close, dialog decision, and staged file upload. Element-target actions accept exactly one `ref` or `selector` and optional `frame_id`; `press` accepts the Standard navigation/editing keys plus optional unique `Alt`, `Control`, `Meta`, and `Shift` modifiers. Lowercase `a`, `z`, and `y` require exactly one of Control or Meta, optionally Shift, never Alt. No coordinate action, generic key injection, clipboard shortcut, or cross-frame target is exposed in Standard mode. When a step fails, the error `details` carry `failed_action_index` (zero-based) and `completed_actions`, the results of the steps that ran first; in a batch of more than one action, the message also names the failing step. A failure after at least one completed step has `outcome: "unknown"`, because those steps already changed the page. An optional `intent`, one line of 1 to 120 characters, replaces the agent pointer's generated caption for every action in the batch, for example "Open the bookmarks folder". It is visible on screen, so it must never carry secrets or field values. | +| `browser_act` | Requires `tab_id`, `expected_page_revision`, and one to 64 typed actions. Actions are click, hover, double-click, context-click, press, type, fill, select, scroll, drag, navigate, history movement, reload, close, dialog decision, and staged file upload. Element-target actions accept exactly one `ref` or `selector` and optional `frame_id`; `press` accepts the Standard navigation/editing keys plus optional unique `Alt`, `Control`, `Meta`, and `Shift` modifiers. Lowercase `a`, `z`, and `y` require exactly one of Control or Meta, optionally Shift, never Alt. No coordinate action, generic key injection, clipboard shortcut, or cross-frame target is exposed in Standard mode. When a step fails, the error `details` carry `failed_action_index` (zero-based) and `completed_actions`, the results of the steps that ran first; in a batch of more than one action, the message also names the failing step. A failure after at least one completed step has `outcome: "unknown"`, because those steps already changed the page. A `close` releases the tab from the task and returns `completed: true` once Chrome closes it, or `started: true, close_pending: true` when the tab is still open two seconds later, as when a "Leave site?" prompt holds it; the tab then closes if the user confirms leaving. An optional `intent`, one line of 1 to 120 characters, replaces the agent pointer's generated caption for every action in the batch, for example "Open the bookmarks folder". It is visible on screen, so it must never carry secrets or field values. | | `browser_wait` | Requires `tab_id` and one load, URL, text, selector, value, network-idle, download, or `challenge_cleared` condition. Selector waits may set `attached`, `detached`, `visible`, `hidden`, or `enabled` state and scope to a `frame_id`; value waits require selector and expected value and may scope to a frame. A download wait may resume after only its opaque tab-owned `after` cursor. A `challenge_cleared` wait takes the `challenge_id` of an active challenge episode on the tab and completes with `matched: true` once the episode clears or is superseded; if the episode reports it no longer expects automatic clearing, the wait returns early with outcome `needs_user` and result `status: "challenge_needs_user"`; an unknown, ended, or other-task episode fails with `challenge_not_found` and outcome `not_started`, and a timeout carries the latest event in `error.details.challenge`. `timeout_ms` is at most 120 seconds. Load and URL waits also follow Chrome's top-frame navigation events, with polling as a fallback: a load condition matches a top-frame load that completes after the wait starts, and a URL condition also matches committed navigations, same-document history changes (`pushState` and `replaceState`), and fragment changes seen during the wait. A top-frame navigation error during a load or URL wait fails with `navigation_failed`, `outcome: "unknown"`, and Chrome's network error name, never the URL; a superseded or cancelled navigation does not fail the wait. A `tab_only` route accepts load and URL conditions only; frame-scoped reads, value/selector state, network-idle, and download attribution, and challenge waits require the tab-scoped debugger connection available on the `full` route. | | `browser_tabs` | Takes an empty object and lists only the current task's tabs, including each tab's `automation_route`. | | `browser_handoff` | Takes one of four operations. `request` requires a task tab, expected page revision, and prompt; optional `completion: {kind: "url"\|"selector"\|"challenge_cleared", value}` and `timeout_ms` (default five minutes, reminder expiry only). It creates a `notice_id` scoped to the task and tab and returns immediately without pausing, focusing, or blocking any agent work. `status`, `resolve`, and `dismiss` take that `notice_id`. `status` also reports `user_idle_state` as `active`, `idle`, or `locked` from Chrome's idle detection, where `idle` means no keyboard or mouse input for 60 seconds; the field is omitted when Chrome cannot report it. The agent tells the user in chat, verifies the page itself, then resolves or dismisses; a `resolve` against an unmet condition fails with `completion_not_met` and leaves the notice open. Works on a `tab_only` route, but selector and `challenge_cleared` completion require the `full` route. A `challenge_cleared` completion takes the `challenge_id` of an active challenge episode on the request's tab; an unknown, ended, or other-task episode, or one on another tab, fails with `challenge_not_found` and outcome `not_started` and creates no notice. The notice then binds to the episode, so each later challenge event carries its `notice_id`. `resolve` rescans the tab first and succeeds only when the episode has cleared or been superseded, returning the terminal event as `challenge`; otherwise it fails with `completion_not_met`, `error.details.challenge` holds the current active event, and the notice stays open. | | `browser_commit` | Requires the staged token returned by a prior `commit_required` action and executes that one staged operation. On a `tab_only` route, only a staged close can execute; page-dependent staged actions require the `full` route. A `browser_profile` bookmark removal stage is bound to the task rather than a tab. | | `browser_credentials` | `prepare` requires a task tab and expected page revision, then returns an opaque short-lived token when the default-enabled 1Password broker is available and one through three Login items match the host-derived current origin. Owner-only policy can disable it. `fill` consumes that token and selected username, password, or one-time-code field refs without returning any value. `next` advances to another bounded candidate. It never submits the form. | | `browser_profile` | Requires one `operation` over the Chrome profile rather than a task tab: bookmark search, tree, update, create, move, and remove; history search; recently closed and synced sessions, plus restore; download search, cancel, pause, and resume. Read operations take no idempotency key and write operations require one. `bookmarks_remove` always returns `commit_required`. Never focuses Chrome. See [Profile operations](#profile-operations). | -| `browser_finish` | Accepts `disposition: "auto" | "close" | "keep"` and optional task-owned `keep_tab_ids`. Automatic mode follows the popup cleanup policy: close task-created tabs while retaining adopted tabs, ask for confirmation, or retain all tabs. Successful finalization ungroups retained tabs, releases ownership, closes the Core connection, and returns closed and retained tab IDs. An open handoff notice never defers finalization; staged Commit review and other in-flight work defer it without destroying resumability. | +| `browser_finish` | Accepts `disposition: "auto" | "close" | "keep"` and optional task-owned `keep_tab_ids`. Automatic mode follows the popup cleanup policy: close task-created tabs while retaining adopted tabs, ask for confirmation, or retain all tabs. Successful finalization ungroups retained tabs, releases ownership, closes the Core connection, and returns closed and retained tab IDs. A tab Chrome has not closed two seconds after removal, as when a "Leave site?" prompt holds it, appears in neither list and closes if the user confirms leaving. An open handoff notice never defers finalization; staged Commit review and other in-flight work defer it without destroying resumability. When the extension refuses to finish, such as for a `keep_tab_ids` entry the task does not own, the error carries the extension's own code and message, and the task stays open. | For `click`, `hover`, `double_click`, `context_click`, `press`, `type`, `fill`, and `upload_file`, supply exactly one of `ref` or `selector`; `frame_id` scopes selector resolution when supplied. A CSS selector is limited to 1–2048 characters and must match exactly one element; missing and ambiguous targets fail before mutation. The same task ownership, revision, sensitive-field, and Commit checks apply to either target form. Before Commit, the selector is resolved again and checked against the staged element identity. Execute against that checked element, never a later selector match. Upload selectors now reject duplicate matches, including duplicates introduced between staging and Commit. A dialog action can provide `prompt_text` only when accepting a JavaScript prompt; the exact payload is included in review binding. Only click, press, double-click, context-click, navigation, history, and reload actions may set `expect_download: true`, which is also review-visible and fingerprint-bound. A successful opted-in action may return an opaque `download_cursor`; pass it only as `browser_wait` download `after`, never to another task or tab. AgentTab reports completion metadata, not a local download path. Gesture dispatch, site acceptance, and external side effects are not proven by accepting or staging a request. diff --git a/host-rs/crates/agenttab-host/src/native.rs b/host-rs/crates/agenttab-host/src/native.rs index 714c18f..ec7d8cb 100644 --- a/host-rs/crates/agenttab-host/src/native.rs +++ b/host-rs/crates/agenttab-host/src/native.rs @@ -30,6 +30,9 @@ pub enum NativeError { Protocol(String), #[error("native transport error: {0}")] Transport(String), + /// The extension answered a lifecycle command with its own failure, such as a refused finish. + #[error("extension error {}: {}", .0.code, .0.message)] + Extension(RpcError), } #[derive(Debug, Clone)] @@ -382,20 +385,23 @@ impl NativeTransport for StdioNative { } Err(mpsc::RecvTimeoutError::Disconnected) => return Err(NativeError::Disconnected), }; + if response.outcome != Outcome::Completed { + return Err(lifecycle_failure( + response, + "native close_task cleanup was not confirmed by the extension", + )); + } let result = response.result.as_ref().and_then(Value::as_object); let expected_task_id = task_id.to_string(); let closed_tab_ids_are_valid = result .and_then(|value| value.get("closed_tab_ids")) .and_then(Value::as_array) .is_some_and(|tab_ids| tab_ids.iter().all(Value::is_u64)); - if response.outcome != Outcome::Completed - || result.is_none_or(|value| { - value.len() != 2 - || value.get("task_id").and_then(Value::as_str) - != Some(expected_task_id.as_str()) - || !closed_tab_ids_are_valid - }) - { + if result.is_none_or(|value| { + value.len() != 2 + || value.get("task_id").and_then(Value::as_str) != Some(expected_task_id.as_str()) + || !closed_tab_ids_are_valid + }) { return Err(NativeError::Protocol( "native close_task cleanup was not confirmed by the extension".into(), )); @@ -436,8 +442,9 @@ impl NativeTransport for StdioNative { Err(mpsc::RecvTimeoutError::Disconnected) => return Err(NativeError::Disconnected), }; if response.outcome != Outcome::Completed { - return Err(NativeError::Protocol( - "native finish_task cleanup was not confirmed by the extension".into(), + return Err(lifecycle_failure( + response, + "native finish_task cleanup was not confirmed by the extension", )); } let result: NativeTaskFinish = @@ -480,6 +487,15 @@ impl NativeTransport for StdioNative { } } +/// Keeps the extension's own error for a failed lifecycle command, so the caller sees why the +/// extension refused, and reports a protocol error only when the failure carries none. +fn lifecycle_failure(response: NativeResponse, unconfirmed: &str) -> NativeError { + response.error.map_or_else( + || NativeError::Protocol(unconfirmed.into()), + NativeError::Extension, + ) +} + #[cfg(test)] mod tests { use super::*; @@ -627,6 +643,68 @@ mod tests { assert_eq!(acknowledgement["outcome"], "completed"); } + #[test] + fn finish_refusal_keeps_the_extension_error() { + let lifecycle = Arc::new(Lifecycle::default()); + lifecycle.begin_reconciliation(); + lifecycle.complete_reconciliation(false); + let output = SharedWriter::default(); + let native = StdioNative::new(output.clone(), lifecycle); + native.disconnected.store(false, Ordering::Release); + let task_id = Uuid::now_v7(); + let finishing = { + let native = native.clone(); + std::thread::spawn(move || { + native.finish_task( + task_id, + FinishDisposition::Auto, + &[99], + Duration::from_secs(5), + ) + }) + }; + + let deadline = std::time::Instant::now() + Duration::from_secs(1); + let command = loop { + let bytes = output.bytes.lock().clone(); + if let Ok(Some(command)) = + read_frame(&mut bytes.as_slice(), HOST_TO_EXTENSION_MAX_BYTES) + { + break command; + } + assert!( + std::time::Instant::now() < deadline, + "finish_task was never sent to the extension" + ); + std::thread::sleep(Duration::from_millis(5)); + }; + assert_eq!(command["kind"], "finish_task"); + native + .handle_inbound(json!({ + "protocol": NATIVE_PROTOCOL, + "version": PROTOCOL_VERSION, + "kind": "response", + "request_id": command["request_id"], + "outcome": "unknown", + "error": { + "code": "ownership_denied", + "message": format!("Tab 99 is not owned by task {task_id}"), + }, + })) + .unwrap(); + + match finishing.join().unwrap() { + Err(NativeError::Extension(refusal)) => { + assert_eq!(refusal.code, "ownership_denied"); + assert_eq!( + refusal.message, + format!("Tab 99 is not owned by task {task_id}") + ); + } + other => panic!("expected the extension's refusal, got {other:?}"), + } + } + #[test] fn version_mismatch_is_terminal() { let lifecycle = Arc::new(Lifecycle::default()); diff --git a/host-rs/crates/agenttab-host/src/runtime.rs b/host-rs/crates/agenttab-host/src/runtime.rs index d71364d..77a3a65 100644 --- a/host-rs/crates/agenttab-host/src/runtime.rs +++ b/host-rs/crates/agenttab-host/src/runtime.rs @@ -502,37 +502,41 @@ impl Runtime { disposition, keep_tab_ids, }), - ) => self - .native - .finish_task(task_id, disposition, &keep_tab_ids, CLOSE_TASK_TIMEOUT) - .map_err(|error| JournalError::NativeTaskCleanup(error.to_string())) - .and_then(|result| { - if result.finished { - self.journal.close_task(task_id)?; - connection.cancel(); - self.native.cancel_connection(connection.connection_id); + ) => match self.native.finish_task( + task_id, + disposition, + &keep_tab_ids, + CLOSE_TASK_TIMEOUT, + ) { + Err(error) => native_cleanup_failure(&request.request_id, error), + Ok(result) => { + let finalized = if result.finished { + self.journal.close_task(task_id).map(|()| { + connection.cancel(); + self.native.cancel_connection(connection.connection_id); + }) + } else { + Ok(()) + }; + match finalized { + Ok(()) => RpcResponse::success( + request.request_id.clone(), + if result.finished { + Outcome::Completed + } else { + Outcome::NeedsUser + }, + serde_json::to_value(result) + .expect("native task finish result serializes"), + ), + Err(error) => RpcResponse::failure( + request.request_id.clone(), + Outcome::Unknown, + journal_rpc_error(error), + ), } - Ok(result) - }) - .map(|result| { - RpcResponse::success( - request.request_id.clone(), - if result.finished { - Outcome::Completed - } else { - Outcome::NeedsUser - }, - serde_json::to_value(result) - .expect("native task finish result serializes"), - ) - }) - .unwrap_or_else(|error| { - RpcResponse::failure( - request.request_id.clone(), - Outcome::Unknown, - journal_rpc_error(error), - ) - }), + } + }, (None, MethodParams::Finish(_)) => RpcResponse::success( request.request_id.clone(), Outcome::Completed, @@ -559,27 +563,25 @@ impl Runtime { if request.method == RpcMethod::AgenttabClose { let task_id = connection.task_id().ok().flatten(); let response = match task_id { - Some(task_id) => self - .native - .close_task(task_id, CLOSE_TASK_TIMEOUT) - .map_err(|error| JournalError::NativeTaskCleanup(error.to_string())) - .and_then(|()| self.journal.close_task(task_id)) - .map(|()| { - connection.cancel(); - self.native.cancel_connection(connection.connection_id); - RpcResponse::success( - request.request_id.clone(), - Outcome::Completed, - json!({ "closed": true }), - ) - }) - .unwrap_or_else(|error| { - RpcResponse::failure( + Some(task_id) => match self.native.close_task(task_id, CLOSE_TASK_TIMEOUT) { + Err(error) => native_cleanup_failure(&request.request_id, error), + Ok(()) => match self.journal.close_task(task_id) { + Ok(()) => { + connection.cancel(); + self.native.cancel_connection(connection.connection_id); + RpcResponse::success( + request.request_id.clone(), + Outcome::Completed, + json!({ "closed": true }), + ) + } + Err(error) => RpcResponse::failure( request.request_id.clone(), Outcome::Unknown, journal_rpc_error(error), - ) - }), + ), + }, + }, None => RpcResponse::success( request.request_id.clone(), Outcome::Completed, @@ -1825,6 +1827,9 @@ fn native_event_result(response: RpcResponse) -> NativeEventResult { fn native_failure(request_id: &str, error: NativeError) -> RpcResponse { let (code, recovery) = match error { + NativeError::Extension(refusal) => { + return RpcResponse::failure(request_id, Outcome::Unknown, refusal); + } NativeError::Disconnected => ( "extension_disconnected", "Wait for Chrome to reconnect AgentTab, inspect task state, then retry safely.", @@ -1849,6 +1854,41 @@ fn native_failure(request_id: &str, error: NativeError) -> RpcResponse { RpcError::new(code, error.to_string()).with_recovery(recovery), ) } + +/// Maps a failed native `finish_task` or `close_task` command. The journal still lists the +/// task as open, so the recovery is to finish or close it again once the extension answers. +fn native_cleanup_failure(request_id: &str, error: NativeError) -> RpcResponse { + let (code, recovery) = match error { + // The extension refused the cleanup itself; its code says why, so keep it. + NativeError::Extension(mut refusal) => { + refusal.recovery.get_or_insert_with(|| { + "The task is still open. Resolve the extension's error, then finish or close the task again.".into() + }); + return RpcResponse::failure(request_id, Outcome::Unknown, refusal); + } + NativeError::Disconnected => ( + "extension_disconnected", + "The task is still open. Wait for Chrome to reconnect AgentTab, then finish or close the task again.", + ), + NativeError::Timeout => ( + "extension_timeout", + "The task is still open. If the AgentTab extension stays unresponsive, reload it, then finish or close the task again.", + ), + NativeError::Protocol(_) => ( + "native_protocol_error", + "The task is still open. Check the finish arguments, update AgentTab if the host and extension versions differ, then finish or close the task again.", + ), + NativeError::Transport(_) => ( + "native_transport_error", + "The task is still open. Repair the AgentTab native host connection, then finish or close the task again.", + ), + }; + RpcResponse::failure( + request_id, + Outcome::Unknown, + RpcError::new(code, error.to_string()).with_recovery(recovery), + ) +} #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub(crate) enum RequestLockScope { Global, @@ -1982,6 +2022,7 @@ mod tests { last_params: Mutex>, last_act_params: Mutex>, closed_tasks: Mutex>, + cleanup_timeouts: AtomicUsize, } impl FakeNative { @@ -1998,6 +2039,7 @@ mod tests { last_params: Mutex::new(None), last_act_params: Mutex::new(None), closed_tasks: Mutex::new(Vec::new()), + cleanup_timeouts: AtomicUsize::new(0), }) } @@ -2013,6 +2055,7 @@ mod tests { origin_policies: Mutex::new(Vec::new()), last_params: Mutex::new(None), closed_tasks: Mutex::new(Vec::new()), + cleanup_timeouts: AtomicUsize::new(0), last_act_params: Mutex::new(None), }) } @@ -2030,6 +2073,7 @@ mod tests { last_params: Mutex::new(None), last_act_params: Mutex::new(None), closed_tasks: Mutex::new(Vec::new()), + cleanup_timeouts: AtomicUsize::new(0), }) } @@ -2046,8 +2090,18 @@ mod tests { last_params: Mutex::new(None), last_act_params: Mutex::new(None), closed_tasks: Mutex::new(Vec::new()), + cleanup_timeouts: AtomicUsize::new(0), }) } + + /// Consumes one queued cleanup timeout, standing in for an extension that never answers. + fn cleanup_times_out(&self) -> bool { + self.cleanup_timeouts + .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |remaining| { + remaining.checked_sub(1) + }) + .is_ok() + } } impl NativeTransport for FakeNative { fn dispatch( @@ -2143,6 +2197,9 @@ mod tests { }) } fn close_task(&self, task_id: Uuid, _timeout: Duration) -> Result<(), NativeError> { + if self.cleanup_times_out() { + return Err(NativeError::Timeout); + } self.closed_tasks.lock().push(task_id); if self.close_task_error { return Err(NativeError::Disconnected); @@ -2156,6 +2213,9 @@ mod tests { keep_tab_ids: &[u64], _timeout: Duration, ) -> Result { + if self.cleanup_times_out() { + return Err(NativeError::Timeout); + } self.closed_tasks.lock().push(task_id); Ok(NativeTaskFinish { task_id, @@ -2507,6 +2567,122 @@ mod tests { assert!(runtime.journal.resume_task(&capability).unwrap().is_none()); } + #[test] + fn task_cleanup_timeout_reports_the_extension_and_keeps_the_task_open() { + let native = FakeNative::normal(); + let (_temp, runtime, connection) = connected_runtime(native.clone()); + let opened = runtime.handle( + &connection, + json!({ + "protocol": RPC_PROTOCOL, + "version": PROTOCOL_VERSION, + "request_id": "tabs", + "method": "browser_tabs", + "params": {} + }), + ); + let capability = opened["task"]["resume_capability"] + .as_str() + .unwrap() + .to_string(); + let task_id = opened["task"]["task_id"] + .as_str() + .unwrap() + .parse::() + .unwrap(); + confirm_new_capability(&runtime, &connection, capability.clone()); + native.cleanup_timeouts.store(2, Ordering::SeqCst); + + for (request_id, method, params) in [ + ("close-timeout", "agenttab.close", json!({})), + ( + "finish-timeout", + "agenttab.finish", + json!({ "disposition": "auto" }), + ), + ] { + let failed = runtime.handle( + &connection, + json!({ + "protocol": RPC_PROTOCOL, + "version": PROTOCOL_VERSION, + "request_id": request_id, + "method": method, + "params": params + }), + ); + + assert_eq!(failed["outcome"], "unknown", "{method}"); + assert_eq!(failed["error"]["code"], "extension_timeout", "{method}"); + let recovery = failed["error"]["recovery"].as_str().unwrap(); + assert!(recovery.contains("reload it"), "{method}: {recovery}"); + assert!(!recovery.contains("disk space"), "{method}: {recovery}"); + assert!(!connection.is_cancelled(), "{method}"); + } + let still_open = runtime.handle( + &connection, + json!({ + "protocol": RPC_PROTOCOL, + "version": PROTOCOL_VERSION, + "request_id": "tabs-after-timeouts", + "method": "browser_tabs", + "params": {} + }), + ); + assert_eq!(still_open["outcome"], "completed"); + assert!(native.closed_tasks.lock().is_empty()); + + let finished = runtime.handle( + &connection, + json!({ + "protocol": RPC_PROTOCOL, + "version": PROTOCOL_VERSION, + "request_id": "finish-retry", + "method": "agenttab.finish", + "params": { "disposition": "auto" } + }), + ); + + assert_eq!(finished["outcome"], "completed"); + assert_eq!(finished["result"]["finished"], true); + assert_eq!(*native.closed_tasks.lock(), vec![task_id]); + assert!(connection.is_cancelled()); + assert!(runtime.journal.resume_task(&capability).unwrap().is_none()); + } + + #[test] + fn task_cleanup_refusal_reports_the_extension_error() { + let refused = native_cleanup_failure( + "finish-refused", + NativeError::Extension(RpcError::new( + "ownership_denied", + "Tab 99 is not owned by task 018f47b8-2f80-7c20-9c77-f8a38c9e621e", + )), + ); + let refused = serde_json::to_value(refused).unwrap(); + + assert_eq!(refused["outcome"], "unknown"); + assert_eq!(refused["error"]["code"], "ownership_denied"); + assert_eq!( + refused["error"]["message"], + "Tab 99 is not owned by task 018f47b8-2f80-7c20-9c77-f8a38c9e621e" + ); + let recovery = refused["error"]["recovery"].as_str().unwrap(); + assert!(recovery.contains("still open"), "{recovery}"); + + let own_recovery = native_cleanup_failure( + "finish-refused", + NativeError::Extension( + RpcError::new("task_window_conflict", "conflict") + .with_recovery("Use placement task."), + ), + ); + assert_eq!( + own_recovery.error.unwrap().recovery.as_deref(), + Some("Use placement task.") + ); + } + fn confirm_new_capability( runtime: &Runtime, connection: &ConnectionContext, diff --git a/packages/extension/src/background.ts b/packages/extension/src/background.ts index e32c6a9..30e3661 100644 --- a/packages/extension/src/background.ts +++ b/packages/extension/src/background.ts @@ -1,6 +1,6 @@ import type { ChallengeEvent } from "./challenge"; import { clearChallengeTask } from "./challenge-store"; -import { removeTabIfPresent, StandardBrowserRuntime } from "./browser"; +import { StandardBrowserRuntime } from "./browser"; import { HandoffController, HANDOFF_ALARM } from "./handoff"; import { NativeBridge, RECONNECT_ALARM } from "./native"; import { OwnershipLedger } from "./ownership"; @@ -121,7 +121,6 @@ browser = new StandardBrowserRuntime( async (tabId) => { await ownership.revoke(tabId, "tab_removed"); await browser.detach(tabId); - await removeTabIfPresent(tabId); }, emit, authorizeOwnedTab, diff --git a/packages/extension/src/bounded.ts b/packages/extension/src/bounded.ts new file mode 100644 index 0000000..288c239 --- /dev/null +++ b/packages/extension/src/bounded.ts @@ -0,0 +1,52 @@ +/** + * Waits for `work` for at most `limitMs`. Resolves `true` when it fulfills in time and rejects + * with its error when it fails in time. Resolves `false` once the limit passes first; `work` + * keeps running, and its later outcome is left to whoever else holds it. + */ +export async function settledWithin(work: unknown, limitMs: number): Promise { + const { promise: expired, resolve: expire } = Promise.withResolvers(); + const timer = setTimeout(() => expire(false), limitMs); + try { + return await Promise.race([Promise.resolve(work).then(() => true as const), expired]); + } finally { + clearTimeout(timer); + } +} + +/** + * A serial operation chain whose operations hold it for at most `holdMs` each. An operation that + * overruns is reported and keeps running, and its caller still receives its eventual result, but + * the next operation starts. Without the cap, one operation awaiting something that never settles + * would stall every later operation for the life of the service worker. + */ +export class BoundedChain { + private tail: Promise = Promise.resolve(); + + constructor( + private readonly name: string, + private readonly holdMs: number, + private readonly onOverrun: (error: Error) => void = (error) => console.warn(error), + ) { } + + run(operation: () => T | Promise): Promise { + const previous = this.tail; + const running = previous.then(operation); + // Time the hold from when the operation starts, not when it is queued, so operations that + // waited behind a stuck one still run one at a time. + this.tail = previous + .then(() => settledWithin(running, this.holdMs)) + .then( + (settled) => { + if (!settled) this.onOverrun(this.overrun()); + }, + () => undefined, + ); + return running; + } + + private overrun(): Error { + return new Error( + `AgentTab ${this.name} operation still running after ${this.holdMs} ms; later operations no longer wait for it`, + ); + } +} diff --git a/packages/extension/src/browser.ts b/packages/extension/src/browser.ts index 98bb714..1ce949d 100644 --- a/packages/extension/src/browser.ts +++ b/packages/extension/src/browser.ts @@ -54,6 +54,7 @@ import { challengeStoreAvailable, readTaskEpisodes, updateTaskEpisodes } from ". import { ACTION_POINTER_CUES, AgentPointer, withoutPointerHost } from "./pointer"; import { mutateState, readState } from "./storage"; import { sensitiveFieldState } from "./sensitive-field"; +import { removeTabs } from "./tab-removal"; import { PAGE_AGENT_FILE, PAGE_AGENT_GLOBAL, @@ -62,13 +63,14 @@ import { type PageAgentRequest, } from "./page-agent-protocol"; -export async function removeTabIfPresent(tabId: number): Promise { - try { - await chrome.tabs.remove(tabId); - } catch (error) { +/** Resolves `false` when the tab is still open after the removal wait, as with a leave-page prompt. */ +export async function removeTabIfPresent(tabId: number): Promise { + const [removal] = await removeTabs([tabId]); + if (removal.state === "failed") { const tabStillExists = await chrome.tabs.get(tabId).then(() => true, () => false); - if (tabStillExists) throw error; + if (tabStillExists) throw removal.error; } + return removal.state !== "pending"; } const DEBUGGER_VERSION = "1.3"; @@ -536,7 +538,8 @@ function releasePageWindowOpen(trapKey: string): unknown[] { return Array.isArray(requests) ? requests : []; } -type CloseTab = (tabId: number) => Promise; +/** Releases a tab from its task, revoking ownership and detaching, before a close action removes it. */ +type ReleaseTab = (tabId: number) => Promise; type EventSink = (event: string, payload: Record) => void; type AuthorizeDebuggerUse = (tabId: number) => Promise; type DebuggerLifecycle = (tabId: number) => Promise; @@ -595,7 +598,7 @@ export class StandardBrowserRuntime { constructor( private readonly revisions: RevisionTracker, - private readonly closeTab: CloseTab, + private readonly releaseTab: ReleaseTab, private readonly emit: EventSink, private readonly authorizeDebuggerUse: AuthorizeDebuggerUse = async () => undefined, private readonly recordDebuggerCandidate: DebuggerLifecycle = async () => undefined, @@ -2660,8 +2663,9 @@ export class StandardBrowserRuntime { return { kind, started: true, ...downloadResult }; } if (kind === "close") { - await this.closeTab(tabId); - return { kind, completed: true }; + await this.releaseTab(tabId); + // A tab held open by a leave-page prompt still closes if the user confirms leaving. + return await removeTabIfPresent(tabId) ? { kind, completed: true } : { kind, started: true, close_pending: true }; } if (kind === "set_viewport") { throw Object.assign(new Error("set_viewport is unavailable in Standard mode"), { diff --git a/packages/extension/src/handoff.ts b/packages/extension/src/handoff.ts index a7925ae..324a51e 100644 --- a/packages/extension/src/handoff.ts +++ b/packages/extension/src/handoff.ts @@ -1,3 +1,4 @@ +import { BoundedChain } from "./bounded"; import { MutationScheduler, NotStartedError } from "./scheduler"; import { mutateState, readState, type HandoffNotice, type NoticeStatus } from "./storage"; import { RevisionTracker } from "./revisions"; @@ -11,6 +12,14 @@ const DEFAULT_TIMEOUT_MS = 300_000; const MAX_NOTICES = 100; /** Seconds without input before Chrome reports the user as idle. */ const IDLE_DETECTION_SECONDS = 60; +/** + * Longest one notice transition holds the handoff chain. Transitions are storage and Chrome calls + * that finish well within this. A request for a tab, or a Pause, can wait longer for that tab's + * or every tab's in-flight mutation; it then keeps waiting outside the chain, so a stuck tab + * cannot stall notice status, dismissal, task cleanup, or alarms. The cap stays well under the + * host's 30-second command deadline. + */ +export const HANDOFF_OPERATION_HOLD_MS = 10_000; type PrivacyScrubber = (tabId: number) => Promise; type EventSink = (event: string, payload: Record) => void; @@ -44,7 +53,7 @@ function terminal(status: NoticeStatus): boolean { } export class HandoffController { - private transitionTail: Promise = Promise.resolve(); + private readonly transitions = new BoundedChain("handoff", HANDOFF_OPERATION_HOLD_MS); private scrubber: PrivacyScrubber | null = null; private challengeScanner: ChallengeScanner | null = null; @@ -146,7 +155,8 @@ export class HandoffController { state.paused = true; }); await barrier; - this.emit("pause_changed", { paused: true }); + // A Resume can run while this waits outside the chain; announce Pause only if it still holds. + if ((await readState()).paused) this.emit("pause_changed", { paused: true }); }); } @@ -397,12 +407,7 @@ export class HandoffController { } private serialize(operation: () => Promise): Promise { - const next = this.transitionTail.then(operation); - this.transitionTail = next.then( - () => undefined, - () => undefined, - ); - return next; + return this.transitions.run(operation); } } diff --git a/packages/extension/src/ownership.ts b/packages/extension/src/ownership.ts index 58f9187..a6a2fb8 100644 --- a/packages/extension/src/ownership.ts +++ b/packages/extension/src/ownership.ts @@ -1,10 +1,25 @@ +import { BoundedChain } from "./bounded"; import { MutationScheduler } from "./scheduler"; import { mutateState, readState, type TaskColor, type TaskRecord } from "./storage"; import { automationRouteFields } from "./routes"; import { RevisionTracker } from "./revisions"; +import { removeTabs } from "./tab-removal"; const GROUP_COLORS: readonly TaskColor[] = ["purple", "cyan", "green", "yellow", "orange", "red", "pink", "blue"]; const NO_GROUP = -1; +/** + * Longest one ownership transition holds the ownership chain. Transitions are a few Chrome tab + * and storage calls, plus at most `TAB_REMOVAL_WAIT_MS` when cleanup closes tabs. The cap stays + * well under the host's 30-second command deadline, so a command queued behind a stuck + * transition can still answer in time. + */ +export const OWNERSHIP_OPERATION_HOLD_MS = 10_000; + +/** Returns the tabs Chrome closed within the removal wait. */ +async function removeTaskTabs(tabIds: readonly number[]): Promise { + const removals = await removeTabs(tabIds); + return removals.filter((removal) => removal.state === "closed").map((removal) => removal.tabId); +} type EventSink = (event: string, payload: Record) => void; @@ -37,8 +52,13 @@ function groupTitle(task: TaskRecord, developerMode: boolean): string { } export class OwnershipLedger { - private transitionTail: Promise = Promise.resolve(); + private readonly transitions = new BoundedChain("ownership", OWNERSHIP_OPERATION_HOLD_MS); private inventoryTail: Promise = Promise.resolve(); + /** + * Tasks finished or closed in this service worker. An ownership operation that overran its + * hold can still resume after cleanup ran past it; it must not recreate the task's record. + */ + private readonly endedTaskIds = new Set(); constructor( private readonly scheduler: MutationScheduler, @@ -469,6 +489,7 @@ export class OwnershipLedger { }; } if (!task) { + this.endedTaskIds.add(taskId); return { task_id: taskId, finished: true, closed_tab_ids: [], retained_tab_ids: [] }; } if (disposition === "auto" && state.cleanupPolicy === "ask") { @@ -516,6 +537,7 @@ export class OwnershipLedger { for (const tabId of task.tabIds) this.scheduler.revokeTab(tabId); await mutateState((next) => { delete next.tasks[taskId]; + this.endedTaskIds.add(taskId); for (const [token, staged] of Object.entries(next.stagedCommits)) { if (staged.task_id === taskId) delete next.stagedCommits[token]; } @@ -530,7 +552,7 @@ export class OwnershipLedger { await chrome.action.setBadgeText({ tabId, text: "" }).catch(() => undefined); } } - const closedTabIds: number[] = []; + const removableTabIds: number[] = []; for (const tabId of closedCandidates) { const tab = (await chrome.tabs.get(tabId).catch(() => null)) as TabLike | null; if (!tab) continue; @@ -541,13 +563,11 @@ export class OwnershipLedger { } continue; } - try { - await chrome.tabs.remove(tabId); - closedTabIds.push(tabId); - } catch { - // Ownership is already durably removed; a missing/closed tab is safe. - } + removableTabIds.push(tabId); } + // Ownership is already durably removed. A tab that failed to close or is still waiting on a + // leave-page prompt is reported as neither closed nor retained. + const closedTabIds = await removeTaskTabs(removableTabIds); this.emit("tab_removed", { task_id: taskId, tab_count: 0 }); await this.emitInventory(); return { @@ -563,6 +583,7 @@ export class OwnershipLedger { if (!existing) { // Profile stages bind to the task, not a tab, so a task without tabs can still own one. await mutateState((state) => { + this.endedTaskIds.add(taskId); for (const [token, staged] of Object.entries(state.stagedCommits)) { if (staged.task_id === taskId) delete state.stagedCommits[token]; } @@ -571,6 +592,7 @@ export class OwnershipLedger { } for (const tabId of existing.tabIds) this.scheduler.revokeTab(tabId); const tabIds = await mutateState((state) => { + this.endedTaskIds.add(taskId); const task = state.tasks[taskId]; if (!task) return []; const ownedTabIds = [...task.tabIds]; @@ -581,17 +603,15 @@ export class OwnershipLedger { return ownedTabIds; }); for (const tabId of tabIds) await this.revisions.remove(tabId); - const closedTabIds: number[] = []; + const removableTabIds: number[] = []; for (const tabId of tabIds) { const tab = (await chrome.tabs.get(tabId).catch(() => null)) as TabLike | null; if (!tab || existing.groupId === null || tab.groupId !== existing.groupId) continue; - try { - await chrome.tabs.remove(tabId); - closedTabIds.push(tabId); - } catch { - // Ownership is already durably removed; a missing/closed tab is safe. - } + removableTabIds.push(tabId); } + // Ownership is already durably removed, so a tab that failed to close or is still waiting on + // a leave-page prompt is simply not reported as closed. + const closedTabIds = await removeTaskTabs(removableTabIds); this.emit("tab_removed", { task_id: taskId, tab_count: 0 }); await this.emitInventory(); return closedTabIds; @@ -655,6 +675,11 @@ export class OwnershipLedger { code: "ownership_denied", }); } + if (!next.tasks[taskId] && this.endedTaskIds.has(taskId)) { + throw Object.assign(new Error("The AgentTab task already finished"), { + code: "ownership_denied", + }); + } const task = next.tasks[taskId] ?? previewTask; task.groupId = groupId; if (!task.tabIds.includes(tabId)) task.tabIds.push(tabId); @@ -772,11 +797,6 @@ export class OwnershipLedger { } private serialize(operation: () => Promise): Promise { - const next = this.transitionTail.then(operation); - this.transitionTail = next.then( - () => undefined, - () => undefined, - ); - return next; + return this.transitions.run(operation); } } diff --git a/packages/extension/src/startup.ts b/packages/extension/src/startup.ts index 86bea43..ab3dce0 100644 --- a/packages/extension/src/startup.ts +++ b/packages/extension/src/startup.ts @@ -1,3 +1,5 @@ +import { settledWithin } from "./bounded"; + export type StartupPhase = "idle" | "starting" | "ready"; export class IdempotentStartup { @@ -52,6 +54,14 @@ export class IdempotentStartup { type StartupOperation = () => void | Promise; +/** + * Longest one queued operation may hold the queue. Tab events, alarms (including native + * reconnect), and `browser_open` load barriers all drain through this one queue, so an + * operation awaiting something that never settles would otherwise stall every later one. + * Every bounded wait an operation legitimately makes is far shorter. + */ +export const STARTUP_OPERATION_TIMEOUT_MS = 30_000; + export class StartupOperationQueue { private readonly pending: StartupOperation[] = []; private draining: Promise | null = null; @@ -105,7 +115,12 @@ export class StartupOperationQueue { const operation = this.pending.shift(); if (!operation) continue; try { - await operation(); + // An operation still running at the cap keeps running but no longer holds the queue. + if (!(await settledWithin(operation(), STARTUP_OPERATION_TIMEOUT_MS))) { + throw new Error( + `AgentTab background operation still running after ${STARTUP_OPERATION_TIMEOUT_MS} ms; later operations no longer wait for it`, + ); + } } catch (error) { this.report(this.onOperationError, error); } diff --git a/packages/extension/src/tab-removal.ts b/packages/extension/src/tab-removal.ts new file mode 100644 index 0000000..e5e7594 --- /dev/null +++ b/packages/extension/src/tab-removal.ts @@ -0,0 +1,31 @@ +import { settledWithin } from "./bounded"; + +/** + * How long AgentTab waits for Chrome to close tabs it removes. `chrome.tabs.remove` settles only + * once Chrome destroys the tab, and a page's beforeunload "Leave site?" prompt postpones that + * until someone answers it; a background tab shows the prompt only when it is activated. Removal + * stays requested after the wait, so the tab still closes if the user confirms leaving. + */ +export const TAB_REMOVAL_WAIT_MS = 2_000; + +export type TabRemoval = + | { tabId: number; state: "closed" } + | { tabId: number; state: "pending" } + | { tabId: number; state: "failed"; error: unknown }; + +/** Requests every removal at once and reports each tab's state when they settle or the wait ends. */ +export async function removeTabs(tabIds: readonly number[]): Promise { + const removals: TabRemoval[] = tabIds.map((tabId) => ({ tabId, state: "pending" })); + await settledWithin( + Promise.all(tabIds.map((tabId, index) => chrome.tabs.remove(tabId).then( + () => { + removals[index] = { tabId, state: "closed" }; + }, + (error: unknown) => { + removals[index] = { tabId, state: "failed", error }; + }, + ))), + TAB_REMOVAL_WAIT_MS, + ); + return [...removals]; +} diff --git a/packages/extension/test/extension.test.ts b/packages/extension/test/extension.test.ts index 5920bee..4f61c3c 100644 --- a/packages/extension/test/extension.test.ts +++ b/packages/extension/test/extension.test.ts @@ -2,12 +2,14 @@ import { beforeEach, describe, expect, test, vi } from "bun:test"; import { removeTabIfPresent, StandardBrowserRuntime } from "../src/browser"; import { HandoffController, HANDOFF_ALARM } from "../src/handoff"; import { NativeBridge, RECONNECT_ALARM } from "../src/native"; -import { OwnershipLedger } from "../src/ownership"; +import { BoundedChain } from "../src/bounded"; +import { OWNERSHIP_OPERATION_HOLD_MS, OwnershipLedger } from "../src/ownership"; +import { TAB_REMOVAL_WAIT_MS } from "../src/tab-removal"; import { parseCommand, parseInboundNativeMessage, type NativeOriginPolicy } from "../src/protocol"; import { RevisionTracker } from "../src/revisions"; import { automationRoute, normalizeRestrictedOriginError } from "../src/routes"; import { MutationScheduler } from "../src/scheduler"; -import { IdempotentStartup, StartupOperationQueue } from "../src/startup"; +import { IdempotentStartup, STARTUP_OPERATION_TIMEOUT_MS, StartupOperationQueue } from "../src/startup"; import { mutateState, readState, resetStateForTest, STATE_KEY } from "../src/storage"; import { isRecord } from "../src/type-guards"; import { targetRefs } from "../src/target-resolution"; @@ -3727,6 +3729,158 @@ describe("ownership and task isolation", () => { expect((await readState()).tasks[TASK_A]).toBeUndefined(); }); + test("finishes a task whose tab is held open by a leave-page prompt", async () => { + vi.useFakeTimers(); + try { + await seedTask(TASK_A, [21, 22]); + await seedTask(TASK_B, [23], 6); + await mutateState((state) => { + state.tasks[TASK_A].createdTabIds = [21, 22]; + state.cleanupPolicy = "automatic"; + }); + const ownership = new OwnershipLedger( + new MutationScheduler(), + new RevisionTracker(), + () => undefined, + ); + const removeTab = chrome.tabs.remove.bind(chrome.tabs); + chrome.tabs.remove = ((tabIds: number | number[]) => { + const tabId = Array.isArray(tabIds) ? tabIds[0] : tabIds; + // Chrome settles tabs.remove only once the tab is destroyed, which a beforeunload prompt defers. + if (tabId === 22) return new Promise(() => undefined); + return removeTab(tabId); + }) as typeof chrome.tabs.remove; + + let result: Record | undefined; + void ownership.finishTask(TASK_A, "auto").then((finished) => { + result = finished; + }); + for (let elapsed = 0; elapsed <= TAB_REMOVAL_WAIT_MS + 1_000 && !result; elapsed += 50) { + await advanceTimers(50); + } + + expect(result).toEqual({ + task_id: TASK_A, + finished: true, + closed_tab_ids: [21], + retained_tab_ids: [], + }); + expect((await readState()).tasks[TASK_A]).toBeUndefined(); + let closedOtherTask: number[] | undefined; + void ownership.closeTask(TASK_B).then((closed) => { + closedOtherTask = closed; + }); + for (let turn = 0; turn < 20 && !closedOtherTask; turn += 1) await advanceTimers(0); + expect(closedOtherTask).toEqual([23]); + } finally { + vi.useRealTimers(); + } + }); + + test("moves the ownership chain past a transition that never settles", async () => { + vi.useFakeTimers(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + try { + await seedTask(TASK_A, [21]); + await seedTask(TASK_B, [23], 6); + const ownership = new OwnershipLedger( + new MutationScheduler(), + new RevisionTracker(), + () => undefined, + ); + const getTab = chrome.tabs.get.bind(chrome.tabs); + chrome.tabs.get = ((tabId: number) => ( + tabId === 21 ? new Promise(() => undefined) : getTab(tabId) + )) as typeof chrome.tabs.get; + + void ownership.finishTask(TASK_A, "auto"); + let closedOtherTask: number[] | undefined; + void ownership.closeTask(TASK_B).then((closed) => { + closedOtherTask = closed; + }); + await advanceTimers(OWNERSHIP_OPERATION_HOLD_MS - 100); + expect(closedOtherTask).toBeUndefined(); + for (let elapsed = 0; elapsed < 1_000 && !closedOtherTask; elapsed += 50) await advanceTimers(50); + + expect(closedOtherTask).toEqual([23]); + expect(warn).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining("ownership operation still running"), + })); + } finally { + warn.mockRestore(); + vi.useRealTimers(); + } + }); + + test("starts operations queued behind a stuck one one at a time", async () => { + vi.useFakeTimers(); + try { + const overruns: Error[] = []; + const chain = new BoundedChain("test", 1_000, (error) => overruns.push(error)); + const started: string[] = []; + const { promise: secondHeld, resolve: releaseSecond } = Promise.withResolvers(); + void chain.run(() => new Promise(() => undefined)); + void chain.run(async () => { + started.push("second"); + await secondHeld; + }); + void chain.run(() => { + started.push("third"); + }); + + await advanceTimers(0); + await advanceTimers(1_000); + expect(started).toEqual(["second"]); + await advanceTimers(900); + expect(started).toEqual(["second"]); + + releaseSecond(); + await advanceTimers(0); + expect(started).toEqual(["second", "third"]); + expect(overruns).toHaveLength(1); + } finally { + vi.useRealTimers(); + } + }); + + test("an open that overran its hold cannot recreate a task that finished past it", async () => { + vi.useFakeTimers(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const create = chrome.tabs.create.bind(chrome.tabs); + try { + const ownership = new OwnershipLedger( + new MutationScheduler(), + new RevisionTracker(), + () => undefined, + ); + const { promise: creationReleased, resolve: releaseCreation } = Promise.withResolvers(); + chrome.tabs.create = (async (options: Parameters[0]) => { + await creationReleased; + return create(options); + }) as typeof chrome.tabs.create; + + const opening = ownership.open(TASK_B, { mode: "create", url: "https://example.test/late" }); + opening.catch(() => undefined); + let finished: Record | undefined; + void ownership.finishTask(TASK_B, "close").then((result) => { + finished = result; + }); + await advanceTimers(0); + await advanceTimers(OWNERSHIP_OPERATION_HOLD_MS + 100); + expect(finished).toMatchObject({ task_id: TASK_B, finished: true }); + + releaseCreation(); + await advanceTimers(0); + await expect(opening).rejects.toMatchObject({ code: "ownership_denied" }); + expect((await readState()).tasks[TASK_B]).toBeUndefined(); + expect(removedTabIds).toHaveLength(1); + } finally { + chrome.tabs.create = create as typeof chrome.tabs.create; + warn.mockRestore(); + vi.useRealTimers(); + } + }); + test("explicit close preserves a tab that left its task group", async () => { await seedTask(TASK_A, [21, 22]); tabStore.get(22)!.groupId = -1; @@ -4182,7 +4336,7 @@ describe("tab removal", () => { }, }); - await expect(removeTabIfPresent(77)).resolves.toBeUndefined(); + await expect(removeTabIfPresent(77)).resolves.toBe(true); tabStore.set(78, { id: 78, @@ -4197,6 +4351,39 @@ describe("tab removal", () => { }); await expect(removeTabIfPresent(78)).rejects.toThrow("Tabs cannot be edited"); }); + + test("returns a close action while a leave-page prompt holds the tab open", async () => { + await mutateState((state) => { + state.skipCommitReview = true; + }); + const revisions = new RevisionTracker(); + const released: number[] = []; + const runtime = new StandardBrowserRuntime( + revisions, + async (tabId) => { + released.push(tabId); + }, + () => undefined, + ); + const pageRevision = await revisions.ensure(7); + // Chrome settles tabs.remove only once the tab is destroyed, which a beforeunload prompt defers. + Object.assign(chrome.tabs, { remove: () => new Promise(() => undefined) }); + vi.useFakeTimers(); + try { + let actions: unknown; + void runtime.act(TASK_A, 7, pageRevision, [{ kind: "close" }]).then((execution) => { + actions = execution.result?.actions; + }); + for (let elapsed = 0; elapsed <= TAB_REMOVAL_WAIT_MS + 1_000 && actions === undefined; elapsed += 50) { + await advanceTimers(50); + } + + expect(actions).toEqual([{ kind: "close", started: true, close_pending: true }]); + expect(released).toEqual([7]); + } finally { + vi.useRealTimers(); + } + }); }); describe("native bridge transport", () => { @@ -4744,6 +4931,33 @@ describe("startup lifecycle", () => { expect(attempts).toBe(2); }); + + test("moves past a queued operation that never settles", async () => { + vi.useFakeTimers(); + try { + const operationErrors: unknown[] = []; + const queue = new StartupOperationQueue( + new IdempotentStartup(async () => undefined), + () => undefined, + (error) => operationErrors.push(error), + ); + let ranNext = false; + + queue.enqueue(() => new Promise(() => undefined)); + queue.enqueue(() => { + ranNext = true; + }); + await advanceTimers(STARTUP_OPERATION_TIMEOUT_MS - 50); + expect(ranNext).toBe(false); + await advanceTimers(100); + + expect(ranNext).toBe(true); + expect(operationErrors).toHaveLength(1); + expect(String(operationErrors[0])).toContain(`still running after ${STARTUP_OPERATION_TIMEOUT_MS} ms`); + } finally { + vi.useRealTimers(); + } + }); }); describe("consequential action staging", () => {