feat: Improve daemon stability and resource management #193
Conversation
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
Reviewer's GuideThis PR tightens systemd resource and restart limits for deepin-anything-daemon and hardens the daemon’s filesystem-related code paths to avoid crashes and leaks by improving error handling and cleanup behavior. Sequence diagram for enhanced scan_directory error handlingsequenceDiagram
participant beh as base_event_handler
participant fs as filesystem
participant handler as PathHandler
participant log as spdlog
beh->>fs: construct recursive_directory_iterator(dir_path, skip_permission_denied)
loop for each entry
beh->>fs: exists(entry.path, ec)
fs-->>beh: exists result
alt path in blacklist or not exists
beh->>fs: iterator.disable_recursion_pending()
else path allowed
beh->>handler: handler(path)
alt handler returns false
beh->>log: error Failed to handle path
beh-->>beh: return false
else handler returns true
alt stop_scan_directory_ is true
beh->>log: info Scanning interrupted
beh-->>beh: return true
end
end
end
end
beh-->>log: info Scanning directory completed
rect rgb(240,200,200)
beh->>fs: recursive iteration may throw filesystem_error
fs-->>beh: throw filesystem_error
beh->>log: error Failed to scan directory with details
end
Sequence diagram for file_index_manager index cleanup with error_codesequenceDiagram
participant fim as file_index_manager
participant fs as filesystem
participant log as spdlog
participant writer as IndexWriter
participant reader as IndexReader
rect rgb(230,230,255)
fim->>log: warn The index is corrupted or invalid
opt writer_ not null
fim->>writer: close()
end
opt reader_ not null
fim->>reader: close()
end
fim->>fs: remove_all(volatile_index_directory_, ec)
fs-->>fim: error_code ec (no exception)
end
rect rgb(230,255,230)
fim->>fs: exists(volatile_index_directory_ + /invalid_index, ec)
alt invalid_index exists
fim->>log: warn The index is invalid, remove it
fim->>fs: remove_all(volatile_index_directory_, ec)
else
fim->>log: warn Index corrupted or version mismatch
fim->>fs: remove_all(volatile_index_directory_, ec)
end
end
Class diagram for updated base_event_handler and file_index_managerclassDiagram
class base_event_handler {
- thread_pool pool_
- vector~Job~ jobs_
- bool stop_scan_directory_
- config_ptr config_
+ void terminate_processing()
+ bool scan_directory(string dir_path, function<bool(string)> handler)
}
class file_index_manager {
- IndexWriterPtr writer_
- IndexReaderPtr reader_
- string volatile_index_directory_
+ file_index_manager(string persistent_index_dir, string volatile_index_dir)
+ void check_index_version()
}
class config {
+ vector~string~ blacklist_paths
}
class spdlog {
+ static void info(string message, string arg1)
+ static void info(string message)
+ static void warn(string message, string arg1)
+ static void error(string message, string arg1)
+ static void error(string message, string arg1, string arg2, string arg3, int arg4)
}
class filesystem {
+ static bool exists(path p, error_code ec)
+ static uintmax_t remove_all(path p, error_code ec)
+ class filesystem_error
}
base_event_handler --> config : uses
base_event_handler ..> spdlog : logging
base_event_handler ..> filesystem : recursive_directory_iterator
file_index_manager ..> filesystem : exists, remove_all
file_index_manager ..> spdlog : logging
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
scan_directory, catchingstd::filesystem::filesystem_errorand then falling through to log "Scanning directory ... completed" and returningtruewill mask real failures; consider returningfalse(or otherwise signaling failure) from the catch block and adjusting the completion log message accordingly. - Now that
std::filesystem::remove_allcalls use anstd::error_code, the error is effectively ignored; if a failure to remove the index directory is meaningful in these code paths, consider loggingec(e.g.,ec.value()/ec.message()) when it indicates an error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `scan_directory`, catching `std::filesystem::filesystem_error` and then falling through to log "Scanning directory ... completed" and returning `true` will mask real failures; consider returning `false` (or otherwise signaling failure) from the catch block and adjusting the completion log message accordingly.
- Now that `std::filesystem::remove_all` calls use an `std::error_code`, the error is effectively ignored; if a failure to remove the index directory is meaningful in these code paths, consider logging `ec` (e.g., `ec.value()`/`ec.message()`) when it indicates an error.
## Individual Comments
### Comment 1
<location> `src/daemon/src/core/base_event_handler.cpp:364-368` </location>
<code_context>
- continue;
- }
- if (!handler(path)) {
- return false;
- }
+ try {
+ // By default, symlinks are not followed
+ std::filesystem::recursive_directory_iterator dirpos{ dir_path, std::filesystem::directory_options::skip_permission_denied };
+ for (auto it = begin(dirpos); it != end(dirpos); ++it) {
+ path = std::move(it->path().string());
+ if (is_path_in_blacklist(path, config_->blacklist_paths) ||
+ !std::filesystem::exists(it->path(), ec)) {
+ it.disable_recursion_pending();
+ continue;
+ }
- if (stop_scan_directory_) {
- spdlog::info("Scanning interrupted");
- return true;
+ if (!handler(path)) {
+ spdlog::error("Failed to handle path: {}", path);
+ return false;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Handler returning false is now treated as an error, which may change existing semantics.
Previously, `handler(path)` returning `false` just stopped the scan without logging an error. Callers may legitimately use `false` for a normal early stop (e.g., “found what I needed”), so this change could both alter behavior and flood logs with errors. Consider either keeping `false` as a non-error early exit (no error log), clearly documenting that `false` now means failure, or introducing a distinct way to signal “stop” vs “error” (e.g., a richer return type).
</issue_to_address>
### Comment 2
<location> `src/daemon/src/core/base_event_handler.cpp:377-380` </location>
<code_context>
+ return true;
+ }
}
+ } catch (std::filesystem::filesystem_error const& e) {
+ spdlog::error("Failed to scan directory: {}, {}, {}, {}",
+ e.what(), e.path1().string(), e.path2().string(), e.code().value());
}
</code_context>
<issue_to_address>
**suggestion:** Filesystem error logging omits the human-readable error message from `std::error_code`.
Right now the log only prints `e.code().value()`, which is just a numeric code. Please also log `e.code().message()` so operators can see the OS-level error text (e.g., "permission denied"). For example:
```cpp
spdlog::error(
"Failed to scan directory: {}, {}, {}, {} ({})",
e.what(),
e.path1().string(),
e.path2().string(),
e.code().value(),
e.code().message());
```
```suggestion
} catch (std::filesystem::filesystem_error const& e) {
spdlog::error(
"Failed to scan directory: {}, {}, {}, {} ({})",
e.what(),
e.path1().string(),
e.path2().string(),
e.code().value(),
e.code().message());
}
```
</issue_to_address>
### Comment 3
<location> `src/daemon/src/core/base_event_handler.cpp:382` </location>
<code_context>
+ e.what(), e.path1().string(), e.path2().string(), e.code().value());
}
spdlog::info("Scanning directory {} completed", dir_path);
</code_context>
<issue_to_address>
**issue (bug_risk):** The "completed" log may be misleading when the scan exits via exception.
Because the `catch` for `std::filesystem::filesystem_error` logs the error and then falls through, we still emit `"Scanning directory {} completed"` even when the scan failed. Please either exit early from the `catch` or gate the final log on a success flag so it only indicates completion on success.
</issue_to_address>
### Comment 4
<location> `src/daemon/src/core/file_index_manager.cpp:187-190` </location>
<code_context>
std::error_code ec;
std::string path;
- // By default, symlinks are not followed
</code_context>
<issue_to_address>
**issue (bug_risk):** Swallowing errors from `remove_all` can leave a corrupted index without any signal.
Switching from the throwing `remove_all` to the `std::error_code` overload makes failures silent. Right now `ec` is ignored, so callers proceed as if the directory was removed even when it wasn’t, which can leave a bad index in place and cause confusing follow‑on behavior.
At minimum, check and log failures, e.g.:
```cpp
std::filesystem::remove_all(volatile_index_directory_, ec);
if (ec) {
spdlog::error("Failed to remove corrupted index {}: {} ({})",
volatile_index_directory_, ec.message(), ec.value());
}
```
The same pattern should be applied to the other `remove_all(..., ec)` calls in `check_index_version()` so cleanup issues are visible during diagnosis.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+187
to
+190
| std::error_code ec; | ||
| if (writer_) writer_->close(); | ||
| if (reader_) reader_->close(); | ||
| std::filesystem::remove_all(volatile_index_directory_); | ||
| std::filesystem::remove_all(volatile_index_directory_, ec); |
There was a problem hiding this comment.
issue (bug_risk): Swallowing errors from remove_all can leave a corrupted index without any signal.
Switching from the throwing remove_all to the std::error_code overload makes failures silent. Right now ec is ignored, so callers proceed as if the directory was removed even when it wasn’t, which can leave a bad index in place and cause confusing follow‑on behavior.
At minimum, check and log failures, e.g.:
std::filesystem::remove_all(volatile_index_directory_, ec);
if (ec) {
spdlog::error("Failed to remove corrupted index {}: {} ({})",
volatile_index_directory_, ec.message(), ec.value());
}The same pattern should be applied to the other remove_all(..., ec) calls in check_index_version() so cleanup issues are visible during diagnosis.
lzwind
approved these changes
Nov 28, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, wangrong1069 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
- Adjust deepin-anything-daemon service unit to limit memory usage and prevent excessive restarts.
- Reduced MemoryHigh from 4G to 2G and MemoryMax from 5G to 3G.
- Added StartLimitIntervalSec and StartLimitBurst to control restart frequency.
- Enhance error handling for filesystem operations in base_event_handler and file_index_manager.
- Wrapped directory scanning in scan_directory with a try-catch block for std::filesystem::filesystem_error.
- Added error code parameter to std::filesystem::remove_all calls to prevent crashes on non-existent directories or permission issues.
- Ensured jobs_ are cleared after termination processing.
|
TAG Bot TAG: 7.0.33 |
deepin pr auto review我来对这段代码进行详细的审查分析：
建议：
建议：
建议：
建议：
整体安全性评估：
性能优化建议：
代码质量建议：
这些修改整体上提高了系统的稳定性和资源管理效率，是一个良好的改进。建议在后续版本中持续监控这些改进的效果，并根据实际使用情况进行调整。 |
Contributor
Author
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary by Sourcery
Improve daemon robustness by tightening systemd resource limits and hardening filesystem-related error handling in the daemon core.
Bug Fixes:
Enhancements: