refactor(core): dedup middlewareContainer by per-instance Key() not type (#672) (#697)

## Summary

\`core.middlewareContainer.Use()\` previously deduped by
\`reflect.TypeOf(m).String()\`, silently dropping any second instance of
the same Go type. This was the structural root cause of
[#670](#670) (multiple
per-job webhooks collapsing into one), worked around at the webhook
layer in PR [#671](#671) with
the \`WebhookMiddleware\` composite. The composite stays — removing it
is a separate PR per the [#672 scope
guidance](#672) — but the
structural bug is now closed at the core layer.

Adds an OPTIONAL \`keyedMiddleware\` interface (\`Key() string\`).
\`Use()\` computes the dedup key via type assertion: implementer returns
a non-empty string → use it; otherwise fall back to
\`reflect.TypeOf(m).String()\`.

Closes [#672](#672).

## Design choice: opt-in via type assertion (not interface contract
change)

Considered adding \`Key()\` to the \`Middleware\` interface; rejected
because every implementer (in-repo AND downstream) would need a stub
method providing no value for the common 1-per-type case. The
assertion-based opt-in keeps the contract small and lets the type-string
fallback do the work it's always done.

**Implementer matrix:**
- \`*middlewares.Webhook\` → \`Key() = Config.Name\` (multiple per job,
distinct names coexist)
- \`Slack\`, \`Mail\`, \`Save\`, \`Overlap\`, \`WebhookMiddleware\` → no
\`Key()\` method (legacy type dedup preserved)
- Downstream custom middleware → no changes required

## Why pointer-identity dedup was rejected

The [#672 issue
body](#672 (comment))
listed pointer-identity as Option 3. Rejected because scheduler-to-job
propagation at
\`core/scheduler.go:315\`/\`:631\`/\`config_webhook.go:289\` calls
\`j.Use(s.Middlewares()...)\`: with pointer-identity dedup, a
scheduler-level Slack middleware would coexist with a job-level Slack
middleware of the same type (different pointers), causing every Slack
notification to fire twice. \`Key()\`-based dedup with type-string
fallback preserves the existing "first wins per type" semantics for
Slack/Mail/Save/Overlap and enables multi-Webhook by construction.

## Tests (TDD)

Six new test functions in \`core/middleware_container_test.go\` (plus
\`TestWebhook_Key\` in \`middlewares/webhook_test.go\`):

- \`TestMiddlewareContainer_Use_DistinctKeysKeepsBoth\` — regression
test for #670 at the core layer. Confirmed failing on pre-fix code.
- \`TestMiddlewareContainer_Use_SameKeyDedups\` — same-name propagation
contract.
- \`TestMiddlewareContainer_Use_UnkeyedFallsBackToType\` —
Slack/Mail/Save/Overlap semantics preserved.
- \`TestMiddlewareContainer_Use_KeyedAndUnkeyedCoexist\` — key-namespace
isolation.
- \`TestMiddlewareContainer_Use_EmptyKeyFallsBackToType\` — explicit
opt-out via \`Key() returning ""\` gets legacy dedup (used by
\`WebhookMiddleware\` composite if it ever needs an explicit opt-out).
- \`TestWebhook_Key\` — pins \`Webhook.Key() == Config.Name\`.

## Out of scope (follow-up PRs)

- Removing the \`WebhookMiddleware\` composite from PR #671 (the
workaround is now redundant for dedup but the composite still serves the
\`ContinueOnStop()\` execution-ordering contract — needs its own
analysis).
- Per-instance dedup for any future N-instance middleware (Slack
channels, Mail recipients, etc.) — just implement \`Key() string\`
returning the operator-supplied name.

## Test plan

- [x] \`go test ./...\` passes (full repo, 14 packages, ~58s)
- [x] \`golangci-lint run\` clean
- [x] \`go vet ./...\` clean
- [ ] CI green

## References

- Structural root cause of:
[#670](#670) (fixed at
webhook layer by [#671](#671))
- Tracks: [#672](#672)

Author: CybotTM

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - `PresetLoader.AddLocalPresetDir` now scans the registered directory for `*.yaml` files whose stem collides with a bundled preset name (`slack`, `discord`, `teams`, `matrix`, `ntfy`, `ntfy-token`, `pushover`, `pagerduty`, `gotify`, `json-post`) and emits a startup `slog.Warn` per collision. Pre-fix, `PresetLoader.Load` resolved bundled presets first and never fell through, so a file at `$LOCAL_DIR/json-post.yaml` placed hoping to override the bundled `json-post` was silently ignored at attach time. The warning matches `Load`'s `.yaml`-only resolution path so a `.yml` rename suggestion never misleads operators. The lookup order is documented in `docs/webhooks.md` under "Preset Lookup Order"; inverting the order to prefer local files is deliberately rejected (a local typo shadowing `slack.yaml` would silently break Slack delivery host-wide). Closes [#679](https://github.com/netresearch/ofelia/issues/679).
 
+- `core.middlewareContainer.Use()` now dedups per-instance via an optional `Key() string` interface instead of by `reflect.TypeOf(m).String()`. Pre-fix, two `*middlewares.Webhook` instances handed to the same job collapsed into the first and silently dropped the rest — the failure mode tracked in [#670](https://github.com/netresearch/ofelia/issues/670) and worked around at the webhook layer by PR [#671](https://github.com/netresearch/ofelia/pull/671) with the `WebhookMiddleware` composite. The composite stays in place (no behavior change there), but the structural bug is now closed at the core layer so any future N-instance middleware (e.g. multiple Slack channels, multiple Mail recipient sets) gets correct semantics by construction. Existing 1-per-type middlewares (`Slack`, `Mail`, `Save`, `Overlap`, `WebhookMiddleware`) do not implement `Key()`, so they fall back to the legacy type-string dedup — `j.Use(s.Middlewares()...)` scheduler-to-job propagation still de-duplicates them as before. `*middlewares.Webhook` returns `Config.Name` from `Key()`, so the same-name propagation case (scheduler-level webhook re-propagated to a job that already has it) still de-duplicates correctly. Opt-in via type assertion means downstream consumers with custom middleware implementations need no changes. Closes [#672](https://github.com/netresearch/ofelia/issues/672).
+
 ### Removed
 
 - **BREAKING (source-only, pre-1.0):** Removed unused `core/adapters/docker.ClientConfig.HTTPClient` field that was declared in [#681](https://github.com/netresearch/ofelia/pull/681) but never read — a caller setting `cfg.HTTPClient = someClient` silently got the auto-constructed transport instead of theirs. Downstream Go consumers that referenced the field in named struct literals or assignments will see a compile-time error after upgrade (semantically a no-op since the field was already ignored at runtime); permitted under SemVer for the current 0.y.z line (cf. [SemVer §4](https://semver.org/#spec-item-4)). Removing the field turns the silent footgun into a loud compile-time error rather than preserving it as a deprecated no-op. If you need a transport-level injection seam, file a feature request with the use case so the suppression of `disableHTTP2AutoConfig` on caller-supplied transports (the [#668](https://github.com/netresearch/ofelia/issues/668) invariant) can be wired in correctly. ([#693](https://github.com/netresearch/ofelia/pull/693), closes [#684](https://github.com/netresearch/ofelia/issues/684))

core/common.go

@@ -295,6 +295,53 @@ type Middleware interface {
 	ContinueOnStop() bool
 }
 
+// keyedMiddleware is an OPTIONAL interface that middleware implementations
+// can satisfy to opt into per-instance dedup keys in middlewareContainer.
+// Implementers that legitimately need multiple instances per job (e.g.
+// multiple Webhook destinations with distinct Config.Name) return a stable,
+// non-empty key per instance — typically the operator-supplied name from
+// configuration. Returning "" or not implementing Key() at all falls back
+// to the legacy reflect.TypeOf(m).String() dedup, preserving the
+// 1-per-type semantics of Slack, Mail, Save, Overlap, and the
+// WebhookMiddleware composite from PR #671.
+//
+// Why opt-in via type assertion (no interface contract change): adding
+// Key() to the Middleware interface would force every implementer (in
+// this repo and any downstream consumer) to add a stub method, providing
+// no value for the common 1-per-type case. The assertion-based opt-in
+// keeps the contract small and lets the type-string fallback do the work
+// it has always done.
+//
+// Tracks the structural fix for
+// https://github.com/netresearch/ofelia/issues/672; sibling to #670 /
+// PR #671 which worked around the multi-webhook case at the webhook layer.
+type keyedMiddleware interface {
+	Key() string
+}
+
+// middlewareKey computes the dedup key for a middleware. Implementers of
+// keyedMiddleware that return a non-empty string get that string as the
+// key; everyone else (including keyedMiddleware implementers that
+// explicitly return "") falls back to reflect.TypeOf(m).String().
+func middlewareKey(m Middleware) string {
+	if k, ok := m.(keyedMiddleware); ok {
+		if key := k.Key(); key != "" {
+			return key
+		}
+	}
+	return reflect.TypeOf(m).String()
+}
+
+// middlewareContainer holds a job's (or scheduler's) middleware chain with
+// per-key dedup so the same logical middleware is not invoked twice when
+// scheduler-level middlewares are propagated to a job via
+// j.Use(s.Middlewares()...). See middlewareKey for the dedup-key strategy.
+//
+// Not safe for concurrent use; callers must serialize. In practice
+// (*Scheduler).Use happens under Scheduler.mu, and (*BareJob).Use is
+// invoked from config-parse paths and from AddJob before the job is
+// published to the cron runner — there is no in-flight reader during
+// mutation.
 type middlewareContainer struct {
 	m     map[string]Middleware
 	order []string
@@ -310,13 +357,13 @@ func (c *middlewareContainer) Use(ms ...Middleware) {
 			continue
 		}
 
-		t := reflect.TypeOf(m).String()
-		if _, ok := c.m[t]; ok {
+		k := middlewareKey(m)
+		if _, ok := c.m[k]; ok {
 			continue
 		}
 
-		c.order = append(c.order, t)
-		c.m[t] = m
+		c.order = append(c.order, k)
+		c.m[k] = m
 	}
 }
 

core/middleware_container_test.go

@@ -0,0 +1,278 @@
+// Copyright (c) 2025-2026 Netresearch DTT GmbH
+// SPDX-License-Identifier: MIT
+
+package core
+
+import (
+	"testing"
+)
+
+// keyedFakeMiddleware exercises the Key()-based dedup path: two distinct
+// instances of the same Go type can coexist in a middlewareContainer when
+// they declare distinct keys. Mirrors the *middlewares.Webhook contract
+// from #670 / #672 at the core layer, without depending on the middlewares
+// package (which would create an import cycle).
+type keyedFakeMiddleware struct {
+	key string
+}
+
+func (k *keyedFakeMiddleware) Run(_ *Context) error { return nil }
+func (k *keyedFakeMiddleware) ContinueOnStop() bool { return false }
+func (k *keyedFakeMiddleware) Key() string          { return k.key }
+
+// unkeyedFakeMiddleware exercises the legacy type-string fallback path.
+// It does NOT implement Key(); middlewareContainer.Use must then dedup by
+// reflect.TypeOf(m).String() so the existing 1-per-type middlewares
+// (Slack, Mail, Save, Overlap) keep their current semantics.
+type unkeyedFakeMiddleware struct {
+	name string
+}
+
+func (u *unkeyedFakeMiddleware) Run(_ *Context) error { return nil }
+func (u *unkeyedFakeMiddleware) ContinueOnStop() bool { return false }
+
+// otherUnkeyedFakeMiddleware is a SECOND legacy-style middleware type used
+// to discriminate "empty-key fallback ran" from "fallback always returns
+// the same string here". With two distinct Go types, the type-string
+// fallback returns two distinct keys and they coexist — which is the
+// observation that kills the `if key != ""` ↔ `if key == ""` mutation.
+type otherUnkeyedFakeMiddleware struct{}
+
+func (o *otherUnkeyedFakeMiddleware) Run(_ *Context) error { return nil }
+func (o *otherUnkeyedFakeMiddleware) ContinueOnStop() bool { return false }
+
+// emptyKeyedFake implements Middleware + Key() returning "". Used to lock
+// the opt-out path: a middleware that explicitly declines a custom key
+// gets the legacy type-string dedup, NOT a distinct empty-string-keyed
+// slot. Mirrors what any keyed-by-default middleware can do if it wants
+// to defer per-instance dedup to a wrapper / composite type.
+type emptyKeyedFake struct{}
+
+func (e *emptyKeyedFake) Run(_ *Context) error { return nil }
+func (e *emptyKeyedFake) ContinueOnStop() bool { return false }
+func (e *emptyKeyedFake) Key() string          { return "" }
+
+// TestMiddlewareContainer_Use_DistinctKeysKeepsBoth pins the fix for
+// https://github.com/netresearch/ofelia/issues/672 at the core layer.
+// Pre-fix, middlewareContainer.Use deduped by reflect.TypeOf(m).String(),
+// silently dropping any second instance of the same Go type. PR #671
+// worked around this at the webhook layer with a composite type; the
+// structural fix is here.
+//
+// Two keyedFakeMiddleware instances with distinct keys ("alpha", "beta")
+// must both end up in Middlewares() in insertion order.
+func TestMiddlewareContainer_Use_DistinctKeysKeepsBoth(t *testing.T) {
+	t.Parallel()
+
+	var c middlewareContainer
+	a := &keyedFakeMiddleware{key: "alpha"}
+	b := &keyedFakeMiddleware{key: "beta"}
+	c.Use(a, b)
+
+	got := c.Middlewares()
+	if len(got) != 2 {
+		t.Fatalf("Middlewares() len = %d, want 2 (distinct keys must coexist)", len(got))
+	}
+	if got[0] != a {
+		t.Errorf("Middlewares()[0] = %p, want %p (alpha; insertion order)", got[0], a)
+	}
+	if got[1] != b {
+		t.Errorf("Middlewares()[1] = %p, want %p (beta; insertion order)", got[1], b)
+	}
+}
+
+// TestMiddlewareContainer_Use_SameKeyDedups pins that adding two distinct
+// instances with the SAME key still dedups to the first (the existing
+// "first wins" semantics from the type-string-keyed dedup). Without this
+// guarantee, scheduler-level webhook propagation via
+// j.Use(s.Middlewares()...) would double-fire when the job already has a
+// webhook with the same Config.Name.
+func TestMiddlewareContainer_Use_SameKeyDedups(t *testing.T) {
+	t.Parallel()
+
+	var c middlewareContainer
+	a := &keyedFakeMiddleware{key: "shared"}
+	b := &keyedFakeMiddleware{key: "shared"}
+	c.Use(a, b)
+
+	got := c.Middlewares()
+	if len(got) != 1 {
+		t.Fatalf("Middlewares() len = %d, want 1 (same key must dedup)", len(got))
+	}
+	if got[0] != a {
+		t.Errorf("Middlewares()[0] = %p, want %p (first wins)", got[0], a)
+	}
+}
+
+// TestMiddlewareContainer_Use_UnkeyedFallsBackToType pins the legacy
+// behavior for middlewares that do not implement Key(): two distinct
+// instances of the same Go type collapse into the first. This preserves
+// the Slack/Mail/Save/Overlap "1-per-type" semantics so j.Use(s.Middlewares()...)
+// propagation does not silently double-send notifications.
+func TestMiddlewareContainer_Use_UnkeyedFallsBackToType(t *testing.T) {
+	t.Parallel()
+
+	var c middlewareContainer
+	a := &unkeyedFakeMiddleware{name: "first"}
+	b := &unkeyedFakeMiddleware{name: "second"}
+	c.Use(a, b)
+
+	got := c.Middlewares()
+	if len(got) != 1 {
+		t.Fatalf("Middlewares() len = %d, want 1 (unkeyed same-type must dedup via reflect.TypeOf)", len(got))
+	}
+	if got[0] != a {
+		t.Errorf("Middlewares()[0] = %p, want %p (first wins on type-fallback dedup)", got[0], a)
+	}
+}
+
+// TestMiddlewareContainer_Use_KeyedAndUnkeyedCoexist pins that a keyed
+// middleware and an unkeyed one share no namespace: the keyed middleware
+// is indexed under its Key() string, the unkeyed one under its
+// reflect.TypeOf string. They cannot collide by construction.
+func TestMiddlewareContainer_Use_KeyedAndUnkeyedCoexist(t *testing.T) {
+	t.Parallel()
+
+	var c middlewareContainer
+	k := &keyedFakeMiddleware{key: "alpha"}
+	u := &unkeyedFakeMiddleware{name: "first"}
+	c.Use(k, u)
+
+	got := c.Middlewares()
+	if len(got) != 2 {
+		t.Fatalf("Middlewares() len = %d, want 2 (different key namespaces must coexist)", len(got))
+	}
+}
+
+// TestMiddlewareContainer_Use_EmptyKeyFallsBackToType pins that a
+// middleware that implements Key() but returns the empty string is
+// treated the same as an unkeyed middleware (type-string fallback). The
+// WebhookMiddleware composite added in PR #671 uses this opt-out: it
+// implements Key() returning "" so it stays 1-per-job (the composite
+// wraps multiple webhooks internally).
+func TestMiddlewareContainer_Use_EmptyKeyFallsBackToType(t *testing.T) {
+	t.Parallel()
+
+	a := &emptyKeyedFake{}
+	b := &emptyKeyedFake{}
+
+	var c middlewareContainer
+	c.Use(a, b)
+
+	got := c.Middlewares()
+	if len(got) != 1 {
+		t.Fatalf("Middlewares() len = %d, want 1 (empty Key() must fall back to type dedup)", len(got))
+	}
+	if got[0] != a {
+		t.Errorf("Middlewares()[0] = %p, want %p (first wins on empty-key type fallback)", got[0], a)
+	}
+}
+
+// TestMiddlewareContainer_Use_EmptyKeyMixWithUnkeyed strengthens the
+// empty-key fallback assertion: an `emptyKeyedFake` (Key() == "") and an
+// `unkeyedFakeMiddleware` of a different type must coexist. They cannot
+// collapse because their reflect.TypeOf strings are distinct. This is
+// the discriminator that kills the `if key != ""` ↔ `if key == ""`
+// mutation cleanly — the prior _EmptyKeyFallsBackToType test used two
+// instances of the SAME type and would have passed under either branch.
+func TestMiddlewareContainer_Use_EmptyKeyMixWithUnkeyed(t *testing.T) {
+	t.Parallel()
+
+	a := &emptyKeyedFake{}
+	b := &otherUnkeyedFakeMiddleware{}
+
+	var c middlewareContainer
+	c.Use(a, b)
+
+	got := c.Middlewares()
+	if len(got) != 2 {
+		t.Fatalf("Middlewares() len = %d, want 2 (empty-key fallback + unkeyed of different type must coexist via distinct type strings)", len(got))
+	}
+}
+
+// TestMiddlewareContainer_Use_InsertionOrderWithDuplicate pins that the
+// order slice tracks unique keys only — adding [a-key, b-key, a-dup-key]
+// must produce [a, b] in insertion order, with a-dup-key skipped from
+// BOTH c.m and c.order. A regression that appended the duplicate to
+// c.order while skipping the map update would yield 3 entries from
+// Middlewares() (the third lookup hitting the wrong map entry).
+func TestMiddlewareContainer_Use_InsertionOrderWithDuplicate(t *testing.T) {
+	t.Parallel()
+
+	var c middlewareContainer
+	a := &keyedFakeMiddleware{key: "alpha"}
+	b := &keyedFakeMiddleware{key: "beta"}
+	aDup := &keyedFakeMiddleware{key: "alpha"}
+	c.Use(a, b, aDup)
+
+	got := c.Middlewares()
+	if len(got) != 2 {
+		t.Fatalf("Middlewares() len = %d, want 2 (duplicate key must not append to order slice)", len(got))
+	}
+	if got[0] != a {
+		t.Errorf("Middlewares()[0] = %p, want %p (alpha first-wins)", got[0], a)
+	}
+	if got[1] != b {
+		t.Errorf("Middlewares()[1] = %p, want %p (beta in insertion order, not displaced by aDup)", got[1], b)
+	}
+}
+
+// TestMiddlewareContainer_Use_SkipsNil pins that nil middlewares are
+// silently dropped — a defensive guard that has lived in Use since the
+// pre-#672 implementation. Without this, a caller building a slice via
+// `var ms []Middleware; ms = append(ms, NewWebhook(nil, l))` (where
+// NewWebhook returns (nil, nil) for a nil config) would panic at the
+// type assertion inside middlewareKey. Closes the coverage hole flagged
+// by the test-effectiveness review on #697.
+func TestMiddlewareContainer_Use_SkipsNil(t *testing.T) {
+	t.Parallel()
+
+	var c middlewareContainer
+	a := &keyedFakeMiddleware{key: "alpha"}
+	c.Use(nil, a, nil)
+
+	got := c.Middlewares()
+	if len(got) != 1 {
+		t.Fatalf("Middlewares() len = %d, want 1 (nil entries must be skipped)", len(got))
+	}
+	if got[0] != a {
+		t.Errorf("Middlewares()[0] = %p, want %p (the only non-nil entry)", got[0], a)
+	}
+}
+
+// TestMiddlewareContainer_ResetMiddlewares_ClearsThenRebuilds pins the
+// behavior of ResetMiddlewares, which is called on every config hot-
+// reload at cli/config.go and cli/config_webhook.go. A regression that
+// clears c.m but not c.order (or vice versa) would leave the container
+// in an inconsistent state where Middlewares() returns dangling nil
+// values. Closes a critical coverage hole flagged by the test-
+// effectiveness review on #697 (ResetMiddlewares had 0% coverage).
+func TestMiddlewareContainer_ResetMiddlewares_ClearsThenRebuilds(t *testing.T) {
+	t.Parallel()
+
+	var c middlewareContainer
+	old := &keyedFakeMiddleware{key: "old"}
+	c.Use(old)
+	if len(c.Middlewares()) != 1 {
+		t.Fatalf("pre-Reset Middlewares() len = %d, want 1", len(c.Middlewares()))
+	}
+
+	// Reset with a fresh middleware set.
+	newA := &keyedFakeMiddleware{key: "new-a"}
+	newB := &keyedFakeMiddleware{key: "new-b"}
+	c.ResetMiddlewares(newA, newB)
+
+	got := c.Middlewares()
+	if len(got) != 2 {
+		t.Fatalf("post-Reset Middlewares() len = %d, want 2 (old entry cleared, two new entries added)", len(got))
+	}
+	if got[0] != newA || got[1] != newB {
+		t.Errorf("post-Reset order = [%p, %p], want [%p, %p]", got[0], got[1], newA, newB)
+	}
+	// Verify old is fully gone — its key must not resolve in the dedup map.
+	c.Use(&keyedFakeMiddleware{key: "old"})
+	got = c.Middlewares()
+	if len(got) != 3 {
+		t.Fatalf("Use(old-key) after Reset Middlewares() len = %d, want 3 (Reset must have cleared the dedup map so the same key inserts fresh)", len(got))
+	}
+}

middlewares/webhook.go

@@ -129,6 +129,25 @@ func (w *Webhook) ContinueOnStop() bool {
 	return true
 }
 
+// Key returns the per-instance dedup key used by core.middlewareContainer
+// so multiple Webhook instances with distinct Config.Name can coexist in a
+// single job's middleware chain. Pre-#672, the container deduped by
+// reflect.TypeOf(m).String() and silently dropped any second *Webhook;
+// PR #671 worked around this with the WebhookMiddleware composite. With
+// the Key() opt-in, two distinct *Webhook instances (e.g. "wh-success"
+// and "wh-error") survive insertion into the chain by themselves.
+//
+// Returning Config.Name (instead of, say, a UUID) preserves the existing
+// "first wins" dedup contract for the scheduler-to-job propagation path
+// at core/scheduler.go:315 / config_webhook.go:289: if the scheduler-level
+// webhook with the same Config.Name is re-propagated to a job that
+// already has it, the second insertion is correctly skipped.
+//
+// See https://github.com/netresearch/ofelia/issues/672.
+func (w *Webhook) Key() string {
+	return w.Config.Name
+}
+
 // Run executes the webhook notification
 func (w *Webhook) Run(ctx *core.Context) error {
 	err := ctx.Next()