Skip to content

fix(contract): eventIterator should cleanup on validation failure - #715

Merged
dinwwwh merged 1 commit into
mainfrom
fix/contract/event-iterator-schema-not-cleanup-on-failed
Jul 2, 2025
Merged

fix(contract): eventIterator should cleanup on validation failure#715
dinwwwh merged 1 commit into
mainfrom
fix/contract/event-iterator-schema-not-cleanup-on-failed

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Jul 2, 2025

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes

    • Improved cleanup handling for async generators when errors occur during value mapping or validation, ensuring proper cancellation and resource release.
  • Tests

    • Added test cases to verify that cleanup logic runs correctly when errors are thrown in value mapping or when validation fails in async generators.

@vercel

vercel Bot commented Jul 2, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
orpc ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 2, 2025 4:17am

@coderabbitai

coderabbitai Bot commented Jul 2, 2025

Copy link
Copy Markdown

Walkthrough

This update introduces new test cases to verify proper cleanup and cancellation behavior in async iterators when errors or validation failures occur. It also modifies the internal cleanup logic of the mapEventIterator function to always await iterator cleanup, simplifying and strengthening resource management during error handling.

Changes

File(s) Change Summary
packages/client/src/event-iterator.test.ts Added a test verifying that the original async iterator is cancelled if an error is thrown in the mapping function.
packages/client/src/event-iterator.ts Updated mapEventIterator to always await iterator.return?.() during cleanup, removing conditional logic.
packages/contract/src/event-iterator.test.ts Added a test ensuring generator cleanup logic runs when validation of yielded values fails.

Sequence Diagram(s)

sequenceDiagram
    participant Test as Test Suite
    participant MapIter as mapEventIterator
    participant Gen as Async Generator

    Test->>MapIter: Start mapped iteration
    MapIter->>Gen: next()
    Gen-->>MapIter: value1
    MapIter-->>Test: mapped value1

    MapIter->>Gen: next()
    Gen-->>MapIter: value2
    MapIter-->>Test: throw error

    Note over MapIter,Gen: Cleanup triggered
    MapIter->>Gen: return()
    Gen-->>MapIter: (finally block runs)
Loading

Possibly related PRs

  • unnoq/orpc#619: Refactored mapEventIterator to use AsyncIteratorClass and updated cleanup logic, which is directly extended and tested in this PR.

Poem

In the garden where iterators hop,
A cleanup dance begins when errors drop.
With every yield and every throw,
The rabbits ensure resources go.
Now tests confirm, with gentle cheer,
That nothing messy lingers here! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbcbae2 and 23a128d.

📒 Files selected for processing (3)
  • packages/client/src/event-iterator.test.ts (1 hunks)
  • packages/client/src/event-iterator.ts (1 hunks)
  • packages/contract/src/event-iterator.test.ts (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/contract/src/event-iterator.test.ts (1)
packages/contract/src/event-iterator.ts (1)
  • eventIterator (18-61)
packages/client/src/event-iterator.test.ts (1)
packages/client/src/event-iterator.ts (1)
  • mapEventIterator (4-41)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: publish-commit
  • GitHub Check: lint
🔇 Additional comments (3)
packages/client/src/event-iterator.ts (1)

38-40: Excellent improvement to cleanup logic!

The change from conditional to unconditional cleanup ensures that iterator.return?.() is always awaited, which properly addresses the PR objective of ensuring cleanup happens on validation failure. This simplification strengthens resource management and eliminates potential edge cases where cleanup might be skipped.

packages/client/src/event-iterator.test.ts (1)

159-188: Comprehensive test coverage for error handling and cleanup!

This test case effectively validates that the unconditional cleanup logic works correctly when errors are thrown in the value mapping function. The test design is solid:

  • Uses a generator with a finally block to track cleanup
  • Tests both successful and error scenarios in sequence
  • Verifies that cleanup occurs even when mapping functions throw errors

This directly supports the cleanup improvements made in the main implementation.

packages/contract/src/event-iterator.test.ts (1)

92-110: Perfect test case for the primary PR objective!

This test directly validates the core issue mentioned in the PR title - ensuring cleanup happens when validation fails. The test design effectively demonstrates:

  • Proper cleanup behavior when validation fails on invalid data ({ order: '2' })
  • The cleanupCalled flag confirms the generator's finally block executes
  • Validates both successful validation and failure scenarios in sequence

This test case provides confidence that the cleanup improvements work correctly for the specific validation failure scenario that motivated this PR.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 2, 2025

@gemini-code-assist gemini-code-assist 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.

Summary of Changes

Hello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical resource management issue within the eventIterator and mapEventIterator utilities. It ensures that underlying iterators are reliably closed and cleaned up whenever an error, such as a data validation failure, interrupts the iteration process. This enhancement improves the stability and resource handling of these core components, preventing potential leaks and ensuring predictable behavior.

Highlights

  • Bug Fix: Iterator Cleanup: This pull request fixes a bug where eventIterator and mapEventIterator did not consistently clean up their underlying iterators when an error, such as a data validation failure, occurred during iteration. This could lead to resource leaks or unexpected behavior.
  • Improved Error Handling: The mapEventIterator function in packages/client/src/event-iterator.ts has been updated to ensure that the original iterator's return() method is always called when an error is thrown, simplifying the cleanup logic and making it more robust.
  • New Test Cases: New test cases have been added in both packages/client/src/event-iterator.test.ts and packages/contract/src/event-iterator.test.ts to specifically verify that iterators are properly cleaned up when errors (including validation errors) occur during the mapping or validation process.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@codecov

codecov Bot commented Jul 2, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces a test case to verify that the underlying event iterator is cleaned up upon a validation failure. The change ensures that the cleanup function is always called, addressing the issue effectively. The added test in the contract package provides coverage for the fix.

Comment thread packages/contract/src/event-iterator.test.ts
@pkg-pr-new

pkg-pr-new Bot commented Jul 2, 2025

Copy link
Copy Markdown
More templates

@orpc/arktype

npm i https://pkg.pr.new/@orpc/arktype@715

@orpc/client

npm i https://pkg.pr.new/@orpc/client@715

@orpc/contract

npm i https://pkg.pr.new/@orpc/contract@715

@orpc/experimental-durable-event-iterator

npm i https://pkg.pr.new/@orpc/experimental-durable-event-iterator@715

@orpc/hey-api

npm i https://pkg.pr.new/@orpc/hey-api@715

@orpc/nest

npm i https://pkg.pr.new/@orpc/nest@715

@orpc/openapi

npm i https://pkg.pr.new/@orpc/openapi@715

@orpc/openapi-client

npm i https://pkg.pr.new/@orpc/openapi-client@715

@orpc/react

npm i https://pkg.pr.new/@orpc/react@715

@orpc/react-query

npm i https://pkg.pr.new/@orpc/react-query@715

@orpc/server

npm i https://pkg.pr.new/@orpc/server@715

@orpc/shared

npm i https://pkg.pr.new/@orpc/shared@715

@orpc/solid-query

npm i https://pkg.pr.new/@orpc/solid-query@715

@orpc/standard-server

npm i https://pkg.pr.new/@orpc/standard-server@715

@orpc/standard-server-aws-lambda

npm i https://pkg.pr.new/@orpc/standard-server-aws-lambda@715

@orpc/standard-server-fetch

npm i https://pkg.pr.new/@orpc/standard-server-fetch@715

@orpc/standard-server-node

npm i https://pkg.pr.new/@orpc/standard-server-node@715

@orpc/standard-server-peer

npm i https://pkg.pr.new/@orpc/standard-server-peer@715

@orpc/svelte-query

npm i https://pkg.pr.new/@orpc/svelte-query@715

@orpc/tanstack-query

npm i https://pkg.pr.new/@orpc/tanstack-query@715

@orpc/trpc

npm i https://pkg.pr.new/@orpc/trpc@715

@orpc/valibot

npm i https://pkg.pr.new/@orpc/valibot@715

@orpc/vue-colada

npm i https://pkg.pr.new/@orpc/vue-colada@715

@orpc/vue-query

npm i https://pkg.pr.new/@orpc/vue-query@715

@orpc/zod

npm i https://pkg.pr.new/@orpc/zod@715

commit: 23a128d

@dinwwwh
dinwwwh merged commit 4008821 into main Jul 2, 2025
8 checks passed
@dinwwwh
dinwwwh deleted the fix/contract/event-iterator-schema-not-cleanup-on-failed branch June 22, 2026 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant