fix(sync-service): Fix shape cleanup race condition

[magnetised]

Fixes #3760

Summary by CodeRabbit

• Bug Fixes

  • Improved sync service reliability by properly addressing offset regression scenarios that can occur during live polling. The system now detects these edge cases and returns appropriate HTTP error responses to trigger necessary data refetch operations, preventing silent failures.

• Tests

  • Added comprehensive test coverage for offset regression edge cases during live polling operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR fixes a race condition in shape invalidation where a shape's offset regression (offset going backwards relative to the client's last offset) wasn't handled, causing a CaseClauseError. The fix adds handling to send a shape rotation signal, triggering a client refetch.

Changes

Cohort / File(s)	Summary
Shape Offset Regression Handling packages/sync-service/lib/electric/shapes/api.ex	Adds case clause in notify_changes_since_request_start/1 to handle offset regression scenario where resolve_shape_handle returns a valid shape handle but with latest_log_offset that doesn't advance past last_offset; sends :shape_rotation signal to trigger refetch; includes comments documenting the race condition window between consumer termination and async cleanup.
Test Coverage for Offset Regression packages/sync-service/test/electric/shapes/api_test.exs	Adds test case "handles shape offset regression during live polling (issue #3760)" that simulates the race condition scenario where offset regression occurs during live polling; validates system responds with 409 status and must-refetch header instead of raising CaseClauseError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• robacourt
• icehaunter

Poem

🐰 A race condition once lurked in the shapes,
Where offsets would vanish like rabbits through drapes!
But now when they regress, the signals ring clear,
A rotation's sent forth—refetch without fear! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: handling a shape cleanup race condition that causes offset regression during shape invalidation, matching the PR objectives and code changes.
Linked Issues check	✅ Passed	The PR directly addresses issue #3760 by adding the suggested case clause to detect offset regression and send a shape rotation signal, matching all coding requirements from the linked issue.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the race condition: adding a match clause for offset regression and a corresponding test case, with no unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c7a4b6 and b2f8d2b.

📒 Files selected for processing (2)

• packages/sync-service/lib/electric/shapes/api.ex
• packages/sync-service/test/electric/shapes/api_test.exs

🧰 Additional context used

🧬 Code graph analysis (1)

packages/sync-service/test/electric/shapes/api_test.exs (5)

packages/sync-service/test/support/test_utils.ex (3)

• expect_shape_cache (218-220)
• patch_shape_cache (222-224)
• patch_storage (214-216)

packages/sync-service/lib/electric/shapes/api/params.ex (1)

• validate (166-179)

packages/sync-service/lib/electric/connection/manager/connection_resolver.ex (1)

• validate (32-34)

packages/sync-service/lib/electric/shapes/api.ex (2)

• serve_shape_response (461-467)
• serve_shape_response (469-482)

packages/sync-service/lib/electric/plug/serve_shape_plug.ex (1)

• serve_shape_response (119-121)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: Test packages/y-electric w/ sync-service
• GitHub Check: Test packages/start w/ sync-service
• GitHub Check: Test packages/react-hooks w/ sync-service
• GitHub Check: Test packages/experimental w/ sync-service
• GitHub Check: Test packages/typescript-client w/ sync-service
• GitHub Check: Build and test sync-service, pg17
• GitHub Check: Build and test sync-service, pg15
• GitHub Check: Build and test sync-service, pg14
• GitHub Check: Build and test sync-service, pg18
• GitHub Check: Run Lux integration tests

🔇 Additional comments (2)

packages/sync-service/lib/electric/shapes/api.ex (1)

737-751: LGTM! Well-documented fix for the race condition.

The new case clause correctly handles the offset regression scenario by catching any remaining cases where the shape handle matches but the offset has not advanced (or has regressed). The pattern matching order ensures:

1. Exact offset match → no-op
2. Offset increased → new changes notification
3. Any other case (offset regression) → shape rotation signal

The detailed comment block clearly explains the race condition sequence, which will help future maintainers understand why this case exists.

packages/sync-service/test/electric/shapes/api_test.exs (1)

1275-1338: LGTM! Comprehensive test for the race condition fix.

The test effectively reproduces the race condition described in issue #3760 by:

1. Setting up a valid offset on the first resolve_shape_handle call (during validation)
2. Returning a regressed offset (LogOffset.last_before_real_offsets()) on the second call (during notify_changes_since_request_start)
3. Verifying the system correctly responds with 409 and must-refetch header

The inline comments clearly document the test flow and expected behavior, making the test self-documenting.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.36%. Comparing base (4c7a4b6) to head (b2f8d2b).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3761   +/-   ##
=======================================
  Coverage   87.36%   87.36%           
=======================================
  Files          23       23           
  Lines        2011     2011           
  Branches      531      534    +3     
=======================================
  Hits         1757     1757           
  Misses        252      252           
  Partials        2        2           

Flag	Coverage Δ
packages/experimental	87.73% <ø> (ø)
packages/react-hooks	86.48% <ø> (ø)
packages/start	82.83% <ø> (ø)
packages/typescript-client	93.47% <ø> (ø)
packages/y-electric	56.05% <ø> (ø)
typescript	87.36% <ø> (ø)
unit-tests	87.36% <ø> (ø)

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.

[msfstef]

My agent had found this race as well but I had disliked this solution - what is it that is actually returning last_before_real_offsets(), and should it be returning something else instead rather than having this as a default? The default should perhaps be set when the shape is created, but if the shape is deleted we shouldn't fall back to the default