commit caa60952b232496abb61fc34c14a1ba583483003
parent 8f323544e21085b7879909fd01c1a44cea630708
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date: Wed, 23 Sep 2026 21:12:34 -0300
odin: read the Go tool's cache as it stands and never write over it blind
The first live run replayed nothing: the Go tool writes each answer's
time as RFC 3339 and the Odin cache read it as a number, so the whole
file failed to load and every ask missed. The time is a string now,
written the same way, so either tool reads the other's file. A file that
was there and could not be read is protected from a save, which would
have replaced every answer it held with one run's few. Both tools name
the skipped jobs in the jobs' own order rather than in the order the
threads finished, and REVIEW_DEBUG_CACHE prints each ask's key.
On the synthetic repository both tools now report the same 18 findings
from the same 8 replayed answers, message for message.
Diffstat:
4 files changed, 101 insertions(+), 12 deletions(-)
diff --git a/client.go b/client.go
@@ -6,6 +6,7 @@ import (
"fmt"
"os"
"regexp"
+ "slices"
"strings"
"sync"
"time"
@@ -192,6 +193,13 @@ func (r Reviewer) Run(ctx context.Context, change *Change, jobs []Job) RunResult
}
wg.Wait()
}
+ // The skipped jobs are named in the jobs' own order, whichever
+ // goroutine said so first, so the report reads the same on every run.
+ order := map[string]int{}
+ for i, job := range jobs {
+ order[job.Name] = i
+ }
+ slices.SortFunc(result.Skipped, func(a, b string) int { return order[a] - order[b] })
if r.Verify {
r.verify(ctx, cuts, jobs, &result)
}
diff --git a/odin/cache/cache.odin b/odin/cache/cache.odin
@@ -12,6 +12,7 @@ package cache
import "core:crypto/sha2"
import "core:encoding/hex"
import "core:encoding/json"
+import "core:fmt"
import "core:os"
import "core:path/filepath"
import "core:slice"
@@ -28,26 +29,32 @@ version :: "review-answers-v1"
// bound is the entry count past which the oldest answers are dropped.
bound :: 4000
-// Entry is one recorded answer and what it cost when it was asked.
+// Entry is one recorded answer and what it cost when it was asked. The
+// time is written as the Go tool writes it, RFC 3339 with the local
+// offset, so that either tool reads the other's file.
Entry :: struct {
text: string `json:"text"`,
tokens_in: int `json:"in"`,
tokens_out: int `json:"out"`,
cached: int `json:"cached"`,
cost: f64 `json:"cost"`,
- at: i64 `json:"at"`,
+ at: string `json:"at"`,
}
// Cache is one file of recorded answers, keyed by the exact question.
// fresh skips the reads but keeps the writes, so --fresh re-asks
-// everything and leaves the answers behind it.
+// everything and leaves the answers behind it. A file that was there and
+// could not be read is kept out of harm's way: nothing is written over
+// it, since a save would replace every answer it holds with this run's
+// few.
Cache :: struct {
- path: string,
- fresh: bool,
- entries: map[string]Entry,
- dirty: bool,
- hits: int,
- lock: sync.Mutex,
+ path: string,
+ fresh: bool,
+ entries: map[string]Entry,
+ dirty: bool,
+ hits: int,
+ protected: bool,
+ lock: sync.Mutex,
}
// open reads the answer file from the user's cache directory, or the path
@@ -76,6 +83,8 @@ open :: proc(fresh: bool, path := "", allocator := context.allocator) -> (c: ^Ca
loaded: map[string]Entry
if json.unmarshal(data, &loaded, allocator = allocator) == nil {
c.entries = loaded
+ } else if len(data) > 0 {
+ c.protected = true
}
return c, true
}
@@ -132,11 +141,29 @@ put :: proc(c: ^Cache, provider_name, system, user: string, answer: provider.Ans
tokens_out = answer.tokens_out,
cached = answer.cached,
cost = answer.cost,
- at = time.time_to_unix(time.now()),
+ at = stamp(),
}
c.dirty = true
}
+// stamp is the present, as RFC 3339 in UTC, which sorts as it ages and
+// which the Go tool's reader accepts.
+stamp :: proc(allocator := context.allocator) -> string {
+ now := time.now()
+ y, mo, d := time.date(now)
+ h, mi, sec := time.clock_from_time(now)
+ return fmt.aprintf(
+ "%04d-%02d-%02dT%02d:%02d:%02dZ",
+ y,
+ int(mo),
+ d,
+ h,
+ mi,
+ sec,
+ allocator = allocator,
+ )
+}
+
// save writes the file back when this run recorded anything, dropping the
// oldest entries past the bound. A failed write stays silent: the cache
// is a saving, not a result.
@@ -146,13 +173,13 @@ save :: proc(c: ^Cache) {
}
sync.mutex_lock(&c.lock)
defer sync.mutex_unlock(&c.lock)
- if !c.dirty {
+ if !c.dirty || c.protected {
return
}
if len(c.entries) > bound {
Aged :: struct {
key: string,
- at: i64,
+ at: string,
}
ages := make([dynamic]Aged, context.temp_allocator)
for k, e in c.entries {
diff --git a/odin/cache/cache_test.odin b/odin/cache/cache_test.odin
@@ -2,6 +2,7 @@ package cache
import "core:os"
import "core:path/filepath"
+import "core:strings"
import "core:testing"
import "../provider"
@@ -39,5 +40,38 @@ answers_replay_across_opens :: proc(t: ^testing.T) {
_, skipped := get(fresh, "p", "s", "u")
testing.expect(t, !skipped, "fresh reads nothing")
testing.expect(t, key("a", "b", "c") != key("a", "b", "d"))
+
+ // The Go tool's file, with its RFC 3339 times, is read as it stands.
+ shared := filepath.join({dir, "go.json"}, context.temp_allocator) or_else ""
+ entry := key("p", "s", "u")
+ testing.expect(
+ t,
+ os.write_entire_file(
+ shared,
+ transmute([]byte)strings.concatenate(
+ {
+ `{"`,
+ entry,
+ `":{"text":"from go","in":1,"out":2,"cached":0,"cost":0.1,"at":"2026-09-23T20:55:09.673084018-03:00"}}`,
+ },
+ ),
+ ) ==
+ nil,
+ )
+ theirs, ok_theirs := open(false, shared)
+ testing.expect(t, ok_theirs)
+ from_go, replayed_go := get(theirs, "p", "s", "u")
+ testing.expect(t, replayed_go, "the Go tool's answer replays")
+ testing.expect_value(t, from_go.text, "from go")
+ testing.expect_value(t, len(stamp()), 20)
+
+ // A file that cannot be read is not written over.
+ broken := filepath.join({dir, "broken.json"}, context.temp_allocator) or_else ""
+ testing.expect(t, os.write_entire_file(broken, transmute([]byte)string("not json")) == nil)
+ guarded, _ := open(false, broken)
+ put(guarded, "p", "s", "u", provider.Answer{text = "mine"})
+ save(guarded)
+ kept, _ := os.read_entire_file_from_path(broken, context.temp_allocator)
+ testing.expect_value(t, string(kept), "not json")
testing.expect_value(t, len(key("a", "b", "c")), 64)
}
diff --git a/odin/reviewer/reviewer.odin b/odin/reviewer/reviewer.odin
@@ -166,6 +166,17 @@ run :: proc(
thread.destroy(th)
}
}
+ // The skipped jobs are named in the jobs' own order, whichever
+ // thread said so first.
+ ordered := make([dynamic]string, allocator)
+ for j in jobs {
+ for name in result.skipped {
+ if name == j.name {
+ append(&ordered, name)
+ }
+ }
+ }
+ result.skipped = ordered
if r.verify {
verify(r, tasks, &result, one_at_a_time, allocator)
}
@@ -284,6 +295,15 @@ answered :: proc(
provider.Answer,
string,
) {
+ if os.get_env("REVIEW_DEBUG_CACHE", context.temp_allocator) != "" {
+ fmt.eprintfln(
+ "cache key %s system=%d user=%d provider=%s",
+ cache.key(r.name, system, user, context.temp_allocator)[:12],
+ len(system),
+ len(user),
+ r.name,
+ )
+ }
if hit, replayed := cache.get(r.cache, r.name, system, user); replayed {
if r.verbose {
fmt.printfln(" %-12s replayed", r.name)
