refactor(tui): extract _dispatch_notification, use time.time_ns() for rid - #27711
refactor(tui): extract _dispatch_notification, use time.time_ns() for rid#27711Ade5954 wants to merge 1 commit into
Conversation
… rid 1. Extract _dispatch_notification() to eliminate duplicated code between the main poller loop and shutdown drain in _notification_poller_loop(). The shared helper handles consumed-skip, formatting, status.update emission, busy guard, and turn dispatch. The caller picks continue vs break based on 'busy' return status. 2. Replace int(time.time() * 1000) with time.time_ns() for notification rid generation to avoid potential collisions when two events arrive in the same millisecond. All 3 notification_poller tests pass.
|
Thanks for the cleanup. The Problems
Suggested changes
This is an automated hermes-sweeper review. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the cleanup. The time.time_ns() RID improvement still applies on current main (tui_gateway/server.py:8775, :8827), but the extraction needs to retain notification-poller behavior added after this branch.
Problems
- PR
tui_gateway/server.py:3057emitsstatus.updateunconditionally. Current main deduplicates requeued notifications attui_gateway/server.py:8752-8759(e7a7872a874837ca36105ca3e90db464cf7c125a); otherwise a busy session receives repeated process-status lines. - The current live and drain paths preserve cross-session ownership and fail closed for unowned async-delegation payloads (
tui_gateway/server.py:8708-8742,:8797-8808). The proposed helper/callers omit those gates. - PR
tui_gateway/server.py:3104immediately retries a busy requeue, removing current main's required 0.25-second backoff attui_gateway/server.py:8769-8772(98804dbeef91c5f1ef517817c8f81dd2aeec523e).
Suggested changes
- Extract only after preserving the live/drain ownership rules, pass through the emitted-dedup state, and retain live-loop backoff versus drain break semantics.
- Add coverage for those current guarantees alongside the RID change.
This is an automated hermes-sweeper review.
| if not text: | ||
| return "empty" | ||
|
|
||
| _emit("status.update", sid, {"kind": "process", "text": text}) |
There was a problem hiding this comment.
Current main emits this only when _notification_event_dedup_key(evt) is absent from the poller's _emitted set (tui_gateway/server.py:8752-8759). Keeping this unconditional reintroduces duplicate process-status updates every time a busy session requeues the same event.
|
|
||
| text = format_process_notification(evt) | ||
| if not text: | ||
| status = _dispatch_notification(evt, sid, session) |
There was a problem hiding this comment.
Before dispatching, current main must preserve the foreign-live-session requeue and fail-closed async-delegation ownership checks (tui_gateway/server.py:8708-8742). Dispatching every dequeued event here can route a notification or delegation payload to the wrong session.
| text = format_process_notification(evt) | ||
| if not text: | ||
| status = _dispatch_notification(evt, sid, session) | ||
| if status == "busy": |
There was a problem hiding this comment.
Please retain the live-loop time.sleep(0.25) backoff after a busy requeue (tui_gateway/server.py:8769-8772; 98804dbeef91c5f1ef517817c8f81dd2aeec523e). An immediate continue restores the busy-poller CPU spin this fix addressed.
Extract _dispatch_notification() to eliminate duplicated code between the main poller loop and shutdown drain in _notification_poller_loop(). The shared helper handles consumed-skip, formatting, status.update emission, busy guard, and turn dispatch. The caller picks continue vs break based on 'busy' return status.
Replace int(time.time() * 1000) with time.time_ns() for notification rid generation to avoid potential collisions when two events arrive in the same millisecond.
All 3 notification_poller tests pass.
What does this PR do?
simplify the code and replace int(time.time() * 1000) with time.time_ns() for notification rid generation to avoid potential collisions when two events arrive in the same millisecond
Related Issue
#26327