This is ready for human review. The REST-500 defect from my last review is fixed, and the fix avoids both traps I flagged. I'm not recommending approval yet: two decisions still need a person, and the PR description is out of date.
What b1cbc5b (Give a disabled app one refusal) changed. ErrAppDisabled now comes from a dedicated disabledError (pkg/deploylifecycle/tracker.go:99-136). It has an Is(ErrAppDisabled) match, ErrorCode() == "conflict" and the category app-disabled. It does not match cond.ErrConflict{}, which matters in two places:
commitActivation now returns straight out of the SetConfiguration loop and out of createNewVersion in api/app/envvar.go. Before, both would have cleaned up and retried.FailIfUnsettled only swallows conflicts.writeRESTError (pkg/rpc/rest.go) uses errors.As, so a caller that wraps the error with %w still gets a 409. Every refusal now goes through CheckEnabled, so they all read the same:
commitActivationDeployVersionrestart (servers/app/app.go:979)servers/sandbox/pools.go:224)The deploy-task check for a disable that lands mid-build is still in place (build_saga.go:722). The new tracker test covers the properties that matter: ErrorIs(ErrAppDisabled), NotErrorIs(cond.ErrConflict{}), the "conflict" code, and the remote error rebuilt on the client side.
A small leftover, not a blocker. Enable's "app %q is not disabled" (servers/app/disable.go) is still a plain fmt.Errorf. So POST /api/v1/apps/{app}/enable on an app that's already enabled returns a 500 over REST. A cond.Conflict is safe there because Enable has no retry loop. It's worth the one-line fix if Cloud will call this endpoint.
Still standing:
scaling.md fits his fallback flow.pkg/workloadroles/roles.go) needs sign-off from whoever owns the roles model.disabled health value.Posted to the PR's comment threads when you submit.
b1cbc5b. disabledError matches ErrAppDisabled and returns "conflict" from ErrorCode(), so writeRESTError answers 409. It uses errors.As, so wrapped errors get the same status. It deliberately doesn't match cond.ErrConflict{}, so neither the SetConfiguration/createNewVersion retry loops nor FailIfUnsettled read a disable as a lost race. The new tracker test asserts both, plus the round-trip through cond.RemoteError. Restart and pool scale-up use the same type now as well. Resolving.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1431,
"verdict": "ready",
"event": "comment",
"summary": "This is ready for human review. The REST-500 defect from my last review is fixed, and the fix avoids both traps I flagged. I'm not recommending approval yet: two decisions still need a person, and the PR description is out of date.\n\n**What `b1cbc5b` (Give a disabled app one refusal) changed.** `ErrAppDisabled` now comes from a dedicated `disabledError` (`pkg/deploylifecycle/tracker.go:99-136`). It has an `Is(ErrAppDisabled)` match, `ErrorCode() == \"conflict\"` and the category `app-disabled`. It does not match `cond.ErrConflict{}`, which matters in two places:\n- A disable caught by `commitActivation` now returns straight out of the `SetConfiguration` loop and out of `createNewVersion` in `api/app/envvar.go`. Before, both would have cleaned up and retried.\n- A refused attempt is still recorded as failed, because `FailIfUnsettled` only swallows conflicts.\n\n`writeRESTError` (`pkg/rpc/rest.go`) uses `errors.As`, so a caller that wraps the error with `%w` still gets a 409. Every refusal now goes through `CheckEnabled`, so they all read the same:\n- Begin and `commitActivation`\n- the build entry points and `DeployVersion`\n- `restart` (`servers/app/app.go:979`)\n- pool scale-up (`servers/sandbox/pools.go:224`)\n\nThe deploy-task check for a disable that lands mid-build is still in place (`build_saga.go:722`). The new tracker test covers the properties that matter: `ErrorIs(ErrAppDisabled)`, `NotErrorIs(cond.ErrConflict{})`, the `\"conflict\"` code, and the remote error rebuilt on the client side.\n\n**A small leftover, not a blocker.** Enable's \"app %q is not disabled\" (`servers/app/disable.go`) is still a plain `fmt.Errorf`. So `POST /api/v1/apps/{app}/enable` on an app that's already enabled returns a 500 over REST. A `cond.Conflict` is safe there because Enable has no retry loop. It's worth the one-line fix if Cloud will call this endpoint.\n\n**Still standing:**\n- The PR description and your top-level comment to @jcasimir still say \"Rollback and env changes still work\". The code and docs now refuse both, so please correct them before merge. Please also check with him that the maintenance-mode workflow in `scaling.md` fits his fallback flow.\n- Letting deployer roles disable and enable apps (`pkg/workloadroles/roles.go`) needs sign-off from whoever owns the roles model.\n- Someone needs to decide whether merging waits for Cloud to understand the `disabled` health value.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss60UfJ5",
"body": "Addressed in `b1cbc5b`. `disabledError` matches `ErrAppDisabled` and returns `\"conflict\"` from `ErrorCode()`, so `writeRESTError` answers 409. It uses `errors.As`, so wrapped errors get the same status. It deliberately doesn't match `cond.ErrConflict{}`, so neither the `SetConfiguration`/`createNewVersion` retry loops nor `FailIfUnsettled` read a disable as a lost race. The new tracker test asserts both, plus the round-trip through `cond.RemoteError`. Restart and pool scale-up use the same type now as well. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"head_sha": "b1cbc5b36571bd81be29312f0b3ca6bbccc31e93"
}This is still ready for human review, but I'm not recommending approval. One new defect should be fixed before merge, the PR description has gone stale, and the two decisions I flagged last time still need a person.
2c77e4b (Freeze a disabled app) does what it says. The refusal sits in Tracker.Begin, which every operation goes through, so it covers the config-change and env paths in servers/app/app.go and servers/deployment/server.go. refuseDisabled also covers callers that pass an AppID. DeployVersion checks early for previews, which never reach Begin. The guard in commitActivation runs after the "already active" early return, so retrying an activation that already committed still succeeds. It also checks the same app revision the pointer patch is guarded by, so a disable that lands mid-attempt is either seen by the check or makes the patch conflict. The tests cover every operation, the AppID path, a disable mid-attempt, redeploy/rollback/preview (including no leftover lock or preview version), and env set/delete.
New: on the REST API, the disabled refusal comes back as HTTP 500. ErrAppDisabled (pkg/deploylifecycle/tracker.go:99) is a plain errors.New. writeRESTError in pkg/rpc/rest.go sets a status only from an ErrorCode (not-found/conflict/validation-failure), and anything else becomes http.StatusInternalServerError. PUT /api/v1/apps/{app}/env, PUT /api/v1/apps/{app}/env/{key} and DELETE /api/v1/apps/{app}/env/{key} all reach Begin. So an env change on a disabled app over REST, which is presumably how Cloud calls these, gets a 500 with no code for an expected refusal the user can act on. The restart refusal (app.go, fmt.Errorf("app %q is disabled…")) and Enable's "is not disabled" have the same problem. Details on the fix are inline. Note that turning it into a plain cond.Conflict would introduce new bugs.
The PR description is out of date. It says "Rollback and env changes still work", and so does your top-level comment to @jcasimir. The code and docs now refuse both, so please update the description before merge.
That also makes the open question to @jcasimir more pointed. A disabled fallback copy now can't run env set without being enabled first. The maintenance-mode workflow in scaling.md is the answer, and it's worth confirming that it fits his flow.
Still standing, unchanged: deployer roles gaining disable/enable in pkg/workloadroles/roles.go needs sign-off from whoever owns the roles model, and someone needs to decide whether merging waits for Cloud to learn the disabled health value.
errors.New, writeRESTError (pkg/rpc/rest.go) finds no ErrorCode and answers 500. PUT/DELETE /api/v1/apps/{app}/env… reach Begin, so a REST caller gets an internal-server-error for an expected refusal.Don't fix it by wrapping in cond.Conflict, though. SetConfiguration (servers/app/app.go) treats errors.Is(err, cond.ErrConflict{}) from ActivateAtRevision as "lost the race": it deletes the minted version and continues, up to 100 times, without re-calling Begin. FailIfUnsettled also swallows conflicts as "already settled", so a refused attempt would never be recorded as failed. A dedicated error type would avoid both problems. Give it Error(), an Is(ErrAppDisabled) match, and an ErrorCode() that returns "conflict", or a new code that restStatusFor maps to 409. That fixes the REST status without matching cond.ErrConflict{}. The restart refusal in app.go could use the same type.
Verdict: ready
This is still ready for human review. I'm still not recommending approval, for the same two reasons as last time.
The new commit (c048e8a) closes the last code-side point. app-disable.md now says task runs already in progress finish and addons keep running. The generated page matches its source, appDisableDescription (cli/commands/commands.go:1515), so regenerating the docs won't undo the change. Letting in-flight runs finish is now a stated choice rather than a gap, so I'm resolving that thread.
I also traced the disable paths again: Disable → launcher hold-at-zero, the pool manager clearing crash state, the SetDesired guard, the schedules_resumed_at watermark in sweepTask, and the SCHEDULE check in start(). I didn't find a new defect. One minor note: running miren app disable a second time without --reason clears the earlier reason, because the CLI always sends one. The comment at servers/app/disable.go:37 says a repeat call "only updates the reason", so this looks intended.
Two decisions still need a human, unchanged since last time:
pkg/workloadroles/roles.go). The linked ticket is exactly the question of whether they should. This PR answers yes, and the roles table in in-cluster-api.md matches. cluster-deployer can now take every app on a cluster offline, so whoever owns the roles model should sign off.disabled health value, or it will show these apps as Unknown. Someone needs to decide whether merging waits for that.There's also an open question on the PR for @jcasimir: does the fallback flow need to deploy to a disabled copy? If it does, that pushes back on "disable blocks deploys", so it's worth answering before merge.
Posted to the PR's comment threads when you submit.
cli/commands/commands.go and the generated app-disable.md match. With that stated as intended behavior, this check only needs to cover scheduled runs that haven't started yet, and it does. That includes runs held back by admission, since they re-enter start() on each reconcile. Resolving.Verdict: ready
This is ready for human review. The last two commits (c8a77e1, 696f174) fix all three code issues I raised before. What's left is two decisions that need a person and one small gap worth documenting. I read the code but didn't run the tests.
Deploys to a disabled app: refused. Blocking the build is the right choice. Skipping deploy tasks would activate code without its migrations, and a deploy that boots nothing can't be health-checked. refuseDisabled (servers/build/authorization.go:43) runs on every build path I could find:
BuildFromTar and BuildFromPrepared (after authorization, before any upload or build work)PrepareUploadnextVersion and ensureApp, so the saga is covered tooA second check in the runDeployTasks action (build_saga.go:723) catches a disable that lands mid-build, and it runs before any deploy run is created. If the app has no deploy tasks, a mid-build disable lets the version activate. The launcher then creates its pools at zero and skips the readiness wait (launcher.go:488, :649), so nothing boots. The app-disable.md page and the schema doc now say deploys are refused. TestPrepareUploadRefusesDisabledApp covers the entry check. Nothing tests the mid-build check, but it's a short, plain branch.
Enable restore failure: fixed. Once the flag is cleared, a failure in restoreFixedPools logs a warning and Enable returns success. TestEnableSucceedsWhenRestoringPoolsFails covers this.
Seeding a new pool for a disabled app: fixed. launcher.go:591 now has the !disabled guard, so a new pool for a disabled app is never seeded above zero.
Runs already in progress keep going. Disable only scales pools. Task runs get their own sandbox from controllers/run/controller.go:421, which isn't part of any pool. The SCHEDULE check at controller.go:120 only fires when a run starts. So a scheduled (or deploy) run that is already RUNNING when you disable the app runs to completion. For the fallback-cluster case the schedule pause was added for, a long nightly job that started just before the disable keeps writing to shared state. Letting in-flight work finish is a reasonable choice, but app-disable.md says "running sandboxes shut down", which reads as if it covers these too. Either say in the docs that in-flight task runs finish, or cancel them in Disable.
Still needs a human decision:
cluster-deployer and app-deployer can now disable and enable apps (pkg/workloadroles/roles.go:85-87). A cluster-deployer token can take every app on the cluster offline and pause all their schedules. MIR-2061 raised exactly this concern. Whoever owns the roles model should sign off, especially for the cluster-scoped role.disabled is a new health value, and Cloud will show disabled apps as Unknown until it knows it. Someone should decide whether this waits for Cloud.I'm not recommending approval. The two open items are judgment calls about policy and coordinating with Cloud. The change also spans authorization, the scheduler, the launcher, ingress and the build path, so a human should look at it.
RUNNING when the app is disabled. That run's sandbox comes from ensureSandbox (line 421), not from a pool, and Disable only scales pools, so the run finishes normally. If that's what you want, say so in app-disable.md. Right now "running sandboxes shut down" reads as covering task runs too. If it isn't, Disable could request cancellation (CancelRequestedAt) on the app's non-terminal SCHEDULE runs.Verdict: ready
This is ready for a human to look at, but it shouldn't merge without review. It changes authorization policy, touches the launcher, scheduler, activator, ingress and pool manager at once, and depends on Cloud changing in step. I also found one place where the code doesn't do what the docs say. I read the code but didn't run the tests.
Deploy tasks still boot on a disabled app. The disabled schema doc and docs/docs/command/app-disable.md both say "nothing boots on a deploy". The launcher does keep service pools at zero. But servers/build/deploy_tasks.go still creates trigger = "deploy" runs, and the new guard in controllers/run/controller.go:120 only skips SCHEDULE runs. So deploying to a disabled app still starts a sandbox and runs its migration or backfill tasks. That's the same shared-state risk the schedule pause was added to prevent for a fallback copy on an old cluster. There's no simple fix: skipping those runs makes awaitDeployRuns fail the deploy with "skipped without running". So someone has to decide whether deploy tasks should run on a disabled app, and then make either the code or the docs match.
Smaller issues
Enable clears the flag before it restores pools (servers/app/disable.go:96-111). If reading the version or ResolveRuntimeConfig fails, the caller gets an error even though the app is already enabled. Retrying then fails with "is not disabled". The launcher resync puts the pools back, so the risk here is a misleading result, not a broken app.controllers/deployment/launcher.go:591). Details are inline.Needs a human decision
pkg/workloadroles/roles.go:85-87). This settles MIR-2061 in favor of deployers. The argument is reasonable: a deployer can already ship code that refuses every connection. But it does mean a cluster-deployer token can now take every app on the cluster offline and pause all their schedules, which is the concern the ticket raised. Whoever owns the roles model should sign off, especially for the cluster-scoped role.disabled is a new health value. Disabled apps will show as Unknown in Cloud until it learns that value. Someone should decide whether this waits for Cloud.What holds up: Disable and Enable authorize through appclient.Authorize, which still calls rpc.AllowApp for bound callers. So the app-scoped appDeploy grant keeps the roles.go tripwire invariant, and authorization_test.go checks that a denied call changes nothing. Every path that could bring an app back checks the flag: launcher reuse, the activator's requestPoolCapacityTo (which also covers scaleUpForDemand), clearing crash cooldown, and SetDesired. Ingress serves the holding page before the lease cache, so a warm app stops serving immediately.
SCHEDULE. Deploy-triggered runs from servers/build/deploy_tasks.go still go through admit/ensureSandbox on a disabled app. So a deploy to a disabled app starts a sandbox and runs its trigger = "deploy" tasks, which contradicts "nothing boots on a deploy" in the schema doc and app-disable.md. Marking them SKIPPED here wouldn't work on its own, because awaitDeployRuns treats SKIPPED as a failed deploy. Pick a behavior: skip deploy tasks in the build path when the app is disabled, refuse the deploy, or keep running them. Then either change the code or change the docs to say deploy tasks still run.Enable returns an error for an app that is in fact enabled. Running miren app enable again then fails with "is not disabled". The launcher resync restores the fixed pools anyway, so this is about the result you report, not about the app staying down. You could log and carry on once the flag is cleared, or say in the error that the app was enabled and only the immediate restore failed.disable/enable in appDeploy gives them to cluster-deployer too, so a token with that role can take every app on the cluster offline and pause their scheduled tasks. The "a deployer can already ship code that refuses connections" argument is fair. Still, the ticket specifically raised the concern about a deployer token reaching every app. Can whoever owns the roles model confirm they're OK with that for cluster-deployer as well as app-deployer?desiredInstances is 0, so this falls into the else if branch. That branch seeds the pool from currentPoolSize, which can raise it above zero if any sibling pool for the service still has desired > 0. That can happen in the window between Disable patching the flag and finishing its scale-down loop, or if Disable errored partway through. The next resync sets it back to zero, so the effect is brief. Still, else if !disabled && ver.EphemeralLabel == "" would make "nothing boots for a disabled app" true by construction, rather than true once the next resync runs.Verdict: ready