Follow-up to #924.
The shutdown path added in #924 cancels the daemon future rather than letting it unwind:
// dstack/certbot/cli/src/main.rs
tokio::select! {
_ = bot.run() => unreachable!("certbot daemon returned"),
result = shutdown_signal() => result?,
}
If the signal lands while renew_inner is mid-ACME-order, bot.run() is dropped at its current await point and the cleanup at the end of the DNS-01 flow never runs:
// dstack/certbot/src/acme_client.rs:185
if let Err(err) = self.dns01_client.remove_record(&challenge.id).await {
error!("failed to remove dns record {}: {err}", challenge.id);
}
Result: a stale _acme-challenge TXT record left in the DNS zone, plus a pending authorization at the CA.
This is not a regression — before #924 the default SIGTERM disposition killed the process at the same point with the same effect — and it is self-healing, because set_txt_records calls remove_txt_records(&acme_domain) before publishing new ones on the next attempt. But "stop the daemon cleanly" currently means "stop promptly", not "stop without leaving state behind", and the gap is worth closing.
Proposal
Give the loop a cancellation token instead of dropping the future:
- check the token at the top of each iteration and in the interval wait (
select! between sleep(renew_interval) and cancellation) — this covers the idle case, which is the overwhelmingly common one and is already instant today;
- for the in-flight case, either let the current renewal run to completion under a bounded grace period before exiting, or make the DNS-01 challenge cleanup drop-safe (scope guard) so cancellation at any await point still removes the TXT record.
The grace period must stay bounded — renew_timeout already caps a single renewal, so reusing it as the shutdown deadline is a reasonable ceiling.
Follow-up to #924.
The shutdown path added in #924 cancels the daemon future rather than letting it unwind:
If the signal lands while
renew_inneris mid-ACME-order,bot.run()is dropped at its current await point and the cleanup at the end of the DNS-01 flow never runs:Result: a stale
_acme-challengeTXT record left in the DNS zone, plus a pending authorization at the CA.This is not a regression — before #924 the default SIGTERM disposition killed the process at the same point with the same effect — and it is self-healing, because
set_txt_recordscallsremove_txt_records(&acme_domain)before publishing new ones on the next attempt. But "stop the daemon cleanly" currently means "stop promptly", not "stop without leaving state behind", and the gap is worth closing.Proposal
Give the loop a cancellation token instead of dropping the future:
select!betweensleep(renew_interval)and cancellation) — this covers the idle case, which is the overwhelmingly common one and is already instant today;The grace period must stay bounded —
renew_timeoutalready caps a single renewal, so reusing it as the shutdown deadline is a reasonable ceiling.