Skip to content

fix: provider-honest compute suspend help copy - #162

Open
tonychang04 wants to merge 3 commits into
mainfrom
fix/suspend-help-copy
Open

fix: provider-honest compute suspend help copy#162
tonychang04 wants to merge 3 commits into
mainfrom
fix/suspend-help-copy

Conversation

@tonychang04

@tonychang04 tonychang04 commented Aug 31, 2026

Copy link
Copy Markdown
Member

insta compute suspend's help text said "RAM snapshot" — Fly-era wording that is false on the insta-compute plane, where suspend releases the microVM rather than snapshotting its memory. The description is now:

Suspend a compute service: takes the machine down until start — do not rely on in-memory or on-disk state surviving (a service with a /data volume cannot be suspended; use stop)

Every clause is verified against current main of both planes rather than asserted:

  • "takes the machine down until start"stop/suspend set autostart:false so the router cannot wake the app; start restores it (insta-platform src/provisioning/services.ts:1288-1289). True on both planes.
  • "do not rely on in-memory … state surviving" — on insta-compute, SuspendService scales the Deployment to 0 (insta-compute internal/runtime/k8s.go:825-830); no memory is snapshotted. On Fly, suspendApp does snapshot RAM (insta-platform src/adapters/fly.ts:1536-1541), so this under-promises there, which is the safe direction — and Fly rows are still live, since computeProviderKind is a create-time switch (insta-platform src/config.ts:169-173).
  • "… or on-disk state surviving" — the VM's writable rootfs is a per-pod CoW clone "deleted on release" (insta-compute internal/runtime/warm.go:1157-1159), and suspendWarm releases the pool pods, deleted and never reused (internal/runtime/k8s_warm.go:450-451). finishSuspend flushes bytes to object storage only for an img /data volume (internal/runtime/k8s.go:843-854), and the interface contract says exactly what survives: "the volume and identity" (internal/runtime/runtime.go:155). So nothing a running app wrote to the container filesystem survives a suspend. The previous revision's "disk state persists" was an explicit affirmative promise that was false on 100% of insta-compute suspends — a user trusting it (SQLite file, uploaded assets) would lose that data silently.
  • "a service with a /data volume cannot be suspended; use stop" — the platform rejects it outright, not provider-gated: insta-platform src/provisioning/services.ts:1305-1307 throws this service has a /data volume — machines with volumes cannot be suspended (no RAM snapshot); use stop. That is why the copy makes no claim about volume durability: the one durable disk belongs to services this verb can never reach.

One-line copy change, matching the surrounding lifecycle descriptions' style. npm run typecheck clean; npm test → 47 files / 672 tests passed; insta compute --help renders and wraps cleanly.

Declined (non-blocking, recorded rather than actioned): keeping a suspend-vs-stop differentiator in the copy — wake latency is not plane-invariant (on insta-compute "the plane always suspends; the distinction is Fly's"), so any such claim would be the next false one. Landing the insta-skills mirror (insta/references/operate.md:102 still says "snapshot RAM for a faster resume") is a follow-up PR in that repo; cli-reference.md's suspend row is already provider-neutral.

🤖 Generated with Claude Code

"RAM snapshot" was Fly-era wording and is false on the insta-compute
provider, where suspend releases the microVM entirely: only the code
disk and /data volume persist, and wake boots fresh in about a second.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 1 file

You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Re-trigger cubic

@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
The PR correctly updates insta compute suspend help copy to avoid claiming memory snapshots.

Requirements context
I used the PR title/description as the behavioral source because I found no linked issue and no local doc that spells out the provider suspend semantics. Local repo docs say insta --help is authoritative for command listings (README.md:193-194), and command/flag reference mirroring is required for new or renamed commands/flags (AGENTS.md:16-17, .claude/skills/developing-insta-cli/SKILL.md:36-38); this PR changes only copy for an existing command/flag surface.

Findings

Critical
(none)

Suggestion
(none)

Information

  • src/index.ts:230-230: Software engineering/functionality: the change is limited to the Commander description for compute suspend and keeps the existing command registration, options, and computeCmd.computeSuspend action wiring intact. Existing lifecycle tests cover runtime output rather than help-copy strings (test/compute-restart.test.ts:24-38), which is acceptable for this static one-line wording fix.
  • src/index.ts:230-230: Security/performance: no security-relevant or performance-relevant behavior changed. There is no new untrusted input handling, logging, dependency, network path, loop, allocation, or I/O.

Verdict
Approved: no blocking findings. I did not rerun npm test or npm run typecheck because this review was constrained to read-only inspection; the PR description reports both passed.

@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

One-line help-copy fix on compute suspend that correctly kills a false "RAM snapshot" claim for the insta-compute plane — no blocking issues, but the replacement is now unconditionally true only on that plane, and the companion agent-facing doc still carries the exact stale wording this PR removes.

Requirements context

No matching spec/plan found — assessing against the PR description alone, plus the repo's own conventions. insta-cli has no /docs/superpowers/ (verified: no superpowers/, specs/ or plans/ directory anywhere in the tree). Intent taken from the PR title/body; conventions taken from AGENTS.md:14-17 and CONTRIBUTING.md:36-44.

Behavioural claims were checked against the control plane at InsForge/insta-platform@main (shallow clone), since the CLI is a thin client and suspend semantics live there.

Findings

Critical

(none)

Suggestion

Functionality — src/index.ts:230: "in-memory state does not [persist]" is stated unconditionally, but is only true on the insta-compute plane.
Fly-backed compute rows are still a live dispatch target on the platform:

  • insta-platform src/adapters/compute-provider.tsCOMPUTE_PROVIDERS = ['fly', 'insta-compute'], and computeDispatchKind() deliberately keeps routing rows to FlyAdapter (ref-shape wins over recorded kind).
  • insta-platform src/config.ts:167-173computeProviderKind is a create-time switch and its documented default is 'fly'; "existing rows keep the kind they were born with", so legacy Fly services persist regardless of what prod sets today.
  • insta-platform src/adapters/fly.ts:1536-1541suspendApp() issues POST /apps/:app/machines/:id/suspend, i.e. Fly's real memory-snapshot suspend. For those services, in-memory state does survive.

So the PR swaps a claim that was false on one plane for a claim that is false on the other. Suggest either naming the plane or using plane-neutral wording that is honest on both, e.g. …(scales to zero; disk state persists — do not rely on in-memory state surviving). The agent-facing reference already qualifies per-plane behaviour this way for restart (insta-skills insta/cli-reference.md:51).

Functionality — src/index.ts:230: "disk/volume state persists" describes a path the platform refuses.
insta-platform src/provisioning/services.ts:1301-1303 hard-rejects a suspend on any volume-bearing service:

this service has a /data volume — machines with volumes cannot be suspended (no RAM snapshot); use stop

That guard is unconditional (not provider-gated), and this CLI's own copy already says so at src/index.ts:139 — "Volume services keep 1 machine and stop (cold wake) instead of suspend when idle". Mentioning volume persistence in the suspend description therefore points at an unreachable case and mildly contradicts the --volume copy. disk state persists alone is accurate; if you want the copy to earn its length, the more useful fact is the 400 a volume user actually hits (a service with a /data volume cannot be suspended — use \stop``).

Project convention — companion doc not updated in the same change set (CONTRIBUTING.md:40-44, AGENTS.md:16-17).
The PR body states the insta-skills suspend copy "is already provider-neutral" — true for insta/cli-reference.md:50, but the grep that backed it was repo-local and missed the sibling repo's other reference file:

InsForge/insta-skills — insta/references/operate.md:102
- `insta compute suspend [service]` — snapshot RAM for a faster resume; stays down until `start`.

That is the same false claim this PR removes, in the file coding agents read via insta setup-agent. Not a blocker for merging this repo's change, but it leaves the fix half-landed per CONTRIBUTING.md:42-44 ("Update it in the same change set"). Worth a follow-up PR in InsForge/insta-skills.

Information

  • Vocabulary collision — "scales to zero". This CLI defines scale-to-zero as auto-wake behaviour (src/index.ts:266: "off = default scale-to-zero (idle instance suspends; first connection cold-starts)"). A manual suspend sets autostart:false so traffic will not wake it (insta-platform src/provisioning/services.ts:1283-1284). The trailing "stays down until start" does carry that, so the line is not wrong — but "releases the machine" would avoid overloading a term the same --help output uses for the opposite wake behaviour.
  • The copy no longer differentiates suspend from stop on the new plane. Per insta-platform src/adapters/insta-compute.ts:1629-1630, "the plane always suspends; the distinction is Fly's". With the RAM-snapshot differentiator gone, stop (src/index.ts:229) and suspend now read nearly identically to a user on insta-compute. That's a truthful consequence of the fix, not a defect of it — flagging only because "which one do I want?" is now unanswered by --help.
  • Test coverage — no gap. Nothing in test/ asserts commander description strings (no test imports src/index.ts; no helpInformation() usage anywhere), so shipping this without a new test matches existing convention rather than skipping TDD. Lifecycle behaviour stays covered by test/compute-restart.test.ts.
  • Gates verified locally at head 876bddb (not taken on trust): npx tsc --noEmit clean; npx vitest run47 test files / 672 tests passed, matching the PR body.
  • Security: no security-relevant changes in this PR. A static description string — no new user input reaching SQL/shell/HTTP, no secrets/PII logged or returned, no auth/authorization path touched, and package.json/package-lock.json are untouched (no new or unpinned dependencies).
  • Performance: no performance-relevant changes in this PR. No new queries, loops, allocations, or blocking I/O; the string is built once at commander registration time.

Verdict

approved — zero Critical findings; the change is accurate for the plane it targets and the gates are green. (Informational: the GitHub green check is still a separate human approval.) The two Suggestions are worth a quick copy tweak, and the insta-skills operate.md:102 line should get a follow-up so the fix lands everywhere agents read it.

Review round 1: legacy Fly-backed services still get a true RAM-snapshot
suspend, so "in-memory state does not persist" was unconditional in the
wrong direction; and volume-bearing services cannot be suspended at all,
so "volume state persists" pointed at an unreachable case. "Releases the
machine" also avoids overloading "scales to zero", which this help output
uses for auto-wake behavior.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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
The help copy change is narrowly scoped and accurately removes the misleading suspend semantics without touching runtime behavior.

Requirements context
I used the PR title/description as the main intent source: make insta compute suspend help provider-honest by avoiding RAM snapshot, volume persistence, and scale-to-zero wording. I found no linked issue in the provided metadata. In-repo guidance says commands are registered in src/index.ts and command/flag surface changes should be mirrored in the agent-facing reference for new or renamed command/flag changes (AGENTS.md:3-17, CONTRIBUTING.md:22-44); README treats insta --help as the authoritative command list (README.md:193-210). The checked-out commit changes only src/index.ts:230; local search found no remaining RAM snapshot or memory snapshot wording in this repo.

Findings

Critical
(none)

Suggestion
(none)

Information
(none)

Review dimensions
Software engineering: no issues; the change follows the existing Commander registration style in src/index.ts. Functionality: no issues; the final help text aligns with the stated plane-neutral suspend behavior. Security: no security-relevant changes. Performance: no performance-relevant changes.

I did not run npm run typecheck or npm test because the review instructions were read-only and tests can create local cache/artifact writes; the PR description reports both passed.

Verdict
approved

jwfing
jwfing previously requested changes Aug 31, 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.

Summary
The revision fixes the two problems flagged at 876bddb (the unreachable "volume state persists" clause and the "scales to zero" collision), but the replacement sentence swaps one plane's falsehood for the other's: disk state persists is false for every insta compute suspend on the insta-compute plane.

Requirements context
This repo has no /docs/superpowers/ — in fact no docs/ directory at all, and no docs/specs/. No matching spec/plan found — assessing against the PR description alone, plus the repo's own conventions in CONTRIBUTING.md:36-44, AGENTS.md:10-17 and .claude/skills/developing-insta-cli/SKILL.md. The PR body states the intent explicitly, and I hold the copy to it: "plane-neutral … the copy promises only what both planes deliver."

Note: an automated review already sits at this head (#pullrequestreview-5062589816) reporting no findings. It assessed the string against this repo only. This review adds verification against the two compute planes' current main (InsForge/insta-platform, InsForge/insta-compute) and reaches a different verdict.


Findings

Critical

Functionality — src/index.ts:230: "disk state persists" is false on the insta-compute plane, which is the only plane where the same sentence's "releases the machine" is true.

Trace of an insta compute suspend on an insta-compute-backed service, against current main of both repos:

  1. The platform rejects suspend outright on any volume-bearing service — insta-platform src/provisioning/services.ts:1299-1303: throw new BadRequestError('this service has a /data volume — machines with volumes cannot be suspended (no RAM snapshot); use stop'). Not provider-gated. So this verb only ever reaches volumeless services.
  2. insta-platform src/adapters/insta-compute.ts:1625-1627POST /services/<name>/suspend.
  3. The plane's SuspendService (insta-compute internal/runtime/k8s.go:825-855) scales the Deployment to 0, then finishSuspend flushes the img disk to object storage only under if svc.Volume != nil && meta.IsImgRef(svc.Volume.DiskRef). A volumeless service flushes nothing.
  4. The VM's writable root filesystem is a per-pod CoW clone of the shared code disk — internal/runtime/warm.go:1155-1160: "each claim gets a CoW clone … its own writable rootfs, like a container's overlay — deleted on release" — and k8s_warm.go:320-338 releaseWarmPod deletes runCodeRef(pod); k8s_warm.go:450-451 suspendWarm "releases the service's pool pods (deleted, never reused)".
  5. The interface contract states exactly what survives — internal/runtime/runtime.go:155: "SuspendService scales to 0; the volume and identity survive." The volume and identity. Not the rootfs.

So on insta-compute: nothing a running app wrote to disk survives a suspend, and the one disk that is durable across suspend (the /data volume, flushed at step 3) belongs to services the platform refuses to suspend at all. The claim is unreachable-true and reachable-false.

This matters more than the wording it replaced. RAM snapshot was a vague implication about durability; disk state persists is an explicit affirmative promise, placed in deliberate contrast with "do not rely on in-memory state surviving" — which invites exactly the reading "writes to disk survive, writes to memory don't." A user who trusts it (SQLite file, uploaded assets, a cache on the container FS) loses that data silently on the next suspend/start. That is the same class of harm this PR exists to remove, pointed at the plane it was written for.

Blocking because the PR's deliverable is the factual claim, the false clause is reachable on 100% of insta-compute suspends, and the fix is a few words.

Suggestion

Functionality — src/index.ts:230: "releases the machine" is false on the Fly plane.
insta-platform src/adapters/fly.ts:1535-1542 suspendApp posts /apps/:app/machines/:id/suspend — a real RAM snapshot; the machine is retained (fly.ts:1197-1199 documents a suspended machine that a config update moves to stopped, "because the update invalidates the RAM snapshot"). Fly-backed services are still live: insta-platform src/config.ts:169-173 documents computeProviderKind as a create-time switch — "existing rows keep the kind they were born with and are routed by it" — so legacy Fly rows persist indefinitely and computeDispatchKind keeps dispatching them.

Combined with the Critical: each clause of the new sentence is true on exactly one plane and false on the other, so the sentence is now wrong on both. The genuinely plane-invariant facts are narrower — the service stays down until start; in-memory state may not survive; only a /data volume is durable, and volume-bearing services can't suspend. Something like "takes the machine down until start; in-memory and on-disk state may not survive (only a /data volume is durable — and volume-bearing services must use stop)" is honest on both planes. On Fly it under-promises, which is the safe direction.

Functionality — the change contradicts this CLI's own copy elsewhere, and erases the suspend/stop distinction.
src/index.ts:139 still tells users "Volume services keep 1 machine and stop (cold wake) instead of suspend when idle", and src/commands/compute.ts:663 prints "suspend fast-wake and scale-out are back" after a volume delete. Both encode wake latency as the thing that makes suspend suspend. After this edit, suspend (line 230) and stop (line 229) state no user-visible difference at all — both read as "machine goes away, stays down until start" — so nothing in the help explains why you'd pick one. Wake latency is the real, plane-invariant distinction (sub-second on Fly, ~1s fresh boot per the PR body); keeping it would fix the copy without leaving the two verbs indistinguishable.

Software engineering — the agent-facing mirror still carries the exact stale claim.
InsForge/insta-skills insta/references/operate.md:102 reads "insta compute suspend [service]snapshot RAM for a faster resume; stays down until start" (verified against that repo's current main), and insta/cli-reference.md:26 still says "suspend fast-wake … return". CONTRIBUTING.md:41-44"A command or flag change is only half done until it is mirrored in the agent-facing command reference … Update it in the same change set" — names cli-reference.md, whose suspend row (line 50) is already provider-neutral, so the letter of the rule is met. The spirit isn't: those files are how coding agents learn this surface, and they'd keep teaching the exact sentence this PR removes. The PR body correctly identifies this and defers it; landing the two together would avoid a window where the CLI and its own docs disagree.

Information

Software engineering — no new test is correct here, not a TDD gap. Nothing in test/ (47 files) asserts a commander description: no helpInformation, no import of src/index.ts for help output (the three commander hits in test/compute-exec.test.ts and test/resolve-service.test.ts are about argv splitting, not descriptions). A test for this one line would be a new convention, not an existing one.

Gates re-run independently in a clean clone at cf823f7: npm run typecheck exits 0 with no output; npx vitest run47 test files / 672 tests passed. Matches the PR body exactly. Style and one-line backticked form match the surrounding lifecycle descriptions, and insta compute --help renders the em dash and wrapping cleanly.

Security — no security-relevant changes. A single static string in the command registration: no user input, no SQL/shell/HTTP path, no auth or authorization check, no secrets or PII, no dependency changes.

Performance — no performance-relevant changes. A literal evaluated once at command-registration time; no runtime path, query, loop or allocation is touched.


Verdict

request_changes — one Critical: src/index.ts:230 promises disk state persists on the one plane where suspend actually discards the writable rootfs. Everything else here is non-blocking. The scoping, style, and gate discipline of the change are otherwise clean, and the two findings from the previous head are properly resolved.

Review round 2 Critical: "disk state persists" is false on every
insta-compute suspend. The platform refuses suspend on volume-bearing
services (services.ts:1305-1307), so the verb only reaches volumeless
ones; there the VM's writable rootfs is a CoW clone deleted on release
(warm.go:1157-1159, k8s_warm.go:450-451) and finishSuspend flushes only
an img /data volume (k8s.go:843-854). The copy now promises nothing about
durability, states the plane-invariant facts, and points volume-bearing
services at `stop`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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
The change removes the false RAM snapshot help claim for insta compute suspend and I found no blocking issues.

Requirements Context
I derived the intent from the PR title and description: update compute suspend help to be provider-neutral and warn users not to rely on memory or container disk state surviving. The repo docs say this CLI is a thin platform API client (AGENTS.md:3-6), insta --help is authoritative while the full command reference lives in insta-skills (README.md:193-209), and command/flag surface changes should be mirrored there (AGENTS.md:15-17, .claude/skills/developing-insta-cli/SKILL.md:36-38). I did not find an in-repo requirements document defining suspend durability semantics beyond the PR description and local command-surface docs.

Findings
Critical:
(none)

Suggestion:
(none)

Information:

  • Software engineering: no issues. The one-line commander description change follows the existing src/index.ts style for long lifecycle/help descriptions (src/index.ts:226-246). I did not rerun npm run typecheck or npm test under the read-only review constraint; the PR description reports both passed.
  • Functionality: no issues. The changed text is attached to compute.command('suspend [service]') (src/index.ts:230-231), while computeSuspend still dispatches the unchanged lifecycle API call (src/commands/compute.ts:353-367), so this updates help copy without altering runtime behavior.
  • Security: no security-relevant changes. Only static help text changed (src/index.ts:230-230); no input handling, auth, logging, dependencies, SQL, shell, or HTTP construction changed.
  • Performance: no performance-relevant changes. No runtime logic or hot path changed; the CLI still registers a static command description (src/index.ts:230-230).

Verdict
Approved: no Critical findings. Per the InsForge bot rule, this is a non-blocking review verdict rather than a GitHub green-check approval.

@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

Round 3 @ 8af2795. The round-2 blocker is fixed — the false disk state persists promise is gone, and the new clause under-promises on both planes, which is the safe direction; one clause (until \start`) is still false on the insta-compute plane, but it is carried over verbatim in meaning from main` and is not amplified here, so it does not block.

Supersedes my CHANGES_REQUESTED at cf823f7 (#pullrequestreview-5062600424) — that Critical is resolved; I am dismissing it.

Requirements context

No matching spec/plan found — this repo has no docs/ directory at all (no docs/superpowers/, no specs/, no plans/). Assessed against the PR title/body, CONTRIBUTING.md, .claude/skills/developing-insta-cli/SKILL.md, and against current main of both compute planes (InsForge/insta-platform @ bbbfae8, InsForge/insta-compute @ cf497ff) — the same cross-repo method the PR body itself uses.

Round-2 Critical: verified resolved

Re-checked every durability claim against insta-compute @ cf497ff:

  • SuspendService scales the Deployment to 0 and calls finishSuspend (internal/runtime/k8s.go:825-830); finishSuspend flushes bytes to object storage only under svc.Volume != nil && meta.IsImgRef(...) (internal/runtime/k8s.go:843-854) — a volumeless service, the only kind that can reach suspend, flushes nothing.
  • The interface contract states exactly what survives: "SuspendService scales to 0; the volume and identity survive" (internal/runtime/runtime.go:155).
  • On Fly, suspendApp really is a RAM snapshot (insta-platform src/adapters/fly.ts:1536-1541), and Fly rows are still live (src/config.ts:169-173, create-time switch).

do not rely on in-memory or on-disk state surviving is a negative, so it is exact on insta-compute and under-promises on Fly. Correct on both planes. This was the right fix.

The parenthetical also checks out and is genuinely provider-neutral: insta-platform src/provisioning/services.ts:1301-1302 throws before any adapter dispatch, gated on service.volume_gib != null, not on computeProviderKind.

Gates re-run in my own clone at 8af2795: npm run typecheck clean; npx vitest run47 files / 672 tests passed (matches the PR body); COLUMNS=100 npx tsx src/index.ts compute --help renders and wraps cleanly.


Findings

Critical

(none)

Suggestion

1. Functionality — until \start`is false on the insta-compute plane, and the PR body's verification of it cites a Fly-only mechanism ·src/index.ts:230`

The PR body says of "takes the machine down until start":

stop/suspend set autostart:false so the router cannot wake the app; start restores it (insta-platform src/provisioning/services.ts:1288-1289). True on both planes.

That is a Fly-only mechanism. The insta-compute adapter's setAutostop takes two parameters — it drops the { autostart } opt entirely, and says so in its own comment:

insta-platform src/adapters/insta-compute.ts:1633"opts.autostart has no plane equivalent (wake-on-request is constitutive); the desired-state verbs own start/stop."

And the desired-state verbs do not carry the intent either. Both stopApp and suspendApp funnel into the same body-less call (src/adapters/insta-compute.ts:1608-1627):

const r = await this.call('POST', `/services/${name}/${verb}`)   // no body

The plane treats a body-less suspend as the historical one (insta-compute internal/api/api.go:918-923):

"The body is OPTIONAL and backward compatible: absent/empty = the historical suspend (the row wakes on traffic); {"stop": true} = the tenant's durable stop (#138) — the executor persists Service.Stopped with the suspend, and the service then stays offline until an explicit wake or deploy."

So Service.Stopped stays false, and the router takes the wake branch (insta-compute internal/router/router.go:601-618):

case meta.StateSuspended:
    if svc.Stopped { /* 503 "service is stopped", no wake enqueued */ }
    // Hold-and-wake: enqueue one wake per service …
    res := r.awaitWake(req.Context(), svc)

Concretely: on an insta-compute-backed project, insta compute suspend web takes the service down, and the next HTTP request to its route — a crawler, an uptime check, a stale browser tab — wakes it back up and bills it as uptime. No start involved. A user who read the help text believes the service is off.

Why this is a Suggestion and not a blocker: main already says "stays down until start" — semantically the same claim — so this PR neither introduces nor amplifies it, and it is a strict net improvement over main (the RAM-snapshot lie and the disk-durability lie are both gone). The same falsehood also sits, in a stronger form, on the sibling line this PR does not touch (src/index.ts:229: "traffic will NOT wake it until start") and in the agent-facing reference (insta-skills insta/cli-reference.md:50). Blocking here would leave main with the worse copy.

Recommendation, in order of preference:

  1. The real fix is in insta-platform, not here — have the insta-compute adapter send {"stop": true} on stopApp/suspendApp. The plane handler exists precisely for this caller (api.go:940-947 names it: "the one caller this exists for is a cross-repo integration"), and once it lands, both line 229 and line 230 become true on both planes with no CLI change. Note the handler is strict: unknown fields, {"stopped": true}, and {"stop": null} are all 400s by design — send exactly {"stop": true}.
  2. Failing that, soften the clause (e.g. "takes the machine down; start brings it back"), which is true on both planes without asserting traffic cannot wake it.

At minimum, please correct the clause-1 justification in the PR body before squash — it becomes the commit message, and it currently records a plane-specific mechanism as verified on both planes.

2. Software engineering — the audit leaves two sibling strings in this repo asserting the property it just removed · src/index.ts:246, src/commands/compute.ts:663

Both still promise that deleting a volume brings back "suspend fast-wake":

  • src/index.ts:246"billing stops now, and suspend fast-wake + scale-out return"
  • src/commands/compute.ts:663"the disk and its data are gone; suspend fast-wake and scale-out are back"

"Fast-wake" is the Fly RAM-snapshot framing this PR removed from line 230 — on insta-compute there is no snapshot at all (k8s.go:825-830), so a volumeless service's wake is a cold pod start. The PR body declines a suspend-vs-stop differentiator on the grounds that "wake latency is not plane-invariant"; that reasoning applies verbatim to these two strings, which the change set left alone. (Also mirrored at insta-skills insta/cli-reference.md:26.) Not in the one-line scope, but they are the remaining instances of exactly the claim this PR exists to retire — worth a follow-up in this repo.

3. Project conventions — the docs mirror is met in letter, not in spirit · CONTRIBUTING.md:41-44

The mandated mirror target is insta-skills insta/cli-reference.md, and the PR body is right that its suspend row (line 50) makes no RAM-snapshot claim, so the rule's letter is satisfied. But insta-skills insta/references/operate.md:102 still reads:

"insta compute suspend [service]snapshot RAM for a faster resume; stays down until start."

That is the exact sentence this PR exists to retire, in the file agents read for operational guidance. The PR body discloses it as a follow-up; landing that follow-up alongside the merge (rather than after) avoids a release cycle where the CLI's help and the agent reference disagree about whether memory survives.

Information

  • Test coverage — no test anywhere in test/ (47 files) asserts a commander .description() string; the four files that mention description( do so incidentally. Shipping a copy change with no new test is this repo's convention, not a TDD gap. .claude/skills/developing-insta-cli/SKILL.md prescribes npm test + npm run typecheck as the gate, and both are green at this head.
  • Security — no security-relevant changes in this PR. One string literal, no new input path, no dependency change, no auth surface touched.
  • Performance — no performance-relevant changes in this PR. A commander description is evaluated once at program construction; no runtime path is affected.
  • Style — the description's length is consistent with its neighbours (restart at src/index.ts:231 and volume at :246 are both substantially longer), and it wraps correctly at 100 columns.

Verdict

approved (informational — a human still gives the GitHub approval via the approve flow). Zero Critical findings: the round-2 blocker is genuinely fixed and the new clause is safe on both planes. The three Suggestions are non-blocking — the until \start`gap is a pre-existing, unamplified claim whose real fix lives ininsta-platform`'s adapter, and the two docs items are follow-ups the PR body already anticipates.

@jwfing
jwfing dismissed their stale review August 31, 2026 04:29

Resolved at 8af2795: the false disk state persists promise is gone. Superseded by #162 (review)

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 1 file (changes from recent commits).

You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

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/index.ts">

<violation number="1" location="src/index.ts:230">
P2: The copy now errs on the opposite plane from the claim it replaces. 'do not rely on in-memory or on-disk state surviving' is accurate for insta-compute (per the reviewer note that disk state doesn't persist there), but a false warning for legacy Fly-backed services, which per the PR description still get memory-snapshot suspend with persistent disk state. Since the PR's goal is copy that promises only what both planes deliver, stating that neither in-memory nor on-disk state survives still isn't plane-neutral — it swaps the falsehood from the insta-compute plane to the Fly plane. Keep the wording to the common denominator (the machine is down until `start`) and drop the blanket state-survival claim, or scope it explicitly to insta-compute.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/index.ts
compute.command('stop [service]').description('Take a compute service offline; traffic will NOT wake it until `start`')
.option('--json').option('--branch <branch>', 'branch (default: current)').action(guard((service, o) => computeCmd.computeStop(service, o)))
compute.command('suspend [service]').description('Suspend a compute service (RAM snapshot); stays down until `start`')
compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` — do not rely on in-memory or on-disk state surviving (a service with a /data volume cannot be suspended; use `stop`)')

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: The copy now errs on the opposite plane from the claim it replaces. 'do not rely on in-memory or on-disk state surviving' is accurate for insta-compute (per the reviewer note that disk state doesn't persist there), but a false warning for legacy Fly-backed services, which per the PR description still get memory-snapshot suspend with persistent disk state. Since the PR's goal is copy that promises only what both planes deliver, stating that neither in-memory nor on-disk state survives still isn't plane-neutral — it swaps the falsehood from the insta-compute plane to the Fly plane. Keep the wording to the common denominator (the machine is down until start) and drop the blanket state-survival claim, or scope it explicitly to insta-compute.

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

<comment>The copy now errs on the opposite plane from the claim it replaces. 'do not rely on in-memory or on-disk state surviving' is accurate for insta-compute (per the reviewer note that disk state doesn't persist there), but a false warning for legacy Fly-backed services, which per the PR description still get memory-snapshot suspend with persistent disk state. Since the PR's goal is copy that promises only what both planes deliver, stating that neither in-memory nor on-disk state survives still isn't plane-neutral — it swaps the falsehood from the insta-compute plane to the Fly plane. Keep the wording to the common denominator (the machine is down until `start`) and drop the blanket state-survival claim, or scope it explicitly to insta-compute.</comment>

<file context>
@@ -227,7 +227,7 @@ compute.command('start [service]').description('Bring a compute service online (
 compute.command('stop [service]').description('Take a compute service offline; traffic will NOT wake it until `start`')
   .option('--json').option('--branch <branch>', 'branch (default: current)').action(guard((service, o) => computeCmd.computeStop(service, o)))
-compute.command('suspend [service]').description('Suspend a compute service (releases the machine; disk state persists — do not rely on in-memory state surviving); stays down until `start`')
+compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` — do not rely on in-memory or on-disk state surviving (a service with a /data volume cannot be suspended; use `stop`)')
   .option('--json').option('--branch <branch>', 'branch (default: current)').action(guard((service, o) => computeCmd.computeSuspend(service, o)))
 compute.command('restart [service]').description("Restart a compute service by re-running the image it already runs against a freshly resolved env bundle — this is how a changed secret or binding reaches a running machine (env is baked into the machine at deploy time), and how a machine that is up but wedged gets cycled (`start` no-ops on one that is already started). No new image, no new spec. The service must be running: a stopped or suspended one comes back with `insta compute start`. All plans; gated: deploy — it lands configuration the same way a deploy does, so a policy denying deploys denies this too (`start`/`stop` stay ungated, and cycle a wedged machine without one). A service whose app fails to answer on its port coming back up reports that failure, and the machines are rolled back, best-effort, to the config they were serving")
</file context>
Suggested change
compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` — do not rely on in-memory or on-disk state surviving (a service with a /data volume cannot be suspended; use `stop`)')
compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` (a service with a /data volume cannot be suspended; use `stop`)')

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