Skip to content

fix(hosted): persist redirect ledger before lockfile writes - #187

Merged
Mikola Lysenko (mikolalysenko) merged 5 commits into
mainfrom
fix/hosted-write-path-atomicity
Aug 15, 2026
Merged

fix(hosted): persist redirect ledger before lockfile writes#187
Mikola Lysenko (mikolalysenko) merged 5 commits into
mainfrom
fix/hosted-write-path-atomicity

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

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 .corrupt quarantine (CorruptRedirectState) instead of being silently replaced. This PR keeps all of #192's machinery and delivers the remainder:

  1. 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.
  2. Non-atomic lockfile writes. The bare fs::write truncates before writing, so a crash mid-write could leave a torn pnpm-lock.yaml behind — the vendored backend treats exactly this as a hard invariant while hosted did not.
  3. Corrupt ledger silently replaced. Subsumed by fix(redirect): atomic fail-closed ledger, ledger-aware updates[] #192's fail-closed load + quarantine.
  4. parse_purl_simple decoded the name but not the version. The API serves canonical percent-encoded purls, so an encoded version (npm build metadata 1.2.3%2Bbuild) produced a DepOverride that silently matched no lock entry.

Fix

  • Write every lockfile through the vendored backend's atomic stage+fsync+rename mode-preserving writer (atomic_write_bytes_preserving_mode, visibility widened pub(crate)pub so the CLI shares the hardened writer instead of duplicating it). A failed lockfile write now leaves that file byte-untouched.
  • FileEdit derives PartialEq and 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_simple percent-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_envelope therefore 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.rs unit test parse_purl_simple_percent_decodes_name_and_version.

This branch's load_redirect_state_strict, its own atomic ledger write, its ledger-first reordering, and corrupt_ledger_is_refused_not_replaced were dropped in the merge as subsumed by #192 (whose quarantine semantics intentionally MOVE the corrupt file aside; corrupt_ledger_fails_closed_and_preserves_the_bytes covers 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 warnings clean.

🤖 Generated with Claude Code

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>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

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.

Comment thread crates/socket-patch-cli/src/commands/scan/hosted.rs Outdated
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>
@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit 0daf2e3 into main Aug 15, 2026
41 of 42 checks passed
@mikolalysenko
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants