Repository navigation
test: stabilize deadline and initial PubSub assertions - #1236
Conversation
TestShadowAndCutoverAllocatorsSerializeMigrationOnGroupZero took its two HLC ceilings from two separate time.Now() calls. When those calls straddle a millisecond boundary, legacyClock's ceiling lands 1 ms above tsoClock's, and the test fails with tso shadow validation: reserve local TSO window: tso minimum N with batch 1 rolls to physical P beyond committed ceiling P-1 HLC.fencedNowMillis returns the ceiling as "now" whenever the ceiling is in the future, so every timestamp legacyClock issues sits exactly at legacyClock's ceiling. The shadow path hands that value to the group-0 allocator as a minimum, and rejectMinimumWindowBeyondCeiling correctly refuses a minimum above tsoClock's committed ceiling. The rejection is right; the fixture was wrong. Production wires one *HLC -- coordinate.Clock() -- into both the dedicated allocator and the legacy path (main.go configureDedicatedCoordinatorTSO), so the two ceilings are the same value by construction and cannot drift. Taking both from one sample makes the test model that. Measured: ~15-22 failures per 20000 runs before, 0 after. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check タイトルはテストの安定化という変更の一部を示しています。ただし、主な変更であるTSOシャドウ移行テストのHLC ceiling統一を明示せず、変更に含まれない「initial PubSub assertions」に言及しています。変更内容との関連性は部分的にあります。
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Create stacked PR
- Commit on current branch
-
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 @coderabbitai help to get the list of available commands.
TestDistributionServerGetTimestamp_LeaderRoutedRejectsLegacyServer asserted require.ErrorIs(err, context.DeadlineExceeded) against a server that never answers. Which error comes back is a timing detail: if the context expires before the RPC reaches gRPC the error IS context.DeadlineExceeded, but if the RPC is already in flight gRPC returns a status error with codes.DeadlineExceeded instead -- and a gRPC status error does not satisfy errors.Is(context.DeadlineExceeded). The test's subject is "a legacy server makes the routed allocator time out", which both representations express, so requireDeadlineExceeded accepts either and nothing weaker. Measured: 12 failures per 400 runs before (and it took down test (ubuntu-latest) on an unrelated branch), 0 per 400 after. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
bootjp
changed the title
|
Added a second, independent flake in 58853c3 — same class (a timing-dependent assertion taking down CI on unrelated branches), found the same way.
The test's subject is "a legacy server makes the routed allocator time out", which both representations express equally, so Measured on
Both flakes in this PR are the same underlying mistake — asserting one of several equally-correct observable outcomes — and neither changes production code. |
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. |
|
Claude finished @bootjp's task in 1m 57s —— View job Review: second flake fix (58853c3)
Both commits are sound, test-only, and match the class of bug described (asserting one of several equally-valid observable outcomes instead of the actual invariant under test).
This is a real narrowing, not a weakening: any other error (e.g., connection refused, a different status code) still fails the test loudly via Minor, non-blocking observations:
I was not able to run No changes requested. |
|
Codex Review: Didn't find any major issues. Swish! 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". |
TestRunLiveBackupRenewalFailureCancelsStreamAndReleasesPin set a 9ms effective TTL, so the renewal timer fired at 3ms with 6ms of slack before the pin deadline. Both routes raise ErrLiveBackupRenewal, but only one calls RenewBackup: liveBackupRenewalAttemptTimeout refuses with "renewal deadline has expired" BEFORE the RPC once the remaining TTL is gone. A scheduling hiccup therefore took the deadline route and the test failed on renewCalls=0 while still seeing the error and the warning it asserted. This test is about the RPC-failure route -- it sets renewErr, and the deadline route has its own test -- so the TTL now leaves the timer room to fire first. Measured: 4-5 failures per 400 runs before, 0 per 300 after. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
There was a problem hiding this comment.
🟢 Approval recommended
The test-only fixes are sound; only the PR scope and evidence metadata need updating.
1 open finding
What changed in this PR
Stabilizes three timing-sensitive tests without changing production code.
Changes:
- Shares one HLC ceiling sample across test clocks.
- Adds scheduling headroom to backup renewal testing.
- Accepts both valid deadline error representations.
| File | Description |
|---|---|
kv/tso_raft_test.go |
Removes HLC ceiling skew. |
internal/backup/live_producer_test.go |
Increases renewal timing margin. |
adapter/distribution_server_test.go |
Handles both deadline error forms. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bootjp
changed the title
Ported from #1236 (live_producer_test.go hunk only) to unblock this PR's CI: TestRunLiveBackupRenewalFailureCancelsStreamAndReleasesPin failed on test (ubuntu-latest) with renewCalls=0. At a 9 ms TTL the renewal timer fires at 3 ms, and a scheduling delay past the 9 ms deadline makes liveBackupRenewalAttemptTimeout refuse before RenewBackup is called, so the test takes the deadline route it does not cover. A 300 ms TTL keeps the RPC-failure route the test asserts. The change no-ops once #1236 lands on main. Reproduced locally under CPU load (1 failure in 400 runs before, 0 in 400 after, go test -race). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz
Design: docs/design/2026_09_26_proposed_positioning_and_roadmap.md ## Summary Follow-up to #1267 (merged as `95889d1f`) and #1276 (merged as `184351ae`). The docs merged with #1256 said `elastickv-snapshot-decode` could not read a live-store snapshot, and named #1267's metadata-key fix as the cure. Review on this PR showed #1267 removed only the first failure, so the decoder fix went to #1276. With #1276 merged, this change records what the two PRs fixed and what remains. It scopes the decoder to one group's DynamoDB, S3, Redis, and SQS keyspaces, and says a dump is not a complete restore source. - **`CONTEXT.md` "Backup and snapshots":** a scope statement only, pointing at the roadmap's §4 row. It says: - the decoder reads the `fsm-snap/<index>.fsm` file a node persists as well as encoder output, and dumps that one group's four adapter keyspaces; - encrypted entries are not decodable offline; - a dump can omit committed data: an unresolved secondary lock's write, a value staged by an in-flight range migration, and keyspaces outside the four adapters such as gRPC RawKV and filesystem keys; - a dump ignores storage-level expiry; - a dump reflects the snapshot's applied index, not the Raft log after it; - dumps of several groups do not share a point in time. - **Roadmap §3 inventory:** the same scope, pointing at §4. - **Roadmap §4 row, what each PR fixed:** - #1267 skips the reserved metadata keys. - #1276 strips the FSM header and verifies the CRC32C footer. - #1276 keeps the newest version of each key from MVCC history. - #1276 accepts `EKVSSTI1` and the stored-key budget, and parses hex `%016x.fsm` names. - `TestRun_DecodesOnDiskFSMFile` pins the on-disk path. - **Roadmap §4 row, open limits, each with its code path:** - encrypted entries (`ErrSnapshotEncryptedEntry`); - unresolved secondary locks (`!txn` records dropped); - staged migration values (`getAtWithStagedVisibility` versus the `!dist` drop); - the gRPC and filesystem keyspaces (`dispatcher.route` counts them as `Unknown`); - storage-level expiry (`dispatcher.handleEntry` drops `ExpireAt`); - the Raft log tail and a cross-group point in time, both non-goals of the decoder design and Phase 1 logical backup's job. - **Roadmap §6.1:** the README backup section may describe the decoder for one group's four adapter keyspaces. It must name the limits the §4 row keeps open and say a dump is not a complete restore source. - **Audit A0 status:** the decoder-rejection defect is closed for cleartext stores. #1267 and #1276 are credited, and the paragraph points at the §4 row. Besides the docs, there is one test-only hunk ported from #1236 in 5d39cc3 to unblock CI: `internal/backup/live_producer_test.go` raises `TestRunLiveBackupRenewalFailureCancelsStreamAndReleasesPin`'s TTL from 9 ms to 300 ms. It becomes a no-op once #1236 lands. a83e602 merges `main` into this branch, so the reviewed tree carries #1276's code. ## Behavior change None. The change is documentation plus one test fixture value. ## Risk None beyond a stale statement, which this change removes. The ported hunk is test-only and identical to #1236's. Every symbol and test the §4 row cites exists on `main` at `184351ae` and in this branch's tree. ## Test evidence - Docs are Markdown only. `git diff -U0 | grep '^+[+-] '` shows no stray diff markers. - Every row in the roadmap's §4 table has three cells. Names that contain a pipe (`!txn|`, `!dist|`, `!fs|`) are written without it, so the row does not split. - Each commit was fetched back and diffed against the local worktree: identical. - Checked on `main` at `184351ae`: - `ReadSnapshotFile`, `latestVersionFilter`, `DecodeSnapshot`, and `SnapshotIndexFromPath` (hex parse at `internal/backup/source.go:49`). - `TestRun_DecodesOnDiskFSMFile`, `ErrSnapshotEncryptedEntry`, and `TestReadSnapshot_RejectsEncryptedEntry`. - The `!txn|` and `!dist|` internal-drop routes, and `dispatcher.route`'s `Unknown` path. - `dispatcher.handleEntry` dropping `SnapshotEntry.ExpireAt` (`internal/backup/snapshot_reader.go:529`, `internal/backup/decode.go:297`), with the store's `PutWithTTLAt` / `ExpireAt` and `kv/fsm_migration_import.go:127` as the writers. - `getAtWithStagedVisibility` and `distribution.MigrationStagedDataKey` in `kv/shard_store.go`. - The unprefixed gRPC raw keys (`adapter/grpc.go`) and the `!fs|` prefixes (`internal/fskeys`). - The decoder design's non-goals: the Raft log tail and the cross-group point in time. - The unresolved-secondary limit, against `kv/lock_resolver_test.go`. - Ported test hunk, run as `go test -race -count=400` under CPU load: - 1 failure before the change and 0 after; - `go test -race ./internal/backup/` passes; - lint reports 0 issues. - No Jepsen runs apply. ## Review rounds - **Codex P2 (cleartext-only):** addressed in bd7d7df / b445803, then superseded by e97663f / f8968bd and 68cfb1d / 4d5c8ae. Resolved. - **CodeRabbit (`CONTEXT.md` glossary-only):** fixed in 1d8c950. Two later Minors, which asked to remove the scope statement, were answered as scope rather than mechanism; CodeRabbit withdrew both and recorded learnings. Resolved. - **Codex P1 / P2 (MVCC versions, SST-ingest, FSM header and CRC footer, stored-key budget):** recorded as open in e97663f / f8968bd, fixed in code by #1276, and recorded as closed in 68cfb1d / 4d5c8ae. Resolved. - **Codex P1 (unresolved secondary locks in the summaries):** fixed in 563b01d. Resolved. - **Codex P2 (`SnapshotIndexFromPath` still parses decimal):** the branch predated #1276; a83e602 merges `main`. Resolved. - **Codex P1 ×3 (staged migration values, unsupported keyspaces, cross-group point in time):** fixed in ba6b729. Resolved. - **Codex P2 ×2 (storage-level expiry, Raft log tail):** fixed in 0ccc0d4. §6.1 now says "the limits" rather than "every limit". Resolved. - **Reviews on 0ccc0d4:** Codex found nothing in its code and security reviews; CodeRabbit generated no actionable comments. ## CI On ba6b729, `test (ubuntu-latest)` failed in two tests this PR does not touch: - The backup renewal test: #1236's fix is ported in 5d39cc3. - `TestRedis_LuaXAddRecreatesTTLExpiredString`: `NOTLEADER` during leader loss in its 3-node cluster. It passed on the rerun. On 0ccc0d4, `test (encryption=on)` failed once in the filesystem Jepsen setup with `reg-0 (File exists)`. `RawFileSystem.Create` returns `EEXIST` for a non-exclusive `O_CREAT` that races another mount; that is existing FUSE behaviour outside this docs PR, so no fix is carried here. It passed on the rerun. 0ccc0d4 is green on every job: build, lint, three test jobs, and CodeQL. ## Self-review lenses 1. **Data loss:** no production code change. The docs list the ways a dump can be incomplete or wrong without an error, so an operator does not treat a dump as a full restore source. 2. **Concurrency / distributed failures:** no production code change. The docs now name the 2PC secondary-resolution window, the staged-visibility window, the log tail, and cross-group consistency. The ported test change only widens a fixture TTL. 3. **Performance:** no production code change. The ported test runs about 100 ms instead of a few milliseconds. 4. **Data consistency:** `CONTEXT.md`, the roadmap (§3, §4, §6.1), and the audit's A0 paragraph agree with `main` at `184351ae`. 5. **Test coverage:** the §4 row names the tests that pin the on-disk path and the encrypted-entry limit. The ported hunk makes an existing test deterministic under load. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **ドキュメント** * 保存済みスナップショットから、単一RaftグループのDynamoDB、S3、Redis、SQSのデータをダンプできることを明記しました。 * 暗号化されたエントリーはオフラインでデコードできないこと、未解決のロックや移行中データなど一部の内容はダンプに含まれないことを説明しました。 * ダンプは適用済みインデックス時点の内容であり、複数グループ間で同一時点のデータを示すものではないことを追記しました。 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
bootjp
changed the title
|
Updated against current main and removed overlapping TSO/backup changes that main already implements. The remaining diff contains the RPC deadline representation assertion and the initial PubSub acknowledgment wait. The PubSub race caused the unrelated proxy failure in PR #1289. A 100 ms server-side processing delay reproduced the old assertion's exact @codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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. |
Summary
Stabilize two response-timing assertions without changing production behavior:
context.DeadlineExceededor gRPCcodes.DeadlineExceededin the leader-routed timestamp test. The representation depends on whether the deadline expires before or during the RPC.Existing Work and Scope
Updated to current main. The previous TSO fixture-ceiling and backup-renewal changes already have equivalent implementations on main and no longer appear in this PR's diff. Only the two test files above remain. The PubSub correction is collected here rather than creating a duplicate test-stabilization PR.
Validation
expected: 1 / actual: 0; acknowledgment verification passed 10/10 times under the same delay. The temporary delay was removed.go test -race ./proxy -count=1passed.golangci-lint run ./proxy/... ./adapter/... --timeout=5mpassed with Go 1.26.6 (0 issues).Author: bootjp