Skip to content

fix: protect against running out of memory#3644

Merged
chronark merged 1 commit intomainfrom
chproxy-oom
Jul 22, 2025
Merged

fix: protect against running out of memory#3644
chronark merged 1 commit intomainfrom
chproxy-oom

Conversation

@chronark
Copy link
Collaborator

@chronark chronark commented Jul 22, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a maximum row limit per batch to prevent oversized batches and improve reliability.
    • Added explicit error handling and user feedback when the service is overloaded, returning a 529 status code if the buffer is full.
  • Bug Fixes

    • Improved logging and error reporting for dropped rows and failed batch persistence.
  • Chores

    • Enhanced internal logic for buffer management and clarified memory release behavior.

@vercel
Copy link

vercel bot commented Jul 22, 2025

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

Name Status Preview Comments Updated (UTC)
dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 22, 2025 0:24am
engineering ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 22, 2025 0:24am

@changeset-bot
Copy link

changeset-bot bot commented Jul 22, 2025

⚠️ No Changeset found

Latest commit: 25dcb4c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 22, 2025

📝 Walkthrough

Walkthrough

The changes introduce a maximum row limit for batches in the buffer processor, ensuring batches do not exceed 5,000 rows by flushing and resetting as needed. The HTTP insert handler now uses non-blocking channel operations to drop and log batches when the buffer is full, returning a 529 status code for overload conditions.

Changes

File(s) Change Summary
apps/chproxy/buffer.go Added logic to enforce a per-batch maximum row limit, flush batches early if exceeded, and improved error logging.
apps/chproxy/main.go Introduced maxBatchRows constant, changed insert handler to non-blocking buffer send, added overload handling.

Estimated code review effort

2 (~15 minutes)


📜 Recent review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 49fef6c and 25dcb4c.

📒 Files selected for processing (2)
  • apps/chproxy/buffer.go (2 hunks)
  • apps/chproxy/main.go (2 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.359Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
apps/chproxy/buffer.go (2)

Learnt from: chronark
PR: #3180
File: go/pkg/buffer/buffer.go:128-143
Timestamp: 2025-04-23T14:59:37.294Z
Learning: In Go buffer implementations using channels, the Close() method should be made thread-safe (using mechanisms like sync.Once and/or atomic flags) to prevent race conditions with concurrent Buffer() calls, as sending to a closed channel causes a panic.

Learnt from: Flo4604
PR: #3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.464Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.

apps/chproxy/main.go (2)

Learnt from: Flo4604
PR: #3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.359Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.

Learnt from: Flo4604
PR: #3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.464Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.

🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.359Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
apps/chproxy/buffer.go (2)

Learnt from: chronark
PR: #3180
File: go/pkg/buffer/buffer.go:128-143
Timestamp: 2025-04-23T14:59:37.294Z
Learning: In Go buffer implementations using channels, the Close() method should be made thread-safe (using mechanisms like sync.Once and/or atomic flags) to prevent race conditions with concurrent Buffer() calls, as sending to a closed channel causes a panic.

Learnt from: Flo4604
PR: #3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.464Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.

apps/chproxy/main.go (2)

Learnt from: Flo4604
PR: #3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.359Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.

Learnt from: Flo4604
PR: #3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.464Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.

⏰ 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). (3)
  • GitHub Check: Test Packages / Test
  • GitHub Check: autofix
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/chproxy/main.go (2)

25-25: Well-defined constant for memory protection.

The maxBatchRows constant provides a clear limit for individual batch sizes, which aligns with the PR's objective to protect against memory exhaustion.


164-179: Excellent implementation of non-blocking backpressure handling.

The change from blocking channel sends to a non-blocking select statement effectively prevents the service from hanging when the buffer is full. The implementation includes:

  • Proper error metrics increment
  • Comprehensive logging with relevant context
  • Appropriate HTTP 529 status code for overload conditions
  • Correct OpenTelemetry span error handling

This approach provides excellent backpressure control and maintains service responsiveness under high load.

apps/chproxy/buffer.go (4)

48-51: Enhanced error logging improves observability.

Adding the rows_dropped field to error logs provides valuable insight into the impact of persistence failures, which is crucial for monitoring and debugging.


55-55: Good documentation of memory management.

The comment clarifies when memory will be freed, which helps with understanding the lifecycle of batch data.


91-117: Robust implementation of per-batch size limit enforcement.

The logic effectively prevents individual batches from exceeding maxBatchRows by:

  • Checking if appending new rows would exceed the limit
  • Flushing the current batch immediately when needed
  • Properly updating the buffered count (subtracting old rows before reset)
  • Handling persist errors gracefully while still freeing memory
  • Resetting the batch to contain only the new incoming rows

The implementation correctly maintains consistency between the buffered counter and actual data, ensuring accurate memory tracking.


120-122: Clear documentation of buffer count management.

The comment clarifies that incoming rows are always added to the buffer count, which helps understand the accounting logic in this complex flow.

✨ 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.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2025

Thank you for following the naming conventions for pull request titles! 🙏

@chronark chronark merged commit 6f974bc into main Jul 22, 2025
20 checks passed
@chronark chronark deleted the chproxy-oom branch July 22, 2025 12:26
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