Add ReportEvent snapshot URI versioning

[charpty]

Summary

• Add the PR230-compatible ReportEvent snapshot proto shape (EVENT_BLOCK_SNAPSHOT and BlockSnapshotEventParams).
• Implement snapshot full-cover semantics with per instance_id + host_ip_port + medium snapshot versions encoded in location URIs.
• Filter stale snapshot locations during query/MightExist, clean stale versions asynchronously, and recover max snapshot versions from stored URIs when the event backend restarts.
• Document the design in docs/design/report_event_snapshot_uri_version.md and update the API/module docs.

Validation

• git diff --check HEAD^
• On 11.122.133.161, screen -x qisa, window 3:
  • bazelisk --output_base=/home/qisa.cb/workdir/bazel/kvcm-code3-snapshot-uri test --config=debug --config=asan --test_env ASAN_OPTIONS=detect_odr_violation=0 //kv_cache_manager/manager/test:CacheManagerTest //kv_cache_manager/manager/test:MetaSearcherTest
  • Result: MetaSearcherTest PASSED, CacheManagerTest PASSED, TEST_EXIT=0

[qoderai]

Review Summary

This PR adds EVENT_BLOCK_SNAPSHOT support with per-scope URI versioning, query-time stale filtering, async background cleanup, and restart recovery. The design is well-documented and the test coverage exercises the key paths (snapshot filtering, empty snapshots, recovery).

Issues found:

1. [Bug] Dangling event_backend pointer in async cleanup — The cleanup lambda submitted to schedule_plan_executor_ in ReportEvent does not capture the event_backend_holder shared_ptr. CleanupStaleSnapshotLocations runs asynchronously after ReportEvent has returned and the holder is destroyed. While the cleanup function does its own LookupEventReportingBackend, the specific backend instance from ReportEvent may no longer exist. Suggest capturing the holder or passing it directly.

2. [Maintainability] Significant code duplication — SnapshotUriInfo, ParseUint64, HostIpPortFromUri, ParseSnapshotUriInfo are duplicated between vineyard_backend.cc and cache_manager.cc. WithLocationId is duplicated between meta_searcher.cc and select_location_policy.cc. These should be extracted to shared headers.

3. [Observation] Full-scan cleanup cost — Each snapshot commit triggers a CleanupLocationsByPredicate that scans the entire instance CacheMeta. For high-frequency snapshot reporters, this could become a bottleneck.

4. [Question] Partial recovery treated as success — RecoverEventSnapshotVersions marks recovery complete even on EC_PARTIAL_OK, meaning missed locations could lead to version collisions on the next snapshot allocation.

5. [Nit] %lu format for uint64_t — Should use PRIu64 or %llu for portability.

6. [Nit] Unused spec_names field — Added to BlockDeleteEventParams in proto but not consumed in the EVENT_BLOCK_DELETE handler.

7. [Nit] Redundant defensive checks in the snapshot commit loop (the tasks are only enqueued when per_item_ec is already EC_OK).

The core snapshot-versioning logic (allocate → write → commit → filter → cleanup) is sound, and the two-phase commit semantics (uncommitted versions are not filtered) are correct.

---

🤖 Generated by Qoder

[qoderai]

[Bug] The event_backend raw pointer is obtained from event_backend_holder, which is a local shared_ptr in ReportEvent. The cleanup lambda is submitted to schedule_plan_executor_ for asynchronous execution, but it only captures this and value copies of primitive/string arguments — it does not extend the lifetime of event_backend_holder.

By the time the background task calls CleanupStaleSnapshotLocations, ReportEvent has already returned and destroyed event_backend_holder. If the shared_ptr refcount drops to zero, this->registry_manager_ may still return a valid backend inside CleanupStaleSnapshotLocations (which does its own lookup), but the specific backend instance that was used during ReportEvent could have been destroyed.

More critically, the lambda in CleanupStaleSnapshotLocations captures event_backend (raw pointer from the local holder) and passes it to IsSnapshotLocationStale — but that holder is local to CleanupStaleSnapshotLocations, so the pointer is valid for the duration of that function call. However, the root issue remains: the background task relies on LookupEventReportingBackend returning a live object, which is a timing-dependent assumption.

Suggestion: Capture the event_backend_holder shared_ptr in the cleanup lambda to guarantee the backend stays alive:

schedule_plan_executor_->SubmitTask(
    [this, holder = event_backend_holder, instance_id, host_ip_port,
     medium = task.medium, version = task.version, requested_type] {
        this->CleanupStaleSnapshotLocations(instance_id, host_ip_port, medium, version, requested_type);
    });

Or pass the holder directly to CleanupStaleSnapshotLocations to avoid the redundant second lookup.

---

🤖 Generated by Qoder

[qoderai]

[Maintainability] The types and helpers SnapshotUriInfo, ParseUint64, HostIpPortFromUri, ParseSnapshotUriInfo are duplicated verbatim between vineyard_backend.cc (anonymous namespace) and cache_manager.cc (anonymous namespace). Similarly, WithLocationId is duplicated between meta_searcher.cc and select_location_policy.cc.

This creates a maintenance burden: any change to URI parsing or staleness logic must be applied in two places, and the two copies can silently diverge.

Suggestion: Move the shared helpers into a common header (e.g., kv_cache_manager/data_storage/snapshot_uri_utils.h or kv_cache_manager/manager/meta_utils.h) and include it from both translation units. This would also make unit-testing the parsers easier.

---

🤖 Generated by Qoder

[qoderai]

[Observation] CleanupLocationsByPredicate performs a full scan of the entire meta_indexer_ (all blocks for the instance) on every committed snapshot. For high-frequency snapshot reporters, this could become a performance bottleneck — each snapshot commit triggers a scan of potentially millions of keys.

The current design intentionally defers cleanup to a background scan (documented as a non-goal to do synchronously), but consider:

1. Scoping the scan to only the affected location_id prefix (host+medium) rather than all blocks, or
2. Throttling cleanup tasks (e.g., only run cleanup if the previous one for the same scope has completed), or
3. Batching multiple snapshot commits before triggering a single cleanup scan.

This is not blocking, but worth tracking as a known scalability limitation.

---

🤖 Generated by Qoder

[qoderai]

[Nit] The commit loop performs defensive checks on task.event_index (bounds check and per_item_ec re-check), but these are redundant: snapshot_commit_tasks are only populated when per_item_ec[i] == EC_OK (line ~1828), and event_index is always a valid index from the loop. The checks don't hurt, but they add noise. Consider simplifying:

for (const auto &task : snapshot_commit_tasks) {
    event_backend->CommitSnapshotVersion(instance_id, host_ip_port, task.medium, task.version);
    if (schedule_plan_executor_) {
        schedule_plan_executor_->SubmitTask(...);
    }
}

---

🤖 Generated by Qoder

[qoderai]

[Nit - Portability] %lu is used for uint64_t in multiple log statements (lines ~2096, 2131, 2138). On some platforms (notably Windows and 32-bit systems), uint64_t is unsigned long long, not unsigned long, and %lu would be incorrect. Prefer PRIu64 from <cinttypes> or cast to unsigned long long with %llu:

KVCM_LOG_INFO("... snapshot_version [%" PRIu64 "] ...", snapshot_version);

---

🤖 Generated by Qoder

[qoderai]

[Observation] The message types are renamed (NodeRegisterParams → NodeRegisterEventParams, BlockAddParams → BlockAddEventParams, etc.) for naming consistency. This is wire-compatible (field numbers and types are unchanged), so existing serialized data and old clients can still communicate. However, it is source-breaking for C++ code that references the old class names.

The new spec_names field (field 3) in BlockDeleteEventParams is added but not yet consumed in the EVENT_BLOCK_DELETE handler in cache_manager.cc (line ~1741-1749). If this field is intended for future use, consider adding a comment or TODO to clarify. If it's meant to be used now, the handler should be updated.

---

🤖 Generated by Qoder

[qoderai]

[Question] When VisitAllLocations returns EC_PARTIAL_OK (some keys failed to scan or fetch), the recovery is still considered successful and instance_id is added to snapshot_version_recovered_instances_. This means partial failures are silently accepted and recovery won't be retried for this instance until DoCleanup clears the set.

Is this intentional? If some locations were missed during recovery, the recovered max version could be lower than the true max, causing the next snapshot to allocate a version that collides with an existing (but missed) URI. Consider only marking recovery as complete on EC_OK:

if (ec == EC_OK) {
    std::lock_guard<std::mutex> guard(snapshot_version_recovery_mutex_);
    snapshot_version_recovered_instances_.insert(instance_id);
}
return ec;

Or add a comment explaining why partial recovery is acceptable.

---

🤖 Generated by Qoder

[qoderai]

[Nit] The nested map type std::unordered_map<std::string, std::unordered_map<std::string, std::unordered_map<std::string, SnapshotVersionState>>> is verbose and creates a lot of small allocations. Consider using a composite key struct:

struct SnapshotKey {
    std::string instance_id;
    std::string host_ip_port;
    std::string medium;
    bool operator==(const SnapshotKey&) const = default;
};
// with a hash specialization
std::unordered_map<SnapshotKey, SnapshotVersionState> snapshot_versions_;

This would simplify the code, reduce allocation overhead, and make the locking scope clearer. Not blocking, but worth considering for a follow-up.

---

🤖 Generated by Qoder