Skip to content

fix(net): cap coordinator request bodies, bound baseline threads, index hot lookups, throttle rate-limit sweep - #49

Merged
wikithoughts merged 1 commit into
mainfrom
overnight/coordinator-request-limits
Oct 11, 2026
Merged

wikithoughts merged 1 commit into
mainfrom
overnight/coordinator-request-limits

Conversation

@wikithoughts

Copy link
Copy Markdown
Owner

Fixes review findings F11, F12, F13, F16 (overnight review of 30ae508, task T9). None of the four had been addressed on main (checked af2e814). PRs #29–#47 touched other findings, and grep on main showed no CREATE INDEX, no body cap outside put_blob, and the per-job pw-baseline thread still in place.

What changed

  • F11 – request-size cap (net/coordinator_app.py): a pure-ASGI BodySizeLimit middleware rejects bodies larger than PW_MAX_BODY_BYTES with 413 before any parsing or auth.
    • Default is 4 MiB. That is above the largest legitimate body: a 200k-char /deliver, JSON-escaped by json.dumps, is at most about 2.4 MB.
    • A declared Content-Length over the cap is refused before the app runs.
    • A chunked or no-length body is counted as it streams and cut off at the first message past the cap.
    • Invalid or non-positive settings fall back to the default; there is no unbounded mode.
    • The cap is documented in the module docstring. Blob uploads keep their own 512 KiB cap.
  • F12 – bounded baseline (net/coordinator_app.py): at most PW_BASELINE_WORKERS (default 4) pw-baseline threads run at once, gated by a non-blocking semaphore. A job submitted while every slot is busy skips its best-effort baseline and logs a line; nothing queues.
    • These are daemon threads, not a ThreadPoolExecutor, because executor workers are joined at interpreter exit and the 900 s settle-wait would stall coordinator shutdown.
    • A baseline that raises still releases its slot.
  • F13 – SQLite indexes (net/store.py): CREATE INDEX IF NOT EXISTS runs on every Store open, so existing DBs get the indexes too. It covers nodes(secret_hash), users(secret_hash), tasks(job_id), tasks(node_id,status), jobs(asker,created) and jobs(status).
  • F16 – rate-limiter sweep (net/ratelimit.py):
    • Once over max_keys, the O(n) sweep runs at most once per 10 s instead of on every allow().
    • It drops idle keys first, then evicts the least-recently-hit keys back to max_keys.
    • An evicted key only loses its own history, so it is never more limited than before.
    • One existing assertion in test_sweep_drops_idle_keys_on_overflow was updated for this eviction; its idle-sweep check is unchanged.

Left open (outside this lane's ownership)

  • F11 per-field caps (RegisterBody.profile, JobBody.requires/then) were not added; this lane covered the middleware and executor only. The 4 MiB cap now bounds them in aggregate.
  • F16's XFF part (key on the right-most trusted hop in _client_key) is not changed.
  • New env knobs PW_MAX_BODY_BYTES and PW_BASELINE_WORKERS are documented in code only; docs/network/ was not updated (out of ownership).

Verification

Command Exit Result
python -m py_compile $(git ls-files '*.py') 0 —
ruff check . && pytest tests/ -q --cov=passiveworkers --cov-fail-under=75 0 All checks passed!, 782 passed, 1 skipped, coverage 83.49%, no warnings
New/changed tests against origin/main's coordinator_app.py/store.py/ratelimit.py 1 13 failed, 6 passed — every F-item has failing tests on old code. The 6 that pass are the pre-existing ratelimit behaviour tests plus the under-cap pass-through check.

Local env: Python 3.12.3, .venv in the worktree. No browser check because no served UI changed. CI was not watched (lane rule); the required ci-ok result is unverified at PR open.

🤖 Generated with Claude Code

…ex hot lookups, throttle rate-limit sweep

- F11: pure-ASGI BodySizeLimit middleware refuses bodies over PW_MAX_BODY_BYTES
  (default 4 MiB) with 413 before parsing or auth, both on a declared
  Content-Length and while streaming a chunked/no-length body.
- F12: baselines run on at most PW_BASELINE_WORKERS (default 4) daemon threads
  gated by a semaphore; a job submitted while all are busy skips its baseline.
- F13: CREATE INDEX IF NOT EXISTS on nodes(secret_hash), users(secret_hash),
  tasks(job_id), tasks(node_id,status), jobs(asker,created), jobs(status) on
  every Store open (existing DBs included).
- F16: the over-capacity sweep runs at most once per 10 s and evicts the
  least-recently-hit keys back to max_keys when no key is idle.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wikithoughts
wikithoughts merged commit 722caf1 into main Oct 11, 2026
11 checks passed
@wikithoughts
wikithoughts deleted the overnight/coordinator-request-limits branch October 11, 2026 08:32
wikithoughts added a commit that referenced this pull request Oct 11, 2026
Root cause of PR #48's test (3.10) abort (CI run 38124448503, exit 134,
"Fatal Python error: _enter_buffered_busy ... possibly due to daemon threads").
It predates this PR; the fetch watchdog is not involved. Every POST /jobs in
the suite starts a pw-baseline daemon thread (net/coordinator_app.py). The
thread polls for up to 900 s, tries Ollama on loopback with the default
PW_BASELINE_LOCAL_MODEL, and then prints. One still running when the suite
ended printed "[baseline] generation failed" into interpreter shutdown, and
CPython 3.10 aborted.

An autouse conftest fixture now refuses to start a pw-baseline thread while the
coordinator module's current _baseline_async is the real one. It raises
RuntimeError, which takes _start_baseline's own "can't start a thread" path
(added in #49), so the slot is released. Emptying PW_BASELINE_LOCAL_MODEL is not
enough: the thread still starts, polls and prints. A module-level patch is
undone by the importlib.reload() calls many tests make, so the guard sits on
Thread.start. The tests from #49 that swap in their own _baseline_async still
get real threads.

On Python 3.10.21 with the [all,dev] extras, a thread probe at session end
before this change: no pw-baseline threads at exit, but [baseline] lines did
print mid-suite. After: 0 pw-baseline threads at exit and 0 [baseline] lines.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
wikithoughts added a commit that referenced this pull request Oct 11, 2026
…nd gzip drips (#48)

* fix(research): hard wall-clock fetch deadline that also cuts header and gzip drips

#31 gave fetch_extract a 30 s deadline, but it is only checked between
redirect hops and between body chunks. A server dripping one byte every few
seconds through its status line/headers (inside requests' header parse), or
through a gzip header comment (urllib3's read1 loops over it internally),
never trips a per-read timeout and still held the fetch thread for as long as
it kept dripping: 4.0 s against a 0.5 s deadline in a loopback probe, i.e.
unbounded (review F8 residual, noted in _guarded_get's own docstring).

fetch_extract now GETs through a per-fetch requests Session whose pools hand
every new socket to a _FetchWatchdog. At the deadline the watchdog shuts down a
dup() of each socket, so any blocked recv returns at once, and fetch_extract
raises TimeoutError. A body cut short by the watchdog is never returned as
complete. Dups are closed and the timer cancelled on every path.

Tests: header, gzip-comment and TLS-handshake drips against a loopback stub
(header drip ran 8.1 s on main, now about 0.5 s); a socket connected after the
deadline is cut at once; a real loopback page still fetches through the
watched session with no timer thread or fd left behind. Existing fetch_extract
tests now patch Session.get as well as requests.get.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(research): join the fetch watchdog timer so it never outlives fetch_extract

_FetchWatchdog.close() cancelled its threading.Timer but did not join it, so the
pw-fetch-deadline daemon thread could still be alive for a moment after
fetch_extract returned. close() now cancels and then joins it. join() only waits
out a _fire that has already started, which holds the lock briefly and makes
non-blocking shutdown() calls. The watchdog never writes to stdout.

New test: no pw-fetch-deadline thread is alive right after fetch_extract
returns, on success, on error and when the deadline fires. The real-socket
happy-path test now checks this directly instead of polling for up to 1 s.
Both tests fail against the cancel-only close().

Note: this does not explain PR #48's test (3.10) abort (CI run 38124448503).
The lines printed just before "Fatal Python error: _enter_buffered_busy" come
from the coordinator's pw-baseline daemon thread (net/baseline.py:42 and
net/coordinator_app.py:413). Tests that POST /jobs start that thread with the
default PW_BASELINE_LOCAL_MODEL, and it races interpreter shutdown. That code is
outside this change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test: never start the coordinator's real baseline thread in tests

Root cause of PR #48's test (3.10) abort (CI run 38124448503, exit 134,
"Fatal Python error: _enter_buffered_busy ... possibly due to daemon threads").
It predates this PR; the fetch watchdog is not involved. Every POST /jobs in
the suite starts a pw-baseline daemon thread (net/coordinator_app.py). The
thread polls for up to 900 s, tries Ollama on loopback with the default
PW_BASELINE_LOCAL_MODEL, and then prints. One still running when the suite
ended printed "[baseline] generation failed" into interpreter shutdown, and
CPython 3.10 aborted.

An autouse conftest fixture now refuses to start a pw-baseline thread while the
coordinator module's current _baseline_async is the real one. It raises
RuntimeError, which takes _start_baseline's own "can't start a thread" path
(added in #49), so the slot is released. Emptying PW_BASELINE_LOCAL_MODEL is not
enough: the thread still starts, polls and prints. A module-level patch is
undone by the importlib.reload() calls many tests make, so the guard sits on
Thread.start. The tests from #49 that swap in their own _baseline_async still
get real threads.

On Python 3.10.21 with the [all,dev] extras, a thread probe at session end
before this change: no pw-baseline threads at exit, but [baseline] lines did
print mid-suite. After: 0 pw-baseline threads at exit and 0 [baseline] lines.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
wikithoughts added a commit that referenced this pull request Oct 11, 2026
…50)

Follow-ups from the PR #49 review:

- Per-route request-body caps under the PW_MAX_BODY_BYTES ceiling (4 MiB
  default): /tasks/{id}/deliver is capped at 2,416,384 bytes (a 200k-char
  deliverable fully JSON-escaped, plus signature fields), field-capped
  small bodies (/users, /nodes/register, heartbeat, progress, accept,
  feedback, rate, /admin/enroll) and non-POST requests at 64 KiB. The
  free-form routes (/result dicts, /jobs, blob uploads) keep the ceiling
  because they have no documented smaller bound. A lower ceiling still
  lowers every cap.
- A baseline skipped because every PW_BASELINE_WORKERS slot is busy is
  now counted (baseline_skipped_busy on GET /metrics) and the log line
  names the running total.
- Tests: an exact-cap /deliver body passes and cap+1 is 413, small-route
  caps (declared and chunked), the route-to-cap mapping, and the skip
  counter. README documents PW_MAX_BODY_BYTES and PW_BASELINE_WORKERS.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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