feat(sync): gracefully handle SIGTERMs#516
Merged
ErikBjare merged 2 commits intoActivityWatch:masterfrom Apr 4, 2025
Merged
Conversation
5e86d5b to
0da116b
Compare
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 5e86d5b in 1 minute and 52 seconds
More details
- Looked at
150lines of code in3files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. aw-sync/src/main.rs:115
- Draft comment:
Consider trimming whitespace in parse_list so that comma-separated inputs like "host1, host2" do not yield strings with extra spaces. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. aw-sync/src/main.rs:144
- Draft comment:
Comment discrepancy: the comment states the default subcommand is Sync, but the code defaults to Daemon. Please update the comment to match the implementation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. aw-sync/src/main.rs:179
- Draft comment:
When validating the sync_db path, the check using starts_with might be affected by symlinks or path normalization. Consider canonicalizing paths to ensure robust verification. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. aw-sync/src/main.rs:220
- Draft comment:
In the daemon loop, errors during a sync cycle cause an immediate return. If some errors are transient, consider logging and continuing rather than terminating the daemon. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. aw-sync/src/main.rs:128
- Draft comment:
Use.into()for error conversion inErr("Sync dir must be absolute")?. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. aw-sync/src/main.rs:182
- Draft comment:
Wrap error message with.into()inErr("Sync db path must be absolute")?. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. aw-sync/src/main.rs:185
- Draft comment:
Wrap error message with.into()inErr("Sync db path must be in sync directory")?. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. aw-sync/src/main.rs:115
- Draft comment:
Consider trimming whitespace inparse_listfor better input handling. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. aw-sync/src/main.rs:144
- Draft comment:
Comment says default subcommand is Sync but defaults to Daemon; update comment or default value. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. aw-sync/src/main.rs:10
- Draft comment:
Remove unnecessaryextern cratedeclarations for edition 2021. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. aw-sync/src/main.rs:205
- Draft comment:
Typo: The comment currently reads '...give it some wiggleroom.' Consider changing 'wiggleroom' to 'wiggle room' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_qDj5Gz81m3qQp2xO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
a8c89a4 to
630f0df
Compare
Member
Author
|
@ellipsis-dev review |
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 630f0df in 1 minute and 5 seconds
More details
- Looked at
166lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. aw-sync/src/main.rs:1
- Draft comment:
TODO list: Consider updating TODO comments to reflect that start date now exists in SyncAdvanced command. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
2. aw-sync/src/main.rs:137
- Draft comment:
Consider simplifying port extraction. Instead of mapping port to Ok(a) then unwrapping, use a more straightforward pattern. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
3. aw-sync/src/main.rs:180
- Draft comment:
When checking the sync_db path, consider canonicalizing both paths to mitigate symlink issues. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. aw-sync/src/main.rs:113
- Draft comment:
Consider trimming whitespace when splitting the list in parse_list. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
5. aw-sync/src/main.rs:10
- Draft comment:
Remove redundantextern cratedeclarations (not needed in 2021 edition). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. aw-sync/src/main.rs:142
- Draft comment:
The comment states the default subcommand is Sync, yetunwrap_or(Commands::Daemon {})defaults to Daemon. Clarify intended behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. aw-sync/src/main.rs:113
- Draft comment:
Consider trimming whitespace inparse_listto avoid issues with spaces in comma‐separated inputs. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. aw-sync/src/main.rs:127
- Draft comment:
Consider using more descriptive error types (or wrapping errors) instead of bare string errors for invalid sync directory or db paths. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_9IrljRwfrkCezRmC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
630f0df to
6f03e5a
Compare
Member
|
Nice, thanks! |
0xbrayo
added a commit
to 0xbrayo/aw-server-rust
that referenced
this pull request
Apr 26, 2025
* chore(sync): add ctrlc dependency * feat(sync): gracefully handle SIGTERMs
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
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.
#502
Important
Adds graceful SIGTERM handling to
aw-syncdaemon usingctrlccrate for clean shutdown.daemon()inaw-sync/src/main.rsusingctrlccrate.ctrlccrate toaw-sync/Cargo.tomlandCargo.lockfor signal handling.coloredandfernversions inCargo.lock.Cargo.lockversion to 4.This description was created by
for 630f0df. It will automatically update as commits are pushed.