mirror of
https://relay.ngit.dev/npub15qydau2hjma6ngxkl2cyar74wzyjshvl65za5k5rl69264ar2exs5cyejr/ngit-grasp.git
synced 2026-10-05 15:08:24 +00:00
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
This commit is contained in:
+2
-1
@@ -13,7 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
to `refs/nostr/<event-id>` whose PR event is not yet known is staged in the
|
to `refs/nostr/<event-id>` whose PR event is not yet known is staged in the
|
||||||
repository that received it, promoted into shared storage when the event is
|
repository that received it, promoted into shared storage when the event is
|
||||||
accepted, and reclaimed after its pending ref expires. Pushes backed by a
|
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
|
- 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
|
lost its purgatory placeholder, matching it by the event's service-local
|
||||||
|
|||||||
@@ -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
|
checkpoint. A view whose remaining staged objects belong to pending refs is
|
||||||
parked until the next request rather than polled.
|
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
|
### Limits
|
||||||
|
|
||||||
- Staging is not a storage quota. It bounds how long an unsigned upload is
|
- Staging is not a storage quota. It bounds how long an unsigned upload is
|
||||||
|
|||||||
@@ -216,9 +216,10 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
let purgatory = Purgatory::new(fixture.storage.git_data_path());
|
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 {
|
purgatory.set_prs_cleanup_ctx(crate::purgatory::PrsCleanupCtx {
|
||||||
git_data_path: fixture.storage.git_data_path().to_owned(),
|
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)
|
recover_placeholders(&fixture.storage, &purgatory, &db)
|
||||||
.await
|
.await
|
||||||
@@ -231,11 +232,15 @@ mod tests {
|
|||||||
.await;
|
.await;
|
||||||
purgatory.cleanup();
|
purgatory.cleanup();
|
||||||
if prs {
|
if prs {
|
||||||
|
super::super::worker::maintain(&fixture.view, &locks)
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
assert!(!fixture.view.exists());
|
assert!(!fixture.view.exists());
|
||||||
} else {
|
} else {
|
||||||
compact(&fixture.view).unwrap();
|
compact(&fixture.view).unwrap();
|
||||||
assert!(!object_exists(&fixture.view, &oid).unwrap());
|
assert!(!object_exists(&fixture.view, &oid).unwrap());
|
||||||
}
|
}
|
||||||
|
assert!(!view.record.exists());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -263,4 +263,52 @@ mod tests {
|
|||||||
assert_eq!(outcome.unwrap(), Compaction::Done);
|
assert_eq!(outcome.unwrap(), Compaction::Done);
|
||||||
assert!(!fixture.view.exists());
|
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();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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
|
/// Remove a `/prs/` repository that has no refs and no push in flight, so
|
||||||
/// abandoned repositories do not accumulate. Returns whether it was removed.
|
/// 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
|
/// The caller holds `state.mu`. That mutex also gates `in_flight` updates, so
|
||||||
/// a repository is never removed while a push is being received.
|
/// 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;
|
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) {
|
match std::fs::remove_dir_all(repo_path) {
|
||||||
Ok(()) => {
|
Ok(()) => {
|
||||||
debug!(repo = %repo_path.display(), "Removed zero-ref /prs/ repository");
|
debug!(repo = %repo_path.display(), "Removed zero-ref /prs/ repository");
|
||||||
|
|||||||
Reference in New Issue
Block a user