Skip to content

fix(sdk): re-check cancellation after acquiring resource lock in ParallelToolExecutor - #4875

Open
shanemort1982 wants to merge 1 commit into
OpenHands:mainfrom
shanemort1982:fix/parallel-executor-cancel-after-lock
Open

fix(sdk): re-check cancellation after acquiring resource lock in ParallelToolExecutor#4875
shanemort1982 wants to merge 1 commit into
OpenHands:mainfrom
shanemort1982:fix/parallel-executor-cancel-after-lock

Conversation

@shanemort1982

@shanemort1982 shanemort1982 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

HUMAN:
Hit this on my self-hosted agent-canvas deployment: a cancelled tool call kept running after interrupt. Validated the fix end-to-end on my live box before submitting, and reviewed the diff myself.


AGENT:

Why

ParallelToolExecutor._run_safe checks cancel_token.is_cancelled once, before resolving resource locks. A tool call that blocks waiting for a ResourceLockManager lock and is cancelled during that wait still invokes tool_runner once the lock becomes available. This violates the documented contract that pending tool calls are skipped after cancellation, and lets a queued file/terminal/browser operation start after the user interrupted the run.

Summary

  • Re-check cancel_token.is_cancelled immediately after acquiring the resource lock, returning the existing synthetic cancellation error instead of running the tool.
  • Add tests/sdk/agent/test_parallel_executor_cancel_wait.py: parametrised regression test covering sync declared-resource, sync tool-mutex, and async paths, plus an uncancelled-path sanity test.

Issue Number

Fixes #4777

How to Test

Run the new regression tests:

uv run pytest tests/sdk/agent/test_parallel_executor_cancel_wait.py -v

End-to-end verification performed on a self-hosted agent-canvas deployment (SDK v1.44.1, ghcr.io/openhands/agent-canvas:main):

  1. Before the fix, a cancelled tool call still invoked tool_runner after the lock became available (reproduces on v1.44.1 and current main).
  2. After the fix, the cancelled call returns the synthetic AgentErrorEvent ("Tool call cancelled by interrupt.") and tool_runner is never invoked; the uncancelled path still returns normal tool output.
  3. Live e2e on the running agent-server: created a conversation, the agent started a sleep 120 terminal tool call, POST /api/conversations/{id}/interrupt moved the conversation from running to paused immediately. Server log: Skipping tool 'terminal' -- cancelled while waiting for lock.

Video/Screenshots

N/A - no UI change; behaviour is terminal-observable only.

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

Notes

  • The same guard was independently proposed in fix(sdk): skip cancelled lock waiters #4780 (opened first); credit to @jstar0. This PR carries the same fix with additional coverage (the uncancelled path) and live end-to-end evidence.
  • Residual gap, out of scope here: cancellation does not wake a lock waiter, so an interrupted call still blocks until the holder releases or the per-resource timeout fires (30s file, 60s tool, 300s terminal/browser/mcp). A cancel-aware FIFOLock.acquire would be the proper follow-up.

ParallelToolExecutor._run_safe checked the cancel token only before
resource resolution. A call cancelled while blocked on a resource lock
still invoked tool_runner once the lock became available. Re-check the
token after acquiring the lock and return the synthetic cancellation
error instead.

Fixes OpenHands#4777

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

📁 PR Artifacts Notice

This PR contains a .pr/ directory with temporary PR-specific documents. Because this is a fork PR, the directory will be automatically removed from main immediately after merge.

@shanemort1982

Copy link
Copy Markdown
Contributor Author

Heads up on the two failing sdk-tests checks: both are pre-existing on main (run 34000698924, commit fe91d7d) and unrelated to this change:

  • tests/sdk/llm/test_runtime_metadata.py::test_effective_unchanged_before_resolution
  • tests/sdk/llm/test_model_features.py::test_reasoning_effort_support[openrouter/moonshotai/kimi-k2-thinking-False]

Both fail identically on main's own CI. The four tests added by this PR all pass in the same run (visible in the 6092 passed).

@shanemort1982
shanemort1982 marked this pull request as ready for review September 6, 2026 06:04
@all-hands-bot

Copy link
Copy Markdown
Collaborator

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

@shanemort1982

Copy link
Copy Markdown
Contributor Author

@enyst could you take a look when you get a chance? Small fix for #4777 with a regression test. The two failing sdk-tests checks are pre-existing on main (run 34000698924, commit fe91d7d); happy to do anything needed to move it forward.

@shanemort1982

Copy link
Copy Markdown
Contributor Author

Tracked the two failing sdk-tests in #4877 (red on main from model-metadata data drift, not a code change). This PR's own four tests pass; happy to help land the metadata-test fix if useful.

@jstar0 jstar0 mentioned this pull request Sep 7, 2026
5 tasks
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]: ParallelToolExecutor starts a pending tool after cancellation while waiting for a resource lock

2 participants