fix(store): unique event IDs on lease/expiry, honest duplicate-ingest signal (S5, S6)
S5: Store.Lease and Store.ExpireLeases both set Event.ID to the task id, so
every TaskLeased/TaskReleased event for a given task collided on ID across
every lease of that task — unsound for ApplyAdvisory or any future
ID-based lookup. Both now call domain.NewID().
S6: Append's TaskCreated dedup path returned nil (success) without
appending anything. main.go's handler then did
`s.Events(0)[len(s.Events(0))-1]` and returned that — an unrelated event —
with 201 Created, and every other Append caller (Gitea poll/webhook, JSONL
ingest) had no way to distinguish "duplicate, as expected" from "genuinely
appended".
Add domain.ErrDuplicate, returned instead of nil on a duplicate
(source, external_id). Add Store.TaskBySource to resolve the
already-ingested task by that same dedup key. Update every caller:
- main.go's POST /v1/tasks now returns 200 with the existing task on
ErrDuplicate instead of fabricating a 201 with the wrong event.
- provider.Gitea.Poll/IngestWebhook and provider.JSONL.Ingest treat
ErrDuplicate as expected (already-seen issue/line), not a failure —
without this, Gitea polling would have errored out of its loop on the
first already-ingested issue in every batch, since Poll previously
relied on the old nil-on-dup behavior to keep scanning.
TestLeaseAndExpireEventIDsAreUnique and TestTaskBySourceResolvesDuplicate
cover the store-level fixes; TestAppendReplayAndDeduplicate updated for the
new error signal.
AUDIT.md S5, S6.
This commit is contained in:
+13
-1
@@ -3,6 +3,7 @@ package main
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"io"
|
"io"
|
||||||
"log"
|
"log"
|
||||||
"net"
|
"net"
|
||||||
@@ -168,6 +169,18 @@ func main() {
|
|||||||
b, _ := json.Marshal(p)
|
b, _ := json.Marshal(p)
|
||||||
e := domain.Event{ID: id(), Type: "TaskCreated", TaskID: id(), Version: 1, Payload: b, Surface: string(surface(r))}
|
e := domain.Event{ID: id(), Type: "TaskCreated", TaskID: id(), Version: 1, Payload: b, Surface: string(surface(r))}
|
||||||
if err := s.Append(e); err != nil {
|
if err := s.Append(e); err != nil {
|
||||||
|
if errors.Is(err, domain.ErrDuplicate) {
|
||||||
|
// (source, external_id) was already ingested. Nothing was
|
||||||
|
// appended; report the existing task idempotently rather
|
||||||
|
// than fabricating a 201 with an unrelated event (S6).
|
||||||
|
source, _ := p["source"].(string)
|
||||||
|
externalID, _ := p["external_id"].(string)
|
||||||
|
if t, ok := s.TaskBySource(source, externalID); ok {
|
||||||
|
w.WriteHeader(200)
|
||||||
|
json.NewEncoder(w).Encode(t)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
}
|
||||||
http.Error(w, err.Error(), 400)
|
http.Error(w, err.Error(), 400)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -176,7 +189,6 @@ func main() {
|
|||||||
log.Printf("route task: %v", err)
|
log.Printf("route task: %v", err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
e = s.Events(0)[len(s.Events(0))-1]
|
|
||||||
w.WriteHeader(201)
|
w.WriteHeader(201)
|
||||||
json.NewEncoder(w).Encode(e)
|
json.NewEncoder(w).Encode(e)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -17,6 +17,11 @@ var ErrConflict = errors.New("task version conflict")
|
|||||||
var ErrNotFound = errors.New("task not found")
|
var ErrNotFound = errors.New("task not found")
|
||||||
var ErrInvalid = errors.New("invalid event")
|
var ErrInvalid = errors.New("invalid event")
|
||||||
|
|
||||||
|
// ErrDuplicate is returned by Store.Append for a TaskCreated event whose
|
||||||
|
// (source, external_id) pair was already ingested. The caller already has a
|
||||||
|
// task for this content; nothing was appended.
|
||||||
|
var ErrDuplicate = errors.New("duplicate task ingestion")
|
||||||
|
|
||||||
// CurrentEventSchema is 2: schema 2 requires every event to declare its
|
// CurrentEventSchema is 2: schema 2 requires every event to declare its
|
||||||
// authorizing Surface (see ValidateEvent), enforced at the store append
|
// authorizing Surface (see ValidateEvent), enforced at the store append
|
||||||
// boundary. Schema 1 events already on disk replay unchanged — tolerant
|
// boundary. Schema 1 events already on disk replay unchanged — tolerant
|
||||||
|
|||||||
@@ -137,10 +137,13 @@ func (j JSONL) Ingest(r io.Reader, sink Sink) (int, error) {
|
|||||||
return count, fmt.Errorf("line %d: %w", line, err)
|
return count, fmt.Errorf("line %d: %w", line, err)
|
||||||
}
|
}
|
||||||
b, _ := json.Marshal(p)
|
b, _ := json.Marshal(p)
|
||||||
if err := sink.Append(domain.Event{ID: domain.NewID(), Type: "TaskCreated", TaskID: domain.NewID(), Version: 1, Payload: b, Surface: string(authz.System)}); err != nil {
|
err := sink.Append(domain.Event{ID: domain.NewID(), Type: "TaskCreated", TaskID: domain.NewID(), Version: 1, Payload: b, Surface: string(authz.System)})
|
||||||
|
if err != nil && !errors.Is(err, domain.ErrDuplicate) {
|
||||||
return count, fmt.Errorf("line %d: %w", line, err)
|
return count, fmt.Errorf("line %d: %w", line, err)
|
||||||
}
|
}
|
||||||
count++
|
if err == nil {
|
||||||
|
count++
|
||||||
|
}
|
||||||
}
|
}
|
||||||
return count, sc.Err()
|
return count, sc.Err()
|
||||||
}
|
}
|
||||||
@@ -321,7 +324,10 @@ func (g Gitea) IngestWebhook(body []byte, signature string, sink Sink) error {
|
|||||||
project = h.Repository.FullName
|
project = h.Repository.FullName
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return sink.Append(g.event(h.Issue, g.sourceName(), project))
|
if err := sink.Append(g.event(h.Issue, g.sourceName(), project)); err != nil && !errors.Is(err, domain.ErrDuplicate) {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
return nil
|
||||||
}
|
}
|
||||||
func validSignature(body []byte, got, secret string) bool {
|
func validSignature(body []byte, got, secret string) bool {
|
||||||
if secret == "" || got == "" {
|
if secret == "" || got == "" {
|
||||||
@@ -379,7 +385,7 @@ func (g Gitea) Poll(ctx context.Context, sink Sink) (int, error) {
|
|||||||
project = g.Repo
|
project = g.Repo
|
||||||
}
|
}
|
||||||
for _, i := range issues {
|
for _, i := range issues {
|
||||||
if err := sink.Append(g.event(i, g.sourceName(), project)); err != nil {
|
if err := sink.Append(g.event(i, g.sourceName(), project)); err != nil && !errors.Is(err, domain.ErrDuplicate) {
|
||||||
return 0, err
|
return 0, err
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
+16
-3
@@ -189,7 +189,7 @@ func (s *Store) Append(e domain.Event) error {
|
|||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
if id := s.external[p["source"].(string)+"\x00"+p["external_id"].(string)]; id != "" {
|
if id := s.external[p["source"].(string)+"\x00"+p["external_id"].(string)]; id != "" {
|
||||||
return nil
|
return domain.ErrDuplicate
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
t, taskExists := s.tasks[e.TaskID]
|
t, taskExists := s.tasks[e.TaskID]
|
||||||
@@ -326,6 +326,19 @@ func (s *Store) Task(id string) (domain.Task, bool) {
|
|||||||
return t, ok
|
return t, ok
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TaskBySource resolves the task ingested for a given (source, external_id)
|
||||||
|
// pair — the dedup key Append.ErrDuplicate rejects re-ingestion against.
|
||||||
|
func (s *Store) TaskBySource(source, externalID string) (domain.Task, bool) {
|
||||||
|
s.mu.Lock()
|
||||||
|
defer s.mu.Unlock()
|
||||||
|
id := s.external[source+"\x00"+externalID]
|
||||||
|
if id == "" {
|
||||||
|
return domain.Task{}, false
|
||||||
|
}
|
||||||
|
t, ok := s.tasks[id]
|
||||||
|
return t, ok
|
||||||
|
}
|
||||||
|
|
||||||
func (s *Store) Lease(id, harness string, ttl time.Duration) (domain.Event, error) {
|
func (s *Store) Lease(id, harness string, ttl time.Duration) (domain.Event, error) {
|
||||||
if ttl <= 0 {
|
if ttl <= 0 {
|
||||||
return domain.Event{}, fmt.Errorf("%w: ttl must be positive", domain.ErrInvalid)
|
return domain.Event{}, fmt.Errorf("%w: ttl must be positive", domain.ErrInvalid)
|
||||||
@@ -338,7 +351,7 @@ func (s *Store) Lease(id, harness string, ttl time.Duration) (domain.Event, erro
|
|||||||
return domain.Event{}, domain.ErrConflict
|
return domain.Event{}, domain.ErrConflict
|
||||||
}
|
}
|
||||||
p, _ := json.Marshal(map[string]any{"harness_id": harness, "ttl": ttl.Seconds(), "until_ns": time.Now().Add(ttl).UnixNano(), "expected_version": t.Version})
|
p, _ := json.Marshal(map[string]any{"harness_id": harness, "ttl": ttl.Seconds(), "until_ns": time.Now().Add(ttl).UnixNano(), "expected_version": t.Version})
|
||||||
e := domain.Event{ID: id, Type: "TaskLeased", TaskID: id, Version: t.Version + 1, Payload: p, Surface: string(authz.System)}
|
e := domain.Event{ID: domain.NewID(), Type: "TaskLeased", TaskID: id, Version: t.Version + 1, Payload: p, Surface: string(authz.System)}
|
||||||
return e, s.Append(e)
|
return e, s.Append(e)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -347,7 +360,7 @@ func (s *Store) ExpireLeases(now time.Time) ([]domain.Event, error) {
|
|||||||
for _, t := range s.Tasks() {
|
for _, t := range s.Tasks() {
|
||||||
if t.State == domain.StateLeased && t.Lease != nil && !t.Lease.Until.After(now) {
|
if t.State == domain.StateLeased && t.Lease != nil && !t.Lease.Until.After(now) {
|
||||||
p, _ := json.Marshal(map[string]any{"reason": "lease_expired", "harness_id": t.Lease.HarnessID})
|
p, _ := json.Marshal(map[string]any{"reason": "lease_expired", "harness_id": t.Lease.HarnessID})
|
||||||
e := domain.Event{ID: t.ID, Type: "TaskReleased", TaskID: t.ID, Version: t.Version + 1, Payload: p, Surface: string(authz.System)}
|
e := domain.Event{ID: domain.NewID(), Type: "TaskReleased", TaskID: t.ID, Version: t.Version + 1, Payload: p, Surface: string(authz.System)}
|
||||||
if err := s.Append(e); err != nil {
|
if err := s.Append(e); err != nil {
|
||||||
return out, err
|
return out, err
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,9 +2,11 @@ package store
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"testing"
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
"orchestra/internal/authz"
|
"orchestra/internal/authz"
|
||||||
"orchestra/internal/domain"
|
"orchestra/internal/domain"
|
||||||
@@ -24,8 +26,8 @@ func TestAppendReplayAndDeduplicate(t *testing.T) {
|
|||||||
if err := s.Append(created("e1")); err != nil {
|
if err := s.Append(created("e1")); err != nil {
|
||||||
t.Fatal(err)
|
t.Fatal(err)
|
||||||
}
|
}
|
||||||
if err := s.Append(created("e2")); err != nil {
|
if err := s.Append(created("e2")); !errors.Is(err, domain.ErrDuplicate) {
|
||||||
t.Fatal(err)
|
t.Fatalf("expected ErrDuplicate, got %v", err)
|
||||||
}
|
}
|
||||||
if got := len(s.Events(0)); got != 1 {
|
if got := len(s.Events(0)); got != 1 {
|
||||||
t.Fatalf("duplicate ingest appended %d events", got)
|
t.Fatalf("duplicate ingest appended %d events", got)
|
||||||
@@ -53,6 +55,63 @@ func TestAppendReplayAndDeduplicate(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestLeaseAndExpireEventIDsAreUnique guards S5: Event.ID was set to the
|
||||||
|
// task id in both Lease and ExpireLeases, so every lease of the same task
|
||||||
|
// produced a TaskLeased/TaskReleased event with a colliding ID — unsound
|
||||||
|
// for ApplyAdvisory or any future ID-based lookup.
|
||||||
|
func TestLeaseAndExpireEventIDsAreUnique(t *testing.T) {
|
||||||
|
s, err := Open(t.TempDir())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if err := s.Append(created("e1")); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
task := s.Tasks()[0]
|
||||||
|
leaseEvt, err := s.Lease(task.ID, "h1", time.Millisecond)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if leaseEvt.ID == task.ID || leaseEvt.ID == "" {
|
||||||
|
t.Fatalf("lease event ID %q collides with task ID %q", leaseEvt.ID, task.ID)
|
||||||
|
}
|
||||||
|
time.Sleep(2 * time.Millisecond)
|
||||||
|
expired, err := s.ExpireLeases(time.Now())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if len(expired) != 1 {
|
||||||
|
t.Fatalf("expected 1 expiry, got %d", len(expired))
|
||||||
|
}
|
||||||
|
if expired[0].ID == task.ID || expired[0].ID == leaseEvt.ID || expired[0].ID == "" {
|
||||||
|
t.Fatalf("expiry event ID %q collides", expired[0].ID)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestTaskBySourceResolvesDuplicate covers the S6 fix: a caller that gets
|
||||||
|
// ErrDuplicate from Append must be able to look up the already-ingested
|
||||||
|
// task by its dedup key instead of guessing at "the last event in the log".
|
||||||
|
func TestTaskBySourceResolvesDuplicate(t *testing.T) {
|
||||||
|
s, err := Open(t.TempDir())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if err := s.Append(created("e1")); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
want := s.Tasks()[0]
|
||||||
|
if err := s.Append(created("e2")); !errors.Is(err, domain.ErrDuplicate) {
|
||||||
|
t.Fatalf("expected ErrDuplicate, got %v", err)
|
||||||
|
}
|
||||||
|
got, ok := s.TaskBySource("jsonl", "42")
|
||||||
|
if !ok || got.ID != want.ID {
|
||||||
|
t.Fatalf("TaskBySource = %+v, ok=%v, want %+v", got, ok, want)
|
||||||
|
}
|
||||||
|
if _, ok := s.TaskBySource("jsonl", "does-not-exist"); ok {
|
||||||
|
t.Fatal("expected not found")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestArtifactIsContentAddressed(t *testing.T) {
|
func TestArtifactIsContentAddressed(t *testing.T) {
|
||||||
s, err := Open(t.TempDir())
|
s, err := Open(t.TempDir())
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
Reference in New Issue
Block a user