Compare commits
4
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
  samiandClaude Sonnet 5
|
4e177ec809 | ||
|
sami
|
0468b0176a | ||
|
sami
|
d8b7c128be | ||
|
samiandClaude Sonnet 5
|
e30d994d74 |
@@ -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);")
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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"
|
||||
]
|
||||
}
|
||||
|
||||
@@ -20,7 +20,7 @@
|
||||
],
|
||||
"defaults": {
|
||||
"url": "https://example.org/",
|
||||
"categoryId": "compressed",
|
||||
"categoryId": "programs",
|
||||
"startMode": "queue",
|
||||
"queueId": "main"
|
||||
}
|
||||
|
||||
@@ -30,5 +30,6 @@
|
||||
"the version check is transport-independent; this is replayed on the Unix socket so it is not masked by -32002",
|
||||
"data.expected is the daemon's own current protocol version string (kProtocolVersion), not a bare major and not pinnable in a golden file -- the conformance compare on error payloads is on `code` only, structural elsewhere, so echoing the live version is fine"
|
||||
],
|
||||
"transport": "uds"
|
||||
"transport": "uds",
|
||||
"closesConnection": true
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
]
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
+39
-13
@@ -40,16 +40,26 @@ while [ $# -gt 0 ]; do
|
||||
esac
|
||||
done
|
||||
|
||||
# Kill the server and anything it spawned. `kill $!` alone would only reap the subshell
|
||||
# wrapper and leave the node process holding the port, which then breaks the next run.
|
||||
# Kill the server and anything it spawned. `pkill -P "$pid"` only reaps direct children —
|
||||
# tsx's actual listener is often a grandchild, which that missed, leaving it holding the
|
||||
# port and breaking the next run (a leaked mockd once did exactly this). Every server
|
||||
# below is launched via `setsid`, which makes it the leader of its own new session/process
|
||||
# group (pgid == its own pid), so `kill -TERM -"$pid"` (negative: a process-group kill)
|
||||
# reaches it and everything it spawned in one shot, however deep.
|
||||
stop() {
|
||||
local pid="$1"
|
||||
[ -n "$pid" ] || return 0
|
||||
pkill -P "$pid" 2>/dev/null || true
|
||||
kill "$pid" 2>/dev/null || true
|
||||
kill -TERM -"$pid" 2>/dev/null || kill "$pid" 2>/dev/null || true
|
||||
wait "$pid" 2>/dev/null || true
|
||||
}
|
||||
|
||||
# A free loopback TCP port, kernel-assigned (bind :0) rather than a fixed number — a
|
||||
# hardcoded port means one leaked process from a previous run makes every future run fail
|
||||
# EADDRINUSE instead of just picking a different port.
|
||||
free_port() {
|
||||
python3 -c "import socket; s=socket.socket(); s.bind(('127.0.0.1',0)); print(s.getsockname()[1]); s.close()"
|
||||
}
|
||||
|
||||
cleanup() {
|
||||
stop "$MOCKD_PID"
|
||||
stop "$SLOW_PID"
|
||||
@@ -84,8 +94,8 @@ step "generated TypeScript against a live server"
|
||||
if [ -z "$EXTERNAL_UDS" ] && [ -z "$EXTERNAL_WS" ]; then
|
||||
( cd "$REPO/tools/mockd" && npm install --silent --no-audit --no-fund )
|
||||
UDS="$WORK/velox.sock"
|
||||
WS_PORT=52080
|
||||
( cd "$REPO/tools/mockd" && exec ./node_modules/.bin/tsx src/index.ts \
|
||||
WS_PORT="$(free_port)"
|
||||
( cd "$REPO/tools/mockd" && exec setsid ./node_modules/.bin/tsx src/index.ts \
|
||||
--uds "$UDS" --ws-port "$WS_PORT" --allowed-root "$WORK" ) >"$WORK/mockd.log" 2>&1 &
|
||||
MOCKD_PID=$!
|
||||
# Wait for the socket rather than sleeping a guessed amount.
|
||||
@@ -139,18 +149,29 @@ if [ -z "$EXTERNAL_UDS" ] && [ -z "$EXTERNAL_WS" ]; then
|
||||
VXDG="$WORK/veloxd-xdg"
|
||||
mkdir -p "$VXDG/runtime" "$VXDG/data" "$VXDG/config" "$VXDG/downloads"
|
||||
|
||||
# VELOX_PAIR_AUTO=1: the pairing approver is the D1 dev stub (EnvAutoApprover,
|
||||
# daemon/src/rpc/pairing.cpp) and denies every pairing without it — without this,
|
||||
# session.pair never issues a token and the WS half of this step can't even connect.
|
||||
XDG_RUNTIME_DIR="$VXDG/runtime" XDG_DATA_HOME="$VXDG/data" XDG_CONFIG_HOME="$VXDG/config" \
|
||||
"$VELOXD_BIN" >"$WORK/veloxd.log" 2>&1 &
|
||||
VELOX_PAIR_AUTO=1 \
|
||||
setsid "$VELOXD_BIN" >"$WORK/veloxd.log" 2>&1 &
|
||||
VELOXD_PID=$!
|
||||
VUDS="$VXDG/runtime/velox/velox.sock"
|
||||
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]
|
||||
@@ -160,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",
|
||||
@@ -183,7 +209,7 @@ fi
|
||||
step "capture.offer fails open when the daemon is too slow"
|
||||
if [ -z "$EXTERNAL_UDS" ]; then
|
||||
SLOW_UDS="$WORK/slow.sock"
|
||||
( cd "$REPO/tools/mockd" && exec ./node_modules/.bin/tsx src/index.ts \
|
||||
( cd "$REPO/tools/mockd" && exec setsid ./node_modules/.bin/tsx src/index.ts \
|
||||
--uds "$SLOW_UDS" --no-ws --slow 2000 ) >"$WORK/slow.log" 2>&1 &
|
||||
SLOW_PID=$!
|
||||
for _ in $(seq 1 50); do [ -S "$SLOW_UDS" ] && break; sleep 0.2; done
|
||||
|
||||
@@ -76,6 +76,12 @@ interface Fixture {
|
||||
/** A condition the server cannot produce from the request alone. Skipped unless the
|
||||
* harness has arranged it — see tests/integration. */
|
||||
requires?: string;
|
||||
/** This request is documented to make the *server* close the connection after replying
|
||||
* (e.g. a mismatched protocol major on the Unix socket). replay() reconnects afterward
|
||||
* so every later fixture in the shared-connection replay isn't sent into a dead socket
|
||||
* and left to time out one by one — which is silent when the fixture in question is
|
||||
* also on the xfail allowlist, since applyXfail accepts any failure reason. */
|
||||
closesConnection?: boolean;
|
||||
transport?: TransportName;
|
||||
deadlineMs?: number;
|
||||
request?: { jsonrpc: '2.0'; id: number | string; method: string; params?: unknown };
|
||||
@@ -196,11 +202,13 @@ function staticChecks(fixtures: readonly Fixture[]): Outcome[] {
|
||||
}
|
||||
|
||||
/**
|
||||
* Methods that destroy the state later fixtures rely on. Replayed last so the suite does
|
||||
* not depend on file order, which is the sort of thing that goes green locally and red in
|
||||
* CI on a different filesystem.
|
||||
* Methods that destroy state, or consume state another fixture creates. Replayed last so
|
||||
* the suite does not depend on file order, which is the sort of thing that goes green
|
||||
* locally and red in CI on a different filesystem — alphabetical happens to put
|
||||
* category.remove.json before category.upsert.json, and category.remove's fixture only
|
||||
* has a "firmware" category to delete because category.upsert's fixture just created one.
|
||||
*/
|
||||
const DESTRUCTIVE = new Set<string>(['download.remove']);
|
||||
const DESTRUCTIVE = new Set<string>(['download.remove', 'category.remove']);
|
||||
|
||||
function replayOrder(a: Fixture, b: Fixture): number {
|
||||
const rank = (f: Fixture): number => (DESTRUCTIVE.has(f.request?.method ?? '') ? 1 : 0);
|
||||
@@ -247,10 +255,13 @@ async function setupBindings(conn: Conn): Promise<{ bindings: Record<string, str
|
||||
return { bindings, setup };
|
||||
}
|
||||
|
||||
async function replay(conn: Conn, fixtures: readonly Fixture[],
|
||||
async function replay(initialConn: Conn, fixtures: readonly Fixture[],
|
||||
bindings: Record<string, string>,
|
||||
includeRequires = false): Promise<Outcome[]> {
|
||||
includeRequires = false,
|
||||
reconnect?: () => Promise<Conn>):
|
||||
Promise<{ outcomes: Outcome[]; conn: Conn }> {
|
||||
const out: Outcome[] = [];
|
||||
let conn = initialConn;
|
||||
const t = conn.transport;
|
||||
|
||||
for (const f of [...fixtures].sort(replayOrder)) {
|
||||
@@ -266,7 +277,14 @@ async function replay(conn: Conn, fixtures: readonly Fixture[],
|
||||
}
|
||||
|
||||
const deadline = f.deadlineMs ?? Math.max(METHODS[method].deadlineMs, 2000);
|
||||
if (process.env.DEBUG_CONFORMANCE) process.stderr.write(`>>> [${t}] ${f.file} ${method}\n`);
|
||||
const frame = await conn.request(method, bind(f.request.params ?? {}, bindings), deadline);
|
||||
if (process.env.DEBUG_CONFORMANCE) process.stderr.write(`<<< [${t}] ${f.file} ${frame ? 'ok' : 'TIMEOUT'}\n`);
|
||||
|
||||
if (f.closesConnection && reconnect) {
|
||||
conn.close();
|
||||
conn = await reconnect();
|
||||
}
|
||||
|
||||
if (f.kind === 'timeout') {
|
||||
out.push({
|
||||
@@ -313,7 +331,7 @@ async function replay(conn: Conn, fixtures: readonly Fixture[],
|
||||
out.push({ fixture: f.file, transport: t, ok: mismatch === null,
|
||||
detail: mismatch ?? 'result validates and matches the golden shape' });
|
||||
}
|
||||
return out;
|
||||
return { outcomes: out, conn };
|
||||
}
|
||||
|
||||
/** The transport rules are part of the contract, so they get replayed too. */
|
||||
@@ -364,10 +382,18 @@ function loadXfail(path: string): XfailEntry[] {
|
||||
* Reconciles outcomes against the allowlist. A listed fixture that failed is downgraded
|
||||
* to a pass (its detail says why). A listed fixture that *passed* is flipped to a
|
||||
* failure: the entry is stale and must be deleted from the list, not left to rot.
|
||||
*
|
||||
* Never touches a 'static' outcome: those validate the golden fixture against the
|
||||
* generated validators offline and never talk to a server, so a stub handler can't make
|
||||
* one fail in the first place — matching them here would just relabel an
|
||||
* always-true check as "xfail" and then, since it always stays true, immediately flag it
|
||||
* as an unexpected pass. (A fixture also gets *two* static outcomes — params and result —
|
||||
* so without this exclusion a single xfail entry would print that "duplicate" twice.)
|
||||
*/
|
||||
function applyXfail(results: readonly Outcome[], xfail: readonly XfailEntry[]): Outcome[] {
|
||||
const matches = (e: XfailEntry, r: Outcome): boolean =>
|
||||
e.fixture === r.fixture && (e.transport === undefined || e.transport === r.transport);
|
||||
r.transport !== 'static' && e.fixture === r.fixture &&
|
||||
(e.transport === undefined || e.transport === r.transport);
|
||||
|
||||
return results.map((r) => {
|
||||
const entry = xfail.find((e) => matches(e, r));
|
||||
@@ -409,14 +435,18 @@ async function main(): Promise<void> {
|
||||
const udsPath = arg('--uds');
|
||||
const wsPort = arg('--ws-port');
|
||||
|
||||
if (udsPath) {
|
||||
const conn = await connectUds(udsPath);
|
||||
await conn.call('session.hello', { clientType: 'test', clientName: 'conformance', protocolVersion: '1.0.0' });
|
||||
const { bindings, setup } = await setupBindings(conn);
|
||||
results.push(...setup, ...(await replay(conn, fixtures, bindings, includeRequires)));
|
||||
conn.close();
|
||||
// Each opens (and re-opens, via `reconnect`) with the same handshake: session.hello on
|
||||
// the Unix socket, session.pair + session.hello on the WebSocket. Needed because at
|
||||
// least one fixture (session.hello.version-mismatch) documents that the *server* closes
|
||||
// the connection after replying — replay() calls this to get a working connection back
|
||||
// rather than leaving every later fixture on the shared connection to time out.
|
||||
async function freshUds(): Promise<Conn> {
|
||||
const conn = await connectUds(udsPath!);
|
||||
await conn.call('session.hello',
|
||||
{ clientType: 'test', clientName: 'conformance', protocolVersion: '1.0.0' });
|
||||
return conn;
|
||||
}
|
||||
if (wsPort) {
|
||||
async function freshWs(): Promise<Conn> {
|
||||
const conn = await connectWs(Number(wsPort));
|
||||
const paired = await conn.request(
|
||||
'session.pair',
|
||||
@@ -427,10 +457,23 @@ async function main(): Promise<void> {
|
||||
if (token === undefined) throw new Error('pairing failed: no token issued');
|
||||
await conn.request('session.hello',
|
||||
{ clientType: 'test', clientName: 'conformance', protocolVersion: '1.0.0', token }, 5000);
|
||||
return conn;
|
||||
}
|
||||
|
||||
if (udsPath) {
|
||||
const conn = await freshUds();
|
||||
const { bindings, setup } = await setupBindings(conn);
|
||||
results.push(...setup, ...(await replay(conn, fixtures, bindings, includeRequires)));
|
||||
results.push(...(await privilegeChecks(conn)));
|
||||
conn.close();
|
||||
const { outcomes, conn: last } = await replay(conn, fixtures, bindings, includeRequires, freshUds);
|
||||
results.push(...setup, ...outcomes);
|
||||
last.close();
|
||||
}
|
||||
if (wsPort) {
|
||||
const conn = await freshWs();
|
||||
const { bindings, setup } = await setupBindings(conn);
|
||||
const { outcomes, conn: last } = await replay(conn, fixtures, bindings, includeRequires, freshWs);
|
||||
results.push(...setup, ...outcomes);
|
||||
results.push(...(await privilegeChecks(last)));
|
||||
last.close();
|
||||
}
|
||||
if (!udsPath && !wsPort) {
|
||||
process.stdout.write('no --uds or --ws-port given: ran static checks only\n');
|
||||
|
||||
@@ -1,46 +1,26 @@
|
||||
[
|
||||
{ "fixture": "contracts/fixtures/download.probe.json", "reason": "D2: download.probe -> -32603, needs the engine probe path" },
|
||||
{ "fixture": "contracts/fixtures/errors/download.probe.probe-failed.json", "reason": "D2: download.probe -> -32603, needs the engine probe path" },
|
||||
{ "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/download.pause.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/download.resume.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/download.start.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/download.cancel.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/download.remove.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/download.addBatch.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "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/download.provideAuth.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/errors/download.provideAuth.not-found.json", "reason": "D3: stub handler, -32603 instead of -32010" },
|
||||
{ "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/rules.list.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/rules.upsert.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "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/settings.get.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/settings.set.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "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/limiter.get.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/limiter.set.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "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/schedule.get.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/schedule.set.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "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/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.upsert.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/queue.reorder.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/queue.start.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/queue.stop.json", "reason": "D3: stub handler, -32603" },
|
||||
|
||||
{ "fixture": "contracts/fixtures/category.upsert.json", "reason": "D3: stub handler, -32603" },
|
||||
{ "fixture": "contracts/fixtures/category.remove.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": "stub handler, -32603 -- NOT in daemon/docs/deferrals.md; filed to DAEMON to add a D-row" },
|
||||
{ "fixture": "contracts/fixtures/capture.offer.take.json", "reason": "stub handler, -32603 -- NOT in daemon/docs/deferrals.md; filed to DAEMON to add a D-row" },
|
||||
{ "fixture": "contracts/fixtures/errors/capture.offer.ignore.json", "reason": "stub handler, -32603 -- NOT in daemon/docs/deferrals.md; filed to DAEMON to add a D-row" }
|
||||
{ "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" }
|
||||
]
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
Reference in New Issue
Block a user