diff --git a/daemon/docs/engine-api-review.md b/daemon/docs/engine-api-review.md new file mode 100644 index 0000000..b907e91 --- /dev/null +++ b/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)` 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.