Repository navigation
fix(client): retry family sync target when covering plain sync fails (#1053) - #1054
Conversation
The #1051 unread guard drops the family sync target (and direct/descendant children) when a covering plain sync is merely in flight. If that read then fails, nobody publishes the dropped snapshot, yet the family key completed, so later family syncs no-oped and the stale snapshot stuck (#1053). Track drops that are only due to a pending covering read (pendingCover: not unread, but the covering session key is still pending, not settled) at all three unread publish sites - the family target/direct children and syncDescendants listings - and treat such a walk like a truncated one: invalidate the family key so the next family sync re-reads the dropped session. A covering read that has already settled has published, so #1051 behaviour is preserved exactly. Sibling audit: ancestors use an existence-only filter, so a skipped ancestor was actually published by someone and cannot be lost this way; children use unread, so they are fixed the same way via the syncDescendants droppedPending result and the direct-children check at the family publish.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Track pending covering reads packages/client/src/solid/data.ts |
pendingCover identifies sessions with pending reads. syncDescendants returns listing completeness and whether it dropped snapshots covered by pending reads. |
Invalidate family sync when snapshots are dropped packages/client/src/solid/data.ts, packages/client/test/solid-data.test.ts |
Family sync checks its target and direct children, then invalidates its key when reads or listings are incomplete or snapshots were dropped. A regression test verifies that a later family sync re-reads the target. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
#1053— The changes invalidate the family key when a pending covering read causes a snapshot to be dropped, allowing a later family sync to retry.
Possibly related PRs
- developing-today/opencode#843 — This PR changes the family-sync traversal and adds retry behavior for snapshots omitted during that traversal.
- developing-today/opencode#64 — This PR addresses a failure race in the family-sync implementation introduced by the recursive family discovery work.
- developing-today/opencode#60 — This PR fixes a failure path in the shared family-sync implementation used by nested-ask data loading.
Suggested reviewers: kitlangton
Merge Risk: 
203e2
-
Family sync can retry snapshots dropped during pending reads. No merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the primary fix: retrying the family sync target when a covering plain sync fails. Description check The description is mostly complete. It includes the issue, problem, fix, affected sibling paths, regression test, verification command, and test results. It does not select a change type or include th… Linked Issues check The PR addresses all coding criteria for active issue #1053.pendingCoverdistinguishes an in-flight coveringsession:<id>read from a settled read. Family publish andsyncDescendantsreport pen…Out of Scope Changes check The reported changes are limited to the linked issue scope. They update family-sync drop detection and retry invalidation in packages/client/src/solid/data.ts, and add the corresponding regression t…Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Description check
-
Explanation
The description is mostly complete. It includes the issue, problem, fix, affected sibling paths, regression test, verification command, and test results. It does not select a change type or include the template checklist and screenshots section, but those omissions do not prevent understanding the change.
Full details: Linked Issues check
-
Explanation
The PR addresses all coding criteria for active issue
#1053.pendingCoverdistinguishes an in-flight coveringsession:<id>read from a settled read. Family publish andsyncDescendantsreport pending-cover drops and invalidatesession.family:<id>, so a later family sync retries the target or child. The PR adds the requested regression test inpackages/client/test/solid-data.test.tsfor a failed concurrent plain sync. The reported test run passed 43 tests, including the#631,#1038,#1039, and#1051family-sync tests. The existing settled-read guard preserves the#1051success behavior.
Full details: Out of Scope Changes check
-
Explanation
The reported changes are limited to the linked issue scope. They update family-sync drop detection and retry invalidation in
packages/client/src/solid/data.ts, and add the corresponding regression test inpackages/client/test/solid-data.test.ts. These implementation and test changes directly support#1053. No unrelated change is identified.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Fixes #1053
Problem
The #1051
unreadguard drops the family sync's target snapshot when a concurrent plain sync's covering read is merely in flight. If that covering read then fails, nobody publishes the target — and the family sync still completedsession.family:<id>, so later family syncs no-oped and the pre-walk snapshot stuck indefinitely.Fix
pendingCoverhelper inpackages/client/src/solid/data.ts: a session is at risk only whenunreaddrops it and its coveringsession:<id>key is still pending (not settled). A settled covering read has already published, so fix(client): family sync publishes its own target's snapshot unconditionally — a newer snapshot that arrived during the ancestor walk is overwritten (the #1038 residual) #1051 behaviour is preserved exactly.sync.invalidate(session.family:<id>)— so the next family sync re-reads the dropped session instead of no-oping.syncDescendantsnow returns{complete, droppedPending}and reports the same pending-cover drops for listed descendants; the family sync invalidates on those too.Sibling audit (acceptance criterion)
children.filter(unread)at the family publish and insyncDescendants(formerly data.ts:589): same loss was possible — a listed child dropped for a pending cover whose read then failed would stick at its older store snapshot under a completed family key. Fixed the same way, above.!store.session.info[ancestor.id]), so a skipped ancestor was actually published by someone — nothing can be lost there. No change.Tests
packages/client/test/solid-data.test.tsmirroring the fix(client): family sync publishes its own target's snapshot unconditionally — a newer snapshot that arrived during the ancestor walk is overwritten (the #1038 residual) #1051 test with the concurrent plain sync held in flight at family publish time and then failing (HTTP 500): the target stays at the pre-walk snapshot after the failed cover, then a follow-up family sync actually re-reads (child get count rises past 3) and recovers the family snapshot/mid.bun test test/solid-data.test.tsfrompackages/client(laptop): 43 pass / 0 fail, including the ci(app): e2e app-2 regression specs fail on subagent navigation/timeline visibility — reproduced on unrelated PR #607 (attempts 1+2) #631/fix(client): family sync publishes ancestor snapshots unconditionally, overwriting newer store state that arrived during the walk #1038/fix(client): an ancestor walk that fails partway is cached as a complete family sync, so the missing ancestor is never retried #1039/fix(client): family sync publishes its own target's snapshot unconditionally — a newer snapshot that arrived during the ancestor walk is overwritten (the #1038 residual) #1051 family-sync tests.Queue task 4203.
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit