From f58b3dd181e060029abb69d6441618ad53717427 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Wed, 4 Nov 2026 13:43:15 -0500 Subject: [PATCH 1/7] test: pin idle floor document reads at polling seams Co-Authored-By: Claude Opus 5.5 --- internal/factory/lease/floor_reads_test.go | 51 +++++++++++++++++++++ internal/factory/serve/floor_reads_test.go | 52 ++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 internal/factory/lease/floor_reads_test.go create mode 100644 internal/factory/serve/floor_reads_test.go diff --git a/internal/factory/lease/floor_reads_test.go b/internal/factory/lease/floor_reads_test.go new file mode 100644 index 0000000000..cb72e0167e --- /dev/null +++ b/internal/factory/lease/floor_reads_test.go @@ -0,0 +1,51 @@ +package lease_test + +import ( + "errors" + "testing" + + "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/lease" + "github.com/Agent-Field/codeaf/internal/factory/store" +) + +func TestIdleFloorReadsOnlyChangedJobs(t *testing.T) { + for _, route := range []string{"Pull", "OverLimit"} { + t.Run(route, func(t *testing.T) { + f := setup(t) + for range 300 { + f.add(t, factory.Item{State: factory.StateNew}) + } + look := func() { + t.Helper() + if route == "Pull" { + if _, err := f.desk.Pull(worker("idle", "sam")); !errors.Is(err, lease.ErrNoJob) { + t.Fatalf("Pull: %v", err) + } + } else if n, err := f.desk.OverLimit(worker("idle", "sam")); err != nil || n != 0 { + t.Fatalf("OverLimit: %d, %v", n, err) + } + } + look() + before := f.store.DocumentReads() + for range 3 { + look() + } + if n := f.store.DocumentReads() - before; n != 0 { + t.Fatalf("unchanged 300-job floor: read %d documents, want 0", n) + } + other, err := store.Open(f.store.Root()) + if err != nil { + t.Fatal(err) + } + if err := other.Update(150, func(it *factory.Item) error { it.Title = "changed elsewhere"; return nil }); err != nil { + t.Fatal(err) + } + before = f.store.DocumentReads() + look() + if n := f.store.DocumentReads() - before; n != 1 { + t.Fatalf("one changed job: read %d documents, want 1", n) + } + }) + } +} diff --git a/internal/factory/serve/floor_reads_test.go b/internal/factory/serve/floor_reads_test.go new file mode 100644 index 0000000000..0c73a162e0 --- /dev/null +++ b/internal/factory/serve/floor_reads_test.go @@ -0,0 +1,52 @@ +package serve_test + +import ( + "net/http/httptest" + "testing" + + "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/store" +) + +// runner/self crosses ServeHTTP's settle scan and the capacity scan on the +// same route the idle runner calls, without asking the UI to load the floor. +func TestIdleRequestsReadOnlyChangedJobs(t *testing.T) { + f := workerSetup(t) + t.Cleanup(f.handler.Close) + for range 300 { + if _, err := f.st.Create(factory.Item{Repo: "owner/repo", State: factory.StateNew}); err != nil { + t.Fatal(err) + } + } + runner := f.attach(t, "sam") + look := func() { + t.Helper() + r := httptest.NewRequest("GET", "/v1/runner/self", nil) + r.Header.Set("Authorization", "Bearer "+runner.Credential) + w := httptest.NewRecorder() + f.handler.ServeHTTP(w, r) + if w.Code != 200 { + t.Fatalf("runner/self: %d %s", w.Code, w.Body.String()) + } + } + look() + before := f.st.DocumentReads() + for range 3 { + look() + } + if n := f.st.DocumentReads() - before; n != 0 { + t.Fatalf("settle and capacity on unchanged 300-job floor: read %d documents, want 0", n) + } + other, err := store.Open(f.st.Root()) + if err != nil { + t.Fatal(err) + } + if err := other.Update(150, func(it *factory.Item) error { it.Title = "changed elsewhere"; return nil }); err != nil { + t.Fatal(err) + } + before = f.st.DocumentReads() + look() + if n := f.st.DocumentReads() - before; n != 1 { + t.Fatalf("settle and capacity after one change: read %d documents, want 1", n) + } +} From a9b38cb6221bb3d3a68785f31c4b48149559d462 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Wed, 4 Nov 2026 13:44:07 -0500 Subject: [PATCH 2/7] fix: reuse decoded floor documents by file identity List stats every numeric job file on every call and copies its cached JSON graph before migration, retention or caller mutation. Locked reads and writes bypass the cache. Co-Authored-By: Claude Opus 5.5 --- internal/factory/store/list.go | 96 +++++++++++++++++++++++++++++++++ internal/factory/store/store.go | 11 +++- 2 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 internal/factory/store/list.go diff --git a/internal/factory/store/list.go b/internal/factory/store/list.go new file mode 100644 index 0000000000..6a8fc0b85f --- /dev/null +++ b/internal/factory/store/list.go @@ -0,0 +1,96 @@ +package store + +import ( + "os" + "reflect" + "sync" +) + +// listIndex owns the decoded documents. Nothing returned by List aliases them. +// Writes never take this lock: migration may itself write while List holds it. +type listIndex struct { + mu sync.Mutex + docs map[string]listedDoc +} + +type listedDoc struct { + file os.FileInfo + doc doc +} + +// listed reads only changed documents. Stat precedes read: a rename during +// read may cause one extra read next time, never cache old bytes as a new file. +// EVERY LOOK STATS EVERY FILE. A folder timestamp is not sufficient evidence +// for a grant candidate: another process's rename must be seen on the next look, +// including a replacement whose size and modification time stayed identical. +func (st *Store) listed(path string, previous map[string]listedDoc, next map[string]listedDoc) (doc, error) { + info, err := os.Stat(path) + if err != nil { + return doc{}, err + } + cached, ok := previous[path] + if !ok || !sameDocument(cached.file, info) { + d, err := st.read(path) + if err != nil { + return doc{}, err + } + cached = listedDoc{file: info, doc: d} + } + next[path] = cached + // Copy before migration or retention as well as before the caller sees it. + // A retained view must not discard bytes needed by a later retention policy. + return copyListed(reflect.ValueOf(cached.doc)).Interface().(doc), nil +} + +// copyListed copies the mutable JSON graph without encoding or decoding it +// again. Struct values retain their private fields (notably time.Time); all +// exported pointers, slices and maps are copied recursively, including fields +// added to Item later. Documents come from JSON and therefore contain no cycles. +func copyListed(v reflect.Value) reflect.Value { + switch v.Kind() { + case reflect.Pointer: + if v.IsNil() { + return reflect.Zero(v.Type()) + } + out := reflect.New(v.Type().Elem()) + out.Elem().Set(copyListed(v.Elem())) + return out + case reflect.Interface: + if v.IsNil() { + return reflect.Zero(v.Type()) + } + out := reflect.New(v.Type()).Elem() + out.Set(copyListed(v.Elem())) + return out + case reflect.Slice: + if v.IsNil() { + return reflect.Zero(v.Type()) + } + out := reflect.MakeSlice(v.Type(), v.Len(), v.Len()) + for i := 0; i < v.Len(); i++ { + out.Index(i).Set(copyListed(v.Index(i))) + } + return out + case reflect.Map: + if v.IsNil() { + return reflect.Zero(v.Type()) + } + out := reflect.MakeMapWithSize(v.Type(), v.Len()) + iter := v.MapRange() + for iter.Next() { + out.SetMapIndex(iter.Key(), copyListed(iter.Value())) + } + return out + case reflect.Struct: + out := reflect.New(v.Type()).Elem() + out.Set(v) + for i := 0; i < v.NumField(); i++ { + if v.Type().Field(i).IsExported() { + out.Field(i).Set(copyListed(v.Field(i))) + } + } + return out + default: + return v + } +} diff --git a/internal/factory/store/store.go b/internal/factory/store/store.go index 43fc9e92cc..42ce4068e3 100644 --- a/internal/factory/store/store.go +++ b/internal/factory/store/store.go @@ -65,6 +65,8 @@ type Store struct { reads atomic.Int64 // talks is what [Store.TalkStop] last read. talks talkIndex + // list owns the documents last decoded by List. + list listIndex } // DocumentReads is how many job documents this store has decoded so far. @@ -227,10 +229,13 @@ func (st *Store) Get(id int) (factory.Item, error) { return d.Item, nil } -// List reads every item, newest first by Created. An unreadable or +// List lists every item, newest first by Created. Unchanged files reuse their +// decoded documents; every returned item owns its mutable data. An unreadable or // newer-schema document is skipped rather than fatal: ONE DOCUMENT FROM THE // FUTURE, OR ONE TORN BY A FULL DISK, MUST NOT EMPTY THE WHOLE FLOOR. func (st *Store) List() ([]factory.Item, error) { + st.list.mu.Lock() + defer st.list.mu.Unlock() entries, err := os.ReadDir(st.root) if err != nil { if os.IsNotExist(err) { @@ -238,6 +243,7 @@ func (st *Store) List() ([]factory.Item, error) { } return nil, err } + next := make(map[string]listedDoc, len(entries)) items := make([]factory.Item, 0, len(entries)) var ids repositoryIDs loadedIDs := false @@ -252,7 +258,7 @@ func (st *Store) List() ([]factory.Item, error) { if _, err := strconv.Atoi(strings.TrimSuffix(entry.Name(), ".json")); err != nil { continue } - d, err := st.read(filepath.Join(st.root, entry.Name())) + d, err := st.listed(filepath.Join(st.root, entry.Name()), st.list.docs, next) if err != nil { continue } @@ -270,6 +276,7 @@ func (st *Store) List() ([]factory.Item, error) { st.retainOutput(&d.Item) items = append(items, d.Item) } + st.list.docs = next sort.SliceStable(items, func(a, b int) bool { if items[a].Created.Equal(items[b].Created) { return items[a].ID > items[b].ID From 47ccf298310669428d871767df0e5113aeb82052 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Wed, 4 Nov 2026 13:50:16 -0500 Subject: [PATCH 3/7] test: prove floor ownership and expose stop during reservation Co-Authored-By: Claude Opus 5.5 --- internal/factory/lease/floor_reads_test.go | 148 +++++++++++++++++ internal/factory/store/list_test.go | 179 +++++++++++++++++++++ 2 files changed, 327 insertions(+) create mode 100644 internal/factory/store/list_test.go diff --git a/internal/factory/lease/floor_reads_test.go b/internal/factory/lease/floor_reads_test.go index cb72e0167e..3fa9d62e34 100644 --- a/internal/factory/lease/floor_reads_test.go +++ b/internal/factory/lease/floor_reads_test.go @@ -2,6 +2,9 @@ package lease_test import ( "errors" + "os" + "strings" + "sync" "testing" "github.com/Agent-Field/codeaf/internal/factory" @@ -49,3 +52,148 @@ func TestIdleFloorReadsOnlyChangedJobs(t *testing.T) { }) } } + +func TestPullAndCapacitySeeExternalFloorChanges(t *testing.T) { + for _, change := range []string{"stop", "start", "budget", "create", "delete", "take", "release", "rename"} { + t.Run(change, func(t *testing.T) { + f := setup(t) + in := factory.Item{State: factory.StateQueued, Taker: "sam", OverLimit: true, Cap: 10, + Autonomy: factory.AutonomyAuto, Stream: &factory.Stream{Spent: 2}, Recovery: &factory.StepRecovery{State: "queued"}} + if change == "start" || change == "create" { + in.State = factory.StateNew + } + if change == "release" { + in.Taker = "other" + } + it := f.add(t, in) + if _, err := f.desk.OverLimit(worker("idle", "sam")); err != nil { + t.Fatal(err) + } + other, err := store.Open(f.store.Root()) + if err != nil { + t.Fatal(err) + } + want := false + switch change { + case "create": + in.State = factory.StateQueued + it, err = other.Create(in) + want = true + case "delete": + err = os.Remove(other.ItemPath(it.ID)) + case "rename": + path := other.ItemPath(it.ID) + info, e := os.Stat(path) + if e != nil { + t.Fatal(e) + } + data, e := os.ReadFile(path) + if e != nil { + t.Fatal(e) + } + if strings.Count(string(data), `"sam"`) != 1 { + t.Fatal("expected one owner to replace") + } + tmp := path + ".replacement" + if e = os.WriteFile(tmp, []byte(strings.Replace(string(data), `"sam"`, `"pat"`, 1)), 0o600); e != nil { + t.Fatal(e) + } + if e = os.Chtimes(tmp, info.ModTime(), info.ModTime()); e != nil { + t.Fatal(e) + } + err = os.Rename(tmp, path) + default: + err = other.Update(it.ID, func(cur *factory.Item) error { + switch change { + case "stop": + factory.MarkPersonStopped(cur) + case "start": + factory.ClearPersonStop(cur) + cur.State = factory.StateQueued + want = true + case "budget": + cur.Cap = 1 + case "take": + cur.Taker = "other" + case "release": + cur.Taker = "" + want = true + } + return nil + }) + } + if err != nil { + t.Fatal(err) + } + w := worker("idle", "sam") + w.TakesUnassigned = map[string]bool{"acme/api": true} + n, err := f.desk.OverLimit(w) + if err != nil || (n == 1) != want || n > 1 { + t.Fatalf("capacity after %s: %d, %v; eligible=%v", change, n, err, want) + } + held, err := f.desk.Pull(w) + if want { + if err != nil || held.Job != it.ID { + t.Fatalf("next Pull missed %s: %+v, %v", change, held, err) + } + } else if !errors.Is(err, lease.ErrNoJob) { + t.Fatalf("next Pull admitted after %s: %+v, %v", change, held, err) + } + }) + } +} + +// The live output-policy seam runs after List has copied its document, before +// returning the candidate. Commit a second store's change there to force the +// race between candidate selection and the locked reservation without sleeps. +func TestPullRechecksLockedJobAfterListing(t *testing.T) { + for _, change := range []string{"stop", "stop with reply", "budget", "take"} { + t.Run(change, func(t *testing.T) { + f := setup(t) + in := recoveryJob() + if change == "budget" { + in.Recovery = &factory.StepRecovery{State: "queued"} + } + if change == "stop with reply" { + in.Talk = "chat" + in.Thread = []factory.Message{{State: "pending", Text: "explain"}} + } + it := f.add(t, in) + if _, err := f.store.List(); err != nil { + t.Fatal(err) + } + other, err := store.Open(f.store.Root()) + if err != nil { + t.Fatal(err) + } + var once sync.Once + f.store.SetOutputPolicy(func() (int, int) { + once.Do(func() { + if err := other.Update(it.ID, func(cur *factory.Item) error { + switch change { + case "stop", "stop with reply": + factory.MarkPersonStopped(cur) + case "budget": + cur.Cap = 1 + case "take": + cur.Assignee = "other" + } + return nil + }); err != nil { + t.Fatal(err) + } + }) + return 0, 0 + }) + held, err := f.desk.Pull(worker("idle", "sam")) + if change == "stop with reply" { + if err != nil || !held.ThreadOnly { + t.Fatalf("Stop during selection granted job steps: %+v, %v", held, err) + } + f.state(t, it.ID, factory.StateQueued) + } else if !errors.Is(err, lease.ErrNoJob) { + t.Fatalf("locked reservation ignored %s: %+v, %v", change, held, err) + } + }) + } +} diff --git a/internal/factory/store/list_test.go b/internal/factory/store/list_test.go new file mode 100644 index 0000000000..eee145c00c --- /dev/null +++ b/internal/factory/store/list_test.go @@ -0,0 +1,179 @@ +package store_test + +import ( + "os" + "path/filepath" + "reflect" + "strings" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/store" +) + +func listStore(t *testing.T) *store.Store { + t.Helper() + st, err := store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + return st +} + +func listOne(t *testing.T, st *store.Store) factory.Item { + t.Helper() + items, err := st.List() + if err != nil || len(items) != 1 { + t.Fatalf("List: %d items, %v", len(items), err) + } + return items[0] +} + +func TestListOwnsMutableDocuments(t *testing.T) { + st := listStore(t) + stopped := false + made, err := st.Create(factory.Item{ + Repo: "acme/api", State: factory.StateNew, PersonStopped: &stopped, + SourceAssignees: []string{}, Labels: []string{"original"}, + BriefSeen: map[string]int{"sam": 2}, Stages: []factory.Stage{{Name: "test", Proof: []string{"original"}}}, + Stream: &factory.Stream{Phases: []factory.Phase{{RoundCosts: map[int]float64{1: 2}, + Result: &factory.StageResult{Files: []string{"original"}, Tests: &factory.TestCounts{Passed: 2}}}}, + Log: []factory.LogLine{{Text: "original"}}}, + AnsweredQuestions: []factory.Question{{Options: []string{"original"}, Answer: &factory.Answer{Text: "original"}}}, + }) + if err != nil { + t.Fatal(err) + } + want := listOne(t, st) + before := st.DocumentReads() + mutate := func(it factory.Item) { + *it.PersonStopped = true + it.Labels[0] = "mutated" + it.BriefSeen["sam"] = 99 + it.Stages[0].Proof[0] = "mutated" + it.Stream.Phases[0].RoundCosts[1] = 99 + it.Stream.Phases[0].Result.Files[0] = "mutated" + it.Stream.Phases[0].Result.Tests.Passed = 99 + it.Stream.Log[0].Text = "mutated" + it.AnsweredQuestions[0].Options[0] = "mutated" + it.AnsweredQuestions[0].Answer.Text = "mutated" + } + var wg sync.WaitGroup + for range 8 { + wg.Add(1) + go func() { + defer wg.Done() + for range 10 { + mutate(listOne(t, st)) + } + }() + } + wg.Wait() + if got := listOne(t, st); !reflect.DeepEqual(got, want) { + t.Fatal("mutating a List result changed a later result") + } + if n := st.DocumentReads() - before; n != 0 { + t.Fatalf("copies read %d documents, want 0", n) + } + if got, err := st.Get(made.ID); err != nil || !reflect.DeepEqual(got, want) { + t.Fatal("mutating a List result changed the stored document", err) + } +} + +func TestListReappliesOutputRetention(t *testing.T) { + st := listStore(t) + _, err := st.Create(factory.Item{Stream: &factory.Stream{Log: []factory.LogLine{ + {At: time.Now(), Text: "older"}, {At: time.Now(), Text: "newer"}, + }}}) + if err != nil { + t.Fatal(err) + } + bytes := 100 + st.SetOutputPolicy(func() (int, int) { return 1, bytes }) + if got := listOne(t, st); len(got.Stream.Log) != 2 { + t.Fatal("initial output missing") + } + before := st.DocumentReads() + bytes = 5 + if got := listOne(t, st); len(got.Stream.Log) != 1 || got.Stream.Log[0].Text != "newer" { + t.Fatal("cached output ignored stricter retention") + } + bytes = 100 + if got := listOne(t, st); len(got.Stream.Log) != 2 || got.Stream.Log[0].Text != "older" { + t.Fatal("a retained view destroyed the cached output") + } + if n := st.DocumentReads() - before; n != 0 { + t.Fatalf("retention read %d unchanged documents, want 0", n) + } +} + +func TestListReappliesRepositoryMigration(t *testing.T) { + st := listStore(t) + made, err := st.Create(factory.Item{Repo: "ledger", Places: []string{"ledger"}}) + if err != nil { + t.Fatal(err) + } + if got := listOne(t, st); got.Repo != "ledger" { + t.Fatal("unresolved repository changed") + } + if err := st.SetRepos([]string{"acme/ledger"}); err != nil { + t.Fatal(err) + } + if got := listOne(t, st); got.Repo != "acme/ledger" || got.Places[0] != "acme/ledger" || !got.Changed.Equal(made.Changed) { + t.Fatalf("cached legacy document did not migrate: %+v", got) + } + if got, err := st.Get(made.ID); err != nil || got.Repo != "acme/ledger" { + t.Fatal("migration was not persisted", err) + } + listOne(t, st) // The migration itself replaced the file. + before := st.DocumentReads() + listOne(t, st) + if n := st.DocumentReads() - before; n != 0 { + t.Fatalf("migration kept rereading: %d", n) + } +} + +func TestListSeesRenameWithSameSizeAndTime(t *testing.T) { + st := listStore(t) + made, err := st.Create(factory.Item{Title: "before"}) + if err != nil { + t.Fatal(err) + } + listOne(t, st) + path := st.ItemPath(made.ID) + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + // Replace one equal-length string without changing the document's encoding. + if strings.Count(string(data), `"before"`) != 1 { + t.Fatal("expected exactly one title to replace") + } + data = []byte(strings.Replace(string(data), `"before"`, `"after!"`, 1)) + tmp := filepath.Join(st.Root(), ".replacement") + if err := os.WriteFile(tmp, data, 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(tmp, info.ModTime(), info.ModTime()); err != nil { + t.Fatal(err) + } + if err := os.Rename(tmp, path); err != nil { + t.Fatal(err) + } + if next, err := os.Stat(path); err != nil || next.Size() != info.Size() || !next.ModTime().Equal(info.ModTime()) || os.SameFile(info, next) { + t.Fatal("replacement did not preserve size/time with a new identity", err) + } + before := st.DocumentReads() + if got := listOne(t, st); got.Title != "after!" { + t.Fatal("same-size/time replacement was hidden") + } + if n := st.DocumentReads() - before; n != 1 { + t.Fatalf("replacement read %d documents, want 1", n) + } +} From 1ddfae4b44a19a35d17d2c283ab3c1df45c5ad69 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Wed, 4 Nov 2026 13:51:56 -0500 Subject: [PATCH 4/7] fix: authorize lease steps from the locked job after floor selection Keep file-identity reuse and detached List results. Prove external changes, retention, migration and concurrent looks. Derive thread-only authority under the item lock so a Stop during selection cannot restart job steps. Co-Authored-By: Claude Opus 5.5 --- docs/changes/unreleased/c61-floor-reads.md | 15 ++++ internal/factory/lease/floor_reads_test.go | 45 +++++++++++ internal/factory/lease/lease.go | 11 ++- internal/factory/serve/floor_reads_test.go | 89 ++++++++++++++++++++++ internal/factory/store/list_test.go | 9 ++- 5 files changed, 163 insertions(+), 6 deletions(-) create mode 100644 docs/changes/unreleased/c61-floor-reads.md diff --git a/docs/changes/unreleased/c61-floor-reads.md b/docs/changes/unreleased/c61-floor-reads.md new file mode 100644 index 0000000000..580a9b3949 --- /dev/null +++ b/docs/changes/unreleased/c61-floor-reads.md @@ -0,0 +1,15 @@ +--- +kind: fixed +title: Idle factory polls read only job documents that changed +surface: [engine] +invalidates: + - "Idle lease pulls, settlement and runner capacity checks decoded every job document on each look. They now reuse unchanged documents after checking every file's identity, size and modification time." +--- + +Each List result owns its mutable data. Repository migration and output retention +still run on each look. Writes from another store and replacements with unchanged +size and modification time are visible on the next look. + +Lease grants and execution gates still read the job under its lock. A Stop that +lands during selection permits only a thread-only lease for a pending reply, +without restarting job steps. diff --git a/internal/factory/lease/floor_reads_test.go b/internal/factory/lease/floor_reads_test.go index 3fa9d62e34..f3bc8cf42f 100644 --- a/internal/factory/lease/floor_reads_test.go +++ b/internal/factory/lease/floor_reads_test.go @@ -197,3 +197,48 @@ func TestPullRechecksLockedJobAfterListing(t *testing.T) { }) } } + +func TestFloorLooksRaceExternalWrites(t *testing.T) { + f := setup(t) + it := f.add(t, factory.Item{State: factory.StateQueued, Assignee: "sam", OverLimit: true}) + other, err := store.Open(f.store.Root()) + if err != nil { + t.Fatal(err) + } + start := make(chan struct{}) + var wg sync.WaitGroup + for range 3 { + wg.Add(1) + go func() { + defer wg.Done() + <-start + for range 30 { + if _, err := f.desk.Pull(worker("stranger", "other")); !errors.Is(err, lease.ErrNoJob) { + t.Error(err) + } + if _, err := f.desk.OverLimit(worker("idle", "sam")); err != nil { + t.Error(err) + } + } + }() + } + close(start) + for i := range 30 { + stopped := i%2 == 0 + if err := other.Update(it.ID, func(cur *factory.Item) error { + if stopped { + factory.MarkPersonStopped(cur) + } else { + factory.ClearPersonStop(cur) + } + return nil + }); err != nil { + t.Fatal(err) + } + n, err := f.desk.OverLimit(worker("idle", "sam")) + if err != nil || (n == 0) != stopped { + t.Errorf("next look after write %d: capacity=%d, stopped=%v, err=%v", i, n, stopped, err) + } + } + wg.Wait() +} diff --git a/internal/factory/lease/lease.go b/internal/factory/lease/lease.go index 2116faa171..ff880df2f4 100644 --- a/internal/factory/lease/lease.go +++ b/internal/factory/lease/lease.go @@ -239,8 +239,8 @@ func (d *Desk) Pull(worker Worker) (Lease, error) { if _, err := rand.Read(token[:]); err != nil { return false, err } - taken = Lease{ID: "lease_" + hex.EncodeToString(token[:]), Job: it.ID, Worker: worker.ID, Machine: worker.Machine, Handle: worker.Handle, Since: now, HeartbeatAt: now, ExpiresAt: now.Add(d.TTL()), ThreadOnly: factory.StoppedByPerson(it) || it.State != factory.StateQueued || paused(it)} - return false, d.reserve(doc, worker, taken) + taken = Lease{ID: "lease_" + hex.EncodeToString(token[:]), Job: it.ID, Worker: worker.ID, Machine: worker.Machine, Handle: worker.Handle, Since: now, HeartbeatAt: now, ExpiresAt: now.Add(d.TTL())} + return false, d.reserve(doc, worker, &taken) } if full { return false, ErrWorkerBusy @@ -257,13 +257,16 @@ func (d *Desk) Pull(worker Worker) (Lease, error) { // may have changed it after List. THE LEASE IS DURABLE BEFORE RUNNING: a crash // between files leaves a reserved queued job that expiry can recover, never // running work without a lease that another worker could immediately take. -func (d *Desk) reserve(doc *document, worker Worker, taken Lease) error { +func (d *Desk) reserve(doc *document, worker Worker, taken *Lease) error { return d.store.Update(taken.Job, func(it *factory.Item) error { if !eligible(*it, worker) { return ErrNoJob } + // A Stop may have landed after List. A pending reply may still borrow + // its manager, but only the locked job can authorize running steps. + taken.ThreadOnly = factory.CheckPersonStop(*it) != nil || it.State != factory.StateQueued || paused(*it) delete(doc.Withdrawn, worker.ID) - doc.Leases = append(doc.Leases, taken) + doc.Leases = append(doc.Leases, *taken) if err := d.write(*doc); err != nil { return err } diff --git a/internal/factory/serve/floor_reads_test.go b/internal/factory/serve/floor_reads_test.go index 0c73a162e0..1675ce96fb 100644 --- a/internal/factory/serve/floor_reads_test.go +++ b/internal/factory/serve/floor_reads_test.go @@ -2,6 +2,7 @@ package serve_test import ( "net/http/httptest" + "sync" "testing" "github.com/Agent-Field/codeaf/internal/factory" @@ -50,3 +51,91 @@ func TestIdleRequestsReadOnlyChangedJobs(t *testing.T) { t.Fatalf("settle and capacity after one change: read %d documents, want 1", n) } } + +func TestSettleSeesExternalStopStartAndBudget(t *testing.T) { + for _, change := range []string{"stop", "start", "budget", "create"} { + t.Run(change, func(t *testing.T) { + f := newV2Jobs(t) + in := autoProof() + in.Autonomy = factory.AutonomyManual + if change == "start" { + in.Autonomy = factory.AutonomyAuto + factory.MarkPersonStopped(&in) + } + if change == "budget" { + in.Autonomy, in.Cap = factory.AutonomyAuto, 1 + } + it := f.create(t, in) + f.read(t, it) // Warm List while publication is blocked. + other, err := store.Open(f.st.Root()) + if err != nil { + t.Fatal(err) + } + if change == "create" { + in = autoProof() + in.Repo = "acme/api" + it, err = other.Create(in) + } else { + err = other.Update(it.ID, func(cur *factory.Item) error { + cur.Autonomy = factory.AutonomyAuto + switch change { + case "stop": + factory.MarkPersonStopped(cur) + case "start": + factory.ClearPersonStop(cur) + case "budget": + cur.Cap = 10 + } + return nil + }) + } + if err != nil { + t.Fatal(err) + } + f.read(t, it) // The next request must settle the new facts. + got, err := f.st.Get(it.ID) + if err != nil || got.Publishing != (change != "stop") { + t.Fatalf("next settlement after %s: publishing=%v, err=%v", change, got.Publishing, err) + } + }) + } +} + +func TestSettleRechecksStopAndBudgetAfterListing(t *testing.T) { + for _, change := range []string{"stop", "budget"} { + t.Run(change, func(t *testing.T) { + f := newV2Jobs(t) + it := f.create(t, autoProof()) + if _, err := f.st.List(); err != nil { + t.Fatal(err) + } + other, err := store.Open(f.st.Root()) + if err != nil { + t.Fatal(err) + } + // Retention is the public seam after a candidate is copied. Change + // the disk before settlement can act on that candidate. + var once sync.Once + f.st.SetOutputPolicy(func() (int, int) { + once.Do(func() { + if err := other.Update(it.ID, func(cur *factory.Item) error { + if change == "stop" { + factory.MarkPersonStopped(cur) + } else { + cur.Cap = 1 + } + return nil + }); err != nil { + t.Fatal(err) + } + }) + return 0, 0 + }) + f.read(t, it) + got, err := f.st.Get(it.ID) + if err != nil || got.Publishing { + t.Fatalf("settlement used stale %s authority: publishing=%v, err=%v", change, got.Publishing, err) + } + }) + } +} diff --git a/internal/factory/store/list_test.go b/internal/factory/store/list_test.go index eee145c00c..7c0c6affd4 100644 --- a/internal/factory/store/list_test.go +++ b/internal/factory/store/list_test.go @@ -85,13 +85,14 @@ func TestListOwnsMutableDocuments(t *testing.T) { func TestListReappliesOutputRetention(t *testing.T) { st := listStore(t) _, err := st.Create(factory.Item{Stream: &factory.Stream{Log: []factory.LogLine{ - {At: time.Now(), Text: "older"}, {At: time.Now(), Text: "newer"}, + {At: time.Now().Add(-48 * time.Hour), Text: "older"}, {At: time.Now(), Text: "newer"}, }}}) if err != nil { t.Fatal(err) } bytes := 100 - st.SetOutputPolicy(func() (int, int) { return 1, bytes }) + days := 3 + st.SetOutputPolicy(func() (int, int) { return days, bytes }) if got := listOne(t, st); len(got.Stream.Log) != 2 { t.Fatal("initial output missing") } @@ -104,6 +105,10 @@ func TestListReappliesOutputRetention(t *testing.T) { if got := listOne(t, st); len(got.Stream.Log) != 2 || got.Stream.Log[0].Text != "older" { t.Fatal("a retained view destroyed the cached output") } + days = 1 + if got := listOne(t, st); len(got.Stream.Log) != 1 || got.Stream.Log[0].Text != "newer" { + t.Fatal("cached output ignored age retention") + } if n := st.DocumentReads() - before; n != 0 { t.Fatalf("retention read %d unchanged documents, want 0", n) } From 0f16d36b7c78b977f5f7eb1655b383aeaf749847 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Wed, 4 Nov 2026 14:00:13 -0500 Subject: [PATCH 5/7] docs: name pull request 2361 in its change entry Co-Authored-By: Claude Opus 5.5 --- .../unreleased/{c61-floor-reads.md => 2361-floor-reads.md} | 1 + 1 file changed, 1 insertion(+) rename docs/changes/unreleased/{c61-floor-reads.md => 2361-floor-reads.md} (98%) diff --git a/docs/changes/unreleased/c61-floor-reads.md b/docs/changes/unreleased/2361-floor-reads.md similarity index 98% rename from docs/changes/unreleased/c61-floor-reads.md rename to docs/changes/unreleased/2361-floor-reads.md index 580a9b3949..3f540b6a5e 100644 --- a/docs/changes/unreleased/c61-floor-reads.md +++ b/docs/changes/unreleased/2361-floor-reads.md @@ -1,6 +1,7 @@ --- kind: fixed title: Idle factory polls read only job documents that changed +pr: 2361 surface: [engine] invalidates: - "Idle lease pulls, settlement and runner capacity checks decoded every job document on each look. They now reuse unchanged documents after checking every file's identity, size and modification time." From 71062aa156c05c8c95442f5b0940e5196969445a Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Wed, 4 Nov 2026 15:02:49 -0500 Subject: [PATCH 6/7] fix: List does not hold its index across migration An Update callback holding an item lock could deadlock against a List migrating that item. Published indexes are never changed, so a scan now reads a snapshot and the lock guards only the pointer. Co-Authored-By: Claude Opus 5.5 --- internal/factory/store/list.go | 2 +- internal/factory/store/list_test.go | 55 +++++++++++++++++++++++++++++ internal/factory/store/store.go | 10 ++++-- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/internal/factory/store/list.go b/internal/factory/store/list.go index 6a8fc0b85f..c1e3f15f90 100644 --- a/internal/factory/store/list.go +++ b/internal/factory/store/list.go @@ -7,7 +7,7 @@ import ( ) // listIndex owns the decoded documents. Nothing returned by List aliases them. -// Writes never take this lock: migration may itself write while List holds it. +// mu guards only the docs pointer and is never held across a read or a write. type listIndex struct { mu sync.Mutex docs map[string]listedDoc diff --git a/internal/factory/store/list_test.go b/internal/factory/store/list_test.go index 7c0c6affd4..da1b1df0aa 100644 --- a/internal/factory/store/list_test.go +++ b/internal/factory/store/list_test.go @@ -11,6 +11,7 @@ import ( "github.com/Agent-Field/codeaf/internal/factory" "github.com/Agent-Field/codeaf/internal/factory/store" + "sync/atomic" ) func listStore(t *testing.T) *store.Store { @@ -182,3 +183,57 @@ func TestListSeesRenameWithSameSizeAndTime(t *testing.T) { t.Fatalf("replacement read %d documents, want 1", n) } } + +// An Update callback holding an item's lock may list the floor while another +// List is migrating that item. List must not wait on the item lock while holding +// anything the callback's List needs. +func TestListDoesNotHoldItsIndexAcrossMigration(t *testing.T) { + st := listStore(t) + if _, err := st.Create(factory.Item{Repo: "ledger", Stream: &factory.Stream{}}); err != nil { + t.Fatal(err) + } + legacy, err := st.Create(factory.Item{Repo: "ledger"}) + if err != nil { + t.Fatal(err) + } + if err := st.SetRepos([]string{"acme/ledger"}); err != nil { + t.Fatal(err) + } + locked, listing, proceed := make(chan struct{}), make(chan struct{}), make(chan struct{}) + finished := make(chan error, 2) + var once atomic.Bool + st.SetOutputPolicy(func() (int, int) { + if once.CompareAndSwap(false, true) { + close(listing) + <-proceed + } + return 0, 0 + }) + go func() { + finished <- st.Update(legacy.ID, func(*factory.Item) error { + close(locked) + <-listing + if err := st.SetRepos(nil); err != nil { + return err + } + close(proceed) + _, err := st.List() + return err + }) + }() + <-locked + go func() { + _, err := st.List() + finished <- err + }() + for range 2 { + select { + case err := <-finished: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("deadlock: List held its index while waiting for an item lock") + } + } +} diff --git a/internal/factory/store/store.go b/internal/factory/store/store.go index 42ce4068e3..4386b64ac1 100644 --- a/internal/factory/store/store.go +++ b/internal/factory/store/store.go @@ -234,8 +234,12 @@ func (st *Store) Get(id int) (factory.Item, error) { // newer-schema document is skipped rather than fatal: ONE DOCUMENT FROM THE // FUTURE, OR ONE TORN BY A FULL DISK, MUST NOT EMPTY THE WHOLE FLOOR. func (st *Store) List() ([]factory.Item, error) { + // A published index is never changed, so a scan reads its snapshot unlocked: + // migration below takes item locks. An older scan publishing last costs only + // reads, because each file's identity is checked on every look. st.list.mu.Lock() - defer st.list.mu.Unlock() + previous := st.list.docs + st.list.mu.Unlock() entries, err := os.ReadDir(st.root) if err != nil { if os.IsNotExist(err) { @@ -258,7 +262,7 @@ func (st *Store) List() ([]factory.Item, error) { if _, err := strconv.Atoi(strings.TrimSuffix(entry.Name(), ".json")); err != nil { continue } - d, err := st.listed(filepath.Join(st.root, entry.Name()), st.list.docs, next) + d, err := st.listed(filepath.Join(st.root, entry.Name()), previous, next) if err != nil { continue } @@ -276,7 +280,9 @@ func (st *Store) List() ([]factory.Item, error) { st.retainOutput(&d.Item) items = append(items, d.Item) } + st.list.mu.Lock() st.list.docs = next + st.list.mu.Unlock() sort.SliceStable(items, func(a, b int) bool { if items[a].Created.Equal(items[b].Created) { return items[a].ID > items[b].ID From 4b9702dcb9fbd75500d93b350012c08ca955af72 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Wed, 4 Nov 2026 15:04:55 -0500 Subject: [PATCH 7/7] fix: List re-reads every job document at least once a minute File identity cannot see an in-place rewrite that keeps its size and time, and a Stop or budget must not rest on file times with no limit. Co-Authored-By: Claude Opus 5.5 --- docs/changes/unreleased/2361-floor-reads.md | 4 +- internal/factory/store/list.go | 36 ++++++++++- internal/factory/store/list_reread_test.go | 71 +++++++++++++++++++++ internal/factory/store/store.go | 11 +--- 4 files changed, 111 insertions(+), 11 deletions(-) create mode 100644 internal/factory/store/list_reread_test.go diff --git a/docs/changes/unreleased/2361-floor-reads.md b/docs/changes/unreleased/2361-floor-reads.md index 3f540b6a5e..aef96fb5a7 100644 --- a/docs/changes/unreleased/2361-floor-reads.md +++ b/docs/changes/unreleased/2361-floor-reads.md @@ -9,7 +9,9 @@ invalidates: Each List result owns its mutable data. Repository migration and output retention still run on each look. Writes from another store and replacements with unchanged -size and modification time are visible on the next look. +size and modification time are visible on the next look. Every document is read +again at least once a minute, so an in-place edit that keeps its size and time is +seen within that bound. Lease grants and execution gates still read the job under its lock. A Stop that lands during selection permits only a thread-only lease for a pending reply, diff --git a/internal/factory/store/list.go b/internal/factory/store/list.go index c1e3f15f90..e525d088a7 100644 --- a/internal/factory/store/list.go +++ b/internal/factory/store/list.go @@ -4,13 +4,47 @@ import ( "os" "reflect" "sync" + "time" ) +// listRereadEvery bounds how long a document may be trusted by file identity. An +// in-place rewrite keeping its size and time looks unchanged, and a Stop or a +// budget must not rest on file times with no limit. +const listRereadEvery = time.Minute + // listIndex owns the decoded documents. Nothing returned by List aliases them. -// mu guards only the docs pointer and is never held across a read or a write. +// mu guards only these fields and is never held across a read or a write. type listIndex struct { mu sync.Mutex docs map[string]listedDoc + // fullAt is when the running epoch began. Each epoch starts empty, so its + // first look reads every document. + fullAt time.Time + // epoch stops a scan that began before a full read from publishing over it. + epoch int +} + +// listSnapshot is what one scan reuses. A published map is never changed, so the +// scan may read it unlocked: migration during a scan takes item locks. +func (st *Store) listSnapshot() (map[string]listedDoc, int) { + st.list.mu.Lock() + defer st.list.mu.Unlock() + if now := st.now(); now.Sub(st.list.fullAt) >= listRereadEvery { + st.list.docs, st.list.fullAt = nil, now + st.list.epoch++ + } + return st.list.docs, st.list.epoch +} + +// listPublish keeps a scan's documents unless a newer epoch began meanwhile. A +// scan publishing over a newer one of its epoch costs only reads, because each +// file's identity is checked on every look. +func (st *Store) listPublish(docs map[string]listedDoc, epoch int) { + st.list.mu.Lock() + defer st.list.mu.Unlock() + if epoch == st.list.epoch { + st.list.docs = docs + } } type listedDoc struct { diff --git a/internal/factory/store/list_reread_test.go b/internal/factory/store/list_reread_test.go new file mode 100644 index 0000000000..def4ea5c89 --- /dev/null +++ b/internal/factory/store/list_reread_test.go @@ -0,0 +1,71 @@ +package store + +import ( + "os" + "strings" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory" +) + +// File identity cannot see an in-place rewrite that keeps the size and time, so +// no decision may rest on it for longer than the bound. +func TestListSeesInPlaceSameTimeEditWithinItsBound(t *testing.T) { + st, err := Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + now := time.Now() + st.clock = func() time.Time { return now } + made, err := st.Create(factory.Item{Title: "before"}) + if err != nil { + t.Fatal(err) + } + title := func() string { + t.Helper() + items, err := st.List() + if err != nil || len(items) != 1 { + t.Fatalf("list: %v, %v", items, err) + } + return items[0].Title + } + if got := title(); got != "before" { + t.Fatalf("title = %q", got) + } + path := st.ItemPath(made.ID) + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(strings.Replace(string(data), `"before"`, `"after!"`, 1)), info.Mode()); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(path, info.ModTime(), info.ModTime()); err != nil { + t.Fatal(err) + } + if next, err := os.Stat(path); err != nil || !os.SameFile(info, next) || next.Size() != info.Size() || !next.ModTime().Equal(info.ModTime()) { + t.Fatalf("the edit did not keep the file's identity: %v", err) + } + + now = now.Add(listRereadEvery - time.Second) + before := st.DocumentReads() + title() + if got := st.DocumentReads() - before; got != 0 { + t.Fatalf("inside the bound a look read %d documents, want 0", got) + } + + now = now.Add(time.Second) + if got := title(); got != "after!" { + t.Fatalf("at the bound title = %q, want the edit", got) + } + before = st.DocumentReads() + title() + if got := st.DocumentReads() - before; got != 0 { + t.Fatalf("after the re-read a look read %d documents, want 0", got) + } +} diff --git a/internal/factory/store/store.go b/internal/factory/store/store.go index 4386b64ac1..95693b4434 100644 --- a/internal/factory/store/store.go +++ b/internal/factory/store/store.go @@ -234,12 +234,7 @@ func (st *Store) Get(id int) (factory.Item, error) { // newer-schema document is skipped rather than fatal: ONE DOCUMENT FROM THE // FUTURE, OR ONE TORN BY A FULL DISK, MUST NOT EMPTY THE WHOLE FLOOR. func (st *Store) List() ([]factory.Item, error) { - // A published index is never changed, so a scan reads its snapshot unlocked: - // migration below takes item locks. An older scan publishing last costs only - // reads, because each file's identity is checked on every look. - st.list.mu.Lock() - previous := st.list.docs - st.list.mu.Unlock() + previous, epoch := st.listSnapshot() entries, err := os.ReadDir(st.root) if err != nil { if os.IsNotExist(err) { @@ -280,9 +275,7 @@ func (st *Store) List() ([]factory.Item, error) { st.retainOutput(&d.Item) items = append(items, d.Item) } - st.list.mu.Lock() - st.list.docs = next - st.list.mu.Unlock() + st.listPublish(next, epoch) sort.SliceStable(items, func(a, b int) bool { if items[a].Created.Equal(items[b].Created) { return items[a].ID > items[b].ID