Implement manifest-driven remote cloud backup transport - #6212
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>
Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
✅ Wave C Policy Gate: Edition & License ValidationResult: ✅ Edition & License Validation PASSED Checks
|
|
Target: pr Doxygen Governance Gate
Doxygen warnings
source_workflow: Gate: PR Doxygen Governance |
✅ Wave C Policy Gate: Hash & SBOM IntegrityResult: ✅ Hash & SBOM Integrity PASSED Checks
|
Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Implements manifest-driven remote cloud backup upload/restore (S3/GCS/Azure) and hardens several runtime “fail-closed vs fallback” behaviors across storage, server, and acceleration components.
Changes:
- Add manifest + payload-blob contract for remote cloud backup transport in
BackupManager(upload + restore) and relax hash verification for bootstrap reads. - Harden integrity-signature storage to require explicit in-memory fallback opt-in when RocksDB is absent.
- Extend server/GPU runtime contracts (gRPC-Web status capability signaling, real RoPE disablement, MCP stdio self-disable, break-even validator production wiring + test updates) and sync docs/roadmaps/changelogs.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_security_signature_rocksdb_iteration.cpp | Update tests to assert explicit fallback opt-in and fail-closed behavior when backend is null |
| tests/test_http_timeseries.cpp | Assert new source and degraded_mode fields returned by time-series metadata endpoints |
| tests/test_http_rope.cpp | Assert RoPE disable endpoint returns enabled=false and config fetch errors afterward |
| tests/test_grpc_web_proxy_handler.cpp | Assert new gRPC-Web status capability fields are present and correct across build modes |
| tests/test_grpc_web_proxy_bridge.cpp | Validate status behavior in non-gRPC builds when an override backend function is injected |
| tests/test_cloud_storage_backup_comprehensive.cpp | Broaden expected failure modes for real remote cloud transport (credentials/unavailable/not linked/etc.) |
| tests/stub_remediation_test.cpp | Assert time-series endpoints expose degraded/provider metadata fields |
| tests/security/test_security_signature_rocksdb_iteration.cpp | Same as non-namespaced variant (explicit fallback opt-in + fail-closed checks) |
| tests/network/test_http_timeseries.cpp | Same as non-namespaced variant for network test layout |
| tests/network/test_http_rope.cpp | Same as non-namespaced variant for network test layout |
| tests/legacy/grpc/test_grpc_web_proxy_handler.cpp | Same gRPC-Web status assertions for legacy test tree |
| tests/legacy/grpc/test_grpc_web_proxy_bridge.cpp | Same bridge/status assertions for legacy test tree |
| tests/legacy/cloud/test_cloud_storage_backup_comprehensive.cpp | Same cloud-backup expectation broadening for legacy test tree |
| tests/gpu/test_break_even_validation.cpp | Expand break-even tests for new reasons, injected profilers, metrics sink, and fail-closed validation |
| tests/gpu/CMakeLists.txt | Stop linking fallback shim and rely on production break-even implementation in tests |
| src/storage/security_signature_manager.cpp | Add explicit fallback option; fail closed without backend unless opted-in |
| src/storage/blob_backend_s3.cpp | Skip hash verification when reference hash is empty (bootstrap reads) |
| src/storage/blob_backend_azure.cpp | Skip hash verification when reference hash is empty (bootstrap reads) |
| src/storage/backup_manager.cpp | Implement manifest-driven remote upload/restore over blob backends; keep local mirror behavior |
| src/storage/ROADMAP.md | Document closure of remote transport + signature fallback gaps |
| src/storage/README.md | Document remote transport contract and signature persistence expectations |
| src/storage/PRODUCTION_REQUIREMENTS.md | Add MUST/MUST NOT requirements around signature backend and manifest transport |
| src/storage/CHANGELOG.md | Record fail-closed signature behavior + manifest-driven cloud backup transport |
| src/server/timeseries_api_handler.cpp | Add source, degraded_mode, and optional degraded_reason fields to metadata endpoints |
| src/server/rope_api_handler.cpp | Make RoPE DELETE endpoint actually disable RoPE at runtime |
| src/server/mcp_server.cpp | Make MCP stdio transport self-disable on unsupported platforms unless injected reader exists |
| src/server/grpc_web_proxy_handler.cpp | Expose explicit gRPC backend availability/mode in status endpoint |
| src/server/ROADMAP.md | Document completion of Wave 4-A server hardening batch |
| src/index/vector_index.cpp | Implement VectorIndexManager::disableRotaryEmbedding() |
| src/access_model/ROADMAP.md | Sync roadmap status/known-issues text to reflect completed Phase 5–6 work |
| src/acceleration/oneapi_backend.cpp | Fail closed on USM allocation failure before device memcpy |
| src/acceleration/break_even_validator.cc | Replace placeholder timings with hookable profilers + deterministic estimators; add metrics sink |
| src/acceleration/ROADMAP.md | Record validation date and completion notes for break-even + oneAPI hardening |
| src/acceleration/MODULE_GAPS.md | Update gap notes reflecting replacement of prior break-even TODO |
| src/acceleration/CHANGELOG.md | Document break-even production wiring + oneAPI OOM hardening changes |
| include/storage/security_signature_manager.h | Add Options + result fields (backend_available, etc.) and introspection helpers |
| include/storage/backup_manager.h | Document manifest-driven remote transport and update Result-based APIs |
| include/server/mcp_server.h | Add StdioTransport::isRunning() |
| include/index/vector_index.h | Declare disableRotaryEmbedding() API |
| include/acceleration/break_even_validator.h | Add profiling hooks + metrics sink APIs and supporting helpers |
| cmake/ModularBuild.cmake | Compile break_even_validator.cc in modular build sources |
| cmake/CMakeLists.txt | Compile break_even_validator.cc in core sources |
| ROADMAP.md | Update global roadmap status summaries for server/storage/GPU/access_model |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| std::string actual_hash = computeSHA256(data); | ||
| if (actual_hash != ref.hash_sha256) { | ||
| if (!ref.hash_sha256.empty() && actual_hash != ref.hash_sha256) { | ||
| THEMIS_ERROR("Hash mismatch for blob {}: expected={}, actual={}", | ||
| ref.id, ref.hash_sha256, actual_hash); |
There was a problem hiding this comment.
Applied: SHA-256 is now computed only when ref.hash_sha256 is present (d785092).
| std::string actual_hash = computeSHA256(data); | ||
| if (actual_hash != ref.hash_sha256) { | ||
| if (!ref.hash_sha256.empty() && actual_hash != ref.hash_sha256) { | ||
| THEMIS_ERROR("Hash mismatch for blob {}: expected={}, actual={}", | ||
| ref.id, ref.hash_sha256, actual_hash); |
There was a problem hiding this comment.
Applied: Azure blob hash computation is now guarded by !ref.hash_sha256.empty() (d785092).
| if (!manifest_json.contains("format_version") || | ||
| manifest_json["format_version"].get<std::string>() != kRemoteBackupFormatVersion) { | ||
| return ErrVoid(errors::ErrorCode::ERR_BACKUP_MANIFEST_CORRUPT, | ||
| "Remote backup manifest has unsupported format version"); | ||
| } | ||
| if (!manifest_json.contains("entries") || !manifest_json["entries"].is_array()) { | ||
| return ErrVoid(errors::ErrorCode::ERR_BACKUP_MANIFEST_CORRUPT, | ||
| "Remote backup manifest is missing entries"); | ||
| } | ||
|
|
||
| const fs::path restore_root = fs::path(local_path); | ||
| for (const auto& entry : manifest_json["entries"]) { | ||
| if (!entry.contains("kind") || !entry.contains("relative_path")) { | ||
| return ErrVoid(errors::ErrorCode::ERR_BACKUP_MANIFEST_CORRUPT, | ||
| "Remote backup manifest entry is incomplete"); | ||
| } | ||
|
|
||
| const std::string kind = entry["kind"].get<std::string>(); | ||
| const fs::path relative_path(entry["relative_path"].get<std::string>()); | ||
| if (!isSafeRelativeBackupPath(relative_path)) { | ||
| return ErrVoid(errors::ErrorCode::ERR_BACKUP_INVALID_TYPE, | ||
| "Remote backup manifest contains unsafe path: " + | ||
| relative_path.generic_string()); | ||
| } | ||
|
|
||
| const fs::path target_path = restore_root / relative_path; | ||
| if (kind == "directory") { | ||
| std::error_code ec; | ||
| fs::create_directories(target_path, ec); | ||
| if (ec) { | ||
| return ErrVoid(errors::ErrorCode::ERR_UTIL_FILE_OPERATION_FAILED, | ||
| "Failed to create restore directory '" + target_path.string() + | ||
| "': " + ec.message()); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (kind != "file" || !entry.contains("blob_id")) { | ||
| return ErrVoid(errors::ErrorCode::ERR_BACKUP_MANIFEST_CORRUPT, | ||
| "Remote backup manifest file entry is incomplete"); | ||
| } | ||
|
|
||
| storage::BlobRef payload_ref; | ||
| payload_ref.id = entry["blob_id"].get<std::string>(); | ||
| payload_ref.type = storage::BlobStorageType::CUSTOM; | ||
| payload_ref.uri = cloud_path; | ||
| payload_ref.size_bytes = entry.value("size_bytes", 0); | ||
| payload_ref.hash_sha256 = entry.value("hash_sha256", std::string{}); | ||
| auto payload_result = backend_impl->get(payload_ref); | ||
| if (!payload_result.has_value()) { | ||
| return ErrVoid(payload_result.error().code(), payload_result.error().message()); | ||
| } | ||
|
|
||
| auto write_result = writeBinaryFileBytes(target_path, payload_result.value()); | ||
| if (!write_result.has_value()) { | ||
| return write_result; | ||
| } |
There was a problem hiding this comment.
Applied: manifest validation/iteration in downloadFromCloud() is wrapped with exception handling returning ERR_BACKUP_MANIFEST_CORRUPT (d785092).
| if (segments.size() < 2) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| RemoteBackupLocation location; | ||
| if (segments.size() >= 3) { | ||
| location.authority = segments[0]; | ||
| location.container = segments[1]; | ||
| location.prefix = trimSlashes(joinPathSegments(segments, 2)); | ||
| } else { | ||
| location.container = segments[0]; | ||
| location.prefix = trimSlashes(joinPathSegments(segments, 1)); | ||
| } |
There was a problem hiding this comment.
Applied: API docs now explicitly document both Azure URI forms (azure://account/container/path and azure://container/path) and account derivation behavior (d785092).
| case StorageBackend::S3: | ||
| #if defined(THEMIS_HAS_AWS_SDK) && THEMIS_HAS_AWS_SDK | ||
| return std::make_shared<storage::S3BlobBackend>( | ||
| location.authority, | ||
| [&config]() { | ||
| const auto it = config.find("region"); | ||
| return it == config.end() || it->second.empty() ? std::string("us-east-1") | ||
| : it->second; | ||
| }(), | ||
| location.prefix); | ||
| #else | ||
| return {}; | ||
| #endif | ||
| case StorageBackend::AZURE: | ||
| #if defined(THEMIS_HAS_AZURE_STORAGE) && THEMIS_HAS_AZURE_STORAGE |
There was a problem hiding this comment.
Applied: provider availability checks/factory wiring are standardized on THEMIS_HAS_* flags across BackupManager remote transport code (d785092).
| Result<std::vector<uint8_t>> readBinaryFileBytes(const fs::path& file_path) { | ||
| std::ifstream input(file_path, std::ios::binary); | ||
| if (!input) { | ||
| return Err<std::vector<uint8_t>>( | ||
| errors::ErrorCode::ERR_STORAGE_FILE_NOT_FOUND, | ||
| "Failed to open file: " + file_path.string()); | ||
| } | ||
|
|
||
| std::vector<uint8_t> data((std::istreambuf_iterator<char>(input)), | ||
| std::istreambuf_iterator<char>()); | ||
| return Ok(std::move(data)); | ||
| } |
There was a problem hiding this comment.
Applied: remote transport now enforces a per-object in-memory payload limit (256 MiB) for upload/download paths with clear errors, and the contract is documented in the header (d785092).
| | core | 9 listed | Mostly DOC / evidence gaps | Runtime adapter registry and plugin loading are delivered; remaining items are Wave D operability and refreshed evidence | | ||
| | base | 8 listed | Mostly historical scanner noise | `src/base/MODULE_GAPS.md` re-scan shows 0 actionable current gaps; remaining items are documented false positives or follow-up docs | | ||
| | server | 4 residual source gaps | REAL IMPL gaps | gRPC-Web proxy is still UNIMPLEMENTED-only in fallback builds; time-series aggregate/retention providers and RoPE metrics still rely on degraded fallback paths; non-Linux MCP stdio transport remains unsupported | | ||
| | server | 1 residual source gap | MOSTLY REMEDIATED | gRPC-Web fallback builds now advertise an explicit fail-closed capability contract, RoPE DELETE now disables runtime config, and unsupported MCP stdio self-disables; remaining work is first-class time-series provider DI beyond degraded metadata signaling | |
| | GPU/CUDA | 21+53 listed | REAL IMPL gaps | Break-even routing now uses production build wiring plus explicit CPU/GPU profiling contracts, but CUDA/HIP kernel parity, unchecked-kernel-call closure, and representative-hardware validation remain open release blockers | | ||
| | storage | 11 real gaps after 2026-08-31 revalidation | MIXED | Backup restore fail-closed hardening, ggml bridge runtime wiring, `SecuritySignatureManager` null-backend fail-closed behavior, and remote S3/GCS/Azure manifest transport are in place; biggest remaining gaps are long-run validation evidence and cloud-backend hardening follow-up | | ||
| | access_model | roadmap contradiction | Mostly DOC drift | Source and module evidence show Phase 5-6 observability, e2e/concurrency tests, and GATE-ACM-01..06 are complete; stale checklist/known-issues text must stay synchronized | |
|
@copilot fix all |
✅ Wave C Policy Gate: Community Fail-Closed ValidationResult: ✅ Community Fail-Closed Validation PASSED Checks
|
✅ Wave C Policy Gate: Private Plugin Boundary EnforcementResult: ✅ Private Plugin Boundary Enforcement PASSED Checks
|
- Add @return to Status::Error, rebuildFromStorage, saveIndex, loadIndex - Add @return to all removeByPk overloads (direct, WriteBatch, TransactionWrapper) - Add @PARAM and @return to AttributeFilterV2::Equals, Range, In - Upgrade removeBatch from plain /// to full @brief/@param/@return doc - Add @brief and @return to computeCentroid and computeVariance - Add full @brief/@param/@return docs to addEntityWithRotation and addEntityWithRelationalRotation Scanner now reports 0 blocking structural findings (was 26 remaining after prior fixes) Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
…out .gitmodules entry Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
…allel_downloader.h and storage_interface.h Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
- mcp_server.h: add @Class tag, document constructors, destructor, isRunning, all undocumented public methods (start/stop/register*/ unregister*/attach*/get*Transport/handleRequest) - break_even_validator.h: replace unknown @thread tag with @note Thread safety: across all affected methods - backup_manager.h: remove @enum tags that caused 'Documentation for undefined enum' warnings (bare names inside namespace); Doxygen auto-associates adjacent doc comments with enum definitions - security_signature_manager.h: split shared doc block into per- constructor docs so @PARAM options is only on the two-arg ctor Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
…modules entry The path was registered in the git index as a submodule commit (mode 160000) but had no corresponding entry in .gitmodules, causing 'git submodule foreach' to exit with code 128 and failing the copilot-setup-steps CI job. Co-authored-by: makr-code <150588092+makr-code@users.noreply.github.com>
Pull Request
Target Version (Required)
Target Version: [Unreleased]
Runtime hardening for an open storage gap; not a contract break and best tracked in the next unreleased train.
Description
backup_manager.cppstill treated S3/GCS/Azure as effectively local-mirror-only, leaving the largest remaining real storage gap in cloud backup restore. This change wires real remote transport through the existing blob backends using a manifest + payload-object contract.Remote transport
BackupManager::uploadToCloud()now uploads a manifest plus per-file payload blobs for S3/GCS/Azure.BackupManager::downloadFromCloud()now restores from that manifest instead of requiring provider-side listing or a local mirror path.file:/// absolute-path mirror behavior stays unchanged.Manifest contract
Backend compatibility
Tests and docs
credentials, unavailable backend, not linked, not found, unsupported) instead of stub-only expectations.Example manifest shape:
{ "format_version": "1", "source_type": "directory", "entries": [ { "kind": "directory", "relative_path": "meta" }, { "kind": "file", "relative_path": "meta/backup.json", "blob_id": "payload/meta/backup.json", "size_bytes": 1024, "hash_sha256": "..." } ] }Linked Issues
N/A
Type of Change
Breaking Change Checklist
VERSIONandCMakeLists.txtdocs/migration/### Removed/### Changedsection updatedTesting
Security Tiering Impact (Required for Runtime Changes)
Impacted tier(s):
Trust-boundary crossings documented in PR description (example: T3 -> T2, T5 -> T4 brokered call)
Boundary controls validated for affected T3/T4/T5 paths (AuthN/AuthZ, validation, rate limits, audit)
Boundary-focused tests added/updated or explicit N/A rationale provided
If trust level/privilege increased, security maintainer approval is attached
📚 Research & Knowledge (wenn applicable)
/docs/research/angelegt?/docs/research/implementation_influence/eingetragen?Relevante Quellen:
AI-Generated Code (KI-generierter Code)
GetSymbolReferences_CppToolsgeprüft (siehe.github/instructions/cpp-language-service-tools.instructions.md)new/deleteohne explizites Review eingeführtAI Review Workflow (Required for AI-assisted PRs)
.github/prompts/pr-diff-findings-review.prompt.md.github/prompts/security-hardening-review.prompt.md(or N/A documented).github/prompts/api-change-impact-review.prompt.md(or N/A documented).github/copilot/REVIEW_SEVERITY_POLICY.mdHigh-Finding Exception Record (only if High is accepted)
High-finding exception claimed in this PR
Finding reference:
Maintainer approver:
Mitigation in current release:
Target fix milestone:
Tracking issue:
Validation evidence:
Release Readiness Gate (Required for release-scoped changes)
.github/prompts/release-readiness-check.prompt.mdfor branch transition scopeBRANCHING_STRATEGY.mdandRELEASE_STRATEGY.mdVERSIONING.mdandCHANGELOG.mdChecklist
[Unreleased]Scanner and IntelliSense Gates
criticalfindings in categoriessecurity,input_validation,query_correctness,distributed_consistency,concurrency,memoryhighfindings in the same categories (or explicitly approved)unknownscanner findings triaged (fixed, re-categorized, or justified)