Skip to content

feat(worker): add worker.close() and allow context management of worker class#77

Merged
shijiesheng merged 5 commits intocadence-workflow:mainfrom
shijiesheng:worker-context
Mar 25, 2026
Merged

feat(worker): add worker.close() and allow context management of worker class#77
shijiesheng merged 5 commits intocadence-workflow:mainfrom
shijiesheng:worker-context

Conversation

@shijiesheng
Copy link
Copy Markdown
Member

@shijiesheng shijiesheng commented Mar 24, 2026

What changed?

  • add worker.close() method
  • add aenter and aexit methods

Why?

make it easier to use worker

How did you test it?

Unit Test

Potential risks

Release notes

Documentation Changes

Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Mar 24, 2026

Code Review ✅ Approved 4 resolved / 4 findings

Implements context management for worker.run() with proper resource cleanup, addressing client channel leaks and silent task failures. Worker exceptions are now logged and cleanup is guaranteed through context manager semantics.

✅ 4 resolved
Bug: Client context manager dropped in integration test helper

📄 tests/integration_tests/helper.py:25-27
The old code used async with self.client() as client: which properly called Client.__aenter__() (initializing the gRPC channel via ready()) and Client.__aexit__() (closing the channel via close()). The new code passes self.client() directly to Worker(...) without entering the context manager, so:

  1. ready() is never called — the client may not be properly initialized.
  2. close() is never called — gRPC channel resources will leak after each test.

This will likely cause integration test failures or resource leaks.

Edge Case: Worker tasks may fail silently before yield completes

📄 cadence/worker/_worker.py:43-50 📄 cadence/worker/_worker.py:38-47
In Worker.run(), tasks are created via asyncio.create_task() and then yield self is reached. If a task raises an exception before the caller's async with block body executes (e.g., connection failure on first poll), that exception is silently swallowed by return_exceptions=True in the finally block's asyncio.gather(). With the old TaskGroup approach, such exceptions would propagate. Consider checking task results after gather and re-raising if any contain non-CancelledError exceptions.

Bug: Client gRPC channel leaked — never closed in helper

📄 tests/integration_tests/helper.py:20-21 📄 cadence/worker/_worker.py:44-54
The old helper.worker() wrapped the client in async with self.client() as client:, which ensured the gRPC channel was closed on exit via Client.__aexit__self._channel.close(). The new code returns Worker(self.client(), ...) directly, and Worker.__aexit__ only cancels worker tasks — it never closes the client. Every integration test using helper.worker() will leak an open gRPC channel.

This is a real resource leak in tests and also sets a problematic pattern for production usage.

Edge Case: Worker task exceptions silently swallowed

📄 cadence/worker/_worker.py:44-47
With the old TaskGroup-based run(), if a worker task raised an unexpected exception it would propagate as an ExceptionGroup. Now, close() uses asyncio.gather(*self._tasks, return_exceptions=True), which captures all exceptions (including non-CancelledError ones) and discards them — the return value of gather is never inspected. If a decision or activity worker crashes before close() is called, the failure is completely silent. Additionally, between run() returning and close() being called, there is no mechanism to detect or surface a task failure.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@shijiesheng shijiesheng changed the title feat(worker): worker.run() is now context managed feat(worker): add worker.close() and allow context management of worker class Mar 24, 2026
Copy link
Copy Markdown
Member

@timl3136 timl3136 left a comment

Choose a reason for hiding this comment

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

LGTM

@shijiesheng shijiesheng merged commit 66774a3 into cadence-workflow:main Mar 25, 2026
9 checks passed
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.

2 participants