Planning and implementing source code for module improvements - #6236
Conversation
Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new scheduler accepts cron syntax (whitespace and '/' steps) that the executor cannot reliably parse/match, and the HSM stub cert-serial path currently introduces avoidable noisy/error logging and INFO-level identifier logging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends GAP-008 backup scheduling from a pure in-memory registry to an in-process background executor, and improves the HSM stub’s certificate-serial behavior to be deterministic (for dev/CI) with corresponding test coverage.
Changes:
- Start a background scheduler thread in
BackupManagerto trigger scheduled backups and add cron matching logic. - Add a scheduled-backup execution test (in addition to existing schedule registration/cancellation tests).
- Replace the HSMPKIClient cert-serial hardcoded stub with a deterministic derived fallback and add a test for determinism.
File summaries
| File | Description |
|---|---|
| tests/test_gap008_backup_automation.cpp | Adds a test that verifies scheduled backups result in an on-disk backup payload. |
| tests/security/test_hsm_provider.cpp | Adds a test asserting deterministic fallback behavior for HSMPKIClient::getCertSerial(). |
| src/storage/backup_manager.cpp | Implements the scheduler loop, cron matching, and backup execution from schedules. |
| src/security/hsm_provider.cpp | Derives a deterministic fallback cert serial in the stub HSM implementation. |
| include/storage/backup_manager.h | Adds scheduler state, thread, and bookkeeping for last-triggered minute. |
Review details
Suppressed comments (1)
src/storage/backup_manager.cpp:723
- shouldRunScheduledBackup() splits the cron expression using getline(..., ' '), which makes multiple spaces produce empty fields and cause schedules (which scheduleBackup() accepts, since validation uses operator>> and collapses whitespace) to never trigger.
std::stringstream stream(entry.cron_expression);
std::string field;
std::array<std::string, 5> fields{};
std::size_t index = 0;
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| auto cert = hsm_ ? hsm_->getCertificate("") : std::optional<std::string>{}; | ||
| const std::string fallback_context = hsm_ ? hsm_->getTokenInfo() : std::string{}; | ||
| const std::string serial = derive_fallback_cert_serial(cert.value_or(""), fallback_context); | ||
|
|
||
| if (serial.empty()) { | ||
| THEMIS_WARN("HSMPKIClient::getCertSerial() could not derive a fallback serial."); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| THEMIS_INFO("HSMPKIClient::getCertSerial() derived deterministic fallback serial '{}'", serial); | ||
| return serial; |
| const auto dash = token.find('-'); | ||
| if (dash == std::string::npos) { | ||
| try { | ||
| if (std::stoi(token) == value) { | ||
| return true; | ||
| } | ||
| } catch (const std::exception&) { | ||
| return false; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const int start = std::stoi(token.substr(0, dash)); | ||
| const int end = std::stoi(token.substr(dash + 1)); | ||
| if (start <= end && start <= value && value <= end) { | ||
| return true; | ||
| } | ||
| } catch (const std::exception&) { | ||
| return false; | ||
| } | ||
| } |
| std::error_code ec; | ||
| std::filesystem::create_directories(backup_dir, ec); | ||
|
|
| std::this_thread::sleep_for(std::chrono::seconds(2)); | ||
|
|
||
| auto backups = backup_manager_->listBackups(backup_dir); | ||
| EXPECT_FALSE(backups.empty()) << "Scheduled backup should create a backup payload"; |
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
Pull request created by AI Agent