[two-headBoy]

What problem does this PR solve?

Issue Number: Fixes #1038

Problem Summary:
When _rq is full, push_rq() sleeps the worker thread and retries. The
owner worker is the only consumer of _rq (other workers only steal), so
once every worker spins there, no one drains any queue and the whole
scheduler livelocks.

What is changed and the side effects?

Changed:
When _rq is full, pop one pending task and run it in place, then retry
the push. The pop frees one slot, which guarantees the push of the task
being scheduled out succeeds, since _rq has a single producer and
stealers never push; the original push is then retried until the queue
has room. The task being scheduled out is saved with current_tid()
(remained-callbacks re-enter push_rq() on the next task's stack).
Pthread-mode tasks cannot be switched out, so they fall back to waiting,
and a pthread-mode task popped by the main task is put back instead of
being silently dropped.

Verified with a recursive spawner repro (every task spawns more tasks until stopped):
before the fix all workers froze in the usleep loop with CPU ~0%;
after the fix workers kept draining the queues (CPU ~400%) and the echo_c++ smoke test passes.

// Repro for brpc issue #1038: all local run queues full -> livelock.
#include <bthread/bthread.h>
#include <stdio.h>
#include <time.h>
#include <unistd.h>

static void* spawner(void*) {
    while (true) {
        bthread_t tid;
        // The spawned task is itself a spawner: a worker that steals it
        // is "converted" into a producer.
        bthread_start_background(&tid, nullptr, spawner, nullptr);
    }
    return nullptr;
}

int main() {
    // The concurrency floor is BTHREAD_MIN_CONCURRENCY (=3+epoll, 4 on
    // Linux); setting 1 fails with EINVAL. Use the floor and seed every
    // worker with one spawner.
    const int n = BTHREAD_MIN_CONCURRENCY;
    bthread_setconcurrency(n);
    for (int i = 0; i < n; ++i) {
        bthread_t tid;
        bthread_start_background(&tid, nullptr, spawner, nullptr);
    }
    fprintf(stderr, "spawner x%d started, waiting for livelock...\n", n);
    time_t start = time(nullptr);
    while (true) {
        sleep(1);
        fprintf(stderr, "main alive, %lds elapsed\n", time(nullptr) - start);
    }
    return 0;
}

Side effects:

• Performance effects: none on the hot path; only runs when the queue is full.
• Breaking backward compatibility: none.

Note: this fix guarantees scheduler liveness, not per-task completion
deadlines; rate limiting under sustained overload remains the
application's responsibility. The window where every worker is inside a
pthread-mode task when its queue fills is left as-is, consistent with
the existing stance for pthread-mode tasks in exchange().

---

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 https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md.

[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

4 open issues

• _rq.push(...) is called without checking whether it succeeded. In this function we’re explicitly… · New
• _rq.push(...) is called without checking whether it succeeded. In this function we’re explicitly… · New
• This enqueues current_tid() onto _rq before the current task has actually been scheduled out… · New
• This enqueues current_tid() onto _rq before the current task has actually been scheduled out… · New

What changed in this PR

Addresses a scheduler livelock when all TaskGroup local runqueues are full by making progress (running a queued task inline) instead of sleeping/retrying forever under full-queue conditions.

Changes:

• Replace the “sleep and retry” behavior on _rq full with “pop one task, run it in place, then retry push”.
• Add special handling for pthread-mode tasks to avoid illegal switches and prevent accidentally dropping popped pthread tasks.

File	Description
src/​bthread/​task_group_inl.h	Changes TaskGroup::push_rq() to drain work when _rq is full, preventing livelock and handling pthread-mode edge cases.

_{Review effort: Lite}

[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

2 open issues

• This branch makes no progress toward freeing space for the original tid enqueue: it pops one item… · New
• _rq.push() appears to be a fallible operation (it’s checked elsewhere in this function). Ignoring… · New

4 resolved since last review

• This enqueues current_tid() onto _rq before the current task has actually been scheduled out…
• This enqueues current_tid() onto _rq before the current task has actually been scheduled out…
• _rq.push(...) is called without checking whether it succeeded. In this function we’re explicitly…
• _rq.push(...) is called without checking whether it succeeded. In this function we’re explicitly…

_{Review effort: Lite}

[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

4 open issues

• The scan for a non-pthread task is capped at 16 entries. If there are >16 consecutive pthread-mode… · New
• The scan for a non-pthread task is capped at 16 entries. If there are >16 consecutive pthread-mode… · New
• Under BTHREAD_FAIR_WSQ, pop_task() uses _rq.steal() rather than an owner-side pop. If… · New
• This code path is explicitly a resource-exhaustion/overload path (full runqueue). A CHECK(pushed)… · New

2 resolved since last review

• This branch makes no progress toward freeing space for the original tid enqueue: it pops one item…
• _rq.push() appears to be a fallible operation (it’s checked elsewhere in this function). Ignoring…

_{Review effort: Lite}

[wwbmmm]

[replied by brpc-oncall robot] Deferring the publication of the outgoing task through set_remained() fixes the publish-before-switch race, but the remaining implementation still has two blockers: sched_to() can be re-entered from remained callbacks (where _sched_recursive_guard is 1, so non-NDEBUG builds abort), and after the switch the current task may have been stolen, so this is no longer the running TaskGroup. Please also add a regression test under test/.

[wwbmmm]

[replied by brpc-oncall robot] The new pthread-task scanning and the CHECK on the re-queue are fine, but the two blockers from the previous review are untouched: sched_to() can still be re-entered from within a remained callback (debug builds abort), and after the switch the current task may have been stolen, so this is no longer the running TaskGroup. A regression test is still missing.

[two-headBoy]

Thanks for the detailed reviews. The latest five commits address the identified issues:

• _sched_recursive_guard is now maintained in all build types and remains held throughout remained-callback execution, preventing recursivesched_to() calls.
• Whenpush_rq() hits a full queue inside a remained callback, it makes room without switching context and defers the popped task until the guard is balanced. The deferred task is requeued from both the fresh-stack and resumed-stack paths.
• The pop result is now stored in a temporary and saved as pending only after a successful pop, so a pop/steal race cannot leave a stolen TID in_pending_rq_tid .
• After a context switch,push_rq() uses theTaskGroup returned throughsched_to() , andready_to_run() re-fetches the current group before updating nosignal accounting or signalling, avoiding stale-group access after stealing.
• Addedtest/bthread_runqueue_unittest.cpp , covering runqueue saturation with a single worker and multi-worker execution with yields and stealing. The test checks completion, start failures, and exactly-once execution of all tasks.
  The regression test passes.

cc @wwbmmm

[wwbmmm]

I think we should be very careful to merge this PR. It change the core behavior of bthread. This issue exists for a long time but no one succeed to fix it before.
When you encounter this "_rq is full" issue, you should first try to increase your worker count or queue size.

[wwbmmm]

@chenBright What do you think about this PR?

[chenBright]

I think we should be very careful to merge this PR. It change the core behavior of bthread. This issue exists for a long time but no one succeed to fix it before. When you encounter this "_rq is full" issue, you should first try to increase your worker count or queue size.

Agree. Making push_rq() a potential context-switching point changes a fundamental assumption across its callers, including bthread_start_background() and internal wakeup paths.

[chenBright]

push_rq() can now suspend the caller and resume it on another worker. Updating the local pointer in ready_to_run() does not update its callers: after butex_wake_except() wakes many joiners, task_runner() still uses its old g to install _release_last_context and call ending_sched().

[chenBright]

This switch makes bthread_start_background() an implicit scheduling point. Existing callers, including ExecutionQueueBase::start_execute(), deliberately use background submission because they may hold a pthread mutex.

[two-headBoy]

Thanks for the reviews. I agree the root problem is the design: making push_rq() an implicit scheduling point breaks existing assumptions of bthread_start_background() and its callers. The follow-up fixes only address individual consequences of this change.

I’ll abandon this implementation and close this PR.