refactor(storage): remove Engine abstraction - #365
Merged
Conversation
Wrap the concrete DB in a non-cloneable shutdown owner so Redis waits for active RocksDB background work before the final handle can be released from a callback thread. Expose only a borrowed DB accessor and cover the active compaction-filter drop race with a real RocksDB regression test. Tested: cargo +1.95-x86_64-pc-windows-msvc test --package storage --quiet Tested: 5x cargo +1.95-x86_64-pc-windows-msvc test --package storage --lib dropping_last_owner_waits_for_active_compaction_filter_before_reopen -- --test-threads=1 Tested: rustfmt +1.95-x86_64-pc-windows-msvc --edition 2024 --check src/storage/src/redis.rs src/storage/src/data_compaction_filter.rs src/storage/tests/redis_basic_test.rs src/storage/tests/redis_hash_test.rs src/storage/tests/redis_set_test.rs src/storage/tests/redis_zset_test.rs Tested: cargo +1.95-x86_64-pc-windows-msvc clippy --package storage --lib -- -D warnings -D clippy::unwrap_used Co-authored-by: OmX <omx@oh-my-codex.dev>
Signal from inside the unique owner Drop before background cancellation so the active-compaction regression cannot pass merely because the drop thread was not scheduled. Route the exported DB/CF macro through Redis::db() and exercise its expansion from an external integration crate. Tested: 10x cargo +1.95-x86_64-pc-windows-msvc test --package storage --lib dropping_last_owner_waits_for_active_compaction_filter_before_reopen -- --test-threads=1 Tested: cargo +1.95-x86_64-pc-windows-msvc test --package storage --lib --quiet Tested: cargo +1.95-x86_64-pc-windows-msvc test --package storage --quiet Tested: rustfmt +1.95-x86_64-pc-windows-msvc --edition 2024 --check src/storage/src/redis.rs src/storage/tests/redis_basic_test.rs Tested: cargo +1.95-x86_64-pc-windows-msvc clippy --package storage --lib -- -D warnings -D clippy::unwrap_used Tested: cargo +1.95-x86_64-pc-windows-msvc clippy --package storage --test redis_basic_test --no-deps -- -D warnings -D clippy::unwrap_used Co-authored-by: OmX <omx@oh-my-codex.dev>
Start a dedicated temporary Kiwi instance for Ubuntu integration tests and make server availability a strict pytest gate in CI. Isolated runs flush the dedicated database around every test while ordinary local runs retain skip and prefix-cleanup behavior. Constraint: Keep lifecycle management in the test harness and do not modify product or test-case code. Tested: strict unavailable-server failure; ordinary local skip; missing binary, occupied port, and early server exit paths; bash syntax; staged diff check; isolated list sequence reports 2 passed. Not-tested: Full Python suite depends on the remaining Redis compatibility fixes in later plan tasks. Co-authored-by: OmX <omx@oh-my-codex.dev>
Correct MSET compatibility expectations and make concurrent observations use one MGET snapshot. Propagate worker failures through futures so pytest cannot silently pass thread assertion errors. Constraint: Python integration tests only; no Rust, fixture, or CI harness changes. Rejected: Sequential GET and LLEN/LRANGE comparisons because they observe different command boundaries. Confidence: high Scope-risk: Test expectations and concurrency orchestration only. Tested: py_compile; pytest --collect-only (55 tests); targeted old-binary probe (2 expected atomicity failures, corrected semantic tests pass). Not-tested: Full Python suite awaits the matching multi-instance storage fix and rebuilt Kiwi binary. Co-authored-by: OmX <omx@oh-my-codex.dev>
Let ThreadPoolExecutor futures preserve worker tracebacks, remove the stress-test exception sink, and register exact test keys for unconditional cleanup after failures. Constraint: Follow-up touches only the four Python integration test files from the prior semantics commit. Rejected: Prefix-based cleanup and collected exception counters because they can hide worker failures or affect unrelated local keys. Confidence: high Scope-risk: Test orchestration and cleanup only. Tested: WSL venv py_compile; pytest --collect-only (55 tests); git diff --cached --check. Not-tested: Runtime suite awaits the concurrent Rust storage changes in the shared worktree. Co-authored-by: OmX <omx@oh-my-codex.dev>
Observe DEL/MSET races with a synchronized MGET worker, namespace every touched Redis key, and bound concurrent and slow tests with pytest-timeout plus future deadlines. Constraint: Only the four Python integration test files are changed; existing Rust work remains separate. Rejected: Final-state-only atomicity checks, generic local Redis key names, and unbounded executor waits. Confidence: high Scope-risk: Test naming, isolation, synchronization, and timeout behavior only. Tested: WSL venv py_compile; pytest --collect-only (55 tests); combined Python diff check from 4b43a22^. Not-tested: Runtime Python suite awaits the final rebuilt Kiwi binary. Co-authored-by: OmX <omx@oh-my-codex.dev>
Bound redis-py connect/read operations and make the DEL/MSET observer handshake wait for a completed mutation before sampling while holding the last mutator until that sample completes. Constraint: Only tests/python/conftest.py and the MSET concurrency test are changed; Rust work remains separate. Rejected: Permanently-set activity flags because they do not prove the sample happened before mutation completion. Confidence: high Scope-risk: Python test client timeouts and deterministic concurrency synchronization only. Tested: WSL venv py_compile; pytest --collect-only (55 tests); strict no-server mode exits non-zero without skipping; combined Python diff check. Not-tested: Runtime suite awaits the final rebuilt Kiwi binary. Co-authored-by: OmX <omx@oh-my-codex.dev>
Pause both mutators immediately after their first DEL or MSET until the observer validates a single MGET snapshot, then let them finish the remaining iterations before marking mutation completion. Constraint: Only tests/python/test_mset_concurrent.py changes; client timeout and Rust history remain untouched. Rejected: End-of-loop overlap gates because they do not force observation before subsequent mutations. Confidence: high Scope-risk: Deterministic test synchronization only. Tested: WSL venv py_compile; pytest --collect-only (55 tests); current and combined Python diff checks. Not-tested: Runtime suite awaits the final rebuilt Kiwi binary. Co-authored-by: OmX <omx@oh-my-codex.dev>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change removes the Engine abstraction, adds RAII storage-access coordination, hardens snapshot installation, preserves binary Redis and RESP behavior, and introduces a managed Python integration-test lifecycle. ChangesStorage, Raft, and snapshot flow
Redis and integration compatibility
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| match parser.parse(Bytes::copy_from_slice(&buf[..n])) { | ||
| RespParseResult::Complete(data) => return data, | ||
| RespParseResult::Incomplete => continue, | ||
| RespParseResult::Error(e) => panic!("RESP parse error: {:?}", e), |
| observed.extend(remaining); | ||
|
|
||
| let expected: HashSet<Vec<u8>> = (0..PRODUCERS) | ||
| .flat_map(|producer_id| { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/raft/src/log_store_rocksdb.rs (1)
964-973: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the touched test
unwrap()calls.These writes violate the project-wide
clippy::unwrap_usedrule. Use descriptiveexpect(...)calls, or explicitly allow the lint for the test module if that is intentional.Proposed fix
- engine_clone.write(&batch).unwrap(); + engine_clone + .write(&batch) + .expect("test database batch write should succeed");As per coding guidelines, “Do not use
unwrap;clippy::unwrap_usedis denied project-wide.”Also applies to: 1100-1109, 1152-1159, 1226-1235, 1291-1298, 1692-1701, 1772-1781, 1866-1875, 2006-2015, 2131-2140, 2208-2217, 2284-2293, 2373-2382, 2489-2498, 2546-2553, 2611-2620, 2703-2712, 2771-2780, 2838-2847, 2928-2937, 3030-3039, 3106-3115, 3196-3203, 3253-3262, 3318-3325, 3354-3361, 3399-3406
🤖 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 `@src/raft/src/log_store_rocksdb.rs` around lines 964 - 973, Replace every touched unwrap call in the test write paths, including the loops around sequential_entries and the additional referenced ranges, with descriptive expect messages that identify the failed operation; alternatively, explicitly allow clippy::unwrap_used for the intentional test module. Preserve the existing write behavior and error propagation.Source: Coding guidelines
🧹 Nitpick comments (6)
tests/python/test_list_commands.py (2)
339-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
strict=Truetozip().Ruff B905 flags this
zip()without an explicitstrictparameter; silent truncation on length mismatch would otherwise hide a bug in this binary round-trip test.♻️ Proposed fix
- for original, retrieved_item in zip(reversed(binary_items), retrieved): + for original, retrieved_item in zip(reversed(binary_items), retrieved, strict=True):🤖 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/python/test_list_commands.py` at line 339, Update the zip call in the binary round-trip test to pass strict=True, ensuring mismatched binary_items and retrieved lengths raise an error instead of being silently truncated.Source: Linters/SAST tools
130-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer truthy checks over
== True.Ruff flags
assert r.lset(...) == Trueandassert r.ltrim(...) == True(lines 130, 134, 178). Idiomatic Python prefers the direct truthiness check.♻️ Proposed fix
- assert r.lset('test_kiwi_list_commands_list', 1, 'modified') == True + assert r.lset('test_kiwi_list_commands_list', 1, 'modified') ... - assert r.lset('test_kiwi_list_commands_list', -1, 'last') == True + assert r.lset('test_kiwi_list_commands_list', -1, 'last') ... - assert r.ltrim('test_kiwi_list_commands_list', 1, 4) == True + assert r.ltrim('test_kiwi_list_commands_list', 1, 4)🤖 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/python/test_list_commands.py` around lines 130 - 178, In the list command tests, replace equality comparisons against True in the lset assertions within the surrounding test and the ltrim assertion in test_ltrim with direct truthiness assertions, preserving the existing calls and expected behavior.Source: Linters/SAST tools
src/cmd/src/hscan.rs (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
TestStreamtest double. The no-opStreamTraitimplementation is duplicated verbatim between the two test modules (and likely other cmd test modules per theexecutor.rsgraph context). Moving it to a shared test-utility module would reduce duplication as more scan-family commands gain binary-safety tests.
src/cmd/src/hscan.rs#L149-166: remove the localTestStreamand import it from a shared test-utils module instead.src/cmd/src/sscan.rs#L147-164: remove the localTestStreamand import it from the same shared test-utils module.🤖 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 `@src/cmd/src/hscan.rs` at line 1, Extract the duplicated no-op TestStream and its StreamTrait implementation from the hscan and sscan test modules into a shared test-utility module. Remove both local definitions and update the tests in hscan and sscan to import and reuse the shared TestStream.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared cursor/MATCH/COUNT parsing helper. The cursor-parsing plus
MATCH/COUNToption-parsing loop is duplicated verbatim betweenhscan.rsandsscan.rs. A shared helper (e.g.parse_scan_cursor_and_options(argv) -> Result<(u64, Option<Vec<u8>>, Option<usize>), RespData>) would remove the duplication and prevent future drift between the two commands' error messages/semantics.
src/cmd/src/hscan.rs#L67-117: replace this block with a call to the shared parsing helper.src/cmd/src/sscan.rs#L67-117: replace this block with a call to the same shared parsing helper.🤖 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 `@src/cmd/src/hscan.rs` at line 1, Extract the duplicated cursor, MATCH, and COUNT parsing loop from the HSCAN and SSCAN command handlers into a shared helper, such as parse_scan_cursor_and_options, returning the parsed cursor and optional pattern/count or the existing RespData errors. Replace the corresponding parsing blocks in both hscan and sscan with calls to this helper, preserving their current validation, error messages, and semantics.Cargo.toml (1)
77-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
windows-sysworkspace dependency.windows-sysis not referenced by any Rust source orCargo.tomlin this repository, soversion = "0.61.2"withWin32_Storage_FileSystemadds dead dependency metadata.🤖 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 `@Cargo.toml` at line 77, Remove the unused windows-sys workspace dependency declaration, including its version and Win32_Storage_FileSystem feature configuration, from the workspace dependency definitions in Cargo.toml.src/cmd/src/mget.rs (1)
38-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
STORAGE_EXCLUSIVEon MGET serializes a hot read path against all other commands.Marking MGET exclusive makes it take the write side of
command_access_gate, blocking (and being blocked by) every other command including single-keyGETand otherMGETs. Since the multi-key writers (MSET/MSETNX/DEL) already hold exclusive access, a shared MGET is still mutually excluded from them via theRwLock, so it would still observe atomic multi-key writes without partial results. Exclusive access here appears stronger than needed for cross-instance visibility and can materially reduce read concurrency.Please confirm whether MGET truly needs exclusive access, or whether shared access is sufficient (which would also require updating the
only_multi_instance_atomic_commands_require_exclusive_storage_accesscontract test insrc/cmd/src/lib.rs).🤖 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 `@src/cmd/src/mget.rs` at line 38, The MGET command currently uses STORAGE_EXCLUSIVE, unnecessarily serializing concurrent reads. Update its CmdFlags to use shared read access while preserving exclusion from multi-key writers, and adjust the only_multi_instance_atomic_commands_require_exclusive_storage_access contract test in lib.rs to reflect that MGET no longer requires exclusive storage access.
🤖 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 `@src/cmd/src/keys.rs`:
- Around line 80-81: Add #![allow(clippy::unwrap_used)] at the beginning of the
inline tests module in keys.rs, immediately inside mod tests, so its existing
unwrap calls are permitted during linting. Follow the established pattern used
by the glob_tests module.
In `@src/raft/src/state_machine.rs`:
- Around line 571-574: In the pause handling flow, create
ResumeBeforeMarkerGuard before calling
self.pause_controller.request_pause().await so cancellation during or
immediately after the pause request always has a resume guard. Preserve the
existing snapshot_operation_gate acquisition and guard lifetime.
---
Outside diff comments:
In `@src/raft/src/log_store_rocksdb.rs`:
- Around line 964-973: Replace every touched unwrap call in the test write
paths, including the loops around sequential_entries and the additional
referenced ranges, with descriptive expect messages that identify the failed
operation; alternatively, explicitly allow clippy::unwrap_used for the
intentional test module. Preserve the existing write behavior and error
propagation.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 77: Remove the unused windows-sys workspace dependency declaration,
including its version and Win32_Storage_FileSystem feature configuration, from
the workspace dependency definitions in Cargo.toml.
In `@src/cmd/src/hscan.rs`:
- Line 1: Extract the duplicated no-op TestStream and its StreamTrait
implementation from the hscan and sscan test modules into a shared test-utility
module. Remove both local definitions and update the tests in hscan and sscan to
import and reuse the shared TestStream.
- Line 1: Extract the duplicated cursor, MATCH, and COUNT parsing loop from the
HSCAN and SSCAN command handlers into a shared helper, such as
parse_scan_cursor_and_options, returning the parsed cursor and optional
pattern/count or the existing RespData errors. Replace the corresponding parsing
blocks in both hscan and sscan with calls to this helper, preserving their
current validation, error messages, and semantics.
In `@src/cmd/src/mget.rs`:
- Line 38: The MGET command currently uses STORAGE_EXCLUSIVE, unnecessarily
serializing concurrent reads. Update its CmdFlags to use shared read access
while preserving exclusion from multi-key writers, and adjust the
only_multi_instance_atomic_commands_require_exclusive_storage_access contract
test in lib.rs to reflect that MGET no longer requires exclusive storage access.
In `@tests/python/test_list_commands.py`:
- Line 339: Update the zip call in the binary round-trip test to pass
strict=True, ensuring mismatched binary_items and retrieved lengths raise an
error instead of being silently truncated.
- Around line 130-178: In the list command tests, replace equality comparisons
against True in the lset assertions within the surrounding test and the ltrim
assertion in test_ltrim with direct truthiness assertions, preserving the
existing calls and expected 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 556711ce-eb57-42a9-bbec-389adeb0002c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (66)
.github/workflows/ci.ymlCLAUDE.mdCargo.tomlREADME.mdREADME_CN.mddocs/superpowers/plans/2026-07-22-remove-engine-trait-phase-a.mddocs/superpowers/plans/2026-07-22-snapshot-install-transaction-hardening.mddocs/superpowers/plans/2026-07-23-python-integration-and-redis-compat.mddocs/superpowers/specs/2026-07-21-remove-engine-trait-design.mddocs/superpowers/specs/2026-07-23-python-integration-and-redis-compat-design.mdsrc/cmd/src/del.rssrc/cmd/src/get.rssrc/cmd/src/hscan.rssrc/cmd/src/keys.rssrc/cmd/src/lib.rssrc/cmd/src/mget.rssrc/cmd/src/mset.rssrc/cmd/src/msetnx.rssrc/cmd/src/sscan.rssrc/common/runtime/lib.rssrc/common/runtime/storage_server.rssrc/engine/Cargo.tomlsrc/engine/src/engine.rssrc/engine/src/lib.rssrc/engine/src/rocksdb_engine.rssrc/executor/src/executor.rssrc/net/tests/storage_command_e2e_tests.rssrc/raft/Cargo.tomlsrc/raft/src/grpc/client.rssrc/raft/src/log_store_rocksdb.rssrc/raft/src/node.rssrc/raft/src/state_machine.rssrc/raft/tests/log_store_rocksdb_test.rssrc/raft/tests/snapshot_logindex_test.rssrc/raft/tests/snapshot_roundtrip_test.rssrc/resp/src/encode.rssrc/resp/tests/integration_tests.rssrc/server/src/main.rssrc/storage/Cargo.tomlsrc/storage/src/batch.rssrc/storage/src/checkpoint.rssrc/storage/src/data_compaction_filter.rssrc/storage/src/durable_fs.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_multi.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/storage.rssrc/storage/src/storage_impl.rssrc/storage/tests/checkpoint_test.rssrc/storage/tests/redis_basic_test.rssrc/storage/tests/redis_hash_test.rssrc/storage/tests/redis_list_test.rssrc/storage/tests/redis_set_test.rssrc/storage/tests/redis_string_test.rssrc/storage/tests/redis_zset_test.rssrc/storage/tests/ttl_test.rstests/python/conftest.pytests/python/test_list_commands.pytests/python/test_mset.pytests/python/test_mset_concurrent.pytests/python/test_wrongtype_errors.pytests/run_python_integration.sh
💤 Files with no reviewable changes (4)
- src/engine/Cargo.toml
- src/engine/src/engine.rs
- src/engine/src/lib.rs
- src/engine/src/rocksdb_engine.rs
Share cursor, MATCH, and COUNT parsing across HSCAN, SSCAN, and ZSCAN while preserving binary patterns and existing error semantics. Align the KEYS test module with the project unwrap lint convention. Constraint: Keep storage scan APIs and command-visible behavior unchanged. Rejected: Changing MGET locking, snapshot pause handling, Windows durability dependencies, or unrelated review suggestions. Confidence: high Scope-risk: Command parser refactor and test lint annotation only. Tested: WSL Rust 1.95 cargo fmt --check; cargo test -p cmd (105 passed); cargo clippy -p cmd --all-features with warnings and unwrap denied. Not-tested: Full workspace test suite and remote CI; both will run after push. Co-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/cmd/src/scan_options.rs`:
- Line 64: Remove the #[allow(clippy::unwrap_used)] attribute and update the
affected test unwrap() calls to use descriptive expect(...) messages, preserving
the existing test behavior while complying with the project-wide unwrap
prohibition.
In `@src/cmd/src/zscan.rs`:
- Around line 74-76: The ZSCAN command currently converts option patterns
through String::from_utf8_lossy, corrupting invalid UTF-8 before matching.
Update the pattern handling in the command flow and Storage::zscan/glob-matching
path to preserve raw bytes, and add a command-level test proving a non-UTF-8
pattern such as b"\xff?" matches the original bytes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38066d81-efd1-4a4b-b7d5-058ba0c30f21
📒 Files selected for processing (6)
src/cmd/src/hscan.rssrc/cmd/src/keys.rssrc/cmd/src/lib.rssrc/cmd/src/scan_options.rssrc/cmd/src/sscan.rssrc/cmd/src/zscan.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/cmd/src/lib.rs
- src/cmd/src/hscan.rs
- src/cmd/src/keys.rs
- src/cmd/src/sscan.rs
AlexStocks
commented
Jul 23, 2026
| KiwiSnapshotBuilder { | ||
| _storage: Arc::clone(&self.storage_swap), | ||
| storage: self.storage_swap.load_full(), | ||
| _snapshot_operation: snapshot_operation, |
Contributor
Author
There was a problem hiding this comment.
[P1] 不要让完整快照打包过程阻塞 Raft apply
这里把 snapshot_operation_gate 的 guard 存进 builder，而 apply() 也获取同一把锁。OpenRaft 0.9.24 会把 build_snapshot() 放到独立任务执行，因此从 builder 创建开始，到 RocksDB checkpoint、pack_dir_to_vec() 读取整个数据库、persist_current_snapshot() 同步落盘结束之前，后续已提交日志都无法 apply。默认每 5000 条日志触发一次，大库会周期性产生随数据量增长的写停顿和 replication lag；Base 没有这个全程阻塞。建议只在固化 collector state 与 RocksDB checkpoint 的短阶段阻塞 apply，随后释放该门禁；另用 publication/install 门禁串行 tar 发布与 snapshot install。请补故障注入测试，在 checkpoint 完成后阻塞打包/落盘，验证 apply 仍能前进，同时旧 builder 不能覆盖 install 后的 current snapshot。
AlexStocks
commented
Jul 23, 2026
| return t_idx >= text.len(); | ||
| } | ||
| pub(crate) fn glob_match_bytes(pattern: &[u8], text: &[u8]) -> bool { | ||
| if text.is_empty() { |
Contributor
Author
There was a problem hiding this comment.
[P1] 连续星号也必须匹配空字节串
这个 early return 使 **、*** 对空 key/field/member 返回 false；Base 的 matcher 会把剩余全部 * 视为匹配零个字符，Redis 实测 KEYS "**" 会返回空 key，SSCAN ... MATCH "**" 也会返回空 member。本 PR 因此引入了 Redis glob 兼容回归，并在 keys_command_preserves_allkeys_behavior_for_empty_key 与本文件测试中把错误结果固化。建议删除该特判，让空输入也走末尾连续 * 消耗逻辑，并把 */**/*** 对空字节串的断言全部改为 true；同时补 KEYS/HSCAN/SSCAN 命令级回归。
Split snapshot state and publication coordination so apply resumes after checkpoint while snapshot publication remains serialized. Correct repeated-star matching for empty Redis keys and scan entries, with deterministic cancellation and error-path coverage. Constraint: Preserve fail-closed snapshot install marker semantics and existing public storage APIs. Rejected: A single long-held snapshot mutex, fixed-duration concurrency assertions, or public test hooks. Confidence: high Scope-risk: Raft snapshot locking and cancellation plus empty-byte glob matching in KEYS, HSCAN, and SSCAN. Tested: WSL Rust 1.95 workspace clippy with warnings and unwrap denied; cargo test -p raft (106 passed); cargo test -p storage glob_tests (10 passed); cargo test -p cmd (107 passed); Windows and WSL cargo fmt --check; git diff --check. Not-tested: Full workspace cargo test, Python integration suite, and remote CI. Co-authored-by: OmX <omx@oh-my-codex.dev>
| Ok(()) | ||
| } | ||
|
|
||
| async fn assert_pending_after_one_poll<F: Future>(mut future: Pin<&mut F>, message: &str) { |
| Ok(()) | ||
| } | ||
|
|
||
| async fn assert_pending_after_one_poll<F: Future>(mut future: Pin<&mut F>, message: &str) { |
Restore Redis-compatible initial-empty-string handling in the generic glob matcher while retaining the command-level exact single-star special case. Update KEYS, HSCAN, and SSCAN tests to distinguish '*' from repeated-star patterns. Constraint: Preserve the snapshot coordination fixes, public storage APIs, and all non-empty glob behavior. Rejected: Updating the existing storage integration tests to accept repeated-star matches, because Redis 8.4 source and a live Redis comparison show that behavior is incompatible. Confidence: high Scope-risk: Empty key, hash field, and set member filtering for KEYS, HSCAN, and SSCAN patterns only. Tested: Live Redis comparison for '*', '**'; WSL Rust 1.95 full workspace cargo test; workspace clippy with warnings and unwrap denied; six focused glob and command tests; cargo fmt --check; git diff --check. Not-tested: Local address sanitizer instrumentation and remote CI rerun. Co-authored-by: OmX <omx@oh-my-codex.dev>
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.
Description
Closes #349
本 PR 完成方案 A：删除没有第二生产后端价值的通用
Engine伪抽象，明确 RocksDB 是主数据与 Raft 日志的唯一持久化后端，并收口 DB 所有权、shutdown、snapshot 热切换和迁移失败边界。主要改动：
src/enginecrate、Enginetrait、RocksdbEngine一对一转发层以及 workspace 依赖。Redis、RocksBatch、RocksdbLogStore直接使用具体rocksdb::DB/Arc<DB>；保留具有真实双实现的Batch抽象。FILE_FLAG_BACKUP_SEMANTICS，不再静默 no-op。Type of Change
Checklist
main(or appropriate feature branch)cargo +stable test --workspace --all-features)cargo +stable clippy --all-features --workspace -- -D warnings -D clippy::unwrap_used)cargo +stable fmt --all -- --check)Testing
WSL / Ubuntu：
cargo +stable fmt --all -- --checkcargo +stable clippy --all-features --workspace -- -D warnings -D clippy::unwrap_usedRUST_TEST_THREADS=1 cargo +stable test --workspace --all-featurescargo +stable build --bin kiwitests/run_python_integration.sh：55 passedWindows：
cargo +stable fmt --all -- --checkCreateFileW(GENERIC_WRITE, FILE_FLAG_BACKUP_SEMANTICS)+FlushFileBuffers目录同步探针成功dlltool.exe无法启动其导入库子工具；CI 的 Windows job 继续作为正式构建/测试门禁最新主干组合验证：
origin/main@85eaf27的 synthetic merge tree：2d52cf42717614b4de304dc8a3242b872bd22b80main后的实际 tree 与该 synthetic tree 完全一致Additional Context
.restore_temp_*staging 目录。旧名称没有目标 DB 归属信息，不能安全自动删除；本 PR 不加入可能误删其他实例/用户目录的启发式清理，后续若自动清理应使用带 target manifest 的新格式。AI assistance: code / tests / docs
Human verification: maintainer review required before merge
Summary by CodeRabbit
New Features
Bug Fixes
Tests