Skip to content

feat: implement handle signal#84

Open
timl3136 wants to merge 3 commits intocadence-workflow:mainfrom
timl3136:signal-handling
Open

feat: implement handle signal#84
timl3136 wants to merge 3 commits intocadence-workflow:mainfrom
timl3136:signal-handling

Conversation

@timl3136
Copy link
Copy Markdown
Member

@timl3136 timl3136 commented Apr 9, 2026

What changed?
Added inbound signal handling. Workflows can now receive signals via @workflow.signal_method and block until a condition is met with workflow.wait_condition(predicate).

Why?
Signaling is a core feature

How did you test it?
unit tests

Potential risks

Release notes

Documentation Changes

Signed-off-by: Tim Li <ltim@uber.com>
timl3136 added 2 commits April 9, 2026 10:26
Signed-off-by: Tim Li <ltim@uber.com>
Address PR review comments:
- Wrap _run_handle in try/except to match CPython Handle._run()
  behavior, preventing unhandled exceptions from crashing the event loop
- Fix mypy no-any-return in signal.py params_from_payload
- Fix mypy attr-defined errors for Handle private attributes
- Update tests to verify exceptions are logged rather than propagated

Signed-off-by: Tim Li <ltim@uber.com>
Made-with: Cursor
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 9, 2026

Responding to your feedback

CI failed: Code formatting violation in deterministic_event_loop.py: Black detected 1 file that would need reformatting, causing CI check to fail with exit code 1.

Overview

A single CI failure was detected across 1 log. The code formatter (Black) identified a formatting violation in cadence/_internal/workflow/deterministic_event_loop.py that was introduced by changes in this PR. The CI check runs Black in validation mode and fails when formatting issues are found.

Failures

Black Formatting Check Failed (confidence: high)

  • Type: configuration
  • Affected jobs: 70663341477
  • Related to change: yes
  • Root cause: The file cadence/_internal/workflow/deterministic_event_loop.py contains code that does not conform to Black formatting standards. This file was modified in the PR (+16,-1 lines), introducing the formatting violation. The CI job runs Black in check mode, which reports but does not auto-fix violations, causing the build to fail with exit code 1.
  • Suggested fix: Run uv run black cadence/_internal/workflow/deterministic_event_loop.py locally to auto-format the file according to Black standards, then commit and push the changes.

Summary

  • Change-related failures: 1 — Formatting violation directly caused by PR changes to deterministic_event_loop.py
  • Infrastructure/flaky failures: None detected
  • Recommended action: Apply Black auto-formatting to the modified file and commit the changes. This is a straightforward formatting fix unrelated to logic or functionality.
Code Review ⚠️ Changes requested 1 resolved / 3 findings

Signal handling implementation adds exception handling to _run_handle but introduces two issues: predicate exceptions in notify_state_changed abort the waiter loop prematurely, and async signal handlers silently fail with NotImplementedError instead of propagating errors to the task.

⚠️ Edge Case: Predicate exception in notify_state_changed skips remaining waiters

📄 cadence/_internal/workflow/context.py:138-148

In notify_state_changed, if any predicate() call raises an exception, the loop aborts and all subsequent waiters are never evaluated. This means one misbehaving predicate can permanently block unrelated wait_condition callers.

Wrap each iteration in a try/except so that a failing predicate doesn't prevent other waiters from being resolved.

Suggested fix
def notify_state_changed(self) -> None:
    remaining: list[tuple[Callable[[], bool], Any]] = []
    for predicate, future in self._waiters:
        if future.done():
            continue
        try:
            if predicate():
                future.set_result(None)
            else:
                remaining.append((predicate, future))
        except Exception:
            # Keep the waiter so it can be re-evaluated later
            remaining.append((predicate, future))
    self._waiters = remaining
💡 Bug: Async signal handlers silently fail instead of failing the task

📄 cadence/_internal/workflow/workflow_instance.py:65-69 📄 cadence/_internal/workflow/workflow_instance.py:82-85

When an async signal handler is encountered, _invoke_signal raises NotImplementedError. This exception propagates through _deliver_signal_run_handle, where it's caught and logged but otherwise swallowed. The on_applied() callback (which calls notify_state_changed) is never invoked, so any wait_condition predicates that depended on this signal's side-effects will never be re-evaluated.

Consider either calling on_applied() in a finally block in _deliver_signal, or failing the decision task explicitly so the server can retry.

Suggested fix
def _deliver_signal(
    self, signal_name: str, payload: Payload, on_applied: Callable[[], None]
) -> None:
    try:
        self._invoke_signal(signal_name, payload)
    finally:
        on_applied()
✅ 1 resolved
Bug: _run_handle bypasses Handle._run(), losing exception handling

📄 cadence/_internal/workflow/deterministic_event_loop.py:170-175
The new _run_handle static method directly invokes handle._callback(*handle._args) instead of calling handle._run(). CPython's Handle._run() wraps the callback in a try/except that catches BaseException (except SystemExit/KeyboardInterrupt) and routes it to loop.call_exception_handler(). The custom implementation loses this safety net, meaning any unhandled exception in a callback will propagate directly and could crash the event loop or leave errors unlogged.

The context-aware dispatch (handle._context.run(...)) is correct, but the missing exception handling is a regression from the previous code that used handle._run().

🤖 Prompt for agents
Code Review: Signal handling implementation adds exception handling to _run_handle but introduces two issues: predicate exceptions in notify_state_changed abort the waiter loop prematurely, and async signal handlers silently fail with NotImplementedError instead of propagating errors to the task.

1. ⚠️ Edge Case: Predicate exception in notify_state_changed skips remaining waiters
   Files: cadence/_internal/workflow/context.py:138-148

   In `notify_state_changed`, if any `predicate()` call raises an exception, the loop aborts and all subsequent waiters are never evaluated. This means one misbehaving predicate can permanently block unrelated `wait_condition` callers.
   
   Wrap each iteration in a try/except so that a failing predicate doesn't prevent other waiters from being resolved.

   Suggested fix:
   def notify_state_changed(self) -> None:
       remaining: list[tuple[Callable[[], bool], Any]] = []
       for predicate, future in self._waiters:
           if future.done():
               continue
           try:
               if predicate():
                   future.set_result(None)
               else:
                   remaining.append((predicate, future))
           except Exception:
               # Keep the waiter so it can be re-evaluated later
               remaining.append((predicate, future))
       self._waiters = remaining

2. 💡 Bug: Async signal handlers silently fail instead of failing the task
   Files: cadence/_internal/workflow/workflow_instance.py:65-69, cadence/_internal/workflow/workflow_instance.py:82-85

   When an async signal handler is encountered, `_invoke_signal` raises `NotImplementedError`. This exception propagates through `_deliver_signal` → `_run_handle`, where it's caught and logged but otherwise swallowed. The `on_applied()` callback (which calls `notify_state_changed`) is never invoked, so any `wait_condition` predicates that depended on this signal's side-effects will never be re-evaluated.
   
   Consider either calling `on_applied()` in a `finally` block in `_deliver_signal`, or failing the decision task explicitly so the server can retry.

   Suggested fix:
   def _deliver_signal(
       self, signal_name: str, payload: Payload, on_applied: Callable[[], None]
   ) -> None:
       try:
           self._invoke_signal(signal_name, payload)
       finally:
           on_applied()

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

@timl3136
Copy link
Copy Markdown
Member Author

timl3136 commented Apr 9, 2026

Gitar fix CI

@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 9, 2026

Gitar fix CI

Applied Black formatting to cadence/_internal/workflow/deterministic_event_loop.py to fix the CI failure. The commit has been pushed.

Comment on lines +138 to +148
def notify_state_changed(self) -> None:
"""Re-evaluate all wait_condition predicates. Resolve those that are now True."""
remaining: list[tuple[Callable[[], bool], Any]] = []
for predicate, future in self._waiters:
if future.done():
continue
if predicate():
future.set_result(None)
else:
remaining.append((predicate, future))
self._waiters = remaining
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Edge Case: Predicate exception in notify_state_changed skips remaining waiters

In notify_state_changed, if any predicate() call raises an exception, the loop aborts and all subsequent waiters are never evaluated. This means one misbehaving predicate can permanently block unrelated wait_condition callers.

Wrap each iteration in a try/except so that a failing predicate doesn't prevent other waiters from being resolved.

Suggested fix:

def notify_state_changed(self) -> None:
    remaining: list[tuple[Callable[[], bool], Any]] = []
    for predicate, future in self._waiters:
        if future.done():
            continue
        try:
            if predicate():
                future.set_result(None)
            else:
                remaining.append((predicate, future))
        except Exception:
            # Keep the waiter so it can be re-evaluated later
            remaining.append((predicate, future))
    self._waiters = remaining

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

Comment on lines +65 to +69
def _deliver_signal(
self, signal_name: str, payload: Payload, on_applied: Callable[[], None]
) -> None:
self._invoke_signal(signal_name, payload)
on_applied()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Bug: Async signal handlers silently fail instead of failing the task

When an async signal handler is encountered, _invoke_signal raises NotImplementedError. This exception propagates through _deliver_signal_run_handle, where it's caught and logged but otherwise swallowed. The on_applied() callback (which calls notify_state_changed) is never invoked, so any wait_condition predicates that depended on this signal's side-effects will never be re-evaluated.

Consider either calling on_applied() in a finally block in _deliver_signal, or failing the decision task explicitly so the server can retry.

Suggested fix:

def _deliver_signal(
    self, signal_name: str, payload: Payload, on_applied: Callable[[], None]
) -> None:
    try:
        self._invoke_signal(signal_name, payload)
    finally:
        on_applied()

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 9, 2026

Gitar fix CI

Fixed the Black formatting issue in cadence/_internal/workflow/deterministic_event_loop.py — converted single-quoted strings to double-quoted strings in the _run_handle exception handler dict, which is what Black requires. Committed in ee1cda9.

@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 9, 2026

Gitar fix CI

Fixed the Black formatting issue in cadence/_internal/workflow/deterministic_event_loop.py. Black reformatted the call_exception_handler dict call — converting single-quoted strings to double-quoted strings and adjusting the call structure. Committed in e8b839a.

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