Make dry-run sync apply the CRDT side to a throwaway copy - #2492
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
|
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 |
|
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}
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
| public Task<PartOfSpeech> UpdatePartOfSpeech(Guid id, UpdateObjectInput<PartOfSpeech> update) | ||
| { | ||
| return api.GetPartOfSpeech(id)!; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AGENTS files in chain =="
for d in . backend backend/FwLite backend/FwLite/FwLiteProjectSync; do
[ -f "$d/AGENTS.md" ] && { echo "-- $d/AGENTS.md"; sed -n '1,160p' "$d/AGENTS.md"; }
done
echo "== locate file =="
fd -a 'WriteIgnoringMiniLcmApi.cs|CrdtMiniLcmApi.cs' backend || true
echo "== file outline =="
ast-grep outline backend/FwLite/FwLiteProjectSync/WriteIgnoringMiniLcmApi.cs --view compact || true
echo "== relevant WriteIgnoringMiniLcmApi.cs section =="
sed -n '1,140p' backend/FwLite/FwLiteProjectSync/WriteIgnoringMiniLcmApi.cs
echo "== relevant CrdtMiniLcmApi.cs signatures =="
rg -n "GetPartOfSpeech|GetSemanticDomain|GetComplexFormType|GetMorphType|GetPublication" backend/FwLite/FwLiteProjectSync/CrdtMiniLcmApi.cs -C 3
echo "== Nullable/Warnings in csproj =="
rg -n "<Nullable|<WarningsAsErrors|<TreatWarningsAsErrors|<ImplicitUsings" backend -g '*.csproj' -C 2 || true
echo "== targeted grep for ! on Task in project =="
rg -n "\.Get(Author|Publication|ComplexFormType|MorphType|PartOfSpeech|SemanticDomain)\([^)]*\)\s*!" backend/FwLite/FwLiteProjectSync backend/FwLite || trueRepository: sillsdev/languageforge-lexbox
Length of output: 22617
🏁 Script executed:
#!/bin/bash
set -u
echo "== exact CrdtMiniLcmApi file =="
find /home/jailuser/git/backend -name 'CrdtMiniLcmApi.cs' -type f -print
echo "== signature search CrdtMiniLcmApi.cs =="
if [ -f "/home/jailuser/git/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs" ]; then
rg -n "Task<.*>(?:GetPartOfSpeech|GetSemanticDomain|GetEntry|GetComplexFormType|GetMorphType|GetPublication)\(|GetPartOfSpeech|GetSemanticDomain|GetEntry|GetComplexFormType|GetMorphType|GetPublication" /home/jailuser/git/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs -C 2
else
echo "CrdtMiniLcmApi.cs not found"
fi
echo "== WriteIgnoringMiniLcmApi null-forgiving/await patterns =="
fd -a 'WriteIgnoringMiniLcmApi.cs' /home/jailuser/git/backend -x sh -c '
echo "-- $1"
rg -n "return api\.Get.*\([^)]*\)\s*!/|await api\.Get.*\([^)]*\)\s*\?\?|Task<.*> (Create|Update)[A-Za-z]+" "$1" -C 2
' sh {}
echo "== nullable/build properties =="
for f in $(git ls-files 'backend/**/*.csproj' 'Directory.Build.props' 'global.json'); do
echo "-- $f"
rg -n "<Nullable|<WarningsAsErrors|<TreatWarningsAsErrors>|<GenerateNullableSuppression|<ImplicitUsings" "$f" -C 2 || true
done
echo "== IMiniLcmReadApi Get method definitions =="
rg -n "IMiniLcmReadApi|Task<.*?> (?:Get[A-Z][A-Za-z]+|Get[A-Z][A-Za-z]+Async)" /home/jailuser/git/backend -g '*.cs' | head -n 200Repository: sillsdev/languageforge-lexbox
Length of output: 45289
🌐 Web query:
C# postfix ! operator applies to outermost type Task<T?> to Task<T> nullable warnings errors
💡 Result:
In C#, the postfix! operator is the null-forgiving (or null-suppression) operator, which is used to tell the compiler that a specific expression is not null, even if the compiler's static flow analysis cannot verify that [1][2]. When dealing with Task<T?> and Task, applying! at the end of an expression (e.g., myTask!) acts on the entire expression, not the internal type T [1][3]. It suppresses warnings by forcing the compiler to treat the expression as non-null [1][2]. However, it does not change the underlying type of the object at runtime [1]. Key points regarding Task<T?> and Task nullability: 1. Runtime vs. Compile Time: The! operator has no effect at runtime [1]. It is purely a directive for the compiler's static analysis [1][2]. 2. Covariance Issues: The C# compiler does not inherently treat Task as covariant with Task<T?> regarding nullability [4]. Returning a Task from a method signature that expects Task<T?> will often trigger a compiler warning or error because the compiler cannot guarantee that the downstream consumer of the Task<T?> will handle potential null values correctly [5][4]. 3. Suppression Limitations: While you can use task! to suppress a warning when returning a Task where a Task<T?> is expected, this is generally discouraged for public APIs [6][2]. It effectively hides a potential contract mismatch [6][4]. If the consumer of your API expects a nullable result (Task<T?>) but receives a non-nullable one (Task) that has been suppressed, they may inadvertently write code that ignores null checks, potentially leading to runtime NullReferenceExceptions [5][2]. 4. Recommended Approaches: - Constrain T: If you can, use generic constraints (e.g., where T: class) to clarify nullability intent [5][7]. - Explicit Conversion: Consider if the API design itself needs adjustment, such as providing separate overloads for nullable and non-nullable scenarios [5][7]. - Use Attributes: For method parameters or return values, use nullable analysis attributes like [MaybeNull] or [NotNull] to provide better metadata to the compiler without relying on the null-forgiving operator [8][6]. Use the null-forgiving operator sparingly, and only when you are certain that the static analysis is incorrectly flagging a safe operation [6][2].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-forgiving
- 2: https://www.aicodesnippet.com/c-sharp-tutorials/modern-c-sharp-features/what-is-the-null-forgiving-operator-and-when-should-it-be-used.html
- 3: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/nullable-warnings
- 4: Cannot implicitly convert
Task<T>toTask<T?>dotnet/roslyn#38261 - 5: https://stackoverflow.com/questions/61448239/nullability-warning-for-task-fromresult
- 6: https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/null-safety/resolve-warnings
- 7: https://stackoverflow.com/questions/79534411/persistent-error-cannot-implicitly-convert-type-t-to-t
- 8: Cannot Have Nullable Return of Async Generic Method dotnet/csharplang#4231
🏁 Script executed:
#!/bin/bash
set -u
echo "== FwLiteProjectSync project files =="
fd -a -e csproj 'FwLiteProjectSync' /home/jailuser/git/backend -x sh -c 'echo "-- $1"; sed -n "1,220p" "$1"' sh {}
echo "== interface return nullability around relevant methods =="
sed -n '1,90p' /home/jailuser/git/backend/FwLite/MiniLcm/IMiniLcmReadApi.cs | cat -n
sed -n '1,160p' /home/jailuser/git/backend/FwLite/MiniLcm/IMiniLcmApi.cs | cat -nRepository: sillsdev/languageforge-lexbox
Length of output: 8221
Await and validate PartOfSpeech/SemanticDomain getters before returning the update.
GetPartOfSpeech and GetSemanticDomain return Task<PartOfSpeech?>/Task<SemanticDomain?>; applying ! to the task expression only suppresses the nullable annotation on that expression at compile time and does not change the runtime result. Make these match the sibling update methods: await then throw when the id is missing, so deleted refs fail fast instead of returning null typed as non-null.
Also applies to lines 71-74.
🤖 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/WriteIgnoringMiniLcmApi.cs` around lines 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.
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
|
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. |
2 participants
[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.