Skip to content

fix(certificates): do not verify DNS again for a job the endpoint just verified - #13765

Draft
Meldiron wants to merge 1 commit into
mainfrom
fix/skip-domain-validation-after-api-check
Draft

Meldiron wants to merge 1 commit into
mainfrom
fix/skip-domain-validation-after-api-check

Conversation

@Meldiron

Copy link
Copy Markdown
Contributor

Option 1 of two: the explicit variant. Option 2 is cloud-only (appwrite-labs/cloud#5963) and infers the same thing from the rule's status. One of the two will be kept.

Problem

The proxy create endpoints and the re-verify endpoint run the multi-region DNS validator, write the rule as verifying, and enqueue generation. Seconds later the certificates worker ran the identical validator again. When both pass, that is doubled DNS traffic and latency for nothing. When the second run fails on a transient, the worker writes unverified with "DNS verification failed" on a rule the API just accepted, and the console contradicts itself. That flip is the only outcome the second check can produce that the first could not.

Change

  • Appwrite\Event\Message\Certificate gets skipDomainValidation, default false, serialized with the rest. Jobs already in the queue when this ships carry no key and behave as before.
  • The worker's generation path skips validateDomain when the flag is set. Unlike skipRenewCheck, the renew check stays in place, so an existing certificate is still reused rather than re-issued. That distinction matters for self-hosted Let's Encrypt, where a needless re-issue counts against rate limits.
  • Every enqueuer that verified DNS itself sets the flag: the API, function, site and redirect rule create endpoints when the rule came out verifying, the re-verify endpoint, and the worker's own domain-verification action when it hands off to generation.

Not set by maintenance renewals, the SSL task, the main-domain bootstrap, or the auto-issue path for Appwrite-owned subdomains, none of which verified anything first. The worker's DNS check still runs for all of them.

If the DNS really did change between the endpoint's check and the job, the provider's own challenge does not complete and the status stays pending; nothing is issued for a domain that does not point at the instance.

Tests

  • tests/unit/Event/Message/CertificateTest.php: the flag round-trips through the queue payload with the renew check untouched, and a payload from before the flag reads as false.
  • tests/unit/Workers/CertificatesDomainValidationTest.php drives the worker's public action with a domain under a TLD no public suffix list knows, which the DNS check refuses before touching the network, so the tests observe whether the check ran: with the flag the job reaches issueCertificate and the rule stays verifying; with the flag and no renewal needed nothing is issued and the rule still stays verifying; without the flag the job never reaches issuance and the rule is written unverified.

composer lint passes. PHPStan passes on the touched files (the full composer analyze exceeds Composer's process timeout on this machine). Tests were not run locally.

🤖 Generated with Claude Code

…t verified

The proxy create and re-verify endpoints run the multi-region DNS validator,
write the rule as verifying and enqueue generation. The worker then ran the
identical validator again seconds later: doubled DNS traffic when both pass,
and when the second run failed on a transient it wrote unverified over a
status the endpoint had just written. The certificate message now carries
skipDomainValidation, set by every enqueuer that verified DNS itself, and
the worker honours it. The renew check stays in place, unlike a forced job.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

✨ Benchmark results

Comparing main (before) → fix/skip-domain-validation-after-api-check (after).

Metric Before After Change
🚀 Requests/sec 429.61 468.75 🟢 +9.1%
⏱️ Latency P50 40.65 ms 36.77 ms 🟢 -9.5%
⏱️ Latency P95 94.8 ms 88.55 ms 🟢 -6.6%
Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario P50 (ms) P95 (ms) Requests RPS Δ P95 (ms)
API total 36.77 88.55 28,557 468.75 -6.26
Account 65.76 136.98 1,503 25.01 -2.93
TablesDB 34.8 69.11 15,531 257.01 -7.82
Storage 34.46 82.51 7,515 127.03 -9.02
Functions 53.7 110.5 4,008 68.56 -3.04

Top API waits (after)

API request Max wait (ms)
functions.create 251.5
account.prefs.update 208.49
account.get 208.48
storage.files.create 208.26
account.name.update 206.99

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.

1 participant