test(signal): add deterministic durability chaos coverage - #1049
Conversation
📝 WalkthroughWalkthroughAdds deterministic signal-cache chaos tests, SQLite subprocess crash-recovery tests, durability documentation, and a configurable nightly GitHub Actions workflow with failure artifacts. ChangesSignal durability validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
📦 Binary size report
.text per crate
Baseline: |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/signal-durability-nightly.yml:
- Line 41: Update the actions/checkout step in the nightly workflow to disable
credential persistence, ensuring the repository token is not available to
subsequent build or test processes.
In `@tests/signal_durability_sqlite.rs`:
- Around line 204-209: Update the async test call sites that invoke
remove_database to run the cleanup via tokio::task::spawn_blocking, matching the
existing child-process pattern. Move the Path argument into the blocking closure
as needed, await the task, and preserve cleanup of the database, WAL, and SHM
files.
In `@wacore/src/store/signal_cache_durability_chaos.rs`:
- Around line 668-684: Update env_u64 and env_usize to reject malformed or
out-of-range environment values instead of defaulting or clamping them. Report
the variable name and supplied value when parsing or validation fails, while
preserving valid hexadecimal seeds, decimal values, and counts within the
supported range.
- Around line 93-110: Make the group recovery path reachable by adding a
deterministic chaos action that invalidates receiver sender-key state or forces
the forward-jump boundary, then handle it in the action execution and related
group-delivery/retry logic. Update the action selection in action, the Action
handling around the referenced group recovery branches, and any queue setup
needed so NoSenderKeyState or too-far-future recovery is exercised without
exceeding the existing queue constraints.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f957733-5fd8-4fe2-b527-d95ab6c18358
📒 Files selected for processing (7)
.github/workflows/signal-durability-nightly.ymlAGENTS.mdagent_docs/signal_durability.mdtests/signal_durability_sqlite.rswacore/Cargo.tomlwacore/src/store/signal_cache.rswacore/src/store/signal_cache_durability_chaos.rs
Why
Signal counter leases are security-critical, but the existing tests mostly cover one interleaving at a time. We need repeatable coverage for failures that only appear when cancellation, cache lifecycle, failed storage, tombstones, and recovery interact.
What changed
SIGKILL, and the parent verifies recovery at the reservation ceiling plus exact same-incarnation reloads.agent_docs/signal_durability.mdwith the invariants, publication boundary, lifecycle rules, tuning constraints, and review checklist.Everything that executes is test-only (
cfg(test), dev dependencies, or CI), so this adds no production allocations or hot-path work.Lease-size check
I also checked the supplied production trace. It contains 12 logical group messages, 23 outbound stanzas after 11 pairwise retry resends, and one sender-key creation. Since group retries use pairwise Signal rather than spending sender-key iterations, the observed sender-key run is consistent with 12 iterations. That supports batching, but the log does not expose counters and is too short to prove that 64 is optimal.
Keeping 64 is still a conservative tradeoff: one crash burns at most 3.2% of the peer forward-jump bound (64 / 2000), while a monotonic chain needs at most one synchronous reservation write per 64 sends. The new doc explicitly requires run-length/restart telemetry before retuning it and calls out cumulative real-crash gaps.
Validation
cargo fmt --all -- --checkcargo clippy --all --tests -- -D warningscargo test --workspace --exclude e2e-tests --no-fail-fastcargo test -p wacore --no-default-features --libSIGKILL/restart testactionlint .github/workflows/signal-durability-nightly.yml