Repository navigation
Remove ringbuffer dependency from independent periodic tasks - #8445
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical public API incompatibility and periodic-task worker-pool contention remain unresolved.
Review effort: Balanced
Findings: 1
What changed in this PR
Decouples independent periodic maintenance from the aggregate ringbuffer tick. The diff also includes broader Rust SDK, public API, KV, crypto, formal-model, and CI changes.
Changes:
- Adds host-driven task timing and owner-managed frontend and historical-cache maintenance.
- Refactors component dependencies, configuration interfaces, KV behavior, and crypto integration.
- Adds Rust application support and updates tests, formal proofs, documentation, and CI.
Review findings:
- Critical (3 votes) —
include/ccf/node/node_configuration_interface.h:16: Changingnode_configfromStartupConfigtoCCFConfigbreaks existing consumers. Preserve compatibility or document the migration and correct the compatibility claim. - Nit (1 vote) —
src/rust/ccf-app/Cargo.toml:2: Rust SDK and other public API changes exceed the described scope. Split them out or update the description and compatibility implications. - Critical (1 vote) —
src/tasks/periodic_task_owner.h:51: Overlapping maintenance executions can exhaust workers waiting on a mutex. Use a non-blocking guard to skip overlaps while retaining elapsed-time accounting.
| File | Description |
|---|---|
tla/consensus/abs.tla |
Corrects the minimum-term assumption. |
tests/tls_groups.py |
Checks negotiated-group logging. |
tests/reconfiguration.py |
Removes unavailable Azure test cases. |
tests/infra/network.py |
Retries uncommitted node removals. |
tests/infra/locust_benchmark_support.py |
Publishes the master's assigned port. |
tests/governance.py |
Tests file-backed data and join retries. |
tests/ci-buckets.txt |
Routes the Rust application test. |
tests/ccfapp/ccfapp.cpp |
Exercises exported third-party headers. |
src/tasks/periodic_task_owner.h |
Adds lifetime-managed periodic callbacks. |
src/tasks/job_board.h |
Exposes task-clock time. |
src/tasks/job_board.cpp |
Implements synchronized clock reads. |
src/service/tables/signatures.h |
Uses shared internal table names. |
src/service/tables/shares.h |
Uses the shared ledger-secret name. |
src/service/tables/config.h |
Removes relocated JSON declarations. |
src/rust/src/lib.rs |
Removes the COSE crate export. |
src/rust/ccf-app/Cargo.toml |
Defines the Rust application SDK crate. |
src/rust/ccf-app/Cargo.lock |
Locks SDK dependencies. |
src/rust/Cargo.toml |
Removes the COSE dependency. |
src/rust/Cargo.lock |
Updates locked dependencies. |
src/pal/uvm_endorsements.h |
Reduces node-layer dependencies. |
src/pal/uvm_endorsements.cpp |
Uses the relocated PAL header. |
src/pal/test/verify_uvm_attestation_and_endorsements.h |
Updates relocated includes. |
src/pal/test/verify_uvm_attestation_and_endorsements.cpp |
Updates the endorsements include. |
src/node/tx_receipt_impl.h |
Includes relocated claims helpers. |
src/node/test/snapshotter.cpp |
Updates the null-encryptor include. |
src/node/test/snapshot.cpp |
Updates claims and encryptor includes. |
src/node/test/open_service.cpp |
Tests recovered-secret opening versions. |
src/node/test/node_info_json.cpp |
Includes address helpers explicitly. |
src/node/test/ledger_secrets.cpp |
Updates the null-encryptor include. |
src/node/test/jwt_key_auto_refresh.cpp |
Updates the null-encryptor include. |
src/node/test/history.cpp |
Updates the null-encryptor include. |
src/node/test/env.cpp |
Uses the relocated environment header. |
src/node/test/endorsements.cpp |
Uses the PAL endorsements header. |
src/node/rpc/test/frontend_test_infra.h |
Updates the null-encryptor include. |
src/node/rpc/serialization.h |
Removes relocated response serialization. |
src/node/rpc/rpc_handler.h |
Adds periodic-tick initialization. |
src/node/rpc/node_operation_interface.h |
Updates the COSE configuration include. |
src/node/rpc/node_interface.h |
Separates configuration and node data. |
src/node/rpc/node_frontend.h |
Updates node-data and recovery handling. |
src/node/rpc/node_call_types.h |
Defines independent genesis data. |
src/node/rpc/network_identity_subsystem.h |
Updates the configuration include. |
src/node/rpc/network_identity_accessors_impl.h |
Extracts identity snapshot reading. |
src/node/rpc/member_frontend.h |
Updates component dependencies. |
src/node/rpc/http_rpc_context.h |
Uses the relocated RPC context. |
src/node/rpc/frontend.h |
Adds periodic ownership and structured errors. |
src/node/rpc/call_types.h |
Removes relocated common response types. |
src/node/recovery_snapshot_ledger.h |
Updates the configuration include. |
src/node/recovery_decision_protocol.h |
Updates the configuration include. |
src/node/quote.cpp |
Uses PAL endorsements. |
src/node/open_service.h |
Adds recovered-secret version adjustment. |
src/node/node_to_node.h |
Implements the consensus channel interface. |
src/node/node_configuration_subsystem.h |
Publishes node data separately. |
src/node/identity.h |
Updates the COSE configuration include. |
src/node/hooks.h |
Updates the reconfiguration include. |
src/node/historical_queries.h |
Adds owner-managed cache maintenance. |
src/node/historical_queries_adapter.cpp |
Hosts committed-receipt construction. |
src/node/gov/handlers/helpers.h |
Uses relocated governance logging. |
src/node/gov/handlers/acks.h |
Extracts acknowledgement handler functions. |
src/node/gov/extensions/node.h |
Declares the relocated node extension. |
src/node/gov/extensions/node.cpp |
Updates extension and logging includes. |
src/node/gov/extensions/network.h |
Declares the relocated network extension. |
src/node/gov/extensions/network.cpp |
Updates the extension include. |
src/node/env.h |
Declares environment expansion helpers. |
src/node/env.cpp |
Uses the relocated helper header. |
src/node/commit_callback_subsystem.h |
Implements commit observation. |
src/kv/version_v.h |
Removes per-value read-version metadata. |
src/kv/untyped_map_handle.cpp |
Simplifies read dependencies. |
src/kv/untyped_change_set.h |
Simplifies read-set representation. |
src/kv/tx.cpp |
Simplifies map construction. |
src/kv/test/null_tx_history.h |
Updates the configuration include. |
src/kv/test/kv_serialisation.cpp |
Updates the null-encryptor include. |
src/kv/test/kv_dynamic_tables.cpp |
Updates the null-encryptor include. |
src/kv/test/kv_contention.cpp |
Updates the null-encryptor include. |
src/kv/test/kv_bench.cpp |
Adds a transaction-diff benchmark. |
src/kv/snapshot.h |
Includes claims helpers explicitly. |
src/kv/README.md |
Documents dependency-policy enforcement. |
src/kv/null_encryptor.h |
Relocates the null encryptor. |
src/kv/kv_types.h |
Simplifies internal interfaces and dependencies. |
src/kv/internal_table_names.h |
Centralizes KV-inspected table names. |
src/kv/generic_serialise_wrapper.h |
Uses relocated claims helpers. |
src/kv/deserialise.h |
Simplifies deserialization and dependencies. |
src/kv/compacted_version_conflict.h |
Includes utility support explicitly. |
src/kv/committable_tx.h |
Updates version allocation and reserved writes. |
src/kv/claims.h |
Makes claims helpers independently includable. |
src/kv/apply_changes.h |
Simplifies version resolution. |
src/js/registry.cpp |
Uses the relocated RPC context. |
src/js/extensions/snp_attestation.cpp |
Uses PAL endorsements. |
src/js/extensions/console.cpp |
Uses relocated governance logging. |
src/js/extensions/ccf/crypto.cpp |
Drains certificate parsing errors. |
src/js/extensions/ccf/converters.cpp |
Replaces node-dependent includes. |
src/indexing/test/indexing.cpp |
Updates asynchronous ledger-request helpers. |
src/indexing/test/common.h |
Updates historical-store construction. |
src/host/task_ticker.h |
Advances the task clock independently. |
src/endpoints/endpoint_registry.cpp |
Removes relocated receipt construction. |
src/endpoints/common_endpoint_registry.cpp |
Defines common response types locally. |
src/endpoints/authentication/cose_auth.cpp |
Uses crypto-layer COSE helpers. |
src/enclave/main.cpp |
Passes maintenance timing into initialization. |
src/enclave/entry_points.h |
Updates configuration interfaces. |
src/enclave/enclave.h |
Starts independent maintenance tasks. |
src/ds/notifying.h |
Removes unused outbound notifications. |
src/ds/gov_logging.h |
Centralizes governance logging macros. |
src/crypto/test/mldsa.cpp |
Checks error-queue cleanup. |
src/crypto/openssl/verifier.h |
Removes an unused declaration. |
src/crypto/openssl/rsa_public_key.h |
Adds an ownership-transfer constructor. |
src/crypto/openssl/rsa_public_key.cpp |
Updates ownership and error handling. |
src/crypto/openssl/rsa_key_pair.cpp |
Uses shared parse-error checks. |
src/crypto/openssl/mldsa_public_key.cpp |
Adds drained error diagnostics. |
src/crypto/openssl/mldsa_key_pair.cpp |
Adds drained parse-error diagnostics. |
src/crypto/openssl/eddsa_public_key.cpp |
Cleans up verification errors. |
src/crypto/openssl/eddsa_key_pair.cpp |
Uses shared parse-error checks. |
src/crypto/openssl/ec_public_key.h |
Documents transferred key ownership. |
src/crypto/openssl/ec_public_key.cpp |
Adopts generated keys directly. |
src/crypto/openssl/ec_key_pair.cpp |
Queries signature size and updates ownership. |
src/crypto/openssl/cose_verifier.h |
Uses native COSE keys. |
src/crypto/openssl/base64.h |
Drains OpenSSL diagnostics. |
src/crypto/key_exchange.h |
Transfers peer-key ownership. |
src/cose/cose_rs/Cargo.toml |
Removes the old COSE crate manifest. |
src/cose/cose_rs/Cargo.lock |
Removes its lockfile. |
src/consensus/consensus_types.h |
Removes relocated configuration serialization. |
src/consensus/aft/test/view_straddling_common.h |
Updates the null-encryptor include. |
src/consensus/aft/raft_types.h |
Removes an RPC dependency. |
src/consensus/aft/consensus_channels.h |
Defines authenticated consensus channels. |
src/consensus/aft/commit_observer.h |
Defines commit notification. |
src/common/enclave_interface_types.h |
Removes relocated startup types. |
scripts/source-dependencies.json |
Expands component dependency policies. |
scripts/requirements.txt |
Updates dependency constraints. |
scripts/headers-are-included.sh |
Excludes compatibility headers. |
scripts/check-source-dependencies.py |
Requires complete, acyclic policies. |
samples/CMakeLists.txt |
Includes the Rust sample. |
samples/apps/logging/logging.cpp |
Migrates node-data access. |
samples/apps/basic_rust/rust-toolchain.toml |
Pins the sample toolchain. |
samples/apps/basic_rust/CMakeLists.txt |
Registers the Rust application. |
samples/apps/basic_rust/Cargo.toml |
Defines sample dependencies. |
samples/apps/basic_rust/Cargo.lock |
Locks sample dependencies. |
README.md |
Scopes the CI badge to main. |
python/tests/test_merkletree.py |
Tests partial trees and boundary cases. |
python/src/ccf/merkletree.py |
Rejects unsupported empty-tree states. |
lean/disaster-recovery/lakefile.toml |
Updates the canonical-check entrypoint. |
lean/disaster-recovery/DisasterRecovery/Tests/RaftFreshness.lean |
Checks freshness and strict majorities. |
lean/disaster-recovery/DisasterRecovery/Tests/ProofCoverage.lean |
Checks theorem and witness coverage. |
lean/disaster-recovery/DisasterRecovery/Tests/Initial.lean |
Defines test initial states. |
lean/disaster-recovery/DisasterRecovery/Tests/Execution.lean |
Tests trace validity. |
lean/disaster-recovery/DisasterRecovery/Shared/TransitionSystem.lean |
Defines reusable reachability. |
lean/disaster-recovery/DisasterRecovery/Shared/Capabilities.lean |
Defines recorded protocol outputs. |
lean/disaster-recovery/DisasterRecovery/Protocol/Invariants.lean |
Removes relocated invariants. |
lean/disaster-recovery/DisasterRecovery/Protocol/Committed.lean |
Removes relocated commit assumptions. |
lean/disaster-recovery/DisasterRecovery/Properties/Utils.lean |
Defines model-facing property helpers. |
lean/disaster-recovery/DisasterRecovery/Proofs/Predicates.lean |
Collects proof-only predicates. |
lean/disaster-recovery/DisasterRecovery/Proofs/History.lean |
Links model traces to decorated executions. |
lean/disaster-recovery/DisasterRecovery/Proofs/Execution.lean |
Separates proof-only execution histories. |
lean/disaster-recovery/DisasterRecovery/Proof.lean |
Exports property theorems. |
lean/disaster-recovery/DisasterRecovery/Model.lean |
Composes the executable network model. |
lean/disaster-recovery/DisasterRecovery.lean |
Updates package-wide imports. |
lean/AGENT.md |
Documents model and proof separation. |
include/ccf/service/service_config.h |
Exposes configuration serialization. |
include/ccf/service/reconfiguration_type.h |
Retains a compatibility shim. |
include/ccf/service/node_info_network.h |
Uses extracted address helpers. |
include/ccf/service/consensus_config.h |
Exposes consensus serialization. |
include/ccf/reconfiguration_type.h |
Declares the relocated enum. |
include/ccf/node/start_type.h |
Exposes startup types. |
include/ccf/node/node_configuration_interface.h |
Changes public configuration access. |
include/ccf/node/cose_signatures_config.h |
Retains a compatibility shim. |
include/ccf/kv/untyped_map_diff.h |
Restricts diff construction. |
include/ccf/kv/map_diff.h |
Corrects deletion and iteration handling. |
include/ccf/ds/net_address.h |
Extracts address parsing helpers. |
include/ccf/ds/locking.h |
Adds annotated shared-mutex guards. |
include/ccf/crypto/verifier.h |
Clarifies expired-certificate results. |
include/ccf/crypto/openssl/openssl_wrappers.h |
Centralizes error-queue draining. |
include/ccf/crypto/cose_verifier.h |
Adds native COSE-key verification. |
include/ccf/cose_signatures_config.h |
Declares relocated configuration. |
include/ccf/cose_signatures_config_interface.h |
Updates the configuration include. |
doc/operations/platforms/snp.rst |
Documents recovery TCB policy. |
doc/contribute/build_ccf.rst |
Documents Rust API generation. |
doc/conf.py |
Enables Rust documentation support. |
doc/build_apps/rust_api.rst |
Links the experimental SDK reference. |
doc/build_apps/logging.rst |
Warns about host-visible Rust output. |
doc/build_apps/kv/index.rst |
Clarifies Rust KV limitations. |
doc/build_apps/index.rst |
Adds Rust development navigation. |
doc/build_apps/get_started.rst |
Introduces experimental Rust applications. |
doc/build_apps/crypto.rst |
Documents COSE keys. |
doc/build_apps/build_app.rst |
Documents native executable builds. |
doc/build_apps/auth/index.rst |
Clarifies Rust authentication limitations. |
cmake/verify_ccf_rs_exports.cmake |
Checks Rust C ABI exports. |
cmake/tools.cmake |
Propagates object-library coverage linkage. |
cmake/gersemi_definitions.cmake |
Declares Rust helper formatting metadata. |
cmake/crypto.cmake |
Includes COSE-key implementation. |
cmake/ccf_rs.cmake |
Isolates Rust runtime symbols. |
cmake/ccf_app.cmake |
Adds Rust application builds. |
3rdparty/internal/cose-openssl/src/lib.rs |
Removes the old crate entrypoint. |
3rdparty/internal/cose-openssl/Cargo.toml |
Removes the old crate manifest. |
3rdparty/internal/cose-openssl/.gitignore |
Removes obsolete crate exclusions. |
.github/workflows/release.yml |
Checks exported headers and collects ledgers. |
.github/workflows/README.md |
Documents ownership and coverage workflows. |
.github/workflows/long-test.yml |
Collects additional ledger directories. |
.github/workflows/cross-platform-lts.yml |
Restores workspace ownership. |
.github/workflows/codeql-analysis.yml |
Updates pinned CodeQL actions. |
.github/workflows/ci.yml |
Collects additional ledger directories. |
.github/workflows/ci-al4.yml |
Collects additional ledger directories. |
.github/workflows/bencher.yml |
Collects additional ledger directories. |
.github/skills/testing/SKILL.md |
Documents cross-build coverage merging. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Amaury Chamayou (achamayou)
left a comment
There was a problem hiding this comment.
Reviewed with a focus on the concurrency and correctness implications of moving StateCache::tick and RpcFrontend::tick from the enclave main thread onto job-board worker threads.
Overall: the concurrency model looks sound. One merge artefact in ~Enclave() should be removed before merging (see inline); the rest are minor notes and one question.
Paths traced (all OK):
- Owner lifetime:
weak_self.lock()holds a strong reference for the whole offunction, sotick()can never observe a partially destroyed frontend/cache, and thetry_to_lockguard is released beforeowner, so an owner being destroyed on a worker is safe. In production theEnclaveis never destroyed, so the owners outlive the job board anyway. - Overlap coalescing:
try_to_lockprevents the worker-pool exhaustion flagged earlier, andlast_run = nowcarries skipped time forward so noelapsedis lost. Concurrentweak_ptr::lock()and conststd::functioninvocation from two queued copies of the same task is well-defined. - Lock ordering: the task body takes
timing->lockthentasks_mutex(viaget_current_time);JobBoard::tickholdstasks_mutexand releases it beforeadd_task.StateCacheImpl::tickruns entirely underrequests_lock, thenLedgerSubsystem::get_rangetakes the board mutex, and result callbacks takerequests_lockon workers; no cycle (pre-existing pattern). enable_shared_from_thiswiring:register_frontend(shared_ptr<RpcHandler>)frommake_unique<...Frontend>andmake_shared<StateCache>both initialiseweak_this; there is no competingenable_shared_from_thisbase in either hierarchy, and the earlyexpired()throw catches misuse.- Shutdown: the uv timer cannot fire after
uv_runreturns and~proxy_ptrcloses it;JobBoard::shutdown()discards queued copies; the hostledger_subsystem.shutdown()runs after the enclave threads join, so no historical tick can race the lane drain. - Exceptions:
try_do_taskaborts on throw; previously the dispatcher rethrew, which was also fatal. Equivalent.
Question: every tick_interval (10ms by default) now enqueues four tasks (three frontends + the cache), each with a mutex/condvar wake, where there used to be four inline calls. Fine for now, but was a single aggregate periodic task considered?
Validation: built task_system_test with clang 21 under TSAN (-Wthread-safety -Werror): 24 cases / 661 assertions pass; the new coalescing test passed 30x unpinned and 40x pinned to a single busy CPU alongside 4 spinning competitors, with no TSAN reports. Since this moves work onto worker threads and the TSAN/ASAN/long-e2e jobs were skipped, I have added the run-long-test label so they run before merge.
Custom instructions used: .github/copilot-instructions.md, .github/instructions/reviewing.instructions.md.
|
On the aggregate-task question: yes, this was considered—the old ringbuffer tick was exactly an aggregate fan-out, and a single job-board task would be the closest replacement. I kept the tasks separate deliberately because aggregation would retain cross-subsystem coupling: a slow historical-cache tick would delay all frontend ticks, one callback would need to coordinate several owners/lifetimes, and future period changes would return to a central dispatch point. The current cost is at most four submissions per task-clock tick (often fewer because deadlines can require a second host tick). I have not benchmarked that overhead. If it proves material, I would prefer optimising JobBoard’s batch enqueue/wakeup path so independently owned tasks remain independent, rather than recreating an aggregate subsystem tick. I think the current trade-off is acceptable for this removal PR, but this is worth measuring. |
Drive the task clock from a host-side libuv timer while retaining the existing tick semantics and configured interval. Let historical state caches and RPC frontends own their periodic task lifetimes, and remove the unused consensus periodic_end hook. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the StubLedgerReader introduced by the ledger ringbuffer removal base instead of the removed ringbuffer StubWriter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Satisfy the clang-tidy named-parameter check for the default no-op RPC handler implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Satisfy clang-tidy's member-initialization check in the shared periodic task owner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Clarify that JobBoard may enqueue on later ticks, while PeriodicTaskOwner skips concurrent executions and carries their elapsed time into the next successful run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document worker-thread execution and timer granularity, state the initialization-only registration contract, and make the overlap regression robust on loaded runners. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2bd5e38 to
b81b427
Compare
Resolve the test insertion conflict in src/node/test/historical_queries.cpp by keeping both the maintenance regression tests and the new periodic tick test from #8445. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Motivation
Following #8405 as the next step of the ringbuffer-removal plan. Delayed and periodic tasks, historical-state cache maintenance, and frontend endpoint maintenance currently depend on the aggregate
AdminMessage::tickringbuffer message even though they do not need the node/consensus tick's serialization.Consensus, node-to-node channel maintenance, indexing, and the aggregate tick message remain in place for the later ordering-sensitive work.
Implementation summary
tick_interval. Its callback only makes due tasks runnable; task bodies still execute on task workers.PeriodicTaskOwnerfor weak-owner lifetime, cancellation, elapsed-time accounting, and non-blocking coalescing of overlapping executions.periodic_end()/tick_end()hook.ccf::tasks::tick()API, recurrence behavior, cancellation behavior, configured period, and deterministic fake-time tests.Safety and compatibility
requests_lock; frontend ticking retains its atomic open check. Periodic callbacks hold owners weakly, are cancelled on destruction, and skip overlap without consuming additional workers; elapsed time is carried into the next successful execution.EndpointRegistry::tick()overrides now run on task-system workers instead of the enclave main thread. A registry's ticks do not overlap, but may run concurrently with endpoint execution and ticks for other registries. This is documented in the public API and changelog.RpcFrontend::tick()checksis_open_, and current production registry ticks are no-ops. The guard originally protected signature emission, which moved toTxHistoryin 2020; history already starts its signature timer only after public recovery.tick_intervalmay therefore fire every one or two host ticks when a callback arrives marginally early, but elapsed time is carried forward so expiry/retry accounting is not lost.Validation:
task_system_test,historical_queries_test,indexing_test,frontend_test; fully linkedjs_generic; affected Debug clang-tidy targets; C++/CMake formatting, ASCII, copyright, TODO, and include-policy checks.