diff --git a/PERF.md b/PERF.md index 9afc0805e9..3dff833578 100644 --- a/PERF.md +++ b/PERF.md @@ -2929,3 +2929,16 @@ receipts are atomically replaced and synced before committing deletion. Startup walks only the two saved-conversation directory levels, without following symlinked buckets. `internal/session/deletion_revision_test.go` drives completion and timeout with held channels and a zero test grace rather than sleeping for the production deadline. + +## Learned provider-fact persistence + +Within one process, `quirksStore.saveMutex` serializes saves from before the +snapshot through atomic file replacement, so an older snapshot cannot replace +newer learned facts. The memo-reader mutex is still held only while copying +maps, never across file I/O. Requests still schedule persistence asynchronously; +waiting writers do not move a disk wait onto the request path. The existing +unique temporary files and atomic rename remain unchanged. This is an in-process +ordering guarantee, not a merge of caches written by separate processes. + +Restart fixtures drain their owned scheduled writes before discarding process +memory. No timeout, response limit or request/retry budget changes. diff --git a/docs/changes/unreleased/1824-ordered-provider-memo-saves.md b/docs/changes/unreleased/1824-ordered-provider-memo-saves.md new file mode 100644 index 0000000000..4dacd5d9e5 --- /dev/null +++ b/docs/changes/unreleased/1824-ordered-provider-memo-saves.md @@ -0,0 +1,8 @@ +--- +kind: fixed +title: concurrent provider-memo saves preserve every learned fact at restart +pr: 1824 +surface: [engine, chat] +invalidates: + - "Atomic provider-memo replacement kept valid JSON but could let an older in-process snapshot overwrite a newer learned fact. Saves now serialize from before snapshot through replacement, while requests still persist asynchronously; separate processes do not merge their caches." +--- diff --git a/internal/manual/chat/models-and-cost.md b/internal/manual/chat/models-and-cost.md index ce26e86d46..23347eb2ff 100644 --- a/internal/manual/chat/models-and-cost.md +++ b/internal/manual/chat/models-and-cost.md @@ -2769,6 +2769,19 @@ An endpoint's explicit total limit is remembered by base URL, model and provider prompt's length is **not** a total context limit. Old model-wide `served_window` guesses are no longer used to size requests. +## Learned model request facts after restarting + +codeaf remembers request shapes an endpoint refused, such as a disabled reasoning +pass or a cache breakpoint, in `model-quirks.json`. Saves made by one running +process preserve its latest learned facts when the writes finish, even when +several requests learn at once. Saving stays in the background; the next request +uses the fact already held in memory. + +A missing, unreadable or unwritable memo is a cache miss: codeaf may learn the +endpoint's refusal again. Concurrent separate processes sharing the same profile +still replace the same cache file; their learned sets are not merged. Endpoint +context limits retain their existing 30-minute lifetime. + ## How much room is left for the answer — the output cap, max_tokens and the safety margin When no output cap was requested, the total output allowance is the smaller of the answer diff --git a/internal/provider/quirks.go b/internal/provider/quirks.go index adb07f0d2d..474814e213 100644 --- a/internal/provider/quirks.go +++ b/internal/provider/quirks.go @@ -40,6 +40,10 @@ const quirksFile = "model-quirks.json" type quirksStore struct { mutex sync.Mutex path string + // saveMutex orders each snapshot with its replacement on disk. Without + // it, an older snapshot can finish after a newer one and erase a fact + // already saved by this process. Readers never take this writer lock. + saveMutex sync.Mutex // mandatory is the learned set, keyed by normalized model. mandatory map[string]time.Time // disableIgnored is the answer-side twin of mandatory: models whose endpoint @@ -484,6 +488,10 @@ func (q *quirksStore) settle() { q.writes.Wait() } // fact, and a failure is silent: a cache that could not be written is a cache // that will be rebuilt, not an error the run should carry. func (q *quirksStore) save() { + // Take the writer lock before snapshotting, not only before renaming: a + // snapshot copied while another writer owns the file can already be stale. + q.saveMutex.Lock() + defer q.saveMutex.Unlock() path, wire := q.snapshot() if path == "" { return diff --git a/internal/provider/quirks_persistence_test.go b/internal/provider/quirks_persistence_test.go new file mode 100644 index 0000000000..936788115c --- /dev/null +++ b/internal/provider/quirks_persistence_test.go @@ -0,0 +1,43 @@ +package provider + +import ( + "fmt" + "path/filepath" + "sync" + "testing" + "time" +) + +// Concurrent request completions teach one process different facts. Once its +// scheduled writes have finished, a fresh process must recover every fact; +// atomic individual files alone do not establish that ordering. +func TestConcurrentQuirkSavesRetainEveryLearnedFactAtRestart(t *testing.T) { + store := &quirksStore{mandatory: map[string]time.Time{}} + path := filepath.Join(t.TempDir(), quirksFile) + store.load(path) + const facts = 32 + start := make(chan struct{}) + var learners sync.WaitGroup + for i := 0; i < facts; i++ { + learners.Add(1) + go func(i int) { + defer learners.Done() + <-start + if store.note(fmt.Sprintf("concurrent/model-%d", i), time.Now()) { + store.persist() + } + }(i) + } + close(start) + learners.Wait() + store.settle() + + fresh := &quirksStore{mandatory: map[string]time.Time{}} + fresh.load(path) + for i := 0; i < facts; i++ { + model := fmt.Sprintf("concurrent/model-%d", i) + if !fresh.knows(model) { + t.Errorf("a completed save lost %s at restart", model) + } + } +}