mirror of
https://github.com/dtzp555-max/olp.git
synced 2026-07-21 21:15:10 +00:00
feat+ci: D38 — maxConcurrent runtime enforcement (issue #1)
ADR 0002 Amendment 1 declared `hints.maxConcurrent` declarative-only at
v0.1 — type-validated at startup but with no runtime enforcement.
D38 wires the runtime enforcement via a per-provider in-flight spawn
counter with immediate-advancement on saturation.
Design choice: **immediate-advancement via fallback engine** (queue +
timeout DEFERRED). When a provider is at its maxConcurrent limit, the
spawn call synchronously fails with a new `CONCURRENCY_LIMIT` error
code; the fallback engine treats this as a hard trigger and advances
to the next chain hop. If the entire chain is saturated, the user
sees a chain-exhausted error (existing path).
Rationale for immediate-advancement over queue+timeout:
1. The fallback chain exists precisely for this kind of overflow —
adding a queue layer would duplicate the advancement semantics.
2. Queue + timeout adds new config surface (timeout duration, queue
depth bounds, queue eviction policy) that isn't needed at the
personal/family scale OLP serves.
3. Head-of-line blocking risk: a long-running spawn would stall
queued requests behind it even though other providers in the chain
could serve them immediately.
4. Fail-fast latency aligns with the multi-provider proxy philosophy
("spread risk across providers, not within a provider").
Queue + timeout is deferred to a v1.x design ADR if real usage shows
demand. ADR 0002 Amendment 6 and ADR 0004 Amendment 4 capture the
decision explicitly.
Changes (8 files, +<delta>):
**Code**
1. **lib/providers/base.mjs** — add `CONCURRENCY_LIMIT` to
`PROVIDER_ERROR_CODES`. JSDoc clarifies the code is synthesised by
the orchestration layer, not thrown by provider plugins themselves.
2. **lib/providers/index.mjs** — new semaphore primitives:
- `tryAcquireSpawn(providerName, maxConcurrent)` — atomic
check-then-increment. Returns `true` on success, `false` if at
limit. Atomicity rests on the JS single-threaded invariant
(read + write synchronous, NO `await` between them); module-level
comment block warns future maintainers against breaking this.
- `releaseSpawn(providerName)` — decrement; throws on
under-decrement (defensive bug guard for missing acquire / double
release). Map.delete at zero for clean memory footprint.
- `getActiveSpawnCount(providerName)` — returns current count
(0 for unseen providers). For diagnostics + tests.
- `DEFAULT_MAX_CONCURRENT_SPAWNS = 4` — defense-in-depth fallback
matching the v0.1 plugin defaults (anthropic/codex/mistral all
declare hints.maxConcurrent: 4). Also coerces non-integer / NaN
/ negative inputs to the default.
- `__resetSpawnCounters()` — internal test seam.
3. **lib/fallback/engine.mjs** — `CONCURRENCY_LIMIT: true` added to
`HARD_TRIGGER_CODES`. `classifyTrigger` and `evaluateHardTriggers`
both pick it up via the same lookup. v0.1 live hard-trigger codes
are now 5 (was 4 post-D34): SPAWN_FAILED, CLI_NOT_FOUND,
AUTH_MISSING:false, SPAWN_TIMEOUT, CONCURRENCY_LIMIT.
4. **server.mjs** — gate the spawn call in handleChatCompletions at
BOTH spawn call sites:
- **Buffered path** (executeHopFn → collectAllChunks): acquire
before provider.spawn; on failure synthesise
`ProviderError(CONCURRENCY_LIMIT)` with providerName /
maxConcurrent / activeSpawns diagnostic fields and throw —
fallback engine catches and advances. On success, outer try/
finally wraps the inner D16 truncation-salvage try/catch so
releaseSpawn fires on EVERY exit path (return, D16 salvage
return, re-throw).
- **Streaming path** (single-hop real-SSE branch): acquire BEFORE
the streaming branch entry. If acquire fails, branch is skipped
and request falls through to buffered path (whose own gate
surfaces chain-exhausted for single-hop chains). If acquire
succeeds, existing streaming try/catch gains
`finally { releaseSpawn(streamProvider) }` — slot releases on
stop-chunk completion, generator exhaustion, abort, or any
exception path. `releaseSpawn` fires at END of stream
consumption, not when spawn() returns.
**Tests** (test-features.mjs): 431 → 447 (+16):
Suite 18 — D38 — maxConcurrent runtime enforcement:
- 18a: PROVIDER_ERROR_CODES.CONCURRENCY_LIMIT exists
- 18b: evaluateHardTriggers true for CONCURRENCY_LIMIT
- 18c: AUTH_MISSING regression guard (D38 did NOT flip it to hard)
- 18d.1-18d.4: semaphore unit (increment / saturate-no-increment /
release / map-delete-at-zero)
- 18e: tryAcquireSpawn returns false without side-effect when at limit
- 18f: releaseSpawn throws on under-decrement
- 18g.1-18g.2: DEFAULT_MAX_CONCURRENT_SPAWNS applied for invalid
inputs (undefined / NaN / negative / non-integer)
- 18h: __resetSpawnCounters clears state
- 18i: HTTP integration — 5 concurrent requests to single-hop
chain w/ maxConcurrent=2 → peak in-flight exactly 2, 2 succeed,
3 fail
- 18j: counter releases after buffered request (sequential test)
- 18k: 2-hop chain — saturated primary advances to fallback
- 18l: streaming counter releases at END of stream (not at spawn)
**ADR amendments**
5. **docs/adr/0002-plugin-architecture.md** — Amendment 6 added.
Removes "Declarative hint only at v0.1" caveat from the
`maxConcurrent` description in the Provider contract section.
Adds implementation reference + design-choice rationale (4 points).
Lists all exported symbols.
6. **docs/adr/0004-fallback-engine.md** — Amendment 4 added.
CONCURRENCY_LIMIT added to v0.1 hard-trigger taxonomy. Documents
synthesis-vs-plugin-thrown distinction. Documents first-chunk
safety (acquire before any res.write). v1.x re-evaluation triggers
named.
**CHANGELOG**
7. **CHANGELOG.md** — Unreleased sentinel replaced with proper D38
entry. Per CLAUDE.md release_kit phase_rolling_mode, this lands
under Unreleased; promotion to ## v0.1.1 happens at the v0.1.1
release. The D37 phase_rolling_mode gate now correctly fires if
anyone tags v0.1.x with this content present without promotion —
intentional.
Pre-commit fold-ins (per evidence-first checkpoint #4):
- **Reviewer Suggestion #1 (test 18c name mismatch)**: 18c is named
"classifyTrigger returns hard for CONCURRENCY_LIMIT" but body tests
AUTH_MISSING regression. Renamed header comment to
"AUTH_MISSING regression guard" and clarified that
CONCURRENCY_LIMIT classification is covered by 18b.
- **Reviewer Suggestion #3 (activeSpawns diagnostic field)**: server
.mjs:532 set `concurrencyErr.activeSpawns = maxConcurrent` (the
limit). Technically correct (since acquire just failed, live
count == limit) but confuses future readers. Changed to query
`getActiveSpawnCount(hopProvider)` directly. Added import of the
new symbol to the lib/providers/index.mjs import block.
- **Reviewer Suggestion #5 (ADR overstatement)**: ADR 0002 Amendment 6
said getActiveSpawnCount is "exported for /health, diagnostics,
and tests" but /health integration is not wired at D38. Reworded to
"exported for diagnostics and tests" + "/health integration
deferred — when surfaced there will land at providers.status.<name>
.activeSpawns; not wired at D38." Avoids overstating current state.
Two reviewer suggestions not folded:
- Suggestion #2 (streaming test 18l comment about branch entry
conditions) — low priority; the test passes and the branch is
taken (verified by reviewer). Future polish if confusion arises.
- Suggestion #4 (additional test for streaming-saturation → chain-
exhausted) — code path is straightforward and intentional; 18i
covers the buffered-path version directly. Defer.
Authority:
- ADR 0002 Amendment 1 (declarative-only caveat) — superseded by
Amendment 6
- ADR 0002 Amendment 6 (this commit) — runtime enforcement landed
- ADR 0004 Amendment 4 (this commit) — CONCURRENCY_LIMIT added to
v0.1 hard-trigger taxonomy
- GitHub issue #1 — closed by this commit
- CC 开发铁律 v1.6 § 10.x — independent fresh-context reviewer
- CLAUDE.md release_kit_overlay phase_rolling_mode — no version
bump; Unreleased entry written
Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus,
independent of drafter): APPROVE. Critical depth checks:
- Atomicity in tryAcquireSpawn (lines 285-290 read+set with no
intervening await) — confirmed; module-level invariant comment
warns future maintainers
- Release on every exit path: buffered (outer finally wraps inner
try/catch covering D16 salvage return + normal return + re-throw);
streaming (try/finally covers stop-chunk return + loop exhaustion +
catch paths). No double-release path identified.
- Streaming release timing: fires in finally after res.end() at all
exit paths, never before stream consumption completes
- Counter leak: every code path traced — no orphan acquire identified
- 447/447 tests pass in reviewer's independent npm test run
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
+105
-33
@@ -29,7 +29,16 @@ import {
|
||||
generateRequestId,
|
||||
SSE_DONE,
|
||||
} from './lib/ir/ir-to-openai.mjs';
|
||||
import { loadProviders, listAllProviderNames, getAliasMap, getModelCreated } from './lib/providers/index.mjs';
|
||||
import {
|
||||
loadProviders,
|
||||
listAllProviderNames,
|
||||
getAliasMap,
|
||||
getModelCreated,
|
||||
tryAcquireSpawn,
|
||||
releaseSpawn,
|
||||
getActiveSpawnCount,
|
||||
DEFAULT_MAX_CONCURRENT_SPAWNS,
|
||||
} from './lib/providers/index.mjs';
|
||||
import { ProviderError } from './lib/providers/base.mjs';
|
||||
import { computeCacheKey, hasCacheControl, extractCacheControlMarkers } from './lib/cache/keys.mjs';
|
||||
import { CacheStore } from './lib/cache/store.mjs';
|
||||
@@ -503,43 +512,80 @@ async function handleChatCompletions(req, res) {
|
||||
// conditions item 1 requires no truncation). A non-enumerable __truncated marker
|
||||
// is set on the returned array so executeHopFn can evict the cache entry below.
|
||||
async function collectAllChunks() {
|
||||
// D38 (issue #1): maxConcurrent runtime enforcement.
|
||||
// Authority: ADR 0002 Amendment 6 + ADR 0004 Amendment 4.
|
||||
//
|
||||
// Try to acquire a spawn slot for this provider BEFORE invoking spawn().
|
||||
// If at limit, synthesise ProviderError(CONCURRENCY_LIMIT) which the
|
||||
// fallback engine treats as a hard trigger — the chain advances to the
|
||||
// next hop. validateProvider guarantees hints.maxConcurrent is present;
|
||||
// DEFAULT_MAX_CONCURRENT_SPAWNS is a defense-in-depth fallback.
|
||||
const maxConcurrent = hopProviderPlugin.hints?.maxConcurrent ?? DEFAULT_MAX_CONCURRENT_SPAWNS;
|
||||
if (!tryAcquireSpawn(hopProvider, maxConcurrent)) {
|
||||
const concurrencyErr = new ProviderError(
|
||||
`provider ${hopProvider} at maxConcurrent (${maxConcurrent}) — advancing to next hop`,
|
||||
'CONCURRENCY_LIMIT',
|
||||
);
|
||||
concurrencyErr.providerName = hopProvider;
|
||||
concurrencyErr.maxConcurrent = maxConcurrent;
|
||||
// activeSpawns reflects the live counter at the rejection moment,
|
||||
// queried directly. Since tryAcquireSpawn returned false the value
|
||||
// equals maxConcurrent — read it explicitly for diagnostic clarity
|
||||
// rather than echoing the limit (avoids future-reader confusion).
|
||||
concurrencyErr.activeSpawns = getActiveSpawnCount(hopProvider);
|
||||
throw concurrencyErr;
|
||||
}
|
||||
|
||||
const chunks = [];
|
||||
// try/finally: releaseSpawn MUST fire on every exit path — success
|
||||
// (return at end), spawn throw (caught and re-thrown below), or the
|
||||
// D16 truncation-salvage return. The finally is the only mechanism
|
||||
// that guarantees release across all three.
|
||||
try {
|
||||
for await (const irChunk of hopProviderPlugin.spawn(irReq, authContext)) {
|
||||
// D16: check error chunks BEFORE pushing — preserves the invariant that
|
||||
// chunks array contains only delta/stop chunks. Without this, the catch
|
||||
// block's `chunks.length > 0` would mistake a single error chunk for
|
||||
// "usable content streamed" (Case B) and synthesize a stop + return,
|
||||
// sending an empty body to the client when the correct behavior is to
|
||||
// re-throw and let the fallback engine advance the chain.
|
||||
if (irChunk.type === 'error') {
|
||||
throw new ProviderError(
|
||||
irChunk.error ?? 'Provider emitted error chunk',
|
||||
'SPAWN_FAILED',
|
||||
);
|
||||
try {
|
||||
for await (const irChunk of hopProviderPlugin.spawn(irReq, authContext)) {
|
||||
// D16: check error chunks BEFORE pushing — preserves the invariant that
|
||||
// chunks array contains only delta/stop chunks. Without this, the catch
|
||||
// block's `chunks.length > 0` would mistake a single error chunk for
|
||||
// "usable content streamed" (Case B) and synthesize a stop + return,
|
||||
// sending an empty body to the client when the correct behavior is to
|
||||
// re-throw and let the fallback engine advance the chain.
|
||||
if (irChunk.type === 'error') {
|
||||
throw new ProviderError(
|
||||
irChunk.error ?? 'Provider emitted error chunk',
|
||||
'SPAWN_FAILED',
|
||||
);
|
||||
}
|
||||
chunks.push(irChunk);
|
||||
if (irChunk.type === 'stop') break;
|
||||
}
|
||||
chunks.push(irChunk);
|
||||
if (irChunk.type === 'stop') break;
|
||||
}
|
||||
} catch (spawnErr) {
|
||||
if (spawnErr instanceof ProviderError && spawnErr.code === 'SPAWN_FAILED' && chunks.length > 0) {
|
||||
// Case B (ADR 0004 Amendment 1): provider emitted usable chunks then exited
|
||||
// non-zero. Synthesize a truncated stop and surface the partial response.
|
||||
chunks.push({ type: 'stop', finish_reason: 'length' });
|
||||
logEvent('warn', 'spawn_failed_after_usable_chunks', {
|
||||
chunks_count: chunks.length - 1, // exclude the synthesized stop
|
||||
provider: hopProvider,
|
||||
model: hopModel,
|
||||
});
|
||||
// Mark as truncated so the caller can evict this entry from cache.
|
||||
Object.defineProperty(chunks, '__truncated', { value: true, enumerable: false });
|
||||
return chunks;
|
||||
} catch (spawnErr) {
|
||||
if (spawnErr instanceof ProviderError && spawnErr.code === 'SPAWN_FAILED' && chunks.length > 0) {
|
||||
// Case B (ADR 0004 Amendment 1): provider emitted usable chunks then exited
|
||||
// non-zero. Synthesize a truncated stop and surface the partial response.
|
||||
chunks.push({ type: 'stop', finish_reason: 'length' });
|
||||
logEvent('warn', 'spawn_failed_after_usable_chunks', {
|
||||
chunks_count: chunks.length - 1, // exclude the synthesized stop
|
||||
provider: hopProvider,
|
||||
model: hopModel,
|
||||
});
|
||||
// Mark as truncated so the caller can evict this entry from cache.
|
||||
Object.defineProperty(chunks, '__truncated', { value: true, enumerable: false });
|
||||
return chunks;
|
||||
}
|
||||
// Case A (SPAWN_FAILED with no chunks) or any other error: re-throw.
|
||||
// Fallback engine fires hard trigger and advances chain as before.
|
||||
throw spawnErr;
|
||||
}
|
||||
// Case A (SPAWN_FAILED with no chunks) or any other error: re-throw.
|
||||
// Fallback engine fires hard trigger and advances chain as before.
|
||||
throw spawnErr;
|
||||
return chunks;
|
||||
} finally {
|
||||
// D38: spawn lifecycle ended (drain completed, stop chunk received,
|
||||
// SPAWN_FAILED salvage returned, or any unexpected throw). Release
|
||||
// the slot so the next caller can acquire it. Single-threaded JS
|
||||
// guarantees no other caller has incremented this provider's count
|
||||
// between our tryAcquireSpawn() above and this releaseSpawn().
|
||||
releaseSpawn(hopProvider);
|
||||
}
|
||||
return chunks;
|
||||
}
|
||||
|
||||
// D23: cacheable opt-out check (ADR 0002 Amendment 3 + ADR 0005 Amendment 3).
|
||||
@@ -626,7 +672,24 @@ async function handleChatCompletions(req, res) {
|
||||
//
|
||||
// On success: write chunks to res AND cache so subsequent identical requests
|
||||
// hit the burst-replay path.
|
||||
// D38 (issue #1): pre-acquire a concurrency slot for the streaming path so
|
||||
// saturation here behaves identically to saturation on the buffered path.
|
||||
// If acquire fails, fall through (skip this branch) — the buffered path
|
||||
// below also gates via tryAcquireSpawn and will surface a chain-exhausted
|
||||
// error through executeWithFallback (correct behaviour: a single-hop chain
|
||||
// at maxConcurrent has no other hop to advance to).
|
||||
//
|
||||
// Acquired here, released in the `finally` below. The gate intentionally
|
||||
// lives BEFORE the streaming-branch entry check so the buffered fallthrough
|
||||
// can re-attempt acquire from a clean slate.
|
||||
let streamingAcquired = false;
|
||||
if (ir.stream && chain.length === 1 && !bypassCacheForFirstHop && !preCheckHit && cacheableForFirstHop) {
|
||||
const candidatePlugin = loadedProviders.get(chain[0].provider);
|
||||
const candidateMax = candidatePlugin?.hints?.maxConcurrent ?? DEFAULT_MAX_CONCURRENT_SPAWNS;
|
||||
streamingAcquired = candidatePlugin ? tryAcquireSpawn(chain[0].provider, candidateMax) : false;
|
||||
}
|
||||
|
||||
if (ir.stream && chain.length === 1 && !bypassCacheForFirstHop && !preCheckHit && cacheableForFirstHop && streamingAcquired) {
|
||||
const streamProvider = chain[0].provider;
|
||||
const streamModel = chain[0].model;
|
||||
const streamCacheKey = computeCacheKey(streamProvider, streamModel, ir);
|
||||
@@ -634,6 +697,8 @@ async function handleChatCompletions(req, res) {
|
||||
|
||||
if (!streamPlugin) {
|
||||
// Provider disappeared between chain build and here (edge case).
|
||||
// Release the slot we acquired above so the counter stays balanced.
|
||||
releaseSpawn(streamProvider);
|
||||
return sendError(res, 503, `Provider ${streamProvider} is not enabled`, 'no_enabled_provider',
|
||||
olpErrorHeaders({ startMs, model: ir.model }));
|
||||
}
|
||||
@@ -783,6 +848,13 @@ async function handleChatCompletions(req, res) {
|
||||
res.end();
|
||||
}
|
||||
}
|
||||
} finally {
|
||||
// D38 (issue #1): streaming spawn lifecycle ended — drain completed,
|
||||
// stop chunk seen, generator exhausted without stop, or any catch
|
||||
// path returned via res.end(). Release the slot acquired before
|
||||
// entering the streaming branch. The finally fires on every JS exit
|
||||
// path including the `return;` statements inside the try/catch body.
|
||||
releaseSpawn(streamProvider);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user