perf(ci): build artifact-mutating tests into per-file repo sandboxes - #8938
Merged
Conversation
WIP — see the PR description for two unresolved failures. The serial `test:ci:artifacts` pass (~115s of CI) existed only because five tests shared one on-disk artifact tree. scripts/lib.ts is the single place `repoRoot` is computed and all 78 scripts import it, so one env override (METAGRAPH_REPO_ROOT) redirects all 48 `public/**` write sites across 23 scripts, both artifact roots, and every R2_STAGING_RELATIVE_ROOT consumer. tests/helpers/repo-sandbox.ts clones the repo's data directories (or the whole working tree, for tests that run a script which WALKS the repo) into a temp root and points the real scripts at it. Code still loads from its real location; only data is redirected. node_modules is symlinked rather than copied. Converted the four real writers — artifacts, public-safety, refresh-build-summary, and validate-error-messages (which mutates a real registry/subnets file in place; it was wrongly assumed to be a reader). discovery-artifacts needed no change: it is a pure reader that was serial only because the others rebuilt underneath it. Paths are computed explicitly from the sandbox root rather than by setting the env var before importing scripts/lib.ts: since #8936 the module registry is shared across files, so lib.ts may already be evaluated with its repoRoot-derived consts frozen against the real repo. With METAGRAPH_REPO_ROOT unset, behaviour is unchanged: `npm run build` reports 8/8 steps passed with committed artifacts byte-identical. Refs #8937
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
metagraphed-data-api | a4da665 | Jul 31 2026, 02:25 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
metagraphed-registry-sync-api | a4da665 | Jul 31 2026, 02:25 PM |
JSONbored
marked this pull request as ready for review
July 31, 2026 14:25
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-31 16:00:42 UTC
Review summary Blockers
Nits — 6 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
metagraphed-ui | a4da665 | Commit Preview URL Branch Preview URL |
Jul 31 2026, 02:29 PM |
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Cloning the working tree per test file was racy by construction: scripts/lib.ts writes JSON atomically (write `<name>.<pid>.<rand>`, then rename), so any concurrent writer leaves temp files that exist when the copy lists a directory and are gone when it reads them — an intermittent ENOENT on dist/metagraph-r2/metagraph/testnet/subnets/*.json.*. No copy tool fixes that; cp, fs.cpSync and rsync only report the vanish differently, and tolerating it would silently accept a half-copied sandbox. Take ONE pristine copy in vitest globalSetup instead, before any worker starts and while the tree is quiescent, and clone every sandbox from that. No sandbox can then observe a partial write, whatever the suite does concurrently. The snapshot path is derived deterministically from the repo root so workers recompute it with no IPC. Any rsync failure now throws. Also fixes the `scope: "full"` copy: it iterated top-level entries, but the repo root holds regular files too and `rsync -a file/ dest/` is invalid — it now copies the snapshot root in one pass. Verified: 10/10 consecutive green `npm run test:ci` runs (12,517 shared + 1,064 isolated), where the live-tree version was red in 2 of 5 and the per-entry copy red in 6 of 6. `npm run build` reports 0 failed steps with committed artifacts unchanged.
The two deliverChangeEvent retry tests were the only ones in the suite that awaited the real default sleepFn -- 500ms in one, 500ms + 1s in the other -- and they are the two that timed out at 30s in CI once the artifact builds joined the shared parallel pass. A pending timer only fires when its worker's event loop runs, so a half-second wait is a half-second window for a saturated box to stall it in; the tests that inject a sleepFn never flaked. Drop backoffBaseMs to 1ms rather than stubbing sleepFn, so the default sleepFn stays covered (stubbing it left src/webhooks.ts:579 with two uncovered functions) while the stall window shrinks ~500x. The backoff schedule itself is still asserted exactly, by the sibling test that injects a sleepFn. tests/webhooks.test.ts: 2033ms -> 37ms.
…ree real-repo paths Two defects that only bite once fileParallelism is on. 1. Leaked fake timers. Ten files install vi.useFakeTimers(); four restore it inline rather than in a finally, so an assertion throwing past the restore leaves the clock faked. Under isolate:false that clock is shared with every LATER file in the same worker, and nothing advances it -- so anything awaiting a real setTimeout hangs until the test timeout regardless of the delay it asked for. That is what timed out the two deliverChangeEvent retry tests in CI: a 1ms backoff hung for the full 30s, which rules out slow-box starvation. Restoring real timers in the shared afterAll fixes it for every file, not just this one. Verified with a leak probe: without the guard a real 1ms sleep times out, with it it passes. 2. Three paths in artifacts.test.ts were repo-relative, so they resolved against process.cwd() -- the real repo -- instead of the sandbox #8937 gave the file. SUPPORT_ARTIFACT_PATHS was the damaging one: restoreSupportArtifacts truncates and rewrites the committed public/metagraph/r2-manifest.json while eight other test files read it, and writeFileSync is not atomic. It restores identical bytes, so git status stays clean and it went unnoticed -- but the file's mtime advances on every run, which is how it was caught. The .cache health path was also pointed at the real tree, making that test's setup a no-op against its own purpose (the rebuild reads the SANDBOX's .cache), and the r2-manifest dry-run test read the real manifest but restored into the sandbox. Verified: real r2-manifest.json mtime is now unchanged across a full tests/artifacts.test.ts run (23/23 passing).
…l timer Dropping the backoff to 1ms was not enough -- CI timed out on the same two tests again, at 30s each, for a 1ms sleep. A delay that small hanging that long rules out a slow or contended box: the timer is not firing at all in that worker. Whatever installs that state, these are the only two tests in the suite exposed to it, because they were the only ones awaiting the real default sleepFn; every sibling that injects one has always passed. So inject one here too. The retry BEHAVIOUR under test -- attempt counts, terminal status, the 4xx/5xx split -- is what these assert, and none of it needs a real clock. The backoff SCHEDULE is still asserted exactly, by the sibling test that records the delays it is handed. This gives up coverage of the default sleepFn's arrow (src/webhooks.ts:579). That is a project-coverage rounding difference, not a patch-gate one: no src line changes here, so codecov/patch has nothing to measure. A fake-timer test was tried instead and rejected -- the signing and URL-resolution awaits ahead of the retry loop do not settle before the clock is advanced, so it hung too. Also make the timer guard self-reporting: it now names the condition when a file finishes with fake timers installed, so the leak is attributable rather than silently absorbed.
The guard's comment claimed leaked fake timers were what timed out the two deliverChangeEvent retry tests. They were not. The guard reports zero leaks across a full green run, so no file finished with a fake clock installed and the cause of those timeouts is still unexplained -- injecting a sleepFn removed the suite's only exposure to it rather than explaining it. Keep the guard: the failure mode it covers is real, silent, and lands on an unrelated file 30s later. But a comment asserting a cause that has been ruled out is worse than no comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8937
Approach
The serial
test:ci:artifactspass (~115s of CI) existed only because five tests shared one on-disk artifact tree.scripts/lib.tsis the single placerepoRootis computed and all 78 scripts import it, so one env override (METAGRAPH_REPO_ROOT) redirects all 48public/**write sites across 23 scripts, both artifact roots, and everyR2_STAGING_RELATIVE_ROOTconsumer. No per-write-site threading needed.Each mutating test then builds into its own tree, so the race is removed rather than serialized around.
fileParallelismgoes totrue, andtest:ci:artifacts(script,ARTIFACT_WRITERSlist, and CI step) is deleted.Converted the four real writers:
artifacts,public-safety,refresh-build-summary, andvalidate-error-messages— which mutates a realregistry/subnetsfile in place. #8937 listed it as a pure reader; it isn't, and it was the cause ofvalidate-surface-schema-url.test.tsfailing once these went parallel.discovery-artifactsgenuinely is a reader and needed no change.Paths are computed explicitly from the sandbox root rather than by setting the env var before importing
scripts/lib.ts— since #8936 the module registry is shared, solib.tsmay already be evaluated with its consts frozen against the real repo.The race the earlier revision had — now fixed at the cause
The first revision cloned the live working tree per test file. That is racy by construction:
scripts/lib.tswrites JSON atomically (write<name>.<pid>.<rand>, then rename), so a concurrent writer leaves temp files that exist when the copy lists a directory and are gone when it reads them — intermittentENOENTondist/metagraph-r2/metagraph/testnet/subnets/*.json.*.No copy tool fixes that.
cp,fs.cpSyncandrsynconly report the vanish differently, and tolerating it (my first attempt) is worse — it silently accepts a half-copied sandbox.The fix is to remove the live source:
tests/setup/artifact-snapshot.tstakes one pristine copy in vitestglobalSetup, before any worker starts and while the tree is quiescent. Every sandbox clones from that frozen snapshot, so none can observe a partial write regardless of what the suite does concurrently. Any rsync failure now throws rather than being tolerated.That revision also fixed a second bug:
scope: "full"iterated top-level entries, but the repo root holds regular files andrsync -a file/ dest/is invalid.Verification
npm run test:ci, 10 consecutive runsnpm run buildtypecheck/lint/format:checkARTIFACT_WRITERS, and CI step removedThe three-way comparison is the point: the intermittent failure is reproducible on the older revisions and absent on this one, so the 10/10 is a property of the snapshot design rather than luck.