From a5d0d1a070363f26d6fb1944a8ea5c5b7d8cdd91 Mon Sep 17 00:00:00 2001 From: Samuel Enocsson Date: Tue, 13 Oct 2026 11:55:05 +0200 Subject: [PATCH] fix(master): review fixes for newer-tag suggestions (#11) --- CLAUDE.md | 4 +- internal/master/poller/poller.go | 67 +++++++++++++++++---------- internal/master/poller/poller_test.go | 32 +++++++++++++ internal/master/registry/registry.go | 2 +- internal/master/store/merge.go | 7 +-- internal/master/store/services.go | 2 +- internal/master/web/server.go | 2 +- internal/master/web/static/app.css | 3 +- internal/master/web/versions_test.go | 15 ++++++ internal/master/web/views.go | 14 ++++-- internal/master/web/views_test.go | 1 + 11 files changed, 111 insertions(+), 38 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1ec3ad3..56c85c1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -83,7 +83,9 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/ - **`services.newer_ref` is a suggestion only**: no auto jobs, not even for auto services. The poller writes it only on change; a failed tag list keeps the old value and never sets `last_poll_error`. The UI re-checks it at render time with `tagver.Newer`, so a stale value - is hidden until the next cycle. Tags with one numeric component or no digits (`7`, `pg16`, + is hidden until the next cycle; it is also hidden when its repo is not the one the service + follows. The suggestion is computed from the service list read at the start of the cycle, so + if the service moved on meanwhile, the render-time check hides it. Tags with one numeric component or no digits (`7`, `pg16`, `latest`) never get suggestions. - **`image_versions` uses `''` instead of NULL** for a missing tag or digest, so `UNIQUE (repo, tag, digest)` deduplicates. The webhook records `(repo, tag, '')`; that diff --git a/internal/master/poller/poller.go b/internal/master/poller/poller.go index ea84db2..1462c9d 100644 --- a/internal/master/poller/poller.go +++ b/internal/master/poller/poller.go @@ -164,16 +164,26 @@ func (p *Poller) Cycle(ctx context.Context) { } p.record(write, l, byRef[l.ref], &stats) } + // Publish after the digest phase too: listing tags can take a while and + // an "Update available" should not wait for it. + p.publishIfWrote(&stats) p.syncNewer(ctx, write, svcs, &stats) - if stats.wrote { - p.pub.Publish(events.Services) - } + p.publishIfWrote(&stats) p.log.Info("poll_cycle", "refs", stats.refs, "ok", stats.ok, "failed", stats.failed, "suggested", stats.suggested, "jobs", stats.jobs, "skipped", stats.skipped, "repos_listed", stats.reposListed, "tags_failed", stats.tagsFailed, "newer", stats.newer, "duration_ms", p.now().Sub(began).Milliseconds()) } +// publishIfWrote tells the browsers to refresh if a phase of the cycle wrote +// anything, and starts the next phase with a clean slate. +func (p *Poller) publishIfWrote(stats *cycleStats) { + if stats.wrote { + p.pub.Publish(events.Services) + stats.wrote = false + } +} + // forEach calls fn(i) for i in [0, n), Config.Workers at a time, and stops // starting new calls once ctx is done. It returns when all started calls // have returned. @@ -246,27 +256,35 @@ func (p *Poller) listAll(ctx context.Context, repos []string) []tagList { return out } -// followedTag returns the repo and tag svc follows. ok is false unless svc is -// polled and the tag can have a newer tag. -func followedTag(svc model.Service) (repo, tag string, ok bool) { +// Followed returns the repo and tag svc follows: a bare image follows +// "latest", and a digest-only reference follows no tag. +func Followed(svc model.Service) (repo, tag string) { + ref := imageref.Parse(jobs.FollowedRef(svc.ActualRef)) + return ref.Repo, ref.Tag +} + +// followedComparable returns the repo and tag svc follows. ok is false unless +// svc is polled and the tag can have a newer tag. +func followedComparable(svc model.Service) (repo, tag string, ok bool) { if SkipReason(svc) != "" { return "", "", false } - ref := imageref.Parse(jobs.FollowedRef(svc.ActualRef)) - return ref.Repo, ref.Tag, tagver.Comparable(ref.Tag) + repo, tag = Followed(svc) + return repo, tag, tagver.Comparable(tag) } // syncNewer lists the tags of every repo that has a service following a // version-like tag, once per repo, and stores the newer tag of each service. // A service whose tag cannot have a newer tag any more gets its suggestion -// cleared; one whose repo could not be listed keeps it. Nothing is written +// cleared; one whose repo could not be listed keeps it, unless it names +// another repo than the service follows now. Nothing is written // when the value is unchanged. It never starts a job, not even for an auto // service: a newer tag may be a major step the user has not chosen. func (p *Poller) syncNewer(ctx, write context.Context, svcs []model.Service, stats *cycleStats) { seen := map[string]bool{} var repos []string for _, svc := range svcs { - if repo, _, ok := followedTag(svc); ok && svc.RemovedAt == nil && !seen[repo] { + if repo, _, ok := followedComparable(svc); ok && svc.RemovedAt == nil && !seen[repo] { seen[repo] = true repos = append(repos, repo) } @@ -274,11 +292,10 @@ func (p *Poller) syncNewer(ctx, write context.Context, svcs []model.Service, sta lists := map[string][]string{} for _, l := range p.listAll(ctx, repos) { if l.err != nil { - if ctx.Err() != nil { - continue // cancelled mid-listing: not the registry's fault - } - stats.tagsFailed++ p.log.Warn("poll_tags_failed", "repo", l.repo, "error", truncate(l.err.Error())) + if ctx.Err() == nil { // cancelled mid-listing: not the registry's fault + stats.tagsFailed++ + } continue } stats.reposListed++ @@ -292,22 +309,24 @@ func (p *Poller) syncNewer(ctx, write context.Context, svcs []model.Service, sta continue } want := "" - if repo, tag, ok := followedTag(svc); ok { + if repo, tag, ok := followedComparable(svc); ok { tags, listed := lists[repo] - if !listed { + if !listed && imageref.Parse(svc.NewerRef).Repo == repo { if svc.NewerRef != "" { stats.newer++ } continue // not listed this cycle: keep what is stored } - key := repoTag{repo, tag} - n, done := newest[key] - if !done { - n, _ = tagver.Newer(tag, tags) - newest[key] = n - } - if n != "" { - want = repo + ":" + n + if listed { + key := repoTag{repo, tag} + n, done := newest[key] + if !done { + n, _ = tagver.Newer(tag, tags) + newest[key] = n + } + if n != "" { + want = repo + ":" + n + } } } if want != "" { diff --git a/internal/master/poller/poller_test.go b/internal/master/poller/poller_test.go index 831a172..3907a61 100644 --- a/internal/master/poller/poller_test.go +++ b/internal/master/poller/poller_test.go @@ -888,3 +888,35 @@ func TestCancelledListKeepsNewer(t *testing.T) { assert.Empty(t, st.newer) assert.Empty(t, pub.topics) } + +func TestFailedListClearsNewerFromAnotherRepo(t *testing.T) { + s := newerSvc(1, repoB, "1.27.3") + s.NewerRef = repoA + ":1.27.4" + st := &fakeStore{services: []model.Service{s}} + reg := newerRegistry(nil, repoB+":1.27.3") + reg.tagErrs = map[string]error{repoB: errors.New("list x: registry answered 500 Internal Server Error")} + p, _ := newPoller(st, &fakeJobs{}, reg, Config{}) + + p.Cycle(context.Background()) + + assert.Equal(t, []newerCall{{1, "", time.Date(2026, 10, 13, 12, 0, 0, 0, time.UTC)}}, st.newer) +} + +func TestDigestWritesArePublishedBeforeTagsAreListed(t *testing.T) { + st := &fakeStore{services: []model.Service{svc(1, repoB+":1.27.3")}} + reg := newerRegistry(map[string][]string{repoB: {"1.27.3"}}, repoB+":1.27.3") + reg.tagRelease = make(chan struct{}) + reg.tagStarted = make(chan struct{}, 1) + p, pub := newPoller(st, &fakeJobs{}, reg, Config{}) + done := make(chan struct{}) + go func() { p.Cycle(context.Background()); close(done) }() + <-reg.tagStarted + + pub.mu.Lock() + published := append([]events.Topic(nil), pub.topics...) + pub.mu.Unlock() + assert.Equal(t, []events.Topic{events.Services}, published, "published while the listing is still blocked") + + close(reg.tagRelease) + <-done +} diff --git a/internal/master/registry/registry.go b/internal/master/registry/registry.go index eb50c06..e8d2d55 100644 --- a/internal/master/registry/registry.go +++ b/internal/master/registry/registry.go @@ -103,7 +103,7 @@ func (c *Client) lookupError(verb, target, ref string, err error) error { if errors.As(err, &terr) { return fmt.Errorf("%s %s: %s", verb, target, describe(terr)) } - c.log.Warn("registry_lookup_failed", "ref", ref, "error", err) + c.log.Warn("registry_request_failed", "ref", ref, "error", err) return fmt.Errorf("%s %s: %s", verb, target, classify(err)) } diff --git a/internal/master/store/merge.go b/internal/master/store/merge.go index fb7a60e..6a23570 100644 --- a/internal/master/store/merge.go +++ b/internal/master/store/merge.go @@ -107,8 +107,8 @@ type mergeRow struct { // MergeService moves the live service toID onto the removed service fromID: // the old row keeps its id, first_seen_at, update mode and desired ref, takes // the location and inventory of the live row (and its available and newer-tag -// suggestions), becomes live, and the live row is deleted. The jobs of the live row move to -// the old one. This is the only place a services row is deleted. +// suggestions), becomes live, and the live row is deleted. The jobs of the live +// row move to the old one. This is the only place a services row is deleted. // // It returns *model.MergeRefusedError when the pair cannot be merged, // model.ErrJobActive when either row has a queued or running job, @@ -213,8 +213,9 @@ func (s *Store) MergeService(ctx context.Context, fromID, toID int64) error { config_files = $7, built = $8, swarm_cluster_id = $9, swarm_service = $10, container_name = $11, image_repo = $12, actual_ref = $13, actual_digest = $14, status = $15, self = $16, updated_at = $17, available_ref = $18, available_digest = $19, available_source = $20, available_at = $21, - last_poll_error = $22, newer_ref = $24, newer_at = $25, + last_poll_error = $22, last_tag_digest = CASE WHEN last_tag_digest <> '' THEN last_tag_digest ELSE $23 END, + newer_ref = $24, newer_at = $25, removed_at = NULL, awaiting_inventory = false WHERE id = $1`, fromID, t.AgentID, t.LocationKey, t.ComposeProject, t.ComposeService, t.WorkingDir, diff --git a/internal/master/store/services.go b/internal/master/store/services.go index f0c087f..7d2a378 100644 --- a/internal/master/store/services.go +++ b/internal/master/store/services.go @@ -340,7 +340,7 @@ func (s *Store) GetImageVersion(ctx context.Context, id int64) (model.ImageVersi return v, nil } -// GetTagVersion returns the digest-less version row with an empty digest, which +// GetTagVersion returns the version row of repo and tag with an empty digest, which // SetNewer and the webhook create. Returns model.ErrNotFound if there is none. func (s *Store) GetTagVersion(ctx context.Context, repo, tag string) (model.ImageVersion, error) { var v model.ImageVersion diff --git a/internal/master/web/server.go b/internal/master/web/server.go index cfc2605..fa17540 100644 --- a/internal/master/web/server.go +++ b/internal/master/web/server.go @@ -393,7 +393,7 @@ func (s *Server) serviceDetail(r *http.Request, id int64) (pageData, error) { return data, err } data.NewerTag = newerTag(svc) - if data.NewerTag != "" && !slices.ContainsFunc(versions, func(v model.ImageVersion) bool { return v.Tag == data.NewerTag }) { + if data.NewerTag != "" && !slices.ContainsFunc(versions, func(v model.ImageVersion) bool { return v.Tag == data.NewerTag && v.Digest == "" }) { // The suggested tag can be older than the listed versions; it must // still be pickable. v, err := s.store.GetTagVersion(r.Context(), svc.ImageRepo, data.NewerTag) diff --git a/internal/master/web/static/app.css b/internal/master/web/static/app.css index 70b2c17..fcccf00 100644 --- a/internal/master/web/static/app.css +++ b/internal/master/web/static/app.css @@ -106,10 +106,9 @@ summary.btn::-webkit-details-marker{display:none} .tag em{font:500 11.5px var(--mono);font-style:normal;margin-left:6px} .tag em:empty{display:none} .f-up{color:var(--orange);background:var(--orangebg)} -.f-newer{color:var(--mut);box-shadow:inset 0 0 0 1px var(--line2)} .f-newer em{color:var(--orange)} .f-err{color:var(--red);background:var(--redbg)} -.f-notag{color:var(--mut);box-shadow:inset 0 0 0 1px var(--line2)} +.f-newer,.f-notag{color:var(--mut);box-shadow:inset 0 0 0 1px var(--line2)} .f-out{color:var(--dim);box-shadow:inset 0 0 0 1px var(--line)} .mode{display:flex;flex-direction:column;align-items:flex-start;gap:2px} .mode form{margin:0;display:flex;flex-direction:column;align-items:flex-start;gap:2px} diff --git a/internal/master/web/versions_test.go b/internal/master/web/versions_test.go index 167d168..525f24e 100644 --- a/internal/master/web/versions_test.go +++ b/internal/master/web/versions_test.go @@ -247,6 +247,21 @@ func TestServicePage_NewerTagAlreadyListedIsNotAppendedTwice(t *testing.T) { assert.Equal(t, 1, strings.Count(body, `name="version"`)) } +func TestServicePage_PinnedRowOfTheNewerTagDoesNotHideThePlainRow(t *testing.T) { + srv, store, _, _, _ := newTestEnv(t) + store.services[0].ImageRepo = sluskRepo + store.services[0].NewerRef = sluskRepo + ":1.5.0" + store.versions = []model.ImageVersion{ + {ID: 31, Repo: sluskRepo, Tag: "1.5.0", Digest: "sha256:d", Source: "poll"}, + {ID: 32, Repo: sluskRepo, Tag: "1.5.0", Source: "tags"}, + } + + _, body := get(t, srv, "/services/1") + + assert.Equal(t, 2, strings.Count(body, `name="version"`)) + assert.Contains(t, body, ``) +} + func TestServicePage_NoNewerTagRowWithoutSuggestion(t *testing.T) { srv, _, _, _, _ := newTestEnv(t) diff --git a/internal/master/web/views.go b/internal/master/web/views.go index 113803f..2639f3d 100644 --- a/internal/master/web/views.go +++ b/internal/master/web/views.go @@ -249,17 +249,21 @@ func followsNoTag(svc model.Service) bool { // newerTag returns the tag of the service's newer-tag suggestion, or "" when // there is none or it no longer holds. The poller writes the suggestion once // per cycle, so after the service moved to another tag the stored value can -// be stale until the next cycle; checking again here hides it meanwhile. +// be stale until the next cycle; checking again here hides it meanwhile. A +// suggestion from another repo than the service follows now is hidden too. func newerTag(s model.Service) string { if s.RemovedAt != nil || s.NewerRef == "" { return "" } - tag := imageref.Parse(s.NewerRef).Tag - followed := imageref.Parse(jobs.FollowedRef(s.ActualRef)).Tag - if _, ok := tagver.Newer(followed, []string{tag}); !ok { + newer := imageref.Parse(s.NewerRef) + repo, followed := poller.Followed(s) + if newer.Repo != repo { return "" } - return tag + if _, ok := tagver.Newer(followed, []string{newer.Tag}); !ok { + return "" + } + return newer.Tag } // versionRow is an image version as shown on the service page. diff --git a/internal/master/web/views_test.go b/internal/master/web/views_test.go index e900f99..ce515b2 100644 --- a/internal/master/web/views_test.go +++ b/internal/master/web/views_test.go @@ -223,6 +223,7 @@ func TestNewerTag(t *testing.T) { {"bare image follows latest", model.Service{ActualRef: "gitea/gitea", NewerRef: "gitea/gitea:1.28.0"}, ""}, {"stale after the tag changed", model.Service{ActualRef: "gitea/gitea:1.28.0", NewerRef: "gitea/gitea:1.28.0"}, ""}, {"stale after the tag moved past it", model.Service{ActualRef: "gitea/gitea:1.29.0", NewerRef: "gitea/gitea:1.28.0"}, ""}, + {"other repo", model.Service{ActualRef: "redis:7.2.4", NewerRef: "bitnami/redis:7.2.5"}, ""}, {"removed", model.Service{ActualRef: "gitea/gitea:1.27.3", NewerRef: "gitea/gitea:1.28.0", RemovedAt: &removed}, ""}, {"empty", model.Service{ActualRef: "gitea/gitea:1.27.3"}, ""}, }