Skip to content

Stabilize backup request policy test cleanup - #3564

Merged
chenBright merged 1 commit into
apache:masterfrom
wasphin:fix-channel-backup-policy-test
Sep 27, 2026
Merged

chenBright merged 1 commit into
apache:masterfrom
wasphin:fix-channel-backup-policy-test

Conversation

@wasphin

@wasphin wasphin commented Sep 23, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

The backup request policy test used a fatal assertion while waiting for short
connections to close. On a slow CI runner, the assertion could return from the
helper before StopAndJoin(), leaving the acceptor active while later parameter
combinations continued to run.

What is changed and the side effects?

Changed:

Record a connection-close timeout, stop and join the acceptor, and then report
the failure. Allow up to one second for asynchronous short-connection cleanup
so the test does not enforce a 100 ms timing assumption unrelated to backup
request policy behavior.

Side effects:

  • Performance effects: Test-only change; no production performance impact.

  • Breaking backward compatibility: No.


Check List:

@wasphin
wasphin requested a lite review from Copilot September 23, 2026 14:58

Copilot AI 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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 Low severity

Open (1)
What changed in this PR

This PR stabilizes the TestBackupRequestPolicy unit test by avoiding a fatal assertion while waiting for short connections to close, ensuring the acceptor is always stopped/joined before reporting a failure (especially on slow CI runners).

Changes:

  • Replace a fatal 100ms timeout assertion with a non-fatal timeout flag and post-cleanup expectation.
  • Increase the wait time for short-connection cleanup to 1 second to reduce CI flakiness.
  • Ensure StopAndJoin() runs even when the close-wait times out by breaking out of the parameter loop and reporting afterward.
File Description
test/​brpc_channel_unittest.cpp Makes short-connection cleanup waiting non-fatal and increases timeout to reduce test flakiness while preserving cleanup (StopAndJoin()).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/brpc_channel_unittest.cpp Outdated
@wasphin
wasphin force-pushed the fix-channel-backup-policy-test branch from d4dede8 to 309ecf2 Compare September 25, 2026 14:58
@wwbmmm
wwbmmm requested a lite review from Copilot September 26, 2026 11:25
@wasphin

wasphin commented Sep 26, 2026

Copy link
Copy Markdown
Member Author

A flaky unit-test is fixed by #3567

Copilot AI 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.

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (1)

@chenBright chenBright 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.

LGTM

@chenBright
chenBright merged commit 535d8f8 into apache:master Sep 27, 2026
45 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants