Skip to content

Commit 5e00f40

Browse files
committed
fix(api): revalidate on a ready event, not on the connection opening
Both watch clients revalidated from `onopen`, which fires as soon as the response headers arrive. Starlette flushes those BEFORE it starts iterating the stream generator, so the sequence could be: headers out, client refetches the record log, a change lands and publishes, and only then does the generator's `subscribe` complete. That change reaches neither the refetch nor the stream, and on an idle screen the fallback poll is zero, so the transcript stays stale until something else wakes it. The endpoint now emits a `ready` event once the subscription is live, right behind the `retry:` preamble, and both clients revalidate on that instead. `onopen` keeps only what it can honestly report: the connection is up, so reset the backoff. The new test drives the generator and asserts the channel is already subscribed by the time `ready` is yielded, which is the whole invariant.
1 parent 2e2819d commit 5e00f40

5 files changed

Lines changed: 69 additions & 16 deletions

File tree

api/oss/src/apis/fastapi/sessions/watch.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
from oss.src.dbs.redis.sessions.contract import (
1414
WATCH_EVENT_INTERACTION,
1515
WATCH_EVENT_LIFECYCLE,
16+
WATCH_EVENT_READY,
1617
WATCH_EVENT_RECORDS_CHANGED,
1718
)
1819
from oss.src.utils.logging import get_module_logger
@@ -27,6 +28,11 @@ def retry_frame(retry_milliseconds: int) -> str:
2728
return f"retry: {retry_milliseconds}\n\n"
2829

2930

31+
def ready_frame() -> str:
32+
"""Emitted once the Redis subscription is live: the client's cue to revalidate."""
33+
return "event: " + WATCH_EVENT_READY + "\ndata: {}\n\n"
34+
35+
3036
_KNOWN_EVENTS = {
3137
WATCH_EVENT_RECORDS_CHANGED,
3238
WATCH_EVENT_LIFECYCLE,
@@ -62,8 +68,15 @@ async def watch_event_stream(
6268
6369
The first frame is a ``retry:`` preamble: it pins the client's built-in
6470
auto-reconnect delay (implementation-defined otherwise) so a server-side
65-
drop — an API restart, a deploy — cannot reconnect-storm us. It also flushes
66-
the response headers, so the client sees ``open`` before the first event.
71+
drop — an API restart, a deploy — cannot reconnect-storm us.
72+
73+
The second is a ``ready`` event, and that is what a client revalidates on.
74+
``onopen`` fires as soon as the response headers arrive, and Starlette flushes
75+
those BEFORE it starts iterating this generator — so a revalidation driven by
76+
``onopen`` can read the record log, and a change can land and publish, all before
77+
the ``subscribe`` below completes. That change would reach neither the refetch nor
78+
the stream. ``ready`` is emitted once the subscription is live, so a revalidation
79+
keyed on it cannot straddle the gap.
6780
6881
The subscription is torn down in ``finally`` — a client disconnect cancels
6982
the generator (GeneratorExit/CancelledError), which is exactly the cleanup
@@ -73,6 +86,7 @@ async def watch_event_stream(
7386
try:
7487
await pubsub.subscribe(channel)
7588
yield retry_frame(retry_milliseconds)
89+
yield ready_frame()
7690
while True:
7791
message = await pubsub.get_message(
7892
ignore_subscribe_messages=True,

api/oss/src/dbs/redis/sessions/contract.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ def make_displacement_payload(*, by: str) -> dict:
100100
WATCH_EVENT_RECORDS_CHANGED = "records-changed"
101101
WATCH_EVENT_LIFECYCLE = "lifecycle"
102102
WATCH_EVENT_INTERACTION = "interaction"
103+
# Emitted by the SSE endpoint itself, never published: it marks the point where the Redis
104+
# subscription is live, so a client can revalidate without racing the events it is about to
105+
# start receiving.
106+
WATCH_EVENT_READY = "ready"
103107

104108
WATCH_LIFECYCLE_RUNNING = "running"
105109
WATCH_LIFECYCLE_ENDED = "ended"

api/oss/tests/pytest/unit/sessions/test_watch_endpoint.py

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
from oss.src.apis.fastapi.sessions.watch import (
1919
HEARTBEAT_FRAME,
2020
format_watch_frame,
21+
ready_frame,
2122
retry_frame,
2223
watch_event_stream,
2324
)
@@ -73,22 +74,24 @@ async def test_stream_yields_event_frames_then_heartbeats():
7374
frames = []
7475
async for frame in stream:
7576
frames.append(frame)
76-
if len(frames) == 5:
77+
if len(frames) == 6:
7778
await stream.aclose()
7879
break
7980

80-
# The preamble pins the client's built-in auto-reconnect delay.
81+
# The preamble pins the client's built-in auto-reconnect delay, then `ready` marks the
82+
# point where the subscription is live and a client may safely revalidate.
8183
assert frames[0] == retry_frame(5000)
82-
assert frames[1].startswith("event: records-changed\n")
83-
assert json.loads(frames[1].split("data: ")[1]) == {
84+
assert frames[1] == ready_frame()
85+
assert frames[2].startswith("event: records-changed\n")
86+
assert json.loads(frames[2].split("data: ")[1]) == {
8487
"type": "records-changed",
8588
"session_id": "s1",
8689
}
87-
assert frames[2].startswith("event: lifecycle\n")
88-
assert '"state": "running"' in frames[2]
89-
assert frames[3].startswith("event: interaction\n")
90+
assert frames[3].startswith("event: lifecycle\n")
91+
assert '"state": "running"' in frames[3]
92+
assert frames[4].startswith("event: interaction\n")
9093
# Queue drained -> the idle path emits keep-alive comments.
91-
assert frames[4] == HEARTBEAT_FRAME
94+
assert frames[5] == HEARTBEAT_FRAME
9295
assert pubsub.subscribed == ["watch:p:session:s1"]
9396

9497

@@ -103,6 +106,7 @@ async def test_stream_cleans_up_subscription_on_close():
103106
)
104107
# Take the preamble + one heartbeat, then simulate the client disconnecting.
105108
assert await stream.__anext__() == retry_frame(5000)
109+
assert await stream.__anext__() == ready_frame()
106110
assert await stream.__anext__() == HEARTBEAT_FRAME
107111
await stream.aclose()
108112

@@ -127,6 +131,7 @@ async def test_stream_skips_malformed_and_unknown_payloads():
127131
retry_milliseconds=5000,
128132
)
129133
assert await stream.__anext__() == retry_frame(5000)
134+
assert await stream.__anext__() == ready_frame()
130135
frame = await stream.__anext__()
131136
await stream.aclose()
132137
# The three junk messages are dropped; the first frame is the real event.
@@ -189,6 +194,7 @@ async def test_stream_delivers_publisher_events_end_to_end():
189194
)
190195
# Preamble, then a heartbeat — proves the subscription is live before publishing.
191196
assert await stream.__anext__() == retry_frame(5000)
197+
assert await stream.__anext__() == ready_frame()
192198
assert await stream.__anext__() == HEARTBEAT_FRAME
193199

194200
publisher = SessionsWatchPublisher(redis_client=redis)
@@ -295,3 +301,26 @@ async def test_watch_endpoint_streams_the_configured_retry_preamble():
295301
await response.body_iterator.aclose()
296302

297303
assert first == "retry: 9000\n\n"
304+
305+
306+
@pytest.mark.asyncio
307+
async def test_ready_is_not_emitted_before_the_subscription_is_live():
308+
"""The whole point of the `ready` event. A client revalidating on `onopen` races the
309+
subscription: Starlette flushes the response headers before it iterates this generator, so a
310+
change can land and publish in between and reach neither the refetch nor the stream. `ready`
311+
is only reachable after `subscribe` has returned."""
312+
pubsub = _FakePubSub([])
313+
stream = watch_event_stream(
314+
channel="watch:p:session:s1",
315+
pubsub_factory=lambda: pubsub,
316+
heartbeat_seconds=0.01,
317+
retry_milliseconds=5000,
318+
)
319+
320+
assert await stream.__anext__() == retry_frame(5000)
321+
assert await stream.__anext__() == ready_frame()
322+
assert pubsub.subscribed == ["watch:p:session:s1"], (
323+
"ready reached the client before the channel was subscribed"
324+
)
325+
326+
await stream.aclose()

web/mobile/src/features/chat/useSessionWatch.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,15 @@ export const useSessionWatch = ({
9696
es.onopen = () => {
9797
setConnected(true)
9898
attempt = 0
99-
// Missed-event coverage: one revalidation per (re)connect replaces
100-
// any replay/cursor semantics on the server.
99+
}
100+
// Missed-event coverage: one revalidation per (re)connect replaces any
101+
// replay/cursor semantics on the server. Keyed on `ready`, not `onopen`:
102+
// headers reach us before the server's Redis subscription is live, so a
103+
// change landing in that window would miss both this refetch and the stream.
104+
es.addEventListener("ready", () => {
101105
notifyOnConnect()
102106
invalidateBadges()
103-
}
107+
})
104108
es.addEventListener("records-changed", () => onRecordsChangedRef.current())
105109
es.addEventListener("lifecycle", invalidateBadges)
106110
es.addEventListener("interaction", invalidateBadges)

web/oss/src/components/AgentChatSlice/hooks/useSessionRecordsWatch.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,12 +109,14 @@ export const useSessionRecordsWatch = ({
109109
withCredentials: true,
110110
})
111111
source = es
112-
// One revalidation per (re)connect covers events missed while disconnected — the
113-
// server has no replay/cursor semantics.
114112
es.onopen = () => {
115113
attempt = 0
116-
notify()
117114
}
115+
// One revalidation per (re)connect covers events missed while disconnected — the
116+
// server has no replay/cursor semantics. Keyed on `ready` rather than `onopen`:
117+
// headers arrive before the server's Redis subscription is live, so a change
118+
// landing in that window would miss both this refetch and the stream.
119+
es.addEventListener("ready", notify)
118120
es.addEventListener("records-changed", notify)
119121
es.onerror = () => {
120122
// CONNECTING = built-in auto-reconnect; only a fatal CLOSED needs us.

0 commit comments

Comments
 (0)