Refuse to start Stripe or Discourse without temporal-host

Second section of #141. Stripe and Discourse receive webhook events
that only a Temporal worker processes. Configured without temporal-host,
the console stored each event and never processed it, and with the
first section's key set at boot a Stripe checkout or plan move would
reach Stripe while the events it causes waited forever. Start now
refuses that configuration:

  Stripe needs Temporal, which processes its events: set temporal-host
  (or leave stripe-api-key and stripe-webhook-secret empty to disable it)

ConfigKey gains NeedsTemporal. Stripe declares it on stripe-api-key and
Discourse on discourse-base-url; FedWiki declares nothing, because it
receives no events and each of its Temporal-backed actions answers that
it is not available. ValidateStart checks every group that has every key
set and carries a NeedsTemporal key against a trimmed temporal-host, in
first-seen order; a partly set group gets only the partial-group
refusal. CheckSpecs refuses NeedsTemporal on a key without a
RequiredGroup. validateRequiredGroups and the new check share one
collection of the groups, with the same results and order as before.

Every configuration the repository ships or boots its tests with sets
temporal-host wherever it sets the Stripe or Discourse keys.

Full suite on reset databases, before the review fixes: 4382 pass, 15
skip, none failed; the console booted on the test stack. The review
fixes change comments and internal/config's tests only; after them
internal/config, internal/integrations and the Stripe and Discourse
trees pass, and make lint passes. Breaks, each caught:
- validateTemporalNeeds not called from ValidateStart
- the check reads temporal-namespace in place of temporal-host
- the check reads temporal-host untrimmed
- the check fires on a partly set group
- CheckSpecs without the NeedsTemporal group check
- Discourse ConfigSpec without NeedsTemporal
- Stripe ConfigSpec without NeedsTemporal
- FedWiki ConfigSpec declaring NeedsTemporal
This commit is contained in:
2026-10-23 15:15:23 -04:00
parent 29520a8120
commit 6b57598df6
10 changed files with 263 additions and 61 deletions
+17 -3
View File
@@ -93,6 +93,16 @@ type ConfigKey struct {
// (security-audit-remediation-3 design D1). Secret implies it; declaring
// both is a declaration error CheckSpecs refuses.
DeploymentOnly bool
// NeedsTemporal declares that the integration whose RequiredGroup this
// key belongs to receives events from outside that only Temporal
// processes, so ValidateStart refuses a configured group while
// temporal-host is empty (design D11, serve-before-dependencies). It
// belongs on a key of a RequiredGroup, because the group is what says the
// integration is configured; CheckSpecs refuses it elsewhere. An
// integration whose actions run on Temporal but that receives no events
// does not declare it: each action answers that it is not available, and
// nothing is stored for a worker that never comes.
NeedsTemporal bool
}
// RuntimeManaged reports whether k may be set at runtime as an override:
@@ -174,9 +184,10 @@ func typeContradictsDefault(t Type, def any) bool {
// CheckSpecs validates every declared key's own shape, independent of any
// resolved value: a declared Type that disagrees with Default's Go type, a
// TypeEnum declared with no Enum members, a non-empty Enum whose Default is
// not a string, and Positive on a key whose type is not int or duration
// (design D1, D6). cmd/start.go runs it over every installed integration's
// spec at boot, before ValidateStart: a bad declaration is a programming
// not a string, Positive on a key whose type is not int or duration
// (typed-config-keys design D1, D6), and NeedsTemporal on a key without a
// RequiredGroup (serve-before-dependencies design D11). cmd/start.go runs it
// over every installed integration's spec at boot, before ValidateStart: a bad declaration is a programming
// error in an integration, not a bad deployment value, so it fails boot in
// development regardless of what any environment sets. The same call
// exercises every installed integration's ConfigSpec in tests.
@@ -202,6 +213,9 @@ func CheckSpecs(keys []ConfigKey) error {
if k.Secret && k.DeploymentOnly {
errs = append(errs, fmt.Errorf("%s: declares both Secret and DeploymentOnly; Secret already implies deployment-only", k.Name))
}
if k.NeedsTemporal && k.RequiredGroup == "" {
errs = append(errs, fmt.Errorf("%s: declares NeedsTemporal without a RequiredGroup; the need belongs to the group whose keys enable the integration", k.Name))
}
}
return errors.Join(errs...)
}
+4
View File
@@ -70,6 +70,10 @@ func TestCheckSpecs(t *testing.T) {
{keys: []ConfigKey{{Name: "it-token", Secret: true, DeploymentOnly: true}}, want: []string{"it-token", "DeploymentOnly"}},
{keys: []ConfigKey{{Name: "it-api-url", Type: TypeURL, DeploymentOnly: true}}},
}},
{"NeedsTemporal without a RequiredGroup is refused with the whole sentence", []specCheck{
{keys: []ConfigKey{{Name: "it-events", NeedsTemporal: true}}, want: []string{"it-events: declares NeedsTemporal without a RequiredGroup; the need belongs to the group whose keys enable the integration"}},
{keys: []ConfigKey{{Name: "it-events", RequiredGroup: "It", NeedsTemporal: true}, {Name: "it-secret", RequiredGroup: "It"}}},
}},
{"a type that contradicts its default is refused naming the key and both types", []specCheck{
{keys: []ConfigKey{{Name: "it-flag", Type: TypeDuration, Default: true}}, want: []string{"it-flag", "duration", "bool"}},
}},
+58 -27
View File
@@ -115,6 +115,7 @@ func ValidateStart(integrationSpecs []ConfigKey) error {
// --- Conditional: installed integrations ---
errs = append(errs, validateRequiredGroups(integrationSpecs)...)
errs = append(errs, validateTemporalNeeds(integrationSpecs)...)
errs = append(errs, validateTypes(integrationSpecs)...)
return errors.Join(errs...)
@@ -143,42 +144,72 @@ func validateTypes(specs []ConfigKey) []error {
return errs
}
// requiredGroup is one RequiredGroup's keys, how many of them are set, and
// whether any of them declares NeedsTemporal.
type requiredGroup struct {
name string
keys []string
set int
needsTemporal bool
}
// collectRequiredGroups gathers the declared keys by RequiredGroup, in
// first-seen group order so aggregated errors are deterministic. A key counts
// as set when its resolved value is non-empty after trimming.
func collectRequiredGroups(specs []ConfigKey) []*requiredGroup {
byName := make(map[string]*requiredGroup)
var order []*requiredGroup
for _, k := range specs {
if k.RequiredGroup == "" {
continue
}
g, ok := byName[k.RequiredGroup]
if !ok {
g = &requiredGroup{name: k.RequiredGroup}
byName[k.RequiredGroup] = g
order = append(order, g)
}
g.keys = append(g.keys, k.Name)
g.needsTemporal = g.needsTemporal || k.NeedsTemporal
if strings.TrimSpace(viper.GetString(k.Name)) != "" {
g.set++
}
}
return order
}
// validateRequiredGroups checks every non-empty RequiredGroup shared by two
// or more declared keys: either all keys in the group are set, or none are.
// A group with some but not all keys set is rejected with a single
// aggregated error per group, naming every key in it — the same shape and
// UX the hardcoded "Conditional: Stripe" block used to produce by hand.
func validateRequiredGroups(specs []ConfigKey) []error {
type group struct {
names []string
set int
}
groups := make(map[string]*group)
// Preserve first-seen group order so aggregated errors are deterministic.
var order []string
for _, k := range specs {
if k.RequiredGroup == "" {
continue
}
g, ok := groups[k.RequiredGroup]
if !ok {
g = &group{}
groups[k.RequiredGroup] = g
order = append(order, k.RequiredGroup)
}
g.names = append(g.names, k.Name)
if strings.TrimSpace(viper.GetString(k.Name)) != "" {
g.set++
}
}
var errs []error
for _, name := range order {
g := groups[name]
if g.set > 0 && g.set < len(g.names) {
for _, g := range collectRequiredGroups(specs) {
if g.set > 0 && g.set < len(g.keys) {
errs = append(errs, fmt.Errorf(
"%s is partially configured: set %s together (or leave them all empty to disable it)",
name, strings.Join(g.names, " and ")))
g.name, strings.Join(g.keys, " and ")))
}
}
return errs
}
// validateTemporalNeeds refuses a configured group that carries a
// NeedsTemporal key while temporal-host is empty (serve-before-dependencies design D11). A group with
// only some of its keys set is left to validateRequiredGroups, which refuses
// it on its own. Stripe and Discourse declare the need today: both receive
// events from outside that only Temporal processes.
func validateTemporalNeeds(specs []ConfigKey) []error {
if strings.TrimSpace(viper.GetString("temporal-host")) != "" {
return nil
}
var errs []error
for _, g := range collectRequiredGroups(specs) {
if g.needsTemporal && g.set == len(g.keys) {
errs = append(errs, fmt.Errorf(
"%s needs Temporal, which processes its events: set temporal-host (or leave %s empty to disable it)",
g.name, strings.Join(g.keys, " and ")))
}
}
return errs
+85
View File
@@ -338,3 +338,88 @@ func TestValidateRequiredGroups(t *testing.T) {
}
})
}
// temporalNeedSpec mirrors Stripe's two keys and Discourse's two keys, each
// group carrying NeedsTemporal on one key, as the adapters declare them.
var temporalNeedSpec = []ConfigKey{
{Name: "stripe-api-key", Secret: true, RequiredGroup: "Stripe", NeedsTemporal: true},
{Name: "stripe-webhook-secret", Secret: true, RequiredGroup: "Stripe"},
{Name: "discourse-base-url", Type: TypeURL, RequiredGroup: "Discourse", NeedsTemporal: true},
{Name: "discourse-api-key", Secret: true, RequiredGroup: "Discourse"},
}
const (
stripeNeedsTemporal = "Stripe needs Temporal, which processes its events: set temporal-host (or leave stripe-api-key and stripe-webhook-secret empty to disable it)"
discourseNeedsTemporal = "Discourse needs Temporal, which processes its events: set temporal-host (or leave discourse-base-url and discourse-api-key empty to disable it)"
stripePartial = "Stripe is partially configured: set stripe-api-key and stripe-webhook-secret together (or leave them all empty to disable it)"
dbDSNRequired = "db-dsn is required"
)
// TestValidateStartRefusesAGroupThatNeedsTemporal holds ValidateStart to
// design D11 and copy row 7: a fully configured group with a NeedsTemporal
// key and no temporal-host is refused with the whole sentence, a partly
// configured one only with the partial-group refusal.
func TestValidateStartRefusesAGroupThatNeedsTemporal(t *testing.T) {
cases := []struct {
name string
set map[string]string
want []string
}{
{"both Stripe keys set, no temporal-host", map[string]string{"stripe-api-key": "k", "stripe-webhook-secret": "s"}, []string{stripeNeedsTemporal}},
{"both Stripe keys set, temporal-host set", map[string]string{"stripe-api-key": "k", "stripe-webhook-secret": "s", "temporal-host": "localhost:7233"}, nil},
{"only the Stripe key set, partial refusal and not copy 7", map[string]string{"stripe-api-key": "k"}, []string{stripePartial}},
{"no key set", nil, nil},
{"both Discourse keys set, no temporal-host", map[string]string{"discourse-base-url": "https://forum.example.com", "discourse-api-key": "k"}, []string{discourseNeedsTemporal}},
{"both Stripe keys set, temporal-host only whitespace", map[string]string{"stripe-api-key": "k", "stripe-webhook-secret": "s", "temporal-host": " "}, []string{stripeNeedsTemporal}},
{"both Stripe keys set, no temporal-host and no db-dsn", map[string]string{"stripe-api-key": "k", "stripe-webhook-secret": "s", "db-dsn": ""}, []string{stripeNeedsTemporal, dbDSNRequired}},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
checkTemporalNeed(t, tc.set, tc.want)
})
}
}
// checkTemporalNeed runs ValidateStart over a valid configuration with set
// layered on top and holds its error to as many lines as wanted, each wanted
// message contained in one of them.
func checkTemporalNeed(t *testing.T, set map[string]string, want []string) {
t.Helper()
validConfig()
t.Cleanup(viper.Reset)
// temporal-namespace carries a default in a running process, so it is set
// whether or not temporal-host is.
viper.Set("temporal-namespace", "default")
for k, v := range set {
viper.Set(k, v)
}
err := ValidateStart(temporalNeedSpec)
if len(want) == 0 {
if err != nil {
t.Fatalf("want no error, got: %v", err)
}
return
}
if err == nil {
t.Fatalf("want an error containing %q, got nil", want)
}
lines := strings.Split(err.Error(), "\n")
if len(lines) != len(want) {
t.Errorf("want %d refusals, got %d: %q", len(want), len(lines), lines)
}
for _, w := range want {
if !containsLine(lines, w) {
t.Errorf("error %q has no refusal containing %q", err.Error(), w)
}
}
}
// containsLine reports whether any of lines contains want.
func containsLine(lines []string, want string) bool {
for _, l := range lines {
if strings.Contains(l, want) {
return true
}
}
return false
}
+5 -2
View File
@@ -119,10 +119,13 @@ func (Adapter) MigrationSource() db.MigrationSource {
// (typed-config-keys design D1). discourse-sync-interval declares Positive:
// EnsureSyncSchedule (workflows/schedule.go) replaces a saved zero with
// DefaultSyncInterval silently, which would make the settings page state a
// value the scheduler ignores (design D6).
// value the scheduler ignores (design D6). discourse-base-url declares
// NeedsTemporal because Discourse's webhook events are reconciled by a
// workflow, and the sweep that restores what a failed event missed is itself
// a Temporal schedule.
func (Adapter) ConfigSpec() []config.ConfigKey {
return []config.ConfigKey{
{Name: "discourse-base-url", Type: config.TypeURL, DeploymentOnly: true, RequiredGroup: "Discourse", Usage: "Base URL of the Discourse instance (e.g., https://forum.example.com)"},
{Name: "discourse-base-url", Type: config.TypeURL, DeploymentOnly: true, RequiredGroup: "Discourse", NeedsTemporal: true, Usage: "Base URL of the Discourse instance (e.g., https://forum.example.com)"},
{Name: "discourse-api-key", Secret: true, RequiredGroup: "Discourse", Usage: "Discourse admin API key (global key; used with discourse-api-username)"},
{Name: "discourse-api-username", Default: "system", Usage: "Api-Username header value for Discourse admin API calls"},
{Name: "discourse-linkage-mode", Default: "oidc", Enum: []string{"email", "oidc", "discourseconnect"}, Usage: "How persons are matched to Discourse users: email (admin lookup by verified primary email), oidc (by-external lookup via the bundled OpenID Connect plugin), or discourseconnect (by-external lookup via DiscourseConnect SSO)"},
+60
View File
@@ -0,0 +1,60 @@
// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Commercial
// SPDX-FileCopyrightText: 2025-2026 Christian Galo
package integrations
import (
"testing"
"git.coopcloud.tech/wiki-cafe/member-console/internal/config"
)
// needsTemporalByIntegration is which registered integrations declare
// NeedsTemporal on a key of a RequiredGroup (design D11): the two that
// receive events from outside and process them only on Temporal.
var needsTemporalByIntegration = map[string]bool{
"stripe": true,
"discourse": true,
"fedwiki": false,
}
// TestIntegrationsThatReceiveEventsNeedTemporal holds each registered
// integration's ConfigSpec to the table above: Stripe and Discourse declare
// NeedsTemporal on a grouped key, FedWiki on none.
func TestIntegrationsThatReceiveEventsNeedTemporal(t *testing.T) {
seen := 0
for _, integ := range All() {
want, ok := needsTemporalByIntegration[integ.Key()]
if !ok {
t.Errorf("integration %q is not in the table; say whether it needs Temporal", integ.Key())
continue
}
seen++
checkNeedsTemporal(t, integ, want)
}
if seen != len(needsTemporalByIntegration) {
t.Errorf("table names %d integrations, registry has %d of them", len(needsTemporalByIntegration), seen)
}
}
// checkNeedsTemporal holds one integration's declarations to want.
func checkNeedsTemporal(t *testing.T, integ Integration, want bool) {
t.Helper()
cp, ok := integ.(config.ConfigProvider)
if !ok {
t.Fatalf("integration %T declares no ConfigSpec", integ)
}
got := false
for _, k := range cp.ConfigSpec() {
if !k.NeedsTemporal {
continue
}
if k.RequiredGroup == "" {
t.Errorf("%s declares NeedsTemporal without a RequiredGroup", k.Name)
}
got = true
}
if got != want {
t.Errorf("%T: NeedsTemporal declared = %v, want %v", integ, got, want)
}
}
+4 -2
View File
@@ -121,10 +121,12 @@ func (Adapter) MigrationSource() db.MigrationSource {
// key holds Accounts Read, an optional permission; without it the read
// falls back to what the key can already see (merchant-disclosure D2).
// Dashboard links stay unscoped and resolve in the operator's own
// dashboard session.
// dashboard session. stripe-api-key declares NeedsTemporal because Stripe's
// renewals, cancellations and payment failures arrive as webhook events that
// only Temporal applies.
func (Adapter) ConfigSpec() []config.ConfigKey {
return []config.ConfigKey{
{Name: "stripe-api-key", Secret: true, RequiredGroup: "Stripe", Usage: "Stripe API key"},
{Name: "stripe-api-key", Secret: true, RequiredGroup: "Stripe", NeedsTemporal: true, Usage: "Stripe API key"},
{Name: "stripe-webhook-secret", Secret: true, RequiredGroup: "Stripe", Usage: "Stripe webhook signing secret"},
}
}
@@ -53,7 +53,7 @@ A background loop (`(*Config).Discover`, started by the bound server's `Serve` o
A request to `/login`, `/register` or `/callback` that finds the sign-in holder empty (it is never filled while the verifier is empty) sends a non-blocking wake on a channel with a buffer of one and answers 503 at once; it does not wait for the attempt. The loop's wait ends on a wake, but no sooner than one second after the previous attempt began, and a wake does not shorten a wait that follows a document the loop marked `AtCap` (D4, D17), so a burst of sign-in requests makes at most one attempt per second, one at a time, and an unusable document is fetched at the cap and no faster.
Every handler that needs the discovery reads each pointer it needs once per request and uses that one value for the whole request. `/login`, `/register`, `/callback` and `POST /logout` load the sign-in pointer and fall back to the verifier pointer only when it is nil, so the endpoint and the verifier they use always come from one published state; without that rule, `POST /logout` could load a nil verifier, then find an endpoint stored in between, and take the end-session path with no verifier to mint the hint or revoke tokens with. Re-derivation loads the verifier pointer. A `Config` that has no holders (every hand-built configuration, which is every test that builds an `auth.Config` literal) reads values built from its exported fields, exactly as it does today: both accessors return a value whenever `OAuthConfig` is set, copying the fields as they are, an empty `EndSessionEndpoint` and a nil `Verifier` included. The gates and the branch of D5 test the pointer and not the endpoint's string, so a hand-built `Config` with an empty endpoint keeps today's answers, and `refreshIdentity` keeps its nil-`Verifier` check, applied to the value. `Setup` never sets the exported fields, so a running console has one path.
Every handler that needs the discovery reads each pointer it needs once per request and uses that one value for the whole request. `/login`, `/register`, `/callback` and `POST /logout` load the sign-in pointer and fall back to the verifier pointer only when it is nil, so the endpoint and the verifier they use always come from one published state; without that rule, `POST /logout` could load a nil verifier, then find an endpoint stored in between, and take the end-session path with no verifier to mint the hint or revoke tokens with. Re-derivation loads the verifier pointer. A `Config` that has no holders (every hand-built configuration, which is every test that builds an `auth.Config` literal) reads values built from its exported fields, exactly as it does today: both accessors always return a value, never nil, copying the fields as they are, a nil `OAuthConfig`, an empty `EndSessionEndpoint` and a nil `Verifier` included. The checks downstream of the gates already test `OAuthConfig` (`auth.go:839`, `:882`, `:1106`), now on the value, so the gates fire only for a `Config` built by `NewAwaitingDiscovery`. The gates and the branch of D5 test the pointer and not the endpoint's string, so a hand-built `Config` with an empty endpoint keeps today's answers, and `refreshIdentity` keeps its nil-`Verifier` check, applied to the value. `Setup` never sets the exported fields, so a running console has one path.
Reason: an atomic pointer to an immutable value is the cheapest correct way to publish one value from one writer to many readers; `sync.Once` and `sync.OnceValue` would cache a failure, and a mutex around a lazy fetch would make a request wait on the identity provider. Two holders, not one, because what a signed-in session needs (the verifier) must not wait for what only sign-in needs (the end-session endpoint): one value published only when both are present would stop re-deriving every session's roles for as long as a provider's document lacks the endpoint (D4). The provider keeps the context's client for its key set (go-oidc `oidc.go`, `remoteKeySet`), so the dedicated client also bounds every later JWKS fetch, which today uses `http.DefaultClient` with no timeout. A per-request fetch under `singleflight` would put the identity provider's latency inside sign-in requests and would still need the cache. Endpoints from configuration (`ProviderConfig`) would add configuration keys and drop the issuer check; discovery from an internal URL is a deployment workaround that keeps the coupling.
@@ -111,7 +111,7 @@ Reason: Stripe and Discourse receive events from outside that are processed only
### D12. The Stripe key is set at boot; the boot mode is the only mode
`start` calls `stripeintegration.UseKey(key)`, which assigns the trimmed key to `stripego.Key` when it is not empty (the mode and the fingerprint are derived from the trimmed key, so the SDK sends the value they describe), once the mode and the fingerprint are derived and before any goroutine starts: it is the first step of the serving half (D19), so the boot test sees it. The adapter's `Startup` no longer writes it. Nothing writes the key after that call, which runs before any goroutine and before the listener.
`start` calls `stripeintegration.UseKey(key)`, which assigns the trimmed key to `stripego.Key` when it is not empty (the mode and the fingerprint are derived from the trimmed key, so the SDK sends the value they describe), once the mode and the fingerprint are derived and before the listener and before any goroutine that reads the key: it is the first step of the serving half (D19), so the boot test sees it. The adapter's `Startup` no longer writes it. Nothing writes the key after that call, which runs before the listener and before any goroutine that reads the key.
`SDKKeyMode`, `server.Config.StripeKeyMode` and `workflows.WorkerConfig.StripeKeyMode` are removed. The handlers that took a `KeyMode func() string` (`BillingCheckoutHandler`, the member products and plans handlers) read the boot mode, `StripeMode`. The fulfillment entry points that took `stripeKeyMode func() string` (`SwitchPlan`, `KeepCurrentPlan`, `CancelSubscription`, `ReconcileSubscription`, `ReconcileLatestSubscriptionForCustomer`, `SweepDueScheduledChanges`) take `keyMode string`; the server passes `StripeMode`, the billing activities a `StripeMode string` from `WorkerConfig`, and the Stripe workflows' activities the result of their package-private `sdkKeyMode()`, which reads the key boot set.
@@ -125,7 +125,7 @@ Reason: viper is not safe for a write concurrent with reads. Today the only writ
### D14. The session store
`auth.Setup` probes the store once with the existing dial timeouts (connect 5 seconds, read and write 3 seconds). An error reply that refuses authentication (a `redis.Error` beginning `WRONGPASS`, `NOAUTH` or `NOPERM`, or an `ERR` reply to `AUTH` during the dial) fails `Setup`, and `start` exits 1. Any other probe error is logged at Error (copy row 17) and `Setup` continues; the pool dials again on each request. That covers a refused or timed-out connection, a TLS handshake that fails, and the error replies that say the store is busy, not that it refuses the console: `LOADING` (the store answers `PING` with it while it loads its dataset, and `AUTH` is allowed then), `BUSY` and `MASTERDOWN`.
`auth.Setup` probes the store once with the existing dial timeouts (connect 5 seconds, read and write 3 seconds). The probe takes its connection with `pool.GetContext`, which returns the dial's error apart from `PING`'s (`pool.Get` hides it inside an `errorConn`, so a reply to `AUTH` and a reply to `PING` would come out of the same `Do`). An error reply that refuses authentication (a `redis.Error` beginning `WRONGPASS`, `NOAUTH` or `NOPERM`, or an `ERR` reply to `AUTH` during the dial) fails `Setup`, and `start` exits 1, with one exception: a Valkey over `maxclients` writes `-ERR max number of clients reached` to a new connection before it reads any command, redigo returns that line as the reply to `AUTH`, and it means the store is busy, not that it refuses the console, so it is logged as copy row 17 and `Setup` continues. Any other probe error is logged at Error (copy row 17) and `Setup` continues; the pool dials again on each request. That covers a refused or timed-out connection, a TLS handshake that fails, and the error replies that say the store is busy, not that it refuses the console: `LOADING` (the store answers `PING` with it while it loads its dataset, and `AUTH` is allowed then), `BUSY`, `MASTERDOWN` and the max-clients reply.
The classification is by origin, not by error type. The store handed to the session manager is wrapped (`taggedStore{scs.Store}`, implementing `Find`, `Commit` and `Delete` and the `Ctx` variants scs prefers) so that every error the store returns is wrapped in `storeError`. `NewAwaitingDiscovery` does the wrapping and sets the session manager's `ErrorFunc`, so `Setup` and the server's tests get both from one place. The `ErrorFunc` answers 503, `Retry-After: 5` and copy row 3 exactly when `errors.As(err, &storeError)`, and the styled 500 page, as the default does today, for any other error (a session the codec cannot decode); each writes one structured log line (copy row 19) in place of scs's unstructured one. The page's action is `validReturnTo(r.URL.RequestURI())` for a GET or HEAD and `/` when it refuses or for any other method, because a request line such as `//host/x` reaches `LoadAndSave` before the mux.
@@ -278,7 +278,7 @@ A request that read an empty holder answers for that state even if the holder is
| F5 | `/login` and `POST /logout` dereference a nil OAuth configuration | `auth.go` `LoginHandler`, `LogoutHandler` | D3, D5: gated on the holders |
| F6 | Discovery fields swapped while handlers read them | `auth.Config` | D3: two atomic pointers, each to an immutable value written once |
| F7 | A session cannot be re-derived while no verifier is published. The window is unbounded while the provider's document names a different issuer (D4), and lasts until the first fetch succeeds after a cold start. A removed operator role or a disabled account keeps working for the session's seven days in that window | `refreshIdentity` | D3, D4: the verifier is published on its own as soon as the issuer matches, so a document without `end_session_endpoint` does not open the window; with no verifier `refreshIdentity` keeps the last known state and asks nothing, and no new session is made without discovery because `/callback` is gated. Named in Risks |
| F8 | `stripe.Key` written by a hook while requests read it | `stripe.go` `Startup` | D12: set before any goroutine |
| F8 | `stripe.Key` written by a hook while requests read it | `stripe.go` `Startup` | D12: set before the listener and before any goroutine that reads it |
| F9 | Checkout and the plan moves read the SDK key's mode while pages read the boot mode | `billing.go`, `member_plans.go` | D12: the boot mode only |
| F10 | A bounded grant written with no expiry workflow | `operator_enrollment.go` | D15 |
| F11 | A retry loop that never ends floods the log | `client.go` `dialWithRetry` | D17: log decay with a proved bound |
@@ -363,7 +363,7 @@ Removed: `Grant issued, but expiration scheduling FAILED (no scheduler); it will
| 14 | connector, connected (Info), reused | `connected to Temporal server`, attributes `host`, `namespace`, `attempts`, `elapsed`; `connected to Temporal after retry` goes |
| 15 | connector, open runs of a removed type (Error from the first line), message reused, the error text changed | message `refused to start the Temporal worker`, attributes `error`, `retry_in`; the error reads `1 FedWiki workflow run of types this release removed is still open: <types>. Let it finish on the previous release, or terminate it; the worker starts once none is open.`, and in the plural `<n> FedWiki workflow runs of types this release removed are still open: <types>. Let them finish on the previous release, or terminate them; the worker starts once none is open.` (`internal/integrations/fedwiki/fedwiki.go`, `openRunsError`; the old advice, "before starting this release", is wrong once the console is serving) |
| 16 | connector, the worker fails to build or start (Error from the first line), reused | `failed to create Temporal worker` and `failed to start Temporal worker`, attributes `error`, `retry_in` |
| 17 | `auth.Setup`, the session store gives no answer, or an error reply that is not an authentication refusal such as `LOADING` (Error) | `session store not reachable at startup; requests that need a session answer 503`, attributes `addr`, `error` |
| 17 | `auth.Setup`, the session store gives no answer, or an error reply that is not an authentication refusal such as `LOADING` (Error) | `session store not ready at startup; requests that need a session answer 503`, attributes `addr`, `error` |
| 18 | `start`, the session store refuses authentication (Error), reused | `server failed to start` with the error `failed to connect to Valkey: <error>`, then exit 1 |
| 19 | `ErrorFunc` (Error) | `session store request failed` for a store failure; `session could not be decoded` otherwise; attributes `method`, `path`, `error` |
| 20 | `POST /logout` while the end-session endpoint is not published (Info) | `session ended; the identity provider's session was not ended because its end-session endpoint is not known` |
@@ -15,12 +15,13 @@ optional because it requires certificates to be provisioned.
Dialling the store SHALL be bounded by connect, read and write timeouts. Startup
SHALL probe the store once before it serves. An error reply that refuses
authentication (a wrong or missing password: a reply beginning `WRONGPASS`,
`NOAUTH` or `NOPERM`, or an `ERR` reply to `AUTH`) SHALL fail startup with a
`NOAUTH` or `NOPERM`, or an `ERR` reply to `AUTH` other than
`ERR max number of clients reached`) SHALL fail startup with a
non-zero exit status. Any other probe error SHALL be logged at Error and SHALL
NOT hold the listener for longer than the dial timeouts; the store is dialled
again on each request. That covers a refused or timed-out connection, a failed
TLS handshake, and an error reply that says the store is busy, such as
`LOADING` while it reads its dataset.
`LOADING` while it reads its dataset, or `ERR max number of clients reached`.
A request whose session the store cannot load or save SHALL be answered 503 with
a `Retry-After` header (Requirement: A request whose dependency is not ready
@@ -4,7 +4,7 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
**The race baseline.** `make test` never runs `-race`, and the gates run whole packages (`internal/server`, the three integrations, `cmd`) that may never have run under it. Before section 1, run item 6.11's command without `./internal/retryloop/`, which does not exist before section 3 (go stops on a missing package path before it runs anything), on `bdd042bc`: `test/reset-test-db.sh` first, then the command with `-p 1`, because the packages share one test database and parallel packages make count-based tests claim each other's rows (`make test` runs `-p 1` for the same reason). Record its result in section 1's commit message. The run made on `bdd042bc` before this correction, with `./internal/retryloop/` in the list and without `-p 1`, found no data race; the only failures were `internal/retryloop` (does not exist yet) and two `internal/workflows/desiredstate` relay tests that failed on counts because packages ran in parallel, and that pass under `-race` alone on a reset database. If a later run shows races that these sections did not introduce, each gate in sections 3 to 6 is narrowed with `-run` to the new concurrency tests of its section (`TestDiscoveryPublishesUnderLoad`, `TestHolderUnderConcurrentReads`, `TestServeConsoleUnderLoad`, the retry loop's tests), and the old races are handed to the maintainer as candidate issues; none is filed without his go.
**Breaks.** Each section's list is written to a file outside the repository and run with `go run ./scripts/mutation-check -list <file> -side new` on the section's commit, after its green control run; the tests are new or rewritten in the section, so there is no old side. Every record's `want_new` is a text the failing test prints only when it fails, the message of the assertion that the break trips, never a test or subtest name, which `go test` prints on a pass as well. Every record must be CAUGHT. `scripts/mutation-check` runs its tests with `go test -count=1 -v -run` and no `-race`, so a break that only a `-race` run catches is written as a command record, `"cmd": "go test -race -p 1 -count=1 -run '^TestX$' ./pkg"` with `"exit": 1`, and the item below marks it as one. An `old` text below is the code as the section's tasks write it; when the committed code words it differently, the record is corrected to the committed text before the run, never dropped. A break that no test of its own section can see is named with the section whose test catches it, and runs with that section's list. The commit message lists every break with its result.
**Breaks.** Each section's list is written to a file outside the repository and run with `go run ./scripts/mutation-check -list <file> -side new` on the section's commit, after its green control run; the tests are new or rewritten in the section, so there is no old side. Every record's `want_new` is a text the failing test prints only when it fails, the message of the assertion that the break trips, never a test or subtest name, which `go test` prints on a pass as well. Every record must be CAUGHT. `scripts/mutation-check` runs its tests with `go test -count=1 -v -run` and no `-race`, so a break that only a `-race` run catches is written as a command record, `"cmd": "go test -race -p 1 -count=1 -run '^TestX$' ./pkg", "exit": 1, "want_new": "WARNING: DATA RACE"`, and the item below marks it as one. A command record is CAUGHT only when the exit code matches and `want_new` is found in the output (`scripts/mutation-check/main.go:387-399`); without `want_new` the run reports WRONG-MESSAGE. An `old` text below is the code as the section's tasks write it; when the committed code words it differently, the record is corrected to the committed text before the run, never dropped. A break that no test of its own section can see is named with the section whose test catches it, and runs with that section's list. The commit message lists every break with its result.
## 1. The Stripe key is set at boot, and every surface reads the boot mode
@@ -12,8 +12,8 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
- [x] 1.2 `cmd/start.go`: call `stripeintegration.UseKey(viper.GetString("stripe-api-key"))` directly after `FingerprintForKey`, with a comment that says why it is there. Remove `serverConfig.StripeKeyMode` and `workerCfg.StripeKeyMode`; set `workerCfg.StripeMode = stripeMode`.
- [x] 1.3 `internal/server`: delete `Config.StripeKeyMode` and its doc comment. `BillingCheckoutHandler.KeyMode` becomes `StripeMode string`, set from `cfg.StripeMode`, and `billing.go` reads it; `MemberProductsConfig.KeyMode` and the plans handler's `KeyMode` (`member_plans.go`) are deleted and the handlers read their `StripeMode`. `eagerReconcileOnReturn` passes `cfg.StripeMode`. `NewHandler`'s doc comment drops its sentence about `StripeKeyMode`, and the comment above the checkout registration says that `start` sets the SDK's key at boot.
- [x] 1.4 `internal/fulfillment`: `SwitchPlan`, `KeepCurrentPlan`, `CancelSubscription`, `ReconcileSubscription`, `ReconcileLatestSubscriptionForCustomer`, `SweepDueScheduledChanges` and the unexported functions they pass it to take `keyMode string` in place of `stripeKeyMode func() string`; each doc comment says it is the mode of the key the process runs under. `internal/workflows/worker.go`: `WorkerConfig.StripeKeyMode` becomes `StripeMode string`, and `wfBilling.NewActivities` takes the string. The Stripe workflows' activities pass `sdkKeyMode()` where they passed the function, and the comment of `key_mode.go` says the key it reads is set at boot, before the worker exists.
- [x] 1.5 Tests move to the boot mode on purpose (design F21): every call that passed `stripetest.KeyMode` passes `stripetest.KeyMode()`, and every `func() string { return "test" }` literal becomes `"test"`, in `internal/fulfillment` and `internal/server` (`billing_checkout_gate_test.go`, `billing_checkout_offer_db_test.go`, `billing_checkout_environment_db_test.go`, `new_handler_test.go`, `new_handler_db_test.go`, `plan_switch_timing_db_test.go`) and in the Stripe workflows' tests. The results of those tests do not change.
- [x] 1.6 `internal/integrations/stripe/stripe_test.go`: `TestSDKKeyMode` is deleted with its function. `TestUseKey`, one row per case through a named function: an empty key and a key of only whitespace leave `stripego.Key` as it was; `sk_test_usekey` is assigned; `" sk_test_usekey\n"` is assigned as `sk_test_usekey`. `TestCheckoutThroughTheRouter` (`internal/server/new_handler_db_test.go`) gains a live-price row, because the counter that used to show the router handing Checkout its mode is gone: a second product and price on a ladder of its own, mapped with `w.MapPriceIn(livePrice, true)`, posted through the same environment, which runs in `"test"` mode, answers 422 with a body containing `That product is no longer available`, and the test still sees exactly one `/v1/checkout/sessions` call; the test's doc comment says that Checkout, built from the server's configuration, checks the posted price against the boot mode and sends Stripe the configured base URL to return to. `TestStartupLeavesTheSDKKeyAlone` (a database test): with `stripego.Key` set to `sk_test_boot` and `stripe-api-key` set to `sk_test_hook`, `Startup` run with the smallest fake client that answers the calls it makes (the pattern of the existing fakes in `internal/server`'s tests) leaves `stripego.Key` at `sk_test_boot`.
- [x] 1.5 Tests move to the boot mode on purpose (design F21): every call that passed `stripetest.KeyMode` passes `stripetest.KeyMode()`, and every `func() string { return "test" }` literal becomes `"test"`, in `internal/fulfillment` and `internal/server` (`billing_checkout_gate_test.go`, `billing_checkout_offer_db_test.go`, `billing_checkout_environment_db_test.go`, `new_handler_test.go`, `new_handler_db_test.go`, `plan_switch_timing_db_test.go`) and in the Stripe workflows' tests. The two doc comments that told a test to pass the function to where the console passes its configured key mode now say that a test passes `stripetest.KeyMode()` where the console passes the mode boot derived: `docs/testing.md:183-185` and the comment above `KeyMode` in `internal/stripetest/mock.go:456-459`. The results of those tests do not change.
- [x] 1.6 `internal/integrations/stripe/stripe_test.go`: `TestSDKKeyMode` is deleted with its function. `TestUseKey`, one row per case through a named function: an empty key and a key of only whitespace leave `stripego.Key` as it was; `sk_test_usekey` is assigned; `" sk_test_usekey\n"` is assigned as `sk_test_usekey`. `TestCheckoutThroughTheRouter` (`internal/server/new_handler_db_test.go`) gains a live-price row, because the counter that used to show the router handing Checkout its mode is gone: a second product on no ladder, with a price mapped as live (`w.MapPriceIn(livePrice, true)`; the sentence below is the answer for a product on no ladder, and a product on a ladder would answer `That plan is no longer available.`), posted through the same environment, which runs in `"test"` mode, answers 422 with a body containing `That product is no longer available`, and the test still sees exactly one `/v1/checkout/sessions` call; the test's doc comment says that Checkout, built from the server's configuration, checks the posted price against the boot mode and sends Stripe the configured base URL to return to. `TestStartupLeavesTheSDKKeyAlone` (a database test): with `stripego.Key` set to `sk_test_boot` and `stripe-api-key` set to `sk_test_hook`, `Startup` run with the smallest fake client that answers the calls it makes (the pattern of the existing fakes in `internal/server`'s tests) leaves `stripego.Key` at `sk_test_boot`.
- [x] 1.7 Breaks:
- `UseKey` assigns an empty key (its emptiness check removed): caught by `TestUseKey`'s empty row;
- `UseKey` assigns the untrimmed key: caught by `TestUseKey`'s `" sk_test_usekey\n"` row;
@@ -27,13 +27,13 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
## 2. Integrations whose events are processed on Temporal need `temporal-host`
- [ ] 2.1 `internal/config/spec.go` (design D11): `ConfigKey` gains `NeedsTemporal bool`, with a doc comment that says what it declares, that it belongs on a key of a RequiredGroup, and why an integration that receives no events does not declare it. `CheckSpecs` refuses it on a key without a RequiredGroup with copy 8.
- [ ] 2.2 `internal/config/validate.go`: `validateTemporalNeeds(specs)`, appended after `validateRequiredGroups` in `ValidateStart`, reports copy 7 for each group, in first-seen order, that carries a `NeedsTemporal` key, has every key set, and meets an empty `temporal-host` (trimmed). A partly set group gets only the partial-group refusal, because it does not have every key set. Its doc comment names the two integrations that declare it today.
- [ ] 2.3 Stripe's `ConfigSpec` declares `NeedsTemporal` on `stripe-api-key`, and Discourse's on `discourse-base-url`; each `ConfigSpec` doc comment says why in one sentence. FedWiki declares nothing.
- [ ] 2.4 Tests. `TestValidateStartRefusesAGroupThatNeedsTemporal` (`internal/config/validate_test.go`), one row per case through a named function, with a spec list that mirrors Stripe's two keys and Discourse's two keys, each group carrying `NeedsTemporal` on one key: both Stripe keys set and `temporal-host` empty, copy 7 for Stripe in full; the same with `temporal-host` set, no error; only the Stripe key set, the partial-group refusal and not copy 7; no key set, no error; both Discourse keys set, copy 7 for Discourse in full; both Stripe keys set, `temporal-host` empty and `db-dsn` unset, both refusals in one joined error. `TestCheckSpecs` gains a row for copy 8. `TestIntegrationsThatReceiveEventsNeedTemporal` (a new `internal/integrations/registry_test.go`): over `integrations.All()`, Stripe and Discourse each declare `NeedsTemporal` on a key of a RequiredGroup, FedWiki on none.
- [ ] 2.5 Breaks:
- [x] 2.1 `internal/config/spec.go` (design D11): `ConfigKey` gains `NeedsTemporal bool`, with a doc comment that says what it declares, that it belongs on a key of a RequiredGroup, and why an integration that receives no events does not declare it. `CheckSpecs` refuses it on a key without a RequiredGroup with copy 8.
- [x] 2.2 `internal/config/validate.go`: `validateTemporalNeeds(specs)`, appended after `validateRequiredGroups` in `ValidateStart`, reports copy 7 for each group, in first-seen order, that carries a `NeedsTemporal` key, has every key set, and meets an empty `temporal-host` (trimmed). A partly set group gets only the partial-group refusal, because it does not have every key set. Its doc comment names the two integrations that declare it today.
- [x] 2.3 Stripe's `ConfigSpec` declares `NeedsTemporal` on `stripe-api-key`, and Discourse's on `discourse-base-url`; each `ConfigSpec` doc comment says why in one sentence. FedWiki declares nothing.
- [x] 2.4 Tests. `TestValidateStartRefusesAGroupThatNeedsTemporal` (`internal/config/validate_test.go`), one row per case through a named function, with a spec list that mirrors Stripe's two keys and Discourse's two keys, each group carrying `NeedsTemporal` on one key: both Stripe keys set and `temporal-host` empty, copy 7 for Stripe in full; the same with `temporal-host` set, no error; only the Stripe key set, the partial-group refusal and not copy 7; no key set, no error; both Discourse keys set, copy 7 for Discourse in full; both Stripe keys set, `temporal-host` empty and `db-dsn` unset, both refusals in one joined error. `TestCheckSpecs` gains a row for copy 8. `TestIntegrationsThatReceiveEventsNeedTemporal` (a new `internal/integrations/registry_test.go`): over `integrations.All()`, Stripe and Discourse each declare `NeedsTemporal` on a key of a RequiredGroup, FedWiki on none.
- [x] 2.5 Breaks:
- `validateTemporalNeeds` not called from `ValidateStart`: caught by the Stripe row;
- the check reads `temporal-namespace` in place of `temporal-host`: caught by the row with `temporal-host` set;
- the check reads `temporal-namespace` in place of `temporal-host`: caught by the rows with no `temporal-host` (Stripe, Discourse, db-dsn), because every row sets `temporal-namespace` to its process default;
- the check fires on a partly set group: caught by the partial row (two errors where it wants one);
- `CheckSpecs` without the group check: caught by the copy 8 row;
- Discourse's `ConfigSpec` without `NeedsTemporal`: caught by `TestIntegrationsThatReceiveEventsNeedTemporal`;
@@ -43,11 +43,11 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
## 3. A retry loop, and sign-in that waits for the discovery document
- [ ] 3.1 `internal/retryloop` (new package, design D17): `Run(ctx, policy, attempt)` calls `attempt(ctx)` until it returns nil or `ctx` ends, and returns nil or `ctx.Err()`. `Policy` carries the first wait, the cap, the escalation time, an optional wake channel, the minimum gap after an attempt before a wake counts, the default message for a failed attempt that returns a plain error, the message for the success, the base attributes every line carries, a logger, and an injectable clock and random source (the real ones when unset). An attempt returns a plain error, or a `retryloop.Failure{Message, Level, Err}` carrying its own message and a level floor (Error for copy rows 10, 15 and 16); `AtCap(err)` marks an error after which the next wait is the cap, and a wake does not end that wait. Every line goes through the decay: no caller logs from inside an attempt (design D17). The decay and escalation are D17's table. The package doc comment states the numbers and the bound on lines per hour.
- [ ] 3.2 `internal/auth/auth.go` (design D3, D4): two unexported immutable values behind two `atomic.Pointer`s in `Config`, with a wake channel of buffer one: the verifier (the `*oauth2.Config`, the verifier, the provider, the revocation endpoint) and the sign-in value, `signIn{verifier *discovered; endSession string}`, which holds the end-session endpoint and a pointer to the verifier value it was published with, so that a request that loads the sign-in value never meets a verifier from another document. `NewAwaitingDiscovery(sessionManager)` returns a `Config` with empty holders; `Setup` builds its `Config` with it, reads from viper once every key the loop needs, and no longer calls `oidc.NewProvider`. `providerEndpoints(provider, issuer)` compares the document's `issuer` claim with the configured one and returns a typed `issuerMismatchError`; with a matching issuer it returns both endpoints, and a missing end-session endpoint is its own error, returned together with the revocation endpoint the verifier needs (today's error path returns it as `""`, `auth.go:149-150`); the provider is built with `oidc.InsecureIssuerURLContext(ctx, issuer)` so that go-oidc's own untyped issuer error cannot occur, and ID tokens are still checked against the configured issuer. `(*Config).Discover(ctx)` runs `retryloop.Run` with D17's numbers and a ninety-second escalation: each attempt builds the provider under a context carrying a dedicated `*http.Client` (`Timeout` from the package variable `discoveryAttemptTimeout`, ten seconds, which a test shortens) and a deadline of the same length, and stores only inside the matching-issuer branch: the verifier once (if it is not stored), logging copy 12 when the document has no revocation endpoint, and then, when the same document carries `end_session_endpoint`, the sign-in value once, after the verifier and pointing at the stored verifier. A document whose issuer does not match stores neither, whatever it carries. With both stored the loop logs copy 11 and returns. An `issuerMismatchError` or a missing end-session endpoint is returned as a `retryloop.Failure` marked `AtCap` with copy 10's message; any other failure is copy 9's message. `verifier()` and `signIn()` return the holders' values for a `Config` built by `NewAwaitingDiscovery`. For a hand-built `Config` both return a value whenever `OAuthConfig` is set, copying the exported fields as they are, an empty `EndSessionEndpoint` and a nil `Verifier` included, and nil only when `OAuthConfig` is nil; their doc comments say why both exist.
- [ ] 3.2 `internal/auth/auth.go` (design D3, D4): two unexported immutable values behind two `atomic.Pointer`s in `Config`, with a wake channel of buffer one: the verifier (the `*oauth2.Config`, the verifier, the provider, the revocation endpoint) and the sign-in value, `signInState{verifier *discovered; endSession string}`, which holds the end-session endpoint and a pointer to the verifier value it was published with, so that a request that loads the sign-in value never meets a verifier from another document. `NewAwaitingDiscovery(sessionManager)` returns a `Config` with empty holders; `Setup` builds its `Config` with it, reads from viper once every key the loop needs, and no longer calls `oidc.NewProvider`. `providerEndpoints(provider, issuer)` compares the document's `issuer` claim with the configured one and returns a typed `issuerMismatchError`; with a matching issuer it returns both endpoints, and a missing end-session endpoint is its own error, returned together with the revocation endpoint the verifier needs (today's error path returns it as `""`, `auth.go:149-150`); the provider is built with `oidc.InsecureIssuerURLContext(ctx, issuer)` so that go-oidc's own untyped issuer error cannot occur, and ID tokens are still checked against the configured issuer. `(*Config).Discover(ctx)` runs `retryloop.Run` with D17's numbers and a ninety-second escalation: each attempt builds the provider under a context carrying a dedicated `*http.Client` (`Timeout` from the package variable `discoveryAttemptTimeout`, ten seconds, which a test shortens) and a deadline of the same length, and stores only inside the matching-issuer branch: the verifier once (if it is not stored), logging copy 12 when the document has no revocation endpoint, and then, when the same document carries `end_session_endpoint`, the sign-in value once, after the verifier and pointing at the stored verifier. A document whose issuer does not match stores neither, whatever it carries. With both stored the loop logs copy 11 and returns. An `issuerMismatchError` or a missing end-session endpoint is returned as a `retryloop.Failure` marked `AtCap` with copy 10's message; any other failure is copy 9's message. `verifier()` and `signIn()` return the holders' values for a `Config` built by `NewAwaitingDiscovery`. For a `Config` without holders (every hand-built one) both always return a value, never nil, copying the exported fields as they are: a nil `OAuthConfig`, an empty `EndSessionEndpoint` and a nil `Verifier` included. The checks downstream of the gates already test `OAuthConfig` (`auth.go:839`, `:882`, `:1106`), now on the value, so the gates fire only for a `Config` built by `NewAwaitingDiscovery`, which is what "exactly as it does today" needs. The method `signIn()` keeps its name, because the package's test helper `signIn` (`auth_session_person_test.go:51`) is a package-level function and a method name is not in package scope; their doc comments say why both exist.
- [ ] 3.3 The handlers read each pointer they need once per request. `/login`, `/register`, `/callback` and `POST /logout` load the sign-in pointer and fall back to the verifier pointer only when it is nil, so the pair they use always comes from one document; re-derivation loads the verifier pointer. The gates and the logout branch test the pointer and not the endpoint's string, so a hand-built `Config` with an empty `EndSessionEndpoint` keeps today's answers. `LoginHandler`, `CallbackHandler` and `RegistrationHandler` begin with a gate: with the sign-in value missing, wake the loop with a non-blocking send, set `Retry-After: 5`, and answer 503 through `showFailure` with copy 1 (`/login` and `/callback`) or copy 2 (`/register`), before anything touches the session. `LogoutHandler` with no sign-in value destroys the session, revokes the refresh tokens when the verifier is published, logs copy 20, and answers `HX-Redirect: /` with 200 for an htmx request and 303 to `/` otherwise (design D5); with the sign-in value it runs as today, passing the value's verifier to `idTokenHint` and `revokeRefreshTokens`, which read nothing from `c`'s exported discovery fields and dereference nothing the value does not hold. `refreshIdentity` and `rederive` read the verifier value and keep today's nil-`Verifier` check (`auth.go:1106`), now applied to that value; with none, the session keeps its state and nothing is asked of the provider. The comment on `EndSessionEndpoint` and the "Setup guarantees one" log line of `LogoutHandler` are reworded to say where the endpoint comes from.
- [ ] 3.4 `internal/server/server.go`: `Start` runs `go authConfig.Discover(ctx)` after `NewHandler` and before `ListenAndServe`; the doc comment of `Start` says so. Nothing else in the boot order changes in this section.
- [ ] 3.5 `internal/retryloop` tests, each with a fake clock and a seeded source: `TestRunWaitsFollowThePolicy` (the exact waits of eight failures that the seeded source yields, each the base wait plus the seeded fraction of it, the cap reached and kept; a range check would accept zero jitter); `TestRunWakeEndsTheWait` (a wake during a fifteen-second wait starts the next attempt one second after the last began, and a wake within that second waits for the second); `TestRunAtCapWaitsTheCap` (the wait after an `AtCap` result is the cap, and a wake during it does not end it); `TestRunStopsWhenTheContextEnds` (returns `context.Canceled`, no further attempt); `TestRunLogDecay`, one row per pattern through a named function: an hour of one error (at most fourteen lines; the first is Warn, every line from ninety seconds on is Error, and the first Error line is written when the escalation time passes), an hour of an error whose text changes on every attempt (no two changed-text lines within thirty seconds), a `Failure` with level Error (Error from the first line, and the hour's count stays within the bound), and three failures then a success (three lines, then one Info line with `attempts` 4 and the elapsed time). Every line carries `attempts`, `elapsed`, `retry_in` and `error`.
- [ ] 3.6 `internal/auth` tests, with an `httptest` identity provider whose answer the test switches (`discoverytest` helpers in the test file, building on the fake discovery document of `logout_test.go`): `TestSignInWaitsForDiscovery`: while the fake answers 503, `/login` answers 503, `Retry-After: 5`, copy 1's heading as plain text without a renderer, no `Set-Cookie`, and the session store holds no new entry; after the switch to a usable document, a `/login` within the loop's wait answers 302 to the fake's authorization endpoint; `/callback` and `/register` give copy 1 and copy 2 before the switch. `TestDiscoveryKeepsAnUnusableDocumentOut`, one row per document: no `end_session_endpoint` (the verifier is published, the sign-in value is not, and `/login` still answers 503), and an issuer that is not the configured one, in a document that carries `end_session_endpoint` (neither value is published, `/login` still answers 503, and the error is the typed `issuerMismatchError`); each logs copy 10 at Error from the first line, and the same fake switched to a usable document is published without a new `Config`. `TestDiscoveryAttemptEnds`, one row per fake, with `discoveryAttemptTimeout` shortened to 200 milliseconds: a fake that accepts the document request and never answers (the attempt's line is logged within a second), and a usable document whose `jwks_uri` accepts and never answers (a `Verify` of an ID token, called with `context.Background()` in a goroutine, returns within a second, and the test fails with "Verify outlasted the attempt timeout" otherwise; a `Verify` under a deadline of its own returns at that deadline whatever client the key set uses, so only a `Verify` with no deadline shows that the dedicated client bounds the fetch). `TestSignInRequestWakesDiscovery`: the loop with an injected clock (an unexported package variable beside `discoveryAttemptTimeout` holds the clock `Discover` hands to the loop) waiting out a fifteen-second backoff; a `/login` starts the next attempt without advancing the clock past one second. `TestSignOutWithoutAnEndSessionEndpoint`, one row per state: before publication (the session is gone from the store, the answer is 303 to `/` and 200 with `HX-Redirect: /` for an htmx request, copy 20 is logged, and the fake received no request after the test began counting), and with the verifier published and no end-session endpoint (the same answers, and the fake received a revocation request carrying the session's refresh token). `TestSignedInSessionPassesBeforeDiscovery`: a session whose re-derivation is due, served through `Middleware` before publication, reaches the handler with its roles, and the fake received no request. `TestRolesAreRederivedWhileTheDocumentIsUnusableForSignIn`: the fake serves a document with the configured issuer and no `end_session_endpoint`, the role of a signed-in session is removed at the fake's token endpoint, and the session's next request after the interval carries no such role while `/login` answers 503. `TestDiscoveryPublishesUnderLoad`: twenty goroutines request `/login` and a signed-in page through `Middleware` while the fake switches; every answer is 503 or 302 for `/login` and 200 for the page, and the test runs under `-race` (item 3.10). Three existing tests change with the accessors, and each keeps its result because a hand-built `Config` returns values whenever `OAuthConfig` is set: `auth_login_test.go` (an `OAuthConfig` with no `EndSessionEndpoint` still gets 302 at `/login`), `logout_test.go` (an empty `EndSessionEndpoint` still answers 500, `:530`) and `new_handler_test.go`'s `routeAuth` (no `Verifier`, so re-derivation keeps today's nil check). `TestProviderEndpointsComeFromDiscovery` (`logout_test.go:486`) is rewritten for `providerEndpoints(provider, issuer)`: its "no end-session endpoint" subtest wants the missing-endpoint error together with the revocation endpoint, and a new subtest wants `issuerMismatchError`.
- [ ] 3.6 `internal/auth` tests, with an `httptest` identity provider whose answer the test switches (`discoverytest` helpers in the test file, building on the fake discovery document of `logout_test.go`): `TestSignInWaitsForDiscovery`: while the fake answers 503, `/login` answers 503, `Retry-After: 5`, copy 1's heading as plain text without a renderer, no `Set-Cookie`, and the session store holds no new entry (the test keeps its own reference to the `memstore.MemStore` it built, because from section 4 `NewAwaitingDiscovery` replaces `sm.Store` with the tagged store and a read through `cfg.SessionManager.Store` no longer reaches it); after the switch to a usable document, a `/login` within the loop's wait answers 302 to the fake's authorization endpoint; `/callback` and `/register` give copy 1 and copy 2 before the switch. `TestDiscoveryKeepsAnUnusableDocumentOut`, one row per document: no `end_session_endpoint` (the verifier is published, the sign-in value is not, and `/login` still answers 503), and an issuer that is not the configured one, in a document that carries `end_session_endpoint` (neither value is published, `/login` still answers 503, and the error is the typed `issuerMismatchError`); each logs copy 10 at Error from the first line, and the same fake switched to a usable document is published without a new `Config`. `TestDiscoveryAttemptEnds`, one row per fake, with `discoveryAttemptTimeout` shortened to 200 milliseconds: a fake that accepts the document request and never answers (the attempt's line is logged within a second), and a usable document whose `jwks_uri` accepts and never answers (a `Verify` of an ID token, called with `context.Background()` in a goroutine, returns within a second, and the test fails with "Verify outlasted the attempt timeout" otherwise; a `Verify` under a deadline of its own returns at that deadline whatever client the key set uses, so only a `Verify` with no deadline shows that the dedicated client bounds the fetch). `TestSignInRequestWakesDiscovery`: the loop with an injected clock (an unexported package variable beside `discoveryAttemptTimeout` holds the clock `Discover` hands to the loop) waiting out a fifteen-second backoff; a `/login` starts the next attempt without advancing the clock past one second. `TestSignOutWithoutAnEndSessionEndpoint`, one row per state: before publication (the session is gone from the store, the answer is 303 to `/` and 200 with `HX-Redirect: /` for an htmx request, copy 20 is logged, and the fake received no request after the test began counting), and with the verifier published and no end-session endpoint (the same answers, and the fake received a revocation request carrying the session's refresh token). `TestSignedInSessionPassesBeforeDiscovery`: a session whose re-derivation is due, served through `Middleware` before publication, reaches the handler with its roles, and the fake received no request. `TestRolesAreRederivedWhileTheDocumentIsUnusableForSignIn`: the fake serves a document with the configured issuer and no `end_session_endpoint`, the role of a signed-in session is removed at the fake's token endpoint, and the session's next request after the interval carries no such role while `/login` answers 503. `TestDiscoveryPublishesUnderLoad`: twenty goroutines request `/login` and a signed-in page through `Middleware` while the fake switches; every answer is 503 or 302 for `/login` and 200 for the page, and the test runs under `-race` (item 3.10). Existing tests change with the accessors, and each keeps its result because a `Config` without holders always returns values: the tests that call a gated handler through `newTestConfig()`, which is `&Config{SessionManager: sm}` with no `OAuthConfig` (`TestRegistrationHandlerRotatesTheSessionToken` in `auth_session_rotation_test.go` still gets its rotated token, `TestAStateCarryingNewlinesStaysOneLogRecord` in `auth_logging_test.go` still gets 400, and `auth_failure_page_test.go` at `:60`, `:92`, `:138`, `:161` and `:179` still get 400, because `CallbackHandler` refuses on the state or on the provider's `error` before it reads `OAuthConfig`), and three more: `auth_login_test.go` (an `OAuthConfig` with no `EndSessionEndpoint` still gets 302 at `/login`), `logout_test.go` (an empty `EndSessionEndpoint` still answers 500, `:530`) and `new_handler_test.go`'s `routeAuth` (no `Verifier`, so re-derivation keeps today's nil check). `TestProviderEndpointsComeFromDiscovery` (`logout_test.go:486`) is rewritten for `providerEndpoints(provider, issuer)`: its "no end-session endpoint" subtest wants the missing-endpoint error together with the revocation endpoint, and a new subtest wants `issuerMismatchError`.
- [ ] 3.7 `internal/server`: `answer()` (the route table's recorder) records a `Retry-After` header when a response carries one, so `routes.golden` does not change. `TestNewHandlerRouteTableNotReady` builds the route environment with `auth.NewAwaitingDiscovery` over the test's session manager and writes `testdata/routes-not-ready.golden` with the rows of `TestNewHandlerRouteTable`. Its diff from `routes.golden` is the sign-in rows only: `/login`, `/register` and `/callback` answer 503 with `Retry-After: 5` and copy 1 or 2, and `POST /logout` lands on `/`. `TestNewHandlerUnavailableSignInIsStyled`: a browser `/login` before publication renders the styled page with copy 1's heading and one action, `Try again`, to `/login`.
- [ ] 3.8 Breaks:
- `LoginHandler`'s gate after `RenewToken`: caught by `TestSignInWaitsForDiscovery` (a `Set-Cookie` is written);
@@ -76,20 +76,22 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
- the escalation never reached (Warn throughout): caught by the same row;
- the changed-text rule without its thirty-second floor: caught by the changing row;
- a wake that ignores the one-second gap: caught by `TestRunWakeEndsTheWait` (the gap is asserted there; `TestSignInRequestWakesDiscovery` has one wake);
- `answer()` without the `Retry-After` line: caught by `TestNewHandlerRouteTableNotReady` (`want_new` is `testdata/routes-not-ready.golden differs from what the code produced`, which `internal/testkit/golden/golden.go:42` prints only on a mismatch);
- `Start` without `go authConfig.Discover(ctx)`: no test of this section runs `Start`; its break runs with section 6's list (item 6.10).
- [ ] 3.9 `internal/embeds/static/error-handler.js`: the 5xx toast reads `Server error. Try again later.` (copy row 24). The check `filler-copy` cannot make, because it does not scan JavaScript, is `git grep "Please try again later" -- internal/embeds test` finding nothing after the edit; no test, golden or screen baseline carries the old string, so nothing else changes.
- [ ] 3.10 `go test -race -p 1 -count=1 ./internal/retryloop/ ./internal/auth/ ./internal/server/` passes.
- [ ] 3.10 After `test/reset-test-db.sh`, `go test -race -p 1 -count=1 ./internal/retryloop/ ./internal/auth/ ./internal/server/` passes.
## 4. The session store: a refusal stops the start, an outage answers 503
- [ ] 4.1 `internal/auth/auth.go` and `valkey.go` (design D14): `Setup`'s probe returns its error only when the store's reply refuses authentication: a `redis.Error` beginning `WRONGPASS`, `NOAUTH` or `NOPERM`, or an `ERR` reply to `AUTH` during the dial. Every other probe error logs copy 17 and continues, an error reply such as `LOADING` (the store answers `PING` with it while it loads its dataset, which is the state of a restart of everything together), `BUSY` or `MASTERDOWN` included. The session pool's dial takes `valkey-addr` once, when `Setup` builds the pool, in place of reading it on every dial (design D13). `NewAwaitingDiscovery` wraps the session manager's store in `taggedStore{scs.Store}` (`Setup` builds its `Config` through it, and so does the server's test of item 4.4), implementing `Find`, `Commit` and `Delete` and the `Ctx` variants scs prefers, so that every error the store returns is wrapped in `storeError`; there is no list of error types, and the doc comment says why (the codec returns `io.EOF` and `io.ErrUnexpectedEOF`, and redigo returns TLS and protocol errors of types no list names). `FailurePage` gains `RenderErrorPage(w, r, status, msg)`, which `SafeTemplates` already has. `NewAwaitingDiscovery` also sets the session manager's `ErrorFunc`, which writes through the latch writer of item 4.2: for `errors.As(err, &storeError)` it deletes the headers a handler may have set, which are every header whose name begins `HX-` (`HX-Redirect`, `HX-Refresh` and `HX-Trigger` among them), `Location`, `Set-Cookie` and `Content-Type`, answers `Retry-After: 5` and 503 through `showFailure` with copy 3 (action: `validReturnTo(r.URL.RequestURI())` for GET and HEAD, `/` when it refuses and for any other method) and logs copy 19's first message; any other error does the same with the styled 500 page, the recovery page's existing message (`An unexpected error occurred. Try again.`) and copy 19's second message. Either answer marks the latch.
- [ ] 4.1 `internal/auth/auth.go` and `valkey.go` (design D14): `Setup`'s probe takes its connection with `pool.GetContext`, which returns the dial's error apart from `PING`'s (`pool.Get` hides it in an `errorConn`, so a reply to `AUTH` and a reply to `PING` would come out of the same `Do`), and classifies the two separately. It returns its error only when the store's reply refuses authentication: a `redis.Error` beginning `WRONGPASS`, `NOAUTH` or `NOPERM`, or an `ERR` reply to `AUTH` during the dial, unless that reply begins `ERR max number of clients reached`, which a Valkey over `maxclients` writes to a new connection before it reads any command; that is a busy store, and it takes the next sentence's path. Every other probe error logs copy 17 and continues, an error reply such as `LOADING` (the store answers `PING` with it while it loads its dataset, which is the state of a restart of everything together), `BUSY` or `MASTERDOWN` included. The session pool's dial takes `valkey-addr` once, when `Setup` builds the pool, in place of reading it on every dial (design D13). `NewAwaitingDiscovery` wraps the session manager's store in `taggedStore{scs.Store}` (`Setup` builds its `Config` through it, and so does the server's test of item 4.4), implementing `Find`, `Commit` and `Delete` and the `Ctx` variants scs prefers, so that every error the store returns is wrapped in `storeError`; there is no list of error types, and the doc comment says why (the codec returns `io.EOF` and `io.ErrUnexpectedEOF`, and redigo returns TLS and protocol errors of types no list names). `FailurePage` gains `RenderErrorPage(w, r, status, msg)`, which `SafeTemplates` already has. `NewAwaitingDiscovery` also sets the session manager's `ErrorFunc`, which writes through the latch writer of item 4.2: for `errors.As(err, &storeError)` it deletes the headers a handler may have set, which are every header whose name begins `HX-` (`HX-Redirect`, `HX-Refresh` and `HX-Trigger` among them), `Location`, `Set-Cookie` and `Content-Type`, answers `Retry-After: 5` and 503 through `showFailure` with copy 3 (action: `validReturnTo(r.URL.RequestURI())` for GET and HEAD, `/` when it refuses and for any other method) and logs copy 19's first message; any other error does the same with the styled 500 page, the recovery page's existing message (`An unexpected error occurred. Try again.`) and copy 19's second message. Either answer marks the latch.
- [ ] 4.2 `internal/auth` and `internal/server/server.go` (design D14): the latch writer lives in `internal/auth`, because the `ErrorFunc` there cannot import `internal/server`, which imports `internal/auth`. `(*Config).LoadAndSave(next)` wraps `w` in the latch writer and calls the session manager's `LoadAndSave`: once the `ErrorFunc` marks the latch, the writer drops every later `WriteHeader` and `Write`, so a handler's headers and body cannot follow the 503. `preAuthStack`'s session layer (`server.go:443`) becomes `skipSession(exempt, authConfig.LoadAndSave)`, which calls the next handler without loading the session for a request under `/static/`, for `/favicon.ico`, or whose path is in `exempt` (`/domains/ask` and the paths the mounts declare, the list `registerIntegrationRoutes` returns), and calls `LoadAndSave` for every other request. Its doc comment says which routes and why. `routes.golden` does not change, because `routeAuth` builds a hand-made session manager that keeps scs's default `ErrorFunc`, which never marks the latch.
- [ ] 4.3 `cmd/start.go` (design D18): when `server.Start` returns, `Run` logs copy 21 and calls `os.Exit(1)`.
- [ ] 4.4 Tests. `TestSetupRefusesAStoreThatRefuses` (`internal/auth`): against the stack's session store with a password the store does not accept, `Setup` returns an error that contains `failed to connect to Valkey`. `TestSetupServesWithoutAStore`, one row per store through a named function: `valkey-addr` naming a port that refuses (listened on, then closed), and a fake store that answers every command with `-LOADING Redis is loading the dataset in memory`; each gives a `Config` and copy 17 at Error with `addr`. `TestSessionStoreFailureAnswers503` (`internal/auth`, over a handler wrapped by `(*Config).LoadAndSave`, where the latch lives), with an `scs.Store` fake whose `Find` and `Commit` return a `*net.OpError`, one row per request through a named function: a browser GET with a session cookie to a page (503, `Retry-After: 5`, the styled page with copy 3's heading and `Try again` to the requested path), an htmx request with the cookie (503, the plain text `Sign-in unavailable`), a POST with the cookie (the action leads to `/`), a GET to `//evil.example/x` with the cookie (the action leads to `/`), a store whose `Find` returns a `tls.RecordHeaderError` (503, which no type list names), and `/login` without a cookie once discovery is published (503 from the commit, no `Location` header, and a body equal to the styled page alone, with no `Found` link after it); a second request without a cookie to a route whose handler sets `HX-Redirect` before its `WriteHeader` (the `bounceToLogin` shape) answers 503 with no `HX-Redirect` header; a handler that puts a value in the session and sets `HX-Refresh: true` without writing (the shape of `workspace_partials.go:211-214`) answers 503 with no `HX-Refresh` header when the commit fails; each logs copy 19's first message with `method`, `path` and `error`. `TestUndecodableSessionAnswers500`, one row per store answer: bytes the codec cannot decode, an empty value (`io.EOF`) and a truncated one (`io.ErrUnexpectedEOF`) answer the styled 500 page and log copy 19's second message, with no `Retry-After`. `TestSessionFreeRoutesSkipTheStore` (`internal/server`, the route environment over the failing store, its `Config` built through `NewAwaitingDiscovery` so that it carries the tagged store and the `ErrorFunc`): `/static/app.css` answers 200 with a session cookie, `/favicon.ico` answers 404 and not 503 with a session cookie (no route serves that path, so the styled 404 answers without loading the session), `/domains/ask` without its `domain` parameter answers 400 with a cookie, as the route table's anonymous probe does, the fake mount's declared exempt path answers as without a store, and `/` with the cookie answers 503.
- [ ] 4.4 Tests. `TestSetupRefusesAStoreThatRefuses` (`internal/auth`): against the stack's session store with a password the store does not accept, `Setup` returns an error that contains `failed to connect to Valkey`. `TestSetupServesWithoutAStore`, one row per store through a named function: `valkey-addr` naming a port that refuses (listened on, then closed), and a fake store that answers every command with `-LOADING Redis is loading the dataset in memory`; a third row, a fake store that answers the connection with `-ERR max number of clients reached`, gives the same; each gives a `Config` and copy 17 at Error with `addr`. `TestSessionStoreFailureAnswers503` and `TestUndecodableSessionAnswers500` build their `Config` through `NewAwaitingDiscovery`, because they need its `ErrorFunc` and its tagged store; the `/login` row publishes the holders by storing them directly, which the test can do from inside the package. `fakeFailurePage` (`auth_failure_page_test.go:16-31`), assigned to `Failures` at `:63`, `:141`, `:164` and `:182` and at `callback_edges_test.go:307`, gains `RenderErrorPage`, recording the status and the message, so that the package's tests compile, and the tests that use it keep their results. `TestSessionStoreFailureAnswers503` (`internal/auth`, over a handler wrapped by `(*Config).LoadAndSave`, where the latch lives), with an `scs.Store` fake whose `Find` and `Commit` return a `*net.OpError`, one row per request through a named function: a browser GET with a session cookie to a page (503, `Retry-After: 5`, the styled page with copy 3's heading and `Try again` to the requested path), an htmx request with the cookie (503, the plain text `Sign-in unavailable`), a POST with the cookie (the action leads to `/`), a GET to `//evil.example/x` with the cookie (the action leads to `/`), a store whose `Find` returns a `tls.RecordHeaderError` (503, which no type list names), and `/login` without a cookie once discovery is published (503 from the commit, no `Location` header, and a body equal to the styled page alone, with no `Found` link after it); a second request without a cookie to a route whose handler sets `HX-Redirect` before its `WriteHeader` (the `bounceToLogin` shape) answers 503 with no `HX-Redirect` header; a handler that puts a value in the session and sets `HX-Refresh: true` without writing (the shape of `workspace_partials.go:211-214`) answers 503 with no `HX-Refresh` header when the commit fails; each logs copy 19's first message with `method`, `path` and `error`. `TestUndecodableSessionAnswers500`, one row per store answer: bytes the codec cannot decode, an empty value (`io.EOF`) and a truncated one (`io.ErrUnexpectedEOF`) answer the styled 500 page and log copy 19's second message, with no `Retry-After`. `TestSessionFreeRoutesSkipTheStore` (`internal/server`, the route environment over the failing store, its `Config` built through `NewAwaitingDiscovery` so that it carries the tagged store and the `ErrorFunc`): `/static/app.css` answers 200 with a session cookie, `/favicon.ico` answers 404 and not 503 with a session cookie (no route serves that path, so the styled 404 answers without loading the session), `/domains/ask` without its `domain` parameter answers 400 with a cookie, as the route table's anonymous probe does, the fake mount's declared exempt path answers as without a store, and `/` with the cookie answers 503.
- [ ] 4.5 Breaks:
- the probe fatal for every error, as today: caught by `TestSetupServesWithoutAStore`;
- the probe fatal for every error reply, `LOADING` included: caught by `TestSetupServesWithoutAStore`'s `-LOADING` row;
- the probe never fatal: caught by `TestSetupRefusesAStoreThatRefuses`;
- max clients fatal (the exception removed): caught by `TestSetupServesWithoutAStore`'s max-clients row;
- `ErrorFunc` not set (scs's default): caught by `TestSessionStoreFailureAnswers503` (`want 503`);
- the store not wrapped (`taggedStore` returns the store's errors as they are): caught by the same test (the `tls.RecordHeaderError` row answers 500);
- every error classified as a store error: caught by `TestUndecodableSessionAnswers500` (the empty-value row answers 503);
@@ -101,7 +103,7 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
- `skipSession` without `/favicon.ico`: caught by its favicon row;
- `skipSession` without the exempt list: caught by its ask row;
- the `os.Exit(1)` after `server.Start`: `Run` is not run by a test; the boot test of section 6 proves that the serving half returns the error, and the line after it is read in review.
- [ ] 4.6 `go test -race -p 1 -count=1 ./internal/auth/ ./internal/server/` passes.
- [ ] 4.6 After `test/reset-test-db.sh`, `go test -race -p 1 -count=1 ./internal/auth/ ./internal/server/` passes.
## 5. Temporal-backed actions read the connection per request
@@ -112,7 +114,7 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
- [ ] 5.5 `internal/server/operator_enrollment.go` (design D15): the issuance handler refuses a submission with `valid_until` while `Temporal.Client()` is nil, with copy 6 as the field error on `valid_until`, through the same refusal path as the form's other field errors, after the end date is parsed and checked (so a past or malformed date keeps its own field error, such as `Choose a time in the future.`, as `grant_resumption_operator_test.go:1412-1413` asserts for a save) and before `ConferGrant`. The management handler refuses, before its write and inside the `ValidUntil` callback (`operator_enrollment.go`, where `scheduleGrantExpiration` is chosen from `result.ValidUntil.Valid`), a submission whose end date differs from the grant's current one, whose new bound is set (`bound.Valid`) and while the client is nil, with the same field error, in the same place relative to the parse. A save that clears the bound is not refused. The two "no scheduler" branches and their messages are deleted; `scheduleGrantExpiration` keeps its branch for a start that fails.
- [ ] 5.6 `cmd/start.go`: build `clientholder.New()` and pass it as `serverConfig.Temporal`. When `temporal-host` is set, publish the connected client after `worker.Start` returns and before the first `Startup` hook (design D7); the hooks, pollers and schedules keep receiving the client. Temporal still connects before the listener in this section.
- [ ] 5.7 `TestNoHandlerHoldsATemporalClient` (`internal/server`, a structural test with `go/parser`): in every non-test file of `internal/server`, of `internal/integrations/*/web` and of the three adapters' own packages, no struct field's type is `client.Client` from `go.temporal.io/sdk/client`, under any import name; a failure names the file, the line and the field. The scan is a function over source text, and the test runs it on an in-memory source that imports the package as `temporalsdk` and one that imports it unaliased, each with a field of the package's `Client` type, and expects both found. The workflows packages' schedule managers, which receive a client per call from the hooks, are outside the scan, and the test's comment says why.
- [ ] 5.8 Tests. `internal/workflows/clientholder`: `TestHolder`, one row per case (a nil holder, before and after `Publish`, a second `Publish` panics) and `TestHolderUnderConcurrentReads` (readers loop on `Client()` while one goroutine publishes; every reading is nil or the client). The nine test files that pass a fake client as `TemporalClient` or `Temporal` publish the fake to a holder (`clientholder.New()`, then `Publish`), and their results do not change: `internal/integrations/fedwiki/web/oracle_test.go` and `requests_db_test.go`; in `internal/server`, `grant_resumption_operator_test.go`, `member_domains_rollback_db_test.go`, `new_handler_test.go`, `operator_integrations_stripe_test.go`, `operator_org_type_change_test.go` and `operator_stripe_public_details_test.go`; and `test/e2e/plan-management/e2e_test.go`, which `go test ./...` builds and whose `TemporalClient: nil` (`:220`) no longer compiles after item 5.2. The operator harnesses build their configuration without a client, so under D15 every save that sets an end date would be refused with copy 6 and change the result of tests that do not name the warning. The shared harnesses therefore take a published holder over a fake client whose `ExecuteWorkflow` succeeds, and every bounded save keeps its result: `newGLHarness` (`operator_grants_list_test.go:184-196`, used by `operator_grants_delivery_state_test.go:105` `TestGrantDeliveryState_EndToEnd` and `operator_grants_list_test.go:294` `issueAgreementGrants`), `newOtcHarness`, `newGRWorld` (`grant_resumption_operator_test.go:836`, `:1226` and `:1509` save with no `w.temporal()` call before them), and the plan-management `httpHarness`, whose `issueTrial` (`:345-365`) and `extendGrant` (`:389-430`) save a `valid_until` and want the trial conferred and a `transfer` transition. `operator_enrollment_manage_guard_test.go` (`TestManageGrant_SucceedsForSinglePoolOrg` at `:256`, `TestManageGrant_ExtendsExactlyTheNamedGrant` at `:292` and `:392`) takes its holder from the same harness. The e2e harness comment (`:202-205`) is rewritten to say that the harness publishes a fake client. The commit message lists these files. `TestDomainClaimReadsTheConnectionPerRequest` (`internal/server`, a database test): a member domains handler built once with an empty holder answers `Domain verification is not available right now.` and starts nothing; after `Publish` of a fake that records starts, the same handler, not rebuilt, starts the verification workflow. The same pattern for one FedWiki site action (`web/site_actions_test` or its database test) and for the Stripe environment check (`Temporal is unavailable.`, then a start). `TestWebhookBeforeTemporalAnswers503` (Stripe `web`, a database test with the committed-rows fixture of the existing receiver tests): a signed event with an empty holder answers 503, `Retry-After: 5`, the body of copy 4, a row `received`, and copy 22 logged; after `Publish`, the redelivery answers 200 and the fake recorded one start keyed by the event id. `TestWebhookBeforeTemporalRecordsFailed` (Discourse `web`, a database test): a `user_added_to_group` event for a managed group and a linked user with an empty holder answers 200, the row reads `failed` with `error_message` copy 5, and nothing panics; after `Publish`, the same kind of event reads `processed` and the reconcile started. `TestBoundedGrantNeedsTemporal` (`internal/server`, a database test), one row per case: issuance with an end date and an empty holder re-renders with copy 6 on the end date and writes no grant row; issuance without an end date issues; a management save that sets a new end date is refused with copy 6 and the grant is unchanged; a management save that clears the end date is saved with an empty holder; with a published fake, issuance with an end date issues and the fake recorded the expiration start. No test asserts the deleted "no scheduler" warnings (`git grep "no scheduler"` over the tests finds nothing), so the files above are the tests whose results the refusal would change without a published fake, and the commit message names them.
- [ ] 5.8 Tests. `internal/workflows/clientholder`: `TestHolder`, one row per case (a nil holder, before and after `Publish`, a second `Publish` panics) and `TestHolderUnderConcurrentReads` (readers loop on `Client()` while one goroutine publishes; every reading is nil or the client). The nine test files that pass a fake client as `TemporalClient` or `Temporal` publish the fake to a holder (`clientholder.New()`, then `Publish`), and their results do not change: `internal/integrations/fedwiki/web/oracle_test.go` and `requests_db_test.go`; in `internal/server`, `grant_resumption_operator_test.go`, `member_domains_rollback_db_test.go`, `new_handler_test.go`, `operator_integrations_stripe_test.go`, `operator_org_type_change_test.go` and `operator_stripe_public_details_test.go`; and `test/e2e/plan-management/e2e_test.go`, which `go test ./...` builds and whose `TemporalClient: nil` (`:220`) no longer compiles after item 5.2. The operator harnesses build their configuration without a client, so under D15 every save that sets an end date would be refused with copy 6 and change the result of tests that do not name the warning. The shared harnesses therefore take a published holder over a fake client whose `ExecuteWorkflow` succeeds, and every bounded save keeps its result: `newGLHarness` (`operator_grants_list_test.go:184-196`, used by `operator_grants_delivery_state_test.go:105` `TestGrantDeliveryState_EndToEnd` and `operator_grants_list_test.go:294` `issueAgreementGrants`), `newOtcHarness`, `newGRWorld` (built on `newGLHarness`, `grant_resumption_operator_test.go:65`, so it needs no change of its own; `:836`, `:1226` and `:1509` save with no `w.temporal()` call before them), and the plan-management `httpHarness`, whose `issueTrial` (`:345-365`) and `extendGrant` (`:389-430`) save a `valid_until` and want the trial conferred and a `transfer` transition. `operator_enrollment_manage_guard_test.go` (`TestManageGrant_SucceedsForSinglePoolOrg` at `:256`, `TestManageGrant_ExtendsExactlyTheNamedGrant` at `:292` and `:392`) takes its holder from the same harness (`newOtcHarness`). `w.temporal()` (`grant_resumption_operator_test.go:710-718`) assigns `w.h.handler.TemporalClient` and resets it to nil; once `newGLHarness` publishes its holder, a second `Publish` on it panics (item 5.1), so `w.temporal()` assigns a new published holder to the handler and its cleanup restores the harness's holder. The e2e harness comment (`:202-205`) is rewritten to say that the harness publishes a fake client. The commit message lists these files. `TestDomainClaimReadsTheConnectionPerRequest` (`internal/server`, a database test): a member domains handler built once with an empty holder answers `Domain verification is not available right now.` and starts nothing; after `Publish` of a fake that records starts, the same handler, not rebuilt, starts the verification workflow. The same pattern for one FedWiki site action (`web/site_actions_test` or its database test) and for the Stripe environment check (`Temporal is unavailable.`, then a start). `TestWebhookBeforeTemporalAnswers503` (Stripe `web`, a database test with the committed-rows fixture of the existing receiver tests): a signed event with an empty holder answers 503, `Retry-After: 5`, the body of copy 4, a row `received`, and copy 22 logged; after `Publish`, the redelivery answers 200 and the fake recorded one start keyed by the event id. `TestWebhookBeforeTemporalRecordsFailed` (Discourse `web`, a database test): a `user_added_to_group` event for a managed group and a linked user with an empty holder answers 200, the row reads `failed` with `error_message` copy 5, and nothing panics; after `Publish`, the same kind of event reads `processed` and the reconcile started. `TestBoundedGrantNeedsTemporal` (`internal/server`, a database test), one row per case: issuance with an end date and an empty holder re-renders with copy 6 on the end date and writes no grant row; issuance without an end date issues; a management save that sets a new end date is refused with copy 6 and the grant is unchanged; a management save that clears the end date is saved with an empty holder; with a published fake, issuance with an end date issues and the fake recorded the expiration start. No test asserts the deleted "no scheduler" warnings (`git grep "no scheduler"` over the tests finds nothing), so the files above are the tests whose results the refusal would change without a published fake, and the commit message names them.
- [ ] 5.9 `TestNewHandlerRouteTableNotReady` builds its environment with an empty Temporal holder as well; `routes-not-ready.golden` is regenerated, and every row that changes from section 3's version is listed in the commit message with the reason it changed (a route table probe that reaches a Temporal-backed answer).
- [ ] 5.10 Breaks:
- the member domains handler reads the client at construction (a field filled in `NewMemberDomainsHandler` from `cfg.Temporal.Client()`): caught by `TestDomainClaimReadsTheConnectionPerRequest` (no start after `Publish`);
@@ -131,14 +133,14 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
- the management refusal applied to a save that clears the end date: caught by the clearing row;
- `Publish` stores a second client without panicking: caught by `TestHolder`;
- `Publish` in `start` after the hooks: no test of this section runs `start`; the order is the connector's from section 6, whose test catches it (item 6.10).
- [ ] 5.11 `go test -race -p 1 -count=1 ./internal/workflows/clientholder/ ./internal/server/ ./internal/integrations/stripe/... ./internal/integrations/discourse/... ./internal/integrations/fedwiki/...` passes.
- [ ] 5.11 After `test/reset-test-db.sh`, `go test -race -p 1 -count=1 ./internal/workflows/clientholder/ ./internal/server/ ./internal/integrations/stripe/... ./internal/integrations/discourse/... ./internal/integrations/fedwiki/...` passes.
## 6. Temporal connects in the background, and start serves before it
- [ ] 6.1 `internal/workflows/client.go` (design D17): delete `dialWithRetry`, its constants and `ClientConfig.ConnectTimeout`. `NewClient(ctx, cfg)` becomes one `client.DialContext` under a 20-second deadline, with the OAuth headers provider and the logger as today. `client_test.go`'s four `TestDialWithRetry_*` tests are deleted with the function; a test of the single attempt's deadline replaces them (a listener that accepts and never answers; the dial returns within the shortened deadline the test sets through a package variable).
- [ ] 6.2 `internal/workflows`: `ErrCannotCheck`, a sentinel a `WorkerPrecondition` wraps when it could not read what it checks. FedWiki's `listRunsError` reports `errors.Is(err, workflows.ErrCannotCheck)`; `openRunsError` does not. The doc comment of `WorkerPrecondition` says what the connector does with each (design D10). `openRunsError.Error()` (`internal/integrations/fedwiki/fedwiki.go`) takes copy 15's text in both forms: `... is still open: <types>. Let it finish on the previous release, or terminate it; the worker starts once none is open.` and the plural; its existing test, if it compares the sentence, compares the new one.
- [ ] 6.3 `cmd/temporal_connect.go` (new, design D6, D7, D10): `connectTemporal(ctx, t temporalConnect)`, where `temporalConnect` carries the client configuration, the worker configuration, the holder, the logger, the escalation time (`temporal-connect-timeout`, 90 seconds when zero), the policy's first wait and cap (two package variables, 0.5 and 15 seconds, which the boot test of item 6.8 shortens to 50 milliseconds), and the injected `dial`, `newWorker` and `afterConnect` functions. It runs `retryloop.Run` with D17's numbers and copies 13, 14, 15 and 16, and the attempt logs nothing itself: every failure is returned as a `retryloop.Failure`. One attempt: dial (a plain error, copy 13's message); `newCheckedWorker`; on a refusal that is not `ErrCannotCheck`, keep the client and return a `Failure` with copy 15's message and level Error, marked `AtCap`, so that the next attempt reads the list again on the same client; on `ErrCannotCheck`, a build error or a `worker.Start` error, close the client and return the error (a `Failure` with copy 16's message and level Error for the worker's two failures). On success, `Publish`, then `afterConnect(ctx, c, worker)`. `afterConnect` in production is today's block of `cmd/start.go` from the `Startup` hooks to the webhook partition schedule, moved unchanged into `startTemporalWork`.
- [ ] 6.4 `cmd/start.go` (design D1, D13, D18, D19): `Run` keeps steps 1 to 5 of the design's boot sequence (the key derivations without `UseKey`), then calls `serveConsole(ctx, logger, boot, database, integs, listenTCP(":"+port))`, where `boot` carries the Stripe mode, the key's fingerprint and the integrations' specs `Run` derived, logs copy 21 when it returns and exits 1. `serveConsole` calls `stripeintegration.UseKey` first, then runs the overrides, the boot data state, the connection binding, the mounts and the holder, then `server.Bind(serverConfig, listen)`, then `go connectTemporal(...)` when `temporal-host` is set (the Warn and the backlog otherwise, as today), then the bound server's `Serve(ctx)`. When `ctx` ends it waits for `connectTemporal` and `Discover` to return (a `sync.WaitGroup`) before it returns. A comment above the first `go` statement says that every configuration write is above it and that the goroutines take their keys once (design D13). The block comments of the old Temporal section and `startStripeEnvironmentCheck`'s "boots at the same speed" paragraph are rewritten to the new order.
- [ ] 6.3 `cmd/temporal_connect.go` (new, design D6, D7, D10): `connectTemporal(ctx, t temporalConnect)`, where `temporalConnect` carries the client configuration, the worker configuration, the holder, the logger, the escalation time (`temporal-connect-timeout`, 90 seconds when zero), the policy's first wait and cap (two package variables, 0.5 and 15 seconds, which the boot test of item 6.8 shortens to 50 milliseconds), and the injected `dial`, `newWorker` and `afterConnect` functions. It runs `retryloop.Run` with D17's numbers and copies 13, 14, 15 and 16, and the attempt logs nothing itself: every failure is returned as a `retryloop.Failure`. One attempt: dial (a plain error, copy 13's message); `newCheckedWorker`; on a refusal that is not `ErrCannotCheck`, keep the client and return a `Failure` with copy 15's message and level Error, marked `AtCap`, so that the next attempt reads the list again on the same client; on `ErrCannotCheck`, a build error or a `worker.Start` error, close the client and return the error (a `Failure` with copy 16's message and level Error for the worker's two failures). On success, `Publish`, then `afterConnect(ctx, c, worker)`. `connectTemporal` and the `temporal-host` test that moves into it read the host trimmed, so a host of only whitespace counts as unset (a row of item 6.8 covers it). `afterConnect` in production is today's block of `cmd/start.go` from the `Startup` hooks to the webhook partition schedule, moved unchanged into `startTemporalWork`.
- [ ] 6.4 `cmd/start.go` (design D1, D13, D18, D19): `Run` keeps steps 1 to 5 of the design's boot sequence (the key derivations without `UseKey`), then calls `serveConsole(ctx, logger, boot, database, integs, listenTCP(":"+port))`, where `boot` carries the Stripe mode, the key's fingerprint and the integrations' specs `Run` derived, logs copy 21 when it returns and exits 1. `serveConsole` calls `stripeintegration.UseKey` first, then runs the overrides, the boot data state, the connection binding, the mounts and the holder, then `server.Bind(serverConfig, listen)`, then `go connectTemporal(...)` when `temporal-host` is set (the Warn and the backlog otherwise, as today), then the bound server's `Serve(ctx)`. When `ctx` ends it waits for `connectTemporal` and `Discover` to return (a `sync.WaitGroup`) before it returns. A comment above the first `go` statement says that every configuration write is above it and that the goroutines take their keys once (design D13). The words "before any goroutine starts" are no longer true once `UseKey` runs inside `serveConsole`, after `openDB` has started database/sql's connection opener, so they become "before the listener, and before any goroutine that reads the key" in the `UseKey` doc comment (`stripe.go:288-291`) and in the comment above the call in `cmd/start.go`. The block comments of the old Temporal section and `startStripeEnvironmentCheck`'s "boots at the same speed" paragraph are rewritten to the new order.
- [ ] 6.5 `internal/server/server.go` (design D19): `Start(ctx, cfg)` becomes two steps. `Bind(cfg, listen func() (net.Listener, error))` runs `auth.Setup`, `NewHandler` and `listen` and returns the bound server, or the first error having started nothing. `(*Bound).Serve(ctx)` starts the discovery loop (`go authConfig.Discover(ctx)`) and serves on the listener; when `ctx` ends it calls `Shutdown` with a bounded context (five seconds), waits for `Discover` to return, and returns. Their doc comments say that nothing before `listen` waits on Temporal or the identity provider and that a failure of `Bind` starts no background work.
- [ ] 6.6 `--temporal-connect-timeout`'s help is copy 23; the viper default stays 90 seconds.
- [ ] 6.7 `internal/integrations/stripe/workflows/webhook.go` (design F34): `ProcessWebhookEvent`'s first `UPDATE` adds `AND status IN ('received','processing','failed')` and the activity checks the rows affected; when none is, it returns nil without dispatching, with a comment that says the sweep and a redelivery can start a completed event's id again. The guard is in the activity because the sweep's status read and the workflow start are two steps, and the workflow id's reuse policy lets a completed id run again. Test (`TestProcessWebhookEventSkipsAFinishedEvent`, a database test, one row per status through a named function): a row at `completed`, `skipped`, `refused` and `dead_letter`, with an event type whose handler fails on the empty payload, is returned nil, unchanged and not dispatched; a row at `received`, one at `processing` (a worker that died mid-attempt, which Temporal then retries) and one at `failed` are each dispatched: the activity returns the dispatch error, and the row reads `failed` with `retry_count` 1 and the error, because the handler fails on the payload and the row does not rest at `processing`.
@@ -174,7 +176,7 @@ Sections 1 to 7 are one commit each, and their items are that commit's parts; se
- [ ] 7.3 `docs/fedwiki-setup.md`, "Serving authorization": the paragraph about an unreachable console says that the console answers the ask as soon as it serves, whatever Temporal, the identity provider and the session store do, and keeps the bound for a console that is down.
- [ ] 7.4 `docs/identity-provider-setup.md` and `internal/auth/README.md`: "refuses to start" and "fails startup" become what the code does: the console serves and keeps sign-in unavailable, logging at Error, until the provider publishes the endpoint. The logout section says what sign-out does without an end-session endpoint, and the provider setup says that a document with the configured issuer and no `end_session_endpoint` leaves sign-in unavailable but does not stop session re-derivation.
- [ ] 7.5 `docs/temporal-authorization-setup.md`: one paragraph on the token URL: an internal address keeps the Temporal connection off the TLS proxy; the console no longer needs it before it serves.
- [ ] 7.6 `docs/stripe.md` and `docs/discourse-setup.md`: one sentence each that `temporal-host` is required, and why.
- [ ] 7.6 `docs/stripe.md` and `docs/discourse-setup.md`: one sentence each that `temporal-host` is required, and why. `docs/building-an-integration.md` section 6 gains a `NeedsTemporal` bullet: it goes on a key of the RequiredGroup of an integration that receives events processed only on Temporal, and boot refuses the configured group without `temporal-host`.
- [ ] 7.7 No code changes; `make test` passes.
## 8. Landing