Skip to content

fix: keep catalog push alive when deletion checks fail #2314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
charlietlamb wants to merge 5 commits into from
Closed

fix: keep catalog push alive when deletion checks fail #2314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f31c2e
fix(catalog): create explicit plan versions
charlietlamb Jul 17, 2026
a4f8bf6
fix(catalog): validate explicit version order
charlietlamb Jul 17, 2026
6380543
fix(sync): preserve matched plan version
charlietlamb Jul 17, 2026
ecf6f0f
fix: keep catalog push alive when deletion checks fail
charlietlamb Jul 20, 2026
222be1c
fix(catalog): track renamed plan versions by target
charlietlamb Jul 20, 2026
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
57 changes: 45 additions & 12 deletions packages/atmn/src/commands/push/push.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,19 @@ export async function checkFeatureDeleteInfo(
}

// Check API for product references
const response = await getFeatureDeletionInfo({ secretKey, featureId });
let response;
try {
response = await getFeatureDeletionInfo({ secretKey, featureId });
} catch {
return {
id: featureId,
canDelete: false,
reason: "deletion_check_failed",
featureType,
};
}

if (response && response.totalCount > 0) {
if (response && response.totalCount > 0) {
Comment on lines +192 to +194

@greptile-apps greptile-apps Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Extra indentation on the if statement — it is left over from the refactor and the surrounding code is at a single-tab indent level.

Suggested change
}
if (response && response.totalCount > 0) {
if (response && response.totalCount > 0) {
}
if (response && response.totalCount > 0) {
Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/atmn/src/commands/push/push.ts
Line: 192-194

Comment:
Extra indentation on the `if` statement — it is left over from the refactor and the surrounding code is at a single-tab indent level.

```suggestion
	}

	if (response && response.totalCount > 0) {
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

return {
id: featureId,
canDelete: false,
Expand All @@ -204,22 +214,27 @@ export async function checkFeatureDeleteInfo(
// Check if a plan can be deleted
async function checkPlanDeleteInfo(planId: string): Promise<PlanDeleteInfo> {
const secretKey = getSecretKey();
const response = await getPlanDeletionInfo({ secretKey, planId });
try {
const response = await getPlanDeletionInfo({ secretKey, planId });

if (response && response.totalCount > 0) {
return {
id: planId,
canDelete: false,
customerCount: response.totalCount,
firstCustomerName: response.customerName,
};
}

if (response && response.totalCount > 0) {
return { id: planId, canDelete: true, customerCount: 0 };
} catch {
return {
id: planId,
canDelete: false,
customerCount: response.totalCount,
firstCustomerName: response.customerName,
customerCount: 0,
deletionCheckFailed: true,
};
}
Comment on lines +230 to 237

@greptile-apps greptile-apps Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 deletionCheckFailed flag is never read — fail-closed intent is not enforced

When getPlanDeletionInfo throws, the catch block returns canDelete: false, customerCount: 0, deletionCheckFailed: true. Because customerCount === 0, createPlanDeletePrompt routes to createPlanDeleteNoCustomersPrompt, which presents "Delete permanently" as the default option. Nothing in usePush.ts (or anywhere else) reads deletionCheckFailed to override this behaviour, so if the user accepts the default the push will attempt to delete a plan whose customer status is genuinely unknown. The same pattern appears in checkFeatureDeleteInfo: reason: "deletion_check_failed" falls through createFeatureDeletePrompt to createFeatureDeleteNoDepsPrompt, again defaulting to "Delete permanently" as though the feature has no dependencies. Both cases contradict the PR's stated "fail closed and defer deletion" goal. At minimum the prompt should default to "Skip (keep as is)" when deletionCheckFailed is true; alternatively the failed-check entries could be excluded from plansToDelete / featuresToDelete entirely and silently deferred.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/atmn/src/commands/push/push.ts
Line: 230-237

Comment:
**`deletionCheckFailed` flag is never read — fail-closed intent is not enforced**

When `getPlanDeletionInfo` throws, the catch block returns `canDelete: false, customerCount: 0, deletionCheckFailed: true`. Because `customerCount === 0`, `createPlanDeletePrompt` routes to `createPlanDeleteNoCustomersPrompt`, which presents **"Delete permanently"** as the default option. Nothing in `usePush.ts` (or anywhere else) reads `deletionCheckFailed` to override this behaviour, so if the user accepts the default the push will attempt to delete a plan whose customer status is genuinely unknown. The same pattern appears in `checkFeatureDeleteInfo`: `reason: "deletion_check_failed"` falls through `createFeatureDeletePrompt` to `createFeatureDeleteNoDepsPrompt`, again defaulting to "Delete permanently" as though the feature has no dependencies. Both cases contradict the PR's stated "fail closed and defer deletion" goal. At minimum the prompt should default to "Skip (keep as is)" when `deletionCheckFailed` is true; alternatively the failed-check entries could be excluded from `plansToDelete` / `featuresToDelete` entirely and silently deferred.

How can I resolve this? If you propose a fix, please make it concise.


return {
id: planId,
canDelete: true,
customerCount: 0,
};
}

// Check if updating a plan will create a new version
Expand Down Expand Up @@ -572,6 +587,8 @@ function normalizePlanForCompare(
version:
license.version ?? licenseVersionFallbacks?.get(license.licensePlanId),
included: license.included ?? 0,
prepaidOnly: license.prepaidOnly ?? true,

@cubic-dev-ai cubic-dev-ai Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: License comparison does not actually observe the new prepaidOnly or customize values because the API-to-SDK transform drops both fields before normalizePlanForCompare runs. Updating the license model and transform alongside this comparison would prevent these lines from being dead and ensure plan changes are detected.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At packages/atmn/src/commands/push/push.ts, line 590:

<comment>License comparison does not actually observe the new `prepaidOnly` or `customize` values because the API-to-SDK transform drops both fields before `normalizePlanForCompare` runs. Updating the license model and transform alongside this comparison would prevent these lines from being dead and ensure plan changes are detected.</comment>

<file context>
@@ -572,6 +587,8 @@ function normalizePlanForCompare(
 			version:
 				license.version ?? licenseVersionFallbacks?.get(license.licensePlanId),
 			included: license.included ?? 0,
+			prepaidOnly: license.prepaidOnly ?? true,
+			customize: license.customize,
 		}));
</file context>

customize: license.customize,
}));

return result;
Expand Down Expand Up @@ -644,6 +661,12 @@ function toApiCustomizePlan(customize: CustomizePlan): Record<string, unknown> {
...(customize.price.intervalCount !== undefined
? { interval_count: customize.price.intervalCount }
: {}),
...(customize.price.additionalCurrencies
? {
additional_currencies:
customize.price.additionalCurrencies,
}
: {}),
}
: null,
}
Expand All @@ -668,6 +691,15 @@ function toApiCustomizePlan(customize: CustomizePlan): Record<string, unknown> {
: null,
}
: {}),
...(customize.upsertLicenses !== undefined
? {
upsert_licenses: transformPlanToApi({

@cubic-dev-ai cubic-dev-ai Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Variant license customization loses its optional fields and changes the meaning of omitted included values in every payload using this path. Mapping customize-license entries directly while preserving optional included, prepaidOnly, and nested customize fields would avoid sending 0 or dropping the requested customization.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At packages/atmn/src/commands/push/push.ts, line 696:

<comment>Variant license customization loses its optional fields and changes the meaning of omitted `included` values in every payload using this path. Mapping customize-license entries directly while preserving optional `included`, `prepaidOnly`, and nested `customize` fields would avoid sending `0` or dropping the requested customization.</comment>

<file context>
@@ -668,6 +691,15 @@ function toApiCustomizePlan(customize: CustomizePlan): Record<string, unknown> {
 			: {}),
+		...(customize.upsertLicenses !== undefined
+			? {
+					upsert_licenses: transformPlanToApi({
+						id: "license-customize",
+						name: "License customize",
</file context>

id: "license-customize",
name: "License customize",
licenses: customize.upsertLicenses,
}).licenses,
}
: {}),
};
}

Expand All @@ -678,6 +710,7 @@ function toCatalogVariantParams(
): Record<string, unknown> {
return {
variant_plan_id: variant.id,
...(variant.version !== undefined ? { version: variant.version } : {}),
name: variant.name,
customize: toApiCustomizePlan(variant.customize ?? {}),
...(intent === "create_version" ? { force_version: true } : {}),
Expand Down
3 changes: 2 additions & 1 deletion packages/atmn/src/commands/push/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type { Feature, Plan } from "../../compose/models/index.js";
export interface FeatureDeleteInfo {
id: string;
canDelete: boolean;
reason?: "credit_system" | "products";
reason?: "credit_system" | "products" | "deletion_check_failed";
referencingCreditSystems?: string[]; // IDs of credit systems using this feature
referencingProducts?: { name: string; count: number };
featureType?: "boolean" | "metered" | "credit_system"; // For sorting (delete credit systems first)
Expand All @@ -20,6 +20,7 @@ export interface PlanDeleteInfo {
canDelete: boolean;
customerCount: number;
firstCustomerName?: string;
deletionCheckFailed?: boolean;
}

// Plan update info
Expand Down
1 change: 1 addition & 0 deletions server/src/internal/billing/v2/actions/sync/subscriptionToSyncParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const matchedPlanToSyncPlan = ({
: matchedPlan.customize;
return {
plan_id: matchedPlan.product.id,
version: matchedPlan.product.version,
quantity: matchedPlan.quantity,
customize,
expire_previous: true,
Expand Down
60 changes: 56 additions & 4 deletions server/src/internal/catalog/actions/catalogPlanDependencies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
import { type CatalogPlanParams, ErrCode, RecaseError } from "@autumn/shared";
import {
type CatalogPlanParams,
ErrCode,
type FullProduct,
RecaseError,
} from "@autumn/shared";

const referenceKey = (id: string, version?: number) =>
version === undefined ? id : `${id}@${version}`;

const targetId = (plan: CatalogPlanParams) => plan.new_plan_id ?? plan.plan_id;

const dependenciesForPlan = (plan: CatalogPlanParams) =>
(plan.licenses ?? []).map((license) => ({
id: license.license_plan_id,
Expand All @@ -14,8 +21,8 @@ export const sortCatalogPlansByDependencies = (
): CatalogPlanParams[] => {
const byReference = new Map<string, CatalogPlanParams>();
for (const plan of plans) {
byReference.set(referenceKey(plan.plan_id, plan.version), plan);
const latestKey = plan.new_plan_id ?? plan.plan_id;
byReference.set(referenceKey(targetId(plan), plan.version), plan);
const latestKey = targetId(plan);
const latest = byReference.get(latestKey);
if (!latest || (plan.version ?? 0) > (latest.version ?? 0)) {
byReference.set(latestKey, plan);
Expand All @@ -26,7 +33,7 @@ export const sortCatalogPlansByDependencies = (
const visited = new Set<string>();
const sorted: CatalogPlanParams[] = [];
const visit = (plan: CatalogPlanParams) => {
const key = referenceKey(plan.plan_id, plan.version);
const key = referenceKey(targetId(plan), plan.version);
if (visiting.has(key)) {
throw new RecaseError({
message: "Plan dependency cycle detected.",
Expand All @@ -36,6 +43,12 @@ export const sortCatalogPlansByDependencies = (
}
if (visited.has(key)) return;
visiting.add(key);
if (plan.version !== undefined && plan.version > 1) {
const previous = byReference.get(
referenceKey(targetId(plan), plan.version - 1),
);
if (previous) visit(previous);
}
for (const reference of dependenciesForPlan(plan)) {
const dependency =
byReference.get(referenceKey(reference.id, reference.version)) ??
Expand All @@ -49,3 +62,42 @@ export const sortCatalogPlansByDependencies = (
for (const plan of plans) visit(plan);
return sorted;
};

export const validateCatalogPlanVersionTargets = ({
plans,
products,
}: {
plans: CatalogPlanParams[];
products: FullProduct[];
}) => {
const existing = new Set(
products.map((product) => referenceKey(product.id, product.version)),
);
const latestById = new Map<string, number>();
for (const product of products) {
latestById.set(
product.id,
Math.max(latestById.get(product.id) ?? 0, product.version),
);
}

for (const plan of sortCatalogPlansByDependencies(plans)) {
const id = targetId(plan);
if (
plan.version === undefined ||
existing.has(referenceKey(id, plan.version))
) {
continue;
}
const expected = (latestById.get(id) ?? 0) + 1;
if (plan.version !== expected) {
throw new RecaseError({
message: `Plan ${id} version must be ${expected}, received ${plan.version}.`,
code: ErrCode.InvalidRequest,
statusCode: 400,
});
}
existing.add(referenceKey(id, plan.version));
latestById.set(id, plan.version);
}
};
8 changes: 6 additions & 2 deletions server/src/internal/catalog/actions/catalogPlanPreflight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import {
} from "@/internal/licenses/actions/links/syncPlanLicenses.js";
import { previewAffectedLicenses } from "@/internal/product/actions/previewUpdatePlan/previewAffectedLicenses.js";
import { ProductService } from "@/internal/products/ProductService.js";
import { sortCatalogPlansByDependencies } from "./catalogPlanDependencies.js";
import {
sortCatalogPlansByDependencies,
validateCatalogPlanVersionTargets,
} from "./catalogPlanDependencies.js";

const virtualProduct = ({
current,
Expand Down Expand Up @@ -50,6 +53,7 @@ const preflightCatalogLicenses = async ({
env: ctx.env,
returnAll: true,
});
validateCatalogPlanVersionTargets({ plans, products: persistedProducts });
const currentById = new Map<string, FullProduct>();
for (const product of persistedProducts) {
const current = currentById.get(product.id);
Expand All @@ -74,7 +78,7 @@ const preflightCatalogLicenses = async ({
? previews[index]?.versionable
? (latest?.version ?? current.version) + 1
: current.version
: 1,
: (plan.version ?? 1),
archived: plan.archived,
});
});
Expand Down
4 changes: 2 additions & 2 deletions server/src/internal/catalog/actions/previewUpdateCatalog/previewCatalogPlanUpdate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const previewNewPlan = ({
const {
plan_id,
new_plan_id,
version: _version,
version,
variants: _variants,
include_versions: _includeVersions,
include_variants: _includeVariants,
Expand All @@ -43,7 +43,7 @@ const previewNewPlan = ({
});
const product = {
env: ctx.env,
version: 1,
version: version ?? 1,
created_at: Date.now(),
archived: false,
...resolved,
Expand Down
2 changes: 1 addition & 1 deletion server/src/internal/catalog/actions/previewUpdateCatalog/previewUpdateCatalog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const productUsesFeature = ({
product: FullProduct;
featureId: string;
}) =>
product.entitlements.some(
(product.entitlements ?? []).some(
(entitlement) => entitlement.feature.id === featureId,
) || product.prices.some((price) => price.config?.feature_id === featureId);

Expand Down
26 changes: 26 additions & 0 deletions server/src/internal/catalog/actions/updateCatalog/updateCatalog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,32 @@ const upsertPlans = async ({

if (!current) {
const variantUpdates = variants ?? [];
const latest =
version === undefined
? null
: await ProductService.getFull({
db,
idOrInternalId: plan_id,
orgId: org.id,
env,
allowNotFound: true,
});
if (latest) {
const updates = apiPlan.map.paramsV1ToProductV2({
ctx,
currentFullProduct: latest,
params: { id: new_plan_id ?? plan_id, ...rest },
}) as UpdateProductV2Params;
await updateProduct({
ctx,
productId: plan_id,
query: { force_version: true },
updates,
initialFullProduct: latest,
variantUpdates,
});
continue;
}
const createParams = apiPlan.map.paramsV1ToProductV2({
ctx,
params: {
Expand Down
Loading