Clean up failed clones and improve error logging#2188
Open
Conversation
github-actions
bot
added
💻 FW Lite
📝 WalkthroughWalkthroughThis change introduces a new custom exception ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
Contributor
C# Unit Tests164 tests 164 ✅ 18s ⏱️ Results for commit aa7a4b8. |
|
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
🧹 Nitpick comments (2)
backend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs (1)
115-121: KeepSetCloneResultbehavior consistent with the default clone mock.This helper currently skips the post-clone fwdata-file creation path that
SetupDefaultMocks()applies. Aligning them reduces test setup surprises.♻️ Proposed refactor
public void SetCloneResult(SendReceiveHelpers.LfMergeBridgeResult result) { SendReceiveMock .Setup(s => s.Clone(It.IsAny<FwDataProject>(), ProjectCode)) - .Callback(() => Steps.Add(Clone)) + .Callback(() => + { + Steps.Add(Clone); + if (result.Success && _createFwDataFileAfterClone) EnsureFwDataFileExists(); + }) .ReturnsAsync(result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs` around lines 115 - 121, SetCloneResult currently stubs SendReceiveMock.Clone but omits the post-clone fwdata-file creation that SetupDefaultMocks applies; update SetCloneResult so its SendReceiveMock.Setup(s => s.Clone(...)) uses the same Callback and post-clone behavior used in SetupDefaultMocks (i.e., call Steps.Add(Clone) and invoke the same fwdata-file creation routine or helper that SetupDefaultMocks triggers after clone) so tests using SetCloneResult mirror the default clone mock behavior.backend/Testing/FwHeadless/Services/SyncWorkerTests.cs (1)
306-307: Rename this test to match the asserted status.The method name says
ReturnsUnableToSync, but the assertion expectsProjectIncompatible. Renaming will make failures easier to interpret.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/Testing/FwHeadless/Services/SyncWorkerTests.cs` around lines 306 - 307, The test method ExecuteSync_FwDataFileStillMissingAfterPreSetup_ReturnsUnableToSync has a mismatched name vs its assertion; rename the method to reflect the asserted status (e.g., ExecuteSync_FwDataFileStillMissingAfterPreSetup_ReturnsProjectIncompatible) so the test name matches the expected ProjectIncompatible outcome and update any references to this method accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/FwHeadless/Services/SyncHostedService.cs`:
- Around line 325-332: The current catch block that attempts to delete
fwDataProject.ProjectsPath can throw and replace the original clone/validation
exception; wrap the cleanup (Directory.Exists, logger.LogInformation,
Directory.Delete) in its own try-catch so any exception thrown during cleanup is
caught and logged (including the cleanup exception details and the path via
fwDataProject.ProjectsPath) but not rethrown, then rethrow the original
exception outside that inner try-catch to preserve the root cause; reference the
existing symbols fwDataProject.ProjectsPath, logger.LogInformation,
Directory.Delete and the surrounding catch/throw logic in SyncHostedService.cs.
---
Nitpick comments:
In `@backend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs`:
- Around line 115-121: SetCloneResult currently stubs SendReceiveMock.Clone but
omits the post-clone fwdata-file creation that SetupDefaultMocks applies; update
SetCloneResult so its SendReceiveMock.Setup(s => s.Clone(...)) uses the same
Callback and post-clone behavior used in SetupDefaultMocks (i.e., call
Steps.Add(Clone) and invoke the same fwdata-file creation routine or helper that
SetupDefaultMocks triggers after clone) so tests using SetCloneResult mirror the
default clone mock behavior.
In `@backend/Testing/FwHeadless/Services/SyncWorkerTests.cs`:
- Around line 306-307: The test method
ExecuteSync_FwDataFileStillMissingAfterPreSetup_ReturnsUnableToSync has a
mismatched name vs its assertion; rename the method to reflect the asserted
status (e.g.,
ExecuteSync_FwDataFileStillMissingAfterPreSetup_ReturnsProjectIncompatible) so
the test name matches the expected ProjectIncompatible outcome and update any
references to this method accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/FwHeadless/Services/InvalidFwDataProjectException.csbackend/FwHeadless/Services/SyncHostedService.csbackend/LexCore/Sync/SyncJobResult.csbackend/Testing/FwHeadless/Services/SyncWorkerTestHarness.csbackend/Testing/FwHeadless/Services/SyncWorkerTests.csfrontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Sync/SyncJobStatusEnum.ts
Comment on lines
+325
to
+332
| catch | ||
| { | ||
| logger.LogInformation("Clone result before CRDT sync: {Output}", srResult.Output); | ||
| if (Directory.Exists(fwDataProject.ProjectsPath)) | ||
| { | ||
| logger.LogInformation("Cleaning up project folder after failed clone: {ProjectFolder}", fwDataProject.ProjectsPath); | ||
| Directory.Delete(fwDataProject.ProjectsPath, true); | ||
| } | ||
| throw; |
There was a problem hiding this comment.
Don’t let cleanup failures mask the real clone error.
Directory.Delete(...) inside this catch can throw and replace the original clone/validation exception, which obscures root cause and complicates status reporting.
🛠️ Proposed fix
- catch
+ catch
{
- if (Directory.Exists(fwDataProject.ProjectsPath))
+ try
{
- logger.LogInformation("Cleaning up project folder after failed clone: {ProjectFolder}", fwDataProject.ProjectsPath);
- Directory.Delete(fwDataProject.ProjectsPath, true);
+ if (Directory.Exists(fwDataProject.ProjectsPath))
+ {
+ logger.LogInformation("Cleaning up project folder after failed clone: {ProjectFolder}", fwDataProject.ProjectsPath);
+ Directory.Delete(fwDataProject.ProjectsPath, true);
+ }
}
+ catch (Exception cleanupEx)
+ {
+ logger.LogWarning(cleanupEx, "Failed to clean up project folder after failed clone: {ProjectFolder}", fwDataProject.ProjectsPath);
+ }
throw;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/FwHeadless/Services/SyncHostedService.cs` around lines 325 - 332, The
current catch block that attempts to delete fwDataProject.ProjectsPath can throw
and replace the original clone/validation exception; wrap the cleanup
(Directory.Exists, logger.LogInformation, Directory.Delete) in its own try-catch
so any exception thrown during cleanup is caught and logged (including the
cleanup exception details and the path via fwDataProject.ProjectsPath) but not
rethrown, then rethrow the original exception outside that inner try-catch to
preserve the root cause; reference the existing symbols
fwDataProject.ProjectsPath, logger.LogInformation, Directory.Delete and the
surrounding catch/throw logic in SyncHostedService.cs.
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
1 participant
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.
We recently had a sync failure with the message:
It took me a little while to realize how that happened: It was a WeSay project tagged as a FieldWorks project.
So, I decided:
That's what this PR does ☝️