Skip to content

Stop calling the startup warning the only sign of an accept stall - #370

Merged
diolektor merged 1 commit into
mainfrom
fix/architecture-overview-omits-runtime-accept-stall-signals
Sep 19, 2026
Merged

diolektor merged 1 commit into
mainfrom
fix/architecture-overview-omits-runtime-accept-stall-signals

Conversation

@diolektor

Copy link
Copy Markdown
Contributor

Problem

docs/architecture/overview.md ended the connection-budget paragraph with "The server warns about that at startup." That was true when it was written — the startup check was the only signal there was. It is not true now. An accept that has to wait for a MAX_CONNECTIONS permit logs a WARN naming the budget, the permit arriving logs an INFO with how long the loop was parked, and two metrics export the state: oxphp_accept_stalled and oxphp_accept_stalls_total.

The configuration reference, the metrics reference and the health-checks page all describe them. The architecture overview was the last public page where the state still read as unobservable at runtime — and it is the page a reader consults when sizing a deployment for overload, so the conclusion it invited was that an exhausted connection budget can only be anticipated, never watched.

The sentence beside it made a second, quieter mistake. "Clearing it is necessary rather than sufficient: connections that never reach PHP hold permits too" left the startup check and the stall itself looking like one mechanism. They are not. The startup check reads configuration only — PHP_WORKERS + QUEUE_CAPACITY + QUEUE_MAX_WAITING against MAX_CONNECTIONS, evaluated once — and its own doc comment says what follows from that: "A configuration this clears can still exhaust the budget." A park caused by slow clients, idle keep-alives or long h2 streams is one it cannot predict, and that is exactly the case where the runtime signals are all an operator has. Naming the signals without separating the two mechanisms would have left the reader with the more useful half still attached to the wrong one.

What changed

  • Both runtime signals are named in the paragraph, with the qualification that matters: the log pair is rate-limited, so a server flapping at the ceiling does not turn its own log into the outage. The metrics are not — accept_stall_begin() runs as the first statement of the no-permits branch, before the suppression window is computed, so the two series are updated on every park including the ones the log passes over in silence. The paragraph says so, and links to the metrics reference for the rest.
  • The metrics are noted as served outside the budget. /metrics is routed only by the internal listener, whose accept loop takes no MAX_CONNECTIONS permit at all — which is why the series can be read during a stall, when the main listener is answering nothing.
  • Clearing it became Clearing the startup warning. The pronoun was ambiguous between the warning and the condition. The sentence gains a tail saying that a park caused by connections which never reach PHP is one the startup check cannot predict, which is the distinction above stated where the reader meets it.
  • llms-full.txt regenerated.

Two wordings this change carried in its own first draft were dropped before review, both in the direction of promising more than the code does:

  • "a park they cause is visible only through those runtime signals" — a false absolute. oxphp_active_connections reaching its ceiling remains an indirect indication; it is awkward to use, because reading it requires knowing the configured budget and comparing two sources, but it exists. The sentence now makes a claim about the startup check instead, which is the claim that actually holds.
  • "which record every park" → "which are updated on every park". "Record" reads as observability at scrape time, and for the gauge that is not guaranteed — a stall that starts and ends between two scrapes is exactly what the counter beside it exists to catch. The rate-limit qualification concerns the moment of writing, not what a scraper will see, and the wording now keeps those apart.

No behaviour changes — documentation only.

Verification

  • scripts/gen-llms-txt.sh --check — up to date, 58 pages.
  • scripts/check-links.sh — 649 in-repo links, none broken (the one this change adds included).
  • cargo fmt --check, cargo clippy --no-default-features -D warnings, cargo test --no-default-features — 1170 passed, 0 failed, 3 ignored. No Rust or C source is touched on this branch, so these cannot be affected by it; CI runs them regardless.
  • Every sentence the change adds was checked against the code rather than against the reference pages that already describe the same behaviour: the startup warning and the configuration-only check behind it in src/config/mod.rs; the WARN, the INFO carrying stalled_secs, the five-second suppression window and the unconditional counter increment ahead of it in src/main.rs; the gauge and counter definitions and their exposition types in src/metrics.rs; and the absence of any connection semaphore on the internal listener in src/server/internal.rs, which is what makes the metrics readable through a stall.
  • A repo-wide grep for the class — the accept loop parking, the connection budget, the startup warning — across docs/, README.md and CHANGELOG.md finds one page that was incomplete, and this is it. The configuration reference, the metrics reference, the health-checks page and the released changelog entry describing these signals are all accurate and are left alone. The released QUEUE_MAX_WAITING changelog entry mentions the startup check without claiming it is the only signal, so it is correct as history and is also left alone.

Docs:
  - The architecture overview ended the connection-budget paragraph on the startup warning, so a reader planning for overload concluded the state cannot be watched at runtime — the last public page still reading that way. It now names both signals: a WARN when the loop parks and an INFO with the duration when it resumes, rate-limited so a server flapping at the ceiling does not turn its own log into the outage, plus the oxphp_accept_stalled gauge and the oxphp_accept_stalls_total counter, which are updated on every park including the ones the rate limit passes over.
  - The sentence beside it treated the startup warning and the stall as one mechanism. The check reads configuration only, so a park caused by slow clients or idle keep-alives is one it cannot predict — which is exactly when the runtime signals are all an operator has.
@diolektor
diolektor merged commit 9156749 into main Sep 19, 2026
7 checks passed
@diolektor
diolektor deleted the fix/architecture-overview-omits-runtime-accept-stall-signals branch September 19, 2026 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant