fix(PROD-2211): robust handling of model "Unable to save the model" failures

src/core/logs.ts

@@ -754,8 +754,12 @@ export class Logs {
     error: (payload: any, apiError: any, targetGuid?: string) => {
       const itemName = payload?.referenceName || payload?.displayName || `Model ${payload?.id || "Unknown"}`;
       const baseMessage = apiError?.message || "Unknown error";
-      const responseData = apiError?.response?.data ?? apiError?.response?.body ?? apiError?.data;
-      const responseStatus = apiError?.response?.status ?? apiError?.status;
+      // The management SDK wraps the underlying axios error as `innerError`, so the real status/body
+      // live there — unwrap it (falling back to the top-level error) so the log captures them.
+      const inner = apiError?.innerError ?? apiError;
+      const responseData =
+        inner?.response?.data ?? inner?.response?.body ?? inner?.data ?? apiError?.response?.data ?? apiError?.data;
+      const responseStatus = inner?.response?.status ?? inner?.status ?? apiError?.response?.status ?? apiError?.status;
       const serverDetail = responseData
         ? ` | Server response (${responseStatus ?? "?"}): ${typeof responseData === "string" ? responseData : safeStringify(responseData)}`
         : "";

src/lib/pushers/model-pusher.ts

@@ -4,6 +4,48 @@ import { PusherResult, FailureDetail } from "../../types/sourceData";
 import { ModelMapper } from "lib/mappers/model-mapper";
 import { Logs } from "core/logs";
 
+/**
+ * Two Agility models can share a `referenceName` while differing in `contentDefinitionTypeID`
+ * (e.g. a Content List vs. a Module/Component named "PromoBanner"). A name-only lookup conflates
+ * them (the PROD-1505 / PROD-2211 type-blind-lookup family). This compares the model TYPE in
+ * addition to the name. `contentDefinitionTypeID` is present in the pulled model JSON but not on the
+ * SDK `Model` type, so we read it defensively; when either side lacks it (older pulls / fixtures) we
+ * fall back to name-only matching.
+ */
+function modelTypeMatches(a: mgmtApi.Model, b: mgmtApi.Model): boolean {
+  const aType = (a as any)?.contentDefinitionTypeID;
+  const bType = (b as any)?.contentDefinitionTypeID;
+  if (aType === undefined || aType === null || bType === undefined || bType === null) return true;
+  return aType === bType;
+}
+
+/**
+ * Re-query the target for a model matching (referenceName, contentDefinitionTypeID).
+ *
+ * Used after a `saveModel` rejection to detect a false-negative — the SDK rethrows any failure as
+ * "Unable to save the model." even when the API actually persisted the model (timeout-with-server-
+ * completion, response-parse race, etc.). Returns the matching target model, or null if the re-query
+ * fails or finds nothing. Matching by type avoids mistaking a same-name different-type collision for
+ * a recovered save.
+ */
+async function findTargetModelAfterSave(
+  sourceModel: mgmtApi.Model,
+  apiClient: mgmtApi.ApiClient,
+  targetGuid: string
+): Promise<mgmtApi.Model | null> {
+  try {
+    // includeDefaults + includeModules so both content models and module/component models are returned.
+    const targetModels = await apiClient.modelMethods.getContentModules(true, targetGuid, true);
+    return (
+      (targetModels || []).find(
+        (t) => t.referenceName === sourceModel.referenceName && modelTypeMatches(sourceModel, t)
+      ) || null
+    );
+  } catch {
+    return null;
+  }
+}
+
 /**
  * Simple change detection for models
  */
@@ -73,7 +115,8 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
     // Handle models that exist in target but have no mapping
     // This ensures downstream containers can find their model mappings
     const targetModelByReference = targetData.find(
-      (targetModel) => targetModel.referenceName === sourceModel.referenceName
+      (targetModel) =>
+        targetModel.referenceName === sourceModel.referenceName && modelTypeMatches(sourceModel, targetModel)
     );
     const existsInTargetWithoutMapping = !sourceMapping && targetModelByReference;
     if (existsInTargetWithoutMapping) {
@@ -102,6 +145,20 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
               `a rename or reassignment on the source. Stopping sync to avoid a partial push; review the model mappings and re-run. Please contact AgilityCMS Support to resolve this issue`
           );
         }
+
+        // PROD-2211: the model exists on the target by (referenceName, type) but has no mapping row
+        // (e.g. a prior create succeeded server-side yet was logged as a false-negative failure, or a
+        // model adopted by referenceName without a mapping ever being written). Previously this branch
+        // set `targetModel` but fell through every downstream condition (all gated on `sourceMapping`),
+        // so the model was silently dropped — not created, skipped, or failed — and the wedge never
+        // self-healed. Write the mapping now so downstream containers/content can translate and so
+        // re-syncs converge.
+        referenceMapper.addMapping(sourceModel, targetModel);
+        shouldSkip.push({
+          model: sourceModel,
+          reason: "Model already exists on target without a mapping; mapping row created.",
+        });
+        continue;
       }
     }
 
@@ -170,7 +227,11 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
       targetGuid[0],
       logger
     );
-    if (result) {
+    // PROD-2211: `updateExistingModel` returns the string "updated" | "failed". A bare `if (result)`
+    // treats "failed" as truthy, so failed field-updates were counted as successes — the run reported
+    // "N successful, 0 failed" with a green banner while models had genuinely failed. Compare
+    // explicitly so the summary, ERROR SUMMARY, and exit status reflect reality.
+    if (result === "updated") {
       successful++;
     } else {
       failed++;
@@ -219,6 +280,16 @@ const createNewModel = async (
     referenceMapper.addMapping(model, newModel);
     return "created";
   } catch (error: any) {
+    // PROD-2211: saveModel can reject even though the API created the model server-side. Before
+    // declaring failure — which leaves the mapping file without a row and wedges future syncs —
+    // re-query the target. If a model with the matching (referenceName, type) now exists, the stub
+    // create really succeeded: write the mapping and report created.
+    const recovered = await findTargetModelAfterSave(model, apiClient, targetGuid);
+    if (recovered) {
+      logger.model.created(model, "created", targetGuid);
+      referenceMapper.addMapping(model, recovered);
+      return "created";
+    }
     logger.model.error(model, error, targetGuid);
     return "failed";
   }
@@ -246,11 +317,16 @@ async function updateExistingModel(
     referenceMapper.addMapping(sourceModel, updatedModel);
     return "updated";
   } catch (error: any) {
-    const axiosErr = error?.innerError;
-    console.error(`[model-pusher] SAVE FAILED for ${sourceModel?.referenceName}:`);
-    console.error(`  message: ${error?.message}`);
-    console.error(`  status:  ${axiosErr?.response?.status ?? axiosErr?.status ?? "n/a"}`);
-    console.error(`  responseData: ${JSON.stringify(axiosErr?.response?.data ?? axiosErr?.data ?? null, null, 2)}`);
+    // PROD-2211: saveModel can reject even though the field update was persisted server-side. Re-query
+    // the target; treat it as a recovered update ONLY when the saved field set matches the source
+    // (field count). A genuine reject — e.g. a 404 "Definition for setting X not found" — leaves the
+    // fields unapplied, so the count won't match and it correctly stays a failure.
+    const recovered = await findTargetModelAfterSave(sourceModel, apiClient, targetGuid);
+    if (recovered && (recovered.fields?.length ?? 0) === (sourceModel.fields?.length ?? 0)) {
+      logger.model.updated(sourceModel, "updated", targetGuid);
+      referenceMapper.addMapping(sourceModel, recovered);
+      return "updated";
+    }
     logger.model.error(sourceModel, error, targetGuid);
     return "failed";
   }

src/lib/pushers/tests/model-pusher.test.ts

@@ -43,10 +43,11 @@ function makeModel(overrides: Record<string, any> = {}): any {
   };
 }
 
-function makeApiClient(saveModelImpl?: jest.Mock): any {
+function makeApiClient(saveModelImpl?: jest.Mock, getContentModulesImpl?: jest.Mock): any {
   return {
     modelMethods: {
       saveModel: saveModelImpl ?? jest.fn().mockResolvedValue(makeModel({ id: 999 })),
+      getContentModules: getContentModulesImpl ?? jest.fn().mockResolvedValue([]),
     },
   };
 }
@@ -202,3 +203,156 @@ describe("pushModels — source-side rename orphans a mapping and halts the sync
     expect(saveModel).not.toHaveBeenCalled();
   });
 });
+
+// ─── PROD-2211: honest failure reporting ──────────────────────────────────────
+
+describe("pushModels — failed update is reported as failed, not success (PROD-2211)", () => {
+  it("counts a model whose field-update genuinely fails as failed", async () => {
+    // Stub create succeeds; the follow-up field update is rejected (e.g. 404). Must be a failure.
+    const saveModel = jest
+      .fn()
+      .mockResolvedValueOnce(makeModel({ id: 55, referenceName: "FooterLinks" }))
+      .mockRejectedValue(new Error("Unable to save the model."));
+    // Re-query returns the stub (0 fields) — does NOT match the source field count, so no recovery.
+    const getContentModules = jest
+      .fn()
+      .mockResolvedValue([makeModel({ id: 55, referenceName: "FooterLinks", fields: [] })]);
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules));
+
+    const { pushModels } = await import("../model-pusher");
+    const sourceModel = makeModel({
+      id: 410,
+      referenceName: "FooterLinks",
+      contentDefinitionTypeID: 1,
+      fields: [{ name: "a" }, { name: "b" }],
+    });
+
+    const result = await pushModels([sourceModel], []);
+
+    expect(result.failed).toBe(1);
+    expect(result.successful).toBe(0);
+    expect(result.failureDetails?.some((f) => f.name === "FooterLinks")).toBe(true);
+  });
+});
+
+// ─── PROD-2211: false-negative create recovery ────────────────────────────────
+
+describe("pushModels — false-negative create recovery (PROD-2211)", () => {
+  it("recovers as success + writes a mapping when the create throws but the model exists on target", async () => {
+    const sourceModel = makeModel({ id: 700, referenceName: "RetailerLocatorSearchPanel", contentDefinitionTypeID: 1 });
+    // Stub create rejects (false-negative); the follow-up update then succeeds normally.
+    const saveModel = jest
+      .fn()
+      .mockRejectedValueOnce(new Error("socket hang up"))
+      .mockResolvedValue(makeModel({ id: 8, referenceName: "RetailerLocatorSearchPanel" }));
+    const getContentModules = jest
+      .fn()
+      .mockResolvedValue([
+        makeModel({ id: 8, referenceName: "RetailerLocatorSearchPanel", contentDefinitionTypeID: 1 }),
+      ]);
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules));
+
+    const { ModelMapper } = await import("lib/mappers/model-mapper");
+    const { pushModels } = await import("../model-pusher");
+
+    const result = await pushModels([sourceModel], []);
+
+    expect(getContentModules).toHaveBeenCalled();
+    expect(result.failed).toBe(0);
+    expect(result.successful).toBe(1);
+    const mapper = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]);
+    expect(mapper.getModelMappingByID(700, "source")?.targetID).toBe(8);
+  });
+
+  it("stays failed when the create throws and only a different-typed same-name model exists", async () => {
+    const sourceModule = makeModel({ id: 800, referenceName: "PromoBanner", contentDefinitionTypeID: 2 });
+    const saveModel = jest.fn().mockRejectedValue(new Error("Unable to save the model."));
+    // Only a same-name Content List (type 1) exists — not a match for the Module (type 2).
+    const getContentModules = jest
+      .fn()
+      .mockResolvedValue([makeModel({ id: 9, referenceName: "PromoBanner", contentDefinitionTypeID: 1 })]);
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules));
+
+    const { pushModels } = await import("../model-pusher");
+    const result = await pushModels([sourceModule], []);
+
+    expect(result.failed).toBe(1);
+    expect(result.successful).toBe(0);
+  });
+
+  it("recovers a failed UPDATE when the saved field set matches the source", async () => {
+    // Mapping exists (model already on target); the update throws but the fields were persisted.
+    const { ModelMapper } = await import("lib/mappers/model-mapper");
+    const seeder = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]);
+    seeder.addMapping(
+      { id: 300, referenceName: "Header", lastModifiedDate: new Date(2024, 0, 1).toISOString() } as any,
+      { id: 30, referenceName: "Header", lastModifiedDate: new Date(2024, 0, 1).toISOString() } as any
+    );
+
+    const saveModel = jest.fn().mockRejectedValue(new Error("Unable to save the model."));
+    // Re-query shows the target now has the same field count as the source → recovered.
+    const getContentModules = jest
+      .fn()
+      .mockResolvedValue([makeModel({ id: 30, referenceName: "Header", fields: [{ name: "x" }, { name: "y" }] })]);
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules));
+
+    const { pushModels } = await import("../model-pusher");
+    const sourceModel = makeModel({
+      id: 300,
+      referenceName: "Header",
+      lastModifiedDate: new Date(2025, 0, 1).toISOString(), // newer than mapping → triggers update
+      fields: [{ name: "x" }, { name: "y" }],
+    });
+
+    const targetModel = makeModel({
+      id: 30,
+      referenceName: "Header",
+      lastModifiedDate: new Date(2024, 0, 1).toISOString(),
+    });
+
+    const result = await pushModels([sourceModel], [targetModel]);
+
+    expect(result.successful).toBe(1);
+    expect(result.failed).toBe(0);
+  });
+});
+
+// ─── PROD-2211: adopt existing target model without a mapping ─────────────────
+
+describe("pushModels — exists in target without mapping, non-default (PROD-2211)", () => {
+  it("writes a mapping row and skips instead of silently dropping the model", async () => {
+    const saveModel = jest.fn().mockResolvedValue(makeModel({ id: 999 }));
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
+
+    const { ModelMapper } = await import("lib/mappers/model-mapper");
+    const { pushModels } = await import("../model-pusher");
+
+    const sourceModel = makeModel({ id: 501, referenceName: "Header", contentDefinitionTypeID: 1 });
+    const targetModel = makeModel({ id: 10, referenceName: "Header", contentDefinitionTypeID: 1 });
+
+    const result = await pushModels([sourceModel], [targetModel]);
+
+    expect(saveModel).not.toHaveBeenCalled();
+    expect(result.skipped).toBe(1);
+    expect(result.failed).toBe(0);
+
+    const mapper = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]);
+    expect(mapper.getModelMappingByID(501, "source")?.targetID).toBe(10);
+  });
+
+  it("does NOT adopt a same-name target model of a different type (type-blind lookup)", async () => {
+    const createdStub = makeModel({ id: 888, referenceName: "PromoBanner", contentDefinitionTypeID: 2 });
+    const saveModel = jest.fn().mockResolvedValue(createdStub);
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
+
+    const { pushModels } = await import("../model-pusher");
+
+    const sourceModel = makeModel({ id: 600, referenceName: "PromoBanner", contentDefinitionTypeID: 2 });
+    const targetContentList = makeModel({ id: 9, referenceName: "PromoBanner", contentDefinitionTypeID: 1 });
+
+    const result = await pushModels([sourceModel], [targetContentList]);
+
+    expect(saveModel).toHaveBeenCalled(); // goes through create, not adopted
+    expect(result.skipped).toBe(0);
+  });
+});