Skip to content

Commit 9d46742

Browse files
committed
Make repair's wiring undo FIFO-safe, atomic and symlink-preserving
Bugbot on #283: the identity-undo snapshot read lockfiles with bare tokio::fs::read (a FIFO at a wiring path blocks open(2)) and skipped symlinked lockfiles, and the put-back wrote them in place (no stage+fsync+rename, mode bits dropped). The snapshot now reads through read_regular_to_bytes and records a symlinked lockfile's link text; the undo re-links a link that the engine's rename replaced, then writes the target through atomic_write_bytes_preserving_mode. Pinned by repair_identity_undo_follows_a_symlinked_lockfile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMhWChtaNX5FJYNDq3NBJa
1 parent 5f8573b commit 9d46742

2 files changed

Lines changed: 81 additions & 23 deletions

File tree

‎crates/socket-patch-cli/src/commands/vendored_backend/repair.rs‎

Lines changed: 58 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,9 @@ use socket_patch_core::api::client::{get_api_client_with_overrides, ApiClient};
3030
use socket_patch_core::constants::SOCKET_DIR;
3131
use socket_patch_core::manifest::schema::{PatchManifest, PatchRecord};
3232
use socket_patch_core::patch::copy_tree::remove_tree;
33-
use socket_patch_core::utils::fs::read_regular_to_string;
33+
use socket_patch_core::utils::fs::{
34+
atomic_write_bytes_preserving_mode, read_regular_to_bytes, read_regular_to_string,
35+
};
3436
use socket_patch_core::utils::purl::normalize_purl;
3537
use socket_patch_core::vendor::{
3638
self, artifact_is_file_shaped, check_vendored_artifact, load_state, parse_vendor_path,
@@ -800,9 +802,11 @@ impl VendoredBackend<'_> {
800802
}
801803
}
802804
if !retry.is_empty() {
803-
scratch
804-
.events
805-
.retain(|e| !retry.iter().any(|c| e.purl.as_deref() == Some(c.purl.as_str())));
805+
scratch.events.retain(|e| {
806+
!retry
807+
.iter()
808+
.any(|c| e.purl.as_deref() == Some(c.purl.as_str()))
809+
});
806810
no_source |= run_engine(
807811
&engine_common,
808812
None,
@@ -1046,9 +1050,18 @@ async fn run_engine(
10461050
no_source
10471051
}
10481052

1049-
/// The bytes of each candidate's recorded wiring files — what a re-vendor
1050-
/// may rewrite for it. `None` records a file that did not exist.
1051-
async fn snapshot_wiring(cwd: &Path, candidates: &[Candidate]) -> Vec<(PathBuf, Option<Vec<u8>>)> {
1053+
/// One wiring file as it was before the re-vendor: its bytes (`None`: it
1054+
/// did not exist) and, for a symlinked lockfile, the link text — lockfile
1055+
/// writers rename over the path, which replaces a link with a file.
1056+
struct WiringSnapshot {
1057+
path: PathBuf,
1058+
link: Option<PathBuf>,
1059+
bytes: Option<Vec<u8>>,
1060+
}
1061+
1062+
/// Snapshot each candidate's recorded wiring files — what a re-vendor may
1063+
/// rewrite for it. Reads are FIFO-safe; a non-regular file is skipped.
1064+
async fn snapshot_wiring(cwd: &Path, candidates: &[Candidate]) -> Vec<WiringSnapshot> {
10521065
let mut rels: Vec<String> = Vec::new();
10531066
for c in candidates {
10541067
rels.extend(c.entry.wiring.iter().map(|w| w.file.clone()));
@@ -1058,15 +1071,13 @@ async fn snapshot_wiring(cwd: &Path, candidates: &[Candidate]) -> Vec<(PathBuf,
10581071
let mut out = Vec::with_capacity(rels.len());
10591072
for rel in rels {
10601073
let path = cwd.join(&rel);
1061-
match tokio::fs::symlink_metadata(&path).await {
1062-
Ok(meta) if meta.is_file() => {
1063-
if let Ok(bytes) = tokio::fs::read(&path).await {
1064-
out.push((path, Some(bytes)));
1065-
}
1066-
}
1067-
Ok(_) => {}
1068-
Err(_) => out.push((path, None)),
1069-
}
1074+
let bytes = match read_regular_to_bytes(&path).await {
1075+
Ok(bytes) => Some(bytes),
1076+
Err(e) if e.kind() == std::io::ErrorKind::NotFound => None,
1077+
Err(_) => continue,
1078+
};
1079+
let link = tokio::fs::read_link(&path).await.ok();
1080+
out.push(WiringSnapshot { path, link, bytes });
10701081
}
10711082
out
10721083
}
@@ -1083,20 +1094,34 @@ fn keeps_identity(health: &ArtifactHealth) -> bool {
10831094
}
10841095

10851096
/// Undo what a re-vendor may have written for `c`: its recorded wiring
1086-
/// files go back to their snapshotted bytes and its ledger entry to the
1097+
/// files go back to their snapshotted bytes (a replaced symlink is
1098+
/// re-linked and its target rewritten) and its ledger entry to the
10871099
/// original. Other candidates' files are left alone.
1088-
async fn undo_candidate(cwd: &Path, c: &Candidate, snapshot: &[(PathBuf, Option<Vec<u8>>)]) {
1100+
async fn undo_candidate(cwd: &Path, c: &Candidate, snapshot: &[WiringSnapshot]) {
10891101
let wired: HashSet<PathBuf> = c.entry.wiring.iter().map(|w| cwd.join(&w.file)).collect();
1090-
for (path, bytes) in snapshot.iter().filter(|(p, _)| wired.contains(p)) {
1091-
if tokio::fs::read(path).await.ok().as_ref() == bytes.as_ref() {
1102+
for snap in snapshot.iter().filter(|s| wired.contains(&s.path)) {
1103+
if let Some(link) = &snap.link {
1104+
if tokio::fs::read_link(&snap.path).await.ok().as_ref() != Some(link) {
1105+
let _ = tokio::fs::remove_file(&snap.path).await;
1106+
if relink(link, &snap.path).await.is_err() {
1107+
continue;
1108+
}
1109+
}
1110+
}
1111+
if read_regular_to_bytes(&snap.path).await.ok() == snap.bytes {
10921112
continue;
10931113
}
1094-
match bytes {
1114+
match &snap.bytes {
1115+
// Staged, fsynced and renamed over the link's target (or the
1116+
// file itself), keeping its mode.
10951117
Some(bytes) => {
1096-
let _ = tokio::fs::write(path, bytes).await;
1118+
let target = tokio::fs::canonicalize(&snap.path)
1119+
.await
1120+
.unwrap_or_else(|_| snap.path.clone());
1121+
let _ = atomic_write_bytes_preserving_mode(&target, bytes).await;
10971122
}
10981123
None => {
1099-
let _ = tokio::fs::remove_file(path).await;
1124+
let _ = tokio::fs::remove_file(&snap.path).await;
11001125
}
11011126
}
11021127
}
@@ -1108,6 +1133,16 @@ async fn undo_candidate(cwd: &Path, c: &Candidate, snapshot: &[(PathBuf, Option<
11081133
}
11091134
}
11101135

1136+
#[cfg(unix)]
1137+
async fn relink(link: &Path, at: &Path) -> std::io::Result<()> {
1138+
tokio::fs::symlink(link, at).await
1139+
}
1140+
1141+
#[cfg(windows)]
1142+
async fn relink(link: &Path, at: &Path) -> std::io::Result<()> {
1143+
tokio::fs::symlink_file(link, at).await
1144+
}
1145+
11111146
/// One candidate's outcome in the engine's scratch envelope.
11121147
struct EngineOutcome {
11131148
/// The engine vendored it (`applied`).

‎crates/socket-patch-cli/tests/repair_vendor_e2e.rs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1375,6 +1375,18 @@ async fn repair_offline_harvests_a_corrupt_artifacts_valid_members() {
13751375
/// the recorded bytes, and leaves the lockfile and ledger byte-identical.
13761376
#[tokio::test]
13771377
async fn repair_never_rewires_to_different_service_bytes() {
1378+
identity_kept_over_different_service_bytes(false).await;
1379+
}
1380+
1381+
/// Same, with the wired `package-lock.json` a symlink to the real lock: the
1382+
/// put-back rewrites the link's target and leaves the link in place.
1383+
#[cfg(unix)]
1384+
#[tokio::test]
1385+
async fn repair_identity_undo_follows_a_symlinked_lockfile() {
1386+
identity_kept_over_different_service_bytes(true).await;
1387+
}
1388+
1389+
async fn identity_kept_over_different_service_bytes(symlinked_lock: bool) {
13781390
let mock = MockServer::start().await;
13791391
mount_patch_api(&mock).await;
13801392
let tmp = tempfile::tempdir().unwrap();
@@ -1384,6 +1396,13 @@ async fn repair_never_rewires_to_different_service_bytes() {
13841396
"sha512-orig==",
13851397
);
13861398
let tgz = vendor_project(tmp.path(), &mock.uri(), &[]);
1399+
// The wired lock moved behind a symlink after vendoring.
1400+
#[cfg(unix)]
1401+
if symlinked_lock {
1402+
let lock = tmp.path().join("package-lock.json");
1403+
std::fs::rename(&lock, tmp.path().join("real-lock.json")).unwrap();
1404+
std::os::unix::fs::symlink("real-lock.json", &lock).unwrap();
1405+
}
13871406
let tgz_bytes = std::fs::read(&tgz).unwrap();
13881407
let lock1 = std::fs::read(tmp.path().join("package-lock.json")).unwrap();
13891408
let state1 = std::fs::read(tmp.path().join(".socket/vendor/state.json")).unwrap();
@@ -1426,4 +1445,8 @@ async fn repair_never_rewires_to_different_service_bytes() {
14261445
state1,
14271446
"ledger untouched"
14281447
);
1448+
if symlinked_lock {
1449+
let meta = std::fs::symlink_metadata(tmp.path().join("package-lock.json")).unwrap();
1450+
assert!(meta.file_type().is_symlink(), "the lockfile link is kept");
1451+
}
14291452
}

0 commit comments

Comments
 (0)