[anandh8x]

Summary

• Add Prometheus histograms for successful push and fetch durations.
• Track the origin ref-update timestamp through both peer transports and the sync queue.
• Record sync lag only after the mirror is registered and the queue item completes.

Validation

• cargo fmt --all -- --check
• cargo check --locked --workspace --all-targets
• cargo clippy --locked -p gitlawb-node --bin gitlawb-node -- -D warnings
• Focused metrics, sync, notification, and migration tests
• cargo test --locked -p gitlawb-node (825 passed; two pre-existing concurrency test failures remain outside this change)

[github-actions]

Thanks for the contribution. A couple of things will help us review this faster:

• Link the issue this addresses (Closes #123). For protocol changes, open an issue first.

See CONTRIBUTING.md. Update the PR and these notes will clear automatically.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d99e82ab-a88a-4d52-87ab-a16e23ee60b0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jatmn]

I found issues that need to be addressed before this is ready.

Merge readiness

• [P3] Link the tracking issue required by the contribution policy
  CONTRIBUTING.md:55
  This feature PR has no linked issue, and the active quality-signal triage request still asks for one. That leaves the expected semantics for the new operational metrics—including whether sync lag means public servability or end-to-end queue completion—without a durable, reviewable decision. Please add the issue that approved this work (or open one), state the intended metric boundaries and trust model there, and link it from the PR.

Findings

• [P2] Sample sync lag when the mirror actually becomes visible
  crates/gitlawb-node/src/sync.rs:428
upsert_mirror_repo has already registered a public, servable mirror, but the new observation is timestamped only after replicate_encrypted_blobs finishes and mark_sync_done succeeds. The blob-replication step performs additional origin/network/storage work that is unrelated to whether Git smart HTTP can serve the ref, and it can be arbitrarily slow. A healthy mirror that is immediately cloneable can therefore be reported as minutes behind, so gitlawb_sync_lag_seconds does not measure the advertised origin-to-visible latency.

  The root cause is using queue completion as a proxy for the visibility transition. Record a visible_at timestamp immediately after the successful, admitted mirror registration and use that for this histogram; retain a separate completion metric if encrypted-blob replication and queue settlement also need an SLO. Add a test that makes the post-registration replication work slow and proves the visibility histogram is unaffected.

• [P2] Do not count quarantined mirrors as successful visibility
  crates/gitlawb-node/src/sync.rs:414
  In iCaptcha enforcement mode, an unavailable proof makes quarantined true, but a successful upsert_mirror_repo still sets mirror_visible and records the lag. Quarantined mirrors are intentionally hidden from clone, fetch, and listings, so this reports successful visibility for a ref no caller can serve. This becomes especially misleading during an iCaptcha outage: the histogram will show completed low-lag mirrors while every affected repository remains unavailable.

  The root cause is treating successful persistence of a mirror row as equivalent to its admission for serving. Carry the actual admission/servability result through the sync success path and observe visibility only when the row is non-quarantined; a pre-existing quarantine must remain non-visible after a resync because upsert_mirror_repo deliberately preserves it. Add an enforced-iCaptcha regression test for both a newly quarantined mirror and a resync of one already quarantined.

• [P2] Bound untrusted origin timestamps before adding them to the histogram
  crates/gitlawb-node/src/sync.rs:187
  The new helper accepts any parseable past timestamp. Both ingress paths persist a peer-provided value, and the default HTTP peer-write mode accepts an unsigned request claiming a known peer. A request with 1970-01-01T00:00:00Z reaches the successful sync path and permanently adds a multi-decade value to the process-wide histogram's sum/+Inf bucket. Because Prometheus histograms retain their cumulative sum and bucket counts for the lifetime of the process, one request can corrupt latency alerts, averages, and quantiles until restart; repeated requests can keep the SLO unusable.

  The root cause is promoting a remote timestamp to trusted measurement input without a local receipt anchor or a maximum acceptable age. Preserve the origin timestamp as optional diagnostic metadata, but only observe it when it is within a documented bounded skew of a locally recorded receipt/enqueue time; otherwise increment a distinct invalid/stale-origin counter or omit the sample. This should be enforced uniformly for HTTP and gossipsub ingress, with tests for malformed, future, plausible, and arbitrarily old values.

[euxarist]

Label hygiene is good (the new histograms carry no labels, so no cardinality issue) and the migration upgrade-path test satisfies the versioned-format rule. Two things before this leaves draft:

1. The sync-lag origin timestamp is accepted on /api/v1/sync/notify, which is deliberately unsigned. Past and non-positive values are ignored (tested), but there is no upper clamp, so a known peer can push arbitrarily future-stale origins and inflate the >= 3600s / infinity buckets of gitlawb_sync_lag_seconds at will. Since the input channel is authenticated only by peer existence, please clamp the origin age to a sane maximum before it reaches the histogram.

2. Worth a sentence in the description: the origin timestamp is taken at replication-tail start rather than push start, so the metric slightly under-measures true lag. Fine for a draft, just document it.

Also, the description does not say why this is still a draft; if there are known gaps, listing them helps reviewers.

[Vasanthdev2004]

Checked at 9f4f5096, which is still the head @jatmn reviewed, so their three P2s stand as written. The lag is sampled after replicate_encrypted_blobs and mark_sync_done rather than when the mirror is registered. mirror_visible is true for a quarantined upsert_mirror_repo. And sync_lag_from_origin_timestamp accepts any past timestamp, so one /api/v1/sync/notify from a known peer with a 1970 date puts a multi-decade sample in the histogram until the process restarts. The issue link is still missing too.

One more thing for the rebase. The branch is 116 commits behind main, and the only conflict is the migration slot: main already has version: 18 (pinned_cids_cid_index), and it's at 26 now. The runner skips any version already recorded in schema_migrations and never reads the name, as the comment above main's v17 explains, so keeping both entries at 18 means one of them never runs. On a node that already has main's 18 applied it's this one, and then enqueue_sync and dequeue_pending_syncs fail on the missing origin_timestamp column, which stops the sync queue. It needs a number nothing else has claimed, and migration_v18_adds_sync_queue_origin_timestamp then has to delete that row rather than 18, which on main belongs to the pinned-CID index.

I haven't run the suite here; this comes from the code and a trial merge against current main. Requesting changes until those are in.

[beardthelion]

Thanks @anandh8x: your commit was carried over unchanged into #580. That branch lands the outstanding review asks: the sync-lag observation now happens when the mirror becomes visible rather than after the encrypted-blob tail, a quarantined mirror reports no visibility lag, and an absurd sender-supplied origin timestamp is clamped at a day instead of parking a fabricated observation in the tail buckets. The migration moved to v47 after main's slot moved. Closing this one.