fix(tests): make replay and shard read setup deterministic#26051
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
changed the title
XuPeng-SH
changed the title
7 tasks
XuPeng-SH
pushed a commit
that referenced
this pull request
Jul 23, 2026
…#26063) ## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #26045 ## What this PR does / why we need it: `TestReadValidatesAllRemoteShardsBeforeSending` was added with #25872 to protect a specific ordering contract: when a binary prepared-parameter read targets multiple remote shards, every selected target must pass commit compatibility validation before the first RPC is sent. The observed failure was initially reported as a remote-read timeout, but no remote read had started: the counting client remained at zero `AsyncSend` calls. The roughly 60-second delay came from the test setup. It created a real three-CN shard topology through asynchronous heartbeat, allocation, rebalance, and read-cache refresh. If allocation started before all CNs were visible, replicas first accumulated on one CN; subsequent balancing could wait for the one-minute CN freeze interval. The test then chose an incompatible target from asynchronously maintained state, while `Read` could observe a different target ordering and return after the first incompatible shard, leaving only one `Adjust` observation. #26051 improved that setup by waiting for all CNs and separating setup/read contexts. This PR narrows the test further to the contract it owns: - install two single-replica remote shards directly in the read cache; - make the first target compatible and the second target incompatible; - keep the real `service.Read`, replica selection, cluster compatibility lookup, and counting RPC client; - require both shard adjustments, the exact incompatibility error, and zero `AsyncSend` calls. This deliberately does not increase timeouts, add retries, or change production behavior. Scheduler, heartbeat, rebalance, and real remote-handler behavior remain covered by their owning tests, including `TestBinaryReadUsesVersionedRemoteHandler`; they are not prerequisites for this preflight-ordering regression. A regression that validates and sends in the same loop will send after the first compatible shard and fail the zero-send assertion. Validation after the final edit: - focused regression: `-count=100` passed in 4.407s; - focused race regression: `-race -count=20` passed; - full `./pkg/shardservice` package passed in 17.895s; - `go build ./pkg/shardservice` passed; - `go vet ./pkg/shardservice` passed; - `git diff --check` passed. For comparison, current `main` with the asynchronous setup passed 100 repetitions locally but took 109.544s. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.
What type of PR is this?
What this PR does / why we need it
This PR fixes two independent nondeterministic test setups without relaxing production behavior.
TAE checkpoint replay
TestAppendAndGC2 created its database and tables through low-level TAE catalog APIs. Those APIs mutate the in-memory catalog but do not append rows to mo_database, mo_tables, and mo_columns. After a forced checkpoint truncates the DDL WAL, checkpoint replay can therefore lose the still-live database.
The prepared-DML replay fence added by #26027 exposed this invalid setup as ExpectedEOB. The previous version of this PR skipped missing catalog entries, but diagnostics proved the missing IDs were the live test database and table. That production-code relaxation was removed.
The test now uses the full DDL helpers and, after restart, opens both relations and scans every column to verify that each still contains all 99 rows.
Shard read compatibility test
TestReadValidatesAllRemoteShardsBeforeSending could create its table before all three CNs reported to the shard server. In the failed Ubuntu job, all shards were initially allocated to cn1 and balancing the second shard took 60 seconds. The shared setup context expires after 10 seconds, so the first remote compatibility check returned context deadline exceeded after one Adjust call instead of reaching the intended incompatible CN.
The test now waits for every CN before creating shards, bounds every replica wait, uses separate setup and read contexts, and asserts the exact incompatibility error. This keeps the test fast and prevents unrelated timeout errors from satisfying require.Error.
No production code is changed.
Validation