feat(relay): inventory unreachable Git objects (relay-only reland) - #2339
feat(relay): inventory unreachable Git objects (relay-only reland)#2339thomaspblock wants to merge 1 commit into
Conversation
…2264) Reland of #2264 (reverted in #2275), trimmed to buzz-relay only per review: - Dropped all Helm chart changes (deployment.yaml, values.yaml, values.schema.json). The worker is configured entirely via BUZZ_GIT_GC_* env vars with safe defaults (disabled); operators can opt in through the chart's existing relay.extraEnv. Dedicated chart knobs can land later as a deploy-only change. - Dropped the Hermit helm 3.16.4 pin (bin/helm) — it existed only to lint the removed chart changes. - Dropped the buzz-db rename (UsageMetricsLeader -> AdvisoryLockLeader). The GC worker reuses the existing try_lock_usage_metrics(lock_key) API with its own advisory lock key; the rename is deferred. The opt-in worker elects one relay replica via a Postgres advisory lock, marks objects reachable from every current repository pointer, and inventories the shared manifests/, packs/, and idx/ namespaces, exporting candidate counts, bytes, scan freshness, and completeness metrics. Bounded pointer count, listed objects, manifest transfer, and wall-clock duration. Dry-run only: nothing is ever deleted, and the worker does not start unless BUZZ_GIT_GC_ENABLED=true. Co-authored-by: Thomas Petersen <thomasp@squareup.com> Signed-off-by: Thomas Petersen <thomasp@squareup.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 7e34bee62cacaa9d8a96c14d5892a471b59a1983..9841b4e8d84af0b6971b31a565cfe0e392db500d (exact head 9841b4e8d84af0b6971b31a565cfe0e392db500d)
Risk: high. This is disabled-by-default, dry-run observability today, but its classifications are explicitly groundwork for future physical deletion. False candidate telemetry is therefore a data-safety design hazard, and the worker also changes unrelated exporter behavior when enabled.
Blocking findings
-
The production S3 scan path is absent from required automation, and its opt-in test is too permissive to serve as deletion-safety groundwork.
crates/buzz-relay/src/api/git/gc.rs:476-526silently returns success unlessBUZZ_GIT_S3_PROBE=1; neither the exact-head Unit Tests nor Relay E2E logs contain evidence that the flag/test ran. When manually enabled, the test asserts only lower bounds (>= 3reachable,>= 1candidate), not the exact isolated candidate set. Thus the normal suite can stay green while pointer listing, verified manifest loading, aggregate limits, or live-vs-orphan integration regresses; and the live row itself would not reject over-classification. The pure classifier test atgc.rs:415-431does causally protect the final reachability predicate (I mutation-proved that removing!reachable.contains(...)fails with(2,30)vs(1,20)), but it does not execute the production object-store workflow. Please run an isolated MinIO row in CI and assert the exact candidate result: orphan present, current manifest/pack/idx absent. Add causal boundary rows for truncated pointer/object listings and manifest-budget/timeout failures. -
Enabling GC globally extends stale retention for every gauge.
crates/buzz-relay/src/main.rs:135-144passesmax(usage_interval, gc_interval)intousage_metrics_idle_timeout_secs;crates/buzz-relay/src/metrics.rs:66-72applies that timeout toMetricKindMask::GAUGE, not just GC gauges. At defaults, hourly GC raises global stale-gauge retention from 900s to 10,800s; a 24h scan raises it to 72h. Departed community/connection series can therefore look current because an unrelated inventory cadence is long. Keep the exporter-wide timeout tied to usage emission and refresh/register GC gauges without changing all gauge lifetimes.
Contract trace
Deployment-global scope is appropriate: repo pointers include the community in their key while immutable manifests/packs/idx are shared CAS, so collecting every repos/ pointer before classifying the shared namespaces avoids cross-tenant false candidates. Corrupt/missing/oversized live manifests and truncated pointer inventory abort classification; object-prefix truncation marks the report incomplete; leadership uses a distinct detached advisory-lock session and demotes on failed liveness. Production code has no delete path. Scope matches the reland intent: .env.example, crates/buzz-relay, and the Git object-store documentation only; no DB/schema, Helm, Hermit, Desktop, mobile, or UI changes.
Validation at exact head
git diff --check 7e34bee...9841b4e...— PASS.cargo fmt --all -- --check— PASS.cargo clippy -p buzz-relay --all-targets -- -D warnings— PASS.- Fresh isolated MinIO with
BUZZ_GIT_S3_PROBE=1 cargo test -p buzz-relay api::git::gc::tests::live_scan_marks_current_objects_and_observes_orphans -- --exact --nocapture— PASS (1/1), but with the assertion weakness above. - Mutation of
classify_candidatesremoving the reachability exclusion, then the exact pure classifier test — causal FAIL as expected; original restored, clean tree. - Full
cargo test -p buzz-relay— environment gate not green: 712 passed, 12 failed, 27 ignored. Eleven failures were caused by the shared local Postgres lackingcommunities; the remaining mesh echo 504 matches the PR-described environment baseline. New GC tests passed, but the live test self-skips in the ordinary suite. - Exact-head GitHub checks are green and PR remains mergeable. CI success does not cover the opt-in MinIO row.
Manual/native evidence: not applicable; no Desktop/mobile/UI surface changed.
Residual risk: no exact-candidate isolated workflow assertion or automated limit/error-boundary evidence exists yet. Any new head invalidates this review until its delta and affected evidence are rechecked.
Summary
Relay-only reland of #2264 (reverted in #2275). Per review feedback from Tyler and Wes, this version removes everything outside
crates/buzz-relay:BUZZ_GIT_GC_*env vars with safe defaults (disabled). Operators can enable bounded dry-run scans in staging through the chart's existingrelay.extraEnv— nodeployment.yaml/values.yaml/values.schema.jsonedits. Dedicated chart knobs can land later as a separate deploy-only PR.bin/helmexisted only to lint the removed chart changes.buzz-dbchanges. The previous PR renamedUsageMetricsLeader→AdvisoryLockLeadercosmetically; this version reuses the existingtry_lock_usage_metrics(lock_key)API unchanged with a distinct GC advisory lock key (a code comment marks the deferred rename).Functionality is unchanged from #2264: an opt-in worker elects one relay replica, marks objects reachable from every current repository pointer, and inventories the shared
manifests/,packs/, andidx/namespaces. It exports observed candidate counts, bytes, scan freshness, and completeness while bounding pointer count, listed objects, manifest transfer, and wall-clock duration.This PR deliberately does not delete anything. Candidate status is not yet deletion proof: a later phase must persist continuous unreachability for a grace period and coordinate with concurrent publishers before any sweep can be safe. Disabled by default (
BUZZ_GIT_GC_ENABLED=false) — dead code at runtime until an operator opts in.Test plan
cargo test -p buzz-relay— 723 passed, 27 ignored (the one failure,api::mesh_demo::demo_join_forwarded_arm_round_trips_echo, fails identically onorigin/mainin this environment and is unrelated)cargo clippy -p buzz-relay --all-targets -- -D warnings— cleancargo fmt— cleancrates/buzz-relay,.env.example, anddocs/— no db, helm, or hermit changes