mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
2df0241a4fc1fe79d58684e5825aa43fd2601e41
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Justin Maier
|
2df0241a4f |
feat(test-cache): skip unit test files unchanged since they last passed (#4971)
* feat(test-cache): shadow-record what a content-keyed result cache would skip Phase 1 of a test-result cache: nothing is skipped. A vitest reporter keys each test file on the content of every first-party module it depends on, plus the inputs no import records (lockfile, vitest config, tsconfig, node, platform), and records files that passed in full. A later run whose key matches is one the cache WOULD skip; if that file then fails, the key missed a dependency and the run is logged as a false skip. Dependencies come from vite's server-side ssr module graph, not diagnostic().importDurations: on a fixture, importDurations missed an `await import()` made inside a test body, and the ssr graph caught it cold and warm. The graph also leaves out the subtree behind a vi.mock factory, which never executes, while keeping the mocked module itself. The store lives in the COMMON git dir, shared by every worktree, and keys use repo-relative paths, so one tree's green run covers every tree whose files are identical. Files that read the filesystem, spawn processes, or import a non-literal specifier always run (243 of 1,880, 6.5% of modelled worker time). The queue gains a hot-configurable cache mode (`test config --cache shadow`, TEST_CACHE_MODE in the skill .env). In shadow mode a queued unit run gets the primary checkout's reporter appended, plus `--reporter=default` when the caller named none, so turning it on never strips a run's normal output. Fixture sequence, each step as predicted: cold 0 skipped; unchanged 1/1; runtime-imported dep changed 0; unchanged again 1/1; dep behind a mock changed still 1/1; env-driven failure with an unchanged key flagged as 1 false skip. 89-file yardstick, cold then warm: 89/89 passed both times, warm would skip 85/89 (98% of worker time), 0 false skips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(test-cache): skip unit test files unchanged since they last passed Turns the shadow recorder into a real cache. Before a queued unit run, a custom sequencer re-fingerprints each test file's recorded dependencies and drops the files whose key still matches a recorded pass; vitest runs exactly the list the sequencer returns. Reusing the OLD dependency list is sound: gaining a dependency means editing a file already in the list, which changes the key. A key covers the test file, every first-party module it imports (from vite's module graph, in whichever environment loaded it, so happy-dom files count too), every file or directory it read at runtime (a setup-file fs tracker, directories fingerprinted by their whole subtree), and the lockfile, configs, node, platform, vitest and the cache's own code. Records are shared by every worktree through the common git dir, up to 8 per test file, so two trees on different code both stay fast. Still always run: files that spawn, glob, or import a computed specifier (19 files, 0.7% of modelled worker time). Known blind spot: environment variables are not in the key. A random ~5% of skippable files run anyway. If one fails, the cache predicted a pass it could not deliver; it writes TRIPPED.json and runs everything until a human removes it. Modes off|shadow|on are hot-configurable on the queue (`test config --cache on`); never on in CI, never applied to a named-files run. Measured, 89-file yardstick: cold 294s, warm 24s (85 skipped, 3 re-sampled, 0 false skips). Fixture scenarios: runtime-imported dep edited, fixture file edited, dependency of a happy-dom test edited each re-ran exactly the affected file; an env-driven failure was flagged as a false skip and tripped the cache. Fixes found by those checks: a fully cached run exited 1 ("no test files"); 44 happy-dom files were never cached; a builtin heuristic dropped top-level directories like `src` from the key. Full unit suite, cache off: Test Files 1 failed | 1904 passed | 3 skipped (1908); the one failure is rest-error-envelope-ledger, which fails identically on origin/main CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test-cache): close three false-skip paths found by review Adversarial review of |
||
|
Justin Maier
|
a658c3f6df |
perf(dev-server): let the test queue cap each run's vitest pool (#4965)
* perf(dev-server): let the test queue cap each run's vitest pool The queue serialises full-suite runs at concurrency 1, which makes a run wait behind every other agent's. Measured over 23.4h of the daemon's own history (50 runs, 12 worktrees): median run 549s, median wait 186s, mean wait 405s, worst 2247s. Raising concurrency is the only lever that helps a change whose closure reaches the hot services, but it cannot be raised alone: vitest sizes its pool at `cpus - 1`, so two uncapped runs ask for 62 workers on a 32-core box. VITEST_MAX_WORKERS cannot carry the cap here — the daemon spawns the child with the daemon's own environment, so the caller's copy never arrives and the daemon's is fixed at start. The CLI flag is the only channel that reaches a queued run, and it is forwarded through `pnpm run` into vitest. Verified by pool id rather than by argv alone: 8 files at --max-workers=2 ran on workers [1 2]; the same 8 uncapped ran on [1 2 3 4 5 6 7 8]. Adds a runtime setter beside it so the width can be tuned without a second daemon restart, and each key of `test config` is applied only when sent — a concurrency change must not silently drop the cap. Also replaces the "~75s" figure in the full-suite hook, which was off by 7x against the measured median and was what every agent budgeted against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(dev-server): queue full typechecks in their own lane A full `pnpm run typecheck` is one core and up to an 8 GB heap, and several agents starting one at once pegs the box the same way concurrent suites did. With CIVITAI_TEST_QUEUE set it now goes through the dev-server queue. The queue gains run kinds with separate limits rather than one pool, because the loads differ: a suite saturates every core, tsc is effectively single-threaded. One shared limit would either hold a typecheck behind every queued suite or let two suites run at once. Each lane takes only its own head of the queue, and a run's position is reported within its lane. The scalar concurrency every existing caller passes still sets the unit lane only; reading it as "every lane" would raise the typecheck limit on any machine that had only ever tuned the suite. A typecheck stays direct in CI, with any argument (the scripts gate's `-p tsconfig.scripts.json`), with the tsc test seam in use (otherwise the typecheck tests would queue behind real runs and assert on the daemon's REAL tsc), and with a heap override (a queued run gets the daemon's environment, so the override would be silently dropped). typecheck.mjs reuses test-unit-run.mjs's queue client rather than a copy. Also fixes the worker cap missing a caller's camelCase `--maxWorkers`, which vitest treats as the same flag — the queue would have appended a second, conflicting width after it. Nine revert controls, each red on its own named test, restores verified by hash — including the one nothing else catches: a typecheck posted without its kind is accepted as a unit run and spawns a full suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(hooks): send full-program tsc runs to the queued typecheck script A direct `npx tsc --noEmit` skips the typecheck lane, so agents doing it at once are N single-core 8 GB heaps pegging the box. It is also the wrong check: tsc at node's default heap can abort part-way with zero diagnostics and a log that reads clean. scripts/typecheck.mjs raises the heap and names that crash. The hook now denies a full-program tsc (no -p, or -p at the root tsconfig) and points at `pnpm run typecheck`. Narrow runs pass untouched: a sub-project (`-p tsconfig.scripts.json`, which the scripts gate itself recommends), named files, --build, and informational flags. TYPECHECK_DIRECT=1 opts out for diagnosing tsc itself. Selftest: 68 rows green. Controls: disabling the guard fails all 10 block rows; matching `tsc\b` instead of `tsc(?=\s|$)` fails only "tsc-alias is not tsc". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
314663b1b1 |
fix(dev-server): give a queued test run a FILE, not a pipe — a child that exits without flushing was losing most of its log (#4190)
* fix(dev-server): give a queued test run a FILE, not a pipe — a child that exits without flushing was losing most of its log
Closes two ClickUp tickets that share one capture path but are different
defects, and the head-to-head measurement is what picked the fix.
868ktwgyc — output lost before the daemon ever received it. Node makes a child's
stdout SYNCHRONOUS when it refers to a regular file and ASYNCHRONOUS when it
refers to a pipe, so a child calling `process.exit()` discards whatever is still
queued on a pipe. Measured head-to-head, same child, same process, old
mechanism vs new:
consumer=slow PIPES received= 172 missing=4828 | FILE received=5000 missing=0
consumer=slow PIPES received= 172 missing=4828 | FILE received=5000 missing=0
consumer=slow PIPES received= 172 missing=4828 | FILE received=5000 missing=0
consumer=fast PIPES received=1473 missing=3527 | FILE received=5000 missing=0
consumer=fast PIPES received=5000 missing= 0 | FILE received=5000 missing=0
consumer=fast PIPES received=5000 missing= 0 | FILE received=5000 missing=0
Two separate runs would not have been a comparison; this is the same child
through both arms. Numbering the lines is what makes it readable — a bare count
cannot tell truncation from sampling, and every missing line here is a
contiguous tail, which is what identifies the mechanism.
That measurement also ruled out the two fixes the ticket proposed. The daemon
CANNOT detect the loss by reading: it sees a clean EOF with no signal to compare
against, because the bytes never reached the pipe. And "flush before exit" is
not ours to call — the child is vitest. A file needs no cooperation from the
thing losing the data.
868kubfd9 — the reader tore lines at chunk boundaries. `d.toString().split('\n')`
per chunk with no carry buffer split any line straddling a boundary in two and
recorded BOTH halves as lines: 5,000 in, 4,998 recognised, 6 fragments invented,
353,893 of 353,893 bytes. Complete delivery, corrupted log, and `logsDropped`
correctly reports 0 for it — which is what made it invisible behind a counter
readers had been trained to trust. The new reader carries the partial line.
Third thing, not in either ticket and the one that protects a reader most: the
final drain now runs BEFORE onExit. A waiter wakes on the terminal status and
then reads the log, so reporting the exit while lines are still unread produces
a complete-LOOKING log that is still filling — the same false green, reached a
different way. The old pipe path could do that, since 'exit' does not wait for
pending 'data'.
Trade-off, stated rather than buried: one file for both streams, so the
interleaving is the real one, at the cost of no longer distinguishing stdout
from stderr. Lines are recorded as `output` rather than claiming to be one or
the other — nothing renders the level for a test run (both consumers print
`entry.message`), and a label we cannot support is worse than an honest one.
Seven mutants, each alone, tree cmp-verified between:
carry buffer removed -> the straddle case (+2 more)
final flush removed -> the trailing-line case
back to pipes -> both seam cases
onExit before the drain -> the ordering case
drain reads a single chunk -> the 5,000-line case + straddle
stderr to another fd -> the shared-descriptor case
capture file not deleted -> the cleanup case
All killed. The ordering mutant is killed ONLY by the ordering test, which is
why it exists.
344 passed (15 files). typecheck 0 errors. New test file prettier-clean.
* fix(dev-server): the capture had no owner — a forced or shut-down run leaked two fds, an unbounded /tmp file and a live interval
Audit round on #4190. Both blockers share one root cause: `finish` was bound only
to the child's own 'exit', and there are two live paths where that event never
comes.
🔴 The sweep's force-release — the wedge this queue exists to prevent — set
`run.handle = null` and dropped the queue's last reference while the interval,
both descriptors and the capture file stayed alive inside the closure. Measured
on a forced run: 2 fds still open and the log still growing 2s after the run was
reported terminal (logIndex 75 -> 471), file at 102,465 bytes, never unlinked.
A reader fetching logs after the verdict would get lines emitted AFTER it, with
`logsDropped` reading 0. `shutdown()` had the same gap.
🔴 And it was already leaking on every CI run, from a test this PR did not
update: `dev-server-test-queue-spawn.test.ts` calls `defaultStartRun` twice and
never emits 'exit'. Not hypothetical — 15 stale `civitai-test-run-*.log` files
were sitting in this box's /tmp from my own runs, 420 KB. Verified fixed by
counting: 0 before, 0 after, where it was 2 per run.
Both closed by `dispose()` on the handle, called from the force-release and from
`shutdown()`. Idempotent, and deliberately does NOT call onExit — the caller has
already settled the run.
Also from the audit:
- A carry buffer fixes a torn LINE and does nothing for a torn CHARACTER.
Decoding each read independently turned a 3-byte `⎯` starting at byte 65535
into THREE U+FFFD with the original gone — and vitest builds its failure output
from `⎯`/`✓`/`×`/`❯`, one boundary per 64 KiB. `StringDecoder` now spans reads.
The existing straddle test is ASCII and structurally could not see this.
- The final drain no longer stops at the first short read. Truncating there would
report `logsDropped: 0` over a clipped log — the exact thing
`warnIfLogsDropped` exists to make impossible.
- The straddle test's positive control is pinned from both sides; `> WINDOW`
alone is also satisfied by padding that already exceeded it, which would put
the line wholly inside the second read and never exercise the hazard.
- The `output` level is now pinned, the read buffer is hoisted out of a 10x/s
allocation, and the spawn-failure paths return a handle with the same shape.
Seven mutants, each alone, tree cmp-verified between:
force-release stops disposing -> the queue seam case
shutdown stops disposing -> the queue seam case
per-read decode -> the multi-byte case
lines relabelled 'stdout' -> the level case
dispose also calls onExit -> the dispose-contract case
dispose not idempotent -> the dispose-contract case
final drain stops on short read-> SURVIVED, and it is honest that it does
That survivor is stated rather than papered over: on a regular file a short read
only happens at EOF, so no test on this filesystem can distinguish the two. It
is contract hardening, not covered behaviour.
🔴 The seam lesson again: the first version of the dispose test called
`handle.dispose()` directly, which proves the method EXISTS and not that the
queue ever calls it — a `dispose()` nobody invokes is exactly the shape this
missed the first time. The killing test now drives TestQueue and asserts on
both release paths.
MEASURED, not fixed here: a fully PASSING unit run emits 5,390 lines against
`MAX_LOG_LINES = 2000`, so now that the capture actually delivers everything the
"INCOMPLETE" warning will fire on every full-suite run — and a warning that
always fires is one people stop reading. Raising the cap has memory arithmetic
attached (`KEEP_FINISHED = 50` retained runs x the window), so it is a tuning
decision that does not belong inside a correctness fix.
349 passed. typecheck 0 errors. 0 leaked captures.
* fix(dev-server): shutdown disposed the handle and settled nothing — the fix for the leak wedged the queue
Delta re-audit round on #4190. The previous round's `dispose()` closed both
leaks and introduced one genuine regression, which is fixed here.
🔴-in-effect: `dispose()` and `finish()` share the `finished` flag, so disposing
DISABLES the child's exit callback. The sweep's force-release settles the run
itself right after disposing; `shutdown()` did not. The SIGKILLed child's 'exit'
then arrived and returned early, `onExit` never fired, the run stayed `running`
and `running.size` never dropped — so `pump()` could never start another run.
That is the wedge this queue exists to prevent, reintroduced by the fix for a
leak. Measured in both arms: 300 ms after shutdown(), `running` / size 1 at
HEAD, against `cancelled` / 0 with the dispose removed. `shutdown()` now
settles what it disposes.
Survivable today only because every caller exits the process straight after —
but each first awaits rgbProxy.stop(), authHub.stop() and stopSpokeApps(), and
if any of those hangs the daemon stays up serving a queue that can never run
anything again.
🔴 And the comment I wrote to justify that dispose was FALSE. It claimed the
capture file would otherwise "survive the daemon that created it, every time".
Measured in both arms: the file was unlinked either way, because the daemon does
linger long enough for the child's exit. The dispose earns its place by
releasing the interval and the descriptors deterministically — not by that. The
comment now says so, including that it was wrong.
Also from the re-audit, all on lines the previous round added:
- The multi-byte test was VACUOUS without a straddle — it passed with the
StringDecoder removed entirely, because "no U+FFFD" is equally satisfied by a
character that never crossed a boundary. Now pinned from both sides, the same
control its sibling got last round.
- `decoder.end()` had no test. A child killed mid-sequence leaves an incomplete
character; without the flush those bytes vanish silently instead of surfacing
as U+FFFD.
- `dispose()` draining before releasing had no test, on the one path where the
child is KNOWN to still be writing — `drain(true)` after `close()` emits
nothing at all.
- A read that THROWS on the final drain was silent. Same argument as the
short-read break beside it: it would report a clipped log with
`logsDropped: 0`. It now emits `[capture truncated: <code>]`.
- The `!final` comment overstated its scope: on the `finish()` path the writer
is already dead, so it changes nothing there. It has effect only on `dispose()`.
Battery, each mutant alone, tree cmp-verified:
shutdown disposes but never settles -> the new queue-level case
dispose closes before draining -> the drain-before-release case
decoder.end() removed -> the incomplete-character case
final-drain read error silent -> the truncation-marker case
dispose leaves the interval running -> SURVIVED
That survivor is stated, not papered over: once the descriptors are closed the
stray interval's read just throws and breaks, so it has no observable effect on
the log. The ordering is still correct and still load-bearing — fd numbers are
reused, so a surviving reader could splice another file's bytes into this run —
but nothing pins it without exposing internals.
🔴 The harness lied to me first. Four mutants reported SURVIVED with an EMPTY
test count, because the runner list was an unquoted `$T` and zsh does not
word-split — vitest matched zero files and printed project globs. That is the
exact trap I wrote into the auditors' own briefing. The battery now refuses to
report a verdict when no `Tests` line is present, and the baseline control
proves one appears.
353 passed. typecheck 0 errors. 0 leaked captures.
* fix(dev-server): freeing the SLOT must not sit downstream of IO that can throw — and I was wrong that the stray interval was harmless
Round-3 delta re-audit. It refuted two of my own claims with measurements; both
are corrected here rather than argued with.
🔴-in-effect: `dispose()` does real IO and calls back into `onLog`, and it sat
ABOVE the detach/release/settle that free the slot — in both `shutdown()` and
the sweep's force-release. A throw there skips all three and the queue is wedged:
the identical failure the settle was added to fix, one round earlier. Measured
with a throwing dispose: status `running`, running.size 1, against `cancelled`
/ 0 when it returns normally. It also aborts the loop, leaving every remaining
run unkilled, and because both daemon handlers are async the rejection means
`process.exit(0)` is never reached — SIGINT would appear to do nothing.
Worse, this round ADDED a throw source inside that block: `err.code` raises on a
null/undefined throw. The repo already documents this hazard one file over
(`scripts/test-unit-run.mjs` uses `err?.message ?? String(err)` with a comment
saying why). Now `err?.code ?? err?.message ?? String(err)`, and both dispose
call sites are guarded — releasing the slot matters more than releasing the fds.
🔴 RETRACTION. I reported the `clearInterval` mutant as surviving with "no
observable effect once the descriptors are closed". That is measurably false,
and my own test comment said so while I argued the opposite. Descriptor NUMBERS
are reused: once the next `openSync` claims the freed fd, a surviving interval
reads whatever file now owns it. Measured: 4,246 lines of an unrelated file
spliced into a terminal run's log, against 0 with the clearInterval in place.
It is now pinned rather than argued away.
That test needed a second try to be able to fail at all. `openSync` returns the
LOWEST free descriptor, and the tail reads from `readFd` — the HIGHER of the
pair the capture just closed. Claiming one descriptor takes `writeFd`'s number
and the mutant goes unobserved, which is exactly how the first version passed.
It now claims both.
🔴 SECOND RETRACTION. Last round's message said the multi-byte test was
"vacuous without a straddle" and passed with the StringDecoder removed. The
re-audit re-ran that mutant against the round-2 file: KILLED. The padding was
already `READ_WINDOW - 1`, so the straddle always existed — the "survived" came
from my own broken harness, the one that reported four false SURVIVED verdicts
with an empty test count. The added assertions were still worth having, but the
justification I gave for them was wrong.
The residual behind it was real and is fixed: the test held a private copy of
`64 * 1024`. Widen the module's buffer and both boundary controls go vacuous
while staying green — proven: at 128 KiB the whole suite passed with the
StringDecoder deleted outright. The window is now exported and imported, so the
control cannot drift from what it controls.
Also: the test named 'drains before releasing, and stops the tail before closing
the descriptors' asserted only the first half. Renamed to what it checks, and
the second half is now its own test.
Battery, each mutant alone, tree cmp-verified, every verdict gated on a `Tests`
line being present:
dispose leaves the tail running -> the fd-reuse case
shutdown's dispose unguarded -> frees-the-slot-on-shutdown
sweep force-release dispose unguarded -> frees-the-slot-on-force-release
read window widened + decoder removed -> the multi-byte case
356 passed. typecheck 0 errors. 0 leaked captures, 0 leaked probes.
* test(dev-server): assert the exact terminal status per release path, not either-of
`toContain(['cancelled','timeout'])` passes with the WRONG status for the path
under test — a shutdown reported as a timeout, or the reverse. That is the shape
of assertion that lets a real mix-up through, and this file now has two paths
that settle differently.
Controlled: making shutdown settle 'timeout' now fails two tests; before this it
failed neither.
* fix(dev-server): a throwing log consumer leaked both descriptors and the file, and the healthy path's tail guard was never pinned
Round-4 delta re-audit. It reported no 🔴 and confirmed the previous round's
fixes hold — each killed by a mutant dying for its own reason, which is the
first round that has been true. These are its two 🟡 follow-ups, folded in
rather than deferred.
Draining calls back into `onLog`. With the close outside a `finally`, a consumer
that throws left BOTH descriptors open and the capture file on disk — measured:
fds 20 before and 20 after, file still present, 1 of 3 lines lost. Per run, in a
daemon that runs for days. And the slot-freeing guard added last round would
then swallow it, so it was silent as well as leaky. Both `finish()` and
`dispose()` now close in a `finally`.
That guard is no longer silent either. Swallowing the error hid a clipped log
behind `logsDropped: 0`, which is the one outcome the module's own comment says
the log contract rules out — and which the `[capture truncated]` marker two
lines below exists to prevent. Both sites now `addLog(run, 'error', …)`.
🔴 The tail guard covered the RARER path. `finish()` and `dispose()` have the
identical clearInterval -> drain -> close shape, and last round pinned only
`dispose()`. `finish()` runs on every normal child exit, so the stray-interval
hazard the round proved is not harmless was pinned on the edge case and left
open on the common one. Removing `finish()`'s clearInterval survived the whole
suite; it now fails.
Also: the fd-reuse tests had no self-check that the reclamation they depend on
actually happened. If `openSync` ever stops handing back the released numbers,
`readSync` gets EBADF, `drain()` breaks silently, and the test goes on passing
while protecting nothing. Both now assert the descriptor was reclaimed.
And a sentence on why `kill(true)` beside the guarded dispose is NOT guarded —
`defaultStartRun`'s kill body is wholly inside its own try/catch and the daemon
always uses that runner — so the asymmetry does not have to be re-derived.
Battery, each alone, every verdict gated on a `Tests` line:
finish() loses clearInterval -> the healthy-path tail case (was surviving)
dispose closes outside finally -> the throwing-consumer case
finish closes outside finally -> the throwing-consumer case
CORRECTION to the previous message: I described `err?.code ?? err?.message ??
String(err)` as a fix. It is an EQUIVALENT MUTANT — `readSync` only ever throws
a Node SystemError, so reverting it is undetectable on any reachable input. It
costs nothing and I have kept it, but it is defensive hardening, not covered
behaviour, and I should not have implied otherwise.
358 passed. typecheck 0 errors. 0 leaked captures, 0 leaked probes.
* fix(dev-server): the test asserted deletion of files it did not own, and the round's headline change was untested
Round-5 delta re-audit. No 🔴 and no regression introduced — the first time that
has been true twice running. These are its three follow-ups.
🔴-in-practice: the throwing-consumer test snapshotted /tmp by PREFIX and
asserted every match was deleted. That matches captures owned by the operator's
own daemon, which creates one whenever it runs a queued test — so the suite goes
red with no defect present. Not theoretical: during the audit a single foreign
file made the UNMUTATED test fail, and made three mutants report a false KILLED.
The filename carries the owning pid for exactly this reason; both snapshots are
now scoped to `civitai-test-run-${process.pid}-`. Controlled: with a foreign
file planted, 359 pass and the file is left untouched.
The headline change of the previous round was untested. `finish()` and
`dispose()` have the identical drain-then-close shape and the throwing-consumer
test drove only `dispose()` — reverting `finish()`'s try/finally survived the
whole suite. `finish()` is the path every healthy run takes. It now has its own
case driving `child.emit('exit', 0)`, and the revert fails.
Nothing asserted the `capture release failed` line either, so both catch sites
could go back to a silent swallow with the suite green — which would restore
exactly the hidden-clipped-log outcome the line was added to prevent. Asserted
now; both silent-swallow mutants fail.
🔴 CORRECTION. The previous message claimed I had added a comment explaining why
`kill(true)` is unguarded while its neighbour is. That comment was NOT in the
tree — my edit's anchor did not match and I did not verify that one landed. The
claim itself was true and the auditor re-derived it independently, but I stated
as done something I had not checked. It is there now. This is the second false
claim I have put in a commit message on this branch; the pattern in both cases
was asserting an edit without grepping for it afterwards.
Battery — the three that survived the last round:
finish() reverts try/finally -> the normal-exit release case
sweep catch goes silent -> the sweep force-release case
shutdown catch goes silent -> the shutdown case
All killed. Every verdict gated on a `Tests` line.
359 passed. typecheck 0 errors. Clean /tmp before and after the shipped suite:
0 captures, 0 probes. (The one file that appeared mid-battery was the M3 MUTANT
leaking by design — content `boom one/boom two/x`, the `x` proving the
descriptor stayed open, which is what the kill detects.)
* test(dev-server): a positive control on the pid-scoped /tmp filter, so an empty match cannot pass as a no-op
Scoping the snapshot to this process fixed the cross-process false failure, and
introduced the opposite hazard: `for (const f of mine) expect(...)` over an
EMPTY list is a loop that runs zero assertions and passes. If the capture
filename format ever changes, both deletion tests would go on passing while
protecting nothing — the same vacuous-green shape this branch has now hit in
three different places.
Measured at exactly 1 match, and pinned there. Controlled: breaking the scope so
it matches nothing fails both tests, where before it would have been silent.
* test(dev-server): own captures by DELTA, so a sibling's defect stops failing this test on bookkeeping
Round-6 delta re-audit: no 🔴, no production defect, no regression — the range's
only non-test change was three comment lines. This is its one 🟡 plus two
accuracy fixes.
The capture snapshot claimed every live capture for this pid, and these cases
share a fork. So a defect in the SIBLING test's subject left its file behind and
this test failed on the file count rather than on its own assertion. The mutant
still died — at the wrong line, which silently over-credits whatever this test
was meant to cover. That is the same wrong-reason-kill problem this branch has
been chasing since round one, arriving in the fixture instead of the guard.
Now a before/after delta, so ownership is exact. Controlled: with dispose()'s
try/finally removed, ONE test now fails — the dispose one, at
`expected [Function] to throw an error`, its own assertion. Previously BOTH
failed and the finish() case reported
`expected [ …(2) ] to have a length of 1 but got 2`.
Two comments corrected:
- "it is the only `new TestQueue`" was false repo-wide — three more exist in the
test file, two passing a startRun. The load-bearing claim (production never
injects a runner) is true; the sentence was not, and the next reader greps it.
- The throwing-consumer leak is NOT reachable through the daemon today: its
`onLog` is `addLog`, which cannot throw. The round-5 comment described it as
happening "per run, in a long-lived daemon". It is cheap insurance against a
future consumer, and now says so.
359 passed. typecheck 0 errors. 0 leaked captures.
|
||
|
Justin Maier
|
e056028dbe |
fix(dev-server): stop a queued test run from enqueueing itself
The daemon runs `pnpm run test:unit:run`, which is the script that routes to the queue, and passed its own environment through. The child saw CIVITAI_TEST_QUEUE still set, enqueued a second run and waited for it while the first held the slot that run needed: a deadlock on every full-suite run. One wedged run held the only slot for 20 minutes with another agent's run queued behind it. Raising concurrency does not fix it — each logical run would then occupy two slots, so agents starting together refill them with waiters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |