This is still ready to merge. The only new commit is f204eeb. It replaces the app pool list call in fetchServiceHealth with a ListPage loop over entity.Ref(compute_v1alpha.SandboxPoolAppId, appEntity.ID), 200 entries per page, and keeps going until the cursor comes back empty. That's the fix I asked for in my inline note, and it follows the guidance in rpc.yml. Errors and decoding are handled the same way as in the sandbox walk just below it, and nothing else in the file changed in a way that affects the grouping or exit-code logic.
I'm also closing my four threads from the first round. I'd already confirmed those fixes in the code but left the threads open, which I shouldn't have.
Posted to the PR's comment threads when you submit.
ListPage until the cursor comes back empty, the same way the sandbox walk does. Thanks for taking this.CrashStreak > 0, it now reads last within 10m, and the healthy web assertion covers it. Resolving.miren debug reindex --dry-run, a fix that actually works. You've said the full dry-run scan on every doctor run is a limitation you're accepting, and that's reasonable. Resolving.miren sandbox-pool list, and a test checks the exact string. Resolving.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1270,
"verdict": "ready",
"event": "approve",
"summary": "This is still ready to merge. The only new commit is f204eeb. It replaces the app pool `list` call in `fetchServiceHealth` with a `ListPage` loop over `entity.Ref(compute_v1alpha.SandboxPoolAppId, appEntity.ID)`, 200 entries per page, and keeps going until the cursor comes back empty. That's the fix I asked for in my inline note, and it follows the guidance in `rpc.yml`. Errors and decoding are handled the same way as in the sandbox walk just below it, and nothing else in the file changed in a way that affects the grouping or exit-code logic.\n\nI'm also closing my four threads from the first round. I'd already confirmed those fixes in the code but left the threads open, which I shouldn't have.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6ltu2Z",
"body": "Confirmed in f204eeb: the pool lookup now pages through `ListPage` until the cursor comes back empty, the same way the sandbox walk does. Thanks for taking this.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6ltP_x",
"body": "Checked this in the current code: the streak text only shows when `CrashStreak \u003e 0`, it now reads `last within 10m`, and the healthy `web` assertion covers it. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6ltP_5",
"body": "The resource scan now runs under its own 30s deadline, and each kind is paged to the end of its cursor. If the scan fails or comes back incomplete, it skips instead of reporting healthy, and a test covers that path. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6ltP_8",
"body": "The index check now has its own 30s budget, and when it times out it points to `miren debug reindex --dry-run`, a fix that actually works. You've said the full dry-run scan on every doctor run is a limitation you're accepting, and that's reasonable. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6ltQAF",
"body": "The command is now spelled `miren sandbox-pool list`, and a test checks the exact string. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"head_sha": "f204eeb870629d09ebeb9bc854715998ba76a6b5"
}I think this is ready to merge. All four problems from my last review are fixed in the code, and three of the fixes have tests.
app status wording. The streak text only appears when CrashStreak > 0, and it now reads last within 10m. The new assertion checks that nothing after web: mentions a crash, so a healthy service can't quietly pick up that text again.gatherDoctorResources now runs under its own 30s deadline and pages each kind through ListPage, 200 entries at a time. If the deadline passes partway through, it returns an error, and resourceUnavailable turns that into a skip, so a partial scan never shows up as healthy. The new test that sets resourcesErr and expects all three checks to skip covers this.miren debug reindex --dry-run action on the "unavailable" warning. That action also works against older servers that don't have check_index_health, so the warning always comes with a fix that works. Two costs come with this choice, and they look deliberate: every miren doctor run starts a full dry-run scan on the server, and a slow entity server can now keep doctor waiting for up to 30s.miren sandbox-pool list, and a test checks the exact string.I followed the sandbox pool label back to where the launcher and sandboxpool.Manager set it. It carries the pool ID, which is what both new consumers match against, so the per-pool grouping holds up. I've left one small inline note: the new list call in fetchServiceHealth goes against a rule written in rpc.yml. It doesn't block the merge.
list_page doc in api/entityserver/rpc.yml asks explicitly: "do not add another caller of list." This adds one. An app's pool set is small, so it doesn't cause a problem today. But the sandbox walk a few lines below already uses ListPage with a cursor loop, and the same loop over entity.Ref(compute_v1alpha.SandboxPoolAppId, appEntity.ID) would follow the project's direction at almost no cost. Not a merge blocker.Verdict: ready
I don't think this is ready yet. The new app status output misreports every healthy service, and the new doctor rows have three problems: one probe is unbounded, one is almost certainly bounded too tightly to finish on a real cluster, and one row tells the user to run a command that doesn't exist. Each is a small fix.
app status wording. renderServiceHealth always adds "; %d crashes in current streak (last crash in 10 minutes)". So a healthy service prints web: 1 running, 0 dead; 0 crashes in current streak (last crash in 10 minutes), which reads as though it just crashed. Your test only checks the web: 1 running, 0 dead prefix, so it doesn't catch this.
Doctor timeouts. The doctor code says every probe is bounded so that one slow dependency can't hold up the rest (see the gatherDoctorEnv and gatherCLI comments). gatherDoctorResources breaks that rule: it runs four cluster-wide List calls on the bare ctx, and gatherCluster waits for all of them.
gatherDoctorIndex goes the other way. It gives CheckIndexHealth the 3s probeTimeout, but on the server that call runs a full CleanupStaleCollectionEntries pass: it scans every collection key and batch-reads every entity those keys point to, with full payloads. miren debug reindex does the same scan and itself warns that it "may take several minutes depending on the number of entities". On any cluster of real size I'd expect this row to show "index scan unavailable" on every run, and its suggested fix ("retry the bounded scan") won't help. That's exactly the kind of warning the checkResult doc says doctor must not emit. It also means every miren doctor run starts a whole-store scan on the server.
Wrong command in the Pools row. The action there is miren sandbox pool list, but the registered command is miren sandbox-pool list (commands.go, line 484). The one fix this row offers doesn't work as written.
The rest checks out: the scale-down vs. crash logic matches how sandboxpool.Manager sets ConsecutiveCrashCount, CooldownUntil and DesiredInstances. The app prefix added to service names in checkSandboxes stops two apps' db services from adding up to a false alert, and it has a test. TestCheckIndexHealthIsReadOnly checks that the scan changes nothing and that a cancelled scan never reports a clean result.
CrashStreak is 0, so every healthy service shows 0 crashes in current streak (last crash in 10 minutes), which reads like a recent crash. Only add the streak text when svc.CrashStreak > 0, and word it as a window (e.g. %d crashes in current streak, last within 10m). Adding an assertion that the healthy web line contains no crash text would lock this in.List calls on the bare ctx with no deadline, and gatherCluster blocks until they finish. That breaks doctor's rule that every network probe is bounded: a slow entity server stalls the whole sweep. Wrap it in a timeout like the other probes. It already falls back to a skip through resourceUnavailable.CheckIndexHealth is a full CleanupStaleCollectionEntries dry run: it scans every collection key and batch-reads every entity they point to. miren debug reindex does the same scan and warns it can take several minutes, so 3s (probeTimeout) won't cover it on a real cluster. The row will then warn on every run with advice that can't fix it. Please give it a separate, realistic budget, make the server side sampled or cached, or drop it from doctor and point users to miren debug reindex --dry-run.miren sandbox pool list isn't a command. The registered one is miren sandbox-pool list (commands.go:484). Since the only fix this row offers has to actually work, please correct the spelling.Verdict: not_ready