diff --git a/backend/internal/observe/scm/observer.go b/backend/internal/observe/scm/observer.go index bfb56c5702..4419c40ebe 100644 --- a/backend/internal/observe/scm/observer.go +++ b/backend/internal/observe/scm/observer.go @@ -13,6 +13,7 @@ import ( "fmt" "log/slog" "path/filepath" + "sort" "strings" "sync" "time" @@ -330,10 +331,11 @@ func (o *Observer) Poll(ctx context.Context) error { return err } - for key, obs := range observations { + for _, key := range dispatchOrder(observations, selection.subjectsByPR) { if err := ctx.Err(); err != nil { return err } + obs := observations[key] subj, ok := selection.subjectsByPR[key] if !ok { continue @@ -413,6 +415,30 @@ func (o *Observer) Poll(ctx context.Context) error { return nil } +// dispatchOrder returns observation keys in a deterministic order so lifecycle +// notifications for a session are stable across polls. +func dispatchOrder(observations map[string]ports.SCMObservation, subjectsByPR map[string]*subject) []string { + keys := make([]string, 0, len(observations)) + for key := range observations { + keys = append(keys, key) + } + sessionOf := func(key string) string { + if s := subjectsByPR[key]; s != nil { + return string(s.session.ID) + } + return "" + } + sort.Slice(keys, func(i, j int) bool { + if si, sj := sessionOf(keys[i]), sessionOf(keys[j]); si != sj { + return si < sj + } + if ni, nj := observations[keys[i]].PR.Number, observations[keys[j]].PR.Number; ni != nj { + return ni < nj + } + return keys[i] < keys[j] + }) + return keys +} func (o *Observer) checkCredentials(ctx context.Context) (bool, error) { var probe observe.CredentialProbe if checker, ok := o.provider.(credentialChecker); ok { @@ -573,11 +599,7 @@ func (o *Observer) workspaceSCMSessionRepos(ctx context.Context, proj domain.Pro func repoForTrackedPR(pr domain.PullRequest, repos []ports.SCMRepo) (ports.SCMRepo, bool) { if pr.Provider != "" && pr.Host != "" && pr.Repo != "" { - owner, name, ok := strings.Cut(pr.Repo, "/") - if !ok || owner == "" || name == "" { - return ports.SCMRepo{}, false - } - return ports.SCMRepo{Provider: pr.Provider, Host: pr.Host, Owner: owner, Name: name, Repo: pr.Repo}, true + return ports.SCMRepo{Provider: pr.Provider, Host: pr.Host, Repo: pr.Repo}, true } if pr.Repo != "" { for _, repo := range repos { diff --git a/backend/internal/observe/scm/observer_test.go b/backend/internal/observe/scm/observer_test.go index 7766d4e352..d0c3e89fe7 100644 --- a/backend/internal/observe/scm/observer_test.go +++ b/backend/internal/observe/scm/observer_test.go @@ -12,6 +12,7 @@ import ( "io" "log/slog" "os/exec" + "path/filepath" "strings" "sync" "testing" @@ -228,6 +229,30 @@ func newTestObserver(store *fakeStore, provider *fakeProvider, lc Lifecycle, now return New(provider, store, lc, Config{Clock: func() time.Time { return now }, Tick: time.Hour, Logger: quietSlog(), CacheMax: 128}) } +func TestDispatchOrderIsDeterministic(t *testing.T) { + obs := map[string]ports.SCMObservation{ + "a#9": {PR: ports.SCMPRObservation{Number: 9}}, + "a#7": {PR: ports.SCMPRObservation{Number: 7}}, + "b#3": {PR: ports.SCMPRObservation{Number: 3}}, + } + subjects := map[string]*subject{ + "a#9": {session: domain.SessionRecord{ID: "sess-a"}}, + "a#7": {session: domain.SessionRecord{ID: "sess-a"}}, + "b#3": {session: domain.SessionRecord{ID: "sess-b"}}, + } + want := []string{"a#7", "a#9", "b#3"} + for run := 0; run < 8; run++ { + got := dispatchOrder(obs, subjects) + if len(got) != len(want) { + t.Fatalf("run %d: got %d keys, want %d (%v)", run, len(got), len(want), got) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("run %d: order[%d]=%q, want %q (full %v)", run, i, got[i], want[i], got) + } + } + } +} func quietSlog() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } func testStoreWithSession() *fakeStore { @@ -281,9 +306,6 @@ func TestRepoForTrackedPRUsesPersistedRepoWhenCurrentScanDropsUpstream(t *testin if repo.Provider != "github" || repo.Host != "github.com" || repo.Repo != "upstream/api" { t.Fatalf("repo = %#v, want persisted upstream/api tuple", repo) } - if repo.Owner != "upstream" || repo.Name != "api" { - t.Fatalf("repo owner/name = %q/%q, want upstream/api", repo.Owner, repo.Name) - } } func TestStartAsyncPerformsImmediatePollAndStopsOnCancel(t *testing.T) { @@ -631,7 +653,7 @@ func TestPoll_DiscoversWorkspaceChildRepoPR(t *testing.T) { func TestPoll_DiscoversWorkspaceChildRepoUpstreamPR(t *testing.T) { oldRemoteURLs := gitRemoteURLsFunc gitRemoteURLsFunc = func(path string) []string { - if strings.HasSuffix(path, "/api") { + if strings.HasSuffix(filepath.ToSlash(path), "/api") { return []string{"https://github.com/o/api.git", "https://github.com/upstream/api.git"} } return nil