Skip to content

fix(net): per-route coordinator body caps and counted baseline skips - #50

Merged
wikithoughts merged 1 commit into
mainfrom
overnight/coordinator-route-caps
Oct 11, 2026
Merged

wikithoughts merged 1 commit into
mainfrom
overnight/coordinator-route-caps

Conversation

@wikithoughts

Copy link
Copy Markdown
Owner

Follow-ups from the PR #49 review (claude-review-pw-49.md: findings 1, 2 and 4). #49 merged as 722caf1.

What changed

  • Per-route body caps (passiveworkers/net/coordinator_app.py, BodySizeLimit via the new _body_cap_for). PW_MAX_BODY_BYTES (default 4 MiB) is now the ceiling. Routes with a documented smaller bound get that bound:
    • POST /tasks/{id}/deliver: _DELIVER_BODY_CAP = 2,416,384 bytes. That is 200k chars × 12 bytes (an astral char JSON-escaped as a surrogate pair) plus 16 KiB for signature/signer_pub/keys.
    • POST /users, /nodes/register, /nodes/heartbeat, /admin/enroll, /tasks/{id}/progress|accept and /jobs/{id}/feedback|rate, plus every non-POST request: 64 KiB.
    • /tasks/{id}/result (bare dict), /jobs (free-form requires/then, 200 items) and blob uploads (which keep their own 512 KiB check) keep the ceiling. None of them has a documented smaller bound, and the largest real node result is unknown, so a tighter cap could drop legitimate results.
    • A lower PW_MAX_BODY_BYTES still lowers every cap.
  • Baseline skipped while slots are busy: _start_baseline now counts each skip. The count is exposed as baseline_skipped_busy on GET /metrics, and the log line now gives the running total: [baseline] skipped for job X: all N slots busy (PW_BASELINE_WORKERS); K skipped since start. The baseline.py docstring no longer says "one thread per job" and describes the bounded slots.
  • Docs: the README network section documents PW_MAX_BODY_BYTES and the per-route caps, plus PW_BASELINE_WORKERS and its skip behaviour. Neither variable was documented before. This PR adds no new env knob.

Findings addressed

  1. A flat 4 MiB pre-auth cap applied to every route, so unauthenticated POST /users could make the server buffer 4 MiB. It now gets 64 KiB, and /deliver gets its exact derived cap. /result is unchanged; see "Unverified".
  2. Baselines were silently skipped under load. Skips are now counted, logged with a total and documented.
  3. There was no test that a max-size /deliver body passes. New test: a worst-case 200k-astral-char deliverable, padded to exactly the cap, reaches the endpoint (200), and cap+1 gets a 413 without running the endpoint.

Tests (tests/test_coordinator_limits.py, +13)

  • /deliver: a body exactly at the cap passes, and cap+1 gets a 413.
  • The small-route 64 KiB cap, checked on 8 routes, both declared (cap+1 → 413, cap → normal path) and chunked.
  • The free-form routes still accept 1 MiB.
  • The route→cap mapping and the ceiling override.
  • The skip counter, log line and /metrics key.
  • Against old code (origin/main coordinator_app.py with the new tests): 12 failed, 12 passed. The only new test that passes on old code is the ceiling-preservation test, which guards unchanged behaviour by design.

Verify

  • ruff check . && pytest tests/ -q --cov=passiveworkers --cov-fail-under=75 → exit 0: All checks passed!, 799 passed, 1 skipped in 42.43s, Total coverage: 83.61%
  • python -m py_compile $(git ls-files '*.py') → exit 0
  • No browser-served HTML/JS changed (/metrics gains a key; app.py's only consumer reads named fields), so no browser check was run.

Unverified / limitations

  • The largest real node /result payload is still unknown, so /result keeps the 4 MiB ceiling rather than a guessed smaller cap. Bounding the result dict per field would need changes in the owner-reviewed store/ledger files, which are out of scope here.
  • Routes are matched on scope["path"]. If the coordinator is mounted under a root_path prefix, the small/deliver patterns don't match and those routes fall back to the ceiling, which is the pre-PR behaviour. Nothing is broken, but nothing is tightened either.
  • Review finding 3 (LRU eviction needs ~50k source IPs, ratelimit.py) is outside this lane's file ownership and was left as-is; the review rated it acceptable.
  • CI was not polled (lane rule).

🤖 Generated with Claude Code

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>
@wikithoughts
wikithoughts merged commit 7b0e3c0 into main Oct 11, 2026
11 checks passed
@wikithoughts
wikithoughts deleted the overnight/coordinator-route-caps branch October 11, 2026 09:20
wikithoughts added a commit that referenced this pull request Oct 11, 2026
…ttled baseline-skip log (#52)

Follow-ups from the PR #50 review:
- Blob uploads get their own route cap, _BLOB_BODY_CAP = _BLOB_CAP + 16 KiB slack, derived from
  the handler's chunk constant (now defined with the other caps), instead of the 4 MiB ceiling.
- _body_cap_for matches the app-relative path: a whole-segment root_path prefix (uvicorn
  --root-path puts it in scope["path"]) and trailing slashes are stripped first, as Starlette's
  router does, so /pw/users and /users/ get /users's 64 KiB cap instead of the ceiling.
- The "baseline skipped" log line is printed for the first skip and every 100th; the
  /metrics counter stays exact.
- README cap wording fixed (blob uploads are no longer "at the ceiling").
- Tests: exact-cap passes and cap+1 is 413 for /users, /result, /jobs and blobs; GET with a
  >64 KiB body is 413; trailing-slash and root_path-prefixed paths get the route cap; the
  log throttle keeps an exact count.

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