Skip to content

feat(sync-service): Terminate shape consumer processes after timeout #3455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat(sync-service): Terminate shape consumer processes after timeout #3455

wants to merge 7 commits into from
+382 −44

Conversation

@magnetised
Copy link
Contributor

@magnetised magnetised commented Nov 18, 2025

Closes #3253

@magnetised magnetised force-pushed the magnetised/issue-3253-suspend-consumers-after-timeout branch from 04d5efa to b1744df Compare November 18, 2025 14:45
@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 18, 2025 •
edited
Loading

Open in StackBlitz

npm i https://pkg.pr.new/@electric-sql/react@3455

npm i https://pkg.pr.new/@electric-sql/client@3455

npm i https://pkg.pr.new/@electric-sql/y-electric@3455

commit: d09ab1d

@codecov
Copy link

codecov bot commented Nov 18, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.57%. Comparing base (e7b8bd0) to head (7d4d7f3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...-service/lib/electric/shape_cache/shape_cleaner.ex 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3455      +/-   ##
==========================================
+ Coverage   64.45%   67.57%   +3.11%     
==========================================
  Files         173      182       +9     
  Lines        8713     9745    +1032     
  Branches      106      367     +261     
==========================================
+ Hits         5616     6585     +969     
- Misses       3096     3158      +62     
- Partials        1        2       +1
Flag Coverage Δ
elixir 64.12% <91.66%> (+0.08%) ⬆️
elixir-client 73.94% <ø> (ø)
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/typescript-client 93.60% <ø> (?)
packages/y-electric 55.12% <ø> (ø)
postgres-140000 62.77% <91.66%> (+0.11%) ⬆️
postgres-150000 62.72% <91.66%> (-0.09%) ⬇️
postgres-170000 62.96% <91.66%> (+0.20%) ⬆️
postgres-180000 62.98% <91.66%> (+0.27%) ⬆️
sync-service 63.13% <91.66%> (+0.10%) ⬆️
typescript 87.18% <ø> (+14.78%) ⬆️
unit-tests 67.57% <91.66%> (+3.11%) ⬆️

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.

@magnetised magnetised force-pushed the magnetised/issue-3253-suspend-consumers-after-timeout branch from b1744df to c8e7da0 Compare November 19, 2025 11:17
@magnetised magnetised marked this pull request as ready for review November 19, 2025 11:44
msfstef
msfstef approved these changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Very nice, love that it's such a simple change!

Main nit is that I'd like to see the shape_enable_suspend? work as something that can be enabled via ELECTRIC_FEATURE_FLAGS - this is easy to configure on cloud as well and doesn't require threading through new variables every time.

packages/sync-service/lib/electric/shapes/consumer_registry.ex Outdated
:ets.new(ets_name(stack_id), [
:public,
:named_table,
write_concurrency: true,
Copy link
Contributor

@msfstef msfstef Nov 19, 2025

Choose a reason for hiding this comment

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

why not keep it :auto? do we not trust OTP enough, or do OTP registries themselves use true explicitly?

Copy link
Contributor Author

@magnetised magnetised Nov 19, 2025

Choose a reason for hiding this comment

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

My thinking was: now we know that multiple processes will be concurrently writing why not just enable it. Reading the docs (😲 ) I see :auto is the recommended setting. I should read more docs.

@magnetised magnetised force-pushed the magnetised/issue-3253-suspend-consumers-after-timeout branch from d09ab1d to b11338f Compare November 19, 2025 16:27
@magnetised magnetised force-pushed the magnetised/issue-3253-suspend-consumers-after-timeout branch from b11338f to 7d4d7f3 Compare November 19, 2025 16:31
@magnetised magnetised requested a review from msfstef November 19, 2025 16:35
msfstef
msfstef approved these changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Brilliant - this'll come in handy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msfstef msfstef msfstef approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inactive consumers should be put offline

3 participants

@magnetised @msfstef