From 1fb665eb8b4bee985bdf84798e8a1367d1f81732 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 12:13:34 -0500 Subject: [PATCH 1/6] Factory: a keyed request is never answered from a cached 5xx or 429; a rate-limited poll says so Carried from #2439 and #2460 on workstreams/main (closed integration PR #2306). The idempotency cache no longer keeps or replays a try-again answer (5xx, 408, 429), including rows an older build stored, so a keyed retry after a transient failure can succeed. A source poll that GitHub answers 429, or 403 with a retry time, reads "rate limited" instead of "not reachable". Co-Authored-By: Claude Mythos 5.2 --- internal/factory/github/poll.go | 8 + internal/factory/github/poll_limit_test.go | 36 +++++ internal/factory/serve/idempotency.go | 5 + .../serve/idempotency_transient_test.go | 145 ++++++++++++++++++ 4 files changed, 194 insertions(+) create mode 100644 internal/factory/github/poll_limit_test.go create mode 100644 internal/factory/serve/idempotency_transient_test.go diff --git a/internal/factory/github/poll.go b/internal/factory/github/poll.go index f2a2c98da0..5b1e00a072 100644 --- a/internal/factory/github/poll.go +++ b/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" diff --git a/internal/factory/github/poll_limit_test.go b/internal/factory/github/poll_limit_test.go new file mode 100644 index 0000000000..445c7fe092 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/factory/serve/idempotency.go b/internal/factory/serve/idempotency.go index 872f553e21..2324d8c230 100644 --- a/internal/factory/serve/idempotency.go +++ b/internal/factory/serve/idempotency.go @@ -113,6 +113,11 @@ func (h *Handler) idempotent(w http.ResponseWriter, r *http.Request) { // 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 { + // Replaying a try-again answer (5xx, 408, 429) for a day defeats the retry + // it asks for, on every keyed route and not only a worker's. + if api.WorkerResendable(row.Status) { + 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") { diff --git a/internal/factory/serve/idempotency_transient_test.go b/internal/factory/serve/idempotency_transient_test.go new file mode 100644 index 0000000000..0bdf35c9be --- /dev/null +++ b/internal/factory/serve/idempotency_transient_test.go @@ -0,0 +1,145 @@ +package serve + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory/store" +) + +func transientReplayHandler(t *testing.T, durable bool) (*Handler, *time.Time) { + t.Helper() + clock := time.Date(2026, 11, 5, 12, 0, 0, 0, time.UTC) + h := &Handler{mux: http.NewServeMux(), config: Config{Now: func() time.Time { return clock }}} + if durable { + var err error + h.config.Store, err = store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + } + return h, &clock +} + +func transientKeyedRequest() *http.Request { + r := httptest.NewRequest("POST", "/v1/jobs?mode=test", strings.NewReader(`{}`)) + r.Header.Set("Authorization", "Bearer reader") + r.Header.Set("Cookie", "session=reader") + r.Header.Set("Idempotency-Key", "same-attempt") + return r +} + +func seedTransientReplay(t *testing.T, h *Handler, status int) { + t.Helper() + r := transientKeyedRequest() + sum := sha256.Sum256([]byte(r.Header.Get("Authorization") + "\x00" + r.Header.Get("Cookie") + "\x00" + r.URL.RequestURI() + "\x00" + r.Header.Get("Idempotency-Key"))) + id := hex.EncodeToString(sum[:]) + row := replay{At: h.clock(), Status: status, Body: []byte("old transient answer")} + if h.config.Store == nil { + h.replays.rows = map[string]replay{id: row} + return + } + dir := filepath.Join(h.config.Store.Root(), "idempotency") + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatal(err) + } + raw, err := json.Marshal(row) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, id+".json"), raw, 0600); err != nil { + t.Fatal(err) + } +} + +// C1, C2, C4: A transient answer reaches its caller once, then the same key +// runs again and replays its success. Older memory and disk rows are bypassed. +func TestAKeyedServerErrorRunsAgainOnRetry(t *testing.T) { + for _, durable := range []bool{false, true} { + for _, seeded := range []bool{false, true} { + for _, status := range []int{500, 503, 429} { + name := strconv.Itoa(status) + map[bool]string{false: "/memory", true: "/disk"}[durable] + map[bool]string{false: "/fresh", true: "/old-row"}[seeded] + t.Run(name, func(t *testing.T) { + h, _ := transientReplayHandler(t, durable) + if seeded { + seedTransientReplay(t, h, status) + } + calls := 0 + h.mux.HandleFunc("/v1/jobs", func(w http.ResponseWriter, _ *http.Request) { + calls++ + code, body := 200, "accepted" + if !seeded && calls == 1 { + code, body = status, "retry this answer" + } + w.Header().Set("X-Answer", body) + w.WriteHeader(code) + _, _ = w.Write([]byte(body)) + }) + for attempt := range 3 { + w := httptest.NewRecorder() + h.idempotent(w, transientKeyedRequest()) + wantStatus, wantBody, wantCalls := 200, "accepted", 2 + if seeded { + wantCalls = 1 + if durable { + // A fresh handler must read the replacement from disk. + h = &Handler{mux: h.mux, config: h.config} + } + } else if attempt == 0 { + wantStatus, wantBody, wantCalls = status, "retry this answer", 1 + } + if w.Code != wantStatus || w.Body.String() != wantBody || w.Header().Get("X-Answer") != wantBody || calls != wantCalls { + t.Fatalf("attempt %d = %d %q, header=%q calls=%d; want %d %q calls=%d", attempt, w.Code, w.Body.String(), w.Header().Get("X-Answer"), calls, wantStatus, wantBody, wantCalls) + } + } + }) + } + } + } +} + +// C3: Successful and definitive refusal answers still replay for 24 hours +// in memory and across a store-backed restart, then expire as before. +func TestKeyedSuccessAndRefusalsStillReplay(t *testing.T) { + for _, durable := range []bool{false, true} { + for _, status := range []int{200, 422} { + t.Run(strconv.Itoa(status)+map[bool]string{false: "/memory", true: "/disk"}[durable], func(t *testing.T) { + h, clock := transientReplayHandler(t, durable) + calls := 0 + h.mux.HandleFunc("/v1/jobs", func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.WriteHeader(status) + _, _ = w.Write([]byte(strconv.Itoa(calls))) + }) + for attempt := range 3 { + w := httptest.NewRecorder() + h.idempotent(w, transientKeyedRequest()) + wantCalls := 1 + if attempt == 2 { + wantCalls = 2 + } + if w.Code != status || w.Body.String() != strconv.Itoa(wantCalls) || calls != wantCalls { + t.Fatalf("attempt %d = %d %q calls=%d, want status=%d calls=%d", attempt, w.Code, w.Body.String(), calls, status, wantCalls) + } + if attempt == 0 { + *clock = clock.Add(24*time.Hour - time.Nanosecond) + if durable { + h = &Handler{mux: h.mux, config: h.config} + } + } else { + *clock = clock.Add(time.Nanosecond) + } + } + }) + } + } +} From 3489017f304afe8e98df1c87d74fc7775e68574a Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 12:22:16 -0500 Subject: [PATCH 2/6] changes: entry for this pull request Co-Authored-By: Claude Mythos 5.2 --- .../unreleased/2619-factory-keyed-retry.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 docs/changes/unreleased/2619-factory-keyed-retry.md diff --git a/docs/changes/unreleased/2619-factory-keyed-retry.md b/docs/changes/unreleased/2619-factory-keyed-retry.md new file mode 100644 index 0000000000..2dad56109f --- /dev/null +++ b/docs/changes/unreleased/2619-factory-keyed-retry.md @@ -0,0 +1,18 @@ +--- +kind: fixed +title: A keyed Factory request runs again after a server error; a rate-limited poll says so +pr: 2619 +surface: [engine, factory] +invalidates: + - "Was true: a keyed Factory request replayed its first answer for 24 hours even when that answer was a server error. Is true now: 5xx, 408 and 429 answers are not replayed, so a retry with the same key runs again." + - "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." +--- + +Under an `Idempotency-Key`, 5xx, 408 and 429 answers are no longer kept or +replayed, including rows stored by an older build; successes and other +refusals still replay for 24 hours. 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". + +Carried from #2439 and #2460, which merged only into the closed integration +branch of #2306. From 9b590fe6956774fdf6f5ad3546d4906425a9c65f Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 18:28:27 -0500 Subject: [PATCH 3/6] Factory: a keyed create is replayed unless it wrote nothing The keyed-answer rule this branch carried from #2439 stopped keeping every 5xx, 408 and 429. A job create that failed after it had made its job, or after its GitHub issue request had gone out, then ran again on the same-key resend the engine calls safe after an uncertain answer, and made a second job or a second GitHub issue. The rule is now: a keyed answer is replayed unless its handler said it committed nothing. The handler says so with the existing phase marker (X-Codeaf-Phase: pre-mutation). POST /v1/jobs and /v1/issues now start marked. newJob clears the marker immediately before seam.New/NewJob, and githubJobJSON clears it once GitHub may have acted: the request got a connection, or GitHub answered anything but its own 4xx refusal. An answer still marked (a refusal, the visibility check, a sign-in or read failure, a GitHub refusal, or GitHub unreachable before the request was sent) is not kept, so a resend runs again once the cause passes. Every answer after a write is replayed, a 5xx included. The capture writer now receives the marker from the real writer, so clearPhase reaches it, and a replay never carries it. Worker and runner routes keep dev's rule. Co-Authored-By: Claude Opus 5.5 --- .../unreleased/2619-factory-keyed-retry.md | 28 +- docs/design/app-v3/PROTOCOL.md | 17 + internal/factory/serve/idempotency.go | 54 +++- .../serve/idempotency_transient_test.go | 137 ++++++-- internal/factory/serve/job_screens.go | 19 +- internal/factory/serve/keyed_create_test.go | 301 ++++++++++++++++++ internal/factory/serve/phase.go | 6 +- internal/factory/serve/verbs.go | 9 + internal/manual/chat/factory.md | 9 + 9 files changed, 522 insertions(+), 58 deletions(-) create mode 100644 internal/factory/serve/keyed_create_test.go diff --git a/docs/changes/unreleased/2619-factory-keyed-retry.md b/docs/changes/unreleased/2619-factory-keyed-retry.md index 2dad56109f..0b0fb089d6 100644 --- a/docs/changes/unreleased/2619-factory-keyed-retry.md +++ b/docs/changes/unreleased/2619-factory-keyed-retry.md @@ -1,18 +1,28 @@ --- kind: fixed -title: A keyed Factory request runs again after a server error; a rate-limited poll says so +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 request replayed its first answer for 24 hours even when that answer was a server error. Is true now: 5xx, 408 and 429 answers are not replayed, so a retry with the same key runs again." + - "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: 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." --- -Under an `Idempotency-Key`, 5xx, 408 and 429 answers are no longer kept or -replayed, including rows stored by an older build; successes and other -refusals still replay for 24 hours. 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". +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 +never makes a second job or GitHub issue. Unkeyed creates carry the same marker. +Worker and runner routes keep their rule: a 5xx, 408 or 429 is never kept. -Carried from #2439 and #2460, which merged only into the closed integration -branch of #2306. +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. diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 897d253df7..b28b34a961 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -432,6 +432,23 @@ 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, and a resend with the same key never makes a +second job or issue. 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 diff --git a/internal/factory/serve/idempotency.go b/internal/factory/serve/idempotency.go index 2324d8c230..93e4d6738a 100644 --- a/internal/factory/serve/idempotency.go +++ b/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,16 +106,27 @@ 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 { - // Replaying a try-again answer (5xx, 408, 429) for a day defeats the retry - // it asks for, on every keyed route and not only a worker's. - if api.WorkerResendable(row.Status) { +// 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) && @@ -126,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...) } diff --git a/internal/factory/serve/idempotency_transient_test.go b/internal/factory/serve/idempotency_transient_test.go index 0bdf35c9be..61cc3177fa 100644 --- a/internal/factory/serve/idempotency_transient_test.go +++ b/internal/factory/serve/idempotency_transient_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/Agent-Field/codeaf/internal/factory/api" "github.com/Agent-Field/codeaf/internal/factory/store" ) @@ -30,17 +31,28 @@ func transientReplayHandler(t *testing.T, durable bool) (*Handler, *time.Time) { return h, &clock } -func transientKeyedRequest() *http.Request { - r := httptest.NewRequest("POST", "/v1/jobs?mode=test", strings.NewReader(`{}`)) +func transientKeyedRequest(path string) *http.Request { + r := httptest.NewRequest("POST", path+"?mode=test", strings.NewReader(`{}`)) r.Header.Set("Authorization", "Bearer reader") r.Header.Set("Cookie", "session=reader") r.Header.Set("Idempotency-Key", "same-attempt") return r } -func seedTransientReplay(t *testing.T, h *Handler, status int) { +// servedKeyed sends r as ServeHTTP does: a route that markPreMutation lists +// starts with the phase marker on the writer, which only its handler clears. +func servedKeyed(h *Handler, r *http.Request) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + if markPreMutation(r) { + w.Header().Set(api.PhaseHeader, api.PhasePreMutation) + } + h.idempotent(w, r) + return w +} + +func seedTransientReplay(t *testing.T, h *Handler, path string, status int) { t.Helper() - r := transientKeyedRequest() + r := transientKeyedRequest(path) sum := sha256.Sum256([]byte(r.Header.Get("Authorization") + "\x00" + r.Header.Get("Cookie") + "\x00" + r.URL.RequestURI() + "\x00" + r.Header.Get("Idempotency-Key"))) id := hex.EncodeToString(sum[:]) row := replay{At: h.clock(), Status: status, Body: []byte("old transient answer")} @@ -61,44 +73,109 @@ func seedTransientReplay(t *testing.T, h *Handler, status int) { } } -// C1, C2, C4: A transient answer reaches its caller once, then the same key -// runs again and replays its success. Older memory and disk rows are bypassed. +func storeName(durable bool) string { return map[bool]string{false: "/memory", true: "/disk"}[durable] } + +// C1, C2, C4: An answer that wrote nothing reaches its caller once, then the +// same key runs again and replays its success. On a job create that is any +// answer still carrying the phase marker, a refusal as much as a 5xx; on a +// worker's route it is a 5xx, 408 or 429, and an older build's row of one is +// bypassed, as before. func TestAKeyedServerErrorRunsAgainOnRetry(t *testing.T) { + for _, route := range []struct { + path string + statuses []int + seeded []bool + }{ + {"/v1/jobs", []int{500, 503, 429, 408, 422, 409}, []bool{false}}, + {"/v1/runners/pull", []int{500, 503, 429, 408}, []bool{false, true}}, + } { + for _, durable := range []bool{false, true} { + for _, seeded := range route.seeded { + for _, status := range route.statuses { + name := route.path + "/" + strconv.Itoa(status) + storeName(durable) + map[bool]string{false: "/fresh", true: "/old-row"}[seeded] + t.Run(name, func(t *testing.T) { + h, _ := transientReplayHandler(t, durable) + if seeded { + seedTransientReplay(t, h, route.path, status) + } + calls := 0 + h.mux.HandleFunc(route.path, func(w http.ResponseWriter, _ *http.Request) { + calls++ + code, body := 200, "accepted" + if !seeded && calls == 1 { + code, body = status, "retry this answer" + } else { + clearPhase(w) + } + w.Header().Set("X-Answer", body) + w.WriteHeader(code) + _, _ = w.Write([]byte(body)) + }) + for attempt := range 3 { + w := servedKeyed(h, transientKeyedRequest(route.path)) + wantStatus, wantBody, wantCalls, wantPhase := 200, "accepted", 2, "" + if seeded { + wantCalls = 1 + if durable { + // A fresh handler must read the replacement from disk. + h = &Handler{mux: h.mux, config: h.config} + } + } else if attempt == 0 { + wantStatus, wantBody, wantCalls = status, "retry this answer", 1 + if route.path == "/v1/jobs" { + wantPhase = api.PhasePreMutation + } + } + if w.Code != wantStatus || w.Body.String() != wantBody || w.Header().Get("X-Answer") != wantBody || calls != wantCalls || w.Header().Get(api.PhaseHeader) != wantPhase { + t.Fatalf("attempt %d = %d %q, header=%q phase=%q calls=%d; want %d %q phase=%q calls=%d", attempt, w.Code, w.Body.String(), w.Header().Get("X-Answer"), w.Header().Get(api.PhaseHeader), calls, wantStatus, wantBody, wantPhase, wantCalls) + } + } + }) + } + } + } + } +} + +// A keyed answer given after the handler's first change is kept, whatever its +// status, so a resend can never make that change twice. The replay never +// carries the phase marker. A create row an older build kept is replayed for +// the same reason: nothing in it says the create wrote nothing. +func TestAKeyedAnswerAfterAWriteIsReplayed(t *testing.T) { for _, durable := range []bool{false, true} { for _, seeded := range []bool{false, true} { - for _, status := range []int{500, 503, 429} { - name := strconv.Itoa(status) + map[bool]string{false: "/memory", true: "/disk"}[durable] + map[bool]string{false: "/fresh", true: "/old-row"}[seeded] + for _, status := range []int{500, 502, 503, 429, 408, 409} { + name := strconv.Itoa(status) + storeName(durable) + map[bool]string{false: "/fresh", true: "/old-row"}[seeded] t.Run(name, func(t *testing.T) { h, _ := transientReplayHandler(t, durable) + wantBody := "the job exists; its start failed" if seeded { - seedTransientReplay(t, h, status) + seedTransientReplay(t, h, "/v1/jobs", status) + wantBody = "old transient answer" } calls := 0 h.mux.HandleFunc("/v1/jobs", func(w http.ResponseWriter, _ *http.Request) { calls++ - code, body := 200, "accepted" - if !seeded && calls == 1 { - code, body = status, "retry this answer" + clearPhase(w) + if calls > 1 { + w.WriteHeader(201) + _, _ = w.Write([]byte("a second job")) + return } - w.Header().Set("X-Answer", body) - w.WriteHeader(code) - _, _ = w.Write([]byte(body)) + w.WriteHeader(status) + _, _ = w.Write([]byte("the job exists; its start failed")) }) + wantCalls := 1 + if seeded { + wantCalls = 0 + } for attempt := range 3 { - w := httptest.NewRecorder() - h.idempotent(w, transientKeyedRequest()) - wantStatus, wantBody, wantCalls := 200, "accepted", 2 - if seeded { - wantCalls = 1 - if durable { - // A fresh handler must read the replacement from disk. - h = &Handler{mux: h.mux, config: h.config} - } - } else if attempt == 0 { - wantStatus, wantBody, wantCalls = status, "retry this answer", 1 + w := servedKeyed(h, transientKeyedRequest("/v1/jobs")) + if w.Code != status || w.Body.String() != wantBody || calls != wantCalls || w.Header().Get(api.PhaseHeader) != "" { + t.Fatalf("attempt %d = %d %q phase=%q calls=%d; want %d %q replayed, calls=%d", attempt, w.Code, w.Body.String(), w.Header().Get(api.PhaseHeader), calls, status, wantBody, wantCalls) } - if w.Code != wantStatus || w.Body.String() != wantBody || w.Header().Get("X-Answer") != wantBody || calls != wantCalls { - t.Fatalf("attempt %d = %d %q, header=%q calls=%d; want %d %q calls=%d", attempt, w.Code, w.Body.String(), w.Header().Get("X-Answer"), calls, wantStatus, wantBody, wantCalls) + if durable { + h = &Handler{mux: h.mux, config: h.config} } } }) @@ -112,7 +189,7 @@ func TestAKeyedServerErrorRunsAgainOnRetry(t *testing.T) { func TestKeyedSuccessAndRefusalsStillReplay(t *testing.T) { for _, durable := range []bool{false, true} { for _, status := range []int{200, 422} { - t.Run(strconv.Itoa(status)+map[bool]string{false: "/memory", true: "/disk"}[durable], func(t *testing.T) { + t.Run(strconv.Itoa(status)+storeName(durable), func(t *testing.T) { h, clock := transientReplayHandler(t, durable) calls := 0 h.mux.HandleFunc("/v1/jobs", func(w http.ResponseWriter, _ *http.Request) { @@ -122,7 +199,7 @@ func TestKeyedSuccessAndRefusalsStillReplay(t *testing.T) { }) for attempt := range 3 { w := httptest.NewRecorder() - h.idempotent(w, transientKeyedRequest()) + h.idempotent(w, transientKeyedRequest("/v1/jobs")) wantCalls := 1 if attempt == 2 { wantCalls = 2 diff --git a/internal/factory/serve/job_screens.go b/internal/factory/serve/job_screens.go index 1ab801e74e..9a316773c4 100644 --- a/internal/factory/serve/job_screens.go +++ b/internal/factory/serve/job_screens.go @@ -7,8 +7,10 @@ import ( "fmt" "io" "net/http" + "net/http/httptrace" "net/url" "strings" + "sync/atomic" "time" "github.com/Agent-Field/codeaf/internal/factory" @@ -351,6 +353,13 @@ func (h *Handler) searchIssues(w http.ResponseWriter, r *http.Request) { // githubJobJSON uses the signed-in person's credential. Only a solo host may // use its local profile; a team's read grant is never used for a write. +// +// For a write it is the handler's first change, so it clears the phase marker +// (api.PhaseHeader) once GitHub may have acted: when the request got a +// connection, since nothing is sent before that, and when GitHub answered +// anything but its own 4xx refusal, which makes nothing. A failure before the +// request was sent, or a refusal, therefore stays marked as having written +// nothing; a lost answer, a GitHub 5xx or an unreadable receipt does not. func (h *Handler) githubJobJSON(w http.ResponseWriter, r *http.Request, method, path string, body any, out any) bool { session, _ := h.credential(r) token := session.githubToken @@ -369,7 +378,9 @@ func (h *Handler) githubJobJSON(w http.ResponseWriter, r *http.Request, method, return false } } - req, err := http.NewRequestWithContext(r.Context(), method, "https://api.github.com"+path, ioBody(raw)) + var connected atomic.Bool + ctx := httptrace.WithClientTrace(r.Context(), &httptrace.ClientTrace{GotConn: func(httptrace.GotConnInfo) { connected.Store(true) }}) + req, err := http.NewRequestWithContext(ctx, method, "https://api.github.com"+path, ioBody(raw)) if h.failed(w, err) { return false } @@ -380,10 +391,16 @@ func (h *Handler) githubJobJSON(w http.ResponseWriter, r *http.Request, method, client := h.githubClient() response, err := client.Do(req) if err != nil { + if connected.Load() { + clearPhase(w) + } h.fail(w, 502, "GitHub isn't answering; retry this job change") return false } defer response.Body.Close() + if response.StatusCode < 400 || response.StatusCode >= 500 { + clearPhase(w) + } if response.StatusCode < 200 || response.StatusCode >= 300 { h.fail(w, 502, fmt.Sprintf("GitHub refused this job change (HTTP %d)", response.StatusCode)) return false diff --git a/internal/factory/serve/keyed_create_test.go b/internal/factory/serve/keyed_create_test.go new file mode 100644 index 0000000000..95a47c1b21 --- /dev/null +++ b/internal/factory/serve/keyed_create_test.go @@ -0,0 +1,301 @@ +package serve_test + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + + "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/api" + "github.com/Agent-Field/codeaf/internal/factory/pipeline" +) + +// issueGitHub is GitHub's issue endpoint behind a real TLS connection, so the +// factory's own HTTP client, not the fake, is what knows whether the issue +// request went out. down makes every dial fail: nothing reaches GitHub. +type issueGitHub struct { + srv *httptest.Server + down atomic.Bool + + mu sync.Mutex + answer string // "" answers 201; "lost", "5xx" and "refuse" are failures + issues int // issues GitHub made +} + +func newIssueGitHub(t *testing.T) *issueGitHub { + t.Helper() + g := &issueGitHub{} + g.srv = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.Copy(io.Discard, r.Body) + if r.Method != http.MethodPost || r.URL.Path != "/repos/acme/api/issues" { + t.Errorf("unexpected GitHub request %s %s", r.Method, r.URL) + w.WriteHeader(http.StatusNotFound) + return + } + g.mu.Lock() + answer := g.answer + if answer == "refuse" { + g.mu.Unlock() + // GitHub's own refusal: the issue was not made. + w.WriteHeader(http.StatusGone) + _, _ = w.Write([]byte(`{"message":"Issues are disabled for this repo"}`)) + return + } + g.issues++ + n := g.issues + 10 + g.mu.Unlock() + switch answer { + case "lost": + // The issue is made and the answer never arrives. + conn, _, err := w.(http.Hijacker).Hijack() + if err == nil { + _ = conn.Close() + } + case "5xx": + w.WriteHeader(http.StatusBadGateway) + default: + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"number":%d,"title":"Fix it","html_url":"https://github.com/acme/api/issues/%d"}`, n, n) + } + })) + t.Cleanup(g.srv.Close) + return g +} + +func (g *issueGitHub) set(answer string) { + g.mu.Lock() + defer g.mu.Unlock() + g.answer = answer +} + +func (g *issueGitHub) made() int { + g.mu.Lock() + defer g.mu.Unlock() + return g.issues +} + +// client reaches the fake for https://api.github.com through a real transport. +func (g *issueGitHub) client() *http.Client { + tr := g.srv.Client().Transport.(*http.Transport).Clone() + tr.TLSClientConfig.ServerName = "example.com" + addr := g.srv.Listener.Addr().String() + tr.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) { + if g.down.Load() { + return nil, &net.OpError{Op: "dial", Net: network, Err: errors.New("connection refused")} + } + return (&net.Dialer{}).DialContext(ctx, network, addr) + } + return &http.Client{Transport: tr} +} + +// keyedCreate is the factory as the engine reaches it for a keyed create: one +// solo host that opens issues through g and makes jobs in its own store. +type keyedCreate struct { + *v2Jobs + github *issueGitHub + news atomic.Int64 + launches atomic.Int64 + launch func(int) error + pipeline func() (pipeline.File, error) +} + +func newKeyedCreate(t *testing.T) *keyedCreate { + t.Helper() + k := &keyedCreate{v2Jobs: newV2Jobs(t), github: newIssueGitHub(t)} + k.cfg.Mode = api.FactorySolo + k.cfg.GitHubToken = func(context.Context) string { return "private-token" } + k.cfg.GitHubHTTPClient = k.github.client() + k.pipeline = func() (pipeline.File, error) { + p := pipeline.Defaults() + p.DefaultBudget = 4 + p.Pipeline[factory.Kind("repair")] = []string{"write", "proof"} + return p, nil + } + k.cfg.Pipeline = func(context.Context, string, string) (pipeline.File, error) { return k.pipeline() } + k.seam.RecipeAt = func(string) (factory.Recipe, []factory.Problem, string, error) { + p, err := k.pipeline() + return p.Recipe(), nil, "", err + } + k.seam.New = func(repo, words string) (int, error) { + k.news.Add(1) + it, err := k.st.Create(factory.Item{State: factory.StateNew, Repo: repo, Title: "Fix it", Kind: factory.KindIssue, Origin: factory.OriginTerminal}) + return it.ID, err + } + launch := k.seam.Launch + k.launch = launch + k.seam.Launch = func(id int) error { + k.launches.Add(1) + return k.launch(id) + } + reopenJobs(t, k.v2Jobs) + return k +} + +func (k *keyedCreate) jobs(t *testing.T) int { + t.Helper() + items, err := k.st.List() + if err != nil { + t.Fatal(err) + } + return len(items) +} + +func phaseOf(w *httptest.ResponseRecorder) string { return w.Header().Get(api.PhaseHeader) } + +// A KEYED CREATE ANSWERED AFTER ITS FIRST WRITE IS REPLAYED, WHATEVER ITS STATUS. +// The engine calls a 5xx or a lost answer uncertain and the desk resends the +// same key: the resend must get the first answer back and make nothing new, +// neither a second GitHub issue nor a second job. +func TestAKeyedCreateAnsweredAfterItsWriteIsReplayed(t *testing.T) { + for _, c := range []struct { + name, path, body string + arm func(*keyedCreate) + heal func(*keyedCreate) + status int + issues, jobs int + launches int64 + }{ + {name: "GitHub made the issue and its answer was lost", path: "/v1/issues", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app"}`, + arm: func(k *keyedCreate) { k.github.set("lost") }, heal: func(k *keyedCreate) { k.github.set("") }, + status: http.StatusBadGateway, issues: 1}, + {name: "GitHub answered 5xx to the issue request", path: "/v1/issues", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app"}`, + arm: func(k *keyedCreate) { k.github.set("5xx") }, heal: func(k *keyedCreate) { k.github.set("") }, + status: http.StatusBadGateway, issues: 1}, + {name: "the job was made and its start failed", path: "/v1/jobs", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app","start":"now"}`, + arm: func(k *keyedCreate) { + launch := k.launch + k.launch = func(id int) error { + k.launch = launch + return errors.New("the runner pool is unavailable") + } + }, + heal: func(*keyedCreate) {}, + status: http.StatusInternalServerError, jobs: 1, launches: 1}, + } { + t.Run(c.name, func(t *testing.T) { + k := newKeyedCreate(t) + c.arm(k) + first := keyedRequest(k.h, c.path, c.body, "workstream:5:ws_ab:3:d_1") + checkJSON(t, first, c.status) + if phaseOf(first) != "" { + t.Fatalf("an answer after a write claims nothing was written: %s", first.Body) + } + c.heal(k) + second := keyedRequest(k.h, c.path, c.body, "workstream:5:ws_ab:3:d_1") + if second.Code != first.Code || second.Body.String() != first.Body.String() || phaseOf(second) != "" { + t.Fatalf("resend = %d %s (phase %q), want the first answer %d %s replayed", second.Code, second.Body, phaseOf(second), first.Code, first.Body) + } + if got := k.github.made(); got != c.issues { + t.Fatalf("GitHub issues made = %d, want %d", got, c.issues) + } + if got := k.jobs(t); got != c.jobs { + t.Fatalf("jobs = %d, want %d", got, c.jobs) + } + if got := k.launches.Load(); got != c.launches { + t.Fatalf("starts = %d, want %d", got, c.launches) + } + }) + } +} + +// A KEYED CREATE REFUSED OR FAILED BEFORE ANYTHING WAS WRITTEN RUNS AGAIN. Its +// answer says so (api.PhaseHeader) and is not kept, so once the cause passes or +// is fixed the same key makes the job, and that success is what replays. +func TestAKeyedCreateThatWroteNothingRunsAgain(t *testing.T) { + issue := `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app"}` + for _, c := range []struct { + name, path, body string + arm, heal func(*keyedCreate) + status int + words string + issues int + }{ + {name: "GitHub was unreachable before the issue request was sent", path: "/v1/issues", body: issue, + arm: func(k *keyedCreate) { k.github.down.Store(true) }, heal: func(k *keyedCreate) { k.github.down.Store(false) }, + status: http.StatusBadGateway, words: "GitHub isn't answering", issues: 1}, + {name: "GitHub refused the issue", path: "/v1/issues", body: issue, + arm: func(k *keyedCreate) { k.github.set("refuse") }, heal: func(k *keyedCreate) { k.github.set("") }, + status: http.StatusBadGateway, words: "GitHub refused this job change (HTTP 410)", issues: 1}, + {name: "the pipeline read failed before the job", path: "/v1/jobs", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app","pipeline":"repair"}`, + arm: func(k *keyedCreate) { + read := k.pipeline + k.pipeline = func() (pipeline.File, error) { + k.pipeline = read + return pipeline.File{}, errors.New("the factory's store is busy") + } + }, + heal: func(*keyedCreate) {}, status: http.StatusInternalServerError, words: "store is busy"}, + {name: "a refusal the person fixed", path: "/v1/issues", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app","pipeline":"triage"}`, + arm: func(*keyedCreate) {}, + heal: func(k *keyedCreate) { + read := k.pipeline + k.pipeline = func() (pipeline.File, error) { + p, err := read() + p.Pipeline[factory.Kind("triage")] = []string{"write"} + return p, err + } + }, + status: http.StatusUnprocessableEntity, words: "pipeline not found", issues: 1}, + } { + t.Run(c.name, func(t *testing.T) { + k := newKeyedCreate(t) + c.arm(k) + first := keyedRequest(k.h, c.path, c.body, "workstream:5:ws_ab:3:d_1") + checkJSON(t, first, c.status) + if !strings.Contains(first.Body.String(), c.words) { + t.Fatalf("first = %d %s, want %q", first.Code, first.Body, c.words) + } + if k.github.made() != 0 || k.jobs(t) != 0 { + t.Fatalf("the failure made something: issues=%d jobs=%d", k.github.made(), k.jobs(t)) + } + c.heal(k) + second := keyedRequest(k.h, c.path, c.body, "workstream:5:ws_ab:3:d_1") + if second.Code != http.StatusCreated { + t.Fatalf("resend after the cause passed = %d %s, want the job made", second.Code, second.Body) + } + checkJSON(t, second, http.StatusCreated) + if phaseOf(first) != api.PhasePreMutation { + t.Fatalf("first = %d %s carries phase %q, want it marked as answered before any change", first.Code, first.Body, phaseOf(first)) + } + if phaseOf(second) != "" { + t.Fatalf("a made job claims nothing was written: %v", second.Header()) + } + var made api.JobCreated + if err := json.Unmarshal(second.Body.Bytes(), &made); err != nil || made.ID == 0 { + t.Fatalf("created %s: %v", second.Body, err) + } + third := keyedRequest(k.h, c.path, c.body, "workstream:5:ws_ab:3:d_1") + if third.Code != http.StatusCreated || third.Body.String() != second.Body.String() || phaseOf(third) != "" { + t.Fatalf("a resend after success = %d %s (phase %q), want the success replayed", third.Code, third.Body, phaseOf(third)) + } + if k.github.made() != c.issues || k.jobs(t) != 1 || k.news.Load() != 1 { + t.Fatalf("issues=%d jobs=%d news=%d, want issues=%d and one job", k.github.made(), k.jobs(t), k.news.Load(), c.issues) + } + }) + } +} + +// An unkeyed create carries the same marker: present on a refusal before any +// write, gone once the job exists. +func TestAnUnkeyedCreateSaysWhetherItWroteAnything(t *testing.T) { + k := newKeyedCreate(t) + refused := verbRequest(k.h, "POST", "/v1/jobs", `{"repo":"acme/api"}`) + checkJSON(t, refused, http.StatusUnprocessableEntity) + if phaseOf(refused) != api.PhasePreMutation { + t.Fatalf("refusal phase %q", phaseOf(refused)) + } + made := verbRequest(k.h, "POST", "/v1/jobs", `{"repo":"acme/api","title":"Fix it"}`) + checkJSON(t, made, http.StatusCreated) + if phaseOf(made) != "" { + t.Fatalf("made job phase %q", phaseOf(made)) + } +} diff --git a/internal/factory/serve/phase.go b/internal/factory/serve/phase.go index 0d47bd70ae..cd1e5568f6 100644 --- a/internal/factory/serve/phase.go +++ b/internal/factory/serve/phase.go @@ -31,7 +31,11 @@ func markPreMutation(r *http.Request) bool { } switch r.Method { case http.MethodPost: - return r.URL.Path == "/v1/access/links" || r.URL.Path == "/v1/access/revoke-sessions" + // A job create, with or without its GitHub issue: exactly /v1/jobs + // and /v1/issues. Its marker also decides whether a keyed answer is + // kept ([keepReplay]). + return r.URL.Path == "/v1/access/links" || r.URL.Path == "/v1/access/revoke-sessions" || + r.URL.Path == "/v1/jobs" || r.URL.Path == "/v1/issues" case http.MethodPut: // The factory's own rules, its routing table and its pipeline // library: exactly /v1/rules, /v1/routes and /v1/pipeline-library. diff --git a/internal/factory/serve/verbs.go b/internal/factory/serve/verbs.go index 213b7fac07..ec3f7aba24 100644 --- a/internal/factory/serve/verbs.go +++ b/internal/factory/serve/verbs.go @@ -802,6 +802,13 @@ func (h *Handler) reply(w http.ResponseWriter, r *http.Request, it factory.Item) } } +// newJob makes one job, opening its GitHub issue first when asked. EVERY ANSWER +// BEFORE ITS FIRST WRITE CARRIES THE PHASE MARKER (api.PhaseHeader, phase.go): +// a refusal, the visibility check, a sign-in or read failure, and a GitHub +// failure before the issue request was sent. The issue request +// ([Handler.githubJobJSON]) and the job's own make clear it, so everything +// after them, the stamps, the issue link, the budget and a start, answers +// unmarked and a keyed resend replays it instead of making another job. func (h *Handler) newJob(w http.ResponseWriter, r *http.Request) { if !h.requireVerb(w, h.seam.New != nil || h.seam.NewJob != nil) { return @@ -929,6 +936,7 @@ func (h *Handler) newJob(w http.ResponseWriter, r *http.Request) { var err error literal := body.Source == factory.AskApp || body.Source == "hand-off" || body.Text != "" if literal && h.seam.NewJob != nil { + clearPhase(w) id, err = h.seam.NewJob(body.Repo, body.Title, body.Text) } else { if !h.requireVerb(w, h.seam.New != nil) { @@ -938,6 +946,7 @@ func (h *Handler) newJob(w http.ResponseWriter, r *http.Request) { if body.Text != "" { words += "\n\n" + body.Text } + clearPhase(w) id, err = h.seam.New(body.Repo, words) } if h.failed(w, err) { diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 7c3416a4d7..1ff24e3794 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -1165,6 +1165,15 @@ The new job gets its own branch and conversations when it runs. It starts with n This is a person's Start: the same model, checkout, repository switch and start refusals apply. If Start is refused, no new job remains. Accepted work waits for runner capacity under the usual Jobs at once limit. Omitting an assignee starts it as the caller's job; an explicit assignee names the new job's person. +## Resending a create with the same key — retry key, Idempotency-Key, uncertain create, no second job + +A job create sent to a Factory with an `Idempotency-Key` header is remembered for 24 hours, per caller, route (`/v1/jobs`, or `/v1/issues` when it opens the GitHub issue) and key. A resend with the same key never makes a second job or a second GitHub issue: + +- An answer given before anything was written is not kept, so the same key runs the create again. That covers every refusal of the request (a bad field, a repository you can't see or that is off, a pipeline that doesn't exist), a sign-in or read failure, GitHub refusing the issue, and GitHub not answering before the issue request was sent. Fix the cause and resend the same key. These answers carry the response header `X-Codeaf-Phase: pre-mutation`. +- An answer given once the issue request may have reached GitHub, or once the job exists, is replayed as it was, success or failure. That includes a server error: GitHub's answer to the issue request was lost or was an error, or the job was made and its start failed. If a resend gets the same failure back, the first attempt may have made the job or the issue, so read the queue before trying again. A new key would make another job. + +A create without a key is never replayed: sending it again makes another job. + ## App job title and details — literal words, auto-save, budgets and hand-off Creating a job with a separate title in the app, or handing off an agent, From 878ecd9a03e71853bae34354274ac97af55acba4 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 19:53:21 -0500 Subject: [PATCH 4/6] Factory: GET /v1/features lists keyed_create The Factory now says it applies the keyed create rule: a keyed POST /v1/jobs or /v1/issues answered before its first write is not kept, so the same key runs again. An older Factory keeps every keyed answer for a day, refusals and server errors included, so an engine must be able to tell the two apart before it sends a create key. The word is additive and lives in the request path's rule, so a host without a store lists it too. The constant sits in its own file, identical on #2617, so the two branches merge without a conflict. Co-Authored-By: Claude Opus 5.5 --- .../unreleased/2619-factory-keyed-retry.md | 5 +++ docs/design/app-v3/PROTOCOL.md | 10 ++++-- internal/factory/api/goahead.go | 3 +- internal/factory/api/keyed_create.go | 11 ++++++ internal/factory/serve/goahead.go | 9 +++-- .../serve/keyed_create_feature_test.go | 35 +++++++++++++++++++ internal/manual/chat/factory.md | 2 +- 7 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 internal/factory/api/keyed_create.go create mode 100644 internal/factory/serve/keyed_create_feature_test.go diff --git a/docs/changes/unreleased/2619-factory-keyed-retry.md b/docs/changes/unreleased/2619-factory-keyed-retry.md index 0b0fb089d6..21410e4a36 100644 --- a/docs/changes/unreleased/2619-factory-keyed-retry.md +++ b/docs/changes/unreleased/2619-factory-keyed-retry.md @@ -5,6 +5,7 @@ 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." --- @@ -19,6 +20,10 @@ exists, is still replayed for 24 hours, a 5xx or 408 included, so a resend never makes a second job or GitHub issue. 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". diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index b28b34a961..caf3d0d9bc 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/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 @@ -435,7 +436,10 @@ 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, and a resend with the same key never makes a -second job or issue. An answer given before the create wrote anything carries +second job or issue. 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 diff --git a/internal/factory/api/goahead.go b/internal/factory/api/goahead.go index c10acb1c9e..384d00fdcb 100644 --- a/internal/factory/api/goahead.go +++ b/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. diff --git a/internal/factory/api/keyed_create.go b/internal/factory/api/keyed_create.go new file mode 100644 index 0000000000..71abb0fc5f --- /dev/null +++ b/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" diff --git a/internal/factory/serve/goahead.go b/internal/factory/serve/goahead.go index f594893051..579c541765 100644 --- a/internal/factory/serve/goahead.go +++ b/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) } diff --git a/internal/factory/serve/keyed_create_feature_test.go b/internal/factory/serve/keyed_create_feature_test.go new file mode 100644 index 0000000000..0ba259b90c --- /dev/null +++ b/internal/factory/serve/keyed_create_feature_test.go @@ -0,0 +1,35 @@ +package serve_test + +import ( + "encoding/json" + "net/http" + "slices" + "testing" + + "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/api" + "github.com/Agent-Field/codeaf/internal/factory/serve" +) + +// THE FACTORY SAYS IT RUNS A KEYED CREATE THAT WROTE NOTHING AGAIN, in GET +// /v1/features, because an engine sends a create key only to a Factory that +// says so: an older one keeps every keyed answer, a refusal or a 5xx included, +// and would replay it for a day. The rule lives in the request path, not the +// store, so a host without a store says it too. +func TestTheFactorySaysItRunsAKeyedCreateThatWroteNothingAgain(t *testing.T) { + stored := newKeyedCreate(t).h + bare := serve.New(factory.Seam{}, serve.Config{Mode: api.FactorySolo, Address: config.Address, Version: config.Version}) + t.Cleanup(bare.Close) + for name, h := range map[string]http.Handler{"with a store": stored, "without a store": bare} { + t.Run(name, func(t *testing.T) { + w := verbRequest(h, "GET", "/v1/features", "") + var got api.Features + if w.Code != http.StatusOK || json.Unmarshal(w.Body.Bytes(), &got) != nil { + t.Fatalf("features = %d %s", w.Code, w.Body) + } + if !slices.Contains(got.Features, api.FeatureKeyedCreate) { + t.Fatalf("features = %v, want %q listed", got.Features, api.FeatureKeyedCreate) + } + }) + } +} diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 1ff24e3794..3631e2d0fa 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -1167,7 +1167,7 @@ This is a person's Start: the same model, checkout, repository switch and start ## Resending a create with the same key — retry key, Idempotency-Key, uncertain create, no second job -A job create sent to a Factory with an `Idempotency-Key` header is remembered for 24 hours, per caller, route (`/v1/jobs`, or `/v1/issues` when it opens the GitHub issue) and key. A resend with the same key never makes a second job or a second GitHub issue: +A job create sent to a Factory with an `Idempotency-Key` header is remembered for 24 hours, per caller, route (`/v1/jobs`, or `/v1/issues` when it opens the GitHub issue) and key. A Factory with this rule lists `keyed_create` in `GET /v1/features`, and an engine sends a create key only to such a Factory: an older one keeps every keyed answer, refusals and server errors included, for a day. A resend with the same key never makes a second job or a second GitHub issue: - An answer given before anything was written is not kept, so the same key runs the create again. That covers every refusal of the request (a bad field, a repository you can't see or that is off, a pipeline that doesn't exist), a sign-in or read failure, GitHub refusing the issue, and GitHub not answering before the issue request was sent. Fix the cause and resend the same key. These answers carry the response header `X-Codeaf-Phase: pre-mutation`. - An answer given once the issue request may have reached GitHub, or once the job exists, is replayed as it was, success or failure. That includes a server error: GitHub's answer to the issue request was lost or was an error, or the job was made and its start failed. If a resend gets the same failure back, the first attempt may have made the job or the issue, so read the queue before trying again. A new key would make another job. From 0a97315e774399c3332bee45f3b42ff3624cc43b Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 19:53:28 -0500 Subject: [PATCH 5/6] serve tests: kept keyed answers go through the route's marker; NewJob's clear is proven TestKeyedSuccessAndRefusalsStillReplay called idempotent without the marker ServeHTTP sets, so it claimed a /v1/jobs 422 replays for a day, which the real route no longer does. It becomes TestAKeptKeyedAnswerReplaysForADay: a create's success and an unmarked verb's refusal, both through the marker. TestAKeyedCreateThatWroteNothingRunsAgain gains a real /v1/jobs refusal the person fixed. A create through the seam's NewJob whose start failed must be replayed, and an unkeyed one made through NewJob carries no marker: both fail if the clearPhase before seam.NewJob is deleted. Co-Authored-By: Claude Opus 5.5 --- .../serve/idempotency_transient_test.go | 40 +++++++---- internal/factory/serve/keyed_create_test.go | 72 ++++++++++++++++--- 2 files changed, 90 insertions(+), 22 deletions(-) diff --git a/internal/factory/serve/idempotency_transient_test.go b/internal/factory/serve/idempotency_transient_test.go index 61cc3177fa..68c235fe0e 100644 --- a/internal/factory/serve/idempotency_transient_test.go +++ b/internal/factory/serve/idempotency_transient_test.go @@ -184,28 +184,44 @@ func TestAKeyedAnswerAfterAWriteIsReplayed(t *testing.T) { } } -// C3: Successful and definitive refusal answers still replay for 24 hours -// in memory and across a store-backed restart, then expire as before. -func TestKeyedSuccessAndRefusalsStillReplay(t *testing.T) { - for _, durable := range []bool{false, true} { - for _, status := range []int{200, 422} { - t.Run(strconv.Itoa(status)+storeName(durable), func(t *testing.T) { +// C3: A keyed answer that is kept replays for 24 hours, in memory and across a +// store-backed restart, then expires. Each request goes through the marker +// ServeHTTP sets (servedKeyed), so this is what the route really does: a job +// create's success, whose handler cleared the marker before making the job, +// and a refusal of a job verb that is not marked (markPreMutation), such as +// approve, which keeps every keyed answer as before. A job create's own +// refusal is marked and is not kept (TestAKeyedServerErrorRunsAgainOnRetry, +// and on the real handler TestAKeyedCreateThatWroteNothingRunsAgain). +func TestAKeptKeyedAnswerReplaysForADay(t *testing.T) { + for _, route := range []struct { + name, path string + status int + write bool // the handler clears the marker, as before a write + }{ + {"a job create's success", "/v1/jobs", http.StatusCreated, true}, + {"an unmarked verb's refusal", "/v1/jobs/7/approve", http.StatusUnprocessableEntity, false}, + {"an unmarked verb's conflict", "/v1/jobs/7/approve", http.StatusConflict, false}, + } { + for _, durable := range []bool{false, true} { + t.Run(route.name+storeName(durable), func(t *testing.T) { h, clock := transientReplayHandler(t, durable) calls := 0 - h.mux.HandleFunc("/v1/jobs", func(w http.ResponseWriter, _ *http.Request) { + h.mux.HandleFunc(route.path, func(w http.ResponseWriter, _ *http.Request) { calls++ - w.WriteHeader(status) + if route.write { + clearPhase(w) + } + w.WriteHeader(route.status) _, _ = w.Write([]byte(strconv.Itoa(calls))) }) for attempt := range 3 { - w := httptest.NewRecorder() - h.idempotent(w, transientKeyedRequest("/v1/jobs")) + w := servedKeyed(h, transientKeyedRequest(route.path)) wantCalls := 1 if attempt == 2 { wantCalls = 2 } - if w.Code != status || w.Body.String() != strconv.Itoa(wantCalls) || calls != wantCalls { - t.Fatalf("attempt %d = %d %q calls=%d, want status=%d calls=%d", attempt, w.Code, w.Body.String(), calls, status, wantCalls) + if w.Code != route.status || w.Body.String() != strconv.Itoa(wantCalls) || calls != wantCalls || w.Header().Get(api.PhaseHeader) != "" { + t.Fatalf("attempt %d = %d %q phase=%q calls=%d, want status=%d calls=%d", attempt, w.Code, w.Body.String(), w.Header().Get(api.PhaseHeader), calls, route.status, wantCalls) } if attempt == 0 { *clock = clock.Add(24*time.Hour - time.Nanosecond) diff --git a/internal/factory/serve/keyed_create_test.go b/internal/factory/serve/keyed_create_test.go index 95a47c1b21..01d3794ab2 100644 --- a/internal/factory/serve/keyed_create_test.go +++ b/internal/factory/serve/keyed_create_test.go @@ -103,6 +103,7 @@ type keyedCreate struct { *v2Jobs github *issueGitHub news atomic.Int64 + newJobs atomic.Int64 // of news, those made through NewJob launches atomic.Int64 launch func(int) error pipeline func() (pipeline.File, error) @@ -140,6 +141,19 @@ func newKeyedCreate(t *testing.T) *keyedCreate { return k } +// literal makes jobs through the seam's NewJob, the door an app create with a +// separate title takes, instead of New. +func (k *keyedCreate) literal(t *testing.T) { + t.Helper() + k.seam.NewJob = func(repo, title, text string) (int, error) { + k.news.Add(1) + k.newJobs.Add(1) + it, err := k.st.Create(factory.Item{State: factory.StateNew, Repo: repo, Title: title, Body: text, Kind: factory.KindIssue, Origin: factory.OriginTerminal}) + return it.ID, err + } + reopenJobs(t, k.v2Jobs) +} + func (k *keyedCreate) jobs(t *testing.T) int { t.Helper() items, err := k.st.List() @@ -163,6 +177,7 @@ func TestAKeyedCreateAnsweredAfterItsWriteIsReplayed(t *testing.T) { status int issues, jobs int launches int64 + literal bool // the job is made through NewJob, not New }{ {name: "GitHub made the issue and its answer was lost", path: "/v1/issues", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app"}`, arm: func(k *keyedCreate) { k.github.set("lost") }, heal: func(k *keyedCreate) { k.github.set("") }, @@ -180,9 +195,22 @@ func TestAKeyedCreateAnsweredAfterItsWriteIsReplayed(t *testing.T) { }, heal: func(*keyedCreate) {}, status: http.StatusInternalServerError, jobs: 1, launches: 1}, + {name: "the job was made through NewJob and its start failed", path: "/v1/jobs", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app","start":"now"}`, + arm: func(k *keyedCreate) { + launch := k.launch + k.launch = func(id int) error { + k.launch = launch + return errors.New("the runner pool is unavailable") + } + }, + heal: func(*keyedCreate) {}, + status: http.StatusInternalServerError, jobs: 1, launches: 1, literal: true}, } { t.Run(c.name, func(t *testing.T) { k := newKeyedCreate(t) + if c.literal { + k.literal(t) + } c.arm(k) first := keyedRequest(k.h, c.path, c.body, "workstream:5:ws_ab:3:d_1") checkJSON(t, first, c.status) @@ -203,6 +231,9 @@ func TestAKeyedCreateAnsweredAfterItsWriteIsReplayed(t *testing.T) { if got := k.launches.Load(); got != c.launches { t.Fatalf("starts = %d, want %d", got, c.launches) } + if c.literal && k.newJobs.Load() != 1 { + t.Fatalf("NewJob made %d jobs, want the one job made through it", k.newJobs.Load()) + } }) } } @@ -245,6 +276,17 @@ func TestAKeyedCreateThatWroteNothingRunsAgain(t *testing.T) { } }, status: http.StatusUnprocessableEntity, words: "pipeline not found", issues: 1}, + {name: "a job create refusal the person fixed", path: "/v1/jobs", body: `{"repo":"acme/api","title":"Fix it","text":"brief","source":"app","pipeline":"triage"}`, + arm: func(*keyedCreate) {}, + heal: func(k *keyedCreate) { + read := k.pipeline + k.pipeline = func() (pipeline.File, error) { + p, err := read() + p.Pipeline[factory.Kind("triage")] = []string{"write"} + return p, err + } + }, + status: http.StatusUnprocessableEntity, words: "pipeline not found"}, } { t.Run(c.name, func(t *testing.T) { k := newKeyedCreate(t) @@ -287,15 +329,25 @@ func TestAKeyedCreateThatWroteNothingRunsAgain(t *testing.T) { // An unkeyed create carries the same marker: present on a refusal before any // write, gone once the job exists. func TestAnUnkeyedCreateSaysWhetherItWroteAnything(t *testing.T) { - k := newKeyedCreate(t) - refused := verbRequest(k.h, "POST", "/v1/jobs", `{"repo":"acme/api"}`) - checkJSON(t, refused, http.StatusUnprocessableEntity) - if phaseOf(refused) != api.PhasePreMutation { - t.Fatalf("refusal phase %q", phaseOf(refused)) - } - made := verbRequest(k.h, "POST", "/v1/jobs", `{"repo":"acme/api","title":"Fix it"}`) - checkJSON(t, made, http.StatusCreated) - if phaseOf(made) != "" { - t.Fatalf("made job phase %q", phaseOf(made)) + for _, literal := range []bool{false, true} { + t.Run(map[bool]string{false: "New", true: "NewJob"}[literal], func(t *testing.T) { + k := newKeyedCreate(t) + if literal { + k.literal(t) + } + refused := verbRequest(k.h, "POST", "/v1/jobs", `{"repo":"acme/api"}`) + checkJSON(t, refused, http.StatusUnprocessableEntity) + if phaseOf(refused) != api.PhasePreMutation { + t.Fatalf("refusal phase %q", phaseOf(refused)) + } + made := verbRequest(k.h, "POST", "/v1/jobs", `{"repo":"acme/api","title":"Fix it","source":"app"}`) + checkJSON(t, made, http.StatusCreated) + if phaseOf(made) != "" { + t.Fatalf("made job phase %q", phaseOf(made)) + } + if want := map[bool]int64{false: 0, true: 1}[literal]; k.newJobs.Load() != want { + t.Fatalf("NewJob made %d jobs, want %d", k.newJobs.Load(), want) + } + }) } } From c9375327027327a57af93759731b05b9f1547c8b Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 12 Nov 2026 19:54:11 -0500 Subject: [PATCH 6/6] docs: a keyed create's replay has two limits The docs said a resend with the same key never makes a second job or issue. The replay row is saved only after the create commits, so a Factory that stops in between runs the resend again, and the row is keyed by the exact Authorization and Cookie headers, so a resend under a new bearer or session cookie is a new create. PROTOCOL.md, the manual and the change entry now say both. Co-Authored-By: Claude Opus 5.5 --- docs/changes/unreleased/2619-factory-keyed-retry.md | 5 ++++- docs/design/app-v3/PROTOCOL.md | 8 ++++++-- internal/manual/chat/factory.md | 4 ++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/changes/unreleased/2619-factory-keyed-retry.md b/docs/changes/unreleased/2619-factory-keyed-retry.md index 21410e4a36..b82c4083c0 100644 --- a/docs/changes/unreleased/2619-factory-keyed-retry.md +++ b/docs/changes/unreleased/2619-factory-keyed-retry.md @@ -17,7 +17,10 @@ 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 -never makes a second job or GitHub issue. Unkeyed creates carry the same marker. +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 diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index caf3d0d9bc..5598fff84d 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -435,8 +435,12 @@ 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, and a resend with the same key never makes a -second job or issue. A Factory with this rule lists `keyed_create` in +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 diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 3631e2d0fa..e016d98a00 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -1167,12 +1167,12 @@ This is a person's Start: the same model, checkout, repository switch and start ## Resending a create with the same key — retry key, Idempotency-Key, uncertain create, no second job -A job create sent to a Factory with an `Idempotency-Key` header is remembered for 24 hours, per caller, route (`/v1/jobs`, or `/v1/issues` when it opens the GitHub issue) and key. A Factory with this rule lists `keyed_create` in `GET /v1/features`, and an engine sends a create key only to such a Factory: an older one keeps every keyed answer, refusals and server errors included, for a day. A resend with the same key never makes a second job or a second GitHub issue: +A job create sent to a Factory with an `Idempotency-Key` header is remembered for 24 hours, per caller, route (`/v1/jobs`, or `/v1/issues` when it opens the GitHub issue) and key. A Factory with this rule lists `keyed_create` in `GET /v1/features`, and an engine sends a create key only to such a Factory: an older one keeps every keyed answer, refusals and server errors included, for a day. A resend with the same key does not make a second job or GitHub issue: - An answer given before anything was written is not kept, so the same key runs the create again. That covers every refusal of the request (a bad field, a repository you can't see or that is off, a pipeline that doesn't exist), a sign-in or read failure, GitHub refusing the issue, and GitHub not answering before the issue request was sent. Fix the cause and resend the same key. These answers carry the response header `X-Codeaf-Phase: pre-mutation`. - An answer given once the issue request may have reached GitHub, or once the job exists, is replayed as it was, success or failure. That includes a server error: GitHub's answer to the issue request was lost or was an error, or the job was made and its start failed. If a resend gets the same failure back, the first attempt may have made the job or the issue, so read the queue before trying again. A new key would make another job. -A create without a key is never replayed: sending it again makes another job. +Two limits remain. The caller is the exact sign-in the request carried, so a resend after signing in again or under a new session cookie is not matched and runs as a new create. The answer is saved just after the create commits, so if the Factory stops in between, a resend runs again. A create without a key is never replayed: sending it again makes another job. ## App job title and details — literal words, auto-save, budgets and hand-off