You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fix calculate_sla duplicate detection to compare config_version_hash in addition to mttr_minutes and threshold_minutes. Previously, submitting a duplicate outage_id after a set_config change would silently return the stale cached result, ignoring that the config (and therefore the deterministic result) had changed.
The doc comment on the duplicate check promises idempotency only when "execution inputs resolve to the same deterministic result." Since config_version_hash is part of those inputs, the comparison must include it to honour the contract invariant.
Type of Change
Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
apexchainx_calculator/src/lib.rs: In calculate_sla, wrap the existing mttr_minutes/threshold_minutes duplicate check inside a config_version_hash equality guard. If the hashes differ (config was updated), execution falls through to the fresh calculation path — the new result is appended to history, stats are incremented, and events are emitted. If hashes match, the existing logic applies: same inputs → idempotent return, different inputs → DuplicateOutageInput error.
test_config_bumped_duplicate_treated_as_fresh_calculation — reward_base changed, same outage_id/mttr/threshold → recalculates with new config, history has 2 entries, stats show 2 calculations
test_config_bumped_duplicate_threshold_change_is_fresh — threshold lowered, same outage_id/mttr → result flips from met→viol
test_duplicate_same_config_still_idempotent — no config change → identical duplicate still returns cached result
test_duplicate_same_config_with_different_mttr_still_panics — same config, different mttr → still errors with DuplicateOutageInput
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
All 393 tests pass (3 slow stress tests skipped). cargo fmt --all --check and cargo clippy --all-targets produce no new warnings.
Checklist
My code follows the project's style guidelines
I have performed a self-review of my own code
I have commented complex logic
I have updated relevant documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Excellent fix @MerlinTheWhiz! Your analysis of the idempotency invariant is spot-on — the doc-comment contract clearly says idempotency holds when execution inputs resolve to the same deterministic result, and config_version_hash is part of those inputs, so wrapping the existing mttr_minutes/threshold_minutes check in a config-hash guard was the right move. The four new tests pin down exactly the right matrix: bumped-config → fresh calculation with history+stats updated; threshold flip → met/viol reversal; same config + same inputs → cached idempotent; same config + different inputs → DuplicateOutageInput. 393/393 passing with no new clippy warnings — clean. Note: PR #48 (the rollback/finalize work for #32) has already landed on main, and the cross_contract_safety.rs / coordination_harness.rs changes here are byte-identical to those, so they apply as a no-op. Merging this one too.
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
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.
Description
Closes #29
Fix
calculate_sladuplicate detection to compareconfig_version_hashin addition tomttr_minutesandthreshold_minutes. Previously, submitting a duplicateoutage_idafter aset_configchange would silently return the stale cached result, ignoring that the config (and therefore the deterministic result) had changed.The doc comment on the duplicate check promises idempotency only when "execution inputs resolve to the same deterministic result." Since
config_version_hashis part of those inputs, the comparison must include it to honour the contract invariant.Type of Change
Related Issues
Relates to #29
Changes Made
apexchainx_calculator/src/lib.rs: Incalculate_sla, wrap the existingmttr_minutes/threshold_minutesduplicate check inside aconfig_version_hashequality guard. If the hashes differ (config was updated), execution falls through to the fresh calculation path — the new result is appended to history, stats are incremented, and events are emitted. If hashes match, the existing logic applies: same inputs → idempotent return, different inputs →DuplicateOutageInputerror.apexchainx_calculator/src/tests.rs: Added 4 tests:test_config_bumped_duplicate_treated_as_fresh_calculation— reward_base changed, same outage_id/mttr/threshold → recalculates with new config, history has 2 entries, stats show 2 calculationstest_config_bumped_duplicate_threshold_change_is_fresh— threshold lowered, same outage_id/mttr → result flips from met→violtest_duplicate_same_config_still_idempotent— no config change → identical duplicate still returns cached resulttest_duplicate_same_config_with_different_mttr_still_panics— same config, different mttr → still errors withDuplicateOutageInputTesting
All 393 tests pass (3 slow stress tests skipped).
cargo fmt --all --checkandcargo clippy --all-targetsproduce no new warnings.Checklist
Screenshot (the 4 new added tests)