miren deploy: wait for healthy and report the truth

[phinze]

miren deploy used to call it done too early. The instant it flipped the deployment record to active server-side, it printed "All traffic moved to new version" and exited, without ever waiting to see the new instance serve a request. The pointer moved, but whether anything was actually answering was anyone's guess. A crash-looping app, a still-booting app, and a healthy one all looked exactly the same from the deploy output.

Now deploy waits for the new version to actually come up healthy before it reports, and tells you what really happened: live and serving, or never became healthy with the crash logs to show why. A version that never comes up exits non-zero, so CI and scripts catch it. "Healthy" is read from the signal we already trust (sandbox RUNNING plus the network health check, read through app-status), not a new definition invented in the deploy path, so it'll ride along with whatever the richer health-checks work (MIR-1251) lands later.

It's three commits. The first gets the readiness signal to where deploy can see it: the pool-state-to-health derivation already existed but only fed m app list, not the response the deploy poller reads, so it moves into a shared helper and onto ApplicationStatus. The second is the consumer side, the poller and the reporting, wired into deploy, rollback, and env set/env delete. The third closes a producer-side gap: a redeploy that reused a pool which had drained to zero (an autoscale app gone idle) pointed the pool at the new version without ever booting it, so we'd have nothing to health-check. Fresh pools already seed one instance on deploy; this floors a drained pool back to one the same way, so every deploy actually boots and verifies what it shipped, then scales back down.

A couple of things fell out of this naturally. The port-divergence warning from MIR-1246 (when an app ignores $PORT and binds somewhere else) now shows up as a real deploy-time warning. And the wait renders as a phase of the existing build TUI instead of flat text printed underneath it, so build and health read as one process. That last bit meant working with bubbletea's scrollback-versus-live-region behavior: finished lines (including the health verdict) get committed with tea.Println, and only the spinner lives in the redrawn region.

One caveat worth flagging: the TUI render path is verified by hand (success, crash-loop, port-divergence, scaled-to-zero), not by tests, since bubbletea plus a real TTY don't unit-test well. The decision and reporting logic underneath it is covered.

Closes MIR-1252

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR adds shared app health constants and extends the RPC schema and generated types with health, readiness, crash, cooldown, and bound-port fields. Server code now aggregates pool health and bound-port divergence into app status responses. The CLI replaces activation polling with health-wait flows, updates deploy UI phase handling, and rewires deploy, rollback, env, blackbox deploy-error, and launcher paths to wait for health before reporting success.

---

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

[phinze]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

servers/app/runtime.go (1)

159-162: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Return an explicit health state when pool lookup fails.

This branch logs the sandbox-pool lookup failure but leaves rai.Health unset. The RPC contract now documents health as one of the shared apphealth values, and the deploy health waiter consumes that field; returning "" here can turn a transient backend failure into ambiguous client behavior instead of a clear unknown state. Set apphealth.Unknown in the error branch before continuing.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/app/runtime.go` around lines 159 - 162, The sandbox-pool lookup
failure path in runtime.go logs the error but leaves the health field empty, so
update the error branch in the logic around a.EC.List and rai.Health to
explicitly set apphealth.Unknown before continuing. Keep the existing warning
log, but ensure the returned health is always one of the shared apphealth
values, especially when poolsResp lookup fails.

servers/app/app.go (1)

165-182: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Don't report active zero-pool apps as unknown.

poolStateMap only gets an entry when a pool entity exists, so an app with an active version but zero current pools falls through to apphealth.Unknown here. servers/app/runtime.go classifies the same empty-pool state as idle via health.classify(), which means m app list can disagree with app status/deploy for autoscale-to-zero or just-activated apps. Seed a zero-valued poolHealth for active versions and emit 0 ready/desired so both surfaces share the same contract.

Also applies to: 240-246

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/app/app.go` around lines 165 - 182, The app health aggregation in the
pool listing path is treating active versions with no current pools as unknown
because poolStateMap is only populated from existing pool entities. Update the
logic around poolStateMap, poolHealth.accumulate, and the autoscale app health
classification so active versions always get a zero-valued poolHealth entry even
when no pools exist, and ensure ready/desired are emitted as 0 in that case.
Mirror the same handling in the related health summary code so the app list and
app status/deploy surfaces both classify zero-pool active apps as idle instead
of unknown.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cli/commands/app_rollback.go`:
- Around line 123-126: The rollback command currently prints a success message
before the health check completes, so move the “✓ Rolled back” wording out of
the pre-check path in appRollback and awaitHealthy flow. Update the rollback
progress output in rollback/app_rollback.go to use an in-progress message before
calling awaitHealthy, and only emit the success message after awaitHealthy
returns nil so the final status matches the health verdict.

In `@cli/commands/deploy_ui.go`:
- Around line 361-373: Finalize the in-flight upload before processing
buildDoneMsg in deployUI's message handler, since clearing m.isUploading
directly can drop the upload summary when the build finishes before the
buildProgress/Launching builder transition. Update the buildDoneMsg case to
commit the upload phase first using the existing phase-completion flow (the same
mechanism used for build and other phases), then reset m.isUploading and
continue with the activating state so fast or cached builds preserve the upload
phase in the TUI.

In `@cli/commands/deploy.go`:
- Around line 217-220: The deploy flow in deploy.go is reporting success too
early: the ctx.Printf call in the deploy path prints a checked “Deployed”
message before awaitHealthy can still fail. Change the messaging around the
deploy/health-check sequence so the initial message is neutral progress text,
then print the success checkmark only after awaitHealthy returns nil, keeping
the version/deploy status accurate in the deploy command.

In `@cli/commands/env.go`:
- Around line 158-163: The env update flow is printing a success checkmark
before the new version is confirmed healthy in the command path around
awaitHealthy. Update the user-facing messages in the env command flow (including
the code that prints after setting env vars and the analogous version creation
path) so they are neutral “new version created” notices without a success
symbol, and let awaitHealthy be the only place that reports final success or
failure for the new version.

In `@cli/commands/health_wait.go`:
- Around line 294-298: The timeout path in the health wait logic is discarding
the result of the final poll, so a terminal state can still be reported as a
timeout. Update the `deadline` handling in `check()`-based flow so the last
`check()` result is captured and, if it reclassifies the version as healthy,
scaled-to-zero, or crashed, return that outcome instead of always forcing
`outcomeTimeout`. Apply the same fix in both timeout branches that currently
refresh `snap` but ignore the terminal decision.
- Around line 387-411: The TUI flow in health_wait.go is treating a canceled
health outcome as a normal failure by passing it through healthOutcomeText and
reportHealthResult, which can render it as a timeout and emit crash logs. Update
the final-result handling in the wait/healthDone path to check for the canceled
outcome before formatting or reporting details, and return the cancellation
error immediately just like the plain path does. Use the existing
outcomeCanceled/ctx.Err() handling and keep the logic around waitFinal,
healthOutcomeText, and reportHealthResult consistent between the two paths.

In `@servers/app/health.go`:
- Around line 43-95: The collectBoundPortDivergence path is doing an unscoped
KindSandbox list and filtering in memory, which makes every health poll scan the
whole cluster. Update AppInfo.collectBoundPortDivergence to query sandboxes
through a pool/version-scoped entity ref (or add a scoped ref helper) instead of
calling EC.List on all sandboxes, while preserving the existing pool label and
RUNNING/PENDING filtering logic. Keep the bound-port extraction behavior in
place, but ensure the lookup is narrowed before iterating.

---

Outside diff comments:
In `@servers/app/app.go`:
- Around line 165-182: The app health aggregation in the pool listing path is
treating active versions with no current pools as unknown because poolStateMap
is only populated from existing pool entities. Update the logic around
poolStateMap, poolHealth.accumulate, and the autoscale app health classification
so active versions always get a zero-valued poolHealth entry even when no pools
exist, and ensure ready/desired are emitted as 0 in that case. Mirror the same
handling in the related health summary code so the app list and app
status/deploy surfaces both classify zero-pool active apps as idle instead of
unknown.

In `@servers/app/runtime.go`:
- Around line 159-162: The sandbox-pool lookup failure path in runtime.go logs
the error but leaves the health field empty, so update the error branch in the
logic around a.EC.List and rai.Health to explicitly set apphealth.Unknown before
continuing. Keep the existing warning log, but ensure the returned health is
always one of the shared apphealth values, especially when poolsResp lookup
fails.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a7848fc-244e-4bf8-b263-cefab5a8c14a

📥 Commits

Reviewing files that changed from the base of the PR and between 9d1c679 and 47a2be5.

📒 Files selected for processing (16)

• api/app/app_v1alpha/rpc.gen.go
• api/app/rpc.yml
• blackbox/error_test.go
• cli/commands/activation_poller.go
• cli/commands/app_rollback.go
• cli/commands/deploy.go
• cli/commands/deploy_display.go
• cli/commands/deploy_ui.go
• cli/commands/env.go
• cli/commands/health_wait.go
• cli/commands/health_wait_test.go
• pkg/apphealth/apphealth.go
• servers/app/app.go
• servers/app/health.go
• servers/app/health_test.go
• servers/app/runtime.go

💤 Files with no reviewable changes (1)

• cli/commands/activation_poller.go

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

servers/app/runtime.go (1)

169-203: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Mirror fixed-service detection before classifying app-status health.

health is initialized as autoscale and never corrected for fixed services before health.classify(). The list path checks the resolved config and flips isAutoscale for Concurrency.Mode == "fixed"; app-status should do the same, otherwise deploy can treat fixed apps with no ready/desired instances as autoscale-idle and report the wrong outcome.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/app/runtime.go` around lines 169 - 203, The app-status health path is
treating all pools as autoscale because health is initialized in the pools loop
and never corrected for fixed services before health.classify(). Mirror the
fixed-service detection used in the list path by checking the resolved config’s
Concurrency.Mode and flipping health.isAutoscale to false when it is "fixed",
using the same runtime/app-status flow around poolHealth, classify(), and the
config resolution logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@servers/app/app.go`:
- Around line 245-252: The active-version/no-pools branch in app health
classification is too broad because it marks every “just activated” app as idle;
update the logic in the `entry.activeVersion` handling to check the resolved
active-version config and only call `a.SetHealth(ps.classify())` for services
that can autoscale to zero. For fixed services, keep the status non-terminal
instead of classifying it as idle, so the `poolHealth` path and the app-state
outcome in `servers/app/app.go` reflect whether the app can actually scale down.

---

Outside diff comments:
In `@servers/app/runtime.go`:
- Around line 169-203: The app-status health path is treating all pools as
autoscale because health is initialized in the pools loop and never corrected
for fixed services before health.classify(). Mirror the fixed-service detection
used in the list path by checking the resolved config’s Concurrency.Mode and
flipping health.isAutoscale to false when it is "fixed", using the same
runtime/app-status flow around poolHealth, classify(), and the config resolution
logic.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fddefae1-9600-4090-bd3b-59fbd69b77ac

📥 Commits

Reviewing files that changed from the base of the PR and between 47a2be5 and f301189.

📒 Files selected for processing (16)

• api/app/app_v1alpha/rpc.gen.go
• api/app/rpc.yml
• blackbox/error_test.go
• cli/commands/activation_poller.go
• cli/commands/app_rollback.go
• cli/commands/deploy.go
• cli/commands/deploy_display.go
• cli/commands/deploy_ui.go
• cli/commands/env.go
• cli/commands/health_wait.go
• cli/commands/health_wait_test.go
• pkg/apphealth/apphealth.go
• servers/app/app.go
• servers/app/health.go
• servers/app/health_test.go
• servers/app/runtime.go

💤 Files with no reviewable changes (1)

• cli/commands/activation_poller.go

✅ Files skipped from review due to trivial changes (1)

• cli/commands/deploy_display.go

🚧 Files skipped from review as they are similar to previous changes (12)

• pkg/apphealth/apphealth.go
• blackbox/error_test.go
• servers/app/health.go
• api/app/rpc.yml
• servers/app/health_test.go
• cli/commands/app_rollback.go
• cli/commands/env.go
• api/app/app_v1alpha/rpc.gen.go
• cli/commands/health_wait_test.go
• cli/commands/deploy_ui.go
• cli/commands/deploy.go
• cli/commands/health_wait.go

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cli/commands/deploy_ui.go`:
- Around line 365-382: The phase summary output in the deploy UI can print out
of order because the upload completion and build completion commands are
appended separately while using concurrent command execution. In the update flow
around m.isUploading, m.completePhase, and buildStepsSummary, change the command
construction so “Upload artifacts” is emitted before “Build & push image” using
an ordered mechanism like tea.Sequence or a single combined command.

In `@servers/app/runtime.go`:
- Around line 223-228: The bound-port divergence check in runtime.go is
incorrectly gated on health.ready, which can hide the warning when a bad port is
preventing readiness. Update the logic around collectBoundPortDivergence in the
AppInfo/health path to track whether any pool has a running/current instance
during the loop, and use that boolean as the scan guard instead of ready > 0.
Keep the divergence scan tied to actual instance activity so the warning can
appear even before the app becomes healthy.
- Around line 170-194: The fixed-service detection in the pool health path is
happening too late inside the pool iteration, so an empty pool list leaves
health.isAutoscale incorrectly true; move the spec-based check earlier in the
same runtime health flow and initialize health.isAutoscale from the resolved
config before iterating pools. Use the existing coreutil.ResolveConfig result
and the poolHealth setup in runtime.go to set the correct autoscale/fixed mode
up front, then keep the per-pool loop focused on accumulating pool state.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 045efe57-7652-45ba-8c65-2e313c4cd298

📥 Commits

Reviewing files that changed from the base of the PR and between f301189 and 2830d57.

📒 Files selected for processing (16)

• api/app/app_v1alpha/rpc.gen.go
• api/app/rpc.yml
• blackbox/error_test.go
• cli/commands/activation_poller.go
• cli/commands/app_rollback.go
• cli/commands/deploy.go
• cli/commands/deploy_display.go
• cli/commands/deploy_ui.go
• cli/commands/env.go
• cli/commands/health_wait.go
• cli/commands/health_wait_test.go
• pkg/apphealth/apphealth.go
• servers/app/app.go
• servers/app/health.go
• servers/app/health_test.go
• servers/app/runtime.go

💤 Files with no reviewable changes (1)

• cli/commands/activation_poller.go

✅ Files skipped from review due to trivial changes (1)

• api/app/app_v1alpha/rpc.gen.go

🚧 Files skipped from review as they are similar to previous changes (10)

• pkg/apphealth/apphealth.go
• blackbox/error_test.go
• cli/commands/deploy_display.go
• api/app/rpc.yml
• cli/commands/app_rollback.go
• servers/app/health_test.go
• cli/commands/env.go
• cli/commands/health_wait.go
• cli/commands/health_wait_test.go
• cli/commands/deploy.go

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

servers/app/app.go (1)

165-182: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Filter app-list health aggregation to active-version pools only.

List aggregates every SandboxPool by app name, while ApplicationStatus scopes pools to the active version. If old-version pools still exist, m app list can classify the newly active version from stale pools and disagree with deploy/app-status health. Track active version IDs separately and skip non-active pools before ps.accumulate.

Suggested fix

 	var apps []appEntry
 	specMap := make(map[string]*core_v1alpha.ConfigSpec)
+	activeVersionIDs := make(map[string]struct{})
@@
 		if app.ActiveVersion != "" {
 			var appVer core_v1alpha.AppVersion
 			if verEnt, err := r.EC.GetByIdWithEntity(ctx, entity.Id(app.ActiveVersion), &appVer); err == nil {
 				entry.activeVersion = &appVer
 				entry.activeVersionEntity = verEnt
+				activeVersionID := entity.Id(app.ActiveVersion).String()
+				activeVersionIDs[activeVersionID] = struct{}{}
 				if resolvedCfg, err := coreutil.ResolveConfig(ctx, r.EC.EAC(), &appVer); err == nil {
-					specMap[appVer.ID.String()] = resolvedCfg
+					specMap[activeVersionID] = resolvedCfg
 				}
 			}
 		}
@@
 		poolList.Read(&pool)
 
+		if _, ok := activeVersionIDs[pool.SandboxSpec.Version.String()]; !ok {
+			continue
+		}
+
 		appName := ui.CleanEntityID(pool.App.String())

Also applies to: 240-253

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@servers/app/app.go` around lines 165 - 182, The app-list health aggregation
in List is counting all SandboxPool records for an app, which can let stale
version pools influence the result and diverge from ApplicationStatus. Update
the aggregation flow around poolList iteration, appName, and ps.accumulate to
first determine the active version IDs for each app and skip any pool that does
not belong to the active version before accumulating health.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@controllers/deployment/launcher.go`:
- Around line 435-447: The reused drained-pool path in ensurePoolForService is
still returning an empty pool ID after flooring DesiredInstances back to 1,
which skips the readiness wait in reconcileAppVersion. Update the branch that
revives a drained pool so it returns the reused pool’s ID when it changes
DesiredInstances from 0 to 1, while keeping the existing update/logging
behavior, so downstream cleanup waits for the booted pool before removing old
versions.

---

Outside diff comments:
In `@servers/app/app.go`:
- Around line 165-182: The app-list health aggregation in List is counting all
SandboxPool records for an app, which can let stale version pools influence the
result and diverge from ApplicationStatus. Update the aggregation flow around
poolList iteration, appName, and ps.accumulate to first determine the active
version IDs for each app and skip any pool that does not belong to the active
version before accumulating health.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6b56d235-bc31-44d9-b46e-61acbdef04cc

📥 Commits

Reviewing files that changed from the base of the PR and between 2830d57 and 2c823b6.

📒 Files selected for processing (17)

• api/app/app_v1alpha/rpc.gen.go
• api/app/rpc.yml
• blackbox/error_test.go
• cli/commands/activation_poller.go
• cli/commands/app_rollback.go
• cli/commands/deploy.go
• cli/commands/deploy_display.go
• cli/commands/deploy_ui.go
• cli/commands/env.go
• cli/commands/health_wait.go
• cli/commands/health_wait_test.go
• controllers/deployment/launcher.go
• pkg/apphealth/apphealth.go
• servers/app/app.go
• servers/app/health.go
• servers/app/health_test.go
• servers/app/runtime.go

💤 Files with no reviewable changes (1)

• cli/commands/activation_poller.go

✅ Files skipped from review due to trivial changes (1)

• api/app/app_v1alpha/rpc.gen.go

🚧 Files skipped from review as they are similar to previous changes (12)

• pkg/apphealth/apphealth.go
• blackbox/error_test.go
• cli/commands/deploy_display.go
• api/app/rpc.yml
• cli/commands/env.go
• cli/commands/app_rollback.go
• servers/app/health_test.go
• cli/commands/health_wait_test.go
• cli/commands/deploy_ui.go
• servers/app/health.go
• cli/commands/deploy.go
• cli/commands/health_wait.go

[evanphx]

A nice improvement! I was wondering if a faster version could just make sure that the new version booted, but then it's a question of how, and then we're back to port checking!