fix: remove panic paths from patch, ip_allocator, label_selector, cronjob#35
Open
indyjonesnl wants to merge 5 commits into
Open
fix: remove panic paths from patch, ip_allocator, label_selector, cronjob#35indyjonesnl wants to merge 5 commits into
indyjonesnl wants to merge 5 commits into
Conversation
Malformed PATCH bodies could panic the handler at several places:
- as_object_mut/as_object unwraps on merge-patch + strategic-merge-patch
bases and patches
- op.value.as_ref().unwrap() for Add/Replace/Test JSON Patch operations
- result_obj[key].as_array().unwrap() in strategic merge array branches
- path.rfind('/').unwrap() in split_path
Convert each to a descriptive PatchError::InvalidPatch via ok_or_else.
Tighten the named-array detection with is_some_and. Tests in #[cfg(test)]
are unchanged — their .unwrap() calls operate on Result values returned
by the patch functions themselves.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add three tests covering the panics fixed in this branch: JSON Patch add/replace/test ops that omit `value`. Before the fix these called `op.value.as_ref().unwrap()` on None and crashed the handler. The tests now assert apply_patch returns PatchError::InvalidPatch instead of unwinding, so reverting the production fix re-introduces test failures.
A panic in any thread that held the allocator lock would cause every subsequent lock().unwrap() to panic too, locking out all ClusterIP allocation. Route all 5 callers through a lock_allocated() helper that recovers via PoisonError::into_inner() with a single warn-level log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Display impl indexed req.values.as_ref().unwrap()[0] for Equals, NotEquals, In, and NotIn. A LabelRequirement built without values (a hand-constructed one, not from the parser) would panic when formatted. Fall back to '' for the first-value case and an empty list for joined values, and add a regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
successful_jobs_history_limit and failed_jobs_history_limit are i32. A negative value cast via 'as usize' wraps to ~4 billion, which made the size comparison pass and the subtraction also wrap, preventing any history cleanup. Switch to checked usize::try_from(...).unwrap_or(0). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Problem
Five panic sites reachable from runtime inputs. Each is a process-killing failure for what should be a request-level error.
Fix
Verification
`cargo build --workspace --locked` clean. New regression tests pass. No new test failures vs the baseline.
Dependency
Depends on #32 for the test suite to show green.