fix(process_registry): keep poll() read-only — do not mark completions consumed - #31756
Closed
someaka wants to merge 1 commit into
Closed
fix(process_registry): keep poll() read-only — do not mark completions consumed#31756someaka wants to merge 1 commit into
someaka wants to merge 1 commit into
Conversation
…s consumed poll() was marking sessions as completed in _completion_consumed, which silently suppressed notify_on_complete watcher notifications. poll() is a read-only status query and should not have the side effect of consuming the notification event. The correct consumers (wait(), read_log()) already mark consumed. Includes: - Remove self._completion_consumed.add(session_id) from poll() - Update docstrings: 'wait/poll/log' -> 'wait/log' - Regression test: test_poll_does_not_mark_completion_consumed Fixes: NousResearch#10156 Ref: NousResearch#10156
alt-glitch
added
type/bug
Collaborator
|
Duplicate of #10158 (canonical, open since April) which implements the same poll() read-only fix for #10156. Also duplicate of #31750 (same author's prior submission). Note: this PR bundles unrelated memory_manager.py changes (multi-provider support, threading lock) that are outside the poll() fix scope — consider splitting. |
Author
|
Closing — duplicate of #10158 per @alt-glitch. The poll() read-only fix is already tracked there. |
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
2 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.
Summary
Keeps
process_registry.poll()read-only for exited background processes.poll()should report status, not mark anotify_on_completeresult as consumed. Otherwise a normal status check can race with the gateway watcher and permanently suppress the synthetic completion event before it is delivered back to the chat.Root cause
PR #8228 added
_completion_consumedtracking to avoid duplicate completion notifications. That behavior is correct for explicit consumption paths likewait()andread_log(), but not forpoll():wait()is a blocking consume — "give me the result, I'm done"read_log()is an explicit read — "I'm reading the output, don't notify me"poll()is a read-only status query — "what's the current state?"Treating
poll()as consumption causes this race:notify_on_complete=trueprocess(action="poll")poll()marks session as consumed → watcher skips notification → silent lossChanges Made
tools/process_registry.py: Removeself._completion_consumed.add(session_id)frompoll(). Update docstrings from "wait/poll/log" to "wait/log".tests/tools/test_process_registry.py: Addtest_poll_does_not_mark_completion_consumed— verifies that afterpoll()on an exited process,drain_notifications()still returns the completion event.Test plan
Related
Closes #10156
Refs PR #10158 (existing open PR by @de1tydev — this is a focused minimal version with only the
poll()change and a regression test)