perf(VSY-45): Bound scratch scanning CPU while preserving complete size readings - #83
Open
vanillagreen-fleet-lanes[bot] wants to merge 5 commits into
Open
vanillagreen-fleet-lanes[bot] wants to merge 5 commits into
vanillagreen-fleet-lanes[bot] wants to merge 5 commits into
Conversation
Scratch traversal held 123 to 137 percent of one core because it submitted one asynchronous status read per entry on the dashboard's own thread and became eligible again the instant a scan longer than the rescan interval finished. It now runs on a worker thread with synchronous batched reads, rests for the share of each slice its duty does not grant, and measures eligibility from completion on the monotonic clock. D004 records the trade-off against a stored filesystem index.
Address PR review
Address PR review
Address PR review
Copilot started reviewing on behalf of
vanillagreen (vanillagreencom)
September 15, 2026 05:55
View session
There was a problem hiding this comment.
🟡 Changes recommended
Release packages omit the required worker artifact, and the benchmark does not fail when worker-side throttling is bypassed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Moves scratch-directory scanning to a duty-cycled worker thread while preserving complete readings and responsive collection.
Changes:
- Adds configurable scratch-scan CPU duty cycling and completion-based scheduling.
- Adds worker lifecycle, traversal, UI-state, and failure-handling tests.
- Extends build, CI, benchmark, architecture, and changelog coverage.
File summaries
| File | Description |
|---|---|
src/ui/storage-screen.tsx |
Unifies scratch status and empty-state rendering. |
src/ui/storage-screen.test.tsx |
Tests scratch summary states. |
src/ui/settings.ts |
Exposes the scan-share setting. |
src/config/config.ts |
Defines and validates scan duty. |
src/config/config.test.ts |
Tests duty bounds. |
src/collect/settings.ts |
Marks duty as collection-affecting. |
src/collect/scratch.ts |
Adds worker orchestration and scheduling. |
src/collect/scratch.test.ts |
Tests collector behavior and failures. |
src/collect/scratch-worker.ts |
Implements the worker entry point. |
src/collect/scratch-worker.test.ts |
Tests worker lifecycle and replies. |
src/collect/scratch-scan.ts |
Implements paced synchronous traversal. |
src/collect/scratch-scan.test.ts |
Tests traversal and pacing. |
scripts/ci.py |
Gates benchmark and build artifacts. |
scripts/ci_test.py |
Tests the expanded CI contract. |
scripts/bench-scratch.ts |
Measures scratch traversal behavior. |
package.json |
Builds the worker and adds its benchmark. |
docs/decisions/INDEX.md |
Indexes D004. |
docs/decisions/D004-scratch-scan-duty.md |
Records the worker-duty decision. |
docs/architecture/storage.md |
Documents scratch invariants. |
DEVELOPMENT.md |
Updates build and benchmark guidance. |
changelog.d/fixed/scratch-scan-failure.md |
Records failure preservation. |
changelog.d/fixed/scratch-first-scan-line.md |
Records corrected empty-state messaging. |
changelog.d/changed/scratch-scan-thread.md |
Announces worker-based scanning. |
AGENTS.md |
Updates the full-check command description. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ); | ||
| } | ||
| const full = await measure(100); | ||
| const bounded = await measure(defaults().scratchDutyPercent); |
Comment on lines
+27
to
+30
| const candidates = ["./scratch-worker.ts", "./scratch-worker.js"].map( | ||
| (name) => new URL(name, import.meta.url), | ||
| ); | ||
| const found = candidates.find((url) => existsSync(fileURLToPath(url))); |
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
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.
Summary
scratchDutyPercent, sets that share and defaults to 25.bun run bench:scratchas a gate, so the processor bound cannot be deleted with every check green.Context
docs/decisions/D004-scratch-scan-duty.mdCompleted Issues
QA Metrics
Performance QA at
ca52326againstorigin/main1039bc5. Linux 6.19.14, 8 cores, Bun 1.4.2. Full percentile set is on the Linear issue.Whole-application processor use, share of one core:
Scan cost on the same tree, 5 runs, identical 202,320,253-byte total in every variant: 4868 ms of processor time per scan on main against 1406 ms here, a sustained 31.6% of one core.
Keyboard round-trip latency, 200 key samples per variant through a pseudo-terminal:
The last row is the one trade this change makes. main's lower figure there is taken while it holds 1.4 cores: a saturated main thread keeps the renderer awake, so a keypress paints without a wake-up. This branch's figures in that row match its own figures with no scan running at all (13.40 against 11.45 at the median), so typing latency here does not move with the traversal. No project document sets a keyboard latency budget.
Foreground refresh is unchanged, measured rather than assumed: 16 collection samples per 16 s window in every variant of both builds, and the same rendered frame counts in both.
Size
771 production lines, 537 test lines, 0 render-mirror lines. The issue states no Expected delta, so the verdict is
allowance_missing: the counts are reported, not judged.Known residue
.github/workflows/ci.ymlline 15 names the jobBun (lint, types, tests, build), which now omits the scan-bound step. That string is the commit-status context branch protection requires, so renaming it strands the required check until branch protection is updated in the same move. Left unchanged deliberately; both changes have to land together.python3 scripts/ci.pyrun exited 1 during development with its output discarded and could not be reproduced. Three later full runs, eight benchmark runs and this commit are clean.bun install --frozen-lockfileis the only non-deterministic step in the contract. Recorded rather than filed, because it is neither reproducible nor evidenced. Worth watching on CI.scanRoot.walkwould save about 16% of scan processor time for an identical reading. Declined at priority 4, estimate 2: it clears no filing bar and is too large to absorb here.Test Plan
python3 scripts/ci.py— the full contract: install, lint, types, tests, build, both build artifacts present and non-empty, and the scan bound. Green in ~18 s, 525 tests across 56 files, plus 8 check-runner tests.bun run bench:scratch— refuses a scan that reported a source error, a bounded scan whose total differs from a full one, a scan that asked for no rest, and a scan that finished in under 95% of the rest it asked for. Margin measured 1.31 to 1.37 against the 0.95 floor across runs; load can only widen it, because elapsed time contains the actual sleeps.bun src/main.ts --once— still waits for a complete reading and still exits 2 on source errors.