mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
a7b09fbefea27ea696c8eec0a6ff622ed4d2b787
201 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 briant
|
c681b72c9d |
chore(clickhouse): drop the one-shot user_activity_rollup apply script
The backfill it existed for ran on 2026-09-04 and the table is now kept current
by the user-activity-rollup cron, so this is a tool with no remaining caller.
Nothing is lost by deleting it. The statements themselves live in the tracked
migration (src/server/clickhouse/migrations/2026-09-04-user-activity-rollup.sql)
along with the measured actuals from the run, and the script is recoverable from
|
||
|
briant
|
a78e58362e |
chore(clickhouse): apply script for user_activity_rollup, and the real backfill numbers
The migration is 30 statements against a cloud console that drops connections, which is how half a backfill happens without anyone noticing. The script runs them in order with backoff retries on transport errors only — a syntax error fails immediately rather than burning five attempts and burying the message — and prints a resume command naming the partition it died on. Dry run by default; --apply writes. Applied to prod 2026-09-04, 246s, no retries needed. Replaced the predicted "~7.4M distinct users" with the actual 10,719,260 in both the migration and the script's threshold: the estimate only counted pageViews and hand-waved the rest, and the other three sources turned out to add 3.36M accounts that have loaded no page since 2024-09-26. The split is also the arm-by-arm correctness check, so it is written down: 10,719,260 total less 3,361,661 with no country leaves 7,357,599, against the 7,356,496 distinct users pageViews held a few hours earlier — and only pageViews can set a country. Verified end to end after applying: the page's own query returns in 0.56s for a real 15,000-follower set, and 86.2% of those followers have a country, matching the figure measured independently against pageViews before the table existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
8085d81651 |
chore(articles): remove the official-articles backfill script
It was a one-off and it has run. Justin's call: not worth keeping in the repo, and not worth a PR to remove. What it did, recorded here because the code is going and the fact should not go with it. Prod, 2026-09-04: --user-ids 12042163,1,3,43555,5418 --exclude-ids 6222,6339,6454,6338,5344,28893 --apply 214 articles marked — Maxfield 65, JustMaier 58, CivitaiOfficial 51, Faeia 38, theally 2. Read back afterwards rather than trusting the update count: 214 total, 0 marked outside those five accounts, all six excluded ids confirmed still false. Excluded five abandoned drafts titled "test"/"rtert", and 28893 "Farewell, Civitans!" — a departing staff member's goodbye rather than Civitai speaking. CivBot's 438 automated posts were deliberately left out. Anything marked from here on is marked by a moderator: the ⋯ menu on an article, the toggle in the editor, or `article.mjs --official`. To reverse the backfill, take the file back out of history: git show <this commit>^:scripts/backfill-official-articles.mjs > backfill.mjs node backfill.mjs --user-ids 12042163,1,3,43555,5418 --unmark --apply Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHZuQDTCG159qcPrCkP7SF |
||
|
Zachary Lowden
|
4ede37c9c5 |
ci: re-pin BASE_TESTS to 37,857 — the unit shard gate is red on main (#4621)
* ci: re-pin BASE_TESTS to 37,857 — the unit shard gate is red on main `scripts/ci/assert-shard-ran.mjs` pins `BASE_TESTS = 22157`, measured 2026-08-25. The suite has since grown to 37,857 executed across 1,626 files, so shards 3 and 4 exceed the derived ceiling of 9,971 and the job fails with every test passing. This is red on `main`, not just on PRs: runs 33900980756, 33901008291 and 33901251166 all fail the same two shards on unrelated commits. A permanently-red gate trains people to click through, so it is re-pinned rather than worked around. Re-measured from run 33900980756: 7177 + 5769 + 14724 + 10187 = 37,857. Cause (2) confirmed against cause (1) by the script's own discriminator: the four counts are unequal proportionate shares (2.55x spread) and each shard ran ~4 min against a ~10.4 min full suite. `--shard` failing to reach vitest would instead give four near-identical full-suite counts at full-suite runtime. Also corrects two claims in the comment that are no longer true and that are the stated justification for the 0.3 / 1.8 multipliers: - imbalance was "~5% off the mean"; the heaviest shard is now +55% - ceiling headroom was "~1.7x"; it is now 1.16x vitest shards by file, so a few test-dense files skew a shard badly and the next one trips the ceiling well before suite growth would. Recorded in the comment: if this reds again on a green suite, balance the shards rather than raising the number again. Verified: the new band [2839, 17036] admits all four current shards, and still rejects a collapsed shard (0, 99) and a shard that ran the whole suite (37857). * ci: make the shard-gate tests follow BASE_TESTS instead of restating it CI caught what the previous commit missed: re-pinning BASE_TESTS turned `Unit tests (3)` and `(4)` green but broke `Unit tests (1)`, because scripts/__tests__/assert-shard-ran.test.ts carried a SECOND copy of the constant — `const BASE_TESTS = 22157`, under a comment saying it was "kept in step with BASE_TESTS in the script". Kept in step by convention, which is what failed. Three boundary tests asserted a stale band while the script itself was correct. The test now reads the constant out of the script source. Parsing rather than importing is deliberate: the script is a CLI that reads process.argv and calls process.exit at module scope, so an import would execute it. The regex throws a named error if the constant is renamed, so that fails loudly instead of silently falling back to a default. Also fixes the third failing test, which read `const count = 3000` under a comment claiming it was "computed rather than hardcoded so it follows BASE_TESTS". It did not follow it. When BASE_TESTS moved the interesting region slid from 1662..3324 to 4259..5679 and 3000 fell out of it. It is now derived from the two bounds, with an assertion that the region is non-empty so a multiplier change fails loudly rather than silently picking a midpoint that satisfies neither bound. Validated rather than assumed: - constant moved to 50000 -> 14/14 still pass (the tests follow it) - constant renamed -> throws the named error, 1 file failed - restored -> byte-clean, 14/14 pass * ci: retrigger preview checks (empty) |
||
|
Justin Maier
|
ac9117af6a |
feat(articles): official toggle in the editor, and an official filter on the feed (#4630)
* feat(articles): official toggle in the editor, and an official filter on the feed Follows #4624, which added the column, the moderator-only mutation and the badge. Justin asked for three more things: the mark reachable from the article editor rather than only the ⋯ menu, the agent CLI able to set it, and a way to browse official articles. The CLI half is in the civitai-user-skill repo; this is the app. 🔴 A non-moderator's `isOfficial` is DROPPED, not refused. That distinction is the whole design and it is not defensive coding: the edit form seeds itself from the article, so an owner editing an article a moderator had marked would send the flag straight back. Refusing there locks the author out of their own article with a bare UNAUTHORIZED and nothing explaining it — which is exactly the defect the review found in the tag version of this feature (#4618). Dropping it means their save succeeds and changes nothing about the mark, because `upsertArticle` leaves the column alone when the field is undefined. Both halves are tested, and the "still saves" half is the one a future tidy-up will break. The editor toggle sits in the moderator-only panel beside Locked properties, and the form omits the key entirely for everyone else rather than sending a value the server would discard. The feed filter is a `Civitai Official` chip in the filters dropdown, plus `?isOfficial=true` so an official feed is a link you can send. **Only `true` filters.** An explicit `false` is treated as absent, deliberately: nobody browses FOR community articles, and a `false` that filtered would let a stale url quietly hide every official article from someone's feed. That is asserted on the emitted SQL rather than on the arguments — a test that only checked the service was CALLED with the flag would pass for a service that ignored it, which is what "the filter does nothing" looks like to a user. Controls, every mutation red, pristine 8 passed (5 + 3): non-mod value passed through instead of dropped → 2 failed refuse instead of drop → 3 failed drop becomes `false` instead of undefined → 2 failed WHERE clause removed → 1 failed WHERE clause applied unconditionally → 2 failed `false` filters too → 1 failed ⚠️ One of those tests was written wrong first and is worth the warning it now carries: the SQL helper read only the template strings, but `getArticles` interpolates its WHERE clause as a VALUE, so it captured SQL that could never contain the filter — and both negative assertions passed against it. Every assertion in that file now carries a `FROM "Article" a` control so an empty capture fails loudly instead of quietly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHZuQDTCG159qcPrCkP7SF * feat(articles): backfill script for the official mark `isOfficial` starts false on every existing article, so the badge is invisible until the back catalogue is marked. The rule is `constants.system.officialUserId` (12042163) — the same id `resource-select.service.ts` already uses to mean "official" — with `--user-id` to point it elsewhere. 🔴 Dry run by default. It writes nothing without `--apply`, and `--unmark` reverses it. The column is a public provenance claim, so a backfill that marks the wrong author is a false claim on somebody else's writing. Two things it prints that nobody asks for and everybody wants afterwards: the database it is actually connected to (`.env` here has been swapped between dev and prod before), and the count of articles marked by SOMEONE ELSE that this rule would not have set — so a hand-marked article cannot be silently overwritten by the rule without you seeing it first. The apply path re-reads the count afterwards rather than trusting `rowCount`, and exits non-zero if the two disagree. Exercised against dev: dry run listed 49, `--apply` updated 49, read-back confirmed 51 (2 were marked by hand first). NOT run against prod. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHZuQDTCG159qcPrCkP7SF --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
938d281e4c |
fix(tests): make five suites pass on Windows
All eleven failures were path and spawn portability bugs in the tests, not defects in the code under test, so CI never saw them. Every fix is a no-op on Linux, so no guard is weakened. - credential-detection-superset.guard, appModeratorMessageForm.callSites: path.resolve() yields '\' on Windows while the ledgers are written with '/', so both compared two spellings of the same module. The credential guard was reporting bearer-token.ts as neither scanned nor declared. - blocks/tools/registry: new URL().pathname is '/C:/...', which readFileSync resolved to 'C:\C:\...'. Pass the URL itself. - assert-component-suite-ran: the gate names the missing file by its on-disk path, so the separator is the platform's; the assertion hard-coded a posix one. - test-perf/trace-flush: node_modules/.bin/vitest is an extensionless shell script that spawnSync cannot execute, so the child produced no output and every assertion failed as an empty stdout rather than as anything about the tracer. Spawn node with vitest.mjs, as the other two spawn sites in that file already do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Zachary Lowden
|
6de64ee5a8 |
fix(tests): unzero the preview / component-tests tier, and make a zero-collected run say so (#4531)
* fix(tests): unzero the component tier — one mock factory was aborting the whole run `preview / component-tests` was reporting `failure` on `main` while executing ZERO tests. Reproduced deterministically (3/3 at |
||
|
Zachary Lowden
|
2ba7ecf186 |
refactor(apps): remove the orphaned 5-star AppBlockReview system (#4501)
* refactor(apps): remove the orphaned 5-star AppBlockReview system
Thumbs (AppListingReview) supersedes it. The 5-star write form had no
reachable entry point: its only two hosts were the /apps/[appBlockId]
detail route (retired, now redirects to the store detail) and
AppDetailsModal, which is opened only from AppBlockCard, which renders
only inside MarketplaceBody / RecentlyOpenedAppsView -- and MarketplaceBody
has had no importer in app code since /apps swapped to
AppListingsMarketplaceBody. The page's own header comment already recorded
this and flagged deciding the form's fate as the follow-up; this is it.
Removed:
- appBlockReview.service.ts and its two suites
- the blue-buzz appBlockReview reward + its registration and suite
- blocks.upsertReview / listReviews / getMyReview / setReviewExcluded
(setReviewExcluded had zero call sites at all) + their zod schemas
- AppBlockReviews.tsx and its browser suite
- the Bayesian machinery that was exclusive to it in
block-registry.service.ts: the AVG/COUNT/SUM correlated subqueries,
bayesianRatingSortKey, getGlobalMeanRating and the
app-rating:global-mean cache tag
- avgRating / reviewCount from AvailableBlock and PublicAppDetail, and
the card's rating chip
blocks.listAvailable no longer offers a "rating" sort and defaults to
"popular". The keyset cursor drops its pinned-mean third field; the decoder
still tolerates a stale 3-field cursor so an in-flight page resumes rather
than 500ing.
MARKETPLACE SORT: the live store is unaffected. /apps renders
AppListingsMarketplaceBody, which reads appListings.listAvailable and
defaults to sort "top-rated" -- a Bayesian shrinkage over
AppListingMetric.thumbsUpCount/thumbsDownCount in app-listing.service.ts,
with its own LISTING_BAYES_PRIOR and its own
app-listing:recommend-global-mean cache tag. It never touched
app_block_reviews. Only blocks.listAvailable (the retired grid) read the
star table.
DATABASE: the DROP TABLE migration is committed for history and is NOT
applied anywhere. This project does not run prisma migrate deploy; a human
applies it per environment, after this ships.
Kept deliberately: MarketplaceBody / AppBlockCard / AppDetailsModal (the
documented one-line rollback path for /apps, minus the star bits) and
listAppInsiderUserIds, which now has no production caller but carries a
documented displayed-vs-capability asymmetry that is under test.
* refactor(apps): address audit round 1 on the AppBlockReview removal
Four audit findings, none deploy-blocking.
F1 — the tolerant 3-field cursor decode was documented as a live back-compat
path ("an in-flight page resumes instead of 500ing"), and its guard did not
actually test that. Both halves are corrected:
- The comment now says what is true. A 3-field cursor was only ever minted by
the `rating` sort, and a client resuming that view sends `sort: 'rating'`
with it — a value `marketplaceSortSchema` no longer accepts, so zod rejects
at the router before the decoder runs. The tolerant split is defence in
depth over a door the sort removal already closed, not the thing keeping
that page alive.
- The guard is now real. It asserted `items.length` off a mocked return plus
a SQL-shape regex, but `capturedSql()` exposes only the assembled string
with `?` placeholders — the bound values are exactly what it cannot see, so
nothing could observe the decoded `cursorId`. Added `capturedValues()` and
asserted the parsed resume tuple directly.
Mutation-checked: rewriting the decoder to `decoded.slice(sep1 + 1)` (which
concatenates the dead mean onto the id and resumes at the wrong tuple)
SURVIVED the old guard and now dies on this guard's own assertion —
`expect(values).toContain('ab_5')`, 1 of 19 tests failing, the other 18
still green so the kill is attributable to this assertion rather than to
some other test's error.
F2 — the migration header undercounted the read paths and did not say that
applying it forecloses the revert. It named two; three queried the table at
base (listAvailable, getAppDetail and getFeaturedBlocks all projected
avg_rating off it). Header now states plainly that running the DROP makes the
PR-level revert a one-way door, and explicitly does not conflate that with the
store-grid rollback note, which stays safe.
F3 — regenerated the direct-mock allowlist with
scripts/test-perf/gen-mock-allowlist.mjs, dropping the entry for the test file
this PR deletes. The regeneration also sweeps in 43 entries that had accrued on
main since the file was last generated (all 43 exist at the base commit; the
canonical list is byte-identical, and totals are self-consistent again).
F4 — made the DROP's precondition executable instead of a comment asking a
human to run a COUNT. A DO block raises and aborts when the table is non-empty.
It carries its own to_regclass existence check so a re-run against an
already-dropped table stays a no-op, matching the IF EXISTS guards below it;
the COUNT is dynamic so it is never planned when the table is absent.
Verified against a throwaway PostgreSQL 17.10 cluster in all three states:
table absent (exit 0, no-op), table empty (exit 0, dropped), table with 3 rows
(exit 3, aborts with the row count, and the table, its rows and all three
indexes survive intact).
Full unit suite: 1519 files / 23963 tests passed, 0 failures.
typecheck: 0 errors. typecheck-tests-gate: 1030 errors/190 files at base ->
1010/188 here, unchanged by this commit.
* fix(db): make the app_block_reviews DROP guard hold under a plain psql -f
Self-review of the F4 guard added in the previous commit found it was only
effective under specific psql flags, which defeated its whole purpose.
A RAISE stops the rest of the FILE only when psql was invoked with
`-1`/`--single-transaction` or `-v ON_ERROR_STOP=1`. Under a plain
`psql -f migration.sql` — an entirely reasonable way to apply a
hand-applied migration — psql runs each statement in its own implicit
transaction and continues past the error: the guard raised, and the DROPs
then executed anyway.
Measured against a throwaway PostgreSQL 17.10 cluster before the fix: the
table and all 3 of its rows were destroyed and psql still exited 0. Silent
and total — the guard read as protection while providing none in that mode.
Wrapping the guard and the DROPs in one explicit BEGIN/COMMIT closes it. The
RAISE poisons the transaction, every later statement is rejected with
`current transaction is aborted`, and the COMMIT degrades to a ROLLBACK.
Re-verified across all 9 combinations of {plain `-f`, `-1`,
`-v ON_ERROR_STOP=1`} x {3 rows, empty, already dropped}: the table and its
rows survive every non-empty case, and the empty and absent cases drop and
no-op as intended. The header now also notes that without ON_ERROR_STOP psql
exits 0 even when the guard refused, so the exit code is not a usable success
signal and the operator should read the output.
* fix(db,test): stop the DROP guard hijacking the caller's transaction
Round 2's delta re-audit found that round 1's own fix introduced a worse
defect, plus one assertion that could not see the mutant it was written for.
1. The migration no longer issues BEGIN;/COMMIT;. Wrapping the guard and the
DROPs in an explicit transaction closed the plain-`psql -f` fail-open, but a
file that commits a transaction it did not open hijacks the caller's
transaction state, and that fails destructively in two shapes an operator
is likely to use:
- `BEGIN; \i this; \i next; COMMIT;` under ON_ERROR_STOP=1 (the header's
own recommended invocation, applied to more than one file). Our COMMIT
ends the operator's transaction early, so when the second file fails
the DROP is already committed and the table is gone anyway, together
with any unrelated row the operator had written in that transaction.
- `\set AUTOCOMMIT off` (the pgAdmin/DBeaver/Retool posture; Retool is a
documented apply path here). The operator's ROLLBACK; prints only
WARNING: there is no transaction in progress, exit 0.
It also silently defeated `psql -1`.
The two DROP INDEXes and the DROP TABLE now live inside the existing DO
block as EXECUTE statements. A lone DO block is one statement and therefore
atomic by itself, so the fail-open protection is retained without the file
taking any position on the caller's transaction state.
2. The legacy-cursor test pinned the bound values as a multiset, not a tuple.
toContain is order-blind, so a decoder that transposes its two return
fields still binds both required strings, merely swapped into the wrong
sides of (sort_key, ab.id) < (?, ?) -- a wrong page boundary, i.e. the
silent skip/duplicate the guard exists to prevent. That mutant survived all
19 tests. The assertion now pins the ordered tail of the bound array.
3. The header cited PostgreSQL 17.10. Production runs 18.3.
Verification, all on PostgreSQL 18.3 (server_version_num 180003, the
production server version):
- 15-cell invocation matrix over {plain -f, -1, -v ON_ERROR_STOP=1, a
failing-second-file batch, AUTOCOMMIT off + ROLLBACK} x {3 rows, empty,
already dropped}: rows survive every non-empty case, empty/absent drop and
no-op, and no case leaves the caller's transaction in a state it did not
choose.
- Negative control first: the guard-stripped file destroys all 3 rows under
-f, -1 and ON_ERROR_STOP=1, so the harness can observe destruction.
- The BEGIN;/COMMIT; draft reproduces the audited defect on 18.3: table
dropped-and-committed with the operator's pending row committed alongside
it, in both nested shapes.
- Transposition mutant: 1 failed / 18 passed, failing on the new tuple
assertion; the same mutant passes 19/19 against the previous toContain
form. The author's original mutant (dead mean concatenated onto the id)
still fails, on the same assertion.
- pnpm typecheck: 0 errors. typecheck-tests-gate (already red on main):
base 1030/190 vs HEAD 1010/188, the 68 unbaselined files identical.
* style(test): prettier-collapse the new cursor-tuple assertion
Formatting only, on the three lines added by the previous commit -- prettier
--check flagged them and CI runs that gate on changed files. Re-verified after
the reformat: 19/19 pass, and the transposition mutant still dies on this
assertion (1 failed / 18 passed, line 329).
* docs(db): correct four false or stale claims in the DROP migration header
Prose only. The SQL is byte-identical: stripped of comments and blanks, the
file matches
|
||
|
Justin Maier
|
c930757e2c |
feat(cosmetic-phash): widen the lane to 256 bits, and make the badge tunable (#4454)
* feat(cosmetic-phash): widen the lane to 256 bits, and make the badge tunable At 64 bits the near-match panel could not separate a copy from a coincidence. Measured over the 1,719 hashable cosmetics, the two badges reported as imitations of official artwork ranked 7th and 116th of their corpus against a 1st percentile of 18 — inside the noise. The orchestrator now offers `perceptualDct256`, and at 256 bits the same two rank 1st and 5th. Both were confirmed by eye, along with three pixel-identical re-uploads of official badges that the submission-time sha256 cannot see, because official cosmetics carry no `imageHash`. The sweep drains the corpus on `pHashVersion`, so bumping the lane is the backfill. `COSMETIC_SIMILARITY_CLOSE_RATIO` is unchanged at 0.125, and that is a finding rather than an omission: it selects the same five cross-creator pairs anywhere between 8 and 40 of 256, so the value sits on a plateau rather than an edge. Being a fraction of the width is what carried it across the upgrade. Three things that were silent before: - A lane bump that moved only two of the three fields left the third disagreeing, and nothing failed. Two of those three mutations passed the suite. `COSMETIC_PHASH_LANE` is now asserted to spell one lane in all three fields, which kills both. - The near-identical decision moved out of the component. It is now decided server-side and carried on the match, because the threshold is operator- tunable at runtime and a client recomputing it from a bundled constant disagrees with the server until every tab reloads. - The threshold reads from a `KeyValue` row so it can be moved without a deploy. Read per call rather than memoised — a TTL would delay the change by the length of the TTL, which is the whole point of the row. It degrades to the built-in default on a malformed or out-of-range value rather than throwing, because this gates a badge and a throw would take out the ranking underneath it. Out-of-range is rejected, not clamped: clamping 1.5 to 1 would badge every match as near-identical and read as a working threshold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cosmetic-phash): close the gaps the five review lanes found The knob shipped wired to nothing, and the tests said otherwise. Two mutations survived the previous commit's suite: replacing the new `close` expression with a constant, and dropping the KeyValue read in favour of the built-in default. Both passed 36/36. `close` had no assertion at either layer, and every `getSimilarCosmetics` test reset the KeyValue mock to undefined, so the whole suite only ever exercised the fallback path. One test now drives the ranking at the default ratio and again at an operator-set one, and both mutations die. `getCosmeticSimilarityCloseRatio` guarded the value it read but not the read. An unguarded rejection escapes `getSimilarCosmetics` AFTER the ranking is complete, and the card renders that as "this artwork was not compared against anything" — false, and the exact confusion the card exists to remove. The read is wrapped; the docblock now describes what the code does. Three of the five malformed-value cases were passing on residue. `loggingMock.logToAxiom` is reset once per FILE, so a later case was satisfied by the first case's warning. With a per-test clear and `toHaveBeenCalledTimes`, a mutant that returns the default silently kills five tests instead of two. Dropped a test that asserted a ratio it claimed was "what the UI thresholds on". It wasn't: both imitation pairs sit above the shipped 0.125, so it checked nothing the neighbouring test didn't already pin exactly, and it hardcoded /256 so the next lane bump would have divided by the wrong denominator in silence. Boundary corrected. `0` is now accepted — "only an exact match is near-identical" is a coherent request, and rejecting it fell back to the LOOSER built-in, i.e. more red badges than the operator asked for. `1` is rejected instead, being the same degenerate outcome `1.5` was already refused for. `normalizeCosmeticHashHex` now refuses a hash wider than the lane. `padStart` returns an over-wide hash unchanged, so it would be stored at full width under the current version — accepted as a comparison target, then excluded from every candidate set by the length filter, reporting "nothing was close" forever on a row that looks correctly hashed. Adds `scripts/oneoffs/drain-cosmetic-phash-lane.ts`. The mitigation the PR depends on — crossing the two-lane window in minutes rather than the sweep's ~2h15m — existed only as a scratch file. It loops the tested sweep rather than issuing its own UPDATE, so the rules about what a correct row looks like stay in one place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cosmetic-phash): make a refused hash say so, and stop the drain script hiding a timeout The previous commit added a silent failure while removing silent failures. `normalizeCosmeticHashHex` now throws on an over-wide hash, and the sweep's catch was a bare `} catch {` with no log — so a lane whose `hexLength` was left behind would hash every row (billed), throw at store, count them all as `failed`, stamp them, and retry once a day forever, with nothing anywhere distinguishing that from dead artwork. The catch now names the row and the reason. It logs fire-and-forget with its own `.catch`, because a logger that throws inside a catch converts a diagnosable failure into a lost one. The drain script asserted a cause it cannot know. `failed` counts rows the sweep could not store this run, and three different things land in it identically: dead CDN artwork, an orchestrator still working when the 30s wait elapsed, and a hash the store refused. The old closing line called all of them "permanently unhashable", which is a guess printed in the voice of a measurement — and the operator's next move differs for each. It now says what was counted and points at the per-row log. Dropped the drain script to the cron's 200/5 from 500/10. `getPerceptualHash` returns `undefined` for both a real failure and a workflow still running, and the sweep stamps either for 24h. More concurrency means more timeouts, so the script written to shorten the window a row spends outside the lane could have EXTENDED it to a day for a slice of the corpus — then printed "done", because a stamped row drops out of the predicate and the next batch comes back empty. The speed was always in removing the 15-minute gap, not in working a tick harder. `MAX_BATCHES` 40 -> 20 with the smaller batch, ~2x the corpus rather than 11x. Corrected the lane docblock, which still described the pre-guard symptom: a stale `hexLength` no longer reaches a comparison-time throw, it fails at the store and writes nothing. The new log path is asserted rather than assumed — deleting it turns the sweep suite red, with the unmutated control green either side. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(cosmetic-phash): say what the drain script's failure count can and cannot tell you Comment and console output only; no logic. The closing line has now asserted a cause twice and been wrong both times, in opposite directions. First it called every failure "permanently unhashable". Then, correcting that, it claimed the cause "is not recorded per row" and sent the operator away — but two of the three paths to an undefined hash DO log, and so does the store-refused throw. That version would have had an operator ignore records that exist, including the one class whose correct action is neither "nothing" nor "re-run" but "fix the lane". The coverage is now spelt out rather than summarised, because summarising it is what went wrong twice: a relative media url and any network error or abort log as `perceptual-hash`; a refused width logs as `cosmetic-phash-sweep`; a workflow that simply did not succeed logs nothing, and that silent case is both the 30s timeout and dead artwork — the two most likely reasons a row is in the count. Also drops "re-run tomorrow" for "re-run once 24h have elapsed". The stamp is a 24-hour window, not a calendar day, so a next-morning re-run of an overnight drain finds every row still suppressed, breaks immediately on an empty batch and prints a clean zero — indistinguishable from a completed drain, which is the silent-failure shape this script keeps trying not to reproduce. And it no longer calls a repeat failure dead artwork. A width that disagrees with the lane fails identically on every run forever, an unset NEXT_PUBLIC_IMAGE_LOCATION fails for every row, and timeouts are correlated rather than independent draws — so "failed twice" does not imply "dead". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
c576281835 |
fix(test-perf): the module tracer never flushed under Vitest 4, so a traced run wrote nothing (#4441)
* fix(test-perf): the module tracer never flushed under Vitest 4, so a traced run wrote nothing
scripts/test-perf/trace-setup.ts flushed its counters only from process.on('exit') and a 15s
interval. Vitest's forks pool kills its workers rather than letting them exit, so the exit
handler never ran, and the README's own workflow - "trace one file at a time" - is a 5-10s run
that never reached the interval either.
The failure mode is the expensive kind: .test-perf/trace was never created at all, and
trace-report.mjs answered "no .test-perf/trace - run a traced suite first", which reads as
operator error rather than as a dead instrument. Since graph.mjs's model of what a worker loads
is validated against this tracer, the validation was unreproducible for as long as it was dead.
Flush from afterAll, which runs inside the worker before the pool can kill it. The exit handler
and the interval stay as backstops.
Verified on origin/main in a clean worktree: before the change the documented workflow creates no
trace directory; after it, trace-report.mjs prints a real report (1 worker snapshot, 45 distinct
modules).
The regression test spawns a real traced run and requires a snapshot naming the module the
fixture imported - asserting that this file contains the string "afterAll" would pass just as
happily with the hook registered somewhere it never fires. Both mutants die for their own
reason: reverting to the old exit-only flush fails on "expected 0 to be greater than 0", and a
flush that writes an empty snapshot fails on the module-name assertion. It runs in 1.7s.
TESTPERF_TRACE_DIR is new, so that test can redirect its snapshots instead of clobbering a trace
someone is reading.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): name the traced project apart, or its dep cache eats the unit suite's
Audit round 1 on this PR found the regression test was deploy-blocking, and reproducing it
independently was worse than the report: 16 of 53 files red, not 5 of 6.
Vitest keys a project's dep-optimizer cache on sha1(projectName) and Vite's config hash includes
the plugin names, so the traced project - which spread the unit project and kept its name while
adding the tracer plugin - resolved to the SAME node_modules/.vite/vitest/<hash>/deps_ssr as the
normal unit suite and hashed differently. Vite responds by deleting and re-bundling that
directory, while unrelated workers in the same shard are importing chunks out of it:
"Cannot find module '.../deps_ssr/prom-client.js'". assert-shard-ran.mjs would not have caught
it either - losing ~400 of ~5539 tests stays inside its band.
Naming the traced project unit-trace gives it its own cache dir. Same 53-file selection:
53 passed, 0 deps_ssr errors, two cache dirs on disk.
Also from the audit:
- trace-report.mjs now honours TESTPERF_TRACE_DIR, which trace-setup.ts already read. Exporting
it used to reproduce this PR's own bug: snapshots on disk, report says "run a traced suite
first".
- The trace dir is cleared at the start of each traced run. The report SUMS every snapshot it
finds and snapshots outlive their run, so a second run silently reported roughly doubled
numbers - a wrong number rather than a missing one.
- Snapshots are keyed by a per-worker id rather than bare pid. forks gives one process per file
so pid was unique there, but threads puts every worker in one process (measured: 3 files gave
3 snapshots on forks, 1 on threads), and per-file flushing would have several workers writing
one path concurrently.
- The flush no longer swallows its error. A silently unwritten snapshot is the exact failure
this PR exists to close.
- The child run's interval backstop is pushed to an hour, so afterAll is the only path that can
satisfy the test. Otherwise a child that lives past 15s passes it through the timer with or
without the fix.
- README: the traced invocation is --project unit-trace, and the paragraph claiming a multi-file
traced run keeps only the last file's counters is no longer true.
Fixing the clear-at-start introduced its own regression, caught by re-running the battery: it
removed the caller-supplied directory rather than its snapshots, turning a clean assertion into
an ENOENT crash. It now unlinks *.json only.
Six mutants, six deaths, each on its own assertion: no afterAll -> expected 0 to be greater than
0; empty snapshot -> expected [] to include the fixture module; project renamed back to unit ->
startup error, no "1 passed"; clearing disabled -> expected 2 to be 1; bare pid -> filename shape.
The bare-pid case is killed by a filename-shape PROXY and is labelled as one in the test - the
real property needs a threads-pool child with several workers, which these tests do not build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): close the rename's own coverage hole, and stop the fix round's new sharp edges
Delta re-audit of the previous round. Its verdict was "safe to merge"; its findings were not
empty, and the findings are what decide.
The gap that mattered: every assertion named unit-trace, so renaming the project back to unit in
BOTH the config and the test passed green while fully restoring the cache collision that took 16
of 53 files red. The previous tip was literally that configuration and shipped green. There is
now a case that runs the traced config with --project unit and requires it to FAIL with "No
projects matched", so the two spellings can no longer be reverted together in silence.
The bare-pid guard was a filename-shape proxy, and the audit was right that the real property is
cheap to test: two more three-line fixtures and one --pool=threads child. It now asserts the DATA
survived - all three fixture modules present in the merged snapshot - rather than the filename.
Reverting to a bare pid loses two workers' counters to a clean last-wins overwrite and the case
goes red.
That mutant also exposed a coupling the audit did not see: the writer's filename format and the
clear's delete predicate were two copies of one rule, and with a bare pid the clear silently
stopped matching, so a second run summed on top of the first. Both now come from
trace-snapshot-name.ts.
Other audit findings:
- trace-report.mjs created no .test-perf/ before writing trace.json, so with TESTPERF_TRACE_DIR
set on a fresh clone the documented workflow crashed with a raw ENOENT after doing all the
merge work. mkdirSync first.
- TESTPERF_TRACE_INTERVAL_MS ran through bare Number(), so abc/empty/-5 all became a 1ms
synchronous whole-snapshot write loop inside every worker - measured 45 fires in 50ms - which
charges its own cost to the measurement. Validated with the same idiom vitest.config.mts uses.
- The snapshot clear deleted every top-level *.json in a caller-supplied directory. Scoped to
this tool's own filename shape, and wrapped: a throw there happens at config load and would
abort the whole run with a raw stack.
- trace-config.mts advertised a bench.mjs invocation that its own rename breaks (bench hardcodes
--project unit). Docstring corrected rather than widening bench's filter, which would drag
unit-native into every yardstick run.
- Two comments corrected against measurement: the flush fires once per test FILE, not per suite,
and the thread-pool failure is a silent last-wins overwrite, not a truncated read.
- README now discloses what the rename costs - a second 87MB dep bundle - and that under --watch
only the first re-run clears.
Seven mutants, six dead on their own assertion: no afterAll -> expected 0 to be greater than 0;
empty snapshot -> expected [] to include; clearing disabled and bare pid -> expected 2 to be 1;
full rename -> expected +0 not to be 0. M7, reverting the interval validation, SURVIVES: no test
supplies garbage to that knob, so it is an unpinned defensive guard and is called one here rather
than counted as covered.
🔴 One of the previous round's mutants did not actually apply - backtick escaping - and its
"survived" reading was a fact about the harness, not the code. Every mutation in this round
asserts its own target was found before the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): pin the two behaviours the last round asserted but never checked
Round 3 of the delta audit. Verdict was "ship"; it still found two guards that no test could see,
and both are now killed by a mutant.
- The clear's PROTECTIVE half was unpinned. Widening SNAPSHOT_FILE_RE back to /\.json$/ left every
test green while destroying a caller's other JSON - exactly what the README promises it will not
do, and the whole reason trace-snapshot-name.ts exists. Only the drift direction was covered.
Test 1 now writes important-notes.json into the trace dir and requires it to survive.
- The interval validation was called an unpinnable defensive guard. That was giving up early: the
audit pointed out this PR had just established the pattern for pinning it. resolveIntervalMs is
now a pure helper in the shared module with a nine-case table, no child process, and reverting
it turns seven cases red.
Fixing that found a real hole in my own guard: Number.parseInt reads '1e10' as 1, which is the
1ms flush loop the check exists to prevent, and a case bare Number() handled correctly. It now
uses Number with an n >= 1 bound.
Also from the audit:
- The clear caught its error around the whole loop, so one undeletable snapshot abandoned every
other stale file - and the survivors are summed into this run's numbers. Now per entry.
- The docstring said a typo'd --project unit "fails loudly". True, but not harmlessly: the clear
runs at config load, before the project filter, so that invocation empties the trace directory
first. Said so.
- The suffix comment claimed once per worker; measured once per test FILE under isolation.
- Fixture c's stated rationale was wrong - two files already discriminate, because the tracer
re-initialises per file rather than per worker, so a scheduler batching them cannot hide an
overwrite. Deleted it and the threads case runs on two.
Nine mutants, nine deaths, each on its own assertion: no afterAll / bare pid / writer filename
drift -> expected 0 to be greater than 0; empty snapshot -> expected [] to include; clearing
disabled -> expected 2 to be 1; full rename -> expected +0; interval validation reverted ->
expected NaN to be 15000; regex widened -> expected false to be true. Every mutation asserts its
target matched exactly once before the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): the interval guard had an upper bound problem, and the test asserted the broken value
Round 4 of the delta audit, and it caught my round-3 fix landing on the same pathology from the
other side.
resolveIntervalMs bounded the input below but not above. setInterval stores its delay in a 32-bit
signed int, so anything over TIMEOUT_MAX (2147483647) is clamped to 1ms - measured on Node
24.19.0, 1e10 fired 281 times in a 300ms window with "TimeoutOverflowWarning: Timeout duration was
set to 1", and it reproduces end to end in a real traced run. That is the ~900 writes/second
inside every worker that the function's own docstring exists to prevent, reachable from the "make
it huge so it never fires" direction. Worse, the it.each table I added asserted 1e10 -> 1e10 as
the correct answer, so the defect was enshrined as deliberate. Bounded at both ends now, with
cases at MIN-1, MIN, MAX, MAX+1 and 1e10.
Four behaviours the audit showed were unpinned or unmodelled:
- The per-entry catch in the clear was real and load-bearing - 0 survivors vs 5 - and no test
could tell it from the pre-fix shape. The clear is now an exported clearStaleSnapshots with a
synthetic-directory test including an EISDIR entry; reverting it to a rethrow goes red.
- mergeSnapshots in the test had stopped modelling trace-report.mjs: it filtered by the snapshot
regex while the real reader took any *.json. Running the real reader against the directory this
PR's own test creates gave "20 distinct modules | NaN executions | NaNs self time". The reader
now accepts snapshots by SHAPE rather than extension - it is a .mjs file and the shared
constants are TS, so a value check is what cannot drift - and a test spawns it against a foreign
file and requires no NaN.
- Dropping the regex's $ anchor survived green: it still matches 1234-abcd.json.bak and .json.swp,
i.e. editor and backup files next to a caller's data. The survives-set now includes one.
- The summing merge survived green too, because with clearing on there is only ever one snapshot.
It is now pinned where two genuinely coexist - the threads case - against the sum of a module
common to both files.
Also: TESTPERF_TRACE_DIR used ?? in all three readers, so an exported-but-empty value kept '',
mkdirSync('') threw, no snapshot was written and the report said "run a traced suite first" - the
dead-instrument failure this whole change exists to remove. Now || with a trim, single-sourced.
And trace-setup.ts's header rationale claimed globalThis makes counters survive across files;
measured, each file writes a disjoint snapshot on both pools and the totals are correct because
the report SUMS. The stale per-worker framing is gone from the report's label too - that count is
test files.
Thirteen mutants, thirteen deaths, each on its own assertion. Every mutation asserts its target
matched exactly once before the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): guard the tool, not the test's mirror of it — closing three named gaps
Round 5 of the delta audit named three fixes and a mechanical closing condition: re-run five
specific mutants rather than open a sixth round. All three are here and all five now die.
The pattern the audit named, which is what actually cost the last two rounds: the tests were
guarding their own mirror of the tool rather than the tool.
- The interval table derived every expectation from the constant it was testing, so it could no
longer see a WRONG CONSTANT. DEFAULT -> 1 and MAX -> 2**31 both survived a green suite, and both
are the 1ms flush loop the function exists to prevent - reachable on the default path a
developer hits by not setting the variable at all. That is round 3's defect one notch out,
introduced by round 3's own fix. Two literal rows now sit beside the derived ones.
- The shipped reader's sum was unpinned. mergeSnapshots in the test is a reimplementation, and it
was the only thing any mutant could see; the single test that ran the real reader had a
one-snapshot fixture, where summing and overwriting are identical. The fixture now holds two
snapshots naming one module, asserted against the totals the tool itself prints.
- The .mjs reader cannot import the shared TS resolver, so it carries its own copy of the
empty-string rule - the same drift the shared module exists to stop, one layer out where the
module cannot reach. Reverting it to ?? was green while reintroducing half the dead-instrument
failure this PR removes. A second spawn with TESTPERF_TRACE_DIR='' closes it.
Free corrections from the same round: trace-config.mts still documented one snapshot "per worker"
while trace-snapshot-name.ts three files away says that is not true; a test title and the README
carried the same stale framing; fixture b's comment counted three fixtures when there are two.
Closing condition, each mutation asserted applied and its mutant text confirmed on disk before the
run: DEFAULT->1 dies on `expected 1 to be 15000`; MAX->2**31 on `expected 2147483648 to be 15000`;
reader loads += -> = and selfMs += -> = both on the printed totals; .mjs ?? on the fallback path.
28 tests, 4.6s.
Not taken, deliberately: the report still exits with a raw ENOTDIR stack when TESTPERF_TRACE_DIR
names a regular file, and isSnapshot's guards are wider than the one test covering them. Both were
called optional, neither is a live defect, and adding more scaffolding to a gitignored dev tool is
the thing this ladder has been overdoing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
afe84cf7c8 |
ci: shard the unit suite across 4 runners, 10.4m -> 3.8m (#4392)
Measured from the Actions API (n=12): the job was 555.5s of test work plus 66.5s of fixed overhead = 622s, the 10.4m median, and 3.4x the next-slowest job. T(N) = 66.5 + 555.5/N puts N=4 at ~3.4m. Measured after: 10.3m -> 3.8m on the critical path (2.7x) at +36% runner-minutes, reproduced across two runs with a 1.12-1.14x shard spread. N=4 rather than more because `App unit tests + typecheck` is 2.9m — past N=5 sharding optimises something that is no longer the bottleneck. Ships with a positive control, `scripts/ci/assert-shard-ran.mjs`. Sharding adds a failure this job did not have: a `--shard=i/N` selecting no files exits 0 and reports green, and four green checks look identical whether they ran 22,000 tests or none. The guard asserts per shard that work happened, counting EXECUTED assertions rather than `numTotalTests` (which includes skipped, so an all-self-skipping shard would pass a total-based floor), and its bounds scale with `total` so a change to the matrix does not trip them. It earned its keep immediately: on the first run it failed all four shards while `Unit tests` reported SUCCESS. `pnpm run … -- --shard=…` forwards the `--` literally, vitest DISCARDS everything after it, and so `--shard` and `--outputFile` were both silently dropped — each runner executed the entire suite (7m30s-9m54s per step) and wrote no report. Without the control this would have merged as four green checks that tested nothing verifiable. A subsequent adversarial audit found three further claims that measurement did not support, all fixed here: bounds hardcoded off a ten-day-stale suite size that would have tripped within weeks and blamed the wrong cause; a comment asserting post-`--` args become filename filters when they are discarded; and a "mutation-verified" claim that five of twelve mutants walked through. Four are now covered by named cases; the fifth was unreachable dead code and is removed rather than papered over with a test that could not kill it. Final sweep, re-run after the Prettier pass: 10 mutants, 10 killed, each by its own named test, with a no-op negative control that correctly survived. Suites 20/20. Verified live across two runs; the typecheck-scripts gate was confirmed to have executed rather than assumed green. Not verified: behaviour on a push to `main`, where `continue-on-error` is false and a red shard renders as a genuine failure. That path only exercises on merge. |
||
|
Zachary Lowden
|
b34d90fd1c |
fix(apps): make the listing-completeness advisory KIND-AWARE (on-site copy lives in block.manifest.json) (#4370)
* fix(apps): make the listing-completeness advisory KIND-AWARE
`computeListingProblems` was kind-blind. It emitted `empty-description`,
`empty-tagline` and `empty-category` with the label "Missing <field>" for
EVERY listing. On an OFF-SITE listing that is correct — the author typed that
copy into the submit wizard and can go and fix it. On an ON-SITE listing it is
wrong, not merely terse: those scalars have NO author surface other than
`block.manifest.json`, and `approveRequest`'s (3b-sync) MANIFEST-GOVERNED COPY
RE-SYNC (`publish-request.service.ts`, scoped `kind: 'onsite'`) re-derives them
from the manifest on every subsequent-version approve. An on-site author who
found some other way to set a tagline would have it reverted at the next
approve — so `/apps/mine` was telling them to do something that cannot work.
KEEP-WITH-CORRECTED-LABELS, not suppress. The gap is real (the store page
genuinely has no tagline), so hiding it would trade wrong advice for no advice;
and suppression would silently kill the on-site branch of a released CLI that
consumes these codes. The codes and severities are therefore KIND-INVARIANT and
only the three labels move.
`kind` is a REQUIRED input, which is what makes "a caller was missed" a compile
error rather than a whole surface keeping the old advice. All three callers are
threaded, each PROJECTING the column rather than restating what its own filter
implies:
- appListings.listMine (app-access.service) — both kinds, one page
- appListings.listMySubmissions (offsite-listing.service) — on-site media
revisions appear here too, via its `{ kind: 'onsite' }` OR-branch
- blocks.listMyPublishRequests (blocks.router) — projects `kind` even
though its `where` filters on it
The kind lookup is an explicit equality branch, not a `TABLE[kind] ?? default`:
`kind` is an untrusted cast off the `app_listings.kind` column, and indexing an
object literal with an inherited key (`'constructor'`) returns something truthy,
which `??` accepts and which then yields a problem with NO label. An
unrecognised kind degrades to the original labels and never throws.
No component change: `ListingProblemsIndicator` renders whatever `label` it is
given.
Tests: 12 regression cases RED at origin/main
|
||
|
Justin Maier
|
06e2651d47 |
fix(dev-server): stop the daemon popping a terminal window that steals focus (#4373)
`startDaemon` spawned the daemon with `shell: true`. Node applies `windowsHide` only to the process it creates, so the flag landed on cmd.exe and never on the node cmd.exe then started. That node got a fresh console, Windows 11 handed it to the default terminal app, and a Windows Terminal window opened and took focus off whatever the user was doing. Measured: the window appeared on every run of the unpatched code and stole focus on four of six. Dropping the shell fixes that, but it also leaves the daemon with no console of its own — so every child it spawns is then in the same position and pops its own window, once per dev server started, per taskkill, per queued vitest run, per branch-watch `git`. Verified that too, so `windowsHide` goes on all of them. Guarded by scripts/__tests__/dev-server-no-console-window.test.ts, which fails if either half regresses (checked by mutating both back). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
0d1142ce4c |
fix(skills): a flag with no value is an error, not a silent true (#4263)
* fix(skills): a flag with no value is an error, not a silent true
metabase and cloudflare both degraded a value-taking flag whose value was
missing, empty, or --prefixed into a truthy placeholder: boolean true in one,
the string 'true' in the other. Every downstream guard tests truthiness, so
create-question posted native: { query: true } and Metabase stored a card with
no query on it — created successfully, opens blank, indistinguishable from a
permissions problem.
Both parsers now reject a missing value and accept --key=value for a value that
legitimately starts with --. create-question reads the card back and fails if
the stored SQL is not what was sent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(skills): document the flag-value rule in both SKILL.md files
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(skills): keep --json parsing, close the --key= hole, and separate a failed read-back from a failed create
Review of #4263 found three things. --json is advertised in metabase's own usage
text, so rejecting it was a regression introduced by the fix rather than by the
bug. The --key=value form — the one the error message sends people to — accepted
an empty value the space-separated form rejects. And a cloudflare boolean flag
no longer consumed an explicitly spelled , which shifted positional[1].
The read-back now distinguishes 'created but could not be read back' from
'created with the wrong SQL': a GET that fails must not be reported as a create
that failed, since the card exists either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
bdb55c9ef2 |
feat(observability): expose Flipt eval-cache stats — a hit rate alone cannot pick the right knob (#4202)
* feat(observability): expose Flipt eval-cache stats — a hit rate alone cannot pick the right knob
The per-eval TTL cache in `@civitai/flipt` tracked `hit` internally and threw the
result away, so from outside the process it was a black box. It has two tuning
knobs — `FLIPT_EVAL_CACHE_TTL_MS` and the `evalCacheMaxEntries` ceiling — and no
signal to choose between them.
That matters because the two failure modes are indistinguishable from a hit rate
and have OPPOSITE remedies:
* misses dominated by expired entries -> TTL-bound. A longer TTL converts them
into hits.
* generation rotations climbing -> capacity-bound. Entries are evicted before
they can expire, so a longer TTL recovers NOTHING; it is an inert change that
reads as a fix. The ceiling is the knob.
The cache key is (flag, entityId, context), so a per-user entityId multiplies the
key space by the active-user count — capacity-bound is the likelier of the two on
hot paths, which is exactly the case a bare hit rate would have hidden.
Adds cumulative counters to TtlCache (hits, misses, expiredMisses, rotations,
size), surfaces them via `getCacheStats()`, and exports them from the monolith as
`civitai_app_flipt_eval_cache_*` on the same default registry /api/metrics
scrapes. Registered by side-effect import there, matching its neighbours, so the
series exist from the first scrape — an absent series would read as "the cache is
idle" rather than "nobody loaded the module".
The package gains no new dependency: the counters are plain numbers and all
prom-client wiring lives app-side.
Verified rather than assumed. Every new guard was mutation-tested and each mutant
died by its OWN named test, with the control restored byte-identical:
* expiredMisses bumped on every miss -> killed by the cold-vs-expired test
* rotation counted on every set -> killed by the overflow test
* promoted read not counted as a hit -> killed by the promotion test
* reset() dropped from collect() -> killed by the double-count test
* boolean stats reported for both labels -> killed by the per-kind test
Typecheck: 317 errors at origin/main and 317 on this branch, with byte-identical
error sets (zero branch-only errors) — the baseline is a stale generated client,
not this change. eslint clean on the touched files and on packages/civitai-flipt,
verified with a negative control that the linter actually processed them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(flipt): pin the client on globalThis — the metric was measuring ONE of TWO caches per pod
Audit found the observability this PR adds was itself half-blind, in the same
shape the immediately-preceding merged PR (#4173) reverted the pg-pool gauges for.
MEASURED, not theorised: `src/server/flipt/client.ts` is emitted TWICE in the
production server build. `[flipt] eval cache TTL:` appears exactly 2x on every
pod — 488 lines across 244 pod streams — against one `[instrumentation] Running
in nodejs runtime`. Each emitted copy owned a private wasm client, a private 60s
config poller and a private pair of eval caches, and `getFliptCacheStats` closed
over whichever copy its chunk resolved.
That is worse than a scale error, because the deliverable is a KNOB DECISION and
the bias has a direction: rotations are superlinear in per-instance key space, so
one key space split across two caches at the same ceiling rotates far less than
one cache holding all of it. The split reads as "TTL-bound" and sends the reader
to the knob that changes nothing — the exact inert change these metrics exist to
prevent.
Pins the client on `globalThis.__civitaiFliptClient` using the repo's canonical
idiom and enrols the module as SHARED_STATE in server-graph-watchlist.mjs, so a
refactor that drops the pin fails that gate. Side benefit: one wasm engine and
one config poll per pod instead of two.
Also fixes three defects the audit found in this PR's own tests:
* The seam was untested. Deleting the side-effect import in
src/pages/api/metrics.ts left the whole src/server/metrics/ suite green
(131/131) — registered-but-unreached, this repo's #1 metric-death mode. Now
asserted by loading the module that SERVES the scrape, matching the
substitutions and bitdex-feed-serve seam tests.
* One asserted cell was vacuous: read() ended `?? 0`, so asserting the variant
cache's expiredMisses is 0 passed whether or not the series existed —
the absent-vs-zero ambiguity this metric exists to remove, reintroduced in
the test guarding it. Returns NaN on a missing label now.
* prettier failed on the new file, making CI red. The PR body claimed "lint
clean on the touched files"; that was true of eslint and false of prettier —
the negative control was run on the wrong instrument.
Mutation-verified, each killed by its own named test, sources restored
byte-identical with controls green:
* delete the side-effect import -> seam test (previously SURVIVED)
* gate the variant inc on non-zero -> per-kind test (previously SURVIVED)
* `??=` downgraded to `=` -> watchlist gate
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
5770b83f4e |
ci: give a commit on main a CI verdict again, and pin that it keeps one (#4234)
* ci: give a commit on `main` a CI verdict again, and pin that it keeps one Nothing has produced a CI verdict for a commit on `main` since 2026-06-14. The cause was not a decision: `.github/workflows/pr-check.yml` was the only workflow this repo has ever had with `push: branches: [main]`, and #2547 deleted it while moving PR checks to Tekton. Every workflow written since — lint (#3362), submodule-pin-guard, schema-drift (#3643), windows-dev-env (#4162) — is `pull_request`-only, so the main-push trigger was never rebuilt and nothing said it was gone. Tekton does not close the gap. Its `pr-check` pipeline is driven by a resource that watches open pull requests targeting `main`; the only main-branch trigger builds a container image for the staging deployment, runs no test suite and posts no commit status. "Tekton covers main" is the assumption that let this persist for two months. The old June runs stayed `success`, so "is main green?" went on answering yes. That is the expensive half: the signal was not wrong, it was stale, and staleness is invisible unless you read the dates. |
||
|
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.
|
||
|
briant
|
c92c71e2a5 |
Merge remote-tracking branch 'origin/main' into moderator-feedback
# Conflicts: # scripts/__tests__/dev-server-daemon-port.test.ts # scripts/__tests__/typecheck-apps.test.ts # src/__tests__/source-nul-bytes.test.ts # src/components/Apps/__tests__/appListingStatChips.test.ts # src/server/services/__tests__/model-file-hash-writers.test.ts # src/tests/build/standalone-boot-graph.test.ts # src/utils/__tests__/rating-label.test.ts |
||
|
briant
|
ba7504aac3 |
fix(tests): make seven suites pass on Windows
17 tests across 7 files failed on a Windows checkout and pass on CI. None was a product defect; all were portability bugs in the harness code, and two of them were reporting the thing under test as broken when it was not. Path separators. `path.relative` yields backslashes on Windows while the ledgers these tests compare against are written with `/`, so the walk matched nothing and the ledger read as "no consumers" — `rating-label`, `model-file-hash-writers`, `dev-server-daemon-port`. Normalised at the walk. `new URL(..).pathname` gives `/C:/…`, which resolves to `C:\C:\…` and ENOENTs — `appListingStatChips` now uses `fileURLToPath`. `standalone-boot-graph` asserted on `esm/index.js` in output where Node prints a NATIVE path. `dev-server-daemon-port` spawned `import(<absolute windows path>)`, which ESM rejects with ERR_UNSUPPORTED_ESM_URL_SCHEME because the drive letter parses as a scheme. The child exited 1 and the test — "can be imported without throwing" — reported the daemon as broken for a reason unrelated to the daemon. Now `pathToFileURL`. Two are genuine environment limits and are SKIPPED rather than weakened, so the gap stays visible. The three symlink CONTROLs in `source-nul-bytes` need elevation on Windows (EPERM, reproduced); they are gated on a capability probe rather than `process.platform`, so Linux CI and an elevated shell still run them, and the main NUL-byte assertion is untouched. `typecheck-apps` puts a `#!/usr/bin/env bash` fake `pnpm` on PATH with `chmod 0755` — neither means anything on Windows — so it is skipped there; its `:` PATH separator is fixed to `path.delimiter` regardless. Counts, since skips can hide regressions: 19502 passed + 27 skipped + 17 failed before, 19506 passed + 40 skipped + 0 failed after. Same 19546 total; the 13 new skips are exactly the 3 symlink controls and the 10 typecheck-apps cases. Also clears two pre-existing eslint errors in a file already being touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Zachary Lowden
|
94510da5f3 |
fix(metrics): bulkhead + pg-pool gauges emitted zero series — a globalThis flag guarding a per-graph prom registry (#4173)
* fix(metrics): a globalThis flag guarding a per-graph registry made the bulkhead gauges emit nothing for 74 days `civitai_app_heavy_bulkhead_active` / `_rejects` have produced ZERO series since they shipped in #2428 (2026-06-07), while every link in the chain looked healthy: registered in code, merged to release, in the running image, on a hot route, on pods emitting hundreds of thousands of other series. Two independent defects, and fixing either one alone leaves the metric inert. 1. REGISTRY. Next.js compiles instrumentation.ts into a separate webpack bundle from the pages/API bundle, and prom-client is not in serverExternalPackages, so each graph gets its own `client.register`. /api/metrics scrapes the PAGES graph's default registry plus the globalThis-pinned instrumentationRegistry; the instrumentation graph's default registry is scraped by nobody. The gauges were wrapped in `if (!global.heavyBulkheadGaugeInitialized)`, which pairs a PROCESS-scoped flag with a GRAPH-scoped registry. That is worse than no guard: instrumentation.node.ts -> eventloop-longtask -> prom/client runs at pod start, claims the flag, registers into its own unscraped registry, and the pages graph then takes the early-out and registers nothing. Now registered via registerInstrumentationMetric, which dedupes against the same shared registry it writes to, so the two scopes agree. 2. STATE. The gauges are collect()-based over `bulkheadSnapshot()`, and request-bulkhead's slot/reject Maps were module-local — so whichever graph won registration read its own permanently empty maps. Pinned on globalThis, the same mechanism instrumentationRegistry and __civitaiRedisMetrics already use. Admission control was always correct in the graph serving requests; this makes the cap genuinely per-POD rather than per-pod-per-graph, which is what it was always documented to mean. The nine `node_postgres_*` pool gauges carried defect 1 via `pgGaugeInitialized` and were measured at 0 series too. Fixed in the same commit: same bug, same file. Measured on production before the fix, which is what isolates the flag as the variable rather than the registry or the graph: civitai_app_heavy_bulkhead_{active,rejects} 0 series (default reg + flag) node_postgres_* (9 gauges) 0 series (default reg + flag) civitai_app_image_ingestion_backlog 640 series (shared registry) images_search_* (same request path) 181 series (default reg, no flag) Tests: 8 new cases in src/server/__tests__/prom-cross-graph-registration.test.ts, using vi.resetModules() between dynamic imports to reproduce the two-graph split (fresh module instance, intact globalThis — the exact asymmetry the bug lived in). Matrix: 8/8 RED at origin/main, 8/8 green here. With ONLY defect 1 fixed, 3 pass and the two seam cases still fail — the gauges register, get scraped, and render nothing — which is what proves the second half is load-bearing rather than tidy-up. Also corrects a comment in @civitai/telemetry that cited these guards as the working precedent for pinning on globalThis. They pinned the flag, not the thing it guarded, and every metric behind them was dead. The Grafana panel's `or vector(0)`, which turned this absence into a drawn zero line, is removed separately in the infra repo. Refs #2428. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: prettier — formatting only, no behaviour change Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(metrics): audit round — guard the scrape, kill 5 surviving mutants, enrol the pin in the gate Adversarial audit findings. No deploy-blocker; these are the three 🟡s worth closing before merge, plus the fixture defect that let them hide. 1. GUARD THE SCRAPE (src/pages/api/metrics.ts). `Registry.metrics()` is a Promise.all over every metric's get(), so ONE collect() that throws rejects the whole call — and `Gauge.set` throws on a non-number. This PR moved 11 new collect() bodies into that awaited path, six of them reading pool counters unguarded, so a single bad read would 500 the ENTIRE scrape: default metrics, every instrumentation metric and the Prisma series with it, precisely when you most need to see the pod. Both registries now degrade to losing one block, with a seeded `registry_scrape_failures_total{registry}` so the loss is observable rather than silent. The file already wrapped a strictly smaller risk the same way. 2. THE TESTS DID NOT PIN THE PG GAUGES — proven, not assumed. The stub gave every pool `{totalCount: 0, idleCount: 0, waitingCount: 0}`. Identical, default values across four pools make every transposition render byte-identical output, and the nine gauges are (name, help, reader) triples where a wrong pool or wrong counter is a one-token slip no type checker can catch. Five mutants SURVIVED a fully green suite. Fixture values are now pairwise distinct (11/12/13, 21/22/23, 31/32/33, 41/42/43) and two cases assert rendered VALUES, including an absent-pool case that reaches the null-guard branch — which no all-pools-present fixture can. Battery re-run after every change in this commit, baseline green either side: M1 labelled write reads pgDbRead ........ KILLED (was SURVIVED) M2 read_idle reads .totalCount .......... KILLED (was SURVIVED) M3 write_waiting reads pgDbRead ......... KILLED (was SURVIVED) M4 drop the labelled null-guard ......... KILLED (was SURVIVED) M5 swap read_long/datapacket labels ..... KILLED (was SURVIVED) PC1 pin `??=` weakened to `=` ........... KILLED (the original bug, one character) PC2 un-pin entirely ..................... KILLED (10/10 fail) PC3 rejects gauge reports active ........ KILLED The harness itself needed two fixes first: the vitest summary is ANSI-prefixed so the grep matched nothing and scored every mutant SURVIVED including the positive control, and the copied tree's .envrc was un-allowed so nothing ran at all. Both produced a confident, uniform, meaningless result — hence the mutator now refuses to report when the file hash did not move or no summary line was found. 3. ENROL THE PIN IN THE GATE THAT EXISTS FOR THIS (scripts/server-graph-watchlist.mjs). The repo has a build gate that fails when a module needing process-wide identity loses its globalThis pin. This PR added such a pin and enrolled it nowhere, so a future refactor reintroduces the defect silently — which is how it happened. Both maps now live under ONE key, `__civitaiBulkheadState`. Two keys would mean two watchlist entries naming the same module, which the gate cannot express: its fixture emits one chunk per entry carrying only that entry's key, so each entry fails on the other's chunk. Measured — it red-lined the gate's own positive control. The pin also moves to the repo's canonical `??=` form, which the gate's test asserts against comment-stripped source so a key surviving only in prose cannot pass for a binding. Verified: typecheck 0 errors (103s); prettier clean on all 5 files; 109 files / 1895 tests green across src/server/__tests__, scripts/__tests__ and src/tests/api/v1; test:lint-rules 243/243. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * revert(metrics): keep the pg pool gauges OFF the shared registry — moving them made them WRONG Reverts the pg-gauge half of this PR. The bulkhead fix is untouched. Two independent agents flagged the same unverified risk, so I measured it, and it is real. A collect()-based metric needs BOTH halves shared: the registry it registers into AND the state its closure reads. The bulkhead has both — request-bulkhead.ts pins its maps on globalThis. The pg gauges have only the first: src/server/db/pgDb.ts globalThis-pins the pools ONLY in its `!isProd` branch (pgDb.ts:26-42), so in production every emitted copy of that module builds its own pools. The graph that wins registration is the instrumentation graph, whose pools serve nothing but the ingestion-backlog query in prom/client.ts. Measured on a preview running exactly the reverted change: metric idle under 30 concurrent /api/v1/images node_postgres_read_total_count 1 1 node_postgres_read_waiting_count 0 0 node_postgres_write_total_count 0 0 node_postgres_pool_total_count 1 / 0 / 0 / 0 (unchanged under load) Frozen, plausible-looking and wrong. That is strictly WORSE than the honest absence they have today: an absent metric prompts a question, a confident 0 ends one — the same false-all-clear class as the `or vector(0)` this PR removes from the reject panel, which is the defect the whole change exists to fix. Shipping it would have re-enacted the bug while claiming to fix it. So they go back to exactly their `origin/main` form, under `global.pgGaugeInitialized`, with a long note at the site saying what was tried, what was measured, and why the next person must not repeat it. The test that asserted they reach the shared registry is INVERTED: it now pins that they stay off it, so a future well-meant move has to change the test and read the note. Making them real means pinning the pools in pgDb.ts for prod. That changes production DB connection topology (today: one pool set per emitted graph, which is also worth someone's attention on its own) and is its own change with its own blast radius, not a rider on a metrics fix. Consequence for the previous commit: its mutation battery covered the pg refactor that no longer exists. The five mutants it killed are moot; the three that matter — the `??=` pin weakened to `=`, the pin removed, and the rejects gauge reporting active — are all on the bulkhead half and still die. Re-verified after this revert: typecheck 0 errors, prettier clean, 109 files / 1893 tests green. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(metrics): delta re-audit round — the pg test was VACUOUS, and the new guard had no test A blind delta re-audit of the previous two commits returned two blockers, both of them the half-revert signature: the test offered as the pin for the reverted pg change asserted nothing, and a comment claimed coverage that no assertion provided. 🔴 THE INVERTED PG TEST WAS VACUOUS — it asserted about code that never ran. `clearCrossGraphState` reset the bulkhead pin and the shared registry but NOT `global.pgGaugeInitialized`. The first test to import prom/client set that flag, so every later import took the early-out and the pg block never executed. Two mutants proved it, both SURVIVING a fully green run: adding `registers: [instrumentationRegistry]` to a pg gauge (the exact "future well-meant move" the comment said the test would catch), and disabling the whole pg block with `if (false)`. The test could not tell "deliberately on the default registry" from "deleted". Resetting the flag alone does not fix it — the re-audit found that too. `prom-client` is EXTERNALIZED, so `vi.resetModules()` hands every "graph" the same module instance and the same default `client.register`; re-running the pg block then throws "already registered" and reds four tests on unmutated code. Both must be reset together, so `clearCrossGraphState` now also clears the default registry. That is worth knowing on its own: this suite's two "graphs" share one default registry, so no test may infer graph identity from it. Written into the comment. The test now pins BOTH halves — present on the default registry (proving the block RAN) and absent from the shared one (the invariant). Without the first half the second is satisfied by a deleted block. 🔴 A NINE-LINE COMMENT CLAIMED TRANSPOSITION COVERAGE THAT DID NOT EXIST. The revert deleted the value assertions but left the pairwise-distinct fixture and a comment reading "EVERY ONE OF THESE TWELVE NUMBERS IS DISTINCT, AND THAT IS THE WHOLE POINT… Keep them distinct if you touch this." Setting all 24 numbers to 0 — the exact configuration the comment warns against — SURVIVED. Value assertions are restored against the default registry, so the apparatus and its comment are true again. Battery re-run, baseline green either side: A-M4 pg gauge moved to instrumentationRegistry ... KILLED (was SURVIVED) A-M3 whole pg block disabled `if (false)` ........ KILLED (was SURVIVED) A-M1 all 24 fixture numbers -> 0 ................. KILLED (was SURVIVED) T1 write_total reads pgDbRead ................... KILLED PC pin `??=` weakened to `=` .................... KILLED 🟡 THE SCRAPE GUARD HAD NO TEST — added in response to an audit finding, and nobody had watched it go red. New suite `metrics-endpoint-registry-failure.test.ts`, 5 cases, mirroring the sibling Prisma-failure harness. It plants a gauge whose collect() really throws rather than stubbing `metrics()`, so it walks the production rejection path. Verified red: dropping the guard and awaiting the registries directly fails exactly the 3 🔴 cases (5 passed -> 3 failed | 2 passed); the positive control and the seed test correctly stay green, since neither depends on the guard. 🟡 THE FAILURE COUNTER WAS DROPPED BY THE FAILURE IT COUNTS. It lived on `client.register`, so a default-registry rejection removed that whole block — counter included — leaving a permanently absent series exactly where a rising one is expected. Moved to `instrumentationRegistry`; one test now asserts a default-registry failure is reported in the SAME response that lost the block. Residual blind spot documented, not hidden: an instrumentation-registry failure still loses it, but that also removes every instrumentation metric at once. 🟡 THE RECOVERY PATH COULD THROW OUT OF THE GUARD. `registryScrapeFailures.inc` inside the catch was unguarded while the module-scope seed of the same counter was wrapped for exactly that reason. Unchecked `as` cast + a labelset collision would have thrown out of the block whose job is to prevent a 500. 🟡 A HEADING FALSIFIED BY THE REVERT: "WHY EVERY GAUGE BELOW USES registerInstrumentationMetric" sat above nine gauges that use the flag. Corrected, along with a cross-repo "this PR" reference a civitai reader cannot follow, and the fixture comment's description of a code shape the revert removed. Verified: typecheck 0 errors; prettier clean on 6 files; 110 files / 1900 tests green; test:lint-rules 243/243. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: close the four cosmetic findings from the delta re-audit - Restore the blank line before the Buzz-escrow JSDoc that the pg revert swallowed, so the reverted block is now blank-line-identical to origin/main (verified: zero whitespace-only hunks remain in the diff for that file). - Restore the paragraph break the single-key note ran into, so two separate arguments stop reading as one run-on block. - "Destructured once" described two property reads; say what the code does. - A backtick standing in for an apostrophe inside a single-quoted string. No behaviour change. Verified: prettier clean, 66 files / 1208 tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(metrics): round-3 audit — unbreak two unrelated suites, and a test that passed on residue Round 3 found a REGRESSION I shipped in round 2, reproduced independently by CI. 🔴 MOVING THE COUNTER LOOKUP TO MODULE SCOPE BROKE TWO UNRELATED SUITES. `src/__tests__/setup.ts` wholesale-mocks `~/server/prom/client` and omitted `instrumentationRegistry`. While that lookup sat inside the handler body nothing noticed; hoisted to module scope it throws during module EVALUATION, so any suite that merely IMPORTS the metrics page dies — `No "instrumentationRegistry" export is defined on the "~/server/prom/client" mock`. Measured per-ref, each in its own run: `f7a6ccc85f` 5 passed · `497876f896` 3 failed | 2 passed · `origin/main` 5 passed. Casualties were metrics-endpoint-seeds-substitutions and metrics-endpoint-seeds-bitdex-feed-serve — both import-only suites, which is exactly why every metrics-SPECIFIC suite stayed green: they each mock the module themselves. The blocking `Unit tests` job was red with the same error at the same line. Fixed in one place: the mock now supplies a REAL throwaway `Registry`, not a stub, because consumers call `.metrics()`, `.getSingleMetric()` and register into it — a vi.fn() stub would let a suite assert against a registry that never held anything. 🔴 MY GETTER-BASED pgDb MOCK RED-LINED A REPO-WIDE GATE. `pgDbMock.parity` scans `vi.mock` factories for the literal `name:` spelling, so `get pgDbRead()` — which does supply the export — read as "missing: pgDbRead, pgDbReadLong, pgDbWrite". Introduced in round 1 and missed by two audits. The factories are back to literal properties; the absent-pool case uses `vi.doMock` with its own COMPLETE factory rather than mutating a getter. I did not loosen the gate to fit my code. 🟡 THE SEEDING TEST PASSED ON RESIDUE. It ran last, and by then two earlier cases had incremented both labels on a module imported once per file — so deleting both `inc(…, 0)` calls SURVIVED the full-file run and failed only in isolation. The seeded-at-0 assertion moves into the POSITIVE CONTROL, which runs first, where it is the only place it can mean anything. The trailing case now asserts something the first cannot: both label series are still rendered while one registry is failing. 🟡 THE FIXTURE HAD A LIVE HALF AND A DEAD HALF. `beforeEach` overwrote `pools` from `POOL_DEFAULTS`, so the twelve numbers in the `pools` initializer were never read — zeroing them SURVIVED while zeroing the copy was caught, and the nine-line "keep these distinct" warning sat above the dead half. One declaration now, mutated in place. 🟢 Also: the absent-pool case covered 2 of the 3 labelled gauges (`?? 0` -> `?? -1` on the idle guard SURVIVED); the new suite un-mocked prom/client without mocking pgDb, so a real `pgDbRead.connect()` fired once per run; and a docblock named the wrong tests. Battery re-run over BOTH suites, baseline 15/15 green either side: M1 scrape guard removed .............. KILLED (3) M2 seeding inc(...,0) removed ........ KILLED (1) <- SURVIVED before this round M3 counter back on client.register ... KILLED (3) M4 POOL_VALUES all zeroed ............ KILLED (2) <- SURVIVED before this round M5 pg gauge -> instrumentationRegistry KILLED (2) M6 whole pg block if(false) .......... KILLED (3) M7 idle labelled guard ?? 0 -> ?? -1 . KILLED (1) <- SURVIVED before this round PC pin `??=` weakened to `=` ......... KILLED (5) Verified: typecheck 0 errors; prettier clean on 7 files; 123 files / 2034 tests green; test:lint-rules 243/243. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
6c9904b389 |
fix(tests): unbreak the unit suite on Windows — 16 failures across 7 files, and two of them could never pass
Measured on a Windows checkout: 7 files / 16 tests fail identically before and after any local
change, so every Windows developer has to hold "these are expected" in their head to read a suite
result. 868kubhjr tracked four of them; the set had grown to seven.
Five are the separator bug that ticket describes — `path.relative` yields backslashes and the
assertion compares against forward-slash literals — fixed with `.split(path.sep).join('/')`, the
prescribed shape. `standalone-boot-graph` is the same fault in a Node error string, so it matches
`/esm[\/]index\.js/` instead of a literal.
Two were NOT cosmetic, and would have kept failing after the separator fixes:
* `appListingStatChips` read `new URL(…).pathname`, which is `/C:/…` on Windows, so `readFileSync`
resolved it against the drive root as `C:\C:\…`. Now `fileURLToPath`.
* `dev-server-daemon-port` spawned `await import("C:\…")`, which node rejects with
ERR_UNSUPPORTED_ESM_URL_SCHEME — it drives a child and asserts exit 0, so on Windows it could
never observe the property it claims. Now `pathToFileURL`. Verified the harness is live again:
the real daemon with a bad port exits 0, a deliberately throwing module exits 1.
`source-nul-bytes` needs symlinks, which Windows refuses without SeCreateSymbolicLinkPrivilege.
Junctions need no privilege and the walk cannot tell them apart (`lstat().isSymbolicLink()` is true,
dangling ones included), so the three controls still run rather than skipping.
`typecheck-apps` needed the script itself. `spawnSync` skips PATHEXT so a bare `pnpm` is ENOENT on
Windows, and naming `pnpm.cmd` is EINVAL — node refuses to spawn a .cmd without a shell. Hence
`shell` on win32 only; ubuntu CI takes the false branch and is byte-identical.
That shell brings a hazard, so it does not arrive alone: `shell: true` applies NO per-argument
quoting, and `app.name` is read raw from `apps/*/package.json`. A name containing `&` ends the
command under cmd.exe, so cmd returns the trailing command's exit code and a genuinely-red typecheck
reports 0 — reproduced, and precisely the reports-success class this script exists to close. Names
are now validated with a hard error.
The suite's fake pnpm answered by exit code alone, so it could not see WHAT the script asked pnpm to
run: mutating the spawn to `--filter totally-wrong-<dir> run lint` left all tests green. It now
echoes argv and one case asserts it, which also pins argv fidelity through cmd.exe.
Windows unit suite: 16 failures -> 0. Nothing changes on ubuntu CI.
Closes 868kubhjr
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
69b5ce5497 |
fix(typecheck): typecheck scripts/ — 15 non-test files now covered, scripts/__tests__ excluded like src/ (#4189)
* fix(typecheck): close the scripts/ blind spot — 31 files now checked, 5 quarantined behind a ratchet The root tsconfig named `scripts/local-dev/*.ts` in BOTH `include` and `exclude`. Exclude wins, so nothing under `scripts/` was typechecked at all — not even the one directory the include list appeared to name. Measured on |
||
|
briant
|
15c1408d3d |
fix(db-schema): the enum generator emitted unformatted output, so db:check-generated failed on a clean checkout
`pnpm run db:check-generated` has been failing for anyone who ran it, with no change of their own. `scripts/prisma-enum-generator.mjs` wrote each type alias on one line and never formatted the result, while the committed `enums.ts` is prettier-wrapped at the repo's printWidth of 100. Generator output could therefore never equal the committed file, and `postinstall` runs `db:generate` — so every `pnpm install` left the file dirty and every run of the check gate went red. CI does not run that gate, which is why this survived; the only person who hits it is someone following "Before Committing" step 5. The generator now formats with the resolved prettier config before writing. That is scoped to the one file this generator owns rather than a prettier pass over `packages/civitai-db-schema/src`, because the neighbouring generated files are NOT prettier-clean — `models.ts` is emitted with double quotes against a `singleQuote: true` config and passes the check exactly as it is. Formatting the directory would reformat it wholesale for no reason. With the generator fixed, the ~90 lines of churn collapse to one real staleness: `CollectionItemRejectionReason` was committed as raw generator output in `1ad8ae3443` while every other alias in the file was wrapped. That line is now regenerated, and `db:check-generated` passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
b8f84204d7 |
fix(auth): fix the two type errors gating apps/auth, and bring it into the CI typecheck gate (#4188)
apps/auth was the last app excluded from `scripts/ci/typecheck-apps.mjs` (#4156), on two pre-existing errors. Both are fixed at the root, not silenced, and the exclusion is gone — the gate now covers all 7 apps. providers.ts:165 — Parameter 'p' implicitly has an 'any' type The stub entry's key is a COMPUTED property (`['stub' as ProviderId]`) whose type is the whole ProviderId union rather than one literal, so TS cannot match it to a member of `Record<ProviderId, ProviderDef>` and drops contextual typing for the value. `satisfies ProviderDef` restores it. Measured, both arms: with `satisfies`, misspelling a required field (`scope` -> `scopes`) is caught; with it removed, the same misspelling is silently accepted and only the implicit-any resurfaces. So the whole entry was unchecked against ProviderDef, not just that one parameter. establish-session.test.ts:100 — Property '_store' does not exist on type 'never' The Cookies stub was cast `as never` to satisfy establishSession's parameter, but `never` has no properties, so reading `_store` back in an assertion was itself an error. Cookies has exactly five members, so the stub now implements all of them and needs no cast at all. Removing the exclusion breaks the guard suite, which is the real work here: six of its eight cases put an `auth` app in every fixture purely to satisfy the stale-exclusion guard, so an empty map made them fail for reasons unrelated to what they test. Rather than delete those guards, the exclusion map is now a parameter of an exported `runTypecheckApps({ excluded })`, and the tests drive it with a synthetic app. Deliberately a function parameter and not an env var: an env-var override would be a live way to un-gate an app in CI, which is the failure class the script exists to close. All ten cases now pass regardless of what the shipped map contains. Also neutralises the "six apps" counts in the script header and workflow comment, which this change would otherwise make wrong, and applies prettier to two pre-existing unformatted lines in the touched files. |
||
|
Zachary Lowden
|
1891ce76aa |
fix(dev-server): the daemon never read DEV_DAEMON_PORT, so setting it broke the CLI instead of moving the daemon (#4181)
* fix(dev-server): the daemon never read DEV_DAEMON_PORT, so setting it broke the CLI instead of moving the daemon The variable was read where the client decides what to CONNECT to and ignored where the daemon decides what to LISTEN on. `DEV_DAEMON_PORT=9555 cli.mjs status` therefore pointed the CLI at :9555, spawned a daemon that bound :9444, and could not reach it — the daemon has always accepted `--port`, the spawn simply passed no arguments and the daemon read no environment. Four files each decided the port for themselves and two of them were wrong: cli.mjs and scripts/test-unit-run.mjs read the variable, console.mjs hardcoded 9444, and daemon.mjs saw only argv. That is not a bug any one of them contains — it is a bug in the set, so the number now lives in exactly one module and all four resolve it there. A daemon a client spawns inherits that client's environment, so both ends read the same variable through the same function; an explicit `--port` still wins for a daemon started by hand. resolveDaemonPort also refuses a value that is not a port rather than handing back parseInt's NaN, which used to reach a URL as `http://127.0.0.1:NaN` and fail a long way from its cause. Verified by reproducing the reported path, not by reading the code. On pre-change code with DEV_DAEMON_PORT=19461 the daemon logged `Daemon port: 9444`, emitted no ready line, and nothing ever listened on 19461. After the change it reports and binds 19461, and the pid it reports is the child that was spawned — a port that answers proves a listener, not THIS listener. Six isolated mutants, each killed by its own named assertion: daemon ignores the environment -> both behavioural cases daemon ignores --port -> the --port precedence case resolver stops validating -> the rejects-a-non-port case console.mjs re-hardcodes 9444 -> both halves of the ledger test-unit-run.mjs re-hardcodes -> both halves of the ledger the default drifts to 9445 -> the default case console.mjs is a TUI with no end-to-end case here, which is why the ledger exists: it fails when the set of files deciding the port grows or shrinks. Closes ClickUp 868kuaa4e. * fix(dev-server): the audit round — the test deleted the pid file it was written to protect, and the ledger could not see the set grow 🔴 The blocking one. `POST /shutdown` replies 200 and only THEN schedules `unlinkSync(pidFile); process.exit(0)` on a 100 ms timer, so the helper that saved and restored the developer's `daemon.pid` restored it ~100 ms before the daemon deleted it. `scripts/**/*.test.ts` is in the `unit` project, so every `pnpm test:unit:run` removed `.claude/skills/dev-server/daemon.pid` — and the file is only written at daemon START, so it did not come back. That breaks the recipe SKILL.md itself gives for checking the daemon's interpreter, and makes `cli.mjs shutdown`'s cleanup a no-op. `shutdown()` now waits for the port to stop answering before returning. The ledger scanned a FIXED list of four readers, so it could not detect the set GROWING — which the PR description claimed it did. A fifth hardcoded 9444 already existed while the test was green: `.claude/hooks/check-writable.mjs` baked it into the dev-port regex, so `DEV_DAEMON_PORT=9555` silently switched that nudge off for the daemon, the one long-lived server it most exists for. The ledger now walks the tree, so a file that does not exist yet is covered, and the hook reads the port from the module. Verified with a negative control: with DEV_DAEMON_PORT=9555, `curl :9555` nudges and `curl :7777` stays silent. The ledger's assertions were also spelled rather than structural. `includes('daemon-port.mjs')` is satisfied by a COMMENT — and this file's own prose names the module in several — and `includes('resolveDaemonPort(')` is satisfied by any call whatever is done with the result, so `resolveDaemonPort() + 1` passed all of it while console.mjs has no behavioural test. Both clients now take a whole URL from `resolveDaemonUrl()` and do no arithmetic on a port; the assertions pin an import line and that call. Three more the audit found: - `scripts/test-unit-run.mjs` lost its "never leave a caller unable to run tests" guarantee. Resolution moved inside `runQueued` but OUTSIDE its try, and `runQueued` is un-awaited, so a malformed DEV_DAEMON_PORT became an unhandled rejection and no tests ran. Measured against base: base printed "running directly", HEAD died. Restored, with the outer `.catch()` as the general net. - `--port` still open-coded `parseInt`, so the HIGHER-precedence input was the unvalidated one — `--port abc` gave ERR_SOCKET_BAD_PORT, which via cli.mjs is invisible (detached, stdio ignored) and reads only as "Failed to start daemon". Both inputs now go through `parsePort`, which names the input it is complaining about. - Resolving at module load made a bad DEV_DAEMON_PORT throw on merely IMPORTING daemon.mjs, which would have broken collection of the port-reservation suite. Resolution moved into `parseArgs`. Nits: SKILL.md said `cli.mjs:69`, the line is 70 — my own earlier check of that number had gone stale under a later edit. Out-of-range is now a different message from unparseable ('0' is a number, just not a port). The 9444 scan uses a word boundary so 19444 is not a false positive. Seven mutants, each applied alone, tree restored and cmp-verified between: shutdown() stops waiting -> the pid-file case a NEW file hardcodes 9444 -> the ledger (the growth case) the hook re-hardcodes 9444 -> the ledger console.mjs does resolvePort() + 1 -> the client-address case the targeted fallback is removed -> the degrade-to-direct case --port back to parseInt -> the argv validation case port resolved at module load -> the import-safety case 🔴 One of them SURVIVED the first run and the fix is the point: the fallback case asserted the generic phrase "running directly", which BOTH guards on that path print, so deleting the targeted guard left the suite green — the mutant died to the other guard. The assertion now names the specific message. scripts/__tests__ 324 passed. typecheck 0 errors. Hook selftest green. * fix(dev-server): round 3 — the last round's hook fix un-guarded the default port, and its catch would have double-run the suite Both regressions were introduced by the previous commit, not pre-existing, and a delta re-audit of 7dd65a9d62..85cb922567 found them. 🔴-in-effect: `DEV_DAEMON_PORT` REPLACED the guarded port instead of adding to it, so setting it un-guarded the shared daemon. Reproduced through the hook's real stdin contract: with the override set, `curl http://localhost:9444/...` went from ask to allowed, and the hook's own selftest went from `all green` to `1 FAILURES`. SKILL.md documents the override as standing a daemon BESIDE the shared one — both are live, so both need guarding. `daemonPortsGuarded()` now returns the union, and the selftest is green both plain and under the override. The other one is subtler and worse in effect. The outer `.catch()` I added wrapped the whole `runQueued` lifecycle, not just queue acquisition — so a socket dropped mid-poll abandoned a run the daemon had ALREADY ACCEPTED and started a second, unqueued full suite beside it. That defeats the serialisation this script exists for, and the file had already decided the opposite for the status-code form of the same condition (`Lost contact with the test queue` -> exit 2). It now tracks whether the run was accepted: before acceptance it degrades to a direct run, after acceptance it exits 2. `err?.message`, because a non-Error throw printed `(undefined)` and a null throw raised inside the handler — back to the unhandled rejection the catch exists to prevent. Also from the re-audit: - The port scrape in the hook is a dependency on one line's formatting in another file and nothing tested it. A vitest case now drives the real `unboundedDevRequest`, with a negative control on a port the skill does not use. Breaking the scrape regex now fails two tests; it used to fail none. - `console.mjs` had no behavioural coverage, and every structural assertion was walkable by resolving the URL correctly and then drifting it. It now runs for real against a stub daemon on an ephemeral port — via `--tail`, since the dashboard refuses to start without a TTY and exits before contacting anything. - `--base-dev-port` was still `parseInt`, which made "argv gets the same validation the environment gets" false about half of argv. - The port range's upper bound was untested: fixtures were 0 and 70000, so 65535 could drift to 65536 unnoticed. Now pinned ON the boundary. - SKILL.md's `console.mjs:88` — the previous commit fixed the cli.mjs citation and moved console.mjs's line to 89 in the same change. Now 89. - The module header claimed the scan covers "the whole skill ... anywhere". It covers three roots and four extensions; prose and test files are deliberately out of scope. Said so instead. 🔴 The ledger caught ME during this round: a comment I wrote explaining the un-guarding bug spelled the port, and `spells the port in exactly one source file` went red. That is the guard working on its author — reworded, not exempted. Round-3 battery, each mutant alone, tree cmp-verified between: outer .catch() deleted -> the accepted-run case accepted-check removed -> the accepted-run case hook override replaces default -> the union case hook scrape regex broken -> both hook cases upper bound 65535 -> 65536 -> the boundary case console.mjs drifts the URL -> the behavioural case + the ledger --base-dev-port back to parseInt -> the base-dev-port case All killed. One deliberate survivor: a copy OUTSIDE the three scanned roots, which is the documented scope and is now stated in the header rather than overclaimed. 330 passed. typecheck 0 errors. Hook selftest green plain and under override. * style: prettier the added test file — the CI gate checks ADDED files and I never ran it `ESLint + Prettier (changed files)` went red on the round-3 push for one reason: `scripts/__tests__/dev-server-daemon-port.test.ts` was not prettier-formatted. Four lines, all wrapping. Reproduced the gate's exact scope locally rather than guessing at it — it runs `prettier --list-different` over ADDED files only, which is why the four `.mjs` files this branch MODIFIES were never checked. Those four are unformatted, and the control says that is not mine: at the merge base `bc74ba06ea`, all four are already unformatted. Reformatting them would rewrite files this change does not own, which is the breadth rule in CLAUDE.md, so they are left alone. Local prettier is 2.8.8, the same version CI installs, so the local format is the one the gate will read. 21 tests still pass. * fix(dev-server): round-4 tidy — the test that proves the console bug was itself destroying the pid file on its red path Follow-ups from the round-3 delta re-audit, which found no 🔴 and confirmed round 3 did not reintroduce a regression. These are the 🟡/🟢 it did find. The console test was not wrapped in `withPidFilePreserved`, and the exposure is specifically on the FAILING path: on green the stub answers `/`, so the console never starts a daemon. Under the mutant the test exists to catch, it cannot reach the stub, falls through to its own `startDaemon`, and overwrites `daemon.pid` with a dead pid — the test that proves the bug also damaged the thing the rest of the file is careful about. Controlled both ways: with the drift mutant applied the test now goes red AND the pid file's md5 is unchanged. The port scrape in the hook is anchored on `export const` again. Dropping the anchor was justified as surviving a reformat, and that reasoning was wrong — a reformat does not rewrite `export const NAME =`. What it actually bought was letting the FIRST match anywhere in the file win, comments included: a `// historical: DEFAULT_DAEMON_PORT = 9999` line above the declaration made the hook guard 9999 and stop guarding the real port. Controlled: that same line now yields 9444. Three comments that no longer described their code. `test-unit-run.mjs` had the previous round's two lines left verbatim above the block that replaced them, so the same sentence appeared twice. The hook still said the port "is overridable via DEV_DAEMON_PORT" — the exact semantics round 3 removed, since the set is now additive — and its "resolved lazily" note sat above `daemonPortsGuarded`, which is neither lazy nor cached; the laziness is in `devPorts`. Numeric line references (`:123`, `:128`) replaced with named ones. Both had already rotted by three lines. This is the same class as the SKILL.md citation fixed last round, and a line number in a comment will rot again — a quoted message will not. Named the residual hole rather than implying it away: the daemon enqueues INSIDE the response write, so the slot is taken before the client can observe it. Lose the response between those points and `accepted` is still false while the run is queued. That window needs an idempotency key on the enqueue, not a flag here. Not changed, deliberately. `.claude/**/*.mjs` fails `prettier --check` at the merge base as well as here, so it is pre-existing and reformatting it would rewrite files this change does not own. And `scripts/__tests__/*.ts` is typechecked by nothing — `tsconfig.json`'s `include` omits it — so this PR's "typecheck 0 errors" says nothing about the new test file's annotations. Both are stated rather than quietly folded in. 330 passed. Hook selftest green plain and under the override. Test file prettier-clean. |
||
|
Zachary Lowden
|
23cecb57c0 |
ci: run per-app typecheck scripts in CI (#4156)
* ci: run per-app typecheck scripts in CI
The root tsconfig.json include is:
scripts/local-dev/*.ts src packages/*/src tests .next/types/**/*.ts
No apps/ entry, so `pnpm run typecheck` does not reach any sibling app.
Each app defines its own typecheck script but none were run by any CI job.
Six of seven apps pass today; wire them up as steps in the existing `apps`
(App unit tests) job, which already has the workspace installed.
app script result
event-engine tsc --noEmit -p tsconfig... clean
notifications tsc --noEmit clean
orchestrator-gateway tsc --noEmit clean
storage tsc --noEmit clean
creator-studio svelte-check --tsconfig ./... clean
moderator svelte-check --tsconfig ./... clean
auth svelte-check --tsconfig ./... 2 errors + 1 warning
apps/auth is excluded from the CI wiring. Its pre-existing errors:
src/lib/server/auth/providers.ts:165
Parameter 'p' implicitly has an 'any' type.
src/lib/server/auth/__tests__/establish-session.test.ts:100
Property '_store' does not exist on type 'nev
src/routes/login/+page.svelte:28
state_referenced_locally (warning)
Ticket: 868kt8pfu
* ci: make the app typecheck prove it ran, and report every failing app
Review follow-up on the six `pnpm --filter <pkg> run typecheck` steps. Two ways they could
report SUCCESS while checking nothing or hiding work, plus a blame problem.
🔴 `pnpm --filter <name> run <script>` EXITS 0 WHEN THE FILTER MATCHES NOTHING. Measured on
pnpm 10.28.1: a bogus package name prints "No projects matched the filters" and returns 0.
Six hardcoded package names are six chances for a rename to turn a gate into a green no-op
— the same shape as `prettier --check "$FILES"` reporting "All matched files use Prettier
code style!" across zero files. Nothing in the six steps could notice.
🔴 A NEW app under `apps/` is simply absent from a hardcoded list. The gate stays green and
the app is unchecked — this PR's own hole, reopened by the next person to add an app.
So the app set is now a LEDGER READ FROM DISK: every `apps/*` with a `typecheck` script must
run, each run must be proven to have selected a real package, and `auth`'s exemption is an
explicit entry that hard-errors if it goes stale. Adding an app wires it automatically. This
mirrors `scripts/ci/assert-workspace-suites-ran.mjs`, which exists for the same reason on
the vitest side.
Failures are COLLECTED rather than fatal on the first. Six sequential steps abort the job at
the first red app, so a shared type change breaking four of them reports one.
The job is renamed `App unit tests` -> `App unit tests + typecheck`. The typecheck steps were
added under the old name, so a type error would have rendered in the checks list as a failing
unit-test job, pointing the reader at the wrong suite. Safe: neither `main` nor `release` has
any required_status_checks — re-verified 2026-08-20 against the branch-protection API rather
than inherited from the note already in this file.
Kept in the `apps` job on purpose: `pnpm install` is the expensive part and this job has
already paid for it. A matrix would give parallel legs and per-app check names for the price
of six installs; the rename plus collected failures buys most of the legibility for free.
Eight guard tests in `scripts/__tests__/typecheck-apps.test.ts`, each driven against a stub
apps/ tree with a fake `pnpm` on PATH so the no-match and failure paths are exercised for
real: happy path, stale filter caught, red app, failures collected not masked, empty
discovery refused, new app auto-wired, stale exclusion hard-errors, auth excluded while the
rest still run. All 8 pass; the first draft had six failing for the wrong reason (fixtures
omitted `auth`, so the stale-exclusion guard fired first) — fixtures fixed, not the guard.
Two things deliberately NOT changed:
- The Svelte apps' bare `typecheck` script is fine as-is. Their tsconfig extends
`./.svelte-kit/tsconfig.json`, absent from a fresh checkout, which looks like a vacuous
green waiting to happen. It is not: `"prepare": "svelte-kit sync"` runs during
`pnpm install`. And if it ever stops, svelte-check FAILS LOUDLY rather than passing —
verified by moving `.svelte-kit` aside, which gave exit 1, "Cannot read file
.svelte-kit/tsconfig.json", `1 FILES 1 ERRORS`. An earlier review comment of mine claimed
it would go silently vacuous; that was wrong, and switching to `check` is unnecessary.
- `scripts/__tests__/` is outside every typecheck program (root `include` carries only
`scripts/local-dev/*.ts`), so this new test file is unchecked — as are the 12 already
there. Not introduced here; it is the `scripts/` half of this ticket's widening.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* style: prettier the two new files
The `Prettier (added files)` step is BLOCKING by design — a new file has no pre-existing
findings, so holding it to the rules is free. Both new files failed it and I did not run the
formatter before pushing. Reproduced locally with `prettier --list-different` (both listed),
fixed with `--write`, and re-ran the 8 guard tests after the reformat: still 8/8, so the
formatting did not disturb the fixture strings or the fake-pnpm heredocs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: DevPod Agent <agent@devpod.local>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
481582d969 |
flake: own the dev toolchain, guard the pins, one command to a running app (#4107)
* feat(flake): make the Nix flake own the dev toolchain, and add one command to start The flake shipped nodejs_22 while package.json declares engines.node ">=24.0.0 <25", .nvmrc pins 24.19.0 and the Dockerfile builds production on node:24.19.0-alpine3.24. A NixOS developer was running a major the repo does not support, and nothing said so. Toolchain: - node and pnpm are now DERIVED from .nvmrc and package.json's packageManager rather than named twice. .nvmrc is treated as the authority because it is what every workflow's actions/setup-node reads and what the Dockerfile tracks. - flake.lock moved 2026-04-23 -> 2026-08-18 (117 days). At that rev nodejs_24 is exactly 24.19.0, which is what made agreeing with .nvmrc possible at all. - pnpm now comes from `pnpm_10`, not the unversioned `pkgs.pnpm`. At the new rev the unversioned attribute resolves to 11.21.0 -- a major bump that rewrites pnpm-lock.yaml -- so this bump would otherwise have shipped pnpm 11 to every dev shell silently. - postgresql_16 -> postgresql_17, matching the primary `db` container. The postgres/redis/clickhouse entries are CLIENTS for the compose-hosted servers; that is now stated in the file instead of left to be guessed. - npm_config_manage_package_manager_versions=false. Measured: without it, pnpm downloads and re-execs the exact version from the packageManager field, so the flake's pnpm pin was being defeated at runtime (`pnpm --version` returns 10.28.1 with the var unset, 10.34.5 with it set). Guards (`nix flake check`, 4 checks): - toolchain-pins: the flake's node must satisfy engines.node and equal .nvmrc, and its pnpm must share a major with packageManager. Deliberately does NOT re-check the .nvmrc/Dockerfile/engines triangle -- node-version-consistency.test.ts already owns that, and a predicate open-coded twice starts disagreeing. - prisma-pin: re-derives the resolved @prisma/client AND its engine commit from pnpm-lock.yaml and compares them to the values flake.nix hardcodes. These were correct but unguarded: package.json declares `^6.3.0`, a caret range, so a routine lockfile refresh moves the client while the flake's engines stay put, and the failure surfaces at runtime in every dev shell. - pin-guards-selftest: breaks each pin on purpose and requires the guard that owns it to fire while the others stay silent. - dev-scripts: builds the shell entrypoints, which is what runs their shellcheck. (`nix flake check` builds checks.* but only EVALUATES packages.*, measured.) Entrypoints: - `nix run .#dev` - docker preflight, submodule, .env.development, compose up, wait for postgres, pnpm install, then `next dev`. Every step idempotent and non-destructive; migrations and seeding stay opt-in. - `nix run .#dev-server` - runs the dev-server CLI on the flake's node. The daemon re-execs itself with process.execPath, so whichever node starts the CLI is the node it runs on until it is restarted. - `nix run .#doctor` - the same pin checks against the working tree. Compose project is pinned to `civitai` so every worktree shares the one local stack instead of each spawning a duplicate that fails on the port binds. * fix(flake): give `nix run` the same env as the dev shell, not just the shell Found by running the bootstrap on a genuinely clean worktree rather than reasoning about it. `mkShell`'s `env` applies to `nix develop` only, so both values it carried were absent from `nix run .#dev`: - `pnpm install`'s postinstall runs `prisma generate`. Without PRISMA_QUERY_ENGINE_LIBRARY et al, prisma tried to fetch an engine for platform `linux-nixos` and the bootstrap died on `404 ... /linux-nixos/libquery_engine.so.node.sha256`. - pnpm re-execed itself as 10.28.1 from the packageManager field even though PATH pointed at the flake's 10.34.5, so the app reported a pnpm the flake had not pinned. The env is now one attrset (`devEnv`) rendered two ways: `env` for the shell and an `export` preamble for the apps, so they cannot drift. `nix run .#dev-server` gets it too -- the daemon runs `pnpm install` / `db:generate` on its own when it sees the lockfile move, which would have hit the identical 404. * docs: describe the toolchain the repo actually has, not the one it used to Every claim below was checked against the code before rewriting, and the measurements are quoted where they are load-bearing. README.md - "Node.js (version 20 or later)" -> 24.19.0, with .nvmrc named as the authority. - `make init` was DEAD, not merely awkward: it ran `npm i`, and package.json's `preinstall` runs `only-allow pnpm`, which exits 1 under an npm user agent (measured, with the pnpm-user-agent control exiting 0). Both bootstrap paths the README offered went through it. - MinIO console is on :9001, not :9000 (:9000 is the S3 API). The instructions sent people to the wrong port to mint the keys the next step needs. - `git submodule update --recursive` -> `--init`; without `--init` it is a no-op on a fresh clone, which is precisely when it is being run. - Data Migrations step 1 pointed at `schema.prisma`, which is gitignored and regenerated from `schema.full.prisma` on every `db:generate`, so edits to it were silently discarded. - Adds the Nix path (`nix run .#dev`) and a real non-Nix sequence. - engines.node is ADVISORY, stated plainly: pnpm 10.34.5 under node 26.7.0 against ">=24.0.0 <25" prints `WARN Unsupported engine` and exits 0. An earlier draft of this very README claimed it refuses. It does not, and that is the reason the drift survived so long. Makefile - `npm i` -> `pnpm install` (see above). `npm-install` kept as an alias. - `gen-prisma` ran a bare `prisma generate`, which reads the gitignored slim schema that does not exist yet on a fresh clone; now `pnpm run db:generate`, which generates it first. - `dev` ran bare `cross-env`/`next`, requiring the caller to put node_modules/.bin on PATH by hand; now via `pnpm exec`. - `docker-compose` (EOL v1) -> `docker compose`. - COMPOSE_PROJECT_NAME pinned to `civitai`. Reproduced first: `make start` in a worktree died with `Bind for :::15434 failed: port is already allocated` because compose named the project after the directory. .envrc.example (new, tracked) + .gitignore - `.env*` matched `.envrc` too, so nothing tracked in the repo mentioned the flake at all -- the only reference was a line in CLAUDE.md filed under worktree hygiene. Placeholders only; the real .envrc stays ignored. .claude/skills/dev-server/SKILL.md - The skill said nothing about node. The daemon is spawned with `process.execPath` (cli.mjs:66, console.mjs:87) and hands its env to every `next dev` it supervises, so the first shell to run a CLI verb decides the node for everything, indefinitely. Measured on this box: daemon on 26.7.0, with no pnpm on PATH at all. Documents `nix run .#dev-server` and how to check. - `npm run dev:daemon` -> `pnpm run dev:daemon`, in a repo that bans npm. src/__tests__/node-version-consistency.test.ts - Comment-only. It said flake.nix "is on a different major" and could not be aligned because the pinned nixpkgs had no Node 24 this new. Both halves are now false, and a comment a maintainer might act on is worth correcting. Also: docs/pnpm-migration.md's "Node.js 18.x or later"; the generated-header line in scripts/generate-slim-schema.js telling readers to run `npm run db:generate`; CLAUDE.md's local-dev section (no node version, no services) and its stale "flake's 22.22.2" figure. NOT changed, because it could not be exercised here: the devcontainer pins typescript-node:1-22 (Node 22, outside engines.node). Flagged in README with the tag to use -- there is no `1-24`, the template major moved on, so `3-24`. * docs(flake): the four postgres containers are not all one version prisma-pit and db are postgres 17; notification-db and logical-db are 15. The comment justifying postgresql_17 read as though they were uniform, which would have made the next person's version decision from the wrong premise. * docs: keep the non-Nix path the default, demote the flake to optional The flake is used by one maintainer. Everyone else uses Docker + nvm, and that has to stay the path a contributor lands on. The previous revision inverted that: README's Installation section led with "With Nix (recommended...)" and titled the standard path "Without Nix" — framing the majority workflow as the fallback. CLAUDE.md opened "From nothing to a running app, one command:" with `nix run .#dev`, and the dev-server skill led its fix with "Start it through the flake and this cannot happen". None of that made Nix *required* — verified: `.github/` is untouched by this branch, no workflow references Nix (the apparent hits are substrings of `eslint-unix.json` and `--format unix`), and `nix flake check` is not wired to any CI gate. It was purely an ordering-and-emphasis problem, which is the kind that costs a new contributor twenty minutes before they find the section that applies to them. Changes, all editorial: - README: `#### Standard setup` now precedes `#### Optional: Nix flake`, and the Nix section opens with a blockquote saying it is not the supported default, that nothing requires it, and why it exists at all (NixOS has no published `linux-nixos` Prisma engine, so a flake is the practical way to work there). The signals/buzz instructions lead with `docker compose up -d` and mention `nix run .#dev -- --full` parenthetically. - CLAUDE.md: the bootstrap block is now the nvm/docker sequence, labelled as the default path, with the flake shown after it as NixOS-only and explicitly flagged as something not to assume a contributor has. The dev-server step no longer instructs going through `nix run .#dev-server`; it states the requirement (a shell whose node matches `.nvmrc`) and notes the flake does that for you on NixOS. - dev-server SKILL.md: the fix is now stated setup-agnostically — start the daemon from a shell whose node matches `.nvmrc` with pnpm on PATH, which `nvm use` gives you — with the flake wrapper presented as the optional NixOS convenience, and an explicit note that nothing in the document depends on Nix. No behaviour, tooling or gate changes: the Makefile, flake, guards and their tests are untouched by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
f419a64461 |
chore(deps): delete the inert next@16.3.1 patch and correct the comments about it (#4086)
Next 16.3.1 is the first release containing upstream's SVG-loader fix (vercel/next.js#96681) — it ships 'VipsForeignLoadSvg' in the sharp.unblock list itself, in both the CJS and ESM image-optimizer copies. Our local patch was therefore inert: the installed files are byte-identical (sha256) to the published tarball, and the patch does not even apply (its hunk context wants Png immediately followed by Tiff; 16.3.1 has Svg between them). It looked healthy because pnpm silently no-ops an already-applied patch — install exits 0, prints no warning, and still creates a patch_hash= virtual-store directory. Deletes the patch and its patchedDependencies entry (@mantine/hooks untouched), regenerates the lockfile, and corrects three comments that asserted upstream still lacked the entry. The CI guard stays: it asserts the outcome (the installed Next unblocks the SVG loader), so it keeps protecting against a future Next regressing this the way 16.3.0 did. Verified post-removal: guard green, still goes red when the loader entry is stripped from both installed copies, and /api/og renders PNGs on both paths on the preview build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
7826ba9bfb |
fix(dev-server): one exit-code rule for both test-queue waiters, and a log that admits when it was clipped (#4102)
`exitCodeFor` in the queue module exists to stop a signal-killed run reporting a shell 255 -- its own comment says so. `test wait` used it; the wrapper behind `pnpm run test:unit:run` kept a second copy of the rule, `state.exitCode || 1`, which passes the recorded -1 straight to `process.exit`. Measured through a real daemon: a run cancelled mid-flight exited 255, where `test wait` on the same run exited 1. The copy is deleted rather than corrected -- the wrapper now imports `exitCodeFor` and uses it. The queue also kept the last 2000 lines of a run and dropped the rest in silence, so a fragment was byte-for-byte indistinguishable from a whole log. Measured: a child writing 5000 lines produced 1998 through `test wait` with nothing said. The window is unchanged; the drop is now counted (`logsDropped` on the run view, and `dropped` on the log response for callers that fetch logs directly) and both waiters name the number at the moment the verdict lands. `scripts/test-unit-run.mjs` now honours `DEV_DAEMON_PORT` like the CLI already did. That is what makes the verdict testable at all: with the port hardcoded there was no way to stand a stub daemon beside the shared one, which is why a decision this load-bearing had no test and was free to drift. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
c78f85dd5a |
fix(deps): Next 16.3.0 → 16.3.1, and make the compiled-branch gate hard (#3983) (#4075)
* fix(deps): Next 16.3.0 -> 16.3.1, and make the compiled-branch gate hard (#3983) This is the fix for #3983. The defect was in the bundler, not in our source. Turbopack's value analyzer in 16.3.0 models a bare `return someAsyncFn()` tail call as `Promise<Promise<T>>`. That is always truthy, so a caller that `await`s it is analysed as always-true and every statement after the resulting conditional is eliminated as dead code. `isAppListingsEnabled` ends in exactly that shape, which is why `resolveStoreVisibilityScopeUninstrumented` lost two of its three returns, fell off the end, and produced `undefined` for every non-privileged caller — served as the whole catalog on one read path (`?? 'full'`) and as an empty store on the other (`?? 'none'`). Upstream: vercel/next.js#96601 "[turbopack] Collapse nested promises in the analyzer", backported as #96675, shipped in 16.3.1. MEASURED, not inferred. Two production builds of THIS commit on one machine, same Node 24.19.0, differing only in the pinned Next: 16.3.0 async function S(e){if(await p(e))return"full"} 16.3.1 async function w(e){return await c(e)?"full":await y(e)?"public-external":"none"} Both read out of the emitted `.next/server` chunks by source-map attribution and identified by their source neighbour `STORE_SCOPE_FLAGS`, never by minified name. Note the fixed form is a TERNARY — `grep 'return"public-external"'` returns zero on the FIXED build too, which is why the gate reads source maps. `package.json` already allowed 16.3.1 (`^16.3.0`); only the lockfile pinned 16.3.0, so the substance here is the lockfile. The floor is raised to `^16.3.1` so a fresh resolution cannot land back on the broken compiler. `patches/next@…` is renamed and its `patchedDependencies` key updated — that patch is the unrelated libvips/SVG one-liner (vercel/next.js#96681), it still applies cleanly, and 16.3.1 still does not carry the loader entry upstream, so it stays. `--warn-only` is removed from `scripts/assert-compiled-branches.mjs` in the same commit. It existed only because the 16.3.0 build genuinely violated the gate, and a permanently-red gate trains everyone to click through. Keeping the bump and the strictness atomic means the gate's strictness always matches the toolchain: a revert of the bump turns it red instead of silently passing. Verified on this commit: - gate exit 0 (hard, no --warn-only) against the 16.3.1 build - gate exit 1 against a 16.3.0 build of the same tree — watched red - `scripts/ci/assert-next-svg-patch-applied.mjs` OK on both installed copies - unit suite 1149 files / 18,123 tests passed, 0 failed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(build): ship @swc/helpers' module-sync branch into the standalone image (#3983) The bump built clean, passed every source-level gate — unit suites, typecheck, ESLint + Prettier, schema drift, the event-engine pin, and the now-hard compiled-branch gate — and the container could not boot: Error: Cannot find module '.../@swc/helpers/esm/_interop_require_default.js' at ... next/dist/server/require-hook.js code: 'MODULE_NOT_FOUND' Same shape as the defect this PR exists to fix: a correct source tree producing a broken artefact, invisible to everything that reads source. ROOT CAUSE, measured on the published artefact rather than inferred. `output: 'standalone'` does not ship node_modules; it ships the subset @vercel/nft traced. nft resolves a bare specifier under the `require`/`default` conditions. Node (>= 22.10) additionally honours `module-sync` for a CJS `require`. When a package's `exports` map points those two at different files, the build traces one and the running process asks for the other. next/dist/shared/lib/constants.js does `require('@swc/helpers/_/_interop_require_default')`, reached from the generated server.js via `next` -> config.js -> constants.js, i.e. before any application code. The relevant delta is not next itself but next's own dependency: next 16.3.0 next 16.3.1 @swc/helpers 0.5.15 0.5.23 ./_/_interop_require_default {import,default} {module-sync,webpack,import,default} require.resolve() under CJS cjs/...cjs esm/...js Both resolutions were RUN, not reasoned about. nft still traced the cjs file, so the published image carried that package as exactly cjs/_interop_require_default.cjs, cjs/_interop_require_wildcard.cjs and package.json — no esm/ directory at all. Adding only the missing esm/ directory to that exact image, nothing else changed, boots it: "Next.js 16.3.1 ... Ready". FIX. `outputFileTracingIncludes` force-includes BOTH condition branches of EVERY installed @swc/helpers copy — not the one file missing today, because which helper Next requires and which branch each resolver picks are upstream details that move. Globs are version- and hash-agnostic (`@swc+helpers@*`), plus a flat form for a hoisted layout. ~950 KB per copy. Verified on a local production build of this commit: both copies land in .next/standalone with complete esm/ (108 and 105 files) and cjs/, and next's virtual store links the 0.5.23 copy. Attached to three existing API-route keys rather than a `'**'` key. copyTracedFiles unions every entry's traced set into the single .next/standalone node_modules, so one entry carrying it is enough, while `'**'` would make all 572 entries read/parse/rewrite their .nft.json concurrently — 826 MB of JSON in one Promise.all — on a build already tuned against OOM. GATE, because a glob is a silent no-op once it stops matching. scripts/ci/assert-standalone-boot-graph.mjs runs in the Dockerfile's RUNNER stage: the first gate in that file to run against the runtime filesystem rather than the build tree, and the only one that can see this class of defect. It reads the GENERATED server.js for the specifiers that process requires at module scope and loads them in a child rooted at the shipped tree — no package, version, virtual-store path or patch hash hardcoded, so it keeps covering this after the next bump. Exit 2, never 0, when it cannot observe its input. It must run in the runner and not the builder: /app there is byte-for-byte what ships, whereas the builder's complete node_modules sits above .next/standalone on the resolution path and can satisfy a require the image cannot. Watched red and green on real artefacts, not only fixtures: - exit 1 with this exact MODULE_NOT_FOUND against the published broken image; - exit 0 against the same image with only the esm/ directory added; - exit 0 against the local production build of this commit, isolated from any parent node_modules; - exit 1 again after deleting exactly esm/_interop_require_default.js from that same local build. src/tests/build/standalone-boot-graph.test.ts pins the MECHANISM (a module-sync/default split with only the default branch present) rather than the package, and all 5 of its cases were watched to fail against a neutered gate. NOT VERIFIED. Nothing about production: this is only true of production once it merges, is promoted main -> release, is built and is serving. The gate covers the ENTRYPOINT's require graph; route chunks load lazily, so a condition mismatch reachable only from a route would still surface at request time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: retrigger preview The previous preview run (pr-preview-4075-b2wnw) never scheduled: its build-image and typecheck pods sat Pending with ExceededNodeResources for 82 minutes and the run hit the 1h30m PipelineRunTimeout. No verdict was produced — this was build-pool capacity contention, not a code failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
394b4bc806 | migrate retool endpoints to moderator endpoints | ||
|
Zachary Lowden
|
8dd728eabe |
feat(build): assert security-relevant branches survive into the compiled output (#3983 detection) (#4068)
* feat(build): assert security-relevant branches survive into the compiled output
Release 5.1.18 shipped `resolveStoreVisibilityScopeUninstrumented` as
async function S(e){if(await p(e))return"full"}
Two of its three `return`s were absent from the emitted server chunk, so the
function fell off the end and produced `undefined` for every non-privileged
caller. One read path defaulted that to `'full'` and served the whole App-store
catalog to anonymous callers; the other defaulted the same missing value to
`'none'` and showed the intended cohort an empty store (#3983).
The TypeScript is correct — that is the whole problem. A 75-test unit suite, an
integration suite driving the real feature-flag client, and four rounds of
review were all structurally incapable of seeing it, because every one of them
exercises the source. Nothing looked at the artefact.
This adds a gate that looks at the artefact. It reads the emitted `.js.map`
`mappings` and asserts that each watched fail-closed branch still has a
representation in the output, attributing code to source modules by source map
rather than by grepping minified JS — minified names are per-chunk, the module
is inlined into 234 chunks, the literal `"public-external"` appears ~481 times
in the build without ever being a return, and the flag name
`app-listings-public-external` contains that literal as a substring.
Every entry carries a positive control: a branch known to survive that must also
be mapped. If the control is missing the gate exits 2 ("cannot observe") instead
of exit 1 ("violation"), so a build that simply did not emit the module is never
reported as a pile of violations.
Wired into the Dockerfile beside `check-server-graph-singletons.mjs`, and for
the same reason — `.next` exists in that stage, so there is no second build.
🔴 It runs `--warn-only` for now, because the underlying bundler defect is NOT
fixed: `main` and `release` carry byte-identical source for that function and
both emit it truncated, so a hard gate would fail every production build today.
`--warn-only` does not downgrade exit 2. Removing that flag is the definition of
done for #3983.
Verification: 13 cases, each asserting its own failure branch's specific message
and exit code, driven over synthetic `.next` trees with real base64-VLQ maps
encoded by an independent implementation. Six mutations of the gate were each
killed by their own case — one of them (breaking the VLQ sign branch) initially
SURVIVED, because every fixture listed source lines in ascending order and so
never produced a negative delta; the reordered-segments case was added to close
that. The gate was also run against the real 5.1.18 server artefact, where it
correctly reports both missing branches.
* style(build): prettier the compiled-branch gate + watchlist
|
||
|
Justin Maier
|
98f42aab65 |
fix(3d-models): correct the cutover constant and drop the superseded backfill (#4054)
* fix(3d-models): correct the cutover constant and drop the superseded backfill
The constant said 2026-08-18, picked by hand before the deploy. Tracking actually
started 2026-08-17 17:21:31 UTC, so the Creator Studio would have marked the
boundary a day late. Now read off production, with a note that the day is MIXED -
backfilled before 17:21:31, live beacons after - so a consumer marks that day and
not the one after it.
Removes scripts/oneoffs/backfill-model3d-views.{ts,helpers.ts} and its test. The
backfill now lives in civitai-scripts (backfill/model3d-views.js), and the copy
here still carried the design that shipped wrong: an --until DATE cannot express
the real boundary, because tracking starts mid-day. Any date either drops that
day's pre-deploy views or double-counts its post-deploy ones - measured on the
real cutover, 1,488 views before the first beacon and 296 after. The replacement
derives the boundary as min(time) of the first tracked day, so the two sources
abut exactly and there is no value left to typo.
Leaving a second, wrong copy in this repo is worse than having none: it is the
one a reader finds by grepping.
The DDL file is kept as the record of what ran, now marked applied.
Backfill result, verified independently: 11,336 rows / 175,964 views across 476
models, 2026-06-19 to the cutover. The boundary day reads 1,650 = 162 beacons +
1,488 backfilled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS
* refactor(3d-models): keep the cutover constant hardcoded, trim comments
The constant stays and stays hardcoded - the backfill derives its own boundary
from data and never reads it, so its only job is captioning the Studio chart,
and a hand-set date is fine for that.
Corrected to 2026-08-17, the day tracking actually began (17:21:31 UTC). The
comment now says the thing a reader needs and nothing else: the two spans are
different measurements, page loads ran ~1.5x the beacons that replaced them, so
a chart crossing that day steps down for reasons unrelated to the creator.
SQL comments cut 60 lines to 19, keeping only what a future editor would get
wrong without them - that MODIFY COLUMN replaces rather than appends, that
entityType is a sorting key so renumbering means a table rewrite, and why the
MV goes last.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
088a9d09b1 |
fix(release): refuse an app release from a branch behind its released history (#4046)
`release-app.mjs` derives the next version from apps/<app>/package.json on the
CURRENT branch. When that branch is behind the app's released history the tag it
computes does not continue that line, in one of two ways:
* it already exists -> `git tag` aborts, but only after the release commit has
been made, leaving a junk commit on the branch;
* it is a different line (a minor/major bump off a stale base) -> nothing
collides, the tag becomes the HIGHEST for that app, and the Flux ImagePolicy
selects the highest semver rather than the most recent push. That stale build
is then what production runs.
apps/moderator is in exactly this state, measured 2026-08-17: 0.0.1 on main,
0.0.26 live, all 26 releases cut from `moderator-app-pages` — 211 commits and
+38,630 lines that never merged to main. `pnpm release:moderator` from main
collides on the existing 0.0.2 and aborts; `release:moderator:minor` computes
0.1.0, which does NOT exist, and would deploy main's stale copy to production.
One command, no collision.
So this does NOT bump moderator's version to 0.0.26. That is the obvious fix and
it is the wrong one: it removes the collision that is currently the only brake,
while leaving the app itself 211 commits stale. The real remedy is to land the
branch; the guard is what makes the trap loud until someone does.
Checked after `git pull --rebase` so the tag list is current, and before
`npm version` so a refusal leaves the tree exactly as it found it.
Coverage: 19 tests. The version arithmetic is unit-tested, and — because a guard
nothing calls is not a guard — release-app.mjs is also driven end-to-end as a
real process against a throwaway git repo with a local bare remote (offline; no
tags or commits touch this repository).
Mutation-tested, 4 mutants, each the narrowest expression that can be wrong:
invert the behind-comparison -> 5 tests fail
lexicographic highest-tag compare -> 6 tests fail
guard computed but never acted on -> ONLY the 2 behavioural tests fail,
which is what pins reachability
drop the unparseable-tag skip -> exactly 1 test fails
Baseline restored green (19/19) after the battery.
NOTE: scripts/**/*.test.ts runs in the `unit` vitest project, which is
`continue-on-error: true` in lint.yml — so these tests run but cannot currently
fail CI. That is tracked separately (868kp7fdr); it does not make them useless,
it makes them a local and post-fix gate.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Justin Maier
|
eb848582ef |
feat(3d-models): track Model3D views, with a pageViews backfill (#3997)
* feat(3d-models): track Model3D views, with a pageViews backfill 3D models had no entity view tracking: no TrackView on any /3d-models route, no Model3D arm in the ClickHouse enums, nothing in daily_views. The history exists in pageViews but nothing reads it. Adds one TrackView on the detail page, reusing the existing TrackView -> /api/internal/pulse -> Tracker.view() path. No new component, no new endpoint, no new rollup table, no materialized view: daily_views is already ORDER BY (entityType, entityId, createdDate), so a per-creator query over at most 539 ids is a primary-key prefix seek. Ownership resolves from Postgres. A view is one load of a model's public detail page. /edit and /reviews are excluded by name rather than by path shape, because /3d-models/481/edit and /3d-models/481/my-slug are structurally identical - an anchored id regex matches the edit page too. Verified in ClickHouse's own RE2 against 30 days of prod pageViews: 82,319 counted, 399 edit+reviews excluded, 21 junk excluded. The DDL is not applied. It must run after the comics DDL (PR #3993), and it restates all twelve enum arms because MODIFY COLUMN replaces an Enum8 rather than appending to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS * fix(3d-models): address adversarial + simplification review Backfill correctness: - The shared ClickHouse client sets wait_for_async_insert: 0, so insert() resolved before the rows were queryable and the read-back raced the flush - a successful run could report "boundary mismatch, found 0" and then be blocked from retrying by its own guard. Wait on this one insert. - Verify the whole written range (count + sum) instead of one boundary day. A partial write that lost June and July but landed the last day passed before; a boundary day with zero rows verified nothing at all. - Assert the cutover constant against live data: the first day `views` holds a Model3DView IS the cutover. Nothing tied the constant to the deploy, so a deploy landing a day early or late left a doubled or permanently empty day that nothing detected. - Derive the id-extraction pattern and the filter from one constant. They had to agree or extract() returns '' and toUInt32('') throws mid-query. - The documented `npx ts-node` command could not resolve the ~/ imports. DDL: - Say to apply before deploying the emitting code, and to run steps 1-3 without a pause. Between step 2 and step 3 a Model3D row can reach an MV whose declared header is still nine arms, and every view type on the site shares that insert path. - State what step 0 expects rather than asking the operator to confirm against nothing. - Note that metadata-only holds only because every existing name->index pair is preserved; entityType is a sorting key, so renumbering means a rewrite. Local dev ClickHouse (containers/clickhouse/docker-init/init.sh) declared the nine-arm enums, so a fresh container silently dropped every Model3DView row. Tests: delete two DETAIL_PREDICATE assertions that could not fail on a revert - both matched substrings present in the surviving half of the predicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS * fix(3d-models): harden the cutover guard against incidental tracking rows Applying the enum DDL removed an accidental protection: before it, a dev server or preview deploy on this branch writing a Model3DView row was rejected by the nine-arm enum. Now it succeeds. Local dev points at the same ClickHouse as prod, so one developer opening a 3D model page writes a real row - and the cutover guard took a bare min(createdDate) over them, which would move the detected cutover back to that day and silently drop every day between it and the real one. The cutover is now the first day clearing a floor no incidental page load reaches (50 rows against ~2,700/day of real traffic). Days below it that fall before the cutover are reported rather than ignored silently. The mismatch error also prescribed a lossy repair as if it were the fix: the mismatched day is almost always partial, since tracking starts mid-day. It now names both options and says not to take the first by default. Also: - select_sequential_consistency on the readback. Prod is a two-replica SharedMergeTree behind a load balancer, so the insert and the read can land on different replicas; wait_for_async_insert guarantees the write committed, not that the next query sees it. A stale read reports a false failure and the pre-flight guard then blocks the retry. - Restore the Number() coercion dropped in the last commit. It was what kept the script from depending on a client-level output format setting. - --dry-run works again before the deploy, which the live check had broken. - Restore an assertion on the edit/reviews exclusion. The previous commit claimed both deleted tests were vacuous; only one was. The other failed on a real revert, and it was dropped because ID_PATTERN broke its regex, not because it caught nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
ebe058067f |
fix(test-perf): stop the mock-allowlist generator reading a directory (#3976)
`globSync('src/**/*.test.{ts,tsx}')` matches directories as well as files, and
vitest browser mode names each snapshot directory after its spec — so
`__screenshots__/AppListingCard.browser.test.tsx` is a DIRECTORY that matches,
and the readFileSync below it dies with EISDIR before the generator produces
anything.
Those directories are gitignored, so the generator works on a fresh checkout and
breaks permanently for anyone who has ever run a browser test locally — which
reads as "this script is broken for me specifically" rather than as a bug.
Filter to regular files. Verified against a tree with three such directories:
EISDIR before, `canonical 218 -> 218 files (0 migrated)` after.
Claude-Session: https://claude.ai/code/session_01KvXBiAVpWyhNS85tsBMuDU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Justin Maier
|
533640662d |
perf(tests): services a-m onto the canonical shared mocks (127 of 129) (#3973)
* perf(tests): move services a-m onto the canonical shared mocks 70 of 129 files in the services `__tests__` a-m slice: 62 converted by scripts/test-perf/codemod-shared-mocks.mjs, 8 finished by hand where it refused one specifier and took another. The hand cases were a hand-rolled auto-vivifying Proxy standing in for dbRead/dbWrite, and a `safeError: (e) => e` identity stub the canonical registration replaces with the real implementation. Neither was asserted on; both were scaffolding to keep the import graph off real infra, which is what the canonical mocks do centrally. Eleven of the converted files carried a hand-written REDIS_KEYS subset that disagreed with the real table and now get the real constant. Every dropped literal appeared exactly once, inside its own factory: no test asserted on one and none used one to select a fixture branch, so the swap is invisible in both directions rather than merely silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): repair two files the shared-mock conversion broke `collection-collaborator.service.test.ts` collected ZERO tests, losing all 51. The codemod lifted a default out of a `vi.hoisted` block body into module scope, but the value it referenced — `OWNER_ID` — was a local of that block, so the file threw at import. It reported as one failed suite with no failing tests, which is the silent class the collected-count diff exists to catch. The file already sets the same default in `beforeEach`, so the surviving copy sits with the other module-scope constants. `creator-program.service.test.ts` asserted on `cp:*` cache keys it had invented in its own factory; the real ones are `packed:caches:creator- program:*`. Three tests went red on the swap. They now name `REDIS_KEYS.CREATOR_PROGRAM.*` so the test cannot re-invent a key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(redis): pin the wire values the services a-m tests re-invented Nine files in that slice carried a hand-written REDIS_KEYS subset whose values disagreed with the real table — `packed:caches:announcements` for a real `packed:caches:announcement`, `kill` for `system:blocks:emergency-kill-list`, `cp:*` for `packed:caches:creator-program:*`. Every one was used only inside the factory that defined it, so no assertion could see the divergence and none of them could go red. A key's wire value addresses live entries written by deployed code, so a rename orphans whatever sits under the old name. Pinning them here makes that a deliberate decision rather than something a test silently follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the direct-mock allowlist after services a-m 345 -> 275 canonical files; the guard passes in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): split the single-client db aliases in services a-m Eleven files mocked `~/server/db/client` with one local object serving both `dbRead` and `dbWrite`, so a write could satisfy a read assertion. Each is bound to the one client the module under test actually spells for every call it makes, checked against the service source rather than guessed from the local's name — `mockDbRead` in two of the block-registry files turned out to be right for the wrong reason, and `mockDb` in `account-deletion-images` was serving writes. Two things the split surfaced, both kept rather than papered over: `get-engaged-models-by-ids` carried a real in-memory fixture on `resourceReview.findMany`, not a restated default, so binding the client alone dropped four tests' data and they went red. The fixture is now an explicit `mockImplementation` on the canonical node, and the four tests are the evidence it is load-bearing. `account-deletion-images` asserted on `'pending-restores'`, a key it had invented; the real one is `system:pending-image-restores`. It now names `REDIS_SYS_KEYS.SYSTEM.PENDING_IMAGE_RESTORES`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after the alias split 275 -> 264 canonical files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate 18 more services a-m tests onto the canonical mocks Seven bound automatically where a balanced behaviour check cleared them, nine converted by hand where it refused, and two whose single aliased local was split by routing each call site to the client the service actually spells for it. Three cases the run settled rather than the diff: `minor-hash` wired two separately-declared spies in as client leaves, so deleting the client literal left both alive, armed by `beforeEach`, and connected to nothing — the primary-vs-replica re-read then saw the canonical `null` default. Behaviour-free is not the same as safe to delete; both names now point at the canonical nodes. `article-` and `bounty-locked-properties` spread their transaction object into `dbWrite`, so the canonical `$transaction` default preserves the identity they relied on. `model-appeal` did NOT — its transaction client was a separate object, and inheriting the default would let an in-transaction write satisfy an assertion that means "written outside the transaction". It keeps its own `tx`. `model-version.{donation-goals-cache,idempotent}` each carried a second direct mock of `~/server/redis/client` alongside the db one; both are taken, so neither file is left half-converted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after the second services a-m batch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test-perf: add the client-local binder, with the refusal a run taught it Binds a test's own client locals to the canonical mock nodes for the shape the codemod refuses as "hoisted entry is not a bare vi.fn()". It refuses two things rather than guessing. A leaf carrying behaviour a canonical default does not cover, extracted with balanced parens — a lazy `vi.fn\([^;]*?\)` stops inside an arrow's own parameter list and hands the check a truncated call that reads as behaviour-free, which is how five files passed a check written to stop them. And a leaf that is a bare identifier declared elsewhere. The client literal is the only thing wiring such a spy to the module, so deleting it leaves the spy alive, still armed by `beforeEach`, connected to nothing, while the code under test reads the canonical default instead. That cost two tests in minor-hash.service.test.ts and no static check caught it — behaviour-free is not the same as safe to delete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): per-case analysis of the parameterised-client alias split Six files in the services a-m slice mock one local as both db clients against services that choose their client at runtime, so the usual "grep the source for dbRead.model.method" routing finds nothing. Corrects my earlier report: these are NOT permanent hand-work. No test in the six passes a `db` option, so every case falls to its entry point's default, and those are fixed and listed. Four call sites default to dbWrite and one inverts to dbRead, which is the trap. Records the 14 negative assertions individually, because routing one to the client the code never touches makes it pass trivially — the failure a run cannot show — and names the one path I could not resolve rather than guessing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate two more services a-m tests, and harden the binder `model-version-count` named its spy with ES6 shorthand (`{ groupBy }`), which the binder read as an empty object and would have orphaned; bound by hand. `cover-image.service.logging` declares its `logToAxiom` rejection on the canonical node — the rejection IS the fixture there, since the tests exist to prove the `.catch()` on the best-effort log calls, so it is moved rather than dropped. Three refusals added to the binder, each after catching it wrong on a real file: - one local bound to both clients is an alias, and binding it here would silently pick whichever came last; - ES6 shorthand entries, per the orphaning above; - a factory with a BLOCK body, which opened on the block's brace, read an empty object, found no exports to object to, and deleted the whole factory with every local it named. It cleared two files that way and nothing caught it but reading the diff. It now auto-converts none of the 23 remaining files in this slice, which is the honest number: what is left is hand work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate six more services a-m tests onto the canonical mocks All six by hand; the binder refuses every remaining file in this slice. `commentsv2-owner` wired ONE `findUnique` spy into twelve tables, so a case naming `entityType: 'challenge'` could be satisfied by any of them. Each entity type now arms its own table, and the two "no lookup happened" assertions check all twelve rather than the single shared spy — stronger than what they replaced. Four of the twelve have a case; the other eight were in the fixture because the service can read them, not because anything checks that it does. `leaderboard-rank-showcase` records every statement it issues into an array that every assertion reads. The canonical `$executeRaw` returns 0 and records nothing, so the recorder is declared rather than inherited — dropping it would empty the array and throw, which is the loud case. `challenge-results-notification` and the two `contest-entry-*` gates were aliases hiding nothing: every path they assert on is spelled `dbRead` in the service. `bust-caches-for-posts-empty` replaces a permissive Proxy whose `apply` trap answered every un-stubbed call with `[]`; the two assertions in it are on `$queryRaw`, whose canonical default is also `[]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after batch 4 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): add lag-selected clients as a second unresolvable mechanism `getDbWithoutLag` picks dbRead or dbWrite from runtime replication state, so anything reached through it has no fixed client in the source. Five more files in the slice route reads that way. Under test it does not even behave as production does: REPLICATION_LAG_DELAY is a zod `.default(0)` key absent from TEST_ENV_DEFAULTS, so the canonical env reads it undefined, and `undefined <= 0` is false where `0 <= 0` is true — the staleness branch production never takes runs in every test that reaches it without stubbing db-lag-helpers. 73 defaulted keys, 59 absent from the table, 40 of those numeric or boolean. Also records why the already-converted files were safe: they happen to mock db-lag-helpers, which pins the client. The safe and unsafe conversions are separated by that coincidence, so the population at risk cannot be read off which files converted cleanly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): the lag-selected bucket is two files, not five Corrects the section I added an hour ago. Three of the five resolve by the entry point the test imports: deleteVersionById uses dbWrite only, and neither model-file service mentions getDbWithoutLag at all. Only earlyAccessPurchase (via getVersionById) and publishModelVersionById genuinely defer the choice to runtime. The mistake is the one the section is about — reading BOTH off a whole-module scan and treating it as a property of the file. A module containing both spellings says nothing; the entry point the test calls is what decides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): handover for the services a-m mock migration The state a successor cannot recover from the diff: the four remaining buckets with the discriminator for each rather than just their names, the binder's five refusals with the case that taught each one, and what to check on the seam-blocked files after that change lands — they will look convertible the moment it does. Also splits the post-tooling-scare audit into three categories rather than two: claims re-verified explicitly, claims structurally immune because they came from node rather than a shell pipe (an accident of format, not a method), and claims not re-derived at all, named individually. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate four more services a-m tests onto the canonical mocks `engagement-toggle.idempotent` was an alias hiding a real read/write split: `toggleUserBountyEngagement` reads bountyEngagement on dbRead and writes it on dbWrite, through one spy, so a case asserting the read could have been satisfied by a write. Routed per call site from the two entry points the file imports — `toggleModelEngagement` does every modelEngagement operation on dbWrite, and the dbRead spelling elsewhere in user.service belongs to a function this test never calls. `block-registry.slot-reservation` keeps its distinct read/write locals, so the parameterised-client question does not arise. Its `redis.scanIterator` is an empty async generator consumed with `for await`; the canonical node would vivify it as a spy returning undefined, which throws rather than iterating, so the generator is declared explicitly. `file-download-lookup` carried a hand-written `safeError` under a comment claiming the global setup does not provide one. It does — the canonical registration spreads the real module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after batch 5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): correct the handover after batch 5, and add two mock-system constraints 111 of 129, not the 112 the placeholder claimed. The ordinary bucket grew from 3 to 7 because three files moved back out of "unresolvable" when I re-checked them; the seam-blocked three are now unblocked and are the easiest remaining work, with the check that is not "do they pass". Adds two constraints that belong to the mock system rather than to this slice: a canonical mock cannot statically import anything reading mocked env at module scope, because setup.ts loads it earlier than every hoisted factory it registers — and the failure is zero tests collected reported as one failed suite. And ~25 typecheck errors on this base are phantom, with the tell being that they sit in files you did not touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate the three withSysReadDeadline seam tests onto the canonical mocks These three were blocked until `withSysReadDeadline` became a seam node on the canonical redis mock ( |
||
|
Justin Maier
|
51eb5d7bf0 |
test(perf): canonical shared-module mocks, a migration ratchet, and the first 100 files (#3959)
* test(perf): canonical shared-module mocks for running without per-file isolation
The unit suite spends ~81% of worker-time importing modules. `vitest --no-isolate`
collapses that but breaks ~1,500 tests, and the mechanism is narrower than
"vi.mock is per file": a test file that does not mock a module still gets the real
one. The damage travels through ordinary source modules — a module that imports a
mocked module is evaluated ONCE per worker and captures its bindings then, in
whatever mock context that first evaluation happened to occur. Every later file
reuses it, still pointing at the first file's mock object.
So the fix must keep the mocked shape complete and function identities stable for
the worker's lifetime, and swap only behaviour. src/__tests__/mocks/ does that: a
hybrid node that is both a vi.fn() and a proxy vivifying cached children, one
canonical mock per specifier, registered globally in setup.ts and reset per file
(setupFiles re-run per file in both isolation modes).
Inert under isolation, where it is registered but nothing can leak: 174 files,
2089 tests, 0 failed.
Ships the tooling to continue the migration: a codemod that converts only shapes it
can prove equivalent and reports every refusal with a reason, an allowlist
generator that refuses to grow, and a run/compare pair that diffs per-file
collected counts — because under --no-isolate a file whose module scope throws
collects ZERO tests and the run still reads as green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): migrate 83 files onto the canonical mocks, and ratchet the rest
Converted by scripts/test-perf/codemod-shared-mocks.mjs. The test bodies are
untouched: only the vi.mock factory and its vi.hoisted spies go, replaced by consts
bound to canonical nodes, so the change is mechanical and reviewable as such.
Measured on the 83 files that are clean for all three specifiers, against the same
files isolated at 4 workers (856 tests, 0 failed):
--no-isolate, 4 workers 846 tests 11 failed wall 62.6s -> 22.9s
--no-isolate, 12 workers 853 tests 1 failed wall 62.6s -> 18.8s
The residual failures are not mock shape. eventloop-longtask.ts registers prom
metrics at module scope, so under --no-isolate the first file to import it in a
worker takes the registration and later files see an empty registry; that belongs
with no-module-scope-cache.
Migration is all-or-nothing per specifier — one hold-out re-poisons its whole
worker — so no-direct-shared-module-mock guards it with an allowlist at 433 files.
It ratchets both ways: a new direct mock fails, and a migrated file left on the list
fails too, so the count cannot be padded.
purge-review-snapshots proved "the gate reads the primary" with
`expect(dbRead.x.findFirst).toBeUndefined()` — the replica fixture simply lacked the
method. The canonical mock vivifies every method, so absence stops being observable;
rewritten as `not.toHaveBeenCalled()`, which asserts the behaviour rather than the
fixture's shape and survives another file populating that method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): type the shared-mock self-tests against the canonical nodes
`tsconfig.tests.json` sees these files even though `pnpm typecheck` does not, and
calling through the re-exported `dbRead`/`redis` demanded real Prisma `where`
objects and literal Redis key unions — 8 errors about argument shapes, in tests
about default return values. The canonical nodes are the same objects (asserted in
the file), so routing the calls through them keeps each test about what it is for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): migrate 17 more files, and teach the codemod the real REDIS_KEYS
The codemod now static-parses REDIS_KEYS_UNPREFIXED / REDIS_SYS_KEYS out of
packages/civitai-redis/src/client.ts and deep-compares each hand-written literal
against it: dropped where every leaf matches, refused and printed where one does
not. That was the largest refusal class.
42 files diverge across 73 leaves. Some are placeholders ("rl", "kill"); others
read as real and are wrong — CACHE_LOCKS as "caches:lock" against a real
"cache-lock", TRPC.LIMIT.BASE as "trpc:rate-limit" against "packed:trpc:limit", a
system:-prefixed sys key written without the prefix, and a cache key still on v1
after the real one was bumped to -2. None are live bugs: a test asserting against
its own copy uses the same wrong string on both sides, which is why nothing in the
repo would ever surface them.
Allowlist 433 -> 416.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): teach the codemod three more shapes, migrate 31 more files
Repo-wide convertible goes 10 -> 84 on the current allowlist. New shapes:
- a whole client built as an object literal of spies, collapsed to the canonical
root: binding the root makes every leaf vivify at exactly the path the literal
named, so `mockDbRead: { collection: { findFirstOrThrow: vi.fn() } }` becomes
`const mockDbRead = dbMock.dbRead` and the test body needs no edits. Leaves
carrying real behaviour are still refused rather than dropped.
- `vi.hoisted` with a block body whose returned property names a local; the local
is removed too, but only when the block reads it exactly once, so a `make()`
helper shared by two clients is left alone.
- an import alias when a test's own local collides with the canonical mock's name.
`const { redisMock } = vi.hoisted(...)` is real in this repo, and lifting it
produced the self-referential `const redisMock = redisMock.redis`.
Verified on the 120 files now clean for all three specifiers, against the same
files isolated (1268 tests, 0 failed): 1268/1268 collected under `--no-isolate` at
both 4 and 12 workers, no file lost tests. The remaining failures are other shared
specifiers that have no canonical mock yet — `~/server/flipt/client` is the loudest
— which is the same mechanism, not a regression in these conversions.
Allowlist 416 -> 396.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): widen the mock guard to every shared specifier, counted honestly
The three specifiers with canonical mocks are not the whole job, and an allowlist
covering only them would reach zero with `isolate: false` still unflippable.
Measured: a 120-file set clean for all three still failed 110 tests at 4 workers
under --no-isolate, entirely through specifiers nobody had listed — loudest being
`No "isFliptSync" export is defined on the "~/server/flipt/client" mock`. Same
mechanism, different module.
So the guard now tracks 15 specifiers, split by obligation:
CANONICAL (3, enforced) 562 sites across 396 files
PENDING (12, counted) 856 sites across 392 files
PENDING is counted rather than enforced because those modules have no canonical
mock to migrate to; enforcing would push every new test file onto the allowlist and
measure churn instead of work. Its recorded count is asserted against reality, so
the remaining scope cannot silently understate itself.
The specifier lists live in one TS module and the generator PARSES them rather than
holding a second copy — a generator that disagreed with the guard about which
modules are guarded is the one failure this pair must not have.
Stated in the guard, because it is the part that is easy to get wrong: the flip
criterion is not "the allowlist reaches zero", it is "every specifier a test file
shares with another test file has a canonical mock".
Next specifier to do is `~/env/server`: 109 sites, every one partial, and it is
already globally mocked in setup.ts with a Proxy — so the canonical shape exists
and the work is per-file behaviour plus a reset, not a new design.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): give the six external mock factories a `default` key
Pre-bundling wraps a CJS dep for interop, so the consumer resolves through
`default` and a factory that returns only named exports yields undefined. Adding
`default` lets `redis`, `@aws-sdk/client-s3` and `@aws-sdk/lib-storage` join the
SSR pre-bundling safelist.
Measured under a pre-bundling config, same six files, back to back:
without `default` 6 files 7 tests collected 6 files failed to load
with `default` 6 files 106 tests collected 0 failed
7 -> 106. Note the failure mode: the run does not report 99 failures, it reports
almost no tests. A file whose module scope throws collects nothing, the failure
count barely moves, and a summary line reads as a pass. The acceptance check is
therefore the collected count, per file — `s3-utils.test.ts` alone is 66 of the
106, so the total can look healthy while that one file is empty.
`importOriginal` does NOT protect against this, despite the repo mandating that
form: the spread copies the original module NAMED exports and does not synthesise
a `default`. `s3-utils.test.ts` already used `importOriginal` and still collected
nothing without the key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): stop the canonical mocks evaluating the real db/redis shims
`~/server/db/client` and `~/server/redis/client` are shims: they re-export their
package wholesale AND construct real clients at module scope. Registering them with
an `importOriginal` spread therefore forced real Prisma/Redis construction into
EVERY test file, where before only files without their own db mock paid it.
That is a correctness surface, and it fails in the worst available shape. A file
whose own `@prisma/client` mock omits `PrismaClient` died during module evaluation
with `PrismaClient is not a constructor`, so it collected ZERO tests — the failure
count stayed at 0 and the run read as green. arabella found it on a control run
before converting anything, which is the only reason it was found at all.
The package re-exports are all the spread was ever protecting, so the fix is to
spread `@civitai/db/client` / `@civitai/redis/client` directly. Same exports, no
construction, and the precondition for the whole failure class disappears rather
than being enumerated: a file no longer needs its own db mock to be protected.
Verified: `process-vault-items.test.ts` collects 15 tests again; all ten files that
mock `@prisma/client` collect (134 tests, 0 failed); and a file simulated into the
MIGRATED state — its own db mock removed, its `@prisma/client` factory still
lacking `PrismaClient` — collects 8/8, so the five files predicted to die when
migrated no longer will.
The 120 migrated files were unaffected: identical per-file collected counts before
and after (1268), 0 zero-collect files in either.
Pinned by a test asserting neither shim's globalThis client cache is populated —
absent globals are direct evidence the module body never ran.
Also adds the canonical `~/env/server` mock (worker-level defaults + per-file
overrides + reset) and drops three per-file env mocks onto it. Under `--no-isolate`
at 4 workers the 120-file set goes 110 -> 58 failures, 1268/1268 collected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): the shim-vs-package rule and env's two buckets
Both are things a migrator gets wrong silently rather than loudly.
Spreading a shim evaluates a module that constructs clients; spreading its package
does not. Stated with the failure shape, because the symptom is a file collecting
zero tests behind a green run rather than anything that looks like an error.
`~/env/server` splits where the other specifiers do not: a per-file override cannot
reach a value read at module scope, since under `isolate: false` that module is
evaluated once per worker. Module-load values belong in the worker defaults,
call-time values in per-file `setEnv`. Recorded with the reason it is not a
shortcoming of the design — a per-file `vi.mock` factory has the same problem — so
nobody reverts to per-file mocks trying to fix it.
Plus the `Object.defineProperty` trap: an inconsistent proxy descriptor throws on
the SECOND call, which surfaces as a dozen unrelated failures pointing nowhere near
the cause.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): make the whole-suite collected-count diff a blocking flip gate
Per-slice verification is sound and insufficient at the same time. Every slice
owner diffing per-file collected counts over their own files is correct practice
and still leaves a gap: a file nobody owns can collect zero and no owner's control
covers it.
That is not hypothetical. The `importOriginal`-on-a-shim regression made one file
die at module scope and contribute nothing; the 120-file pilot was verified by
per-file collected counts and could not have caught it, because the affected file
was outside the set. It was found by someone taking a control run on a different
slice before converting anything. The failure was not in anyone's work — it was in
the gap between everyone's work.
So the gate is whole-suite, diffed per file against a `main` control taken in the
same window, zero files losing tests and totals matching exactly — and it runs
immediately before the flip, because the property is only true of the tree that
ships. Cheap: the integration run was 1069 files / 16806 tests in about four
minutes.
Records the general form too, since it outlives this migration: the day's two most
valuable findings each came from someone else running a control on another person's
work, and a third correction went the other way. Authors verify what they changed;
nobody verifies what changed around them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): four more codemod shapes, taking convertible from 53 to 204
Widening the tool beats converting files by hand: every shape lands on four slices
at once. Two of the four came from arabella's refusal taxonomy, and one of those
was a case of the tool asking the wrong question rather than being too conservative.
1. LIFT inline behaviour instead of refusing it. A leaf carrying behaviour is
already an expression, so it becomes an explicit assignment on the canonical
node - mockImplementation / mockResolvedValue - emitted at module scope. A spy
reached through an object (`mocks.findMany`) is WRAPPED rather than rebound, so
`expect(mocks.findMany).toHaveBeenCalled()` still holds and the assertions do
not move. Skipped entirely in a file that calls resetAllMocks or
restoreAllMocks, where a module-scope assignment would be wiped before the
first test runs.
2. A client literal may now carry behaviour-bearing leaves. `$executeRaw:
vi.fn().mockResolvedValue(undefined)` beside four bare spies was the only thing
separating several files from the object-literal collapse.
3. Factory-supplied constants are DELETED, not proved equal. The registration
spreads @civitai/redis/client, where both key tables are defined, so the
factory's copy is redundant whatever it contained - and proving the literal
equals the real constant is unanswerable for `completeKeys({ ... })` and is not
the question that decides safety. What decides it is whether the test names the
constant outside the factory, which is a check the tool can make. Divergences
are still reported, as findings rather than blockers.
4. Any export other than the client roots is dropped on the same rule - safeError,
withSysReadDeadline - since the spread already supplies them. The factories only
declared them because replacing the module wholesale meant they had to.
Also: a block-body factory may now hold arbitrary locals. It used to refuse
anything that was not `const actual = await importOriginal()`, which rejected a
`make()` key-proxy helper for no reason - the whole factory is deleted, so its
locals go with it.
Verified on the 11 files this converts in my slice, against a control of the same
27: 582/582 tests collected, 0 failed, isolated. One file went red and it is the
finding the docs predict: middleware.trpc.test.ts asserted
`stringContaining('trpc:rate-limit:')` against its own fixture's copy of
REDIS_KEYS.TRPC.LIMIT.BASE. The real key is `packed:trpc:limit`, so both sides of
that comparison were the fixture's invention - it could never fail and never
matched production. The assertion now names the real key.
Allowlist 396 -> 385.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): widen mock-guard DETECTION to .tsx, leave conversion on .ts
A .browser.test.tsx adding a direct canonical mock is a class the guard was
structurally unable to observe, which is different from a class that happens to be
empty (it is: 0 such files today). Detection is the half with value now.
The codemod deliberately stays .ts. Converting a browser-mode file would put it in
a regime the canonical mocks have never been proven in, and a glob change would
make that look like a supported path. Proving browser mode is real work, not a
one-word edit. (donovan's finding, and his own suggested split.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): lift behaviour on declarations and hoisted entries too, 204 -> 223
The lift rule already applied to a factory leaf; a spy declared WITH behaviour —
`mockUpdateMany: vi.fn(async (args) => ({ count: 1 }))` in a vi.hoisted object —
still blocked its whole file. Same treatment now: the behaviour becomes a
mockImplementation on the canonical node and the binding converts.
Validated on a real file rather than by the report: agent-report-callback.test.ts
converts with zero refusals and passes 14/14. Its own slice owner is mid-probe on
that directory, so the file was restored afterwards and nothing there is committed.
Remaining refusals are now dominated by the dbRead/dbWrite aliasing class (43),
which is deliberately manual — splitting an alias needs someone to know which
client the code under test exercises, and some of those are supposed to go red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): record the browser-mode gap as work, not verification
The codemod stays .ts while the guard detects .tsx. Worth stating that there are
ZERO .tsx files mocking a canonical specifier today, so closing the gap means
writing one rather than migrating one — a piece of work, not a check somebody can
tick off. (donovan's framing, sharper than mine.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): three unsound codemod shapes, found by two probe runs
223 -> 164 convertible. The drop is the point: 59 files that were converting
silently wrong now refuse. All three were caught by other people running a
control on the tool's output, and two of the three do not fail loudly.
1. DROPPING A NON-ROOT EXPORT removed a test's control surface. The registration
does supply the real `withSysReadDeadline` / `safeError` — but a factory that
replaces one with a SPY is using it to drive the behaviour under test.
session-verifier.test.ts injected a timeout through it; with the spy gone the
deadline never fired and two fail-open legs asserted nothing while still
passing. Available is not the same as redundant. Only plain DATA drops now;
anything carrying behaviour refuses.
2. LIFTING AT A CLIENT ROOT treated a whole client object as a leaf spy.
`redis: mainFake.client` became `redisMock.redis.mockImplementation(...)`,
which loses every method on the object — `scanIterator` yields nothing, `del`
is gone. It does not throw, it returns empty, so a test asserting "nothing was
deleted" would have gone GREEN. Lifting is now confined to method positions.
The wiring bug underneath it is worth naming: the guard was passed
`spec.flat`, which is `undefined` for db and redis, and `undefined` triggers
the parameter's `= true` default. The check was present and inert.
3. LIFTING AN EXPRESSION OUT OF A HOISTED BLOCK broke its references.
`$queryRaw: vi.fn(queryRaw)` became a module-scope `mockImplementation(queryRaw)`
while `queryRaw` was a const inside the `vi.hoisted` body the conversion
deletes — `ReferenceError` at import, so the file collects zero tests. A lifted
expression may now only reference names declared at the file's top level. Type
annotations are skipped, since they are erased before any of this runs.
Verified: all four files from the two probes now refuse, and a known-good
conversion still converts and passes 14/14.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): state what the flip gate cannot catch
Collected counts and residual-mocks detect ABSENCE. Neither can see a test that
still runs, still passes, and no longer asserts anything real — which is what two
of the three codemod defects produced. Both files converted with zero refusals,
kept every test, and read clean on residuals; one of them returned empty instead of
throwing, so a test asserting 'nothing was deleted' would have gone green.
What caught them was a control pair at assertion level, on a small set, run by
someone who did not write the tool. Recorded beside the gate as the half the gate
cannot do, so nobody reads a clean gate as completeness. (archer's point.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): the migration guard was inert in every full-suite run
It scanned the test tree in the `describe` body, so a throw during that scan was a
COLLECTION failure: the file contributed zero tests, the suite's failure count did
not move, and the guard was simply absent. It passed whenever invoked as a named
file, which is how it was checked all day. The allowlist, the ratchet and the
both-directions property were all unenforced in the run that matters.
Three changes, because the symptom and the shape both needed fixing:
- The scan runs INSIDE the tests. The same fault is now a red test with a stack
instead of a file that quietly is not there.
- It skips anything that is not a regular file. A full-suite run creates
directories under `src/` while the walk is happening, and one whose name matches
the glob reaches readFileSync as EISDIR.
- A POSITIVE CONTROL asserts the walk actually saw the tree (>800 files against a
real ~1,200). Every claim this guard makes is about a set of files; an empty walk
makes all of them vacuously true. Every other guard on this project has such a
control and this one did not, which is how it got here.
`compare-runs.mjs` gains the condition that would have caught it: every file the
candidate ADDS must collect at least one test. A new file has nothing on the
control to lose against, so it can collect zero and diff perfectly clean — the
gate's own blind spot, found by running the gate rather than by reasoning about it.
Also records that the PENDING specifier list is a FLOOR rather than an inventory.
It was assembled from a static scan, and specifiers keep arriving from the other
direction, as failures in a --no-isolate run of files already clean for everything
listed. flipt/client arrived that way; middleware/block-scope.middleware (27 files)
is a live candidate. The remaining work is discovered, not known.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): the unit of work is a cluster of specifiers, not one
Canonicalising block-scope.middleware took one pair from 13 failures to 7, and the
remaining 7 were a different class — so that pair shares at least one more
poisoning specifier nobody has named. 13 -> 7 is the dangerous shape: it reads as
progress and is not completion.
A set is done when its --no-isolate failures reach zero, not when the specifier you
were working on stops appearing. (arabella's measurement.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* perf(tests): migrate src/server (other) onto the canonical mocks
20 of the 56 candidate files in the slice — src/server/** minus services/,
jobs/ and routers/, which belong to other slices. Converted with the codemod at
95ac2f9e6a; nothing hand-written.
Verified on the full protocol rather than on a green:
control -> convert -> per-file collected-count diff -> pass/fail diff -> mutation
20 files, collected 162 -> 162, 0 regressed, 0 failures
Mutation sample across the mock kinds, on the converted files:
purgeCache remove the fail-soft catch -> 1 failed, bites
stored-image-probe drop the ETag passthrough -> 1 failed, bites
base.reward rethrow a transient CH error -> 5 failed converted,
5 failed unconverted
The base.reward pair is the claim worth making: the conversion preserves the
file's discriminating power, not merely its pass/fail state.
Two mutations came back green and are NOT findings — the same mutations were
equally green against the unconverted files, so they were mis-aimed at paths
those files never exercise. A mutation that does not bite is evidence of nothing
until it has been shown to bite on the original.
Three files keep a direct mock for a specifier the codemod refused, having been
converted for the others: challenge-helpers (redis), challenge-winner-payout-dedupe
and challenge-winner-persistence (logging). Per-specifier atomicity holds — no
file is half-converted for any one specifier — but those specifiers stay poisoned
for the worker, so the slice is not a clean read for redis or logging on its own.
The codemod also reported 9 constants the fixtures had invented, e.g.
REDIS_KEYS.CACHE_LOCKS real "cache-lock" vs test "caches:lock". None of the
affected tests named those constants outside their factory, which is why nothing
went red — both sides of any such comparison would have been the fixture's own
invention. The factory copies are gone; the tests now see the real values.
* test(perf): migrate the six aliased download fixtures by hand
All six aliased dbRead and dbWrite onto ONE spy, so a read routed to the primary
would have satisfied a replica assertion silently. Every handler reads the replica
only - dbRead.keyValue.findUnique for the blocklists, plus dbRead.vaultItem and
dbRead.modelVersion in the vault route - so each binds dbRead alone. That is
strictly more discriminating than the fixture it replaces.
Codemod refuses this class deliberately: splitting an alias needs someone to read
the production path and decide which client the code exercises, and getting it
wrong produces a passing test asserting the wrong thing.
7 files, 71 tests, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(test-perf): regenerate the allowlist after merging archer's slice
383 -> 366 canonical files, 518 sites. The allowlist is derived state and three
people are converting on separate branches, so it is regenerated ONCE here rather
than churned on each of them — a both-directions ratchet in a shared JSON is a pure
conflict generator otherwise.
Guard green after the merge, which is the case it exists for: a merge that resolves
a converted test file leaves its entry stale, and that is the direction that has
already caught something today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): a drifted constant nothing asserts on is invisible both ways
The doc already said to expect redness when the real constant swaps in. The more
common case is the opposite: in one slice nine invented constants were replaced and
none went red, because no affected test named them outside its own factory — so
both sides of any comparison were the fixture's invention.
That is worse than a failure, not better. It cannot fail and it cannot be reviewed,
and six of the nine were plausible variants of the real key
(new-order:sanity-check-failures against a real new-order:sanity-failures) which is
exactly how they survive a reading. Log what the codemod reports as drifted even
when the suite stays green. (archer's slice.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): why the PENDING list misses what it misses, and how to find it
The list was built from repo-wide mock counts. Services is a fan-in problem and
the list covers it — every high-fan-in specifier is on it, first undiscovered one
ranks 12th. Routers is a clique: locally dense, repo-wide rare, so a repo-wide
count structurally cannot see it. Two different problems needing two different
searches, not more of the same one.
The method that finds the missing ones is a per-pair shared-specifier graph for the
directory being worked on, not a sorted repo-wide count.
Also records arabella's untested idea, which is the highest-value thing nobody has
tried: a third of services pairs share NO specifier, so a worker assignment
grouping non-overlapping files could make much of that suite clean under
isolate:false without canonicalising anything — statically evaluable from the same
graph before any code is written.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): migrate arabella's routers/jobs slice, 22 files
Codemod at
|
||
|
Justin Maier
|
7048aadfa1 |
perf(tests): split sharp-executing tests onto their own pool, pre-bundle five externals (#3960)
* perf(tests): route sharp-executing tests to their own forks project sharp 0.32.6's addon is not context-aware, so a worker_threads worker that has run a libvips operation segfaults at thread teardown - after the tests pass and the summary prints. That takes the whole run down with an exit code and no failing test. It is a race, not a threshold. On the six affected files, three repeats per width: vmThreads crashed 3/3 at 1 worker, 2/3 at 2, 1/3 at 3, 0/3 at 4; threads crashed at every width. A green run is not evidence of safety. Importing sharp is harmless; only executing an operation arms it. 100 test files carry sharp in their static closure and exactly six call it. That set was measured by aliasing sharp to a recording proxy and running all 100 under forks, not by grepping and not by a crash-scan - with a race, "ran alone and didn't crash" builds the list out of the files that got lucky. - unit -> the suite minus those six, pool unchanged (forks) - unit-native -> pool: forks pinned, including only those six unit deliberately does NOT move to threads. threads measured 1.04x at 4 workers and 0.94x at 16 - no win - and it segfaults mid-run on the full suite at roughly 1 in 4, after completing hundreds of files cleanly and with no unit-native file having run, so a second crasher exists that is not sharp and is not diagnosed. What the split buys is that the sharp crash is deterministic and gone, so anyone experimenting with --pool=threads no longer has to fight it too. Excluded from unit rather than merely claimed by unit-native, so that a run naming a sharp file under --project unit reports "No test files found" rather than running it on a thread pool if one is ever selected. Shared settings are hoisted into one object both projects spread, so they cannot drift apart. Every selector moves to a unit* project pattern. Verified by file count rather than by a green summary: vitest list over unit* is 1065 files and unit-native is 6, summing to the pre-split baseline. no-sharp-outside-native-project.test.ts is the positive control, since nothing else in the suite can notice this breaking: the realistic failure is a rename, which stops matching unit-native's include AND unit's exclude. Mutation-tested, not assumed green. * perf(tests): pre-bundle five externals nothing mocks Every test file gets a fresh module registry - under forks with isolate: true it is literally a fresh child process per file (N files at maxWorkers=1 give N distinct pids), so Node's module cache dies with it and each externalised package is imported cold once per file that reaches it. Pre-bundling collapses a package's many-hundred-file native load into one chunk, paid once per run. Full suite, control then treatment in one window: control wall 217.2s collect 4730s 1066 files 16787 tests 17 failed treatment wall 192.9s collect 4082s 1066 files 16787 tests 17 failed The failure SET is unchanged, diffed both directions - nothing appeared, nothing cleared. This alters timing, not behaviour. The effect tracks exposure, which is what separates it from ambient drift: reaches 0 of the 5 353 files collect 113s -> 105s -6.9% reaches 1 115 files collect 150s -> 103s -31.2% reaches 2 209 files collect 529s -> 373s -29.6% reaches 3 18 files collect 69s -> 52s -24.3% reaches 5 371 files collect 3869s -> 3449s -10.9% Exposure 5 shows the smallest percentage and the largest absolute saving (420s of 648s) because those are the heavyweight files: five packages are a small share of a 1,300-module closure and a large share of a small one. Percentage tracks share-of-closure, absolute tracks file weight. A control-vs-control run to size run-to-run drift directly was attempted and died to an unrelated crash, so the residual drift term is unmeasured and the figure above should be read as an upper bound. The list is confined to packages nothing mocks, and that is load-bearing. Pre-bundling wraps a package as a CJS-interop chunk, so a vi.mock factory returning only named exports stops satisfying its consumers - adding redis and the mocked aws-sdk clients takes four mock-holding files from 92 tests passing to 7 collected. The importOriginal form does not protect against this. Those three are worth ~275s more and need a default export added to six mock factories first; that is a separate change. The treatment paid its cold optimize pass inside the measured run - the shared .vite cache was not cleared - so the number is not flattered by a warm cache, and a fresh CI runner pays the same thing. * test(perf): pin the native project's pool independently of unit's The guard asserted unit ran on threads, which was the state the split shipped in for about ten minutes. It caught its own config change, which is the behaviour wanted, but the assertion was aimed at the wrong invariant: what must hold is that unit-native stays on a process-based pool whatever unit is pointed at, not that unit is on any particular one. * test(perf): acceptance harness for the six external-mock factories The change that brings redis and the aws-sdk clients into the pre-bundling safelist cannot be verified on a tree that does not enable the optimizer: without pre-bundling the package is not wrapped as a CJS-interop chunk, the missing default export never bites, and a green run proves only that the old config still works. This runs the six affected files under a config with all three candidates pre-bundled. Compares per-file collected counts rather than the total. s3-utils is 66 of the 106, so a sum of 40 could be one file collecting zero and still read as a partial pass. Negative control on the unchanged tree: 5 of 6 files collect 0, and the harness exits 1 naming each one. * docs(test-perf): record the measurement envelope this box imposes Two identical full runs, back to back, nothing changed between them, came out +20.5% apart on collect. That pair was contaminated, so it is not a drift figure - but it demonstrates the box can move further than most of the effects measured today, which means any comparison assembled from two windows is unreadable. Collects the methodology that follows: quote in-pair controls rather than cross-window deltas; a control group must be comparable in cost and not merely in count; a dose-response on an axis confounded with file cost is suggestive rather than conclusive; a crashed run's wall clock is not a fast run; and check for the workload rather than for the runtime when deciding the box is quiet. A clean drift pair still has not been taken and is the denominator for everything else here. * docs(test-perf): scout Bun and node:test as vitest replacements Recommendation is to stay on vitest, but the measurement overturns the cost model we spent the day optimising against. Same 84-module first-party graph: vitest collect 5298ms, bun 3.3ms, node+tsx 8.5ms. Whole-suite arithmetic gives vitest 10.2ms per static module-instance against 0.04ms for bun. The cost is the module runner, not the modules - which matches this morning's tracer result (569 module bodies in ~0.4s against a 25.4s import phase) and locates the time in vite-node's per-module fetch/instantiate rather than in compile-and-evaluate. Unrealisable, though. Bun cannot load any graph reaching the React/Next side - it dies resolving use-sidecar's package exports - so the numbers are measured on the light stratum only, which is the flattering-slice trap: 383 of 1065 files have no infra dependency and the largest of those is an 86-module closure. Module-scope env aborts the import under both runtimes, and cache-helpers hung past 300s under bun after the env gate was satisfied. The mock surface is the wall: 1053 of 1065 files import from vitest, 3883 vi.mock sites across 651 files, plus 8053 vi.fn and the fake-timer, spy and importActual surface. The canonical mock system, its guard, the allowlist ratchet, reporter.mjs, the dashboard and the queue integration are all vitest-shaped as well. Retarget rather than switch: if per-module cost is vite-node overhead, shrinking the graph attacks a term worth ~0.04ms of real work per module, and the leverage is in how many times a module is INSTANTIATED - which is what isolate:false removes. * docs(test-perf): retract the per-module ratio in the runner scouting It divided collect by inventory.json's static module counts, and that artifact was wrong by up to 75x and selectively so - it followed lazy dynamic import edges that never execute and ignored vi.mock factories. Honest suite union is 1321, not 3230. The per-file wall clock the recommendation rests on needs no denominator and is unaffected: same 84-module closure, vitest collect 5298ms against bun 3.3ms and node+tsx 8.5ms. So is the tracer result behind it - 569 module bodies executing in ~0.4s against a 25.4s import phase, measured with no static count at all. No counterpart figure is quoted for bun, because its denominator came from the same artifact. * docs(test-perf): scrub the remaining per-module claims from the runner scout Two survivors of the retraction: an 'orders of magnitude per module' headline and a stratum characterisation quoting closure sizes, both resting on the same broken counts. Restated against the per-file wall clock, which needs no denominator, and the observed hard failure, which is not a count. * docs(test-perf): correct the runner comparison to like-for-like The headline compared vitest's collect for a TEST FILE against a probe importing only the SOURCE module underneath it - a different and much smaller graph. That is where '~1600x' came from. Like-for-like, on the same 82-module test-file closure: vitest collect 5298ms, bun 259ms (median of 5, 250-262). ~20x, not three orders of magnitude. node+tsx cannot import a test file at all - 'Vitest cannot be imported in a CommonJS module using require()'. Per-module refit against aidan's honest closures.json (mode: 'real') joined to the pre-ctl full run: 1065 files, 104797 real module-instances, collect 4729s -> vitest 45.1 ms/module, independently agreeing with aidan's 43.6. bun 3.2 ms/module on the file both can load. The recommendation is unchanged and the mechanism finding is unchanged; the size of the gap was overstated. * docs(vitest): say why the unit projects set no per-project maxWorkers Per-project maxWorkers does apply at runtime, but two projects with different counts need different sequence.groupOrder values, and different groups run serially. For a 1059/6 split that trades the concurrency between them for a knob nobody needs - the six-file project would gate the other 1059 instead of filling spare capacity beside it. Currently reads as an omission, so a future reader adds one and loses concurrency without knowing they traded for it. * docs(test-perf): final form of the runner scouting result Leads with both corrections stated in place rather than silently edited out, and records that neither changed the recommendation. Promotes the cross-validation to a finding of its own: 45.1 ms per module-instance here against aidan's independent 43.6, from a different artifact by a different route. Two wrong denominators would not have agreed, so the pair is what licenses everything downstream that divides by a module count. Names what both errors had in common - each a denominator error producing a number right about the thing it measured and wrong about what that thing was. Checking two runtimes are comparable is not checking the two quantities are. * fix(test): pin unit-native's pool against a CLI --pool, and correct the project selector `unit-native`'s static `pool: 'forks'` loses to a CLI `--pool=threads`: resolveProjects builds cliOverrides from a list that includes `pool` and spreads it after options.test, so the flag wins. The six sharp-executing files would then follow `unit` onto a thread pool and segfault AFTER printing a green summary. configureVitest hooks run after resolveProjects(cliOptions), and getFilePoolName -- `browser.enabled ? 'browser' : project.config.pool` -- is what stamps each spec's pool, so re-asserting there outranks the flag. The other two readers of project.config.pool populate task metadata from the same field and cannot disagree with it. The comment already claimed this guarantee; without the plugin it was false, and false in the reassuring direction. Also corrects CLAUDE.md: the unit suite is two projects now, so `--project unit` silently runs 1059 of 1065 files and exits 0. Select it as `--project 'unit*'`, which is what package.json's own scripts already do. Bound: the pin covers `pool` and nothing else. isolate, fileParallelism, sequence, testTimeout and retry are on the same cliOverrides list and remain overridable. Not verified by a run -- the mechanism was read from vitest 4.0.18's cli-api chunk twice, independently, by two readers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtTG4QQR29eWf7kjM6HiLU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
f4dce6a3c9 |
perf(tests): cut test-body time in the five slowest unit-suite files (#3962)
* perf(tests): move the challenge job-lock wiring assertion off the ladder suite challenge-ladder.test.ts was the slowest file in the unit suite at 37.9s of test-body time, and 35.3s of that was a single test dynamically importing ~/server/jobs/daily-challenge-processing to read one job option. The file is otherwise pure arithmetic (collect 476ms). challenge-jobs-scale.test.ts already loads that module behind a mock preamble, so the wiring half of the assertion lands there at no marginal cost. The arithmetic half (lock < interval) stays beside the constants. challenge-ladder.test.ts 37875ms -> 427ms test-body; challenge-jobs-scale 29ms -> 17ms with its collect unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): stub model.service’s graph in the orphan-relations regression suite prisma-inconsistent-orphan-relations.test.ts loaded the real model.service from a beforeAll with a 120s timeout, which billed the whole graph to test-body time: 31963ms duration against 237ms collect in the full-suite baseline. Replaced the wholesale @prisma/client Proxy stub with the mock scaffold seven sibling model.service suites already use, and hoisted every dynamic import to module scope. The scaffold cuts the graph rather than relocating it: measured in one window, 19.6s wall / 19148ms test-body -> 7.2s wall / 9ms test-body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): stub the lazy DB read in the challenge-category fallback tests The "preset fallback (no DB available)" tests left ~/server/db/client unmocked and relied on a real connection attempt failing. That cost ~4s per call — four calls, 18400ms of test-body time — and made the branch under test depend on what the box could reach rather than on the code. Stubbing findMany to reject drives the same catch, in 39ms. The old suite could not tell the failure path from a successful read of zero rows: merging presets with an empty row set gives the same answer, so flipping the stub from reject to resolve([]) left all 16 tests green. Added the two assertions that separate them — the failure path deliberately does not cache, the success path does — so the stub cannot quietly become the thing under test. 18400ms -> 39ms test-body; 16 tests -> 18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): drive the s3 upload retries on a fake clock, and cover the backoff Six tests in s3-upload.store.test.ts waited out real retry sleeps: withRetries holds a flat second between the three /api/upload/complete attempts, so each of them cost ~3s of wall clock. 16358ms of test-body time, 17.6s wall. They now run through a runUpload() helper that advances a fake clock until the upload settles. The loop is bounded and throws rather than draining while timers happen to exist: at the moment the upload is handed back no timer exists yet (the first is scheduled after fetch('/api/upload') resolves), so a getTimerCount() guard exits before the run begins and the test hangs. Bounding it also means a retry loop that stops terminating fails with a message in 113ms instead of wedging the runner, which is the failure mode the fake would otherwise create. The change exposed a gap it then closes: nothing exercised the TRANSIENT part retry path. Making 429 and 5xx non-retryable left all 17 tests green, because covering it meant sitting through 1s + 2s + 4s + 8s of backoff. On the fake clock it is free, so the path and its MAX_PART_ATTEMPTS bound are now pinned, and getPartRetryDelay - whose policy was observable only as elapsed wall clock - gets real unit tests beside isTerminalCompleteStatus. 16358ms -> 36ms test-body. 17 tests -> 19, and upload-retry 10 -> 17. Also records why audit-matching-equivalence.test.ts must stay slow: its brute-force oracle is valuable because it is a copy, and an optimised copy is a second implementation you believe is equivalent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): hoist the listing-asset service imports out of the test bodies listing-asset-upload-integrity.test.ts reached app-listing-assets.service, offsite-listing.service and stored-object-integrity through `await import()` inside its helpers, so the whole graph was billed to whichever test ran first: 6664ms of the file's 6992ms sat on one test, behind a `collect` of 561ms. This is a RELOCATION, not a cut. Test-body time drops 6992ms -> 773ms and collect rises 561ms -> 6140ms; wall clock is unchanged within this box's noise. It is worth doing anyway because a graph billed to `duration` is invisible to the lane that attacks import cost - the file reads as cheap to import and expensive to run, when the opposite is true. No mock scaffold added. The file is a seam test that deliberately runs the real persist and attach procs with only the backend accessor replaced, so cutting the graph would cost it the thing it exists to check. Verified the suite is still live after the move - mocks apply because vi.mock is hoisted above imports - by making classifyStoredObjectIntegrity always report a match: 11 tests fail across all three asset kinds, including "REFUSES the attach once the stored object is no longer the measured one". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(gate): pin that a wrapper which never launches cannot pass The typecheck-tests gate was checked for the spawn-laundering class found in the packages suite, where execFile on an extensionless node_modules/.bin script never started on Windows and the ENOENT landed in the same `code` field as a real exit status - so a process that never ran reported as one that exited 2. It is not present, and structurally cannot be: the gate spawns process.execPath, which always exists, and passes the wrapper as an argument. A missing wrapper is therefore node's own exit 1 with no diagnostics, which the FAILURE-TO-RUN rule already owns. There is no resolved-binary lookup on this path to fail silently. Verified against the real gate: res.error is undefined, status 3, CANNOT MEASURE. Nothing pinned that, so this adds the case. It asserts the run is refused rather than which guard refuses it, which is deliberate: with the FAILURE-TO-RUN rule disabled the positive-control floor catches the same run at 0 test files against a floor of 844, and pinning one mechanism would make the test fail on a change that left the property intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): stop paying the full port-claim deadline twice Two cases in dev-server-port-reservation.test.ts pass claimPortForReuse a 5000ms timeout and a port that never frees, so each polls for the whole deadline before moving: 10.0s of the file's 10.6s, in two tests. The file's own header claimed the opposite - that a 1ms poll with a generous deadline keeps every case count-bound rather than clock-bound. That holds for the cases where the port FREES, where a probe mock ends the loop. It cannot hold where the port never frees, because expiry is the mechanism under test. Those two now take a 60ms deadline. Shortening it cannot make them flaky: the assertion is that the session moved, and the move is what happens once the deadline passes, however few probes fit inside it. The comment now says which cases are which. 10600ms -> 734ms test-body, 37 tests unchanged. Mutation-tested, both directions, at the new deadline: drop `session.port = moved` -> expected 3101 to be 3102 (68ms) make findAvailablePort ignore other sessions' reservations -> expected 3100 to be 3102, and expected 3100 to be 3103 (67ms, 71ms) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): drive the reward batch-retry backoff on a fake clock `(e) rethrows on update failure in the batch process path` waited out updateBuzzEvents' real retry budget - 5 retries at a 500ms backoff - costing 2556ms of the file's 3837ms. The budget is the behaviour under test, so shrinking it would change what the test covers. The clock is driven instead, with a bounded loop and the assertion awaited afterwards, so a retry chain that stopped terminating fails on the rejection rather than spinning. 3837ms -> 1270ms test-body, 10 tests unchanged. Adds an attempt-count assertion, which pins the retry budget and doubles as the control on the fake clock: without it driving the backoff those attempts would not have happened. That assertion also corrected a wrong reading of my own. It pins the literal `5` that updateBuzzEvents passes to withRetries, NOT BATCH_RETRY_COUNT, which is addBuzzEvent's default and which this path never reads - dropping BATCH_RETRY_COUNT to 1 leaves the file green, dropping the literal gives "expected 6 times, but got 2 times". The comment records both so the next reader does not credit the constant with coverage it does not have. Mutation-tested: batch path swallows instead of rethrowing -> promise resolved "undefined" instead of rejecting updateBuzzEvents retry literal 5 -> 1 -> expected "vi.fn()" to be called 6 times, but got 2 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
21ab65ba14 |
fix(tests): make the packages and unit suites readable on Windows (48 failures -> 0) (#3961)
* fix(tests): run the drift CLIs through node, not the POSIX bin shim `node_modules/.bin/tsx` is a shell script, so `execFile` on Windows failed ENOENT before any CLI started — 32 tests across the three schema-drift files, i.e. every case in `test:packages:run` that spawns a CLI, red on any Windows checkout. `execFile` reports that in the same `code` field as a real exit status, so they read `expected 'ENOENT' to be 2`: an assertion about the CLI's behaviour rather than about a CLI that never ran. Resolve tsx's own entry and spawn it under `process.execPath` instead, and throw when a spawn produced no numeric exit code so the two can never be confused again. Mutation-tested: dropping `process.exitCode` in gate-cli gives `expected +0 to be 1`, and dropping the `capturedAt` stamp in cli.ts gives `expected undefined to be truthy`. * fix(tests): give the source-scanning guards posix path identifiers Four drift guards build a repo-relative path with `path.relative()` and then use it as an IDENTIFIER — matched against `/`-separated literals, or split on `/`. On Windows `path.relative()` returns backslashes, so the match can never hit and 14 tests were red on any Windows checkout. Normalise separators at the point the path stops being a filesystem path; nothing asserted changes, and the paths handed to the filesystem are untouched. One of these did more than go red. In app-spend-tier-privilege the mismatched key is `alwaysDecode`, whose stated job is to read the publisher-facing modules unconditionally so that a renamed or deleted path is loud rather than a quietly vacuous pass. With backslash keys it never hit, so it force-read nothing and could not tell a stale path from a present one — the mechanism against a vacuous pass was itself vacuous. Any Windows checkout has been in that state since the guard was written. Mutation-tested, each fix separately: - rename a PUBLISHER_REACHABLE entry -> `blocks.router.RENAMED.ts was not read — is the path stale?: expected undefined to be defined` - drop 'generation-resources' from KNOWN_STATIC_ENDPOINT_SEGMENTS -> `expected '/api/v1/blocks/:seg' to be '/api/v1/blocks/generation-resources'` - drop the 'user-settings:write' label -> `expected [ 'user-settings:write' ] to deeply equal []` - change orchestrator-chat's wait to 60000 -> `expected 60000 to be less than or equal to 150`, and the ledger diff names `server/services/comics/orchestrator-chat.ts:60000` * fix(scripts): name emitted chunks with posix separators, and cover exit 137 The server-graph gate keyed its chunk map on `relative()` output, so on Windows every violation named `chunks\ssr\b.js`. That key is what the report prints, so it is a name, not a path: normalise it, and keep the absolute path beside it so reading a chunk never goes back through the key. `typecheck.test.ts` simulated an outside kill with SIGKILL, which Windows cannot deliver — the child exits 1 with `signal === null` and the wrapper correctly reports a generic crash instead. Skip that case there for the stated reason and add the other half of the same branch, `exit 137`, which is what a container actually reports and which runs everywhere. Mutation-tested: dropping `|| code === 137` from the wrapper's classifier fails the new case with `expected '...TYPECHECK CRASHED...' to contain 'killed from outside'`. The gate's own synthetic negative control covers the chunk name. * fix(tests): resolve tsx per call, read the walked path, name a signal kill Three review findings on this branch, all one-liners, all the same shape as the bug the branch fixes: a failure reported as something other than what it is. `tsx/cli` was resolved at MODULE scope. All three drift test files import that module, so a resolve that throws — a tsx release dropping the `./cli` export subpath, a partial install — would take all three down during module evaluation and each would collect ZERO tests while the failure count stayed 0. Resolving inside the call surfaces it as a test failure naming the module instead. Latent, not live: tsx 4.20.3 declares `./cli`. `app-spend-tier-privilege` now reads the absolute path the walk produced rather than `join(ROOT, <normalised key>)`. The key is an identifier; handing a posix-separated string back to the filesystem works today and would not under a `\?\` prefixed path. This is also what the PR body already claimed it did. `runTsxCli` announced a signal-killed process as "did not run", which sends the reader looking for a spawn failure. A signal kill did run. |
||
|
Justin Maier
|
93ee73cd84 |
test(perf): instrument the unit suite and track the isolation migration (#3957)
* test(perf): instrument the unit suite and track the isolation migration
The unit suite spends 81% of its worker time importing, and nothing in the repo
could say which modules or which files. This adds the measurement that answers it,
plus a dashboard so the isolation migration has a burn-down nobody has to maintain.
The finding that motivated the shape of it: traced at 1 worker, the module BODIES
of two of the heaviest test files total ~0.4s against a 25.4s import phase. The
cost is vite-node's per-module fetch, which under the `forks` pool is a
child-process IPC round trip per module per file. So cost is linear in module
COUNT, not module weight, and the static import graph is the right thing to rank by.
- graph.mjs static first-party import graph + vi.mock inventory
- reporter.mjs per-file collect/setup/test timings from any run
- bench.mjs fixed 90-file stratified yardstick, so two measurements taken
hours apart by different people are comparable
- sweep.mjs pool x isolation x worker-count matrix, run back to back
- trace-* module-execution tracer; counts what actually ran, which the
static graph cannot know (a vi.mock factory stops the real
module and its subtree from executing)
- why.mjs shortest import path between two modules
- dashboard.mjs builds .test-perf/dashboard.html from whatever is on disk
Output goes to .test-perf/, gitignored. Nothing here runs in CI or changes how
any suite executes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): add the per-worker union report, and record three dead ends
Under isolate:false a worker keeps one module registry, so its cost is the UNION
of what its files import rather than the sum. order.mjs reports that union, which
is the number that bounds what removing isolation can deliver: measured at ~36
seconds per worker to build, near-constant at 8 and at 24 workers, and a wall-clock
floor more workers cannot shrink.
Three things measured today that do NOT help, written down so the next person does
not pay for them again:
- Affinity file ordering. A greedy graph-similarity sequencer gave a mean per-worker
union of 1139 modules at 31 workers against alphabetical's 1084 - slightly worse.
Alphabetical already groups by directory and directory already correlates with the
graph. The sequencer is deleted; the measurement is kept.
- NODE_COMPILE_CACHE. Cold 26.8s, warm 51.4s, warm again 33.9s on the yardstick. The
cache filled (5.3MB) so it was active; vite-node does not evaluate through the
loader it covers.
- vmThreads. Two clean 90-file runs, then the five sharp-executing files crashed or
passed on identical input at 2 and 3 workers. It is a race, and CI's 4 vCPU
resolves to the width measured at 1-in-3 SIGSEGV.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): three measurement rules this project had to learn twice
- The 90-file yardstick understates isolate:false and cannot judge it. That flag
amortises the registry build across the files a worker runs, so its win scales
with files-per-worker: 90 files at 16 workers is ~6 each and measures 1.65x,
while 1065 files at 8 workers is ~133 each and measures 16x on the same phase.
- No --no-isolate number is quotable without a per-file collected count. Its damage
is not only failing assertions: files silently collect ZERO tests, and how many is
width-dependent (9 of 90 at forks/4, 14 of 90 at threads/4, 0 at threads/16, same
input). A summary line cannot show this.
- The noise floor on a shared box is +/-30%: one configuration measured 53.3s and
76.6s in a single session. Below ~20%, quote phase numbers or nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): show the guard allowlist as the migration burn-down
The dashboard was deriving migration status from the static vi.mock inventory,
which is an estimate. The authoritative number is the length of the guard's
allowlist: it ratchets in both directions, so a new direct mock fails and a
migrated file left on the list fails too, and it therefore cannot drift from what
the suite will actually accept.
Reads it from the working tree when present, otherwise from the branch carrying
it, so a dashboard built on main still shows the real number rather than nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): name the baseline, retract the IPC mechanism, fix the yardstick's promise
Three defects found by an adversarial review of this PR, all in the class of
asserting something nobody re-checked.
- The README quoted one baseline while shipping an artifact with different
numbers for the same tree - 4565.5s against 5476.3s of import, 36% apart. The
tree did not change; the box did. All three measurements of `3863adcbb0` are
now tabulated with the rule that a main-relative figure must name the run it
was measured against, because that spread is the same size as several of the
effects being measured against it. The derived import share is 81% or 84.3%
depending which row you pick, and both are now stated.
- The per-module IPC mechanism was retracted in mail hours ago and left standing
here. The pool sweep refutes it: `threads` beat `vmThreads` while paying a cold
fetch, which shipping module source cannot explain. Now labelled inferred, with
the competing reading and the note that V8 compile time was never instrumented.
- The yardstick claimed it made measurements "hours apart" comparable, three
bullets above a +/-30% noise floor measured inside one session. It fixes what is
measured, not when. Also records that a null from a 90-of-1065 sample is not
evidence of no effect - two changes measured flat there were later shown real.
And the gitignore claim is now scoped to "by this change", since it is false on
main until this merges.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): count the modules a worker loads, not the ones a bundler compiles
The static graph over-counted a page-gate test by ~75x, which put four of the
cheapest files in the suite at the top of the closure ranking. Their measured
worker time ranks 202-572 of 1065.
Four independent causes, each removing modules the naive walk counted:
- lazy `import()` is not followed, EXCEPT from a test file itself. A
`dynamic(() => import())` in a page never runs; an `await import()` in a
test body is what the test exists to do. Collapsing the two makes those
files either the top of the ranking or 1 module each.
- a `vi.mock` factory without `importOriginal` truncates the subtree behind
it. The mocked module itself is still counted -- registering the mock is
what causes the transform -- and is counted even when nothing imports it.
- `import { type X } from` erases the statement. Only `import type {` was
handled, and the inline form is the common shape here.
- a line filter cannot strip a multi-line `import type`: it leaves
`} from '...'` behind, and IMPORT_RE's lazy `[\s\S]*?` glues that orphan
onto the previous import, inventing an edge. Stripped as statements now.
Plus `event-engine-common` in SRC_DIRS -- a submodule imported by relative
path from src/server/services, so its modules and the civitai-db-queries
files reachable only through it were invisible.
Validated by diffing the model against a transform-hook trace as SETS, not
counts: 3 of 5 files exact with empty diffs both ways, 16 modules of
symmetric error across 691 traced (2.32%). Counts alone agreed often enough
to hide two of the four causes -- a count agreeing is not the rule agreeing.
`graphModules` is now the honest count and stays the default field, so
dashboard.mjs is corrected without a change of its own; `graphModulesRaw`
keeps the bundler view. closures.json gains `mode: 'real'` and a note
describing the truncation rules, so a consumer can refuse a naive one.
Also flags the two order.mjs union figures as pre-honest levels needing a
re-run; the ordering conclusion is a ratio and survives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): stop the type-statement stripper eating real code, and record maxWorkers honestly
Three review findings, all mine.
1. graph.mjs -- the type-statement stripper swallowed spans of real code.
`TYPE_STATEMENT_RE` also started matching on a bodyless `export type X = ...`
(no `from`), and the lazy `[\s\S]*?from '...'` then scanned FORWARD across the
file to the next `from '...'`-shaped text -- inside comments and strings
included -- deleting every statement in between before IMPORT_RE saw them.
src/server/common/enums.ts lost a real `@civitai/notifications/constants`
edge across a 7,881-character span.
The gap is now tempered: between `type` and `from` a real type-import
statement holds only a binding clause, so it may not cross `;`, `=`, or
another `import`/`export` keyword. Measured across 5,294 files, spans of
more than 12 lines fall from 73 to 28, and every one of the 28 remaining is
a genuinely long multi-line type-import list. Re-validated against the
transform-hook traces: symmetric error 2.32% -> 2.03% over 691 traced
modules, so the fix is strictly in the right direction.
Direction of the bug was UNDER-count, and `graphModules` is the ranking key
for the dashboard, order.mjs and bench.mjs --make-subset. The earlier
validation did not cover it: 5 files of 1,065, none of the affected ones.
2. reporter.mjs -- the one unguarded filesystem call. It sits inside an awaited
vitest lifecycle hook, so a throw (read-only cwd, `.test-perf` existing as a
file, ENOSPC, an AV EPERM) would red a run whose tests all passed, and the
failure would be attributed to the code under test. A measurement tool must
never be able to fail a suite; losing the recording is the correct trade.
3. reporter.mjs -- `maxWorkers` is not on vitest 4's `ctx.config`. Verified by
execution: the emitted config was `{isolate, pool, argv}`. Every run ever
recorded therefore stored null, and dashboard.mjs rendered all of them as
"(default) workers" -- claiming a fact nobody measured. Recovered from argv
(`--max-workers=8`, `--max-workers 8`, `--maxWorkers=8`), falling back to
VITEST_MAX_WORKERS, and reported as 'unknown' rather than null when neither
is present. The env var and the flag are recorded separately because they do
not behave the same -- the flag reaches a queued run and the env var does
not. The dashboard now shows historical nulls as 'unrecorded', not
'default'.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Justin Maier
|
6f54a21085 |
feat(dev-server): per-service dev/prod env modes on start (#3954)
* feat(dev-server): per-service dev/prod env modes on start A session picks one .env and now applies a per-service overlay on top of it: `start --prod buzz` moves Buzz to production and leaves everything else on dev. Groups come from a gitignored env-modes.local (db, buzz, search, signals, redis today), so adding a service is an edit to that file rather than to code. Every group defaults to dev. DEVSERVER_PROD_GROUPS in the skill .env moves a default when a dev service is unreliable, and a flag beats both. The flag applies to that start only, including when it takes over a dead session, so a prod choice never leaks into the next bare start. Asking for different modes while a session is already running that worktree is refused rather than answered with the running session. The orchestrator, payments, S3, ClickHouse, the notifications DB, the feeds proxy and OpenSearch have no dev counterpart at all, so the summary prints them after every mode line: dev mode is not a claim that nothing here is production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): close the ways an env mode could lie about itself Review found four paths where a mistake resolved quietly to the wrong environment, three of them ending on production while the session reported dev. A malformed section header (`[db prod]`, a missing bracket) matched neither the section nor the KEY=VALUE pattern, and the parser left the previous section open — so every following key landed under it, filing the production DATABASE_URL as `[db.dev]`. Any parse error now closes the open section, and the daemon refuses to start at all when the definitions file did not parse cleanly, rather than logging a warning nobody reads and carrying on. `all` was expanded before group names were validated, so `--prod all,typo` discarded the typo and `--prod all` against a machine with no env-modes.local reported success having moved nothing. Names are checked first, `all` errors when it matches no groups, and it now selects only groups that have that mode instead of failing the whole start over one dev-only group. `--prod=` with an empty value errors like the spaced form already did. A DEVSERVER_PROD_GROUPS entry that matches no group produces a note instead of silently leaving the service on dev — the failure an operator setting it is specifically trying to avoid. The busy-session refusal compared flag text, so it refused `--prod all` against a session started with every group named, and accepted a bare start against a session whose modes a bare start would no longer produce. It compares resolved modes now. That refusal also broke the dashboard: console.mjs sends no modes, so a 409 sent it down a fallback that picked whichever session was first in the map — another worktree, another branch, its logs, silently. It watches the refused session when one is named and otherwise looks only at this worktree. The example understated each group's key set: db without DATABASE_IS_PROD (which gates the S3 delete paths) or DATABASE_REPLICA_LONG_URL left a session writing to production and reading long queries from dev, and redis without REDIS_CLUSTER would point a single-node client at a cluster. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): stop the mode guard reporting a stale or partial answer Second review round, on the hardening itself. The 409 mismatch check read `session.modes`, which only start() writes. On the takeover path the request's modes were stored and restart() awaited, so for the seconds spent stopping and reclaiming the port the session still described the run being torn down — long enough for another agent's bare start to match it and be handed 200 for a session coming up on production. The resolved modes are stamped before the await now, and cleared at the top of start() so a session that errors out stops reporting an env it never applied. The endpoint also resolved against a cached skill config while start() reloaded it, so an edited DEVSERVER_PROD_GROUPS made the two disagree; it reloads first. `--prod all --dev search` — everything on prod except one — threw a conflict, because the conflict check ran after `all` expanded. Expansion now skips groups named on the other flag and conflicts are judged on explicit names only, so the exception form works and `--prod all --dev all` is rejected on its own terms. A group named beside `all` that has no section for that mode is an error rather than a silent fallback to dev. Parser errors no longer echo the offending line: they reach an HTTP 400 body and a log buffer any agent can read, and a stray connection-string line splits on its first `=` with the password on the left. The example understated three more groups: search without METRICS_SEARCH_* is a second Meilisearch the app writes to, redis without REDIS_SYS_SENTINELS leaves the system client on production (with sentinels set, the URL supplies only the password), and db without DATAPACKET_DATABASE_RO_URL keeps a read pool on production Postgres. The dashboard counts a `base` group as production, since base means the .env value stood and this .env is production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(dev-server): pin the env-mode rules, and stop the guard overreaching Third review round. The resolver was the most intricate code on the branch and had no test file — the checks proving it lived in a scratchpad, which protects nothing after this session ends. `scripts/__tests__/dev-server-env-modes.test.ts` now covers the resolution rules, the overlay, the parser and the session comparison, beside the queue's tests for the same reason (the daemon is not in the app's module graph). Both mutations fail legibly: reverting the parser fix prints `expected 'PROD-host' to be 'dev-host'`, and reverting the all/dev exception prints the wrong mode map. Fixes with it: The session comparison compared whole maps, so ADDING a group to env-modes.local made every already-running session disagree with every new bare start — `start`, which is meant to be idempotent, would 409 everywhere until each session was restarted. It compares the groups both sides resolved. A group only one side knows about is a definitions edit, not two agents wanting different environments. `DEVSERVER_PROD_GROUPS` is pinned on the session when it is created rather than re-read on every start, because start() also runs unattended — a branch switch, a crash restart — and an edit to that file would otherwise move a LIVE session onto production with nobody having asked. A start refused over its env modes answered 201 and the CLI exited 0, so `start && curl` proceeded as though a server had come up. It answers 5xx with the session's own error line, on both the new-session and the reuse path. The takeover stamp is `pendingModes`, separate from `modes`: a crashed session's process can still be serving on the old env for the length of the port wait, so `modes` stays true to what is running and the mismatch check reads what is coming. `PROD_ONLY_GROUPS` is filtered by what the definitions file defines, so defining `[s3.*]` no longer produces a summary that says `s3=dev` and `always prod: s3` on the same line. Documented rather than fixed: the auth hub is one shared process reading its own apps/auth/.env, so it cannot follow a per-session db mode — a `--prod db` login mints a token for a user id from the other database. And the build dir is keyed on branch, not mode, so changing search/signals mode on a warm `.next` can leave the previous NEXT_PUBLIC_* host inlined in client chunks; keying it on mode would multiply an 8GB cache per combination. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): close the fail-open half of the mode comparison Fourth review round, and the worst finding was in the previous round's fix. Comparing only the groups both sides resolved made an EMPTY request match anything: with env-modes.local momentarily absent — an atomic-rename save, a move, a git clean — a bare start resolved to no groups, the mismatch guard did not fire, and the daemon handed back the running session as though it were the dev one asked for, while it ran on production. The comparison is asymmetric now. A group the request has and the session does not is an added section and still matches, which is what keeps `start` idempotent; a group the SESSION has and the request lost does not. `auth-hub` stays in the always-prod list whatever env-modes.local says. Defining [auth-hub.dev] cannot move the hub — it is a separate shared process reading its own apps/auth/.env — so letting a config edit delete that warning would leave the summary asserting something the mechanism cannot deliver. `--prod all` now leaves a note naming each group it could not move for want of a section, instead of being the one unhonourable request in this resolver that says nothing. The takeover stamp is cleared in a `finally`, so a restart that throws cannot leave a session advertising modes that will never be applied. And the dashboard reports a refused start even when it finds a session to attach to anyway — a malformed definitions file returns no session in the body, and repainting a healthy dashboard over the pre-edit session read as the edit having worked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): close the remaining fail-open directions in the mode guard Fifth review round, and again the findings were the mirror image of the previous round's fixes. Each is the direction where a mistake resolved to production while the summary said otherwise. A session that resolved NO groups came up before env-modes.local existed and is running the base .env. `every` over no keys is vacuously true, so once the file appeared, the first bare start was told its dev modes held while the process was still on the .env. An empty running side now matches only an empty request. `--dev all` left any group with no [x.dev] section on the .env — production — and succeeded with a note, while `--dev <group>` for the same group threw. The safety direction should not be the one that fails open, so it throws too. Skipping stays a note in the prod direction, where the group stays on dev. `formatModeSummary` dropped a prod-only service from the always-prod tail whenever it resolved to `base`. `base` means no section applied and the .env value stands, which for those services is production — so a half-written definition removed the warning exactly when it was most needed. Only a group that actually moved leaves the list now. `auth-hub` was described as unmovable but nothing enforced it: defining [auth-hub.dev] resolved normally and wrote its keys into the MAIN app's env, repointing the app at a hub that is not listening while deleting the warning. Defining it is refused at load. And a reuse that threw left the failed request's --prod set pinned to the session, so the next unattended restart would have brought it up on production off the back of a start that errored. The previous overrides are restored on that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): a named group the session never had is a mismatch Sixth review round, two more directions into the same guard. Leniency about a group the running session does not know is only defensible when nobody asked for it. `--dev search` against a session that started before [search.*] was defined came back 200 "existing", CLI exit 0, while search was still on the base .env — production Meilisearch. Groups named on a flag are a hard requirement now; a bare start against a session missing a newly-defined group is still idempotent, which was the point of the leniency. The override restore lived only in `catch`, but start() reports a mode failure by setting status and RETURNING rather than throwing — the exact case the 500 branch handles. So a failed `--prod db` left the session pinned to it, and the dashboard's restart key would have brought it up on production off a start the CLI reported as failed. Also: the dashboard's mismatch notice was a 3-second flash, after which a dashboard attached to a session that is not what it asked for looked identical to a healthy one — it carries a sticky `!env` marker in the header now. And the redis example restates REDIS_CLUSTER_NODES and REDIS_SYS_SENTINEL_PASSWORD in both blocks, since the overlay applies onto the .env rather than onto the other mode, and the file's own rule says every key of a service moves together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
4f619f0f39 |
fix(tests): clear the three type errors that reddened the typecheck-tests gate (#3956)
* fix(tests): clear the three type errors that reddened the tests gate `pnpm typecheck` excludes `src/**/__tests__/**`, so these landed green: - minimax-h3-license: `BaseModel` is re-imported, not re-exported, by `~/server/common/constants`. Take it from its origin. - cosmetic-phash: BigInt literals need ES2020; the repo targets ES2018. - sticker-placement: `createStickerPlacement` grew a required `spendType` and the shared fixture never did, so 17 call sites disagreed with it. The fixture carries 'yellow' deliberately — the escrow test asserts 'green', so a placement that dropped the caller's currency cannot pass by matching the fixture. Typing the fixture as `CreateStickerPlacement` means the next required field fails once, at the fixture, not at every call site. Baseline regenerated: sticker-placement leaves it entirely (16 -> 0), and remix-gallery drops 29 -> 28 from an unrelated merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(sticker-placement): make the currency guard structural, not a comment The escrow test's non-vacuity depended on the fixture default differing from the value it asserts. That was a comment; now the two are named constants and the test asserts they differ, so a future edit that collides them fails on the spot instead of going permanently green against a service that stopped forwarding the caller's currency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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> |
||
|
Justin Maier
|
510952f37a |
feat(cosmetics): version cosmetic hashes, close the write-path hole, surface near-matches (#3948)
* feat(cosmetics): version cosmetic hashes, close the write-path hole, surface near-matches Two badges in a creator shop imitating official Civitai badges were reported and nothing flagged them. Auditing why turned up three separate problems. **Nothing read the hash.** `Cosmetic.pHash` has been written since 2026-08-01 and is consumed by no code at all — no lookup, no threshold, no mod surface. The review panel's only originality check is a sha256 of the uploaded bytes against other creator-shop submissions, which (a) a re-encode defeats and (b) cannot see official cosmetics, since they carry no `imageHash` (468 of 829 shop items have one). Cosmetic 1162 "Civitwave" is a distance-0 clone of official cosmetic 322, published and on sale, and that check passed it with a green "Original artwork". **Two write paths never hashed.** `submitCreatorShopItem` created the cosmetic inside its transaction and `updateCreatorShopItem` replaced `data.url` with a raw update, neither queuing a hash. That is where 228 of the 231 unhashed cosmetics in prod came from — every creator submission since the one-off backfill ran, growing at roughly the submission rate. **64 bits is too few, and that is not ours to fix.** Measured over the 950 hashed cosmetics: the two reported imitations sit at Hamming 17 and 22 against a corpus whose 1st percentile is 17 and median is 31, so no threshold separates them from unrelated artwork, and a top-N list ranks the real match 7th and 67th. The orchestrator's other lane, `perceptualDct`, is also 64 bits and does not help (14 and 24 on the same pairs, probed live). A wider hash has to come from the orchestrator; a 256-bit DCT over the same artwork ranks both pairs 1st with nothing false above them. So this ships the machinery and leaves it dark: - `pHashHex` + `pHashVersion` + `pHashCheckedAt`. The hash is TEXT so a width change needs a re-hash, not a migration, and the version names the lane, since hashes from different algorithms are independent with nothing to signal it. The migration seeds `pHashHex` from the existing BIGINT as the same lowercase hex the orchestrator originally returned, so no row needs re-hashing today. - `cosmetic-phash-sweep`, every 15 minutes, replacing the one-off backfill script. It re-hashes anything missing, stale-against-its-artwork, or in an old lane — so raising `COSMETIC_PHASH_LANE.version` drains the whole corpus by itself when the orchestrator offers more bits. `pHashCheckedAt` is stamped on every attempt, including failures, so the three cosmetics whose CDN artwork is gone cannot starve the queue by matching the predicate forever. - Both write paths now queue a hash. - `creatorShop.getSimilarCosmetics` behind the `cosmeticSimilarity` flag, which is `availability: []` — dark and failing closed. It gates the lookup, not just the panel: a ranked list built on a hash that cannot rank is read by a mod as a ranking. Candidates are gated on lane and on `pHashUrl = data->>'url'`, and the all-zero hash is excluded — 19 near-transparent decorations share it, so an equality match on it returns them all. - A review panel with three distinct states. "Compared against N, nothing close", "these are the nearest", and "this was never fingerprinted" are different instructions to a mod and a blank panel says none of them. The migration needs applying by hand (prod + dev); nothing is user-visible until the flag is created and turned on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cosmetics): make two of the lookup's assumptions unfalsifiable rather than assumed Both of these are currently unreachable. Neither is asserted anywhere, and a future width change is exactly when they stop being true. The sweep's retry window was `NOW() - $1::interval` with the text `'24 hours'` as the parameter. That relies on the driver leaving the parameter untyped for Postgres to coerce, which is a runtime property nothing here tests, and the whole job throws if it is wrong. `24 * INTERVAL '1 hour'` takes an integer parameter and cannot be ambiguous. The candidate query now also filters on hash length. Every row in a lane is the same width by construction — the migration's `lpad(to_hex(...), 16)` cannot produce anything else (`to_hex` on a bigint is at most 16 chars, verified), and `normalizeCosmeticHashHex` pads — but `hammingDistanceHex` THROWS on a width mismatch, so a single odd row would 500 the review panel rather than being skipped. Filtering in SQL makes the throw genuinely unreachable instead of merely unlikely, and the throw stays because a distance between two widths is a meaningless number that looks like a meaningful one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cosmetics): stamp the hash retry window on failure only, so a lane bump can drain The sweep ANDs two predicates: "this row needs re-hashing" and "we have not tried it in the last 24 hours". The second was stamped on every attempt including successes, which quietly defeats the first. Raising `COSMETIC_PHASH_LANE.version` is the whole upgrade path — it is supposed to put every row back in scope and let the job drain them. But every row hashed in the previous day carries a fresh timestamp, so the retry window excludes it, and the rows a lane change most needs re-hashed are precisely the ones hashed most recently. A cosmetic submitted ten minutes before the bump sits in the old lane for a day, and the review panel reports it as unhashed for that whole day. Nothing fails, nothing logs, the job reports a clean run. So the column is `pHashFailedAt` now, not `pHashCheckedAt` — the name says what it is for. It is written when hashing fails and cleared when it succeeds. Suppressing failures is the point (three cosmetics point at dead CDN objects and would otherwise match the predicate on every tick forever, ahead of rows that would succeed); suppressing successes was never wanted and only looked harmless because the two clauses are far apart in the query. The migration is not applied anywhere yet — prod has `pHash` and `pHashUrl` only — so this renames the column rather than adding a second one. Two assertions pin it, both mutation-checked: stamping on success fails with `expected 2026-08-15T01:22:07.866Z to be null`, and moving the retry window onto any other column fails with `expected 'AND (\n "pHashVersion" IS NULL…' not to contain '"pHashVersion"'`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cosmetics): six defects from review — a green pass on zero comparisons, and five more An inline review pass over the branch found six. Every one is a case of the panel or the sweep reporting something reassuring that it had not established. **A clean bill of health computed from nothing.** The candidate set is filtered on the current lane, so immediately after a lane bump it is empty by construction and stays partial for the whole drain. `getSimilarCosmetics` reported that as `status: 'ok'`, which the panel renders as a green tick and "No similar artwork found — compared against 0 cosmetics". Every submission reviewed during the drain got an affirmative all-clear built on zero comparisons. Empty corpus is now `unavailable/no-corpus`. **A permanent spinner on failure.** The card had no error branch, so a failed lookup rendered "Comparing against every fingerprinted cosmetic…" forever. A moderator could not tell "still working" from "this never ran" — the exact ambiguity the card's own header comment says it exists to remove. **An answer that could never refresh.** The repo default is `staleTime: Infinity` and nothing invalidates this query, so the "not fingerprinted yet, picked up within about 15 minutes" state could never be satisfied by coming back, and the panel kept showing a pre-swap comparison after a mod replaced the artwork. **A slow submission treated as a dead one.** `getPerceptualHash` returns undefined both for a real failure and for a workflow still running when the 30s wait elapses, and the write path cannot tell them apart. It was stamping `pHashFailedAt`, putting a merely-slow badge behind the 24h backoff while the panel promised ~15 minutes. The write path no longer stamps; the sweep retries it next tick and stamps only if it fails there. **Cross-lane mixing in the one column that cannot describe itself.** The legacy `Cosmetic.pHash` write was gated on hash WIDTH, and `perceptualDct` is also 64 bits — so bumping the lane to it would have written DCT values beside the perceptual ones, in the single field with no version column to tell them apart. Now gated on the lane, and extracted to `legacyBigIntHash` so it can be tested at a lane we do not yet run: inline, the correct and incorrect forms are identical today and only diverge when nobody is looking. **A tally that could exceed its own total.** The sweep incremented `failed` inside both the try and its catch, so a row whose failure stamp itself threw was counted twice and broke `scanned === hashed + failed` — the only signal anyone reads about this job. All six pinned by assertions, each mutation-checked to fail with a named message: "expected { status: 'ok', …} to deeply equal { status: 'unavailable', …}", "expected -6421916719099894602n to be null", "expected vi.fn() to not be called at all, but actually been called 1 times". Also adds the shop status to each match — a look-alike of something already rejected is a different decision from a look-alike of a live listing, and the two were indistinguishable on the row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
a94f1b70f5 |
feat(dev-server): serialise unit-test runs behind the daemon (#3947)
* feat(dev-server): serialise unit-test runs behind the daemon The unit suite takes every core. One run is fine; several agents each starting one at the same moment is what flattens the machine, and capping the worker pool per run does not stop that. This adds the scheduler. Two calls, because a caller needs to know where it stands before it decides how to wait. `test run` returns immediately with either "started" or a position plus the command to wait on; `test wait` blocks until the run finishes and exits with its exit code. The wait polls from the CLI rather than holding a daemon request, since the daemon is single-process and a held request would stall every other agent's polling. The daemon owns the run, not the caller, which is what makes a dead agent harmless: it releases nothing because it was holding nothing. Slots are held while a run is tracked and released on child exit, never on a status field -- status is a report, not an observation, and cannot see a grandchild that outlived a kill. Three deadlines close the rest: a queued run whose caller stopped polling is dropped, a run that overruns its ceiling is killed, and a kill that produces no exit frees the slot anyway after a grace period. Concurrency defaults to 1 and is configurable. 0 is legal and means paused, with the pause reported to callers rather than left to be inferred from a position that never moves. The 404 on an unknown run id is load-bearing rather than incidental: it is how a waiter learns the daemon was restarted and its run is gone, instead of polling for a result nobody will produce. * fix(dev-server): close four holes an adversarial review found in the queue 1. `test wait` exited 0 for a cancelled or timed-out run whose child happened to exit 0 -- the window between a kill being issued and it landing. This is the worst possible failure for a command meant to substitute for the suite in a verification chain, because it reports a green run that never finished. Only a completed run that itself exited 0 is a pass now, and the decision lives in one exported function so it can be tested rather than inferred. 2. A malformed TEST_CONCURRENCY killed the daemon at import. The queue is built at module scope and the constructor throws, so a typo in an optional test setting stopped the daemon binding at all -- taking every agent's dev server, session list and worktree tooling with it. It now falls back to 1 and says so, like every other setting in that file. 3. A runner reporting its exit synchronously lost the event, because the listener was attached after the runner returned. The finished run held the only slot until the 30-minute ceiling while everything behind it was abandoned rather than run, and it then settled as `timeout` with an error string that was false. The exit path is now built before the runner is called. 4. The line whose removal produces exactly the permanent wedge this feature exists to prevent had no coverage: 15/15 passed with it deleted. The test sweeps inside the grace the way the daemon's own 5s timer does, so a reset deadline now fails as `expected [] to deeply equal ['<id>']`. Also: children get their own process group on POSIX, without which killing by negative pid names no group and silently leaves vitest running while its slot is handed on -- serialisation quietly becoming concurrency 2 under exactly the load this is for. Daemon shutdown kills synchronously so the child cannot outlive it. Every fix has a mutation control: reverting each one fails on a named value. * fix(dev-server): three more from the second review pass The identity half of the exit guard was untested -- 21/21 passed with it removed. It only fires when a late exit arrives after a force-release, and no test delivered one. It was defended in practice only by the exit-code rule catching the consequence, which is two fixes covering for each other rather than either being pinned. Now a test force-releases a slot and then delivers the exit. `exitCode || 1` passed a signal death straight through as -1, which a shell sees as 255 -- and `detached` + SIGKILL means that is now the normal shape of every cancel and timeout on POSIX. Real failing codes still pass through; anything that is not a positive integer becomes 1. The exit-code table gains a row with a distinctive code, since every row it had was one whose answer is 1 anyway, so it could not tell a passed-through code from a hardcoded one. A child that dies by signal reports no code at all. That is an OOM kill or an outside hand, not a verdict on the tests, so it settles as `error` rather than claiming a test result nothing produced. `execFileSync` on shutdown had no timeout. It runs on the daemon's event loop and is also reached from the SIGINT handler, so a taskkill that blocked would leave the daemon unkillable by signal. A kill we cannot complete is better abandoned -- the sweep frees the slot regardless. * fix(dev-server): keep the verdict when a runner reports then throws The catch around startRun neither checked nor set the settled flag, so a runner that reported an exit and then threw had its real result overwritten by the noise that followed it -- completed/0 became error/null. Degrades safe and the shipped runner cannot produce it, but a verdict that exists should not be discarded. * feat(dev-server): route test:unit:run through the queue when opted in Replaces the hook approach. A PreToolUse hook has to decide from the command text whether a run is happening, and three adversarial passes showed that cannot be done: it ended with 20 known bypasses and 6 false denials, including refusing a commit whose message merely mentioned the suite. Inside the script there is nothing to parse -- whatever shell, wrapper, quoting or directory reaches it gets the queue. It routes rather than refuses, which is the difference that makes it work: no second command to learn, nothing to wrap around, and an agent that never read the guidance still gets queued instead of an error it will work around. Off unless CIVITAI_TEST_QUEUE is set, and off in CI regardless, so this is a no-op for everyone who does not run the daemon -- the same vitest invocation as before. A file-scoped run stays direct: queueing a two-second check behind a nine-minute suite would break the fast loop and push callers toward batching more into each run, which is the opposite of the point. The queue being unreachable falls back to running directly. Nobody should be unable to run tests because a daemon is down. |
||
|
Justin Maier
|
7fd52440a9 |
fix(dev-server): a session keeps its port until it is stopped (#3936)
* fix(dev-server): a session keeps its port until it is stopped getUsedPorts() dropped the reservation for any session not `running` or `starting`, so a session marked `crashed` lost its port while its process could still be alive and holding it. The daemon has reported `crashed` for a session that was serving 200s, and there is a window inside restart() where the process is dead and the port is about to be rebound. Status is a report about a process the daemon cannot see into; membership of the session map is the reservation, and DELETE /sessions/:id -- what `cli stop` sends -- is the release. Three causes of the wrong status go with it: - stop() hard-kills, so the process exits nonzero and the exit handler filed every deliberate stop as a crash. It now records `stopped` and detaches before killing. - The exit and error handlers were not identity-guarded, so a process exiting after a restart replaced it marked the new, live session crashed and taskkilled a pid Windows may already have reused. AuthHub already guards this way; DevSession now does too. - POST /sessions for a worktree that already has a dead session created a second session and left the first holding its port. It now restarts that session on its own port, so `start` and `restart` are the same thing for an existing worktree and no crash costs a port. DELETE now probes the port after stopping and says so when something still listens, which is the case a kill that missed an orphaned grandchild leaves behind. daemon.mjs runs main() only as the entry point and exports DevSession, sessions and getUsedPorts, so the tests drive the real classes with fake child processes rather than spawning anything. The daemon must be restarted for any of this to take effect -- a running one holds the old code in memory, which is why #3889 did nothing until it was restarted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): wait a port out before moving a session off it Adversarial review of the first pass found that taking a session over started it on that session's port without ever checking the port was usable. `next dev` does not fail on an occupied port -- it warns and moves to another one -- so a session restarted onto a port an orphan still held would report a url nothing of its own was serving, and the health check would go green against whatever was. Checking naively was worse than not checking. stop() resolves on the first of the process exit or 500ms, the process is the cmd wrapper rather than next-server, and taskkill is spawned without being awaited. Measured on this box, a killed listener releases its socket 630-668ms after the kill is spawned, so a single probe reads "held" on essentially every restart and would move a healthy session off its port -- which for the primary session on 3000 silently unhooks the rgb-proxy (hardcoded to 3000) and rewrites its auth URLs. So the claim now waits the port out, up to 8s, and moves only when it stays held long past any plausible teardown. Also from that review: - start() kills a process still attached before spawning its replacement, so two interleaved starts cannot orphan the loser. - The unattended restart inside a branch switch claims its port too. It was the one restart path running with nobody watching. - The entry-point guard realpaths both sides. `import.meta.url` is realpathed by node and `process.argv[1]` is not, so a launch through a junction -- which this repo's tooling creates routinely -- left the daemon exiting 0 having started nothing. - Worktree lookup is case-folded on Windows, matching worktree.mjs. Without it one tree could hold two sessions, two servers and two reserved ports. - Session listings report `worktreeMissing`, which is where someone hunting a reserved port actually looks. - DELETE probes before releasing, and only reports a listener that survives a second look 300ms later. - stop() no longer overwrites an `error` status, a failed kill is logged rather than swallowed, and the port-exhaustion error names the command that releases a reservation. The tests now cover the wiring rather than only the helpers: that the claim runs between the stop and the start, that a replaced process is killed, and that the worktree match folds case. Reverting each gives a named wrong value -- including `expected 3102 to be 3101` for the healthy session that kept its port. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): serialize a session's lifecycle, bound its filesystem checks Three more rounds of adversarial review over the port reservation work, each of which found something the previous round's fix had introduced. The busy flag went FALSE while a second queued restart was still running. lifecycle() set it at call time and chained each clear onto the lock, so with two queued ops the order is fn A, clear, fn B -- the guard held for the first concurrent request and failed for every one after, which is the pile-up it exists for. It is a depth counter now, with `busy` a getter over it. Restarts and branch switches serialize through that lock, and a session removed mid-flight can no longer spawn: `removed` is checked in start() and at the top of runBranchSwitch, so a DELETE landing during an install stops the work instead of spending minutes on it for a session nobody tracks. POST /sessions/:id/restart refuses with 409 rather than queueing behind an op that may never settle -- the CLI's fetch has no timeout, so queueing hung the caller with no output. The worktree existence check no longer blocks the daemon. It ran synchronously on the only thread, on the endpoint the TUI polls twice a second, and one stat against an unreachable path measured 21s with every other agent's request queued behind it. Making it async moved the block without bounding it: Promise.race does not cancel the loser, so each poll still occupied a libuv slot for the full 21s and the four-slot pool saturated in about two seconds. So a path now gets one probe in flight at a time, hits are reused briefly, and misses are never cached -- a miss is never slow, and caching one made a worktree created seconds after a failed start read as still missing. A timed-out check reports the worktree as present, because slow is not evidence of deletion and calling a live tree missing is the more damaging wrong answer. The listing says the check timed out so it can report "could not tell" rather than "fine". The .env check takes the opposite default: an unknown answer there falls back to the main .env, because picking a file that may not exist over a known-good fallback starts the server with no environment at all. Two callers were reporting things that had not happened. The dashboard's `r` key flashed "Session restarted" and cleared the log pane on a refusal, since daemonRequest resolves rather than throws -- it reads the response now. And getStatus() exposes `busy`, so a session waiting on its own restart no longer lists as plain `stopped`. Tests are 37. Each fix fails legibly on revert, including the two whose absence a green suite hid: the boolean busy flag gives `expected [ true, false ] to deeply equal [ true, true ]`, and dropping the probe dedupe gives `expected "vi.fn()" to be called 1 times, but got 2 times`. Known and not addressed: two concurrent starts on a brand-new worktree can still be handed the same port, which predates this branch; loadEnvFile, resolveGitHeadPath and pruneDistDirs still stat synchronously, off the polled path; and the branch-switch claim is untested, because driving it needs a real HEAD and a fake that proves itself would be worse than the gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
4cdd0f878c |
fix(captcha): execute the siteverify response schema; add failure telemetry to the payment captcha paths (#3695)
Executes the Cloudflare siteverify response schema instead of trusting it, and adds failure telemetry to the payment captcha paths (Paddle/Stripe). Strictly tighter than before on acceptance: `success` is now a real `z.boolean()` rather than a truthy `as`-cast, so a string `"true"` is rejected. Every other field is `.catch`-tolerant, so a legitimate payer can only be turned away if Cloudflare emits a non-boolean `success`. Telemetry cannot take down verification — `logToAxiom` is async and every call is `.catch`ed. Also fixes a cross-request aliasing bug: `.catch([])` stored one array literal and returned that same reference from every parse, process-wide, on a payment path. Now `.catch(() => [])`. Severity fix: an upstream dependency returning something unusable is a 4xx to the caller but a server-side fault in origin. `escalateToServerFault` marks such an error so `classifyErrorFault` logs it at error severity without changing the HTTP status the caller receives. That registry is pinned to `globalThis`, not module scope, and the distinction is load-bearing. Turbopack inlines the reading module into many emitted runtime modules, so a module-scope WeakSet is N independent registries: the mark goes into one, the predicate is asked of another, and the escalation silently never fires. Measured on a production build — pre-fix 10 emitted copies with 0 sharing state, post-fix 12 copies all sharing one. The module is on the `SHARED_STATE` watchlist of scripts/check-server-graph-singletons.mjs, whose list now lives in one place shared by the gate and its test, with per-entry assertions against the real source files so a wrong entry cannot pass. Those assertions require the `??=` adopt form with comments stripped — `=` reintroduces the original orphaned-marks bug in one character and the build gate cannot see it, since that gate checks for a reference to the key and `=` references it too. `server-fault-override.ts` is recorded in KNOWN_REACHABLE: importing the predicate from logging/client ships this leaf to the client bundle. It has zero imports, and the entry is exact-path — a denied import added to that module, or to a sibling under the same prefix, still fails the graph guard. Known-open, deliberately not in this PR: the non-JSON-200 path moved 500 -> 400, so it no longer reaches the 5xx-only `civitai_app_http_errors_total`; `clientReportedIp` reaches three production log sinks with no redaction layer; and a client-supplied `captchaDebug.tokenPrefix` has no length bound. `preview / component-tests` and `preview / smoke-tests` are red for reasons external to this change: the branch predates #3887, which fixed a browser test that collected 0 tests on main, and the `/images` meili-backed smoke assertion fails identically on other open PRs. Both suites are report-only. Every blocking check is green. No file in this PR has been touched on main since the merge base. |
||
|
Justin Maier
|
e1a9859b3c |
fix(dev-server): port picker missed every occupied state but one (#3889)
* fix(dev-server): port picker missed every occupied state but one isPortAvailable bound 127.0.0.1 and read a successful bind as "free". libuv sets SO_REUSEADDR unconditionally, and Windows then allows a specific-address bind underneath a wildcard listener, so the probe only ever detected a listener bound to exactly 127.0.0.1. Measured against a live `next dev` on 3001 (which binds `::` dual-stack): probe said free. Held on `::` v6-only, 0.0.0.0 or ::1: also free. Combined with the picker not reserving the port of a session it has marked `crashed`, a bare start hands out an occupied port and both sessions die. Replace it with a probe that connects to both loopback addresses first -- the only thing that distinguishes "nobody is listening" from "I am allowed to bind too" -- then exclusive-binds each stack. Verified end to end: with a dual-stack listener held, the daemon now returns 400 for an explicit request and skips the port when picking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): stop the probe waiting on close(), and cover the binds Adversarial review of the previous commit, all four confirmed on this host: - close() withholds its callback until every accepted connection is gone, and the probe's transient listener accepts whatever arrives in its bind window. Resolving from that callback made one stray connection enough to wedge isPortFree with no timeout above it — and the daemon awaits it inline in POST /sessions. Resolve on `listening` instead, as the old code did. - The connect half alone catches all five occupied shapes on Windows, so the three binds had no test at all. Cover the state only they see: a port the OS refuses to bind (EACCES). - `exclusive: true` is not SO_EXCLUSIVEADDRUSE — it only diverts listenInCluster away from the cluster handle, so in the daemon it was a no-op. Dropped, and the comment now says what the binds are for. - A silent `return` on an unavailable address family printed a green tick for a case that never ran. ctx.skip() says so instead. SKILL.md no longer claims the picker sees "anything outside the daemon entirely": on Windows a listener bound only to a non-loopback address is still invisible. Both new tests fail on revert: the bind test with `expected true to be false`, the wedge test with `isPortFree never resolved` in 5.2s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(dev-server): make both probe tests run, and pass, off Windows Second review pass, both confirmed: - The wedge test's mock left `close` a no-op, so the probe's transient listener stayed bound and its three sequential binds collided with each other. Green on Windows (a 0.0.0.0 holder does not block a later dual-`::` bind there), red on Linux, which is where CI runs it. Close for real, withhold only the acknowledgement — same defect, no leak. - The EACCES case scanned the host for a port the OS refuses to bind. That port exists on this Windows box and in no container: root and non-root both bind port 1 under Docker's default net.ipv4.ip_unprivileged_port_start, so the test skipped itself in CI and the bind half stayed uncovered exactly where it was claimed covered. Force EACCES through the mock instead, and drop the scan -- it could also have picked 80, where connect succeeds and the verdict never reaches the binds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(dev-server): pin the escape that keeps an IPv6-less host usable Collapsing tryBind's classifier so every bind error reads 'busy' passed the whole suite. On a host without IPv6 the `::` bind returns EAFNOSUPPORT, so that regression would make every port read busy and every dev server start fail with "No available ports found within range". Now fails as `expected false to be true`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |