mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 05:01:28 -10:00
KRATOS
1c30f4c117
* audit fixes: cron lock, accounting race, O(n^2) hot paths, /style wiring
Fixes five verified findings from a code audit. Two related findings were already
addressed upstream and three need larger/separate work (noted below).
H-1 cron store: cross-process lock for the read-modify-write.
Two schedulers seeing a job due raced the metadata read-modify-write; fireJob
even documented it ("full atomicity needs file locking"). Adds a per-job O_EXCL
lock (mirroring internal/oauth's pattern, stale-reclaim included) and a Mutate
primitive doing the re-read+modify+write atomically. Update/Remove lock too. The
lock is held only for the millisecond RMW, never across a job's exec.
M-5 specialist accounting: close the check-then-append race.
specialistEventExists (unlocked ReadEvents) + a later AppendEvent let two
finishers (TaskOutput poll vs onExit, even cross-process) both pass the check and
write duplicate stop/usage events. Adds sessions.Store.AppendEventUnlessExists
(check + append atomic under the session lock) and routes the stop and usage
paths through it.
M-2 hooks audit: O(1) sequence via tail read.
append re-read and scanned the whole audit log every call to find the max
sequence (O(n^2) over a session). Reads only the file tail now; multi-process
safe (the log is a shared global file), unlike an in-memory counter which would
collide sequences across processes.
M-4 transcript rehydration: O(n^2) -> O(n).
The three resume/rewind/compact rehydration loops called appendTranscriptRow per
event, each re-scanning every existing row for the keyed-row dedup. A bulk
appendTranscriptRowsDedup builds the key set once; semantics are identical.
M-6 /style: actually shape responses.
The TUI /style command stored a style but never threaded it anywhere. Adds
agent.Options.ResponseStyle, a system-prompt directive for concise/explanatory/
review, and wires m.responseStyle through the run. "balanced" is a no-op (prompt
byte-identical to before).
Already addressed upstream (no change):
- M-1 forked-session double-count: Store.Fork already skips EventUsage events,
with a comment naming the exact double-count it prevents.
- M-3 lineage O(N^2): Tree already does a single store.List() scan and indexes
children in memory; no path calls List per node.
Deferred (need larger/separate changes):
- M-7 escalate_model in the TUI needs a ModelSwitcher that builds a provider
mid-run; registering the tool alone yields a tool that errors on call.
- L-1 MCP SSE reconnect is a stream-lifecycle change (low-likelihood after the
8 MiB per-event cap).
- L-2 dead-code removal belongs in a dedicated cleanup sweep.
Tests: per-fix unit tests including concurrency (cron Mutate, sessions
AppendEventUnlessExists) under -race; full go test ./... green.
* fix(windows): treat ERROR_ACCESS_DENIED as lock contention (cron + oauth)
The Windows smoke run failed TestMutateIsAtomicUnderConcurrency with
"...lock: Access is denied" (FireCount 19/20). On Windows a concurrent holder's
os.Remove leaves the lock file in a delete-pending state, so an O_EXCL create
races it with ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist — the
retry loop only treated ErrExist as contention and hard-failed.
Treat os.ErrPermission as contention (retry) in the new cron lock and in the
oauth lock it mirrors (oauth's own concurrent test exposed the same pre-existing
bug under the added load). Unix is unaffected — it returns ErrExist.
* fix: address review — cross-process atomic audit append + AppendRun guard
Addresses the review feedback on this PR.
P1 (blocking): AuditStore.append is now cross-process atomic.
The O(1) tail read observes another process's prior appends, but the read-then-
append was not atomic across processes — two could both read sequence N and write
N+1. Wraps lastSequence()+append in a per-file O_EXCL lock (hooks/lock.go, same
stale-reclaim + Windows ErrPermission handling as cron/oauth); store.mu stays as
the in-process fast path. Adds TestAuditStoreSequenceConcurrent (4 stores x 12
goroutines on one file -> unique dense sequences), which the prior sequential
test could not catch. Walked back the comment that oversold the tail read.
P2: AppendRun no longer resurrects a removed job.
fireJob's AppendRun ran after Mutate released the per-job lock; a Remove in that
window let AppendRun's MkdirAll recreate the dir with an orphaned runs.jsonl.
AppendRun now takes the per-job lock and bails if metadata.json is gone. Adds
TestAppendRunDoesNotResurrectRemovedJob.
P2: lastSequence ReadAt tolerates a short final read.
ReadAt may return io.EOF with n < len(buf) if the file shrank between Stat and
the read; treat EOF as non-fatal and use buf[:n] (matches lastEventSequence).
P3: oauth stale-reclaim no-op removed.
The double os.ReadFile "stale == data" guard compared the file to itself read
twice in a row (always true); staleness is decided by the mtime check, so the
guard is dropped (behavior-preserving).
Tests under -race; cron/hooks/oauth green. A shared filelock helper would DRY the
three near-identical locks — left as a follow-up to avoid churning the
already-green cron/oauth packages here.
53 lines
1.6 KiB
Go
53 lines
1.6 KiB
Go
package hooks
|
|
|
|
import (
|
|
"path/filepath"
|
|
"testing"
|
|
)
|
|
|
|
// The audit append must assign monotonic sequences via an O(1) tail read, and a
|
|
// second store on the same (shared, global) file must CONTINUE the sequence — an
|
|
// in-memory counter would have restarted at 1 and collided across processes.
|
|
func TestAuditStoreSequenceTailRead(t *testing.T) {
|
|
path := filepath.Join(t.TempDir(), "audit.jsonl")
|
|
store, err := NewAuditStore(AuditStoreOptions{AuditPath: path})
|
|
if err != nil {
|
|
t.Fatalf("NewAuditStore: %v", err)
|
|
}
|
|
for i := 1; i <= 50; i++ {
|
|
ev, err := store.AppendStarted(AppendStartedInput{HookID: "h", Event: "beforeTool"})
|
|
if err != nil {
|
|
t.Fatalf("append %d: %v", i, err)
|
|
}
|
|
if ev.Sequence != i {
|
|
t.Fatalf("event %d Sequence = %d, want %d", i, ev.Sequence, i)
|
|
}
|
|
}
|
|
|
|
// A separate store handle on the SAME file (like a concurrent process).
|
|
store2, err := NewAuditStore(AuditStoreOptions{AuditPath: path})
|
|
if err != nil {
|
|
t.Fatalf("NewAuditStore #2: %v", err)
|
|
}
|
|
ev, err := store2.AppendStarted(AppendStartedInput{HookID: "h2", Event: "afterTool"})
|
|
if err != nil {
|
|
t.Fatalf("second-store append: %v", err)
|
|
}
|
|
if ev.Sequence != 51 {
|
|
t.Fatalf("second store next Sequence = %d, want 51 (tail read must observe prior events)", ev.Sequence)
|
|
}
|
|
|
|
events, err := store2.ReadEvents()
|
|
if err != nil {
|
|
t.Fatalf("ReadEvents: %v", err)
|
|
}
|
|
if len(events) != 51 {
|
|
t.Fatalf("expected 51 events, got %d", len(events))
|
|
}
|
|
for i, e := range events {
|
|
if e.Sequence != i+1 {
|
|
t.Fatalf("events[%d].Sequence = %d, want %d (must be unique + monotonic)", i, e.Sequence, i+1)
|
|
}
|
|
}
|
|
}
|