Conversation
- New authenticated SSE endpoint at GET /api/os/events streams typed change events (kind + id only, never payload) filtered by kinds query param - New shared client hook useOsEvents(kinds, onEvent) returns connected/stale flags, manages single per-client EventSource with exponential backoff - Reuses existing EventBus SSE plumbing and auth middleware - Backend tests cover auth gate, kind filtering, and multi-subscriber delivery - Frontend tests cover hook mount, kind filtering, dedup, stale flag, and reconnect
📝 WalkthroughWalkthroughAdded an authenticated ChangesOS events streaming
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Desktop as Desktop client
participant Hook as useOsEvents
participant API as /api/os/events
participant Bus as EventBus
Desktop->>Hook: Request event kinds
Hook->>API: Open filtered EventSource connection
API->>Bus: Subscribe to user and broadcast channels
Bus-->>API: Deliver SystemEvent
API-->>Hook: Send payload-free SSE metadata
Hook-->>Desktop: Invoke event handler
API-->>Hook: Close connection
Hook->>API: Reconnect with exponential backoff
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd authenticated OS events SSE stream and
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Kinds change not applied
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| from tinyagentos.routes.os_events import router as os_events_router | ||
| app.include_router(os_events_router, dependencies=_csrf) |
There was a problem hiding this comment.
1. Missing os_events docs update 📜 Skill insight § Compliance
A new route module tinyagentos/routes/os_events.py was added and registered, but docs/agent-coordination.md was not updated to document GET /api/os/events (and no Docs-Reviewed: trailer is visible in this PR diff). This violates the doc-gate requirement for route module changes.
Agent Prompt
## Issue description
A new route module was added/registered (`GET /api/os/events`), but `docs/agent-coordination.md` does not mention it. The doc-gate rule requires either updating the docs or adding a `Docs-Reviewed:` commit trailer when no user-facing doc change is needed.
## Issue Context
`tinyagentos/routes/os_events.py` introduces a new authenticated SSE endpoint and `tinyagentos/routes/__init__.py` registers it.
## Fix Focus Areas
- tinyagentos/routes/__init__.py[350-351]
- docs/agent-coordination.md[113-115]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| }, delay); | ||
| } | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
3. Kinds change not applied 🐞 Bug ≡ Correctness
useOsEvents updates kindsRef when kinds changes but never recreates the EventSource URL, so expanding the kinds list can permanently miss events filtered out by the server. The hook will keep streaming the original server-side subscription until an unmount/remount (or hard-close reconnect) occurs.
Agent Prompt
## Issue description
`useOsEvents(kinds, onEvent)` keeps the EventSource URL fixed to whatever `kinds` were at initial mount because `connect` is memoized with `[]` and only called once. Updating `kindsRef.current` only affects client-side filtering and cannot recover events the server never sent.
## Issue Context
The server endpoint supports `?kinds=...` filtering. If a component adds kinds later, the server continues filtering with the old query string, so new kinds never arrive.
## Fix Focus Areas
- desktop/src/hooks/use-os-events.ts[34-45]
- desktop/src/hooks/use-os-events.ts[94-110]
## Suggested fix
- Derive a stable `kindsKey` (e.g., sorted/joined string) from `kinds`.
- When `kindsKey` changes, close the existing `EventSource`, clear any pending reconnect timer, and open a new `EventSource` with the updated URL.
- Ensure you don’t create parallel connections (guard so only one active ES exists at a time).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| kinds_param = request.query_params.get("kinds", "") | ||
| allowed_kinds = ( | ||
| {k.strip() for k in kinds_param.split(",") if k.strip()} | ||
| if kinds_param |
There was a problem hiding this comment.
4. Empty kinds filters everything 🐞 Bug ≡ Correctness
Backend kinds parsing treats inputs like kinds=, as an empty set (not “all kinds”), causing the stream to drop every event while still returning 200/SSE. This contradicts the endpoint contract that empty/missing kinds means “subscribe to all”.
Agent Prompt
## Issue description
`allowed_kinds` becomes an empty set for syntactically-nonempty but semantically-empty values like `kinds=,` or `kinds=,,`, which makes the generator filter out all events.
## Issue Context
Docs in the os_events route state that an empty or missing `kinds` parameter means "subscribe to all".
## Fix Focus Areas
- tinyagentos/routes/os_events.py[58-64]
- tinyagentos/routes/os_events.py[93-95]
## Suggested fix
- Parse kinds into `parsed = { ... }`.
- Set `allowed_kinds = parsed or None` so empty parses behave like missing/empty.
- Optionally: return 400 for invalid values if you want strictness, but keep behavior aligned with documented "empty means all".
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Merge both channels into a single queue so the generator has one await. | ||
| merged: asyncio.Queue = asyncio.Queue() | ||
|
|
||
| async def _relay(src: asyncio.Queue) -> None: |
There was a problem hiding this comment.
5. Unbounded per-client queue 🐞 Bug ☼ Reliability
The SSE handler drains EventBus subscriber queues into an unbounded merged queue; under sustained event rates and slow/blocked response consumption, this buffer can grow without bound and increase process memory. This creates a per-connection reliability/DoS risk.
Agent Prompt
## Issue description
`merged` is created with default (unbounded) capacity and relay tasks continuously enqueue into it. If the downstream StreamingResponse iteration is backpressured or stalled, events can accumulate indefinitely.
## Issue Context
EventBus subscriber queues are also unbounded and are fed via `put_nowait`, so there is no natural backpressure in the publisher path.
## Fix Focus Areas
- tinyagentos/routes/os_events.py[69-76]
- tinyagentos/routes/os_events.py[82-110]
- tinyagentos/events/bus.py[51-61]
- tinyagentos/events/bus.py[69-75]
## Suggested fix
- Make `merged` a bounded queue (e.g., `asyncio.Queue(maxsize=256)` or similar).
- In `_relay`, use `put_nowait` and implement an explicit overflow policy:
- drop-newest (skip enqueue when full), or
- drop-oldest (e.g., `merged.get_nowait()` then `put_nowait()`), or
- disconnect the client when it falls behind.
- Add a small log/metric when dropping so it’s observable in production.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 20-25: Update the CHANGELOG entry for the OS-level typed
change-event stream and useOsEvents to document the complete event schema,
including the ts metadata field alongside kind and id; keep the description
consistent with the existing OsEvent type and producer behavior.
In `@desktop/src/hooks/use-os-events.ts`:
- Around line 34-36: Update the EventSource connection logic in the useOsEvents
hook so changes to kinds recreate the server subscription using a stable
serialized kind key as the effect dependency, rather than only updating
kindsRef.current. Preserve local filtering and ensure cleanup closes the
previous connection; add a rerender test that confirms the new URL and delivery
of events for the updated kind.
- Around line 83-95: Update the EventSource onerror handler in
desktop/src/hooks/use-os-events.ts (lines 83-95) to handle CONNECTING-state
errors like active-source failures: mark the hook disconnected and stale, close
es, and schedule one guarded retry without duplicate timers. In
desktop/src/hooks/use-os-events.test.ts (lines 25-40), expose CONNECTING, OPEN,
and CLOSED on the mock constructor; update lines 112-135 to cover the CONNECTING
error path, and lines 165-213 to reserve the CLOSED path for fatal closure
behavior.
In `@tests/test_os_events.py`:
- Around line 197-203: Update the negative event-filtering test around the
stream task: after the timeout, assert that task is still running before
cancelling it, then await the cancelled task while suppressing only
asyncio.CancelledError. Do not catch Exception, so unexpected stream failures
propagate instead of allowing the empty-lines assertion to pass.
In `@tinyagentos/routes/os_events.py`:
- Around line 96-103: Keep the SSE payload serialized by the event-stream
generator limited to kind and id by removing ts from the data object in
tinyagentos/routes/os_events.py lines 96-103. Update the corresponding
assertions in tests/test_os_events.py lines 132-137 and 275-282 to remove
expectations for ts.
- Around line 70-80: Update the EventBus subscription path used by the SSE
response and the _relay flow in os_events.py to give each subscriber bounded
buffering instead of an unbounded merged queue. Define and apply the existing or
appropriate drop/coalesce policy for events when a slow client reaches capacity,
ensuring backpressure or dropping occurs at the per-subscriber boundary rather
than merely accumulating upstream.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74ef26cb-cd48-419a-a6db-afc9cdb34e4c
📒 Files selected for processing (6)
CHANGELOG.mddesktop/src/hooks/use-os-events.test.tsdesktop/src/hooks/use-os-events.tstests/test_os_events.pytinyagentos/routes/__init__.pytinyagentos/routes/os_events.py
| - **OS-level typed change-event stream + `useOsEvents` hook.** A new authenticated | ||
| SSE endpoint (`GET /api/os/events`) streams typed change events carrying only the | ||
| event kind and id, never the payload, so apps can opt into live updates with a | ||
| single hook call. The shared `useOsEvents(kinds, onEvent)` hook manages a single | ||
| per-client connection, exposes `connected` and `stale` flags, and handles | ||
| reconnect with exponential backoff. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documented event schema.
The hook exposes OsEvent.ts, and the producer sends ts. The entry says that events carry only kind and id.
Document ts as metadata, or remove it from the producer and client type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 20 - 25, Update the CHANGELOG entry for the
OS-level typed change-event stream and useOsEvents to document the complete
event schema, including the ts metadata field alongside kind and id; keep the
description consistent with the existing OsEvent type and producer behavior.
| useEffect(() => { | ||
| kindsRef.current = kinds; | ||
| }, [kinds]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconnect when kinds changes.
The server filters the SSE stream when it creates the connection. Updating only kindsRef.current leaves the server subscribed to the old kind list.
If a caller changes kinds from ["a"] to ["b"], the client drops a events locally but never receives b events. Recreate the EventSource when a stable serialized kind key changes. Add a rerender test that verifies the new URL and delivery for the new kind.
Also applies to: 99-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/hooks/use-os-events.ts` around lines 34 - 36, Update the
EventSource connection logic in the useOsEvents hook so changes to kinds
recreate the server subscription using a stable serialized kind key as the
effect dependency, rather than only updating kindsRef.current. Preserve local
filtering and ensure cleanup closes the previous connection; add a rerender test
that confirms the new URL and delivery of events for the updated kind.
| es.onerror = () => { | ||
| if (!stoppedRef.current && es.readyState === EventSource.CLOSED) { | ||
| const delay = Math.min( | ||
| RECONNECT_DELAY_MS * 2 ** reconnectAttemptsRef.current, | ||
| MAX_RECONNECT_DELAY_MS, | ||
| ); | ||
| reconnectAttemptsRef.current += 1; | ||
| setConnected(false); | ||
| setStale(true); | ||
| reconnectTimerRef.current = setTimeout(() => { | ||
| if (!stoppedRef.current) connect(); | ||
| }, delay); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the current WHATWG HTML EventSource standard, does a recoverable connection failure dispatch errorwithreadyStateset toCONNECTING, while CLOSED prevents automatic reconnection?
💡 Result:
According to the current WHATWG HTML Living Standard for Server-Sent Events, the behavior is as follows: When a recoverable connection failure occurs (such as a transient network issue), the user agent attempts to reestablish the connection [1][2]. During this process, the EventSource object's readyState is set to CONNECTING (value 0) [3][4][2]. An error event is dispatched, allowing the application to observe the failure even though the connection attempt is ongoing [5][6]. Conversely, when the user agent determines that it must "fail the connection" (typically due to a fatal error, such as a 204 No Content response, an incorrect Content-Type, or when the close method is invoked), it sets the readyState to CLOSED (value 2) and dispatches an error event [4][2]. Once the connection has reached the CLOSED state in this manner, the user agent does not attempt to reconnect [1][4][2]. Therefore, your understanding is correct: a recoverable failure involves the readyState being set to CONNECTING during the reconnection process [4][2], while a transition to the CLOSED state signifies a permanent termination of the connection, preventing any further automatic reconnection [1][4][2].
Citations:
- 1: https://html.spec.whatwg.org/multipage/server-sent-events.html
- 2: https://whatpr.org/html/3851/server-sent-events.html
- 3: https://html.spec.whatwg.org/dev/server-sent-events.html
- 4: https://wicg.github.io/controls-list/html-output/multipage/comms.html
- 5: https://developer.mozilla.org/en-US/docs/Web/API/EventSource/error_event
- 6: https://app.studyraid.com/en/read/16316/572678/handling-connection-errors
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '^desktop/src/hooks/use-os-events(\.test)?\.ts$' || true
printf '\nMain hook outline:\n'
ast-grep outline desktop/src/hooks/use-os-events.ts --view expanded || true
printf '\nMain hook relevant lines:\n'
sed -n '1,140p' desktop/src/hooks/use-os-events.ts | nl -ba
printf '\n Test relevant lines:\n'
sed -n '1,240p' desktop/src/hooks/use-os-events.test.ts | nl -ba
printf '\n Deterministic condition check:\n'
python3 - <<'PY'
import re
from pathlib import Path
code = Path('desktop/src/hooks/use-os-events.ts').read_text()
m = re.search(r'es\.onerror\s*=\s*\(\)\s*=>\s*\{(?P<body>.*?)\n\s*\};', code, re.S)
print('onerror body exists:', bool(m))
if m:
body = m.group('body')
print('uses CLOSED guard:', 'EventSource.CLOSED' in body or 'CLOSED' in body)
print('sets stale:', 'setStale(true)' in body)
print('sets connected false:', 'setConnected(false)' in body)
print('calls close:', 'es.close()' in body)
print('timeout/reconnect:', ('setTimeout' in body) and ('reconnectTimerRef.current' in body))
PYRepository: jaylfc/taOS
Length of output: 599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '^desktop/src/hooks/use-os-events(\.test)?\.ts$' || true
printf '\nMain hook outline:\n'
ast-grep outline desktop/src/hooks use-os-events.ts --view expanded || true
printf '\nMain hook relevant lines:\n'
sed -n '1,140p' desktop/src/hooks/use-os-events.ts | nl -ba
printf '\n Test relevant lines:\n'
sed -n '1,240p' desktop/src/hooks/use-os-events.test.ts | nl -ba
printf '\n Deterministic condition check:\n'
python3 - <<'PY'
import re
from pathlib import Path
code = Path('desktop/src/hooks/use-os-events.ts').read_text()
m = re.search(r'es\.onerror\s*=\s*\(\)\s*=>\s*\{(?P<body>.*?)\n\s*\};', code, re.S)
print('onerror body exists:', bool(m))
if m:
body = m.group('body')
print('uses CLOSED guard:', 'EventSource.CLOSED' in body or 'CLOSED' in body)
print('sets stale:', 'setStale(true)' in body)
print('sets connected false:', 'setConnected(false)' in body)
print('calls close:', 'es.close()' in body)
print('timeout/reconnect:', ('setTimeout' in body) and ('reconnectTimerRef.current' in body))
PYRepository: jaylfc/taOS
Length of output: 5460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' desktop/src/hooks/use-os-events.ts
printf '\n--- test 1-240 ---\n'
sed -n '1,240p' desktop/src/hooks/use-os-events.test.ts
printf '\n--- onerror condition extraction ---\n'
python3 - <<'PY'
from pathlib import Path
import re
code = Path('desktop/src/hooks/use-os-events.ts').read_text()
m = re.search(r'es\.onerror\s*=\s*\(\)\s*=>\s*\{(?P<body>.*?)\n\s*\};', code, re.S)
if m:
body = m.group(0)
print(body)
print('CLOSED guard present:', 'EventSource.CLOSED' in body or 'CLOSED' in body)
print('sets stale in error handler:', bool(re.search(r'setStale\s*\(\s*true\s*\)', body)))
print('sets connected false in error handler:', bool(re.search(r'setConnected\s*\(\s*false\s*\)', body)))
print('calls es.close in error handler:', bool(re.search(r'es\.close\s*\(', body)))
print('schedules reconnect in error handler:', bool(re.search(r'reconnectTimerRef\.current\s*=|setTimeout\s*\(', body)))
PYRepository: jaylfc/taOS
Length of output: 9993
🌐 Web query:
WHATWG HTML EventSource readyState CLOSED reestablish connection error even though readyState is not CLOSED
💡 Result:
In the WHATWG HTML standard, the EventSource API behavior regarding the readyState and reconnection is precisely defined [1][2]. The perception of a mismatch—where a connection error occurs but readyState is not CLOSED—is typically a result of how the browser natively manages automatic reconnections [3][4]. When an EventSource connection encounters an error (such as a temporary network drop or a clean TCP disconnect), the browser fires an error event and automatically transitions the readyState to CONNECTING (0) to initiate an automatic retry [1][4]. Because the browser is actively attempting to reconnect, the readyState will be CONNECTING rather than CLOSED (2) [2][4]. The CLOSED state (2) is reserved for situations where the connection is permanently terminated [2]. According to the standard, the browser stops retrying and sets readyState to CLOSED only in specific, fatal scenarios [2][4]: 1. Explicit Client Action: The user calls the.close method [5][2][3]. 2. Server Rejection: The server returns specific HTTP error codes (often 4xx or 5xx, depending on implementation) or provides an incorrect MIME type (anything other than text/event-stream) [3][4]. 3. CORS Failures: Failures during the CORS preflight process [4]. If you observe an error event but the readyState is not CLOSED, it indicates that the browser's native retry mechanism is currently active [4]. If your intention is to stop these automatic retries, you must manually call.close within your error event listener [3][4]. Conversely, if you are experiencing a state where the connection is clearly dead but the readyState remains OPEN (1), this is often due to a delay in the browser detecting the TCP/network failure, rather than a failure of the EventSource specification itself [4][6].
Citations:
- 1: https://html.spec.whatwg.org/multipage/server-sent-events.html
- 2: https://html.spec.whatwg.org/dev/server-sent-events.html
- 3: https://web.dev/articles/eventsource-basics
- 4: https://www.server-sent-events.com/frontend-consumption-client-patterns/error-handling-and-reconnection-ux/
- 5: https://developer.mozilla.org/en-US/docs/Web/API/EventSource
- 6: eventSource?.readyState Not Updating to closed on network disconnect inaka/EventSource#142
Handle recoverable EventSource error events.
A recoverable EventSource error dispatches error while the browser native connects in CONNECTING state. The current error handler only runs when readyState === EventSource.CLOSED, so normal connection loss leaves connected=true and stale=false and skips this hook’s retry schedule.
desktop/src/hooks/use-os-events.ts#L83-L95: treat active-source errors the same way: set stale/disconnected, calles.close(), and schedule one guarded retry.desktop/src/hooks/use-os-events.test.ts#L25-L40: exposeEventSource.CONNECTING,OPEN, andCLOSEDon the mock constructor.desktop/src/hooks/use-os-events.test.ts#L112-L135/L165-L213: exercise the CONNECTING error path, then reuse the CLOSED path only for fatal closure behavior.
📍 Affects 2 files
desktop/src/hooks/use-os-events.ts#L83-L95(this comment)desktop/src/hooks/use-os-events.test.ts#L25-L40desktop/src/hooks/use-os-events.test.ts#L112-L135desktop/src/hooks/use-os-events.test.ts#L165-L213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/hooks/use-os-events.ts` around lines 83 - 95, Update the
EventSource onerror handler in desktop/src/hooks/use-os-events.ts (lines 83-95)
to handle CONNECTING-state errors like active-source failures: mark the hook
disconnected and stale, close es, and schedule one guarded retry without
duplicate timers. In desktop/src/hooks/use-os-events.test.ts (lines 25-40),
expose CONNECTING, OPEN, and CLOSED on the mock constructor; update lines
112-135 to cover the CONNECTING error path, and lines 165-213 to reserve the
CLOSED path for fatal closure behavior.
| task.cancel() | ||
| try: | ||
| await task | ||
| except (asyncio.CancelledError, Exception): | ||
| pass | ||
|
|
||
| assert not lines, f"expected no events for wrong kind, got: {lines}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not suppress stream task failures in the negative test.
Lines 197-201 discard an exception from the application task. If the stream fails before delivery, lines remains empty and Line 203 passes. The test then does not prove that filtering works.
After the timeout, assert that task is still running before cancellation. During cleanup, suppress only the expected asyncio.CancelledError that follows task.cancel().
🧰 Tools
🪛 Ruff (0.16.1)
[error] 200-201: try-except-pass detected, consider logging the exception
(S110)
[warning] 200-200: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_os_events.py` around lines 197 - 203, Update the negative
event-filtering test around the stream task: after the timeout, assert that task
is still running before cancelling it, then await the cancelled task while
suppressing only asyncio.CancelledError. Do not catch Exception, so unexpected
stream failures propagate instead of allowing the empty-lines assertion to pass.
Source: Linters/SAST tools
| merged: asyncio.Queue = asyncio.Queue() | ||
|
|
||
| async def _relay(src: asyncio.Queue) -> None: | ||
| while True: | ||
| ev = await src.get() | ||
| await merged.put(ev) | ||
|
|
||
| relay_tasks = [ | ||
| asyncio.create_task(_relay(user_q), name="os-events-relay-user"), | ||
| asyncio.create_task(_relay(bcast_q), name="os-events-relay-bcast"), | ||
| ] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound buffering for slow SSE clients.
Lines 70-80 relay every event into an unbounded merged queue. If a connected client stops reading, the response generator stops draining while both relay tasks continue adding events. Each such connection can consume process memory without limit.
Use bounded per-subscriber buffering and define drop or coalesce behavior for slow consumers. Apply the limit through the EventBus subscription path so blocked relay tasks do not only move the unbounded backlog upstream.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/os_events.py` around lines 70 - 80, Update the EventBus
subscription path used by the SSE response and the _relay flow in os_events.py
to give each subscriber bounded buffering instead of an unbounded merged queue.
Define and apply the existing or appropriate drop/coalesce policy for events
when a slow client reaches capacity, ensuring backpressure or dropping occurs at
the per-subscriber boundary rather than merely accumulating upstream.
| data = json.dumps( | ||
| { | ||
| "kind": event.kind, | ||
| "id": event.trace_id, | ||
| "ts": event.ts, | ||
| } | ||
| ) | ||
| yield f"id: {seq}\ndata: {data}\n\n" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep the SSE event schema to kind and id.
Line 100 adds ts, but the endpoint documentation and PR contract specify event data containing only kind and id. This changes the public wire schema.
tinyagentos/routes/os_events.py#L96-L103: removetsfrom the serialized event data.tests/test_os_events.py#L132-L137: remove thetsassertion.tests/test_os_events.py#L275-L282: remove thetsassertion.
📍 Affects 2 files
tinyagentos/routes/os_events.py#L96-L103(this comment)tests/test_os_events.py#L132-L137tests/test_os_events.py#L275-L282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/os_events.py` around lines 96 - 103, Keep the SSE payload
serialized by the event-stream generator limited to kind and id by removing ts
from the data object in tinyagentos/routes/os_events.py lines 96-103. Update the
corresponding assertions in tests/test_os_events.py lines 132-137 and 275-282 to
remove expectations for ts.
|
nemotron-super review VERDICT: Found issues
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
Reviewed. This is careful work: cleanup is in a 1. BLOCKING — subscriptions and relay tasks leak if the generator is never startedThe subscriptions and the two relay tasks are created in the handler body, before user_q = await event_bus.subscribe(user_ch)
bcast_q = await event_bus.subscribe("broadcast")
relay_tasks = [asyncio.create_task(_relay(user_q)), asyncio.create_task(_relay(bcast_q))]but every one of them is torn down inside So if the client disconnects between the handler returning and Fix: move the 2. Unbounded queues make a slow consumer a memory leakThere are three unbounded queues per connection: A client that stops reading but keeps the socket open, or a stalled proxy, accumulates events without limit. SSE clients doing exactly this is the normal failure mode, not an exotic one. The bus's unbounded queue is pre-existing and not this PR's fault, but this route is the first long-lived consumer to make it matter, and 3. doc-gate is legitimately redCI's log is unavailable (0 bytes), so I reproduced locally, running both subcommands as CI does: Correct: 4. Use a changelog fragment, not
|
jaylfc
added
the
lead-blocked
|
SUPERSEDED by card tsk-ronapx, closing this PR. Reason: the branch has been red and unattended since 2026-08-06 (lanes are one-shot - they post [REVIEW] and exit, so the blocking review above could never reach a worker), and dev has moved far enough since that a rebase is a bigger job than a re-cut: changelog.d/ fragments replaced direct CHANGELOG.md edits and doc-gate has been rewritten twice. The work is not lost. Nothing here needs re-deriving; the replacement PR supersedes this one. |
feat(os-events): /api/os/events SSE stream and useOsEvents hook (supersedes #2309)
Supersedes PR jaylfc#2309 (branch exec/tsk-jmctoa), whose lane is gone. That branch's endpoint and hook are merged in here on top of current dev, with the review findings resolved. - Subscriptions and relay tasks leaked. Both subscribe() calls and both create_task() calls ran in the handler body while teardown lived in the generator's finally. An async generator closed without ever being iterated never runs its body, so a client disconnecting between the handler returning and the stream starting leaked two bus subscriptions and two tasks, which the bus then fed forever. Setup now happens inside the generator, paired with the finally that undoes it. - The merged queue was unbounded, so a client that stopped reading grew the process without limit. It is capped at 256 and the relay drops the oldest event rather than blocking, then tells the client with an events.lagged frame carrying the drop count. - Dropped the SSE "id:" line. It is what makes a browser send Last-Event-ID, which this endpoint ignores by design, and seq restarted at 1 per connection so the ids were not stable anyway. - Hook: reopen the stream when kinds changes (the URL is fixed per connection, so a widened list never arrived), and report disconnected on an error while the stream is only CONNECTING. - The EventSource test mock had no static readyState constants, so the disconnect assertions compared undefined to undefined and passed whatever the hook did. - Documented the endpoint in docs/agent-coordination.md and moved the direct CHANGELOG.md edit into a changelog.d fragment.
1 participant
CARD TITLE (intent, not commit subject): Fix-forward PR 2220 (os-events SSE): doc-gate + hook fixes
Autonomous build of board card tsk-jmctoa.
change events (kind + id only, never payload) filtered by kinds query param
flags, manages single per-client EventSource with exponential backoff
Files:
CHANGELOG.md | 6 +
desktop/src/hooks/use-os-events.test.ts | 228 ++++++++++++++++++++++++++
desktop/src/hooks/use-os-events.ts | 113 +++++++++++++
tests/test_os_events.py | 282 ++++++++++++++++++++++++++++++++
tinyagentos/routes/init.py | 3 +
tinyagentos/routes/os_events.py | 121 ++++++++++++++
6 files changed, 753 insertions(+)
Summary by CodeRabbit
New Features
Documentation