After _try_remove_self_if_above detaches and erases the thread, the
destructor's snapshot will not include it — so pthread_join is never
called on it, and the ThreadPool object can be destroyed while this
thread is still returning.
This is safe because the thread only accesses local variables after
detach (name is a std::string copy made at function entry), but the
reasoning is subtle. Add an explicit safety comment explaining why
there is no use-after-free.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The _tl_should_exit mechanism in start_routine (and
start_routine_autoscale) is correct but the control flow is non-obvious:
_remove_thread_scaling_locked enqueues a lambda that sets the
thread-local flag and calls _remove_self (detach + erase), then the
worker checks the flag after running the func and exits.
Replace the terse one-line comments with an explanation of the full
protocol so future readers don't mistake it for dead code.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When throughput dropped below 0.5 (idle), decline_streak was reset but
direction was preserved. When work resumed, the monitor continued in
whatever direction it held before the idle period — often a shrink
direction that was correct for the previous workload but wrong for the
new burst. This could cause the pool to shrink immediately when it
should be growing.
Reset direction to +1 (grow) when entering the idle branch so the pool
starts the next active period biased toward growth, which is the safer
default under new load.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nanosleep returns -1 with errno EINTR when interrupted by a signal.
The remaining time was discarded (second arg was NULL), causing the
monitor to wake early and sample a shortened interval. This produced
artificially low throughput numbers that corrupted the EMA and could
trigger unnecessary scale-down decisions.
Loop on EINTR using the remaining time output parameter so the full
sample interval is always honored. Also check _stop on each retry
so the monitor exits promptly during shutdown.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The CAS on _last_scale_time_usecs was performed before checking whether
the pool was already at max_threads. When the pool was at capacity, the
cooldown window was consumed but no thread was added. This suppressed
legitimate scaling attempts from the monitor thread for the entire
cooldown period.
Move the CAS after the max-threads check and the _scaling_mutex
acquisition so the cooldown is only claimed when a thread will actually
be added. The mutex serializes concurrent callers, and the CAS still
deduplicates concurrent pressure-grow attempts that pass the check
simultaneously.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When auto-scaling is enabled, the constructor did not enforce that the
initial thread_count falls within [min_threads, max_threads]. Starting
with fewer threads than min_threads meant the monitor had to
immediately grow the pool, and starting above max_threads created
threads that the monitor would then immediately try to shrink.
Clamp the count before spawning so the pool starts within its
configured operating range.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
usleep is POSIX-obsolescent and its behavior on signal interruption is
under-specified. Replace with nanosleep which is the current POSIX
standard and has well-defined EINTR semantics. Pre-compute the
timespec once outside the loop since the interval is constant.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The _locked suffix conventionally means the caller already holds the
lock being named. These functions expect _scaling_mutex to be held,
but the old name was ambiguous — it looked like _threads_mutex.
Rename to _add_thread_scaling_locked and _remove_thread_scaling_locked
to make the precondition explicit.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_remove_thread_locked enqueues a self-exit task and blocks on the
future while holding _scaling_mutex. If all workers are busy with
long-running tasks, this blocks indefinitely — starving the monitor
thread (and any concurrent add_thread/set_threads) since they all
contend on _scaling_mutex.
Replace the unbounded wait with a 5-second timed wait. On timeout,
return -EBUSY so callers can retry or report the condition. The
enqueued self-exit task will still be picked up eventually and the
thread will remove itself when a worker becomes free.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The old macro expanded to two statements (mutex_lock + DEFER unlock).
In an unbraced if/else only the lock would be conditional while the
DEFER always executed — a latent double-unlock / unbalanced-unlock bug.
Add ScopedMutexLock to both debug and ndebug headers:
- debug: stores __FILE__/__func__/__LINE__ from the call site and
passes them to _mutex_lock/_mutex_unlock for diagnostics
- ndebug: lightweight wrapper around mutex_lock/mutex_unlock
Redefine mutex_lockguard to create a single uniquely-named
ScopedMutexLock variable (via __COUNTER__), making it safe in any
statement context. Remove the now-unused scope_guard include.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cooldown_usecs was only checked in _maybe_grow_on_pressure (the
fast-path enqueue grow). The monitor thread scaled every
sample_interval_usecs unconditionally, so a recent fast-path grow
could be immediately followed by a monitor grow, defeating the
cooldown.
Check _last_scale_time_usecs against cooldown_usecs before applying
any monitor scaling decision, making the cooldown global across both
scaling paths.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
A single noisy throughput sample would immediately flip the scaling
direction, causing the monitor to oscillate under bursty workloads —
continuously adding and removing threads every sample interval.
Replace raw throughput comparison with an exponential moving average
(alpha=0.3) and require two consecutive declining samples before
reversing direction. This filters out transient dips and keeps the
hill-climber from thrashing.
Also use a 2-sample warmup period to seed the EMA before making any
scaling decisions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
set_threads silently ignored failures from _add_thread_locked (e.g.
pthread_create failure) and _remove_thread_locked (e.g. at min_threads).
The caller had no way to know the pool was undersized or that shrinking
stopped early.
Stop on the first error and return it so callers can detect partial
completion.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Multiple concurrent enqueue_work_autoscale callers could all pass the
cooldown check and each add a thread, overshooting max_threads and
bypassing the intended rate limit.
Use compare_exchange_strong on _last_scale_time_usecs so only one
caller wins the cooldown window. Then hold _scaling_mutex for the
thread-count check and _add_thread_locked call so the grow is atomic
with respect to other scaling operations (monitor, set_threads).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After _try_remove_self_if_above detaches the thread, the destructor
will not join it. If the destructor runs before the detached thread
reaches the syslog call, btp->_name is a dangling reference.
Copy _name into a local string at thread start so the syslog after
self-removal uses stack-local data that outlives the detach.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The file-scope function _tp_now_usecs had a leading underscore
followed by a lowercase letter, which is reserved at file scope
by the C++ standard. Move it to a private static member function
ThreadPool::_now_usecs where leading underscores on member names
are permitted.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ScalingConfig was value-initialized to all zeros when no config
is provided. While the non-autoscale path gates all access, a
zero idle_threshold_usecs would cause try_wait_dequeue_for to
return immediately if the autoscale path were ever reached by
mistake.
Add default member initializers: min_threads=2, max_threads=64,
sample_interval=100ms, cooldown=200ms, idle_threshold=5s. These
match the values recommended in the scaling policy design and
provide safe behavior even if a caller constructs ScalingConfig
without setting all fields.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The counter was incremented before the enqueue call, which can
block indefinitely for bounded queues. If tasks_pending() is used
as a drain-complete signal (pending == 0), the pre-increment
would overcount while a producer is blocked waiting for a slot.
Move the increment to after the enqueue returns in all paths:
enqueue_work, enqueue_work_autoscale, and enqueue_task. The
try_enqueue variants already incremented after success.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The hill-climb monitor and idle self-exit paths could both shrink
the pool past min_threads. The monitor read thread count, released
the lock, then called remove_thread - but between those steps an
idle worker could also self-exit, dropping below the threshold.
Three changes:
- _remove_thread_locked now checks min_threads (from ScalingConfig)
under the same lock as the size check, refusing removal if at or
below the bound.
- Idle self-exit replaced with _try_remove_self_if_above which
atomically checks thread count and erases under a single lock
acquisition, closing the TOCTOU window.
- The previous _remove_self is retained for the non-scaling paths
(poison pill exit during set_threads/remove_thread).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
enqueue_unbounded bypasses the semaphore so control messages
(poison pills, thread removal tasks) can never block on a full
queue. But wait_dequeue always signals a slot after consuming,
so each unbounded item that gets dequeued inflated the semaphore
by +1, permanently raising the effective max queue depth.
Fix by calling tryWait() in enqueue_unbounded before enqueueing.
When the queue has room (common case), this consumes a slot and
the dequeue-side signal restores balance perfectly. When the queue
is genuinely full (tryWait fails), the item still enqueues but
temporarily exceeds max_depth by one until consumed - a transient
overshoot bounded by control messages in flight.
A CAS-based approach that absorbed extra signals on the dequeue
side was also considered but rejected: it can starve blocked
bounded producers by swallowing the signal they are waiting for.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The try-enqueue and fallback block-enqueue both called
std::forward<FuncType>(func_) on the same parameter. If
try_enqueue's internals ever change to move from the argument
before failing, the second forward would operate on a moved-from
object.
Fix by materializing into a Func up front and using std::move
for both attempts. Also extract the duplicated scaling logic into
_maybe_grow_on_pressure(), eliminating the copy-paste between the
PToken and non-PToken overloads.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The destructor and autoscale idle-exit path could race: a worker
calling _remove_self would erase itself from _threads and detach
while the destructor was iterating _threads to enqueue poison pills
and join. This could cause too few poison pills (deadlocking
remaining workers) or pthread_join on a detached thread (UB).
Two changes:
- _remove_self now checks _stop and returns immediately during
shutdown, ensuring no thread detaches after destruction begins.
- Destructor snapshots _threads under the mutex before enqueueing
poison pills and joining, so it operates on a stable copy.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
