external modules: the bell row is keyed on the URL alone, so two refs of one repo share one notification #61

New issue

Open

opened 2026-09-02 13:09:18 +02:00 by julian · 0 comments

julian commented

2026-09-02 13:09:18 +02:00

Owner

Residual from the approving review on #40 — pre-existing, explicitly out of that PR's scope, filed as the follow-up it named.

#40 made (url, ref) first-class in three places: the fault signature (<srcDir>.recovery-fault-<fnv1a(ref)>), the failure record (ModuleFailure.configuredRef), and the retry chain's config lookup. The notification key did not move with them.

externalFailureKey in packages/server/src/modules/alerts.ts is derived from moduleSlug(url), and moduleSlug is deliberately credential-free and ref-free. So two configured entries for one repo —

OPSDECK_EXTERNAL_MODULES=…/mod.git#main,…/mod.git#dev

— write into the same bell row. Consequences, in the order an operator meets them:

#dev failing overwrites the row #main's failure raised, so the bell names one repo and one reason where there were two.
Either entry recovering clears the row, including while the other is still down. /system still shows the failing one, so the two disagree — which is the exact split #40's stage: "retry" handling exists to prevent elsewhere.
The dedupe that keeps a restart loop to one row now also dedupes across refs, which is not the same property.

Not a wedge and nothing is lost on disk: the failure record, the signature and the retry chain are all per-ref already, so recovery behaviour is correct. What is wrong is only what the operator is told.

Why it was not fixed in #40

That PR is seven rounds deep on a destructive-recovery fault, and this is a behaviour change to the alert surface rather than a mechanical rename — it wants a decision and a migration:

The key gains the ref digest, the same way slugify did — but every deployment holding a row under the current key needs the same boot sweep migrateExternalAlertKeys already performs for the pre-digest slug, extended to the ref. That function is the place, and its comment already explains why the migration cannot live in the alert calls.
Or the row stays per-repo and the body carries both refs, which is fewer notifications for the common case (one entry per repo) and no migration at all.

The first matches everything else #40 did; the second is defensible if a repo's refs are meant to read as one module. Whichever, packages/server/tests/external_modules_test.ts has refs that share a work tree do not share a bound as the fixture shape to copy.

Source: #40 (comment) (last non-blocking item).

Residual from the approving review on #40 — pre-existing, explicitly out of that PR's scope, filed as the follow-up it named. #40 made `(url, ref)` first-class in three places: the fault signature (`<srcDir>.recovery-fault-<fnv1a(ref)>`), the failure record (`ModuleFailure.configuredRef`), and the retry chain's config lookup. The notification key did not move with them. `externalFailureKey` in `packages/server/src/modules/alerts.ts` is derived from `moduleSlug(url)`, and `moduleSlug` is deliberately credential-free *and* ref-free. So two configured entries for one repo — ``` OPSDECK_EXTERNAL_MODULES=…/mod.git#main,…/mod.git#dev ``` — write into the same bell row. Consequences, in the order an operator meets them: - `#dev` failing overwrites the row `#main`'s failure raised, so the bell names one repo and one reason where there were two. - Either entry recovering clears the row, including while the other is still down. `/system` still shows the failing one, so the two disagree — which is the exact split #40's `stage: "retry"` handling exists to prevent elsewhere. - The dedupe that keeps a restart loop to one row now also dedupes across refs, which is not the same property. Not a wedge and nothing is lost on disk: the failure record, the signature and the retry chain are all per-ref already, so recovery behaviour is correct. What is wrong is only what the operator is told. ### Why it was not fixed in #40 That PR is seven rounds deep on a destructive-recovery fault, and this is a behaviour change to the alert surface rather than a mechanical rename — it wants a decision and a migration: - The key gains the ref digest, the same way `slugify` did — but every deployment holding a row under the current key needs the same boot sweep `migrateExternalAlertKeys` already performs for the pre-digest slug, extended to the ref. That function is the place, and its comment already explains why the migration cannot live in the alert calls. - Or the row stays per-repo and the *body* carries both refs, which is fewer notifications for the common case (one entry per repo) and no migration at all. The first matches everything else #40 did; the second is defensible if a repo's refs are meant to read as one module. Whichever, `packages/server/tests/external_modules_test.ts` has `refs that share a work tree do not share a bound` as the fixture shape to copy. Source: https://git.imhof.cloud/OpsDeck/core/pulls/40#issuecomment-1229 (last non-blocking item).

No labels

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

OpsDeck/core#61

No description provided.

Rows
Columns