test(storage): probe reachability of the comparisons state-sync path#3209
test(storage): probe reachability of the comparisons state-sync path#3209chefsale wants to merge 2 commits into
Conversation
Adds three WARN-level, greppable probes (target: "sync_probe") at the only sites that can exercise the legacy state-based `StorageDelta::Comparisons` / `compare_trees` sync path: - delta.rs `commit_root` — the sole producer of a Comparisons wire artifact. - root.rs `apply_comparisons` — fires whenever any Comparisons delta (incl. the zero-byte no-op that deserializes to `Comparisons(vec![])`) reaches Root::sync. - root.rs `apply_actions` Action::Compare arm — fires if a Compare action is ever received from peer traffic (the trigger that becomes a Comparisons emission). Purpose: static analysis suggests this path is unreachable in production (HashComparison/LevelWise/DAG-CausalActions handle all sync; Compare actions are only generated inside the non-broadcast `__calimero_sync_next` state-op). These probes let a real multi-node run confirm it with data. If no `sync_probe` line appears across a run that forces Merkle divergence, the whole Comparisons subsystem can be removed. Probes surface guest-side via the env::log bridge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 MeroReviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 157.6s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by MeroReviewer | Review ID: review-d23891c3
Documentation ReviewThe following documentation may need updates based on the changes in this PR:
|
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 31% | Review time: 273.6s
🟡 1 warnings. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-1e24cecc
| comparisons: Vec<Comparison>, | ||
| ctx: &crate::interface::ApplyContext, | ||
| ) -> Result<(), StorageError> { | ||
| // Reachability probe (issue 1): fires whenever a `StorageDelta::Comparisons` |
There was a problem hiding this comment.
🟡 apply_comparisons probe fires on the benign zero-byte no-op artifact, undermining the reachability conclusion
The probe is placed unconditionally at the top of apply_comparisons, before the comparisons.is_empty() check. Per StorageDelta's custom BorshDeserialize (delta.rs), a zero-byte wire artifact (the routine no-op that commit_root emits whenever there are neither actions nor comparisons) always deserializes to Comparisons(vec![]), not to Actions(vec![]). That means any ordinary, harmless empty commit that happens to reach Root::sync will trip this WARN with comparison_count=0, even though it has nothing to do with the legacy compare_trees subsystem being investigated. By contrast, the sibling probe in delta.rs::commit_root is correctly scoped to the (&[], _) (comparisons genuinely non-empty) match arm and will not fire for that same no-op case. This asymmetry means a non-zero sync_probe: apply_comparisons invoked log line does not actually confirm that a peer ever sent a genuine Comparisons payload — it could just as easily be triggered by the unrelated, expected empty-artifact fallback. Since the PR's explicit decision rule is "if zero sync_probe lines appear, the path is confirmed dead," a spurious non-zero count here could incorrectly block the planned removal of the subsystem.
Suggested fix:
Split the probe so the zero-byte/no-op case is distinguishable from a genuine received Comparisons payload, e.g. only warn when `!comparisons.is_empty()`, or add a separate lower-signal log line for the empty case: `if comparisons.is_empty() { tracing::debug!(target: "sync_probe", "apply_comparisons invoked with empty comparisons (likely benign zero-byte no-op)"); } else { tracing::warn!(target: "sync_probe", comparison_count = comparisons.len(), "apply_comparisons invoked with non-empty comparisons"); }`
… CI signal The passive warn! probes proved insufficient: the e2e harness does not surface guest-side (WASM) tracing (storage::root = 0 in captured logs; only explicit env::log app lines appear), so the two guest-reachable probes (apply_comparisons, the Action::Compare receive-arm) could not be confirmed from logs. Convert all three dead-path sites to panic!, each with a distinct message: - delta.rs commit_root — a Comparisons wire artifact was emitted. - root.rs apply_comparisons — a Comparisons/empty artifact reached Root::sync. - root.rs Action::Compare arm — a Compare action was received in a delta. Now reachability manifests as a CI failure that surfaces regardless of log capture and pinpoints the live path. A green e2e run across the divergence scenarios (partition/restart/pause/mesh-soak/concurrent-rotation/reconcile) is then a definitive proof the state-based comparison sync path is dead and can be removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 373.6s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by AI Code Reviewer | Review ID: review-eab9e5bb
What
Adds three WARN-level, greppable probes (
target: \"sync_probe\") at the only sites that can exercise the legacy state-basedStorageDelta::Comparisons/compare_treessync path. No behavior change — probes only log.delta.rscommit_root— the sole producer of aComparisonswire artifact.root.rsapply_comparisons— fires whenever anyComparisonsdelta (including the zero-byte no-op that deserializes toComparisons(vec![])) reachesRoot::sync.root.rsapply_actionsAction::Comparearm — fires if aCompareaction is ever received from peer traffic (the trigger that becomes a Comparisons emission).Why
We want to delete the
compare_trees/Comparisonssubsystem (it's deletion-unaware and superseded by HashComparison/LevelWise/DAG-CausalActions). Static analysis strongly suggests it's already unreachable in production:Action::Compareis pushed only insideapply_action, intocontext.actions(notcomparisons), and only during__calimero_sync_next— a state-op whose artifact isn't broadcast.CausalActionsbuilt from local writes (Add/Update/DeleteRef), neverCompare, never empty.__calimero_sync_nextshould always receiveCausalActions→apply_actions, never theComparearm, neverapply_comparisons.The one residual uncertainty is whether the node-side state-delta handler ever re-authors/propagates a DAG delta from a sync-apply outcome (which would carry
Compareactions to peers). Rather than guess, these probes let a real multi-node run answer it with data.How to read the result
Grep node logs (guest probes surface via the
env::logbridge) for:Run something that forces Merkle divergence (concurrent merges + deletes across nodes). If zero
sync_probelines appear, the path is confirmed dead and the follow-up PR removes the whole subsystem (and the now-false convergence comment atinterface.rs:2043-2049).Follow-ups (not in this PR)
compare_trees/compare_affective/generate_comparison_data/Comparison/StorageDelta::Comparisons/apply_comparisons, pending a clean run.🤖 Generated with Claude Code