From 1b7d59d23c5b0ebbccce5c6518c3e64a8d2de9ea Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Wed, 30 Sep 2026 13:29:03 +0000 Subject: [PATCH] fix(grasp06): reclaim staging metadata before removing empty views Live expiry testing found that purgatory deleted an empty /prs/ directory before staging maintenance could unregister it. The worker then skipped the missing view, leaving its registry JSON until startup recovery. Hand empty staged views to the existing maintenance worker. It reclaims objects and unregisters staging before calling back to remove the directory. Keep the existing path mutex, in-flight push check and owed-history guard; unstaged empty views retain immediate cleanup. This changes runtime cleanup ordering only, not expiry deadlines or startup orphan recovery. Add a regression through purgatory expiry and the real background worker, with a bounded condition wait for both the view and registry to disappear. Update the recovery test to run deferred maintenance and assert unregistering. The regression failed on the original code at premature directory removal. Validation: 19 staging unit tests, 67 GRASP-06 hosting tests and 62 pending upload staging tests pass; cargo fmt --all -- --check and git diff --check pass. Assisted-by: GPT-6 --- CHANGELOG.md | 3 +- docs/explanation/git-family-object-storage.md | 5 ++ src/git/staging/recovery.rs | 7 ++- src/git/staging/worker.rs | 48 +++++++++++++++++++ src/grasp06/receive.rs | 8 ++++ 5 files changed, 69 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74fa6ad..e73ed26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 to `refs/nostr/` whose PR event is not yet known is staged in the repository that received it, promoted into shared storage when the event is accepted, and reclaimed after its pending ref expires. Pushes backed by a - signed State or PR event are stored as before. + signed State or PR event are stored as before. Empty staged `/prs/` + repositories are removed after their staging metadata is reclaimed. - Accept a GRASP-06 `/prs/` PR event whose pushed ref survived a crash that lost its purgatory placeholder, matching it by the event's service-local diff --git a/docs/explanation/git-family-object-storage.md b/docs/explanation/git-family-object-storage.md index 8389bbb..53e5022 100644 --- a/docs/explanation/git-family-object-storage.md +++ b/docs/explanation/git-family-object-storage.md @@ -290,6 +290,11 @@ This closes the window between receive-pack and the periodic purgatory checkpoint. A view whose remaining staged objects belong to pending refs is parked until the next request rather than polled. +Runtime cleanup of an empty staged `/prs/` view also requests maintenance +instead of deleting the directory directly. The worker reclaims its objects +and unregisters staging before removing the empty view, so expiry and failed +pushes cannot leave orphaned registry records. + ### Limits - Staging is not a storage quota. It bounds how long an unsigned upload is diff --git a/src/git/staging/recovery.rs b/src/git/staging/recovery.rs index 62d921c..9fd71e3 100644 --- a/src/git/staging/recovery.rs +++ b/src/git/staging/recovery.rs @@ -216,9 +216,10 @@ mod tests { } } let purgatory = Purgatory::new(fixture.storage.git_data_path()); + let locks = crate::grasp06::receive::new_repo_init_locks(); purgatory.set_prs_cleanup_ctx(crate::purgatory::PrsCleanupCtx { git_data_path: fixture.storage.git_data_path().to_owned(), - repo_init_locks: Default::default(), + repo_init_locks: locks.clone(), }); recover_placeholders(&fixture.storage, &purgatory, &db) .await @@ -231,11 +232,15 @@ mod tests { .await; purgatory.cleanup(); if prs { + super::super::worker::maintain(&fixture.view, &locks) + .await + .unwrap(); assert!(!fixture.view.exists()); } else { compact(&fixture.view).unwrap(); assert!(!object_exists(&fixture.view, &oid).unwrap()); } + assert!(!view.record.exists()); } } diff --git a/src/git/staging/worker.rs b/src/git/staging/worker.rs index b186419..d03cc52 100644 --- a/src/git/staging/worker.rs +++ b/src/git/staging/worker.rs @@ -263,4 +263,52 @@ mod tests { assert_eq!(outcome.unwrap(), Compaction::Done); assert!(!fixture.view.exists()); } + + #[tokio::test] + async fn prs_expiry_reclaims_the_registry_before_removing_the_view() { + use crate::purgatory::{PrsCleanupCtx, Purgatory}; + use nostr_sdk::prelude::Keys; + + let submitter = Keys::generate().public_key(); + let fixture = Fixture::with_view(&format!("prs/{}", submitter.to_hex())); + stage(&fixture.view, &[]).unwrap(); + let abandoned = commit(&fixture.view, "abandoned", None); + super::super::stage_upload( + &fixture.view, + &[], + &[super::super::Tip::new(PENDING, &abandoned)], + ) + .unwrap(); + reference(&fixture.view, PENDING, &abandoned); + let record = View::resolve(&fixture.view).unwrap().unwrap().record; + let locks = new_repo_init_locks(); + let purgatory = Purgatory::new(fixture.storage.git_data_path()); + purgatory.set_prs_cleanup_ctx(PrsCleanupCtx { + git_data_path: fixture.storage.git_data_path().to_owned(), + repo_init_locks: locks.clone(), + }); + purgatory.recover_upload_placeholder( + PENDING.strip_prefix("refs/nostr/").unwrap().to_owned(), + abandoned.clone(), + (submitter, fixture.key.identifier.clone()), + true, + std::time::SystemTime::UNIX_EPOCH, + ); + + assert_eq!(purgatory.cleanup(), (0, 0, 1)); + assert!( + fixture.view.exists(), + "expiry must leave the view for staging maintenance" + ); + assert!(record.exists()); + + let worker = tokio::spawn(run_worker(fixture.storage.clone(), locks)); + wait_until( + "expired PRS view and staging record to be reclaimed", + || !fixture.view.exists() && !record.exists(), + ) + .await; + assert!(!object_exists(&fixture.family(), &abandoned).unwrap()); + worker.abort(); + } } diff --git a/src/grasp06/receive.rs b/src/grasp06/receive.rs index 231d102..16b1de0 100644 --- a/src/grasp06/receive.rs +++ b/src/grasp06/receive.rs @@ -459,6 +459,8 @@ fn finish_prs_receive_pack(state: &PrsPathState, repo_path: &Path) { /// Remove a `/prs/` repository that has no refs and no push in flight, so /// abandoned repositories do not accumulate. Returns whether it was removed. +/// Staged views are queued for maintenance, which unregisters staging before +/// calling back here to remove the directory. /// /// The caller holds `state.mu`. That mutex also gates `in_flight` updates, so /// a repository is never removed while a push is being received. @@ -469,6 +471,12 @@ pub(crate) fn remove_idle_empty_repo(state: &PrsPathState, repo_path: &Path) -> { return false; } + if crate::git::staging::is_staged(repo_path) { + // Deleting the view now would strand its registry record: maintenance + // cannot resolve or unregister a view after its directory is gone. + crate::git::staging::request_maintenance(repo_path); + return false; + } match std::fs::remove_dir_all(repo_path) { Ok(()) => { debug!(repo = %repo_path.display(), "Removed zero-ref /prs/ repository");