fix(hosted): persist redirect ledger before lockfile writes - #187
Merged
Mikola Lysenko (mikolalysenko) merged 5 commits intoAug 15, 2026
Conversation
The hosted write path wrote every rewritten lockfile with a plain fs::write and only afterwards persisted the redirect ledger — the file that records each entry's pre-redirect resolved/integrity originals, the only revert data. A mid-loop write failure (read-only path, disk full, Rush multi-lock repos) therefore exited with some locks already redirected and their originals never persisted; a re-run could not recapture them because an already-redirected entry produces no new edit. The originals were permanently lost. Reorder and harden the write path: - Persist the ledger (merged with any existing one) BEFORE mutating any lockfile, so every planned edit's original is durable first. A recorded edit whose lockfile write then fails is harmless — revert would restore bytes the file already has — and a retried run's re-planned edits are deduplicated instead of appended. - Write lockfiles and the ledger with the vendored backend's atomic stage+fsync+rename writer (mode-preserving), so a crash mid-write can no longer leave a torn lockfile; a ledger failure now leaves the project byte-untouched. - Fail closed on an existing-but-corrupt redirect-state.json (e.g. an unresolved merge conflict): the run refuses before touching any lockfile instead of silently replacing the ledger and discarding the recorded originals (new load_redirect_state_strict; read-only consumers keep the lenient loader). - Percent-decode the version in parse_purl_simple like the name, so a canonical encoded purl version (npm 1.2.3%2Bbuild) matches the decoded form lockfiles store instead of silently redirecting nothing. Regression tests: mid-run partial write failure keeps the ledger's originals and leaves the failed lock byte-identical; a corrupt ledger is refused with the project untouched; an unwritable ledger now fails before (not after) the lockfile rewrite; strict-loader unit tests; and purl version decoding. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c8f5b9e. Configure here.
Wenxin Jiang (Wenxin-Jiang)
approved these changes
Aug 14, 2026
Mikola Lysenko (mikolalysenko)
enabled auto-merge (squash)
August 14, 2026 20:37
Reconciles with #192 (fix/redirect-ledger-atomicity), which already landed the ledger-before-lockfiles ordering, the atomic ledger write (save_redirect_state), and the fail-closed corrupt-ledger load with .corrupt quarantine. Kept all of main's #192 machinery and re-expressed this PR's remaining value on top of it: - Lockfile writes go through atomic_write_bytes_preserving_mode (main still used bare fs::write for the rewrite.files loop). - FileEdit derives PartialEq and the ledger merge skips byte-identical re-planned edits from a retried partial failure. - parse_purl_simple percent-decodes the version like the name. - New test partial_lockfile_write_failure_persists_ledger_originals (mid-run multi-lock failure: originals durable, failed lock byte-untouched). - Leg 3 of the write-failure envelope test obstructs the lock's DIRECTORY (unix-gated): with atomic writes a read-only lock FILE is replaced mode-preserved instead of failing. Dropped as subsumed by #192: load_redirect_state_strict (superseded by CorruptRedirectState), this branch's own atomic ledger write and ledger-first ordering, and corrupt_ledger_is_refused_not_replaced (superseded by corrupt_ledger_fails_closed_and_preserves_the_bytes, whose quarantine semantics intentionally MOVE the corrupt file). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A project with an unreadable redirect-state.json and a bun.lockb must come out of a refused run byte-identical. The refusal already precedes the bun.lockb auto-migration, but nothing pinned that ordering, and the migration is the one write that happens before every rewriter: run it first and the user loses their binary lockfile to a run that redirects nothing and exits 1. The new test grants a redirectable npm override (the migration's gate) and puts a fake bun that would migrate on PATH, so a surviving bun.lockb proves the ordering rather than a skipped gate. Moving the ledger load back below the migration fails it. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko)
deleted the
fix/hosted-write-path-atomicity
branch
August 15, 2026 00:08
Mikola Lysenko (mikolalysenko)
added a commit
that referenced
this pull request
Aug 15, 2026
Refines the scan/mod.rs conflict resolution in the preceding merge of #187 and #191, which crossed with this branch's takeover work. The vendored-direction remediation had taken #191's text verbatim, which tells the operator to hand-edit .socket/vendor/redirect-state.json and delete each superseded package's `records` entry plus its matching `edits`. #191 could not have known this branch makes that automatic: re-running `socket-patch vendor` (or `scan --mode vendored`) now reverts the stale hosted edits from the ledger and drops both halves of the entry itself. The text leads with that and keeps #191's by-hand procedure as the fallback, along with its records-AND-edits reasoning and its never-delete-the-ledger-file warning. Restores the comment explaining why the ledger is never offered up for hand deletion. The hosted direction keeps #191's per-package `remove <purl>` guidance; its comment now also records why `vendor --revert` is not offered -- it unwinds every vendored package, including ones still live in the lockfile. Both sides' tests pass together: #191's following_the_vendored_remediation_clears_the_warning and hosted_remediation_states_removes_full_blast_radius alongside this branch's cargo classifier and GC reclaim tests. 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
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.

Problem
Four hosted-mode write-path findings from the 2026-08-13 audit (origin/main 4e5288e). Since then, #192 (
fix/redirect-ledger-atomicity) landed on main and independently fixed two of them — the ledger is now persisted atomically (save_redirect_state) and before the lockfile writes, and an existing-but-corrupt ledger is a hard error with a.corruptquarantine (CorruptRedirectState) instead of being silently replaced. This PR keeps all of #192's machinery and delivers the remainder:Mid-run lockfile write failure loses revert originals.Subsumed by fix(redirect): atomic fail-closed ledger, ledger-aware updates[] #192's ledger-before-lockfiles ordering; this PR adds the mid-run multi-lock regression test pinning it.fs::writetruncates before writing, so a crash mid-write could leave a tornpnpm-lock.yamlbehind — the vendored backend treats exactly this as a hard invariant while hosted did not.Corrupt ledger silently replaced.Subsumed by fix(redirect): atomic fail-closed ledger, ledger-aware updates[] #192's fail-closed load + quarantine.parse_purl_simpledecoded the name but not the version. The API serves canonical percent-encoded purls, so an encoded version (npm build metadata1.2.3%2Bbuild) produced aDepOverridethat silently matched no lock entry.Fix
atomic_write_bytes_preserving_mode, visibility widenedpub(crate)→pubso the CLI shares the hardened writer instead of duplicating it). A failed lockfile write now leaves that file byte-untouched.FileEditderivesPartialEqand the ledger merge skips byte-identical edits: with the ledger persisted before the lockfile writes (fix(redirect): atomic fail-closed ledger, ledger-aware updates[] #192), a retried run after a partial lockfile failure re-plans the failed lock's edit, and without deduplication it would be appended to the ledger twice.parse_purl_simplepercent-decodes the version like the name.Behavior note: a read-only lockfile is now replaced (mode preserved) instead of failing the run, matching the vendored backend's writer semantics; leg 3 of
redirect_json_mode_write_failures_emit_error_envelopetherefore obstructs the lock's directory instead (unix-gated — a read-only directory does not block file creation on Windows).No findings were skipped.
Testing
New regression tests, each independently verified to fail on pre-fix code:
in_process_redirect::partial_lockfile_write_failure_persists_ledger_originals(unix): Rush two-lock repo with the second lock's directory read-only — exit 1, first lock redirected, ledger already holds its originals + upstream integrity, failed lock byte-identical.hosted.rsunit testparse_purl_simple_percent_decodes_name_and_version.This branch's
load_redirect_state_strict, its own atomic ledger write, its ledger-first reordering, andcorrupt_ledger_is_refused_not_replacedwere dropped in the merge as subsumed by #192 (whose quarantine semantics intentionally MOVE the corrupt file aside;corrupt_ledger_fails_closed_and_preserves_the_bytescovers that finding).Suites run green post-merge:
cargo build --workspace --all-features;cargo test -p socket-patch-cli --all-features --test in_process_redirect(31);cargo test -p socket-patch-cli --all-features --lib(373);cargo test -p socket-patch-core --all-features redirect(90);cargo clippy --workspace --all-features -- -D warningsclean.🤖 Generated with Claude Code