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 -p`s `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.