Skip to content

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

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

fix(rivetkit): unwrap state proxies before persisting to stop exponential proxy nesting#5582
MasterPtato wants to merge 1 commit into
mainfrom
stack/fix-rivetkit-unwrap-state-proxies-before-persisting-to-stop-exponential-proxy-nesting-rqtppzkm

Conversation

@MasterPtato

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)
frontend-cloud 😴 Sleeping (View Logs) Web Aug 22, 2026 at 8:33 pm
frontend-inspector 😴 Sleeping (View Logs) Web Aug 22, 2026 at 3:31 pm
kitchen-sink 😴 Sleeping (View Logs) Web Aug 22, 2026 at 3:23 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
website ❌ Build Failed (View Logs) Web Aug 20, 2026 at 7:08 pm

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Review

Solid, well-targeted fix. The root cause (c.state = { ...c.state, foo } persists a spread whose nested values are still write-through proxies, so the next read wraps them again, causing proxy nesting to grow with every spread-style update) is correctly diagnosed, and the fix (deep-unwrap before persisting, in unwrapWriteThroughProxy) is applied consistently at all four write paths (NativeConnAdapter.set state / initializeState, ActorContextHandleAdapter.set state / initializeState).

Correctness

  • Traced unwrapDeep's handling of plain objects, arrays, Map, and Set, including the seen cycle guard — looks correct, and in-place mutation of the raw target is safe here because unwrapProxy walks back to the object that JS proxies for the current stored state, so subsequent reads through the memoized proxy stay consistent.
  • The new stateProxy/stateProxyTarget memoization added to NativeConnAdapter.get state() mirrors the memoization pattern that already existed for ActorContextHandleAdapter.get state(), so it's a low-risk extension of a proven pattern rather than new design.
  • One edge case worth a second look, low likelihood but not obviously impossible given JsonCompatValue allows object keys in Map/Set: if a Map/Set ends up holding two distinct proxy instances that unwrap to the same underlying raw target, unwrapDeep's delete+set/add replacement could silently coalesce two entries into one. Probably fine in practice since on-change proxies are presumably stable per target, but might be worth a comment or a quick test if that guarantee isn't rock solid.
  • unwrapWriteThroughProxy runs (and mutates nested targets in place) before assertJsonCompatValue in the set state setters. If validation subsequently throws on an unrelated field, the deep-unwrap side effect on shared nested objects (converting proxy children to their raw form) has already happened. This looks harmless in practice since it doesn't change any actual data, but it's a subtle instance of "the rejected write still has an observable side effect" — worth being aware of even if not worth fixing.

Test coverage

Good — both a fast isolated unit test (write-through-proxy.test.ts, including the "old behavior still stacks" regression baseline and a timing-bounded perf assertion) and real driver-level integration tests (actor-conn-state.test.ts via describeDriverMatrix) that exercise the fix through actual actor/connection state. This matches the repo's "test against real infrastructure" convention rather than mocking.

Minor nits

  • proxyDepth is duplicated between the fixture (conn-state.ts) and the test file (write-through-proxy.test.ts). Understandable given the fixture runs as actor code and the test file doesn't share code with fixtures, so not blocking.
  • Comments are complete sentences and avoid fragmented/dash style, consistent with CLAUDE.md conventions.

No blocking issues found. Nice, focused fix with a regression test that actually reproduces the old exponential-growth behavior.


🤖 Generated with Claude Code

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