|
|
|
@@ -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
|
|
|
|
|