Repository navigation
raftengine: test live config persistence and bootstrap guards - #1346
Conversation
Signed-off-by: bootjp <contact@bootjp.me>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
github-actions
Bot
added
the
agent-review-approved
Signed-off-by: bootjp <contact@bootjp.me>
github-actions
Bot
removed
the
agent-review-approved
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (0904fae):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
github-actions
Bot
added
the
agent-review-approved
|
CI follow-up complete on 0904fae. The existing #1347 snapshot-phase fix is incorporated through a signed merge. The original failed Ubuntu job is superseded by passing run https://github.com/bootjp/elastickv/actions/runs/40670552848; encryption off/on Jepsen, CodeQL, lint, build, TLA+ and DCO also passed. The post-push review and security review both completed with no findings on this exact HEAD. No member, snapshot/restore requirement, payload or workload was removed. #1347 remains the dependency; CI must be checked again after retargeting to main. |
github-actions
Bot
removed
the
agent-review-approved
Design: docs/design/2026_10_28_proposed_validation_lint_baseline.md ## Purpose Restore the **unfiltered** repository lint baseline required by the test-cleanup design, and make one native-snapshot acceptance wait honor its existing phase deadline. This is a separate validation prerequisite, not a claim that #1330 or all unfinished designs are complete. The design was committed before implementation and independently reviewed. It covers all eight findings on main `64fc9e62`: four complexity reports, two duplicate bodies, one unused-parameter report, and one nested-test complexity report. Removing the first unused argument revealed a second always-nil wrapper argument; all five callers were audited before removing both. General routed publication still accepts real effect/read-key arguments. ## Changes - Preserve main's migration of the memory dispatcher to `kvtest.LocalCoordinator`; keep the added timestamp-boundary, copy, unsupported-operation and OCC regressions on that shared path. The obsolete local helpers are not restored. - Extract journaled FUSE writes and share unlink/rmdir lifecycle handling. Operation-specific capabilities remain independent; session/capability checks precede operation-ID allocation. Admission, refresh, empty writes, receipts, unknown-outcome classification, and logging retain their order and errno behavior. - Share the existing zero-or-mismatched attempt predicate in journal planning/publication/rejection. Publication and duplicate-resolution ordering is unchanged. - Split the raw verdict predicate into protocol categories with the exact prior sentinel set/evaluation order and readiness exclusion priority. - Preserve all 24 migration-control protection cases and every assertion while extracting their case body. - Replace the separate five-second snapshot wait with a poll bounded by the **existing 60-second write/snapshot phase**. All actual members, including the learner, still need persisted native snapshots covering the ACKed write before restart; restored bytes and PREPARE evidence assertions are unchanged. No production wire/on-disk format, timestamp issuance, Raft settings, authorization, retry policy, or default flags change. No suppression, new exclusion, shorter workload, or test skip is added. PR #1344's accounting trace and workflow changes are not duplicated or incorporated; its changes affect different journal bodies from this refactor. ## Verification - **Unfiltered golangci-lint v2.15.0: PASS, 0 issues** with `GOMAXPROCS=4 golangci-lint run ./... --timeout=10m`. No `--new-from-rev` filter. The same command failed with the eight findings before implementation. - Focused new parity tests passed before and after their refactors: FUSE partial capabilities/admission/session/unknown-outcome behavior; dispatcher copy/boundary/OCC behavior; attempt identity; raw verdict allowlist/exclusion. The extracted migration case set retains all 24 names/assertions. - `GOMAXPROCS=4 go test -p 2 -race ./adapter -run '^TestQuorumAcceptanceSnapshotWait|^TestMigrationQuorumPartitionAcceptance$' -count=2 -timeout=5m`: PASS (74.843s), including actual native snapshot, restart, restored bytes, and resolution-evidence requirements. - Full filesystem/KV/root race: PASS (exit 0), `GOMAXPROCS=4 go test -p 2 -race ./internal/filesystem/... ./kv/... . -count=1 -timeout=20m`. Filesystem 103.569s, fstest 1.822s, fuseadapter 11.005s, KV 69.721s, kvtest 1.781s, root 104.345s. No short/skip flags are used. Kernel FUSE checks require Linux CI rather than macOS local execution. - An earlier worker full filesystem attempt failed in `TestHandleContentionIsTerminalAndReplayable` with deadline/unknown-outcome errors. Its cause was not established; a later parent full filesystem run passed. The earlier failure is not counted as passing evidence. - `git diff --check`: PASS. ### Snapshot Regression Evidence PR #1346's Go CI failed `TestMigrationQuorumPartitionAcceptance` at the five-second wait for all four persisted native snapshots. Three isolated two-CPU race repetitions of that unchanged test passed (90.317s). That does not establish a production regression or identify the physical cause of the CI delay. The new focused regression makes the final member's snapshot coverage arrive after 5.25 seconds, within its phase context. With the old five-second sub-budget it **failed at 5.00s**, showing target 22 and snapshots `[22 22 22 21]`. It passes when the helper uses the caller's existing phase deadline. Separate cases reject partial/empty coverage, canceled phases, and deadline expiration. This changes neither the real acceptance phase's 60-second budget nor its payload, member count, snapshot threshold, or restored-data assertions. The helper reports target and last observed snapshot indexes on failure. The test remains red if actual native snapshots do not cover the write within the phase. It does not substitute applied indexes or a stub for the real four-member acceptance case. ## Conflict Resolution Against Current Main Merged main `75daeeb800ee9b661e5dfd3d9b91fb656928c145` without rewriting existing history. Final HEAD: `3d2d0b436c8d953031713460bf2d1e229c149eae` (bootjp author/committer, `contact@bootjp.me`, signature Verified and DCO sign-off). - Resolve four conflicted files: preserve main's shared coordinator migration and exact-size bootstrap fixture, retain this PR's explicit cap-error and durable peer-metadata assertions, and keep exactly one compile-time applied-index interface assertion. - Adapt the new memory-dispatcher regressions to main's existing invalid-request sentinel, prefix validation order, nested error wrapping and empty-slice copy semantics. No production decision/error contract is changed by this resolution; all dispatcher/service callers were checked. - Include only mechanical lint fixes for shared-helper result declarations and import grouping exposed by the main merge. No workload, timeout, assertion or exclusion was removed. - Independent read-only conflict/FUSE integration review: no findings. - Targeted conflict-area race tests: PASS (fstest, kvtest, fsmwire, live Raft config persistence). - Full filesystem/KV/Raft/fsmwire/root race command: PASS, exit 0 (`GOMAXPROCS=4 go test -p 2 -race ./internal/filesystem/... ./kv/... ./internal/raftengine/... ./internal/encryption/fsmwire . -count=1 -timeout=20m`). Filesystem 101.625s, fuseadapter 10.540s, KV 72.379s, etcd 54.961s, root 108.109s. No explicit short/skip filters; kernel-FUSE checks require Linux CI. - Affected DynamoDB/S3 helper-binding tests plus the real four-member snapshot/restart acceptance and snapshot-wait regressions: race PASS, exit 0 (34.184s). - Initial unfiltered lint on the main merge found four new reports; mechanical fixes were applied. Final unfiltered lint, without auto-fix or a differential filter: PASS, 0 issues (`GOMAXPROCS=4 golangci-lint run ./... --timeout=10m`, exit 0). A subsequent commit hook also passed with no remaining edits. - `git diff --check`: PASS. GitHub reports MERGEABLE. Fresh post-push review and CI are pending; old-head reviews/CI are historical only. ## Risk and Self-review 1. Data loss: journal effect/OCC ordering, receipt handling, and raw error classification are unchanged; native snapshot and restored-byte checks are retained. 2. Concurrency / distributed failures: dispatcher serialization and FUSE mutation-ticket lifetime are preserved. The snapshot poll uses the original phase cancellation/deadline and verifies every member. Full race results and their limits are above. 3. Performance: helpers share existing work rather than add network/consensus operations or new per-key scans. The polling allocation is test-only. 4. Data consistency: all prior verdict sentinels and exclusion precedence, attempt identity checks, mutation copies, timestamp patch bounds, and all migration-control combinations are explicitly pinned. 5. Test coverage: behavior parity was exercised before and after refactoring; the wait regression was observed red before the fix. Unfiltered full lint is distinguished from differential CI. Linux kernel acceptance and remote CI are not claimed complete yet. Related: #1330, #1345, #1346. Earlier PR heads do not retroactively gain this clean baseline; they need to incorporate it and re-run latest-HEAD review/CI. Author: bootjp <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **不具合修正** * 分散環境でのスナップショット確認を、書き込み内容が全メンバーに反映されるまで待つよう変更しました。待機中に期限切れやキャンセルが発生した場合は、状況を示すエラーを返します。 * ファイル操作やデータ移行時の検証を強化し、未対応の操作や不正な入力による予期しない適用を防ぎます。 * **ドキュメント** * 検証基準と受け入れ条件に関する提案書を追加しました。 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Design: docs/design/2026_10_16_proposed_test_suite_cleanup.md
Scope
Implement WP-3: retarget Raft/encryption tests to live code and remove only the resulting unused wrapper and redundant tests. No production behavior, wire format, or persistence format changes.
persistConfigState, using actual temporary data directories and explicit voter metadata.SaveSnapbeforeRelease/completed-publication assertions. Also verify durable peer metadata and the cached config index, so skipping the whole operation cannot satisfy the test.persistConfigSnapshot, which had only those test callers; retain the live payload helper.AppliedIndexWriterassertion.Opensnapshot-index regression and append/layout/allocation regression.This is WP-3 only, not completion of the whole test-cleanup design or #1330. It is independent of WP-1 (#1345).
Depends on #1347 (base:
design/validation-lint-baseline). The signed merge0904faefd257b247ef4bed3b3032efce2cfc0a0dincorporates its lint-baseline and snapshot-wait fixes without rewriting existing commits. The diff against that dependency stays limited to the five WP-3 files. After #1347 merges, retarget this PR to main and recheck CI before merging.Verification
TestKeystore_Concurrent; that failure and interrupted attempts are not passing evidence. The final full run below includes that test without skipping it and passed.golangci-lint v2.15.0 run ./internal/raftengine/... ./internal/encryption/... --timeout=10m).GOMAXPROCS=4 go test -p 2 -race ./internal/raftengine/... ./internal/encryption/... . -count=1 -timeout=15m. Raft etcd 64.204s, transportsoak 3.357s, raftenginetest 1.982s, encryption 379.242s, encryptiontest 1.450s, fsmwire 1.133s, kek 1.457s, root 171.892s. No explicit short/skip filters were used;TestKeystore_Concurrentwas included. Linux kernel-FUSE acceptance is remote-only.golangci-lint v2.15.0 run ./... --new-from-rev=64fc9e62 --timeout=10m.0904faef: unfiltered whole-repository lint PASS, zero issues (GOMAXPROCS=4 golangci-lint run ./... --timeout=10m, exit 0). Dependency validation: restore unfiltered lint and snapshot phase checks #1347 resolves the eight earlier baseline findings. No differential filter or new exclusions were used.git diff --check: PASS.01ec486846db3d38dbb70e13aa69adc6c928a56d(code receipt, security receipt). Remote Jepsen encryption off/on passed. Full Go CI failed in unchangedTestMigrationQuorumPartitionAcceptance: the five-second wait for all four persisted native snapshots did not complete. Three isolated two-CPU race repetitions passed (90.317s), which does not erase the CI failure or establish its physical cause. The validation: restore unfiltered lint and snapshot phase checks #1347 fix is now incorporated in the signed merge head above: polling uses the existing 60-second write/snapshot phase context, still requiring actual persisted native snapshots on every voter and learner plus the original restored-byte and resolution-evidence assertions. Timeout diagnostics report the target and last observed snapshot indexes. The updated HEAD's passing review/CI receipts are recorded below; the older reviews/CI are historical only.Mutation Evidence
Mutations use temporary
go test -overlaycopies only; no broken production code is committed.persistConfigStateTestPersistConfigStateSkipsSerializationWhenNewerSnapshotExistscount > maxBootstrapBatchCountTestBootstrap_RejectsOverCapBatchCount/TestBootstrap_RejectsHugeBatchCountThe initial config mutation also encountered a fixture mismatch between an unspecified suffrage and its persisted
voternormalization. That is not counted as valid evidence. After fixing the fixture, the unmodified test passes and mutation fails on the actual config-index assertion.snapshotPayloadLockedretains a second serialization guard; the test does not falsely claim that serialization now occurs under this mutation.CI Follow-up
The original failure was a five-second snapshot wait at line 700, not a failure of the retargeted configuration-state assertions. The CI log does not identify why a particular runner missed that window; no production Raft or storage defect is asserted from this timeout alone.
The dependency includes a red/green regression where final-member coverage arrives after 5.25 seconds within its phase deadline. With the old sub-budget it fails at five seconds; with phase-bounded waiting it passes. It also tests cancellation, expiry, partial and empty coverage. No member, payload, snapshot or restore requirement is removed.
Integrated head
0904faef:GOMAXPROCS=4 go test -p 2 -race ./adapter -run '^TestQuorumAcceptanceSnapshotWait|^TestMigrationQuorumPartitionAcceptance$' -count=2 -timeout=5mPASS (57.339s, exit 0). This includes two actual four-member snapshot/restart acceptance runs, not only the synthetic timing regression. Independent standards/spec reviews found no new issues.Remote latest-HEAD verification on
0904faefd257b247ef4bed3b3032efce2cfc0a0d: all reported CI checks passed. Ubuntu full Go/race/gRPC CI, Jepsen encryption off/on, CodeQL, lint, build, TLA+ and DCO all succeeded. Post-push SHA-bound code and security review completed with no findings or unresolved findings. This supersedes the old01ec4868failure/reviews without erasing them. #1347 remains the base dependency; after it merges and this PR retargets main, recheck CI before merging.Risk and Self-review
SaveSnap/Releaseorder and completion checks.Author: bootjp