fix(realtime): waitUntil now correctly waits for metadata.streams to finish #2399
Conversation
🦋 Changeset detectedLatest commit: c1b096f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli-v3/src/entryPoints/managed-run-worker.ts (1)
293-298: Avoid future drift between dev-run and managed-run by extracting a shared helper (optional)The registration block is duplicated in dev-run-worker and managed-run-worker. Consider a small shared helper to reduce maintenance overhead.
packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
307-312: Optional: factor out a tiny helper to keep both entry points in lockstepThe registration logic is identical here and in managed-run-worker. Extracting a local helper (or small shared utility) would minimize the chance of future divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/healthy-oranges-refuse.md(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(1 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/cli-v3/src/entryPoints/dev-run-worker.tspackages/cli-v3/src/entryPoints/managed-run-worker.ts
🧬 Code Graph Analysis (2)
packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
packages/core/src/v3/workers/taskExecutor.ts (1)
waitUntil(1092-1109)
packages/cli-v3/src/entryPoints/managed-run-worker.ts (1)
packages/core/src/v3/workers/taskExecutor.ts (1)
waitUntil(1092-1109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
.changeset/healthy-oranges-refuse.md (1)
1-6: Changeset accurately captures scope and impactPatch-level note is clear and aligns with the code changes: "waitUntil now correctly waits for metadata.streams to finish."
packages/cli-v3/src/entryPoints/managed-run-worker.ts (2)
293-298: Correct placement of waitUntil registration within resetExecutionEnvironmentRegistering the waitUntil predicate after resetting managers ensures it’s bound to the fresh runMetadataManager state each run. This addresses the prior stale-registration issue and guarantees TaskExecutor’s waitUntil block will reflect current streams.
293-298: No module-level waitUntil.register found — registrations are per-run and reset beforehandI inspected the codebase. The only waitUntil.register calls are inside resetExecutionEnvironment() in the run worker files and each is preceded by a manager reset, so they don't leave stale top-level predicates.
Locations:
- packages/cli-v3/src/entryPoints/managed-run-worker.ts — resetExecutionEnvironment() (around lines 286–297): register is added after runMetadataManager.reset() and waitUntilManager.reset().
- packages/cli-v3/src/entryPoints/dev-run-worker.ts — resetExecutionEnvironment() (around lines 300–311): same pattern.
- packages/trigger-sdk/src/v3/waitUntil.ts — thin wrapper that delegates to core waitUntil.register.
- packages/core/src/v3/workers/taskExecutor.ts — uses waitUntil.requiresResolving() and waitUntil.blockUntilSettled() to wait for registered promises.
Relevant snippet (updated to show the reset + registration sequence):
runMetadataManager.reset(); waitUntilManager.reset(); // Wait for all streams to finish before completing the run waitUntil.register({ requiresResolving: () => runMetadataManager.hasActiveStreams(), promise: () => runMetadataManager.waitForAllStreams(), });Conclusion: no module-top-level registrations were found and the per-run registrations are guarded by a prior reset — no change required.
packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
307-312: Good fix: registration now correctly binds to the current run stateMoving the waitUntil registration into resetExecutionEnvironment, after resets, ensures the predicate and promise reflect the current run’s streams. Matches the managed variant for consistent behavior.
No description provided.