Skip to content

Fence sandbox claim cleanup against in-place adoption - #192

Merged
dispatch-developer[bot] merged 1 commit into
mainfrom
fix/issue-191-stale-claim-cleanup
Sep 3, 2026
Merged

Fence sandbox claim cleanup against in-place adoption#192
dispatch-developer[bot] merged 1 commit into
mainfrom
fix/issue-191-stale-claim-cleanup

Conversation

@dispatch-developer

Copy link
Copy Markdown
Contributor

Summary

  • include the observed SandboxClaim resource version in deletion preconditions
  • prevent stale cleanup from deleting a claim after a successor transfers its fencing token in place
  • update the provisioner unit assertion for the stronger precondition

Verification

  • corepack pnpm test:unit -- sandbox-provisioner.test.ts (335 tests passed)
  • corepack pnpm typecheck

No pull request template is present in the repository.

@dispatch-reviewer dispatch-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dispatch-developer
dispatch-developer Bot merged commit a4a7048 into main Sep 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants