fix(master): review fixes for drift restore and accept (#10)

CLAUDE.md

@@ -143,7 +143,6 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/
   newer suggestion, and a digest-pinned pick does not clear a digest-less (webhook) one.
 - **Job output is capped at `protocol.MaxJobOutput` (128 KiB, tail).** NUL bytes are
   stripped by the hub because Postgres text rejects them.
-
 - **Drift compares by digest, not by string** (`model.RunsRef`): `repo:tag@D` is run by
   `repo:tag` reporting digest D. Same digest under another tag is not drift. Different
   repo strings (`nginx` vs `docker.io/library/nginx`) are.
@@ -151,9 +150,15 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/
   inventory write for the row; `Service.Drifted()` is false meanwhile, since `desired_ref`
   is ahead of `actual_ref` for a debounce interval after every job.
 - **Accept never touches `last_tag_digest`, `available_*` or `update_mode`**, and the
-  `actual` form field is only a guard against a changed service; the value written comes
+  `actual` form field (`Service.RunningRef()`, ref plus digest) is only a guard against a
-  from the row. `RestoreDesired` sets `last_tag_digest` to the drifted digest when it is
+  changed service; the value written comes from the row. `RestoreDesired` sets
-  another digest of the desired tag.
+  `last_tag_digest` to the drifted digest when it is another digest of the desired tag and
+  `last_tag_digest` is empty or equals the desired digest (otherwise it holds back a newer
+  digest and stays).
+- **`awaiting_inventory` can hide real drift** until the next inventory change when a job
+  result arrives after the snapshot that shows the drift (an agent reconnect resends its
+  outbox after the snapshot), or when Swarm coalesces an update and a rollback into one
+  debounce interval. This is accepted.
 - **Merge lock order is agent, then service rows by id** (as in inventory writes), and the
   duplicate row is deleted before the old row is updated, because `location_key` is unique
   and not deferrable. A removal racing a merge can be lost; the next snapshot fixes it.

README.md

@@ -213,7 +213,8 @@ service, in auto mode too; a person can still start one by hand. There are two w
 - **Restore desired** deploys the recorded version again (a manual job to the exact
   `repo:tag@digest`; an agent speaking protocol 3 is needed). If the service had drifted
   to another digest of the same tag, that digest is remembered, so polling does not
-  deploy it again until the registry shows a newer one.
+  deploy it again until the registry shows a newer one (unless `last_tag_digest` already
+  holds back a newer digest, which then stays).
 - **Accept running** records what runs now as the desired version. It starts no job and
   leaves the available suggestion and the update mode alone.
 
@@ -224,8 +225,10 @@ moved. **Merge** moves the old row, with its history, mode and desired version,
 location and deletes the duplicate. Merging is never automatic. It can also be done by
 hand for any removed service of the same kind (Compose or Swarm, names may differ), but
 not for plain containers, the updater itself, or a service with a queued or running job.
+A merge keeps the old row's mode and desired version, so a manually merged target with its
+own history may show drift right after the merge and can be accepted.
 
-The restore, accept and merge buttons arrive with the UI follow-up; the endpoints
+The restore, accept and merge buttons arrive with the UI follow-up (issue #28); the endpoints
 (`POST /services/{id}/restore`, `/accept`, `/merge`) and the data for them are in place.
 
 ## Adding agents

docs/superpowers/plans/2026-10-13-master-agent-phase-5-drift-merge.md

@@ -8,7 +8,7 @@
 - Model: `Service.Drifted()` plus `RunsRef` and `RunningRef` decide drift and what accept writes. `Move` carries a suggestion.
 - Store: migration 0005 adds `services.awaiting_inventory`; `FinishJob` sets it, every inventory upsert clears it. New `AcceptRunning`, `MoveCandidates`, `MergeSources`, `MergeService` (the only code that deletes a `services` row).
 - Jobs: `Start` refuses poll and webhook jobs for a drifted service. `Restorable` and `RestoreDesired` deploy the recorded ref again as a manual job.
-- Web: three POST routes (`/services/{id}/restore`, `/accept`, `/merge`) and new `pageData` fields. No template or CSS changes: a parallel session rewrites the UI (issue #18), which adds the buttons.
+- Web: three POST routes (`/services/{id}/restore`, `/accept`, `/merge`) and new `pageData` fields. No template or CSS changes: issue #18 (the Ledger rewrite of the Services and Agents pages) is a separate UI change; the restore, accept and merge buttons arrive with the UI follow-up, issue #28.
 
 **Tech Stack:** no new dependencies.
 
@@ -30,7 +30,7 @@
 3. **Poll and webhook jobs are refused for a drifted service** (`ReasonDrifted`). Manual jobs are allowed, so a person can still update or roll back.
 4. **Move suggestion is stricter than "exactly one appears".** Besides one removed and one live row on each side, the live row must be newer, have no desired ref and no jobs, and the removed row must have something worth moving (desired ref, auto mode or jobs). Otherwise every restarted stack would produce a suggestion.
 5. **Manual merge is allowed for Compose and Swarm of the same kind**, names may differ, same agent allowed. Plain containers and the updater itself are refused.
-6. **Accept does not clear `available_*`**, and the form's `actual` field is only a guard; the value written comes from the row.
+6. **Accept does not clear `available_*`**, and the form's `actual` field (`Service.RunningRef()`) is only a guard; the value written comes from the row.
 7. **The overview does not show moves or drift yet.** `MoveCandidates` is only read on the service page. `Service.Drifted()` is a model method, so the UI follow-up needs no further wiring.
 8. **`CreateJob` maps the foreign key violation on `update_jobs_service_id_fkey` to `ErrNotFound`**, so a job for a service that a merge just deleted fails cleanly.
 
@@ -68,10 +68,10 @@
 - `CreateJob`: foreign key violation `update_jobs_service_id_fkey` becomes `ErrNotFound`.
 
 ### C4. Jobs
-`Start` returns `NotUpdatableError{ReasonDrifted}` for a non-manual trigger on a drifted service. `Restorable(svc)`: drifted, then `updatable(svc, true)`, and the desired ref's repo is not the agent image repo. `RestoreDesired(ctx, id, by)`: `Start` with `Ref: svc.DesiredRef`, manual. After a created job, `SetLastTagDigest` to the drifted digest when the actual tag equals the desired tag, the digest is well formed, differs from the desired digest and from `last_tag_digest`.
+`Start` returns `NotUpdatableError{ReasonDrifted}` for a non-manual trigger on a drifted service. `Restorable(svc)`: drifted, then `updatable(svc, true)`, and the desired ref's repo is not the agent image repo. `RestoreDesired(ctx, id, by)`: `Start` with `Ref: svc.DesiredRef`, manual. After a created job, `SetLastTagDigest` to the drifted digest when the actual tag equals the desired tag, the digest is well formed, differs from the desired digest and from `last_tag_digest`, and `last_tag_digest` is empty or equals the desired digest (otherwise it holds back a newer digest and stays).
 
 ### C5. Web
-Routes `POST /services/{id}/restore`, `/accept` (form `actual` required; optional `return=overview`), `/merge` (form `from` = removed row, `{id}` = live row; redirects to the surviving id). `pageData` gains `RestoreBlocked`, `MoveFrom`, `MoveTo`, `MergeSources`. Log events `drift_accepted` and `service_merged`.
+Routes `POST /services/{id}/restore`, `/accept` (form `actual` required, the `Service.RunningRef()` the user saw, compared with the row's; optional `return=overview`), `/merge` (form `from` = removed row, `{id}` = live row; redirects to the surviving id). `pageData` gains `RestoreBlocked`, `MoveFrom`, `MoveTo`, `MergeSources`. Log events `drift_accepted` and `service_merged`.
 
 ### C6. Concurrency
 An upsert for the live row's location waits on its lock, then updates the survivor. A snapshot after the merge lists the survivor's key, so it is not marked removed. A removal racing a merge can be lost; the next snapshot fixes it. `CreateJob` on the deleted row gets `ErrNotFound`; the poller and webhook re-read and log it.

internal/e2e/agent_master_test.go

@@ -11,7 +11,6 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
-	"regexp"
 	"sort"
 	"strings"
 	"sync"
@@ -25,6 +24,7 @@ import (
 	"github.com/shcizo/package-updater/internal/agent/conn"
 	"github.com/shcizo/package-updater/internal/agent/updater"
 	"github.com/shcizo/package-updater/internal/agenttoken"
+	"github.com/shcizo/package-updater/internal/imageref"
 	"github.com/shcizo/package-updater/internal/master/events"
 	"github.com/shcizo/package-updater/internal/master/hub"
 	"github.com/shcizo/package-updater/internal/master/model"
@@ -32,9 +32,6 @@ import (
 	"github.com/shcizo/package-updater/internal/protocol"
 )
 
-// digestPattern is the digest format the real store records.
-var digestPattern = regexp.MustCompile(`^sha256:[0-9a-f]{64}$`)
-
 type memStore struct {
 	mu        sync.Mutex
 	agents    map[string]model.Agent
@@ -246,7 +243,7 @@ func (m *memStore) FinishJob(_ context.Context, agentID, jobID int64, f model.Jo
 		svc := m.svcRows[j.ServiceID]
 		svc.DesiredRef = f.Ref
 		svc.AwaitingInventory = true
-		if _, d, _ := strings.Cut(f.Ref, "@"); !strings.Contains(requestedRef, "@") && digestPattern.MatchString(d) {
+		if _, d, _ := strings.Cut(f.Ref, "@"); !strings.Contains(requestedRef, "@") && imageref.IsDigest(d) {
 			svc.LastTagDigest = d
 		}
 		// Same rule as the SQL: the job was created for the suggested ref after it was reported.

internal/master/jobs/drift_test.go

@@ -50,7 +50,7 @@ func TestRestorable(t *testing.T) {
 		{"not drifted", func(s *model.Service) { s.ActualDigest = driftD1 }, model.ReasonNotDrifted},
 		{"never updated", func(s *model.Service) { s.DesiredRef = "" }, model.ReasonNotDrifted},
 		{"awaiting inventory", func(s *model.Service) { s.AwaitingInventory = true }, model.ReasonNotDrifted},
-		{"removed", func(s *model.Service) { s.RemovedAt = ptr(time.Now()) }, model.ReasonNotDrifted},
+		{"removed", func(s *model.Service) { s.RemovedAt = ptr(time.Now()) }, model.ReasonRemoved},
 		{"flagged self", func(s *model.Service) { s.Self = true }, model.ReasonSelf},
 		{"built", func(s *model.Service) { s.Built = true }, model.ReasonBuilt},
 		{"container", func(s *model.Service) { s.Kind = protocol.KindContainer }, model.ReasonContainer},
@@ -166,6 +166,11 @@ func TestRestoreDesired_LastTagDigest(t *testing.T) {
 		{"digest in the actual ref", func(s *model.Service) {
 			s.ActualRef, s.ActualDigest = driftRepo+":1.4.2@"+driftD3, ""
 		}, driftD3},
+		{"last tag digest holds back a newer one", func(s *model.Service) {
+			s.DesiredRef = driftRepo + ":1.4.2@" + driftD1
+			s.LastTagDigest = driftD3
+		}, ""},
+		{"last tag digest equals the desired digest", func(s *model.Service) { s.LastTagDigest = driftD1 }, driftD2},
 		{"already remembered", func(s *model.Service) { s.LastTagDigest = driftD2 }, ""},
 		{"different tag", func(s *model.Service) { s.ActualRef = driftRepo + ":1.5.0" }, ""},
 		{"actual digest malformed", func(s *model.Service) { s.ActualDigest = "sha256:abc" }, ""},

internal/master/jobs/jobs.go

@@ -120,6 +120,9 @@ func (s *Service) updatable(svc model.Service, explicitRef bool) error {
 // desired version is deployed like an explicit ref, so a service that follows
 // no tag is fine. The service page calls it for every render.
 func (s *Service) Restorable(svc model.Service) error {
+	if svc.RemovedAt != nil {
+		return &model.NotUpdatableError{Reason: model.ReasonRemoved}
+	}
 	if !svc.Drifted() {
 		return &model.NotUpdatableError{Reason: model.ReasonNotDrifted}
 	}
@@ -140,7 +143,9 @@ func (s *Service) Restorable(svc model.Service) error {
 //
 // When the service had drifted to another digest of the same tag, that digest
 // becomes last_tag_digest, so the poller does not deploy it again before the
-// registry shows a newer one (the same rule as for any rollback).
+// registry shows a newer one (the same rule as for any rollback). It is not
+// written when last_tag_digest holds back a newer digest, that is when it is
+// set and differs from the digest the desired ref names.
 func (s *Service) RestoreDesired(ctx context.Context, serviceID int64, requestedBy string) (model.Job, error) {
 	svc, err := s.store.GetService(ctx, serviceID)
 	if err != nil {
@@ -153,7 +158,7 @@ func (s *Service) RestoreDesired(ctx context.Context, serviceID int64, requested
 	if err != nil {
 		return job, err
 	}
-	if d := driftedTagDigest(svc); d != "" && d != svc.LastTagDigest {
+	if d := driftedTagDigest(svc); d != "" && d != svc.LastTagDigest && holdsNothingNewer(svc) {
 		if err := s.store.SetLastTagDigest(ctx, svc.ID, d); err != nil {
 			s.log.Warn("last_tag_digest_not_set", "service_id", svc.ID, "error", err)
 		}
@@ -161,6 +166,13 @@ func (s *Service) RestoreDesired(ctx context.Context, serviceID int64, requested
 	return job, nil
 }
 
+// holdsNothingNewer reports whether last_tag_digest may be overwritten by a
+// restore: it is empty or equals the digest the desired ref names. Anything
+// else is a digest the poller must keep holding back.
+func holdsNothingNewer(svc model.Service) bool {
+	return svc.LastTagDigest == "" || svc.LastTagDigest == imageref.Parse(svc.DesiredRef).Digest
+}
+
 // driftedTagDigest returns the digest a drifted service runs when it runs the
 // same tag as desired at another digest, else "".
 func driftedTagDigest(svc model.Service) string {

internal/master/model/model.go

@@ -15,6 +15,8 @@ import (
 // Sentinel errors returned by storage.
 var (
 	ErrNotFound = errors.New("not found")
+	// ErrConflict means the row already exists, or that it changed meanwhile
+	// (AcceptRunning, MergeService).
 	ErrConflict = errors.New("already exists")
 	ErrRevoked  = errors.New("agent revoked")
 )
@@ -166,6 +168,11 @@ func RunningRef(actualRef, actualDigest string) string {
 	return r.String()
 }
 
+// RunningRef returns the ref accepting the running version records as the
+// desired one; see the function RunningRef. Templates post it as the guard of
+// the accept form.
+func (s Service) RunningRef() string { return RunningRef(s.ActualRef, s.ActualDigest) }
+
 // Move says that the removed service FromID has probably moved to the live
 // service ToID on another agent. The names are the agents' own; a revoked
 // agent keeps its name.

internal/master/store/migrations/0005_drift_merge.sql

@@ -1,7 +1,7 @@
--- true from a successful job until the next inventory write for the row: the
+-- True from a successful job until the next inventory write for the row. The
 -- agent's upsert arrives a debounce interval after job.result, and until then
 -- desired_ref is newer than actual_ref without the service having drifted.
 ALTER TABLE services ADD COLUMN awaiting_inventory boolean NOT NULL DEFAULT false;
 
--- move suggestions look up Compose rows by project + service across agents
+-- Move suggestions look up Compose rows by project and service across agents.
 CREATE INDEX services_by_compose_name ON services (compose_project, compose_service) WHERE kind = 'compose';

internal/master/store/services.go

@@ -259,8 +259,8 @@ func (s *Store) SetPollError(ctx context.Context, ids []int64, msg string) error
 }
 
 // SetLastTagDigest sets last_tag_digest; see model.Service.LastTagDigest. The
-// poller calls it, and FinishJob sets the same column after a tag job.
+// poller and jobs.RestoreDesired call it, and FinishJob sets the same column
-// Inventory writes never change it.
+// after a tag job. Inventory writes never change it.
 func (s *Store) SetLastTagDigest(ctx context.Context, id int64, digest string) error {
 	if _, err := s.pool.Exec(ctx,
 		`UPDATE services SET last_tag_digest = $2 WHERE id = $1`, id, digest); err != nil {

internal/master/web/drift_test.go

@@ -118,7 +118,7 @@ func TestAcceptRunning_WritesRunningRefAndPublishes(t *testing.T) {
 	ch, cancel := broker.Subscribe()
 	defer cancel()
 
-	resp, _ := postTo(t, srv, "/services/1/accept", url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2"}})
+	resp, _ := postTo(t, srv, "/services/1/accept", url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2@" + driftActual}})
 
 	assert.Equal(t, http.StatusSeeOther, resp.StatusCode)
 	assert.Equal(t, "/services/1", resp.Header.Get("Location"))
@@ -139,7 +139,7 @@ func TestAcceptRunning_ReturnToOverview(t *testing.T) {
 	driftService(store)
 
 	resp, _ := postTo(t, srv, "/services/1/accept",
-		url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2"}, "return": {"overview"}})
+		url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2@" + driftActual}, "return": {"overview"}})
 
 	assert.Equal(t, http.StatusSeeOther, resp.StatusCode)
 	assert.Equal(t, "/", resp.Header.Get("Location"))
@@ -151,6 +151,7 @@ func TestAcceptRunning_Refusals(t *testing.T) {
 		path       string
 		form       url.Values
 		notDrifted bool
+		removed    bool
 		acceptErr  error
 		wantCode   int
 		wantBody   string
@@ -158,12 +159,17 @@ func TestAcceptRunning_Refusals(t *testing.T) {
 		{name: "missing actual", path: "/services/1/accept", form: url.Values{}, wantCode: http.StatusBadRequest},
 		{name: "stale actual", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.3.0"}},
 			wantCode: http.StatusConflict, wantBody: "The running version changed; check it again."},
-		{name: "not drifted", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2"}},
+		{name: "same ref, other digest", path: "/services/1/accept",
+			form:     url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2@" + driftDesired}},
+			wantCode: http.StatusConflict, wantBody: "The running version changed; check it again."},
+		{name: "not drifted", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2@" + driftActual}},
 			notDrifted: true, wantCode: http.StatusConflict, wantBody: "Cannot accept: the service runs its desired version."},
-		{name: "changed under our feet", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2"}},
+		{name: "changed under our feet", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2@" + driftActual}},
 			acceptErr: model.ErrConflict, wantCode: http.StatusConflict, wantBody: "The running version changed; check it again."},
 		{name: "unknown service", path: "/services/9/accept", form: url.Values{"actual": {"x"}}, wantCode: http.StatusNotFound},
-		{name: "removed meanwhile", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2"}},
+		{name: "removed", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2@" + driftActual}},
+			removed: true, wantCode: http.StatusNotFound},
+		{name: "removed meanwhile", path: "/services/1/accept", form: url.Values{"actual": {"gitea.shcizo.se/shcizo/slusk:1.4.2@" + driftActual}},
 			acceptErr: model.ErrNotFound, wantCode: http.StatusNotFound},
 	}
 	for _, tt := range tests {
@@ -172,6 +178,10 @@ func TestAcceptRunning_Refusals(t *testing.T) {
 			if !tt.notDrifted {
 				driftService(store)
 			}
+			if tt.removed {
+				removedAt := time.Now()
+				store.services[0].RemovedAt = &removedAt
+			}
 			store.acceptErr = tt.acceptErr
 
 			resp, body := postTo(t, srv, tt.path, tt.form)

internal/master/web/server.go

@@ -349,13 +349,7 @@ func (s *Server) updateService(w http.ResponseWriter, r *http.Request) {
 	case err == nil || errors.Is(err, model.ErrJobActive):
 		http.Redirect(w, r, "/jobs/"+strconv.FormatInt(job.ID, 10), http.StatusSeeOther)
 	case errors.As(err, &nu):
-		data, derr := s.serviceDetail(r, id)
+		s.renderServiceError(w, r, id, http.StatusConflict, "Cannot update: "+nu.Reason+".")
-		if derr != nil {
-			s.failLookup(w, derr)
-			return
-		}
-		data.Error = "Cannot update: " + nu.Reason + "."
-		s.render(w, http.StatusConflict, "service.html", "layout", data)
 	default:
 		s.failLookup(w, err)
 	}
@@ -381,8 +375,9 @@ func (s *Server) restoreDesired(w http.ResponseWriter, r *http.Request) {
 }
 
 // acceptRunning makes the running version of a drifted service the desired
-// one. The form's actual field is the ref the user saw; it only guards against
+// one. The form's actual field is the running ref the user saw (Service.RunningRef);
-// the service having changed since, the value written comes from the row.
+// it only guards against the service having changed since, the value written
+// comes from the row.
 func (s *Server) acceptRunning(w http.ResponseWriter, r *http.Request) {
 	id, ok := pathID(w, r)
 	if !ok {
@@ -394,6 +389,9 @@ func (s *Server) acceptRunning(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	svc, err := s.store.GetService(r.Context(), id)
+	if err == nil && svc.RemovedAt != nil {
+		err = model.ErrNotFound
+	}
 	if err != nil {
 		s.failLookup(w, err)
 		return
@@ -402,11 +400,11 @@ func (s *Server) acceptRunning(w http.ResponseWriter, r *http.Request) {
 		s.renderServiceError(w, r, id, http.StatusConflict, "Cannot accept: the service runs its desired version.")
 		return
 	}
-	if actual != svc.ActualRef {
+	if actual != svc.RunningRef() {
 		s.renderServiceError(w, r, id, http.StatusConflict, "The running version changed; check it again.")
 		return
 	}
-	desired := model.RunningRef(svc.ActualRef, svc.ActualDigest)
+	desired := svc.RunningRef()
 	err = s.store.AcceptRunning(r.Context(), id, svc.ActualRef, svc.ActualDigest, desired)
 	switch {
 	case errors.Is(err, model.ErrConflict):
@@ -458,7 +456,7 @@ func (s *Server) mergeService(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	sess, _ := webauth.SessionFrom(r.Context())
-	s.log.Info("service_merged", "from", from, "to", id, "subject", sess.Subject)
+	s.log.Info("service_merged", "from_id", from, "to_id", id, "subject", sess.Subject)
 	s.broker.Publish(events.Services)
 	s.broker.Publish(events.Jobs)
 	http.Redirect(w, r, "/services/"+strconv.FormatInt(from, 10), http.StatusSeeOther)
@@ -498,13 +496,7 @@ func (s *Server) setMode(w http.ResponseWriter, r *http.Request) {
 		}
 		var nu *model.NotUpdatableError
 		if err := s.jobs.AutoAllowed(svc); errors.As(err, &nu) {
-			data, derr := s.serviceDetail(r, id)
+			s.renderServiceError(w, r, id, http.StatusConflict, "Cannot enable auto: "+nu.Reason+".")
-			if derr != nil {
-				s.failLookup(w, derr)
-				return
-			}
-			data.Error = "Cannot enable auto: " + nu.Reason + "."
-			s.render(w, http.StatusConflict, "service.html", "layout", data)
 			return
 		} else if err != nil {
 			s.fail(w, err)