Skip to content

Commit 7e9fde0

Browse files
committed
fix(storage): in-flight etag provenance closes the upload-to-clear crash wedge
beardthelion round on 3cbcb2b: - [P1] The pending marker now records the upload's intended etag (the archive's content MD5, written just before the PUT — S3 single-part ETags and the fs backend's etags are exactly that value). A crash anywhere between the PUT landing and the marker clear is recognized on recovery as "storage holds this node's own completed upload": sync and the startup sweep adopt it and clear the marker instead of wedging a byte-identical repo behind "reconcile manually". On backends whose etags are not content MD5 (IPFS CIDs), the intent simply never matches and recovery keeps its fail-safe behavior. - [P1] The ack-window test now clears the in-memory etag cache after the simulated crash (a real restart would have lost it), so it goes red if mark_pending is gutted — previously the cache-hit skip masked the marker entirely. - [P2] fs backend etags are content MD5 (persisted in the sidecar, legacy size-mtime fallback unchanged): different content still always differs, identical content is now the same token, and the etag a PUT will produce is predictable. put() restores the previous sidecar when the blob rename fails, so a failed publish can't leave a new etag over old content — which the marker rules would read as terminal divergence. - [P3] The startup sweep logs read_dir failures loudly instead of reporting a clean zero (which also seeded the gauge), and collects marker-tmp litter left by a crash mid-rename. The fork publication window stays deferred as agreed, pending a tracked issue. New regression test simulates the post-PUT crash state and asserts heal-not-wedge; the consecutive-failures test now pins the marker BASE line (the in-flight line legitimately changes per attempt).
1 parent 3cbcb2b commit 7e9fde0

5 files changed

Lines changed: 279 additions & 51 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/gitlawb-node/Cargo.toml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,11 @@ async-trait = "0.1"
3737
cid = { workspace = true }
3838
hex = { workspace = true }
3939
sha2 = { workspace = true }
40+
# Content etags for repo archives: S3 single-part ETags are the body MD5, so a
41+
# client-side MD5 lets the uploader record its *intended* etag before the PUT
42+
# (crash-recovery provenance for the pending-upload marker). Not used for
43+
# anything security-sensitive.
44+
md-5 = "0.10"
4045
hmac = { workspace = true }
4146
http-body-util = "0.1"
4247
tokio-util = { version = "0.7", features = ["io"] }

‎crates/gitlawb-node/src/git/repo_store.rs‎

Lines changed: 191 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -112,12 +112,12 @@ impl RepoStore {
112112
if local_path.exists() {
113113
// The local copy has a write that storage never received (its
114114
// upload failed, or the node stopped first). The marker records
115-
// the storage etag that write was BASED on, so we can tell
116-
// "storage unchanged — local strictly ahead" apart from
117-
// "another node advanced storage — genuine divergence" rather
118-
// than treating local as authoritative forever.
119-
let base = std::fs::read_to_string(&marker).unwrap_or_default();
120-
let base = base.trim();
115+
// the storage etag that write was BASED on — and, if an upload
116+
// was in flight, the etag it was going to produce — so we can
117+
// tell "storage unchanged — local strictly ahead" and "that's
118+
// our own completed upload" apart from "another node advanced
119+
// storage — genuine divergence".
120+
let pm = read_pending_marker(local_path);
121121
let remote = match archive.head_etag(owner_slug, repo_name).await {
122122
Ok(r) => r,
123123
Err(e) => {
@@ -134,7 +134,20 @@ impl RepoStore {
134134
// local is strictly ahead. Serve it; the next successful
135135
// post-write upload re-syncs storage and clears the marker.
136136
None => return Ok(()),
137-
Some(r) if r == base => {
137+
Some(r) if pm.matches_own_inflight(r) => {
138+
// The upload landed but the process died before the
139+
// marker clear: local and storage hold the same
140+
// content. Adopt it as synced.
141+
debug!(repo = %repo_name,
142+
"storage matches our own in-flight upload — marker cleared, synced");
143+
self.versions
144+
.lock()
145+
.await
146+
.insert(format!("{owner_slug}/{repo_name}"), r.to_string());
147+
clear_pending_upload_after_success(local_path, Some(r));
148+
return Ok(());
149+
}
150+
Some(r) if pm.matches_base(r) => {
138151
warn!(repo = %repo_name,
139152
"local copy ahead of storage (pending upload) — skipping download");
140153
return Ok(());
@@ -385,16 +398,14 @@ impl RepoStore {
385398
let base = self.versions.lock().await.get(&key).cloned();
386399
mark_pending_upload(&local_path, base.as_deref())
387400
.context("persisting pending-upload marker")?;
388-
// The marker's recorded base is authoritative, not the cache: if a
401+
// The marker's recorded state is authoritative, not the cache: if a
389402
// marker already existed (earlier failed upload), mark_pending_upload
390403
// preserved its ORIGINAL base while the versions cache was invalidated
391404
// by that failure — or emptied entirely by a restart. Comparing
392405
// against the cache value would misread that state as divergence.
393-
let base = std::fs::read_to_string(pending_upload_marker(&local_path))
394-
.map(|s| s.trim().to_string())
395-
.unwrap_or_else(|_| base.unwrap_or_default());
406+
let pm = read_pending_marker(&local_path);
396407
match self
397-
.upload_locked_with_marker(&owner_slug, repo_name, &local_path, &base)
408+
.upload_locked_with_marker(&owner_slug, repo_name, &local_path, &pm)
398409
.await
399410
{
400411
Ok(PendingUploadOutcome::Uploaded) => Ok(()),
@@ -436,19 +447,37 @@ impl RepoStore {
436447
let mut still_pending = 0usize;
437448

438449
let mut markers: Vec<(String, String, PathBuf)> = Vec::new(); // (slug, repo, local)
439-
let Ok(owners) = std::fs::read_dir(&self.repos_dir) else {
440-
return (0, 0);
450+
// Scan failures are logged loudly: a sweep that scanned nothing must
451+
// not look identical to a node with no pending markers — especially
452+
// since the gauge below is seeded from this same scan.
453+
let owners = match std::fs::read_dir(&self.repos_dir) {
454+
Ok(owners) => owners,
455+
Err(e) => {
456+
warn!(dir = %self.repos_dir.display(), err = %e,
457+
"pending-upload sweep: cannot read repos dir — sweep skipped");
458+
return (0, 0);
459+
}
441460
};
442461
for owner in owners.flatten() {
443462
if !owner.path().is_dir() {
444463
continue;
445464
}
446465
let slug = owner.file_name().to_string_lossy().into_owned();
447-
let Ok(entries) = std::fs::read_dir(owner.path()) else {
448-
continue;
466+
let entries = match std::fs::read_dir(owner.path()) {
467+
Ok(entries) => entries,
468+
Err(e) => {
469+
warn!(dir = %owner.path().display(), err = %e,
470+
"pending-upload sweep: cannot read owner dir — skipped");
471+
continue;
472+
}
449473
};
450474
for entry in entries.flatten() {
451475
let name = entry.file_name().to_string_lossy().into_owned();
476+
// Marker-write temp litter (crash mid-rename): collect it.
477+
if name.starts_with(".pending-upload.tmp-") {
478+
let _ = std::fs::remove_file(entry.path());
479+
continue;
480+
}
452481
// Marker layout: `.{repo}.git.pending-upload`
453482
let Some(repo_dir) = name
454483
.strip_prefix('.')
@@ -479,16 +508,14 @@ impl RepoStore {
479508
clear_pending_upload(&local_path);
480509
continue;
481510
}
482-
let base =
483-
std::fs::read_to_string(pending_upload_marker(&local_path)).unwrap_or_default();
484-
let base = base.trim().to_string();
485-
// The base-vs-remote decision and the upload both happen inside
511+
let pm = read_pending_marker(&local_path);
512+
// The marker-vs-remote decision and the upload both happen inside
486513
// `upload_locked_with_marker`, UNDER the advisory lock: an
487514
// unlocked pre-check here could pass, then block on a concurrent
488515
// push's lock for that push's whole duration, and the stale
489516
// decision would clobber the push's freshly-uploaded archive.
490517
match self
491-
.upload_locked_with_marker(&slug, &repo_name, &local_path, &base)
518+
.upload_locked_with_marker(&slug, &repo_name, &local_path, &pm)
492519
.await
493520
{
494521
Ok(PendingUploadOutcome::Uploaded) => {
@@ -529,7 +556,7 @@ impl RepoStore {
529556
owner_slug: &str,
530557
repo_name: &str,
531558
local_path: &Path,
532-
base: &str,
559+
marker: &PendingMarker,
533560
) -> Result<PendingUploadOutcome> {
534561
let Some(ref archive) = self.archive else {
535562
anyhow::bail!("upload_locked_with_marker called without a storage backend");
@@ -543,11 +570,32 @@ impl RepoStore {
543570
.head_etag(owner_slug, repo_name)
544571
.await
545572
.context("storage head under lock before pending upload")?;
546-
if remote.as_deref().unwrap_or("") != base {
547-
return Ok(PendingUploadOutcome::Diverged);
573+
match remote.as_deref() {
574+
// Storage already holds exactly what this node was uploading
575+
// when it died — synced; no PUT needed.
576+
Some(r) if marker.matches_own_inflight(r) => {
577+
self.versions
578+
.lock()
579+
.await
580+
.insert(label.clone(), r.to_string());
581+
clear_pending_upload_after_success(local_path, Some(r));
582+
return Ok(PendingUploadOutcome::Uploaded);
583+
}
584+
Some(r) if !marker.matches_base(r) => {
585+
return Ok(PendingUploadOutcome::Diverged);
586+
}
587+
None if !marker.matches_base("") => {
588+
return Ok(PendingUploadOutcome::Diverged);
589+
}
590+
_ => {}
548591
}
592+
// Record the intended etag in the marker before the PUT: a crash
593+
// after the PUT lands is then recognizable (above) as our own
594+
// completed upload instead of wedging on false divergence.
549595
let etag = archive
550-
.upload(owner_slug, repo_name, local_path)
596+
.upload_with_intent(owner_slug, repo_name, local_path, |intended| {
597+
record_inflight_upload(local_path, intended);
598+
})
551599
.await
552600
.context("uploading repo to storage under lock")?;
553601
if let Some(ref etag) = etag {
@@ -908,7 +956,12 @@ impl RepoWriteGuard {
908956
warn!(repo = %self.repo_name, err = %e, "failed to write pending-upload marker");
909957
}
910958
match archive
911-
.upload(&self.owner_slug, &self.repo_name, &self.local_path)
959+
.upload_with_intent(
960+
&self.owner_slug,
961+
&self.repo_name,
962+
&self.local_path,
963+
|intended| record_inflight_upload(&self.local_path, intended),
964+
)
912965
.await
913966
{
914967
Ok(Some(etag)) => {
@@ -1007,6 +1060,65 @@ pub(crate) fn clear_pending_upload(local_path: &Path) {
10071060
}
10081061
}
10091062

1063+
/// Etags compared structurally: S3 returns them quoted, our recorded values
1064+
/// are bare, and whitespace can differ across the marker round-trip.
1065+
fn norm_etag(e: &str) -> &str {
1066+
e.trim().trim_matches('"')
1067+
}
1068+
1069+
/// Parsed pending-upload marker. Line 1 is the storage etag the local write
1070+
/// was BASED on; optional line 2 is the etag the in-flight upload was going to
1071+
/// produce (the archive's content MD5, recorded just before the PUT).
1072+
struct PendingMarker {
1073+
base: String,
1074+
inflight: Option<String>,
1075+
}
1076+
1077+
impl PendingMarker {
1078+
/// Storage still holds exactly what the local write was based on: local
1079+
/// is strictly ahead.
1080+
fn matches_base(&self, remote: &str) -> bool {
1081+
norm_etag(remote) == norm_etag(&self.base)
1082+
}
1083+
1084+
/// Storage holds exactly what THIS node was uploading when it died: the
1085+
/// upload completed but the marker was never cleared. Local and storage
1086+
/// are the same content — synced, not diverged. Only recognizable on
1087+
/// backends whose etag is the body MD5 (S3-compatibles, fs); elsewhere
1088+
/// this never matches and recovery keeps its fail-safe wedge.
1089+
fn matches_own_inflight(&self, remote: &str) -> bool {
1090+
self.inflight
1091+
.as_deref()
1092+
.is_some_and(|i| norm_etag(remote) == norm_etag(i))
1093+
}
1094+
}
1095+
1096+
fn read_pending_marker(local_path: &Path) -> PendingMarker {
1097+
let content = std::fs::read_to_string(pending_upload_marker(local_path)).unwrap_or_default();
1098+
let mut lines = content.lines();
1099+
let base = lines.next().unwrap_or("").trim().to_string();
1100+
let inflight = lines
1101+
.next()
1102+
.map(|l| l.trim().to_string())
1103+
.filter(|l| !l.is_empty());
1104+
PendingMarker { base, inflight }
1105+
}
1106+
1107+
/// Best-effort: rewrite the marker as `base\nintended` just before the PUT, so
1108+
/// a crash anywhere between the PUT landing and the post-upload clear leaves a
1109+
/// marker that names the uploaded content — recovery then recognizes storage
1110+
/// as this node's own completed upload instead of wedging on false divergence.
1111+
fn record_inflight_upload(local_path: &Path, intended_etag: &str) {
1112+
let marker = pending_upload_marker(local_path);
1113+
let base = read_pending_marker(local_path).base;
1114+
let tmp = marker.with_file_name(format!(".pending-upload.tmp-{}", uuid::Uuid::new_v4()));
1115+
if std::fs::write(&tmp, format!("{base}\n{intended_etag}")).is_ok() {
1116+
let _ = std::fs::rename(&tmp, &marker);
1117+
} else {
1118+
let _ = std::fs::remove_file(&tmp);
1119+
}
1120+
}
1121+
10101122
/// Remove the marker after a *successful* upload, first atomically rewriting
10111123
/// its base to the just-uploaded etag. A crash between the rewrite and the
10121124
/// unlink then reads as "local ahead, base matches" — which self-heals on the
@@ -1560,16 +1672,18 @@ mod tests {
15601672
std::fs::write(local.join("HEAD"), b"write-1\n").unwrap();
15611673
let guard = store.acquire_write("owner", "repo").await.unwrap();
15621674
assert!(guard.release(true).await.is_err(), "injected put must fail");
1563-
let base_after_first = std::fs::read_to_string(pending_upload_marker(&local)).unwrap();
1564-
assert!(!base_after_first.trim().is_empty(), "base must be recorded");
1675+
let base_after_first = read_pending_marker(&local).base;
1676+
assert!(!base_after_first.is_empty(), "base must be recorded");
15651677

15661678
// Write 2: acquire must succeed (storage unchanged == local-ahead),
15671679
// and the second failed release must NOT re-mark with an empty base.
1680+
// (The in-flight line legitimately changes per attempt; the BASE is
1681+
// the invariant.)
15681682
let guard = store.acquire_write("owner", "repo").await.unwrap();
15691683
std::fs::write(local.join("HEAD"), b"write-2\n").unwrap();
15701684
assert!(guard.release(true).await.is_err());
15711685
assert_eq!(
1572-
std::fs::read_to_string(pending_upload_marker(&local)).unwrap(),
1686+
read_pending_marker(&local).base,
15731687
base_after_first,
15741688
"an existing marker's base must be preserved on re-mark"
15751689
);
@@ -1613,6 +1727,10 @@ mod tests {
16131727
std::fs::write(local.join("HEAD"), b"ACKED\n").unwrap();
16141728
guard.mark_pending().await.unwrap();
16151729
drop(guard); // crash before release() is ever polled
1730+
// A real restart loses the in-memory etag cache; without this the
1731+
// cache-hit skip masks the marker and the test passes with
1732+
// mark_pending gutted.
1733+
store.versions.lock().await.clear();
16161734

16171735
store
16181736
.sync_down_if_stale("owner", "repo", &local, true)
@@ -1625,6 +1743,50 @@ mod tests {
16251743
);
16261744
}
16271745

1746+
/// beardthelion P1 regression: a crash between a successful upload and
1747+
/// the marker clear must NOT read as divergence. The marker records the
1748+
/// upload's intended etag (content MD5) before the PUT; recovery finding
1749+
/// storage at exactly that etag recognizes its own completed upload and
1750+
/// heals instead of wedging a byte-identical repo behind "reconcile
1751+
/// manually".
1752+
#[tokio::test]
1753+
async fn crash_after_upload_before_clear_heals_via_inflight_etag() {
1754+
let store_root = tempfile::tempdir().unwrap();
1755+
let repos_dir = tempfile::tempdir().unwrap();
1756+
let store = store_with_fs_archive(repos_dir.path().to_path_buf(), store_root.path());
1757+
let archive = store.archive.as_ref().unwrap();
1758+
1759+
// Simulate the crash state: storage holds the content this node was
1760+
// uploading (etag E), while the marker still names the pre-upload
1761+
// base B plus the in-flight etag E.
1762+
let seed = tempfile::tempdir().unwrap();
1763+
std::fs::write(seed.path().join("HEAD"), b"uploaded\n").unwrap();
1764+
archive.upload("owner", "repo", seed.path()).await.unwrap();
1765+
let remote = archive.head_etag("owner", "repo").await.unwrap().unwrap();
1766+
1767+
let local = repos_dir.path().join("owner").join("repo.git");
1768+
std::fs::create_dir_all(&local).unwrap();
1769+
std::fs::write(local.join("HEAD"), b"uploaded\n").unwrap();
1770+
mark_pending_upload(&local, Some("pre-upload-base")).unwrap();
1771+
record_inflight_upload(&local, &remote);
1772+
1773+
// Write path must heal, not wedge: marker cleared, cache adopted.
1774+
store
1775+
.sync_down_if_stale("owner", "repo", &local, true)
1776+
.await
1777+
.expect("own completed upload must not read as divergence");
1778+
assert!(
1779+
!pending_upload_marker(&local).exists(),
1780+
"marker must be cleared once storage is recognized as our upload"
1781+
);
1782+
assert_eq!(std::fs::read(local.join("HEAD")).unwrap(), b"uploaded\n");
1783+
// And subsequent syncs skip on the adopted etag.
1784+
store
1785+
.sync_down_if_stale("owner", "repo", &local, true)
1786+
.await
1787+
.unwrap();
1788+
}
1789+
16281790
/// The lazy-migration existence check must propagate failure instead of
16291791
/// reading it as "absent" and uploading over a possibly-newer archive.
16301792
#[sqlx::test]

0 commit comments

Comments
 (0)