-
Notifications
You must be signed in to change notification settings - Fork 19
Guard compute deploy against stale registry.fly.io image references #194
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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'; | ||||||||||||||||||||
|
|
@@ -12,6 +13,10 @@ import { | |||||||||||||||||||
| ensureFlyctlAvailable, | ||||||||||||||||||||
| flyctlBuildAndPush, | ||||||||||||||||||||
| } from '../../lib/flyctl.js'; | ||||||||||||||||||||
| import { | ||||||||||||||||||||
| imageBelongsToOwnService, | ||||||||||||||||||||
| withStaleImageHint, | ||||||||||||||||||||
| } from '../../lib/fly-registry.js'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // `compute deploy` has two modes: | ||||||||||||||||||||
| // | ||||||||||||||||||||
|
|
@@ -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, | ||||||||||||||||||||
| ].filter((id): id is string => Boolean(id)); | ||||||||||||||||||||
|
Comment on lines
+164
to
+167
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The Consider also including Prompt for AI agents
Suggested change
|
||||||||||||||||||||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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>; | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| 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:'); | ||
| }); | ||
| }); |
| 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>'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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, notbranched_from.project_id.Prompt for AI agents