Found by CodeRabbit reviewing #142, verified in the code. Pre-existing in reconcile; filed separately because fixing it properly is a design change rather than a one-liner.
The gap
ResourceLifecycleManager.reconcile deletes a resource whose instance id exceeds targetCount, guarded by sole ownership (ResourceLifecycleManager.java:281-282):
// Case A: Resource ID > target count → should be deleted
if (id > ctx.targetCount) {
if (OwnershipManager.isOwnedSolelyBy(resource, ctx.owner)) {For a claimed instance that guard protects the Deployment and the Services, because claiming adds a Session owner reference (PrewarmedResourcePool.java:1079-1080) and isOwnedSolelyBy is then false.
It does not protect the instance-N-proxy-… and instance-N-email-… ConfigMaps. Those stay owned by the AppDefinition alone for the life of the claim - AGENTS.md states this outright under "The pool's email ConfigMaps carry live session state". So on a scale-down they are deleted while the session, its Deployment and its Services survive.
restoreEmailConfigsOfClaimedInstances does not cover it. It looks the ConfigMap up and, when it is gone, only warns (PrewarmedResourcePool.java:724-726):
if (emailConfigMap == null) {
LOGGER.warn(formatLogMessage(correlationId, "Email config " + emailConfigName ...It rewrites the allow-list of ConfigMaps that still exist; it never recreates a deleted one.
Consequence
A live session whose instance id is above the new minInstances keeps running with its oauth2-proxy ConfigMap deleted. The proxy then has no allow-list to admit anyone, which is the 403-for-everyone failure AGENTS.md already warns about, reached by a different route than the one restoreEmailConfigsOfClaimedInstances was written for.
Reaching it needs minInstances lowered below the id of a currently-claimed instance. reserveInstance hands out the lowest free id, so claimed ids cluster low and this is uncommon - but the admin scaling API can lower minInstances at any time, and the pool runs at 10 in production.
Two ways to fix it
- Give the claim's ConfigMaps a Session owner reference, like the Deployment and Services already get, and drop it on release.
isOwnedSolelyBy then protects them for free and no call site changes. Needs matching cleanup in the release path.
- Teach the ConfigMap phase about claims - pass the claimed ids into the reconcile context and skip deletion for them. Narrower, but it puts the same rule in a second place.
(1) matches the design that is already there, and is what the review suggested.
Relation to #142
#142 does not introduce this, but it widens when it can fire: reconcile now also runs at operator startup, so a scale-down that previously sat harmlessly until the next MODIFIED event is acted on at the next operator restart - and every helm upgrade restarts the operator. An argument for fixing this soon, not for holding #142, which is what stops students being served a stale image.
Found by CodeRabbit reviewing #142, verified in the code. Pre-existing in
reconcile; filed separately because fixing it properly is a design change rather than a one-liner.The gap
ResourceLifecycleManager.reconciledeletes a resource whose instance id exceedstargetCount, guarded by sole ownership (ResourceLifecycleManager.java:281-282):For a claimed instance that guard protects the Deployment and the Services, because claiming adds a
Sessionowner reference (PrewarmedResourcePool.java:1079-1080) andisOwnedSolelyByis then false.It does not protect the
instance-N-proxy-…andinstance-N-email-…ConfigMaps. Those stay owned by the AppDefinition alone for the life of the claim - AGENTS.md states this outright under "The pool's email ConfigMaps carry live session state". So on a scale-down they are deleted while the session, its Deployment and its Services survive.restoreEmailConfigsOfClaimedInstancesdoes not cover it. It looks the ConfigMap up and, when it is gone, only warns (PrewarmedResourcePool.java:724-726):It rewrites the allow-list of ConfigMaps that still exist; it never recreates a deleted one.
Consequence
A live session whose instance id is above the new
minInstanceskeeps running with its oauth2-proxy ConfigMap deleted. The proxy then has no allow-list to admit anyone, which is the 403-for-everyone failure AGENTS.md already warns about, reached by a different route than the onerestoreEmailConfigsOfClaimedInstanceswas written for.Reaching it needs
minInstanceslowered below the id of a currently-claimed instance.reserveInstancehands out the lowest free id, so claimed ids cluster low and this is uncommon - but the admin scaling API can lowerminInstancesat any time, and the pool runs at 10 in production.Two ways to fix it
isOwnedSolelyBythen protects them for free and no call site changes. Needs matching cleanup in the release path.(1) matches the design that is already there, and is what the review suggested.
Relation to #142
#142 does not introduce this, but it widens when it can fire:
reconcilenow also runs at operator startup, so a scale-down that previously sat harmlessly until the next MODIFIED event is acted on at the next operator restart - and everyhelm upgraderestarts the operator. An argument for fixing this soon, not for holding #142, which is what stops students being served a stale image.