fix(cli): write mops.lock with sorted keys - #741
Merged
Conversation
deps, hashes, graph and github came out in insertion/traversal order, so unrelated installs reordered a 69 KB file with 621 per-file hash entries and produced diff churn and merge conflicts for no reason. Sorting happens at the single write site rather than where the records are computed, which is what covers the carry-over path: computeLockFile copies an existing package's hash record verbatim when the version is unchanged, so a sort applied at computation time would preserve whatever order that record already had. Plain code-unit comparison, never localeCompare — a locale-sensitive sort orders differently between machines, which is the opposite of the goal. Not a format change. Validation never inspects key position: hasValidShape, inspectLockFile, checkLockedDeps, checkLockedGithubDeps and checkLockConsistency all index by name or compare key sets. CURRENT_LOCK_VERSION stays 3, an existing unsorted lockfile stays valid and still passes --locked, and it is deliberately not treated as stale — a "rewrite because unsorted" trigger would churn every user's lockfile on their next install for no correctness reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Cursor AI review👍 APPROVE — looks safe to merge
VerdictDecision: APPROVE Generated for commit b944175 |
automation-sa-sre
approved these changes
Aug 12, 2026
automation-sa-sre
left a comment
automation-sa-sre
left a comment
There was a problem hiding this comment.
Automated approval: the AI review verdict for b944175 is APPROVE. See the "Cursor AI review" comment for details.
This was referenced Aug 13, 2026
Kamirus
added a commit
that referenced
this pull request
Aug 13, 2026
Establishes the manual tracking convention for the install benchmark from [#744](#744): the most recent meaningful run lives in `perf/install-bench/RESULTS.md`, replaced wholesale on each re-run, with a one-line history of prior headline numbers. Deliberately not a CI gate — runs are only comparable within one host and network, so a committed run is a point of reference. ## The run itself v2.22.0 vs v3 at f8c619c — the first measurement with the whole performance stack landed (#741 sorted lock, #742 update alignment, #743 resolve memo, #745 parallel installs, #746 in-flight re-check). Medians of 3 against the live registry: | scenario | v2 | v3 | v3 vs v2 | | --- | --- | --- | --- | | install-cold-nolock | 139.6s | 107.4s | 0.77x | | install-cold-validlock | 106.6s | 61.1s | 0.57x | | install-cold-stalelock | 128.0s | 95.7s | 0.75x | | install-warm-nolock | 3.20s | 5.62s | 1.76x † | | install-warm-validlock | 6.20s | 2.99s | 0.48x † | | install-warm-stalelock | 3.31s | 2.45s | 0.74x | | add-two | 4.00s | 3.84s | 0.96x | | update-few | 8.02s | 5.42s | 0.68x | | update-all | 14.9s | 2.87s | 0.19x | † transient registry-latency window, not code: v2's own warm numbers tripled against every prior run of the same released binary. A warm-only re-check half an hour later returned to line — v3 warm-nolock 2.33s (0.75x), warm-validlock 1.08s (0.42x) — and is recorded in RESULTS.md as the representative warm numbers, with the main table kept unedited. **v3 now beats released v2 in every scenario.** The cold no-lock install — v3's one regression before #745 (1.23–1.25x in both pre-parallel baselines) — is 0.77x, and CI-shaped installs (cold + committed lock) are 43% faster. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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.
mops.lockwas serialized with a plainJSON.stringify, sodeps,hashes,graphandgithubcame out in insertion/traversal order. This repo's own lockfile is ~69 KB with 621 per-file hash entries, so an install unrelated to a given package could still reorder its block — diff churn and merge conflicts for no reason.Every key is now sorted: the sections, the package keys inside
hashes, the per-file hash keys inside each package, andgraph's inner per-version edge maps.Two details that decide whether this actually works
Sorting happens at the single write site, not where records are computed.
computeLockFilecarries an existing package's hash record over verbatim when its version has not changed — that is a deliberate optimisation, since published versions are immutable. A sort applied at computation time would therefore preserve whatever order a carried-over record already had, and the file would never converge. There is a test for exactly this: it scrambles an existing lock's file-key order, makes the lock legitimately stale so it gets rewritten, and asserts the carried-over record comes out sorted.Plain code-unit comparison, never
localeCompare. A locale-sensitive sort orders differently depending on the machine's locale, which would make the lockfile less reproducible than before.Not a format change
CURRENT_LOCK_VERSIONstays3. Validation never inspects key position —hasValidShape,inspectLockFile,checkLockedDeps,checkLockedGithubDepsandcheckLockConsistencyall index by name or compare key sets — so key order is invisible to every reader.An existing unsorted lockfile therefore stays valid, still passes
mops install --locked, and is reordered only the next time something legitimately updates it. It is deliberately not treated as stale: a "rewrite because unsorted" trigger would churn every user's lockfile on their next install for no correctness reason, and would fail--lockedon a lock that is otherwise perfectly good. A test pins both halves — an unsorted lock passes--lockedand is byte-identical afterwards, and a plainmops installalso leaves it byte-identical. That second assertion is the regression guard against someone adding such a trigger later.Verification
locked,cache-resilience,local-dep-manifest-lockand the newlockfile-orderingsuites: 40/40. The new tests were confirmed to bite by reverting the call site — the two ordering tests fail without it, while the two--lockedguarantees correctly pass either way, since they assert behaviour that must hold before and after.github-dep-lock.test.tsfails locally, and it is not from this change: with the call site reverted the same suite fails a superset of those tests, and the count varies run to run. It downloads real archives from GitHub and that cache has been churned repeatedly today. Worth a look if CI disagrees.Item 8 in #723, where the triage records that this needs no version bump.
🤖 Generated with Claude Code