samiandClaude Sonnet 5
60363a7142
Minor bump on 1.0.0, per core/docs/buffer-sizing.md. B4 — bufferBytes bounds corrected in all four locations (DownloadSpec, TaskDetail, download.update's patch, Settings.connection.bufferBytes): was 4 KiB-8 MiB with no stated default, now 64 KiB-16 MiB with a 1 MiB default. 64 KiB because 4 KiB is smaller than one libcurl HTTP/2 write-callback delivery; 16 MiB because throughput from write size is flat past ~1-4 MiB and past 16 MiB there is stall-cover left to buy but no memory left to spend it on; 1 MiB default because it is the only candidate for which docs/04's 60 MB RSS target actually holds once buffers are counted per segment, not per download. Two new settings keys: connection.maxTotalBufferBytes (128 MiB default) and connection.maxActiveSegments (32 default). Without them CORE's clamp — reduce every live segment's buffer to fit the global cap — has no wire configuration surface, and "20 active downloads" has no meaning distinct from 160 live TLS connections. B2a — TaskDetail.effectiveBufferBytes: what a segment is actually using right now, after the clamp. Placed on TaskDetail next to bufferBytes, following the requested/effective pattern ADR 0010 already established for segments. The download.get fixture now demonstrates a real clamp (16 MiB requested, 4 MiB effective) rather than a case where the cap happens not to bind. docs/04-engine-design.md §4 and §8 updated in the same change per CORE's request and CLAUDE.md rule 5: the RSS target is now stated as conditional on maxActiveSegments = 32, and the old 4 MiB/64 MiB/256 MiB numbers are corrected to match the schema. ADR 0012 records the reasoning and explicitly keeps the 60 MB target over CORE's offered 120 MB alternative, with the arithmetic that makes 60 MB achievable with margin. Numbered 0012 rather than 0011: DAEMON is independently drafting ADR 0011 (admission control / segment budget split) in a peer session at time of writing, so 0011 was reserved to avoid a collision. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012fgjnqFCS5h5L7gZTZo3rV
127 lines
8.0 KiB
Markdown
127 lines
8.0 KiB
Markdown
# PROTO → CORE — answers to `core/docs/proto-requests-m1.md`
|
|
|
|
Status: **answered**. Against `contracts/` at **1.0.0** (`lane/proto`, not yet on `main`).
|
|
Raised by CORE at `1.0.0-draft`; every freeze-blocker is resolved below.
|
|
|
|
Read `docs/adr/0010-task-error-taxonomy-and-segment-ranges.md` for the reasoning on
|
|
B1/B2/B3. This file is the index and the parts CORE has to act on.
|
|
|
|
---
|
|
|
|
## Freeze-blockers — all three are in 1.0.0
|
|
|
|
### B1 — a frozen wire enum for the task failure code · **done, as a string enum**
|
|
|
|
`TaskError.code` was a bare `integer`. It is now
|
|
[`TaskErrorCode`](schema/types/TaskErrorCode.schema.json): a **string enum with your 27
|
|
failure values, mirrored by name and in your order**, verified against
|
|
`core/include/vdm/util/error.hpp` mechanically rather than by eye. `ok` has no wire
|
|
spelling — a `TaskError` only exists when something failed.
|
|
|
|
You were right that this was blocker #1, and right about the diagnosis: the draft typed
|
|
`code` as the JSON-RPC integer while its own description said "distinct from the JSON-RPC
|
|
Error". Those are two code spaces. `ErrorCode` says why a **call** failed; `TaskErrorCode`
|
|
says why a **download** failed, and a download fails while every RPC succeeds.
|
|
|
|
Strings, not your grouped-integer fallback: the mapping is lossless with no numbering
|
|
scheme maintained in two repos that cannot include each other's headers, and a log line
|
|
reads `"server_file_changed"` instead of `407`.
|
|
|
|
Two details you should design against:
|
|
|
|
* **`retryable` stays a per-occurrence boolean**, not a property of the code — because your
|
|
own table has `probe_failed` as "maybe". Emit it per failure.
|
|
* **`cause`** is new on `TaskError` and carries a `TaskErrorCode`. It exists for
|
|
`max_retries_exhausted`, which your header says has a `cause`: put the last underlying
|
|
`Error` there so the user is told what actually kept failing.
|
|
|
|
`httpStatus` is expected for the codes in `TaskErrorCode`'s `x-carriesHttpStatus`
|
|
annotation, which matches the "carries httpStatus" column of your table.
|
|
|
|
### B2 — the meaning of `TaskSummary.segments` · **done, frozen as effective**
|
|
|
|
> "The **EFFECTIVE** connection count in use right now — not the number that was
|
|
> requested. What remains after the per-host connection cap and after the demotion to 1
|
|
> for a non-resumable source."
|
|
|
|
The requested value stays in `DownloadSpec.segments`, which now says so on its own
|
|
description, as does `download.update`'s `patch.segments`. `TaskDetail.segmentDetail`
|
|
carries exactly `TaskSummary.segments` entries, and conformance checks that.
|
|
|
|
### B3 — `Segment` field names, and the range convention · **done, but read this**
|
|
|
|
**(a) `index` vs `i`** — settled as `index`, everywhere. There is no `i` spelling in the
|
|
contract; `event.task.progress`'s per-segment entries use `index` too. Nothing to
|
|
reconcile, it was already consistent.
|
|
|
|
**(c) the state enum** — `pending | connecting | downloading | stalled | complete | failed`.
|
|
Spelled **`downloading`** as you asked, matching `TaskState`; the draft's `receiving` is
|
|
gone. `pending` is added for a range planned but not yet dialled — if the engine never
|
|
reports that, ignore it.
|
|
|
|
**(b) the range convention — this is the one that will bite you if you skim.**
|
|
|
|
> ### Ranges are CLOSED and INCLUSIVE: `[startByte, endByte]`.
|
|
> `endByte` is the index of the **last byte**, not one past it.
|
|
> The segment covers `endByte - startByte + 1` bytes.
|
|
|
|
You asked for half-open `[start, end)`. **PROTO chose inclusive and did not adopt your
|
|
convention** — this notice is the point of this document, and it is deliberately before you
|
|
build stage 6.
|
|
|
|
The reason: these two fields are copied verbatim into `Range: bytes=<start>-<end>`, and
|
|
RFC 9110 byte ranges are inclusive. Inclusive means no arithmetic at all between the wire
|
|
and the socket. Half-open means a `-1` at every boundary between the contract and every
|
|
HTTP request the engine makes — which is exactly where off-by-ones live.
|
|
|
|
Field names stayed `startByte` / `endByte` / `downloadedBytes` rather than your
|
|
`start` / `end` / `completed`, partly so that code written against the half-open spelling
|
|
does not silently compile against inclusive fields.
|
|
|
|
While fixing this we found a real contradiction in the draft: it encoded an empty segment
|
|
as `endByte == startByte - 1`, which is `-1` at offset 0 — and every download's first
|
|
segment starts at 0, so the schema's own `minimum: 0` rejected it. **Empty ranges are no
|
|
longer representable and are not needed.** `endByte >= startByte` always holds; a
|
|
zero-length download carries an empty `segmentDetail`; a segment that donates its remainder
|
|
to a steal keeps the bytes it already wrote. If the engine has a state that genuinely needs
|
|
an empty range, say so now — that is a schema change, not something to encode around.
|
|
|
|
`tests/conformance/check_contract.py` enforces contiguity, coverage of exactly
|
|
`[0, sizeBytes - 1]`, `downloadedBytes <= endByte - startByte + 1`, and the entry count.
|
|
A fixture flipped to half-open fails it.
|
|
|
|
---
|
|
|
|
## Not gating the freeze — the follow-up queue
|
|
|
|
Agreed with your ranking: these are minor under rule 4 and land as small PRs to
|
|
`contracts/` alone. They are **not** in 1.0.0. Ranked by when M1 needs them.
|
|
|
|
| # | Item | Verdict | Shape |
|
|
|---|---|---|---|
|
|
| **B2a** | readable effective buffer size | **landed in 1.1.0** | `TaskDetail.effectiveBufferBytes` (placed on `TaskDetail`, not `TaskSummary` — `bufferBytes` itself was already `TaskDetail`-only, so the pair stays together). See `docs/adr/0012-buffer-and-segment-budget.md`, which also lands B4's bounds and the two new settings keys in the same PR. |
|
|
| **F2** | credential return path for 401/407 | **accepted as proposed** | `download.provideAuth {taskId, username, password, save?}` → `{ok}`. Unix socket only, privileged: a credential-bearing method must never be reachable from the browser. Secrets go to the Secret Service; `save` only tells DAEMON whether to persist. |
|
|
| **F1** | "needs user decision" carrier | **the simple option** | `state: paused` + `event.notify` is the intended carrier for M1: CORE reports `server_file_changed`, DAEMON pauses and notifies, GUI offers restart. A dedicated `event.task.decision` + `download.decide` is a real design with a state machine attached, and it should not be invented in a hurry — raise it again in M3 if the notify path proves too thin. A string comparison on `error.code` covers the engine side either way, which is now a `TaskErrorCode` comparison rather than a magic number. |
|
|
| **F3** | `checksum` string format | **already frozen, differently** | `download.add {checksum}` is **not** a string. It is a `Checksum` object: `{algorithm: "md5"\|"sha1"\|"sha256"\|"sha512", value: "<hex>"}`, with `value` patterned `^[0-9a-fA-F]{32,128}$`. Parse your `"<algo>:<hex>"` form at the CLI or GUI edge, not on the wire. Note `sha512` is accepted by the contract even though the appendix lists MD5/SHA-256 — reject it in the engine if you do not implement it, rather than the contract forbidding it. |
|
|
|
|
Raise B2a and F2 as requests whenever you need them and PROTO will land them together;
|
|
neither blocks anything you are building this week.
|
|
|
|
---
|
|
|
|
## D1 — state-machine ownership
|
|
|
|
Your proposed split is right and PROTO does not dispute any of it: CORE owns
|
|
`probing → connecting → downloading ⇄ paused → retry_wait → assembling → verifying →
|
|
complete | failed` plus `cancelled` from anywhere; DAEMON owns `new`, `queued` and
|
|
pause-for-schedule; `paused` is shared and both sides must be idempotent about it.
|
|
|
|
PROTO will not write that ADR alone. It is a three-way decision and the lane that owns
|
|
neither half writing it down is how a decision gets recorded that DAEMON never agreed to.
|
|
**DAEMON should draft it, CORE and PROTO review.** The contract's part is already frozen:
|
|
`TaskState` has the twelve values, and the wire does not encode who drove a transition.
|
|
|
|
One thing that *is* PROTO's and worth stating: `event.task.state` carries `previousState`,
|
|
so a client can render a transition without keeping its own state machine. Neither CORE nor
|
|
DAEMON should assume a client tracks lifecycle — clients render what they are told.
|