samiandClaude Sonnet 5
dfb02afc52
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
201 lines
11 KiB
Markdown
201 lines
11 KiB
Markdown
# CORE → PROTO — requests before the `VERSION` freeze
|
||
|
||
Status: **open**. Raised by lane CORE against `contracts/` at `1.0.0-draft`.
|
||
This is a request list, not a decision record. PROTO owns every item here. Once PROTO
|
||
answers D1, that one becomes a three-way ADR (`docs/adr/`) per `contracts/README.md` rule 4.
|
||
|
||
Ranking is by `contracts/README.md` rule 4, not by intuition:
|
||
new method or new optional field = **minor**, can land anytime;
|
||
rename / remove / retype / default change = **major** + ADR.
|
||
|
||
---
|
||
|
||
## Freeze-blockers — get these into `1.0.0` (major if we guess wrong)
|
||
|
||
### B1. A frozen wire enum for `TaskSummary.error.code`
|
||
|
||
`TaskSummary.error.code` is currently a bare `integer`. CORE cannot import a protocol
|
||
header (layering rule — `core/` never sees JSON/RPC), so CORE has its own failure
|
||
taxonomy **regardless** of what PROTO picks; DAEMON owns the projection from one to the
|
||
other. The ask is not "who owns the enum" — it is: **freeze a wire enum with enough
|
||
cardinality that DAEMON's mapping from the CORE taxonomy below is lossless.**
|
||
|
||
If the wire enum is coarser than this table, DAEMON has to collapse distinct engine
|
||
failures into one code and the GUI can't tell "server file changed" from "checksum
|
||
mismatch". Retyping `error.code` later is a major bump.
|
||
|
||
CORE failure taxonomy (this *is* `vdm::Error` in `core/include/vdm/util/error.hpp`):
|
||
|
||
| CORE `Error` | Meaning | `retryable` | carries `httpStatus` |
|
||
|---|---|---|---|
|
||
| `canceled` | user/daemon cancelled | no | no |
|
||
| `resolve_failed` | DNS resolution failed | yes | no |
|
||
| `connect_failed` | TCP connect failed | yes | no |
|
||
| `tls_failed` | TLS handshake / cert rejected | no | no |
|
||
| `connection_reset` | peer reset mid-transfer | yes | no |
|
||
| `timeout` | connect or idle/stall timeout | yes | no |
|
||
| `too_many_redirects` | redirect limit hit | no | no |
|
||
| `http_client_error` | 4xx not otherwise classified | no | yes |
|
||
| `http_server_error` | 5xx | yes | yes |
|
||
| `auth_required` | 401 / 407 — task pauses, not terminal | no | yes |
|
||
| `forbidden` | 403 after the referrer-retry (`docs/04` §7) | no | yes |
|
||
| `not_found` | 404 | no | yes |
|
||
| `range_not_satisfiable` | 416 — triggers re-probe/re-split | yes | yes |
|
||
| `gone` | 410 / expired signed URL, no mirror left | no | yes |
|
||
| `server_file_changed` | `200` where `206` expected, or `If-Range` / ETag drift (`docs/04` §5) | no | yes |
|
||
| `content_length_mismatch` | bytes received ≠ declared length | yes | no |
|
||
| `checksum_mismatch` | post-download hash ≠ expected | no | no |
|
||
| `disk_full` | ENOSPC | no | no |
|
||
| `io_error` | `pwrite` / `fallocate` / `fdatasync` failed | no | no |
|
||
| `path_rejected` | outside allowed roots or not writable | no | no |
|
||
| `permission_denied` | EACCES on the destination | no | no |
|
||
| `meta_corrupt` | `.veloxpart.meta` failed CRC / bad magic | no | no |
|
||
| `meta_version_unsupported` | meta file from a newer engine | no | no |
|
||
| `probe_failed` | probe could not determine file info | maybe | yes |
|
||
| `unsupported_url_scheme` | not http/https | no | no |
|
||
| `max_retries_exhausted` | backoff loop gave up (`data` names the last underlying `Error`) | no | maybe |
|
||
| `internal` | invariant violation / bug | no | no |
|
||
|
||
Concretely: a **string enum** on `error.code` mirroring these names is simplest to keep
|
||
lossless; if it stays an `integer`, please reserve grouped ranges (e.g. 1xx network,
|
||
2xx http, 3xx local-io, 4xx metadata, 5xx internal) with room to grow inside each.
|
||
|
||
### B2. Freeze the meaning of `TaskSummary.segments` — *major, retype-of-meaning*
|
||
|
||
`docs/04` §4/§3 require the *effective* value (after the per-host cap and the
|
||
non-resumable→1 demotion) to reach the UI. `TaskSummary.segments` currently has no stated
|
||
meaning. Please freeze it as **"effective segment count in use right now"**, not the
|
||
requested value. This is the only genuinely freeze-blocking half of the buffer/segment
|
||
story — pinning a field's meaning after `1.0.0` is a major bump under rule 4. It stands
|
||
alone; do not defer it together with B2a below.
|
||
|
||
### B2a. A readable effective buffer size — *minor, but note the trap*
|
||
|
||
Split out from B2 on purpose so PROTO can't use the cheap half to defer the major half.
|
||
|
||
`bufferBytes` is **write-only**: accepted by `download.add` and `download.update.patch`,
|
||
readable on **no** type. `docs/04` §4 says the clamped / "silently reduced to fit" value
|
||
is "reported back to the UI", so CORE will expose an effective value and PROTO needs a
|
||
frozen field to carry it. Adding an optional field is minor under rule 4 — **except**
|
||
that `TaskSummary` is `additionalProperties: false`, so a daemon cannot tack it on as an
|
||
extension; it has to be in the schema or it's dropped by every conformance replay. Name
|
||
and placement are PROTO's call (`effectiveBufferBytes` on `TaskSummary` / `TaskDetail`,
|
||
or a dedicated readback). Land it whenever — it just can't be improvised later.
|
||
|
||
Fold in two more, all discovered via B4 (`buffer-sizing.md`):
|
||
- **`maxTotalBufferBytes` is absent from the contract entirely.** CORE implements the
|
||
clamp against it; the Options dialog can neither show nor set it. Add
|
||
`Settings.schema.json connection.maxTotalBufferBytes` (recommended default
|
||
`134217728` = 128 MiB).
|
||
- **`maxActiveSegments` is new** — the global concurrent-segment cap that actually bounds
|
||
"20 active downloads" for the RSS budget. Add `Settings.schema.json
|
||
connection.maxActiveSegments` (recommended default `32`). DAEMON's scheduler reads it.
|
||
|
||
### B3. Freeze `Segment.schema.json` field names
|
||
|
||
`Segment` is already on PROTO's type list (`contracts/README.md`) and `TaskDetail`
|
||
"includes `segments[]`", so this is mostly field-name matching — but do it **before**
|
||
freeze or each fix is a rename = major.
|
||
|
||
CORE will emit per segment:
|
||
|
||
| CORE field | Type | Note |
|
||
|---|---|---|
|
||
| `index` | int ≥ 0 | **`event.task.progress` currently says `i`.** Pick one name for both. |
|
||
| `startByte` | int ≥ 0 | absolute byte offset, inclusive |
|
||
| `endByte` | int | absolute byte offset, **inclusive** — range is `[startByte, endByte]` (see below) |
|
||
| `completed` | int ≥ 0 | bytes written in this range so far |
|
||
| `speedBps` | int ≥ 0 | current per-segment rate |
|
||
| `state` | enum | `connecting` \| `downloading` \| `stalled` \| `complete` \| `failed` |
|
||
|
||
**Resolved by PROTO at freeze:** `endByte` is **inclusive** (matches HTTP `Range`
|
||
semantics — `Range: bytes=start-end` is inclusive — and removes an off-by-one at the curl
|
||
boundary). CORE designs stage 6 (segmenter/stealer) against inclusive. The earlier
|
||
`[start, end)` ask is withdrawn.
|
||
|
||
Still open: (a) reconcile `i` vs `index`; (b) confirm the segment `state` enum values;
|
||
(c) **empty-segment representation** — see B3a.
|
||
|
||
### B3a. Zero-length segment must be representable — *PROTO is fixing; CORE's requirement*
|
||
|
||
With `endByte` inclusive and `minimum: 0`, a zero-length segment (`endByte = startByte -
|
||
1`) at offset 0 is `endByte = -1`, which the schema forbids. The one case CORE actually
|
||
needs: a **whole-file zero-length download** (`Content-Length: 0`) — one segment, length
|
||
0. It is a valid HTTP response and the daemon/GUI must be able to hold it.
|
||
|
||
CORE will **not** produce empty segments mid-download: the `min_segment_bytes` floor
|
||
(1 MiB, `docs/04` §3) means the segmenter never splits below 1 MiB and the stealer only
|
||
takes a half-range if it is ≥ that floor. So B3a is purely about the degenerate
|
||
whole-file case.
|
||
|
||
Preference: encode segments as `startByte` + `length` (+ `completed`) rather than an
|
||
inclusive `endByte` with a `startByte - 1` sentinel — `length: 0` is then the natural
|
||
representation and there is no negative value to allow. If `endByte` inclusive stays,
|
||
then a 0-byte task needs an explicit encoding (an `empty`/`length` field, or permitting
|
||
`endByte = startByte - 1` with `minimum: -1`) — any of those work for CORE as long as
|
||
total length 0 round-trips. Flag back if the chosen fix needs anything else from CORE.
|
||
|
||
### B4. `bufferBytes` range is wrong in the frozen schema — *see `buffer-sizing.md`*
|
||
|
||
`contracts/` froze `bufferBytes` at 4 KiB – 8 MiB **in four schema files** (`DownloadSpec`,
|
||
`TaskDetail`, `download.update` patch, and `Settings.schema.json connection.bufferBytes`);
|
||
`docs/04` §4 says 64 KiB – 64 MiB default 4 MiB; the RSS DoD (`docs/04` §8 line 125) holds
|
||
with neither. CORE's recommendation — **64 KiB – 16 MiB, default 1 MiB**, plus a new
|
||
**`maxActiveSegments` (32)** global concurrent-segment cap and **`maxTotalBufferBytes`
|
||
lowered to 128 MiB** and *added to the contract*, with line 125 kept at 60 MB "given
|
||
`maxActiveSegments = 32`" (or explicitly raised to 120 MB) — and the per-segment RSS
|
||
arithmetic are in [`core/docs/buffer-sizing.md`](buffer-sizing.md). PROTO to land the four
|
||
schema edits + `docs/04` §4 + §8 + ADR together.
|
||
|
||
---
|
||
|
||
## Do not hold the freeze for these
|
||
|
||
All minor under rule 4 (new methods / new events / a pattern on an existing optional
|
||
string). `contracts/README.md` rule 5 wants contract changes "cheap and frequent" — raise
|
||
these now, land them as small follow-up PRs, but **do not gate `1.0.0` on them.** Each is
|
||
needed *within* M1.
|
||
|
||
### F1. A "needs user decision" carrier — *new event + method, minor*
|
||
|
||
`docs/04` §5 mandates a "File on server has changed — restart download?" prompt, and §7
|
||
has 416/stale-metadata. The event surface has only `event.notify` + task states. Either:
|
||
- confirm `state: paused` + an `event.notify {level:"warn", …}` is the intended carrier
|
||
(CORE reports `server_file_changed`, DAEMON pauses + notifies, GUI offers restart), **or**
|
||
- add `event.task.decision {taskId, kind, options[]}` + `download.decide {taskId, choice}`.
|
||
|
||
CORE needs to know which, but a string comparison on `error.code` covers the engine side
|
||
either way, so this doesn't block freeze.
|
||
|
||
### F2. A credential return path for 401/407 — *new method, minor, M1-needed*
|
||
|
||
`event.auth.required` exists; there is **no method to hand credentials back to a paused
|
||
task**. `session logins` / Secret Service is M3, but "401 handled" is in CORE's M1 DoD.
|
||
Requesting `download.provideAuth {taskId, username, password, save?}` →
|
||
`{ok}`. Secrets go to the Secret Service, never SQLite/logs (`CLAUDE.md` §4) — `save`
|
||
just tells DAEMON whether to persist.
|
||
|
||
### F3. `checksum` string format — *pattern on an existing optional field, borderline*
|
||
|
||
`download.add {checksum?}` has no shape. CORE's verifier will accept `"<algo>:<hex>"`
|
||
with `algo ∈ {md5, sha1, sha256}` (appendix lists MD5/SHA-256). Adding a `pattern` to an
|
||
existing optional string is close to free; PROTO's call whether it's worth a fixture now
|
||
or a doc note.
|
||
|
||
---
|
||
|
||
## Three-way ADR material (after PROTO answers)
|
||
|
||
### D1. State-machine ownership split — CORE / DAEMON / PROTO
|
||
|
||
`docs/04` §1 hands CORE the lifecycle, but `queued` and every scheduler transition are
|
||
DAEMON's. Proposed split, to be recorded as an ADR once all three agree:
|
||
|
||
- **CORE owns:** `probing → connecting → downloading ⇄ paused → retry_wait → assembling
|
||
→ verifying → complete | failed`, plus `cancelled` from any state.
|
||
- **DAEMON owns:** `new`, `queued`, and pause-for-schedule (enters CORE's `paused`).
|
||
- **Shared:** `paused` is reachable from either side; both must be idempotent about it.
|
||
|
||
This needs an ADR regardless of the answer (`contracts/README.md` rule 4: the lifecycle
|
||
enum is a frozen type and any change to who drives it is a documented decision).
|