Skip to content

Synchronize SAVEPOINT replay cancellation before releasing the mock RPC #973

Description

@apstndb

Problem and evidence

The existing TestSavepointReplayCancellationCleansUpWithoutCommit intermittently lets successful reconstruction beat cancellation. This failed CI job reports reconstruction error = <nil>, want savepoint reconstruction failed; transaction ended at line 850 on commit 0f7522b1bd9c832b65ca9a94bbfb7270402351dc.

The test cancels the command and immediately closes releaseSQL, making the mock's successful-response path available before the cancelled context necessarily reaches its RPC handler. The test and production replay code were unchanged by the DirectedRead PR that encountered this failure. The observed failure and source ordering justify fixing the test synchronization; a later passing run does not eliminate the race.

Bounded implementation

  • Confine changes to this cancellation test and the smallest necessary existing heartbeat/RPC test-harness seam. Start from current main; use a separate feature branch and one PR.
  • Keep the reconstructed candidate SQL blocked while cancellation is being exercised. Wait for a concrete cancellation/result signal before allowing any successful-response release; reuse the existing context-aware blocking hook if sufficient. Ensure cleanup also unblocks the mock on test failure.
  • Preserve the point at which a distinct candidate transaction ID has been assigned. Keep assertions for the cancelled cause, ended logical owner, cleared recovery state, rollback of the candidate and absence of Commit.
  • Do not add sleeps, retries of the test assertion, swallowed errors, weaker expected results, or production behavior changes merely to make this test pass. If investigation identifies a production defect, report that separately with evidence before broadening this PR.
  • Distinct DDL cancellation draft Enter DDL wait loop before canceling RPC test #947 is outside this change; do not take over or modify it.

Acceptance and validation

  • Explain the original cancel-versus-success schedule and how the updated synchronization excludes it without weakening the cleanup contract.
  • Run go test ./internal/mycli -run '^TestSavepointReplayCancellationCleansUpWithoutCommit$' -count=50 and a focused race run with -race -count=10. Capture output to files and check the actual exit codes.
  • Run make check before any push and make check-race for the final change; all applicable CI checks must pass before acceptance. Preserve the 80% coverage floor.
  • Keep the fix small, retain independent exact-HEAD review, and link the PR with Fixes for this Issue.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions