main
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  samiandClaude Sonnet 5
|
322a20efa5 |
core: fix SegmentBudget over-admission and add a wait-list wakeup
Root cause of the M7 RSS gap (core/docs/m7-baseline.md: ~70 MB measured against a 60 MB target): SegmentBudget::confirm_slot() only checked a task's own held count against its own target -- never the engine-wide active_ sum. reallocate_locked()'s two-pass fairness allocation does bound sum(target) <= max_active_ at the moment it computes a plan, but that bound says nothing about sum(held): a task can be legitimately holding more than its own just-lowered target for a while (yield is deferred to a segment boundary, never mid-segment -- ADR 0011 A1), and another task's target can correctly rise to claim that capacity before the first task has physically released it. Both confirm_slot() calls could then succeed against their own, individually-correct targets while sum(held) exceeded max_active_ -- tools/bench heap-profile caught this directly: budget.active reading 56-86 against a total of 32. confirm_slot() now also checks active_ < max_active_, unconditionally, as a backstop that doesn't depend on any task's target bookkeeping being in sync with what every other task holds. That creates a liveness question the original design never answered: a task denied only by this new check has a target that's already correct, so it never changes again and reallocate_locked()'s plain "fire a callback when a task's target changes" mechanism never revisits it. Task gained a waiting_for_slot flag, set on exactly this denial; release_slot()/deregister_task() (the only two places that free real capacity) now hand a freed slot directly to the highest-priority waiting task via wake_one_waiter_locked(), if reallocate_locked()'s own plan didn't already produce a callback for anyone. download_task.cpp's fill_slots_locked() needed a matching fix: a woken task's stalled segments (SegState::stalled -- backed off mid-retry, its own release_slot() already called) have no live worker and never surface through Segmenter::assign_slot(), which only hands out unassigned or fresh ranges. fill_slots_locked() now restarts any stalled segment with no live worker directly (bounded by slot_target, same as its assign_slot() loop) before looking for new work; a segment it doesn't get to keeps its own scheduled retry_worker() timer as a second chance. Also fixes a real TSan-caught data race this work surfaced: SegWorker:: speed_bps was written only by its own segment's curl callback and, before Progress.speed_bps's polled-path fix, only ever read from that same thread -- safe without synchronization. snapshot_progress() reading it from whatever thread calls DownloadHandle::progress() broke that invariant (workers_mu's shared_lock protects the workers map's structure, not an individual SegWorker's fields). Now std::atomic<double> with relaxed ordering -- an informational EMA, nothing synchronizes real state on it -- rather than adding a lock to the write side. core/tests/segment/budget_test.cpp adds two tests reproducing the actual gap (budget_active_never_exceeds_max_active_segments_under_concurrent_load, budget_wait_list_wakes_a_task_whose_target_never_changed) plus a sanity baseline (budget_release_wakes_a_denied_waiter), and introduces AsyncFakeTask + TestTimer for the one existing test that drives the budget from multiple concurrent threads -- mirroring production's real dispatch (register_task()'s on_target lambda posts through host.schedule(), download_task.cpp, never a synchronous call) rather than adding reentrancy-guarding machinery to SegmentBudget itself to compensate for a synchronous test double being unlike production. See docs/adr/0017 for the full writeup, including what an earlier version of this fix got wrong chasing a same-thread reentrancy hazard that doesn't actually exist in production. core/docs/m7-baseline.md updated: the RSS number now clears the DoD line (45.41 MiB via heap-profile), root-caused rather than just re-measured. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Q3QrF7rCt21bkAjt9BCDFQ |
||
|
samiandClaude Sonnet 5
|
b60d4e6f5b |
core: add tools/bench (throughput/load/alloc-check) and record the M7 baseline
Three subcommands in one binary, driving vdm::Engine directly (docs/04 §8): - throughput: a single download against a fast local origin (support/local_server.hpp, busybox httpd), reporting Mbps/CPU%/RSS. Gates on --require-mbps/--max-cpu-pct only when passed, so the ctest smoke registration stays a correctness check, not a hardware-dependent perf gate -- the real 1-Gbit-link sign-off is a manual/CI job (see the file's header comment). - load: N concurrent tasks against tools/testserver's `throttled` mode (support/testserver_client.hpp), reporting peak RSS via getrusage(). Paced externally rather than through the engine's own rate::RateLimiter or busybox: the limiter's pause/resume path allocates on every throttle event (would contaminate alloc-check's measurement) and under heavy segment contention was found to starve individual tasks indefinitely (see docs/adr/0016, added here); busybox couldn't sustain the DoD's ~160 concurrent connections (20 tasks * default_segments=8) reliably. The ctest registration runs at reduced concurrency under sanitizer presets -- see the CMakeLists.txt comment and the ADR's postscript. - alloc-check: operator new/delete overridden process-wide, sampling the allocation count across a steady mid-transfer window against a paced tools/testserver origin. Caught a real bug in the same change (see the http_client.cpp commit) and, by dropping its Engine mid-download to end cleanly, also surfaced the quiesce() use-after-free (see that commit). core/docs/m7-baseline.md records actual measured numbers against the M1/M7 DoD lines, including where they don't clear yet (RSS ~70 MB vs a 60 MB target; throughput/CPU only measured on loopback, no 1 Gbit link available here) rather than rounding them away. docs/adr/0016 documents a rate::RateLimiter fairness gap found building the load subcommand: a single shared TokenBucket under heavy segment contention has no fairness ordering across its peek/commit race and can starve a waiter well past what its configured rate implies. Filed as a follow-up (it's a core/src/rate design question, not a tools/bench one) rather than fixed here, along with a related TSan-only load-test straggler that could not be root-caused in this environment. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Q3QrF7rCt21bkAjt9BCDFQ |
||
|
samiandClaude Sonnet 5
|
3da4cd6e91 |
core: rate/token_bucket + fold in DAEMON's engine-API review (stage 7)
rate/token_bucket.hpp — a lazily-refilled TokenBucket (starts full: burst
then throttle, IDM behaviour; rate 0 == unlimited; burst caps idle
accumulation) and RateLimiter, the global -> per-queue -> per-task
hierarchy (docs/04 §6). acquire(task, n) peeks every applicable level and
commits on all-or-none so a blocked attempt never leaks tokens at a level
that had them; held under one mutex so a concurrent detach can't dangle
the bucket it's using. vdm/ids.hpp gains QueueId.
Tests: burst/refill/cap/unlimited for the bucket; tightest-level-binds,
no-partial-consumption, detach-safety, and an 8-thread aggregate-rate
check for the hierarchy. Green under ASan/UBSan and TSan.
Engine-API review (DAEMON signed off, no sched/ or dispatch rewrite):
- Engine::rate_limiter() accessor added (limiter.set -> set_global_limit).
- Checksum::Algo gains sha512 to match the wire Checksum set.
- DownloadSpec: DAEMON creates save_path's parent dir before start();
missing dir -> Error::path_rejected (made explicit).
- cancel(): documented to fire on_state(_, cancelled, nullopt) then
on_finished(Err{canceled}), in that order; download.cancel ==
cancel(false), download.remove == cancel(true).
- engine-api-m1.md: the five open questions resolved with DAEMON's
answers (probe_hint optional, single cancel flag, {restart,
keep_partial, abort} is the whole set, per-task 4 Hz is fine,
refresh_url restarts all segments after a validating re-probe).
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS
|
||
|
samiandClaude Sonnet 5
|
d6cf1fe7dc |
core: post the engine API for DAEMON review (pre-stage-7)
The download entry point the AGENT-CORE brief asked for on day one and that slipped. DAEMON has an RPC surface and a store and, until this is agreed, nothing in velox::core to call. vdm/task/download.hpp — DownloadSpec (the resolved subset DAEMON hands in: absolute save_path, verbatim browser headers, requested segments/buffer, optional probe_hint / checksum / auth, allow_resume), EngineState (the CORE-owned subset of the wire TaskState), Progress / SegmentProgress, DownloadCallbacks (on_progress <=4 Hz, on_state for every transition incl. auto-pauses, on_auth_required, on_decision_needed, on_finished last), DownloadHandle (pause/resume/cancel — idempotent per the ADR 0013 signature — plus provide_auth / decide / refresh_url, and synchronous state()/progress() snapshots). vdm/engine.hpp — Engine: start(spec, callbacks) -> handle, segment_budget() (DAEMON's sched/ admission surface, ADR 0011), live connection.* setters, a standalone probe() on the pool outside the segment budget. core/docs/engine-api-m1.md — the review doc: field semantics, the state machine, threading/lifetime rules (which thread callbacks arrive on, what is legal from inside one, handle/engine lifetime), the shared-`paused` idempotency contract as a signature, and five open questions for DAEMON. Value types compile and are covered by api_compiles_test; Engine / DownloadHandle bodies land in stage 8, built against whatever DAEMON signs off here. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS |
||
|
samiandClaude Sonnet 5
|
e8a6b64c2f |
core: file conformance-cpp CMake correction for PROTO
tests/conformance/cpp/CMakeLists.txt (PROTO's lane) compiles core/generated/velox_proto.cpp directly while its comment claims it links libveloxproto. Now that the veloxproto target exists it should link velox::proto, with a TARGET-guarded fallback to the direct-compile for standalone configures. Filed, not edited — not CORE's file. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS |
||
|
samiandClaude Sonnet 5
|
c65e664c29 |
core: sign off on ADR 0013 (task state-machine ownership)
Accept as written — no amendments. Answers to the four open items:
1. tasks_starved / starved_tasks() count ONLY tasks in {connecting,
downloading} with segments_active == 0 (the allocator owes a slot to a
task that is asking). retry_wait and paused (either-initiated) are
outside that state set, so they are excluded by construction, not by a
special case. Design commitment; the accessor is stage 6/8.
2. pause() is idempotent: no-op success on an already-paused or terminal
task; resume() no-op success on a non-paused task; only task_not_found
errors. No state-change event for a no-op.
3. PROTO's item — CORE confirms its half: auto-pause reports the
transition with ErrorInfo populated (auth_required / server_file_changed
/ disk_full / path_rejected), already in util/error.hpp. Ready once
PROTO permits error on state=="paused".
4. CORE adopts "auto-pause"; the wire/API discriminator stays
state==paused + presence of the Error code.
Notes back: pause during verifying re-hashes from scratch on resume;
pause during assembling is M4; restart handling in §5 agreed —
start(TaskId) re-derives resume position from .veloxpart.meta + If-Range.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS
|
||
|
samiandClaude Sonnet 5
|
e442c99130 |
core: sign off on ADR 0011 with three amendments
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 <[email protected]>
Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS
|
||
|
samiandClaude Sonnet 5
|
dfb02afc52 |
core: redo bufferBytes default and RSS reconciliation (per-segment)
First pass counted one buffer per download; it is one per segment. 20 active downloads at the default 8 segments = 160 buffers, so at 20 tasks the binding constraint is the global cap, not the per-segment default — 256 MiB and the "<=60 MB RSS / 20 downloads" target (line 125) cannot both hold whatever the default is. Floor (64 KiB) and ceiling (16 MiB) unchanged — the 256/64 unreachability argument is stronger under per-segment accounting. Changes: - default 1 MiB (was 2): with the cap below, 32 live segments x 1 MiB = 32 MiB buffers -> ~45-50 MiB RSS, line 125 holds with margin. - NEW maxActiveSegments (default 32): a global concurrent-segment cap is the actual mechanism that bounds "20 active downloads"; docs/01 §2 implies it, docs/04 never states it. Without it no buffer policy hits 60 MB. - maxTotalBufferBytes 128 MiB (was 256) and it must be ADDED to the contract — currently absent, so the clamp CORE implements has no wire representation and Options can't show/set it. Folded into B2a. - line 125: keep 60 MB "given maxActiveSegments=32 and default buffers", or explicitly raise to 120 MB — ADR records which. Flagged that changing it is a defensible outcome CORE owns, not a number that quietly loses. - bufferBytes bounds are in FOUR schema files, not three: Settings.schema.json connection.bufferBytes also has 4096-8388608. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS |
||
|
samiandClaude Sonnet 5
|
c13b5dff21 |
core: answer PROTO's bufferBytes range question; accept inclusive endByte
buffer-sizing.md: the frozen 4 KiB–8 MiB and docs/04's 64 KiB–64 MiB both miss. Recommend 64 KiB – 16 MiB, default 2 MiB, max_total_buffer_bytes unchanged at 256 MiB: - 4 KiB floor is smaller than one libcurl write callback -> a syscall per chunk; 64 KiB is the smallest floor that coalesces. - throughput vs write size is flat past ~8 MiB on NVMe; 8–16 MiB is disk-stall absorption headroom for the fast-pipe/slow-disk case; 64 MiB is cache pressure for zero gain. - 32 segments x 64 MiB = 2 GiB vs the 256 MiB cap means the docs/04 max is unreachable past 4 total active segments — a misleading Options value. 16 MiB is reachable for single-/light-multitask and clamps to 8 MiB under heavy parallelism, which is correct. - default 4 MiB x 20 downloads = 80 MiB, busting the "<=60 MB RSS / 20 downloads" DoD; 2 MiB fits. Filed as request B4. proto-requests-m1.md: B3 endByte accepted as inclusive (HTTP Range semantics, no curl-boundary off-by-one); [start,end) ask withdrawn; stage 6 designed against inclusive. New B3a: the Content-Length: 0 whole-file case needs a representable zero-length segment — min_segment_bytes means CORE never makes empty segments mid-download, so it's only the degenerate case; mild preference for startByte+length over an endByte=startByte-1 sentinel. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS |
||
|
samiandClaude Sonnet 5
|
910ce4a638 |
core: add request docs for PROTO and PKG before the contract freeze
proto-requests-m1.md: freeze-blockers (error.code wire enum with the full CORE failure taxonomy, TaskSummary.segments meaning, Segment field names) separated from cheap follow-ups (decision event, credential return path, checksum pattern); state-machine ownership split flagged as three-way ADR material. pkg-requests-m1.md: uncomment add_subdirectory(core), pick a test framework, guard VELOX_BUILD_FUZZ on Clang. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HPPSGhiArbvQgwC2DNiURS |