OpenConceptLab/ocl_online#275 | Capacity limit on heavy calls (semantic $match, $rerank), in shadow mode - #922
Conversation
…ic $match, $rerank), in shadow mode Counts semantic and reranked $match calls and $rerank calls in flight, in Redis lanes shared by the API tasks: cluster-wide, per task, semantic kNN, per plan tier and per user, with one cluster slot kept for single-row $match calls. Each call takes a lease in every lane that applies in one Lua script, renews it from a background thread and releases it when it finishes; a lease that isn't renewed expires, so a worker that dies frees its slots. - Shadow mode (the default) never refuses. Every gated call writes one JSON log line (decision, tier, lane, in-flight counts, endpoint, rows, held and queue time) for CloudWatch metric filters. - Responses carry X-OCL-Capacity-Decision, -Limit, -In-Flight, -Tier, -Tier-Limit, -Tier-In-Flight and -Suggested-Concurrency, exposed to browsers through CORS. - Enforce mode is built and tested, and ships off: a 429 with Retry-After before any work or quota charge. With enforce_for=aware, only clients that send capacity_aware in X-OCL-Event-Metadata are refused. - The mode and every number are runtime config: staff GET/PATCH/PUT /capacity/config/ or `manage.py capacity`, applied within 10 s, each change kept with who, when, old and new, and logged. - Fail open: if Redis can't be reached, calls go ahead uncounted and it's logged. The limiter uses its own short-timeout Redis client and skips Redis for 30 s after an error. - Tests run the real Lua scripts with fakeredis (CI has no Redis), and pass against Redis 7.0 too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 1 (codex-cli 0.159.2, commit a6431c0)
Posted verbatim; run read-only against git diff origin/master...HEAD.
Significant findings remain, including risks to shadow-mode latency and lease correctness. I ran git diff origin/master...HEAD; no files were modified and no network was used.
-
High —
core/capacity/config.py:155: Config refresh can block a shadow request indefinitely.
get_config()performs a synchronous database query while holding_cache_lock, with no connection/query deadline. Catching an exception only helps after the query returns.
Scenario: PostgreSQL stalls when the cache expires. A$rerankcall that could otherwise run waits for the config lookup until gunicorn kills its worker. Shadow mode has changed the call’s outcome.
Fix: Refresh config outside the request path, serving the last snapshot immediately. Bound database connection/query timeouts and avoid making requests wait for a refresh lock. Test a blocked refresh, not just an immediately raised exception. -
High —
core/capacity/limiter.py:102: Redis timeouts do not bound total fail-open latency.
socket_connect_timeoutdoes not bound synchronous DNS resolution. Sentinel discovery can also visit multiple Sentinels sequentially, each with its own timeout, before connecting to the master. The “at most one short timeout” claim is incorrect. Renewal and release additionally bypassis_available(), so already-admitted calls keep attempting Redis during the circuit-breaker interval.
Scenario: Sentinel DNS stalls or several Sentinels are unreachable. Acquire holds a sync worker for seconds or longer; finishing calls can incur both the renewer join timeout and another release/discovery attempt.
Fix: Put Redis operations behind an overall deadline using a bounded executor, with no unbounded backlog. Explicitly configure Sentinel retries, and make renewal/release respect the breaker while relying on lease expiry for cleanup. Test blocked DNS, multiple unavailable Sentinels, and release during an outage. -
High —
core/capacity/limiter.py:63and:76: A shorter lease can expire an entire shared lane containing longer active leases.
Every acquire/renew unconditionally sets the key TTL to that call’slease_ms * 2. Active gates retain their original config, so runtime changes produce mixed lease durations.
Scenario: A call holds a 60-second lease and renews every 20 seconds. Staff change the config to a 5-second lease with 1-second renewal. A short call acquires and finishes just after the old call renews, leaving the shared key with a 10-second TTL. The key disappears before the old call’s next renewal, deleting its still-valid lease and allowing excess concurrency.
Fix: Derive key expiry from the largest member expiry, or only extend an existing TTL and ensure it covers all members. Add a mixed-config test that advances time between renewals. -
Medium —
core/capacity/limiter.py:307and:366: Failure to start the renewal thread prevents release.
self.reneweris assigned beforestart(). Ifstart()raises, acquire fails open with a lease already held. During release,stop()callsjoin()on the unstarted thread, which raises; the sharedtrythen skipsRedisLanes.release().
Scenario: Thread creation fails under resource pressure. The request proceeds, but all its leases remain until expiry. Repeated failures can exhaust enforce-mode capacity.
Fix: Attempt Redis release in a separatefinally, regardless of renewal-thread cleanup. Track whether the thread started successfully. TestThread.start()failure and verify every lane is cleared. -
Medium —
core/capacity/logs.py:9: Synchronous flushed logging can stall shadow calls.
Every gated call performsprint(..., flush=True)before returning. The exception guard handles a broken output stream, but cannot handle a blocked one. Config-change logging has the same issue.
Scenario: Gunicorn’s captured stdout or its log collector backs up. Completed requests block inrelease(), occupying all sync workers despite Redis being healthy.
Fix: Send records through a bounded, nonblocking logging queue with a defined overflow policy and a separate writer. Test a blocked sink, not only the existingOSErrorcase. -
Medium —
core/capacity/config.py:156: Config failures can still produce enforced refusals.
A failed config read silently returns the last known config, includingmode=enforce. Acquire cannot distinguish a healthy snapshot from this failure and may return 429. This conflicts with the stated requirement that limiter failures allow the request through.
Scenario: Staff turn enforcement off, but a worker cannot refresh its cached enforce config because the database read fails. That worker continues refusing calls using the stale configuration.
Fix: Return config health alongside the snapshot, and force shadow/fail-open admission after a failed refresh until a successful refresh. Test failure with a cached enforce configuration and a full lane. -
Medium —
core/capacity/config.py:190: A config update can commit successfully and then return an error without its required log.
emit()runs after the transaction commits and is unguarded.
Scenario: Stdout raisesBrokenPipeError. The staff PATCH returns 500, although the new version is already active; the required change log is absent. Existing tests mockemit()successfully throughout config saves.
Fix: Make post-commit logging failure unable to change the API result. For guaranteed eventual change logging, use a transactional outbox tied to the config version. Add a test where emission fails after persistence. -
Medium —
core/capacity/config.py:116: Validation permits renewal intervals with effectively no safety margin.
Checking onlyrenew_seconds < lease_secondsaccepts, for example, a 5-second lease renewed every 4 seconds. Scheduling delay and Redis discovery/operation latency can exceed the remaining second.
Scenario: A valid config causes a live call’s leases to expire before renewal. Another acquire removes them and admits additional heavy work. The old gate recordslease_lostbut continues running uncounted.
Fix: Require a substantial renewal margin, accounting for the maximum operation deadline—for example, renewal at no more than one-third of the lease duration. Test delayed renewal and acquisition between expiry and renewal. -
Low —
core/capacity/limiter.py:204: Queue-time parsing mishandles whitespace and trusts client-supplied trace timestamps.
Field names are not stripped, soRoot=...; Self=...ignoresSelfand falls back toRoot. A preserved client Root timestamp can represent trace age rather than arrival at this API, and callers can influence the logged queue time.
Scenario: A trace started several minutes earlier is logged as several minutes of worker queueing, distorting capacity measurements.
Fix: Strip field names/values, validate the trace structure, and distinguish trusted load-balancer arrival timestamps from client trace timestamps. Label the value as an estimate when provenance is uncertain. Add whitespace and spoofed-Root tests.
Things checked and found correct:
- Lua acquisition checks all lanes before inserting any lease; shadow acquisition forces admission. Argument indexing, millisecond units, and Lua truthiness for a returned
ZSCOREare correct. - Redis 7 supports the script’s
TIMEusage with effects replication. - Normal
$matchrefusal occurs beforecheck_and_consume_capability;$rerankrefusal occurs before model construction. - Normal and exceptional view exits release through
finally; DRF-handled exception responses reach the header mixin. - Staff endpoints use
IsAdminUser. PostgreSQL config changes take the advisory transaction lock before reading and merging the previous version. - Numeric validation rejects booleans, negative values, unknown settings, and malformed nested shapes. PUT’s reset-to-default behavior is documented.
- Capacity headers and
Retry-Afterare exposed through CORS. The migration adds a new table without rewriting existing application tables. - Capacity tests default to fakeredis with Lua support, without requiring a Redis service. I found no added credentials or concrete production host/account identifiers.
Verification limitation: Docker access was denied, and local pylint failed before analysis because the read-only sandbox has no writable temporary directory. Tests, pylint, and the 95% coverage threshold therefore remain unverified.
…ew, pass 1 - A lane's TTL is only ever extended, so a shorter lease (after a runtime change) can't expire a lane that holds longer leases. - If the renewer thread fails to start, the leases are still released: stopping the renewer and releasing are separate steps. - Renewal and release skip Redis while it's marked failed (the leases expire), and release no longer waits on a renewal in flight, which can only extend leases the call still holds. - A config that can't be re-read never refuses: a cached enforce config falls back to shadow until a read succeeds. - A config change that's saved no longer fails the request if its log line can't be written. - renew_seconds must be at most a third of lease_seconds, and lease_seconds at least 15. - Sentinel connections are explicitly retry-free; the trace header parser tolerates whitespace. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
|
Response to Codex pass 1, fixed in 626b666:
Capacity tests: 61 pass, on fakeredis and on Redis 7.0. |
…ew, pass 2 - Every Redis operation runs on a per-process Redis thread with an overall deadline (CAPACITY_REDIS_DEADLINE_SECONDS, 1 s), which also bounds DNS and a walk through the Sentinels. An operation still queued when its caller gives up is skipped. - Log lines go through a bounded queue and a writer thread, so a backed-up log sink can't hold a call; when the queue is full a line is dropped, and the next one says how many were. - Renewal is all or nothing: leases are renewed only if the call still holds every one unexpired, so an expired lease is never revived; once a lease is lost the renewer stops. - An error inside acquire never refuses a call: a call that doesn't hold a lease becomes unavailable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 2 (codex-cli 0.159.2, commit 626b666)
Posted verbatim; run read-only against git diff origin/master...HEAD, with pass 1's fixes and declines as context.
-
High —
core/capacity/limiter.py:159–164, 301–305, 364–386: The breaker does not bound outage latency as claimed.
It opens only after an operation fails; there is no atomic probe reservation. A renewer blocked in Redis can overlap with release or a new request, and DNS resolution has no overall deadline. An indefinitely blocked attempt never opens the breaker. With eight sync workers, this can still exhaust API capacity in shadow mode. Existing Django Redis exposure does not make this additional path bounded.Fix: Add a nonblocking, per-process single-flight probe and an overall deadline covering discovery/DNS and Redis execution. Skip counting when either is unavailable. Remove the remaining
join(timeout=0.2)response delay. Test overlapping renewal/release and a connection attempt that never returns. -
Medium —
core/capacity/limiter.py:312–320: An acquisition failure can leave a call refused.
The exception handler preserves an already assigned decision. Ifget_retry_after()raises afterDECISION_REFUSEDis set, the request still returns 429 despite a limiter failure, with no valid Retry-After value. I reproduced this using the actual class with an injected retry-calculation exception:refused=True,retry_after=None.Fix: On acquisition exceptions, unconditionally switch to
DECISION_UNAVAILABLEand clear refusal metadata while retaining any acquired lease for cleanup. Add a view-level regression test asserting work and quota consumption proceed after this failure. -
Medium —
core/capacity/limiter.py:79–84: Renewal resurrects expired leases and can renew only part of a call’s lanes.
ZSCOREchecks membership, not whether the score is still in the future. After a delayed renewal, another user’s acquisition can prune this call from shared lanes while its expired user-lane member remains. Renewal then revives that user lease, reportslease_lost, and continues renewing it. The original call is absent from cluster accounting but can keep subsequent calls from that user refused.Fix: Check expiry against
now_ms, and verify all required leases before renewing any. If a lease is missing or expired, mark the lease lost and stop renewal; do not revive surviving expired members. The current test explicitly expects an expired score of1000to be revived, so it needs changing. -
Medium —
core/capacity/logs.py:9,core/capacity/limiter.py:389–393: Shadow mode still adds an unbounded stdout wait on previously print-free paths.
Catching exceptions does not bound blockingprint(..., flush=True). The declined rationale about existing per-row timing prints does not cover an over-quota$match: previously it returned 403 before search and those prints; now gate cleanup synchronously emits JSON first. A backed-up stdout pipe can hold that response indefinitely.Fix: Emit through a bounded, nonblocking logging queue with an explicit overflow policy. Test a blocked sink, rather than only a sink that raises immediately.
Checked and found correct by source inspection:
keep()prevents shorter leases from shortening shared-key TTLs.- Renewer startup failure now permits release; stop and release failures are handled separately.
- Failed config refresh changes cached enforce mode to shadow.
- Config emit exceptions are guarded after persistence.
- Renewal-margin validation, trace whitespace parsing, staff permissions, PostgreSQL advisory locking, PUT default replacement, and history recording are consistent.
- Admission precedes capability consumption; refusal takes no lease. CORS exposes the new headers.
- Lua admission is atomic, uses Redis time, and has consistent argument indexing. Migration is additive; no concrete secrets or infrastructure host identifiers appeared in the diff.
No files were modified or network used. Docker checks were blocked by socket permissions; Django tests and coverage were not run. Limited capacity-package pylint passed with import errors disabled; git diff --check passed.
|
Response to Codex pass 2, fixed in 570833a:
Capacity tests: 69 pass, on fakeredis and on Redis 7.0. |
…ew, pass 3 - Redis operations run on a daemon thread with a short queue (core/capacity/threads.py) instead of a ThreadPoolExecutor, whose threads the interpreter waits for at exit: an operation stuck in DNS no longer holds up a gunicorn worker's recycle. When the queue is full, a call fails at once. - The log sink tracks lines in flight, and at exit waits for them for 2 s at most, so the last line (e.g. `manage.py capacity set`'s change) isn't lost, and a blocked sink can't hold the exit. The dropped-line count is kept under a lock. - The config refresh on the request path runs with a 500 ms statement timeout (CAPACITY_CONFIG_READ_TIMEOUT_MS), so a locked or stalled table can't hold a call; the last known config then applies, with enforce falling back to shadow. - Tests: process exit with a hung Redis call and with a blocked log sink (subprocesses), the last line printed at exit, concurrent emitters, a full Redis queue, and a locked config table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 3 (codex-cli 0.159.2, commit 570833a)
Posted verbatim; run read-only against git diff origin/master...HEAD, with pass 2's fixes as context.
Significant findings remain in 570833af.
-
High —
core/capacity/config.py:151–164: config refresh can block requests indefinitely, including shadow requests.get_current()runs synchronously while holding_cache_lock. Neither the database connection nor this query has a limiter-specific deadline. The exception fallback only helps once the database returns or raises; the Redis executor deadline does not cover this path.Failure scenario: a transaction holds an exclusive lock on
capacity_configs. As each worker’s cache expires, its next heavy request blocks before matching or reranking. All eight workers can become occupied by the limiter, even though the tables needed by the original requests remain available. Threaded callers additionally wait indefinitely on_cache_lock.Fix: refresh config outside the request path using a bounded background refresh and appropriately bounded database connection/query timeouts. Requests should immediately use the last valid config, falling back from enforce to shadow when freshness cannot be confirmed. Add a regression test with a stalled refresh, rather than only an immediately raised database exception.
-
High —
core/capacity/limiter.py:159,176–182: a timed-out Redis operation can prevent a gunicorn worker from exiting.ThreadPoolExecutorworkers participate in Python’s interpreter shutdown join. Timing outfuture.result()abandons the caller’s wait; it does not stop the running operation or remove that shutdown dependency.Failure scenario: DNS resolution remains stuck after the request’s one-second deadline. Requests subsequently fail open, but when the worker reaches
--max-requests, interpreter shutdown waits for the executor thread. The worker stops serving and remains alive until gunicorn forcibly kills it. With the configured 600-second timeout, recycling can leave worker capacity unavailable for minutes.I reproduced this shutdown behavior in a subprocess: the main thread finished, but the process remained alive waiting for the executor.
Fix: use a dedicated daemon worker with a bounded submission queue and no interpreter-exit join dependency. While an operation remains stuck, keep that executor unavailable instead of repeatedly queueing abandoned probes. Add a subprocess test proving that process exit completes while an operation remains blocked; the current finite-sleep tests cannot catch this.
-
Medium —
core/capacity/logs.py:45–47,58–66: exit draining can lose the final log line or block shutdown indefinitely.The writer removes a line from the queue before writing it.
drain()only sees queued lines and does not coordinate with the writer’s in-flight line. It also writes synchronously without a deadline.Failure scenario:
manage.py capacity setqueues its change record; the daemon writer dequeues it but has not finished writing when the command exits.drain()sees an empty queue, returns, and interpreter shutdown terminates the writer, losing the config-change log. Conversely, with queued records and blocked stdout,drain()itself can hang during worker recycling.I reproduced the first race using the actual module:
drain()returned with an empty queue and no written record while the writer held the final line. The drain test patches out the writer, so it misses this case.Fix: track in-flight writes and implement coordinated shutdown with a strict deadline. Give the writer bounded time to finish both queued and in-flight records; avoid unbounded synchronous writes in
atexit. Test both an in-flight final record and a blocked sink during process exit. -
Low —
core/capacity/logs.py:23–32: concurrent emitters corrupt dropped-line accounting.Reading
dropped, enqueueing the associated record, and subtracting that count are not atomic.Failure scenario: two emitters both read
dropped=3, successfully enqueue records reporting three lost lines, then each subtracts three. The counter becomes-3, and subsequent records can report negative loss. I reproduced that interleaving. Sync gunicorn limits its production likelihood, but the module is unsafe for concurrent callers.Fix: serialize dropped-counter bookkeeping and the nonblocking enqueue under a short lock; leave stdout writes outside it. Add a coordinated concurrent-emitter test.
Things checked and found correct by inspection:
- Pass 2’s acquire-error fix clears refusal when acquisition fails without a held lease.
- Renewal checks every lease for unexpired ownership before extending any; the renewer stops after detecting lease loss.
- All Redis I/O entry points use
RedisLanes.call(). Queued abandoned operations skip execution. - With no preload, executor initialization happens within workers. I found no lock cycle between ordinary executor submission and Redis client initialization.
- Enforced refusal precedes
$matchquota consumption and reranking work. The extractedmatch()preserves the existing charge/refund logic. - Normal and exceptional view exits release through
finally; handled DRF responses receive headers throughfinalize_response. - Lua argument indexing, expiry comparisons, TTL extension, and shared single-row reserve logic are consistent.
- Staff endpoints use
IsAdminUser; PostgreSQL advisory locking serializes config updates; the migration is additive. - Capacity response headers are exposed through CORS. I found no new credentials or concrete infrastructure addresses in the change.
Changed-module pylint passed at 10.00/10, and git diff --check passed. I did not run database-writing tests or full CI coverage during this read-only review. No files were modified and no network was used.
|
Response to Codex pass 3, fixed in 4be4acf:
Capacity tests: 75 pass, on fakeredis and on Redis 7.0. |
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 4 (codex-cli 0.159.2, commit 4be4acf)
Posted verbatim; run read-only against git diff origin/master...HEAD, with pass 3's fixes as context.
-
Medium —
core/capacity/config.py:153–169: config refresh still has no end-to-end deadline.SET LOCAL statement_timeoutbounds execution on PostgreSQL; it does not bound waiting for connection establishment, sending that statement, receiving results, or completing the transaction. All this runs synchronously in the request thread.Failure scenario: a staff
$rerankrequest finishes authentication, then PostgreSQL connectivity stalls during a failover just as its config cache expires. The new config read can occupy the sync worker until TCP recovery or gunicorn’s 600-second timeout, although reranking could otherwise proceed without another database query. The fallback never runs while the database operation remains blocked. The table-lock test atcore/capacity/tests/tests.py:1107cannot detect this—it uses a responsive server.Fix: refresh config using a bounded daemon reader that owns a separate Django database connection. Requests should use the cached config immediately, downgrading enforce to shadow when freshness cannot be confirmed. Keep the SQL timeout, add connection timeouts, and test a reader that never returns.
Things checked and found correct:
- Pass 3’s Redis daemon, bounded queue, cancellation, and caller deadline fixes are present.
- The log sink tracks pending writes under its condition and bounds its exit drain.
- Config refresh correctly bounds PostgreSQL table-lock waits and downgrades enforce after read failures.
- Lua admission is atomic; Redis 7 time handling, expiry comparisons, and TTL extension look correct.
- Renewal cannot recreate released leases; release runs in
finally; stale leases expire. - Capacity refusals precede
$matchquota consumption. Shadow refusals do not addRetry-After. - Staff permissions, PATCH/PUT semantics, advisory-lock serialization, history, CORS headers, and the additive migration look correct.
- No newly committed secrets or concrete infrastructure identifiers found.
Targeted pylint passed 10.00/10. Tests and coverage were not run because they modify the test database. No files were modified and no network access was used.
|
Response to Codex pass 4. Declined, with no code change:
Residual risks, for the merge decision:
Codex found nothing further in passes 1–4 that this PR leaves open. Full suite locally, as CI runs it: 2,385 tests OK, 95% coverage. |
… too late takes no lease Found in a local soak with Redis behind Sentinel: while Redis hung (docker pause), acquire scripts that had timed out sat in the socket buffers and ran when Redis came back, taking leases nobody held. For up to lease_seconds after recovery the counts read high (in enforce mode that would refuse calls). The acquire now carries a not-after time (the caller's deadline plus 2 s for clock differences) and does nothing if Redis runs it later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
…the paused lane when one is (Codex review of #922 + oclmap#86 together) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
…fter an error, not 30, so one error can't cost a long call its lease (independent review of #922 + oclmap#86) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review across both PRs (codex-cli 0.159.2): #922 at 2701b02 + OpenConceptLab/oclmap#86 at 7519657
From the overnight integration test of the two PRs as a set (OpenConceptLab/ocl_online#275). Focus: the contract between them. Posted verbatim; read-only. Since then: finding 1 (the capacity_aware flag) is fixed in oclmap#86 43649ce + 9d1bb52, and finding 5 in oclapi2#922 2f399a1. Findings 2–4 are listed for OpenConceptLab/ocl_online#340, before enforcement.
Significant issues remain. The planned shadow rollout is compatible, but switching only runtime config to enforcement will not enforce against this Mapper under the default enforce_for=aware.
-
High — Enforcement prerequisite missing.
oclmap:src/services/attribution.js:64; src/components/map-projects/MapProject.jsx:3811
Nocapacity_awareoccurs anywhere in Mapper’ssrc. The attribution builder omits it, and the interactive single-row$matchpath sends no attribution metadata at all.Failure: changing the API to
mode=enforce,enforce_for=awareleaves all these Mapper calls effectively in shadow: they acquire leases even when lanes are full. Switching toallactivates refusals, including for old tabs that cannot handle them safely.Fix: before #340 activation, merge
"capacity_aware":"true"into metadata for every retry-capable heavy OCL call: bulk and interactive semantic$match,$matchwith inline reranking, and standalone$rerank. Preserve existing metadata; use a JSON string, not booleantrue. Do not rely solely on modifying the builder—the interactive path must actually send its headers. This is an activation blocker, not a blocker for shipping shadow mode. -
High — Ambiguous failures can duplicate heavy work and consumption.
oclmap:src/services/capacity.js:37,260; oclapi2:core/concepts/views.py:1223
Mapper retries network failures and 502/503/504 twice, without request deduplication. An 11-minute browser timeout does not guarantee that every earlier gateway failure means the original operation stopped.Failure: an ALB/gateway returns 504 or the connection breaks while Django continues searching. Mapper sends another
$match; both execute and consume operations if successful. During Redis failure, both are admitted uncounted. Even when Gunicorn actually kills the worker at 600 seconds, the precharged operations have no guaranteed refund because process death bypasses the exception handler.Fix: add server-backed idempotency/deduplication for
$match, including consumption and result recovery; otherwise avoid automatic retries after ambiguous execution failures. Validate the actual ALB timeout alongside Gunicorn’s timeout. The ALB configuration was unavailable locally. -
Medium — The “per-tab” gate and rerank limit reset on project unmount.
oclmap:src/components/map-projects/MapProject.jsx:236,361,1804
Gates and the two-slot rerank limiter are component refs. Unmount does not cancel capacity waits or invalidate the run ticket; cancellation checks only Stop and ticket changes in that component.Failure: start work in project A, navigate to project B, then start another run. A’s waits and outstanding calls can continue, while B creates an independent gate and two more rerank slots. One tab can exceed two concurrent
$rerankcalls and ignore A’s server pause.Fix: keep server gates and the rerank limiter in a tab-scoped service. On unmount, cancel queued work and waits through a component/run cancellation token. Outstanding server work may finish, but must retain its shared limiter slot until settlement.
-
Medium — Capacity refusals consume DRF rate allowance; Mapper conflates the two 429s.
oclapi2:core/concepts/views.py:952,1213; oclmap:src/services/capacity.js:242
DRF throttle checks run before the view’s capacity gate. A request subsequently refused for capacity has already passed—and consumed—the rate allowance. Mapper treats every 429 identically, without inspectingerror_code, scope, or rate-limit headers.Failure: at a five-second capacity delay, one waiting request can make roughly 288 attempts in 30 minutes with average jitter. Repeated runs or parallel calls can burn substantial portions of the standard 5,000/day
$matchallowance without matching anything. Eventually a DRF daily throttle replaces capacity refusals; Mapper still describes the condition as capacity throttling. A minute throttle also pauses unrelated heavy endpoints sharing the server gate.Fix: ensure capacity-refused calls do not retain DRF allowance consumption, using a concurrency-safe design. Classify
error_code === "capacity_exceeded"separately from DRF throttles; retain bounded waiting for both, but show the correct reason and apply appropriately scoped pauses. -
Low —
pausedcan contradict the reason for the long retry delay.
oclapi2:core/capacity/limiter.py:405,460
Retry delay considers any full lane with limit zero, butscopeandpauseddescribe only the first full lane.Failure: the cluster lane is full and the preview tier is paused at zero. The response uses the paused delay of 120 seconds but reports
scope:"api_heavy", paused:false. Mapper currently ignores these fields, so waiting works, but consumers cannot reliably distinguish a configured pause from temporary congestion.Fix: prioritize a zero-limit lane as the reported scope, or define
pausedas “any applicable lane is paused” and expose all blocking scopes.
What I checked and found correct:
- Headers/CORS: all seven emitted names match Mapper’s case-insensitive parser. Decision/tier are strings; counts, limits and suggested concurrency parse as numbers.
Retry-Afterand all seven capacity headers are explicitly exposed through CORS;X-OCL-Event-Metadatais allowed for preflight. Mapper’s optional suggested-spacing header is absent on this API and safely omitted. - Response coverage: gated 200s, capacity 429s and DRF-handled errors after acquisition receive capacity headers. Errors before acquisition—authentication, DRF throttling,
$rerankinput validation—do not. Off mode, ungated calls and preflight have no capacity decision headers. Proxy-generated errors cannot receive Django’s headers. - 429 shape:
detail,error_code, andscopeare strings;pausedis boolean;retry_afteris an integer in seconds. Capacity refusal precedes capability consumption. DRF throttles have a different body and ordinarily no capacity headers. - Retry parsing: numeric seconds and HTTP dates work; zero/past dates get a one-second minimum wait. Missing headers fall back to numeric body seconds, then exponential delays of 5–60 seconds. An invalid present header bypasses body fallback. Huge delays immediately end as throttled without poisoning the shared gate. Header-driven waits receive 0–50% jitter.
- Bounds and Stop: Auto Match accumulates up to 30 minutes of waiting, not total elapsed execution time. Single-row
$matchuses five minutes; interactive$rerankcurrently uses the default 30 minutes. Stop interrupts configured waits within a 250-ms poll and prevents further run retries; already executing server work continues. - Leases: 60 seconds is a renewable lease, not a request duration limit. Healthy 20-second renewal keeps a long call counted. Renewal is atomic across all lanes and never revives expired leases. With default settings, Gunicorn kills an overlong sync request at roughly 600 seconds, before the 900-second renewal ceiling; its count then expires within 60 seconds of the last renewal. If a call survives beyond the renewal ceiling, or renewal is lost during a Redis outage, it eventually continues uncounted. Fail-open therefore provides availability, not a hard concurrency guarantee.
- Mixed versions: old Mapper + new shadow API remains compatible. Old tabs +
awareenforcement remain admitted; old tabs +allcan hang on their legacy 429 path. New Mapper + old API tolerates absent capacity headers and handles ordinary DRF 429s. During a mixed API rollout, old tasks perform uncounted work, so cluster limits cannot be authoritative until every task is upgraded. New workers also converge on runtime config over the cache interval, rather than instantly. - Verification: confirmed both requested HEADs and ran 130 focused Mapper tests: all passed. API behavior was reviewed from source and existing tests; no live Django/Redis/ALB verification was performed. No files were modified and no network was used.
|
Independent Claude review across both PRs (a read-only Claude subagent, run in the overnight integration test of #922 + OpenConceptLab/oclmap#86, OpenConceptLab/ocl_online#275). Reviewed: oclapi2#922 at 2701b02, oclmap#86 at 7519657. Posted verbatim. Since then: finding 1 is fixed in oclmap#86 (43649ce, 9d1bb52), and finding 7 in oclapi2#922 (040a689: skip window 15 s). Findings 2–6 are listed for OpenConceptLab/ocl_online#340, before enforcement. Cross-repo contract review: oclapi2 PR 922 and oclmap PR 86I only read code. No files were changed and no APIs were called. Nothing here breaks the shadow-mode deploy. Findings 1–3 must be fixed before enforcement is turned on. Findings1. HIGH: the Mapper never sends
2. HIGH (only once enforced): the Mapper runs more calls at once than the server allows per user, and the retrying batch starves.
3. MEDIUM: a capacity 429 and a DRF throttle 429 look the same to the Mapper, and capacity retries use up DRF quota.
4. MEDIUM: heavy calls are re-sent after long failures.
5. MEDIUM-LOW: one gate per server ignores
6. LOW: a manual
7. LOW: a single Redis error can lose a long call's lease.
8. NIT: the Mapper parses Checked and found correct
|
Overnight integration test: #922 + OpenConceptLab/oclmap#86 (2026-10-01)The two PRs were tested together on a local production-like stack:
No merges or deploys. Every decisive row was re-run at the final heads (oclapi2#922 c5282f4, oclmap#86 9d1bb52; CI green on both). Compatibility (decides the deploy order): both directions work.
The new API in shadow mode:
This Mapper against real capacity 429s (enforce mode, which ships off):
Commits added overnight:
Before enforcement (OpenConceptLab/ocl_online#340), from the test and the reviews:
The full readout, reviews and evidence are on OpenConceptLab/ocl_online#275. |
Part of OpenConceptLab/ocl_online#275: a capacity limit on heavy calls, shipping in shadow mode. It counts and logs how many heavy calls run at once and refuses nothing.
A capacity limit caps how many heavy requests run at once across all users, to protect the API workers and Elasticsearch. It isn't a cap (what an account can have) or a quota (what it can use per period). It charges no quota, and once enforced, a refused call is asked to retry shortly.
What counts as heavy
$matchwithsemantic=true(kNN search, bridge calls included) orreranker=true(the in-request rerank).$rerank.Lexical
$matchwithout the reranker isn't gated. The gate sits after authentication, permissions and throttling, and before the match-operations quota is charged.Lanes
Each lane is a Redis sorted set of leases, with the score set to the lease's expiry by Redis's own clock. One Lua script takes a lease in every lane that applies, atomically. A background thread renews the leases every 20 s, all or nothing (an expired lease is never revived), and they're released in
finally. A lease that isn't renewed expires after 60 s, so a worker that dies frees its slots. A lane's TTL is only ever extended.api_heavy, cluster-wide$matchcalls (TBv3 search, the Mapper's row panel)api_heavy_task, per API taskes2_knn, semantic$matchonlyA user's tier is their highest plan: staff >
core_user>early_access> preview (the default).Modes
off: nothing is counted and no headers are sent.shadow(the default): every call goes ahead. A call that finds a lane full is markedshadow-refusedin its log line and headers.enforce: a call that finds a lane full gets 429 withRetry-Afterand{"error_code": "capacity_exceeded", "scope": <lane>, "paused": bool, "retry_after": N}. It's refused before any work or quota charge, so refused calls aren't metered.Retry-Afteris 5 s per call ahead in the fullest lane, up to 30 s, or 120 s when a lane's limit is 0 (paused).enforce_for=aware(the default) refuses only clients that send"capacity_aware": "true"inX-OCL-Event-Metadata. Other clients (older Mapper tabs, TBv3, scripts) stay in shadow mode.Headers
Gated responses carry the headers below, and CORS exposes them to browsers:
X-OCL-Capacity-Decisionadmitted,shadow-refused,refusedorunavailableX-OCL-Capacity-Limit,-In-FlightX-OCL-Capacity-Tier,-Tier-Limit,-Tier-In-FlightX-OCL-Capacity-Suggested-ConcurrencyRuntime config
The mode and every number can be changed at runtime by staff, with no deploy or restart:
GET/PATCH/PUT /capacity/config/,GET /capacity/config/history/andGET /capacity/status/(live lanes), allIsAdminUser.python manage.py capacity show|set|reset|history|status, for examplecapacity set tiers.preview=1 --note "...".Each change adds a
CapacityConfigrow recording who made it, when, and the old and new config, and writes oneocl_capacity_configlog line. Each API process re-reads the config at most every 10 s (CAPACITY_CONFIG_CACHE_SECONDS).CAPACITY_LIMIT_MODE(defaultshadow) sets the mode only until staff first change it.Fail open
If Redis can't be reached, calls go ahead uncounted, marked
unavailablewith the error.CAPACITY_REDIS_DEADLINE_SECONDS(1 s), which covers DNS and Sentinel discovery. A hung call never holds up a worker's exit, and a full queue fails at once.CAPACITY_CONFIG_READ_TIMEOUT_MS), so a locked table can't hold a call.log_dropped. At exit, a process waits up to 2 s for lines still queued or being written.Measurement
Every gated call writes one JSON line (
"event": "ocl_capacity") when it finishes. The line has the decision, mode, endpoint, rows, tier, the full lanes, each lane's in-flight count and limit, suggested concurrency,retry_after,held_ms,queue_ms, the request source,algorithm_id,automatch_run_idand cid.queue_msis how long the call waited between the load balancer and the view, read fromX-Amzn-Trace-Id, to whole-second resolution.api_transactions.Also in this PR
get_event_metadata(request)incore/common/utils.py, shared with$match's usage attribution.fakeredis[lua]andlupainrequirements.txt. CI's test job has no Redis, and these run the real Lua scripts in tests. The same tests also pass against Redis 7.0 (CAPACITY_TEST_REDIS_URL=redis://…).capacity_configs(migrationcapacity/0001_initial).Tests
core.capacity: 75 tests covering config validation and history, every lane, the reserve, all three modes,enforce_for, a paused tier,Retry-After, lease expiry and all-or-nothing renewal, the renewer thread, fail-open with backoff and the Redis deadline, the non-blocking log queue, process exit with a hung Redis call or a blocked log (subprocesses), a locked config table, headers, the log line, the staff views and the command.$match/$rerankview tests check that an enforce refusal returns 429 before any quota charge or work, and that a match that raises still releases its lease.$match/$reranktests exercise the fail-open path.Integration test with OpenConceptLab/oclmap#86 (overnight 2026-10-01)
Run locally on a production-like stack:
Results:
capacity_aware.Fixed in this PR from the test and its reviews:
Findings that matter before enforcement are listed on OpenConceptLab/ocl_online#340.
Not in this PR
🤖 Generated with Claude Code
https://claude.ai/code/session_01TyAC9YAn5hP3yGSREkSN8m