diff --git a/contracts/codegen/gen_cpp.py b/contracts/codegen/gen_cpp.py index 402fda8..987a49a 100644 --- a/contracts/codegen/gen_cpp.py +++ b/contracts/codegen/gen_cpp.py @@ -506,7 +506,12 @@ def emit_field_parse(f: Field, indent: str) -> list[str]: f'{i} const auto it = j.find("{f.name}");'] if f.optional: # Absent and null mean the same thing: the field is not set. A client that omits - # a nullable field and one that sends null are treated identically on purpose. + # a nullable field and one that sends null are treated identically on purpose -- + # correct for create-style params, where there is no existing value to distinguish + # "never set" from "explicitly cleared". Patch-style fields need the distinction + # (download.update's patch: "an explicit null clears a nullable field") and get an + # opt-in exception via x-clearable per ADR 0018 (not implemented yet: this is the + # decision record, not the generator change). o.append(f"{i} if (it != j.end() && !it->is_null()) {{") o += emit_value_parse(f.type, "(*it)", "val", "fp", i + " ") o.append(f"{i} out.{m} = std::move(val);") diff --git a/contracts/fixtures/README.md b/contracts/fixtures/README.md index 9aee282..c15d5be 100644 --- a/contracts/fixtures/README.md +++ b/contracts/fixtures/README.md @@ -21,7 +21,7 @@ fixtures/ ```jsonc { - "name": "download.add — start an ISO now, into the Programs category", + "name": "download.add — add an ISO for later, into the Programs category", "description": "Why this case is worth pinning.", "transport": "uds", // optional: replay only on this transport "requires": "...", // optional: a condition a plain server cannot produce @@ -81,3 +81,27 @@ cases in `tests/integration/`. correct response is *no response*: past 750 ms the extension must abandon the offer and let Firefox download normally. A download manager that eats downloads when its daemon is down is worse than no download manager. + +## No fixture may pair a real external URL with `startMode: "now"` + +This suite replays every fixture against a real, live `veloxd` (`tests/conformance/run.sh`), +not just `mockd`. `mockd` never actually fetches anything, so it hid this for a while: a +fixture with `startMode: "now"` (or `"queue"` into a running queue — anything that gets +admitted to the scheduler right away) and a real, resolvable URL makes a **real** daemon +actually start downloading it, for real, onto whatever machine runs the suite. This +happened — twice, with `download.add.json` pointed at a ~6 GB Ubuntu ISO, straight into the +developer's real `~/Downloads`. + +The fix in each case is one of: +- `startMode: "later"` — exercises the add path (validation, category assignment, the + event) without ever handing the task to the engine; +- a URL under `example.org`/`example.com` (IANA-reserved for exactly this, RFC 2606) — + resolvable enough to validate as a URL, never a real download source; +- `requires`, if the fixture's entire point needs a real transfer to fail in a specific way + (see `errors/download.add.disk-full.json`) — skipped by default, so it only ever runs + where the condition has actually been arranged. + +A real `saveDir` gets the same treatment for the same reason: an absolute path like +`/home/sami/Downloads/...` only means anything on the machine that fixture was written on. +Omit `saveDir` and let `saveTo.defaultDir` apply, or use a relative-feeling path under a +root the runner controls. diff --git a/contracts/fixtures/capture.offer.take.json b/contracts/fixtures/capture.offer.take.json index ff3b13a..f5db860 100644 --- a/contracts/fixtures/capture.offer.take.json +++ b/contracts/fixtures/capture.offer.take.json @@ -1,22 +1,23 @@ { "name": "capture.offer — attachment on a monitored type is taken", - "description": "Golden fixture. tests/conformance replays this against the real daemon AND the TS client. If either side drifts, this goes red before the lanes ever integrate.", + "description": "Golden fixture. tests/conformance replays this against the real daemon AND the TS client. If either side drifts, this goes red before the lanes ever integrate. url is example.org (RFC 2606), not a real download source: 'take' against a real veloxd (tests/conformance/run.sh) admits a real task and hands it to the engine for real, and no fixture may do that against a real external URL. contentLength is a plausible-but-small 5 MiB rather than a real ISO's size: the 'Programs' category's saveDir is a migration-seeded builtin (~/Downloads/Programs, daemon/src/store/migrations/0001_initial.sql), not something an isolated test run's settings can redirect, so 'take' always sparse-preallocates into that real path on whatever machine runs this suite -- keeping the declared size small keeps that footprint trivial instead of a real ISO's worth of disk. transport is uds only: a real 'take' persists an active task, so replaying this same fixture again on a second live transport against the same daemon would correctly dedupe against it (capture.offer dedupes by exact URL) and get 'ignore' instead -- an artifact of replaying one fixture against one shared daemon over two transports, not a behaviour to golden.", + "transport": "uds", "request": { "jsonrpc": "2.0", "id": 42, "method": "capture.offer", "params": { - "url": "https://releases.ubuntu.com/26.04/ubuntu-26.04-desktop-amd64.iso", + "url": "https://example.org/dl/ubuntu-26.04-desktop-amd64.iso", "method": "GET", - "tabUrl": "https://releases.ubuntu.com/26.04/", + "tabUrl": "https://example.org/26.04/", "headers": { "User-Agent": "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:154.0) Gecko/20100101 Firefox/154.0", - "Referer": "https://releases.ubuntu.com/26.04/", + "Referer": "https://example.org/26.04/", "Accept": "*/*" }, "cookies": [], "contentType": "application/octet-stream", - "contentLength": 6228541440, + "contentLength": 5242880, "contentDisposition": "attachment; filename=\"ubuntu-26.04-desktop-amd64.iso\"", "filename": "ubuntu-26.04-desktop-amd64.iso", "origin": "moz-extension://11111111-2222-3333-4444-555555555555" diff --git a/contracts/fixtures/download.add.json b/contracts/fixtures/download.add.json index 9733715..ad1a9f2 100644 --- a/contracts/fixtures/download.add.json +++ b/contracts/fixtures/download.add.json @@ -1,6 +1,6 @@ { - "name": "download.add \u2014 start an ISO now, into the Programs category", - "description": "The ordinary add path. saveDir is canonicalized and checked against the allowed roots before anything is written.", + "name": "download.add — add an ISO for later, into the Programs category", + "description": "The ordinary add path. saveDir is canonicalized and checked against the allowed roots before anything is written. startMode is 'later' deliberately: this suite replays against a real veloxd (tests/conformance/run.sh), and a real daemon given startMode 'now' would actually start fetching url for real. No fixture may pair a real external URL with startMode 'now' -- see contracts/fixtures/README.md.", "request": { "jsonrpc": "2.0", "id": 11, @@ -8,10 +8,9 @@ "params": { "url": "https://releases.ubuntu.com/26.04/ubuntu-26.04-desktop-amd64.iso", "filename": "ubuntu-26.04-desktop-amd64.iso", - "saveDir": "/home/sami/Downloads/Programs", "categoryId": "programs", "segments": 8, - "startMode": "now" + "startMode": "later" } }, "response": { @@ -19,13 +18,13 @@ "id": 11, "result": { "taskId": "$uuid", - "state": "connecting", + "state": "paused", "duplicate": null } }, "assertions": [ - "the .veloxpart file is created sparse and preallocated at the final size", - "saveDir resolves inside saveTo.allowedRoots, or the call fails -32011 having written nothing", + "saveDir is omitted here on purpose: it resolves to saveTo.defaultDir, which is itself checked against saveTo.allowedRoots the same way an explicit saveDir would be -- see errors/download.add.invalid-path.json for the -32011 case", + "startMode 'later' lands the task in 'paused' and never hands it to the engine, so nothing is fetched and no .veloxpart is created yet -- that only happens once the task is actually started (download.start.json, or startMode 'now'/'queue' against a source this suite controls)", "event.task.added is emitted to every subscriber before this reply is sent" ] } diff --git a/contracts/fixtures/limiter.get.json b/contracts/fixtures/limiter.get.json index e618c9c..e7a75c7 100644 --- a/contracts/fixtures/limiter.get.json +++ b/contracts/fixtures/limiter.get.json @@ -1,6 +1,6 @@ { "name": "limiter.get \u2014 the limiter is off", - "description": "globalBps still carries the last configured value so the GUI can restore it when the user re-enables the limit.", + "description": "globalBps still carries the last configured value so the GUI can restore it when the user re-enables the limit. applyToRunning is a write-only instruction on limiter.set (\"retune already-running transfers now\", not a persisted setting), so it never comes back from get.", "request": { "jsonrpc": "2.0", "id": 52, @@ -12,11 +12,11 @@ "id": 52, "result": { "enabled": false, - "globalBps": 2097152, - "applyToRunning": false + "globalBps": 2097152 } }, "assertions": [ - "enabled false means no throttling regardless of globalBps" + "enabled false means no throttling regardless of globalBps", + "applyToRunning is absent, not false: it's meaningless outside a limiter.set call" ] } diff --git a/docs/adr/0018-nullable-optional-fields-absent-vs-explicit-null.md b/docs/adr/0018-nullable-optional-fields-absent-vs-explicit-null.md new file mode 100644 index 0000000..ef5333b --- /dev/null +++ b/docs/adr/0018-nullable-optional-fields-absent-vs-explicit-null.md @@ -0,0 +1,107 @@ +# ADR 0018 — Nullable optional fields: absent vs. explicit null + +**Status:** accepted · **Date:** 2026-09-13 · **Lane:** PROTO +**Prompted by:** a DAEMON report against `download.update`: the generated C++ parser gives +`VeloxDispatcher` no way to tell "the caller left this field alone" from "the caller wants +it cleared," so `download.update` and (the moment a nullable `SettingKey` exists) +`settings.set` can set a nullable field but never clear it back to `null`. + +## Context + +`download.update`'s `patch` object documents the convention plainly: "Only the present +fields change. An explicit null clears a nullable field." That is a deliberate, already- +committed wire contract — not something up for redesign here. The gap is one layer down: +`contracts/codegen/gen_cpp.py`'s `emit_field_parse` collapses "key absent" and "key present +with value `null`" to the same `std::nullopt`, on purpose, and the comment says so: + +> Absent and null mean the same thing: the field is not set. A client that omits a +> nullable field and one that sends null are treated identically on purpose. + +That collapse is *correct* for the common case — most nullable-optional fields are on +create-style params (`DownloadSpec.saveDir`, `.categoryId`, …) where there is no existing +value to distinguish "never set" from "explicitly cleared" in the first place; either way +the daemon just uses a default. It is wrong specifically for **patch-style** params, where +a field can already hold a value and the caller needs to say which of two different things +they mean: "leave it" or "clear it." + +The schema IR (`schema_ir.py`) already tracks `required` and `nullable` as two independent +booleans per `Field`, so the information needed to make this distinction exists all the way +through parsing — `emit_field_parse` just doesn't act on it. Only `download.update`'s +`patch` object is affected today (`filename`, `saveDir`, `categoryId`, `queueId`, +`description`, `segments`, `bufferBytes`, `checksum` — all eight of its fields are +nullable-and-optional with exactly this "leave vs. clear" meaning). No `SettingKey` is +nullable yet, so `settings.set` has no live instance of the bug, but the same shape +(`values` patches an existing bag) means the first nullable settings key will hit the exact +same gap. + +## Decision + +**A JSON-null-aware optional, opt in per field via a new `x-clearable: true` annotation — +not a blanket rule and not a companion "clear list" field.** + +- New per-field schema annotation, `x-clearable: true`, valid only on a field whose type + already includes `null` (schema error otherwise — clearable implies nullable). Marks + "this field distinguishes absent from explicit null"; every other nullable-optional field + keeps today's collapse. +- The generated C++ type for a `x-clearable` field becomes `std::optional>`: + outer `nullopt` = absent (leave unchanged), outer engaged with an inner `nullopt` = + explicit `null` (clear it), outer engaged with an inner value = set it. One field, three + states, no parallel bitset to keep in sync and no second field to forget to check. +- `emit_field_parse` for such a field stops folding `is_null()` into "absent": absent skips + the assignment (outer stays `nullopt`); present-and-null assigns an engaged-but-empty + inner optional; present-and-valued parses normally into the inner optional. Every other + field's codegen (the `required`/`nullable`-but-not-`clearable` majority) is unchanged. +- TypeScript needs no generator change: `field?: T | null` already round-trips this exactly + the way JSON does — an omitted key serializes as absent, `null` serializes as `null`, and + `"field" in obj` / `obj.field === null` already distinguish the three states natively. + This gap is a C++-generator-only problem. +- Applies now to `download.update`'s eight `patch` fields. `Settings` gets no annotation + today (nothing nullable to mark); the day a nullable `SettingKey` is added, it gets + `x-clearable: true` in the same PR, not left to rediscover this ADR. + +## Versioning + +Per ADR 0015: this retypes a generated C++ field (`optional` -> `optional>`) +with the wire byte-for-byte unchanged — a client sending the same JSON parses correctly +either way. **Minor bump, with a migration note** for anyone reading `patch.filename` et al. +directly (unwrap twice: check the outer, then the inner). Not major; `session.hello`'s +major-only check must not refuse a wire-compatible peer over a binding-only change. + +## Consequences + +- `on_download_update` (DAEMON, not this lane) can finally implement "explicit null + clears": read the outer optional for presence, the inner for clear-vs-value, exactly the + three states the schema already promised. +- The collapse comment in `emit_field_parse` stays as the default behavior and gets a + pointer to this ADR for the opt-in exception, instead of being read as an oversight. +- Implementation (schema annotation support in `schema_ir.py`, the `gen_cpp.py` emission + change above, regenerating `core/generated/`, the `x-clearable: true` annotations on + `download.update`'s eight fields, the VERSION bump and migration note) is **not** done in + this change — recorded here so DAEMON isn't blocked on relitigating the design, tracked as + its own PROTO PR per the normal contracts process (schema + regenerated code + fixtures + + VERSION bump together, CLAUDE.md §2). + +## Alternatives rejected + +**An explicit clear list** (e.g. `patch.clearFields: ["categoryId", …]`, plain non-nullable +`optional` fields otherwise). Rejected: the wire contract "an explicit null clears a +nullable field" is already written into `download.update`'s schema description and is what +DAEMON built against — this would be a real, disruptive wire redesign to route around a +generator gap, not a fix for it. It also doesn't compose: every patch-shaped object gains a +second array to keep in sync with the first, by hand, forever. + +**A parallel "which fields were present" bitset** (struct of `optional` fields plus a +sibling presence-flags struct or bitset). Rejected: two things to check per field instead +of one, and nothing stops a caller from reading the optional and forgetting the presence +bit — exactly the class of bug this ADR exists to close. + +**Apply the tri-state to every `nullable && !required` field automatically**, using the IR +flags already present, no annotation needed. Rejected: `emit_field_parse` only backs +`parse()`, used for *params* types the daemon receives — but the conformance C++ runner +also instantiates `parse()` for **result** types (round-tripping golden fixtures), and +plenty of those are nullable-optional with no patch semantics at all (`TaskSummary.effectiveUrl`, +"null until the first probe succeeds" — a plain nullable value, not a leave-or-clear +choice). Blanket application would retype those too, forcing every read site across the +daemon that already does `if (summary.effectiveUrl)` into an unwanted double-unwrap for a +distinction that field doesn't have. Opt-in keeps the blast radius at exactly the fields +that need it. diff --git a/tests/conformance/run.sh b/tests/conformance/run.sh index 4199b50..1db6943 100755 --- a/tests/conformance/run.sh +++ b/tests/conformance/run.sh @@ -160,11 +160,18 @@ if [ -z "$EXTERNAL_UDS" ] && [ -z "$EXTERNAL_WS" ]; then for _ in $(seq 1 50); do [ -S "$VUDS" ] && break; sleep 0.2; done [ -S "$VUDS" ] || { echo "veloxd did not start:"; cat "$WORK/veloxd.log"; exit 1; } - # saveTo.allowedRoots defaults to ["~/Downloads"]; download.add.json (fixture) asks - # for a saveDir under $HOME/Downloads, so both that and download.add's own isolated - # downloads dir need to be allowed roots, or every download.add fixture fails -32011 - # before the point of this runner is even reached. settings.set is itself a D3 stub, - # so this is written straight into the isolated velox.db rather than over the wire. + # saveTo.allowedRoots defaults to ["~/Downloads"]; download.add's own isolated + # downloads dir needs to be an allowed root too, or every download.add fixture fails + # -32011 before the point of this runner is even reached. $HOME/Downloads stays in + # the list alongside it: a few fixtures still set an explicit saveDir there + # (category.upsert.json, download.update.json) rather than take the default. + # capture.minSizeBytes defaults to 0 (nothing is ever "too small"), which makes + # errors/capture.offer.ignore.json's below-minimum-size case impossible to reach + # against a fresh daemon; raised here so that fixture's scenario is actually + # reachable. Written straight into the isolated velox.db, before veloxd has any RPC + # session to write it through: settings.set is real now (D9), but this has to be in + # place before the very first fixture runs, and setup happens before any connection + # exists. python3 - "$VXDG/data/velox/velox.db" "$VXDG/downloads" "$HOME/Downloads" <<'PY' import json, sqlite3, sys db_path, isolated_downloads, home_downloads = sys.argv[1:4] @@ -174,6 +181,11 @@ db.execute( "ON CONFLICT(key) DO UPDATE SET value = excluded.value", ("saveTo.allowedRoots", json.dumps([isolated_downloads, home_downloads])), ) +db.execute( + "INSERT INTO settings(key, value) VALUES(?, ?) " + "ON CONFLICT(key) DO UPDATE SET value = excluded.value", + ("capture.minSizeBytes", json.dumps(1000000)), +) db.execute( "INSERT INTO settings(key, value) VALUES(?, ?) " "ON CONFLICT(key) DO UPDATE SET value = excluded.value", diff --git a/tests/conformance/veloxd-xfail.json b/tests/conformance/veloxd-xfail.json index cde996f..59e33b7 100644 --- a/tests/conformance/veloxd-xfail.json +++ b/tests/conformance/veloxd-xfail.json @@ -1,40 +1,26 @@ [ - { "fixture": "contracts/fixtures/download.refreshUrl.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/download.update.json", "reason": "D3: stub handler, -32603" }, + { "fixture": "contracts/fixtures/grabber.harvest.json", "reason": "D3: stub handler, -32603 (M4 territory per deferrals.md)" }, + { "fixture": "contracts/fixtures/grabber.start.json", "reason": "D3: stub handler, -32603 (M4 territory per deferrals.md)" }, + { "fixture": "contracts/fixtures/grabber.status.json", "reason": "D3: stub handler, -32603 (M4 territory per deferrals.md)" }, - { "fixture": "contracts/fixtures/rules.list.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/rules.upsert.json", "reason": "D3: stub handler, -32603" }, - - { "fixture": "contracts/fixtures/settings.get.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/settings.set.json", "reason": "D3: stub handler, -32603" }, - - { "fixture": "contracts/fixtures/limiter.get.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/limiter.set.json", "reason": "D3: stub handler, -32603" }, - - { "fixture": "contracts/fixtures/schedule.get.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/schedule.set.json", "reason": "D3: stub handler, -32603" }, - - { "fixture": "contracts/fixtures/queue.reorder.json", "reason": "D3: stub handler, -32603" }, - - { "fixture": "contracts/fixtures/grabber.harvest.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/grabber.start.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/grabber.status.json", "reason": "D3: stub handler, -32603" }, - - { "fixture": "contracts/fixtures/media.addVariant.json", "reason": "D3: stub handler, -32603" }, - { "fixture": "contracts/fixtures/media.listVariants.json", "reason": "D3: stub handler, -32603" }, - - { "fixture": "contracts/fixtures/capture.getRules.json", "reason": "D3: stub handler, -32603 -- DAEMON is filing capture.offer next" }, - { "fixture": "contracts/fixtures/capture.offer.take.json", "reason": "D3: stub handler, -32603 -- DAEMON is filing capture.offer next" }, - { "fixture": "contracts/fixtures/errors/capture.offer.ignore.json", "reason": "D3: stub handler, -32603 -- DAEMON is filing capture.offer next" }, + { "fixture": "contracts/fixtures/media.addVariant.json", "reason": "D3: stub handler, -32603 (M4 territory per deferrals.md)" }, + { "fixture": "contracts/fixtures/media.listVariants.json", "reason": "D3: stub handler, -32603 (M4 territory per deferrals.md)" }, { "fixture": "contracts/fixtures/errors/download.provideAuth.not-found.json", "reason": "real bug: on_download_provideAuth (dispatcher.cpp) never checks the task exists -- TaskActionPort::provide_auth returns false for an unknown id, which the handler folds into a normal {ok:false} result instead of -32010" }, { "fixture": "contracts/fixtures/category.list.json", "reason": "documented gap (deferrals.md D3a note): categories table has no mimeTypes/sortOrder columns, so category.upsert accepts them but category.list never echoes mimeTypes back" }, + { "fixture": "contracts/fixtures/capture.getRules.json", "reason": "not a bug: capture.monitoredMimeTypes defaults to [] (store/settings.cpp's kDefaults) on a fresh daemon; the golden's non-empty example illustrates a configured one" }, + { "fixture": "contracts/fixtures/download.probe.json", "reason": "not a bug: requiresAuth is optional-and-omitted-when-false (schema doesn't require it); the golden shows it because that fixture's probe hit a 401, this run's doesn't" }, { "fixture": "contracts/fixtures/download.get.json", "reason": "not a bug: effectiveUrl is 'null until the first probe succeeds' (schema) and omitted rather than sent as null; our bound $taskId is a fresh, never-started task, so it's never been probed -- the golden depicts an in-progress download instead" }, { "fixture": "contracts/fixtures/download.list.json", "reason": "same as download.get.json: effectiveUrl omitted for our never-started bound tasks, golden depicts an in-progress download" }, - { "fixture": "contracts/fixtures/session.hello.json", "reason": "not a bug: capabilities is genuinely empty because media/grabber/Secret Service aren't implemented yet; the golden's ['media','grabber','secretservice'] illustrates a future daemon, not this one" }, + { "fixture": "contracts/fixtures/download.update.json", "reason": "not a bug: etaSeconds is only known for a task the engine has probed/is running; our bound $taskId is a fresh, never-started task, so it's absent -- same class as download.get.json's effectiveUrl" }, + { "fixture": "contracts/fixtures/session.hello.json", "reason": "not a bug: capabilities is genuinely empty because media/grabber aren't implemented yet (capture/Secret Service's parts of it now are); the golden's example list illustrates a future daemon, not this one" }, + { "fixture": "contracts/fixtures/queue.start.json", "reason": "not a bug: startedTaskIds is empty because nothing is a member of queue 'main' in this isolated run; the golden depicts a queue with real membership" }, + { "fixture": "contracts/fixtures/queue.reorder.json", "reason": "not a bug: the fixture's taskIds are two literal ids that only ever existed in a seeded mock; queue 'main' has no members at all in this isolated run, so any non-empty list is correctly rejected as not a permutation of (empty) membership" }, + { "fixture": "contracts/fixtures/rules.list.json", "reason": "not a bug: no rule is ever seeded in a fresh daemon; the golden depicts a configured rule set" }, + { "fixture": "contracts/fixtures/schedule.set.json", "reason": "documented gap (deferrals.md D3f note): nextRunAt is deliberately left unset -- computing it needs DST-aware next-transition logic sched/schedule_window.hpp doesn't have yet" }, { "fixture": "contracts/fixtures/category.remove.json", "reason": "not a bug: reassignedTaskIds is empty because nothing was ever filed under the 'firmware' category this run creates; the golden depicts a category with real membership" } ] diff --git a/tools/mockd/src/dispatch.ts b/tools/mockd/src/dispatch.ts index 5664315..9c29fbd 100644 --- a/tools/mockd/src/dispatch.ts +++ b/tools/mockd/src/dispatch.ts @@ -288,7 +288,10 @@ export class Dispatcher { } case 'limiter.get': - return state.limiter; + // applyToRunning is a write-only instruction on limiter.set ("retune already- + // running transfers now"), not a persisted setting, so get never echoes it back + // (contracts/fixtures/limiter.get.json). + return { enabled: state.limiter.enabled, globalBps: state.limiter.globalBps }; case 'limiter.set': { state.limiter = {