Skip to content

fix(sync-service): Add a temporary shape mailbox#3967

Closed
magnetised wants to merge 1 commit intomainfrom
magnetised/delivery-for-mr-shape
Closed

fix(sync-service): Add a temporary shape mailbox#3967
magnetised wants to merge 1 commit intomainfrom
magnetised/delivery-for-mr-shape

Conversation

@magnetised
Copy link
Copy Markdown
Contributor

Add a single use mailbox for passing a shape to a consumer process without going through the disk

This ensures that shapes are passed cleanly to new consumer processes without increasing RAM by duplicating the shape in the supervisor's state.

@magnetised magnetised requested a review from alco March 6, 2026 11:59
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.75%. Comparing base (cea4c13) to head (3a16519).
⚠️ Report is 13 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3967   +/-   ##
=======================================
  Coverage   75.75%   75.75%           
=======================================
  Files          11       11           
  Lines         693      693           
  Branches      174      174           
=======================================
  Hits          525      525           
  Misses        167      167           
  Partials        1        1           
Flag Coverage Δ
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/start 82.83% <ø> (ø)
packages/y-electric 56.05% <ø> (ø)
typescript 75.75% <ø> (ø)
unit-tests 75.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude
Copy link
Copy Markdown

claude bot commented Mar 6, 2026

Claude Code Review - iteration 5 (2026-03-06)

Summary: This PR adds a single-use ETS mailbox in ShapeStatus to pass shapes to new consumer processes without duplicating shape data in the supervisor state. The design is clean and sound.

What is Working Well: (1) Clean ETS mailbox using insert_new for one-shot delivery; fallback in deliver_shape! handles test scenarios. (2) Cleanup thorough: entries deleted in remove_shape, reset, and after delivery. (3) Consumer uses restart: :temporary so fallback path is correctly test-only. (4) Typo retreival->retrieval fixed. (5) Changeset present.

Important (Should Fix): Missing tests for post_shape/3 and deliver_shape!/2 in shape_status_test.exs. Raised in iteration 2, still unaddressed. Need tests for: normal delivery, duplicate detection, fallback to ShapeDb when no mailbox entry, ArgumentError when shape missing from both sources.

Suggestion: Spec vs caller mismatch on post_shape/3. shape_status.ex:337 spec says :ok | {:error, term()} but caller asserts :ok so error path causes MatchError. Consider changing spec to :: :ok and raising directly in the duplicate case.

Issue Conformance: No linked issue. PR description matches implementation.

Previous Review: Typo fixed. Missing tests for post_shape/3 and deliver_shape!/2 remain open.

CI: InMemoryStorage and ShapeCacheTest killed failures indicate CI resource exhaustion not regressions. PublicationManagerTest is the previously noted flaky test.

@blacksmith-sh

This comment has been minimized.

@magnetised magnetised force-pushed the magnetised/delivery-for-mr-shape branch 3 times, most recently from 0dc5af5 to 81cb579 Compare March 6, 2026 12:19
@blacksmith-sh

This comment has been minimized.

Add a single use mailbox for passing a shape to a consumer process without going through the disk
@magnetised magnetised force-pushed the magnetised/delivery-for-mr-shape branch from 81cb579 to 3a16519 Compare March 6, 2026 12:23
Copy link
Copy Markdown
Member

@alco alco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good but I'm really concerned about the runtime characteristics of this.

ETS read concurrency optimizes for read bursts and helps in situations where reads are more frequent than writes. Here we have parity between reads and writes and so the contention on access to the single ETS table is the biggest concern which we cannot control much (the VM decides how many locks to create and how to distribute keys across them).

I think a much simpler solution would be for the ShapeCache process to send the shape to the started consumer process as a regular message. This may or may not slow down consumer's initialization (compared to reading from ShapeDb) but it doesn't suffer from lock contention and has almost the same memory profile as passing the shape through an ETS table. It actually leads to less copying since it's a single message send compared to copying to ETS, followed by copying from ETS.

if :ets.insert_new(shape_mailbox(stack_id), {shape_handle, shape}) do
:ok
else
{:error, "Duplicate shape in mailbox: #{inspect(shape_handle)}"}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this here. ShapeStatus already handles the duplicate shape case and it runs before this code. Here we can make a best-effort of "insert into ETS if not there and return :ok otherwise".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should never happen, absolutely but no harm is just making sure of that is there?

end

%Shape{} = shape ->
:ets.delete(table, shape_handle)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm worried that the ETS tables becomes the bottleneck and we get another bad case of consumers failing to start on time in a loaded system.

@magnetised
Copy link
Copy Markdown
Contributor Author

ETS read concurrency optimizes for read bursts and helps in situations where reads are more frequent than writes

I don't think the speed of ets will be our problem. These are consumers started one at a time by a single process, so we're talking about a single process reading from the ets at a time since the read is done in the init/1 callback. Plus ets's "slow" is still probably ok! The slow we're worried about is multiple millisecond EBS slow.

@magnetised magnetised closed this Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants