From e52c68f47128d8e906a6319d4048462d3e041970 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 19 Aug 2026 11:00:49 +0000 Subject: [PATCH] fix(controller): admission-gate OPCode_schedAdmin's blocking wait (Req 5 AC 10) The schedAdmin dispatch case blocks its vtask handler thread for up to 5s waiting on a cross-process reply while holding c->sched.lock -- bounded blocking on a vtask thread, which ha-async-io.md permits only under an admission reservation. The real risk isn't just the wait itself: since c->sched.lock serializes concurrent requests one at a time, a burst of N concurrent schedAdmin calls could already occupy N vtask threads for up to N*5s each before this fix. Now acquires vtaskmgr_blocking_acquire(&c->vtaskmgr) before ever locking c->sched.lock, releasing on every exit path -- the same shared admission budget commit.c's commit-wait barrier already gates on (wired via commit_set_pool). Admission-exhausted requests fast-reject with {ok=false, error="server busy, try again"} rather than queuing. New test: test_schedadmin_fast_rejects_when_admission_exhausted, using a poison c->sched.ep sentinel to prove the fast-reject happens before any lock/forward attempt. --- .kiro/specs/scheduler-core/design.md | 6 +- .kiro/specs/scheduler-core/requirements.md | 3 + src/controller/cluster.c | 35 +++++++++++ .../testsuite/test_sched_admin_dispatch.c | 62 +++++++++++++++++++ 4 files changed, 103 insertions(+), 3 deletions(-) diff --git a/.kiro/specs/scheduler-core/design.md b/.kiro/specs/scheduler-core/design.md index d9a3fa4c..f76c7c86 100644 --- a/.kiro/specs/scheduler-core/design.md +++ b/.kiro/specs/scheduler-core/design.md @@ -682,7 +682,7 @@ struct SchedProject $C.mapname("xce_sched_project_t") { **Responsibilities & Constraints** - Structurally mirrors the existing `OPCode_upgradeConfig` case (`cluster.c:670-705`) for the primary-gate/delegate/explicit-reply shape: primary-only gate via `ha_is_primary()`, explicit reply on every path, 5s timeout. - Owns zero recording state — every field of `SchedAdminRequest`/`Reply` passes through untouched. -- **Blocking-wait scope decision (accepted, not hardened)**: unlike `OPCode_upgradeConfig` (in-process commit-barrier wait, already `COMMIT_WAIT_RESERVED`-admitted), this case blocks its `vtask` handler thread for up to 5s waiting on a cross-process reply from the scheduler daemon while holding `c->sched.lock`. Reviewed against `ha-async-io.md`'s vtaskmgr admission rule and accepted as a boundary case (project owner, 2026-08-02): `xceadmin scheduler` commands are cluster-administrator-triggered, low-frequency, human-paced — not a request class expected to burst concurrently the way heartbeat/replication traffic does. No admission-reservation mechanism (`COMMIT_WAIT_RESERVED`-style) is added for this opcode. Revisit if usage patterns change (e.g. automated/scripted recording control at scale). +- **Blocking-wait admission (hardened 2026-08-19, design review follow-up)**: like `OPCode_upgradeConfig` (in-process commit-barrier wait, `COMMIT_WAIT_RESERVED`-admitted), this case now also admission-gates its blocking wait. Originally (accepted, project owner, 2026-08-02) this case blocked its `vtask` handler thread for up to 5s waiting on a cross-process reply from the scheduler daemon while holding `c->sched.lock`, with no reservation — a boundary case reviewed against `ha-async-io.md`'s vtaskmgr admission rule and accepted on the grounds that `xceadmin scheduler` commands are cluster-administrator-triggered, low-frequency, human-paced traffic, not expected to burst concurrently the way heartbeat/replication traffic does. A retrospective design review flagged this as a real (if low-probability) starvation risk under a burst of concurrent/scripted requests, since each blocked vtask thread occupies a worker for up to 5s and `c->sched.lock` already serializes them one at a time — N concurrent requests could occupy N threads for up to N×5s. Fix: `vtaskmgr_blocking_acquire(&c->vtaskmgr)`/`vtaskmgr_blocking_release(&c->vtaskmgr)` now wrap the ENTIRE forward+wait region (acquired before `mylock_lock(&c->sched.lock)`, released on every exit path) — the same shared budget `commit.c`'s `wait_admit_locked()`/`wait_release_locked()` already gate on (wired via `commit_set_pool(&c->commit, &c->vtaskmgr)`). Admission-exhausted requests fast-reject with `{ok=false, error="server busy, try again"}` before ever touching `c->sched.lock` — never queued. See requirements.md Req 5 AC 10's own Amendment for the full rationale. **Dependencies** - Inbound: XceAdminSchedulerCLI, via TLS+capnp (P0) @@ -694,7 +694,7 @@ struct SchedProject $C.mapname("xce_sched_project_t") { ##### API Contract | Method | Endpoint (opcode) | Request | Response | Errors | |--------|--------------------|---------|----------|--------| -| Request/Reply | `OPCode_schedAdmin` | `SchedAdminRequest` | `SchedAdminReply` | `-EAGAIN` (not primary), `"scheduler not connected"`, `"request timeout"`, `"invalid record file name"`, `"already recording"`, `"not recording"` (full table: requirements.md Req 5 AC 10) | +| Request/Reply | `OPCode_schedAdmin` | `SchedAdminRequest` | `SchedAdminReply` | `-EAGAIN` (not primary), `"scheduler not connected"`, `"request timeout"`, `"invalid record file name"`, `"already recording"`, `"not recording"`, `"server busy, try again"` (admission exhausted) (full table: requirements.md Req 5 AC 10) | **Implementation Notes** - Integration: one new `case` in `myreq_handler`'s switch (`cluster.c`); no new file. @@ -781,7 +781,7 @@ _Feature-specific targets only; general practices are in steering docs._ - **Recording overhead**: zero file I/O, zero extra allocation when `recording == false` — a single boolean branch per session (Req 5 AC 10, explicit requirement, inherited here as a design constraint on `NormalDriver`). - **Determinism vs throughput**: `jbuf` cbuf capacity is permanently 1 (rendezvous, not a buffer) — this bounds picker→allocator throughput to strictly sequential handoff. This is an intentional trade-off (determinism over pipelining) already made at the requirements level (Req 5 AC 7) and is not revisited here; any future throughput work must find a different mechanism that preserves the ordering guarantee (e.g. deterministic batching with an explicit order tag), not simply raise the cbuf capacity. - **Simulation fast mode**: `speed_num=0` means the virtual clock never advances between events at wall-clock cost — `--sim` on a large `.replay` file runs at CPU-bound speed, not wall-clock speed. No specific throughput target is set here (none given in requirements); implementation should avoid introducing any per-event syscall beyond what decoding the capnp stream requires. -- **`OPCode_schedAdmin` vtask occupancy**: each request blocks its vtask handler thread up to 5s (cross-process wait on the scheduler's reply) while holding `c->sched.lock` — no admission reservation, accepted as a boundary case given expected low, human-paced request volume (see `SchedAdminOpcode` component, Design Review 2026-08-02). Not bounded or load-tested against concurrent bursts; out of scope unless usage patterns change. +- **`OPCode_schedAdmin` vtask occupancy (hardened 2026-08-19)**: each request blocks its vtask handler thread up to 5s (cross-process wait on the scheduler's reply) while holding `c->sched.lock` — now gated by the same `vtaskmgr_blocking_acquire`/`_release` admission reservation `commit.c`'s commit-wait barrier shares (see `SchedAdminOpcode` component above). Admission-exhausted bursts fast-reject instead of occupying vtask threads; not separately load-tested against a real concurrent burst (unit-tested only, `test_schedadmin_fast_rejects_when_admission_exhausted`), but the mechanism itself is the same one already proven under `test_vtask_admission.c` and production commit-wait traffic. ## Open Questions / Risks diff --git a/.kiro/specs/scheduler-core/requirements.md b/.kiro/specs/scheduler-core/requirements.md index eb2b98ab..ad605905 100644 --- a/.kiro/specs/scheduler-core/requirements.md +++ b/.kiro/specs/scheduler-core/requirements.md @@ -655,6 +655,9 @@ if (arguments.sim_file) { | Request timeout (5s) | false | `"request timeout"` | | Disk full / write error (start) | false | `"cannot write to : "` | | `recordFile` contains `/` or `..`, or resolves outside `record_dir` | false | `"invalid record file name"` | + | Admission reservation exhausted (see Amendment below) | false | `"server busy, try again"` | + + **Amendment (2026-08-19, design review follow-up)**: the controller's forward-and-block-for-reply wait (this AC's own "Admin command flow" — held for the full duration under `c->sched.lock`, up to the 5s deadline) is bounded blocking on a vtask thread, which `ha-async-io.md`'s async-I/O discipline permits only under an admission reservation. A burst of concurrent `schedAdmin` requests could otherwise occupy N vtask threads for up to N×5s each, starving the pool the reservation exists to protect. The dispatch case now acquires the SAME shared admission budget `commit.c`'s commit-wait barrier already gates on (`vtaskmgr_blocking_acquire`/`vtaskmgr_blocking_release` against `c->vtaskmgr`, wired via `commit_set_pool`) before ever locking `c->sched.lock`, and fast-rejects with `{ok=false, error="server busy, try again"}` — never queuing — when the reservation is exhausted. Sharing the budget with commit-wait traffic is deliberate: the reservation protects a fixed number of vcpus from any class of admitted bounded-blocking work, not a per-caller quota. Status output (xceadmin formats the capnp reply): ``` diff --git a/src/controller/cluster.c b/src/controller/cluster.c index ee092661..06404fc7 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -860,6 +860,38 @@ static int myreq_handler(vtask_t* task) break; } + /* ha-async-io.md Rule 3 (design review follow-up, 2026-08-19): + * bounded blocking on a vtask thread is permitted only under an + * admission reservation. The blocking region below isn't just the + * pthread_cond_timedwait a few lines down -- mylock_lock(&c->sched. + * lock) immediately after this comment is ALSO a bounded wait (up + * to this same ~5s deadline, times however many requests are + * already queued ahead of this one), since c->sched.lock is + * deliberately held for the whole forward+wait duration (see that + * design note below). A burst of concurrent xceadmin schedAdmin + * calls could otherwise occupy N vtask threads for up to N*5s -- + * exactly the starvation pattern this reservation exists to + * prevent. Reuses the SAME shared budget commit.c's wait_admit_ + * locked()/wait_release_locked() already gate on c->vtaskmgr (wired + * via commit_set_pool(&c->commit, &c->vtaskmgr) below) -- the + * reservation protects a fixed number of vcpus from ANY class of + * admitted bounded-blocking work, not a per-caller quota, so + * schedAdmin and commit-wait traffic intentionally share it. Fast- + * reject (never touch c->sched.lock) rather than queue behind an + * exhausted cap. */ + if (vtaskmgr_blocking_acquire(&(c->vtaskmgr)) != 0) { + sched_admin_reply_t busy_reply; + memset(&busy_reply, 0, sizeof(busy_reply)); + busy_reply.error = "server busy, try again"; + + reply.status = 0; + reply.data_which = RequestReply_data_schedadminreplymsg; + reply.data.schedadminreplymsg = &busy_reply; + xce_reply(ep, c->myhost, &reply); + rc = 0; + break; + } + /* c->sched.lock is held for the FULL forward+wait duration below -- * an accepted trade-off (task 10.1 design notes): xceadmin admin * commands are low-frequency, human-paced traffic, not a hot path. @@ -874,6 +906,7 @@ static int myreq_handler(vtask_t* task) nc_reply.error = "scheduler not connected"; mylock_unlock(&(c->sched.lock)); + vtaskmgr_blocking_release(&(c->vtaskmgr)); reply.status = 0; reply.data_which = RequestReply_data_schedadminreplymsg; @@ -898,6 +931,7 @@ static int myreq_handler(vtask_t* task) if (rc < 0) { schedadmin_unregister(&pending); mylock_unlock(&(c->sched.lock)); + vtaskmgr_blocking_release(&(c->vtaskmgr)); reply.status = rc; reply.data_which = RequestReply_data_nodata; @@ -982,6 +1016,7 @@ static int myreq_handler(vtask_t* task) } mylock_unlock(&(c->sched.lock)); + vtaskmgr_blocking_release(&(c->vtaskmgr)); rc = 0; break; } diff --git a/src/controller/testsuite/test_sched_admin_dispatch.c b/src/controller/testsuite/test_sched_admin_dispatch.c index e8b90a06..a9077a69 100644 --- a/src/controller/testsuite/test_sched_admin_dispatch.c +++ b/src/controller/testsuite/test_sched_admin_dispatch.c @@ -268,6 +268,67 @@ static void test_schedadmin_rejects_when_not_connected(void) printf("test_schedadmin_rejects_when_not_connected: PASS\n"); } +/* ── Observable 2b (design review follow-up, 2026-08-19): admission- + * exhausted requests fast-reject instead of blocking a vtask thread ──────── + * + * ha-async-io.md Rule 3: bounded blocking on a vtask thread requires an + * admission reservation. c->vtaskmgr starts gate-DISABLED by a bare + * sa_make_cluster() (liveness_reserved defaults to 0 on a calloc'd + * vtaskmgr_t -- vtaskmgr_blocking_acquire's own "Gate disabled: admit + * uncounted (legacy: nothing reserved)" branch, vtaskmgr.c), matching + * production behavior before vtaskmgr_set_liveness_reserved is called + * (cluster.c's real init path) -- this is why every OTHER test in this + * file, which never calls that setter, is completely unaffected by this + * feature. This test explicitly enables the gate with a 1-slot cap + * (vtaskmgr_set_liveness_reserved(..., 1) floors cap at 1 regardless of + * n_vcpus, vtaskmgr_blocking_cap's own doc comment) and pre-saturates that + * one slot -- simulating a concurrent commit-wait or another in-flight + * schedAdmin request already holding it -- before dispatching. The poison + * c->sched.ep sentinel (same technique test_schedadmin_rejects_when_not_ + * primary uses above) proves the fast-reject happens BEFORE + * mylock_lock(&c->sched.lock)/any forwarding attempt, not merely that the + * eventual reply looks right. */ +static void test_schedadmin_fast_rejects_when_admission_exhausted(void) +{ + cluster_t* c = sa_make_cluster(); + atomic_store(&c->ha.role, CTRL_PRIMARY); + + endpoint_t* poison = (endpoint_t*)(uintptr_t)0x1; + c->sched.ep = poison; + + vtaskmgr_set_liveness_reserved(&c->vtaskmgr, 1); + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_blocking_acquire(&c->vtaskmgr)); + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + ep.owner = c; + + io_packet_t* pkt = sa_build_request_pkt(); + int rc = myreq_handler_for_test(&ep, pkt); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_TRUE_MESSAGE( + c->sched.ep == poison, + "c->sched.ep must be untouched -- admission-exhausted requests must " + "fast-reject before ever locking c->sched.lock or forwarding"); + + xce_request_t decoded; + sa_decode_queued_reply(&ep, &decoded); + TEST_ASSERT_EQUAL_INT(Request_body_replymsg, decoded.body_which); + xce_reply_t* rr = decoded.body.replymsg; + TEST_ASSERT_NOT_NULL(rr); + TEST_ASSERT_EQUAL_INT(0, rr->status); + TEST_ASSERT_EQUAL_INT(RequestReply_data_schedadminreplymsg, rr->data_which); + TEST_ASSERT_NOT_NULL(rr->data.schedadminreplymsg); + TEST_ASSERT_EQUAL_STRING("server busy, try again", + rr->data.schedadminreplymsg->error); + xce_free_request(&decoded); + + vtaskmgr_blocking_release(&c->vtaskmgr); /* release our pre-saturating slot */ + sa_teardown(c); + printf("test_schedadmin_fast_rejects_when_admission_exhausted: PASS\n"); +} + /* ── Basic round trip: forward -> scheduler reply wakes the waiter ────────── * * c->sched.ep points at a real (if unconnected) endpoint_t so the dispatch @@ -612,6 +673,7 @@ void test_sched_admin_dispatch(void) UnitySetTestFile(__FILE__); RUN_TEST(test_schedadmin_rejects_when_not_primary); RUN_TEST(test_schedadmin_rejects_when_not_connected); + RUN_TEST(test_schedadmin_fast_rejects_when_admission_exhausted); RUN_TEST(test_schedadmin_round_trip_success); RUN_TEST(test_schedadmin_forward_timeout); RUN_TEST(test_schedadmin_round_trip_rpc_status_only);