Factory: a keyed create that wrote nothing runs again, announced as keyed_create; a rate-limited poll says so

docs/changes/unreleased/2619-factory-keyed-retry.md

@@ -0,0 +1,36 @@
+---
+kind: fixed
+title: A keyed Factory create runs again when it wrote nothing; a rate-limited poll says so
+pr: 2619
+surface: [engine, factory]
+invalidates:
+  - "Was true: a keyed Factory job create replayed its first answer for 24 hours, even a server error or refusal given before anything was written. Is true now: such an answer carries X-Codeaf-Phase pre-mutation and is not kept, so the same key runs the create again."
+  - "Was true: GET /v1/features listed group_policy and waits_on, or nothing on a factory without a store. Is true now: it also lists keyed_create, on every factory with this rule."
+  - "Was true: a source poll that GitHub rate limited read not reachable. Is true now: a 429, or a 403 that says when to retry, reads rate limited."
+---
+
+A job create (`POST /v1/jobs`, `POST /v1/issues`) sent with an
+`Idempotency-Key` now keeps only the answers it gave after its first write.
+An answer given before then, whether a refusal, a sign-in or read failure,
+GitHub refusing the issue or GitHub not answering before the issue request was
+sent, carries `X-Codeaf-Phase: pre-mutation` and is not kept. A resend with the
+same key then runs the create again and succeeds once the cause has passed.
+An answer given once the issue request may have reached GitHub, or once the job
+exists, is still replayed for 24 hours, a 5xx or 408 included, so a resend
+under the same sign-in does not make a second job or GitHub issue. Two gaps
+remain: a resend under a new bearer or session cookie is a different caller,
+and a Factory that stops after the create commits but before its answer is
+saved runs the resend again. Unkeyed creates carry the same marker.
+Worker and runner routes keep their rule: a 5xx, 408 or 429 is never kept.
+
+`GET /v1/features` (`factory.features`) lists `keyed_create`, so an engine can
+tell this Factory from an older one, which keeps every keyed answer for 24
+hours, refusals and server errors included, and send a create key only here.
+
+A source poll's trouble words say "rate limited" for any GitHub 429 and for a
+403 that carries a retry time; a 403 without one still reads "not reachable".
+
+The poll words are carried from #2460. The keyed rule replaces #2439's, which
+stopped keeping every 5xx, 408 and 429, so a create that failed after making
+its job or issue ran again and made a second one. Both merged only into the
+closed integration branch of #2306.

docs/design/app-v3/PROTOCOL.md

@@ -402,8 +402,9 @@ sent; an unconfirmed write is `uncertain`.
 An engine whose hello carries `factory.group_policy` and `factory.waits_set`
 (and capabilities `group_policy: true`, `waits_on: true`) serves go-aheads the
 factory follows by itself. `factory.features {factory}` answers `{features}`, what
-that one factory keeps (`GET /v1/features`): `group_policy` and `waits_on`, or `[]`
-from a factory older than the route. `factory.policy_get {factory, group}` and
+that one factory keeps (`GET /v1/features`): `group_policy` and `waits_on` (a factory
+with a store), `keyed_create` (the keyed create rule below), or `[]` from a factory
+older than the route. `factory.policy_get {factory, group}` and
 `factory.policy_set {factory, group, stopped, allowances}` (`GET /v1/policy?group=`,
 `PUT /v1/policy`) answer `{group, version?, stopped, owner?, updated_at?, spent_usd,
 allowances: [{id, jobs, cap_usd, ends_at, spent_usd, state}]}`; a set replaces the
@@ -432,6 +433,30 @@ carries `waits_on` and `waiting_on`.
 Stopping, ending or withdrawing an allowance starts nothing further and stops nothing
 already running.
 
+A Factory job create (`POST /v1/jobs`, or `POST /v1/issues` when it opens the
+GitHub issue first) sent with an `Idempotency-Key` header is remembered for 24
+hours per caller, route and key, so a resend with the same key does not make a
+second job or issue, within two limits. The caller is the exact `Authorization`
+and `Cookie` headers: a resend under a new bearer or a rotated session cookie is
+not matched to the first answer and runs as a new create. And the answer is saved
+only after the create has committed, so a Factory that stops between the two runs
+the resend again. A Factory with this rule lists `keyed_create` in
+`GET /v1/features`; an older one keeps every keyed answer, a refusal or a 5xx
+included, for 24 hours, so an engine sends a create key only to a Factory that
+lists it. An answer given before the create wrote anything carries
+the response header `X-Codeaf-Phase: pre-mutation` and is not kept, so the same
+key runs the create again. Such answers are refusals of the request (a bad field,
+a repository the caller cannot see or that is off, a missing pipeline), sign-in and
+read failures, GitHub's own 4xx refusal of the issue, and GitHub not answering
+before the issue request was sent. An answer given once the issue request may
+have reached GitHub or the job exists is replayed unchanged, including a 5xx or
+408. Examples are a lost or 5xx GitHub answer, or a job made whose start failed.
+After an `uncertain` keyed create, resend the same key: a create that wrote nothing
+runs again, and one that did returns its first answer. If that is the same failure
+again, read the queue, since the job or issue may exist. A create without a key is
+never replayed. Worker and runner routes keep their own rule: a 5xx, 408 or 429 is
+never kept, and a report refused as invalid is checked again.
+
 `factory.job_create` keeps `title` trimmed and separate from literal `text`,
 including details whitespace in the job ask and body; empty details stay empty.
 Neither field lifts

internal/factory/api/goahead.go

@@ -15,7 +15,8 @@ import (
 // ([Features]). A factory older than these answers 404, which a client reads
 // as "this factory does not have it".
 
-// The words GET /v1/features answers.
+// The words GET /v1/features answers, with [FeatureKeyedCreate]
+// (keyed_create.go).
 const (
 	// FeatureGroupPolicy: the factory keeps group policies and starts the jobs
 	// they cover by itself.

internal/factory/api/keyed_create.go

@@ -0,0 +1,11 @@
+package api
+
+// FeatureKeyedCreate is the GET /v1/features word of a Factory that keeps a
+// keyed job create's answer only once the create has written something. A
+// POST /v1/jobs or POST /v1/issues sent with an Idempotency-Key and answered
+// before any write carries PhaseHeader: PhasePreMutation and is not kept, so
+// the same key runs the create again; an answer given once the job or its
+// issue may exist is replayed for 24 hours. An engine sends a create key only
+// to a Factory that lists this word: an older one keeps every keyed answer, a
+// refusal or a 5xx included, and would replay it for a day.
+const FeatureKeyedCreate = "keyed_create"

internal/factory/github/poll.go

@@ -199,6 +199,14 @@ func TroubleWords(err error) string {
 			return "token refused"
 		case http.StatusNotFound:
 			return "repository not found"
+		case http.StatusTooManyRequests:
+			return "rate limited"
+		case http.StatusForbidden:
+			// GitHub also answers 403 for a spent limit; only one that says
+			// when to come back is a limit and not a refusal.
+			if apiErr.RetryAfter > 0 {
+				return "rate limited"
+			}
 		}
 	}
 	return "not reachable"

internal/factory/github/poll_limit_test.go

@@ -0,0 +1,36 @@
+package github
+
+import (
+	"fmt"
+	"testing"
+	"time"
+
+	forge "github.com/Agent-Field/codeaf/internal/praf/github"
+)
+
+// B5: Spent limits have their own facts words, including wrapped errors;
+// ordinary refusals and the existing 401/404 words remain unchanged.
+func TestTroubleWordsNameASpentRateLimit(t *testing.T) {
+	for _, tc := range []struct {
+		status int
+		retry  time.Duration
+		want   string
+	}{
+		{403, time.Minute, "rate limited"},
+		{429, time.Minute, "rate limited"},
+		{403, 0, "not reachable"},
+		{429, 0, "rate limited"},
+		{500, time.Minute, "not reachable"},
+		{401, 0, "token refused"},
+		{401, time.Minute, "token refused"},
+		{404, 0, "repository not found"},
+		{404, time.Minute, "repository not found"},
+	} {
+		t.Run(fmt.Sprintf("%d/%s", tc.status, tc.retry), func(t *testing.T) {
+			err := fmt.Errorf("repository read: %w", &forge.APIError{StatusCode: tc.status, RetryAfter: tc.retry})
+			if got := TroubleWords(err); got != tc.want {
+				t.Fatalf("trouble words = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}

internal/factory/serve/goahead.go

@@ -19,10 +19,13 @@ func (h *Handler) goAheadRoutes() {
 	h.postJob("waits_on", h.config.Store != nil, h.onlyOwner(h.setWaits))
 }
 
-// features lists what this factory does beyond its contract. Both words need
-// the factory's store, which is where policies and waits are kept.
+// features lists what this factory does beyond its contract. The go-ahead
+// words need the factory's store, which is where policies and waits are kept.
+// keyed_create does not: the rule that a keyed create answered before its
+// first write is not kept lives in the request path (keepReplay), and an
+// engine sends a create key only to a factory that lists it.
 func (h *Handler) features(w http.ResponseWriter, r *http.Request) {
-	out := api.Features{Features: []string{}}
+	out := api.Features{Features: []string{api.FeatureKeyedCreate}}
 	if h.config.Store != nil {
 		out.Features = append(out.Features, api.FeatureGroupPolicy, api.FeatureWaitsOn)
 	}

internal/factory/serve/idempotency.go

@@ -54,9 +54,9 @@ func (h *Handler) idempotent(w http.ResponseWriter, r *http.Request) {
 			}
 		}
 		row, ok := h.replays.rows[id]
-		if !ok || !cacheWorkerResponse(r, row) {
-			row = h.capture(r)
-			if cacheWorkerResponse(r, row) {
+		if !ok || !keepReplay(r, row) {
+			row = h.capture(w, r)
+			if keepReplay(r, row) {
 				h.replays.rows[id] = row
 			}
 		}
@@ -86,9 +86,9 @@ func (h *Handler) idempotent(w http.ResponseWriter, r *http.Request) {
 	if err == nil && h.failed(w, json.Unmarshal(raw, &row)) {
 		return
 	}
-	if row.At.IsZero() || h.clock().Sub(row.At) >= 24*time.Hour || !cacheWorkerResponse(r, row) {
-		row = h.capture(r)
-		if !cacheWorkerResponse(r, row) {
+	if row.At.IsZero() || h.clock().Sub(row.At) >= 24*time.Hour || !keepReplay(r, row) {
+		row = h.capture(w, r)
+		if !keepReplay(r, row) {
 			sendReplay(w, row)
 			return
 		}
@@ -106,13 +106,29 @@ func (h *Handler) idempotent(w http.ResponseWriter, r *http.Request) {
 	sendReplay(w, row)
 }
 
-// A retryable worker refusal must not become a cached day-long outage. A report
-// refused by validation has not committed its results: after a server repair it
-// must be validated again, including when an older server cached its 422. Other
-// verbs retain their original response and successful reports stay deduplicated.
-// This bridge still has a handler-commit/response-cache crash gap; only an
-// acknowledgment committed with the job itself can close it.
-func cacheWorkerResponse(r *http.Request, row replay) bool {
+// keepReplay is whether a keyed answer is kept and replayed for 24 hours. A
+// KEYED ANSWER IS REPLAYED UNLESS ITS HANDLER SAID IT COMMITTED NOTHING, so a
+// resend can never make a change twice, whatever the first answer's status.
+//
+// A handler says so with the phase marker (api.PhaseHeader, phase.go): its
+// route starts marked and the handler clears the marker immediately before its
+// first write. An answer still marked wrote nothing, a refusal or a 5xx alike,
+// so it is not kept and the same key runs again once the cause has passed or
+// been fixed. An answer given after the first write, including a 5xx, a 408 or
+// a lost GitHub answer, is kept. A row an older build kept never carries the
+// marker, so it is replayed too.
+//
+// Worker and runner routes keep their own rule. A retryable worker refusal
+// must not become a cached day-long outage. A report refused by validation has
+// not committed its results: after a server repair it must be validated again,
+// including when an older server cached its 422. Other verbs retain their
+// original response and successful reports stay deduplicated. This bridge
+// still has a handler-commit/response-cache crash gap; only an acknowledgment
+// committed with the job itself can close it.
+func keepReplay(r *http.Request, row replay) bool {
+	if row.Header.Get(api.PhaseHeader) == api.PhasePreMutation {
+		return false
+	}
 	if api.ReportCommittedNothing(row.Status) &&
 		(strings.HasPrefix(r.URL.Path, "/v1/workers/leases/") || strings.HasPrefix(r.URL.Path, "/v1/runners/leases/")) &&
 		strings.HasSuffix(r.URL.Path, "/report") {
@@ -121,13 +137,22 @@ func cacheWorkerResponse(r *http.Request, row replay) bool {
 	return !workerPath(r.URL.Path) || !api.WorkerResendable(row.Status)
 }
 
-func (h *Handler) capture(r *http.Request) replay {
+// capture runs the handler into a buffer. The phase marker ServeHTTP put on
+// the real writer goes into the buffer, where the handler's clearPhase reaches
+// it, so the captured row says whether the handler wrote anything.
+func (h *Handler) capture(real http.ResponseWriter, r *http.Request) replay {
 	w := &replayWriter{header: make(http.Header)}
+	if phase := real.Header().Get(api.PhaseHeader); phase != "" {
+		w.header.Set(api.PhaseHeader, phase)
+	}
 	h.mux.ServeHTTP(w, r)
 	return replay{At: h.clock(), Status: w.status(), Header: w.Header().Clone(), Body: w.body.Bytes()}
 }
 
+// sendReplay answers with row alone: the marker ServeHTTP put on the real
+// writer is the row's to carry, and a kept row never carries it.
 func sendReplay(w http.ResponseWriter, row replay) {
+	w.Header().Del(api.PhaseHeader)
 	for k, vals := range row.Header {
 		w.Header()[k] = append([]string(nil), vals...)
 	}