Skip to content

fix(rivetkit): unwrap state proxies before persisting to stop exponential proxy nesting - #5582

Merged
MasterPtato merged 1 commit into
mainfrom
stack/fix-rivetkit-unwrap-state-proxies-before-persisting-to-stop-exponential-proxy-nesting-rqtppzkm
Aug 27, 2026
Merged

MasterPtato merged 1 commit into
mainfrom
stack/fix-rivetkit-unwrap-state-proxies-before-persisting-to-stop-exponential-proxy-nesting-rqtppzkm

Conversation

@MasterPtato

@MasterPtato MasterPtato commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@MasterPtato

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/actors

Get stack: forklift get 5582
Push local edits: forklift submit
Merge when ready: forklift merge 5582

change rqtppzkm

@railway-app

railway-app Bot commented Aug 20, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5582 environment in rivet-frontend

Service Status Web Updated (UTC)
website ❌ Build Failed (View Logs) Web Aug 27, 2026 at 10:36 pm
kitchen-sink 🕒 Building (View Logs) Web Aug 27, 2026 at 10:36 pm
frontend-cloud 😴 Sleeping (View Logs) Web Aug 27, 2026 at 9:40 pm
frontend-inspector 😴 Sleeping (View Logs) Web Aug 22, 2026 at 3:31 pm
ladle ✅ Success (View Logs) Web Aug 20, 2026 at 7:10 pm
mcp-hub ✅ Success (View Logs) Web Aug 20, 2026 at 7:08 pm

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Code Review

Reviewed the fix for exponential state-proxy nesting (write-through-proxy.ts unwrap on persist).

Findings

1. Commit path bypasses the unwrap, so mutation can still trigger the bug this PR fixesrivetkit-typescript/packages/rivetkit/src/registry/native.ts:1307 (and the ActorContextHandleAdapter mirror around line 2711)

set state(value) / initializeState call unwrapWriteThroughProxy(value) before persisting, but the proxy own mutation-triggered commit callback calls #writeState(nextValue, ...) directly without unwrapping. Code such as c.state.foo = c.state.bar (reading a nested object property off state and assigning it back onto state), c.state.list.push(c.state.other), or Object.assign(c.state, ...) with a state-derived value fires the commit callback with a live nested proxy embedded in the value, which then gets persisted. The next c.state read re-wraps it, reproducing the same exponential nesting/perf regression via mutation instead of spread, exactly the class of bug this PR sets out to fix. The new tests only cover the spread-reassignment pattern, not in-place mutation, so this gap is not caught. Suggest moving the unwrap into #writeState itself so both the setter and the commit callback go through one path instead of relying on every call site to remember to unwrap.

2. Map/Set unwrap reorders entriesrivetkit-typescript/packages/rivetkit/src/registry/write-through-proxy.ts:83

Unwrapping object-typed Map/Set entries via delete-then-set/add moves any entry whose key/value identity changed during unwrap to the end of iteration order. If application code relies on Map/Set iteration order (ordered tag sets, LRU-style maps), a state spread-and-reassign persist can silently reorder entries even though nothing in app logic reordered them. Worth preserving original position (e.g. rebuild via a single pass that keeps insertion order) if this is a supported state shape.

3. Minor: inconsistent env-var validationexamples/kitchen-sink/scripts/integrity-sweep.ts:41

SEED_START is parsed with bare Number(), while SEED_COUNT/SWEEP_CONCURRENCY go through the validating envNum() helper. A malformed SEED_START silently becomes NaN and produces keys like grow-bias1-000NaN instead of the clear error the other env vars get. Low severity since it is example/test tooling, but worth the same validation for consistent failure behavior.

4. Nit: isPlainObject duplicated a third timerivetkit-typescript/packages/rivetkit/src/registry/write-through-proxy.ts:58

There are now three separate implementations of isPlainObject (common/encoding.ts, common/database/shared.ts, and this new one). Not a bug, but a drift risk: if one gets tightened for an edge case, the others will not follow automatically, and assertJsonCompatValue vs. unwrapWriteThroughProxy could end up disagreeing on what counts as a plain object. Consider exporting and reusing the existing helper instead.

Summary

The core fix (unwrapping proxies before persist on the setter/initialize paths) looks correct for the reassignment case demonstrated in the new tests, but finding #1 suggests the fix is incomplete for the mutation-based commit path, which can reintroduce the same nested-proxy persistence bug. Recommend addressing that before merge, or explicitly scoping the fix to reassignment-only in the PR description if mutation-triggered commits are out of scope for this change.

@MasterPtato
MasterPtato force-pushed the stack/fix-rivetkit-unwrap-state-proxies-before-persisting-to-stop-exponential-proxy-nesting-rqtppzkm branch from 67cfa59 to 71f371b Compare August 27, 2026 22:36
@MasterPtato
MasterPtato merged commit 71f371b into main Aug 27, 2026
13 of 18 checks passed
@MasterPtato
MasterPtato deleted the stack/fix-rivetkit-unwrap-state-proxies-before-persisting-to-stop-exponential-proxy-nesting-rqtppzkm branch August 27, 2026 22:36
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