refactor(banners): Replace the payment-failure evaluator with a generic backend command intake by MontaGhanmy · Pull Request #4909 · linagora/cozy-stack

MontaGhanmy · 2026-09-10T11:25:27Z

Context

The stack was deciding when a payment problem deserves a banner. It read the Stripe subscription status and the invoice attempt count off the bus, ran the escalation rules itself, and picked the wording from its own translation catalog. None of that is ours: the stack should be writing banners, not deciding them. Every wording change or rule change meant a stack release, and the billing rules already live on the backend side that owns the subscription.

Solution

billing.go, the BillingLifecycleMessage contract, its handler, its queue and the four billing translations are removed. In their place a backend publishes a banner.materialize or a banner.clear command on the platform exchange, carrying the wording it already decided, and the stack validates it and stores it. docs/banners.md describes that contract for the producing side.

The intake is not a pass-through. A command is refused if its category is not listed in banner_command_categories for the instance context, and the quota category is refused always, because the stack measures disk usage itself and no backend should be able to speak for it. The routing key decides materialize versus clear, never the payload, so a producer cannot clear a category by naming a field. The CTA has to be an absolute https URL, the payload has to fit MaxCommandBytes, and the bannerId has to match the format the doctype documents.

Ordering is the part worth reading. Delivery is at-least-once and unordered, so the intake needs to know what it already applied. That record is a new stack-private doctype, io.cozy.banners.commands, blocklisted so no application can reach it, holding the last accepted revision per category. It is separate from io.cozy.banners on purpose: an app has write access there to carry a dismissal, and if the ordering rode on the visible document that app could rewind it. A stale revision is refused and the same revision applied twice converges, so a redelivery is a no-op. A command that retrying cannot fix is nacked and the broker dead letters it after delivery_limit attempts.

The second commit is unrelated and reads on its own. Extend() returned nothing, so the renewal goroutine of a long operation could not tell a renewed lease from a lost one: it kept renewing a lease Redis no longer held, and it outlived Unlock. It now returns an error and the goroutine stops on the first failure. The two callers, the RAG workspace and the VFS migration, do not observe the lease themselves, so for them this is a goroutine that stops instead of one that lies.

Worth knowing

Tested locally against a real broker: materialize inline and modal, clear, a stale revision refused, and malformed and reserved-category commands dead lettered. TestHandlers/InstallApp fails, but it fails identically on master.

One gap is open and I do not think it blocks: a commanded banner does not re-localize when the user changes their language, because we keep only the locale picked when the command arrived. The backend sends the full set, we drop the rest. Follow-up.

The old stack.billing.lifecycle queue and its dead letter queue stay declared on the broker. That is a deployment cleanup, not a code change.

rezk2ll · 2026-09-10T12:44:56Z

 msgstr "You have reached your storage limit."
-
-
-msgid "Banners Billing Restricted Text"


Why did you remove the translations here?

The idea is that the stack knows the instance language and it will materialize the banner using that instance language.

we should just keep that system, but the events and calculations are done in cloudery

The stack doesn't choose billing wording anymore, so those entries would never be read again. What we did lose is re-localization, and that's a separate thing: fixed in 36d95f94b, where the private command document keeps every locale the Cloudery sends and the refresh hook picks one again.

rezk2ll · 2026-09-10T12:45:32Z

too much text here, and it feels we are just repeating what the ADR said

Trimmed in 49db1bf

rezk2ll · 2026-09-10T12:51:28Z

+		Title:        cmd.Title[locale],
+		Text:         cmd.Text[locale],
+		Lang:         locale,


A banner made from a command is stored in one language only. The Cloudery sends
every language it has, but here we pick one and throw the others away.

This breaks something ADR 054 decided. The trigger table has a row for it:

| Instance locale change | Every document for that instance, re-localized |

and the Translation section spells out the mechanism: "a language change
re-materializes the documents, which clients pick up on their next refetch".

The stack already does exactly that. Instance patching flags a banner refresh
when the locale changes, and the refresh hook runs. That hook is only wired to
the quota rule, so quota banners get reworded and command banners do not.

I tested this locally. An instance in French with both kinds of banner, then one
language change to English: the storage warning became English, the payment
warning stayed French and still declared lang: "fr". The user sees two banners
in two languages, side by side.

The fix does not need the deleted translation files, and does not need the
Cloudery to resend anything. The command already carried the English text, we
just dropped it. Keep the full set of languages on the private command document
and let the existing refresh hook pick again.

Worth separating two things here. Moving the wording to the Cloudery is right ( in a sense ),
and it removes a cost the ADR accepted: that new wording and new locales need a
stack release ( and it's fine ). Dropping re-localization is a different change, and those two
were separable. If we do want to change the decision rather than fix the code,
that needs an ADR amendment, not a paragraph in docs/banners.md.

Fixed in 36d95f94b. The private command document now keeps every locale, and the existing refresh hook picks the language again when the instance changes it. I kept the whole command rather than only the wording, so we never have to rebuild a banner from the public document that apps can write.

rezk2ll · 2026-09-10T12:58:25Z

+// ErrInvalidCommand marks a command no retry can fix. A transport rejects such
+// a delivery instead of requeuing it; anything else is a storage failure worth
+// retrying.


This comment says a transport rejects an invalid command instead of putting it
back on the queue. Nothing does that. The queue runner puts every failed message
back, no matter the error, and no code outside the tests ever checks for this
error type.

we need to fix this comment

You're right, nothing checks it. The comment now says the runner nacks every error the same way. Fixed in 16cb17427.

rezk2ll · 2026-09-10T13:01:35Z

+		SecondaryCTA: cmd.SecondaryCTA.pick(locale),
+		Dismissible:  cmd.Dismissible,
+		Priority:     cmd.Priority,
+		StartsAt:     cmd.StartsAt,


startsAt is read from the command here, but it gets thrown away later if the
banner already exists with the same bannerId. endsAt and the text are
applied, startsAt is not.

I tested this locally. I sent a first command with a window in August 2026, then
a second command moving both ends into March 2027. The result was a banner
running from August 2026 to March 2027, which is neither of the two windows I
asked for, and no error was raised.

So the Cloudery extending a window gets half of the change applied. Either honour
an explicit startsAt, or document that the start is locked once an occurrence
exists and a new window needs a new bannerId.

Fixed in de627cb63. A command that sends no startsAt keeps the start the occurrence already had, and one that sends it replaces the stored value. There was a worse case than the one you hit: validation checked the window the command sent, not the one we actually store, so moving a window backwards could store a start after its end.

rezk2ll · 2026-09-10T13:36:01Z

+	// queue is declared with a delivery limit, so the broker dead letters it
+	// after those attempts rather than redelivering it forever.
+	if err := banner.ApplyCommand(cmd); err != nil {
+		return fmt.Errorf("banner.commands: %s revision %d: %w", d.RoutingKey, cmd.Revision, err)


This logs "applied" even when the command was ignored. ApplyCommand returns no error both when it wrote something and when it skipped a stale revision, so the handler cannot tell them apart.

Locally, one command produced both of these lines:

billing: ignoring revision 7, not newer than the stored 50 banner.commands: banner.materialize applied to bob.cozy.example

Since ignoring a stale revision is the normal outcome of at-least-once delivery, this is the line someone will be reading when they are trying to work out why a banner never appeared, and it tells them the opposite. Either return an applied/ignored result, or say "accepted" instead of "applied".

Fixed in 16cb17427. It says accepted now, and the line names the other outcomes that reach it, a stale revision and an instance with banners off.

rezk2ll · 2026-09-10T13:36:57Z

+	// A command the queue cannot fix is nacked like any other failure. The
+	// queue is declared with a delivery limit, so the broker dead letters it
+	// after those attempts rather than redelivering it forever.


This states a deployment setting as a fact from inside the code. It is true of the example config, but not of any deployment that leaves out delivery_limit, and the code has no way to know. The rabbitmq doc is the right place for it and already says it.

Fixed in 16cb17427. The comment now leaves the delivery limit to the rabbitmq doc.

rezk2ll · 2026-09-10T13:37:47Z

+	target := cmd.WorkplaceFqdn
+	if target == "" {
+		target = cmd.Domain
 	}


Four lines to build one log value. Exactly one of the two fields is set, so concatenating them works, and that is the idiom the command file already uses at line 295.

Fixed in 16cb17427.

rezk2ll · 2026-09-10T13:39:01Z

 	if err := l.lock.Lock(); err != nil {
 		return err
 	}
-	l.tick = time.NewTicker(l.timeout / 3)
+	l.mu.Lock()
+	done := make(chan struct{})
+	l.done = done
+	l.mu.Unlock()
 	go func() {
-		defer l.mu.Unlock()
+		tick := time.NewTicker(l.timeout / 3)
+		defer tick.Stop()
 		for {
-			l.mu.Lock()
-			if l.tick == nil {
-				return
-			}
-			ch := l.tick.C
-			l.mu.Unlock()
-			<-ch
-			l.mu.Lock()
-			if l.tick == nil {
+			select {
+			case <-done:
 				return
+			case <-tick.C:
+				// A lease that cannot be renewed is not going to start
+				// renewing again, so the goroutine stops rather than logging
+				// the same failure every tick until Unlock.
+				if err := l.lock.Extend(); err != nil {
+					logger.WithNamespace("lock").
+						Warnf("cannot extend a long operation lease: %s", err)
+					return
+				}
 			}
-			l.lock.Extend()
-			l.mu.Unlock()
 		}
 	}()
 	return nil
 }



The fix is right and the two new tests cover both halves. Worth saying out loud that the renewer stopping does not stop the operation. The VFS migration never checks its lease, so it keeps running while the lock expires and another process can take it. That was already true before this change, and the new warning is the first signal it has ever had.

Moved to #4911 and written into its description. Neither caller watches its lease, so the operation keeps running while the lock expires. That was already true before the change, the new warning is just the first time anything says so.

rezk2ll · 2026-09-10T13:39:17Z

+// ErrLockLost means that an operation no longer owns its distributed lock.
+var ErrLockLost = errors.New("lock ownership lost")


ErrLockLost is exported but nothing outside this package ever checks for it. Could stay unexported until something needs it.

Unexported in #4911.

shepilov · 2026-09-10T15:01:38Z

+
+	// Exactly one of Domain and WorkplaceFqdn is set. Domain addresses a
+	// B2B organization, and every instance under it gets the banner.
+	Domain        string `json:"domain,omitempty"`


can we use here tenant(org id) instead of domain?

Fixed in 861eb4f: organization commands now use tenant (org ID) with ListOrgInstancesByID, with updated validation, fixtures, documentation, and passing banner, RabbitMQ, and API tests.

shepilov · 2026-09-10T15:02:33Z

+	}
+	for _, inst := range instances {
+		if inst.HasBannersEnabled() && !inst.AllowsBannerCategory(cmd.Category) {
+			return fmt.Errorf("%w: the context of %s does not accept commands for the %s category",


should we just skip this instance and not return error for others?

Updated in 2328c50, now we skip instances whose context disallows the category and continue processing eligible instances, while still returning storage errors for retries.

but for errors as well, if we had an error for one instance, why should we stop processing others?

Agreed, done in a0811b1. We now try every member and return all the failures together so the delivery is retried, and the test checks that a member after the failing ones still gets its banner.

shepilov · 2026-09-10T15:13:49Z

+	}
+	ctx := context.Background()
+	if _, err := couchdb.CheckStatus(ctx); err == nil {
+		if err := couchdb.InitGlobalDB(ctx); err != nil {


mmmm, why do we need this init here?

It brings up the global database, which the command tests need because they create instances. It's spelled out here instead of calling our shared test helper because importing tests/testutils from inside package banner is an import cycle: testutils pulls in model/stack, which pulls in pkg/rabbitmq, which imports model/banner. That was already true before this PR, pkg/rabbitmq imported model/banner on master too.

that's why it's better then to put test in test package, actually to banner_test package

Done in 3a93280. The command tests moved to banner_test and use the shared testutils setup, so the inlined TestMain is gone.

shepilov · 2026-09-10T15:19:47Z

+
+// commandState records the last command accepted for a category, so ordering
+// survives clears (which leave no public document) and unchanged decisions.
+type commandState struct {


Why do we need a new doc type? Why banners isn't enough?

Because io.cozy.banners isn't in the permission blocklist and io.cozy.banners.commands is. An app needs write access to the banners doctype to record a dismissal, so if the revision lived there the same app could rewind it and we'd accept a stale command. A clear also deletes the public document, which would leave nothing to compare the next revision against.

Then we shouldn't delete the public document, or whatever. To have a permssion blacklist as a separate type is kind of everkill. There is multiple variants how you can implement it, to have multiple banners of the same catergoy and the filter/group on get query or just let the app do anything it wants as we have actually for files. Could you please describe the use case: when do we need this "blacklist", I just don't understand what problem we can have with this "dismissal" record. If it's ABA problem, then just, ok, let's record timestamp or optimistiv locking on a revision

You're right, the separate doctype was overkill, so we dropped it in 2816d56. The revision and the accepted command now live on the banner document itself, a clear expires that document instead of deleting it, and dismissals go through the normal _rev.

…nd intake The stack was reading Stripe subscription statuses and attempt counts and deciding from them what to tell the user. That is billing logic, and it does not belong here: the stack should write banners, not decide them. billing.go, the BillingLifecycleMessage contract, its handler, its queue and the four billing translations are gone. In their place, a backend publishes a banner.materialize or banner.clear command carrying the wording it wants, and the stack validates it and stores it. What the stack still decides: - The category must be allowed for the instance context, through banner_command_categories. The quota category is always refused, because the stack measures disk usage itself. - The routing key decides materialize versus clear, never the payload. - The CTA must be an absolute https URL, the payload must fit MaxCommandBytes, and the bannerId must match the doctype's format. Ordering no longer rides on the visible document. A command carries a revision, and the last accepted one per category is kept in io.cozy.banners.commands, blocklisted so no application can touch it. An app with dismissal rights on io.cozy.banners could otherwise rewind the ordering by rewriting the document it is allowed to write. Delivery is at-least-once and unordered: a stale revision is refused and the same revision applied twice converges, so a redelivery is a no-op. A command that cannot be fixed by retrying is nacked and the broker dead letters it after delivery_limit attempts. docs/banners.md describes the contract for the producing side.

The shared wire fixtures were decoded by a helper that panicked on a read or unmarshal error, so a broken fixture reported a stack trace rather than the test that wanted it. It takes t now, marks itself a helper and uses require.NoError, as asked in review.

A commanded banner was stored in one language and never revisited, so an instance that switched language ended up with the quota banner reworded and the billing banner still in the old one, side by side, as the review reported. The private io.cozy.banners.commands record now keeps the accepted command whole, with every locale the backend sent, and the existing RefreshBanners hook picks a language again from it. Reconstructing severity, surface, occurrence id and the action URLs from the public document instead would mean trusting fields an application can rewrite, which is the reason that doctype is private. Refresh takes the same instance lock a command takes, so it cannot race a newer command into restoring what that command replaced. It moves no revision, since re-picking a language is not a decision, and it keeps a dismissal because the occurrence id is unchanged. A cleared category, a record written before the stack retained the wording, and a category the context no longer accepts are all left alone. Re-localizing rewrites a banner rather than restoring one: a category whose document is gone stays gone until the next command. Recreating it from the retained command alone would move a scheduled banner to the decision time of whichever command wrote last.

An explicit startsAt was accepted and then discarded whenever a banner already existed with the same bannerId, while endsAt was applied, so a backend moving its window got half the change: a first command running August 2026 followed by one moving both ends into March 2027 left a banner running August 2026 to March 2027, which is neither window and raised no error. A nil StartsAt now means the producer stated no window, and the moment the occurrence began is carried forward. A producer that states one owns it. Both producers had to stop stating a default for that rule to hold. The quota rule no longer assigns StartsAt on every evaluation, and a command no longer defaults it to the decision time, so Materialize fills a nil start from the producer's own decision: the evaluation time for a rule, the command timestamp for a command. Quota's stored documents are unchanged, since the value it used to assign itself is now the value Materialize fills from the same clock reading. Validation cannot see the stored occurrence, so it no longer substitutes the decision time for an omitted start. It judges a window only when the command states both ends, which stops it refusing an end the backend is entitled to move on its own. Carrying a start forward is skipped when it would land after an end the producer just stated, so a shortened window cannot invert.

ErrInvalidCommand claimed that a transport rejects such a delivery instead of requeuing it. Nothing does: the queue runner nacks every handler error alike, and no code outside the tests inspects the error. The comment now says what happens, and keeps the classification for a transport that learns to reject. The handler logged a command as applied even when ApplyCommand had ignored it, because a stale revision and a successful write both return nil. Since ignoring a stale revision is the normal outcome of at-least-once delivery, that line was telling whoever was working out why a banner never appeared the opposite of what happened. It says accepted now, and names the other outcomes that reach it. Also drops two comments that restated the code below them, and builds the log target by concatenation, which validation makes safe and which the command file already does.

A missing workplace was described as retried to give an instance still being provisioned time to appear. Nothing in the path delays a redelivery, so the attempts are consumed as fast as the consumer loops and the command reaches the dead letter queue without waiting for anything. It says so now, and says to repair the instance and replay. The delivery limit bounds deliveries rather than retries, which is why a limit of five runs the handler six times on the version the test fixture pins. That number is attributed rather than promised: how a redelivery is counted has changed between RabbitMQ releases, and this stack requeues with basic.nack, so a deployment should confirm it against its own broker.

rezk2ll

still two things missing:

a disallowed category is now silently swallowed
no CTA allowlist yet
the Revision collision issue

rezk2ll · 2026-09-11T13:38:21Z

+}
+
+func (cmd Command) applyTo(inst *instance.Instance) error {
+	// Instances that disable banners or disallow this category are skipped.


But the single-instance path now has no signal at all. Nothing written, nothing dead-lettered, and the only line an operator sees is the handler's, which says the command was accepted:

banner.commands: banner.materialize accepted for dave.cozy.example (category billing, revision 9, event skip1)

Right, fixed in a27af44. A skipped instance now logs a warning with the reason, either the category or the CTA host, and its revision doesn't move.

An instance skipped for a disallowed category or CTA host now logs a warning with the reason. CTA URLs must use a host listed in the context's banner_cta_hosts, checked on apply and on refresh. The docs state that the revision is kept per instance and category, shared by tenant and workplace commands.

shepilov · 2026-09-14T15:33:14Z

+
+	// Exactly one of Tenant and WorkplaceFqdn is set. Tenant is the B2B
+	// organization ID, and every instance under it gets the banner.
+	Tenant        string `json:"tenant,omitempty"`


shepilov · 2026-09-14T15:34:50Z

+    # this context. Off by default, so the rules can ship before the clients
+    # that render them. Turning it back off stops the writes and leaves the
+    # documents already materialized in place.
+    enable_banners: true


group to the banner: and one struct

shepilov · 2026-09-14T15:35:35Z

+      - billing
+      - trial
+    # The hosts a banner command's call to action may link to.
+    banner_cta_hosts:


how I can enable everything?

MontaGhanmy force-pushed the ref/banner-billing-sanatize branch from d09cabb to c275d9b Compare September 10, 2026 11:33

rezk2ll reviewed Sep 10, 2026

View reviewed changes

shepilov reviewed Sep 10, 2026

View reviewed changes

Comment thread pkg/lock/lock.go Outdated

shepilov reviewed Sep 10, 2026

View reviewed changes

Comment thread model/banner/command_test.go Outdated

shepilov reviewed Sep 10, 2026

View reviewed changes

MontaGhanmy added 6 commits September 11, 2026 10:22

MontaGhanmy force-pushed the ref/banner-billing-sanatize branch from c275d9b to 515f6c5 Compare September 11, 2026 09:27

MontaGhanmy mentioned this pull request Sep 11, 2026

fix(lock): stop renewing a lease that cannot be extended #4911

Open

ref(docs): trim banners.md, remove redundant information

49db1bf

MontaGhanmy force-pushed the ref/banner-billing-sanatize branch from dc52450 to 49db1bf Compare September 11, 2026 10:34

MontaGhanmy added 2 commits September 11, 2026 11:56

fix(banners): target organizations by tenant ID

861eb4f

fix(banners): skip instances that disallow command categories

2328c50

MontaGhanmy requested review from rezk2ll and shepilov September 11, 2026 11:10

rezk2ll reviewed Sep 11, 2026

View reviewed changes

MontaGhanmy added 6 commits September 14, 2026 12:30

fix: update banner documents to retain command state

2816d56

fix: update banner integration tests to use shared setup

3a93280

fix: update banner fanout to continue after instance errors

a0811b1

fix: update banner test whitespace for lint

f5cdc7f

fix: update MinIO test image registry

c31b2b3

shepilov reviewed Sep 14, 2026

View reviewed changes

MontaGhanmy added 2 commits September 14, 2026 17:00

refactor(banners): rename the organization target to orgId

2b20252

refactor(banners): group banner settings and allow a category wildcard

3d0c7d6

shepilov approved these changes Sep 14, 2026

View reviewed changes

MontaGhanmy merged commit f4d9f59 into master Sep 14, 2026
4 checks passed

MontaGhanmy deleted the ref/banner-billing-sanatize branch September 14, 2026 16:38

		msgstr "You have reached your storage limit."


		msgid "Banners Billing Restricted Text"

		// ErrLockLost means that an operation no longer owns its distributed lock.
		var ErrLockLost = errors.New("lock ownership lost")

Uh oh!

Conversation

MontaGhanmy commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Solution

Worth knowing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MontaGhanmy commented Sep 10, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading

MontaGhanmy Sep 11, 2026 •

edited

Loading