feat(pipeline): Add Finisher lifecycle hook for stateful plugin cleanup#401
Conversation
Introduces pipeline.Finisher, an optional interface that lets a plugin
register a release point paired with its OnRequest acquire — fired
once per request regardless of whether the pipeline allowed, denied,
or errored.
Closes the "cleanup leaks on denial" class of bugs flagged by Evaline
Ju: a rate-limiter that reserves a slot in OnRequest and releases in
OnResponse silently leaks whenever a later plugin denies, because
OnResponse is only walked on pipelines that returned Continue. Same
shape hits audit plugins (orphaned "request started" events),
distributed tracers (unclosed spans), and any plugin that holds a
lease across phases. Today no in-tree plugin is stateful on response
so the gap is latent, but that makes it a framework correctness trap
to resolve before a second stateful plugin ships.
type Finisher interface {
OnFinish(ctx context.Context, pctx *Context)
}
type Outcome struct {
FinalAction OutcomeAction // allow | deny | error
StatusCode int
DenyingPlugin string
Duration time.Duration
}
Design — answers to the 9 questions raised during discussion:
1. Timing: fires AFTER OnResponse if it ran, on every request end.
Not an error-only hook; stateful plugins get one release site.
2. API shape: separate opt-in interface (type assertion). Symmetric
with Initializer / Shutdowner / Readier.
3. Participation: only plugins whose OnRequest actually ran,
including the denier. Pipeline.Run tracks the dispatched set on
pctx as each plugin enters OnRequest.
4. Outcome info: hybrid. Consistent (ctx, pctx) signature; outcome
reached via pctx.Outcome() which returns non-nil ONLY during
OnFinish. Nil everywhere else so accidental reads in the wrong
phase surface as a nil deref rather than a stale zero value.
5. Context: fresh ctx derived from context.Background() with a
framework-set deadline (DefaultFinishTimeout = 2s, overridable via
WithFinishTimeout). Client disconnect during the request does not
cancel OnFinish I/O. The listener-supplied ctx argument is reserved
for future parent-cancel needs but intentionally not wired.
6. Order: LIFO, symmetric with Shutdowner and RunResponse.
7. State: full pctx including response body. Framework holds the
buffer across the finish window.
8. Errors: best-effort. Each plugin's OnFinish runs under its own
recover so a panic is logged and the LIFO chain continues. One
misbehaving plugin cannot leak state in every other plugin.
9. Invocations: OnFinish is silent. pctx.Record / SetBody /
SetResponseBody called during OnFinish are dropped with WARN logs
(SessionEvent published, response on the wire).
Implementation surface:
authlib/pipeline/plugin.go Finisher interface + detailed godoc
authlib/pipeline/outcome.go Outcome struct, OutcomeAction enum,
Context.Outcome() getter
authlib/pipeline/context.go dispatched []int ledger, outcome *Outcome,
inFinish bool, Record/SetBody guards
authlib/pipeline/pipeline.go DefaultFinishTimeout, WithFinishTimeout
Option, Pipeline.finishTimeout,
Pipeline.RunFinish + dispatchFinish
authlib/pipeline/finisher_test.go 13 tests covering LIFO, only-
dispatched, outcome visibility, nil-
outside-OnFinish, panic recovery,
fresh-ctx-not-cancelled, deadline,
default timeout, off-policy skip,
non-Finisher skip, Record-drop,
SetBody-drop.
Docs:
framework-architecture.md §6 — full "Per-request finish hook" section
next to the Start/Stop lifecycle.
§12 — versioning entry.
§13 — cross-reference updated.
plugin-reference.md — new "Finishing requests (stateful
plugins)" section with rate-limiter
example + the four "can't break" rules.
No listener wiring yet. Listeners call Pipeline.RunFinish in a future
commit; until then no plugin implements Finisher and the dispatcher
remains dormant (empty dispatched list = immediate return from
RunFinish). Existing plugins and tests unchanged.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Adds the missing half of the Finisher contract from 4306fbd: until the listeners call Pipeline.RunFinish on each request's exit path, the hook is dormant (no plugin's OnFinish can ever fire). This commit threads the dispatch through all four listener implementations and adds one end-to-end integration test. Design keeps the per-listener footprint minimal: authlib/pipeline/outcome.go OutcomeFromContext(pctx) derives a best-effort Outcome from pctx state: deny Invocation wins, else StatusCode > 0 = Allow, else Error. Each listener is a one-liner (defer RunFinish + OutcomeFromContext). authlib/pipeline/holder.go Holder.RunFinish delegate so listeners that hold *Holder (all of them, post-reloader) can call it without an .Load() indirection. authlib/pipeline/finisher_test.go TestOutcomeFromContext covering allow / error / deny (inbound, outbound-wins-over-inbound, shadow-ignored) cases. Per-listener wiring: reverseproxy/server.go defer in handleRequest after pctx is constructed. Populates StartedAt so Outcome.Duration is derived. forwardproxy/server.go same pattern as reverseproxy. extauthz/server.go two defers — inbound first (early, to fire even if inbound denies), outbound second (only if inbound allowed). Explicit StatusCode = 200 on allow paths because ext_authz has no HTTP status concept and OutcomeFromContext defaults StatusCode 0 to OutcomeError. extproc/server.go one defer at the top of Process() that reads pctx + requestDirection from the stream's local state. pctx may be nil if the stream never reached Run (early Recv error); skip in that case. Integration test: reverseproxy/finisher_test.go Allow path: OnFinish fires once with OutcomeAllow + StatusCode 200. Deny path: pipeline [before, denier, after]. OnFinish fires on before AND denier with OutcomeDeny + DenyingPlugin="denier"; after's OnFinish does NOT fire (its OnRequest never ran, so it's not in pctx.dispatched). Risk assessment: none of the in-tree plugins implement Finisher, so RunFinish walks an empty Finisher subset of pctx.dispatched and returns. Behavior for existing chains is unchanged. The only new cost is the deferred function call per request (one atomic Load from Holder + the no-op RunFinish body). Full authlib + cmd/authbridge test suites pass under -race. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
…mics Addresses five review comments from PR rossoctl#401, four code changes plus one comment-only tweak: rossoctl#1 — Framework-stamped rejecting plugin (robustness). OutcomeFromContext previously inferred the denying plugin from pctx.Extensions.Invocations, making correctness dependent on plugin authors pairing Action{Type: Reject} with a pctx.Record call. A plugin that returned Reject without recording was misclassified as OutcomeAllow or OutcomeError. Pipeline.Run / RunResponse now stamp pctx.rejectingPlugin on every enforce-policy Reject. OutcomeFromContext reads that first; the Invocation walk stays as defense-in-depth for bespoke dispatchers. Shadow rejections (on_error: observe) do not stamp the field — matches "the pipeline effectively allowed" semantics already used by abctl. Exposed via pctx.RejectingPlugin() getter for listeners that build Outcome explicitly. rossoctl#2 — context.WithoutCancel in dispatchFinish (ergonomics). OnFinish previously ran under context.Background()-derived ctx, discarding any values (slog fields, request ID, tracing span) the caller carried. Now uses context.WithoutCancel(parent) as the base: preserves values while still detaching cancellation so client disconnect doesn't abort cleanup I/O. The caller-supplied ctx is wired through RunFinish; its parameter is no longer vestigial. rossoctl#3 — Drop ext_authz StatusCode=200 sentinel (cleanup). OutcomeFromContext classifies pctx.StatusCode == 0 as OutcomeError, which is correct for HTTP listeners where 0 means "no response written." ext_authz has no HTTP status concept, so it was setting StatusCode=200 as a sentinel to coerce the helper into returning OutcomeAllow. With rossoctl#1 landed, ext_authz now calls a local authzOutcome(pctx) helper that reads pctx.RejectingPlugin() directly and defaults to OutcomeAllow — no sentinel, intent is explicit. rossoctl#4 — RunFinish double-call guard (defense-in-depth). A pctx.finished flag, set at RunFinish entry, causes a second call to log a WARN and early-return rather than double-release every Finisher's per-request state. All current listeners call RunFinish exactly once; this is purely protection against a refactor accident (two defers registered) or dispatch bug. rossoctl#5 — Reword ext_authz LIFO comment (readability). Moved the LIFO note to the outbound defer's block; the inbound defer now just says "fires whether the pipeline allows, denies, or Check returns early." The prior wording made the inbound defer's comment easy to misread as "inbound runs first." New tests: TestRunFinish_DoubleCallGuard second call is no-op TestRunFinish_CtxValuePropagation values carry through detach TestRun_StampsRejectingPlugin allow/enforce-deny/shadow-deny TestOutcomeFromContext_PrefersRejectingPlugin plugins that return Reject without recording are still Deny Full authlib + cmd/authbridge test suites pass under -race. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
esnible
left a comment
There was a problem hiding this comment.
Careful design, thorough tests, and honest scope discipline (out-of-scope items called out explicitly in the PR body). Framework-only change with no behavior impact on existing chains since no in-tree plugin implements Finisher. The 9 design-decision table in the PR description shows the trade-offs were worked through before coding. LIFO walk, panic recovery, fresh-but-values-preserving context (via WithoutCancel + WithFinishTimeout), and the double-call guard are all in place and covered by the 13 unit tests plus the reverseproxy allow/deny integration tests.
Areas reviewed: Go (pipeline/*, 4 listener servers, 13 unit tests, 1 integration test), Go docs
Commits: 3 commits, all signed off (DCO passes)
CI status: All required checks pass
Summary
Adds
pipeline.Finisher, an optional lifecycle hook that lets statefulplugins pair
OnRequestacquires with guaranteed releases — firedonce per request regardless of whether the pipeline allowed, denied,
or errored.
Closes the "cleanup leaks on denial" class of bugs flagged in
Evaline Ju's plugin-framework feedback: a rate-limiter that reserves
a slot in OnRequest and releases in OnResponse silently leaks
whenever a later plugin denies, because OnResponse is only walked on
pipelines that returned Continue. Same shape hits audit plugins
(orphaned "request started" events), distributed tracers (unclosed
spans), and any plugin that holds a lease across phases. Today no
in-tree plugin is stateful on response so the gap is latent — that
makes it a framework correctness trap worth resolving before a
second stateful plugin ships.
API
Plugins that implement
FinishergetOnFinishcalled once perrequest. The dispatcher walks in LIFO over plugins whose
OnRequestactually ran (including the denier, if any). Stateless plugins don't
implement the interface; the dispatcher skips them. During
OnFinish,pctx.Outcome()returns a non-nil*Outcomecarryingthe terminal state; everywhere else the getter returns nil.
Canonical use:
Design decisions
Answers to the 9 design questions worked through in the pre-
implementation discussion:
pctx.Outcome()getter returns non-nil ONLY during OnFinish. Consistent(ctx, pctx)signature; namespaced to avoid god-struct drift.context.Background()with a framework-set deadline (DefaultFinishTimeout= 2s,WithFinishTimeoutoverrides). Client disconnect never cancels cleanup I/O.pctx.Record/SetBody/SetResponseBodydropped with WARN logs (SessionEvent is published, response on the wire).Commits
feat(pipeline): Add Finisher hook for stateful plugin cleanup— interface +Outcome+ dispatcher + full godoc; framework-only, no listener changes (dormant until listeners wire it). Tests: 13 unit tests infinisher_test.gocovering LIFO, participation, outcome visibility, panic recovery, fresh-ctx-not-cancelled, deadline, off-policy skip, non-Finisher skip, Record-drop, SetBody-drop. Docs:framework-architecture.md§6 "Per-request finish hook", §12 versioning entry, §13 cross-reference;plugin-reference.md"Finishing requests (stateful plugins)" section.feat(listener): Wire RunFinish in all four listeners— one defer per listener (reverseproxy, forwardproxy, extauthz, extproc) with anOutcomeFromContexthelper keeping each wiring to a one-liner.Holder.RunFinishdelegate. End-to-end integration test inreverseproxy/finisher_test.goverifying the allow path, the deny path, and the "non-dispatched plugins don't get OnFinish" contract.Risk assessment
Behavior change for existing chains: none. No in-tree plugin implements
Finisher, soRunFinishwalks an empty subset ofpctx.dispatchedand returns. The only per-request cost is one atomicHolder.Load+ the no-opRunFinishbody.Test plan
go test -race ./...inauthbridge/authlib— passesgo test -race ./...inauthbridge/cmd/authbridge— passesgofmt -lon touched files — clean (unrelated pre-existing drift insession.go/session_test.gonot touched)OutcomeFromContextcasesreverseproxyAllow + Deny end-to-end with stub Finisher plugins-racedetector cleanOut of scope (possible followups)
rate-limiterplugin usingFinisher— would be the first in-tree consumer and a concrete example. Deferred because arbitration design is orthogonal (in-memory vs distributed, per-tenant vs global).Durationaccounting inInvocation— theOutcome.Durationderived inRunFinishis available; exposing it as part of the existing Invocation vocabulary is a separate design choice./v1/sessions. If operators need to debug sink-down conditions, adding a per-plugin finish-failure count to/statsis a small follow-on.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com