daemon: review CORE's engine API — sign off, answer the five questions

Review of core/docs/engine-api-m1.md (lane/core@d6cf1fe) in
daemon/docs/engine-api-review.md. Sign off: nothing forces a sched/ or
RPC-dispatch rewrite; the split matches ADR 0011 and ADR 0013.

Answers: (1) keep probe_hint optional — DAEMON has a ProbeResult only
on the File Info path; (2) one cancel(discard_partial), download.remove
is cancel(true) + DAEMON-side row/file cleanup; (3) {restart,
keep_partial, abort} suffices if the engine owns the mechanical 416
re-probe/re-split; (4) per-task 4 Hz progress is fine — DAEMON
re-batches across tasks for event.task.progress anyway; (5) refresh_url
restarts all segments on the new URL (the signed-URL case), mirror
rotation is spec.mirrors not refresh_url.

Four things to confirm, none blocking: vdm::TaskId copy/hash semantics
and that DAEMON never constructs one; who mkdir -p's save_path's
parent; sha512 (in the wire Checksum, not the engine enum) rejected at
the RPC edge; on_finished(Err{cancelled}) code + ordering vs
on_state(_, cancelled, _).

Integration timing: wire after sched/ lands. sched/ builds against
these signatures in parallel with CORE stage 8.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Upd9WhG9oppieig5nRDLig

daemon/docs/engine-api-review.md

@@ -0,0 +1,90 @@
+# 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.
+
+## To confirm — none block sign-off
+
+1. **`vdm::TaskId` ↔ wire UUID.** `sched/` keeps a `vdm::TaskId → wire taskId` map, keyed
+   off `handle.id()` at `start()`. Confirm `vdm::TaskId` is cheap to copy and hashable
+   (`std::unordered_map` key), and that DAEMON is never expected to *construct* one — it
+   only ever receives them from `start()` / callbacks and passes them back to
+   `set_task_order()`.
+2. **Parent directory creation.** The engine "never canonicalises or root-checks"
+   `save_path` — does it `mkdir -p` the parent (the category folder may not exist yet), or
+   must DAEMON ensure the directory exists before `start()`? DAEMON assumes the latter
+   (it owns category/path logic) unless you say otherwise.
+3. **`Checksum::Algo` has no `sha512`**, but `Checksum.schema.json` accepts it (PROTO F3).
+   DAEMON will reject `algorithm: "sha512"` at the RPC edge with `-32602` before `start()`
+   unless CORE would rather carry a fourth enum value. Either is fine; say which.
+4. **`on_finished(Err{cancelled})`** — DAEMON keys on `ErrorInfo.code == cancelled` to
+   write a `cancelled` history row rather than surfacing it as a user-facing failure.
+   Confirm `cancelled` is the code and that `on_state(_, cancelled, nullopt)` always
+   precedes it (the §4 table implies this).
+
+## 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.