maintainer: 'Maintenance took too long' WARN fires structurally — 10s threshold is below the chunk-pause floor, and its advice is inverted

[tomerweller]

Summary

henyey_app::maintainer logs WARN Maintenance took too long; consider increasing AUTOMATIC_MAINTENANCE_COUNT or performing manual database maintenance on the mainnet validator, with elapsed_ms steady in the 20–28 s range against a hardcoded 10 s threshold. It has fired 76 times on the current process (uptime 25d 19h).

The warning is a false positive introduced by the chunked-delete fix, and its remediation advice is backwards — following it makes elapsed_ms larger, not smaller.

No operational impact: the whole cycle runs off the ledger-close path in spawn_blocking_logged, the node has stayed Validating throughout with no ledger gaps, and RPC pruning is tracking normally (gap 466 vs bound 1580).

Evidence

Recent occurrences on the live validator (build 0ac84d42):

2026-07-24T19:21:26Z  elapsed_ms=21295
2026-07-24T21:06:26Z  elapsed_ms=21170
2026-07-25T15:21:27Z  elapsed_ms=21589
2026-07-27T15:21:29Z  elapsed_ms=23881
2026-07-27T16:21:31Z  elapsed_ms=25777

Historical range on this process: 20 379 – 67 219 ms. 76 WARN lines total.

Config in configs/validator-mainnet-rpc.toml:

[rpc]
retention_window = 360

[maintenance]
enabled = true
period_secs = 900
count = 50000

Root cause

Two constants are in direct conflict.

1. The 10 s threshold (crates/app/src/maintainer.rs:220 and the duplicate at :269):

let elapsed = start.elapsed();
if elapsed > Duration::from_secs(10) {
    warn!(elapsed_ms = elapsed.as_millis(),
          "Maintenance took too long; consider increasing AUTOMATIC_MAINTENANCE_COUNT \
           or performing manual database maintenance");
}

2. The deliberate per-chunk sleep (crates/app/src/maintainer.rs:343-348), added to stop maintenance from holding the WAL write lock and stalling ledger closes:

const MAINTENANCE_CHUNK_LEDGERS: u32 = 2;
const MAINTENANCE_CHUNK_PAUSE: Duration = Duration::from_millis(25);

delete_in_chunks sleeps MAINTENANCE_CHUNK_PAUSE between every chunk, and elapsed is measured around the whole of run_maintenance — so the sleeps are counted.

run_maintenance calls delete_in_chunks for five tables (maintainer.rs:469, 477, 481, 487, 491): SCP, ledger header, tx history, event, ledger close meta.

At period_secs = 900 and ~5 s/ledger, roughly 180 ledgers accrue per cycle, so each table walks ~180/2 = 90 chunks:

5 tables x 90 chunks x 25 ms = ~11.25 s of pure sleep

The structural sleep floor alone (~11 s) already exceeds the 10 s threshold, before a single row is deleted. The remaining ~10–17 s is real delete work against a 158 GB mainnet.db. So this warning cannot not fire in steady state on an RPC-enabled validator.

The advice is inverted

The message says "consider increasing AUTOMATIC_MAINTENANCE_COUNT". count is the per-table ledger budget passed to delete_in_chunks(count, ...). Raising it can only allow more chunks — hence more 25 ms sleeps and a larger elapsed_ms.

It is also already irrelevant here: count = 50000 is ~278x the ~180 ledgers/period actually produced. The loop exits early on Ok(0), so the budget is never the binding constraint — the sleep count is. An operator who follows the advice changes nothing at best.

Proposed fix

Pick one (a) and optionally (b):

(a) Make the threshold aware of the chunk floor. Either

• derive it from the design (expected_chunks * MAINTENANCE_CHUNK_PAUSE * safety_factor), or
• express it as a fraction of the maintenance period (e.g. warn at period / 3 = 300 s at period_secs=900) — the operationally meaningful question is "is maintenance at risk of not keeping up with its own period?", which 25 s clearly is not.

A flat 10 s predates the chunking change and no longer describes anything.

(b) Fix the remediation text. Increasing count is the wrong lever. If a cycle is genuinely slow the useful knobs are MAINTENANCE_CHUNK_LEDGERS (more work per lock acquisition) or MAINTENANCE_CHUNK_PAUSE (less idle time) — both of which trade against the WAL-lock-hold regression the chunking was added to fix.

Also worth folding the two duplicated threshold sites (:220, :269) into one named constant so they can't drift.

Not doing

No node code committed by the monitor. No validator impact; deploys remain held per #3702.

---

Filed by /monitor-tick 2801 at L63674835, build 0ac84d42.

[tomerweller]

Triage Report

Classification: bug, crates/app (maintainer/DB-maintenance logging), low severity — no consensus/ledger impact, confirmed cosmetic/observability-only.

Root cause verified against issue body: the 10s WARN threshold in crates/app/src/maintainer.rs:220,269 predates the chunked-delete fix that added a deliberate MAINTENANCE_CHUNK_PAUSE (25ms) sleep between chunks across 5 tables. At period_secs=900 (~180 ledgers/cycle, ~90 chunks/table), the structural sleep floor alone is ~11.25s — already exceeding the 10s threshold before any row deletion. The WARN is therefore guaranteed to fire in steady state on any RPC-enabled validator; it is not a real slowness signal.

Compounding defect — remediation text is inverted: the message advises increasing AUTOMATIC_MAINTENANCE_COUNT, but that only permits more chunks (more 25ms sleeps), which increases elapsed_ms. It's also moot here: count=50000 is ~278x the actual ~180 ledgers/period, so the loop already exits early on Ok(0) — count was never the binding constraint.

No consensus/ledger impact confirmed: the whole maintenance cycle runs off the ledger-close path via spawn_blocking_logged; node stayed Validating throughout with no gaps; RPC pruning tracking normally (gap 466 vs bound 1580).

Scope: NOT self-modifying pipeline tooling — this is a genuine crates/app production-code fix (threshold constant + log message text, two call sites at maintainer.rs:220/269 that should be consolidated into one named constant). No concurrent operator PR found (checked 3754 in:body). Proceeding through the standard triage→plan→do→review-pr pipeline.

Proposed fix (from issue body): either derive the threshold from the chunking design (expected_chunks * MAINTENANCE_CHUNK_PAUSE * safety_factor) or express it as a fraction of period_secs (e.g. period/3), and fix the remediation text to point at MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE instead of count. Consolidate the duplicated threshold at the two call sites.

[tomerweller]

Triage Report

Verdict: ACCEPT

Summary: henyey_app::maintainer's "Maintenance took too long" WARN (10s threshold, crates/app/src/maintainer.rs:220,269) fires structurally on any RPC-enabled validator because it predates the chunked-delete fix's deliberate MAINTENANCE_CHUNK_PAUSE sleep floor (~11.25s across 5 tables at typical period/ledger-rate), and its remediation advice ("increase AUTOMATIC_MAINTENANCE_COUNT") is inverted — raising count only permits more chunks/sleeps, increasing elapsed_ms, not decreasing it. No consensus/ledger impact (runs off ledger-close path via spawn_blocking_logged; validator stayed Validating throughout).

Kind: bug-fix

Severity: low

Crate: crate:app

Full analysis in the preceding comment. No concurrent operator PR found (3754 in:body). Not self-modifying pipeline tooling — proceeding through standard plan/do/review-pr pipeline.

[tomerweller]

📝 Plan Draft (Round 1)

Summary: Replace the flat, pre-chunking Duration::from_secs(10) "maintenance took too long" threshold with one derived from the configured maintenance period, consolidate the duplicated check into one named helper, and fix the remediation text to name the actual tuning knobs (MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE) instead of the irrelevant AUTOMATIC_MAINTENANCE_COUNT.

Files to modify:

• crates/app/src/maintainer.rs — add fn maintenance_warn_threshold(period: Duration) -> Duration, use it at both existing call sites (Maintainer::start, ~line 219-228, and Maintainer::perform_maintenance, ~line 268-277), replacing the duplicated Duration::from_secs(10) + warn text; add unit tests for the new helper in the existing #[cfg(test)] mod tests block.

Approach:

Root cause (confirmed by reading crates/app/src/maintainer.rs on origin/main): both warn sites compare cycle elapsed against a flat Duration::from_secs(10) (lines 220 and 269). This predates MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE (lines 343/348), added for #3702 to stop delete_in_chunks (line 357) from holding the WAL write lock across a whole table's deletion. delete_in_chunks sleeps MAINTENANCE_CHUNK_PAUSE (25ms) between every chunk of at most MAINTENANCE_CHUNK_LEDGERS (2) ledgers, across up to 5 tables in run_maintenance (SCP, ledger headers, tx history, and — when RPC retention is configured — events and ledger_close_meta). At the mainnet-rpc config's period_secs=900 (~180 ledgers/cycle at ~5s/ledger), that's ~90 chunks/table × 5 tables × 25ms ≈ 11.25s of pure sleep — already over the flat 10s threshold before any row is deleted, so the WARN is structurally guaranteed to fire on every cycle in steady state. The remediation text ("increase AUTOMATIC_MAINTENANCE_COUNT") is also backwards: count only bounds the per-table chunk budget; raising it permits more chunks (more sleeps), increasing elapsed_ms, and is moot here since count=50000 is ~278x the ~180 ledgers/period actually produced (the loop already exits early on Ok(0), so budget was never the binding constraint).

Add a single small pure helper next to the other maintenance constants:

/// Warn threshold for a maintenance cycle's elapsed time.
///
/// A flat threshold predates the per-chunk WAL-lock-yielding pauses added
/// for #3702 (`MAINTENANCE_CHUNK_PAUSE` between chunks, across up to 5
/// tables in `run_maintenance`), whose structural floor alone can exceed a
/// few seconds on short periods — firing this warning in steady state
/// rather than signaling genuine slowness. Instead, warn only when a cycle
/// risks not keeping up with its own configured period: a cycle longer than
/// a third of the period is the operationally meaningful risk, independent
/// of how many tables/chunks the current build happens to have.
fn maintenance_warn_threshold(period: Duration) -> Duration {
    period / 3
}

Replace both if elapsed > Duration::from_secs(10) checks with if elapsed > maintenance_warn_threshold(self.config.period), and update the warn message at both sites to:

"Maintenance cycle exceeded its expected-relative-to-period threshold; if \
 consistently slow, consider tuning MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE \
 or performing manual database maintenance"

(keep elapsed_ms as a structured field on the event, unchanged). At period_secs=900 this yields a 300s threshold — comfortably above the observed 20-28s (and the process's historical max of 67s) — while still catching a cycle that's genuinely at risk of not keeping up with its own period. At the crate's DEFAULT_MAINTENANCE_PERIOD (4h), it yields 4800s (80min), which is intentionally loose since the default period itself is loose; this is a false-positive-elimination fix, not a new fine-grained slowness detector.

Both call sites (start()'s periodic loop and perform_maintenance()'s manual/administrative path) share the same self.config.period, so a single helper taking period as a parameter (rather than being a method) keeps the two sites from drifting again and is trivially unit-testable without constructing a Maintainer.

Kind: bug-fix

Test plan:

• Regression tests (crates/app/src/maintainer.rs, in the existing mod tests):
  • test_maintenance_warn_threshold_scales_with_period: asserts maintenance_warn_threshold(Duration::from_secs(900)) == Duration::from_secs(300), matching the mainnet-rpc config's observed period_secs=900 from the issue. This is a new function, so it cannot be run against origin/main verbatim (the function doesn't exist there yet); the equivalent "would fail on main" claim is: on origin/main today the threshold used at both call sites is the flat Duration::from_secs(10), so a maintenance cycle in the 20-28s range observed in the issue (and up to the process's historical max of 67s) would trigger the WARN on every cycle — reproducing the false positive. After this change, elapsed (20-67s) stays under the new 300s threshold for a period_secs=900 config, so the WARN no longer fires for that traffic pattern.
  • test_maintenance_warn_threshold_uses_default_period: asserts maintenance_warn_threshold(DEFAULT_MAINTENANCE_PERIOD) == DEFAULT_MAINTENANCE_PERIOD / 3, guarding the formula for the crate's default 4-hour period.
• Existing tests preserved (unaffected by this change — verified by reading each, none reference the warn threshold or its message): test_maintenance_first_tick_is_delayed, test_bare_interval_first_tick_is_immediate, test_maintenance_config_default, test_maintenance_config_disabled, test_checkpoint_frequency, test_maintainer_creation, test_maintainer_disabled, test_perform_maintenance_empty_db, test_lmin_calculation, test_perform_maintenance_with_count_uses_correct_thresholds, test_delete_in_chunks_bounds_each_call_and_exhausts, test_delete_in_chunks_respects_total_budget, test_delete_in_chunks_stops_on_error, and the remaining test_headers_*/test_tx_history_*/test_events_*/eviction tests (none touch the warn path).

Parity considerations: n/a — non-parity path. This is henyey-internal operational logging/observability (database maintenance scheduling); stellar-core has no equivalent chunked-delete-with-pause maintenance scheduler to compare against, and no consensus/ledger/wire-protocol behavior is touched.

Risks:

• The exact divisor (/3) is a judgment call, not derived from a first-principles safety margin against MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE/table-count directly. I chose it because it matches the example the issue itself proposes (period/3 = 300s at period_secs=900) and is verifiably well above all observed elapsed times on the live process. A chunk-floor-derived formula (tables × ceil(ledgers_per_period / MAINTENANCE_CHUNK_LEDGERS) × MAINTENANCE_CHUNK_PAUSE × safety_factor) was considered but rejected as needlessly coupling the threshold to internal constants that may change independently (table count varies with RPC config; chunk size/pause may be retuned for other reasons) — the period-based check answers the operationally meaningful question directly and is simpler to reason about and test.
• This changes an operator-visible log line's wording; not a behavior change to maintenance itself (no change to run_maintenance, delete_in_chunks, or any deletion threshold/query).

[tomerweller]

Plan: Blocked — Agent tool dispatch failure (environment-level, not plan-specific)

All three round-1 critics failed to launch, both on first attempt and on retry (per the /plan failure-handling policy: retry once, then treat as failure). The error returned for every attempt, regardless of specified model (opus, unspecified/inherit, and even a trivial no-tool diagnostic prompt with sonnet explicitly):

There's an issue with the selected model (gpt-5.4). It may not exist or you may not have access to it. Run /model to pick a different model.

This is not a per-critic or per-plan issue — a bare diagnostic agent with no tool use and an explicit model: sonnet parameter failed identically. This indicates the session's Agent-tool dispatch is globally pinned to an invalid gpt-5.4 model reference that the per-call model parameter cannot override, and requires a /model reset by the operator (not something invocable via my available tools).

Round 1 plan draft is posted above (## 📝 Plan Draft (Round 1)) and is unreviewed — do not treat it as converged. Once Agent-tool dispatch is restored, this issue should be picked back up at Step 3 (spawn the three critics) of /plan, or an operator/human can review the round-1 draft directly and either approve manually or request revisions.

Moving to blocked with this comment. Unassigning. This same failure will block any future /plan, /do, or /review-pr sub-agent dispatch for the rest of this loop session — flagging prominently in the tick report so the board-watcher does not keep re-attempting sub-agent-dependent pipeline stages until the environment issue is resolved.

[tomerweller]

🔍 Critic C (scope) — Round 1

Verdict: REVISE-MINOR

Key concerns:

• Scope is right in shape: single file (crates/app/src/maintainer.rs), one small pure helper (maintenance_warn_threshold(period) -> Duration), swapped into both existing call sites (start() L220, perform_maintenance() L269 — verified both still use the raw Duration::from_secs(10) on origin/main), corrected remediation text, 2 new unit tests. Nothing here touches run_maintenance, delete_in_chunks, or the chunk constants themselves — no scope creep into unrelated maintenance logic.
• period/3 is not "arbitrary" in the same sense as the flat 10s it replaces: the sleep floor is tables × ceil(ledgers_per_period / MAINTENANCE_CHUNK_LEDGERS) × MAINTENANCE_CHUNK_PAUSE, and since ledgers_per_period ≈ period_secs/5, the floor is linear in period_secs — same as the new threshold. So floor/threshold is a constant ratio (≈3.75% at 5 tables, current constants) independent of period_secs, which is exactly the axis the old bug broke on (period-independent constant vs. period-scaling reality). That's a materially different, more robust failure mode than the bug being fixed, not a recurrence of it.
• However, that ratio does still depend on table count (3 vs 5, RPC-gated) and MAINTENANCE_CHUNK_PAUSE/MAINTENANCE_CHUNK_LEDGERS, none of which the formula references. The plan's own "Risks" section acknowledges this trade-off and explains why it deliberately avoided coupling to those internals (to not re-introduce a different drift risk) — a defensible engineering call for a low-severity, non-consensus logging fix — but right now that margin (~25x headroom before the floor could approach period/3) is only established by hand-computation in a GitHub comment, not by any test artifact in the repo.
• Suggested minor addition (still single-file, no scope expansion): a third unit test that computes the theoretical sleep floor from the actual current constants (MAINTENANCE_CHUNK_LEDGERS, MAINTENANCE_CHUNK_PAUSE, worst-case 5 tables, the same period/5 ledger-rate assumption already used in start()) and asserts it stays comfortably under maintenance_warn_threshold(period) for a couple of representative periods (e.g. 900s, DEFAULT_MAINTENANCE_PERIOD). That converts the plan's hand-verified safety margin into a guardrail that fails CI if MAINTENANCE_CHUNK_PAUSE/table count are ever retuned enough to threaten the same bug class again — closing the loop on concern (1) without touching any additional files.
• No overreach found: nothing here rewrites delete_in_chunks/run_maintenance, no unrelated maintenance code touched, no formatting-only edits smuggled in.
• Confirmed single-file/single-PR sized: everything is additive to crates/app/src/maintainer.rs (new fn + 2-3 tests + 2 call-site swaps + message text), consistent with the bug's low severity/no-consensus-impact classification from triage.

Full review

Verified against origin/main (crates/app/src/maintainer.rs):

• Both call sites (Maintainer::start, L220; Maintainer::perform_maintenance, L269) still use Duration::from_secs(10) verbatim with the identical warn message referencing AUTOMATIC_MAINTENANCE_COUNT.
• MAINTENANCE_CHUNK_LEDGERS = 2, MAINTENANCE_CHUNK_PAUSE = 25ms (L343, L348).
• run_maintenance always prunes 3 tables (SCP, ledger header, tx history) and conditionally 2 more (events, ledger close meta) when rpc_retention_window is Some — confirming the issue's "3 vs 5 tables" claim.

Question 1 (too narrow?): No. period/3 is not equivalent-in-kind to the flat 10s constant it replaces — the flat constant was period-independent while the real floor scales with period; period/3 scales with period too, so the ratio between them is period-invariant. The plan explicitly considered and rejected the chunk-floor-derived formula, giving a sound rationale (avoiding coupling the threshold to internals like table count/chunk pause that can change for unrelated reasons, e.g. future WAL-contention tuning). This is the same trade-off the issue itself offers as an equally-acceptable option ("(a) ... or express it as a fraction of the maintenance period"). The one gap: the safety margin between the heuristic and the actual structural floor is currently established only by arithmetic in prose, not asserted anywhere in code/tests — recommend one additional test tying them together (see above), non-blocking for merge but worth adding in this same PR since it's still confined to the same file's test module.

Question 2 (too broad?): No. The diff surface described (one helper fn, two call-site swaps, message text, tests) stays entirely within crates/app/src/maintainer.rs and does not modify delete_in_chunks, run_maintenance, the chunk constants, or any other maintenance code path.

Question 3 (single-file/single-PR sized?): Yes, correctly identified as such. Matches the triage's "low severity, cosmetic/observability-only, no consensus/ledger impact" classification — proportionate scope for the bug.

[tomerweller]

🔍 Critic B (parity) — Round 1

Verdict: REVISE-MINOR

Key concerns:

• The plan's parity claim ("stellar-core has no equivalent... to compare against") is an overstatement. Stellar-core did have a directly analogous "maintenance took too long" warning — it just isn't in the currently pinned version because the whole feature was deliberately removed.
• The removed mechanism's design (flat, small, absolute threshold) is materially different from what the plan proposes (period-scaled period/3), which is useful context the plan is missing rather than a blocking divergence.
• Bottom-line conclusion (no consensus/ledger/wire impact, no specific value to mirror) is still correct — just for a more specific reason than the plan states.

Full review

What I checked: stellar-core/ submodule (pinned v27.0.0) — src/main/Maintainer.{h,cpp}, src/main/Config.cpp (AUTOMATIC_MAINTENANCE_*), src/util/LogSlowExecution.{h,cpp}, and full git history of Maintainer.cpp/Maintainer.h back to their introduction.

Current pinned version (v27.0.0): confirmed no equivalent exists.
AUTOMATIC_MAINTENANCE_PERIOD/AUTOMATIC_MAINTENANCE_COUNT are now deprecated no-ops in Config.cpp ("... is deprecated and ignored. Please remove this from config"). There is no Maintainer.cpp/.h anywhere in the tree, and grepping all of src/ for "maintenance"/"Maintenance" turns up nothing outside unrelated Rust Soroban benchmark files. So for the pinned parity target, the plan's practical conclusion — there's no current stellar-core value/formula for this warning to mirror — is correct.

But this is a removal, not an absence. git log --all on stellar-core/ shows the feature existed for years and was explicitly deleted in commit dc44d5bd6 ("Remove maintenance (#5157)", landed between v25.1.1 and v26.0.0, i.e. before the currently pinned v27.0.0): "Gets rid of automatic maintenance, the ledgerheaders table, and lastclosedledger in storestate." Before that removal, Maintainer::performMaintenance looked like this (last version, dc44d5bd6~1:src/main/Maintainer.cpp):

auto logSlow = LogSlowExecution(
    "Performing maintenance", LogSlowExecution::Mode::AUTOMATIC_RAII,
    "performance issue: check database or perform a large manual "
    "maintenance followed by database maintenance. Maintenance took",
    std::chrono::seconds{2});

LogSlowExecution (src/util/LogSlowExecution.cpp) is a RAII wall-clock timer around the whole maintenance pass — this is a direct ancestor of henyey's "maintenance took too long" WARN, not an unrelated henyey-only concept. Its logic, though, differs meaningfully from both henyey's existing 10s check and the plan's proposed period/3:

• It logs at DEBUG once elapsed > threshold (2s), and only escalates to WARNING once elapsed > threshold * 10 (20s).
• The threshold is a small fixed absolute value (2s/20s), not scaled to AUTOMATIC_MAINTENANCE_PERIOD at all.
• Log emission is rate-limited to once/second process-wide (across all LogSlowExecution call sites), which is a different anti-noise mechanism than a single elevated threshold.

Separately, stellar-core also had (and henyey's maintainer.rs already mirrors, in the code untouched by this issue) a different, config-validation-time check: commit bc3adf72d ("warn if maintenance is set too low to keep up with network") compares AUTOMATIC_MAINTENANCE_COUNT against expected ledgers-per-period at startup and warns "Maintenance may not be able to keep up: AUTOMATIC_MAINTENANCE_COUNT={} <= {}" if the budget can't keep pace. Henyey's current code already has an equivalent warning ("Maintenance may not keep up: count <= ledgers produced per period"). This is not the warning under review in #3754 (which is the wall-clock elapsed-time check), but it's worth noting the plan's "no equivalent" framing doesn't quite fit this history either — henyey has, in fact, already mirrored the pre-removal stellar-core design in this one respect.

On chunking specifically: the plan's claim that stellar-core has no chunked-delete-with-pause pacing is accurate even in the pre-removal design — the old performMaintenance ran HerderPersistence::deleteOldEntries and LedgerHeaderUtils::deleteOldEntries as single bulk calls (with header deletion optionally offloaded to a background thread under parallel ledger close), with no MAINTENANCE_CHUNK_LEDGERS/pause pacing loop of any kind. So the WAL-lock-driven chunking that created this issue's structural sleep floor is genuinely henyey-specific (added for #3702) and has no stellar-core precedent either way — nothing to be inconsistent with there.

Net assessment: the plan's fix (period-scaled threshold, corrected remediation text, consolidated helper) doesn't conflict with any current-version stellar-core behavior, and no consensus/ledger/wire-protocol determinism is touched — so this isn't a functional parity violation. But the plan should not assert blanket "stellar-core has no equivalent... to compare against" as if the concept never existed; the accurate framing is "stellar-core had a comparable slow-maintenance warning (LogSlowExecution, 2s/20s absolute thresholds, DEBUG→WARNING escalation) but removed automatic maintenance entirely in dc44d5bd6 before the pinned v27.0.0, and its absolute-threshold design predates and differs from the period-scaled approach proposed here, so there is no current mechanism to mirror and no reason the old design should be copied verbatim." Recommend updating the plan's "Parity considerations" section to this more accurate statement before landing; no change to the proposed code fix is required.

[tomerweller]

🔍 Critic A (correctness) — Round 1

Verdict: REVISE-MINOR

Key concerns:

• Everything the plan asserts about origin/main:crates/app/src/maintainer.rs checks out exactly, down to the cited line numbers (220/269 flat Duration::from_secs(10); 343/348 MAINTENANCE_CHUNK_LEDGERS=2/MAINTENANCE_CHUNK_PAUSE=25ms; 357 delete_in_chunks; 469/477/481/487/491 the 5 chunked tables) — I independently re-derived the 11.25s structural-floor math and it's internally consistent (90 chunks/table × 25ms × 5 tables = 2.25s×5 = 11.25s), and it matches the code's own ledgers_per_period = period_secs/5 assumption already used at line 185.
• AUTOMATIC_MAINTENANCE_COUNT really is a real config knob (config.rs/compat_config.rs, maps to MaintenanceConfig.count), and it really is non-binding here: delete_in_chunks breaks on Ok(0) (line 371) before exhausting count, so raising it can't reduce elapsed_ms in the observed scenario; the plan's "inverted advice" framing is fair.
• Gap not addressed by the plan: Maintainer::start() guards self.config.period.is_zero() before ever entering the loop, but Maintainer::perform_maintenance() (the second call site the plan patches) has no such guard — it's a public method callable directly with any MaintenanceConfig. Under a zero-period config, maintenance_warn_threshold(Duration::ZERO) == Duration::ZERO, so the WARN would fire unconditionally on any positive elapsed, reproducing the exact false-positive bug this fix removes, for a config value the type system doesn't reject. More generally, for any sufficiently small (but nonzero, hence "valid" per start()'s own check) period, period/3 can again fall below the real per-cycle sleep/query floor. The plan should either clamp the threshold with a minimum (e.g. period.max(X)/3, or period.checked_div(3).unwrap_or(MIN)) or explicitly test/document behavior at period = Duration::ZERO / very small periods so this doesn't silently regress for the untested call site.
• Minor/non-blocking: the plan's inline code sketch references self.config.period directly inside the move || closure at the start() call site (where today only config_count/rpc_retention_window/cloned Arcs are pre-extracted). I verified with a minimal repro that Rust 2021 disjoint-closure-capture makes this compile fine (Duration is Copy, so the field is copied into the closure without invalidating self for the next loop iteration/the shutdown_rx branch) — so this is not a compile-blocker, just an inconsistency with the file's existing style of pre-extracting fields before the closure. Worth a one-line let period = self.config.period; for consistency, but not required.
• Test-existence claims all check out: DEFAULT_MAINTENANCE_PERIOD = Duration::from_secs(4*60*60) exists (line 39); every existing test name the plan lists as "preserved" (test_maintenance_first_tick_is_delayed, test_bare_interval_first_tick_is_immediate, test_maintenance_config_default, test_maintenance_config_disabled, test_checkpoint_frequency, test_maintainer_creation, test_maintainer_disabled, test_perform_maintenance_empty_db, test_lmin_calculation, test_perform_maintenance_with_count_uses_correct_thresholds, test_delete_in_chunks_bounds_each_call_and_exhausts, test_delete_in_chunks_respects_total_budget, test_delete_in_chunks_stops_on_error, and the test_headers_*/test_tx_history_*/test_events_*/eviction tests) is present in origin/main's mod tests, and the two proposed new test names don't collide with anything existing. The arithmetic in the two new tests is correct (900/3=300, 14400/3=4800, both exact — no truncation surprises to worry about).

Full review

1. Are both call sites really comparing against a flat Duration::from_secs(10)?
Confirmed. origin/main:crates/app/src/maintainer.rs:220 (inside Maintainer::start's spawn_blocking_logged closure) and :269 (inside Maintainer::perform_maintenance) both read if elapsed > Duration::from_secs(10) { warn!(... "consider increasing AUTOMATIC_MAINTENANCE_COUNT ...") }. No other call sites of this pattern exist in the crate (App::perform_maintenance at app/mod.rs:3006, used by the /maintenance HTTP admin endpoint, calls run_maintenance directly with no elapsed/WARN logic at all — it's a distinct, unaffected code path).

2. Is the 11.25s structural-floor arithmetic correct?
Re-derived independently from the actual constants: MAINTENANCE_CHUNK_LEDGERS = 2 (line 343), MAINTENANCE_CHUNK_PAUSE = Duration::from_millis(25) (line 348). delete_in_chunks (line 357-387) sleeps MAINTENANCE_CHUNK_PAUSE after every successful non-empty chunk, provided remaining budget > 0, and breaks (no further sleep) only once a chunk returns Ok(0) rows or the budget is exhausted. At period_secs=900, the code's own comment/logic at line 185 (ledgers_per_period = period.as_secs()/5) yields 180 ledgers/cycle — a reasonable steady-state estimate of newly-prunable rows per table between cycles (assuming a prior cycle already pruned everything below the previous threshold). At MAINTENANCE_CHUNK_LEDGERS=2, that's 90 chunks; since count=50000 budget is never exhausted by 180 real rows, all 90 chunks are followed by a sleep (all except possibly a final Ok(0), which doesn't sleep) → 90 × 25ms ≈ 2.25s/table. run_maintenance (lines 469-494) invokes delete_in_chunks for SCP unconditionally, ledger-header and tx-history unconditionally, and events + ledger_close_meta only when rpc_retention_window is Some — i.e. 5 tables specifically on RPC-enabled validators (3 tables otherwise, ≈6.75s floor, below the flat 10s — consistent with the issue/triage explicitly scoping the bug to "RPC-enabled validator"). 5 × 2.25s = 11.25s, exceeding the flat 10s threshold before any real row deletion or query time. Math checks out exactly as claimed, and is corroborated by real telemetry in the issue body (elapsed_ms steady 20-28s, historical max 67219ms, 76 occurrences over 25d19h uptime on configs/validator-mainnet-rpc.toml with period_secs=900, retention_window=360).

3. Is AUTOMATIC_MAINTENANCE_COUNT really the wrong/irrelevant knob?
Confirmed. AUTOMATIC_MAINTENANCE_COUNT is a real stellar-core-compatible config key (compat_config.rs:78,463; documented in config.rs:1415 as mapping to MaintenanceAppConfig.count → MaintenanceConfig.count). delete_in_chunks's loop (while remaining > 0 { ...; match delete_chunk(chunk) { Ok(0) => break, ... } }) exits as soon as a chunk reports zero rows deleted, before the count/budget is exhausted, whenever real backlog < budget. At count=50000 vs. ~180 real ledgers/period (~278x), the budget is never the binding constraint — confirmed by reading the loop directly. So increasing count cannot reduce elapsed_ms in this scenario (no effect, since the loop already exits via Ok(0) well before hitting budget), and in a hypothetical scenario where a real backlog did exceed count, raising count would only add more chunks/sleeps, increasing (not decreasing) elapsed_ms — the remediation text is indeed backwards for the goal of "reducing how long maintenance takes."

4. Is period/3 reasonable, or is there a gap for zero/small period?
Real gap identified (see Key Concerns). Maintainer::start() explicitly checks self.config.period.is_zero() || self.config.count == 0 and returns before ever entering the loop (line 172), so the periodic call site never sees period=0. But Maintainer::perform_maintenance() — the plan's second patched call site — has no equivalent guard; it's a plain &self method that unconditionally computes elapsed and would unconditionally compute maintenance_warn_threshold(self.config.period). A MaintenanceConfig with period: Duration::ZERO is not rejected by the type/struct (no validation on MaintenanceConfig itself, only inside start()), so Maintainer::with_config(..., MaintenanceConfig { period: Duration::ZERO, .. }, ...) followed by a direct .perform_maintenance() call would hit threshold = 0/3 = 0, and the WARN would fire on every call regardless of actual elapsed time — resurrecting the exact bug being fixed for an edge case the plan's tests don't cover. The plan should add a floor/clamp or at least a test asserting/documenting behavior at period = Duration::ZERO (and ideally make the zero-period disabling behavior consistent between start() and perform_maintenance()).

5. Do DEFAULT_MAINTENANCE_PERIOD and the referenced tests exist?
Confirmed. DEFAULT_MAINTENANCE_PERIOD = Duration::from_secs(4*60*60) at line 39. Grepped all test names the plan lists as both "new" and "preserved" against origin/main's mod tests (lines 497-1476) — every named existing test is present verbatim, and neither of the two new proposed test names (test_maintenance_warn_threshold_scales_with_period, test_maintenance_warn_threshold_uses_default_period) collides with an existing name. Arithmetic in both proposed tests is exact (900s/3=300s; 14400s/3=4800s), no truncation edge cases.

Additional note (non-blocking): verified with a standalone minimal repro that referencing self.config.period directly inside the move || closure passed to spawn_blocking_logged in Maintainer::start() compiles under Rust 2021 disjoint-closure-capture (Duration is Copy, so the field capture doesn't invalidate self for the next loop iteration or the concurrent self.shutdown_rx.changed() select! branch). Not a blocker, but for consistency with the existing pattern (config_count, rpc_retention_window, and the cloned Arcs are all pre-extracted before the closure today) it'd read more consistently to do the same for period.

[tomerweller]

✅ Converged Plan

Summary: Replace the flat, pre-chunking Duration::from_secs(10) "maintenance took too long" threshold with one derived from the configured maintenance period (clamped to a safe floor), consolidate the duplicated check into one named helper, and fix the remediation text to name the actual tuning knobs (MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE) instead of the irrelevant AUTOMATIC_MAINTENANCE_COUNT.

Files to modify:

• crates/app/src/maintainer.rs — add fn maintenance_warn_threshold(period: Duration) -> Duration, use it at both existing call sites (Maintainer::start, ~line 219-228, and Maintainer::perform_maintenance, ~line 268-277), replacing the duplicated Duration::from_secs(10) + warn text; add unit tests for the new helper in the existing #[cfg(test)] mod tests block.

Approach:

Root cause (confirmed against origin/main): both warn sites compare cycle elapsed against a flat Duration::from_secs(10) (lines 220 and 269). This predates MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE (lines 343/348, added for #3702 to stop delete_in_chunks, line 357, from holding the WAL write lock across a whole table's deletion). delete_in_chunks sleeps MAINTENANCE_CHUNK_PAUSE (25ms) between every chunk of at most MAINTENANCE_CHUNK_LEDGERS (2) ledgers, across up to 5 tables in run_maintenance (SCP, ledger headers, tx history, and — when RPC retention is configured — events and ledger_close_meta). At the mainnet-rpc config's period_secs=900 (~180 ledgers/cycle at ~5s/ledger), that's ~90 chunks/table × 5 tables × 25ms ≈ 11.25s of pure sleep — already over the flat 10s threshold before any row is deleted, so the WARN is structurally guaranteed to fire on every cycle in steady state. The remediation text ("increase AUTOMATIC_MAINTENANCE_COUNT") is backwards: count only bounds the per-table chunk budget; raising it permits more chunks (more sleeps), increasing elapsed_ms, and is moot here since count=50000 is ~278x the ~180 ledgers/period actually produced (the loop exits early on Ok(0), so budget was never the binding constraint — verified by Critic A).

Add a single small pure helper next to the other maintenance constants:

/// Warn threshold for a maintenance cycle's elapsed time.
///
/// A flat threshold predates the per-chunk WAL-lock-yielding pauses added
/// for #3702 (`MAINTENANCE_CHUNK_PAUSE` between chunks, across up to 5
/// tables in `run_maintenance`), whose structural floor alone can exceed a
/// few seconds on short periods — firing this warning in steady state
/// rather than signaling genuine slowness. Instead, warn only when a cycle
/// risks not keeping up with its own configured period: a cycle longer than
/// a third of the period is the operationally meaningful risk, independent
/// of how many tables/chunks the current build happens to have. Clamped to
/// a 10s floor (the previous flat threshold) so a misconfigured/degenerate
/// `period` (e.g. zero, not rejected by the type system) cannot make this
/// warning fire unconditionally by driving the threshold itself to zero.
fn maintenance_warn_threshold(period: Duration) -> Duration {
    (period / 3).max(Duration::from_secs(10))
}

Replace both if elapsed > Duration::from_secs(10) checks with if elapsed > maintenance_warn_threshold(self.config.period), and update the warn message at both sites to:

"Maintenance cycle exceeded its expected-relative-to-period threshold; if \
 consistently slow, consider tuning MAINTENANCE_CHUNK_LEDGERS/MAINTENANCE_CHUNK_PAUSE \
 or performing manual database maintenance"

(keep elapsed_ms as a structured field, unchanged). At period_secs=900 this yields a 300s threshold — comfortably above the observed 20-28s (historical max 67s) — while the 10s floor only ever binds for degenerate period values well below any real config (mainnet-rpc=900s, default=4h), so it does not change behavior for any config in use today.

Both call sites share self.config.period, so a single free function taking period as a parameter keeps the two sites from drifting again and is trivially unit-testable without constructing a Maintainer.

Kind: bug-fix

Test plan:

• Regression tests (crates/app/src/maintainer.rs, in the existing mod tests):
  • test_maintenance_warn_threshold_scales_with_period: asserts maintenance_warn_threshold(Duration::from_secs(900)) == Duration::from_secs(300), matching the mainnet-rpc config's observed period_secs=900. New function (doesn't exist on origin/main today); the "would fail on main" equivalent is that today's flat Duration::from_secs(10) at both call sites causes the WARN to fire on every cycle for the observed 20-67s elapsed range at this config, whereas the new 300s threshold does not.
  • test_maintenance_warn_threshold_uses_default_period: asserts maintenance_warn_threshold(DEFAULT_MAINTENANCE_PERIOD) == DEFAULT_MAINTENANCE_PERIOD / 3, guarding the formula for the crate's default 4-hour period.
  • test_maintenance_warn_threshold_zero_period_is_clamped (added per Critic A, Round 1, REVISE-MINOR): asserts maintenance_warn_threshold(Duration::ZERO) == Duration::from_secs(10), guarding against the exact bug being fixed re-appearing for a degenerate period=0 config at the perform_maintenance() call site, which — unlike Maintainer::start() — has no separate zero-period guard upstream.
  • test_maintenance_warn_threshold_exceeds_structural_sleep_floor (added per Critic C, Round 1, REVISE-MINOR): computes the theoretical structural sleep floor from the crate's own real constants (5 tables × ceil(period_ledgers / MAINTENANCE_CHUNK_LEDGERS) chunks × MAINTENANCE_CHUNK_PAUSE, using the mainnet-rpc period_secs=900 scenario from the issue) and asserts it stays strictly under maintenance_warn_threshold(period). This converts the plan's hand-verified ~3.75%-margin arithmetic into a CI guardrail that fails loudly if MAINTENANCE_CHUNK_PAUSE/MAINTENANCE_CHUNK_LEDGERS are ever retuned enough to threaten the invariant again.
• Existing tests preserved (unaffected — verified by reading each; none reference the warn threshold or its message): test_maintenance_first_tick_is_delayed, test_bare_interval_first_tick_is_immediate, test_maintenance_config_default, test_maintenance_config_disabled, test_checkpoint_frequency, test_maintainer_creation, test_maintainer_disabled, test_perform_maintenance_empty_db, test_lmin_calculation, test_perform_maintenance_with_count_uses_correct_thresholds, test_delete_in_chunks_bounds_each_call_and_exhausts, test_delete_in_chunks_respects_total_budget, test_delete_in_chunks_stops_on_error, plus the remaining test_headers_*/test_tx_history_*/test_events_*/eviction tests.

Parity considerations: n/a — non-parity path. Corrected per Critic B (parity, Round 1): the accurate statement is not "stellar-core has no equivalent" but "stellar-core's Maintainer (with a LogSlowExecution-based 2s-DEBUG/20s-WARNING escalating threshold, differently designed — a small absolute threshold on non-chunked bulk deletes, not a period-scaled one) was removed entirely in commit dc44d5bd6 ('Remove maintenance #5157'), landing before v26.0.0." Since the pinned parity target (v27.0.0) has no Maintainer class at all, there is genuinely nothing to mirror for the currently-pinned version, and the old (pre-removal) design's absolute-threshold approach would not be a good template to copy even if it still existed, since it doesn't scale with period the way this fix's structural-floor problem requires. No consensus/ledger/wire-protocol behavior is touched either way.

Minor items to consider during implementation:

• The /3 divisor remains a judgment call (matches the issue's own suggested example and is verifiably well above all observed elapsed times), not derived from a first-principles safety margin against the chunk constants directly — the new test_maintenance_warn_threshold_exceeds_structural_sleep_floor test is the guardrail that catches if this judgment call stops holding.
• This changes an operator-visible log line's wording only; no change to run_maintenance, delete_in_chunks, or any deletion threshold/query.

Convergence: Round 1, verdicts: A=REVISE-MINOR, B=REVISE-MINOR, C=REVISE-MINOR (all three concrete, well-scoped corrections folded directly into this converged plan rather than spinning a second round)