Address TTL Cascade (#277) review comments + NOSCRIPT recovery fix - #278
Open
yedidyakfir wants to merge 7 commits into
Open
Address TTL Cascade (#277) review comments + NOSCRIPT recovery fix#278yedidyakfir wants to merge 7 commits into
yedidyakfir wants to merge 7 commits into
Conversation
…on NOSCRIPT, converge recovery paths execute_pipeline_with_noscript_recovery replayed the FULL command stack on NOSCRIPT. In a transactional MULTI/EXEC a NOSCRIPT surfaces at EXEC time after the non-EVALSHA commands already committed (Redis does not roll back a transaction on a mid-execution command error), so a full replay double-applies non-idempotent native ops (JSON.NUMINCRBY, JSON.ARRAPPEND, SF ops). This affected every write routed through ensure_pipeline/pipeline_with_execution, not just cascade, and only on a script-cache flush/failover. - context.py: replay only the EVALSHA entries (matching the already-correct _apipeline pattern); add an ignore_redis_error param so the two write paths share one recovery implementation (WR-03, IN-01). - base.py: _apipeline now delegates to the shared helper; drop the duplicated backup/replay block. Also document the non-positive-ttl root-delete asymmetry in aset_ttl(cascade=True) (IN-04) and restore valid indentation on contains_sf_field (stray uncommitted corruption). - test_context.py: rewrite the full-stack-replay test to assert EVALSHA-only replay (it had locked in the buggy behavior). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…freeze lifecycle robust - init.py (WR-01): wrap the unfreeze -> configure -> bake -> refreeze sequence in try/finally so a failed init (e.g. validate_cascade_ttl_targets on a mis-configured graph) still refreezes every model instead of leaving them unfrozen with silently-mutable, half-baked Meta.ttl/cascade_ttl. - init.py (WR-02): teardown_rapyer now clears _ttl_frozen so a torn-down model doesn't leak MetaTtlFrozenError into a later init-less path. - config.py (WR-04): freeze cascade_ttl too, not just ttl — both are baked into the per-class Lua plan / _has_cascade gate, so mutating cascade_ttl post-freeze would silently desync the runtime cascade from the baked plan. - cascade/conftest.py: autouse fixture resets _ttl_frozen around each cascade test so the process-global freeze can't make tests order-dependent (WR-02). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g and correct dead-branch comment - registry.py (IN-03): _lua_literal now also escapes newlines/CR so a stray control char in an injected literal yields valid Lua at SCRIPT LOAD instead of a silently broken script body. - apply.lua (IN-02): correct the recurse=false comment — the branch is a not-yet-exercised seam (every emitted edge has recurse=true), and a non-recursing target can still be reached via its override edges since next_hop ignores budget for overrides. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR centralizes Redis pipeline recovery, replays only queued ChangesPipeline recovery
TTL freeze lifecycle
Lua serialization and cascade documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant RecoveryHelper
participant Redis
Pipeline->>RecoveryHelper: execute pipeline
RecoveryHelper->>Redis: submit queued commands
Redis-->>RecoveryHelper: NoScriptError or ResponseError
RecoveryHelper->>Redis: re-register scripts and replay EVALSHA commands
Redis-->>RecoveryHelper: retry results
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage reportTotal coverage: 99% Full report |
Merging this PR will degrade performance by 18.45%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rapyer/init.py (1)
107-117: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
_ttl_frozenreset in afinallyblock
await model.Meta.redis.aclose()can abort the loop before later models are unfrozen, leaving sharedMetastate frozen after teardown. Move the reset into afinally(or a second loop) so every model is cleared even if one close fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rapyer/init.py` around lines 107 - 117, Update teardown_rapyer so each model’s Meta._ttl_frozen is reset in a finally block around its Redis client close, ensuring the reset runs even when aclose() raises and all models are unfrozen.Source: Path instructions
🧹 Nitpick comments (1)
tests/unit/test_context.py (1)
50-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the EVALSHA-only replay fix; consider adding edge-case tests.
The updated test correctly asserts the ride-along
JSON.SETis not replayed and only theEVALSHAresult comes back. As per path instructions to focus on test coverage completeness and edge cases fortests/**, consider adding:
- A case with two different
EVALSHAentries (different SHAs) incommand_stackto pin down current behavior for the multi-script scenario discussed inrapyer/context.py.- A case for
ignore_redis_error=Trueswallowing a non-NOSCRIPTResponseErrorand returning[].- A case for a persistent NOSCRIPT after retry raising
PersistentNoScriptError.These may already exist elsewhere in the file — worth checking before adding.
#!/bin/bash rg -n "ignore_redis_error|PersistentNoScriptError|def test_" tests/unit/test_context.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_context.py` around lines 50 - 83, Expand coverage around execute_pipeline_with_noscript_recovery by first checking existing tests, then add missing cases for two distinct EVALSHA commands, ignore_redis_error=True swallowing a non-NOSCRIPT ResponseError and returning [], and a retry that still raises NOSCRIPT producing PersistentNoScriptError. Reuse the existing pipe, retry-pipe, and metadata helpers and preserve the current EVALSHA-only replay behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rapyer/scripts/lua/cascade/apply.lua`:
- Around line 209-214: Update the IN-02 comment near the non-recursing edge
branch to state that reaching the target conditionally refreshes it only when
edge.ttl is true; clarify that special-only edges do not call queue_refresh,
while preserving the existing traversal-budget and OVERRIDE behavior
description.
---
Outside diff comments:
In `@rapyer/init.py`:
- Around line 107-117: Update teardown_rapyer so each model’s Meta._ttl_frozen
is reset in a finally block around its Redis client close, ensuring the reset
runs even when aclose() raises and all models are unfrozen.
---
Nitpick comments:
In `@tests/unit/test_context.py`:
- Around line 50-83: Expand coverage around
execute_pipeline_with_noscript_recovery by first checking existing tests, then
add missing cases for two distinct EVALSHA commands, ignore_redis_error=True
swallowing a non-NOSCRIPT ResponseError and returning [], and a retry that still
raises NOSCRIPT producing PersistentNoScriptError. Reuse the existing pipe,
retry-pipe, and metadata helpers and preserve the current EVALSHA-only replay
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c224ac26-d39c-41fa-8074-06c932f158c4
📒 Files selected for processing (8)
rapyer/base.pyrapyer/config.pyrapyer/context.pyrapyer/init.pyrapyer/scripts/lua/cascade/apply.luarapyer/scripts/registry.pytests/unit/cascade/conftest.pytests/unit/test_context.py
Comment on lines
+209
to
+214
| -- IN-02: not-yet-exercised seam. Every edge the planner | ||
| -- currently emits has recurse=true, so this branch is dead | ||
| -- today. A non-recursing edge reaches (and refreshes) its | ||
| -- target and yields it zero traversal budget; note that | ||
| -- the target's own OVERRIDE edges can still be followed, | ||
| -- since next_hop ignores budget for overrides. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that non-recursing edges refresh conditionally.
This branch reaches the target, but it only refreshes it when edge.ttl is true; special-only edges do not call queue_refresh. Please avoid documenting unconditional refresh behavior.
Suggested wording
- -- target and yields it zero traversal budget; note that
+ -- target, conditionally refreshes it when edge.ttl is set,
+ -- and yields it zero traversal budget; note that📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| -- IN-02: not-yet-exercised seam. Every edge the planner | |
| -- currently emits has recurse=true, so this branch is dead | |
| -- today. A non-recursing edge reaches (and refreshes) its | |
| -- target and yields it zero traversal budget; note that | |
| -- the target's own OVERRIDE edges can still be followed, | |
| -- since next_hop ignores budget for overrides. | |
| -- IN-02: not-yet-exercised seam. Every edge the planner | |
| -- currently emits has recurse=true, so this branch is dead | |
| -- today. A non-recursing edge reaches its target, | |
| -- conditionally refreshes it when edge.ttl is set, and | |
| -- yields it zero traversal budget; note that the target's | |
| -- own OVERRIDE edges can still be followed, since | |
| -- next_hop ignores budget for overrides. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rapyer/scripts/lua/cascade/apply.lua` around lines 209 - 214, Update the
IN-02 comment near the non-recursing edge branch to state that reaching the
target conditionally refreshes it only when edge.ttl is true; clarify that
special-only edges do not call queue_refresh, while preserving the existing
traversal-budget and OVERRIDE behavior description.
…ig after init, not just ttl Per review: the freeze should cover all config, not only ttl. Rename the _ttl_frozen flag to _frozen, block every public Meta field in __setattr__ once frozen (private attrs stay writable so init/teardown can toggle it), and rename MetaTtlFrozenError -> MetaFrozenError. A global autouse test fixture unfreezes models around each test so the process-global freeze can't leak across tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e cascade script Per review: drop the _has_cascade branch in refresh_ttl and always run the cascade EVALSHA. With no outgoing edges it simply re-arms the model's own keys, so every TTL refresh goes through one path. Tests updated to assert the script call instead of the per-key EXPIRE loop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…contains_sf_field, shorter docstrings Per review, in rapyer/cascade/planner.py: - _classify_edge returns an EdgeClassification dataclass instead of a tuple. - _static_walk_special_suffixes uses contains_sf_field() instead of a manual hasattr(_special_field_names) check. - multi-line docstrings start on their own line and are trimmed to short summaries; drop -> None return annotations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… notation from comments Per review: drop the internal review/plan tags (D-0x, WR-0x, IN-0x, etc.) and RESEARCH.md/Pitfall references from comments and docstrings across the cascade code and tests; keep the actual explanations. Also reword multi-line docstrings to start on their own line. Renamed a few tests that embedded those tags. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yedidyakfir
changed the title
This was referenced Jul 12, 2026
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
2 participants
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.
Addresses the review comments left on #277, plus one correctness fix found along the way.
Review comments addressed
_ttl_frozen→_frozen;__setattr__blocks every public Meta field once frozen;MetaTtlFrozenError→MetaFrozenError; global autouse test fixture unfreezes between testsrefresh_ttl_has_cascadebranch —refresh_ttlalways runs the cascade EVALSHA (no edges ⇒ just re-arms own keys)_classify_edgereturns anEdgeClassificationdataclass instead of a tuple-> None"-> Nonereturn annotationscontains_sf_field?"contains_sf_field()(guarded bysafe_issubclass) instead of the manualhasattrcheckBonus correctness fix (not from a comment)
context.py): the self-heal replayed the full pipeline on NOSCRIPT, double-applying already-committed non-idempotent native ops (JSON.NUMINCRBY/JSON.ARRAPPEND). Now replays EVALSHA-only (matching_apipeline), and_apipelineshares that one helper. Kept because it's a real data-corruption bug on the write path; happy to split it out if you'd rather.Verification
black --checkandruff checkclean.🤖 Generated with Claude Code