fix: send propagationPolicy at the top level of DeleteOptions

[abnegate]

Problem

RunsClusterOperations::delete() claims to support propagationPolicy, but it never reaches the Kubernetes API. Two stacked bugs:

1. propagationPolicy and gracePeriodSeconds were nested inside the preconditions map, where the API ignores them (preconditions only accepts uid and resourceVersion).
2. The $this->refresh() call that followed setAttribute('preconditions', ...) replaces the resource attributes entirely (syncWith() assigns $this->attributes), so even the misplaced fields were wiped before serialization. The actual body sent was the refreshed resource relabelled as kind: DeleteOptions:

{
    "apiVersion": "batch/v1",
    "kind": "DeleteOptions",
    "metadata": {
        "name": "reap",
        "uid": "...",
        "resourceVersion": "12345",
        "namespace": "default"
    }
}

No propagationPolicy anywhere, so Job deletes fall back to the API default for batch/v1 (Orphan). This is the root cause of the DAT-1869 reopen: edge's resize reap calls $job->delete(propagationPolicy: 'Foreground'), the delete orphans the Completed pods (no ownerRef, no TTL), pvc-protection never releases the claim, and the resize wedges permanently.

Fix

Build the DeleteOptions payload explicitly instead of mutating the resource. New body:

{
    "apiVersion": "v1",
    "kind": "DeleteOptions",
    "propagationPolicy": "Foreground",
    "preconditions": {
        "uid": "..."
    }
}

• propagationPolicy and gracePeriodSeconds (when set) are top-level DeleteOptions fields.
• preconditions carries only the resource uid, guarding against deleting a recreated resource of the same name.
• The resourceVersion precondition is dropped deliberately: it never took effect before (see bug 2), and enforcing it now would introduce conflict failures into every consumer's delete path whenever a controller bumps the resource between refresh() and the DELETE call.
• The payload builder is extracted to deleteOptions() so the shape is unit-testable without a cluster.

Tests

tests/DeleteOptionsTest.php asserts the payload shape: propagationPolicy at the top level (and not in preconditions), gracePeriodSeconds at the top level when set, preconditions limited to uid and omitted when the uid is unknown, and no resource attributes leaking into the body.

Consumers

Edge's dedicated-database resize reap depends on this fix; the edge-side bump should happen after this merges and a release is tagged.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[greptile-apps]

Greptile Summary

This PR fixes two stacked bugs in RunsClusterOperations::delete() that caused propagationPolicy to never reach the Kubernetes API, making Job deletes silently fall back to the API default (Orphan) and leaving pods orphaned. The fix replaces in-place attribute mutation with a dedicated deleteOptions() builder that produces a correctly-shaped DeleteOptions payload.

• propagationPolicy and gracePeriodSeconds are now top-level fields in the DeleteOptions body, and preconditions carries only the resource uid, matching the Kubernetes API spec.
• The resourceVersion precondition is intentionally dropped (it never took effect before and would introduce conflict failures on every delete path).
• deleteOptions() is extracted as a public method with unit tests that verify the payload shape, the absence of resource attributes, and the omission of preconditions when the uid is unknown.

Confidence Score: 4/5

The fix is well-scoped and correct — only delete() and the new deleteOptions() builder changed, replacing a clearly broken attribute-mutation pattern with an explicit payload.

The core logic is sound: propagationPolicy and gracePeriodSeconds land at the top level of the DeleteOptions body, refresh() still runs before deleteOptions() so the uid in preconditions reflects the live cluster state, and preconditions are safely omitted when no uid is available. The new tests cover the critical shape assertions. Two minor style suggestions were left — neither affects correctness.

No files require special attention — both changed files are straightforward and well-covered by the new tests.

Important Files Changed

Filename	Overview
src/Traits/RunsClusterOperations.php	Extracts deleteOptions() as a dedicated payload builder placing propagationPolicy and gracePeriodSeconds at the top level, and uid only in preconditions. Fixes two stacked bugs from the original code (wrong nesting and attribute wipeout by refresh()).
tests/DeleteOptionsTest.php	New unit tests verifying the DeleteOptions payload shape: top-level propagationPolicy, conditional top-level gracePeriodSeconds, uid-only preconditions, and no resource attributes leaking into the body.

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/Traits/RunsClusterOperations.php:263-270
The `$propagationPolicy` parameter accepts any string, but the Kubernetes API only recognises `Foreground`, `Background`, and `Orphan`. An invalid value (e.g. a typo like `'foreground'`) is silently forwarded and rejected by the API at request time. Adding a docblock listing the valid values makes the contract clear without requiring a runtime check.

```suggestion
    /**
     * Build the DeleteOptions payload for the delete operation.
     * The propagationPolicy and gracePeriodSeconds fields are top-level
     * DeleteOptions fields; only the resource uid belongs in preconditions.
     *
     * @param  string  $propagationPolicy  One of 'Foreground', 'Background', or 'Orphan'.
     *
     * @return array<string, mixed>
     */
    public function deleteOptions(?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): array
```

### Issue 2 of 2
tests/DeleteOptionsTest.php:76
The `json_decode(json_encode(...))` round-trip is redundant here — `deleteOptions()` already returns a plain PHP array, so the shape can be asserted directly. The round-trip doesn't add safety because `json_encode` of a PHP array is lossless for scalar values, and the assertion on lines 90–91 already guards against resource-attribute leakage.

```suggestion
        $payload = $job->deleteOptions(null, 'Foreground');
```

_{Reviews (1): Last reviewed commit: "fix: send propagationPolicy at the top l..." | Re-trigger Greptile}

[greptile-apps]

The $propagationPolicy parameter accepts any string, but the Kubernetes API only recognises Foreground, Background, and Orphan. An invalid value (e.g. a typo like 'foreground') is silently forwarded and rejected by the API at request time. Adding a docblock listing the valid values makes the contract clear without requiring a runtime check.

Suggested change

	/**
	* Build the DeleteOptions payload for the delete operation.
	* The propagationPolicy and gracePeriodSeconds fields are top-level
	* DeleteOptions fields; only the resource uid belongs in preconditions.
	*
	* @return array<string, mixed>
	*/
	public function deleteOptions(?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): array
	/**
	* Build the DeleteOptions payload for the delete operation.
	* The propagationPolicy and gracePeriodSeconds fields are top-level
	* DeleteOptions fields; only the resource uid belongs in preconditions.
	*
	* @param string $propagationPolicy One of 'Foreground', 'Background', or 'Orphan'.
	*
	* @return array<string, mixed>
	*/
	public function deleteOptions(?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): array

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/Traits/RunsClusterOperations.php
Line: 263-270

Comment:
The `$propagationPolicy` parameter accepts any string, but the Kubernetes API only recognises `Foreground`, `Background`, and `Orphan`. An invalid value (e.g. a typo like `'foreground'`) is silently forwarded and rejected by the API at request time. Adding a docblock listing the valid values makes the contract clear without requiring a runtime check.

```suggestion
    /**
     * Build the DeleteOptions payload for the delete operation.
     * The propagationPolicy and gracePeriodSeconds fields are top-level
     * DeleteOptions fields; only the resource uid belongs in preconditions.
     *
     * @param  string  $propagationPolicy  One of 'Foreground', 'Background', or 'Orphan'.
     *
     * @return array<string, mixed>
     */
    public function deleteOptions(?int $gracePeriod = null, string $propagationPolicy = 'Foreground'): array
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[greptile-apps]

The json_decode(json_encode(...)) round-trip is redundant here — deleteOptions() already returns a plain PHP array, so the shape can be asserted directly. The round-trip doesn't add safety because json_encode of a PHP array is lossless for scalar values, and the assertion on lines 90–91 already guards against resource-attribute leakage.

Suggested change

	$payload = json_decode(json_encode($job->deleteOptions(null, 'Foreground')), true);
	$payload = $job->deleteOptions(null, 'Foreground');

Prompt To Fix With AI

This is a comment left during a code review.
Path: tests/DeleteOptionsTest.php
Line: 76

Comment:
The `json_decode(json_encode(...))` round-trip is redundant here — `deleteOptions()` already returns a plain PHP array, so the shape can be asserted directly. The round-trip doesn't add safety because `json_encode` of a PHP array is lossless for scalar values, and the assertion on lines 90–91 already guards against resource-attribute leakage.

```suggestion
        $payload = $job->deleteOptions(null, 'Foreground');
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!