Skip to content

fix: cancel delegated browser observations with task execution - #31

Open
kvnloo wants to merge 8 commits into
CopilotKit:mainfrom
kvnloo:fix/task-browser-cancellation
Open

kvnloo wants to merge 8 commits into
CopilotKit:mainfrom
kvnloo:fix/task-browser-cancellation

Conversation

@kvnloo

@kvnloo kvnloo commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

What changed

Chat browser reads already thread an AbortSignal through navigation and page reads, but the shared BrowserService.observe() path used by delegated tasks and monitors did not.

That meant pausing/cancelling a task invalidated the later task checkpoint, but an in-flight browser navigation/read could continue until the worker request completed or timed out.

This change makes observe(), create(), and reopen() accept an optional signal, threads the task's existing ctx.signal through delegated read_web and monitor observations, and preserves existing UI/API callers by keeping the parameter optional.

The regression aborts a task-style observation during navigation, asserts that the follow-up page read is never started, and pins that intentional cancellation does not persist the browser session as an error.

Verification

  • Reviewed the cumulative branch against current main at 82ff35b.
  • Existing chat cancellation already exercises the same lower-level signal-aware request path; this extends that invariant to task/monitor callers.
  • The new regression specifically requires only /sessions to be contacted after cancellation, with no /read request, and requires the saved browser session to remain non-error.
  • Repository tests were not run locally because this execution environment cannot clone/install the repository; CI on this PR is the executable verification.

Integration limits

Fixture-only cancellation regression; no live Chromium worker/provider run was performed.

AI-use note: I used an AI assistant to trace the task/browser cancellation paths, draft the regression and patch, and review the final diff. I verified the missing signal propagation against current source before opening this PR.

@jerelvelarde

Copy link
Copy Markdown
Collaborator

Security review — no issues found (reviewed head 65c1b04)

  • Cancellation now actually stops work. Pausing or cancelling a task now aborts in-flight browser work for delegated read_web and monitor observations. Before, a cancelled task could keep driving the browser worker until its request timed out. That's a resource-control improvement.
  • Ownership still comes first. create() records ownership before calling the worker, and the new signal is only threaded through the existing openOwned/readOwned paths. No new route skips the owner check.
  • A cancelled session isn't marked as errored. The new signal?.throwIfAborted() in the openOwned catch block means cancelling doesn't write the session as error. The record stays owned and idle, so nothing is orphaned and it can't be reused across owners.
  • Existing callers are unaffected. The parameter is optional, so UI and API callers behave as before.
  • Scope is limited. It touches only browser.ts, engine/model.ts, engine/service.ts and tests/browser.test.ts: no dependencies, lockfile, CI or config changes.

Heads-up: this conflicts with #16 in engine/service.ts; whichever lands second needs a rebase. CI hasn't run on this PR yet.

@jerelvelarde jerelvelarde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI's quality job fails at the pnpm lint step (biome check .) on head 65c1b04:

tests/browser.test.ts format
  × Formatter would have printed the following content:
    169     │ - ··await·assert.rejects(
    170     │ - ····service.observe("owner",·savedSession.url,·undefined,·controller.signal),
    171     │ - ····{·name:·"AbortError"·},
    172     │ - ··);
    ...
    170 │ + ····name:·"AbortError",
Found 1 error.

The formatter wants the assert.rejects(...) call in the new cancellation test laid out differently: the { name: "AbortError" } matcher gets expanded onto its own lines. Run pnpm exec biome check --write tests/browser.test.ts and push the result.

Lint fails first, so typecheck, test and build:server never ran on this PR, and the new cancellation regression hasn't actually been exercised in CI yet. Please rebase onto current main when you push, so the rerun covers #38's changes. All other jobs passed. The security review above still applies to the code change itself.

@kvnloo

kvnloo commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the biome check . failure you flagged: the assert.rejects(...) call in tests/browser.test.ts is now folded to fit the 100-column formatter width, exactly as the formatter prescribed. Verified biome check passes locally on biome 2.5.13 (matching the lockfile). Pushed as cfd11bafd7e — the quality job should now get past lint to typecheck/test.

Posted by Kevin's agent on his behalf — AI-assisted (Muse, Meta's Muse Spark).

@davidmckayv

Copy link
Copy Markdown
Contributor

Needs a test before merge. The new test covers the browser service only. Reverting the two lines that pass the task's signal into read_web (model.ts) and the monitor check (service.ts) leaves the suite green, so the change the title describes is untested. Add a test that cancels a delegated task during a read_web or a monitor check and asserts that no page read is sent and the task returns to queued, not failed.

@kvnloo kvnloo closed this Sep 26, 2026
@kvnloo
kvnloo force-pushed the fix/task-browser-cancellation branch from 7468a5c to 205cc38 Compare September 26, 2026 02:12
@kvnloo kvnloo reopened this Sep 26, 2026

kvnloo commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

The maintainer-requested production-boundary regression is now on the branch.

Current tests/model-worker.test.ts drives the delegated task through the real model-worker path:

  • model calls read_web;
  • browser navigation reaches /sessions and is held open;
  • server.agent.worker.abort(task.id) cancels through the task execution path;
  • the task settles back to queued with error === null;
  • browser calls are exactly ["/sessions"], so no page /read is sent.

The previous CI run did not reach that test because Biome stopped at formatting. I fixed the exact formatter complaint in 690e0e32046adfe77896f1f2582aa5627d53e4d7.

I am not claiming the new head green until CI runs it; the point of this update is that reverting the model.ts signal propagation should now make the delegated-task regression fail, which addresses the coverage gap called out above.

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.

3 participants