internal/ctlog: evict low priority entries from pool under load#56
internal/ctlog: evict low priority entries from pool under load#56FiloSottile wants to merge 2 commits intomainfrom
Conversation
If the pool is full, any lower-priority entries will be evicted and replaced by higher-priority submissions. Lower-priority entries are precertificates with NotBefore more than 48h in the past, or certificates with an SCT extension. Evicted entries are served a 503 immediately.
There was a problem hiding this comment.
Pull request overview
This PR updates the CT log submission pool behavior so that, when the pool is at capacity, low-priority submissions can be evicted to make room for higher-priority ones, returning 503 to the evicted requests.
Changes:
- Adds low-priority classification (old precerts; certs containing SCTs) and passes it through the HTTP submission path into pool admission.
- Implements pool eviction mechanics and introduces a distinct
errEvictederror. - Extends metrics labeling and adds test coverage for eviction/rate-limit scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ctlog/metrics.go | Adds low_priority label to addchain_requests_total. |
| internal/ctlog/http.go | Computes low-priority classification, threads it into pool admission, and handles eviction/rate-limit outcomes. |
| internal/ctlog/ctlog.go | Implements eviction of low-priority pool entries under load and introduces errEvicted. |
| internal/ctlog/export_test.go | Exposes eviction/rate-limit errors and helpers for tests. |
| internal/ctlog/ctlog_test.go | Adds TestRatelimit to validate eviction and rejection behavior. |
| cmd/sunlight/sunlight.go | Updates PoolSize documentation to describe eviction semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Could we get a prometheus metric recording how many submissions are low priority? |
|
|
oh, perfect - I didn't notice that line in the diff before |
| labels["chain_len"] = fmt.Sprintf("%d", len(chain)) | ||
| labels["root"] = x509util.NameToString(chain[len(chain)-1].Subject) | ||
| labels["issuer"] = x509util.NameToString(chain[0].Issuer) | ||
| labels["low_priority"] = fmt.Sprintf("%v", lowPriority) |
There was a problem hiding this comment.
This is printing a function value into a metric, should be converted to boolean 1/0 or a reason code (AlreadyHasSCT)
There was a problem hiding this comment.
This is adding a label which to the labels set with a value of the boolean lowPriority, which will get you a separate time series.
The resulting metric looks like:
sunlight_addchain_requests_total{chain_len="3",error="",issuer="C=US, O=(STAGING) Let's Encrypt, CN=(STAGING) Mysterious Mulberry E8",log="navigli2026h1",low_priority="false",precert="true",preissuer="",reused="false",root="C=US, O=(STAGING) Internet Security Research Group, CN=(STAGING) Pretend Pear X1",source="sequencer"} 124136
Note the low_priority="false" label on the metric.
So this code is corrrect, though perhaps it's worth considering using a different variable name than the function name it will shadow
There was a problem hiding this comment.
Oh, I somehow thought that was a lookup in the pool map rather than recomputation of the low priority criteria
If the pool is full, any lower-priority entries will be evicted and replaced by higher-priority submissions.
Lower-priority entries are precertificates with NotBefore more than 48h in the past, or certificates with an SCT extension.
Evicted entries are served a 503 immediately.