feat(queue): make the delivered-message copy configurable, restore it by default, and surface it in the UI - #148
Open
EvalAlan wants to merge 5 commits into
Open
feat(queue): make the delivered-message copy configurable, restore it by default, and surface it in the UI#148EvalAlan wants to merge 5 commits into
EvalAlan wants to merge 5 commits into
Conversation
…t by default Every successful delivery writes a copy of the message it just delivered into a consumed-enqueue tombstone: 2,243 bytes per delivery, and the single largest remaining cost on the delivery path. Whether that is worth paying depends on how far back a deployment can roll, so it is now a setting rather than a constant. retain_tombstone_body defaults to true, and this restores the body to the sqlite and postgres backends, which is a correction. Dropping it from those two earlier introduced exactly the hazard the file backend has a test against — a binary rolled back past the content digest reads only that field, finds it empty, decides every retry conflicts and starts refusing mail — and there was no test on those backends to notice. There is now: TestSQLiteTombstoneRemainsReadableByOldBinaries fails on the previous behaviour. The flag is phrased negatively inside the queue package, dropBody rather than retainBody, so the Go zero value keeps the body. Backends are built as struct literals in several tests, and none of them should be able to opt into the unsafe side by omission. The config key is phrased positively, because an operator reading retain_tombstone_body = false should be able to tell what they are turning off. Threaded as a variadic ManagerOption rather than a seventh positional argument to NewManagerFromBackend: a bare bool in seventh position is the shape of parameter that gets passed in the wrong slot and quietly changes durability. The option missed the indexedfs branch on the first attempt — it returned before the options were applied, which is the settings-that-do-nothing failure this repository keeps producing. The test now walks every backend the factory can return and fails on an unhandled type rather than passing by omission.
…eue backend read-only The tombstone body copy is the largest single cost on the delivery path — about 2.2KB written per delivered message — and whether it is worth paying depends on how far back a deployment can roll. That is an operator decision, so it belongs in front of an operator. The control says what it does rather than what it is called. Nobody outside this codebase knows what a tombstone is, so the label is 'Keep a copy of each delivered message for rollback safety' and the hint gives the measured cost, what turning it off buys, and the specific thing that breaks: an older binary reads that copy, and without it treats retried mail as a conflict and starts refusing it. The queue backend is shown read-only. Switching backends does not migrate queued mail — it points the server at a different store and leaves whatever is in the old one behind, which this codebase has already done by accident: persistConfig carries a comment about a rate-limiter toggle that silently reset queue.backend and orphaned the queue. A dropdown would make that a one-click action during an incident, so changing it stays in the config file with a restart, and the UI explains why. Markup follows the idiom already in the settings panel — a plain label wrapping the input and a field-hint div — rather than the config-hint and checkbox-label classes I first reached for, neither of which exists in the stylesheet. The round-trip test fails when the PUT handling is removed, which is the failure that matters here: a control that renders, reports success, and saves nothing.
Two bugs, both reported from actually using the control rather than from any test, and both the same failure in different places: a setting that saves, reports success, and changes nothing. It did not survive a restart. ELEMTA_CONFIG_RESEED is set by 'make install-dev*' to mean 'this deploy is authoritative, take the template again', but compose bakes it into the container environment, so it was still true on every subsequent 'docker restart' — and each restart deleted the runtime config and re-copied the template, discarding everything saved in the web UI. This affected every UI setting, not just this one. The reseed now happens once per container instance: the marker lives in the container's own filesystem, so a recreate, which is what install-dev does, still reseeds, while a plain restart leaves the operator's config alone. It also did not take effect without a restart, and the UI claimed otherwise. The policy was captured when the queue manager was constructed, and the reload path never rebuilds the manager — deliberately, since it owns the queue. Reload now applies settings that only change how the manager writes, to the live manager. That makes the flag readable while mail is being delivered and writable from the reload goroutine, so it is an atomic rather than a plain bool. A data race here would only appear when an operator saves a setting under load, which is exactly when it would happen. The zero value is still 'keep the body', so a backend built as a struct literal cannot opt into the unsafe side by omission.
The first version of this fix put the marker in /tmp, which the shipped compose mounts as tmpfs — memory-backed and empty on every start, restart included. The marker was therefore always absent, the reseed fired every time, and the bug reproduced exactly as before. It looked right and changed nothing, which is the same failure the fix was written to remove. /app is writable by the service user and only its subdirectories are mounts, so a marker there lives in the container's own layer: present across a restart, absent after a recreate. Verified against the running stack rather than by reading it: a value saved into the runtime config survives two restarts, and a deliberate recreate with ELEMTA_CONFIG_RESEED=true still takes the template again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every successful delivery writes a copy of the message it just delivered into a
consumed-enqueue tombstone: 2,243 bytes per delivery, and the largest single
remaining cost on the delivery path. Whether that is worth paying depends on how
far back a deployment can roll, so it becomes a setting rather than a constant.
A correction, not just a feature
retain_tombstone_bodydefaults to true, and that restores the body to thesqlite and postgres backends.
Dropping it from those two earlier introduced exactly the hazard the file backend
has a test against: a binary rolled back past the content digest reads only that
field, finds it empty, decides every retry conflicts, and starts refusing
mail. Those backends had no rollback test, so nothing failed and it merged.
There is one now —
TestSQLiteTombstoneRemainsReadableByOldBinariesfails on theprevious behaviour.
Design choices worth naming
Negative internally, positive externally.
dropBodyinside the queue packageso the Go zero value keeps the body — backends are built as struct literals in
several tests and none should opt into the unsafe side by omission.
retain_tombstone_bodyin config, so= falsereads as what is being turned off.A variadic
ManagerOption, not a seventh positional argument toNewManagerFromBackend. A bare bool in seventh position is the parameter thatgets passed in the wrong slot and quietly changes durability.
It missed the
indexedfsbranch first — that path returned before optionswere applied, which is the settings-that-do-nothing failure this queue keeps
producing. The test now walks every backend the factory can return and fails on
an unhandled type rather than passing by omission.
The web UI
The control says what it does, not what it is called: "Keep a copy of each
delivered message for rollback safety", with the measured cost, what turning it
off buys, and the specific thing that breaks.
The queue backend is read-only. Switching backends does not migrate queued
mail; it points the server at a different store and orphans what is in the old
one — which this codebase has already done by accident, as
persistConfig's owncomment records. A dropdown would make that one click during an incident, so it
stays in the config file with a restart, and the UI says why.
Markup uses the idiom already in the settings panel rather than the
config-hint/checkbox-labelclasses I first reached for, neither of whichexists in the stylesheet.
Verified
gofmt, build, vet, golangci-lint (0 issues), and the api, queue and config
suites. Tests confirmed to fail on the broken behaviour: the sqlite rollback
case, and the API round trip when the PUT handling is removed — a control that
renders, reports success and saves nothing.
Two bugs found by using the control, not by testing it
Reported from the running stack after the first version of this branch: the
checkbox did not survive a restart, and did not appear to do anything.
It did not persist.
ELEMTA_CONFIG_RESEEDis set bymake install-dev*tomean "this deploy is authoritative, take the template again", but compose bakes
it into the container environment, so it was still true on every subsequent
docker restart— and each restart deleted the runtime config and re-copied thetemplate. This discarded every setting saved in the web UI, not just this
one. The reseed now happens once per container instance.
It did not take effect without a restart, and the UI implied otherwise. The
policy was captured when the queue manager was constructed, and the reload path
deliberately never rebuilds the manager because it owns the queue. Reload now
applies settings that only change how the manager writes, to the live manager.
That makes the flag readable by delivery goroutines while the reload goroutine
writes it, so it is an
atomic.Bool— a race there would only appear when anoperator saves under load, which is exactly when it would.
And the first fix for the first bug was wrong. The marker went in
/tmp,which the shipped compose mounts as tmpfs: memory-backed, empty on every start,
so the marker was always absent and the reseed fired anyway. It reproduced the
bug it was meant to fix. The marker now lives in the container's writable layer.
Verified against the running stack: a saved value survives two restarts, and a
deliberate recreate still takes the template.