Stabilize timing-sensitive bthread tests and fix the butex interruption race

[chenBright]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Running unit tests in parallel exposes timing assumptions that can cause intermittent failures:

• Fixed sleeps do not guarantee that workers have entered the expected waiting state.
• Tight elapsed-time assertions fail under normal scheduling delays.
• An increased connection count does not guarantee that an accepted socket has been published to the connection list.
• Some shutdown paths can lose wakeups or leave worker threads unjoined.
• An interruption between pthread butex waiter registration and the underlying wait can return an incorrect error code.
• Multi-tag tests compare prefilled zero values instead of validating the actual tags.

What is changed and the side effects?

Changed:

• Replace fixed sleeps with explicit synchronization, state observation, and completion acknowledgements.
• Check timeout, interruption, wakeup, and completion semantics instead of narrow scheduling-time windows.
• Retain timeout guards and checks that waits do not complete prematurely.
• Fix the pthread butex interruption race while preserving genuine value-mismatch and timeout errors.
• Wait for accepted sockets to appear in the connection list before checking their buffer settings.
• Improve stop-flag synchronization, thread cleanup, and state reset for repeated test execution.
• Correct the result containers and assertions in multi-tag tests.
• Replace public-network connection dependencies with local TCP listeners and add a Linux-specific connection timeout case.

Side effects:

• Performance effects:

• Breaking backward compatibility:

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

🟡 Changes recommended

Unresolved test-synchronization and cleanup issues remain, including a critical timer-thread cleanup path.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Stabilizes timing-sensitive bthread tests and fixes a pthread-backed butex interruption race.

Changes:

• Replaces fixed sleeps with state polling and completion synchronization.
• Improves cleanup, socket readiness checks, and multi-tag assertions.
• Preserves butex timeout and value-mismatch error semantics.

File summaries

File	Reviewed changes and findings
test/bthread_work_stealing_queue_unittest.cpp	Makes stop-state synchronization atomic and resettable.
test/bthread_unittest.cpp	Observes sleep registration before stopping. moderate (1 vote): Fatal assertion cleanup can leave the worker alive.
test/bthread_timer_thread_unittest.cpp	Adds timer state waits. critical (1 vote): Fatal assertions can skip timer cleanup while callbacks reference stack state. moderate (1 vote): _npending is not reset per round.
test/bthread_rwlock_unittest.cpp	Synchronizes lock-state tests. moderate (1 vote each): Fixed reader delays do not guarantee contention, and non-fatal writer registration checks can allow the ordering test to pass spuriously.
test/bthread_mutex_unittest.cpp	Waits for mutex contention. moderate (1 vote): Fatal timeout cleanup can leave a locked mutex and live bthread.
test/bthread_futex_unittest.cpp	Adds bounded waiter cleanup. moderate (1 vote): Cleanup-induced EWOULDBLOCK results are not accepted when appropriate.
test/bthread_fd_unittest.cpp	Improves interruption and local connection tests. moderate (3 votes): pthread_kill may return ESRCH during the completion race.
test/bthread_cond_unittest.cpp	Resets shared condition-test state.
test/bthread_butex_unittest.cpp	Adds interruption and timing tests. moderate (1 vote each): Synchronization does not establish waiter registration, and a non-fatal timer-registration check can allow the interruption test to pass without validating its precondition.
test/bthread_butex_multi_tag_unittest.cpp	Validates actual task tags.
test/brpc_socket_unittest.cpp	Waits for accepted socket publication before checking settings.
src/bthread/butex.cpp	Fixes pthread butex interruption race handling.

Review details

Suppressed comments (8)

test/bthread_butex_unittest.cpp:258

• The gate only acknowledges execution immediately before butex_wait; it does not establish that the waiter has been registered, nor does it hold the task between registration and futex_wait_private. Consequently bthread_stop can occur before registration or after the wait has started, and 100 repetitions can still pass without exercising the new EWOULDBLOCK-to-EINTR path. Add a deterministic synchronization point around waiter registration and the underlying wait.

        arg->entering_wait->signal();
    }
    timespec ts = butil::milliseconds_from_now(arg->wait_msec);
    int rc = bthread::butex_wait(arg->butex, arg->expected_val,
                               arg->wait_msec < 0 ? nullptr : &ts);

test/bthread_butex_unittest.cpp:449

• EXPECT_TRUE is non-fatal here, so if timer registration never appears the test continues to bthread_stop. For the normal-stack case that stop is then consumed before bthread_usleep, which still produces the expected ESTOP; the test can therefore pass without validating interruption after sleep registration. Record the failure and make it fail after cleanup, or otherwise make this precondition mandatory while preserving cleanup.

                EXPECT_TRUE(sleeping) << "Timed out waiting for sleep registration";

test/bthread_futex_unittest.cpp:124

• The failure cleanup below sets lock1 to 1 to release waiters that had not registered yet. Such a waiter returns -1 with EWOULDBLOCK because the futex value no longer matches, but this helper unconditionally expects rc == 0, so a missed registration produces extra assertion failures and obscures the intended deadline failure. Accept the value-mismatch result only when the cleanup store has occurred.

    EXPECT_EQ(0, rc);

test/bthread_mutex_unittest.cpp:62

• When the deadline expires, ASSERT_EQ returns from the test before unlocking m or joining th1; locker is still blocked on the mutex, so this failure path leaks a live bthread and can contaminate later tests. Use a non-fatal check here so the existing cleanup always runs.

    ASSERT_EQ(257u, state->load(butil::memory_order_relaxed));

test/bthread_rwlock_unittest.cpp:536

• The new timed acquisition still relies on the preceding fixed 50 ms sleep to create a continuous reader load. Under scheduler delay, the writer can acquire before any reader has entered, so the test passes without exercising the starvation scenario it claims to cover. Wait on an explicit reader-ready count (and then keep readers active) before starting the writer.

    // A timed acquisition also makes the failure path reachable when the
    // writer really starves, so readers can be stopped and joined safely.
    timespec deadline = butil::seconds_from_now(10);
    int rc = bthread_rwlock_timedwrlock(&rw, &deadline);
    EXPECT_EQ(0, rc) << "Writer starved under concurrent readers";

test/bthread_rwlock_unittest.cpp:358

• EXPECT_TRUE lets the test proceed when the writer was not observed in writer_wait_count. It then starts the reader and may still pass based on scheduling, so the ordering assertion no longer proves the documented writer-priority race. Make registration a mandatory precondition while retaining cleanup if the wait times out.

    EXPECT_TRUE(WaitForRWLockState([&] {
        return reinterpret_cast<butil::atomic<unsigned>*>(rw.writer_wait_count)
            ->load(butil::memory_order_relaxed) == 1;
    }));

test/bthread_timer_thread_unittest.cpp:342

• _npending is the heap size published by the timer thread and is not reset per round. After round 0 it remains at least kBatch, so on later rounds this predicate is already true before the newly scheduled near-term task is consumed; the test can unschedule the batch while it is still in buckets and pass without exercising the sweep path. Wait on a per-round callback acknowledgement before unscheduling.

        ASSERT_TRUE(WaitUntil([&] {
            return timer_thread._npending.load(butil::memory_order_relaxed) >=
                   static_cast<int64_t>(kBatch);
        }));

test/bthread_unittest.cpp:527

• If the sleep-registration deadline expires, this fatal assertion returns before bthread_stop and bthread_join; the 60-second worker is then left alive, contrary to the cleanup comment and potentially affecting the rest of the test process. This should be non-fatal so the stop/join cleanup still executes.

    ASSERT_TRUE(sleeping);

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[Copilot]

🔵 Needs a closer look

Unresolved moderate test-synchronization and failure-cleanup findings remain.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

test/bthread_mutex_unittest.cpp:62

• ASSERT_EQ returns from the test when the worker does not reach the contended state, so the unlock and join below are skipped and th1 can remain alive while holding a reference to this test's stack state. Use a non-fatal assertion (or an equivalent cleanup guard) here so the failure path still stops/joins the worker.

test/bthread_butex_unittest.cpp:257

• entering_wait is signalled before butex_wait starts, and the test never observes TaskMeta::current_waiter. As a result, bthread_stop often runs before waiter registration, so these iterations do not reliably exercise the registration-to-futex_wait_private interruption race that the production change is intended to cover. Synchronize on current_waiter != nullptr (or add a gate after registration) before stopping the task.

    if (arg->entering_wait) {
        arg->entering_wait->signal();
    }
    timespec ts = butil::milliseconds_from_now(arg->wait_msec);
    int rc = bthread::butex_wait(arg->butex, arg->expected_val,

test/bthread_fd_unittest.cpp:580

• If this new deadline assertion fails, the test returns without closing the fd or joining bth, whose callback still holds &arg and may write to it after the stack frame is gone. Please add failure-path cleanup (or make the wait non-fatal with a safe shutdown path) before asserting the observed waiter state.

    while (meta->current_waiter.load(butil::memory_order_acquire) == nullptr &&
           butil::cpuwide_time_us() < deadline) {
        bthread_usleep(1000);
    }
    ASSERT_NE(nullptr, meta->current_waiter.load(butil::memory_order_acquire));

test/bthread_futex_unittest.cpp:137

• The vector only confirms that pthread_create returned; it does not establish that every worker has entered futex_wait_private. A late worker can reach the wait after the final lock1.store(1), get -1/EWOULDBLOCK instead of the expected zero, and leave nwakeup < N, so this test remains timing-dependent. Add a synchronized waiter-registration count/gate and wait for all workers to register before measuring the wake loop.

    std::vector<pthread_t> threads;
    for (size_t i = 0; i < 1000; ++i) {
        pthread_t th;
        if (pthread_create(&th, nullptr, dummy_waiter, &lock1) != 0) {
            break;
        }
        threads.push_back(th);

test/bthread_timer_thread_unittest.cpp:259

• wait_started() only observes the flag stored before _sleep_ms is loaded and before the callback enters futex_wait_private. The main thread can therefore call keeper5.wakeup() in that window; it clears _sleep_ms, the callback skips the wait, and this test never verifies that a sleeping timer task is actually woken. Publish and await a predicate after the callback has registered the futex wait before calling wakeup().

    ASSERT_TRUE(keeper5.wait_started());

test/bthread_timer_thread_unittest.cpp:357

• wait_finished() observes _finished, which is stored inside TimeKeeper::run() before TimerThread::run() returns from run_and_delete() and publishes _npending. The subsequent load can therefore still read the previous heap size, so this loop does not reliably observe the sweep triggered by the current round. Add an acknowledgement after _npending publication or wait for a specific post-sweep value before sampling.

        if (!consumed.wait_finished()) {

test/bthread_unittest.cpp:527

• On a timeout this fatal assertion exits before bthread_stop(th) and bthread_join(th, nullptr), leaving the 60-second sleeper running after the test has returned. Make the observation non-fatal (or add cleanup on failure) so the new deadline cannot leak a live bthread into subsequent tests.

    ASSERT_TRUE(sleeping);

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[wasphin]

A general note for future PRs: since this project uses squash merges, please try to avoid combining multiple independent changes in a single PR. Keeping each PR focused and atomic makes the resulting commit easier to review, revert, backport, and maintain.

[wasphin]

Two failure paths should preserve cleanup so a timing failure does not leave live workers behind.

[wasphin]

Preserve cleanup when sleep registration is not observed.

[wasphin]

Keep the timer thread shutdown ahead of callback-argument destruction.

[Copilot]

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: 6 · 4

Open (10)

• Asserting strictly ETIMEDOUT can be environment-dependent on Linux. For example, if… · New
• Asserting strictly ETIMEDOUT can be environment-dependent on Linux. For example, if… · New
• Asserting strictly ETIMEDOUT can be environment-dependent on Linux. For example, if… · New
• The test assumes all waiters will be registered and individually woken within a fixed 5s deadline,… · New
• The test assumes all waiters will be registered and individually woken within a fixed 5s deadline,… · New
• This assertion now only checks that the task did not run early, but it no longer bounds how late it… · New
• Variable names like _2s_later, _1s_later, and _10s_later no longer match their actual values… · New
• Variable names like _2s_later, _1s_later, and _10s_later no longer match their actual values… · New
• Variable names like _2s_later, _1s_later, and _10s_later no longer match their actual values… · New
• Since g_stop is now an atomic, prefer using g_stop.store(false, ...) (and corresponding… · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical synchronization and object-lifetime issues, plus cleanup and timeout-test corrections, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Protect interrupted flag check and clear with version_lock · New
• Keep butex alive until all waiter joins complete · New

Resolved since last review (10)

• This assertion now only checks that the task did not run early, but it no longer bounds how late it…
• The test assumes all waiters will be registered and individually woken within a fixed 5s deadline,…
• The test assumes all waiters will be registered and individually woken within a fixed 5s deadline,…
• Asserting strictly ETIMEDOUT can be environment-dependent on Linux. For example, if…
• Asserting strictly ETIMEDOUT can be environment-dependent on Linux. For example, if…
• Asserting strictly ETIMEDOUT can be environment-dependent on Linux. For example, if…
• Since g_stop is now an atomic, prefer using g_stop.store(false, ...) (and corresponding…
• Variable names like _2s_later, _1s_later, and _10s_later no longer match their actual values…
• Variable names like _2s_later, _1s_later, and _10s_later no longer match their actual values…
• Variable names like _2s_later, _1s_later, and _10s_later no longer match their actual values…

[Copilot]

Copilot review overview

🟡 Changes recommended

Several tests do not reliably synchronize on waiter registration, including a requeue case that may block indefinitely.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Ensure condition waiters are registered before notifying · New

Resolved since last review (2)

• Keep butex alive until all waiter joins complete
• Protect interrupted flag check and clear with version_lock

[wwbmmm]

LGTM

[wasphin]

LGTM