Skip to content

fix: report retry interval in seconds, not minutes - #889

Open
21Mill wants to merge 1 commit into
MostroP2P:mainfrom
21Mill:fix/scheduler-retry-interval-log-unit
Open

fix: report retry interval in seconds, not minutes#889
21Mill wants to merge 1 commit into
MostroP2P:mainfrom
21Mill:fix/scheduler-retry-interval-log-unit

Conversation

@21Mill

@21Mill 21Mill commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What

job_retry_failed_payments announces its cadence in minutes while printing a
value expressed in seconds:

// src/scheduler.rs:224
let interval = ln_settings.payment_retries_interval as u64;
...
info!(
    "I run async every {} minutes - checking for failed lighting payment",
    interval
);
...
tokio::time::sleep(tokio::time::Duration::from_secs(interval)).await;

Two words change on that line: minutesseconds, and lighting
lightning.

Why the code is right and the message is wrong

The unit is unambiguous everywhere else:

  • the same loop sleeps Duration::from_secs(interval);
  • README.md:563payment_retries_interval = 60 # seconds between retries;
  • docs/STARTUP_AND_CONFIG.md:112 — "Retry interval in seconds (default: 60)";
  • docs/LIGHTNING_OPS.md:115// Seconds between retries (default: 60);
  • settings.tpl.toml:19 ships 60.

So with the shipped default the job runs once a minute and reports once an
hour — a factor of sixty, in the direction that makes an operator wait.

What settles it as a slip rather than a local convention is
job_update_rate_events, eighty lines below in the same file:

// src/scheduler.rs:319-326
let interval = mostro_settings.user_rates_sent_interval_seconds as u64;
...
info!(
    "I run async every {} minutes - update rate event of users",
    interval / 60
);

Same phrasing, same seconds-valued setting — but converted. Two sibling jobs,
one honoured the conversion and the other did not.

How it surfaced

While operating a live instance I needed to know whether the retry job had
already swept a buyer payout stranded in settled-hold-invoice. The log line
implied the next sweep was an hour off, so the natural reading was that
nothing had happened yet. Comparing timestamps showed it had run three times
in the preceding three minutes:

INFO mostrod::scheduler: I run async every 60 minutes - checking for failed lighting payment
INFO mostrod::scheduler: I run async every 60 minutes - checking for failed lighting payment
INFO mostrod::scheduler: I run async every 60 minutes - checking for failed lighting payment

Sixty seconds apart. During an incident that is the difference between "the
retry has not been attempted" and "the retry has already been attempted and
did not fire", which are opposite conclusions about whether to intervene by
hand.

Testing

cargo test --bin mostrod — 1199 passed, 0 failed.
cargo clippy --all-targets -- -D warnings — clean.
cargo fmt --check — clean.

No test is added: the change is the wording of a log line, with no observable
behaviour to assert. Asserting on log output would couple the suite to a
string without testing anything real.

Notes for the reviewer

  • test_lighting_settings in src/config/mod.rs:184 carries the same
    misspelling. Left untouched — renaming a test is a different change from
    correcting operator-facing output, and it belongs in whatever PR next touches
    that module.
  • job_update_rate_events computes interval / 60 with integer division, so
    an interval under 60 seconds would report 0 minutes. Out of scope here and
    not currently reachable with the shipped defaults, but flagging it in case
    you want it tracked.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected the failed Lightning payment job log message.
    • The retry interval is now reported in seconds for clearer diagnostics.

`job_retry_failed_payments` logs "I run async every {} minutes" while
printing `payment_retries_interval`, which is seconds: the same loop
feeds it to `Duration::from_secs`, and README.md,
docs/STARTUP_AND_CONFIG.md and docs/LIGHTNING_OPS.md all document it
as seconds. With the default of 60 the job ticks once a minute while
claiming to tick once an hour.

That the unit is a slip rather than a convention is settled by
`job_update_rate_events` a few lines below: it also reports minutes,
but divides its seconds-valued interval by 60 first.

Found while checking whether the retry job had already passed over a
payout stranded in settled-hold-invoice on a live instance. The log
said the next tick was an hour away; it had in fact already run three
times.

Also correct "lighting" to "lightning" in the same string, so the one
line this touches is left without a known error in it.

No test accompanies this. The change is the wording of a log line and
there is no observable behaviour to assert.
@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a72d0e65-0519-4dc6-9006-568bd2844462

📥 Commits

Reviewing files that changed from the base of the PR and between 2f2b813 and cbe74be.

📒 Files selected for processing (1)
  • src/scheduler.rs

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.


Walkthrough

The scheduler retry-job log now reports its configured interval in seconds. It also corrects “lighting” to “lightning.” No public declarations changed.

Changes

Scheduler logging

Layer / File(s) Summary
Retry log message
src/scheduler.rs
The failed Lightning payment retry log now reports the interval in seconds and uses the corrected “lightning” spelling.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to cbe74

This localized change corrects the retry interval and spelling in an operator-facing log message without changing runtime behavior; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: arkanoider, grunch

Poem

I’m a rabbit, pleased to see
Seconds logged accurately.
Lightning shines, the typo’s gone,
Retry notes now hop along.
Nibble, log, and carry on!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: correcting the retry interval log to report seconds instead of minutes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@AndreaDiazCorreia AndreaDiazCorreia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants