Fence sandbox claim cleanup against in-place adoption - #192
Conversation
There was a problem hiding this comment.
The fix is correct and well-scoped. Here's what I verified:
Logic correctness
The old deletePreconditions used an if-else chain that selected uid or resourceVersion — whichever was present first. Because Kubernetes permits in-place adoption (transferring the fencing annotation while keeping the same claim object, and therefore the same UID), a stale cleanup racing against a successor could satisfy the UID precondition and delete the successor's claim. Requiring both uid and resourceVersion simultaneously makes that race fail with a 409 Conflict instead, which is the right outcome.
The spread construct degrades gracefully: when both fields are absent the result is {}, Object.keys returns empty, and the function returns {} (no body), preserving the pre-existing behaviour for metadata-free claims.
Intentional non-change in releaseCancelledExecution
The cancellation path builds its precondition inline and keeps UID-only fencing. That's correct: the cancellation loop already polls and retries, so it doesn't need the stricter version fence, and applying it there would not fix the problem addressed by this PR (which is about the non-looping release() path).
Test
The updated assertion ({ uid: "claim-uid", resourceVersion: "23" }) matches the new combined precondition, and the test fixture already carries metadata.resourceVersion = "23", so the expectation is internally consistent.
CI
All required checks passed on the head SHA (d0cd92b): typecheck, typecheck:test, test:unit (335 tests), security audit, and dependency review.
Summary
Verification
corepack pnpm test:unit -- sandbox-provisioner.test.ts(335 tests passed)corepack pnpm typecheckNo pull request template is present in the repository.