Skip to content

fix(events): authorize the events queued before a stream opens once - #8718

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/sse-review-nits
Oct 7, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/sse-review-nits

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Review follow-ups to #8700's SSE opening gate.

  • Shared pre-open authorization: events that arrived before a revalidated stream opened each queued their own authorization behind the write chain, so ten of them made ten serial checks. They now share one authorization that starts when the stream opens, while each event is still written in order. The comment on the write chain now says exactly which events share a check.
  • Defensive guards documented: comments explain the two guards no test can reach without depending on promise-scheduling depth:
    • the fast path's empty-queue check, which covers an event sent from a microtask between the opening and the writes queued before it;
    • the rejection handler on each authorization, which keeps a rejection from surfacing as unhandled until the write chain reaches that event.
  • Mock hygiene: the mothership and MCP event route tests reset their ready mocks in beforeEach, so a mockReturnValueOnce cannot leak into the next test.

Type of Change

  • Bug fix

Testing

  • New writes every event queued before it opened once one authorization passes: ten events are queued before the open, and each authorization is a promise the test controls. Settling only the first writes all ten, in order. On staging it fails, because only the first event can be written before the next serial check.
  • SSE, mothership, MCP and desktop route suites pass.
  • The desktop inbox E2E over real HTTP on cold apps passed 3/3.
  • The full gate (lint, typecheck, audits, unit and integration tests) passes.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Events that arrived before the stream opened each queued their own
authorization behind the write chain, so ten of them made ten serial checks.
They now share one that starts when the stream opens, while each is still
written in order.

- Comments explain the two defensive guards no test can reach: the fast
  path's empty-queue check and the rejection handler on each authorization.
- The mothership and MCP event route tests reset their ready mocks between
  tests.
@vercel

vercel Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Oct 7, 2026 2:58am UTC

Request Review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Turn on auto-fix | Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Turn on auto-fix | Re-trigger cubic

@greptile-apps

greptile-apps Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Critical risk] Changes authorization logic for event stream access.

The PR appears safe to merge; no blocking issue remains.

Summary

Events queued before an SSE stream opens now share one authorization check while keeping their write order.

  • The revised test releases one authorization and checks every resulting chunk.
  • Both earlier review threads are addressed: the mock-call assertion is gone, and the full event sequence is checked.
  • The route tests reset their ready mocks between tests.
  • No new actionable issues or changed-line rule violations were found.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Events arrive before opening] --> B[Wait for stream to open]
  B --> C[Share one authorization check]
  C --> D{Check passes?}
  D -->|Yes| E[Write each event in arrival order]
  D -->|No| F[Close the stream]
Loading

Reviews (2) · Last reviewed commit: "test(events): prove the shared pre-open ..." · Reviewed by Greptile

Comment thread apps/sim/lib/events/sse-endpoint.test.ts Outdated
Comment thread apps/sim/lib/events/sse-endpoint.test.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Turn on auto-fix | Re-trigger cubic

@waleedlatif1
waleedlatif1 merged commit ab588d4 into staging Oct 7, 2026
33 of 34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/sse-review-nits branch October 7, 2026 05:48

This branch was previously deployed

1 inactive deployment
Preview — b37a7d9f Deployed Oct 7, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant