Make snapshots acyclic and align private namespaces - #8338
Open
Amaury Chamayou (achamayou) wants to merge 3 commits into
Open
Make snapshots acyclic and align private namespaces#8338Amaury Chamayou (achamayou) wants to merge 3 commits into
Amaury Chamayou (achamayou) wants to merge 3 commits into
Conversation
Break the snapshots-to-host and snapshots-to-consensus dependencies without changing public headers. Refs #3517. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
September 9, 2026 21:20
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new TimeBoundLogger unit tests modify global logger/default state with manual restoration that is not exception-safe, risking cross-test contamination.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors internal snapshot and logging utilities to break include cycles and enforce a cleaner source-component dependency boundary, without modifying any public headers under include/ccf.
Changes:
- Move
TimeBoundLoggerintoccf::dsand update host/snapshot consumers to use the new header/namespace. - Place private snapshot headers under
ccf::snapshots, switch snapshot writer index parameters toccf::SeqNo, and update call sites. - Add standalone-header compilation tests plus targeted regressions (time-bound logger behavior and full-width snapshot index handling), and enforce the new
snapshotsdependency policy.
Custom instructions used:
- Instruction files loaded: none
- Skill guidance applied:
testing
File summaries
| File | Description |
|---|---|
| src/snapshots/test/snapshot_writer_header_test.cpp | Standalone-include coverage for snapshots/snapshot_writer.h. |
| src/snapshots/test/filenames_header_test.cpp | Standalone-include coverage for snapshots/filenames.h. |
| src/snapshots/test/fetch_header_test.cpp | Standalone-include coverage for snapshots/fetch.h. |
| src/snapshots/snapshot_writer.h | Move to ccf::snapshots, switch indices to ccf::SeqNo, and use ccf::ds::TimeBoundLogger. |
| src/snapshots/filenames.h | Move to ccf::snapshots and switch to ccf::ds::TimeBoundLogger with explicit prerequisites. |
| src/snapshots/fetch.h | Move to ccf::snapshots and add missing JSON prerequisite for standalone compilation. |
| src/node/test/snapshotter.cpp | Update references to the new ccf::snapshots namespace. |
| src/host/test/ledger.cpp | Add regression test for full-width snapshot sequence numbers; update namespace usage. |
| src/host/run.cpp | Update include and configuration of TimeBoundLogger::default_max_time in ccf::ds. |
| src/host/lfs_file_handler.h | Update include and use of ccf::ds::TimeBoundLogger. |
| src/host/ledger.h | Update include and use of ccf::ds::TimeBoundLogger. |
| src/host/files_cleanup_timer.h | Update include and snapshot namespace usage; use ccf::ds::TimeBoundLogger. |
| src/ds/time_bound_logger.h | Move TimeBoundLogger into namespace ccf::ds. |
| src/ds/test/time_bound_logger_header_test.cpp | Standalone-include coverage for ds/time_bound_logger.h. |
| src/ds/test/logger.cpp | Add unit coverage for TimeBoundLogger formatting/default capture/log-level behavior. |
| scripts/source-dependencies.json | Enforce snapshots -> {ccf-api, ds, http, http_client} dependency boundary. |
| CMakeLists.txt | Wire new header tests and regressions into existing unit test targets. |
Review details
Suppressed comments (1)
src/ds/test/logger.cpp:107
- This test swaps out the global logger configuration/level and restores them manually; if any call between the exchange and the restore throws (eg allocation in make_unique/emplace_back), subsequent tests may run with the wrong logger state. Use an RAII guard to restore the previous configuration on all exit paths.
ccf::logger::config::loggers() = std::move(previous_loggers);
ccf::logger::config::level() = previous_level;
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Guard global logger sinks, log level, and timing defaults in the new tests. Cover restoration when an exception unwinds the guarded scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a3c18286-7019-45e6-bd42-2f9ef7e10c48
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.
Summary
Make
snapshotsacyclic without changing any public headers.TimeBoundLoggerfromhostintoccf::ds, updating all consumers and preserving the shared configurable logging threshold.ccf::snapshotsand use the existingccf::SeqNofor the writer's index parameters.snapshots -> {ccf-api, ds, http, http_client}. Add standalone-header compilation coverage, logging regressions, and full-width snapshot-index coverage.Snapshot formats and persistence behaviour are unchanged. No files under
include/ccfare modified.The measured source-component graph goes from 106 to 104 edges, 11 to 10 cyclic components, and 14 to 15 acyclic components, with 25 components overall.
Contributes to #3517; the full component DAG goal remains open.
Validation
Built with Clang 21.1.8 in WSL using the default
RelWithDebInfoconfiguration. Validation used an LF export of the staged tree so Unix scripts could run without rewriting the Windows worktree.js_generic,logging,curl_test, and the affected unit targets.logger_test,ledger_test,files_cleanup_test,indexing_test, andsnapshotter_test.recovery_stale_snapshot_join_test,recovery_intermediate_snapshot_join_test, andrecovery_snapshot_endorsements_test.scripts/ci-checks.sh; reran include and formatting checks after adding the explicit JSON prerequisite to the private fetch header.