Ft/v0.3.0#13
Merged
Merged
Conversation
Review Summary by Qodov0.3.0: Conditional writes, batch operations, and DistkitRedisKey refactor
WalkthroughsDescription• **Conditional write operations**: Added CounterComparator enum with variants (Eq, Lt, Gt, Ne, Nil) enabling payload-based conditional writes across all counter types via new inc_if and set_if methods • **Batch operations**: Implemented batch APIs (get_all, inc_all, inc_all_if, set_all, set_all_if) for both plain and instance-aware counters with deduplication, chunking, and stale key refresh • **Redis key type refresh**: Renamed RedisKey to DistkitRedisKey with new helper methods (default_prefix, new, new_or_panic, try_sanitize, sanitize_or_panic) and added backwards-compatible alias • **Lua script unification**: Unified strict counter and strict instance-aware counter write paths so conditional and unconditional writes share the same scripts via compare_values helper function • **Lax counter improvements**: Added pending_flushed tracking, extracted flush logic into separate method, and implemented stale key refresh via batch_refresh_stale helper • **Robust batch execution**: Introduced execute_pipeline_with_script_retry utility for automatic script reloading on pipeline failures • **Activity tracker fix**: Fixed signal ordering in ActivityTracker by adding atomic store before watch signal • **Comprehensive test coverage**: Added tests for all conditional and batch operations, comparator variants, duplicate key handling, and stale refresh scenarios • **Documentation refresh**: Updated all examples and docs to use DistkitRedisKey, added CounterComparator documentation, and included practical conditional write examples • **Benchmark updates**: Renamed inc_batch_10 to inc_all_10 and migrated to public inc_all API Diagramflowchart LR
A["CounterComparator<br/>Eq/Lt/Gt/Ne/Nil"] -->|"matches()"| B["Conditional<br/>Evaluation"]
B -->|"inc_if/set_if"| C["Plain Counters<br/>Strict & Lax"]
B -->|"set_on_instance_if"| D["Instance-Aware<br/>Counters"]
E["Batch APIs<br/>get_all/inc_all"] -->|"dedup & chunk"| F["Pipeline<br/>Execution"]
F -->|"execute_pipeline_with_script_retry"| G["Lua Scripts<br/>with compare_values"]
H["DistkitRedisKey<br/>helpers"] -->|"sanitize/validate"| I["Redis Keys"]
C -->|"uses"| G
D -->|"uses"| G
File Changes1. src/icounter/strict_instance_aware_counter.rs
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+14
to
+19
| To run tests manually (Redis must be running): | ||
| ```bash | ||
| REDIS_URL="redis://127.0.0.1:16379/" cargo test --all-features | ||
| REDIS_URL="redis://127.0.0.1:16379/" cargo test --all-features <test_name> # single test | ||
| cargo test --doc --all-features # doctests only | ||
| ``` |
There was a problem hiding this comment.
1. cargo test --doc missing redis_url 📘 Rule violation ☼ Reliability
CLAUDE.md instructs running cargo test --doc --all-features without setting REDIS_URL, despite stating tests require a live Redis and REDIS_URL. This can cause contributors/automation to run tests in an invalid environment, violating the required test invocation guidance.
Agent Prompt
## Issue description
`CLAUDE.md` includes a bare `cargo test --doc --all-features` command without `REDIS_URL`, which conflicts with the requirement that tests run with a live Redis and `REDIS_URL` set.
## Issue Context
The compliance checklist requires that documentation/instructions use `make test` for the full test suite, and that any direct `cargo test` invocation sets `REDIS_URL` and assumes Redis is running.
## Fix Focus Areas
- CLAUDE.md[14-21]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+868
to
+875
| for ( | ||
| ((key, _, _), local_epoch), | ||
| (_, cumulative, inst_count, redis_epoch, _, matched_raw), | ||
| ) in chunk | ||
| .iter() | ||
| .zip(local_epochs.iter()) | ||
| .zip(chunk_results.into_iter()) | ||
| { |
There was a problem hiding this comment.
2. inc_if_batch uses .zip() 📘 Rule violation ≡ Correctness
StrictInstanceAwareCounter::inc_if_batch zips chunk/local_epochs with chunk_results, aligning by position and ignoring the key string returned in each result tuple. This violates the requirement to associate results with keys using the echoed key, not positional ordering.
Agent Prompt
## Issue description
`StrictInstanceAwareCounter::inc_if_batch` aligns pipeline results to inputs with `.zip()`, discarding the echoed key in the Lua result tuple and relying on positional ordering.
## Issue Context
The Lua scripts already echo keys; batch callers should build a `HashMap` keyed by that returned key string and then update local state/output by looking up the returned key.
## Fix Focus Areas
- src/icounter/strict_instance_aware_counter.rs[839-881]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+981
to
+1040
| let mut conn = self.connection_manager.clone(); | ||
| let mut map: HashMap<DistkitRedisKey, (i64, i64)> = HashMap::with_capacity(updates.len()); | ||
| let mut processed = 0; | ||
|
|
||
| for (key_str, cumulative, inst_count, redis_epoch, _) in chunk_results { | ||
| if let Ok(key) = RedisKey::try_from(key_str.clone()) { | ||
| while processed < updates.len() { | ||
| let end = (processed + MAX_BATCH_SIZE).min(updates.len()); | ||
| let chunk = &updates[processed..end]; | ||
| let script = &self.set_script; | ||
| let local_epochs: HashMap<DistkitRedisKey, u64> = chunk | ||
| .iter() | ||
| .map(|(key, _, _)| ((*key).clone(), self.get_local_epoch(key))) | ||
| .collect(); | ||
|
|
||
| let chunk_results: Vec<(String, i64, i64, u64, i64, i64)> = | ||
| execute_pipeline_with_script_retry(&mut conn, script, chunk, |update| { | ||
| let (key, comparator, count) = update; | ||
| let (lua_comparator, compare_against) = comparator.as_lua_parts(); | ||
| let mut inv = script.key(self.epoch_key()); | ||
| inv.key(self.instances_key()); | ||
| inv.key(self.cumulative_key()); | ||
| inv.key(self.keys_key()); | ||
| inv.key(self.inst_count_key()); | ||
| inv.arg(key.as_str()); | ||
| inv.arg(lua_comparator); | ||
| inv.arg(compare_against); | ||
| inv.arg(*count); | ||
| inv.arg(self.get_local_epoch(key)); | ||
| inv.arg(self.get_local_count(key)); | ||
| inv.arg(self.dead_instance_threshold_ms); | ||
| inv.arg(self.prefix_str()); | ||
| inv.arg(&self.instance_id); | ||
| inv.arg(self.max_epoch); | ||
| inv | ||
| }) | ||
| .await?; | ||
|
|
||
| for (key, cumulative, inst_count, redis_epoch, _, matched_raw) in chunk_results { | ||
| let Ok(key) = DistkitRedisKey::try_from(key.clone()) else { | ||
| continue; | ||
| }; | ||
|
|
||
| let local_epoch = local_epochs.get(&key).copied().unwrap_or(0); | ||
| if matched_raw != 0 || local_epoch == redis_epoch { | ||
| self.update_local_store(&key, redis_epoch, cumulative, inst_count); | ||
| } | ||
| output.push((key_str, cumulative, inst_count)); | ||
|
|
||
| map.insert(key, (cumulative, inst_count)); | ||
| } | ||
|
|
||
| processed = end; | ||
| } | ||
|
|
||
| // All chunks succeeded — drain entire input. | ||
| increments.drain(..processed); | ||
| Ok(updates | ||
| .iter() | ||
| .map(|(k, _, _)| { | ||
| let (cum, inst) = map.get(k).copied().unwrap_or((0, 0)); | ||
| (*k, cum, inst) | ||
| }) | ||
| .collect()) | ||
| } |
There was a problem hiding this comment.
3. Global set batch drops duplicates 🐞 Bug ≡ Correctness
StrictInstanceAwareCounter::set_if_batch stores results in a HashMap keyed by DistkitRedisKey and then rebuilds the output by key lookup, so duplicate keys return the last result for every occurrence. This produces incorrect (cumulative, instance_count) pairs for earlier duplicate entries and breaks the trait’s per-item ordered batch semantics.
Agent Prompt
## Issue description
`StrictInstanceAwareCounter::set_if_batch` uses a `HashMap<DistkitRedisKey, (i64, i64)>` and later reconstructs the output by key lookup. Duplicate keys in the batch overwrite earlier entries, so the returned Vec is wrong for earlier duplicates.
## Issue Context
Redis pipelines return replies in the same order as invocations. The safest approach is to accumulate an output Vec in the same iteration order (e.g., zip `chunk` with `chunk_results`) and avoid de-duplicating results.
## Fix Focus Areas
- src/icounter/strict_instance_aware_counter.rs[971-1040]
- src/icounter/strict_instance_aware_counter.rs[1768-1773]
- src/icounter/mod.rs[467-472]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+389
to
+395
| /// Returns `(key, instance_count)` for each key in `keys`, in the same | ||
| /// order. Pure-local: no Redis round-trip, no staleness check. A key | ||
| /// with no local contribution returns `(key, 0)`. | ||
| async fn get_all_on_instance<'k>( | ||
| &self, | ||
| keys: &[&'k DistkitRedisKey], | ||
| ) -> Result<Vec<(&'k DistkitRedisKey, i64)>, DistkitError>; |
There was a problem hiding this comment.
4. Get_all_on_instance does redis i/o 🐞 Bug ≡ Correctness
InstanceAwareCounterTrait::get_all_on_instance is documented as pure-local with no Redis round-trip, but both lax and strict implementations call Redis refresh/get batch paths. Callers relying on local-only behavior will incur unexpected network latency and staleness checks.
Agent Prompt
## Issue description
`InstanceAwareCounterTrait::get_all_on_instance` is documented as pure-local (no Redis I/O), but current implementations perform Redis calls.
## Issue Context
Either (a) update implementations to compute instance counts strictly from local state (no refresh / no Redis scripts), accepting potentially stale results as documented, or (b) change the trait docs to match the actual behavior.
## Fix Focus Areas
- src/icounter/mod.rs[389-395]
- src/icounter/lax_instance_aware_counter.rs[852-871]
- src/icounter/strict_instance_aware_counter.rs[1734-1740]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1257
to
1259
| if matched_raw != 0 || local_epoch == redis_epoch { | ||
| self.update_local_store(key, redis_epoch, cumulative, inst_count); | ||
| } |
There was a problem hiding this comment.
5. Conditional no-op leaves stale cache 🐞 Bug ≡ Correctness
StrictInstanceAwareCounter::inc_if (and related conditional setters) updates local_store only when the comparator matched or when local_epoch equals redis_epoch, so a comparator failure during an epoch mismatch leaves local epoch/local_count stale. This can cause subsequent operations and heartbeat recovery to use outdated local state.
Agent Prompt
## Issue description
Conditional instance-aware operations skip updating `local_store` when the comparator fails during an epoch mismatch (`local_epoch != redis_epoch`). This leaves cached epoch/local_count stale even though Redis returned fresh state.
## Issue Context
A common scenario is: another instance bumps the epoch (via set/del), this instance has a stale `local_epoch`, and then a conditional write fails its comparator. The method returns fresh `(cumulative, inst_count)` but does not update cache, so future calls keep using stale epoch/local_count.
## Fix Focus Areas
- src/icounter/strict_instance_aware_counter.rs[1219-1262]
- src/icounter/strict_instance_aware_counter.rs[1291-1340]
- src/icounter/strict_instance_aware_counter.rs[1373-1421]
- src/icounter/strict_instance_aware_counter.rs[658-695]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Branch Changelog
Summary
This branch expands the counter APIs, improves the Redis key type, refreshes documentation, and fixes benchmark coverage to match the current public surface.
Added
CounterComparatoracross plain and instance-aware counters:inc_ifset_ifset_on_instance_ifset_all_ifset_all_on_instance_ifinc_allinc_all_ifCounterComparator::Eq(i64)CounterComparator::Lt(i64)CounterComparator::Gt(i64)CounterComparator::Ne(i64)CounterComparator::NilDistkitRedisKeyhelpers:default_prefixnewnew_or_panictry_sanitizesanitize_or_panicChanged
compare_againstparameters.*_ifimplementations withCounterComparator::Nil.DistkitRedisKeyas the primary key type.RedisKeyalias for compatibility while the new public name isDistkitRedisKey.Documentation
CounterTraitandInstanceAwareCounterTraitwith examples and assertions for the new conditional and batch APIs.DistkitRedisKey.DistkitRedisKeyhelper methods.Tests
inc,set, and batch setters.CounterComparator::Nil.inc_allinc_all_ifBenchmarks
DistkitRedisKey.inc_allAPI.inc_batch_10toinc_all_10.Verification
cargo test --doc --all-featurescargo bench --no-run