-
Notifications
You must be signed in to change notification settings - Fork 0
Open
Description
Problem
As identified in PR #136 code review, using a boolean busy flag instead of a counter creates a race condition when BridgeCore multiplexes multiple requests on a single worker.
Current Behavior
interface WorkerProcess {
busy: boolean; // ← Boolean flag
// ...
}
private async sendToWorker<T>(worker: WorkerProcess, payload): Promise<T> {
worker.busy = true;
// ...
try {
return await worker.core.send<T>(payload);
} finally {
worker.busy = false; // ← Sets to false even if other requests in flight
}
}Race Condition Scenario:
- Request A:
busy = true - Request B (multiplexed on same worker):
busy = true(already true, no change) - Request A completes:
busy = false← Bug: B still in flight - Cleanup sees
!busyand may terminate worker with B in flight
Proposed Fix
Replace boolean with counter:
interface WorkerProcess {
inFlightRequests: number; // ← Counter
// ...
}
private async sendToWorker<T>(worker: WorkerProcess, payload): Promise<T> {
worker.inFlightRequests++;
// ...
try {
return await worker.core.send<T>(payload);
} finally {
worker.inFlightRequests--;
}
}
// In cleanup:
const idleWorkers = this.processPool.filter(
w => w.inFlightRequests === 0 && /* other conditions */
);Impact
Low in practice because:
- Idle cleanup protected: Single-worker mode (default) has
minProcesses=1, soprocessPool.length > minProcessesprevents idle termination of the only worker - Overused cleanup rare: Requires 1000+ requests AND the race timing
- Multiplexing requires concurrent calls: Sequential usage never triggers this
However, this is a correctness bug that should be fixed for multi-worker pools with high concurrency.
References
- PR refactor: unify NodeBridge and OptimizedNodeBridge (ADR-0001) #136 CodeRabbit review comment
- Related to ADR-0001 bridge unification
Metadata
Metadata
Assignees
Labels
No labels