Skip to content

fix: narrow send_cancel BaseException catch to Exception#1198

Open
rasdani wants to merge 1 commit intomainfrom
fix/narrow-zmq-send-cancel-catch
Open

fix: narrow send_cancel BaseException catch to Exception#1198
rasdani wants to merge 1 commit intomainfrom
fix/narrow-zmq-send-cancel-catch

Conversation

@rasdani
Copy link
Copy Markdown
Contributor

@rasdani rasdani commented Apr 19, 2026

Summary

ZMQEnvClient.send_cancel caught BaseException and silently passed. Intent was best-effort cleanup — transport/socket errors shouldn't break shutdown. But BaseException also swallows:

  • asyncio.CancelledError — denying the caller's cancellation. Particularly ironic in a method literally named send_cancel.
  • KeyboardInterrupt — eating user Ctrl-C during cleanup.
  • SystemExit — ignoring explicit sys.exit().

Narrowed to except Exception: pass. Transport / socket / ZMQ errors still swallowed as before; cancellation and interrupt signals now propagate correctly.

Changes

  • `verifiers/serve/client/zmq_env_client.py` — single-line catch narrowing with comment
  • `tests/test_env_server.py` — added `TestSendCancelErrorHandling` with 5 tests:
    • `test_send_cancel_swallows_connection_error` — regression: transport errors silently swallowed
    • `test_send_cancel_swallows_oserror` — regression: OSError silently swallowed
    • `test_send_cancel_propagates_cancelled_error` — the headline bug
    • `test_send_cancel_propagates_keyboard_interrupt`
    • `test_send_cancel_propagates_system_exit`

Verification

  • `uv run pytest tests/test_env_server.py -x -q` — 15 passed (10 existing + 5 new)
  • `uv run ruff check` + `ruff format --check` — clean

Context

Part of a small audit of `BaseException` catches in verifiers following bugbot's review on #1196. See the plan file for the full inventory; companion PR is #1197 (same narrowing applied to `math_verify` calls in `math_rubric.py`).


Note

Low Risk
Low risk: a narrowly-scoped exception-handling tweak in send_cancel plus targeted tests; behavior only changes for cancellation/interrupt signals while keeping transport errors best-effort.

Overview
ZMQEnvClient.send_cancel now catches Exception instead of BaseException, so best-effort cancel notifications still ignore socket/transport failures but no longer suppress asyncio.CancelledError, KeyboardInterrupt, or SystemExit.

Adds a new TestSendCancelErrorHandling suite verifying transport errors are swallowed and cancellation/interrupt signals propagate.

Reviewed by Cursor Bugbot for commit 6598c35. Bugbot is set up for automated code reviews on this repo. Configure here.

`ZMQEnvClient.send_cancel` is a best-effort cleanup op - if the socket
fails while sending the cancel notification, we don't want that to
break shutdown. But `except BaseException: pass` is too wide: it also
swallows `asyncio.CancelledError`, `KeyboardInterrupt`, and
`SystemExit`, which is especially wrong in a method called `send_cancel`
(literally denying the caller's own cancellation).

Narrow to `except Exception: pass`. Transport / socket / ZMQ errors
are still quietly swallowed as before; cancellation and interrupt
signals now propagate like they should.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@rasdani rasdani marked this pull request as draft April 19, 2026 21:36
@rasdani rasdani marked this pull request as ready for review May 9, 2026 16:43
@rasdani rasdani requested a review from mikasenghaas May 9, 2026 16:43
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.

1 participant