[feat] Stream session changes and fix session liveness (9/12) - #5688
[feat] Stream session changes and fix session liveness (9/12)#5688ardaerzin wants to merge 18 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesSession watch and approval flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SessionService
participant Redis
participant SSEEndpoint
participant MobileChat
SessionService->>Redis: publish session watch event
MobileChat->>SSEEndpoint: open EventSource
SSEEndpoint->>Redis: subscribe to session channel
Redis-->>SSEEndpoint: event or heartbeat
SSEEndpoint-->>MobileChat: SSE frame
MobileChat->>MobileChat: refresh transcript or approval state
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/mobile/src/features/chat/useApprovalActions.ts (1)
94-115: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not resume unrelated approvals with one invocation.
When
target.allis set, Line 94 selects every pending approval. Line 115 then selects references fromwithRefs[0]. The code documents that parked runs can use different revisions. The resume request can therefore apply approvals from multiple runs with one run's workflow configuration.Partition approvals by their interaction references and resume each group separately. Otherwise, hide Approve all unless all pending approvals have the same references.
api/oss/src/tasks/asyncio/sessions/orphan_sweep.py (1)
90-103: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIsolate per-row Redis cleanup failures so one bad row does not strand the rest.
The Postgres commit at Line 88 happens before this loop. If
force_cancel_alive,clear_running, orforce_clear_ownerraises for one row, the loop stops and later rows keep stale Redis locks. Since the WHERE clause at Lines 62-63 requiresflags @> {"is_alive": True}, a row already committed asis_alive=Falsewill never be reselected by a later sweep pass. This means its stale Redis lock is never retried and persists until its own TTL expires. The new idle branch increases the number of rows that pass through this loop each pass, so this gap now affects more sessions.The three awaited calls per row also run sequentially, adding up to
SWEEP_BATCH_SIZE× 3 round trips per pass.Wrap each row's cleanup so a failure on one row does not block cleanup of the rest, and run the three calls concurrently.
🛡️ Proposed fix for per-row failure isolation
# Bring the Redis locks the SEND gate reads in sync with the rows just written. for row in orphans: project_id = str(row.project_id) - await force_cancel_alive( - lock_engine, project_id=project_id, session_id=row.session_id - ) - await clear_running( - lock_engine, project_id=project_id, session_id=row.session_id - ) - # A swept session is dead; free its affinity like kill does. - await force_clear_owner( - lock_engine, project_id=project_id, session_id=row.session_id - ) + try: + await asyncio.gather( + force_cancel_alive( + lock_engine, project_id=project_id, session_id=row.session_id + ), + clear_running( + lock_engine, project_id=project_id, session_id=row.session_id + ), + # A swept session is dead; free its affinity like kill does. + force_clear_owner( + lock_engine, project_id=project_id, session_id=row.session_id + ), + ) + except Exception: + log.exception( + "orphan_sweep: failed to clear redis locks for a swept session", + extra={"session_id": row.session_id, "stream_id": str(row.id)}, + )
🧹 Nitpick comments (4)
web/mobile/src/features/chat/approvalInputSummary.ts (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce non-exception multi-line comments.
These comments describe UI intent. Replace them with one short line or remove them.
web/mobile/src/features/chat/approvalInputSummary.ts#L8-L9: reduce the primary-field explanation to one short line.web/mobile/src/features/chat/TurnRow.tsx#L16-L17: remove or reduce the message-style explanation.web/mobile/src/features/chat/TurnRow.tsx#L45-L46: reduce the approval-marker explanation to one short line.As per coding guidelines, keep in-code comments to at most one short line unless a bug, race, or ordering requirement needs more detail.
Source: Coding guidelines
web/mobile/src/features/sessions/SessionRow.tsx (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
livenessprop documentation.The row no longer falls back to
session.flags.is_alive. The prop doc above still states thatundefinedmeans "fall back to the list row's own flags". Correct it so callers do not expect the removed behavior.♻️ Suggested doc fix (lines 28-30)
- /** Fresh badge from the shared liveness poll; `undefined` = poll unresolved (fall back to - * the list row's own flags), `null` = poll resolved and this session is idle. */ + /** Fresh badge from the shared liveness poll; `undefined` = poll unresolved (no badge), + * `null` = poll resolved and this session is idle. */api/oss/src/apis/fastapi/sessions/router.py (1)
537-595: 🚀 Performance & Scalability | 🔵 TrivialOperational note: align
watch_heartbeat_secondswith proxy/load-balancer idle timeouts.This handler relies on
HEARTBEAT_FRAMEcomments to keep the SSE connection alive during idle periods, at the interval configured byenv.sessions.watch_heartbeat_seconds. Many reverse proxies and load balancers close idle HTTP connections after a fixed timeout, commonly 30-60 seconds. Confirm the configured heartbeat interval stays comfortably below the idle timeout of every proxy in the request path (nginx, ALB, Cloudflare, etc.) to avoid clients experiencing frequent unnecessary reconnects.The rest of this handler — validation ordering, permission check, lazy stream construction, and response headers — matches the behavior exercised by
test_watch_endpoint_rejects_without_view_sessions,test_watch_endpoint_rejects_invalid_session_id, andtest_watch_endpoint_returns_event_stream_response.api/oss/tests/pytest/unit/sessions/test_watch_endpoint.py (1)
128-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a regression test for non-string
typevalues.
test_format_watch_frame_rejects_non_dict_and_unknown_typecovers a non-dict payload and an unknown stringtype, but not a payload like{"type": ["nope"]}where"type"is itself a list or dict. As noted in the review ofapi/oss/src/apis/fastapi/sessions/watch.py(Lines 31-42), that input currently raises an uncaughtTypeErrorinstead of returningNone. Add a case exercising this input once the fix lands, to prevent regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e3b76ee-1cc2-4e66-afa0-c4f915a0c484
📒 Files selected for processing (37)
api/entrypoints/routers.pyapi/entrypoints/worker_queues.pyapi/entrypoints/worker_streams.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/apis/fastapi/sessions/watch.pyapi/oss/src/core/sessions/interactions/service.pyapi/oss/src/core/sessions/streams/service.pyapi/oss/src/dbs/redis/sessions/contract.pyapi/oss/src/dbs/redis/sessions/watch.pyapi/oss/src/tasks/asyncio/sessions/orphan_sweep.pyapi/oss/src/tasks/asyncio/sessions/records_worker.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_mirrors_existing_row.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_turn_handover.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_clears_redis.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_thresholds.pyapi/oss/tests/pytest/unit/sessions/test_watch_endpoint.pyapi/oss/tests/pytest/unit/sessions/test_watch_interactions_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_lifecycle_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_publish.pydocs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.mdweb/mobile/src/features/chat/ApprovalCard.tsxweb/mobile/src/features/chat/ApprovalDock.tsxweb/mobile/src/features/chat/ChatHeader.tsxweb/mobile/src/features/chat/ChatScreen.tsxweb/mobile/src/features/chat/TurnRow.tsxweb/mobile/src/features/chat/approvalInputSummary.tsweb/mobile/src/features/chat/useApprovalActions.tsweb/mobile/src/features/chat/useSessionTranscript.tsweb/mobile/src/features/chat/useSessionWatch.tsweb/mobile/src/features/chat/watchRelay.tsweb/mobile/src/features/sessions/SessionRow.tsxweb/mobile/src/lib/auth.tsweb/mobile/tests/unit/livenessBadge.test.tsweb/mobile/tests/unit/watchRelay.test.tsweb/mobile/vitest.config.ts
💤 Files with no reviewable changes (1)
- web/mobile/src/features/chat/ApprovalCard.tsx
1d70a28 to
e059cec
Compare
c4025ac to
0cae609
Compare
e059cec to
c110d9b
Compare
0cae609 to
e46be44
Compare
c110d9b to
5a9a6a0
Compare
e46be44 to
9d900a8
Compare
5a9a6a0 to
d3537e8
Compare
9d900a8 to
5321274
Compare
|
All three verified against the code. Two were real; one I am not taking.
End-of-life Fixed with a Two regression tests added, and I verified they fail against the old code: a stale turn's end must leave the live turn's lock and publish nothing, and a turn's own end must still clear and publish. Teardown log — not taking. The logger already carries the exception context at that call site; repeating it in the message duplicates what the handler prints. |
5321274 to
1c4c784
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.md (1)
14-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the watch-relay design status and contract.
This PR delivers
GET /sessions/streams/watchas a metadata-only SSE relay. The document still states that no session watch SSE exists and schedules a cursor-based record-replay endpoint as future M3 work.Mark the delivered relay as implemented. Describe its metadata-only revalidation contract. Keep any cursor-based record replay as a separate future enhancement.
Also applies to: 275-279
🧹 Nitpick comments (1)
web/mobile/src/features/chat/useSessionWatch.ts (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the block comment.
This block documents routine hook behavior across 13 lines. Keep one short in-code comment. Put detailed relay behavior in external documentation if needed.
As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15d03900-a4d3-4cdf-a2b6-a7558723c153
📒 Files selected for processing (38)
api/entrypoints/routers.pyapi/entrypoints/worker_queues.pyapi/entrypoints/worker_streams.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/apis/fastapi/sessions/watch.pyapi/oss/src/core/sessions/interactions/service.pyapi/oss/src/core/sessions/streams/service.pyapi/oss/src/dbs/redis/sessions/contract.pyapi/oss/src/dbs/redis/sessions/locks.pyapi/oss/src/dbs/redis/sessions/watch.pyapi/oss/src/tasks/asyncio/sessions/orphan_sweep.pyapi/oss/src/tasks/asyncio/sessions/records_worker.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_mirrors_existing_row.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_stale_turn_end.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_turn_handover.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_clears_redis.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_thresholds.pyapi/oss/tests/pytest/unit/sessions/test_watch_endpoint.pyapi/oss/tests/pytest/unit/sessions/test_watch_interactions_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_lifecycle_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_publish.pydocs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.mdweb/mobile/src/features/chat/ApprovalDock.tsxweb/mobile/src/features/chat/ChatHeader.tsxweb/mobile/src/features/chat/ChatScreen.tsxweb/mobile/src/features/chat/TurnRow.tsxweb/mobile/src/features/chat/approvalInputSummary.tsweb/mobile/src/features/chat/useApprovalActions.tsweb/mobile/src/features/chat/useSessionTranscript.tsweb/mobile/src/features/chat/useSessionWatch.tsweb/mobile/src/features/chat/watchRelay.tsweb/mobile/src/features/sessions/SessionRow.tsxweb/mobile/src/lib/auth.tsweb/mobile/tests/unit/livenessBadge.test.tsweb/mobile/tests/unit/watchRelay.test.tsweb/mobile/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- api/entrypoints/routers.py
- web/mobile/src/features/chat/ChatScreen.tsx
- web/mobile/src/features/sessions/SessionRow.tsx
- web/mobile/vitest.config.ts
- web/mobile/src/features/chat/useApprovalActions.ts
- api/oss/tests/pytest/unit/sessions/test_heartbeat_mirrors_existing_row.py
- web/mobile/src/features/chat/ApprovalDock.tsx
- web/mobile/tests/unit/livenessBadge.test.ts
- web/mobile/src/features/chat/approvalInputSummary.ts
- web/mobile/src/features/chat/TurnRow.tsx
- web/mobile/src/lib/auth.ts
- api/oss/tests/pytest/unit/sessions/test_watch_interactions_publish.py
- api/oss/tests/pytest/unit/sessions/test_heartbeat_turn_handover.py
- web/mobile/src/features/chat/watchRelay.ts
- api/entrypoints/worker_queues.py
- api/oss/tests/pytest/unit/sessions/test_watch_lifecycle_publish.py
- api/oss/tests/pytest/unit/sessions/test_orphan_sweep_thresholds.py
- web/mobile/src/features/chat/useSessionTranscript.ts
- web/mobile/tests/unit/watchRelay.test.ts
- api/entrypoints/worker_streams.py
- api/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.py
- api/oss/src/apis/fastapi/sessions/router.py
- web/mobile/src/features/chat/ChatHeader.tsx
- api/oss/tests/pytest/unit/sessions/test_orphan_sweep_clears_redis.py
- api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
- api/oss/src/tasks/asyncio/sessions/records_worker.py
- api/oss/src/dbs/redis/sessions/contract.py
- api/oss/src/core/sessions/interactions/service.py
- api/oss/tests/pytest/unit/sessions/test_watch_publish.py
- api/oss/src/utils/env.py
- api/oss/src/core/sessions/streams/service.py
d3537e8 to
672590a
Compare
1c4c784 to
551dc6f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
672590a to
b67caf8
Compare
551dc6f to
28906c3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Log the exception on a failed teardown — real, fixed. The |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.md (1)
14-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the plan for the implemented metadata watch relay.
GET /sessions/streams/watchnow exists. It emits session-change metadata through durable Redis, and clients revalidate records through existing query paths. Update these sections to mark that relay as implemented. Keep any future cursor replay or incremental record reduction as separate work.Also applies to: 275-279
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7db79f78-2543-4fb0-9a26-bf682cd0abd2
📒 Files selected for processing (38)
api/entrypoints/routers.pyapi/entrypoints/worker_queues.pyapi/entrypoints/worker_streams.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/apis/fastapi/sessions/watch.pyapi/oss/src/core/sessions/interactions/service.pyapi/oss/src/core/sessions/streams/service.pyapi/oss/src/dbs/redis/sessions/contract.pyapi/oss/src/dbs/redis/sessions/locks.pyapi/oss/src/dbs/redis/sessions/watch.pyapi/oss/src/tasks/asyncio/sessions/orphan_sweep.pyapi/oss/src/tasks/asyncio/sessions/records_worker.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_mirrors_existing_row.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_stale_turn_end.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_turn_handover.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_clears_redis.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_thresholds.pyapi/oss/tests/pytest/unit/sessions/test_watch_endpoint.pyapi/oss/tests/pytest/unit/sessions/test_watch_interactions_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_lifecycle_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_publish.pydocs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.mdweb/mobile/src/features/chat/ApprovalDock.tsxweb/mobile/src/features/chat/ChatHeader.tsxweb/mobile/src/features/chat/ChatScreen.tsxweb/mobile/src/features/chat/TurnRow.tsxweb/mobile/src/features/chat/approvalInputSummary.tsweb/mobile/src/features/chat/useApprovalActions.tsweb/mobile/src/features/chat/useSessionTranscript.tsweb/mobile/src/features/chat/useSessionWatch.tsweb/mobile/src/features/chat/watchRelay.tsweb/mobile/src/features/sessions/SessionRow.tsxweb/mobile/src/lib/auth.tsweb/mobile/tests/unit/livenessBadge.test.tsweb/mobile/tests/unit/watchRelay.test.tsweb/mobile/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- web/mobile/vitest.config.ts
- web/mobile/src/lib/auth.ts
- web/mobile/src/features/chat/useSessionWatch.ts
- web/mobile/src/features/chat/watchRelay.ts
- web/mobile/src/features/chat/ApprovalDock.tsx
- web/mobile/src/features/chat/TurnRow.tsx
- web/mobile/src/features/chat/ChatHeader.tsx
- api/oss/src/tasks/asyncio/sessions/records_worker.py
- api/entrypoints/worker_queues.py
- web/mobile/tests/unit/livenessBadge.test.ts
- api/oss/src/dbs/redis/sessions/contract.py
- web/mobile/src/features/chat/ChatScreen.tsx
- api/oss/tests/pytest/unit/sessions/test_orphan_sweep_thresholds.py
- api/oss/src/dbs/redis/sessions/locks.py
- web/mobile/src/features/chat/useApprovalActions.ts
- api/oss/tests/pytest/unit/sessions/test_heartbeat_stale_turn_end.py
- web/mobile/src/features/chat/approvalInputSummary.ts
- api/entrypoints/routers.py
- api/oss/src/utils/env.py
- api/oss/tests/pytest/unit/sessions/test_watch_interactions_publish.py
- api/oss/tests/pytest/unit/sessions/test_watch_lifecycle_publish.py
- web/mobile/src/features/sessions/SessionRow.tsx
- web/mobile/tests/unit/watchRelay.test.ts
- api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
- api/oss/tests/pytest/unit/sessions/test_orphan_sweep_clears_redis.py
- api/oss/src/core/sessions/streams/service.py
- api/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.py
- api/oss/src/apis/fastapi/sessions/router.py
- api/entrypoints/worker_streams.py
- api/oss/tests/pytest/unit/sessions/test_heartbeat_mirrors_existing_row.py
- api/oss/src/core/sessions/interactions/service.py
- web/mobile/src/features/chat/useSessionTranscript.ts
| assert total_appended == 0 | ||
| assert publisher.calls == [] | ||
| # Failed appends stay in the existing retry path; publish adds nothing to it. | ||
| assert len(processed_ids) == 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep failed append messages unacknowledged.
processed_ids is the acknowledgment set. This assertion accepts the failed record as processed. The caller can then acknowledge it and lose the record instead of retrying the append.
Expect no processed IDs for this batch. Update RecordsWorker.process_batch so it returns IDs only after a successful append.
Proposed test correction
- # Failed appends stay in the existing retry path; publish adds nothing to it.
- assert len(processed_ids) == 1
+ # Failed appends must remain unacknowledged for retry.
+ assert processed_ids == []Adds the watch:<project_id>:session:<session_id> pub/sub channel to the
sessions Redis contract (durable plane) with three change-notification
payloads: records-changed, lifecycle {running|ended}, and interaction
{pending|resolved}. SessionsWatchPublisher is strictly fire-and-forget —
a publish failure is logged and swallowed so the write path that
triggered it never fails or re-drives.
Publish points (one choke point per event family):
- RecordsWorker.process_batch tees records-changed strictly post-
append_many, once per distinct session in the project batch.
- SessionStreamsService publishes lifecycle running on turn start
(send/steer) and ended on cancel/kill/turn-end heartbeat (transition
only, not on idle heartbeats).
- SessionInteractionsService publishes interaction pending on create and
resolved on transition/cancel-sweep.
Wired in worker_streams (reusing the durable client), routers, and
worker_queues. No behavior change when no watch publisher is injected.
Bridges one durable-Redis pubsub subscription per connection into a text/event-stream StreamingResponse: named events (records-changed, lifecycle, interaction) with minimal metadata only — clients revalidate via the regular query endpoints; no record payloads ride the wire. Idle windows emit ': heartbeat' comments every 15s (env-tunable via AGENTA_SESSIONS_WATCH_HEARTBEAT_SECONDS) so proxies never idle out. Client disconnect cancels the generator, whose finally block tears down the redis subscription — nothing outlives its SSE connection. Multi- replica correct by construction: events originate in worker/API processes and fan out over shared Redis pub/sub, so any replica can serve the GET (no sticky sessions). Auth rides the existing middleware (cookie/ApiKey/Bearer) evaluated at connect; the handler enforces VIEW_SESSIONS like query_records and validates session_id against the contract allowlist. Spec surface: the route lands in OpenAPI (operation_id watch_session_stream) for documentation, but Fern does not model SSE — clients consume it with a native EventSource (documented in the route docstring); no client regeneration needed.
One EventSource per foregrounded chat screen against /sessions/streams/watch (same-origin /api, cookie auth — native EventSource, not the Fern client, which does not model SSE): - records-changed (and every open, as missed-event coverage) drives the transcript tick's exact body — useSessionTranscript now exposes it as a stable refresh() (revalidateSessionRecordsAtom + loadSessionMessages re-read through the shared cache). - lifecycle / interaction events invalidate the shared liveness and actionable-interactions queries instead of duplicating state; their 15s polls stay as the documented fallback. - Foreground-only: closed on visibilitychange->hidden, reopened on visible. Transient errors ride EventSource's built-in reconnect; a fatal CLOSED falls back to today's 4s/7.5s cadence and retries every 60s while visible. While the stream is open the ChatScreen records tick stretches to a 30s safety net (watchAwarePollMs); when the stream is down the cadence is byte-identical to before — the fallback IS the current behavior. The sessions LIST screen keeps its 15s polls in v1 per the plan. vitest config gains the tsconfig '@/' alias so tested modules resolve app imports.
Cancelling the response body aborted the invoke; the agent service treated the disconnect as a stop, so the resumed run died ~200ms in and the gate stayed pending. Also stop badging rows from the lagging flags mirror.
`heartbeat` seeded its result from the row it had just read, which skipped both the create and the update branch whenever a `session_streams` row already existed. The runner's heartbeat is the only producer of `flags` for a real turn (nothing calls the send/steer command path), so the column stayed NULL on every session that had been named first, and froze at `is_running: true` on rows the first beat had created — the turn-end beat could not write either. `updated_at` stayed NULL too, which is the column the orphan sweep filters on. The mirror write is now unconditional: create on first touch, else update. That makes the row track the CURRENT turn, which the `is_current_turn` disambiguation was not written for: an old turn's beat now finds a different turn_id on the row and would read as establishment. Settle the takeover case on the signal that actually means it — a failed nx re-acquire of the alive lock, which only fails while another turn holds it.
The sweep selected on `updated_at < threshold`, and `NULL < threshold` is NULL, so a row whose heartbeat never wrote it could never be reclaimed however long it had claimed to be alive. Locally that left ~1.2k rows stuck at is_running=true for days: invisible to the sweep, counted by the per-project concurrency cap, and returned by the project-wide liveness query as phantom "running" sessions. Select on coalesce(updated_at, created_at), and bound the pass so an accumulated backlog drains over successive sweeps instead of one oversized commit.
…tbeat `_start_turn` was the only publisher of the M3 relay's `running` lifecycle event, and it is reachable only through the send/steer command path — which no shipped client calls. A real turn goes straight to the agent invoke, and the runner mints its own turn id and only heartbeats, so watchers saw `records-changed` and `interaction` events but never `running`; `ended` already rode the turn-end beat. Publish it from the heartbeat on the transition into a turn the row has not yet recorded, so a ~30s beat does not re-announce it and a send-started turn (whose row `_start_turn` already stamped) does not announce twice.
The list badge now rests entirely on the project-wide liveness poll (the row's own flags were dropped as a lagging mirror), so the poll-to-badge mapping is the only thing between a truthful backend and a truthful row. Pin its three states: running, alive-but-idle (a turn parked awaiting approval), and unresolved.
A failed nx acquire is not by itself a takeover: alive outlives its turn, so every follow-up turn on a warm session saw the previous turn's key and was reported interrupted. The running lock is the discriminator. Also gives alive-but-idle rows a 30-minute sweep grace (matching the approval park TTL) and stops announcing running for a killed tombstone.
…o thresholds The failed-acquire branch had no test for the state every follow-up turn is in — a stale alive lock left by this session's own previous turn — so treating it as a takeover shipped unnoticed. Pins the four states that branch must tell apart (warm handover, live takeover, same-turn self-race, resume after a park), the killed-tombstone publish gate, and the sweep's running/idle thresholds by evaluating the real WHERE expression against fake rows.
acquire_running overwrites, so the beat that was just told is_current_turn=false stamped its own dead turn id over the live turn's running key — the exact key the takeover check now reads to tell a warm handover from a real steer. One zombie beat was enough to make the live turn look superseded to the next one, and to resurrect running for a turn a cancel had just cleared.
The cookie authenticates the invoke, but the SDK resolves the model connection by fetching the vault with the caller's Authorization header only. Without it the resumed run got no injected credential and the model rejected it.
The teardown warning carried only the channel, and the except block dropped the exception object, so a genuine Redis failure in production logged nothing to diagnose it with. Teardown stays best-effort; it just says what went wrong.
Teardown ran unsubscribe and aclose in one try block, so an exception from the first skipped the second and the Redis connection outlived the disconnected SSE client. They are separate attempts now, each logging its own failure, both still best-effort. Also corrects a comment on the publish test that claimed failed appends "stay in the existing retry path". There is no such path: process_batch acknowledges at parse time, before the append, and the shared consumer loop deletes what it returns. That predates this work and is shared by every worker on BaseStreamConsumer; the relay tee neither causes it nor repairs it. The comment now says so instead of implying a retry that does not exist.
b67caf8 to
df43ce9
Compare
28906c3 to
090ed5d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.md (1)
14-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the plan for the delivered session-watch relay.
This PR already adds
GET /sessions/streams/watch. It emits metadata notifications, and clients revalidate their existing cache.Lines 14-31 state that the endpoint does not exist. Lines 174-188 and 275-279 schedule a different cursor-and-replay endpoint. Lines 311-315 also mark the relay as pending. Update the status and contract. Keep any future cursor/replay capability as separate work. Otherwise later work can duplicate or replace this API with incompatible behavior.
Also applies to: 174-188, 275-279, 311-315
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79ba048b-8eb4-4db0-9576-d1fb651850ef
📒 Files selected for processing (38)
api/entrypoints/routers.pyapi/entrypoints/worker_queues.pyapi/entrypoints/worker_streams.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/apis/fastapi/sessions/watch.pyapi/oss/src/core/sessions/interactions/service.pyapi/oss/src/core/sessions/streams/service.pyapi/oss/src/dbs/redis/sessions/contract.pyapi/oss/src/dbs/redis/sessions/locks.pyapi/oss/src/dbs/redis/sessions/watch.pyapi/oss/src/tasks/asyncio/sessions/orphan_sweep.pyapi/oss/src/tasks/asyncio/sessions/records_worker.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_mirrors_existing_row.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_stale_turn_end.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_turn_handover.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_clears_redis.pyapi/oss/tests/pytest/unit/sessions/test_orphan_sweep_thresholds.pyapi/oss/tests/pytest/unit/sessions/test_watch_endpoint.pyapi/oss/tests/pytest/unit/sessions/test_watch_interactions_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_lifecycle_publish.pyapi/oss/tests/pytest/unit/sessions/test_watch_publish.pydocs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.mdweb/mobile/src/features/chat/ApprovalDock.tsxweb/mobile/src/features/chat/ChatHeader.tsxweb/mobile/src/features/chat/ChatScreen.tsxweb/mobile/src/features/chat/TurnRow.tsxweb/mobile/src/features/chat/approvalInputSummary.tsweb/mobile/src/features/chat/useApprovalActions.tsweb/mobile/src/features/chat/useSessionTranscript.tsweb/mobile/src/features/chat/useSessionWatch.tsweb/mobile/src/features/chat/watchRelay.tsweb/mobile/src/features/sessions/SessionRow.tsxweb/mobile/src/lib/auth.tsweb/mobile/tests/unit/livenessBadge.test.tsweb/mobile/tests/unit/watchRelay.test.tsweb/mobile/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- api/oss/src/utils/env.py
- api/entrypoints/worker_streams.py
- web/mobile/src/features/chat/useApprovalActions.ts
- api/oss/src/dbs/redis/sessions/contract.py
- web/mobile/src/lib/auth.ts
- web/mobile/tests/unit/livenessBadge.test.ts
- api/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.py
- web/mobile/vitest.config.ts
- api/entrypoints/routers.py
- api/oss/src/tasks/asyncio/sessions/records_worker.py
- web/mobile/tests/unit/watchRelay.test.ts
- api/oss/src/dbs/redis/sessions/locks.py
- api/oss/src/core/sessions/interactions/service.py
- api/entrypoints/worker_queues.py
- web/mobile/src/features/chat/TurnRow.tsx
- api/oss/src/apis/fastapi/sessions/router.py
- api/oss/src/core/sessions/streams/service.py
- api/oss/tests/pytest/unit/sessions/test_orphan_sweep_clears_redis.py
- web/mobile/src/features/chat/ChatHeader.tsx
- api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
- web/mobile/src/features/chat/ApprovalDock.tsx
- api/oss/tests/pytest/unit/sessions/test_watch_interactions_publish.py
- web/mobile/src/features/chat/useSessionTranscript.ts
- api/oss/tests/pytest/unit/sessions/test_watch_lifecycle_publish.py
- web/mobile/src/features/chat/watchRelay.ts
- api/oss/tests/pytest/unit/sessions/test_heartbeat_stale_turn_end.py
- web/mobile/src/features/chat/useSessionWatch.ts
- api/oss/tests/pytest/unit/sessions/test_heartbeat_mirrors_existing_row.py
- api/oss/tests/pytest/unit/sessions/test_orphan_sweep_thresholds.py
- web/mobile/src/features/chat/ChatScreen.tsx
- api/oss/tests/pytest/unit/sessions/test_heartbeat_turn_handover.py
- api/oss/tests/pytest/unit/sessions/test_watch_publish.py
- web/mobile/src/features/chat/approvalInputSummary.ts
- web/mobile/src/features/sessions/SessionRow.tsx
Context
Mobile polled to notice that something changed, which is the wrong shape for a phone: too slow when you are watching a turn, too chatty when you are not. Separately, session liveness was unreliable — rows sat at
is_running=truefor days after a crashed run, so any badge derived from them lied.Changes
A metadata-only SSE relay,
GET /sessions/streams/watch, publishes per-session change notifications on the durable Redis plane. It carries no payload: the client is told that something changed and re-reads through its normal cache, so the stream can never be a second source of truth. Mobile consumes it viauseSessionWatch, and the client reconnect delay is pinned server-side.The liveness fixes are the subtler half. The heartbeat writes its mirror on every beat rather than only the first. A stale
alivelock is treated as handover, not as a turn takeover. A superseded turn's beat can no longer re-arm the running lock — a displaced turn is dead, and its late beats must not resurrect it. The orphan sweep gets two thresholds, one for running turns and a longer one for alive-but-idle sessions, which is what an approval parked for 30 minutes looks like.Tests / notes
What to QA