diff --git a/AGENTS.md b/AGENTS.md index 494dbdaa8f..109cfb070f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,7 @@ This file is split into nested `AGENTS.md` files based on the language/situation - Dig into the root cause and fix it at the source. Never work around a fixable bug with a retry, sleep, or timeout. - Fail loud and early. Error on unsupported or malformed input rather than warn and continue: supported or refused. - Reproduce bugs before fixing them. Land each fix with a regression test that fails without it, when one is easy. +- Unit tests mock time instead of depending on wall-clock timing or sleeps. - Keep the PR focused. No unrelated refactors, formatting churn, or drive-by changes; split when in doubt. - Refactor aggressively for long-term maintainability, but re-evaluate the direction as you learn. - Propose a course change, even suggest abandoning a PR, rather than finish a half-solution. diff --git a/js/common/declarations.test.ts b/js/common/declarations.test.ts new file mode 100644 index 0000000000..79e3b8b328 --- /dev/null +++ b/js/common/declarations.test.ts @@ -0,0 +1,50 @@ +import { expect, test } from "bun:test"; +import { problems } from "./declarations"; + +const root = "/dist"; + +test("an import of a stripped export is reported", () => { + const files = new Map([ + ["/dist/reload.d.ts", "export declare class Reload {}\n"], + ["/dist/index.d.ts", 'import { ReloadDelay } from "./reload.js";\nexport declare const delay: ReloadDelay;\n'], + ]); + expect(problems(root, files)).toEqual([ + "index.d.ts: imports ReloadDelay from ./reload.js, which does not export it", + ]); +}); + +test("an import of a file with no declarations is reported", () => { + const files = new Map([["/dist/index.d.ts", 'import type { Mock } from "./mock.ts";\n']]); + expect(problems(root, files)).toEqual(["index.d.ts: imports ./mock.ts, which emitted no declarations"]); +}); + +test("names re-exported through a star, an alias, a default, or a directory index resolve", () => { + const files = new Map([ + ["/dist/inner.d.ts", "export interface Delay {}\ndeclare const status = 1;\nexport { status as Status };\n"], + ["/dist/outer/index.d.ts", 'export * from "../inner.js";\n'], + ["/dist/element.d.ts", "export default class Element {}\n"], + ["/dist/page.d.ts", 'import { Delay } from "./outer";\nimport { Status } from ".";\n'], + [ + "/dist/index.d.ts", + 'export * from "./outer";\nexport type { default as Element } from "./element.tsx";\nimport { type Delay, Status as S } from "./outer/index.ts";\nimport { Other } from "@moq/other";\nimport icon from "./icon.svg?raw";\n', + ], + ]); + expect(problems(root, files)).toEqual([]); +}); + +test("a default import of a module without a default export is reported", () => { + const files = new Map([ + ["/dist/element.d.ts", "export declare class Element {}\n"], + ["/dist/index.d.ts", 'import type Element from "./element.js";\nexport { Element };\n'], + ]); + expect(problems(root, files)).toEqual(["index.d.ts: imports default from ./element.js, which does not export it"]); +}); + +test("type-only star re-exports, const enums, and let declarations resolve", () => { + const files = new Map([ + ["/dist/types.d.ts", "export declare const enum State { Open }\nexport declare let value: number;\n"], + ["/dist/outer.d.ts", 'export type * from "./types.js";\n'], + ["/dist/index.d.ts", 'import type { State, value } from "./outer.js";\n'], + ]); + expect(problems(root, files)).toEqual([]); +}); diff --git a/js/common/declarations.ts b/js/common/declarations.ts new file mode 100644 index 0000000000..f7e9d21ba5 --- /dev/null +++ b/js/common/declarations.ts @@ -0,0 +1,128 @@ +// Checks a package's emitted `.d.ts` files for imports the target file no longer exports. +// +// `stripInternal` drops an `@internal` export from its own `.d.ts` but leaves the import in any +// file that names the type, so a published consumer sees a module that does not export it. Only +// declaration emit shows this, which is why it runs over `dist/` after the build rather than as a +// test that would have to run the compiler again. + +import { dirname, extname, join, relative, resolve } from "node:path"; +import { parse } from "@babel/parser"; + +type Statement = ReturnType["program"]["body"][number]; +type Declaration = Extract["declaration"]; + +type FileExports = { + names: Set; + stars: string[]; + imports: Array<{ specifier: string; names: string[] }>; +}; + +/** Every import in `files` (path to `.d.ts` source) that its relative target does not export. */ +export function problems(root: string, files: Map): string[] { + const parsed = new Map(); + for (const [file, source] of files) { + parsed.set(file, fileExports(source)); + } + + const found: string[] = []; + for (const [file, info] of parsed) { + for (const { specifier, names } of info.imports) { + const target = dtsPath(parsed, file, specifier); + if (!target) continue; + + if (!parsed.has(target)) { + found.push(`${relative(root, file)}: imports ${specifier}, which emitted no declarations`); + continue; + } + + for (const name of names) { + if (!exported(parsed, target, name)) { + found.push(`${relative(root, file)}: imports ${name} from ${specifier}, which does not export it`); + } + } + } + } + return found; +} + +// The declaration file a relative specifier resolves to, as a file or a directory index. A package +// specifier resolves outside the package, and an asset (`./icon.svg?raw`) is typed by the bundler, +// so neither is checked. +function dtsPath(files: Map, from: string, specifier: string): string | undefined { + if (!specifier.startsWith(".")) return undefined; + const ext = extname(specifier); + if (ext && !/^\.(js|jsx|ts|tsx)$/.test(ext)) return undefined; + const base = resolve(dirname(from), specifier).replace(/\.(js|jsx|ts|tsx)$/, ""); + const index = join(base, "index.d.ts"); + return files.has(index) ? index : `${base}.d.ts`; +} + +function exported(files: Map, file: string, name: string, seen = new Set()): boolean { + if (seen.has(file)) return false; + seen.add(file); + + const info = files.get(file); + if (!info) return false; + if (info.names.has(name)) return true; + + for (const specifier of info.stars) { + const target = dtsPath(files, file, specifier); + if (target && exported(files, target, name, seen)) return true; + } + return false; +} + +function fileExports(source: string): FileExports { + const names = new Set(); + const stars: string[] = []; + const imports: Array<{ specifier: string; names: string[] }> = []; + + const ast = parse(source, { sourceType: "module", plugins: [["typescript", { dts: true }]] }); + for (const statement of ast.program.body) { + switch (statement.type) { + case "ImportDeclaration": + if (statement.specifiers.length === 0) break; + imports.push({ + specifier: statement.source.value, + names: statement.specifiers.flatMap((s) => { + if (s.type === "ImportDefaultSpecifier") return ["default"]; + if (s.type === "ImportSpecifier") return [moduleName(s.imported)]; + return []; + }), + }); + break; + case "ExportNamedDeclaration": { + const imported: string[] = []; + for (const s of statement.specifiers) { + names.add(moduleName(s.exported)); + if (s.type === "ExportSpecifier") imported.push(moduleName(s.local)); + } + if (statement.source) imports.push({ specifier: statement.source.value, names: imported }); + for (const name of declared(statement.declaration)) names.add(name); + break; + } + case "ExportAllDeclaration": + stars.push(statement.source.value); + break; + case "ExportDefaultDeclaration": + names.add("default"); + break; + } + } + + return { names, stars, imports }; +} + +function moduleName(node: { type: "Identifier"; name: string } | { type: "StringLiteral"; value: string }): string { + return node.type === "Identifier" ? node.name : node.value; +} + +// The names an `export declare ...` statement introduces. +function declared(declaration: Declaration | null | undefined): string[] { + if (!declaration) return []; + if (declaration.type === "VariableDeclaration") { + return declaration.declarations.flatMap((d) => (d.id.type === "Identifier" ? [d.id.name] : [])); + } + if (!("id" in declaration) || !declaration.id) return []; + return declaration.id.type === "Identifier" ? [declaration.id.name] : []; +} diff --git a/js/common/package.ts b/js/common/package.ts index 7ad9309b5b..3ada558788 100644 --- a/js/common/package.ts +++ b/js/common/package.ts @@ -6,6 +6,7 @@ import { copyFileSync, existsSync, readFileSync, writeFileSync } from "node:fs"; import { basename, join, resolve } from "node:path"; import { publint } from "publint"; import { formatMessage } from "publint/utils"; +import { problems } from "./declarations.ts"; console.log("✍️ Rewriting package.json..."); const pkg = JSON.parse(readFileSync("package.json", "utf8")); @@ -120,6 +121,18 @@ if (messages.length > 0) { process.exit(1); } +console.log("🔍 Checking declaration imports..."); +const declarations = new Map(); +for (const rel of new Bun.Glob("**/*.d.ts").scanSync("dist")) { + const file = resolve("dist", rel); + declarations.set(file, readFileSync(file, "utf8")); +} +const unresolved = problems(resolve("dist"), declarations); +if (unresolved.length > 0) { + for (const problem of unresolved) console.error(problem); + process.exit(1); +} + console.log("📦 Package built successfully in dist/"); // Optionally emit a jsr.json so the package can also publish to JSR (jsr.io). diff --git a/js/json/src/snapshot/encoder.ts b/js/json/src/snapshot/encoder.ts index 67ce5047b1..82800aa939 100644 --- a/js/json/src/snapshot/encoder.ts +++ b/js/json/src/snapshot/encoder.ts @@ -42,6 +42,14 @@ export interface Config { // `"none"`/unset (the default) writes plaintext JSON frames. A {@link Decoder} reading them // must set the same {@link compression}. compression?: Compression; + + /** + * Bytes a group may hold before it rolls, defaulting to moq-net's per-group cache limit. Lets a + * test reach the limit without megabytes of JSON. + * + * @internal + */ + maxGroupBytes?: number; } /** One encoded frame, and the group boundary it implies. */ @@ -106,6 +114,7 @@ export interface Pending extends Encoded { export class Encoder { #config: Config; #compress: boolean; + #maxGroupBytes: number; // The last encoded value, normalized through JSON so it matches what landed on the wire. The // baseline every delta is diffed against, and `undefined` until the first snapshot. @@ -140,6 +149,7 @@ export class Encoder { constructor(config: Config = {}) { this.#config = config; this.#compress = isDeflate(config.compression); + this.#maxGroupBytes = config.maxGroupBytes ?? Group.MAX_GROUP_CACHE_BYTES; } /** @@ -218,7 +228,7 @@ export class Encoder { // can come out slightly larger than its input, so the plaintext is not an upper bound. // Compressing first advances the window, but `#snapshot` opens a fresh one, so an // over-budget delta costs only the wasted compression. - if (this.#snapshotLen + this.#deltaBytes + payload.length <= Group.MAX_GROUP_CACHE_BYTES) { + if (this.#snapshotLen + this.#deltaBytes + payload.length <= this.#maxGroupBytes) { this.#last = json; this.#deltaBytes += payload.length; this.#groupFrames += 1; diff --git a/js/json/src/snapshot/snapshot.test.ts b/js/json/src/snapshot/snapshot.test.ts index 4200fd9654..4d669e7999 100644 --- a/js/json/src/snapshot/snapshot.test.ts +++ b/js/json/src/snapshot/snapshot.test.ts @@ -1,6 +1,7 @@ import { expect, test } from "bun:test"; import { Group, Error as NetError, StreamCode, Time, Track } from "@moq/net"; import { Consumer } from "./consumer.ts"; +import { Encoder } from "./encoder.ts"; import { Producer } from "./producer.ts"; type Value = Record; @@ -358,38 +359,38 @@ test("a delta that would overflow the snapshot rolls a new one instead", async ( test("a compressed delta is gated on its encoded size, not its plaintext", async () => { // A sync-flushed DEFLATE frame can come out larger than its input, so the plaintext is not an - // upper bound on what lands in the group. A snapshot that fills the cache to within a few bytes - // plus a tiny patch that compresses to more than it measures would otherwise slip through the - // gate and evict frame 0. + // upper bound on what lands in the group. A patch that fits the budget by its plaintext but not + // by its encoded size would otherwise slip through the gate and overflow the group. + const value = { v: "x".repeat(1000) }; + const patched = { ...value, q: "a" }; + const plaintext = JSON.stringify({ q: "a" }).length; + + // Measure the frames with the default budget, which admits the delta. + const probe = new Encoder({ compression: "deflate" }); + const snapshot = probe.update(value); + snapshot?.commit(); + const delta = probe.update(patched); + expect(delta?.keyframe).toBe(false); + expect(delta?.payload.length).toBeGreaterThan(plaintext); + + // A budget with room for the plaintext patch but not the encoded one. + const maxGroupBytes = (snapshot?.payload.length ?? 0) + plaintext; const track = new Track.Producer("test"); - const producer = new Producer({ track, compression: "deflate" }); - - // Highly repetitive, so the compressed snapshot lands just under the cap. - producer.update({ v: "x".repeat(Group.MAX_GROUP_CACHE_BYTES) }); - producer.update({ v: "x".repeat(Group.MAX_GROUP_CACHE_BYTES), q: "a" }); + const producer = new Producer({ track, compression: "deflate", maxGroupBytes }); + producer.update(value); + producer.update(patched); producer.finish(); - // Whatever the split, no group may exceed the cache, and the newest value must be readable. - const subscriber = track.subscribe({ maxAge: REPLAY_LATENCY }).ordered(); - for (;;) { - const group = await subscriber.nextGroup(); - if (!group) break; - let bytes = 0; - for (;;) { - const frame = await group.readFrame(); - if (!frame) break; - bytes += frame.payload.byteLength; - } - expect(bytes).toBeLessThanOrEqual(Group.MAX_GROUP_CACHE_BYTES); - } + // The patch rolled into a fresh snapshot rather than joining the first group. + expect(await structure(track.subscribe({ maxAge: REPLAY_LATENCY }).ordered())).toEqual([1, 1]); const consumer = new Consumer({ track: track.subscribe({ maxAge: REPLAY_LATENCY }), compression: "deflate", }); const values: Value[] = []; - for await (const value of consumer) values.push(value); - expect(values[values.length - 1]).toEqual({ v: "x".repeat(Group.MAX_GROUP_CACHE_BYTES), q: "a" }); + for await (const out of consumer) values.push(out); + expect(values[values.length - 1]).toEqual(patched); }); // A malformed or failed group must reach the caller; only an explicit retention gap is resumable. diff --git a/js/justfile b/js/justfile index bcad24af59..4be426446e 100644 --- a/js/justfile +++ b/js/justfile @@ -84,7 +84,7 @@ test $FILES="": exit 0 fi bun install --frozen-lockfile - bun test common/deps.test.ts common/workers.test.ts + bun test common/declarations.test.ts common/deps.test.ts common/workers.test.ts if tty -s; then bun run --filter='*' --elide-lines=0 test else diff --git a/js/net/src/declarations.test.ts b/js/net/src/declarations.test.ts deleted file mode 100644 index 65636b4610..0000000000 --- a/js/net/src/declarations.test.ts +++ /dev/null @@ -1,136 +0,0 @@ -import { expect, test } from "bun:test"; -import { mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { dirname, join, relative, resolve } from "node:path"; - -const pkg = resolve(import.meta.dir, ".."); -const tsc = join(dirname(Bun.resolveSync("typescript/package.json", pkg)), "lib/tsc.js"); - -// `stripInternal` drops the export from the target `.d.ts` but leaves the import in any file -// that names the type, so a published consumer sees a module that does not export it. -test("emitted declarations import only names the target file exports", () => { - const outDir = mkdtempSync(join(tmpdir(), "moq-net-dts-")); - try { - const result = Bun.spawnSync( - ["bun", tsc, "-p", "tsconfig.build.json", "--emitDeclarationOnly", "--outDir", outDir], - { cwd: pkg, stdout: "pipe", stderr: "pipe" }, - ); - expect(result.exitCode, result.stderr.toString() || result.stdout.toString()).toBe(0); - - const files = dtsFiles(outDir); - expect(files.length).toBeGreaterThan(0); - - const parsed = new Map(); - for (const file of files) { - parsed.set(file, fileExports(readFileSync(file, "utf8"))); - } - - const problems: string[] = []; - for (const [file, info] of parsed) { - for (const { specifier, names } of info.imports) { - const target = dtsPath(file, specifier); - if (!target) continue; - - if (!parsed.has(target)) { - problems.push(`${relative(outDir, file)}: imports ${specifier}, which emitted no declarations`); - continue; - } - - for (const name of names) { - if (!exported(parsed, target, name)) { - problems.push( - `${relative(outDir, file)}: imports ${name} from ${specifier}, which does not export it`, - ); - } - } - } - } - - expect(problems).toEqual([]); - } finally { - rmSync(outDir, { recursive: true, force: true }); - } -}); - -type FileExports = { - names: Set; - stars: string[]; - imports: Array<{ specifier: string; names: string[] }>; -}; - -function dtsFiles(dir: string): string[] { - const found: string[] = []; - for (const entry of readdirSync(dir, { withFileTypes: true })) { - const path = join(dir, entry.name); - if (entry.isDirectory()) found.push(...dtsFiles(path)); - else if (entry.name.endsWith(".d.ts")) found.push(path); - } - return found; -} - -function dtsPath(from: string, specifier: string): string | undefined { - if (!specifier.startsWith(".")) return undefined; - return `${resolve(dirname(from), specifier).replace(/\.(js|ts)$/, "")}.d.ts`; -} - -function exported(files: Map, file: string, name: string, seen = new Set()): boolean { - if (seen.has(file)) return false; - seen.add(file); - - const info = files.get(file); - if (!info) return false; - if (info.names.has(name)) return true; - - for (const specifier of info.stars) { - const target = dtsPath(file, specifier); - if (target && exported(files, target, name, seen)) return true; - } - return false; -} - -function fileExports(source: string): FileExports { - const names = new Set(); - const stars: string[] = []; - const imports: Array<{ specifier: string; names: string[] }> = []; - - const named = /^(import|export)(?:\s+type)?\s*\{([\s\S]*?)\}\s*(?:from\s+["']([^"']+)["'])?/gm; - const star = /^export\s+\*\s+from\s+["']([^"']+)["']/gm; - const asStar = /^export\s+\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+["']([^"']+)["']/gm; - const decl = - /^export\s+(?:declare\s+)?(?:abstract\s+)?(?:type|interface|class|function|const|enum|namespace)\s+([A-Za-z_$][\w$]*)/gm; - - for (const match of source.matchAll(named)) { - const [, kind, list, specifier] = match; - if (specifier) imports.push({ specifier, names: ident(list ?? "", "imported") }); - if (kind === "export") { - for (const name of ident(list ?? "", "exported")) names.add(name); - } - } - - for (const match of source.matchAll(star)) { - if (match[1]) stars.push(match[1]); - } - - for (const match of source.matchAll(asStar)) { - if (match[1]) names.add(match[1]); - } - - for (const match of source.matchAll(decl)) { - if (match[1]) names.add(match[1]); - } - - return { names, stars, imports }; -} - -function ident(list: string, side: "imported" | "exported"): string[] { - return list - .split(",") - .map((part) => part.trim()) - .filter(Boolean) - .map((part) => { - const [left, right] = part.replace(/^type\s+/, "").split(/\s+as\s+/); - const name = side === "imported" ? left : (right ?? left); - return name?.trim() ?? ""; - }) - .filter(Boolean); -} diff --git a/quest/m1/README.md b/quest/m1/README.md index b97087b518..183f86857a 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -48,7 +48,9 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Path patterns](/quest/m1/path-patterns.md) - one matcher for every predicate over broadcast paths: tokens, origins, interest - [Setup token](/quest/m1/setup-token.md) - a moq-transport SETUP `AUTHORIZATION TOKEN` reaches the accepted handshake and the relay's auth request, so a verifier can run on it - [In-band auth](/quest/m1/auth/README.md) - a session tells its peer what it may publish and subscribe to, unions tokens presented in band, and fails loud on an out-of-scope publish -- [Tests under load](/quest/m1/test-flakes.md) - three tests that time out or run out of file descriptors under `just check` are fixed at the cause +- [Shaper virtual time](/quest/m1/shaper-virtual-time.md) - `moq-shaper` tests judge seeded decisions on paused time, not on wall-clock delivery under load +- [Rust compressed gate test](/quest/m1/json-compressed-gate-rs.md) - `rs/moq-json` proves a snapshot delta is gated on its encoded size +- [Shrink the JS overflow test](/quest/m1/json-rolls-snapshot-test.md) - the `js/json` roll-on-overflow test uses a small `maxGroupBytes` budget instead of megabytes of JSON - [Decoded frame ownership](/quest/m1/decoded-frames.md) - retain moq-video Frames across bindings, with native views or CPU conversion as needed - [C++ through moq-ffi](/quest/m1/cpp/README.md) - generated C++ over moq-ffi with futures and expected-style errors, shipped as a tarball, vcpkg, and Conan, and adopted by the OBS plugin - [moq-c](/quest/m1/moq-c.md) - libmoq ships as `moq-c`, beside `moq-cpp`, with its C header and library unchanged diff --git a/quest/m1/json-compressed-gate-rs.md b/quest/m1/json-compressed-gate-rs.md new file mode 100644 index 0000000000..0e84456b81 --- /dev/null +++ b/quest/m1/json-compressed-gate-rs.md @@ -0,0 +1,17 @@ +# [XS] Rust test for the compressed snapshot gate + +## Goal + +`rs/moq-json`'s snapshot encoder has a test proving that a delta is admitted +by its encoded size, not its plaintext: a sync-flushed DEFLATE frame can come +out larger than its input, so a plaintext gate lets a patch overflow the +group and evict the snapshot a late joiner needs. The JS encoder gained this +test in the PR that retired `quest/m1/test-flakes.md`. + +## Plan + +- The gate compares against `moq_net::group::MAX_CACHE_BYTES`, which is too + large to reach cheaply. Mirror the JS approach: a crate-private budget the + test can shrink, so it measures real frames and sets a budget between the + patch's plaintext and encoded sizes. +- Confirm the test fails when the gate measures the plaintext. diff --git a/quest/m1/json-rolls-snapshot-test.md b/quest/m1/json-rolls-snapshot-test.md new file mode 100644 index 0000000000..653d9eba4c --- /dev/null +++ b/quest/m1/json-rolls-snapshot-test.md @@ -0,0 +1,14 @@ +# [XS] Shrink the JS snapshot overflow test + +## Goal + +The `js/json` test "a delta that would overflow the snapshot rolls a new one +instead" stops building two ~19 MiB values (about 1 s idle, several under +load) and exercises the same roll with a few bytes, through the encoder's +internal `maxGroupBytes` budget. + +## Plan + +- Keep one assertion that the default budget is moq-net's group cache limit, + so the shrunk test doesn't hide a drift between the two. +- Confirm the test fails when the gate is removed. diff --git a/quest/m1/shaper-virtual-time.md b/quest/m1/shaper-virtual-time.md new file mode 100644 index 0000000000..3fea8a4fb4 --- /dev/null +++ b/quest/m1/shaper-virtual-time.md @@ -0,0 +1,18 @@ +# [S] moq-shaper tests on virtual time + +## Goal + +The `moq-shaper` unit tests pass under any host load: their verdicts come +from the seeded decisions, not from how fast the kernel delivers datagrams. +`reorder_and_jitter_overtake` and `the_seed_reproduces_the_losses` saw every +datagram lost during a slowed full-workspace run, because `round_trip` stops +at the first 200 ms of silence on a real socket. + +## Plan + +- Drive the tests on paused tokio time (or an injected clock) so delay, + jitter, and the read deadline advance deterministically. +- Watch out: paused time auto-advances while the runtime idles, which can + fire a timer before a real UDP datagram lands. If the real sockets fight + the paused clock, separate the scheduling logic from the socket I/O and test + it directly, keeping one socket smoke test with a generous deadline. diff --git a/quest/m1/test-flakes.md b/quest/m1/test-flakes.md deleted file mode 100644 index 909d6cbc28..0000000000 --- a/quest/m1/test-flakes.md +++ /dev/null @@ -1,22 +0,0 @@ -# [M] Tests hold up under load - -## Goal - -Three tests that pass alone but fail under a full `just check` pass reliably, -fixed at the cause rather than by raising a timeout or adding a retry: - -- `js/json/src/snapshot/snapshot.test.ts:359`, "a compressed delta is gated - on its encoded size", which takes about 4.3 s against a 5 s limit. -- The `js/net/src/declarations.test.ts` test that times out at 5 s. -- `rs/moq-tokio/tests/backend.rs:739` `noq_cert_reload`, which fails with - "Too many open files". - -## Plan - -- Find why each JS test is slow. It should shrink its input or reveal a real - slowdown in the code under test; fix whichever it is. -- For `noq_cert_reload`, find what holds the descriptors: a leak in the test - or code under test, or nextest parallelism against the file limit. Fix a leak - at its source, and otherwise cap the test's concurrency in - `.config/nextest.toml`. -- Prove it by running `just check --all` several times on a loaded machine. diff --git a/rs/moq-tokio/tests/backend.rs b/rs/moq-tokio/tests/backend.rs index 15d850c24b..968f37f730 100644 --- a/rs/moq-tokio/tests/backend.rs +++ b/rs/moq-tokio/tests/backend.rs @@ -338,40 +338,30 @@ async fn reload_test() { server_config.tls.cert = vec![cert.clone()]; server_config.tls.key = vec![key.clone()]; + let server = server_config + .init(moq_tokio::quic::Config::default()) + .expect("failed to init server"); + let server = server.listen().await.expect("failed to listen"); + + // Every process the user runs shares one inotify instance limit, so a loaded host can refuse + // the listener its watcher. Judge by the listener's own watcher: a separate probe would race + // the rest of the host for the same limit. #[cfg(feature = "watch")] - if moq_tokio::watch::Files::new(std::slice::from_ref(&cert)).is_err() { + if tracing_test::internal::logs_with_scope_contain("moq_tokio", "hot reload disabled") { eprintln!("skipping reload_test: host cannot start an inotify watcher"); return; } - let server = server_config - .init(moq_tokio::quic::Config::default()) - .expect("failed to init server"); - let server = server.listen().await.expect("failed to listen"); let certificates = server.certificates(); let before = certificates.fingerprints(); assert_eq!(before.len(), 1); - // The reload task is spawned while the listener is built and registers its - // watcher the first time the runtime polls it, which may be after this point. - // Rotating before then would replace the files with nothing watching them. - tokio::time::sleep(Duration::from_millis(200)).await; - - // Rotate in place, the way cert-manager or a secret mount would. + // Rotate in place, the way cert-manager or a secret mount would. The listener registered its + // watch before returning, so the rotation cannot land before it. let (new_cert, new_key) = write_self_signed(dir.path(), "rotated", "localhost"); std::fs::rename(&new_cert, &cert).expect("rotate cert"); std::fs::rename(&new_key, &key).expect("rotate key"); - tokio::time::sleep(Duration::from_secs(2)).await; - assert!( - tracing_test::internal::logs_with_scope_contain("moq_tokio", "reloading server certificates"), - "no reload log" - ); - assert!( - !tracing_test::internal::logs_with_scope_contain("moq_tokio", "hot reload disabled"), - "watcher failed" - ); - let reloaded = tokio::time::timeout(TIMEOUT, async { loop { let now = certificates.fingerprints(); @@ -384,6 +374,10 @@ async fn reload_test() { .await .expect("certificate reload timed out"); + assert!( + tracing_test::internal::logs_with_scope_contain("moq_tokio", "reloading server certificates"), + "no reload log" + ); assert_eq!(reloaded.len(), 1); drop(server); }