The unbounded growth of CountedFailures is fixed. That was the last thing I had open, so this is ready to merge.
What changed: counted_failures is now a single JSON-string attribute. countStartupFailures (manager.go:826–874) first keeps only the stored IDs whose sandbox still appears in the pool's listSandboxes result, then appends new failures. Stored IDs are now bounded by sandboxes that still exist, and garbage-collected DEAD sandboxes drop out on their own.
The empty-list clearing problem is handled too. retained is built with make(..., 0, n), so an empty list marshals to "[]", not null or "". That's a non-empty string, so Encode() still emits it and meta.Update replaces the stored value. TestCountedFailuresPrunedAndClearedInStore runs through reconcilePool, which is the real controller diff/patch path. It checks both that a vanished ID gets pruned and that the stored attribute ends up as "[]", which was the regression I was worried about.
Outcome labeling: I also checked the rest of the outcome labeling against the code, and it holds up:
recordExit, StopSandbox (which retries on conflict) and nodehealth only set an outcome when none is set yet. So a STOPPED + STARTUP_FAILED sandbox from a boot-time exit still ends up DEAD + STARTUP_FAILED, which the pool counts.markDeadNoRestart are both limited to RUNNING, so neither can stamp STARTUP_RUNNING on a sandbox that never got there.WaitForPort gets its exit signal from monitorTaskExit. The portMap reset in BootContainers happens before MonitorContainer is called, so it can't wipe out a bound-port report.I'm resolving both of my open threads on the CountedFailures line.
Posted to the PR's comment threads when you submit.
countStartupFailures now filters the stored IDs against the pool's current sandboxes before counting, so IDs of garbage-collected DEAD sandboxes get dropped and the list stays bounded by what still exists. Resolving."[]". Encode() still emits that, so meta.Update replaces the old value. TestCountedFailuresPrunedAndClearedInStore runs this through reconcilePool and checks both that a vanished ID gets pruned and that the stored value becomes "[]". That's the regression test I asked for. Resolving.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1269,
"verdict": "ready",
"event": "approve",
"summary": "The unbounded growth of `CountedFailures` is fixed. That was the last thing I had open, so this is ready to merge.\n\n**What changed:** `counted_failures` is now a single JSON-string attribute. `countStartupFailures` (manager.go:826–874) first keeps only the stored IDs whose sandbox still appears in the pool's `listSandboxes` result, then appends new failures. Stored IDs are now bounded by sandboxes that still exist, and garbage-collected DEAD sandboxes drop out on their own.\n\nThe empty-list clearing problem is handled too. `retained` is built with `make(..., 0, n)`, so an empty list marshals to `\"[]\"`, not `null` or `\"\"`. That's a non-empty string, so `Encode()` still emits it and `meta.Update` replaces the stored value. `TestCountedFailuresPrunedAndClearedInStore` runs through `reconcilePool`, which is the real controller diff/patch path. It checks both that a vanished ID gets pruned and that the stored attribute ends up as `\"[]\"`, which was the regression I was worried about.\n\n**Outcome labeling:** I also checked the rest of the outcome labeling against the code, and it holds up:\n- `recordExit`, `StopSandbox` (which retries on conflict) and nodehealth only set an outcome when none is set yet. So a STOPPED + `STARTUP_FAILED` sandbox from a boot-time exit still ends up DEAD + `STARTUP_FAILED`, which the pool counts.\n- The unhealthy-sandbox DEAD path at sandbox.go:1386 and `markDeadNoRestart` are both limited to RUNNING, so neither can stamp `STARTUP_RUNNING` on a sandbox that never got there.\n- `WaitForPort` gets its exit signal from `monitorTaskExit`. The `portMap` reset in `BootContainers` happens before `MonitorContainer` is called, so it can't wipe out a bound-port report.\n\nI'm resolving both of my open threads on the `CountedFailures` line.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6ltQws",
"body": "Fixed. `countStartupFailures` now filters the stored IDs against the pool's current `sandboxes` before counting, so IDs of garbage-collected DEAD sandboxes get dropped and the list stays bounded by what still exists. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6lt12G",
"body": "This is handled, including the empty case. With the attribute stored as a JSON string, an empty result encodes as `\"[]\"`. `Encode()` still emits that, so `meta.Update` replaces the old value. `TestCountedFailuresPrunedAndClearedInStore` runs this through `reconcilePool` and checks both that a vanished ID gets pruned and that the stored value becomes `\"[]\"`. That's the regression test I asked for. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"head_sha": "258ab97d7789bdbbda78731d01416972b53001b1"
}The PENDING-sibling issue is fixed. One caveat still stands, the same one I raised in earlier rounds: CountedFailures only ever grows.
Cooldown cap: manager.go:117–118 now caps desired at max(1, min(pool.DesiredInstances, actual+1)). The else if actual > 0 guard also means a pool whose siblings are all PENDING no longer drops to 1. Crash-loop damping still holds: dead sandboxes aren't in actual, nothing is created during cooldown, and the min keeps desired from rising. TestCooldownPreservesPendingSiblingsAfterTheyStart covers the case I described. In that test the pending siblings finish booting before the cooldown ends, and it asserts that a replacement can start without retiring a healthy sibling. The comment at lines 111–113 now matches what the code does.
Still open: manager.go:845 still only appends to CountedFailures, so the list keeps growing as long as a pool keeps crashing. The fix is in my existing threads on that line, including the explicit clear when the filtered list comes out empty. If you merge without it, you're accepting that growth.
Posted to the PR's comment threads when you submit.
actual (RUNNING + PENDING), and the actual > 0 guard keeps a pool whose siblings are all PENDING from dropping to 1. TestCooldownPreservesPendingSiblingsAfterTheyStart pins the scenario: the pending siblings reach RUNNING before the cooldown ends, and the replacement starts without a healthy sandbox being retired. Resolving.Verdict: caveats
This can merge with two caveats. One is new: the cooldown cap still shrinks pools whose healthy siblings are mid-boot. The other I raised earlier: CountedFailures grows without bound.
The crash-loop concern is fixed. manager.go:118 now caps desired at max(1, min(DesiredInstances, ready+1)). A crash-looping pool can queue at most one replacement beyond its live siblings, and activator bumps get clamped again on every cooldown reconcile. TestCrashLoopCooldownCapsDesiredWithRunningSiblings pins this: 2 running give desired 3. Node loss with RUNNING siblings still keeps its size.
Siblings that are still booting aren't protected. The cap counts only RUNNING sandboxes, and the min means desired can only go down during a cooldown. Take a scale-up from 2 to 5:
min.scaleDown (manager.go:226–235, which only picks RUNNING candidates at 418–422) retires a healthy sandbox.The same thing happens after node loss during a rollout, and when every sibling is PENDING, desired resets to 1. main reset to 1 unconditionally, so this is milder than before. But this PR counts more events as failures than main did: any pre-RUNNING death regardless of age, and node-lost PENDING/NOT_READY sandboxes that main never counted. So this path runs more often, and the comment at lines 111–113 promises more than the code delivers. Details in the inline comment.
Still open: CountedFailures is still append-only at manager.go:845. The fix is in my existing threads on that line, including clearing the attribute explicitly when the filtered list is empty. If you merge without it, you're accepting that the list grows for the life of any pool that crashes repeatedly.
ready only counts RUNNING sandboxes, so siblings that are still PENDING when another instance fails get cut out of desired. And because of the min(pool.DesiredInstances, …), desired stays low after they reach RUNNING. Example: scale-up 2→5, one PENDING instance dies, so desired becomes 3. After the cooldown, actual is 4 and scaleDown stops a healthy RUNNING sandbox. The comment above promises to preserve healthy siblings after a partial failure, and that isn't true for siblings that are mid-boot.Using actual (RUNNING + PENDING) instead of ready here, i.e. max(1, min(pool.DesiredInstances, actual+1)), should keep the crash-loop damping. Dead instances aren't in actual, and nothing gets created during cooldown, so desired still can't grow past what's alive + 1. A test like the one above, but with PENDING siblings that turn RUNNING before the cooldown ends, would pin it.
Posted to the PR's comment threads when you submit.
ready+1, clamped between 1 and the current desired. A crash-looping pool with live siblings can queue at most one extra replacement, and activator requests can't pile up during cooldown. TestCrashLoopCooldownCapsDesiredWithRunningSiblings pins the 2-running → desired-3 case, which is the test I asked for. Resolving this one. I've left a separate note on line 118 about siblings that are still PENDING when the cap is computed.Verdict: caveats
The node-loss concern is handled. CountedFailures still only grows, and the new cooldown rule changes crash-loop damping for pools with more than one instance. Neither blocks a merge, but you should accept both knowingly, so this stays at CAVEATS.
Node loss no longer shrinks healthy pools. You kept STARTUP_FAILED for PENDING/NOT_READY sandboxes on a lost node, and the comment in manager.go says so explicitly. The cooldown branch (manager.go:114–119) now keeps DesiredInstances as it is. It only drops it to 1 when nothing is RUNNING, or to 0 when the pool is unreferenced. So a 5-instance pool that loses one booting sandbox keeps its other four, and the only cost is a short delay before the replacement starts. TestPartialNodeLossKeepsPoolDesiredDuringCooldown covers exactly that: desired stays at 5, a 10s cooldown starts, and nothing is created during it. Putting the fix in the cooldown policy was the right call, because every partial failure gets the same protection, not just node loss.
New trade-off: the ready == 0 guard also exempts crash-looping pools. ready is a count of RUNNING sandboxes at the moment of the reconcile. Take the post-RUNNING quick crash that the lifetime rule counts again: in a pool with desired > 1, the first sandbox to die usually leaves siblings still RUNNING. So ready > 0, and desired is no longer reset during cooldown. The activator can also keep raising it, up to its maxInstances limit. Each time a cooldown ends, the pool relaunches every dead instance, not just one. Backoff still escalates, so this is bounded, but it removes the damping the old reset gave crash loops. The only test covers node loss. If this is intended, a test that pins it for a crash-looping multi-instance pool would make it explicit. If it isn't, you could key the guard on sandboxes that have proven healthy (e.g. the uptime check hasHealthySandbox already does) rather than on any RUNNING sandbox.
Still open: CountedFailures only grows (manager.go:845). Nothing removes IDs, and when the filtered list is empty it needs explicit clearing, as covered in the open threads on that line. I'd still fix it before merging. If you ship without it, do so knowing the list grows for the life of any pool that crashes repeatedly.
ready counts RUNNING sandboxes at this moment, so a multi-instance pool in a post-RUNNING crash loop often has live siblings when the first one dies. That skips the reset, the activator can keep raising desired during cooldown, and each time a cooldown ends every dead instance gets relaunched, not one. If that's intended, please add a test that pins it for a crash-looping pool. Otherwise, consider checking for a sandbox that has proven itself healthy (the same uptime check hasHealthySandbox uses) instead of ready == 0, so node loss keeps its protection and crash loops still get damped.Posted to the PR's comment threads when you submit.
STARTUP_FAILED for node loss, and the cooldown branch in manager.go now only resets desired to 1 when ready == 0, so a pool that still has RUNNING instances keeps its size. TestPartialNodeLossKeepsPoolDesiredDuringCooldown covers exactly the 5-instance case I described. Node loss still triggers a short cooldown before the replacement, but the healthy sandboxes are no longer at risk. That was my real concern, so I'm resolving this. I've left a separate note at line 117 about how the same guard affects crash-looping pools.Verdict: caveats
The regression that held this back last time is fixed, and so is the saga-failure item. CountedFailures pruning is still open, and I found one new behaviour change I think you should decide on deliberately. Neither blocks a merge, which is why this is CAVEATS and not NOT_READY.
Fixed: crash loops after RUNNING back off again. countStartupFailures (manager.go:831) now skips a DEAD sandbox only if it isn't STARTUP_FAILED and it lived at least 60s. An app that binds its port and then dies, or a port-less worker that fails right after boot, gets counted again. Failures before RUNNING are counted however long they took. The updated fast-healthy case and the unchanged long-lived healthy case pin down that boundary.
Fixed: the saga-failure path when GetSandbox fails. It now logs, still patches DEAD, and returns the saga's own error. TestSagaFailureMarksDeadWhenFinalFetchFails covers that. You leave the outcome unset there instead of stamping STARTUP_FAILED. I can live with that: this path needs a failed saga and a failed read, and it just falls back to the lifetime rule.
Still open: CountedFailures only grows (manager.go:842). Nothing ever removes IDs, and every reconcile re-encodes the whole list. It fills fastest for a pool that keeps failing and then briefly recovering, because the backoff starts over at 10s after each recovery. There's one catch in the fix, covered inline: when the pruned list is empty, Encode() emits nothing for it, so the stale values would stay.
New: losing a node now puts the app's pool into backoff. markNodeSandboxesDead (nodehealth/controller.go:248) stamps STARTUP_FAILED on any PENDING or NOT_READY sandbox on a lost node. countStartupFailures counts those regardless of lifetime. On main, those sandboxes were more than 5 minutes old by the time they were marked, so they never counted as quick crashes. Now one sandbox caught mid-boot on a dying node puts its pool into cooldown. The cooldown branch at manager.go:113–126 also resets DesiredInstances to 1 for a referenced pool. So a 5-instance pool that loses a single PENDING sandbox that way comes out of cooldown and scales down its healthy RUNNING sandboxes, unless something raises desired again. The test pins STARTUP_FAILED for this case, so it may be intended. If so, I'd like it to be a conscious choice. If not, leaving the outcome unset for node loss keeps infrastructure failures out of the app's crash streak.
CountedFailures is only ever appended to. sandboxes is the full list for this pool and IDs are never reused, so at the top of the function you can rebuild counted from only the IDs still in sandboxes and write that filtered slice back to pool.CountedFailures. One catch: when the filtered list is empty, pool.Encode() emits no counted_failures attrs. As far as I can tell, meta.Update(pool.Encode()) in updatePoolStatus then leaves the old values in place, so the list never shrinks to zero. That's the same problem updatePoolStatus already works around for CurrentInstances/ReadyInstances, and this attribute needs explicit clearing for the same reason. A line in TestCountStartupFailures asserting that a vanished ID gets dropped would lock it in.STARTUP_FAILED means countStartupFailures counts them however old they are. On main these were more than 5 minutes old when marked, so they never counted as quick crashes. Now a node dying while one sandbox boots puts the pool into cooldown, and the cooldown branch resets DesiredInstances to 1 (manager.go:113–126). A multi-instance pool can then scale down its healthy RUNNING sandboxes once the cooldown ends. If infrastructure loss shouldn't feed the app's crash streak, leave the outcome unset here, the way you already do for STOPPED. The lifetime rule will then ignore these.Posted to the PR's comment threads when you submit.
StartupOutcome != STARTUP_FAILED and a lifetime of at least 60s, so a STARTUP_RUNNING sandbox that dies quickly gets counted again. The fast-healthy case now expects to be counted. Thanks for keeping failures before RUNNING free of the lifetime limit.countStartupFailures only ever appends to pool.CountedFailures, and nothing removes entries. Filtering against sandboxes at the top of the function fixes it. Note that an empty result has to be cleared explicitly, because Encode() emits nothing for an empty slice and meta.Update would keep the old values. I've put the details in a new inline comment on the same line.GetSandbox now logs and still patches DEAD, and the saga's error is what gets returned. TestSagaFailureMarksDeadWhenFinalFetchFails covers both. Leaving the outcome unset rather than stamping STARTUP_FAILED is a defensible choice. Those rare cases just fall back to the lifetime rule.Verdict: caveats
I'm holding this back because of one behaviour regression in the pool manager. The rest of the plumbing looks sound to me.
Regression: crash loops after RUNNING no longer back off. On main, countQuickCrashes counted any DEAD sandbox that lived less than 60s, whether or not it reached RUNNING first. countStartupFailures now skips anything with StartupOutcome == STARTUP_RUNNING, however short its life, and the fast-healthy case in TestCountStartupFailures locks that in. Two common loops now get no cooldown at all:
listen().waitPorts has nothing to wait for, so setRunning stamps STARTUP_RUNNING right after boot. A worker that takes even a moment to fail (missing env var, bad config) looks healthy to the pool.In both cases the sequence is: recordExit moves the sandbox from RUNNING to STOPPED with STARTUP_RUNNING, StopSandbox marks it DEAD, and the pool replaces it on the next reconcile with no delay. Catching failures that happen before RUNNING is the right goal, but it should add to the old rule rather than replace it. My suggestion is to keep the lifetime check for STARTUP_RUNNING sandboxes. If dropping it was deliberate, please say where post-RUNNING crash loops are handled now, because I couldn't find anywhere.
Smaller items (details inline):
CountedFailures only ever grows. Entity.Update replaces many-valued attributes as a whole, so pruning IDs that no longer exist would be cheap.GetSandbox fails, the sandbox never gets marked DEAD.Things I liked:
recordExit no longer turns an already-DEAD sandbox back into STOPPED. Before, the node-loss and saga-failure handlers could race a late exit report and undo each other's work.errProcessExited fast-fail in waitPorts means a crashed process no longer sits out the whole port timeout. It also skips the misleading "listening elsewhere" diagnosis.BootContainers, the portMap entry is reset after task.Start but before MonitorContainer starts. That clears a stale exited flag without dropping an early port bind, and the ordering is right.main has. A sandbox that reaches RUNNING and dies seconds later is never counted, so the pool recreates it with no cooldown. That covers an app that binds its port and then crashes, and any port-less worker: waitPorts has nothing to wait for, so those get STARTUP_RUNNING almost immediately. countQuickCrashes caught these with its under-60s lifetime check. I'd apply that check to STARTUP_RUNNING too, i.e. skip only when updatedAt.Sub(createdAt) >= 60s, and change the fast-healthy test case so it expects the crash to be counted.CountedFailures only grows. The schema doc says it's kept across streak resets, and nothing ever removes entries. DEAD sandboxes are deleted after about an hour, so a pool stuck in a long crash loop keeps collecting IDs that can never match again (about 96 a day at the 15-minute cap). Every reconcile then re-encodes the whole list through meta.Update(pool.Encode()). Since Entity.Update replaces all values of an attribute, the fix is easy: at the top of countStartupFailures, drop any ID that isn't in sandboxes. An ID that no longer exists can't be double-counted.main a failed saga was always patched DEAD. Now, if GetSandbox fails transiently, this returns early and the sandbox stays PENDING until the stale-pending sweep finds it about 5 minutes later. The returned error also replaces the original saga failure, so that gets lost. I'd fall back instead: if the get fails, still patch DEAD with STARTUP_FAILED, since this saga failed and so never reached RUNNING through it. Then wrap or join the get error with the saga error rather than returning it on its own.Verdict: not_ready