From 37ee83d30f5956286adf2248b71444d14d32735e Mon Sep 17 00:00:00 2001 From: Jordan Evans Date: Fri, 17 Jul 2026 09:06:33 -0700 Subject: [PATCH] Scope committee_member.sfid forward mapping key by committee The v2->v1 create path keyed the forward mapping on the v1 API MemberID (contact SFID) alone, which the same contact reuses across every committee it belongs to. Adding that contact to a second committee silently overwrote the first committee's forward mapping, orphaning it so a later v1-WAL delete couldn't find it. Scope the create-path key by committee SFID and update the delete path to look up the scoped key. Add a one-shot backfill to migrate existing flat keys to the scoped format and tombstone stale entries whose committee can no longer be resolved. Issue: LFXV2-2709 Co-Authored-By: Claude Opus 4.8 Signed-off-by: Jordan Evans --- README.md | 66 ++- ...kfill_committee_member_forward_mappings.go | 393 ++++++++++++++++++ ..._committee_member_forward_mappings_test.go | 95 +++++ cmd/lfx-v1-sync-helper/ingest_indexer.go | 112 ++++- cmd/lfx-v1-sync-helper/main.go | 34 +- ...committee-member-forward-mappings-job.yaml | 89 ++++ 6 files changed, 762 insertions(+), 27 deletions(-) create mode 100644 cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings.go create mode 100644 cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings_test.go create mode 100644 manifests/backfill-committee-member-forward-mappings-job.yaml diff --git a/README.md b/README.md index 8159faa..09d1e12 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,12 @@ Payload: ### Available Lookup Patterns -**Note**: While called "sfid", v1 committees and committee members actually store UUIDs in their "sfid" column, so references to `{*_sfid}` for these entities will contain UUIDs. +**Note**: While called "sfid", the v1 committee and committee-member *record* identifiers — used +in the single-token `committee.sfid.{v1_sfid}` and `committee_member.sfid.{v1_sfid}` keys below — +are actually UUIDs, not Salesforce-style IDs; the "sfid" naming is historical. This does **not** +apply to the committee-scoped committee-member key further down: its two tokens +(`{committee_sfid}`, `{member_sfid}`) are genuine v1 API Salesforce-style identifiers (the +committee's own SFID and the contact "MemberID"), not UUIDs — see LFXV2-2709. The following table shows the supported mapping key patterns and their expected response formats: @@ -84,7 +89,8 @@ The following table shows the supported mapping key patterns and their expected | v1→v2 | `committee.sfid.{v1_sfid}` | `committee.sfid.123e4567-e89b-12d3-a456-426614174003` | `{v2_uuid}` | Committee SFID to UUID | | v2→v1 | `committee.uid.{v2_uuid}` | `committee.uid.123e4567-e89b-12d3-a456-426614174001` | `{project_sfid}:{committee_sfid}` | Committee UUID to compound SFID | | **Committee Members** | -| v1→v2 | `committee_member.sfid.{v1_sfid}` | `committee_member.sfid.123e4567-e89b-12d3-a456-426614174004` | `{committee_uuid}:{member_uuid}` | Member SFID to compound UUID | +| v1→v2 | `committee_member.sfid.{v1_sfid}` | `committee_member.sfid.123e4567-e89b-12d3-a456-426614174004` | `{committee_uuid}:{member_uuid}` | Member record SFID to compound UUID | +| v1→v2 | `committee_member.sfid.{committee_sfid}.{member_sfid}` | `committee_member.sfid.a0941000002wBjEAAU.0034100000abcDEAAY` | `{committee_uuid}:{member_uuid}` | Member SFID to compound UUID, scoped by committee since the v1 API MemberID (contact SFID) is reused across committees for the same contact (LFXV2-2709) | | v2→v1 | `committee_member.uid.{v2_member_uuid}` | `committee_member.uid.123e4567-e89b-12d3-a456-426614174002` | `{project_sfid}:{committee_sfid}:{member_sfid}` | Member UUID to compound SFID | ### User SFID Lookup API @@ -194,6 +200,62 @@ Locally (with NATS port-forwarded): lfx-v1-sync-helper --backfill-committee-member-mappings [--dry-run] ``` +**Committee-member forward-mapping migration (`--backfill-committee-member-forward-mappings`):** + +Migrates committee-member forward mappings written by the v2→v1 create path +(`committee_member.sfid.{v1_sfid}`, keyed only on the contact SFID) to the committee-scoped +format `committee_member.sfid.{committee_sfid}.{member_sfid}` — the fix for LFXV2-2709, where an +unscoped key let MemberID reuse across committees silently overwrite an earlier committee's +mapping. Skips v1-ingest forward keys (record sfid, a UUID) untouched. Because the collision +already collapsed colliding committees' entries into a single flat key before this fix shipped, +only the surviving (last-write) entry per contact can be migrated — this is cleanup, not data +recovery. Writes are optimistic-concurrency guarded against the live sync-helper, idempotent, and +safe to re-run. + +**Rollout note:** this release has two competing rollout hazards, and the operator must pick +which to trade against the other: + +- With the default single-replica `RollingUpdate` strategy, a new pod (writing the committee- + scoped key) can briefly run alongside an old pod (whose delete handling only checks the pre- + fix flat key), which can orphan a scoped mapping that this backfill's flat-key scan cannot + find. +- Temporarily switching the Deployment's `spec.strategy.type` to `Recreate` avoids that + overlap, but creates a short window with no subscriber. Indexer events + (`lfx.committee.*` / `lfx.committee_member.*`) are consumed via plain Core NATS + `QueueSubscribe` (fire-and-forget, no replay), so any event published during that gap is + silently dropped — a subsequent v2→v1 create/update/delete could be lost. + +Neither option is strictly safer than the other; pick based on current traffic and how much +each failure mode matters for the environment. The `Recreate` cutover is the recommended +default when it can be performed during a low-traffic maintenance window, since dropping a +handful of events during a short gap is easier to reconcile (via a follow-up sync or manual +touch) than silently orphaned scoped mappings that no existing backfill can detect. In a +non-quiescent environment, prefer the default `RollingUpdate` and accept the (narrow) overlap +window instead. A durable JetStream consumer for these indexer subjects would close both +hazards but is outside the scope of this change. + +Run once after deploying the fix, to migrate 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-forward-mappings-job.yaml +``` + +Add `--dry-run` to the manifest args, apply, inspect logs +(`inspected`/`migrated`/`skipped_record_key`/`malformed`/`tombstoned`/`unresolved`/`conflicted` +counts). A Job's pod template is immutable, so delete the dry-run Job before removing +`--dry-run` and re-applying for the live run: + +```sh +kubectl --context lfx-v2-prod -n v1-sync-helper delete job backfill-committee-member-forward-mappings +``` + +Locally (with NATS port-forwarded): + +```sh +lfx-v1-sync-helper --backfill-committee-member-forward-mappings [--dry-run] +``` + ## Architecture Diagrams Regarding the following sequence diagrams: diff --git a/cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings.go b/cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings.go new file mode 100644 index 0000000..a7722ca --- /dev/null +++ b/cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings.go @@ -0,0 +1,393 @@ +// Copyright The Linux Foundation and each contributor to LFX. +// SPDX-License-Identifier: MIT + +// The lfx-v1-sync-helper service. +package main + +import ( + "context" + "fmt" + "strings" + + "github.com/linuxfoundation/lfx-v1-sync-helper/internal/sfid" + "github.com/nats-io/nats.go/jetstream" +) + +// backfillCommitteeMemberForwardMappingsResult summarizes a backfill run. +type backfillCommitteeMemberForwardMappingsResult struct { + inspected int + migrated int + skippedRecordKey int + malformed int + tombstoned int + unresolved int + conflicted int +} + +// backfillCommitteeMemberForwardMappings migrates committee-member forward mappings +// written by the v2->v1 create path (syncCommitteeMemberCreateToV1) from the flat key +// "committee_member.sfid." to the committee-scoped key +// committeeMemberForwardKey(committeeSFID, memberSFID). See LFXV2-2709. +// +// memberSFID there is the v1 API "MemberID" (the contact SFID), which the same contact +// reuses across every committee it belongs to. Before this fix, the create path's key +// carried no committee scope, so adding the same contact to a second committee silently +// overwrote the first committee's forward mapping under the same key — orphaning it, so +// a later v1-WAL delete for that first committee's member could no longer find it. +// +// This is deliberately kept separate from backfillCommitteeMemberMappings (the +// LFXV2-2673 reverse-mapping repair): different key, different collision, and keeping +// them apart leaves that reviewed backfill untouched. +// +// Enumeration uses ScanSubjectData (sequential GetMsg / next_by_subj), matching the +// reverse-mapping backfill (see its doc comment for why: KV_v1-mappings has ~34M +// sequences and ephemeral-consumer enumeration saturates the NATS server). The single +// trailing wildcard in forwardSubject only matches the flat, single-token key format; +// the newer two-token committee-scoped keys have an extra "." and are never matched, so +// they're never revisited by this backfill. +// +// Writes are guarded by a fresh KV read/CAS at write time, so a concurrent write by the +// live sync-helper is reported as conflicted rather than overwritten. +// +// Migrating also races a concurrent syncCommitteeMemberDeleteToV1 for the same member: +// this backfill checks the member's reverse mapping is live before creating the scoped +// key, then re-checks it immediately after, undoing the create (revision-guarded, not a +// blind write) if the membership died in between. syncCommitteeMemberDeleteToV1 +// tombstones that same reverse mapping before it checks the scoped key, so between the +// two of them most interleavings are caught by one side or the other — except the case +// where that reverse-mapping tombstone write itself fails; see that function's doc +// comment for why that's an accepted, pre-existing limitation rather than something +// this handshake closes. +// +// Known limitation: for a contact whose collision already overwrote an earlier +// committee's forward mapping before this fix shipped, only the surviving (last-write) +// entry is present to migrate — the orphaned committee's original mapping is gone. This +// backfill is cleanup (stop new key collisions, migrate what's left), not recovery of +// data already lost to the bug. +func backfillCommitteeMemberForwardMappings(ctx context.Context, dryRun bool) (backfillCommitteeMemberForwardMappingsResult, error) { + const ( + // kvMappingsStream is the JetStream stream backing the v1-mappings KV bucket. + kvMappingsStream = "KV_v1-mappings" + + // forwardSubject is the next_by_subj filter for flat committee-member forward + // mappings. A single-token wildcard matches only "committee_member.sfid." + // (one further dot-free token) — it does not match the newer two-token + // committee-scoped keys "committee_member.sfid..". + forwardSubject = "$KV.v1-mappings.committee_member.sfid.*" + + // forwardPrefix is stripped from the subject to recover the key token. + forwardPrefix = "$KV.v1-mappings.committee_member.sfid." + ) + + opTimeout := cfg.NATSFetchMaxWait + if opTimeout <= 0 { + opTimeout = defaultNATSFetchMaxWait + } + + subjectData, err := ScanSubjectData(ctx, jsContext, kvMappingsStream, forwardSubject, opTimeout) + if err != nil { + return backfillCommitteeMemberForwardMappingsResult{}, fmt.Errorf("failed to scan committee member forward mappings: %w", err) + } + + var res backfillCommitteeMemberForwardMappingsResult + for subject, data := range subjectData { + if !strings.HasPrefix(subject, forwardPrefix) { + continue + } + res.inspected++ + token := subject[len(forwardPrefix):] + flatKey := "committee_member.sfid." + token + + class := classifyForwardMapping(token, data) + switch class.outcome { + case forwardMappingTombstoned: + res.tombstoned++ + continue + case forwardMappingRecordKey: + // v1-ingest forward key (handlers_committees.go), keyed on the globally + // unique platform-community__c record sfid; not affected by LFXV2-2709. + res.skippedRecordKey++ + continue + case forwardMappingMalformed: + res.malformed++ + logger.With("member_sfid", token, "mapping_value", string(data)). + WarnContext(ctx, "skipping malformed committee member forward mapping") + continue + } + + // forwardMappingNeedsMigration: token is the contact SFID from a v2->v1 + // create-path key; data is "committeeUID:memberUID". + committeeUID, memberUID := class.committeeUID, class.memberUID + log := logger.With("member_sfid", token, "committee_uid", committeeUID, "member_uid", memberUID) + + committeeEntry, err := mappingsKV.Get(ctx, "committee.uid."+committeeUID) + if err != nil && err != jetstream.ErrKeyNotFound && err != jetstream.ErrKeyDeleted { + // A transient read failure (timeout, disconnect) is not the same as the + // committee genuinely being gone — treating it as unresolved would + // tombstone the sole surviving mapping over a blip. Abort the run instead. + return res, fmt.Errorf("failed to read committee reverse mapping committee.uid.%s: %w", committeeUID, err) + } + committeeSFID := "" + if err == nil && !isTombstonedMapping(committeeEntry.Value()) { + if _, csfid, ok := splitTwoParts(string(committeeEntry.Value())); ok && csfid != "" { + committeeSFID = csfid + } + } + + // Resolving the committee alone isn't enough: the member itself may have since + // been deleted (its "committee_member.uid." reverse mapping + // tombstoned) or reassigned, in which case migrating would resurrect a live + // scoped mapping for a membership that no longer exists. Require the reverse + // mapping to be live and to still agree with this committee and contact. + memberLive, err := memberReverseMappingLive(ctx, memberUID, committeeSFID, token) + if err != nil { + return res, err + } + + if committeeSFID == "" || !memberLive { + // Either the committee this stale mapping points at can no longer be + // resolved (deleted, tombstoned, or malformed reverse mapping), or the + // member reverse mapping is gone/tombstoned/no longer matches this + // committee and contact — either way there's no live membership to scope + // the key by, so the stale flat entry is just dead weight. Tombstone it + // rather than migrate a mapping nothing points at anymore. + log.WarnContext(ctx, "cannot verify live committee membership for stale committee member forward mapping, tombstoning instead of migrating") + if dryRun { + log.InfoContext(ctx, "[dry-run] would tombstone unresolvable committee member forward mapping") + res.unresolved++ + continue + } + conflicted, tombErr := tombstoneFlatForwardMappingIfUnchanged(ctx, flatKey, data) + if tombErr != nil { + return res, tombErr + } + if conflicted { + res.conflicted++ + continue + } + res.unresolved++ + continue + } + + scopedKey := committeeMemberForwardKey(committeeSFID, token) + log = log.With("committee_sfid", committeeSFID, "scoped_key", scopedKey) + + if dryRun { + // Read-only preview of the same check createScopedForwardMapping does live, + // so dry-run can report the conflicted outcome instead of always claiming + // migrated. + entry, getErr := mappingsKV.Get(ctx, scopedKey) + if getErr == nil && string(entry.Value()) != string(data) { + log.WarnContext(ctx, "[dry-run] committee member scoped forward mapping already holds an unexpected value, would skip") + res.conflicted++ + continue + } + if getErr != nil && getErr != jetstream.ErrKeyNotFound && getErr != jetstream.ErrKeyDeleted { + return res, fmt.Errorf("failed to read committee member scoped forward mapping %s: %w", scopedKey, getErr) + } + log.InfoContext(ctx, "[dry-run] would migrate committee member forward mapping to committee-scoped key") + res.migrated++ + continue + } + + conflicted, created, scopedRevision, err := createScopedForwardMapping(ctx, scopedKey, data) + if err != nil { + return res, err + } else if conflicted { + res.conflicted++ + log.WarnContext(ctx, "committee member scoped forward mapping already holds an unexpected value, skipping") + continue + } + + // syncCommitteeMemberDeleteToV1 tombstones the reverse mapping before checking + // this scoped key (see its doc comment), so a delete racing the check above can + // still land between that check and the Create just above. Re-verify now: if + // the membership died in between, undo the scoped key we just created rather + // than leak it. The undo is revision-guarded against scopedRevision (the exact + // entry we just wrote), not a blind Put, so an unrelated write racing in + // afterward — e.g. the live create path recreating this same key for a new + // membership — causes a conflict here instead of being clobbered. + // + // Only do this when created is true: if createScopedForwardMapping instead + // observed a pre-existing equal value, that write isn't ours to undo — it may + // belong to a live create still finishing (e.g. it wrote the scoped key but + // hasn't published the reverse mapping yet), and undoing by revision would + // clobber that legitimate write, not ours. + if created { + stillLive, err := memberReverseMappingLive(ctx, memberUID, committeeSFID, token) + if err != nil { + // We don't know whether the reverse mapping is still live, so leaving + // the scoped key we just created in place risks orphaning it exactly + // like the case below. Best-effort roll it back before aborting: the + // revision guard means this only removes it if it's still exactly + // what we wrote. If the rollback itself fails for a reason other than + // a revision mismatch, we genuinely don't know whether it succeeded — + // log that ambiguity explicitly rather than swallowing it. + if _, rbErr := mappingsKV.Update(ctx, scopedKey, []byte(tombstoneMarker), scopedRevision); rbErr != nil && !isRevisionMismatchError(rbErr) { + log.With(errKey, rbErr).WarnContext(ctx, "failed to roll back committee member scoped forward mapping after reverse-mapping recheck error, cleanup outcome unknown") + } + return res, err + } + if !stillLive { + if _, err := mappingsKV.Update(ctx, scopedKey, []byte(tombstoneMarker), scopedRevision); err != nil { + if !isRevisionMismatchError(err) { + return res, fmt.Errorf("failed to tombstone committee member scoped forward mapping %s after losing the reverse-mapping race: %w", scopedKey, err) + } + log.WarnContext(ctx, "committee member scoped forward mapping changed since it was created, leaving the newer write in place") + } + res.conflicted++ + log.WarnContext(ctx, "member reverse mapping changed since scoped key was created, undoing migration") + continue + } + } + + conflicted, tombErr := tombstoneFlatForwardMappingIfUnchanged(ctx, flatKey, data) + if tombErr != nil { + return res, tombErr + } + if conflicted { + res.conflicted++ + log.WarnContext(ctx, "committee member flat forward mapping changed since scan, skipping tombstone") + continue + } + + log.InfoContext(ctx, "migrated committee member forward mapping to committee-scoped key") + res.migrated++ + } + + return res, nil +} + +// memberReverseMappingLive reports whether the "committee_member.uid." +// reverse mapping is live (not absent, not tombstoned) and still agrees with +// committeeSFID and contactSFID. Used both before creating a scoped forward mapping and +// to re-verify immediately after, since syncCommitteeMemberDeleteToV1 can tombstone this +// same reverse mapping concurrently — see that function's doc comment for the ordering +// this depends on. +func memberReverseMappingLive(ctx context.Context, memberUID, committeeSFID, contactSFID string) (bool, error) { + entry, err := mappingsKV.Get(ctx, "committee_member.uid."+memberUID) + if err != nil { + if err == jetstream.ErrKeyNotFound || err == jetstream.ErrKeyDeleted { + return false, nil + } + return false, fmt.Errorf("failed to read committee member reverse mapping committee_member.uid.%s: %w", memberUID, err) + } + if isTombstonedMapping(entry.Value()) { + return false, nil + } + _, mCommitteeSFID, _, mContactSFID, ok := parseCommitteeMemberReverseMapping(string(entry.Value())) + return ok && mCommitteeSFID == committeeSFID && mContactSFID == contactSFID, nil +} + +// createScopedForwardMapping writes value under scopedKey unless it's already present +// with that exact value (idempotent re-run) or holds something else (conflict, don't +// clobber). Returns conflicted=true if the caller should count this as a skip rather +// than treat it as written. created reports whether this call is the one that actually +// wrote value via Create — false when an equal value was merely observed already +// present (e.g. a prior run, or the live create path, wrote it). The caller must use +// created (not just conflicted) to decide whether it owns the write: only a value this +// call created may be undone by revision-guarded Update later, since an observed +// pre-existing value might belong to a live write still in progress and undoing it by +// revision would clobber that legitimate write rather than the backfill's own. +func createScopedForwardMapping(ctx context.Context, scopedKey string, value []byte) (conflicted, created bool, revision uint64, err error) { + entry, getErr := mappingsKV.Get(ctx, scopedKey) + switch { + case getErr == nil && string(entry.Value()) == string(value): + // Already migrated (e.g. a prior run crashed after this write but before + // tombstoning the flat key, or the live create path wrote it directly) — not + // this call's write, so created stays false. + return false, false, entry.Revision(), nil + case getErr == nil: + // Holds something else (tombstoned, or a different committee/member pairing) — + // don't overwrite a value we don't understand. + return true, false, 0, nil + case getErr != jetstream.ErrKeyNotFound && getErr != jetstream.ErrKeyDeleted: + return false, false, 0, fmt.Errorf("failed to read committee member scoped forward mapping %s: %w", scopedKey, getErr) + } + + rev, err := mappingsKV.Create(ctx, scopedKey, value) + if err != nil { + if isRevisionMismatchError(err) || err == jetstream.ErrKeyExists { + return true, false, 0, nil + } + return false, false, 0, fmt.Errorf("failed to create committee member scoped forward mapping %s: %w", scopedKey, err) + } + return false, true, rev, nil +} + +// tombstoneFlatForwardMappingIfUnchanged revision-guards a tombstone write for a flat +// committee_member.sfid. key: it re-reads the key and only tombstones it if the +// value still matches what the scan observed, treating a value that has since changed +// (or a key that's already gone) as a conflict rather than an error, since the live +// sync-helper or another backfill run may have already handled it. +func tombstoneFlatForwardMappingIfUnchanged(ctx context.Context, flatKey string, expectedValue []byte) (conflicted bool, err error) { + entry, getErr := mappingsKV.Get(ctx, flatKey) + if getErr != nil { + if getErr == jetstream.ErrKeyNotFound || getErr == jetstream.ErrKeyDeleted { + return true, nil + } + return false, fmt.Errorf("failed to re-read committee member forward mapping %s: %w", flatKey, getErr) + } + if string(entry.Value()) != string(expectedValue) { + return true, nil + } + if _, err := mappingsKV.Update(ctx, flatKey, []byte(tombstoneMarker), entry.Revision()); err != nil { + if isRevisionMismatchError(err) { + return true, nil + } + return false, fmt.Errorf("failed to tombstone committee member forward mapping %s: %w", flatKey, err) + } + return false, nil +} + +// forwardMappingOutcome is the pre-lookup triage outcome for a scanned flat +// committee-member forward mapping entry, computed without any NATS access so it can be +// unit tested in isolation. +type forwardMappingOutcome string + +const ( + forwardMappingTombstoned forwardMappingOutcome = "tombstoned" + forwardMappingRecordKey forwardMappingOutcome = "recordKey" + forwardMappingMalformed forwardMappingOutcome = "malformed" + forwardMappingNeedsMigration forwardMappingOutcome = "needsMigration" +) + +// forwardMappingClassification is the result of classifyForwardMapping. +type forwardMappingClassification struct { + outcome forwardMappingOutcome + // committeeUID and memberUID are only populated when outcome is + // forwardMappingNeedsMigration. + committeeUID string + memberUID string +} + +// classifyForwardMapping triages a scanned flat committee-member forward mapping +// (key token and value) into one of four outcomes, without performing any NATS access: +// +// - forwardMappingTombstoned: explicitly tombstoned value (tombstoneMarker). +// - forwardMappingRecordKey: the key token is a platform-community__c record sfid +// (a UUID) — the v1-ingest forward key, globally unique, unaffected by LFXV2-2709. +// - forwardMappingMalformed: token isn't a UUID or a valid SFID, or the value isn't +// parseable as "committeeUID:memberUID". +// - forwardMappingNeedsMigration: token is a contact SFID from the v2->v1 create +// path; the caller must resolve the committee SFID to migrate it. +func classifyForwardMapping(token string, data []byte) forwardMappingClassification { + if isTombstonedMapping(data) { + return forwardMappingClassification{outcome: forwardMappingTombstoned} + } + if isUUID(token) { + return forwardMappingClassification{outcome: forwardMappingRecordKey} + } + if !sfid.IsValid(token) { + return forwardMappingClassification{outcome: forwardMappingMalformed} + } + committeeUID, memberUID, ok := splitTwoParts(string(data)) + if !ok || committeeUID == "" || memberUID == "" { + return forwardMappingClassification{outcome: forwardMappingMalformed} + } + return forwardMappingClassification{ + outcome: forwardMappingNeedsMigration, + committeeUID: committeeUID, + memberUID: memberUID, + } +} diff --git a/cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings_test.go b/cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings_test.go new file mode 100644 index 0000000..9f147f6 --- /dev/null +++ b/cmd/lfx-v1-sync-helper/backfill_committee_member_forward_mappings_test.go @@ -0,0 +1,95 @@ +// Copyright The Linux Foundation and each contributor to LFX. +// SPDX-License-Identifier: MIT + +// The lfx-v1-sync-helper service. +package main + +import "testing" + +// Sample UUIDs/SFIDs used across the classification tests below. +const ( + testCommitteeUID = "123e4567-e89b-12d3-a456-426614174001" + testMemberUID = "123e4567-e89b-12d3-a456-426614174002" +) + +func TestClassifyForwardMapping(t *testing.T) { + tests := []struct { + name string + token string + data []byte + want forwardMappingOutcome + wantCommitteeUID string + wantMemberUID string + }{ + { + name: "explicit tombstone marker", + token: testContactSFID, + data: []byte(tombstoneMarker), + want: forwardMappingTombstoned, + }, + { + name: "UUID token is a v1-ingest record sfid key, left untouched", + token: testRecordSFID, + data: []byte(testCommitteeUID + ":" + testMemberUID), + want: forwardMappingRecordKey, + }, + { + name: "non-SFID, non-UUID token is malformed", + token: "not-a-sfid", + data: []byte(testCommitteeUID + ":" + testMemberUID), + want: forwardMappingMalformed, + }, + { + name: "contact SFID token with no colon in value is malformed", + token: testContactSFID, + data: []byte("not-a-mapping"), + want: forwardMappingMalformed, + }, + { + name: "contact SFID token with empty committeeUID is malformed", + token: testContactSFID, + data: []byte(":" + testMemberUID), + want: forwardMappingMalformed, + }, + { + name: "contact SFID token with empty memberUID is malformed", + token: testContactSFID, + data: []byte(testCommitteeUID + ":"), + want: forwardMappingMalformed, + }, + { + name: "contact SFID token with valid committeeUID:memberUID needs migration", + token: testContactSFID, + data: []byte(testCommitteeUID + ":" + testMemberUID), + want: forwardMappingNeedsMigration, + wantCommitteeUID: testCommitteeUID, + wantMemberUID: testMemberUID, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := classifyForwardMapping(tt.token, tt.data) + if got.outcome != tt.want { + t.Fatalf("classifyForwardMapping(%q, %q).outcome = %v, want %v", tt.token, tt.data, got.outcome, tt.want) + } + if tt.want != forwardMappingNeedsMigration { + return + } + if got.committeeUID != tt.wantCommitteeUID { + t.Errorf("committeeUID = %q, want %q", got.committeeUID, tt.wantCommitteeUID) + } + if got.memberUID != tt.wantMemberUID { + t.Errorf("memberUID = %q, want %q", got.memberUID, tt.wantMemberUID) + } + }) + } +} + +func TestCommitteeMemberForwardKey(t *testing.T) { + got := committeeMemberForwardKey(testCommitteeSFID, testContactSFID) + want := "committee_member.sfid." + testCommitteeSFID + "." + testContactSFID + if got != want { + t.Fatalf("committeeMemberForwardKey(%q, %q) = %q, want %q", testCommitteeSFID, testContactSFID, got, want) + } +} diff --git a/cmd/lfx-v1-sync-helper/ingest_indexer.go b/cmd/lfx-v1-sync-helper/ingest_indexer.go index e2c21cd..ead303f 100644 --- a/cmd/lfx-v1-sync-helper/ingest_indexer.go +++ b/cmd/lfx-v1-sync-helper/ingest_indexer.go @@ -12,6 +12,7 @@ import ( "github.com/linuxfoundation/lfx-v1-sync-helper/internal/sfid" nats "github.com/nats-io/nats.go" + "github.com/nats-io/nats.go/jetstream" ) // indexingEvent mirrors the IndexingEvent published by the indexer service after a @@ -192,6 +193,15 @@ func committeeMemberIndexerEventHandler(msg *nats.Msg) { syncCommitteeMemberUpdateToV1(ctx, event.ObjectID, projectSFID, committeeSFID, memberSFID, body.Data) case "deleted": + // Known limitation: if handleCommitteeMemberDelete (handlers_committees.go, the + // v1-WAL-triggered delete path) already tombstoned this same reverse mapping + // before this v2 delete event arrives, parseCommitteeMemberReverseMapping below + // fails on the tombstone marker and this handler bails without ever calling + // syncCommitteeMemberDeleteToV1 — silently skipping the committee-scoped + // forward-key tombstone. This is a pre-existing gap between the two + // independent delete paths (not introduced by the committee-scoped key itself), + // but the new scoped key is now exposed to it same as every other mapping this + // handler is responsible for cleaning up. reverseMappingKey := "committee_member.uid." + event.ObjectID entry, err := mappingsKV.Get(ctx, reverseMappingKey) if err != nil { @@ -420,9 +430,12 @@ func syncCommitteeMemberCreateToV1(ctx context.Context, memberUID, committeeUID, } // Store forward mapping (v1 SFID -> committeeUID:memberUID) and reverse mapping (v2 UID -> projectSFID:committeeSFID:memberSFID). + // The forward mapping is scoped by committee SFID (see committeeMemberForwardKey) because + // memberSFID is the v1 API "MemberID" (contact SFID), which the same contact reuses across + // every committee it belongs to — an unscoped key would collide across committees (LFXV2-2709). memberSFID := result.MemberID forwardMappingValue := committeeUID + ":" + memberUID - if _, err := mappingsKV.Put(ctx, "committee_member.sfid."+memberSFID, []byte(forwardMappingValue)); err != nil { + if _, err := mappingsKV.Put(ctx, committeeMemberForwardKey(committeeSFID, memberSFID), []byte(forwardMappingValue)); err != nil { log.With(errKey, err, "member_sfid", memberSFID). WarnContext(ctx, "failed to store committee member forward mapping after v1 create") } @@ -495,14 +508,33 @@ func syncCommitteeMemberUpdateToV1(ctx context.Context, memberUID, projectSFID, // already-removed v2 member. // // recordSFID is empty both for members created via the v2->v1 create path -// (syncCommitteeMemberCreateToV1 — those key their forward mapping on the contact -// SFID itself, there being no distinct record sfid) and for v1-originated members -// whose record-sfid companion key is missing or unreadable. Since those two cases -// are indistinguishable from recordSFID alone, memberSFID is only used as the -// forward-tombstone key when the "committee_member.sfid." mapping -// actually exists and its value points back at this same memberUID — otherwise the -// forward tombstone is skipped rather than risk tombstoning an unrelated mapping -// (or a no-op) while the real v1-originated forward mapping stays live. +// (syncCommitteeMemberCreateToV1 — those key their forward mapping on +// committeeMemberForwardKey(committeeSFID, memberSFID), scoped by committee since the +// contact SFID alone is reused across every committee a contact belongs to) and for +// v1-originated members whose record-sfid companion key is missing or unreadable. +// +// The record-sfid key and the committee-scoped key are checked and tombstoned +// independently, not as alternatives: a member created via the v2->v1 path can later +// gain a recordSFID companion once its v1 WAL record is ingested, without its original +// scoped forward mapping ever being removed — leaving both live would leak the scoped +// key. The scoped key is only tombstoned when it actually exists and its value points +// back at this same memberUID, to avoid tombstoning an unrelated mapping. +// +// The reverse mapping "committee_member.uid." is tombstoned before the +// scoped-key check runs (rather than after, as in the forward-key case) so that the +// backfillCommitteeMemberForwardMappings migration — which re-checks that same reverse +// mapping before creating a scoped key, and re-checks it again afterward to catch a +// concurrent delete — observes a consistent ordering: this delete's reverse tombstone +// either lands before the migration's first check (migration skips entirely) or after +// its final recheck (migration undoes the scoped key it just created). See that +// function's doc comment for the other half of this handshake. +// +// This ordering guarantee holds only if the reverse-mapping tombstone write below +// actually succeeds; like every other KV write in this function it is fire-and-forget +// (logged on failure, not retried), since this indexer has no message-level retry or +// nack mechanism. A failed reverse tombstone can still leave a live scoped mapping for +// a deleted membership — a pre-existing limitation of this event-driven pipeline, not +// specific to this handshake. func syncCommitteeMemberDeleteToV1(ctx context.Context, memberUID, projectSFID, committeeSFID, memberSFID, recordSFID string) { log := logger.With("member_uid", memberUID, "project_sfid", projectSFID, "committee_sfid", committeeSFID, "member_sfid", memberSFID, "record_sfid", recordSFID) @@ -511,27 +543,56 @@ func syncCommitteeMemberDeleteToV1(ctx context.Context, memberUID, projectSFID, return } - forwardSFID := recordSFID - if forwardSFID == "" { - if entry, err := mappingsKV.Get(ctx, "committee_member.sfid."+memberSFID); err == nil && !isTombstonedMapping(entry.Value()) { - if _, ownerUID, ok := splitTwoParts(string(entry.Value())); ok && ownerUID == memberUID { - forwardSFID = memberSFID + if err := tombstoneMapping(ctx, "committee_member.uid."+memberUID); err != nil { + log.With(errKey, err).WarnContext(ctx, "failed to tombstone committee member reverse mapping after v1 delete") + } + + // The two forward-mapping keys are not mutually exclusive: a member created via the + // v2->v1 create path (committee-scoped key) can later gain a recordSFID companion + // once its v1 WAL record is ingested, without the original scoped key ever being + // removed. Both are checked and tombstoned independently so neither is leaked. + tombstonedAny := false + if recordSFID != "" { + if err := tombstoneMapping(ctx, "committee_member.sfid."+recordSFID); err != nil { + log.With(errKey, err).WarnContext(ctx, "failed to tombstone committee member forward mapping after v1 delete") + } else { + tombstonedAny = true + } + } + scopedKey := committeeMemberForwardKey(committeeSFID, memberSFID) + entry, err := mappingsKV.Get(ctx, scopedKey) + switch { + case err == nil && !isTombstonedMapping(entry.Value()): + if _, ownerUID, ok := splitTwoParts(string(entry.Value())); ok && ownerUID == memberUID { + // Revision-guarded: the ownership check above (Get, then compare ownerUID) + // is not atomic with this write, so a concurrent re-add of the same contact + // to the same committee could replace this key in between. Update against + // entry.Revision() turns that into a conflict instead of tombstoning the + // new mapping out from under the re-add. + if _, err := mappingsKV.Update(ctx, scopedKey, []byte(tombstoneMarker), entry.Revision()); err != nil { + if isRevisionMismatchError(err) { + log.WarnContext(ctx, "committee member scoped forward mapping changed since it was read, leaving the newer write in place") + } else { + log.With(errKey, err).WarnContext(ctx, "failed to tombstone committee member scoped forward mapping after v1 delete") + } + } else { + tombstonedAny = true } } + case err != nil && err != jetstream.ErrKeyNotFound && err != jetstream.ErrKeyDeleted: + // A transient read failure is not the same as the key genuinely being absent — + // log it distinctly so it's visible that the scoped key's fate is unknown, + // rather than silently treating it the same as "nothing to tombstone". + log.With(errKey, err).WarnContext(ctx, "failed to read committee member scoped forward mapping while attempting tombstone after v1 delete") } - if forwardSFID == "" { + if !tombstonedAny { log.WarnContext(ctx, "cannot determine committee member forward mapping key, skipping forward tombstone") - } else if err := tombstoneMapping(ctx, "committee_member.sfid."+forwardSFID); err != nil { - log.With(errKey, err).WarnContext(ctx, "failed to tombstone committee member forward mapping after v1 delete") } if recordSFID != "" { if err := tombstoneMapping(ctx, committeeMemberRecordSFIDKey(memberUID)); err != nil { log.With(errKey, err).WarnContext(ctx, "failed to tombstone committee member record sfid mapping after v1 delete") } } - if err := tombstoneMapping(ctx, "committee_member.uid."+memberUID); err != nil { - log.With(errKey, err).WarnContext(ctx, "failed to tombstone committee member reverse mapping after v1 delete") - } log.InfoContext(ctx, "successfully deleted committee member in v1 from indexer event") } @@ -625,6 +686,17 @@ func parseCommitteeMemberReverseMapping(s string) (projectSFID, committeeSFID, r return projectSFID, committeeSFID, "", third, true } +// committeeMemberForwardKey returns the forward-mapping key for a committee member +// created via the v2->v1 create path (syncCommitteeMemberCreateToV1). It is scoped by +// committee SFID because memberSFID is the v1 API "MemberID" (the contact SFID), which +// the same contact reuses across every committee it belongs to — an unscoped +// "committee_member.sfid." key would collide across committees (LFXV2-2709). +// Distinct from the v1-ingest forward key (handlers_committees.go), which keys on the +// globally unique platform-community__c record sfid and stays flat. +func committeeMemberForwardKey(committeeSFID, memberSFID string) string { + return "committee_member.sfid." + committeeSFID + "." + memberSFID +} + // committeeMemberRecordSFIDKey returns the mapping key that stores the // platform-community__c record sfid for a v2 committee member UID. Kept as a // separate key (rather than a fourth field on the reverse mapping) so the reverse diff --git a/cmd/lfx-v1-sync-helper/main.go b/cmd/lfx-v1-sync-helper/main.go index 42c4870..dcdb44c 100644 --- a/cmd/lfx-v1-sync-helper/main.go +++ b/cmd/lfx-v1-sync-helper/main.go @@ -66,6 +66,7 @@ func main() { var doBackfillProfiles = flag.Bool("backfill-profiles", false, "backfill v1 profile fields to Auth0 user_metadata, then exit") var doBackfillWorkspaces = flag.Bool("backfill-workspaces", false, "backfill legacy workspaces into v2 member-service, then exit") var doBackfillCommitteeMemberMappings = flag.Bool("backfill-committee-member-mappings", false, "repair committee-member reverse mappings that store the record sfid instead of the contact SFID, then exit") + var doBackfillCommitteeMemberForwardMappings = flag.Bool("backfill-committee-member-forward-mappings", false, "migrate committee-member forward mappings from the v2->v1 create path to committee-scoped keys, then exit") var syncUser = flag.String("sync-user", "", "sync profile and alternate emails for a single user by username, then exit") var dryRun = flag.Bool("dry-run", false, "log changes without writing them (applicable with --backfill-* and --sync-user)") var backfillLimit = flag.Int("limit", 1000, "maximum number of users to process per backfill run (applicable with --backfill-alternate-emails and --backfill-profiles)") @@ -78,13 +79,13 @@ func main() { // Enforce mutual exclusion across all one-shot flags. oneShotCount := 0 - for _, b := range []bool{*doBackfillACSProject, *doBackfillACSOrg, *doBackfillWorkspaces, *doBackfillAltEmails, *doBackfillProfiles, *syncUser != "", *doRebuildUserIndexes, *doBackfillCommitteeMemberMappings} { + for _, b := range []bool{*doBackfillACSProject, *doBackfillACSOrg, *doBackfillWorkspaces, *doBackfillAltEmails, *doBackfillProfiles, *syncUser != "", *doRebuildUserIndexes, *doBackfillCommitteeMemberMappings, *doBackfillCommitteeMemberForwardMappings} { if b { oneShotCount++ } } if oneShotCount > 1 { - fmt.Fprintln(os.Stderr, "error: --backfill-acs-project, --backfill-acs-org, --backfill-workspaces, --backfill-alternate-emails, --backfill-profiles, --backfill-committee-member-mappings, --sync-user, and --rebuild-user-secondary-indexes are mutually exclusive") + fmt.Fprintln(os.Stderr, "error: --backfill-acs-project, --backfill-acs-org, --backfill-workspaces, --backfill-alternate-emails, --backfill-profiles, --backfill-committee-member-mappings, --backfill-committee-member-forward-mappings, --sync-user, and --rebuild-user-secondary-indexes are mutually exclusive") os.Exit(2) } @@ -92,11 +93,12 @@ func main() { logger = slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{})) slog.SetDefault(logger) - // --rebuild-user-secondary-indexes and --backfill-committee-member-mappings - // only need NATS KV; skip full config and API client init. + // --rebuild-user-secondary-indexes, --backfill-committee-member-mappings, and + // --backfill-committee-member-forward-mappings only need NATS KV; skip full config + // and API client init. // --backfill-acs-project and --backfill-acs-org require full config and API client init. var err error - if *doRebuildUserIndexes || *doBackfillCommitteeMemberMappings { + if *doRebuildUserIndexes || *doBackfillCommitteeMemberMappings || *doBackfillCommitteeMemberForwardMappings { cfg = LoadReindexConfig() } else { cfg, err = LoadConfig() @@ -365,6 +367,28 @@ func main() { os.Exit(0) } + // Handle --backfill-committee-member-forward-mappings flag: migrate committee-member + // forward mappings written by the v2->v1 create path to committee-scoped keys + // (LFXV2-2709), then exit. + if *doBackfillCommitteeMemberForwardMappings { + logger.With("dry_run", *dryRun).Info("starting committee-member forward-mapping backfill") + res, err := backfillCommitteeMemberForwardMappings(ctx, *dryRun) + if err != nil { + logger.With(errKey, err).Error("error during committee-member forward-mapping backfill") + os.Exit(1) + } + logger.With( + "inspected", res.inspected, + "migrated", res.migrated, + "skipped_record_key", res.skippedRecordKey, + "malformed", res.malformed, + "tombstoned", res.tombstoned, + "unresolved", res.unresolved, + "conflicted", res.conflicted, + ).Info("committee-member forward-mapping backfill completed successfully") + os.Exit(0) + } + // Initialize the distributed sync singleton backed by the mappings KV bucket. distributedSync = newKVMappingLocker(mappingsKV, withLockerOptionMaxRetries(mappingLockRetryAttempts), diff --git a/manifests/backfill-committee-member-forward-mappings-job.yaml b/manifests/backfill-committee-member-forward-mappings-job.yaml new file mode 100644 index 0000000..7f705c5 --- /dev/null +++ b/manifests/backfill-committee-member-forward-mappings-job.yaml @@ -0,0 +1,89 @@ +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +# +# One-shot Job to migrate committee-member forward mappings in the v1-mappings +# NATS KV bucket that were written by the v2->v1 create path under the flat key +# "committee_member.sfid." — memberSFID there is the v1 API +# "MemberID" (the contact SFID), which the same contact reuses across every +# committee it belongs to, so the unscoped key let a second-committee create +# silently overwrite an earlier committee's mapping (LFXV2-2709). Migrates each +# resolvable entry to the committee-scoped key +# "committee_member.sfid.." and tombstones the old +# flat key. Leaves v1-ingest forward keys (record sfid, a UUID) untouched. +# +# One-shot: this manifest is applied manually via kubectl apply — it is NOT +# managed by argocd and runs on demand only, after the fix in this same +# release has been deployed (to migrate already-affected mappings). +# +# Known limitation: mappings already collapsed by the collision (two +# committees' entries overwritten down to one surviving key) can only migrate +# the surviving entry — this is cleanup, not recovery of data already lost to +# the bug. +# +# Dry run: to preview changes without writing, add "--dry-run" to args below +# before applying. Remove it for the live run. +# +# Credentials: --backfill-committee-member-forward-mappings uses +# LoadReindexConfig (main.go), which only reads NATS_URL / NATS_FETCH_MAX_WAIT +# and never initializes the Heimdall or Auth0 clients — those clients are +# simply skipped for this one-shot path, not because the process exits before +# anything starts (the HTTP health server still starts normally). So this +# manifest intentionally carries no secretKeyRef entries; do not add +# Heimdall/Auth0 credentials here, this job cannot use them. +# +# Deploy with: +# kubectl --context lfx-v2-prod -n v1-sync-helper apply -f manifests/backfill-committee-member-forward-mappings-job.yaml +# +# Monitor: +# kubectl --context lfx-v2-prod -n v1-sync-helper logs -f --timestamps=true job/backfill-committee-member-forward-mappings +# +# Cancel: +# kubectl --context lfx-v2-prod -n v1-sync-helper delete job backfill-committee-member-forward-mappings +# +# Clean up (auto-cleans after 1h via ttlSecondsAfterFinished): +# kubectl --context lfx-v2-prod -n v1-sync-helper delete job backfill-committee-member-forward-mappings +--- +apiVersion: batch/v1 +kind: Job +metadata: + name: backfill-committee-member-forward-mappings + namespace: v1-sync-helper + labels: + app.kubernetes.io/name: lfx-v1-sync-helper + app.kubernetes.io/component: backfill +spec: + ttlSecondsAfterFinished: 3600 + backoffLimit: 0 + template: + metadata: + labels: + app.kubernetes.io/name: lfx-v1-sync-helper + app.kubernetes.io/component: backfill + spec: + restartPolicy: Never + serviceAccountName: v1-sync-helper-sa + containers: + - name: backfill + # XXX: Replace with the image SHA for the release you want to run against. + # Find the current prod image with: + # kubectl --context lfx-v2-prod -n v1-sync-helper get deployment \ + # -o jsonpath='{.items[0].spec.template.spec.containers[0].image}' + image: ghcr.io/linuxfoundation/lfx-v1-sync-helper/lfx-v1-sync-helper:latest + imagePullPolicy: IfNotPresent + args: ["--backfill-committee-member-forward-mappings"] + env: + - name: NATS_URL + value: "nats://lfx-platform-nats.lfx.svc.cluster.local:4222" + # Optional: raise if logs show scan-timeout warnings. Defaults to 2m + # (defaultNATSFetchMaxWait) if unset. + # - name: NATS_FETCH_MAX_WAIT + # value: "2m" + resources: + requests: + memory: "128Mi" + cpu: "10m" + limits: + memory: "512Mi" + cpu: "200m" + securityContext: + allowPrivilegeEscalation: false