Make dry-run sync apply the CRDT side to a throwaway copy - #2492
Merged
Conversation
…ay copy The sync writes the CRDT then reads it back within a pass (direction B reads the CRDT after direction A wrote it). The old dry-run wrapped the CRDT in a record-only DryRunMiniLcmApi, so that read-back saw stale state: direction B thought fwdata's whole inventory should be deleted. Mostly that garbage was silently recorded, but MorphTypeSync.Remove threw on it, and the fwdata-side change counts were meaningless (the Sena3 test only compared CRDT changes and documented the fwdata side as garbage). Run the CRDT side of a dry run against a disposable copy of its own database instead, so writes really apply and read back faithfully, and record what was applied. FwData is read once up front (never read back) and its file must not change, so it stays a record-only DryRunMiniLcmApi. - CrdtProjectsService.OpenProjectCopy backs up the sqlite db to a temp file and opens it as a throwaway project in its own scope; TempCrdtProjectCopy disposes the scope and deletes the temp files. - RecordingMiniLcmApi records each write and forwards it to the copy (AutoInterface forwards reads/Submit*/everything else, so it's correct even where it doesn't record). - IDryRunRecorder lets the sync pull records from either wrapper. - CrdtRepairs.SyncMissingTranslationIds drops its dryRun flag: with the copy, the CRDT write is always safe. - Sena3 DryRunSync tests now also assert FwdataChanges; DryRunSync_MakesNoChanges gets a consistent writing-system setup (the faithful run surfaces the inconsistency the fake one hid). This also fixes PR #2483's failure: with morph-type seeding removed the CRDT starts blank, and the dry run no longer throws on the phantom morph-type removal (verified by running the DryRunSync tests with seeding disabled). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Recording lived in two places: the old DryRunMiniLcmApi (record + swallow) and the new RecordingMiniLcmApi (record + apply), duplicating ~50 write descriptions. Split the two concerns instead: RecordingMiniLcmApi is the single recorder, and what its inner api does decides whether writes take effect. - ReadonlyMiniLcmApi (renamed from DryRunMiniLcmApi, recording removed) just discards writes and returns a plausible value; reads pass through. - Both dry-run sides are now RecordingMiniLcmApi(inner): the CRDT side wraps the real throwaway copy (writes apply), the fwdata side wraps a ReadonlyMiniLcmApi (writes discarded). Record strings exist once. - DryRunRecord lifted to a standalone type; IDryRunRecorder dropped (only one recorder). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…yMiniLcmApi Match MiniLcmApiWriteNormalizationWrapper: forward IMiniLcmReadApi via a typed [AutoInterface] property (interface inferred from the property type) instead of a typeof + MemberMatch=Any field. Writes stay manual so the compiler enforces every one is handled. The explicit GetEntries workaround is removed: it guarded the CRDT import destination against a missing writing system, but this class only ever wraps fwdata (which always has writing systems), so it was dead here and the reason MemberMatch was needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback on the dry-run wrappers: - RecordingMiniLcmApi now carries the exact record strings the pre-split DryRunMiniLcmApi produced (verified identical), rather than the terser ad-hoc ones. Each write is the old Add line followed by a forward to the wrapped api. - Forward reads via the read-only [AutoInterface] property (as ReadonlyMiniLcmApi and the other wrappers do) instead of IncludeBaseInterfaces=true. Writes are no longer auto-forwarded, so the compiler enforces that every write is implemented here and thus recorded — nothing can slip through unrecorded (Submit* included). - Rename CrdtProjectsService.OpenProjectCopy to OpenTemporaryProjectCopy so the call site reads as temporary/disposable. CreateEntry forwards the original (possibly null) options — null means "add main publication" to the api, which new CreateEntryOptions() does not — while still logging the same string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dry-run wrappers Address review feedback: - Rename ReadonlyMiniLcmApi to WriteIgnoringMiniLcmApi. "Readonly" implies writes are rejected; this class accepts them and does nothing, returning a plausible value so the sync continues. - Type its wrapped api as IMiniLcmReadApi so it structurally can't forward a write, and drop the redundant _api field in favour of the primary-constructor parameter. The two before/after write overloads that shadow that parameter read via the ReadApi property instead. - RecordingMiniLcmApi keeps the read-only [AutoInterface] property: reads are forwarded and not recorded (only writes are), and because writes aren't auto-forwarded the compiler still forces every one to be implemented here and thus recorded. - Comment pass: drop an unverified "delete-wins" claim, correct a stale OpenProjectCopy cref, explain BackupDatabase as "not File.Copy" rather than an unverified WAL-mode detail, and trim the test comments to the gotcha they guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…up param - Rename TempCrdtProjectCopy's deleteFiles constructor param to cleanup (more generic). - Note in WriteIgnoringMiniLcmApi.CreateEntry why the IncludeComplexFormsAndComponents branch matters: it returns only what a real create would persist (this is where the fabricated return the old dry-run api produced now lives). - Trim every comment added on this branch to the fewest words that carry the point. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The dry-run rebinding only swapped crdtApi to the throwaway copy; crdt still pointed at the real project. CrdtRepairs.SyncMissingTranslationIds (which lost its dryRun guard) is passed crdt, so a dry run on a project with a missing translation ID committed SetFirstTranslationIds to the live CRDT — and the copy, snapshotted first, never got the repair, so the prediction diverged from a real sync too. Rebind crdt to the copy in dry run so the repair (and the morph-type read) hit the copy. Regression test: CrdtEntryMissingTranslationId_DryRunSync_LeavesRealCrdtUntouched (fails without this fix). Also from the review: - OpenTemporaryProjectCopy now disposes the scope and deletes the temp file if it throws before handing them to TempCrdtProjectCopy. - TempCrdtProjectCopy.DisposeAsync uses try/finally so cleanup runs even if scope disposal throws. - Update three AGENTS/agent docs that still named the deleted DryRunMiniLcmApi. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…arg, and two DeepSource nits The branch added a CrdtProjectsService parameter to CrdtFwdataProjectSyncService, but SyncWorkerTestHarness builds a Mock<CrdtFwdataProjectSyncService> with the old 3-arg constructor, so Moq couldn't find a matching constructor and every SyncWorkerTests case threw. Pass the extra null! arg. Also from DeepSource: - Drop the unused syncedIdCount local (the repair's return value was never read). - Remove a redundant else-after-return in WriteIgnoringMiniLcmApi.CreateEntry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
💻 FW Lite
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesDry-run synchronization
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
4 tasks
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs (1)
251-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlign the remaining
null!placeholder with the injected dependency.The constructor order is
miniLcmImport,logger,validationWrapperFactory, thencrdtProjectsService; since the later sync path needsvalidationWrapperFactoryto create wrapper APIs, pass that dependency here instead of a secondnull!/placeholder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs` around lines 251 - 256, Update the CrdtFwdataProjectSyncService construction in SyncWorkerTestHarness to pass the injected validationWrapperFactory as the third constructor dependency, preserving the existing miniLcmImport, logger, and crdtProjectsService argument order and removing the placeholder null value.
🧹 Nitpick comments (2)
backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (1)
159-172: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse structured logging templates instead of pre-interpolated strings.
LogRecordedRuneagerly builds an interpolated string per record regardless of whether theInformationlog level is enabled, and loses field-level queryability. A message-template call avoids both costs.♻️ Proposed refactor
- private void LogRecordedRun(IMiniLcmApi api, string type) - { - if (api is not RecordingMiniLcmApi recorder) return; - foreach (var dryRunRecord in recorder.RunRecords) - { - logger.LogInformation($"Dry run {type} record: {dryRunRecord.Method} {dryRunRecord.Description}"); - } - - logger.LogInformation($"Dry run {type} changes: {recorder.RunRecords.Count}"); - } + private void LogRecordedRun(IMiniLcmApi api, string type) + { + if (api is not RecordingMiniLcmApi recorder) return; + foreach (var dryRunRecord in recorder.RunRecords) + { + logger.LogInformation("Dry run {Type} record: {Method} {Description}", type, dryRunRecord.Method, dryRunRecord.Description); + } + + logger.LogInformation("Dry run {Type} changes: {Count}", type, recorder.RunRecords.Count); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs` around lines 159 - 172, Update LogRecordedRun to use structured logging message templates with separate arguments for type, method, description, and record count instead of interpolated strings. Preserve the existing log messages and iteration behavior while enabling deferred formatting and field-level querying.backend/FwLite/LcmCrdt/CrdtProjectsService.cs (1)
287-319: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTemp CRDT copy lands in the shared OS temp dir — verify file permissions on shared hosts.
The full project database (including commit author metadata) is byte-copied to
Path.GetTempPath()/FwLiteProjectCopies. On a single-user desktop this is likely fine, but per the retrieved learning FwHeadless "primarily orchestrates synchronization" and is documented as a critical/shared-host component — if dry-run sync runs there, default temp-file permissions on Linux (typically world/group-readable) could expose project content to other local users during the window beforeEnsureDeleteProjectruns.Consider setting a restrictive
UnixFileMode/ACL when creating the temp file (or placing it under a per-run private subdirectory) if this ever executes on a shared host.Based on learnings, FwHeadless primarily orchestrates synchronization and shared-host deployment should be assumed for this path; please confirm whether
OpenTemporaryProjectCopyis ever invoked in that context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/FwLite/LcmCrdt/CrdtProjectsService.cs` around lines 287 - 319, Update OpenTemporaryProjectCopy to protect the temporary SQLite copy on shared hosts before sourceConnection.BackupDatabase runs. Create the file or per-run directory with restrictive permissions, using the platform-appropriate Unix mode/ACL, while preserving cleanup through EnsureDeleteProject. Verify callers of OpenTemporaryProjectCopy include the FwHeadless synchronization/dry-run path and ensure that invocation receives the same protection.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/FwLite/FwLiteProjectSync/WriteIgnoringMiniLcmApi.cs`:
- Around line 51-54: Update the PartOfSpeech and SemanticDomain methods in
WriteIgnoringMiniLcmApi to await GetPartOfSpeech/GetSemanticDomain, validate the
awaited result is non-null, and throw when the requested id is missing before
returning it. Remove the null-forgiving operator applied to the task and match
the behavior of the sibling update methods.
---
Outside diff comments:
In `@backend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs`:
- Around line 251-256: Update the CrdtFwdataProjectSyncService construction in
SyncWorkerTestHarness to pass the injected validationWrapperFactory as the third
constructor dependency, preserving the existing miniLcmImport, logger, and
crdtProjectsService argument order and removing the placeholder null value.
---
Nitpick comments:
In `@backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs`:
- Around line 159-172: Update LogRecordedRun to use structured logging message
templates with separate arguments for type, method, description, and record
count instead of interpolated strings. Preserve the existing log messages and
iteration behavior while enabling deferred formatting and field-level querying.
In `@backend/FwLite/LcmCrdt/CrdtProjectsService.cs`:
- Around line 287-319: Update OpenTemporaryProjectCopy to protect the temporary
SQLite copy on shared hosts before sourceConnection.BackupDatabase runs. Create
the file or per-run directory with restrictive permissions, using the
platform-appropriate Unix mode/ACL, while preserving cleanup through
EnsureDeleteProject. Verify callers of OpenTemporaryProjectCopy include the
FwHeadless synchronization/dry-run path and ensure that invocation receives the
same protection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c03220d0-98f9-487c-bf61-55b58c31491c
📒 Files selected for processing (14)
.claude/agents/test-auditor.mdbackend/FwHeadless/AGENTS.mdbackend/FwLite/AGENTS.mdbackend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.csbackend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.csbackend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.csbackend/FwLite/FwLiteProjectSync/CrdtRepairs.csbackend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.csbackend/FwLite/FwLiteProjectSync/RecordingMiniLcmApi.csbackend/FwLite/FwLiteProjectSync/WriteIgnoringMiniLcmApi.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/CrdtProjectsService.csbackend/FwLite/LcmCrdt/TempCrdtProjectCopy.csbackend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs
💤 Files with no reviewable changes (1)
- backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs
OpenCrdtProject is typed to IMiniLcmApi, but a CRDT project always resolves a CrdtMiniLcmApi (registered directly, no decorators). A temp copy is always a CRDT project, so cast once at that boundary and expose TempCrdtProjectCopy.Api as CrdtMiniLcmApi. The dry-run sync call site no longer casts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RecordingMiniLcmApi and WriteIgnoringMiniLcmApi forwarded tasks directly (return _api.X()). Awaiting keeps the wrapper frame on the stack trace when the inner call throws — worth it here since these are dry-run-only diagnostic aids, not a hot path. Task<T> forwarders become `return await`, non-generic Task ones `await`. Record the team convention in backend/AGENTS.md, citing Kevin's rationale on #2435. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
myieye
changed the title
Collaborator
|
Is dry run actually useful do you think? We're only using it in tests right now, which obviously don't really need to use a dry run, they can just do a real run. |
…y with TempCrdtProjectCopy Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
@hahn-kev Definitely a good question. I'd opt for keeping it. The concept has proven itself in other tools even if it hasn't quite proven itself for us. |
Collaborator
Author
|
@hahn-kev In fact I'm currently making use of this branch to debug a real project.
|
hahn-kev
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Claude, autonomous]
Dry-run sync wrapped the CRDT side in a record-only wrapper, so direction B's read-back (it reads the CRDT right after direction A "wrote" it) saw stale data. Mostly that got silently recorded, but
MorphTypeSync.Removecould throw on it, and the predicted fwdata change count was meaningless — the Sena3 test only checked CRDT changes and documented the fwdata side as garbage. That's what fails theDryRunSync_*tests on #2483.Now the CRDT side of a dry run runs against a disposable backup copy of its own sqlite db, so writes really apply and read back correctly; fwdata stays record-only (its file must not change) behind a new
WriteIgnoringMiniLcmApi.Sena3SyncTestsnow assertsFwdataChangesmatches a real sync, and before/after snapshot checks assert the real project is untouched.Considered and rejected: seeding morph-types (the fix on #2483). It papers over the root cause — the dry run was never faithfully simulating the CRDT side, so any diff that reads back a just-written value (not just morph-types) would still be wrong.