vdm/daemon/docs/engine-api-review.md

DAEMON review — CORE's engine API (core/docs/engine-api-m1.md, lane/core@d6cf1fe)

Verdict: sign off. Nothing here forces a daemon/src/sched/ or RPC-dispatch rewrite. The value types are final enough to build sched/ against now; Engine / DownloadHandle bodies landing in stage 8 is fine. The split matches ADR 0011 and ADR 0013 exactly.

Answers to the five open questions, then the small things to confirm.

Answers

Q1 — probe_hint: keep it optional, as sketched

DAEMON has a ProbeResult on exactly one path: the File Info dialog, where the user already waited for download.probe and then clicked Download. Every other entry — capture.offer → take, velox add, download.addBatch, the restart flow — has no probe in hand. Forcing the engine to always probe adds a round trip to the one case where the user just sat through one; forcing DAEMON to always probe first means reimplementing the engine's probe on the daemon side. Pass probe_hint when we have it, omit it otherwise — the two code paths are worth keeping.

Q2 — one cancel(discard_partial), not a separate remove()

The two wire methods map cleanly onto the one call:

wire	live task	already terminal
download.cancel	handle.cancel(discard_partial=false) — keeps .veloxpart	no-op on the handle; DAEMON marks the row cancelled
download.remove {deleteFile}	handle.cancel(discard_partial=true)	no-op on the handle; DAEMON deletes the row and, if deleteFile, the finished file

download.remove on a completed task is pure DAEMON-side work (row + optional file); the handle is already terminal so cancel() no-ops, which is exactly what we want. No handle.remove() needed.

Q3 — {restart, keep_partial, abort} is enough, if the engine owns the mechanical 416 retry

For server_file_changed the three options are right and complete. For range_metadata_stale (416): docs/04 §7 already has the engine re-probe and re-split automatically. Keep that — DAEMON does not want to be in the loop for a routine 416. Only escalate to on_decision_needed{range_metadata_stale} when the automatic re-probe/re-split also fails to reconcile, and at that point "retry the same range once more" is not a useful fourth option (the engine already exhausted it). So: no fourth value, provided the engine handles the common 416 without a callback.

Q4 — per-task 4 Hz on_progress is fine; on_progress_batch optional

DAEMON already has to coalesce across tasks: event.task.progress is "batched ≤ 4 Hz into a single array message" (AGENT-DAEMON.md item 5). So 80 per-task callbacks/s land in DAEMON's fan-out queue and are re-emitted as one array at 4 Hz regardless. Per-task keeps the handle↔callback correspondence simple and is not a bottleneck. If the engine's timer thread is already walking every task to build those callbacks, a on_progress_batch(span<Progress>) is strictly less work for both sides and we'd take it — but it is not needed for M1 and should not hold stage 8.

Q5 — refresh_url while downloading: restart all segments on the new URL

download.refreshUrl's contract is the signed-URL-expiry case: "re-probes and compares size and validator; if they still match, the transfer resumes from where it stopped." That wants consistency — every segment on the new URL once the re-probe validates. Mirror rotation (finish in-flight on the old host, new work elsewhere) is a different mechanism and it is already DownloadSpec.mirrors + the segmenter's 3-failure requeue, not refresh_url. So: on refresh_url, re-probe, and if size+validator match, move all segments to the new URL from their current offsets; if they don't match, surface it (on_decision_needed or a refresh_url error) rather than silently restarting.

Confirmed by CORE (lane/core@3da4cd6)

1. vdm::TaskId — cheap-copy and std::hash-able; DAEMON never constructs one, only receives it from start() / callbacks and passes it back to set_task_order(). sched/ keeps the vdm::TaskId → wire taskId map keyed off handle.id().
2. Parent directory — DAEMON mkdir -ps save_path's parent before start(); the engine opens the file and fails the task with Error::path_rejected if it is missing. Now explicit on DownloadSpec's doc comment.
3. sha512 — added as the fourth Checksum::Algo, matching the wire set. No -32602 at the RPC edge; DAEMON passes it straight through.
4. Cancel ordering — on_state(_, cancelled, nullopt) then on_finished, always in that order. Note the taxonomy value is spelled canceled (one L): the finish is on_finished(Err{Error::canceled}), error.code == canceled. sched/ keys on that to write a cancelled history row rather than a user-facing failure. (The wire TaskState / EngineState spelling stays cancelled; only vdm::Error is one-L.)

Related, landed the same pass

rate/token_bucket — the global → queue → task speed-limiter hierarchy (docs/04 §6), reached via Engine::rate_limiter(). DAEMON's limiter.set {globalBps, enabled} maps to rate_limiter().set_global_limit(...); limiter.get reads it back. Wire this alongside the download.add → start() glue.

Integration timing

Wire it after sched/ lands — the scheduler is what calls start() / pause() / resume() / cancel() and drives segment_budget().set_task_order(). Order: sched/ (against these headers) → download.add → start() glue → the callback→wire projection. sched/ does not need the Engine bodies, only the signatures in this doc, so stage 8 and sched/ proceed in parallel.