From e442c991303dad6b51d1017a5f18c648eed1272d Mon Sep 17 00:00:00 2001 From: sami Date: Wed, 9 Sep 2026 23:21:10 +0400 Subject: [PATCH] core: sign off on ADR 0011 with three amendments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accept the decision (each ceiling enforced once by its unit owner; DAEMON counts tasks, CORE counts segments; the single min() clamp; per-host caps split by unit). daemon/src/sched/ is unblocked. Answers to the five open questions: 1. Min-1 is implementable in the allocator without inversion: guarantee pass (zero-slot tasks, priority order) before growth pass; a released slot always re-enters allocation from the top, never handed back locally. 2. Probe pool size 4, outside the segment budget — confirmed. 3. set_max_active_segments is drain-not-kill; in-flight segments run to their boundary. 4. Priority = an ordered TaskId list pushed on change, not an integer, not per tick — tie-breaking is DAEMON policy. 5. on_budget_changed coalesced at 4 Hz, immediate on the tasks_starved zero-crossing. Amendments: (A1) add "yield" to §3.3 as the non-neutral slot-transfer op that satisfies min-1 when the budget is full — "steal" stays slot-neutral; (A2) "admission implies progress" is bounded-delay not immediate — bounded by an incumbent's next segment boundary, capped by the stall timeout, so §3.6's 2 s assertion window is too tight; (A3) add starved_tasks() / starved_since() and pin the segments_active / tasks_starved definitions (a connecting segment counts as a held slot and is not starvation). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS --- core/docs/adr-0011-core-response.md | 177 ++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 core/docs/adr-0011-core-response.md diff --git a/core/docs/adr-0011-core-response.md b/core/docs/adr-0011-core-response.md new file mode 100644 index 0000000..05678d5 --- /dev/null +++ b/core/docs/adr-0011-core-response.md @@ -0,0 +1,177 @@ +# CORE response to ADR 0011 (admission control & the segment budget) + +**Verdict: accept the decision, with three amendments and one caveat on §3.5.** None of +the amendments change DAEMON's task-unit model — `daemon/src/sched/` is unblocked. The +caveat tightens what "admission implies progress" can promise. + +Signing off on §1 (each ceiling enforced once, by unit owner), §2 (the single clamp), +§4 (per-host caps split by unit from one table), §6 (contract gap / daemon-local stopgap), +and the three rejected alternatives — the shared-semaphore rejection especially. + +--- + +## Answers to the five open questions + +### Q1 — Min-1 before seconds: implementable in the stealer without inversion? **Yes.** + +Slot allocation is a pure function of `(running tasks, their held-slot counts, their +effective per-task caps, DAEMON's priority order, total budget)`. It runs to completion on +every budget-changing edge (segment complete / start / pause / fail, task admit / pause / +resume, a steal, a cap change), in two ordered passes: + +1. **Guarantee pass.** Walk tasks holding **zero** slots *in DAEMON's priority order*; give + each one slot while the pool is non-empty. +2. **Growth pass.** While the pool is non-empty, round-robin over running tasks in priority + order, granting one slot to any task below its effective cap + (`min(spec.segments ?? maxSegmentsPerDownload, host_cap, resumable ? ∞ : 1)`), until the + pool empties or nobody wants more. + +The inversion DAEMON is worried about at slot release does not occur, because **a released +slot always goes to the pool and the allocator re-runs from pass 1** — it is never handed +back locally to the releasing task. If task A's segment finishes and both A and a +lower-priority C now hold zero, the guarantee pass processes the zero-slot set *in priority +order*, so A gets it back, not C. A task that drops to zero re-enters the guarantee queue +at its priority position, not the back. + +Liveness of min-1 depends on DAEMON honouring §2's clamp — never running more tasks than +`maxActiveSegments`. If it admits 3 running tasks against a budget of 2, one starves by +construction and no fairness rule fixes it. §2 already says this; calling it out because +it is load-bearing for Q1. + +### Q2 — Probe pool outside the segment budget, size 4: **confirmed.** + +CORE's probe path is a dedicated pool, independent of `maxActiveSegments`. A probe is a +`Request` with `method = HEAD` (or `range = {0,0}`) whose `on_head` returns `abort`; it +never allocates a transfer slot. Default pool size 4, exposed as +`set_probe_pool_size(uint32_t)`. Probe cancellation is immediate (a timed-out probe frees +its pool slot at once), so `capture.offer` answering `ignore`-first-probe-after works. +Probes still open real sockets — DAEMON should still bound probe *submission* on its side; +CORE bounds *concurrency*, not queue depth. + +### Q3 — `set_max_active_segments()` live-apply: **drain, never kill.** + +- **Raise:** new slots available immediately; allocator runs; starved then growing tasks + take them. +- **Lower:** in-flight segments run to their next boundary. No new segment starts while + `active > new_ceiling`. Completed segments' slots are withheld until + `active ≤ new_ceiling`. Nothing is aborted, no partial range is lost. +- **Edge:** if `new_ceiling < running_task_count`, CORE honours min-1 for the top + `new_ceiling` tasks in priority order; the remainder are held at zero slots and reported + in `tasks_starved`. CORE does **not** auto-pause them — that's policy. After a live + lower, DAEMON must reconcile its running set against the new clamp (pause the + lowest-priority excess). + +### Q4 — Priority shape: **an ordered list, pushed on change — not an integer, not per tick.** + +`set_task_order(std::span)`, called by DAEMON whenever the order changes +(admit, remove, reorder, priority edit, queue switch). CORE caches it and the allocator +walks it. An integer priority would force CORE to implement tie-breaking (FIFO by +admission time, queue precedence) — that's DAEMON policy and DAEMON already has the total +order. Not per tick: an unchanged order isn't re-sent. A running task absent from the list +sorts last (shouldn't happen; defensive). + +### Q5 — Budget-change callback coalescing: **4 Hz for counts, immediate on the edge.** + +`on_budget_changed` coalesced at ≤4 Hz to match `event.task.progress` — DAEMON isn't +making sub-250 ms admission decisions. **Exception:** fire immediately when +`tasks_starved` crosses 0→non-zero or non-zero→0, so DAEMON's invariant check and any UI +reaction see that transition without up to 250 ms of lag. + +--- + +## Amendments requested to the ADR + +### A1 — §3.3 wording: distinguish *steal* from *yield* + +"A steal is slot-neutral" is true for the steal the ADR means (a worker that finished its +range takes the tail of the largest remaining range — the same worker, the same slot). +Min-1 also needs a second, non-neutral operation: + +- **Yield** — the allocator marks an over-quota task to release one slot *at its next + segment boundary*. When that segment completes the slot goes to the pool → guarantee + pass → starved task. It is a slot transfer, not slot-neutral, and it is bounded by the + yielding segment's remaining bytes (never a mid-segment kill). + +Please add "yield" to §3.3 as the mechanism that satisfies §3.1 when the budget is full. +"Steal" stays exactly as written. + +### A2 — §3.5 / §3.6: "admission implies progress" is bounded-delay, not immediate + +When the budget is full of healthy incumbents, a newly admitted task's first slot +materialises only when some incumbent segment reaches a boundary (yield) — bounded by that +segment's remaining bytes, hard-capped by the stall timeout (`low_speed_secs`, default +30 s, after which a stalled segment fails and frees its slot). So the true bound is + +``` +time_to_first_slot ≤ min(incumbent's next boundary, low_speed_secs) + connect_timeout +``` + +not "connect timeout + per-host cap" alone. Two consequences: + +- §3.6's **2 s** assertion window is too tight — a legitimately full budget with a slow + incumbent tail can hold a new task at zero for longer than 2 s with nothing wrong. + Recommend the warning threshold be `low_speed_secs + connect_timeout_ms` (~45 s), or + configurable. +- CORE will expose `starved_since` (a monotonic timestamp) per starved task via the + diagnostics call below, so DAEMON can tell "briefly waiting for a boundary" from + "wedged" without guessing. + +Optional future tightening (not M1): a **preemptive split** — truncate an incumbent's +largest remaining range ahead of its current offset and hand the freed tail to the starved +task as a new segment. Zero bytes lost, first slot within one round-trip. CORE will add +this if the yield delay proves painful in the soak test; it doesn't change the API. + +### A3 — C1 API: add a starved-set accessor and pin two definitions + +``` +struct EngineBudget { uint32_t total; uint32_t active; uint32_t tasks_starved; }; +EngineBudget budget() const; +uint32_t segments_active(TaskId) const; // slots held, any state +std::vector starved_tasks() const; // diagnostics / velox ls --json +std::optional starved_since(TaskId) const; // per A2 +void on_budget_changed(std::function); +``` + +Definitions, so the projection to `TaskSummary.segments` (ADR 0010: effective count) is +unambiguous: + +- **`segments_active(id)`** = slots the task holds, counting a segment in `connecting` + (0 bytes yet) as well as `downloading`. This is what the user sees as "using N + connections." +- **`tasks_starved`** counts running tasks with `segments_active == 0`. A task with a + `connecting` segment is **not** starved — it is progressing. + +--- + +## C2–C6 confirmations + +- **C2:** drain-not-kill, per Q3. Confirmed as DAEMON assumed. +- **C3:** `set_host_segment_cap(std::string host, uint32_t)` — confirmed. CORE keeps the + `host → cap` map, applies it in the per-task effective cap and in the stealer (no Nth + connection to a host capped at N-1). DAEMON owns the table; CORE derives a task's host + from its URL + mirror set. +- **C4:** agreed it's PROTO's, same bundle as CORE's B2a / `buffer-sizing.md` asks + (`connection.maxActiveSegments`, `connection.maxTotalBufferBytes`). DAEMON's local-value + stopgap is fine. +- **C5:** signed off — see Q1 plus amendments A1/A2. Min-1 is buildable; "implies + progress" is bounded-delay; steal stays slot-neutral, yield is the transfer op. +- **C6:** confirmed — see Q2. + +--- + +## New API surface CORE will expose for `sched/` (summary) + +``` +void set_max_active_segments(uint32_t); // drain-not-kill +void set_host_segment_cap(std::string host, uint32_t); +void set_task_order(std::span); // pushed on change +void set_probe_pool_size(uint32_t); // default 4 +EngineBudget budget() const; +uint32_t segments_active(TaskId) const; +std::vector starved_tasks() const; +std::optional starved_since(TaskId) const; +void on_budget_changed(std::function); // 4 Hz + starved edge +``` + +All of it lands with stage 6 (segmenter/stealer) / stage 8 (download_task). None of it is +on the M1 critical path ahead of where DAEMON needs it; flag if the ordering is wrong.