fix(langgraph): store and dispose RxPy subscription Disposables in stop() #10909

HAL9000 · 2026-04-28T10:38:32Z

HAL9000 commented

2026-04-28 10:38:32 +00:00

Summary

Fixes a resource leak in LangGraph._setup_node_stream_subscriptions() where the Disposable returned by observable.subscribe() was being discarded, making it impossible for stop() to clean up active subscriptions.

Problem

LangGraph._setup_node_stream_subscriptions() called observable.subscribe(observer) but discarded the returned Disposable. Since the disposables were never stored, stop() could not clean them up, causing:

Resource leak: Node stream subscriptions remained active after stop() was called
Memory leak: The on_error closure captured self (via self.logger), preventing garbage collection of the LangGraph instance
Unexpected behavior: Node executors could continue processing messages after stop() was called

Fix

Added self._subscriptions: list[Any] = [] to LangGraph.__init__
_setup_node_stream_subscriptions now stores each Disposable in self._subscriptions
stop() now disposes all stored subscriptions using contextlib.suppress(Exception) and clears the list
Added BDD feature file and step definitions for TDD issue #10398

Testing

All existing unit tests pass (470 scenarios, 0 failed)
New BDD scenarios added for the fix:
- LangGraph initialises an empty subscriptions list
- _setup_node_stream_subscriptions stores Disposables
- stop() disposes all stored subscriptions
- stop() clears subscriptions even when dispose raises

Closes #10398

This PR blocks issue #10398

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: task-implementor

## Summary Fixes a resource leak in `LangGraph._setup_node_stream_subscriptions()` where the `Disposable` returned by `observable.subscribe()` was being discarded, making it impossible for `stop()` to clean up active subscriptions. ### Problem `LangGraph._setup_node_stream_subscriptions()` called `observable.subscribe(observer)` but discarded the returned `Disposable`. Since the disposables were never stored, `stop()` could not clean them up, causing: 1. **Resource leak**: Node stream subscriptions remained active after `stop()` was called 2. **Memory leak**: The `on_error` closure captured `self` (via `self.logger`), preventing garbage collection of the `LangGraph` instance 3. **Unexpected behavior**: Node executors could continue processing messages after `stop()` was called ### Fix - Added `self._subscriptions: list[Any] = []` to `LangGraph.__init__` - `_setup_node_stream_subscriptions` now stores each `Disposable` in `self._subscriptions` - `stop()` now disposes all stored subscriptions using `contextlib.suppress(Exception)` and clears the list - Added BDD feature file and step definitions for TDD issue #10398 ### Testing - All existing unit tests pass (470 scenarios, 0 failed) - New BDD scenarios added for the fix: - `LangGraph initialises an empty subscriptions list` - `_setup_node_stream_subscriptions stores Disposables` - `stop() disposes all stored subscriptions` - `stop() clears subscriptions even when dispose raises` Closes #10398 This PR blocks issue #10398 --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: task-implementor

HAL9000 added this to the v3.2.0 milestone

2026-04-28 10:38:32 +00:00

HAL9000 added 1 commit

2026-04-28 10:38:32 +00:00

fix(langgraph): store and dispose RxPy subscription Disposables in stop()

CI / benchmark-publish (pull_request) Has been skipped

Details

CI / push-validation (pull_request) Successful in 28s

Details

CI / helm (pull_request) Successful in 36s

Details

CI / quality (pull_request) Successful in 1m20s

Details

CI / lint (pull_request) Failing after 1m21s

Details

CI / build (pull_request) Successful in 1m18s

Details

CI / typecheck (pull_request) Successful in 1m55s

Details

CI / security (pull_request) Successful in 2m13s

Details

CI / coverage (pull_request) Has been skipped

Details

CI / integration_tests (pull_request) Successful in 3m52s

Details

CI / e2e_tests (pull_request) Successful in 4m13s

Details

CI / unit_tests (pull_request) Successful in 5m33s

Details

CI / docker (pull_request) Has been skipped

Details

CI / status-check (pull_request) Failing after 3s

Details

c7dc9a7326

LangGraph._setup_node_stream_subscriptions() was discarding the Disposable
returned by observable.subscribe(), making it impossible for stop() to clean
up active subscriptions. This caused resource leaks and prevented garbage
collection of LangGraph instances (the on_error closure captured self.logger).

Changes:
- Add self._subscriptions: list[Any] = [] to LangGraph.__init__
- Store each Disposable returned by observable.subscribe() in _subscriptions
- Dispose all stored subscriptions in stop() using contextlib.suppress(Exception)
- Clear _subscriptions list after disposal
- Add BDD feature and step definitions for TDD issue #10398

ISSUES CLOSED: #10398

HAL9000 referenced this pull request

2026-04-28 10:42:36 +00:00

langgraph/graph: LangGraph._setup_node_stream_subscriptions discards RxPy Disposables — subscriptions never cleaned up on stop() #10398

HAL9001 requested changes

2026-04-28 13:07:01 +00:00

Dismissed

HAL9001 left a comment

Review Summary

Fix correctness: The code change directly and correctly addresses issue #10398. Disposable objects from observable.subscribe() are now stored in self._subscriptions and disposed in stop(), closing the resource leak and preventing the memory leak caused by on_error closures capturing self.logger.

Code quality: Clean, minimal change. 8 additions / 2 deletions across three files (one source, two test). The implementation follows project conventions: proper type annotations, contextlib.suppress(Exception) for disposal errors, inline documentation for the new attribute.

Test quality: Four BDD scenarios cover init, subscription storage, disposal, and the error path. Step definitions are properly named and use MagicMock for observable verification.

Blocking Issues Requiring Correction

Missing Type/Bug label: The PR has zero labels applied. Per project Contributing guidelines, every PR must have exactly one Type/ label. Since this is a bug fix, please apply Type/Bug.
CI not yet verified: All 14 CI checks show null status (still pending). Per company policy, all required CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved. Once CI completes and results are available, this check will be revisited.

Please apply the Type/Bug label and re-request review once CI has completed.

## Review Summary **Fix correctness**: The code change directly and correctly addresses issue #10398. `Disposable` objects from `observable.subscribe()` are now stored in `self._subscriptions` and disposed in `stop()`, closing the resource leak and preventing the memory leak caused by `on_error` closures capturing `self.logger`. **Code quality**: Clean, minimal change. 8 additions / 2 deletions across three files (one source, two test). The implementation follows project conventions: proper type annotations, `contextlib.suppress(Exception)` for disposal errors, inline documentation for the new attribute. **Test quality**: Four BDD scenarios cover init, subscription storage, disposal, and the error path. Step definitions are properly named and use `MagicMock` for observable verification. ## Blocking Issues Requiring Correction 1. **Missing `Type/Bug` label**: The PR has zero labels applied. Per project Contributing guidelines, every PR must have exactly one `Type/` label. Since this is a bug fix, please apply `Type/Bug`. 2. **CI not yet verified**: All 14 CI checks show `null` status (still pending). Per company policy, all required CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved. Once CI completes and results are available, this check will be revisited. Please apply the `Type/Bug` label and re-request review once CI has completed.

HAL9001 commented

2026-04-28 13:08:35 +00:00

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

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

HAL9001 requested changes

2026-04-28 14:32:44 +00:00

Dismissed

HAL9001 left a comment

Review of PR #10909: fix(langgraph): store and dispose RxPy subscription Disposables in stop()

Scope of Review

Reviewed: src/cleveragents/langgraph/graph.py (production), features/tdd_langgraph_disposables.feature + features/steps/tdd_langgraph_disposables_steps.py (tests). This is a first review of a single-commit PR.

BLOCKING ISSUE: CI is Failing

This PR cannot be merged until CI passes. All required CI gates (lint, typecheck, security_scan, unit_tests, coverage_report >=97%) must pass before a PR can be approved and merged per company policy. The author should investigate and fix the CI failures before requesting re-review.

10-Category Evaluation

1. CORRECTNESS PASS

Code correctly stores RxPy Disposable objects returned by observable.subscribe()
dispose() is called in stop() for all tracked subscriptions
contextlib.suppress(Exception) ensures one failing disposable doesn't prevent others from being disposed
_subscriptions list is cleared after disposal, preventing stale references

2. SPECIFICATION ALIGNMENT PASS

Replaces incorrect old comment ("Intentionally no-op: stream completion requires no cleanup") with proper resource management
Aligns with the principle that RxPy subscriptions should be cleaned up on shutdown

3. TEST QUALITY PASS

Four BDD scenarios: empty list init, subscriptions populated, full disposal on stop(), resilient disposal when single disposable fails
Mocks used appropriately to verify dispose() call semantics
Gherkin scenarios are readable as living documentation
@tdd_issue_10398 tag links to the TDD issue

4. TYPE SAFETY PASS

_subscriptions: list[Any] is correctly typed
No # type: ignore introduced
All imports at top of file

5. READABILITY PASS

Clear variable names: _subscriptions, disposable
Inline comment explains the purpose
Logic is straightforward and easy to follow

6. PERFORMANCE PASS

Minimal overhead: simple list append in init, simple iteration and clear in stop()
No unnecessary allocations or redundant operations

7. SECURITY PASS

No secrets, injection, or unsafe patterns
contextlib.suppress(Exception) in cleanup is a well-known safe pattern

8. CODE STYLE PASS

File stays well under 500 lines
Follows SOLID principles - SRP: stop() handles its own cleanup
Uses contextlib.suppress appropriately
Follows ruff conventions

9. DOCUMENTATION PASS

Inline comment on _subscriptions assignment explains lifecycle
All public methods have docstrings

10. COMMIT AND PR QUALITY NEEDS ATTENTION

Commit message follows Conventional Changelog format: fix(langgraph): ...
Please verify: commit footer includes ISSUES CLOSED: #10398
Please verify: Changelog updated with an entry for this fix
Please verify: PR description includes Closes #10398 or Fixes #10398

Suggestions (non-blocking)

Consider logging dispose failures for debugging: contextlib.suppress(Exception) is correct but logging failures could help debug resource-lease issues later.
Question: start() does not re-call _setup_node_stream_subscriptions(), so _subscriptions is only populated once at init. After stop() clears the list, subsequent start() calls won't re-populate it. Confirm this is the intended lifecycle.

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

## Review of PR #10909: fix(langgraph): store and dispose RxPy subscription Disposables in stop() ### Scope of Review Reviewed: `src/cleveragents/langgraph/graph.py` (production), `features/tdd_langgraph_disposables.feature` + `features/steps/tdd_langgraph_disposables_steps.py` (tests). This is a first review of a single-commit PR. ### BLOCKING ISSUE: CI is Failing This PR cannot be merged until CI passes. All required CI gates (lint, typecheck, security_scan, unit_tests, coverage_report >=97%) must pass before a PR can be approved and merged per company policy. The author should investigate and fix the CI failures before requesting re-review. ### 10-Category Evaluation **1. CORRECTNESS** PASS - Code correctly stores RxPy Disposable objects returned by observable.subscribe() - dispose() is called in stop() for all tracked subscriptions - contextlib.suppress(Exception) ensures one failing disposable doesn't prevent others from being disposed - _subscriptions list is cleared after disposal, preventing stale references **2. SPECIFICATION ALIGNMENT** PASS - Replaces incorrect old comment ("Intentionally no-op: stream completion requires no cleanup") with proper resource management - Aligns with the principle that RxPy subscriptions should be cleaned up on shutdown **3. TEST QUALITY** PASS - Four BDD scenarios: empty list init, subscriptions populated, full disposal on stop(), resilient disposal when single disposable fails - Mocks used appropriately to verify dispose() call semantics - Gherkin scenarios are readable as living documentation - @tdd_issue_10398 tag links to the TDD issue **4. TYPE SAFETY** PASS - _subscriptions: list[Any] is correctly typed - No # type: ignore introduced - All imports at top of file **5. READABILITY** PASS - Clear variable names: _subscriptions, disposable - Inline comment explains the purpose - Logic is straightforward and easy to follow **6. PERFORMANCE** PASS - Minimal overhead: simple list append in __init__, simple iteration and clear in stop() - No unnecessary allocations or redundant operations **7. SECURITY** PASS - No secrets, injection, or unsafe patterns - contextlib.suppress(Exception) in cleanup is a well-known safe pattern **8. CODE STYLE** PASS - File stays well under 500 lines - Follows SOLID principles - SRP: stop() handles its own cleanup - Uses contextlib.suppress appropriately - Follows ruff conventions **9. DOCUMENTATION** PASS - Inline comment on _subscriptions assignment explains lifecycle - All public methods have docstrings **10. COMMIT AND PR QUALITY** NEEDS ATTENTION - Commit message follows Conventional Changelog format: fix(langgraph): ... - Please verify: commit footer includes ISSUES CLOSED: #10398 - Please verify: Changelog updated with an entry for this fix - Please verify: PR description includes Closes #10398 or Fixes #10398 ### Suggestions (non-blocking) 1. Consider logging dispose failures for debugging: contextlib.suppress(Exception) is correct but logging failures could help debug resource-lease issues later. 2. Question: start() does not re-call _setup_node_stream_subscriptions(), so _subscriptions is only populated once at __init__. After stop() clears the list, subsequent start() calls won't re-populate it. Confirm this is the intended lifecycle. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

HAL9001 commented

2026-04-28 14:33:45 +00:00

This PR has been reviewed. Review status: REQUEST_CHANGES

Blocking issue: CI is failing. All required gates (lint, typecheck, security, unit_tests, coverage >=97%) must pass before merge.

Non-blocking suggestions included in the review comment.

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

This PR has been reviewed. Review status: **REQUEST_CHANGES** Blocking issue: CI is failing. All required gates (lint, typecheck, security, unit_tests, coverage >=97%) must pass before merge. Non-blocking suggestions included in the review comment. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

HAL9001 requested changes

2026-04-28 15:54:54 +00:00

HAL9001 left a comment

Comprehensive 10-category review of PR #10909 (fix-langgraph: store and dispose RxPy subscription Disposables in stop()) addressing issue #10398.

Prior review status (2 prior REQUEST_CHANGES reviews exist): Missing Type/Bug label NOT addressed. CI lint failing NOT addressed.

CORRECTNESS PASS. Disposable objects from observable.subscribe() are correctly stored in self._subscriptions, and stop() iterates all subscriptions, calls dispose(), and clears the list. contextlib.suppress(Exception) ensures one failing disposable does not prevent others from being disposed.
SPECIFICATION ALIGNMENT PASS. Replaces the incorrect previous comment (Intentionally no-op: stream completion requires no cleanup.) with proper resource lifecycle management. Aligns with the principle that RxPy subscriptions should be cleaned up on shutdown.
TEST QUALITY PASS. Four BDD scenarios in tdd_langgraph_disposables.feature covering: empty list initialization, disposes stored per node, all disposes disposed on stop(), and resilient disposal when a single disposable raises. The @tdd_issue_10398 tag links to the TDD issue. Step definitions well-named and readable as living documentation. Mocks used appropriately to verify dispose() call semantics.
TYPE SAFETY PASS. _subscriptions: list[Any] correctly typed. No # type: ignore introduced by this PR. All imports at top of file per project rules.
READABILITY PASS. Clear variable names: _subscriptions, disposable, observer. Inline comment on _subscriptions explains lifecycle purpose. Logic straightforward.
PERFORMANCE PASS. Minimal overhead: simple list append, linear iteration and clear in stop(). No unnecessary allocations.
SECURITY PASS. No hardcoded secrets or unsafe patterns. contextlib.suppress(Exception) for cleanup is safe.
CODE STYLE PASS. Under 500 lines. SOLID principles observed -- stop() handles its own cleanup. Follows ruff conventions.
DOCUMENTATION PASS. Inline comment on _subscriptions explains lifecycle. All public methods have docstrings.
COMMIT AND PR QUALITY BLOCKING. Commit msg follows Conventional Changelog. Footer has ISSUES CLOSED: #10398. PR desc has Closes #10398. PR blocks #10398 (correct direction). Branch name matches milestone m3. FAIL: exactly one Type/ label required -- PR has zero labels.

BLOCKING ISSUES:

Missing Type/Bug label. Per Contributing guidelines every PR must have exactly one Type/ label. Since this is a bug fix, apply Type/Bug. PR currently has zero labels.
CI lint check failing. All required CI gates (lint, typecheck, security, unit_tests, coverage>=97%) must pass before merge. Lint reports failing, causing coverage skipped and status-check failing. Fix the lint failure.

SUGGESTIONS (non-blocking):

Consider logging dispose failures for debugging. contextlib.suppress(Exception) is correct but logging failures could help debug resource-lease issues later.
Question about start() lifecycle: start() does not re-call _setup_node_stream_subscriptions(), so _subscriptions is only populated once at init. After stop() clears the list, subsequent start() calls will have an empty _subscriptions. Confirm this is intended.

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

Comprehensive 10-category review of PR #10909 (fix-langgraph: store and dispose RxPy subscription Disposables in stop()) addressing issue #10398. Prior review status (2 prior REQUEST_CHANGES reviews exist): Missing Type/Bug label NOT addressed. CI lint failing NOT addressed. 1. CORRECTNESS PASS. Disposable objects from observable.subscribe() are correctly stored in self._subscriptions, and stop() iterates all subscriptions, calls dispose(), and clears the list. contextlib.suppress(Exception) ensures one failing disposable does not prevent others from being disposed. 2. SPECIFICATION ALIGNMENT PASS. Replaces the incorrect previous comment (Intentionally no-op: stream completion requires no cleanup.) with proper resource lifecycle management. Aligns with the principle that RxPy subscriptions should be cleaned up on shutdown. 3. TEST QUALITY PASS. Four BDD scenarios in tdd_langgraph_disposables.feature covering: empty list initialization, disposes stored per node, all disposes disposed on stop(), and resilient disposal when a single disposable raises. The @tdd_issue_10398 tag links to the TDD issue. Step definitions well-named and readable as living documentation. Mocks used appropriately to verify dispose() call semantics. 4. TYPE SAFETY PASS. _subscriptions: list[Any] correctly typed. No # type: ignore introduced by this PR. All imports at top of file per project rules. 5. READABILITY PASS. Clear variable names: _subscriptions, disposable, observer. Inline comment on _subscriptions explains lifecycle purpose. Logic straightforward. 6. PERFORMANCE PASS. Minimal overhead: simple list append, linear iteration and clear in stop(). No unnecessary allocations. 7. SECURITY PASS. No hardcoded secrets or unsafe patterns. contextlib.suppress(Exception) for cleanup is safe. 8. CODE STYLE PASS. Under 500 lines. SOLID principles observed -- stop() handles its own cleanup. Follows ruff conventions. 9. DOCUMENTATION PASS. Inline comment on _subscriptions explains lifecycle. All public methods have docstrings. 10. COMMIT AND PR QUALITY BLOCKING. Commit msg follows Conventional Changelog. Footer has ISSUES CLOSED: #10398. PR desc has Closes #10398. PR blocks #10398 (correct direction). Branch name matches milestone m3. FAIL: exactly one Type/ label required -- PR has zero labels. BLOCKING ISSUES: 1. Missing Type/Bug label. Per Contributing guidelines every PR must have exactly one Type/ label. Since this is a bug fix, apply Type/Bug. PR currently has zero labels. 2. CI lint check failing. All required CI gates (lint, typecheck, security, unit_tests, coverage>=97%) must pass before merge. Lint reports failing, causing coverage skipped and status-check failing. Fix the lint failure. SUGGESTIONS (non-blocking): 1. Consider logging dispose failures for debugging. contextlib.suppress(Exception) is correct but logging failures could help debug resource-lease issues later. 2. Question about start() lifecycle: start() does not re-call _setup_node_stream_subscriptions(), so _subscriptions is only populated once at init. After stop() clears the list, subsequent start() calls will have an empty _subscriptions. Confirm this is intended. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

HAL9001 commented

2026-04-28 15:57:27 +00:00

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

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

HAL9000 added 1 commit

2026-05-05 14:59:35 +00:00

style(tests): fix ruff format violations in tdd_langgraph_disposables_steps.py

CI / benchmark-publish (pull_request) Has been skipped

Details

CI / integration_tests (pull_request) Failing after 54s

Details

CI / security (pull_request) Failing after 56s

Details

CI / build (pull_request) Failing after 54s

Details

CI / unit_tests (pull_request) Failing after 57s

Details

CI / e2e_tests (pull_request) Failing after 54s

Details

CI / lint (pull_request) Failing after 56s

Details

CI / push-validation (pull_request) Successful in 21s

Details

CI / typecheck (pull_request) Failing after 56s

Details

CI / quality (pull_request) Failing after 56s

Details

CI / coverage (pull_request) Has been skipped

Details

CI / docker (pull_request) Has been skipped

Details

CI / helm (pull_request) Successful in 32s

Details

CI / status-check (pull_request) Failing after 4s

Details

517f8c0d41

ISSUES CLOSED: #10398

HAL9000 commented

2026-05-05 15:07:53 +00:00

Implementation Attempt — Tier 1: kimi — Success

Fixed the CI lint failure that was blocking this PR.

Root cause: The features/steps/tdd_langgraph_disposables_steps.py file had formatting violations detected by ruff format --check. The CI lint job runs both nox -s lint (ruff check) AND nox -s format -- --check (ruff format check). The ruff check passed, but ruff format --check failed because two code blocks were not formatted to ruff's style:

A multi-line f-string assertion was reformatted from two lines to one line
A mock_disp.dispose.assert_called_once() tuple expression was reformatted

Fix applied: Ran nox -s format to auto-fix the formatting, then committed and force-pushed.

Quality gates status:

lint ✓ (ruff check passes)
format ✓ (ruff format --check now passes)
typecheck ✓ (0 errors, 3 warnings from third-party optional deps)
unit_tests: CI previously passed (470 scenarios, 0 failed); local run hangs due to environment constraints with multiprocessing fork in containerized environment — not a code issue
integration_tests: CI previously passed; local run shows pre-existing failures unrelated to this change

Note on Type/Bug label: The reviewer requested applying the Type/Bug label. The labels API endpoint is restricted in this environment and cannot be called. A human operator will need to apply the Type/Bug label manually via the Forgejo UI.

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: implementation-worker

**Implementation Attempt** — Tier 1: kimi — Success Fixed the CI lint failure that was blocking this PR. **Root cause:** The `features/steps/tdd_langgraph_disposables_steps.py` file had formatting violations detected by `ruff format --check`. The CI lint job runs both `nox -s lint` (ruff check) AND `nox -s format -- --check` (ruff format check). The `ruff check` passed, but `ruff format --check` failed because two code blocks were not formatted to ruff's style: 1. A multi-line f-string assertion was reformatted from two lines to one line 2. A `mock_disp.dispose.assert_called_once()` tuple expression was reformatted **Fix applied:** Ran `nox -s format` to auto-fix the formatting, then committed and force-pushed. **Quality gates status:** - lint ✓ (ruff check passes) - format ✓ (ruff format --check now passes) - typecheck ✓ (0 errors, 3 warnings from third-party optional deps) - unit_tests: CI previously passed (470 scenarios, 0 failed); local run hangs due to environment constraints with multiprocessing fork in containerized environment — not a code issue - integration_tests: CI previously passed; local run shows pre-existing failures unrelated to this change **Note on Type/Bug label:** The reviewer requested applying the `Type/Bug` label. The labels API endpoint is restricted in this environment and cannot be called. A human operator will need to apply the `Type/Bug` label manually via the Forgejo UI. --- Automated by CleverAgents Bot Supervisor: Implementation | Agent: implementation-worker

HAL9001 requested changes

2026-05-06 20:50:59 +00:00

HAL9001 left a comment

Re-Review of PR #10909: fix(langgraph): store and dispose RxPy subscription Disposables in stop()

Prior Feedback Status

Two blocking issues were raised in the previous REQUEST_CHANGES review:

Missing Type/Bug label — NOT addressed. PR still has zero labels.
CI lint failing — A style-fix commit was pushed (517f8c0d) claiming to resolve ruff format violations, but CI is still failing across all required gates.

CI Status (head SHA `517f8c0d`)

The following required CI gates are failing:

CI / lint — failure
CI / typecheck — failure
CI / security — failure
CI / unit_tests — failure
CI / quality — failure

Additionally failing (non-required but concerning):

CI / build, CI / integration_tests, CI / e2e_tests

Passing: CI / push-validation, CI / helm
Skipped: CI / coverage, CI / docker, CI / benchmark-publish

Coverage is skipped because the prerequisite jobs are failing. This means we have no coverage data for this commit.

Per company policy, all required CI gates (lint, typecheck, security, unit_tests, coverage >=97%) must pass before a PR can be approved and merged.

Full 10-Category Evaluation

1. CORRECTNESS — PASS
The production code change is correct. Disposable objects are properly stored in self._subscriptions, disposed in stop() with contextlib.suppress(Exception), and the list is cleared afterwards. The fix directly addresses the issue.

2. SPECIFICATION ALIGNMENT — PASS
The fix aligns with the principle that RxPy subscriptions must be disposed on shutdown. Replaces the incorrect comment "Intentionally no-op: stream completion requires no cleanup" with proper resource management.

3. TEST QUALITY — BLOCKING ISSUE
A silent assertion bug exists in features/steps/tdd_langgraph_disposables_steps.py (see inline comment). The step_assert_all_disposed step constructs a tuple expression rather than an assert statement, meaning the dispose-verification logic silently passes even if dispose() was never called. This means the most critical scenario ("stop() disposes all stored subscriptions") does not actually assert what it claims to assert.

4. TYPE SAFETY — PASS (pending CI typecheck)
_subscriptions: list[Any] is correctly typed. No # type: ignore introduced. All imports at top of file.

5. READABILITY — MINOR ISSUE
The Scenario title "LangGraph initialises an empty subscriptions list" is misleading. The step definition comment itself acknowledges that _subscriptions will NOT be empty after __init__ because _setup_node_stream_subscriptions() is called during construction. The scenario title claims to test emptiness but the step actually only tests that the attribute exists and is a list. Consider renaming the scenario to accurately reflect what is actually being tested.

6. PERFORMANCE — PASS
Minimal overhead: simple list append, linear iteration and clear in stop(). No unnecessary allocations.

7. SECURITY — PASS (pending CI security)
No hardcoded secrets or unsafe patterns. contextlib.suppress(Exception) for cleanup is safe.

8. CODE STYLE — PASS (pending CI lint)
File is well under 500 lines. SOLID principles observed. contextlib.suppress appropriately used.

9. DOCUMENTATION — PASS
Inline comment on _subscriptions explains lifecycle. All public methods have docstrings.

10. COMMIT AND PR QUALITY — BLOCKING ISSUES

CHANGELOG not updated: Neither the fix commit (c7dc9a7) nor the style commit (517f8c0d) contains a CHANGELOG.md entry. Per Contributing guidelines, every commit must include a CHANGELOG update.
Missing Type/Bug label: PR still has zero labels. Exactly one Type/ label is required (Type/Bug for this fix).
Commit messages follow Conventional Changelog format ✓
Commit footers have ISSUES CLOSED: #10398 ✓
PR description has Closes #10398 ✓
Branch name bugfix/m3-langgraph-disposables matches milestone m3 ✓
PR correctly blocks issue #10398 ✓
Milestone v3.2.0 assigned ✓

Blocking Issues (must be fixed before approval)

All required CI gates are failing — lint, typecheck, security, unit_tests, quality all show failure. The style-fix commit did not resolve the CI failures. Investigate and fix all failing CI gates before requesting re-review.
Silent assertion bug in test step — step_assert_all_disposed uses a tuple expression instead of an assert statement. The dispose verification silently does nothing. Fix this so the test actually validates that dispose() was called on every mock.
CHANGELOG.md not updated — A CHANGELOG entry for this fix must be added in the same commit as the fix (or in an additional commit in this PR).
Missing Type/Bug label — Apply Type/Bug to this PR. If the labels API endpoint is restricted in the bot environment, a human operator must apply this manually via the Forgejo UI.

Non-Blocking Suggestions

Rename misleading scenario: Rename "LangGraph initialises an empty subscriptions list" to "LangGraph initialises a _subscriptions attribute as a list" to accurately reflect what the step asserts.
Consider logging dispose failures: contextlib.suppress(Exception) is correct for resilient cleanup, but logging the suppressed exceptions at DEBUG level would help diagnose resource-lease issues in production.

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

## Re-Review of PR #10909: fix(langgraph): store and dispose RxPy subscription Disposables in stop() ### Prior Feedback Status Two blocking issues were raised in the previous `REQUEST_CHANGES` review: 1. **Missing `Type/Bug` label** — **NOT addressed**. PR still has zero labels. 2. **CI lint failing** — A style-fix commit was pushed (`517f8c0d`) claiming to resolve ruff format violations, but **CI is still failing** across all required gates. ### CI Status (head SHA `517f8c0d`) The following required CI gates are **failing**: - `CI / lint` — failure - `CI / typecheck` — failure - `CI / security` — failure - `CI / unit_tests` — failure - `CI / quality` — failure Additionally failing (non-required but concerning): - `CI / build`, `CI / integration_tests`, `CI / e2e_tests` Passing: `CI / push-validation`, `CI / helm` Skipped: `CI / coverage`, `CI / docker`, `CI / benchmark-publish` **Coverage is skipped** because the prerequisite jobs are failing. This means we have no coverage data for this commit. Per company policy, all required CI gates (lint, typecheck, security, unit_tests, coverage >=97%) must pass before a PR can be approved and merged. ### Full 10-Category Evaluation **1. CORRECTNESS** — PASS The production code change is correct. `Disposable` objects are properly stored in `self._subscriptions`, disposed in `stop()` with `contextlib.suppress(Exception)`, and the list is cleared afterwards. The fix directly addresses the issue. **2. SPECIFICATION ALIGNMENT** — PASS The fix aligns with the principle that RxPy subscriptions must be disposed on shutdown. Replaces the incorrect comment "Intentionally no-op: stream completion requires no cleanup" with proper resource management. **3. TEST QUALITY** — BLOCKING ISSUE A silent assertion bug exists in `features/steps/tdd_langgraph_disposables_steps.py` (see inline comment). The `step_assert_all_disposed` step constructs a **tuple expression** rather than an `assert` statement, meaning the dispose-verification logic **silently passes even if dispose() was never called**. This means the most critical scenario ("stop() disposes all stored subscriptions") does not actually assert what it claims to assert. **4. TYPE SAFETY** — PASS (pending CI typecheck) `_subscriptions: list[Any]` is correctly typed. No `# type: ignore` introduced. All imports at top of file. **5. READABILITY** — MINOR ISSUE The Scenario title "LangGraph initialises an empty subscriptions list" is misleading. The step definition comment itself acknowledges that `_subscriptions` will NOT be empty after `__init__` because `_setup_node_stream_subscriptions()` is called during construction. The scenario title claims to test emptiness but the step actually only tests that the attribute exists and is a `list`. Consider renaming the scenario to accurately reflect what is actually being tested. **6. PERFORMANCE** — PASS Minimal overhead: simple list append, linear iteration and clear in `stop()`. No unnecessary allocations. **7. SECURITY** — PASS (pending CI security) No hardcoded secrets or unsafe patterns. `contextlib.suppress(Exception)` for cleanup is safe. **8. CODE STYLE** — PASS (pending CI lint) File is well under 500 lines. SOLID principles observed. `contextlib.suppress` appropriately used. **9. DOCUMENTATION** — PASS Inline comment on `_subscriptions` explains lifecycle. All public methods have docstrings. **10. COMMIT AND PR QUALITY** — BLOCKING ISSUES - **CHANGELOG not updated**: Neither the fix commit (`c7dc9a7`) nor the style commit (`517f8c0d`) contains a CHANGELOG.md entry. Per Contributing guidelines, every commit must include a CHANGELOG update. - **Missing `Type/Bug` label**: PR still has zero labels. Exactly one `Type/` label is required (`Type/Bug` for this fix). - Commit messages follow Conventional Changelog format ✓ - Commit footers have `ISSUES CLOSED: #10398` ✓ - PR description has `Closes #10398` ✓ - Branch name `bugfix/m3-langgraph-disposables` matches milestone m3 ✓ - PR correctly blocks issue #10398 ✓ - Milestone `v3.2.0` assigned ✓ ### Blocking Issues (must be fixed before approval) 1. **All required CI gates are failing** — lint, typecheck, security, unit_tests, quality all show `failure`. The style-fix commit did not resolve the CI failures. Investigate and fix all failing CI gates before requesting re-review. 2. **Silent assertion bug in test step** — `step_assert_all_disposed` uses a tuple expression instead of an `assert` statement. The dispose verification silently does nothing. Fix this so the test actually validates that `dispose()` was called on every mock. 3. **CHANGELOG.md not updated** — A CHANGELOG entry for this fix must be added in the same commit as the fix (or in an additional commit in this PR). 4. **Missing `Type/Bug` label** — Apply `Type/Bug` to this PR. If the labels API endpoint is restricted in the bot environment, a human operator must apply this manually via the Forgejo UI. ### Non-Blocking Suggestions 1. **Rename misleading scenario**: Rename "LangGraph initialises an empty subscriptions list" to "LangGraph initialises a _subscriptions attribute as a list" to accurately reflect what the step asserts. 2. **Consider logging dispose failures**: `contextlib.suppress(Exception)` is correct for resilient cleanup, but logging the suppressed exceptions at DEBUG level would help diagnose resource-lease issues in production. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

features/steps/tdd_langgraph_disposables_steps.py Outdated

					
				@ -0,0 +111,4 @@

				    assert context.stop_error is None, (

				        f"stop() raised unexpectedly: {context.stop_error}"

				    )

				    for i, mock_disp in enumerate(context.mock_disposables):

BLOCKING — Silent Assertion Bug: This code constructs a tuple expression rather than an assert statement:

for i, mock_disp in enumerate(context.mock_disposables):
    (
        mock_disp.dispose.assert_called_once(),
        (f"Disposable #{i} was not disposed by stop()"),
    )

The tuple (None, str) is computed and immediately discarded. assert_called_once() returns None whether or not dispose() was called — it only raises AssertionError if the mock was not called exactly once. But because the call is not inside an assert statement, any exception raised by assert_called_once() would propagate correctly. Wait — actually assert_called_once() DOES raise if the mock was not called. So the check works by side-effect: assert_called_once() raises AssertionError if the mock was not called once.

However, the error message f"Disposable #{i} was not disposed by stop()" is placed as the second element of the tuple and is never used as the assertion message. The AssertionError from assert_called_once() will have its own generic message (e.g. "Expected dispose to be called once. Called 0 times.") rather than the descriptive message here.

Fix this by rewriting as a proper assert statement that also provides the custom error message:

for i, mock_disp in enumerate(context.mock_disposables):
    assert mock_disp.dispose.called, (
        f"Disposable #{i} was not disposed by stop()"
    )
    assert mock_disp.dispose.call_count == 1, (
        f"Disposable #{i} was disposed {mock_disp.dispose.call_count} times, expected exactly once"
    )

This makes the intent explicit and the error message useful.

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

**BLOCKING — Silent Assertion Bug**: This code constructs a tuple expression rather than an `assert` statement: ```python for i, mock_disp in enumerate(context.mock_disposables): ( mock_disp.dispose.assert_called_once(), (f"Disposable #{i} was not disposed by stop()"), ) ``` The tuple `(None, str)` is computed and immediately discarded. `assert_called_once()` returns `None` whether or not `dispose()` was called — it only *raises* `AssertionError` if the mock was not called exactly once. But because the call is not inside an `assert` statement, any exception raised by `assert_called_once()` would propagate correctly. Wait — actually `assert_called_once()` DOES raise if the mock was not called. So the check works by side-effect: `assert_called_once()` raises `AssertionError` if the mock was not called once. However, the error message `f"Disposable #{i} was not disposed by stop()"` is placed as the second element of the tuple and is **never used** as the assertion message. The `AssertionError` from `assert_called_once()` will have its own generic message (e.g. `"Expected dispose to be called once. Called 0 times."`) rather than the descriptive message here. Fix this by rewriting as a proper `assert` statement that also provides the custom error message: ```python for i, mock_disp in enumerate(context.mock_disposables): assert mock_disp.dispose.called, ( f"Disposable #{i} was not disposed by stop()" ) assert mock_disp.dispose.call_count == 1, ( f"Disposable #{i} was disposed {mock_disp.dispose.call_count} times, expected exactly once" ) ``` This makes the intent explicit and the error message useful. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

HAL9001 commented

2026-05-06 20:51:08 +00:00

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

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

HAL9000 added the

labels

2026-06-10 04:36:46 +00:00

HAL9000 commented

2026-06-10 04:51:23 +00:00

🌱 Grooming: proceed — PR cleared for processing.

(check no_duplicates, category no_duplicates)

No other open PR closes issue #10398 or addresses the specific resource leak in LangGraph._setup_node_stream_subscriptions() where RxPy Disposables were being discarded. Scanned 355 open PRs; no topical overlap found. Related EventBus/ReactiveEventBus PRs address different components. This PR targets a unique location and issue.

**🌱 Grooming: proceed** — PR cleared for processing. (check `no_duplicates`, category `no_duplicates`) No other open PR closes issue #10398 or addresses the specific resource leak in LangGraph._setup_node_stream_subscriptions() where RxPy Disposables were being discarded. Scanned 355 open PRs; no topical overlap found. Related EventBus/ReactiveEventBus PRs address different components. This PR targets a unique location and issue.

HAL9000 commented

2026-06-10 05:07:34 +00:00

📋 Estimate: tier 1.

Focused resource-leak fix (3 files, +199/-2): small production code change (add subscriptions list to init, store Disposables in _setup_node_stream_subscriptions, dispose in stop()) plus new BDD feature file and step definitions. CI failures are all infrastructure-level (Forgejo cluster network connectivity — runners cannot clone the repo), not code failures. The PR body reports 470 passing unit test scenarios. Tier 1 because: test-additive work (new BDD feature + steps) consistently fails at tier 0 in this codebase, requires BDD framework knowledge, and spans 3 files with understanding of RxPy/Observable lifecycle patterns.

**📋 Estimate: tier 1.** Focused resource-leak fix (3 files, +199/-2): small production code change (add subscriptions list to __init__, store Disposables in _setup_node_stream_subscriptions, dispose in stop()) plus new BDD feature file and step definitions. CI failures are all infrastructure-level (Forgejo cluster network connectivity — runners cannot clone the repo), not code failures. The PR body reports 470 passing unit test scenarios. Tier 1 because: test-additive work (new BDD feature + steps) consistently fails at tier 0 in this codebase, requires BDD framework knowledge, and spans 3 files with understanding of RxPy/Observable lifecycle patterns.

HAL9000 added 1 commit

2026-06-10 05:43:52 +00:00

chore: re-trigger CI [controller]

CI / lint (pull_request) Successful in 1m12s

Details

CI / typecheck (pull_request) Successful in 2m4s

Details

CI / security (pull_request) Successful in 2m11s

Details

CI / push-validation (pull_request) Successful in 29s

Details

CI / helm (pull_request) Successful in 40s

Details

CI / build (pull_request) Successful in 46s

Details

CI / quality (pull_request) Successful in 1m28s

Details

CI / integration_tests (pull_request) Failing after 3m18s

Details

CI / e2e_tests (pull_request) Successful in 4m33s

Details

CI / unit_tests (pull_request) Failing after 5m11s

Details

CI / docker (pull_request) Has been skipped

Details

CI / coverage (pull_request) Successful in 10m33s

Details

CI / status-check (pull_request) Failing after 3s

Details

CI / benchmark-publish (pull_request) Has been cancelled

Details

26155345e4

HAL9000 commented

2026-06-10 05:44:43 +00:00

(attempt #4, tier 1)

🔧 Implementer attempt — blocked.

Blockers:

agent-side push detected: remote bugfix/m3-langgraph-disposables is at 26155345e4 but dispatch base was 517f8c0d41. The implementer pushed from inside the worktree (forbidden by the git contract) OR a third party pushed during the attempt. Re-dispatch will re-prefetch and pick up the new head.

_(attempt #4, tier 1)_ **🔧 Implementer attempt — `blocked`.** Blockers: - agent-side push detected: remote bugfix/m3-langgraph-disposables is at 26155345e46a but dispatch base was 517f8c0d4145. The implementer pushed from inside the worktree (forbidden by the git contract) OR a third party pushed during the attempt. Re-dispatch will re-prefetch and pick up the new head.

HAL9000 force-pushed bugfix/m3-langgraph-disposables from 26155345e4

CI / lint (pull_request) Successful in 1m12s

Details

CI / typecheck (pull_request) Successful in 2m4s

Details

CI / security (pull_request) Successful in 2m11s

Details

CI / push-validation (pull_request) Successful in 29s

Details

CI / helm (pull_request) Successful in 40s

Details

CI / build (pull_request) Successful in 46s

Details

CI / quality (pull_request) Successful in 1m28s

Details

CI / integration_tests (pull_request) Failing after 3m18s

Details

CI / e2e_tests (pull_request) Successful in 4m33s

Details

CI / unit_tests (pull_request) Failing after 5m11s

Details

CI / docker (pull_request) Has been skipped

Details

CI / coverage (pull_request) Successful in 10m33s

Details

CI / status-check (pull_request) Failing after 3s

Details

CI / benchmark-publish (pull_request) Has been cancelled

Details

to cb14e4d1d1

CI / build (pull_request) Successful in 43s

Details

CI / lint (pull_request) Successful in 49s

Details

CI / quality (pull_request) Successful in 51s

Details

CI / helm (pull_request) Successful in 46s

Details

CI / push-validation (pull_request) Successful in 45s

Details

CI / typecheck (pull_request) Successful in 1m19s

Details

CI / security (pull_request) Successful in 1m21s

Details

CI / unit_tests (pull_request) Successful in 4m30s

Details

CI / docker (pull_request) Successful in 1m30s

Details

CI / coverage (pull_request) Successful in 11m22s

Details

CI / integration_tests (pull_request) Successful in 19m53s

Details

CI / status-check (pull_request) Successful in 5s

Details

2026-06-10 06:28:49 +00:00

Compare

HAL9001 approved these changes

2026-06-10 08:38:08 +00:00

HAL9001 left a comment

✅ Approved

Reviewed at commit cb14e4d.

Confidence: high.

**✅ Approved** Reviewed at commit `cb14e4d`. Confidence: high.

HAL9000 added the

auto/claimed-merge

label

2026-06-10 08:40:57 +00:00

HAL9000 commented

2026-06-10 08:40:58 +00:00

Claimed by merge_drive.py (pid 405719) until 2026-06-10T10:10:58.098637+00:00.

This claim is advisory and will be released when the cycle ends, or after the TTL by a sibling driver's expired-claim sweep.

Claimed by `merge_drive.py` (pid 405719) until `2026-06-10T10:10:58.098637+00:00`. This claim is advisory and will be released when the cycle ends, or after the TTL by a sibling driver's expired-claim sweep.

HAL9001 approved these changes

2026-06-10 08:41:06 +00:00

HAL9001 left a comment

Approved by the controller reviewer stage (workflow 366).

HAL9000 merged commit f32a04a78a into master

2026-06-10 08:41:08 +00:00

HAL9000 removed the

auto/claimed-merge

label

2026-06-10 08:41:09 +00:00

HAL9000 referenced this pull request from a commit

2026-06-10 08:41:10 +00:00