Merge pull request #131 from linuxfoundation/jme/LFXV2-2673

fix(committee-members): sync contact SFID (MemberID) to v1 on delete

Author: jordane

README.md

@@ -176,6 +176,24 @@ lfx-v1-sync-helper --backfill-acs-project [--dry-run]
 lfx-v1-sync-helper --backfill-acs-org [--dry-run]
 ```
 
+**Committee-member reverse-mapping repair (`--backfill-committee-member-mappings`):**
+
+Repairs committee-member reverse mappings (`committee_member.uid.*` in `v1-mappings`) that store the `platform-community__c` record sfid instead of the contact SFID (v1 API "MemberID") — the root cause of v1 committee-member deletes 404ing and leaving members on the committee / meeting invites (LFXV2-2673). Resolves the contact SFID from the `v1-objects` KV bucket; makes no calls to any other service. Writes are optimistic-concurrency guarded against the live sync-helper, idempotent, and safe to re-run.
+
+Run once after deploying the fix, to repair already-affected mappings. A dry-run pass is recommended first:
+
+```sh
+kubectl --context lfx-v2-prod -n v1-sync-helper apply -f manifests/backfill-committee-member-mappings-job.yaml
+```
+
+Add `--dry-run` to the manifest args, apply, inspect logs (`inspected`/`poisoned`/`fixed`/`already_ok`/`unresolved`/`malformed`/`tombstoned`/`conflicted` counts), then re-apply without it for the live run.
+
+Locally (with NATS port-forwarded):
+
+```sh
+lfx-v1-sync-helper --backfill-committee-member-mappings [--dry-run]
+```
+
 ## Architecture Diagrams
 
 Regarding the following sequence diagrams:

cmd/lfx-v1-sync-helper/backfill_committee_member_mappings.go

@@ -0,0 +1,210 @@
+// Copyright The Linux Foundation and each contributor to LFX.
+// SPDX-License-Identifier: MIT
+
+// The lfx-v1-sync-helper service.
+package main
+
+import "testing"
+
+// Sample SFIDs/UUIDs used across the classification and parsing tests below.
+const (
+	testProjectSFID   = "a0941000002wBz9AAE"
+	testCommitteeSFID = "a0941000002wCz9AAE"
+	testRecordSFID    = "51fde723-67df-4e0e-91c6-936d01d59559" // UUID: platform-community__c record sfid
+	testContactSFID   = "0031000001AbCdeAAB"                   // Salesforce ID: contact_name__c
+)
+
+func TestClassifyReverseMapping(t *testing.T) {
+	tests := []struct {
+		name    string
+		data    []byte
+		want    reverseMappingOutcome
+		wantRec string // expected recordSFID, when applicable
+	}{
+		{
+			name: "empty value is malformed, not tombstoned",
+			data: []byte(""),
+			want: reverseMappingMalformed,
+		},
+		{
+			name: "explicit tombstone marker",
+			data: []byte(tombstoneMarker),
+			want: reverseMappingTombstoned,
+		},
+		{
+			name: "legacy 3-field mapping with UUID third field needs a fix",
+			data: []byte(testProjectSFID + ":" + testCommitteeSFID + ":" + testRecordSFID),
+			want: reverseMappingNeedsFix,
+		},
+		{
+			name: "legacy 3-field mapping with contact SFID third field is already OK",
+			data: []byte(testProjectSFID + ":" + testCommitteeSFID + ":" + testContactSFID),
+			want: reverseMappingAlreadyOK,
+		},
+		{
+			name: "3-field mapping with empty third field is malformed",
+			data: []byte(testProjectSFID + ":" + testCommitteeSFID + ":"),
+			want: reverseMappingMalformed,
+		},
+		{
+			name: "value with no colons is malformed",
+			data: []byte("not-a-mapping"),
+			want: reverseMappingMalformed,
+		},
+		{
+			name: "value with one colon is malformed",
+			data: []byte(testProjectSFID + ":" + testCommitteeSFID),
+			want: reverseMappingMalformed,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := classifyReverseMapping(tt.data)
+			if got.outcome != tt.want {
+				t.Fatalf("classifyReverseMapping(%q).outcome = %v, want %v", tt.data, got.outcome, tt.want)
+			}
+		})
+	}
+}
+
+func TestClassifyReverseMapping_NeedsFixCarriesFields(t *testing.T) {
+	data := []byte(testProjectSFID + ":" + testCommitteeSFID + ":" + testRecordSFID)
+	got := classifyReverseMapping(data)
+
+	if got.outcome != reverseMappingNeedsFix {
+		t.Fatalf("outcome = %v, want %v", got.outcome, reverseMappingNeedsFix)
+	}
+	if got.projectSFID != testProjectSFID {
+		t.Errorf("projectSFID = %q, want %q", got.projectSFID, testProjectSFID)
+	}
+	if got.committeeSFID != testCommitteeSFID {
+		t.Errorf("committeeSFID = %q, want %q", got.committeeSFID, testCommitteeSFID)
+	}
+	if got.recordSFID != testRecordSFID {
+		t.Errorf("recordSFID = %q, want %q", got.recordSFID, testRecordSFID)
+	}
+}
+
+func TestBuildRepairedReverseMappingValue(t *testing.T) {
+	got := buildRepairedReverseMappingValue(testProjectSFID, testCommitteeSFID, testContactSFID)
+	want := testProjectSFID + ":" + testCommitteeSFID + ":" + testContactSFID
+	if got != want {
+		t.Fatalf("buildRepairedReverseMappingValue() = %q, want %q", got, want)
+	}
+
+	// The repaired value must itself classify as already OK and re-parse to the same
+	// contact SFID, so a repaired mapping is stable under a second backfill pass.
+	class := classifyReverseMapping([]byte(got))
+	if class.outcome != reverseMappingAlreadyOK {
+		t.Fatalf("classifyReverseMapping(repaired value).outcome = %v, want %v", class.outcome, reverseMappingAlreadyOK)
+	}
+
+	gotProject, gotCommittee, gotRecord, gotContact, ok := parseCommitteeMemberReverseMapping(got)
+	if !ok {
+		t.Fatalf("parseCommitteeMemberReverseMapping(%q) failed to parse", got)
+	}
+	if gotProject != testProjectSFID || gotCommittee != testCommitteeSFID || gotRecord != "" || gotContact != testContactSFID {
+		t.Fatalf("parseCommitteeMemberReverseMapping(%q) = (%q, %q, %q, %q), want (%q, %q, %q, %q)",
+			got, gotProject, gotCommittee, gotRecord, gotContact,
+			testProjectSFID, testCommitteeSFID, "", testContactSFID)
+	}
+}
+
+func TestIsUUID(t *testing.T) {
+	tests := []struct {
+		name string
+		s    string
+		want bool
+	}{
+		{"canonical lowercase UUID", "51fde723-67df-4e0e-91c6-936d01d59559", true},
+		{"canonical uppercase UUID", "51FDE723-67DF-4E0E-91C6-936D01D59559", true},
+		{"salesforce 18-char id", testContactSFID, false},
+		{"salesforce 15-char id", "a094100000TFAKE", false},
+		{"empty string", "", false},
+		{"wrong length", "51fde723-67df-4e0e-91c6-936d01d5955", false},
+		{"missing hyphens", "51fde72367df4e0e91c6936d01d59559", false},
+		{"hyphens in wrong place", "51fde7236-7df-4e0e-91c6-936d01d59559", false},
+		{"non-hex character", "51fde723-67df-4e0e-91c6-936d01d5955g", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isUUID(tt.s); got != tt.want {
+				t.Errorf("isUUID(%q) = %v, want %v", tt.s, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestParseCommitteeMemberReverseMapping(t *testing.T) {
+	tests := []struct {
+		name          string
+		s             string
+		wantOK        bool
+		wantProject   string
+		wantCommittee string
+		wantRecord    string
+		wantContact   string
+	}{
+		{
+			name:          "legacy 3-field mapping with record sfid",
+			s:             testProjectSFID + ":" + testCommitteeSFID + ":" + testRecordSFID,
+			wantOK:        true,
+			wantProject:   testProjectSFID,
+			wantCommittee: testCommitteeSFID,
+			wantRecord:    testRecordSFID,
+			wantContact:   "",
+		},
+		{
+			name:          "legacy 3-field mapping with contact sfid",
+			s:             testProjectSFID + ":" + testCommitteeSFID + ":" + testContactSFID,
+			wantOK:        true,
+			wantProject:   testProjectSFID,
+			wantCommittee: testCommitteeSFID,
+			wantRecord:    "",
+			wantContact:   testContactSFID,
+		},
+		{
+			name:   "too few fields",
+			s:      testProjectSFID + ":" + testCommitteeSFID,
+			wantOK: false,
+		},
+		{
+			// splitThreeParts only splits on the first two colons, so everything after
+			// the second colon (including further colons) folds into the third field;
+			// the result is neither a UUID nor a valid SFID, so it must be rejected as
+			// malformed rather than misclassified as a usable contact SFID.
+			name:   "extra colons fold into an invalid third field",
+			s:      testProjectSFID + ":" + testCommitteeSFID + ":" + testContactSFID + ":extra",
+			wantOK: false,
+		},
+		{
+			name:   "third field is neither a UUID nor a valid SFID",
+			s:      testProjectSFID + ":" + testCommitteeSFID + ":not-a-sfid",
+			wantOK: false,
+		},
+		{
+			name:   "no colons",
+			s:      "not-a-mapping",
+			wantOK: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotProject, gotCommittee, gotRecord, gotContact, gotOK := parseCommitteeMemberReverseMapping(tt.s)
+			if gotOK != tt.wantOK {
+				t.Fatalf("parseCommitteeMemberReverseMapping(%q) ok = %v, want %v", tt.s, gotOK, tt.wantOK)
+			}
+			if !tt.wantOK {
+				return
+			}
+			if gotProject != tt.wantProject || gotCommittee != tt.wantCommittee || gotRecord != tt.wantRecord || gotContact != tt.wantContact {
+				t.Fatalf("parseCommitteeMemberReverseMapping(%q) = (%q, %q, %q, %q), want (%q, %q, %q, %q)",
+					tt.s, gotProject, gotCommittee, gotRecord, gotContact,
+					tt.wantProject, tt.wantCommittee, tt.wantRecord, tt.wantContact)
+			}
+		})
+	}
+}

cmd/lfx-v1-sync-helper/backfill_committee_member_mappings_test.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"strings"
 
+	sfidvalidator "github.com/linuxfoundation/lfx-v1-sync-helper/internal/sfid"
 	committeeservice "github.com/linuxfoundation/lfx-v2-committee-service/gen/committee_service"
 	"github.com/nats-io/nats.go/jetstream"
 )
@@ -291,6 +292,14 @@ func handleCommitteeMemberDelete(ctx context.Context, key string, sfid string, v
 		logger.With(errKey, err, "committee_uid", committeeUID, "member_uid", memberUID).WarnContext(ctx, "failed to tombstone committee member UID mapping")
 	}
 
+	// Tombstone the record-sfid companion too, or it is left as a permanent live key:
+	// the v2-side delete indexer event that would otherwise clean it up either finds
+	// the reverse mapping already tombstoned above, or its own v1 delete call 404s
+	// since this v1-originated deletion already removed the member.
+	if err := tombstoneMapping(ctx, committeeMemberRecordSFIDKey(memberUID)); err != nil {
+		logger.With(errKey, err, "committee_uid", committeeUID, "member_uid", memberUID).WarnContext(ctx, "failed to tombstone committee member record sfid mapping")
+	}
+
 	logger.With("committee_uid", committeeUID, "member_uid", memberUID, "sfid", sfid, "key", key).InfoContext(ctx, "successfully deleted committee member")
 	return false
 }
@@ -520,6 +529,26 @@ func handleCommitteeMemberUpdate(ctx context.Context, key string, v1Data map[str
 		return
 	}
 
+	// Extract the contact SFID (contact_name__c). This is the identifier the v1
+	// project-service DELETE/PATCH .../committees/{c}/members/{MemberID} endpoints
+	// match on (SQL: WHERE contact_name__c = :memberID) — it is the v1 API's
+	// "MemberID", NOT the platform-community__c record sfid (the v1 API's "ID").
+	// Storing the record sfid in the reverse mapping causes v1 member deletes to
+	// 404 and leaves members on the committee / meeting invites (LFXV2-2673).
+	contactSFID, _ := v1Data["contact_name__c"].(string)
+	contactSFID = strings.TrimSpace(contactSFID)
+	if contactSFID == "" {
+		logger.With("sfid", sfid).WarnContext(ctx, "missing contact_name__c on committee member; not storing reverse mapping, v1 delete/update sync will not resolve the member")
+	} else if !sfidvalidator.IsValid(contactSFID) {
+		// parseCommitteeMemberReverseMapping rejects a non-UUID, non-SFID third field
+		// as malformed, so a mapping written with an invalid contactSFID here would
+		// immediately be treated as unusable by every reader. Drop it instead of
+		// publishing a value the readers will reject anyway.
+		logger.With("sfid", sfid, "contact_sfid", contactSFID).
+			WarnContext(ctx, "invalid contact_name__c on committee member; not storing reverse mapping, v1 delete/update sync will not resolve the member")
+		contactSFID = ""
+	}
+
 	// Check for blank email and skip with warning.
 	email := ""
 	if contactEmail, ok := v1Data["contactemail__c"].(string); ok && contactEmail != "" {
@@ -633,10 +662,38 @@ func handleCommitteeMemberUpdate(ctx context.Context, key string, v1Data map[str
 			projectSFID = projSFID
 		}
 
-		reverseMappingKey := fmt.Sprintf("committee_member.uid.%s", memberUID)
-		reverseMappingValue := fmt.Sprintf("%s:%s:%s", projectSFID, collaborationNameV1, sfid)
-		if _, err := mappingsKV.Put(ctx, reverseMappingKey, []byte(reverseMappingValue)); err != nil {
-			logger.With(errKey, err, "committee_uid", committeeUID, "member_uid", memberUID).WarnContext(ctx, "failed to store committee member reverse mapping")
+		// The third field must be the contact SFID (v1 API "MemberID"), which the v1
+		// delete/update endpoints match on — not the record sfid (LFXV2-2673). Without a
+		// contact SFID the mapping cannot drive a v1 delete/update at all, and would
+		// otherwise overwrite any existing usable mapping, so skip the write entirely.
+		//
+		// The reverse mapping is kept to exactly three fields (same field count as
+		// before this fix) so a rolling deploy never has an old pod (parsing this same
+		// key with the previous release's splitThreeParts-based reader) misparse a
+		// value written by a new pod. The record sfid needed to tombstone the forward
+		// mapping "committee_member.sfid.<sfid>" on delete is instead stored in the
+		// separate committeeMemberRecordSFIDKey — see parseCommitteeMemberReverseMapping
+		// in ingest_indexer.go.
+		if contactSFID == "" {
+			logger.With("sfid", sfid, "member_uid", memberUID).
+				WarnContext(ctx, "not storing committee member reverse mapping: missing contact_name__c")
+		} else {
+			// Write the record-sfid companion before the reverse mapping itself: once
+			// the reverse mapping is published with a contact SFID, the backfill
+			// classifies it as already OK and never revisits it, so a companion that
+			// failed to write here would never get repaired. If this write fails,
+			// skip the reverse mapping write too so it isn't left inconsistent —
+			// the next update event for this v1 record will retry both.
+			if _, err := mappingsKV.Put(ctx, committeeMemberRecordSFIDKey(memberUID), []byte(sfid)); err != nil {
+				logger.With(errKey, err, "committee_uid", committeeUID, "member_uid", memberUID).
+					WarnContext(ctx, "failed to store committee member record sfid mapping, skipping reverse mapping write to avoid publishing it without a companion")
+			} else {
+				reverseMappingKey := fmt.Sprintf("committee_member.uid.%s", memberUID)
+				reverseMappingValue := fmt.Sprintf("%s:%s:%s", projectSFID, collaborationNameV1, contactSFID)
+				if _, err := mappingsKV.Put(ctx, reverseMappingKey, []byte(reverseMappingValue)); err != nil {
+					logger.With(errKey, err, "committee_uid", committeeUID, "member_uid", memberUID).WarnContext(ctx, "failed to store committee member reverse mapping")
+				}
+			}
 		}
 	}