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).
Adds a generate command to LcmDebugger that builds a deterministic demo project whose FwData↔CRDT sync is slow for the right reasons (30605-commit CRDT history, fw_snapshot lagging fwdata by 1000 entries, 700 complex-form links among the lagging entries), and a sync command that runs a FwHeadless-style dry-run sync with per-phase timing. The 36MB artifact is committed; scripts/unpack-demo-project.sh retrieves it without regenerating.
Baseline (BASELINE.md): dry-run sync 17m28s, 437ms avg per change record, CrdtChanges 2400 / FwdataChanges 0 — the correctness gate for the optimization PRs that follow.
Includes the dry-run-sync-fidelity commits it builds on (dry-run against a throwaway CRDT copy, phase/progress logging, PreventEviction); the last 4 commits are new.
…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
We reviewed changes in 68bd16d...cb49117 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.
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: 71ca7d2d-a957-49d8-8150-dc2c0b12acf6
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.
The reason will be displayed to describe this comment to others. Learn more.
Variable 'keepFwdataAlive' is declared but not used
A variable is declared but not used within its scope.
Unused variables can clutter the codebase, reduce readability, and potentially indicate logical errors or incomplete implementations.
If you intend to keep a variable unused intentionally, you can use an underscore (_) as the variable name.
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.
Variable 'keepAlive' is declared but not used
A variable is declared but not used within its scope.
Unused variables can clutter the codebase, reduce readability, and potentially indicate logical errors or incomplete implementations.
If you intend to keep a variable unused intentionally, you can use an underscore (_) as the variable name.
The reason will be displayed to describe this comment to others. Learn more.
Property returns an array
Accessors, i.e. getters to be precise, returns a specific value. There are no constraints placed on what a getter can return.
However, in this case, the accessor seems to return an array that is a private field.
In such cases, the returned array may be modified by the user as they are not write-protected.
Instead, it is recommended that you either not return an array or convert this property to a method and return a copy instead.
The reason will be displayed to describe this comment to others. Learn more.
Property returns an array
Accessors, i.e. getters to be precise, returns a specific value. There are no constraints placed on what a getter can return.
However, in this case, the accessor seems to return an array that is a private field.
In such cases, the returned array may be modified by the user as they are not write-protected.
Instead, it is recommended that you either not return an array or convert this property to a method and return a copy instead.
The reason will be displayed to describe this comment to others. Learn more.
Consider using `.TryParse` over `Parse`
Methods such as int.Parse and double.Parse throw Exceptions such as ArgumentNullException, ArgumentException, FormatException or OverflowException depending on the input. Incorrectly handling these Exceptions can cause issues during the runtime. It is therefore suggested that you use the safer alternative .TryParse.
The reason will be displayed to describe this comment to others. Learn more.
Consider using `.TryParse` over `Parse`
Methods such as int.Parse and double.Parse throw Exceptions such as ArgumentNullException, ArgumentException, FormatException or OverflowException depending on the input. Incorrectly handling these Exceptions can cause issues during the runtime. It is therefore suggested that you use the safer alternative .TryParse.
The reason will be displayed to describe this comment to others. Learn more.
Consider using `.TryParse` over `Parse`
Methods such as int.Parse and double.Parse throw Exceptions such as ArgumentNullException, ArgumentException, FormatException or OverflowException depending on the input. Incorrectly handling these Exceptions can cause issues during the runtime. It is therefore suggested that you use the safer alternative .TryParse.
The reason will be displayed to describe this comment to others. Learn more.
Consider using `.TryParse` over `Parse`
Methods such as int.Parse and double.Parse throw Exceptions such as ArgumentNullException, ArgumentException, FormatException or OverflowException depending on the input. Incorrectly handling these Exceptions can cause issues during the runtime. It is therefore suggested that you use the safer alternative .TryParse.
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.
Calling `Environment.Exit()` may terminate the program in an inconsistent manner
Using Environment.Exit(0) terminates the application abruptly in a potentially unsafe manner. An application should always try to quit gracefully irrespective of whether it encounters an error or not. If your application has to terminate, ensure that it implements the required cleanup/dispose methods to at least safely dispose off the resources that it has acquired/locked to the extent possible and then exit cleanly/gracefully.
The reason will be displayed to describe this comment to others. Learn more.
Calling `Environment.Exit()` may terminate the program in an inconsistent manner
Using Environment.Exit(0) terminates the application abruptly in a potentially unsafe manner. An application should always try to quit gracefully irrespective of whether it encounters an error or not. If your application has to terminate, ensure that it implements the required cleanup/dispose methods to at least safely dispose off the resources that it has acquired/locked to the extent possible and then exit cleanly/gracefully.
DeepSource triage: fixed the unused using vars, array-returning properties, Parse→TryParse, and the Environment.Exit follow-ups. Skipped deliberately: the two Async-suffix findings (this codebase doesn't use the suffix — CreateEntry, OpenProject, etc.) and the cyclomatic-complexity finding on DemoSpec.Build (a linear spec builder; splitting it would hurt readability). Those can be ignored in the dashboard if you agree.
The "Build FW Lite and run tests" failure isn't this branch: the build and both repo checks pass, then the workflow's own sudo timedatectl set-timezone step fails with "Access denied" — a runner-image/permissions issue in a workflow file this branch doesn't touch. It's an upstream workflow that apparently slipped past the disable-on-fork pass (I can't disable workflows from this sandbox), so treating it as expected-fail here; it likely needs re-disabling or an upstream fix.
Details and rationale in backend/FwLite/LcmDebugger/BASELINE.md. The harmony fix is the real win — with it in place, disabling validation adds nothing at this scale. Open items for you: promote the harmony branch to sillsdev/harmony (note: lexbox breaks against harmony ≥aae2ee0 with "ChangeTypeListBuilder is frozen"), and the two dashboard-ignorable DeepSource findings keeping its check red.
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.
[Claude, autonomous]
Staging PR — never merge; promoted to sillsdev when polished (see FORK.md).
Adds a
generatecommand to LcmDebugger that builds a deterministic demo project whose FwData↔CRDT sync is slow for the right reasons (30605-commit CRDT history, fw_snapshot lagging fwdata by 1000 entries, 700 complex-form links among the lagging entries), and asynccommand that runs a FwHeadless-style dry-run sync with per-phase timing. The 36MB artifact is committed;scripts/unpack-demo-project.shretrieves it without regenerating.Baseline (BASELINE.md): dry-run sync 17m28s, 437ms avg per change record,
CrdtChanges 2400 / FwdataChanges 0— the correctness gate for the optimization PRs that follow.Includes the
dry-run-sync-fidelitycommits it builds on (dry-run against a throwaway CRDT copy, phase/progress logging,PreventEviction); the last 4 commits are new.Generated by Claude Code