Skip to content

test(morpc): stabilize slow backend creation test - #28673

Merged
XuPeng-SH merged 2 commits into
matrixorigin:mainfrom
ck89119:issue-28532-main
Sep 11, 2026
Merged

test(morpc): stabilize slow backend creation test#28673
XuPeng-SH merged 2 commits into
matrixorigin:mainfrom
ck89119:issue-28532-main

Conversation

@ck89119

@ck89119 ck89119 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #28532

What this PR does / why we need it:

  • Replace scheduler-dependent sleeps in TestGetBackendWithSlowConnection with explicit factory phase barriers.
  • Verify that getBackend returns ErrBackendCreating while factory I/O is blocked.
  • Wait for the exact backend creation state to publish completion before asserting that the backend is available.
  • Release the blocked factory during cleanup so failure paths cannot hang client shutdown.

Testing

  • .agents/skills/mo-dev/scripts/mo-cgo-test -v -count=1 -timeout=120s -run '^TestGetBackendWithSlowConnection$' ./pkg/common/morpc
  • .agents/skills/mo-dev/scripts/mo-cgo-test -count=100 -timeout=120s -run '^TestGetBackendWithSlowConnection$' ./pkg/common/morpc
  • .agents/skills/mo-dev/scripts/mo-cgo-test -race -count=100 -timeout=240s -run '^TestGetBackendWithSlowConnection$' ./pkg/common/morpc
  • .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=300s ./pkg/common/morpc
  • .agents/skills/mo-dev/scripts/mo-cgo-test -race -count=1 -timeout=600s ./pkg/common/morpc

@mergify mergify Bot added the kind/test-ci label Sep 11, 2026
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Sep 11, 2026
@ck89119
ck89119 marked this pull request as ready for review September 11, 2026 03:27
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 543df36 against base cd8e8d9. No concrete blocker found.

The change replaces scheduler-sensitive 50ms/150ms expectations with an actual blocked-factory phase. It asserts ErrBackendCreating and nil results, observes the exact creation state under c.mu, proves a second lookup returns while the factory is still blocked, then releases the factory and waits for that creation's completion before checking availability. The final successful lookup is important: done is a terminal notification and can also close on failure/invalidation, so waiting alone would not prove publication.

Cleanup releases the factory once before synchronous client Close, including FailNow paths after cleanup registration. The lookup result channel is buffered, so a timeout does not strand its sender waiting on the test. The existing creation coalescing rule admits one factory call per remote generation; no sleep, retry, swallowed error, socket, port or global configuration is added. The test continues exercising production asynchronous creation and lookup locking, without weakening the intended non-blocking property into a timing assumption.

Non-blocking: the initial getBackend call remains synchronous in the test, so a regression that blocks that very first call relies on the package timeout rather than the later 5-second guards. Making that initial call bounded would improve failure diagnostics, but does not create a false pass or regress the current implementation.

Evidence: full one-file diff and exact-head factory, getBackend/admission, creation-terminal and Close ownership source review. Author reports focused repeated/race and package runs; I did not rerun native tests or wait for CI. No production code changes. Head/base were rechecked unchanged during final validation.

@mergify mergify Bot added the queued label Sep 11, 2026
@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-11 05:29 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Checks running · in-place
  • 🚫 Left the queue2026-09-11 05:35 UTC · at 5f29c5b716552622c2025fa9b79c89143c54a91a

This pull request spent 6 minutes 48 seconds in the queue, with no time running CI.

Waiting for any of
  • check-neutral = CI Required
  • check-skipped = CI Required
  • check-success = CI Required
All conditions

Reason

Pull request #28673 has been dequeued

Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.

The author needs to enable "Allow edits from maintainers" on this pull request.

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

@XuPeng-SH
XuPeng-SH merged commit 47d72b1 into matrixorigin:main Sep 11, 2026
21 of 23 checks passed
@mergify mergify Bot added dequeued and removed queued labels Sep 11, 2026
@ck89119
ck89119 deleted the issue-28532-main branch September 11, 2026 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dequeued kind/test-ci size/S Denotes a PR that changes [10,99] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants