fix(events): add close() method to ReactiveEventBus to complete RxPY subject #11190

Open
freemo wants to merge 2 commits from PR-fix-wf18 into master
Owner

Fix for issue #10378: ReactiveEventBus._subject (RxPY Subject) is never completed — missing close() method causes RxPY subscriber resource leaks.

Changes:

  • Add _closed flag to track bus shutdown state
  • Prevent emit() after close() with clear RuntimeError
  • Make close() idempotent (safe to call multiple times)
  • Add context manager protocol (__enter__/__exit__) for resource cleanup

Fixes #10378

Fix for issue #10378: ReactiveEventBus._subject (RxPY Subject) is never completed — missing close() method causes RxPY subscriber resource leaks. Changes: - Add `_closed` flag to track bus shutdown state - Prevent `emit()` after `close()` with clear RuntimeError - Make `close()` idempotent (safe to call multiple times) - Add context manager protocol (`__enter__`/`__exit__`) for resource cleanup Fixes #10378
feat(agents): add mandatory PR compliance checklist to implementation-pool-supervisor
Some checks failed
CI / build (pull_request) Successful in 1m12s
CI / lint (pull_request) Successful in 1m25s
CI / helm (pull_request) Successful in 1m12s
CI / quality (pull_request) Successful in 1m35s
CI / typecheck (pull_request) Successful in 1m51s
CI / security (pull_request) Successful in 1m54s
CI / push-validation (pull_request) Successful in 21s
CI / integration_tests (pull_request) Successful in 5m12s
CI / unit_tests (pull_request) Successful in 5m43s
CI / coverage (pull_request) Failing after 1m34s
CI / docker (pull_request) Successful in 2m45s
CI / status-check (pull_request) Failing after 4s
3777eb749e
Add CHANGELOG.md and CONTRIBUTORS.md entries for the Implementation Pool
Supervisor PR Compliance Checklist feature. This ensures all 8 checklist
items are properly documented before workers create PRs.

ISSUES CLOSED: #11015
fix(events): add close() method to ReactiveEventBus to complete RxPY subject
Some checks failed
CI / helm (pull_request) Successful in 48s
CI / lint (pull_request) Failing after 1m22s
CI / quality (pull_request) Successful in 1m27s
CI / build (pull_request) Successful in 1m17s
CI / push-validation (pull_request) Successful in 1m3s
CI / typecheck (pull_request) Successful in 1m53s
CI / security (pull_request) Successful in 1m56s
CI / integration_tests (pull_request) Successful in 4m4s
CI / unit_tests (pull_request) Successful in 5m15s
CI / docker (pull_request) Has been skipped
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s
0f967e04e2
- Add _closed flag to track bus shutdown state
- Prevent emit() after close() with clear RuntimeError
- Make close() idempotent (safe to call multiple times)
- Add context manager protocol (__enter__/__exit__) for resource cleanup

Fixes issue #10378: missing close() method causes RxPY subscriber leaks
freemo added this to the v3.2.0 milestone 2026-05-13 06:03:48 +00:00
HAL9001 left a comment

Review Summary

This PR addresses issue #10378 and the underlying fix in ReactiveEventBus is correct and well-designed. The close() method, _closed flag, idempotency guard, RuntimeError on post-close emit(), and the context manager protocol (__enter__/__exit__) are all implemented correctly and match the acceptance criteria.

However, there are several blocking issues that must be resolved before this PR can be approved and merged:

Blocking Issues

  1. CI lint gate is FAILINGCI / lint failed (1m22s, failing). Per company policy, all required CI gates must be green before merge. The lint failure also causes CI / coverage to be skipped, so coverage compliance cannot be confirmed.

  2. Non-atomic commit history — The PR contains two commits related to this fix:

    • 3459f821 — the main fix, which accidentally removes exc_info=True from the event_handler_failed logging block
    • 4099872a — a follow-up commit immediately restoring exc_info=True
      Per CONTRIBUTING.md, each commit must be atomic and independently correct. A commit that introduces a regression, immediately followed by a fixup commit, is not acceptable in merged history. These must be squashed (via interactive rebase) into a single, clean commit before merge.
  3. Branch name does not follow conventions — The PR head branch is PR-fix-wf18. CONTRIBUTING.md requires bug fix branches to follow bugfix/mN-<descriptive-name> format (e.g. bugfix/m2-reactive-event-bus-close). Additionally, the branch does not match the Branch: field in the issue #10378 Metadata section (fix/reactive-event-bus-close-method), which is also non-conforming.

  4. PR is stale with merge conflicts — The PR is marked as stale with conflicts against master and cannot be merged. The branch must be rebased onto the current master before this PR can land.

  5. Coverage check is skipped — Due to the lint failure, CI / coverage was skipped. Coverage compliance (≥97%) cannot be confirmed until lint passes and coverage runs to completion.

Likely Root Cause of Lint Failure

In features/steps/event_bus_steps.py, the new step @given("a ReactiveEventBus used as a context manager") assigns ctx.cm_bus but the variable is never subsequently read in any step. This is likely triggering a ruff "assigned but never used" or similar warning/error. Fix: either remove the ctx.cm_bus assignment entirely (the with ctx.bus as bus: block is sufficient to exercise __enter__), or use it in the @then step.

Code Quality

Aside from the blocking issues above, the code quality is good:

  • reactive.py implementation is correct, clean, well-documented, and type-annotated
  • BDD scenarios in event_bus.feature and tdd_reactive_event_bus_close.feature cover all acceptance criteria
  • tdd_reactive_event_bus_close_steps.py is thorough and uses proper Behave idioms
  • The @tdd_issue_10377 tag is correctly applied per the TDD bug fix workflow
  • __exit__ is correctly typed with TracebackType from types
  • All public methods have docstrings

Required Actions Before Re-Review

  1. Fix the lint issue (likely ctx.cm_bus unused variable in event_bus_steps.py)
  2. Squash the two events-related commits into one clean atomic commit
  3. Rebase the branch onto current master (resolve all conflicts)
  4. Ensure nox passes locally (all sessions green, coverage ≥97%)
  5. Rename the branch to follow bugfix/m2-reactive-event-bus-close convention

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Review Summary This PR addresses issue #10378 and the underlying fix in `ReactiveEventBus` is **correct and well-designed**. The `close()` method, `_closed` flag, idempotency guard, `RuntimeError` on post-close `emit()`, and the context manager protocol (`__enter__`/`__exit__`) are all implemented correctly and match the acceptance criteria. However, there are several **blocking issues** that must be resolved before this PR can be approved and merged: ### Blocking Issues 1. **CI lint gate is FAILING** — `CI / lint` failed (1m22s, failing). Per company policy, all required CI gates must be green before merge. The lint failure also causes `CI / coverage` to be skipped, so coverage compliance cannot be confirmed. 2. **Non-atomic commit history** — The PR contains two commits related to this fix: - `3459f821` — the main fix, which accidentally removes `exc_info=True` from the `event_handler_failed` logging block - `4099872a` — a follow-up commit immediately restoring `exc_info=True` Per CONTRIBUTING.md, each commit must be atomic and independently correct. A commit that introduces a regression, immediately followed by a fixup commit, is not acceptable in merged history. These must be squashed (via interactive rebase) into a single, clean commit before merge. 3. **Branch name does not follow conventions** — The PR head branch is `PR-fix-wf18`. CONTRIBUTING.md requires bug fix branches to follow `bugfix/mN-<descriptive-name>` format (e.g. `bugfix/m2-reactive-event-bus-close`). Additionally, the branch does not match the `Branch:` field in the issue #10378 Metadata section (`fix/reactive-event-bus-close-method`), which is also non-conforming. 4. **PR is stale with merge conflicts** — The PR is marked as stale with conflicts against master and cannot be merged. The branch must be rebased onto the current master before this PR can land. 5. **Coverage check is skipped** — Due to the lint failure, `CI / coverage` was skipped. Coverage compliance (≥97%) cannot be confirmed until lint passes and coverage runs to completion. ### Likely Root Cause of Lint Failure In `features/steps/event_bus_steps.py`, the new step `@given("a ReactiveEventBus used as a context manager")` assigns `ctx.cm_bus` but the variable is never subsequently read in any step. This is likely triggering a ruff "assigned but never used" or similar warning/error. Fix: either remove the `ctx.cm_bus` assignment entirely (the `with ctx.bus as bus:` block is sufficient to exercise `__enter__`), or use it in the `@then` step. ### Code Quality Aside from the blocking issues above, the code quality is good: - `reactive.py` implementation is correct, clean, well-documented, and type-annotated - BDD scenarios in `event_bus.feature` and `tdd_reactive_event_bus_close.feature` cover all acceptance criteria - `tdd_reactive_event_bus_close_steps.py` is thorough and uses proper Behave idioms - The `@tdd_issue_10377` tag is correctly applied per the TDD bug fix workflow - `__exit__` is correctly typed with `TracebackType` from `types` - All public methods have docstrings ### Required Actions Before Re-Review 1. Fix the lint issue (likely `ctx.cm_bus` unused variable in `event_bus_steps.py`) 2. Squash the two events-related commits into one clean atomic commit 3. Rebase the branch onto current master (resolve all conflicts) 4. Ensure `nox` passes locally (all sessions green, coverage ≥97%) 5. Rename the branch to follow `bugfix/m2-reactive-event-bus-close` convention --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKING — Likely lint failure root cause

ctx.cm_bus is assigned on this line (and updated inside the with block on the next line) but is never read in any subsequent @then step. ruff will flag this as an unused variable assignment, which is likely why CI / lint is failing.

How to fix: Remove the ctx.cm_bus assignment entirely. The with ctx.bus as bus: block is sufficient to exercise __enter__ — you do not need to capture the returned value since the @then("the bus should be closed after the context exits") step only checks ctx.bus._closed, not ctx.cm_bus.

@given("a ReactiveEventBus used as a context manager")
def step_given_bus_context_manager(ctx: Context) -> None:
    ctx.bus = ReactiveEventBus()
    with ctx.bus:  # __enter__ is exercised; no need to capture return value
        pass

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Likely lint failure root cause** `ctx.cm_bus` is assigned on this line (and updated inside the `with` block on the next line) but is never read in any subsequent `@then` step. ruff will flag this as an unused variable assignment, which is likely why `CI / lint` is failing. **How to fix:** Remove the `ctx.cm_bus` assignment entirely. The `with ctx.bus as bus:` block is sufficient to exercise `__enter__` — you do not need to capture the returned value since the `@then("the bus should be closed after the context exits")` step only checks `ctx.bus._closed`, not `ctx.cm_bus`. ```python @given("a ReactiveEventBus used as a context manager") def step_given_bus_context_manager(ctx: Context) -> None: ctx.bus = ReactiveEventBus() with ctx.bus: # __enter__ is exercised; no need to capture return value pass ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKING — Accidental removal of exc_info=True in this commit

This commit (3459f821) removes exc_info=True from the event_handler_failed logger call. A follow-up commit (4099872a) immediately restores it, citing that "The exc_info=True parameter was accidentally removed from the event_handler_failed logging block during a previous formatting cleanup."

Per CONTRIBUTING.md, commits must be atomic — each commit must be independently correct and not introduce a regression that is then immediately fixed by the very next commit. This two-commit sequence must be squashed into a single clean commit via interactive rebase (git rebase -i HEAD~2) before the PR can be approved.

The final state of the file is correct — exc_info=True is present at HEAD. This is not a code quality issue in the resulting code, only a commit hygiene issue.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Accidental removal of `exc_info=True` in this commit** This commit (`3459f821`) removes `exc_info=True` from the `event_handler_failed` logger call. A follow-up commit (`4099872a`) immediately restores it, citing that "The exc_info=True parameter was accidentally removed from the event_handler_failed logging block during a previous formatting cleanup." Per CONTRIBUTING.md, commits must be **atomic** — each commit must be independently correct and not introduce a regression that is then immediately fixed by the very next commit. This two-commit sequence must be squashed into a single clean commit via interactive rebase (`git rebase -i HEAD~2`) before the PR can be approved. The final state of the file is correct — `exc_info=True` is present at HEAD. This is not a code quality issue in the resulting code, only a commit hygiene issue. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

Peer review completed. Formal review submitted with REQUEST_CHANGES status (review ID 8749).

This PR addresses a real and important bug — the ReactiveEventBus fix itself is correct. However, there are 5 blocking issues that must be resolved before approval:

  1. CI / lint is failing — likely caused by an unused ctx.cm_bus variable in event_bus_steps.py
  2. Coverage check is skipped (blocked by lint failure) — cannot confirm ≥97%
  3. Non-atomic commitsexc_info=True was accidentally removed in commit 3459f821 and restored in 4099872a; these must be squashed
  4. Branch name PR-fix-wf18 does not follow bugfix/mN-<name> convention
  5. Stale with merge conflicts — branch must be rebased onto current master

See the inline review comments for specific fix guidance.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

Peer review completed. Formal review submitted with **REQUEST_CHANGES** status (review ID 8749). This PR addresses a real and important bug — the `ReactiveEventBus` fix itself is correct. However, there are 5 blocking issues that must be resolved before approval: 1. **CI / lint is failing** — likely caused by an unused `ctx.cm_bus` variable in `event_bus_steps.py` 2. **Coverage check is skipped** (blocked by lint failure) — cannot confirm ≥97% 3. **Non-atomic commits** — `exc_info=True` was accidentally removed in commit `3459f821` and restored in `4099872a`; these must be squashed 4. **Branch name** `PR-fix-wf18` does not follow `bugfix/mN-<name>` convention 5. **Stale with merge conflicts** — branch must be rebased onto current master See the inline review comments for specific fix guidance. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Some checks failed
CI / helm (pull_request) Successful in 48s
CI / lint (pull_request) Failing after 1m22s
Required
Details
CI / quality (pull_request) Successful in 1m27s
Required
Details
CI / build (pull_request) Successful in 1m17s
Required
Details
CI / push-validation (pull_request) Successful in 1m3s
CI / typecheck (pull_request) Successful in 1m53s
Required
Details
CI / security (pull_request) Successful in 1m56s
Required
Details
CI / integration_tests (pull_request) Successful in 4m4s
Required
Details
CI / unit_tests (pull_request) Successful in 5m15s
Required
Details
CI / docker (pull_request) Has been skipped
Required
Details
CI / coverage (pull_request) Has been skipped
Required
Details
CI / status-check (pull_request) Failing after 3s
This pull request has changes conflicting with the target branch.
  • src/cleveragents/infrastructure/events/reactive.py
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin PR-fix-wf18:PR-fix-wf18
git switch PR-fix-wf18
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!11190
No description provided.