fix(process_registry): keep poll() read-only — do not mark completions consumed - #35552
Closed
someaka wants to merge 1 commit into
Closed
fix(process_registry): keep poll() read-only — do not mark completions consumed#35552someaka wants to merge 1 commit into
someaka wants to merge 1 commit into
Conversation
…s consumed poll() should be a pure read operation. mark_completion_consumed() is now an explicit method for callers that have actually delivered the notification (e.g., TUI notification bridge). The old behavior caused poll() to silently eat completion events, preventing any other consumer from seeing them.
alt-glitch
added
type/bug
Collaborator
tonydwb
approved these changes
May 30, 2026
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review Findings
This PR fixes a data-consistency bug where poll() was marking process completions as consumed — i.e., a read-only query had the side effect of suppressing drain-loop notifications.
✅ Looks Good
- Clean fix: Removes the
self._completion_consumed.add(session_id)line frompoll()— this was a mutation side effect in a read-only method. - Updated docstrings:
is_completion_consumed()anddrain_notifications()docstrings corrected to no longer mention "poll" as a consumption source. - Expose mark_completion_consumed(): Adds a dedicated public method so consumers that genuinely need to mark a session consumed (e.g. TUI notification delivery) can do so explicitly.
- Tests: Existing test suite covers the regression scenario.
- Minimal diff: Just removes the side-effect line and adds the explicit API. Low risk.
No Issues Found
Reviewed by Hermes Agent
Author
|
Closing — duplicate of canonical #10158 per @alt-glitch's review. The poll() read-only fix already landed via #10158. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Removes the
_completion_consumed.add(session_id)call frompoll(), making it a pure read operation. Adds explicitmark_completion_consumed()method for callers that have actually delivered the notification.Why
poll()was silently eating completion events, preventing any other consumer from seeing them. The TUI notification bridge needs to poll for events without consuming them — consumption should only happen after successful delivery.Fixes #10156.
Changes
poll(): removed_completion_consumed.add(session_id)— now read-onlymark_completion_consumed(session_id)— explicit consumption after delivery