feat(api): priority-aware server-side API backpressure#9879
Conversation
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would require human review. This is a large new feature (~2853 lines) introducing priority-aware API backpressure with new middleware, concurrency primitives, and configuration. It modifies the critical request path with potential blast radius on production behavior, requiring human review of correctness and safety.
Re-trigger cubic
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Confidence score: 3/5
- In
backend/infrahub/api/admission/middleware.py, excluded-route matching is too permissive (prefix-based), so endpoints like/healthcheckcan bypass admission control unintentionally, weakening backpressure where it should apply—restrict matching to exact paths or slash-delimited descendants before merging. - In
backend/infrahub/api/admission/capacity.py(derive_max_concurrency), non-finite config values (inf/nan) can trigger a crash because float env inputs are accepted by default; merging as-is risks runtime failure from a bad environment setting—validate/reject non-finite factors and add a guard test. - The spec in
dev/specs/ifc-2886-priority-api-backpressure/research.mdleaves CORS preflight shedding and strict metrics invariants unreconciled, creating ambiguity in intended behavior and observability under load—resolve these edge cases in the spec so implementation and alerts are aligned. backend/tests/component/api/test_admission_middleware.pyuses wall-clock sleep intest_gradient, which can produce CI flakiness and mask regressions; coupled with minor plan inaccuracies indev/specs/ifc-2886-priority-api-backpressure/plan.md, this is low merge risk but worth tightening via condition-based polling and doc updates soon.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/api/admission/capacity.py">
<violation number="1" location="backend/infrahub/api/admission/capacity.py:19">
P2: `derive_max_concurrency` crashes on non-finite `factor` values (inf/nan). Pydantic v2 accepts `inf`/`nan` for float fields by default, so setting `INFRAHUB_API_BACKPRESSURE_MAX_CONCURRENCY_FACTOR=inf` in the environment would pass validation, reach `int(pool_size * factor)`, and raise `OverflowError` during startup. Consider adding a guard — either `math.isfinite(factor)` at the function boundary or `allow_inf_nan=False` on the Pydantic field.</violation>
</file>
<file name="dev/specs/ifc-2886-priority-api-backpressure/research.md">
<violation number="1" location="dev/specs/ifc-2886-priority-api-backpressure/research.md:20">
P2: The spec defines excluded paths for the outermost admission middleware and a strict metrics invariant, but two edge cases are not reconciled:
1. **CORS OPTIONS preflight can be shed.** Since `AdmissionMiddleware` is outermost (runs before `InfrahubCORSMiddleware`), a cross-origin OPTIONS preflight for a request carrying `X-Priority` would be subjected to admission. Under overload, it could be shed with 429, causing CORS clearance to fail and the actual request to never be sent — defeating the frontend-protection goal. Consider adding `"OPTIONS"` as an HTTP-method bypass in the middleware, or at minimum document this known limitation in the spec/contracts so operators deploying cross-origin are aware.
2. **Metrics invariant M-1 breaks on client disconnect.** The invariant `offered_total == admitted_total + rejected_total` (metrics.md M-1) cannot hold when a client disconnects while queued — `offered_total` is incremented before `acquire()`, but cancellation produces neither admission nor rejection, creating a permanent accounting gap of one per cancelled request. Either move the `offered_total` increment after the acquire (accepting that cancelled requests are never counted as offered), add a cancellation counter to close the gap, or document the expected discrepancy as part of M-1 so operators are not confused by it.</violation>
</file>
<file name="backend/tests/component/api/test_admission_middleware.py">
<violation number="1" location="backend/tests/component/api/test_admission_middleware.py:111">
P3: `test_gradient` uses `await asyncio.sleep(0.05)` to wait for the slot pool to saturate before injecting HIGH requests. This is a wall-clock wait rather than a condition-based poll. Under CI contention or scheduler jitter, the pool may not be fully saturated by the time high-priority requests start. Consider replacing the fixed sleep with the `_wait_until_waiting` helper pattern used in the unit tests to poll until a deterministic condition (e.g., `pool.waiters(priority=Priority.LOW, count=...)`) is reached. This removes the timing dependency without changing the test's intent.</violation>
</file>
<file name="backend/infrahub/api/admission/middleware.py">
<violation number="1" location="backend/infrahub/api/admission/middleware.py:75">
P2: Paths that merely begin with an excluded name bypass backpressure; e.g. `/healthcheck` is not `/health` but reaches the fallback handler without admission. Match the exact path or a slash-delimited descendant so only mounted/static routes are excluded.</violation>
</file>
<file name="dev/specs/ifc-2886-priority-api-backpressure/plan.md">
<violation number="1" location="dev/specs/ifc-2886-priority-api-backpressure/plan.md:17">
P3: FastAPI version in plan (0.131) doesn't match the project pin (0.136.3). Update to 0.136.3 to avoid misleading readers about the actual target version.</violation>
<violation number="2" location="dev/specs/ifc-2886-priority-api-backpressure/plan.md:17">
P3: prometheus-client is already a direct dependency in pyproject.toml, not a transitive pin. The plan's claim that it's transitive is inaccurate and the E5 risk note about adding it as an explicit dependency is unnecessary.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| admits a single request at a time. | ||
|
|
||
| """ | ||
| return max(1, int(pool_size * factor)) |
There was a problem hiding this comment.
P2: derive_max_concurrency crashes on non-finite factor values (inf/nan). Pydantic v2 accepts inf/nan for float fields by default, so setting INFRAHUB_API_BACKPRESSURE_MAX_CONCURRENCY_FACTOR=inf in the environment would pass validation, reach int(pool_size * factor), and raise OverflowError during startup. Consider adding a guard — either math.isfinite(factor) at the function boundary or allow_inf_nan=False on the Pydantic field.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/api/admission/capacity.py, line 19:
<comment>`derive_max_concurrency` crashes on non-finite `factor` values (inf/nan). Pydantic v2 accepts `inf`/`nan` for float fields by default, so setting `INFRAHUB_API_BACKPRESSURE_MAX_CONCURRENCY_FACTOR=inf` in the environment would pass validation, reach `int(pool_size * factor)`, and raise `OverflowError` during startup. Consider adding a guard — either `math.isfinite(factor)` at the function boundary or `allow_inf_nan=False` on the Pydantic field.</comment>
<file context>
@@ -0,0 +1,19 @@
+ admits a single request at a time.
+
+ """
+ return max(1, int(pool_size * factor))
</file context>
| return | ||
|
|
||
| path = scope.get("path", "") | ||
| if any(path.startswith(excluded) for excluded in self._excluded_paths): |
There was a problem hiding this comment.
P2: Paths that merely begin with an excluded name bypass backpressure; e.g. /healthcheck is not /health but reaches the fallback handler without admission. Match the exact path or a slash-delimited descendant so only mounted/static routes are excluded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/api/admission/middleware.py, line 75:
<comment>Paths that merely begin with an excluded name bypass backpressure; e.g. `/healthcheck` is not `/health` but reaches the fallback handler without admission. Match the exact path or a slash-delimited descendant so only mounted/static routes are excluded.</comment>
<file context>
@@ -0,0 +1,136 @@
+ return
+
+ path = scope.get("path", "")
+ if any(path.startswith(excluded) for excluded in self._excluded_paths):
+ await self.app(scope, receive, send)
+ return
</file context>
| if any(path.startswith(excluded) for excluded in self._excluded_paths): | |
| if any(path == excluded or path.startswith(f"{excluded}/") for excluded in self._excluded_paths): |
| - `@app.middleware("http")` decorator (used by 3 existing middlewares): rejected — it wraps `BaseHTTPMiddleware`, which fully buffers the response and is a poor fit for a hot admission path. | ||
| - A FastAPI dependency instead of middleware: rejected — dependencies run after routing/auth and per-route wiring; a middleware gives one uniform admission point across all endpoints. | ||
|
|
||
| **Path exclusions**: The middleware MUST bypass admission for `/health` and `/metrics` (liveness and scraping must never be shed) and SHOULD bypass static/docs paths (`/assets`, `/favicons`, `/docs`, `/api/schema`) to match the existing `ConditionalGZipMiddleware` skip set. Non-`http` scopes (WebSocket, lifespan) pass through untouched. |
There was a problem hiding this comment.
P2: The spec defines excluded paths for the outermost admission middleware and a strict metrics invariant, but two edge cases are not reconciled:
-
CORS OPTIONS preflight can be shed. Since
AdmissionMiddlewareis outermost (runs beforeInfrahubCORSMiddleware), a cross-origin OPTIONS preflight for a request carryingX-Prioritywould be subjected to admission. Under overload, it could be shed with 429, causing CORS clearance to fail and the actual request to never be sent — defeating the frontend-protection goal. Consider adding"OPTIONS"as an HTTP-method bypass in the middleware, or at minimum document this known limitation in the spec/contracts so operators deploying cross-origin are aware. -
Metrics invariant M-1 breaks on client disconnect. The invariant
offered_total == admitted_total + rejected_total(metrics.md M-1) cannot hold when a client disconnects while queued —offered_totalis incremented beforeacquire(), but cancellation produces neither admission nor rejection, creating a permanent accounting gap of one per cancelled request. Either move theoffered_totalincrement after the acquire (accepting that cancelled requests are never counted as offered), add a cancellation counter to close the gap, or document the expected discrepancy as part of M-1 so operators are not confused by it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-2886-priority-api-backpressure/research.md, line 20:
<comment>The spec defines excluded paths for the outermost admission middleware and a strict metrics invariant, but two edge cases are not reconciled:
1. **CORS OPTIONS preflight can be shed.** Since `AdmissionMiddleware` is outermost (runs before `InfrahubCORSMiddleware`), a cross-origin OPTIONS preflight for a request carrying `X-Priority` would be subjected to admission. Under overload, it could be shed with 429, causing CORS clearance to fail and the actual request to never be sent — defeating the frontend-protection goal. Consider adding `"OPTIONS"` as an HTTP-method bypass in the middleware, or at minimum document this known limitation in the spec/contracts so operators deploying cross-origin are aware.
2. **Metrics invariant M-1 breaks on client disconnect.** The invariant `offered_total == admitted_total + rejected_total` (metrics.md M-1) cannot hold when a client disconnects while queued — `offered_total` is incremented before `acquire()`, but cancellation produces neither admission nor rejection, creating a permanent accounting gap of one per cancelled request. Either move the `offered_total` increment after the acquire (accepting that cancelled requests are never counted as offered), add a cancellation counter to close the gap, or document the expected discrepancy as part of M-1 so operators are not confused by it.</comment>
<file context>
@@ -0,0 +1,123 @@
+- `@app.middleware("http")` decorator (used by 3 existing middlewares): rejected — it wraps `BaseHTTPMiddleware`, which fully buffers the response and is a poor fit for a hot admission path.
+- A FastAPI dependency instead of middleware: rejected — dependencies run after routing/auth and per-route wiring; a middleware gives one uniform admission point across all endpoints.
+
+**Path exclusions**: The middleware MUST bypass admission for `/health` and `/metrics` (liveness and scraping must never be shed) and SHOULD bypass static/docs paths (`/assets`, `/favicons`, `/docs`, `/api/schema`) to match the existing `ConditionalGZipMiddleware` skip set. Non-`http` scopes (WebSocket, lifespan) pass through untouched.
+
+## R2. Concurrency primitive — priority slot pool
</file context>
|
|
||
| # Let the pool saturate and sojourn climb past the CoDel interval before the | ||
| # interactive stream starts, so shedding is already active. | ||
| await asyncio.sleep(0.05) |
There was a problem hiding this comment.
P3: test_gradient uses await asyncio.sleep(0.05) to wait for the slot pool to saturate before injecting HIGH requests. This is a wall-clock wait rather than a condition-based poll. Under CI contention or scheduler jitter, the pool may not be fully saturated by the time high-priority requests start. Consider replacing the fixed sleep with the _wait_until_waiting helper pattern used in the unit tests to poll until a deterministic condition (e.g., pool.waiters(priority=Priority.LOW, count=...)) is reached. This removes the timing dependency without changing the test's intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/component/api/test_admission_middleware.py, line 111:
<comment>`test_gradient` uses `await asyncio.sleep(0.05)` to wait for the slot pool to saturate before injecting HIGH requests. This is a wall-clock wait rather than a condition-based poll. Under CI contention or scheduler jitter, the pool may not be fully saturated by the time high-priority requests start. Consider replacing the fixed sleep with the `_wait_until_waiting` helper pattern used in the unit tests to poll until a deterministic condition (e.g., `pool.waiters(priority=Priority.LOW, count=...)`) is reached. This removes the timing dependency without changing the test's intent.</comment>
<file context>
@@ -0,0 +1,605 @@
+
+ # Let the pool saturate and sojourn climb past the CoDel interval before the
+ # interactive stream starts, so shedding is already active.
+ await asyncio.sleep(0.05)
+
+ high_statuses: list[int] = []
</file context>
|
|
||
| **Language/Version**: Python 3.14 (backend) | ||
|
|
||
| **Primary Dependencies**: FastAPI 0.131 / Starlette (ASGI middleware), `prometheus_client` 0.25 (already imported directly in-tree; transitive pin), Neo4j async driver 6.2, Pydantic 2.12 / pydantic-settings. No new hard dependency (CoDel + slot pool are custom, stdlib `asyncio`/`collections`/`time.monotonic`). |
There was a problem hiding this comment.
P3: prometheus-client is already a direct dependency in pyproject.toml, not a transitive pin. The plan's claim that it's transitive is inaccurate and the E5 risk note about adding it as an explicit dependency is unnecessary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-2886-priority-api-backpressure/plan.md, line 17:
<comment>prometheus-client is already a direct dependency in pyproject.toml, not a transitive pin. The plan's claim that it's transitive is inaccurate and the E5 risk note about adding it as an explicit dependency is unnecessary.</comment>
<file context>
@@ -0,0 +1,125 @@
+
+**Language/Version**: Python 3.14 (backend)
+
+**Primary Dependencies**: FastAPI 0.131 / Starlette (ASGI middleware), `prometheus_client` 0.25 (already imported directly in-tree; transitive pin), Neo4j async driver 6.2, Pydantic 2.12 / pydantic-settings. No new hard dependency (CoDel + slot pool are custom, stdlib `asyncio`/`collections`/`time.monotonic`).
+
+**Storage**: N/A — the admission layer is fully in-memory, per worker process. No persistence, no schema, no migration.
</file context>
|
|
||
| **Language/Version**: Python 3.14 (backend) | ||
|
|
||
| **Primary Dependencies**: FastAPI 0.131 / Starlette (ASGI middleware), `prometheus_client` 0.25 (already imported directly in-tree; transitive pin), Neo4j async driver 6.2, Pydantic 2.12 / pydantic-settings. No new hard dependency (CoDel + slot pool are custom, stdlib `asyncio`/`collections`/`time.monotonic`). |
There was a problem hiding this comment.
P3: FastAPI version in plan (0.131) doesn't match the project pin (0.136.3). Update to 0.136.3 to avoid misleading readers about the actual target version.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-2886-priority-api-backpressure/plan.md, line 17:
<comment>FastAPI version in plan (0.131) doesn't match the project pin (0.136.3). Update to 0.136.3 to avoid misleading readers about the actual target version.</comment>
<file context>
@@ -0,0 +1,125 @@
+
+**Language/Version**: Python 3.14 (backend)
+
+**Primary Dependencies**: FastAPI 0.131 / Starlette (ASGI middleware), `prometheus_client` 0.25 (already imported directly in-tree; transitive pin), Neo4j async driver 6.2, Pydantic 2.12 / pydantic-settings. No new hard dependency (CoDel + slot pool are custom, stdlib `asyncio`/`collections`/`time.monotonic`).
+
+**Storage**: N/A — the admission layer is fully in-memory, per worker process. No persistence, no schema, no migration.
</file context>
| **Primary Dependencies**: FastAPI 0.131 / Starlette (ASGI middleware), `prometheus_client` 0.25 (already imported directly in-tree; transitive pin), Neo4j async driver 6.2, Pydantic 2.12 / pydantic-settings. No new hard dependency (CoDel + slot pool are custom, stdlib `asyncio`/`collections`/`time.monotonic`). | |
| **Primary Dependencies**: FastAPI 0.136.3 / Starlette (ASGI middleware), `prometheus_client` 0.25 (already imported directly in-tree; direct pin in pyproject.toml), Neo4j async driver 6.2, Pydantic 2.12 / pydantic-settings. No new hard dependency (CoDel + slot pool are custom, stdlib `asyncio`/`collections`/`time.monotonic`). |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/api/admission/slot_pool.py">
<violation number="1" location="backend/infrahub/api/admission/slot_pool.py:69">
P2: A failing observer can now leak admission state: an exception after `_available`/`_in_flight` changes prevents the acquisition or release flow from completing, and an exception on enqueue leaves the waiter in its deque. Isolate observer failures so metrics/monitoring callbacks cannot alter request admission.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| def _notify(self, priority: Priority) -> None: | ||
| if self._on_change is not None: | ||
| self._on_change(priority) |
There was a problem hiding this comment.
P2: A failing observer can now leak admission state: an exception after _available/_in_flight changes prevents the acquisition or release flow from completing, and an exception on enqueue leaves the waiter in its deque. Isolate observer failures so metrics/monitoring callbacks cannot alter request admission.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/api/admission/slot_pool.py, line 69:
<comment>A failing observer can now leak admission state: an exception after `_available`/`_in_flight` changes prevents the acquisition or release flow from completing, and an exception on enqueue leaves the waiter in its deque. Isolate observer failures so metrics/monitoring callbacks cannot alter request admission.</comment>
<file context>
@@ -52,6 +52,21 @@ def __init__(self, *, max_concurrency: int, clock: Callable[[], float] = time.mo
+
+ def _notify(self, priority: Priority) -> None:
+ if self._on_change is not None:
+ self._on_change(priority)
@property
</file context>
| self._on_change(priority) | |
| try: | |
| self._on_change(priority) | |
| except Exception: | |
| pass |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 8 unresolved issues from previous reviews.
Re-trigger cubic
f7d7719 to
362ca0d
Compare
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
3 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/database/load_signal.py">
<violation number="1" location="backend/infrahub/database/load_signal.py:126">
P1: Low-priority requests can still be stress-shed after an idle period has exceeded the rolling window. Expire samples when the controller reads `sample_count()` so a stale stressed window cannot reject the first request before it reaches the reference query.</violation>
</file>
<file name="backend/infrahub/api/admission/controller.py">
<violation number="1" location="backend/infrahub/api/admission/controller.py:143">
P1: Stress-shed requests queue for an admission slot before their random stress decision, so under saturated capacity they do not get the promised fast 429 and consume waiter/backstop capacity. Evaluate the stress fraction and draw before `acquire`; retain post-acquisition evaluation only for CoDel sojourn handling.</violation>
</file>
<file name="backend/tests/unit/database/test_load_signal.py">
<violation number="1" location="backend/tests/unit/database/test_load_signal.py:147">
P3: test_observer_publishes_the_derived_signal_to_the_gauges asserts 3 of the 4 gauges the observer sets, missing REFERENCE_QUERY_STRESS_RATIO_AVG.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| """The all-time minimum observation, or ``None`` before any observation.""" | ||
| return self._floor | ||
|
|
||
| def sample_count(self) -> int: |
There was a problem hiding this comment.
P1: Low-priority requests can still be stress-shed after an idle period has exceeded the rolling window. Expire samples when the controller reads sample_count() so a stale stressed window cannot reject the first request before it reaches the reference query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/database/load_signal.py, line 126:
<comment>Low-priority requests can still be stress-shed after an idle period has exceeded the rolling window. Expire samples when the controller reads `sample_count()` so a stale stressed window cannot reject the first request before it reaches the reference query.</comment>
<file context>
@@ -0,0 +1,170 @@
+ """The all-time minimum observation, or ``None`` before any observation."""
+ return self._floor
+
+ def sample_count(self) -> int:
+ """Number of observations currently inside the window."""
+ return len(self._samples)
</file context>
| # sojourn continuously. Database stress sheds a fraction of the class (a random draw | ||
| # against the escalating schedule); CoDel sheds on sojourn overrun. Stress is | ||
| # attributed first when both fire, so the stress dimension stays visible. | ||
| shed_fraction = self._stress_shed_fraction(priority=priority) |
There was a problem hiding this comment.
P1: Stress-shed requests queue for an admission slot before their random stress decision, so under saturated capacity they do not get the promised fast 429 and consume waiter/backstop capacity. Evaluate the stress fraction and draw before acquire; retain post-acquisition evaluation only for CoDel sojourn handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/api/admission/controller.py, line 143:
<comment>Stress-shed requests queue for an admission slot before their random stress decision, so under saturated capacity they do not get the promised fast 429 and consume waiter/backstop capacity. Evaluate the stress fraction and draw before `acquire`; retain post-acquisition evaluation only for CoDel sojourn handling.</comment>
<file context>
@@ -89,17 +136,41 @@ async def admit(self, *, priority: Priority) -> AdmissionDecision:
+ # sojourn continuously. Database stress sheds a fraction of the class (a random draw
+ # against the escalating schedule); CoDel sheds on sojourn overrun. Stress is
+ # attributed first when both fire, so the stress dimension stays visible.
+ shed_fraction = self._stress_shed_fraction(priority=priority)
+ stressed = shed_fraction > 0.0 and self._rng() < shed_fraction
+ codel_drop = self._codel[priority].should_drop(sojourn=acquisition.sojourn)
</file context>
|
|
||
| assert REFERENCE_QUERY_FLOOR_SECONDS._value.get() == 0.002 | ||
| assert REFERENCE_QUERY_WINDOW_MIN_SECONDS._value.get() == 0.050 | ||
| assert REFERENCE_QUERY_STRESS_RATIO_MIN._value.get() == pytest.approx(25.0) |
There was a problem hiding this comment.
P3: test_observer_publishes_the_derived_signal_to_the_gauges asserts 3 of the 4 gauges the observer sets, missing REFERENCE_QUERY_STRESS_RATIO_AVG.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/database/test_load_signal.py, line 147:
<comment>test_observer_publishes_the_derived_signal_to_the_gauges asserts 3 of the 4 gauges the observer sets, missing REFERENCE_QUERY_STRESS_RATIO_AVG.</comment>
<file context>
@@ -0,0 +1,147 @@
+
+ assert REFERENCE_QUERY_FLOOR_SECONDS._value.get() == 0.002
+ assert REFERENCE_QUERY_WINDOW_MIN_SECONDS._value.get() == 0.050
+ assert REFERENCE_QUERY_STRESS_RATIO_MIN._value.get() == pytest.approx(25.0)
</file context>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 14 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Specify phase — server-side admission layer that sheds API load by X-Priority (high/normal/low) with adaptive CoDel shedding and 429 + Retry-After. Covers server side only per PRD scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Plan phase — pure-ASGI admission middleware (outermost), custom cancellation-safe PrioritySlotPool, per-class CoDel controller with injected clock, capacity derived from a new INFRAHUB_DB_MAX_CONNECTION_POOL_SIZE setting, admission knobs on ApiSettings, and an infrahub_admission_* metrics module on the existing /metrics endpoint. Grounded in the current backend (server.py middleware stack, database/graphql metrics.py, config.py settings groups). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Dual-lens critique (verdict: PROCEED WITH UPDATES). Must-address E1/X1: the slot-sojourn load signal is blind if the derived cap exceeds real Neo4j serving capacity. Applied: spec assumption that slot contention must bind first + headroom factor lever, sharpened SC-001 to validate the signal in the discovery scenario, and plan risk/rollout/metrics/ dependency notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tasks phase — dependency-ordered, organized by user story (US2 header → US1 gradient MVP → US3 shed shape → US4 capacity → US5 observability), with unit + component tests per the no-mocking convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Verdict: MINOR DRIFT (proceeding). All FRs/FR-OBS/SCs/entities/edge cases/assumptions/out-of-scope from the IFC-2886 PRD are present and semantically intact; deltas are two consolidated user stories (substance retained in FR-OBS-7 + tasks) and justified additive refinements from the critique. No remediation pass required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Introduce the configuration surface for priority-aware API backpressure: - new DatabaseSettings.max_connection_pool_size (default 100), wired into the Neo4j driver, preserving current driver behaviour - backpressure knobs on ApiSettings (enable switch, CoDel target/interval, high-priority multiplier, backstop cap, retry-after, concurrency factor) - empty admission package marker for upcoming modules - prometheus-client promoted to an explicit direct dependency Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implement the foundational admission-control primitives for priority-aware API backpressure: - Priority IntEnum (HIGH<NORMAL<LOW) with a Prometheus label helper - derive_max_concurrency capacity function (floored at 1) - infrahub_admission_* Prometheus metric families on the default registry - PrioritySlotPool: cancellation-safe bounded pool with per-class FIFO waiter queues and priority-ordered hand-off - CoDelController: pure CoDel target/interval shedding state machine - AdmissionController composing the pool, per-class CoDel, and backstop, with an Admitted/Rejected frozen tagged union and a settings factory No HTTP middleware, header parser, or tests in this chunk. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add parse_priority() and the PriorityHeaderParseResult frozen dataclass to the admission priority module. Case-insensitive, whitespace-trimmed matching of high/normal/low; missing, empty, or invalid values resolve to NORMAL with was_explicit=False. Parsing never raises. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add AdmissionMiddleware, a pure-ASGI outermost gate that classifies each request by its X-Priority header and hands it to the admission controller, running admitted requests inside their slot (released in a finally) and answering shed requests with a bare 429 + Retry-After without touching the handler. Non-http scopes, the excluded liveness/scrape/static paths, and the disabled kill-switch pass straight through. Wire the controller once at app init in server.py via the factory, publish the derived slot cap to the max_concurrency gauge, store the controller on app.state, and register the middleware last so it is outermost. Cover the slot pool (cross-class hand-off, within-class FIFO, cancellation cleanup and re-release), the CoDel state machine (sub-interval burst, sustained overload onset, bounded recovery, high-target protection) with injected clocks, and the shed gradient end-to-end against a standalone app with no infrastructure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Refine the admission middleware shed path to return a proper 429 error
envelope selected REST vs GraphQL by request path (mirroring the
exception-handler split), carrying a Retry-After header, short-circuiting
before the downstream app so no handler work runs. The codel/backstop
reason already flows to rejected_total{reason} in the controller, so the
middleware only owns envelope/header correctness. Add component tests
covering the backstop and codel shed paths.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (US4) Prove FR-004/SC-003: the X-Priority header is preserved across the request-rebuild paths on both transports and the file-upload path. - T023: GraphQL — exercise the real handleGraphQLAuthError/ retryWithRefreshedToken; a TOKEN_EXPIRED refresh+replay re-carries the original X-Priority via the `...oldHeaders` spread. - T024: REST — onRequest stamps X-Priority before the retry clone is captured, so the 401 stored-clone replay (fetch(clonedRequest)) still carries it. - T025: file upload — createObjectFromApi issues a multipart mutation whose priority-free context inherits `high` through the shared priorityLink (real multipart HTTP deferred to E2E, T031). T026: injection order already guarantees preservation on both transports; no code change needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ission Append "x-priority" to the default CORS allow-headers so cross-origin frontends can send the X-Priority header used by API backpressure. Also exempt CORS OPTIONS preflight requests (those advertising Access-Control-Request-Method) from the outermost admission gate. A preflight carries no X-Priority and would otherwise be classified normal and shed under load, stripping the CORS response and breaking every cross-origin request exactly when the backend is saturated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a Playwright E2E spec proving end-to-end in a browser that the frontend emits the outbound X-Priority header per its contract: - an interactive navigation emits X-Priority: high on every captured Infrahub-API request (SC-002); - a low-declared request emits X-Priority: low (synthetic: the v1 low set is empty, so no UI flow issues one; per-transport injection is covered by unit tests); - no frontend-origin API request leaves as normal or unheadered (FR-003). Also documents the SC-001 adoption-metric check (T032) in the spec file header as a manual live-stack step, since the global counter has no origin dimension to assert on automatically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document the frontend X-Priority emitter: the shared RequestPriority contract, the high default, the four transport injection points, the per-transport low opt-in convention, why watched-status polls stay high, and the backend CORS/preflight note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
All 36 tasks complete across 9 chunks. 21 frontend unit + 3 backend tests pass locally (node mode); E2E written, deferred to CI. Review: no high/critical findings. Admission now exempts CORS preflight. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Vale's Infrahub.spelling rule flagged the possessive "API's". Drop the
possessive ("The API CORS allowed-headers list") to clear the
validate-release-notes-style check without changing meaning.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Background flows set client.request_context from their InfrahubContext or EventContext, so the already-resolved WorkflowPriority is emitted as X-Priority on their outgoing SDK requests and the admission layer can protect frontend traffic. EventContext now carries the priority (surviving the event serialization round-trip) and the workflow queue router honors it. Bumps the python_sdk submodule to the RequestContext.priority commit. Part of INFP-636. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Align the admission `Priority` enum with `WorkflowPriority` (high/medium/low): the header value and label become `medium`, and the "absent/unknown → middle tier" default is preserved (`MEDIUM = 1`, so ordering is unchanged and old `normal` callers still resolve to the same class). With all three enums sharing values, the WorkflowPriority→SDK Priority conversion drops its lookup table for a plain value cast. Updates the frontend fallback description, the knowledge doc, and the changelog. Bumps python_sdk to the matching rename commit. Part of INFP-636. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the infrahub_db_query_available_after_seconds histogram, capturing Neo4j's server-side time-to-first-record (result_available_after) for the permission-check queries the request admission path relies on (account_global_permissions, account_object_permissions). Labelled by query/type/runtime with a 200ms bucket boundary, it isolates database latency from Infrahub-side processing as a health signal for backpressure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Derive a database-stress signal from the reference permission query (account_global_permissions): a per-worker tracker keeps the all-time fastest measured execution time as a floor plus a rolling window, and exposes how much slower the database currently is via new Prometheus gauges. Only read executions feed the signal so writes cannot pollute the floor, and a companion consumed-after histogram records the server-side stream time for the tracked permission queries. Make admission shedding graduated and tiered by that signal, alongside the existing CoDel controller: as a class's stress ratio climbs past its per-class trigger, a growing fraction of the class is shed (20/50/80% at 1-2x/2-5x/>=5x) rather than the whole class at once. Triggers are tiered (low 5x, medium 20x, high 100x) so interactive high-priority traffic is protected until the database is under extreme load. Stress-driven sheds are reported under a distinct reason="stress" label. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sync the generated configuration reference with the current defaults (stress window 20s, min samples 5) so docs.validate stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The database-stress signal now self-times the reference query, so the server-side available-after and consumed-after histograms and their tracked-query set are no longer read; remove them (and the changelog fragment that announced the unreleased available-after histogram). Also drop the stress ratio_min gauge and its calculation: it duplicated the floor and window-min gauges without adding signal. The median ratio remains the shed-decision signal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0d0c3ae to
1664fbe
Compare
Lower the DB connection pool to 50 and the admission concurrency factor to 0.5 (a tighter per-worker slot cap), and raise the low/medium stress shed triggers to 10x/25x so shedding kicks in later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 10 unresolved issues from previous reviews.
Re-trigger cubic
Add a backend knowledge doc for the admission layer (middleware, CoDel, priority slot pool, derived capacity) and the database-stress signal (reference query, rolling-median window, tiered graduated shedding), including the CORS-preflight and metrics-accounting limitations. Wire it into backend/AGENTS.md and architecture.md, cross-link the frontend request-priority doc, and note the normal->medium rename plus the added stress signal in the IFC-2886 spec, which predates both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ifc-2886-priority-api-backpressure/contracts/x-priority-header.md">
<violation number="1" location="dev/specs/ifc-2886-priority-api-backpressure/contracts/x-priority-header.md:7">
P2: Contract body still uses 'normal' throughout, but the added note says shipped values are high/medium/low. Update the table and body to say 'medium' directly so the contract is self-consistent instead of asking readers to mentally substitute.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| This is a **transport-layer contract**. It adds one request header and one possible response outcome. It does **not** change any REST/GraphQL request body, GraphQL schema, or existing 2xx/4xx/5xx semantics of the handlers themselves. | ||
|
|
||
| > **Naming note**: the middle tier was renamed `normal` → `medium` after this contract was written. The shipped header values are `high` / `medium` / `low`; read `normal` as `medium` below. |
There was a problem hiding this comment.
P2: Contract body still uses 'normal' throughout, but the added note says shipped values are high/medium/low. Update the table and body to say 'medium' directly so the contract is self-consistent instead of asking readers to mentally substitute.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-2886-priority-api-backpressure/contracts/x-priority-header.md, line 7:
<comment>Contract body still uses 'normal' throughout, but the added note says shipped values are high/medium/low. Update the table and body to say 'medium' directly so the contract is self-consistent instead of asking readers to mentally substitute.</comment>
<file context>
@@ -4,6 +4,8 @@
This is a **transport-layer contract**. It adds one request header and one possible response outcome. It does **not** change any REST/GraphQL request body, GraphQL schema, or existing 2xx/4xx/5xx semantics of the handlers themselves.
+> **Naming note**: the middle tier was renamed `normal` → `medium` after this contract was written. The shipped header values are `high` / `medium` / `low`; read `normal` as `medium` below.
+
## Request: `X-Priority` (new, optional)
</file context>
The compose config anchor was stale after the new INFRAHUB_API_BACKPRESSURE_* stress settings and the retuned pool-size / concurrency-factor defaults were added. Regenerated via release.update-docker-compose so release.validate-dockercomposeenv passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker-compose.yml">
<violation number="1" location="docker-compose.yml:22">
P2: Seven new `INFRAHUB_API_BACKPRESSURE_*` env vars were added to docker-compose.yml but I couldn't verify corresponding Python config fields consume them. If these map through pydantic-settings auto-discovery, the field names need to match the env-var suffix convention. Recommend verifying each variable links to a defined Python config field; otherwise these defaults are dead config.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| INFRAHUB_ANALYTICS_API_KEY: | ||
| INFRAHUB_ANALYTICS_ENABLE: ${INFRAHUB_ANALYTICS_ENABLE:-true} | ||
| INFRAHUB_ANONYMOUS_ACCESS_ROLE: ${INFRAHUB_ANONYMOUS_ACCESS_ROLE:-Anonymous User} | ||
| INFRAHUB_API_BACKPRESSURE_BACKSTOP_HIGH_MULTIPLIER: ${INFRAHUB_API_BACKPRESSURE_BACKSTOP_HIGH_MULTIPLIER:-4.0} |
There was a problem hiding this comment.
P2: Seven new INFRAHUB_API_BACKPRESSURE_* env vars were added to docker-compose.yml but I couldn't verify corresponding Python config fields consume them. If these map through pydantic-settings auto-discovery, the field names need to match the env-var suffix convention. Recommend verifying each variable links to a defined Python config field; otherwise these defaults are dead config.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docker-compose.yml, line 22:
<comment>Seven new `INFRAHUB_API_BACKPRESSURE_*` env vars were added to docker-compose.yml but I couldn't verify corresponding Python config fields consume them. If these map through pydantic-settings auto-discovery, the field names need to match the env-var suffix convention. Recommend verifying each variable links to a defined Python config field; otherwise these defaults are dead config.</comment>
<file context>
@@ -19,13 +19,20 @@ x-infrahub-config: &infrahub_config
INFRAHUB_ANALYTICS_API_KEY:
INFRAHUB_ANALYTICS_ENABLE: ${INFRAHUB_ANALYTICS_ENABLE:-true}
INFRAHUB_ANONYMOUS_ACCESS_ROLE: ${INFRAHUB_ANONYMOUS_ACCESS_ROLE:-Anonymous User}
+ INFRAHUB_API_BACKPRESSURE_BACKSTOP_HIGH_MULTIPLIER: ${INFRAHUB_API_BACKPRESSURE_BACKSTOP_HIGH_MULTIPLIER:-4.0}
+ INFRAHUB_API_BACKPRESSURE_BACKSTOP_LOW_MULTIPLIER: ${INFRAHUB_API_BACKPRESSURE_BACKSTOP_LOW_MULTIPLIER:-0.5}
INFRAHUB_API_BACKPRESSURE_BACKSTOP_MAX_WAITERS: ${INFRAHUB_API_BACKPRESSURE_BACKSTOP_MAX_WAITERS:-1000}
</file context>
Why
When an Infrahub instance runs heavy background work (generators, artifacts, diffs, repo syncs, computed attributes) while a human uses the frontend, both compete for the same finite uvicorn worker pool and Neo4j connection pool. Today the API is origin-blind — frontend and background requests are indistinguishable — so under heavy background load the API can no longer serve the frontend and the app appears to hang.
Goal: keep the frontend responsive under background overload by shedding load by priority, with no per-customer tuning.
Non-goals (out of scope, tracked separately): the client/SDK backoff that consumes
Retry-After, the frontend settingX-Priority: high, and enforcement that ahighclaim is legitimate. This PR is server-side only.Implements Jira IFC-2886 (which implements JPD idea INFP-636).
What changed
Behavioral changes (what operators/callers observe):
X-Priorityheader (high/normal/low); missing or invalid →normal, so the layer is safe to deploy before any caller is updated.429 Too Many Requests+Retry-Afterwith no handler work performed. Shedding is adaptive (CoDel on measured slot-wait / sojourn time), not a fixed threshold, and self-terminates within a bounded window after overload ends./metricsendpoint (infrahub_admission_*): offered, admitted, rejected-by-reason (codel/backstop), in-flight, waiters, sojourn histogram, derivedmax_concurrency, and a no/invalid-priority counter for adoption tracking.INFRAHUB_API_BACKPRESSURE_*) including a kill-switch (backpressure_enabled, default on) and a newINFRAHUB_DB_MAX_CONNECTION_POOL_SIZE(default100, now passed explicitly to the Neo4j driver).Implementation notes:
backend/infrahub/api/admission/: a pure-ASGIAdmissionMiddleware(registered outermost), a cancellation-safePrioritySlotPool(per-class FIFO, priority hand-off, modelled onasyncio.Semaphore), a pureCoDelControllerwith an injected clock, anAdmissionControllercomposing them, capacity derivation, and the metrics module.max_concurrencyis derived from the process's own Neo4j pool size — no hard-coded constant.What stayed the same:
prometheus-clientwas already present transitively and imported in-tree; it is now declared explicitly).X-Priority, everything isnormaland nothing is shed until load exceeds the derived cap.Suggested review order
contracts/x-priority-header.mdandcontracts/metrics.mdin the spec dir — the external contract.slot_pool.pyandcodel.py— the concurrency core (cancellation safety, CoDel state machine).controller.py→middleware.py→server.py— how they wire together.backend/tests/unit/api/admission/andbackend/tests/component/api/test_admission_middleware.py.Related Context
Full spec, plan, research, data model, contracts, critique, and implementation report live in
dev/specs/ifc-2886-priority-api-backpressure/. Key success criteria:highshed rate ≈ 0% under sustained overload whilelowsheds first (SC-002); sub-interval bursts shed zero (SC-003); every shed is a fast429 + Retry-Afterwith no handler work (SC-004); shedding self-terminates after overload (SC-005).The headline latency bound (SC-001) is intentionally discovery-measured against a live overload scenario (see Test Plan) rather than pre-committed.
Documentation Updates
changelog/+ifc-2886.added.md.docs/docs/reference/configuration.mdxfor the new settings.How to review / test
Both suites pass locally with no external infrastructure (42 tests). CI to watch: backend unit/component, ruff/mypy lint, and
validate-generated-documentation(the regenerated config reference is committed).Live overload / SC-001 discovery (manual, needs a running server):
Impact & rollout
X-Priorityheader →normal, no behaviour change under normal load. No request/response body change for non-shed requests.INFRAHUB_API_BACKPRESSURE_*settings andINFRAHUB_DB_MAX_CONNECTION_POOL_SIZE(default 100 preserves current driver behaviour).INFRAHUB_API_BACKPRESSURE_ENABLED=falseis the instant rollback. Interactive protection is fully realized once the frontend sendsX-Priority: high(separate ticket); until then interactive traffic isnormal.X-Priorityrequest header +429behaviour (public API surface); the middleware sits in the admission path and reads a client-controlled header (borderline auth, no auth logic changed);X-Priorityis untrusted and cooperative in v1 (enforcement deferred).Checklist
changelog/+ifc-2886.added.md)configuration.mdx)dev/specs/ifc-2886-priority-api-backpressure/)🤖 Generated with Claude Code
Summary by cubic
Adds priority-aware backpressure to keep the API responsive under heavy background and database load: low-priority requests get a fast 429 + Retry-After, while interactive traffic stays fast via
X-Priority. Implements IFC-2886 and IFC-2890 end to end.New Features
INFRAHUB_DB_MAX_CONNECTION_POOL_SIZE; CoDel-based shedding (lowfirst,mediumdefault); path-aware REST/GraphQL 429 + Retry-After; excludes health/static/metrics/schema and CORS preflight.low10x,medium25x,high100x); rejected metrics labelreason="stress".infrahub_admission_*metrics (offered,admitted,rejectedbycodel|backstop|stress,in_flight,waiters,sojourn,max_concurrency); DB-stress gauges;INFRAHUB_API_BACKPRESSURE_*knobs and kill-switch;prometheus-clientdeclared; regenerated config docs (stress window 20s, min samples 5); tuned defaults (INFRAHUB_DB_MAX_CONNECTION_POOL_SIZE=50,INFRAHUB_API_BACKPRESSURE_MAX_CONCURRENCY_FACTOR=0.5); backend knowledge doc added and linked frombackend/AGENTS.mdand architecture; docker-compose env-var block updated with backpressure settings; no endpoint/schema changes.X-Priority(highby default,lowopt-in; preserved across retries/uploads; only for the Infrahub API origin); background flows propagate workflow/event priority to SDKRequestContext.priority; backend CORS allowsx-priority.Bug Fixes
in_flight/waitersgauges now update on every enqueue/dequeue/acquire/release and on handler errors; slots are always released; accounting invariants clarified.Written for commit a8abbde. Summary will update on new commits.
Update since original description
normal→medium. Header values are nowhigh/medium/low; a missing or invalid value resolves tomedium. Read everynormalabove asmedium.reason="stress"alongsidecodel/backstop. This augments the CoDel sojourn signal described above.100 → 50, admission concurrency factor1.0 → 0.5, plus newINFRAHUB_API_BACKPRESSURE_*stress knobs.dev/knowledge/backend/api-backpressure.mddocuments the admission layer and the stress signal (the IFC-2886 spec predates the stress signal and the rename).