Skip to content

fix(grpc): support signal shutdown on Windows - #8082

Open
primorLee wants to merge 2 commits into
microsoft:mainfrom
primorLee:fix/windows-grpc-signal-handler
Open

fix(grpc): support signal shutdown on Windows#8082
primorLee wants to merge 2 commits into
microsoft:mainfrom
primorLee:fix/windows-grpc-signal-handler

Conversation

@primorLee

Copy link
Copy Markdown

Why are these changes needed?

GrpcWorkerAgentRuntime.stop_when_signal() currently calls loop.add_signal_handler() unconditionally. The default Windows event loop does not implement that API, so cross-language workers fail with NotImplementedError instead of waiting for shutdown. The gRPC host has the same signal-registration path.

This change keeps native asyncio signal handlers on supported event loops and falls back to signal.signal() when the event-loop API is unavailable. The fallback schedules shutdown on the owning loop with call_soon_threadsafe, and both paths remove or restore their handlers when waiting finishes. A shared helper keeps worker and host behavior consistent.

Related issue number

Closes #5760

Checks

  • I've included any doc changes needed for https://microsoft.github.io/autogen/. No public API or documentation changes are required.
  • I've added tests corresponding to the changes introduced in this PR.
  • I've made sure all auto checks have passed. CI is pending.

Local validation:

  • pytest -q python/packages/autogen-ext/tests/test_grpc_signal_utils.py — 2 passed on Windows
  • Ruff 0.4.8 format and lint — passed for all four changed files
  • Pyright 1.1.389 — 0 errors for all four changed files
  • strict mypy — 0 errors for the new signal helper

AI assistance: Codex was used to inspect the failing paths, implement the fix, and run the checks above. The final diff and test behavior were reviewed before submission.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I read through _signal_utils.py and both call sites. The approach is sound: try add_signal_handler first, catch the NotImplementedError it actually raises on Windows' default event loop, then fall back to signal.signal() with call_soon_threadsafe to safely hop back onto the loop from the signal handler thread. Deduping signals via dict.fromkeys and restoring the previous handlers in finally are nice touches.

One concrete gap: neither new test in test_grpc_signal_utils.py is marked @pytest.mark.windows. This package's CI Windows job runs pytest -m 'windows', and that marker is the established pattern for Windows-only coverage. Without it, these tests execute on the Linux job via a monkeypatched NotImplementedError but are never collected by the actual windows-latest runner — so the fallback path stays CI-unverified on the platform it targets, despite the "2 passed on Windows" note being local-only.

Separately: is SIGBREAK (Ctrl+Break) worth including in the default signal set for stop_when_signal, or intentionally out of scope?

I'm on Windows and can sanity-check the fallback locally if useful.

Requesting changes — logic is correct, but add the windows marker so CI actually exercises the code this PR fixes.

@primorLee

Copy link
Copy Markdown
Author

Addressed the requested Windows CI coverage in 2291f826:

  • added @pytest.mark.windows to both new signal-handler tests;
  • verified the targeted file on Windows: 2 passed;
  • verified the actual Windows selector with pytest -m windows: 2 passed;
  • Ruff 0.4.8 check and format checks pass.

SIGBREAK is intentionally out of scope for this PR. Issue #5760 is specifically about add_signal_handler() being unavailable on Windows, so this change preserves the existing default (SIGTERM, SIGINT) behavior. The API still accepts a caller-supplied signal sequence, so Windows callers can explicitly pass SIGBREAK when needed. I can follow up separately if maintainers prefer adding it to the default set.

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.

running xlang app host on windows, python grpc exception: NotImplementedError

2 participants