Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 51 additions & 18 deletions internal/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -1231,37 +1231,70 @@ func (s *Store) RepairCloudUpgrade(project string, apply bool) (CloudUpgradeRepa
if err != nil {
return CloudUpgradeRepairReport{}, fmt.Errorf("diagnose legacy cloud upgrade mutations: %w", err)
}
// When both repairable and blocked mutations coexist we must not hold the
// entire repair pass hostage to the non-repairable entries. Apply the
// repairable subset first, then surface the residual blockers.
appliedRepairs := false
if apply && legacyReport.RepairableCount > 0 {
if err := s.applyCloudUpgradeLegacyMutationRepairs(project); err != nil {
return CloudUpgradeRepairReport{}, fmt.Errorf("apply cloud upgrade legacy mutation repairs: %w", err)
}
appliedRepairs = true
}

if legacyReport.BlockedCount > 0 {
first := legacyReport.Findings[0]
// Scan for the first non-repairable finding so the operator sees the
// actual blocker. Findings[0] is ordered by seq and may be a repairable
// entry, which previously produced a misleading error message (#446).
var blocked CloudUpgradeLegacyMutationFinding
for _, f := range legacyReport.Findings {
if !f.Repairable {
blocked = f
break
}
}
var msg string
switch {
case appliedRepairs:
msg = fmt.Sprintf("applied %d repairable payload(s); %d remain blocked: manual-action-required: %s (seq=%d entity=%s entity_key=%q op=%s)",
legacyReport.RepairableCount, legacyReport.BlockedCount, blocked.Message, blocked.Seq, blocked.Entity, blocked.EntityKey, blocked.Op)
case legacyReport.RepairableCount > 0:
msg = fmt.Sprintf("%d repairable payload(s) would apply; %d would remain blocked: manual-action-required: %s (seq=%d entity=%s entity_key=%q op=%s)",
legacyReport.RepairableCount, legacyReport.BlockedCount, blocked.Message, blocked.Seq, blocked.Entity, blocked.EntityKey, blocked.Op)
default:
msg = fmt.Sprintf("manual-action-required: %s (seq=%d entity=%s entity_key=%q op=%s)",
blocked.Message, blocked.Seq, blocked.Entity, blocked.EntityKey, blocked.Op)
}
return CloudUpgradeRepairReport{
Class: UpgradeRepairClassBlocked,
ReasonCode: UpgradeReasonBlockedLegacyMutationManual,
Message: fmt.Sprintf("manual-action-required: %s (seq=%d entity=%s op=%s)", first.Message, first.Seq, first.Entity, first.Op),
Applied: false,
Message: msg,
Applied: appliedRepairs,
}, nil
}

if legacyReport.RepairableCount > 0 {
report := CloudUpgradeRepairReport{
Class: UpgradeRepairClassRepairable,
ReasonCode: UpgradeReasonRepairableLegacyMutationPayload,
Message: fmt.Sprintf("project %q has %d repairable legacy mutation payload issue(s)", project, legacyReport.RepairableCount),
PlannedAction: "repair_legacy_mutation_payloads",
Applied: false,
}
if !apply {
return report, nil
}
if err := s.applyCloudUpgradeLegacyMutationRepairs(project); err != nil {
return CloudUpgradeRepairReport{}, fmt.Errorf("apply cloud upgrade legacy mutation repairs: %w", err)
if !appliedRepairs {
return CloudUpgradeRepairReport{
Class: UpgradeRepairClassRepairable,
ReasonCode: UpgradeReasonRepairableLegacyMutationPayload,
Message: fmt.Sprintf("project %q has %d repairable legacy mutation payload issue(s)", project, legacyReport.RepairableCount),
PlannedAction: "repair_legacy_mutation_payloads",
Applied: false,
}, nil
}
report.Applied = true
report.Message = fmt.Sprintf("applied deterministic legacy mutation payload repairs for project %q", project)
_ = s.SaveCloudUpgradeState(CloudUpgradeState{
Project: project,
Stage: UpgradeStageRepairApplied,
RepairClass: UpgradeRepairClassRepairable,
})
return report, nil
return CloudUpgradeRepairReport{
Class: UpgradeRepairClassRepairable,
ReasonCode: UpgradeReasonRepairableLegacyMutationPayload,
Message: fmt.Sprintf("applied deterministic legacy mutation payload repairs for project %q", project),
PlannedAction: "repair_legacy_mutation_payloads",
Applied: true,
}, nil
}

requiresBackfill, err := s.projectSyncBackfillRequired(project)
Expand Down
142 changes: 142 additions & 0 deletions internal/store/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2118,6 +2118,148 @@ func TestUpgradeRepairDryRunAndApply(t *testing.T) {
t.Fatalf("expected missing provenance to remain blocked, got %+v", diagnosis)
}
})

// Regression test for GitHub issue #446: when both repairable and blocked
// mutations coexist, RepairCloudUpgrade(apply=true) must apply the
// repairable subset and return Applied:true with Class=Blocked (and the
// message must reference the actual blocker, not the low-seq repairable
// entry that happens to be first in Findings order).
t.Run("partial apply: repairable mutations applied even when a blocker is queued", func(t *testing.T) {
s := newTestStore(t)
if err := s.CreateSession("partial-repair-s1", "partial-repair-proj", "/tmp/partial-repair"); err != nil {
t.Fatalf("create session: %v", err)
}

// Create an observation so we have authoritative local state for the
// repairable mutation.
obsID, err := s.AddObservation(AddObservationParams{
SessionID: "partial-repair-s1",
Type: "decision",
Title: "Authoritative title",
Content: "Authoritative content",
Project: "partial-repair-proj",
Scope: "project",
})
if err != nil {
t.Fatalf("add observation: %v", err)
}
if err := s.EnrollProject("partial-repair-proj"); err != nil {
t.Fatalf("enroll project: %v", err)
}

// Look up the observation's sync_id for payload construction.
var obsSyncID string
if err := s.db.QueryRow(`SELECT sync_id FROM observations WHERE id = ?`, obsID).Scan(&obsSyncID); err != nil {
t.Fatalf("lookup observation sync_id: %v", err)
}

// Insert a REPAIRABLE observation mutation (missing title — low seq,
// will naturally come before the blocker we insert next).
repairablePayload := `{"sync_id":"` + obsSyncID + `","session_id":"partial-repair-s1","type":"decision","content":"legacy payload missing title","scope":"project"}`
if _, err := s.execHook(s.db,
`INSERT INTO sync_mutations (target_key, entity, entity_key, op, payload, source, project) VALUES (?, ?, ?, ?, ?, ?, ?)`,
DefaultSyncTargetKey,
SyncEntityObservation,
obsSyncID,
SyncOpUpsert,
repairablePayload,
SyncSourceLocal,
"partial-repair-proj",
); err != nil {
t.Fatalf("insert repairable observation mutation: %v", err)
}

// Create two observations for source/target of the relation we use as
// the blocker mutation.
srcID, err := s.AddObservation(AddObservationParams{SessionID: "partial-repair-s1", Type: "decision", Title: "Src", Content: "src", Project: "partial-repair-proj", Scope: "project"})
if err != nil {
t.Fatalf("add source observation: %v", err)
}
dstID, err := s.AddObservation(AddObservationParams{SessionID: "partial-repair-s1", Type: "decision", Title: "Dst", Content: "dst", Project: "partial-repair-proj", Scope: "project"})
if err != nil {
t.Fatalf("add dest observation: %v", err)
}
var srcSyncID, dstSyncID string
if err := s.db.QueryRow(`SELECT sync_id FROM observations WHERE id = ?`, srcID).Scan(&srcSyncID); err != nil {
t.Fatalf("lookup src sync_id: %v", err)
}
if err := s.db.QueryRow(`SELECT sync_id FROM observations WHERE id = ?`, dstID).Scan(&dstSyncID); err != nil {
t.Fatalf("lookup dst sync_id: %v", err)
}

// Insert a BLOCKED relation mutation (missing provenance — high seq,
// comes after the repairable entry so Findings[0] would point to the
// repairable entry without the fix).
const blockerEntityKey = "rel-partial-blocked-446"
blockerPayload := `{"sync_id":"` + blockerEntityKey + `","source_id":"` + srcSyncID + `","target_id":"` + dstSyncID + `","relation":"compatible","project":"partial-repair-proj"}`
if _, err := s.execHook(s.db,
`INSERT INTO sync_mutations (target_key, entity, entity_key, op, payload, source, project) VALUES (?, ?, ?, ?, ?, ?, ?)`,
DefaultSyncTargetKey,
SyncEntityRelation,
blockerEntityKey,
SyncOpUpsert,
blockerPayload,
SyncSourceLocal,
"partial-repair-proj",
); err != nil {
t.Fatalf("insert blocked relation mutation: %v", err)
}

// Confirm pre-conditions: diagnosis must see exactly one repairable
// and one blocked entry.
diag, err := s.DiagnoseCloudUpgradeLegacyMutations("partial-repair-proj")
if err != nil {
t.Fatalf("pre-condition diagnose: %v", err)
}
if diag.RepairableCount != 1 || diag.BlockedCount != 1 {
t.Fatalf("expected 1 repairable + 1 blocked pre-condition, got %+v", diag)
}

// Find the seq of the blocked finding so we can assert the message
// references the right entry.
var blockerSeq int64
for _, f := range diag.Findings {
if !f.Repairable {
blockerSeq = f.Seq
break
}
}
if blockerSeq == 0 {
t.Fatal("pre-condition: could not locate blocked finding seq")
}
// Also verify that Findings[0] is NOT the blocker (i.e. lowest-seq is
// the repairable one); this is the exact condition that triggered the
// wrong-message bug.
if diag.Findings[0].Seq == blockerSeq {
t.Fatalf("pre-condition: expected Findings[0] to be repairable (lowest seq), but got blocker seq=%d", blockerSeq)
}

// === THE ACTUAL ASSERTION ===
// With apply=true, repairable mutations must be applied and the
// report must still surface the blocker clearly.
report, err := s.RepairCloudUpgrade("partial-repair-proj", true)
if err != nil {
t.Fatalf("repair with mixed repairable+blocker: %v", err)
}

// Applied MUST be true: at least the repairable mutation was applied.
if !report.Applied {
t.Fatalf("expected Applied=true (repairable subset was processed), got %+v", report)
}
// Class MUST be Blocked because the non-repairable mutation is still present.
if report.Class != UpgradeRepairClassBlocked {
t.Fatalf("expected Class=%q, got %q (full report: %+v)", UpgradeRepairClassBlocked, report.Class, report)
}
// The message must name the BLOCKER seq, not the repairable entry.
blockerSeqStr := fmt.Sprintf("seq=%d", blockerSeq)
if !strings.Contains(report.Message, blockerSeqStr) {
t.Fatalf("expected message to reference blocker seq (%s), got %q", blockerSeqStr, report.Message)
}
// The message must also include entity_key for debuggability.
if !strings.Contains(report.Message, blockerEntityKey) {
t.Fatalf("expected message to include entity_key=%q, got %q", blockerEntityKey, report.Message)
}
})
}

func TestRollbackCloudUpgradeSafetyBoundary(t *testing.T) {
Expand Down