Skip to content

fix(rivetkit-core): chunk apply_batch puts/deletes at 128#5012

Open
abcxff wants to merge 2 commits into
v4-envoy-protocolfrom
05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128
Open

fix(rivetkit-core): chunk apply_batch puts/deletes at 128#5012
abcxff wants to merge 2 commits into
v4-envoy-protocolfrom
05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128

Conversation

@abcxff
Copy link
Copy Markdown
Contributor

@abcxff abcxff commented May 11, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 11, 2026

🚅 Deployed to the rivet-pr-5012 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web May 11, 2026 at 11:29 pm
website 😴 Sleeping (View Logs) Web May 11, 2026 at 3:53 am
frontend-inspector ❌ Build Failed (View Logs) Web May 11, 2026 at 3:43 am
mcp-hub ✅ Success (View Logs) Web May 11, 2026 at 3:43 am
ladle ❌ Build Failed (View Logs) Web May 11, 2026 at 3:43 am
frontend-cloud ❌ Build Failed (View Logs) Web May 11, 2026 at 3:43 am

Copy link
Copy Markdown
Contributor Author

abcxff commented May 11, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@abcxff abcxff marked this pull request as ready for review May 11, 2026 03:43
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

PR Review: fix(rivetkit-core): chunk apply_batch puts/deletes at 128

Overview

This PR fixes a bug where apply_batch calls with more than 128 puts or deletes would be rejected by the engine (the engine enforces MAX_KEYS = 128 per list in engine/packages/pegboard/src/actor_kv/utils.rs). The fix introduces a chunking loop so large batches are split into ≤128-key sub-batches.


Critical Issues

1. Accidental v4 file committed to repo root

An empty file named v4 appears in the diff at the repo root. This needs to be removed before merging.

2. Payload size limit not addressed

The engine enforces both a key-count limit (MAX_KEYS = 128) and a payload size limit (MAX_PUT_PAYLOAD_SIZE = 976 KiB) per put batch. Since individual values can be up to MAX_VALUE_SIZE = 128 KiB, a chunk of 128 keys could theoretically be up to 16 MiB — far exceeding the 976 KiB limit. The engine would still reject such a request. This PR may not have fully solved the problem for actors with large values.


Correctness Concern: Non-Atomic Batching

The // TODO: make this atomic; store in sqlite comment acknowledges the key regression: previously a single apply_batch was all-or-nothing. Now, if the N-th chunk fails, the first N-1 chunks have already been committed, leaving the actor in a partially-applied state. This is a meaningful correctness regression for state persistence and should be tracked as a follow-up, not just a TODO comment. Consider opening a tracked issue or adding a .agent/todo/ entry.


Code Style (CLAUDE.md violations)

Per the project guidelines, comments must be complete sentences and avoid semicolons as sentence separators.

kv.rs constant comment uses a semicolon to join two sentences. Prefer a period: ...mod.rs. The envoy backend rejects requests above this.`

state.rs TODO comment is fragmented: // TODO: make this atomic; store in sqlite. Prefer: // TODO: Make this atomic. The engine does not guarantee atomicity across chunks.


Minor Suggestions

Chunking independence: puts and deletes are chunked at the same rate and paired per iteration. If puts has 200 items and deletes has 10 items, the first batch sends puts[0..128] + all 10 deletes, and the second sends puts[128..200] + nothing. This is correct per the validation rules (each list is checked independently), but a short comment would help future readers.

No tests added: The in-memory KV backend already exposes test_apply_batch_call_count() and test_last_apply_batch() for exactly this use case. A test that builds a state delta with more than 128 puts and asserts the call count is at least 2 would confirm the chunking behavior.


Summary

Area Status
Root cause fix Correct — engine-side MAX_KEYS = 128 is the right limit
Payload size limit Not addressed — large values could still exceed MAX_PUT_PAYLOAD_SIZE = 976 KiB
Atomicity Regression acknowledged but not tracked as a real issue
Accidental file v4 must be removed before merging
Comment style Two minor CLAUDE.md violations
Tests None added

@abcxff abcxff requested a review from NathanFlurry May 11, 2026 04:03
@abcxff abcxff changed the base branch from 05-07-fix_pegboard_validate_drain_grace_period_request_lifespan to graphite-base/5012 May 11, 2026 22:17
@abcxff abcxff force-pushed the graphite-base/5012 branch from aac9634 to 8a7a683 Compare May 11, 2026 22:17
@abcxff abcxff force-pushed the 05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128 branch from 3d96914 to ccf09c6 Compare May 11, 2026 22:17
@abcxff abcxff changed the base branch from graphite-base/5012 to v4-envoy-protocol May 11, 2026 22:17
@abcxff abcxff force-pushed the v4-envoy-protocol branch from 8a7a683 to fd7d9c2 Compare May 11, 2026 23:28
@abcxff abcxff force-pushed the 05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128 branch from ccf09c6 to f268b35 Compare May 11, 2026 23:28
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