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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions src/commands/compute/deploy.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
import { describe, expect, it, vi, beforeEach } from 'vitest';
import type * as ErrorsModule from '../../lib/errors.js';
import type * as ConfigModule from '../../lib/config.js';

const PROJECT_ID = '6cdb996f-c696-429b-b9a9-d5abd114dce5';

const ossFetchMock = vi.hoisted(() => vi.fn());
vi.mock('../../lib/api/oss.js', () => ({ ossFetch: ossFetchMock }));
vi.mock('../../lib/credentials.js', () => ({ requireAuth: vi.fn().mockResolvedValue(undefined) }));
vi.mock('../../lib/skills.js', () => ({ reportCliUsage: vi.fn() }));
vi.mock('../../lib/config.js', async (importOriginal) => {
const actual = await importOriginal<typeof ConfigModule>();
return {
...actual,
getProjectConfig: () => ({ project_id: '6cdb996f-c696-429b-b9a9-d5abd114dce5' }),
};
});
vi.mock('../../lib/errors.js', async (importOriginal) => {
const actual = await importOriginal<typeof ErrorsModule>();
return {
Expand Down Expand Up @@ -135,3 +145,83 @@ describe('compute deploy --always-on / --scale-to-zero', () => {
).rejects.toThrow(/mutually exclusive/);
});
});

describe('compute deploy stale fly-registry image guard', () => {
const OWN_IMAGE = `registry.fly.io/hospet-api-${PROJECT_ID}:cli-1783654786759`;

function makeCmd() {
const cmd = new Command();
cmd.exitOverride();
const compute = cmd.command('compute');
registerComputeDeployCommand(compute);
return cmd;
}

beforeEach(() => {
ossFetchMock.mockReset();
});

it('fails fast (before any create call) when a fresh create references its own service registry', async () => {
ossFetchMock.mockResolvedValueOnce({ json: async () => [] }); // list: no existing service
await expect(
makeCmd().parseAsync([
'node', 'lim', 'compute', 'deploy',
'--image', OWN_IMAGE, '--name', 'hospet-api',
])
).rejects.toThrow(/deleting a service deletes its registry images/);
// Only the list call went out — no POST that would hang for 60s server-side.
expect(ossFetchMock).toHaveBeenCalledTimes(1);
});

it('does not block fresh creates on foreign registry.fly.io images or other registries', async () => {
for (const image of [`registry.fly.io/other-svc-${PROJECT_ID}:v1`, 'redis:7-alpine']) {
ossFetchMock.mockReset();
ossFetchMock.mockResolvedValueOnce({ json: async () => [] });
ossFetchMock.mockResolvedValueOnce({
json: async () => ({ name: 'hospet-api', status: 'creating' }),
});
await makeCmd().parseAsync([
'node', 'lim', 'compute', 'deploy',
'--image', image, '--name', 'hospet-api',
]);
expect(ossFetchMock).toHaveBeenCalledTimes(2);
}
});

it('appends the stale-image hint when an update of an existing service times out', async () => {
const { CLIError } = await vi.importActual<typeof ErrorsModule>('../../lib/errors.js');
ossFetchMock.mockResolvedValueOnce({
json: async () => [{ id: 'svc-1', name: 'hospet-api' }],
});
ossFetchMock.mockRejectedValueOnce(
new CLIError(
'COMPUTE_CLOUD_UNAVAILABLE: The operation was aborted due to timeout',
1,
'COMPUTE_CLOUD_UNAVAILABLE',
503
)
);
await expect(
makeCmd().parseAsync([
'node', 'lim', 'compute', 'deploy',
'--image', OWN_IMAGE, '--name', 'hospet-api',
])
).rejects.toThrow(/Rebuild and push a fresh image by deploying from source/);
});

it('leaves timeout errors on non-fly images untouched', async () => {
const { CLIError } = await vi.importActual<typeof ErrorsModule>('../../lib/errors.js');
ossFetchMock.mockResolvedValueOnce({ json: async () => [] });
ossFetchMock.mockRejectedValueOnce(new CLIError('OSS request failed: 504', 1, undefined, 504));
let caught: unknown;
try {
await makeCmd().parseAsync([
'node', 'lim', 'compute', 'deploy',
'--image', 'redis:7-alpine', '--name', 'cache',
]);
} catch (err) {
caught = err;
}
expect(String((caught as Error).message)).not.toContain('Hint:');
});
});
61 changes: 47 additions & 14 deletions src/commands/compute/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { existsSync } from 'node:fs';
import { join, resolve } from 'node:path';
import type { Command } from 'commander';
import { ossFetch } from '../../lib/api/oss.js';
import { getProjectConfig } from '../../lib/config.js';
import { requireAuth } from '../../lib/credentials.js';
import { handleError, getRootOpts, CLIError } from '../../lib/errors.js';
import { outputJson, outputSuccess, outputInfo } from '../../lib/output.js';
Expand All @@ -12,6 +13,10 @@ import {
ensureFlyctlAvailable,
flyctlBuildAndPush,
} from '../../lib/flyctl.js';
import {
imageBelongsToOwnService,
withStaleImageHint,
} from '../../lib/fly-registry.js';

// `compute deploy` has two modes:
//
Expand Down Expand Up @@ -148,21 +153,49 @@ export function registerComputeDeployCommand(computeCmd: Command): void {
(s) => s.name === opts.name
);

// A registry.fly.io image whose repo is this service's own Fly app
// (`<name>-<projectId>`) cannot exist when the service doesn't:
// deleting a service destroys the app and its registry images with
// it. Fail fast instead of letting the platform spin in
// MANIFEST_UNKNOWN retries until a timeout that reads as a cloud
// outage.
if (!existing) {
const config = getProjectConfig();
const projectIds = [
config?.project_id,
config?.branched_from?.project_id,

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: Branch deployments can be blocked when reusing a parent project's registry.fly.io/<name>-<parentProjectId> image: the branch service list may have no matching service, but the parent Fly app/image can still exist. The stale fast-fail should only treat the current project's own repo as guaranteed gone, not branched_from.project_id.

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

<comment>Branch deployments can be blocked when reusing a parent project's `registry.fly.io/<name>-<parentProjectId>` image: the branch service list may have no matching service, but the parent Fly app/image can still exist. The stale fast-fail should only treat the current project's own repo as guaranteed gone, not `branched_from.project_id`.</comment>

<file context>
@@ -148,21 +153,49 @@ export function registerComputeDeployCommand(computeCmd: Command): void {
+            const config = getProjectConfig();
+            const projectIds = [
+              config?.project_id,
+              config?.branched_from?.project_id,
+            ].filter((id): id is string => Boolean(id));
+            if (imageBelongsToOwnService(String(opts.image), opts.name, projectIds)) {
</file context>

].filter((id): id is string => Boolean(id));
Comment on lines +164 to +167

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: The projectIds array is populated only from getProjectConfig(), which reads the on-disk config file. When the project ID is supplied solely through process.env.INSFORGE_PROJECT_ID (the env-var path that other parts of the codebase check first), config will be null, projectIds will be empty, and imageBelongsToOwnService will always return false — silently bypassing the fast-fail guard and letting the command proceed to the 60-second server timeout.

Consider also including process.env.INSFORGE_PROJECT_ID in the projectIds array to close this gap.

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

<comment>The `projectIds` array is populated only from `getProjectConfig()`, which reads the on-disk config file. When the project ID is supplied solely through `process.env.INSFORGE_PROJECT_ID` (the env-var path that other parts of the codebase check first), `config` will be `null`, `projectIds` will be empty, and `imageBelongsToOwnService` will always return `false` — silently bypassing the fast-fail guard and letting the command proceed to the 60-second server timeout.

Consider also including `process.env.INSFORGE_PROJECT_ID` in the `projectIds` array to close this gap.</comment>

<file context>
@@ -148,21 +153,49 @@ export function registerComputeDeployCommand(computeCmd: Command): void {
+          // outage.
+          if (!existing) {
+            const config = getProjectConfig();
+            const projectIds = [
+              config?.project_id,
+              config?.branched_from?.project_id,
</file context>
Suggested change
const projectIds = [
config?.project_id,
config?.branched_from?.project_id,
].filter((id): id is string => Boolean(id));
const projectIds = [
config?.project_id,
config?.branched_from?.project_id,
process.env.INSFORGE_PROJECT_ID,
].filter((id): id is string => Boolean(id));

if (imageBelongsToOwnService(String(opts.image), opts.name, projectIds)) {
throw new CLIError(
`Image ${opts.image} lives in the registry of the Fly app that backs service ` +
`"${opts.name}" — but that service doesn't exist, so the image is gone ` +
`(deleting a service deletes its registry images). Deploying this reference ` +
`will always fail.\n` +
`Rebuild and push a fresh image by deploying from source:\n` +
` npx @insforge/cli compute deploy <dir> --name ${opts.name}`
);
}
Comment on lines +163 to +177

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Fast-fail guard misses INSFORGE_PROJECT_ID env var

getProjectConfig() reads only from the on-disk config file; it returns null when the project ID is supplied via process.env.INSFORGE_PROJECT_ID (the mechanism getProjectId() in config.ts checks first). In that case projectIds is empty, imageBelongsToOwnService always returns false, and the fast-fail silently no-ops — the command proceeds to the server and hits the 60-second hang instead of the intended early error. The post-hoc withStaleImageHint catch still fires, so the user eventually gets a hint, but the wasted round-trip remains. Adding process.env.INSFORGE_PROJECT_ID to the projectIds array would close the gap.

}

let res;
if (existing) {
if (!json) outputInfo(`Found existing service "${opts.name}", updating...`);
const updateBody: Record<string, unknown> = { ...body };
delete updateBody.name;
if (opts.protocol === 'tcp') updateBody.protocol = 'tcp';
res = await ossFetch(`/api/compute/services/${encodeURIComponent(existing.id)}`, {
method: 'PATCH',
body: JSON.stringify(updateBody),
});
} else {
res = await ossFetch('/api/compute/services', {
method: 'POST',
body: JSON.stringify(body),
});
try {
if (existing) {
if (!json) outputInfo(`Found existing service "${opts.name}", updating...`);
const updateBody: Record<string, unknown> = { ...body };
delete updateBody.name;
if (opts.protocol === 'tcp') updateBody.protocol = 'tcp';
res = await ossFetch(`/api/compute/services/${encodeURIComponent(existing.id)}`, {
method: 'PATCH',
body: JSON.stringify(updateBody),
});
} else {
res = await ossFetch('/api/compute/services', {
method: 'POST',
body: JSON.stringify(body),
});
}
} catch (deployErr) {
throw withStaleImageHint(deployErr, String(opts.image), opts.name);
}
const service = (await res.json()) as Record<string, unknown>;

Expand Down
78 changes: 78 additions & 0 deletions src/commands/compute/update.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { describe, expect, it, vi, beforeEach } from 'vitest';
import type * as ErrorsModule from '../../lib/errors.js';

const ossFetchMock = vi.hoisted(() => vi.fn());
vi.mock('../../lib/api/oss.js', () => ({ ossFetch: ossFetchMock }));
vi.mock('../../lib/credentials.js', () => ({ requireAuth: vi.fn().mockResolvedValue(undefined) }));
vi.mock('../../lib/skills.js', () => ({ reportCliUsage: vi.fn() }));
vi.mock('../../lib/errors.js', async (importOriginal) => {
const actual = await importOriginal<typeof ErrorsModule>();
return {
...actual,
handleError: (err: unknown) => { throw err; },
};
});

import { Command } from 'commander';
import { registerComputeUpdateCommand } from './update.js';

describe('compute update stale fly-registry image hint', () => {
const FLY_IMAGE = 'registry.fly.io/hospet-api-6cdb996f-c696-429b-b9a9-d5abd114dce5:cli-1783654786759';

function makeCmd() {
const cmd = new Command();
cmd.exitOverride();
const compute = cmd.command('compute');
registerComputeUpdateCommand(compute);
return cmd;
}

async function timeoutError() {
const { CLIError } = await vi.importActual<typeof ErrorsModule>('../../lib/errors.js');
return new CLIError(
'COMPUTE_CLOUD_UNAVAILABLE: The operation was aborted due to timeout',
1,
'COMPUTE_CLOUD_UNAVAILABLE',
503
);
}

beforeEach(() => {
ossFetchMock.mockReset();
});

it('appends the hint when a PATCH with --image on a fly registry ref times out', async () => {
ossFetchMock.mockRejectedValueOnce(await timeoutError());
await expect(
makeCmd().parseAsync([
'node', 'lim', 'compute', 'update', 'svc-1', '--image', FLY_IMAGE,
])
).rejects.toThrow(/Rebuild and push a fresh image by deploying from source/);
});

it('leaves timeouts unhinted when no --image was supplied (stored-image gap)', async () => {
ossFetchMock.mockRejectedValueOnce(await timeoutError());
let caught: unknown;
try {
await makeCmd().parseAsync([
'node', 'lim', 'compute', 'update', 'svc-1', '--memory', '1024',
]);
} catch (err) {
caught = err;
}
expect(String((caught as Error).message)).not.toContain('Hint:');
});

it('leaves timeouts unhinted for non-fly images', async () => {
ossFetchMock.mockRejectedValueOnce(await timeoutError());
let caught: unknown;
try {
await makeCmd().parseAsync([
'node', 'lim', 'compute', 'update', 'svc-1', '--image', 'redis:7-alpine',
]);
} catch (err) {
caught = err;
}
expect(String((caught as Error).message)).not.toContain('Hint:');
});
});
22 changes: 18 additions & 4 deletions src/commands/compute/update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { handleError, getRootOpts, CLIError } from '../../lib/errors.js';
import { outputJson, outputSuccess } from '../../lib/output.js';
import { reportCliUsage } from '../../lib/skills.js';
import { trackCommandUsage } from '../../lib/command-telemetry.js';
import { withStaleImageHint } from '../../lib/fly-registry.js';

const ENV_KEY_REGEX = /^[A-Z_][A-Z0-9_]*$/;

Expand Down Expand Up @@ -122,10 +123,23 @@ export function registerComputeUpdateCommand(computeCmd: Command): void {
);
}

const res = await ossFetch(`/api/compute/services/${encodeURIComponent(id)}`, {
method: 'PATCH',
body: JSON.stringify(body),
});
let res;
try {
res = await ossFetch(`/api/compute/services/${encodeURIComponent(id)}`, {
method: 'PATCH',
body: JSON.stringify(body),
});
} catch (updateErr) {
// A timeout on a registry.fly.io image usually means the tag was
// deleted along with a previous service — say so instead of
// "cloud unavailable". Known gap: an update without --image
// relaunches against the service's *stored* image, which can be a
// dead registry.fly.io tag too, but the CLI doesn't know that URL
// without an extra fetch — those timeouts pass through unhinted.
throw opts.image
? withStaleImageHint(updateErr, String(opts.image))
: updateErr;
}
const service = await res.json() as Record<string, unknown>;

await trackCommandUsage('compute', 'update', true);
Expand Down
87 changes: 87 additions & 0 deletions src/lib/fly-registry.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import { describe, expect, it } from 'vitest';
import { CLIError } from './errors.js';
import {
flyRegistryRepo,
imageBelongsToOwnService,
staleFlyImageHint,
withStaleImageHint,
} from './fly-registry.js';

const PROJECT_ID = '6cdb996f-c696-429b-b9a9-d5abd114dce5';
const OWN_IMAGE = `registry.fly.io/hospet-api-${PROJECT_ID}:cli-1783654786759`;

describe('flyRegistryRepo', () => {
it('extracts the repo from a plain registry.fly.io ref', () => {
expect(flyRegistryRepo(OWN_IMAGE)).toBe(`hospet-api-${PROJECT_ID}`);
});

it('tolerates docker:// and https:// prefixes and digests', () => {
expect(flyRegistryRepo(`docker://registry.fly.io/foo:latest`)).toBe('foo');
expect(flyRegistryRepo(`https://registry.fly.io/foo@sha256:abc`)).toBe('foo');
});

it('returns null for other registries', () => {
expect(flyRegistryRepo('redis:7-alpine')).toBeNull();
expect(flyRegistryRepo('ghcr.io/acme/api:v1')).toBeNull();
expect(flyRegistryRepo('docker.io/library/nginx')).toBeNull();
// registry.fly.io as a path segment, not the host
expect(flyRegistryRepo('example.com/registry.fly.io/foo')).toBeNull();
});
});

describe('imageBelongsToOwnService', () => {
it('matches only the exact <name>-<projectId> repo', () => {
expect(imageBelongsToOwnService(OWN_IMAGE, 'hospet-api', [PROJECT_ID])).toBe(true);
// another service's repo that shares a name prefix must NOT match
expect(
imageBelongsToOwnService(
`registry.fly.io/hospet-api-gateway-${PROJECT_ID}:v1`,
'hospet-api',
[PROJECT_ID]
)
).toBe(false);
// same name, different project — could be a live app elsewhere
expect(
imageBelongsToOwnService(OWN_IMAGE, 'hospet-api', ['00000000-0000-0000-0000-000000000000'])
).toBe(false);
expect(imageBelongsToOwnService('redis:7-alpine', 'hospet-api', [PROJECT_ID])).toBe(false);
});
});

describe('withStaleImageHint', () => {
const timeoutErr = () =>
new CLIError('OSS request failed: 504', 1, undefined, 504);

it('appends the hint for timeout-ish failures on fly registry images', () => {
const wrapped = withStaleImageHint(timeoutErr(), OWN_IMAGE, 'hospet-api') as CLIError;
expect(wrapped.message).toContain('deleted together with');
expect(wrapped.message).toContain('--name hospet-api');
expect(wrapped.statusCode).toBe(504);
});

it('matches COMPUTE_CLOUD_UNAVAILABLE by code and 502/503 by status', () => {
for (const err of [
new CLIError('unavailable', 1, 'COMPUTE_CLOUD_UNAVAILABLE', 503),
new CLIError('bad gateway', 1, undefined, 502),
new CLIError('unavailable', 1, undefined, 503),
]) {
const wrapped = withStaleImageHint(err, OWN_IMAGE, 'x') as CLIError;
expect(wrapped.message).toContain('Hint:');
}
});

it('passes through non-timeout errors, non-fly images, and non-CLIErrors', () => {
const quota = new CLIError('quota exceeded', 1, 'COMPUTE_QUOTA_EXCEEDED', 403);
expect(withStaleImageHint(quota, OWN_IMAGE, 'x')).toBe(quota);

const dockerhubTimeout = timeoutErr();
expect(withStaleImageHint(dockerhubTimeout, 'redis:7', 'x')).toBe(dockerhubTimeout);

const plain = new Error('boom');
expect(withStaleImageHint(plain, OWN_IMAGE, 'x')).toBe(plain);
});

it('falls back to a <name> placeholder when the service name is unknown', () => {
expect(staleFlyImageHint()).toContain('--name <name>');
});
});
Loading
Loading