From 112a753f0665b73094e33dd30de0795d1d4edda4 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Tue, 3 Nov 2026 21:56:39 +0000 Subject: [PATCH] feat(addons): make an operation added by an upgrade pending on its own An upgrade adds operations to an entry that already holds others. With one operation ID per entry, the whole entry went pending until the upgrade completed, so the running old version lost its grants for that time, and a crash removed all of them. Give an operation its own optional operation ID. Only that operation waits for its lifecycle operation, and reconciliation removes only what an unfinished upgrade added. Assisted-by: Claude:claude-sonnet-5-5 go --- core/services/addons/hostgrants/document.go | 8 ++++ core/services/addons/hostgrants/policy.go | 12 +++++- core/services/addons/hostgrants/reconcile.go | 26 ++++++++++++- .../addons/hostgrants/reconcile_test.go | 37 +++++++++++++++++++ 4 files changed, 80 insertions(+), 3 deletions(-) diff --git a/core/services/addons/hostgrants/document.go b/core/services/addons/hostgrants/document.go index 12fa369b2b..903997f465 100644 --- a/core/services/addons/hostgrants/document.go +++ b/core/services/addons/hostgrants/document.go @@ -89,6 +89,11 @@ type OpGrant struct { // BlanketRevision names the blanket switch revision that produced a // grant with Source blanket, so the group can be told apart and revoked. BlanketRevision uint64 `json:"blanketRevision,omitempty"` + // OperationID is set when the operation was added to an entry that already + // existed, by the lifecycle operation of an upgrade. It takes effect only + // once that operation completed, while the other operations of the entry + // keep working. Empty means the operation of the entry counts. + OperationID string `json:"operationId,omitempty"` } // AddonGrants is the UI-made grant entry of one add-on. @@ -343,6 +348,9 @@ func validate(s Snapshot) error { return invalid("operation %q of %q", op.Name, id) } seen[op.Name] = true + if op.OperationID != "" && !validOperationID(op.OperationID) { + return invalid("operation ID of %q for %q", op.Name, id) + } if !validText(op.By) || !validText(op.Version) || !validTime(op.At) { return invalid("metadata of %q for %q", op.Name, id) } diff --git a/core/services/addons/hostgrants/policy.go b/core/services/addons/hostgrants/policy.go index 9f3df3bff8..d671d410b3 100644 --- a/core/services/addons/hostgrants/policy.go +++ b/core/services/addons/hostgrants/policy.go @@ -119,12 +119,22 @@ func (f Floor) Decide(doc Snapshot, c Context, addonID, op string) Decision { if c.Identity == "" || entry.Identity != c.Identity { return Decision{Reason: ReasonIdentityMismatch} } - if c.Completed != nil && !c.Completed(entry.OperationID) { + if c.Completed != nil && !c.Completed(EffectiveOperationID(entry, entry.Ops[i])) { return Decision{Reason: ReasonNotGranted} } return Decision{Allowed: true, Source: entry.Ops[i].Source} } +// EffectiveOperationID is the lifecycle operation whose completion makes one +// operation of an entry effective: its own, when an upgrade added it to an +// existing entry, else the one that created the entry. +func EffectiveOperationID(entry AddonGrants, op OpGrant) string { + if op.OperationID != "" { + return op.OperationID + } + return entry.OperationID +} + // OpStatus is a Decision for one named operation. type OpStatus struct { Op string diff --git a/core/services/addons/hostgrants/reconcile.go b/core/services/addons/hostgrants/reconcile.go index ad65511160..3f619795e5 100644 --- a/core/services/addons/hostgrants/reconcile.go +++ b/core/services/addons/hostgrants/reconcile.go @@ -51,8 +51,30 @@ func PlanReconcile(snap Snapshot, in ReconcileInput) (Patch, []Removal) { reason = RemovedNotInstalled case identity != entry.Identity: reason = RemovedIdentityMismatch - case in.OperationCompleted != nil && !in.OperationCompleted(entry.OperationID): - reason = RemovedOperationIncomplete + } + if reason == "" && in.OperationCompleted != nil { + // An operation added by an upgrade is pending on its own; the + // operations the entry held before keep working. + var kept []OpGrant + var dropped []string + for _, op := range entry.Ops { + if in.OperationCompleted(EffectiveOperationID(entry, op)) { + kept = append(kept, op) + } else { + dropped = append(dropped, op.Name) + } + } + switch { + case len(dropped) == 0: + case len(kept) == 0: + reason = RemovedOperationIncomplete + default: + trimmed := entry + trimmed.Ops = kept + patch.Addons[id] = &trimmed + removals = append(removals, Removal{AddonID: id, Reason: RemovedOperationIncomplete, Ops: dropped}) + continue + } } if reason == "" { continue diff --git a/core/services/addons/hostgrants/reconcile_test.go b/core/services/addons/hostgrants/reconcile_test.go index d68634ee67..57ce38e4f4 100644 --- a/core/services/addons/hostgrants/reconcile_test.go +++ b/core/services/addons/hostgrants/reconcile_test.go @@ -161,3 +161,40 @@ func TestAuditAppendsFsyncedLines(t *testing.T) { t.Error("a symlink must be refused") } } + +func TestUpgradeAddedOperationIsPendingOnItsOwn(t *testing.T) { + entry := *grant("install-op", "workspace.materialize") + entry.Ops = append(entry.Ops, OpGrant{Name: "egress.check", Source: SourceUpgrade, OperationID: "upgrade-op"}) + snap := emptySnapshot() + snap.Grants["a"] = entry + completed := map[string]bool{"install-op": true} + ctx := Context{Identity: "id-1", Completed: func(id string) bool { return completed[id] }} + + // Before the upgrade completes, the old operation works and the new one does not. + var f Floor + if !f.Decide(snap, ctx, "a", "workspace.materialize").Allowed { + t.Fatal("the operation held before the upgrade stopped working") + } + if d := f.Decide(snap, ctx, "a", "egress.check"); d.Allowed || d.Reason != ReasonNotGranted { + t.Fatalf("%+v", d) + } + completed["upgrade-op"] = true + if !f.Decide(snap, ctx, "a", "egress.check").Allowed { + t.Fatal("the operation of a completed upgrade is not effective") + } + + // A crash before the upgrade completed removes only what it added. + delete(completed, "upgrade-op") + in := ReconcileInput{ + Installed: func(string) (string, bool) { return "id-1", true }, + OperationCompleted: func(id string) bool { return completed[id] }, + } + patch, removals := PlanReconcile(snap, in) + if len(removals) != 1 || !reflect.DeepEqual(removals[0].Ops, []string{"egress.check"}) || removals[0].Reason != RemovedOperationIncomplete { + t.Fatalf("%+v", removals) + } + kept := patch.Addons["a"] + if kept == nil || len(kept.Ops) != 1 || kept.Ops[0].Name != "workspace.materialize" { + t.Fatalf("%+v", kept) + } +}