fix: address verified gadfly P5/#5 findings (contrib/store concurrency)
executus CI / test (pull_request) Successful in 1m34s

All 3 cloud models converged on real concurrency bugs in the SQLite stores:

- AppendVersion (HIGH): the seq key was `SELECT MAX(seq)+1` then INSERT in two
  un-transacted statements with a NON-unique index, AND the Scan error was
  swallowed (seq stayed 0 on failure). Concurrent appends could both land the
  same seq, silently breaking newest-first ordering. Now: one transaction, the
  Scan error is propagated, the (skill_id, seq) index is UNIQUE (the loser of a
  race fails loudly), and an empty SkillID is rejected.
- MarkScheduledRun / MarkAgentScheduledRun (all 3): replaced the Get→mutate→Save
  read-modify-write (lost-update window) with a single atomic UPDATE using
  json_set, so a concurrent Mark/edit can't clobber it. json_set keeps the JSON
  blob's NextRunAt/LastScheduledRunAt consistent with the indexed column;
  RFC3339Nano matches Go's time encoding so the blob still round-trips (tested).
- Open: actually applies PRAGMA busy_timeout=5000 (the doc advertised it but it
  was never set) — a contended writer waits instead of erroring SQLITE_BUSY.
- budgetStore.Add: rejects NaN/Inf secondsUsed (would irrecoverably poison the
  column).

Triaged-but-kept: plaintext webhook secret (documented design, high-entropy URL
key, pre-existing); SQL()/free-form `where` helpers (no untrusted input reaches
them — defense-in-depth notes only).

Core go.sum still free of host/DB deps; contrib/store green (incl. a json_set
blob-round-trip test).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-06-26 23:37:37 -04:00
parent b194a9621d
commit c8a87f1733
5 changed files with 100 additions and 16 deletions
+40 -11
View File
@@ -45,7 +45,7 @@ CREATE TABLE IF NOT EXISTS skill_versions (
seq INTEGER NOT NULL, -- append order, for newest-first
data TEXT NOT NULL
);
CREATE INDEX IF NOT EXISTS idx_skill_versions_skill ON skill_versions(skill_id, seq);`)
CREATE UNIQUE INDEX IF NOT EXISTS idx_skill_versions_skill ON skill_versions(skill_id, seq);`)
if err != nil {
return fmt.Errorf("skillStore.Initialize: %w", err)
}
@@ -182,27 +182,56 @@ func (s *skillStore) ListDueScheduled(ctx context.Context, now time.Time) ([]ski
}
func (s *skillStore) MarkScheduledRun(ctx context.Context, skillID string, ranAt, nextAt time.Time) error {
sk, err := s.Get(ctx, skillID)
if err != nil {
return err
// Single atomic statement instead of Get→mutate→Save: a concurrent Mark or
// admin edit can't lose this update (no read-modify-write window). json_set
// keeps the JSON blob's NextRunAt/LastScheduledRunAt consistent with the
// indexed next_run_at column; RFC3339Nano matches Go's time JSON encoding so
// the blob still round-trips through Get.
var next int64
if !nextAt.IsZero() {
next = nextAt.Unix()
}
sk.LastScheduledRunAt = ranAt
sk.NextRunAt = nextAt
return s.Save(ctx, sk)
res, err := s.db.ExecContext(ctx,
`UPDATE skills SET next_run_at=?, data=json_set(data,'$.NextRunAt',?,'$.LastScheduledRunAt',?) WHERE id=?`,
next, nextAt.Format(time.RFC3339Nano), ranAt.Format(time.RFC3339Nano), skillID)
if err != nil {
return fmt.Errorf("skillStore.MarkScheduledRun: %w", err)
}
if n, _ := res.RowsAffected(); n == 0 {
return skill.ErrNotFound
}
return nil
}
func (s *skillStore) AppendVersion(ctx context.Context, sv skill.SkillVersion) error {
if sv.SkillID == "" {
return fmt.Errorf("skillStore.AppendVersion: skill_id is required")
}
blob, err := json.Marshal(sv)
if err != nil {
return fmt.Errorf("skillStore.AppendVersion: marshal: %w", err)
}
// seq = current max+1 for this skill (newest-first ordering key).
// seq = current max+1 for this skill (newest-first ordering key). The
// MAX-then-INSERT runs in ONE transaction and the (skill_id, seq) index is
// UNIQUE, so two concurrent appends can't both land the same seq: the loser
// fails loudly on commit instead of silently corrupting the ordering. The
// Scan error is propagated (was swallowed, leaving seq=0 on failure).
tx, err := s.db.BeginTx(ctx, nil)
if err != nil {
return fmt.Errorf("skillStore.AppendVersion: begin: %w", err)
}
defer tx.Rollback() //nolint:errcheck // no-op after Commit
var seq int64
_ = s.db.QueryRowContext(ctx, `SELECT COALESCE(MAX(seq),0)+1 FROM skill_versions WHERE skill_id = ?`, sv.SkillID).Scan(&seq)
if _, err := s.db.ExecContext(ctx,
if err := tx.QueryRowContext(ctx, `SELECT COALESCE(MAX(seq),0)+1 FROM skill_versions WHERE skill_id = ?`, sv.SkillID).Scan(&seq); err != nil {
return fmt.Errorf("skillStore.AppendVersion: seq: %w", err)
}
if _, err := tx.ExecContext(ctx,
`INSERT INTO skill_versions (id, skill_id, version, seq, data) VALUES (?, ?, ?, ?, ?)`,
sv.ID, sv.SkillID, sv.Version, seq, string(blob)); err != nil {
return fmt.Errorf("skillStore.AppendVersion: %w", err)
return fmt.Errorf("skillStore.AppendVersion: insert: %w", err)
}
if err := tx.Commit(); err != nil {
return fmt.Errorf("skillStore.AppendVersion: commit: %w", err)
}
return nil
}