Skip to content

Guard compute deploy against stale registry.fly.io image references - #194

Open
tonychang04 wants to merge 2 commits into
mainfrom
fix/compute-deploy-stale-image-guard
Open

Guard compute deploy against stale registry.fly.io image references#194
tonychang04 wants to merge 2 commits into
mainfrom
fix/compute-deploy-stale-image-guard

Conversation

@tonychang04

@tonychang04 tonychang04 commented Jul 10, 2026

Copy link
Copy Markdown
Member

Incident context

Project 6cdb996f (customer "Ovixis", hospet-api) — every compute create/update failed for ~4 hours on Jul 10 with OSS request failed: 504 / COMPUTE_CLOUD_UNAVAILABLE. Cloud logs show the real story: each attempt created the Fly app fine (201), then launchMachine exhausted all 5 MANIFEST_UNKNOWN retries (~72s) because the referenced image registry.fly.io/hospet-api-…:cli-1783654786759 no longer exists — the customer's delete-and-retry loop kept destroying the Fly app and its registry images, while image-mode deploys kept referencing the dead tag (zero deploy-token requests all day = nothing was ever re-pushed). The OSS backend's 60s AbortSignal fires before the real 404 arrives, so the user sees a fake platform outage. The same project deployed three fresh test apps fine minutes later.

Fix (CLI, two layers)

  1. Fail fast — image-mode create of a service that doesn't exist, referencing that service's own registry repo (<name>-<projectId>), cannot succeed: the app was just confirmed absent, so its registry is empty. The CLI now errors immediately with the recovery command instead of triggering a guaranteed 60s server-side hang.
  2. Actionable hint — any deploy/update with a registry.fly.io image that fails with COMPUTE_CLOUD_UNAVAILABLE / 502 / 503 / 504 gets the vanished-image explanation and the source-mode recovery command appended. Non-Fly images and non-timeout errors are untouched.

Verification

  • 585 unit tests pass (10 new: fly-registry.test.ts + 4 deploy scenario tests)
  • tsc clean on touched files (pre-existing unrelated errors in payments/config-apply remain), npm run build succeeds
  • End-to-end against a stub OSS backend with the built binary:
    • fresh create + own stale image → immediate clear error, only the list call goes out
    • existing service + timeout → original error + hint appended
    • redis:7-alpine + same timeout → behavior unchanged

Follow-ups (server-side, separate repos)

  • cloud-backend: map exhausted MANIFEST_UNKNOWN to a 422 IMAGE_NOT_FOUND instead of letting Fly's 404 bubble as COMPUTE_CLOUD_UNAVAILABLE; retry backoff (2+4+8+16+30s) alone exceeds the OSS 60s abort budget
  • cloud-backend: confirm the scale_to_zero migration drift from 06:32–06:44 UTC Jul 10 is fully resolved (it failed other projects' launches after successful Fly machine creation)

🤖 Generated with Claude Code

https://claude.ai/code/session_01Xie2GAar6gT2KR3MybTsaQ


Summary by cubic

Prevents misleading timeouts when deploying with stale registry.fly.io image references by failing fast and adding a clear recovery hint. Also documents and tests the compute update without --image gap where timeouts pass through unchanged.

  • Bug Fixes
    • Fail fast on fresh creates when --image points to the service’s own repo (<name>-<projectId>). Shows why it cannot work and suggests: npx @insforge/cli compute deploy <dir> --name <name>.
    • On deploy/update failures for registry.fly.io images with COMPUTE_CLOUD_UNAVAILABLE or 502/503/504, append an actionable stale-image hint. Other registries and non-timeout errors unchanged.
    • Known gap: compute update without --image relaunches the stored image; if that tag is stale, the timeout surfaces without a hint. Added command-level tests to cover hinted and pass-through cases.

Written for commit 378200e. Summary will update on new commits.

Review in cubic

Note

Guard compute deploy against stale registry.fly.io image references

  • Adds a fast-fail check in compute deploy that throws a CLIError when deploying a new service using an image from its own registry.fly.io repository (i.e. <name>-<projectId>), since that image no longer exists after the service was deleted.
  • Wraps deploy/update errors in compute deploy and compute update with a stale-image hint via withStaleImageHint when a timeout-like failure (502/503/504 or COMPUTE_CLOUD_UNAVAILABLE) occurs on a registry.fly.io image.
  • Adds a new src/lib/fly-registry.ts module with helpers: flyRegistryRepo, imageBelongsToOwnService, staleFlyImageHint, and withStaleImageHint.

Changes since #194 opened

  • Added tests validating timeout error hints when updating compute services with registry.fly.io images [378200e]
  • Expanded inline comment in registerComputeUpdateCommand action handler clarifying limitation of hint logic [378200e]
  • Added documentation to fly-registry.imageBelongsToOwnService explaining naming scheme validation [378200e]

Macroscope summarized 82a9063.

Summary by CodeRabbit

  • Bug Fixes

    • Improved error messages for compute deploy/compute update when using --image backed by Fly image registries and the image is stale or unavailable.
    • Added preflight validation to fail fast when the provided Fly image is tied to a service registry that would be deleted alongside the service.
    • Ensured timeouts for non-Fly images (and unrelated failures) do not get stale-image “Hint:” guidance.
    • Reduced deploy blocking by only guarding against the relevant stale/mismatched registry images.
  • Tests

    • Added coverage for Fly registry URL parsing, service/image matching rules, and timeout/hint behavior for deploy and update.

Deleting a compute service destroys its Fly app and every image in that
app's registry. A cached --image registry.fly.io/<app>:<tag> reference
then fails forever: the platform spins in MANIFEST_UNKNOWN retries (~72s)
until the OSS-side 60s abort fires, surfacing as a misleading
COMPUTE_CLOUD_UNAVAILABLE / 504 that reads as a platform outage.

Two layers:
- Fail fast client-side when a fresh create references its own service's
  registry repo (<name>-<projectId>) — the service doesn't exist, so the
  image cannot either. No round-trip, no 60s server-side hang.
- When a deploy/update with a registry.fly.io image fails with
  COMPUTE_CLOUD_UNAVAILABLE or 502/503/504, append a hint explaining the
  vanished-image failure mode and the source-mode recovery command.

Root-caused from production incident on project 6cdb996f (Jul 10): ~15
identical create timeouts, zero deploy-token requests all day, same
project deployed three fresh test apps fine minutes later.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xie2GAar6gT2KR3MybTsaQ
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Changes

Stale Fly Registry Image Handling

Layer / File(s) Summary
Fly registry parsing and stale-image hints
src/lib/fly-registry.ts, src/lib/fly-registry.test.ts
Adds Fly registry parsing, exact service-image matching, stale-image guidance, conditional error wrapping, and focused tests.
Compute deploy image validation and error handling
src/commands/compute/deploy.ts, src/commands/compute/deploy.test.ts
Validates service-owned images before creation and appends stale-image guidance to relevant create/update failures.
Compute update stale-image error handling
src/commands/compute/update.ts, src/commands/compute/update.test.ts
Decorates image-based PATCH failures with stale-image guidance while preserving unrelated errors, with coverage for Fly and non-Fly images.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Poem

A rabbit packed images in a registry bright,
Then guarded stale ones from launch-day flight.
“Rebuild and push,” the helper sang,
While clearer errors softly rang.
Hop, deploy, and all is right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: guarding compute deploy against stale Fly registry image references.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/compute-deploy-stale-image-guard

Comment @coderabbitai help to get the list of available commands.

@tonychang04
tonychang04 marked this pull request as ready for review July 10, 2026 11:41
@cursor

cursor Bot commented Jul 10, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review: Guard compute deploy against stale registry.fly.io image references

Summary: A tight, well-tested fix that fails fast on a provably-dead registry.fly.io/<name>-<projectId> image and appends an actionable "re-push the image" hint to timeout-shaped failures, replacing a misleading 60s "cloud unavailable" hang — no correctness, security, or performance blockers found.

Requirements context

No matching spec/plan found. The repo keeps specs under docs/specs/ (not docs/superpowers/), and the only files there cover the diagnose command and db-migrations — neither relates to compute deploy. Assessed against the PR description, the incident writeup, and existing compute deploy/update conventions alone. The fix's two stated layers (fail-fast on own-service stale image; hint on Fly-registry timeout) both map cleanly to the implementation.

I verified locally in the PR workspace: npm cinpx vitest run on the two touched test files = 19/19 pass, and eslint is clean on all five changed files.


Critical

(none) — no correctness bugs, security holes, data-loss risks, or broken existing behavior. Verdict is approved (informational; a human still clicks approve).

Suggestion

  • Functionality — compute update hint only fires when --image is re-supplied (src/commands/compute/update.ts:132-139). The incident's failure mode is the platform relaunching a machine that points at a vanished tag. On compute update <id> --memory 1024 (no --image), the server re-launches against the service's stored image; if that image is a dead registry.fly.io tag, the PATCH times out the same way — but opts.image is unset, so the user gets the raw COMPUTE_CLOUD_UNAVAILABLE with no hint. The CLI can't know the stored image without an extra fetch, so this is an understandable limitation rather than a bug, but it's worth a comment (or follow-up) so the gap is intentional and documented.

  • Software engineering — update.ts wiring is untested at the command level (src/commands/compute/update.ts:126-139). fly-registry.test.ts exercises withStaleImageHint thoroughly and deploy.test.ts gained 4 command-level scenarios, but the opts.image ? withStaleImageHint(...) : updateErr branch in update.ts has no test. A small mirror of the deploy tests (one hint-appended case with --image, one pass-through without) would lock in that the plumbing passes opts.image correctly and that non-image updates stay untouched. Low risk given the helper's own coverage.

Information

  • Hidden coupling to a server-side Fly-app naming convention (src/lib/fly-registry.ts:23-32, src/commands/compute/deploy.ts:162-178). The guard assumes the Fly app / registry repo is exactly <service-name>-<projectId> (full UUID), but the CLI never constructs that name itself — flyAppId comes back from the server (deploy.ts:264-270). If the platform ever truncates the project id or changes the scheme, the guard silently no-ops and falls back to today's 60s hang. That's the safe failure direction (a mismatch never wrongly blocks a valid deploy), so no action needed — just flagging the undocumented cross-repo assumption; a shared constant or a comment pointing at the server's app-naming code would make it discoverable.

  • Guard is scoped to the local project + its parent (deploy.ts:163-167). Only config.project_id and config.branched_from.project_id are considered, so a fresh create referencing a foreign project's stale Fly image still takes the 60s path (then gets the hint). This matches the "don't block foreign registry images" intent and is covered by the test at deploy.test.ts ("does not block fresh creates on foreign registry.fly.io images"). Working as designed.

  • flyRegistryRepo regex (src/lib/fly-registry.ts:23) is correctly host-anchored — example.com/registry.fly.io/foo returns null, and docker:// / https:// / :tag / @sha256 are all handled. Good, and well covered by tests.

  • No security-relevant changes: the image URL flows only into a regex, a string equality check, and human-readable error text — no SQL/shell/HTTP injection surface, encodeURIComponent on ids is preserved, and no secrets/tokens/PII are logged or added to responses.

  • No performance concerns: the guard adds one synchronous getProjectConfig() file read on the create-when-absent path only (not a hot loop), plus a trivial regex; no new network round-trips, N+1s, or event-loop blocking.


Verdict: approved

Zero Critical findings. The change is focused (5 files, no scope creep), matches the PR's stated design, ships new unit + scenario tests that pass, and lints clean. The Suggestions above (update-command hint gap + a command-level test for update.ts) are worth a look but are non-blocking. Note this PR is still marked draft — flagging for awareness; the code itself reads as review-ready.

jwfing
jwfing previously approved these changes Jul 10, 2026

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - approved.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review: Guard compute deploy against stale registry.fly.io image references

Summary: A tight, well-tested fix that fails fast on a provably-dead registry.fly.io/<name>-<projectId> image and appends an actionable "stale image" hint to timeout-shaped failures — accurately targeting the incident described, with good scope discipline (5 files, no drive-by changes).

Requirements context

No matching spec/plan found — docs/specs/ contains only diagnose-command and db-migrations designs; there is no docs/superpowers/ directory in this repo (consistent with prior reviews). Assessed against the PR description, the incident write-up, and surrounding code.


Findings

Critical

(none)

Suggestion

  • Functionality — src/commands/compute/update.ts:132-139. The stale-image hint is only applied when --image is passed on this update call. An update that changes only env/port/memory on a service whose current image is a stale registry.fly.io tag will still surface the bare COMPUTE_CLOUD_UNAVAILABLE timeout — the exact confusing symptom this PR fixes, just via a different entry point. The CLI can't know the service's current image without an extra fetch, so this is a reasonable limitation; worth a one-line comment noting it, or a follow-up.

  • Software engineering — src/commands/compute/deploy.test.ts:11-17. The fail-fast tests only exercise the config.project_id branch. The new code also matches against config.branched_from?.project_id (deploy.ts:166), which has no test. A branched-project case would lock in that behavior and guard against regressions if the projectIds list changes.

Information

  • Functionality — src/lib/fly-registry.ts:53-67. withStaleImageHint appends the hint to any registry.fly.io image failing with 502/503/504/COMPUTE_CLOUD_UNAVAILABLE, including a genuine transient cloud outage where the image is actually fine. The hint is phrased conditionally ("If the service was deleted…") so it isn't wrong, and this is an intentional tradeoff — just flagging that during a real platform blip the hint will still show.

  • Software engineering — src/commands/compute/deploy.ts:169-176 vs src/lib/fly-registry.ts:staleFlyImageHint. The hand-written fail-fast message duplicates much of staleFlyImageHint's wording. Not a problem, but a shared helper would keep the two recovery messages from drifting.

  • src/lib/fly-registry.ts:15 (flyRegistryRepo). The regex captures only the first path segment ([^:@/\s]+), which is correct for Fly's single-segment registry.fly.io/<app> repos and correctly rejects example.com/registry.fly.io/foo via the ^ anchor (nicely covered by tests). Fine as-is.


Dimension notes

  • Software engineering: Strong. New unit module + 6 helper tests (fly-registry.test.ts) and 4 deploy-scenario tests covering fail-fast, foreign/other-registry pass-through, hint-on-timeout, and non-fly-image-untouched — the close-call edge cases are exercised. ESM .js imports, CLIError usage, and naming all match repo conventions. CLIError(message, exitCode, code, statusCode) is reconstructed with the correct argument order (fly-registry.ts:60-65).
  • Functionality: The fail-fast guard is correctly gated on !existing (runs before the wrapped POST/PATCH, so no double-wrapping) and on the exact <name>-<projectId> repo match, avoiding false positives on prefix-sharing sibling services. Null-safe: unlinked project → getProjectConfig() null → empty projectIds → guard is a no-op.
  • Security: No security-relevant changes — no new user input reaches SQL/shell/HTTP unsafely (image URL is only echoed into thrown error messages), no secrets/PII logged, no auth paths touched, no new dependencies.
  • Performance: No concerns — only string/regex work on the error path; no new network round-trips (reuses the existing /api/compute/services list).

Verdict: approved (informational)

No blocking issues. The suggestions above are non-blocking; a human still gives the explicit GitHub approval.

- Comment the known gap: compute update without --image relaunches the
  stored image, which can also be a dead registry.fly.io tag, but the CLI
  can't hint without knowing that URL.
- Command-level tests for update.ts: hint appended with a fly --image,
  timeouts pass through unhinted with no --image or a non-fly image.
- Point the <name>-<projectId> assumption at the server's makeFlyAppName
  so the cross-repo coupling is discoverable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xie2GAar6gT2KR3MybTsaQ
@cursor

cursor Bot commented Jul 10, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - approved.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

A tightly-scoped, well-tested bug fix that stops compute deploy/update from hanging ~60s and reporting a fake COMPUTE_CLOUD_UNAVAILABLE when a registry.fly.io image reference has gone stale after its backing service was deleted.

Requirements context

No matching spec/plan found under docs/specs/ — the three docs there (diagnose-*, db-migrations-*) cover unrelated commands, and there is no docs/superpowers/ in this repo. Assessed against the PR description and the incident context it links (project 6cdb996f, hospet-api). The implementation matches the stated intent: (1) fail fast on a fresh image-mode create referencing the service's own registry repo, and (2) append a recovery hint to timeout-ish failures on Fly images.

Findings

Critical

(none)

Suggestion

Functionality — compute update still eats the full 60s hang before the hint. src/commands/compute/update.ts:126-139 only wraps the error after the round-trip, so compute update <id> --image registry.fly.io/<own-stale>:tag still blocks for the full server-side abort budget before the user sees the explanation — the fast-fail short-circuit added to deploy.ts:162-178 has no equivalent here. This is acceptable given update keys off an opaque <id> (it can't cheaply know the image is its own service's repo without an extra lookup), and the incident was image-mode create, so it's out of scope to fix now — worth a follow-up note.

Functionality — update's hint omits the concrete service name. src/commands/compute/update.ts:137 calls withStaleImageHint(updateErr, String(opts.image)) with no serviceName, so the recovery command degrades to the --name <name> placeholder (fly-registry.ts:34). update only has the <id>, not the name, so this is understandable, but the resulting hint is less actionable than the deploy-path one. Consider resolving the name (it's already fetched elsewhere in some flows) or noting in the hint that <name> should be substituted.

Software engineering — duplicated recovery guidance. The fast-fail CLIError in deploy.ts:169-176 and staleFlyImageHint in fly-registry.ts:29-37 express the same "rebuild and push from source" recovery with slightly different wording. Not a correctness issue, but a single shared helper for the recovery sentence would keep them from drifting.

Information

  • Regex robustness is good. flyRegistryRepo (fly-registry.ts:16-19) correctly anchors on the host, tolerates docker:///https:// prefixes, :tag, and @sha256 digests, and rejects registry.fly.io appearing as a path segment (example.com/registry.fly.io/foo). The exact <name>-<projectId> match in imageBelongsToOwnService avoids the name-prefix false positive (hospet-api vs hospet-api-gateway) — both cases are covered by tests in fly-registry.test.ts:24-44.
  • False-positive risk is effectively nil. The guard only fires when the service is confirmed absent (!existing) and the repo equals <name>-<projectId> for this project's own (or branched-from) UUID — a globally-unique Fly repo that cannot resolve to another live app, so no legitimate deploy is blocked.
  • Error-shape assumptions verified. ossFetch throws CLIError(message, 1, err.error, res.status) (src/lib/api/oss.ts:225), so withStaleImageHint's checks on err.code === 'COMPUTE_CLOUD_UNAVAILABLE' and statusCode ∈ {502,503,504} line up with what the deploy/update paths actually receive.

Dimension coverage

  • Software engineering: 10 new tests (fly-registry.test.ts + 4 deploy scenarios), including regression coverage for the exact incident image and pass-through/non-matching cases. Follows repo conventions (CLIError, .js ESM imports, ossFetch). Suite runs green locally (19/19 in the touched files).
  • Functionality: Solves the stated problem; two minor non-blocking gaps noted above. Source-mode's final launchMachine PATCH is intentionally not wrapped, which is correct — that path references a freshly-pushed image, never a stale one.
  • Security: No security-relevant changes. The image URL and IDs are interpolated only into error-message strings (never a shell/SQL/HTTP sink), no secrets logged, no auth paths touched, no new dependencies.
  • Performance: Net positive — the fast-fail avoids a guaranteed ~60s server-side hang. Regex and array checks are trivial; the pre-existing list call is reused, no new N+1.

Verdict

approved (informational — zero Critical findings; the Suggestion/Information items are non-blocking). Explicit GitHub approval remains a separate human action.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - approved.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review — Guard compute deploy against stale registry.fly.io image references

Summary: A well-scoped, well-tested two-layer guard (fail-fast pre-flight + post-hoc hint) that turns a misleading 60s "cloud unavailable" hang into an actionable "your image is gone, re-push from source" message; no blocking issues found.

Requirements context

No matching spec/plan found under docs/specs/ (only diagnose and db-migrations designs live there; the repo has no docs/superpowers/). Assessed against the PR description/incident context alone. The implementation matches the stated two-layer fix precisely: (1) fail fast on an image-mode fresh create referencing the service's own <name>-<projectId> registry repo, and (2) append a stale-image hint to timeout-ish failures (COMPUTE_CLOUD_UNAVAILABLE/502/503/504) on any registry.fly.io image.

Findings

Critical

(none)

Suggestion

  • Functionality — hint may fire on a genuine transient outage (src/lib/fly-registry.ts:56-74). The timeoutish heuristic appends the "image was deleted, re-push from source" hint to any registry.fly.io image that hits COMPUTE_CLOUD_UNAVAILABLE/502/503/504 — including a real, temporary cloud outage where the image is perfectly fine. Blast radius is low: the hint is appended (not substituting the original error) and is phrased conditionally ("If the service was deleted…"), so the worst case is a user doing an unnecessary rebuild. Worth a slightly more hedged lead-in, but not blocking.
  • Software engineering — duplicated recovery copy (src/commands/compute/deploy.ts:169-176 vs src/lib/fly-registry.ts:60-67). The fast-fail message hand-rolls the same npx @insforge/cli compute deploy <dir> --name … recovery command that staleFlyImageHint() already produces. The two intentionally differ in framing (definitive "will always fail" vs. speculative hint), so keeping them separate is defensible — but factoring the shared recovery line into one helper would keep them from drifting.

Information

  • branched_from handling is correct and defensive (src/commands/compute/deploy.ts:163-168). Matching against both project_id and branched_from.project_id mirrors the branch-command convention (src/types.ts:113, src/commands/branch/*). When config is null (unlinked), projectIds is empty and imageBelongsToOwnService returns false — so the guard can never wrongly block a deploy. Good fail-open design, matching the module comment.
  • Software engineering — withStaleImageHint rebuilds the error (src/lib/fly-registry.ts:68-74). Reconstructing a fresh CLIError preserves exitCode/code/statusCode (verified against the constructor at src/lib/errors.ts:3-13) but drops the original stack/cause. Immaterial for CLI UX; noting for completeness.
  • Update stored-image gap is acknowledged and tested (src/commands/compute/update.ts:132-142, update.test.ts). An update without --image relaunches against the service's stored image, which the CLI can't inspect without an extra fetch, so those timeouts pass through unhinted. This is documented in a comment and covered by a dedicated test — a reasonable, explicit trade-off.

Dimension notes

  • Software engineering: Strong TDD coverage — 22 tests across the 3 suites pass locally (npx vitest run on the touched files). Edge cases are well chosen: docker:///https:// prefixes, @sha256 digests, name-prefix collision (hospet-api-gateway must not match hospet-api), foreign project id, and non-Fly registries. Import style (.js extensions), CLIError usage, and naming all match repo conventions.
  • Security: No security-relevant changes. Image URLs (non-secret) are echoed into error messages and passed to an already-existing API call; no new user input reaches SQL/shell, no secrets/PII logged, no auth path touched, no new dependencies.
  • Performance: Net positive — the fast-fail path avoids a guaranteed ~60s server-side hang. The added getProjectConfig() is a one-shot local file read on the !existing branch only; no N+1, loops, or hot-path cost.

Verdict

approved (informational; a human still gives the explicit GitHub approval). Zero Critical findings — the two Suggestions are non-blocking polish.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - approved.

@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds two layers of defense against the 60-second hang that occurs when compute deploy or compute update references a deleted registry.fly.io image. A new fly-registry helper module provides the detection and hint logic.

  • Fast-fail guard (deploy.ts): when deploying a new service (no existing record) with --image pointing at that service's own Fly registry repo (<name>-<projectId>), the CLI immediately throws an actionable error instead of sending the request and waiting for a server-side timeout that surfaces as a misleading cloud-unavailable message.
  • Post-hoc hint (deploy.ts, update.ts): for any deploy or update on a registry.fly.io image that fails with COMPUTE_CLOUD_UNAVAILABLE or a 502/503/504 status, the error message is extended with the stale-image explanation and the source-mode recovery command; all other registries and non-timeout errors are untouched.
  • Test coverage: 10 new tests across fly-registry.test.ts, deploy.test.ts, and a new update.test.ts cover the fast-fail, hint, and pass-through paths.

Confidence Score: 4/5

Safe to merge; both the fast-fail and the hint path are conservative — they can only produce false negatives (no early error, hint not shown), never false positives (valid deploy incorrectly blocked).

The fast-fail guard in deploy.ts reads the project ID from the on-disk config file via getProjectConfig() but does not check the INSFORGE_PROJECT_ID environment variable, which getProjectId() checks first. When only the env var is set, projectIds is empty, the fast-fail silently no-ops, and the user still hits the 60-second hang before receiving the hint. Everything else — the regex, the CLIError rewrapping, the test coverage, the source-mode exclusion — is correct and well-scoped.

src/commands/compute/deploy.ts — the INSFORGE_PROJECT_ID env var path is not covered by the fast-fail guard.

Important Files Changed

Filename Overview
src/lib/fly-registry.ts New helper module: flyRegistryRepo (regex extraction), imageBelongsToOwnService (fast-fail check), staleFlyImageHint (message builder), withStaleImageHint (error rewrapper). Logic is correct and well-guarded; all non-matching cases pass through untouched.
src/commands/compute/deploy.ts Adds fast-fail guard for image-mode fresh creates using own stale registry ref, and wraps the PATCH/POST in a try/catch that appends the stale-image hint on timeout errors. Fast-fail reads getProjectConfig() but not the INSFORGE_PROJECT_ID env var, so the guard silently no-ops when project ID comes from the environment alone — falls back safely to the hint path.
src/commands/compute/update.ts Wraps the PATCH call in a try/catch; appends stale-image hint only when --image is a registry.fly.io URL and the error is timeout-class. No serviceName passed (service ID context only), producing a sensible --name placeholder in the hint.
src/lib/fly-registry.test.ts Good unit coverage of flyRegistryRepo (prefix/digest/other-registry cases), imageBelongsToOwnService (prefix-collision, wrong project), and withStaleImageHint (code vs status matching, pass-through for non-CLIError, non-fly image, non-timeout).
src/commands/compute/deploy.test.ts Four new integration-style scenarios: fast-fail fires only once (1 fetch), foreign/non-fly images are not blocked, update timeout gets hint appended, non-fly image timeout is left untouched.
src/commands/compute/update.test.ts Three new update-path tests: hint added for fly image timeout, no hint when --image absent, no hint for non-fly image. Complete new test file with proper mock structure.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant CLI
    participant OSS as OSS Backend
    participant Fly as Fly Platform

    Note over CLI,Fly: Image-mode deploy — service does not exist
    CLI->>OSS: GET /api/compute/services (list)
    OSS-->>CLI: []
    CLI->>CLI: imageBelongsToOwnService?
    alt Own stale registry image
        CLI-->>CLI: CLIError immediately (fast-fail, no server hang)
    else Foreign or non-Fly image
        CLI->>OSS: POST /api/compute/services
        OSS->>Fly: launchMachine
        alt MANIFEST_UNKNOWN retries exhaust
            OSS-->>CLI: CLIError COMPUTE_CLOUD_UNAVAILABLE / 502-504
            CLI->>CLI: withStaleImageHint appends hint
        else Success
            OSS-->>CLI: service JSON
        end
    end

    Note over CLI,Fly: compute update --image url
    CLI->>OSS: PATCH /api/compute/services/:id
    alt Fly registry image + timeout
        OSS-->>CLI: CLIError timeout
        CLI->>CLI: withStaleImageHint appends hint
    else Success or other error
        OSS-->>CLI: response or original error
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant CLI
    participant OSS as OSS Backend
    participant Fly as Fly Platform

    Note over CLI,Fly: Image-mode deploy — service does not exist
    CLI->>OSS: GET /api/compute/services (list)
    OSS-->>CLI: []
    CLI->>CLI: imageBelongsToOwnService?
    alt Own stale registry image
        CLI-->>CLI: CLIError immediately (fast-fail, no server hang)
    else Foreign or non-Fly image
        CLI->>OSS: POST /api/compute/services
        OSS->>Fly: launchMachine
        alt MANIFEST_UNKNOWN retries exhaust
            OSS-->>CLI: CLIError COMPUTE_CLOUD_UNAVAILABLE / 502-504
            CLI->>CLI: withStaleImageHint appends hint
        else Success
            OSS-->>CLI: service JSON
        end
    end

    Note over CLI,Fly: compute update --image url
    CLI->>OSS: PATCH /api/compute/services/:id
    alt Fly registry image + timeout
        OSS-->>CLI: CLIError timeout
        CLI->>CLI: withStaleImageHint appends hint
    else Success or other error
        OSS-->>CLI: response or original error
    end
Loading

Reviews (1): Last reviewed commit: "Address review: document update-without-..." | Re-trigger Greptile

Comment on lines +163 to +177
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)) {
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}`
);
}

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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

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.

2 issues found and verified against the latest diff

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/commands/compute/deploy.ts">

<violation number="1" location="src/commands/compute/deploy.ts:164">
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.</violation>

<violation number="2" location="src/commands/compute/deploy.ts:166">
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`.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

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>

Comment on lines +164 to +167
const projectIds = [
config?.project_id,
config?.branched_from?.project_id,
].filter((id): id is string => Boolean(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: 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));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants