fix(streams): a join on the wrong machine stops claiming the stream does not exist - #2420
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
84a9991 to
c2372ad
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84a9991b3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (joinContext.kind !== 'local') { | ||
| return NextResponse.json({ error: 'Stream not found' }, { status: 404 }); |
There was a problem hiding this comment.
When an authorized join is routed to a non-owning instance, resolveStreamJoinContext() now classifies it as remote, but this branch still returns the exact 404 Stream not found response that caused the reported problem. Because the client can observe only that HTTP response—not the internal classification—it still treats a live stream as missing and falls back to polling; the new database and authorization queries provide no cross-instance join behavior. Handle the remote context through the durable follower or expose a meaningful externally consumable result in this change rather than deferring the actual solution.
AGENTS.md reference: AGENTS.md:L89-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right about the code as it stands in this branch, and the deferral is deliberate rather than accidental — this is the middle PR of a three-PR stack, and the follower that serves the remote context is #2421, which is already open and stacked directly on this branch. Not visible from here, which is why the finding reads the way it does; my apologies for not making the stack legible in the description (now fixed).
The split is:
- fix(streams): a second machine can no longer delete a live reply #2419 — reap by atomic claim (base:
master) - fix(streams): a join on the wrong machine stops claiming the stream does not exist #2420 — this PR: classify the join and move authorization ahead of the registry lookup (base:
pu/reap-claim) - feat(streams): a reply generated on one machine now streams to every machine #2421 — the durable-log follower that actually serves
remoteandterminal(base:pu/stream-join-context)
What this PR does on its own is narrower than the title suggests, and it is not nothing: the registry lookup used to sit ahead of the authz block, structurally, because the authz inputs (pageId, conversationId, stream owner) lived in the same in-memory entry as the frames. So a cross-instance join 404d before anyone asked who the caller was. After this PR the row supplies those inputs and the authz block runs verbatim over them — which is exactly what makes #2421 a ~40-line follower rather than a second authorization path.
Externally observable behaviour is intentionally unchanged here: remote/terminal still 404, and the clients existing poll fallback handles that exactly as today, so cross-instance delivery is not regressed while the stack lands in order. The one visible change is that a caller who may not subscribe is now refused for the right reason instead of by accident of topology.
If you would prefer these squashed into one PR rather than reviewed as a stack, say so and I will collapse #2420 and #2421.
c2372ad to
6414dd4
Compare
…oes not exist `stream-join`'s first gate was `streamChannelRegistry.getMeta`, and it sat AHEAD of the authz block. Not by oversight — structurally: the authz inputs (pageId, conversationId, the stream's owner) lived in the same in-memory entry as the frames, so there was nothing to authorize against until the registry answered. At N>1 a registry miss is the ORDINARY case, because the channel registry is a process-local Map. Every join that load-balanced anywhere but the generating instance 404'd, saying "no such stream" about streams that were very much running. `ai_stream_sessions` already holds every one of those inputs — `stream-lifecycle` writes the row and the registry entry from the same values in the same function — so on a miss they are simply read. New `stream-join-context.ts` answers `local | remote | terminal | missing`, mapping the row to EXACTLY the `StreamMeta` shape the registry produces (`pageId ← channelId`, everything else one-for-one). THAT IS THE WHOLE TRICK: because the shape is identical, the route's authz block is UNCHANGED. `parseGlobalChannelId` → `canUserViewPage` → `canSubscribeToStream`, and the 5s revocation recheck, all run verbatim over DB-sourced meta. There is no second authorization path to keep in step with the first — which is how a cross-instance feature quietly acquires a weaker permission model than the local one it mirrors. `filterSubscribableStreams` stays where it is: it is the batched form, for the `active-streams` listing. A registry hit never pays for the read. A failed read answers `missing`, the same degradation the route already had. `remote` and `terminal` still 404 in this leaf — there is no channel to read from until the durable log's follower lands next — so cross-instance delivery is unchanged and the client's poll fallback handles it exactly as today. What changes here is that the answer is classified, and that a caller who may not subscribe is refused for the right reason rather than by accident of topology. Five seeded mutations verified red, including collapsing `remote` back into `missing`, sourcing the owner from the wrong column, and failing open on an unreadable row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQwbEoZweBEBgsGq9wnqXj
6414dd4 to
20552d2
Compare
86b114a to
37997db
Compare
1 participant
Stack
This is one of three stacked PRs, to be reviewed and merged bottom-up:
masterpu/reap-claimpu/stream-join-contextEach PR is a strict improvement on its own, but the user-visible cross-instance join only works once all three land. Workstream A of "make AI streaming safe at more than one machine" — the goal is raising
pagespace-webfrommin_machines_running = 1to 2.stream-join's first gate wasstreamChannelRegistry.getMeta, and it sat AHEADof the authz block. Not by oversight — structurally: the authz inputs (pageId,
conversationId, the stream's owner) lived in the same in-memory entry as the
frames, so there was nothing to authorize against until the registry answered.
At N>1 a registry miss is the ORDINARY case, because the channel registry is a
process-local Map. Every join that load-balanced anywhere but the generating
instance 404'd, saying "no such stream" about streams that were very much
running.
ai_stream_sessionsalready holds every one of those inputs —stream-lifecyclewrites the row and the registry entry from the same values in the same function
— so on a miss they are simply read.
New
stream-join-context.tsanswerslocal | remote | terminal | missing,mapping the row to EXACTLY the
StreamMetashape the registry produces(
pageId ← channelId, everything else one-for-one).THAT IS THE WHOLE TRICK: because the shape is identical, the route's authz block
is UNCHANGED.
parseGlobalChannelId→canUserViewPage→canSubscribeToStream,and the 5s revocation recheck, all run verbatim over DB-sourced meta. There is no
second authorization path to keep in step with the first — which is how a
cross-instance feature quietly acquires a weaker permission model than the local
one it mirrors.
filterSubscribableStreamsstays where it is: it is the batchedform, for the
active-streamslisting.A registry hit never pays for the read. A failed read answers
missing, the samedegradation the route already had.
remoteandterminalstill 404 in this leaf — there is no channel to readfrom until the durable log's follower lands next — so cross-instance delivery is
unchanged and the client's poll fallback handles it exactly as today. What
changes here is that the answer is classified, and that a caller who may not
subscribe is refused for the right reason rather than by accident of topology.
Five seeded mutations verified red, including collapsing
remoteback intomissing, sourcing the owner from the wrong column, and failing open on anunreadable row.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01SQwbEoZweBEBgsGq9wnqXj
Stacked on #2419.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SQwbEoZweBEBgsGq9wnqXj