Fix auto-mounted local storage orphaning duplicate pools, and stop the activator stranding ingress on them#883
Merged
phinze merged 2 commits intoJul 6, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds stale binding eviction in the activator for pools that stop referencing a version, and moves launcher local-disk auto-mount injection from sandbox-spec construction to config resolution. It also adds tests for direct eviction, watch-driven eviction, duplicate disk-name handling, and draining a superseded pool after local data appears. ChangesRelated Issues: MIR-1293 Sequence Diagram(s)Included in the hidden review stack artifact above. Poem: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/deployment/launcher_test.go (1)
2822-2826: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the container mount too.
This verifies the user-visible part of the auto-mount, not just the volume declaration; otherwise a regression that omits
SandboxSpec.Container[].Mountwould still pass.Proposed test addition
require.Len(t, referencing[0].SandboxSpec.Volume, 1) assert.Equal(t, "local-data", referencing[0].SandboxSpec.Volume[0].Name) assert.Equal(t, "local", referencing[0].SandboxSpec.Volume[0].Provider) assert.Equal(t, "/miren/data/local", referencing[0].SandboxSpec.Volume[0].MountPath) + require.Len(t, referencing[0].SandboxSpec.Container, 1) + require.Len(t, referencing[0].SandboxSpec.Container[0].Mount, 1) + assert.Equal(t, "local-data", referencing[0].SandboxSpec.Container[0].Mount[0].Source) + assert.Equal(t, "/miren/data/local", referencing[0].SandboxSpec.Container[0].Mount[0].Destination)🤖 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 `@controllers/deployment/launcher_test.go` around lines 2822 - 2826, The current test only checks the SandboxSpec.Volume declaration and misses the user-visible container mount. Update the launcher test around the referencing pool assertions to also verify the mount on SandboxSpec.Container[] using the same pool object, so a missing Container[].Mount mapping cannot slip through. Use the existing referencing, SandboxSpec.Volume, and SandboxSpec.Container fields to keep the assertion aligned with the auto-mount behavior.
🤖 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 `@components/activator/activator_test.go`:
- Around line 2576-2577: Replace the fixed sleep in the activator pool watcher
test with a deterministic readiness check. In the test that launches
activator.watchPools, wait until you observe a watcher update or confirm the
activator cache has reflected a harmless pool update before issuing the
dereference Replace, so the WatchIndex subscription is definitely active. Use
the existing watcher/cache assertions around watchPools and the pool update path
instead of time.Sleep to avoid racing the only update.
In `@controllers/deployment/launcher.go`:
- Around line 827-851: The auto-mount logic in buildSandboxSpec only checks
existing disks by MountPath, so it can still append a disk named local-data and
create duplicate volume names or sources. Update the duplicate detection around
svc.Disks to also consider the disk Name (especially local-data) before
appending the new ConfigSpecServicesDisks entry, and skip adding the
auto-mounted disk if either the mount path or name already exists.
---
Nitpick comments:
In `@controllers/deployment/launcher_test.go`:
- Around line 2822-2826: The current test only checks the SandboxSpec.Volume
declaration and misses the user-visible container mount. Update the launcher
test around the referencing pool assertions to also verify the mount on
SandboxSpec.Container[] using the same pool object, so a missing
Container[].Mount mapping cannot slip through. Use the existing referencing,
SandboxSpec.Volume, and SandboxSpec.Container fields to keep the assertion
aligned with the auto-mount behavior.
🪄 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: 992aa21f-513e-4465-b057-dffa33d10e2b
📒 Files selected for processing (4)
components/activator/activator.gocomponents/activator/activator_test.gocontrollers/deployment/launcher.gocontrollers/deployment/launcher_test.go
An app with existing local data but no explicit [[disks]] config gets its storage auto-mounted, but that mount was patched into the sandbox spec at build time rather than the resolved config. So the volume was enough to drift the pool spec, yet serviceHasDisks stayed false and the stale-pool drain never ran. The result: on a spec change the launcher stood up a replacement pool but never dereferenced the superseded one, leaving a duplicate pool that kept a live version reference. Immortal, because reaping and cleanup are both gated on a pool being unreferenced. Register the auto-mount as a real local disk on the resolved config at config-resolution time instead. serviceHasDisks then reports true, so drainStaleDiskPools reaps the superseded pool like any other disk app, and the transitional build-time special case retires. configureLocal- Volume keys the on-disk path purely on app ID, so naming the disk does not move any existing data.
Even with duplicate pools gone at the source, the activator could still strand ingress on the wrong pool. At startup recoverPools binds each (version,service) to the first pool it iterates, first-writer-wins, and watchPools only ever refreshes cached counts. So a binding pinned to a pool the launcher later drains is never re-pointed at the pool that still serves the version. On a restart that lands on a stale, crash- looping pool, the app goes fully offline while a healthy sibling sits idle. Teach watchPools to drop any version->pool binding whose pool no longer references that version, mirroring what removePoolFromTracking already does on delete. The next lease request then re-resolves via findPool- InStore to the pool that does reference the version. This is the same eviction invariant the activator half-implements for deletes, extended to the dereference-without-delete case, so it protects against a stale binding from any cause, not just this bug. The wiring test uses the etcd-backed server: the in-mem index watch reclassifies every mutation as a Create op and never delivers the Update this exercises.
phinze
force-pushed
the
phinze/mir-1293-auto-mounted-local-storage-bypasses-the-stale-pool-drain
branch
from
371d7ae to
aa8324e
Compare
evanphx
approved these changes
Jul 6, 2026
evanphx
left a comment
evanphx
left a comment
Contributor
There was a problem hiding this comment.
Looks good! 🍪 flagged that DataPath might need more enforcement for being correctly set earlier, but not necessary for this change.
phinze
deleted the
phinze/mir-1293-auto-mounted-local-storage-bypasses-the-stale-pool-drain
branch
phinze
added a commit
that referenced
this pull request
Jul 6, 2026
requestPoolCapacity's pool-creation path runs an inner retry loop whose invariant is that a.mu is held at the top of every iteration: the attempt>0 branch releases the lock before its backoff sleep and re-acquires it afterward. The "another goroutine already created the pool" branch broke that invariant. It unlocked and then did a bare `continue`, which re-enters the inner loop rather than the outer loop the comment intended. On the next iteration the backoff's a.mu.Unlock() ran against an already-unlocked RWMutex, a fatal, unrecoverable throw that took down the whole process (embedded etcd and every app on the node). It only fires when a concurrent goroutine wins the pool-creation race on a retry attempt, i.e. under activation contention. #883's pool-eviction churn drove more traffic through this path and surfaced it on garden, though the racy structure itself dates back to October. Label the outer loop and make that branch `continue poolLoop` so it breaks all the way out and picks up the now-cached pool through the normal wait/increment path. The sibling branch just above gets the same explicit label (already correct, just implicit), and the inner loop's lock invariant is now spelled out in a comment. Adds a stress test that drives the exact race: a pool that becomes visible in the store while concurrent callers are mid-backoff. Closes MIR-1306
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This came out of a real outage: an unattended reboot took an app with auto-mounted local storage fully offline for ~90 minutes while a healthy sandbox sat idle in a sibling pool. Two separate problems, one the cause and one the amplifier, so this is two revs.
The cause is that auto-mounted storage was patched into the sandbox spec at build time instead of the resolved config. That drifts the pool spec enough to spawn a replacement pool, but leaves
serviceHasDisksfalse, which is the gate on the stale-pool drain. So the superseded pool never gets dereferenced and lingers as an immortal duplicate. The first rev registers the auto-mount as a real disk on the resolved config, so the existing drain reaps it like any other disk app.The amplifier is that the activator binds each
(version,service)to the first pool it sees at startup and never re-points that binding, so it can get stuck serving from a drained or crash-looping pool while a healthy one sits idle. The second rev makeswatchPoolsdrop a binding once its pool stops referencing the version, so the next request re-resolves to the pool that actually serves it. It keys off the launcher's own reference, so it guards against a stale binding from any cause, which also makes it safe to roll out alongside the first fix.Closes MIR-1293