Give certificate deletion a grace period #48

New issue

Open

opened 2026-10-23 10:22:27 +00:00 by james.oates · 0 comments

james.oates commented

2026-10-23 10:22:27 +00:00

Owner

The hazard

cert_manager::reconcile_once's first loop deletes, immediately and unconditionally, every certificate row whose name is absent from the desired set — row, private key and resolver entry together. There is no grace period and no confirmation that the absence is real.

That is correct when a name genuinely left the route table. It is destructive when the absence is transient, because the deletion is irreversible: the name must be re-ordered from scratch, against Let's Encrypt's 50-new-certificates-per-registered-domain-per-week limit — the limit ORDER_SPACING exists to respect.

Two transient causes are known, and both are now guarded at the trigger rather than here:

A failed list_services read was passed on as an empty desired set, so one failed read wiped the entire store. Fixed by skipping the pass instead (1402be1). list_certificates is a different query on a different table, so it can succeed while list_services fails — a FromRow decode failure on one malformed access_rules/routing_hosts value made that persistent rather than transient.
A config file written in place. reload_routes_config triggered on a bare mtime change, and a prefix of a TOML document very often still parses, so a truncated read shrank the service table, woke the reconciler and deleted the momentarily-missing names' keys. The next reload restored the services but not the keys. Narrowed by debouncing the watcher until the file settles.

Both fixes are at the trigger. Neither makes the deletion itself safe, and the debounce in particular only narrows the window — a writer slower than the settle interval can still be read mid-write.

Proposal

Require a name to be absent for two consecutive passes before deleting it. First pass marks, second pass deletes.

Kills the whole class rather than the two causes we happened to find, including causes nobody has thought of yet.
Costs a departed name's certificate lingering one extra interval, which harms nothing: the route is already gone, so nothing routes to it, and the resolver entry is dropped on the first pass regardless.
State can live in start_reconciler in memory — no migration. An empty set after a restart means the first pass never deletes, which is also the safer default.

reconcile_once's signature grows a &mut BTreeSet<String> of names seen absent, which is why this is a follow-up rather than part of the release: it touches every reconciler test.

Not a regression

Pre-v2.0.0 there was nothing to delete this way — certificates were configured per apex via [[tls]], not derived from the route table, so a disappearing service never removed one. The exposure arrives with derivation.

Test to write first

A pass with an empty desired set must not delete on the first call and must on the second. cert_manager::tests::an_empty_desired_set_deletes_every_certificate currently pins the immediate behaviour and would be updated to the two-pass contract.

## The hazard `cert_manager::reconcile_once`'s first loop deletes, immediately and unconditionally, every certificate row whose name is absent from the desired set — row, private key and resolver entry together. There is no grace period and no confirmation that the absence is real. That is correct when a name genuinely left the route table. It is destructive when the absence is transient, because the deletion is irreversible: the name must be re-ordered from scratch, against Let's Encrypt's 50-new-certificates-per-registered-domain-per-week limit — the limit `ORDER_SPACING` exists to respect. Two transient causes are known, and both are now guarded at the trigger rather than here: - **A failed `list_services` read** was passed on as an *empty* desired set, so one failed read wiped the entire store. Fixed by skipping the pass instead (`1402be1`). `list_certificates` is a different query on a different table, so it can succeed while `list_services` fails — a `FromRow` decode failure on one malformed `access_rules`/`routing_hosts` value made that persistent rather than transient. - **A config file written in place.** `reload_routes_config` triggered on a bare mtime change, and a *prefix* of a TOML document very often still parses, so a truncated read shrank the service table, woke the reconciler and deleted the momentarily-missing names' keys. The next reload restored the services but not the keys. Narrowed by debouncing the watcher until the file settles. Both fixes are at the trigger. Neither makes the deletion itself safe, and the debounce in particular only narrows the window — a writer slower than the settle interval can still be read mid-write. ## Proposal Require a name to be absent for **two consecutive passes** before deleting it. First pass marks, second pass deletes. - Kills the whole class rather than the two causes we happened to find, including causes nobody has thought of yet. - Costs a departed name's certificate lingering one extra interval, which harms nothing: the route is already gone, so nothing routes to it, and the resolver entry is dropped on the first pass regardless. - State can live in `start_reconciler` in memory — no migration. An empty set after a restart means the first pass never deletes, which is also the safer default. `reconcile_once`'s signature grows a `&mut BTreeSet<String>` of names seen absent, which is why this is a follow-up rather than part of the release: it touches every reconciler test. ## Not a regression Pre-v2.0.0 there was nothing to delete this way — certificates were configured per apex via `[[tls]]`, not derived from the route table, so a disappearing service never removed one. The exposure arrives with derivation. ## Test to write first A pass with an empty desired set must **not** delete on the first call and **must** on the second. `cert_manager::tests::an_empty_desired_set_deletes_every_certificate` currently pins the immediate behaviour and would be updated to the two-pass contract.

james.oates referenced this issue from a commit

2026-10-25 15:32:56 +00:00

fix: wait for the config file to settle before reloading it

james.oates referenced this issue

2026-10-26 15:38:00 +00:00

A name has no resolver entry until the serial reconcile loop reaches it, so it is served the fallback #57

james.oates referenced this issue

2026-10-26 17:53:55 +00:00

The WebSocket path has no connect timeout, no idle timeout and no connection cap #62