fix: Race condition for reader after shape writer flushes data

[msfstef]

Summary

Fixes a race condition in PureFileStorage where readers can get empty or partial results when reading from ETS while a concurrent flush clears the buffer.

The Bug

When a reader calls get_log_stream, it reads metadata (including last_persisted_offset) as a snapshot, then uses that to decide whether to read from disk or ETS. However, a concurrent flush can happen between these steps:

T1: Reader reads metadata: last_persisted=X, ets_ref=table
T2: Writer flushes: writes to disk, updates last_persisted=Y, clears ETS
T3: Reader decides to read from ETS (based on stale last_persisted=X)
    → ETS is empty or partially read → returns [] or incomplete data

The data exists on disk but the reader misses it because it made its read decision based on stale metadata.

Two failure modes

1. Empty ETS: Flush completes entirely before reader's first next_lookup call → reader immediately gets '$end_of_table' and returns []

2. Partial ETS read: Reader iterates with multiple next_lookup calls, and delete_all_objects runs between calls:

   next_lookup(ets, start) → {key1, value1}
   next_lookup(ets, key1)  → {key2, value2}
   ← delete_all_objects runs here (atomic) →
   next_lookup(ets, key2)  → '$end_of_table'
   

   Reader got entries 1-2 but missed entries 3-N.

Note on ETS guarantees: The buffer is an ordered_set table. Per Erlang ETS docs, delete_all_objects is atomic and isolated—each individual next_lookup call sees either the full table or the empty table, never a mid-deletion state. However, a sequence of next_lookup calls can span the deletion, resulting in partial data. For ordered_set tables, next_lookup will not crash during concurrent deletion; it simply returns '$end_of_table' when the table is empty.

Both failure modes result in the client receiving less data than expected, but being told (via electric-offset header) that they received data up to the requested offset.

The Fix

Modified read_range_from_ets_cache to return {data, last_offset_read} where last_offset_read is the tuple offset of the last entry actually read (or nil if empty). This allows detecting incomplete reads.

When an incomplete ETS read is detected (empty or partial), we fall back directly to reading from disk. This is safe because of the flush order in write_loop.ex:

1. IO.binwrite + :file.datasync — data written and synced to disk
2. update_persistance_metadata — updates last_persisted_offset in stack ETS
3. trim_ets — clears shape's ETS buffer via :ets.delete_all_objects

If ETS is cleared, data is guaranteed to be on disk. We can read directly using the existing upper_read_bound and boundary_info without re-reading metadata.

Design Decision

The goal was to minimally alter the existing code. Alternative approaches like reader counters, delayed ETS clearing, or rearchitecting read/flush cycles were not considered—the fallback-to-disk approach requires only local changes to the reader path and doesn't affect the writer's eager memory cleanup behavior.

Test Plan

• Added test reproducing empty ETS race condition (fails without fix, passes with fix)
• Added test reproducing partial ETS race condition (fails without fix, passes with fix)
• All PureFileStorage tests pass (34 tests)
• All LogOffset doctests pass (54 doctests)
• Full test suite passes (1315 tests, 363 doctests, 8 properties)
• No dialyzer warnings

Summary by CodeRabbit

• Bug Fixes

  • Fixed a race in file-backed storage where concurrent flushes could produce empty or partial in-memory reads; readers now detect partial/cleared reads and retry using fresh metadata to ensure consistent reads from disk.

• Tests

  • Added tests simulating ETS read/write races to verify retry behavior and correct data recovery under concurrent flush scenarios.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Detects empty or partial ETS reads in PureFileStorage and retries by refreshing metadata and reading from disk when ETS data is missing or truncated; no exported/public signatures were changed.

Changes

Cohort / File(s)	Summary
Changelog Entry \.changeset/hungry-pens-rush.md	Adds a patch changelog entry documenting the ETS read/write race fix for @core/sync-service.
Core Implementation packages/sync-service/lib/electric/shape_cache/pure_file_storage.ex	ETS read logic updated: upper_read_bound converted to a tuple for comparisons; read_range_from_ets_cache gains a new arity/behavior and returns {data_list, last_offset} (or {[], nil} for nil-ETS). Callers detect nil/partial last_offset and fall back to streaming from disk after refreshing metadata; last-offset threading added.
Tests packages/sync-service/test/electric/shape_cache/pure_file_storage_test.exs	Adds "ETS read/write race condition" tests that simulate empty and partial ETS states, patch metadata to simulate concurrent flushes, and assert readers fall back to disk and recover full data.
Project Files manifest_file, mix.exs, mix.lock	Project/manifest updates included in the diff.

Sequence Diagram(s)

sequenceDiagram
    participant Reader
    participant ETS
    participant Metadata
    participant Disk
    participant Writer

    Note over Writer,ETS: Concurrent flush may clear/partially update ETS
    Reader->>ETS: read_range_from_ets_cache(min,max)
    ETS-->>Reader: {data, last_offset} or {[], nil}
    alt ETS returned nil or last_offset < upper_read_bound
        Reader->>Metadata: refresh_metadata()
        Metadata-->>Reader: fresh_metadata (last_persisted)
        Reader->>Disk: stream_from_disk(range_based_on_metadata)
        Disk-->>Reader: full_data_stream
    else ETS returned complete last_offset
        ETS-->>Reader: data (used directly)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I peeked in the table and found empty shelves,
A flush hopped through and hid some themselves.
I fetched fresh maps, then dug where it’s dark,
Pulled whole stories up from the disk’s warm spark.
No more lost crumbs — now every read’s a lark. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: Race condition for reader after shape writer flushes data' accurately and specifically summarizes the main change: fixing a race condition in PureFileStorage where readers encounter empty or partial ETS reads during concurrent flushes.

_{✏️ 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 964f319 and 3ba8822.

📒 Files selected for processing (1)

• packages/sync-service/test/electric/shape_cache/pure_file_storage_test.exs

🧰 Additional context used

🧬 Code graph analysis (1)

packages/sync-service/test/electric/shape_cache/pure_file_storage_test.exs (3)

packages/sync-service/lib/electric/shape_cache/pure_file_storage.ex (2)

• append_to_log! (1293-1307)
• get_log_stream (1082-1113)

packages/sync-service/lib/electric/shape_cache/in_memory_storage.ex (2)

• append_to_log! (295-319)
• get_log_stream (193-195)

packages/sync-service/lib/electric/replication/log_offset.ex (3)

• new (58-60)
• new (62-64)
• to_tuple (295-297)

⏰ 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). (6)

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

🔇 Additional comments (1)

packages/sync-service/test/electric/shape_cache/pure_file_storage_test.exs (1)

974-1104: Race-condition tests look solid.

The new scenarios clearly simulate empty/partial ETS reads and assert the disk fallback path. No issues spotted.

_{✏️ 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 (2788195) to head (3ba8822).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3719   +/-   ##
=======================================
  Coverage   87.36%   87.36%           
=======================================
  Files          23       23           
  Lines        2011     2011           
  Branches      532      528    -4     
=======================================
  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.

[netlify]

✅ Deploy Preview for electric-next ready!

Name	Link
🔨 Latest commit	a33f712
🔍 Latest deploy log	https://app.netlify.com/projects/electric-next/deploys/696f6f970b6d9b0008a959dd
😎 Deploy Preview	https://deploy-preview-3719--electric-next.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[msfstef]

I'm contemplating whether we want to retry, or ti simply directly go to the main log since we know that this happens if we've flushed.

[msfstef]

I've updated the code to now directly read from the disk if the ETS cache is empty, since we know it will have flushed in that case

[magnetised]

Lovely stuff

[icehaunter]

lgtm

[github-actions]

This PR has been released! 🚀

The following packages include changes from this PR:

• @core/sync-service@1.3.2

Thanks for contributing to Electric!