Implementation:
- Add enqueue_task(PToken&, FuncType&&) overload so high-rate callers
can use a dedicated producer token without losing the future-based
return. Mirrors the no-token version: try-enqueue with fast-path
grow on pressure (autoscale pools), block-enqueue otherwise.
Tests (5 new, 87 total):
- tp_enqueue_task_with_ptoken: basic int/string/void futures via ptok
- tp_enqueue_task_ptoken_exception: exception forwarded through future
- tp_enqueue_task_ptoken_multiple_futures: 50 concurrent futures, all
resolved correctly
- tp_construct_scaling_nonpositive_decline_throws: decline_threshold ≤ 0
was the only ScalingConfig validation not covered by an explicit test
- tp_autoscale_fast_path_grow_on_pressure: verifies _maybe_grow_on_pressure
fires (thread_count grows synchronously) when enqueue_work_autoscale
finds the queue full
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- tp_enqueue_task_broken_promise_on_destruction: verifies documented
contract that unfulfilled promises throw broken_promise
- tp_thread_count_matches_threads_size: thread_count() == threads().size()
after each synchronous mutation
- tp_thread_count_settles_after_churn: atomic mirror converges to
threads().size() at rest after add/remove bursts
- tp_try_enqueue_work_for_ptoken_times_out_when_full: ptoken overload
of try_enqueue_work_for was untested for the full-queue timeout path
- tp_tasks_pending_zero_at_construction: sanity check at init
- tp_tasks_pending_zero_after_all_complete: pending drains to 0
- tp_autoscale_verifiable_growth: deterministically asserts thread_count
exceeds min after sustained queue pressure
- tp_autoscale_verifiable_shrink_to_min: threads shed to min_threads
after idle_threshold_usecs
- tp_shutdown_ecanceled: smoke test that racing add/remove/set_threads
against the destructor does not crash or deadlock
- tp_queue_depth_zero_at_idle: queue_depth() == 0 when no work pending
- tp_queue_capacity_unchanged_after_resize: set_threads does not alter
the queue's max depth
- tp_tasks_enqueued_increments_before_completion: tasks_enqueued() leads
tasks_completed() under a blocked worker
All 82 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two doc-only follow-ups to the preceding cleanup series:
- thread_count() is now an atomic-mirror read rather than a locked
read of _threads.size(). Note on the declaration that callers
wanting an exact count after a scaling operation must quiesce the
pool (or use the wait_until pattern from the unit tests).
- add_thread / remove_thread / set_threads now return -ECANCELED
during shutdown (_stop set), where they previously silently
no-oped in some code paths. Extend the class-level shutdown
contract comment so callers treat the non-zero return as
expected, not as a failure.
_maybe_grow_on_pressure and the monitor hill-climber called
thread_count() on every full-queue enqueue and every sample cycle,
which under the hood locked _threads_mutex. Under sustained load
that serialized producers against every scaling operation.
Add _thread_count_atomic, maintained under _threads_mutex at each
push_back/erase site (constructor, _add_thread_scaling_locked,
_try_claim_and_remove_self, _try_remove_self_if_above). Switch
thread_count() to an acquire-load of the mirror. Authoritative
bound enforcement still lives inside _add/_remove_thread_scaling_locked
where _threads.size() is read under the lock, so the approximation
only affects the pre-lock hints that already expect staleness.
Every task completion called _try_claim_and_remove_self, which
acquires _scaling_mutex and _threads_mutex even when _remove_count
is zero (the steady-state common case). Short tasks paid full
mutex overhead per completion. Add a relaxed _remove_count.load
before the locks; if it reads zero, skip the claim entirely. A stale
zero just defers the claim to a later completion on any worker,
matching the existing design where the claim is a cooperative race.
The RAII-guarded locks at function top survive the goto correctly, but
the pattern reads awkwardly — reviewers have to verify that nothing in
the claimed path relies on state that the goto skipped over. Use a
bool flag and a straight-line fall-through instead. No behavior
change.
_add_thread_scaling_locked already bails out with -ECANCELED when
_stop is set. Mirror that check in _remove_thread_scaling_locked so
the monitor cannot enqueue a wakeup poison pill after shutdown begins.
The pill would be benign (workers break on _stop before claiming), but
it would sit in the queue until BoundedQueue is destroyed, and skipping
it also avoids an unnecessary semaphore signal on the teardown path.
Spell out the "producers stop before destruction" requirement at the
class level so callers know that enqueue_task returns a future that
will throw broken_promise, and enqueue_work* variants silently drop
work, if they race the destructor.
The prior relaxed loads defeated the documented "bias toward zero"
invariant on ARM and POWER: the hardware can reorder two relaxed loads
against program order, allowing _tasks_completed to be observed before
_tasks_enqueued and producing a delta larger than any single atomic
snapshot. Upgrade both loads to acquire and put an acquire fence
between them so source-order ordering is preserved on weak memory.
_remove_self and _try_claim_removal had no callers. The CAS loop from
_try_claim_removal was inlined into _try_claim_and_remove_self when
that function was introduced (7ce5364c extracted it, later commits
merged it back under combined lock acquisition), and _remove_self was
superseded by _try_remove_self_if_above / _try_claim_and_remove_self.
The destructor comment referencing _remove_self pointed at a race that
is now closed by those two functions instead.
The comment referenced "line 632" for the pre-lock thread count
read, but prior edits shifted that line. Replace the hard-coded
line number with a positional description ("the earlier unlocked
read") so the comment stays accurate across future edits.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The constructor's spawn loop pushed to _threads without holding
_threads_mutex. A just-spawned autoscale worker could time out on
the empty queue (especially with a small idle_threshold_usecs) and
call _try_remove_self_if_above, which acquires _threads_mutex and
modifies _threads (find + erase). Concurrent unlocked push_back
and locked erase on the same vector is undefined behavior per the
C++ memory model, even when reserve() prevents reallocation.
The same race applied to the _threads.empty() and _threads.size()
reads after the spawn loop.
Fix: lock _threads_mutex around push_back in the spawn loop and
around the post-loop size check.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After a successful _remove_thread_scaling_locked, the monitor set
expected_count via thread_count(), which returns the pre-shrink
count because the removal is deferred (only _remove_count is
incremented; the actual _threads erase happens asynchronously when
a worker claims the removal). This caused two problems:
1. On the next cycle, the deferred removal makes thread_count()
return count-1, which differs from expected_count (count),
triggering the external-scaling-interference detector and
wasting the cycle.
2. If the worker hasn't exited yet, thread_count() still equals
expected_count, so the monitor evaluates throughput with the
pre-shrink thread count still in effect, potentially reversing
direction before the shrink's effect is measurable.
Fix: compute expected_count arithmetically (count-1 for shrink,
count+1 for grow) using the authoritative pre-operation count
read under _scaling_mutex, reflecting the intended outcome rather
than the racy current state.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_remove_thread_scaling_locked floors min_allowed at 1, preventing
remove_thread/set_threads from ever reducing the pool below 1 thread.
But start_routine_autoscale passed raw _scaling_config.min_threads
to _try_remove_self_if_above without the same floor. With
min_threads == 0, all idle workers could self-exit, draining the
pool to zero threads and permanently stalling any work enqueued
via non-autoscale enqueue_work (which doesn't call
_maybe_grow_on_pressure).
Apply the same max(min_threads, 1) floor in start_routine_autoscale
to keep the two shrink paths consistent.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two fixes:
1. Constructor: when no threads can be spawned the constructor throws,
but the destructor never runs for a partially-constructed object.
The pthread_cond_t and pthread_mutex_t initialized earlier in the
constructor body were leaked. Destroy them before throwing.
2. Monitor: the _stop check was outside _monitor_mutex, so the
destructor could set _stop and signal between the check and
pthread_cond_timedwait, losing the wakeup and delaying shutdown
by up to one sample interval (~100ms). Move the _stop check
under _monitor_mutex so it is serialized with the destructor's
signal, closing the race.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous ordering loaded completed before enqueued, which meant a
completion between the two loads would be missed (stale lower c),
inflating the delta and over-reporting pending tasks. This is the
opposite of what the comment claimed. Swapping to load enqueued first
means a completion between the two loads yields a higher c, shrinking
the delta — biasing toward zero, which is the safe direction for
scaling heuristics.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_process_func contained two identical compare_exchange_weak loops for
claiming a _remove_count decrement — one in the poison-pill handler
and one after task completion. Duplicating the loop risks the two
copies diverging during future edits and makes the function harder
to follow.
Extract the loop into _try_claim_removal() which returns true when a
decrement is successfully claimed. Both call sites now delegate to
the shared helper. The relaxed memory ordering rationale (the
subsequent _remove_self acquires _threads_mutex, providing the
happens-before with the scaling-side increment) is documented on the
helper. No functional change.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The PToken overload of enqueue_work_autoscale passed ptok_ to the
blocking fallback enqueue but not to the try_enqueue fast path.
BoundedQueue has a try_enqueue(PToken&, ...) overload, so the token
was silently ignored on the common (non-full) path, forgoing the
per-producer thread-local allocation that moodycamel provides.
Pass ptok_ through to try_enqueue so the fast path benefits from the
same producer-token locality as the fallback path.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The monitor stores _last_scale_time_usecs after _add_thread_scaling_locked
returns (i.e. after pthread_create), but _maybe_grow_on_pressure stored
a timestamp captured before the add. Under resource pressure
pthread_create can take non-trivial time, making the effective cooldown
shorter than configured on the fast-path grow.
Move the store after _add_thread_scaling_locked and use a fresh
_now_usecs() call, matching the monitor's behavior.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pthread_cond_timedwait can return early due to spurious wakeups.
When this happens the throughput delta covers a shorter-than-expected
interval, producing an artificially low sample. Two consecutive
spurious wakes would cause the EMA to decline twice, crossing the
decline_threshold and triggering a false direction reversal — the
monitor would shrink the pool when throughput was actually stable.
Record the wall-clock time before sleeping and discard any sample
where the elapsed time is less than half the target interval. The
_stop check runs first so shutdown is never delayed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
set_threads read _threads.size() under _threads_mutex then read
_remove_count outside the lock. Between the two reads a worker could
claim a _remove_count decrement and erase itself from _threads via
_remove_self, causing set_threads to double-count that shrink: once
from the reduced _threads.size() (had the lock been re-acquired) and
once from the stale _remove_count that was already claimed. This
could cause a spurious over-shrink and a -EINVAL return even though
the pool was at the target size.
Move the _remove_count load inside the _threads_mutex scope so both
values are read atomically with respect to worker self-removal.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tasks_pending() loads _tasks_completed then _tasks_enqueued and
returns the delta. The bias-toward-zero property (safe for scaling
heuristics) depends on completed being observed no later than
enqueued. With relaxed ordering on both loads, a weakly-ordered
CPU (ARM, POWER) can reorder the enqueued load before the completed
load, potentially observing a stale (lower) completed with a fresh
(higher) enqueued — over-reporting pending work.
Use acquire on the first load (_tasks_completed) so that subsequent
loads in program order cannot be reordered before it. On x86 (TSO)
this compiles to the same instruction as relaxed, so there is zero
overhead on the primary target architecture.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_try_remove_self_if_above only held _threads_mutex, while set_threads
and the monitor thread hold _scaling_mutex. An idle worker calling
_try_remove_self_if_above could shrink the pool concurrently with
set_threads reading _threads.size(), causing set_threads to
miscalculate the delta and overshoot (add too many or remove too few
threads).
Fix by acquiring _scaling_mutex before _threads_mutex in
_try_remove_self_if_above. This matches the existing lock ordering
used by _maybe_grow_on_pressure and the monitor/set_threads paths
(_scaling_mutex always acquired before _threads_mutex), so no
deadlock is introduced.
Now all paths that change the thread count — add_thread,
remove_thread, set_threads, monitor hill-climb, pressure grow, and
idle self-exit — are serialized under _scaling_mutex.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The size check and _remove_count increment were not atomic —
_threads_mutex was released between checking _threads.size() and
incrementing _remove_count. A concurrent idle self-exit (which
only holds _threads_mutex, not _scaling_mutex) could shrink the
pool between the two operations, allowing _remove_count to signal
a removal that would take the pool below min_allowed threads.
Fix by holding _threads_mutex across both the size check and the
_remove_count increment. The enqueue_unbounded call is kept
outside the lock since it only needs to wake a worker and does
not need to be atomic with the count check.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The assert guarding against BoundedQueue::try_enqueue moving the
callable on failure is compiled out in release builds (-DNDEBUG).
If the queue implementation ever changed to move-from on failure,
the empty Func would be enqueued as a poison pill, silently causing
a worker thread to exit — a subtle, hard-to-diagnose pool corruption.
Replace both assert() calls with a std::logic_error throw that
remains active in all build configurations. This makes the
invariant violation immediately visible rather than silently
dangerous.
Also remove the now-unused <cassert> include.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In start_routine_autoscale, btp->_scaling_config.min_threads was read
as an argument to _try_remove_self_if_above. If that method returns
true, the calling thread is detached and removed from _threads — at
which point the destructor may run concurrently and destroy the pool
object. Although the dereference technically occurs before entering
the method (and thus before the detach), it happens before the
_threads_mutex lock that serializes with the destructor, making the
ordering fragile.
Snapshot min_threads into a local variable before the call so the
argument evaluation cannot race with pool destruction. This mirrors
the existing pattern of copying _name into a local for the post-detach
syslog call.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_scaling_config was documented as read-only after construction but
nothing prevented accidental mutation. Move its initialization from
the constructor body into the member initializer list (using a
ternary to handle the nullptr case) and mark it const.
_autoscale_enabled cannot be made const because the constructor
clears it if the monitor thread fails to spawn.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These fields are written once in the constructor before any worker or
monitor threads are spawned, and are read-only thereafter. Add an
explicit comment noting the write-once-then-read-only invariant so
future maintainers know that making them mutable would require
synchronization.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The CAS loop on _remove_count uses memory_order_relaxed for both
success and failure, which is correct but non-obvious. Add a comment
explaining that _remove_self acquires _threads_mutex immediately after
the successful CAS, and that mutex acquisition provides the necessary
happens-before edge between the scaling-side increment and the
worker-side erase/detach.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When pthread_create fails (e.g., EAGAIN from OS resource limits),
the monitor and fast-path pressure grow would retry on the next cycle
with no delay, potentially flooding syslog with identical warnings
every 100ms.
Fix: stamp _last_scale_time_usecs on creation failure so the existing
cooldown mechanism (200ms default) suppresses immediate retries. This
reuses the scaling cooldown infrastructure rather than adding a
separate backoff timer.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When throughput dropped to zero, the monitor reset direction and
decline_streak but not warmup_samples. On resumption, the first
meaningful throughput sample was compared against a stale pre-idle
EMA value, which could cause the hill-climber to make a scaling
decision based on a meaningless delta (new throughput vs. zero-decayed
EMA from potentially minutes ago).
Reset warmup_samples to 0 in the idle branch so the EMA is re-seeded
from two fresh samples before the hill-climber acts. This ensures
the first scaling decision after an idle period is based on current
throughput trends rather than stale history.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The monitor thread used nanosleep for its sample interval, which meant
the destructor had to block for up to one full sample period (100ms by
default) waiting for the monitor to wake up and notice _stop.
Replace nanosleep with pthread_cond_timedwait on a dedicated condvar
(_monitor_cond) using CLOCK_MONOTONIC. The destructor now signals
the condvar immediately after setting _stop, so the monitor wakes up
and exits without waiting for the interval to expire.
This also eliminates the EINTR retry loop — pthread_cond_timedwait
handles signals internally and returns when either the timeout expires
or the condvar is signaled.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
set_threads computed its delta from _threads.size(), which does not
reflect _remove_count signals already in flight. If called twice
before workers drained the first batch of removals, the second call
would see the original thread count and issue a full set of redundant
removal signals, potentially shrinking the pool well below the
requested target.
Fix: compute the effective thread count as _threads.size() minus
pending _remove_count, and use that as the baseline for the grow/shrink
delta. This makes set_threads idempotent regardless of drain timing.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The monitor thread checked _last_scale_time_usecs without holding
_scaling_mutex, then called add_thread()/remove_thread() which each
acquired the mutex independently. The fast-path _maybe_grow_on_pressure
could scale between the monitor's cooldown check and its scaling action,
allowing two scaling events closer together than cooldown_usecs.
Fix: hold _scaling_mutex across both the cooldown check and the scaling
action in the monitor loop, using the _locked variants directly. This
ensures the monitor and fast-path pressure grow are mutually exclusive
for the entire check-and-act sequence.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
