You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Staging PR — never merge; promoted to sillsdev when polished (see FORK.md). Stacked on #8 (demo project + harness); the last commit is this change.
Harmony's AlwaysValidateCommits re-validates the whole commit history on every local CRDT commit. FwHeadlessConfig now exposes it (default true — behavior unchanged until ops opt out); remote-sync validation (SyncWith/AddRangeFromSync) is unconditional in Harmony and unaffected. LcmDebugger sync --no-validate measures the same configuration.
Measured on the slow-sync demo (30605 commits), dry run:
baseline
--no-validate
sync time
17m28s
15m40s (−10%)
avg per change record
437ms
392ms
CrdtChanges / FwdataChanges
2400 / 0
2400 / 0 (identical — correctness gate)
Applies to FwHeadless real syncs (when opted in), dry runs, and LcmDebugger. The gain is modest because the flag only gates Harmony's single-commit path (component links); CreateEntry goes through AddManyChanges, which validates unconditionally in Harmony — gating that consistently is a small Harmony-side change I'll measure on the Harmony branch work. Build is green; no test run — the change is config plumbing with an unchanged default, verified by the identical dry-run results above.
…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 sillsdev#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>
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 sillsdev#2435.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g long syncs
- FwDataFactory.PreventEviction resets the LcmCache sliding expiration until disposed; used by the sync service so >30-min syncs don't lose the cache mid-run
- Log each SyncInternal phase and every 50 synced entries
- CrdtRepairs: tolerate an already-patched crdt translation id (interrupted sync)
- LcmDebugger: sbe-flex dry-run harness, optional FTS-less registration
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bugger
generate: builds a deterministic demo project (fwdata + crdt.sqlite +
fw_snapshot.json) with a large single-change commit history, a snapshot
lagging fwdata by ~10% of entries, and complex-form components among the
lagging entries. Entries go through DataModel.AddChanges because the
api's AddManyChanges path validates the whole commit history
unconditionally; AlwaysValidateCommits is only disabled for generation.
sync: runs a FwHeadless-style dry-run sync on a project copy and reports
per-phase timing (from the sync service's own phase log messages) plus
dry-run record counts, for before/after numbers on optimizations.
Fidelity check: a zero-lag generated project dry-run syncs with
CrdtChanges 0 / FwdataChanges 0; with lag, changes are exactly the
lagging creates and component links.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DHonvweBYC4Eo5iNAehfv3
Generated with seed 42: 10000 entries (1000 lagging from fw_snapshot),
800 synced + 700 lagging complex-form links, 30605 CRDT commits.
Fetch with scripts/unpack-demo-project.sh, then:
dotnet run --project backend/FwLite/LcmDebugger -- sync slow-sync-demo
The archive lives here rather than under deployment/_downloads because
that folder is gitignored.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DHonvweBYC4Eo5iNAehfv3
AlwaysValidateCommits re-validates the whole commit history on every
local CRDT commit — O(history) per commit, dominating FwData→CRDT sync
time on large projects. FwHeadlessConfig now exposes it (default true,
so behavior is unchanged until ops opt out). Remote-sync validation
(SyncWith/AddRangeFromSync) is unconditional in Harmony and unaffected.
LcmDebugger sync gains --no-validate to measure the same configuration.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DHonvweBYC4Eo5iNAehfv3
Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58196d58-3a9e-4d92-88bf-09558204dceb
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
🔍 Trigger review
Comment @coderabbitai help to get the list of available commands.
We reviewed changes in 68bd16d...0c73ffc on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The reason will be displayed to describe this comment to others. Learn more.
Method with return type `Task` does not follow the naming convention
The consensus in .NET is to have names of methods dealing with asynchronous operations suffixed with Async. One such example is Stream.ReadAsync from System.IO. Doing so improves readability and provides crucial information at a glance.
The reason will be displayed to describe this comment to others. Learn more.
Build has a cyclomatic complexity of 26 with "very-high" risk
A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.
The reason will be displayed to describe this comment to others. Learn more.
Method with return type `Task` does not follow the naming convention
The consensus in .NET is to have names of methods dealing with asynchronous operations suffixed with Async. One such example is Stream.ReadAsync from System.IO. Doing so improves readability and provides crucial information at a glance.
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
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.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
[Claude, autonomous]
Staging PR — never merge; promoted to sillsdev when polished (see FORK.md). Stacked on #8 (demo project + harness); the last commit is this change.
Harmony's
AlwaysValidateCommitsre-validates the whole commit history on every local CRDT commit.FwHeadlessConfignow exposes it (defaulttrue— behavior unchanged until ops opt out); remote-sync validation (SyncWith/AddRangeFromSync) is unconditional in Harmony and unaffected.LcmDebugger sync --no-validatemeasures the same configuration.Measured on the slow-sync demo (30605 commits), dry run:
--no-validateApplies to FwHeadless real syncs (when opted in), dry runs, and LcmDebugger. The gain is modest because the flag only gates Harmony's single-commit path (component links);
CreateEntrygoes throughAddManyChanges, which validates unconditionally in Harmony — gating that consistently is a small Harmony-side change I'll measure on the Harmony branch work. Build is green; no test run — the change is config plumbing with an unchanged default, verified by the identical dry-run results above.Generated by Claude Code