proto: stop conformance from downloading real files, ADR the null-clearing gap
1. download.add.json had startMode "now" against a real, large (~6 GB)
Ubuntu ISO with saveDir hardcoded to /home/sami/Downloads/Programs.
Against a real veloxd (tests/conformance/run.sh) that's a real
download into the real user's real home, every single run — it had
already happened twice. startMode -> "later" (exercises the add path,
hands nothing to the engine) and saveDir is dropped entirely (resolves
to saveTo.defaultDir instead, checked against allowedRoots the same
way). Documented the rule this fixture was breaking in
contracts/fixtures/README.md so it doesn't happen a third time.
Auditing the rest for the same shape (now real: capture.offer, D7)
found a second, subtler instance: capture.offer.take.json's "take"
admits a real, immediately-started task the same way download.add
does, and the "Programs" category's saveDir is a migration-seeded
builtin (~/Downloads/Programs) that no isolated test setup can
redirect -- so even after pointing the URL at example.org (RFC 2606),
a real ~6 GB sparse .veloxpart still landed in the real home on the
declared Content-Length alone. Shrunk to a plausible-but-small 5 MiB.
Also scoped to "transport": "uds" -- a real "take" persists an active
task, so replaying the same fixture again on the second live transport
against the same shared daemon was hitting capture.offer's own
dedupe-by-URL and failing on a missing taskId, not a bug.
download.add's other real-URL siblings (errors/*.invalid-path,
*.invalid-params, *.disk-full) all fail before admission or are
requires-gated; left alone.
2. ADR 0018: DAEMON can set a nullable field through download.update /
settings.set but never clear it back to null, because the generated
C++ parser collapses "absent" and "explicit null" to the same
std::nullopt for every optional field (contracts/codegen/gen_cpp.py,
on purpose, and correct for create-style params -- just wrong for
patch-style ones, which is the only place the schema documents
"explicit null clears"). Decision: an opt-in x-clearable schema
annotation makes just those fields std::optional<std::optional<T>> in
C++ (TS already round-trips this natively); not a blanket rule
(would retype response fields like TaskSummary.effectiveUrl that have
no clear-vs-absent distinction to make), not an explicit clear-list
field (would redesign a wire contract DAEMON already built against
just to route around a generator gap). Recorded, not implemented here
-- that's its own PROTO PR (schema annotations + gen_cpp.py + gen_ts.py
+ regeneration + a minor VERSION bump per ADR 0015), not bundled into
a fixture-safety pass. Left a pointer to the ADR at the generator
comment it concerns.
3. Re-verified every xfail entry against current deferrals.md rather
than trust the reasons already on file: D7/D8 (capture.offer/
getRules), D3d/e/f/g/h/i (rules, queue.reorder, schedule, limiter,
download.update/refreshUrl) and D9 (settings) have all closed since
the list was last pruned, so most of it was stale. Removed everything
that now cleanly passes; kept and re-reasoned everything that doesn't:
- errors/download.provideAuth.not-found.json stays, as asked: real
bug, on_download_provideAuth never checks the task exists.
- category.list.json (mimeTypes -- documented D3a gap), schedule.set.json
(nextRunAt -- documented D3f gap): unchanged in substance, reason
text was already accurate.
- download.probe/get/list/update.json, session.hello.json,
queue.start/reorder.json, category.remove.json: not bugs -- each
golden depicts a richer lifecycle/config state (a probed download,
real queue or category membership, media/grabber capabilities) than
this harness's fresh, never-started bound tasks and empty isolated
DB can produce.
- limiter.get.json: real fixture bug, not a daemon one -- applyToRunning
is a write-only instruction on limiter.set, on_limiter_get never
returns it; the golden shouldn't have had it either. Fixed the
fixture and tools/mockd's own limiter.get, which had the same field
hardcoded into its in-memory state independent of the fixture file.
- grabber.*/media.*: still genuinely stub (M4 territory).
Only remaining unexpected-pass surfaced while re-verifying
(errors/capture.offer.ignore.json, always "take" instead of "ignore")
traced to capture.minSizeBytes defaulting to 0 on a fresh daemon,
making its below-minimum-size scenario unreachable -- not a bug, so
raised the setting in run.sh's isolated seeding instead of xfailing it.
ctest -L conformance: green, 100% (2/2), ~87s.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01SFeUKLbdHizrJjLBeK7ffz
This commit is contained in:
@@ -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<std::optional<T>>`:
|
||||
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<T>` -> `optional<optional<T>>`)
|
||||
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<T>` 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<T>` 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<T>()`, used for *params* types the daemon receives — but the conformance C++ runner
|
||||
also instantiates `parse<T>()` 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.
|
||||
Reference in New Issue
Block a user