From 98a05997aeb84be78c07e123056c9e6646cccad4 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 10:31:01 +0100 Subject: [PATCH] Assigner: keep a held-back node's record ask, and clear the held record and condition of a stale hold (issue 1906) Review round 1: a record ask is about the composition of any fingerprint a node may still want (trunk, want, have), not only the trunk's, so deleting it looped with the wants rule that asks it again and the operator was never asked. It is now stale only when the module left the trunk or no read composition of those fingerprints has its digest. A hold or restore ask taken away as stale also takes its held record and clears its held condition. --- .../cmd/mesh-delivery-assigner/pass.go | 42 ++++++++- .../cmd/mesh-delivery-assigner/pass_test.go | 91 +++++++++++++++++-- 2 files changed, 121 insertions(+), 12 deletions(-) diff --git a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go index 4185077..dc4eb35 100644 --- a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go +++ b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" "time" @@ -433,15 +434,40 @@ func (p *passer) staleAsk(key string, trunk map[string]delivery.Module) bool { if !on { return true } - if m.FP2 == "" { - return false + // The assigner asks about a composition at more than the trunk fingerprint: a node held back by the rules is asked about the + // revised composition of the fingerprint it runs (ADR 0545), and a node's first want at the older fingerprint it runs. So a + // record ask is stale only when its digest is the composition of none of the fingerprints the node can still want (the + // trunk's, its want's, its have's), and only when every one of them is read. + for _, fp := range p.fingerprintsOf(k.Node, k.Module, m.FP2) { + d, ok := p.w.composed(k.Node, k.Module, fp) + if !ok || d.Digest == k.Composed { + return false + } } - d, ok := p.w.composed(k.Node, k.Module, m.FP2) - return ok && d.Digest != k.Composed + return len(p.fingerprintsOf(k.Node, k.Module, m.FP2)) > 0 } return false } +// fingerprintsOf are the fingerprints a node may still want of module: the trunk's, the one its want names and the one its have +// names, each once. +func (p *passer) fingerprintsOf(node, module, trunkFP string) []string { + var out []string + add := func(fp string) { + if fp != "" && !slices.Contains(out, fp) { + out = append(out, fp) + } + } + add(trunkFP) + add(p.w.wantsOf()[module][node].FP) + if key, err := delivery.HaveKey(node, module); err == nil { + if h, err := delivery.DecodeHave(p.w.value(delivery.HaveBucket, key)); err == nil { + add(h.FP) + } + } + return out +} + // staleAsks takes away, at every pass, each hold, restore and record ask that staleAsk names (issue 1906, note 28): an ask // outlived its fingerprint, and the teller takes its router ask back from the operator when the key goes. With the trunk not // read at all (no module), nothing is taken away. @@ -453,6 +479,14 @@ func (p *passer) staleAsks(ctx context.Context, trunk map[string]delivery.Module if p.staleAsk(key, trunk) { p.logf("the ask %s is about a fingerprint or composition the trunk has left behind and is taken away", key) p.remove(ctx, delivery.AssignerAsksBucket, key) + // A hold or restore ask of a fingerprint the trunk left goes with the hold itself: its held record, and its condition, + // which would else ask for an answer that can no longer be given. + if k, err := delivery.ParseAnyAskKey(key); err == nil && (k.Kind == delivery.AskKindHold || k.Kind == delivery.AskKindRestore) { + if held, err := delivery.HeldKey(k.Module, k.FP); err == nil { + p.remove(ctx, delivery.AssignerWantsBucket, held) + } + p.clearHeld(k.Module, k.FP) + } } } } diff --git a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go index 9f9a5b3..12de196 100644 --- a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go +++ b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go @@ -521,16 +521,20 @@ func TestAsksOfAFingerprintTheTrunkLeftBehindAreTakenAway(t *testing.T) { trunk map[string]delivery.Module gone []string // the keys taken away noDigit bool // the controller's composition is not read + heldAt string // the node's want is at this fingerprint, whose composition is digOld }{ - {"the trunk moved on", trunkOf(fpNew), []string{holdOld, holdGone, restOld, recOld}, false}, - {"the trunk is at the old fingerprint", trunkOf(fpOld), []string{holdNew, holdGone, restNew, recOld}, false}, - {"the trunk's fingerprint is not read yet", trunkOf(""), []string{holdGone}, false}, - {"the composition is not read yet", trunkOf(fpNew), []string{holdOld, holdGone, restOld}, true}, - {"the trunk is not read at all", nil, nil, false}, + {"the trunk moved on", trunkOf(fpNew), []string{holdOld, holdGone, restOld, recOld}, false, ""}, + {"the trunk is at the old fingerprint", trunkOf(fpOld), []string{holdNew, holdGone, restNew, recOld}, false, ""}, + {"the trunk's fingerprint is not read yet", trunkOf(""), []string{holdGone}, false, ""}, + {"the composition is not read yet", trunkOf(fpNew), []string{holdOld, holdGone, restOld}, true, ""}, + {"a node held back at the old fingerprint is still asked about its composition", trunkOf(fpNew), []string{holdOld, holdGone, restOld}, false, fpOld}, + {"the trunk is not read at all", nil, nil, false, ""}, } { t.Run(c.name, func(t *testing.T) { - asksKV := &memKV{values: map[string][]byte{}} - p := newPasser(buckets{delivery.AssignerAsksBucket: asksKV}, time.Now()) + asksKV, wantsKV := &memKV{values: map[string][]byte{}}, &memKV{values: map[string][]byte{}} + p := newPasser(buckets{delivery.AssignerAsksBucket: asksKV, delivery.AssignerWantsBucket: wantsKV}, time.Now()) + var said []asks.Condition + p.say = func(c asks.Condition) error { said = append(said, c); return nil } for _, key := range all { k, _ := delivery.ParseAnyAskKey(key) var a delivery.AssignerAsk @@ -556,9 +560,30 @@ func TestAsksOfAFingerprintTheTrunkLeftBehindAreTakenAway(t *testing.T) { v, _ := delivery.EncodeComposedDigest(delivery.ComposedDigest{Digest: digNew, Judgement: delivery.JudgedUnchanged}) p.w.put(delivery.ControllerComposedBucket, ckey, v) okey, _ := delivery.ComposedKey("novox", "web", fpOld) - p.w.put(delivery.ControllerComposedBucket, okey, v) + old, _ := delivery.EncodeComposedDigest(delivery.ComposedDigest{Digest: digNew, Judgement: delivery.JudgedUnchanged}) + if c.heldAt != "" { + old, _ = delivery.EncodeComposedDigest(delivery.ComposedDigest{Digest: digOld, Judgement: delivery.JudgedUnchanged}) + } + p.w.put(delivery.ControllerComposedBucket, okey, old) } + if c.heldAt != "" { + wkey, _ := delivery.WantKey("novox", "web") + w, _ := delivery.EncodeWant(delivery.Want{FP: c.heldAt, Composed: digOld}) + p.w.put(delivery.AssignerWantsBucket, wkey, w) + } + heldOld, _ := delivery.HeldKey("web", fpOld) + wantsKV.values[heldOld] = []byte("x") + p.w.put(delivery.AssignerWantsBucket, heldOld, []byte("x")) p.staleAsks(context.Background(), c.trunk) + // the held record and condition of a fingerprint whose hold ask went are taken away with it + heldOld, _ = delivery.HeldKey("web", fpOld) + _, heldThere := wantsKV.values[heldOld] + cleared := slices.ContainsFunc(said, func(c asks.Condition) bool { + return c.Key == assign.HeldCondition("web", fpOld, "", "", false, false).Key && c.State == asks.StateCleared + }) + if gone := slices.Contains(c.gone, holdOld); heldThere == gone || cleared != gone { + t.Errorf("hold ask of the old fingerprint gone = %v, but its held record left = %v and its condition cleared = %v", gone, heldThere, cleared) + } for _, key := range all { _, there := asksKV.values[key] want := !slices.Contains(c.gone, key) @@ -593,3 +618,53 @@ func mustKey(key string, err error) string { } return key } + +// 1906 note 28, review: a node held back at an earlier fingerprint is asked about the revised composition of that fingerprint +// (ADR 0545). That record ask is not about the trunk's fingerprint: it survives every pass, so the operator is asked, and on a +// yes the want names the revised digest. +func TestARecordAskOfAHeldBackNodeSurvivesPassesAndItsYesMovesTheWant(t *testing.T) { + fpOld := delivery.SourcePrefix2 + strings.Repeat("a", 64) + revised, was := "sha256:"+strings.Repeat("e", 64), "sha256:"+strings.Repeat("d", 64) + asksKV, wantsKV := &memKV{values: map[string][]byte{}}, &memKV{values: map[string][]byte{}} + p := newPasser(buckets{delivery.AssignerAsksBucket: asksKV, delivery.AssignerWantsBucket: wantsKV}, time.Now()) + p.logf = t.Logf + put := func(bucket, key string, v []byte) { p.w.put(bucket, key, v) } + mod, _ := delivery.EncodeModule(delivery.Module{FP2: fpTest, Repository: "novox/web", Commit: strings.Repeat("1", 40), Path: delivery.RootPath}) + put(delivery.ModulesBucket, "web", mod) // the trunk's build is not done: rule 1 holds g14 back at fpOld + put(provider.AssignmentsBucket, "g14.web", []byte(`{"node":"g14","module":"web","build":"b-1"}`)) + on, _ := delivery.EncodePresence(delivery.Presence{Online: true}) + put(delivery.PresenceBucket, "g14", on) + have, _ := delivery.EncodeHave(delivery.Have{FP: fpOld, Phase: delivery.PhaseRunning, Health: delivery.HealthOK}) + put(delivery.HaveBucket, "g14.web", have) + wkey, _ := delivery.WantKey("g14", "web") + want, _ := delivery.EncodeWant(delivery.Want{FP: fpOld, Composed: was}) + wantsKV.values[wkey] = want + put(delivery.AssignerWantsBucket, wkey, want) + ckey, _ := delivery.ComposedKey("g14", "web", fpOld) + cd, cerr := delivery.EncodeComposedDigest(delivery.ComposedDigest{Digest: revised, Judgement: delivery.JudgedRecreates, Policy: delivery.PolicyRecord, PolicyFrom: "default", Against: was, Words: "recreates"}) + if cerr != nil { + t.Fatal(cerr) + } + put(delivery.ControllerComposedBucket, ckey, cd) + akey, _ := delivery.RecordAskKey("g14", "web", revised) + for i := 0; i < 2; i++ { + p.wants(context.Background()) + if _, there := asksKV.values[akey]; !there { + t.Fatalf("pass %d: the record ask of the held-back node is not there", i+1) + } + } + a, err := delivery.DecodeAssignerAsk(akey, asksKV.values[akey]) + if err != nil { + t.Fatal(err) + } + yes, _ := delivery.EncodeAnswer(delivery.Answer{Option: assign.OptionYes, Warrant: a.Ask.ID + "@2026-10-26T01:00:00Z", Words: "the operator"}) + put(delivery.AnswersBucket, akey, yes) + p.wants(context.Background()) + got, err := delivery.DecodeWant(wantsKV.values[wkey]) + if err != nil || got.FP != fpOld || got.Composed != revised { + t.Errorf("after the yes the want is %+v (%v), want it at %s naming %s", got, err, fpOld, revised) + } + if _, there := asksKV.values[akey]; !there { + t.Error("the answered record ask was taken away") + } +}