fix(replication): cap first-audit pending admission at the launch budget#183
fix(replication): cap first-audit pending admission at the launch budget#183grumbach wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens ADR-0004’s monetized first-audit pipeline so the pending admission queue is sized to what the launch budget can realistically drain within the answerability window, preventing an ever-aging backlog under sustained nomination load. It also expands observability (audit outcome/drops counters, scheduler summary fields) and updates payment-verifier nomination behavior and ADR documentation.
Changes:
- Cap first-audit
pendingadmission atFIRST_AUDIT_PENDING_CAPderived from the launch budget + effective answerability window, with LRU displacement counted ascapacity_evicted. - Add cumulative audit outcome/drop accounting and emit it alongside the existing replication traffic summary.
- Update payment verification to nominate only the actually-paid candidate(s), add settlement payee-prefix checking, and add an opt-in A/B e2e workload driver plus ADR amendments.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/mod.rs | Registers the new opt-in A/B workload module. |
| tests/e2e/first_audit_ab.rs | Adds an environment-gated A/B workload driver for production-shaped paid uploads + first-audit behavior comparison. |
| src/replication/protocol.rs | Adds process-global audit pass/fail-by-reason and responder-drop counters + summary log. |
| src/replication/mod.rs | Implements the capped pending scheduler + reservation/promotion flow, adds new scheduler observability, and wires audit outcome/drop recording. |
| src/replication/config.rs | Documents and centralizes first-audit limiter constants (launch interval, burst, inflight, jitter, ingress capacity, etc.). |
| src/payment/verifier.rs | Narrows deterministic first-audit nomination to paid candidates, adds settlement redirect rejection, and updates test hooks accordingly. |
| docs/adr/ADR-0004-commitment-bound-quote-pricing.md | Adds ADR-0004 Amendments 2–4, including the pending admission sizing rationale and semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(recorded) = recorded_rewards_prefix { | ||
| let expected = Self::rewards_address_prefix(&candidate.quote.rewards_address); | ||
| if recorded != expected { | ||
| return Err(Error::Payment(format!( | ||
| "Median-priced quote settlement for peer {:?} was redirected: recorded rewards address prefix {} does not match the quote's {}", | ||
| candidate.encoded_peer_id, | ||
| hex::encode(recorded), | ||
| hex::encode(expected) | ||
| ))); | ||
| } | ||
| } |
cbc2f5e to
d1fc1dd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
src/replication/protocol.rs:1105
- These assertions use exact deltas on process-global counters, which can fail if another test increments the same counter between the load and the assert. Use a >= check so the test remains correct under parallel execution.
let before = AUDIT_PASS[pass_slot].load(Ordering::Relaxed);
record_audit_pass(AuditOutcomeKind::Subtree);
assert_eq!(AUDIT_PASS[pass_slot].load(Ordering::Relaxed), before + 1);
src/replication/protocol.rs:1126
- Same flakiness risk as the pass assertion: these are global counters and other tests may increment them. Assert that the intended slot increased by at least 1 instead of exactly 1.
assert_eq!(
AUDIT_FAIL[slot].load(Ordering::Relaxed),
before + 1,
"kind {kind_index} reason {reason:?} must land in its own slot",
);
src/replication/protocol.rs:1140
- Same flakiness risk as above: AUDIT_DROPPED is process-global and may be incremented by other tests. Use a >= assertion to avoid nondeterministic failures in parallel runs.
assert_eq!(
AUDIT_DROPPED[kind_index].load(Ordering::Relaxed),
before + 1
);
| @@ -1673,6 +2295,7 @@ impl ReplicationEngine { | |||
| )) => { | |||
| protocol::log_traffic_summary(); | |||
| protocol::log_served_peers_summary(); | |||
| protocol::log_audit_outcome_summary(); | |||
| /// No other test mutates the audit outcome counters, so per-slot deltas are | ||
| /// stable even under parallel test execution. |
| let cutoff_secs = GOSSIP_ANSWERABILITY_TTL | ||
| .as_secs() | ||
| .saturating_sub(MONETIZED_AUDIT_SKEW_MARGIN.as_secs()); | ||
| // Compile-time division-by-zero if the launch interval is ever zeroed: | ||
| // a zero interval makes the budget (and this cap) meaningless. | ||
| let launchable = cutoff_secs / config::FIRST_AUDIT_LAUNCH_INTERVAL.as_secs(); | ||
| (launchable + config::FIRST_AUDIT_BUDGET_BURST as u64) as usize |
dirvine
left a comment
There was a problem hiding this comment.
Reviewed exact head d1fc1ddf0a4f710e16debd471cd3ec4c3916e92f with a six-seat panel (four substantive completions; two tool-failed), then independently checked the scheduler ordering and focused tests.
Verdict: changes requested. The queue-sizing diagnosis is sound, but the current keep-newest cap introduces a deterministic suppression route before the scheduler can reserve work.
Blocking — drain-before-reserve can evict a target deterministically
FIRST_AUDIT_PENDING_CAP is 32 (src/replication/mod.rs:956-984), while FIRST_AUDIT_DRAIN_BATCH is 64 (src/replication/config.rs:399-405). On an ingress wake the event loop enqueues/drains up to 64 nominations first (src/replication/mod.rs:1621-1640) and only then attempts reservation (:1736-1758). At capacity, every new distinct peer deterministically evicts the LRU peer (:199-227, :619-632).
Therefore an ordered batch of target + 32 later distinct-peer nominations removes the target before either newest/oldest lane can inspect it, even when a token and in-flight slot are available. Lane alternation cannot help because the target is already gone.
This cuts the stacked ADR's documented capacity-eviction threshold from 4,096 nominations (docs/adr/ADR-0004-commitment-bound-quote-pricing.md:462-470) to 32 — a 128x reduction — while Amendment 4 says every fresh pin retains an “unpredictable chance” of prompt audit (:527-533). The current replacement is deterministic under controlled arrival order. That matters because the ADR already assumes recyclable payment principal and sidecar-only pins without a lottery backstop (:443-453).
Please either give eligible work a reservation opportunity before destructive overflow, or use genuine adversary-resistant/random/weighted retention rather than deterministic keep-newest LRU. Add a production-order regression with pending_cap < ordered_batch <= drain_batch; the new enqueue-only test currently proves that the oldest target is evicted but does not exercise the drain/reserve boundary.
Also fix in this head
- Cap maths/wording: the strict predicate rejects
age >= 150m(src/replication/mod.rs:920-928). For a fresh quote, burst 2 plus refills at 5…145 minutes gives 31 usable launches, not 32; the max-jitter/slack prefilter is stricter again. The test at:6590-6604repeats the formula rather than simulating the predicate. Derive the cap from the same strict launch predicate/horizon. - Occupancy telemetry: a reservation is held outside
pending(:477-487,:721-728), so the scheduler may hold one reserved + 32 pending while reportingpending=32, pending_cap=32. Report reserved/total schedulable occupancy or narrow the claim. - Stack dependency: #183 must remain after #177. The merge-order comment names stale cap SHA
cbc2f5e; the current cap commit is this head,d1fc1dd.
Verification
cargo fmt --all -- --check— passcargo test first_audit_ --lib --no-fail-fast— 28/28 pass- GitHub CI — all current checks green across Linux/macOS/Windows, clippy, format, docs, no-logging, audit and ADR validation
- Head revalidated immediately before review
The causal note on DEV-02/DEV-03 is fair: that comparison cannot attribute the upload p90 change. I also agree another attribution run is not necessary for this queue finding; the scheduler counters already show the admission/budget mismatch. This review is about the proposed retention policy, not the diagnosis.
d1fc1dd to
3462edf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/payment/verifier.rs:1165
- The settlement-redirect check runs before verifying the on-chain amount is sufficient. For an unpaid quote hash, the vault mapping will usually return a zeroed rewards-address prefix, which will be treated as a “redirected settlement” and yields a misleading error.
Consider applying the redirect check only after on_chain_amount >= candidate.expected_amount so “not paid enough” remains the failure mode when no sufficient settlement exists.
if let Some(recorded) = recorded_rewards_prefix {
let expected = Self::rewards_address_prefix(&candidate.quote.rewards_address);
if recorded != expected {
return Err(Error::Payment(format!(
"Median-priced quote settlement for peer {:?} was redirected: recorded rewards address prefix {} does not match the quote's {}",
| pub(crate) fn record_audit_fail(kind: AuditOutcomeKind, reason: &AuditFailureReason) { | ||
| AUDIT_FAIL[kind as usize * N_FAIL_REASONS + fail_reason_index(reason)] | ||
| .fetch_add(1, Ordering::Relaxed); | ||
| } | ||
|
|
||
| /// Record a responder-side challenge dropped at the admission caps. | ||
| pub(crate) fn record_audit_drop(kind: AuditDropKind) { | ||
| AUDIT_DROPPED[kind as usize].fetch_add(1, Ordering::Relaxed); | ||
| } | ||
|
|
||
| /// Emit the cumulative audit outcome tallies as one INFO summary line, same | ||
| /// target and cadence as [`log_traffic_summary`] (`group = 4`), so the | ||
| /// telegraf→Elasticsearch pipeline lifts each key into a `tail.*` field and | ||
| /// pass/fail/drop rates per hour fall out of max-per-hour deltas. | ||
| pub(crate) fn log_audit_outcome_summary() { | ||
| let pass = |k: usize| AUDIT_PASS[k].load(Ordering::Relaxed); | ||
| let fail = |k: usize, r: usize| AUDIT_FAIL[k * N_FAIL_REASONS + r].load(Ordering::Relaxed); | ||
| let drop = |k: usize| AUDIT_DROPPED[k].load(Ordering::Relaxed); |
| /// ADR-0004: a sender the payment verifier uses to surface monetized pins | ||
| /// (commitments that backed a payment) for a deterministic first audit. | ||
| /// Cloneable; the engine drains the matching receiver. | ||
| /// (commitments that backed a payment) for a first audit. Cloneable; the | ||
| /// engine drains the matching receiver. Bounded: senders must `try_send` | ||
| /// and treat a full queue as a benign drop (Amendment 2 best-effort). | ||
| #[must_use] | ||
| pub fn monetized_pin_sender(&self) -> mpsc::UnboundedSender<MonetizedPinEvent> { | ||
| pub fn monetized_pin_sender(&self) -> mpsc::Sender<MonetizedPinEvent> { |
dirvine
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 3462edf3c4f183f47c7dc83f68a1fb46fb3f2c3a with the six-seat follow-up panel and local focused checks.
The prior production blockers are addressed:
- overflow uses a random incumbent rather than deterministic LRU eviction;
- pending work gets a reservation opportunity before destructive overflow;
- strict launch-horizon maths now yields 31 and is checked by predicate simulation;
- telemetry exposes
pending,pending_cap, and the separate reservation.
One merge blocker remains: first_audit_suppressed_lower_flood_leaves_recency_and_counts still asserts deterministic LRU eviction after the production policy changed to random victim selection (src/replication/mod.rs:7284-7304). Windows CI failed on that exact stale assertion (740 passed, 1 failed). I reproduced it locally on run 5/100. Please make the test verify retained value/count and unchanged recency directly, without requiring a particular random victim, then rerun CI.
Panel dissent, non-blocking: the ADR says random displacement occurs only “when no launch is possible”, but the scheduler permits only one outstanding jitter reservation even though burst/in-flight capacity is two. After the first reservation, a second token/slot may remain while later arrivals use random eviction. The single-reservation design is deliberate and the eviction is now adversary-resistant, so I do not classify this as a production blocker; please narrow the wording to “when no reservation slot is available” (or model multiple reservations if the stronger statement is intended).
Verified locally:
cargo test first_audit_ --lib --no-fail-fast— 30/30 pass in one run- repeated stale test — reproduced failure
cargo fmt --all -- --check— pass
Hold only for the stale test and fresh green CI. No merge performed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/replication/protocol.rs:248
N_FAIL_REASONSis hard-coded to 5 even though it’s intended to track the number ofAuditFailureReasonvariants. If a new failure reason is added andfail_reason_indexis updated butN_FAIL_REASONSis not, indexing intoAUDIT_FAILcan go out of bounds at runtime. Consider deriving these sizes from the enums to keep the arrays and indexing in sync automatically.
/// One slot per [`AuditFailureReason`] variant, per [`AuditOutcomeKind`].
const N_FAIL_REASONS: usize = 5;
const N_OUTCOME_KINDS: usize = 2;
const N_DROP_KINDS: usize = 3;
The pending queue shared the 4096-entry commitment-cache cap while the token bucket can launch at most ~32 audits inside one effective answerability window, so under production-scale arrival rates (staging 2026-07-17: ~113 admitted nominations per node-hour against ~12 launches) over 99% of admitted work could never launch. The queue became an aging backlog whose entries mostly expired, and the pending/oldest-age telemetry measured the backlog instead of schedulable work. pending is now capped at FIRST_AUDIT_PENDING_CAP, derived from the budget and window constants with compile-time guardrails. At capacity the LRU displaces the least-recently-refreshed entry, counted as capacity_evicted; keep-newest displacement preserves the newest-first lane's prompt-audit deterrence under flood, and displacement stamps no suppression state. The summary line reports pending_cap so occupancy is a dashboard quantity. ADR-0004 Amendment 4 documents the sampling coverage semantics.
…n for the pending cap Review findings on the admission cap: - Deterministic keep-newest LRU displacement let an ordered batch of cap distinct-peer nominations flush a chosen target inside one 64-event ingress drain, before either launch lane could see it, cutting the documented suppression cost from 4096 nominations to 32. Overflow now displaces a uniformly random incumbent (per-nomination eviction probability 1/cap, never certain), and the drainer grants pending work a reservation opportunity before any destructive overflow, so a burst can never flush eligible work past a ready token. - The cap derivation over-counted by one: the strict reserve predicate (answerable through max jitter plus send slack) permits 31 usable launches per window, not 32. The unit test now simulates the shipped predicate instant-by-instant instead of repeating the formula. - The reservation is held outside pending, so the summary now reports reserved alongside pending/pending_cap; schedulable occupancy is their sum. Regressions: ordered-flood non-determinism (100-trial statistical bounds with miss odds below 1e-20), reserve-before-eviction at capacity, and displaced-peer re-nomination under random retention.
…viction RNG The suppressed-lower flood test proved recency was untouched by asserting WHICH peer a third arrival evicted, which was deterministic under LRU eviction but a coin flip under random-victim displacement (Windows CI caught the ~50% failure). It now asserts recency directly: the queue's MRU-to-LRU iteration order (the documented lru contract the reserve lanes consume) is snapshotted before the flood and must be unchanged after it, independent of overflow policy. The scheduler now owns a StdRng (OS-entropy seeded in production) and coalesce_first_audit_event takes it as a parameter, so eviction sequences are seedable: the 100-trial ordered-flood regression runs on fixed per-trial seeds and is exactly reproducible with no flake budget. Wording precision from review: the pre-overflow reservation is a single one-entry guard slot (occupancy at most cap+1, reported as pending + reserved), and random displacement applies to any otherwise-admissible arrival that still finds the queue full after that one opportunity -- not 'when no launch is possible', since a second burst token can remain while the jitter reservation is occupied. Swept stale 'bounded LRU' phrasing.
75f4fb4 to
2d1527f
Compare
| if let Some(victim_peer) = victim { | ||
| if let Some(evicted) = pending.pop(&victim_peer) { | ||
| let _ = pending.push(incoming.peer, incoming); | ||
| return FirstAuditQueueOutcome::CapacityEvicted { | ||
| peer: victim_peer, | ||
| pin: evicted.pin, | ||
| }; | ||
| } | ||
| } |
Summary
Follow-up to #177 (stacked on its head; see merge-order note below). Sizes first-audit pending admission to what the launch budget can actually audit.
#177 bounded launches (token bucket) and expired dead entries eagerly, but admission was still unbounded relative to the budget:
pendingshared the 4096-entry commitment-cache cap while the token bucket can launch at most ~31 audits inside one effective answerability window. On the 2026-07-17 DEV-02 staging run this meant ~113 admitted nominations per node-hour against ~12 launches: over 99% of admitted work could never launch, pending sat at ~331-431k fleet-wide, and the oldest-pending age tracked the age of the deployment itself — an aging backlog whose steady state is mass expiry.Changes
pendingis capped atFIRST_AUDIT_PENDING_CAP, derived from the strict reserve-time launch horizon (burst at age zero plus every refill that stays answerable through max jitter + send slack; currently 31). The unit test simulates the shipped predicate instant-by-instant and must agree with the derivation. The dedup set keeps the 4096 bound.capacity_evicted. Per-nomination eviction probability of a specific target is 1/cap regardless of arrival order: suppressing it with 95% confidence costs ~95 distinct-peer paid nominations and is never certain (deterministic keep-newest LRU, flagged in review, allowed a certain flush with 32).pending_capandreserved(the single reservation held outside the queue), so schedulable occupancypending + reservedis directly computable in Elasticsearch.Evidence
pending_cap < 60 <= FIRST_AUDIT_DRAIN_BATCH, 100-trial statistical bounds with miss odds < 1e-20), reserve-before-eviction at capacity, displaced-peer re-nomination, cap-vs-predicate simulation.pending≤ 31 witholdest_pending_quote_age_msfar below the answerability window (queue drains instead of aging), overflow visible ascapacity_evictedgrowth at admission time instead ofoutside_answerability_windowmass expiry hours later.Semver
Patch: no protocol or config-surface change; two additive fields in the summary log line.
Tracking: V2-738