Claude/job failed exit detection lmzgq6#76
Merged
Conversation
Job.status.conditions[].type === 'Failed' alone can appear with status: 'False' or 'Unknown' per the JobCondition spec, causing false-positive aborts. Require status: 'True' as well.
Previously only a generic "Job is failed. Exit!" message was logged, giving no indication of the underlying cause (e.g. BackoffLimitExceeded vs DeadlineExceeded). Surface job name, condition reason and message.
The Job selector only matches on app.kubernetes.io/instance, so a Failed Job left over from a previous release (not yet GC'd) could match and abort a brand-new upgrade before its own Job even starts. Record the watcher start time and skip any matched Job whose creationTimestamp predates it.
…ejection createChildProcess(wait=true) rejects on unexpected kubectl exit codes (network errors, RBAC denials, etc.). Previously this rejection escaped the setInterval callback and was only caught by the global unhandledRejection handler, which aborts via SIGTERM without setting process.exitCode, making the real cause (kubectl failure vs. actual Job failure) unclear. Catch and log it explicitly at the source instead.
createChildProcess(wait=true) accepted exit codes 0 and 1 as success,
inherited from an older "kubectl get job <name>" invocation where 1
meant NotFound. After the switch to --selector-based queries, an empty
match returns 0 with {"items":[]}, so 1 now only masks real errors
(e.g. RBAC Forbidden) as success, silently returning invalid output.
… used Explicitly defer sub-task 6: re-deriving terminal failure from status.failed + backoffLimit would duplicate and could race with the Job controller's own logic, whereas status.conditions is set as soon as the controller reaches a terminal state. Recorded as a comment so the decision isn't re-litigated later.
When multiple Jobs (or conditions) matched as Failed in the same
tick, the forEach loops called process.emit('SIGTERM') once per
match, producing duplicate log lines. Guard with a local flag so
detection and shutdown happen exactly once per tick.
Existing fixtures/CI only cover the successful Job scenario (job-complited.yaml). Add job-failed.yaml, which deploys a Job that deterministically fails on its first attempt (backoffLimit: 0, APP_EXIT_CODE: 1), plus deploy_job_failed.sh which asserts helm-assistant exits non-zero and logs the "Job is failed" detection line, and wire it into the CI workflow as a new step.
…testing Add src/Kubernetes/JobTypes.ts with typed interfaces for the batch/v1 Job API (V1Job, V1JobStatus, V1JobCondition, etc.), no `any` anywhere. Extract evaluateJobItems() (decides whether any matched Job has terminally failed) and interpretChildProcessExit() (decides how a finished child process should settle its promise) as pure, side-effect-free, exported functions. Both take/return plain typed values, so they can be unit tested directly with hand-written fixtures - no real kubectl process or mocked child_process required. watchJobStatus() and createChildProcess() now just call these functions and act on the result; behavior is unchanged.
Cover evaluateJobItems() with hand-written V1Job fixtures: Failed status True/False/Unknown, BackoffLimitExceeded and DeadlineExceeded reasons, stale job filtering, multi-job dedup, missing conditions, Complete condition, and empty item list. Cover interpretChildProcessExit() with exit code 0 (success), 1 and other non-zero codes (now correctly errors, not silently accepted), SIGINT, and the null/no-signal edge case. No real kubectl/cluster or child_process mocking needed - both units are pure functions. Wire `yarn test` into CI as its own step.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50c97dcda6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wirwolf
added a commit
that referenced
this pull request
Jul 14, 2026
* fix(job-watch): require condition.status === 'True' for Failed detection
Job.status.conditions[].type === 'Failed' alone can appear with
status: 'False' or 'Unknown' per the JobCondition spec, causing
false-positive aborts. Require status: 'True' as well.
* feat(job-watch): log job name, reason and message on Failed detection
Previously only a generic "Job is failed. Exit!" message was logged,
giving no indication of the underlying cause (e.g. BackoffLimitExceeded
vs DeadlineExceeded). Surface job name, condition reason and message.
* fix(job-watch): ignore stale Jobs created before this upgrade started
The Job selector only matches on app.kubernetes.io/instance, so a
Failed Job left over from a previous release (not yet GC'd) could
match and abort a brand-new upgrade before its own Job even starts.
Record the watcher start time and skip any matched Job whose
creationTimestamp predates it.
* fix(job-watch): catch polling errors instead of leaking to unhandledRejection
createChildProcess(wait=true) rejects on unexpected kubectl exit codes
(network errors, RBAC denials, etc.). Previously this rejection escaped
the setInterval callback and was only caught by the global
unhandledRejection handler, which aborts via SIGTERM without setting
process.exitCode, making the real cause (kubectl failure vs. actual
Job failure) unclear. Catch and log it explicitly at the source instead.
* fix(job-watch): stop treating kubectl exit code 1 as success
createChildProcess(wait=true) accepted exit codes 0 and 1 as success,
inherited from an older "kubectl get job <name>" invocation where 1
meant NotFound. After the switch to --selector-based queries, an empty
match returns 0 with {"items":[]}, so 1 now only masks real errors
(e.g. RBAC Forbidden) as success, silently returning invalid output.
* docs(job-watch): document why status.failed/succeeded counters aren't used
Explicitly defer sub-task 6: re-deriving terminal failure from
status.failed + backoffLimit would duplicate and could race with the
Job controller's own logic, whereas status.conditions is set as soon
as the controller reaches a terminal state. Recorded as a comment so
the decision isn't re-litigated later.
* fix(job-watch): emit SIGTERM only once per polling tick
When multiple Jobs (or conditions) matched as Failed in the same
tick, the forEach loops called process.emit('SIGTERM') once per
match, producing duplicate log lines. Guard with a local flag so
detection and shutdown happen exactly once per tick.
* test(ci): add CI coverage for the Failed-Job detection path
Existing fixtures/CI only cover the successful Job scenario
(job-complited.yaml). Add job-failed.yaml, which deploys a Job that
deterministically fails on its first attempt (backoffLimit: 0,
APP_EXIT_CODE: 1), plus deploy_job_failed.sh which asserts
helm-assistant exits non-zero and logs the "Job is failed" detection
line, and wire it into the CI workflow as a new step.
* refactor(job-watch): extract typed, pure decision functions for unit testing
Add src/Kubernetes/JobTypes.ts with typed interfaces for the batch/v1
Job API (V1Job, V1JobStatus, V1JobCondition, etc.), no `any` anywhere.
Extract evaluateJobItems() (decides whether any matched Job has
terminally failed) and interpretChildProcessExit() (decides how a
finished child process should settle its promise) as pure,
side-effect-free, exported functions. Both take/return plain typed
values, so they can be unit tested directly with hand-written
fixtures - no real kubectl process or mocked child_process required.
watchJobStatus() and createChildProcess() now just call these
functions and act on the result; behavior is unchanged.
* test: add unit tests for Job-failed detection and exit-code handling
Cover evaluateJobItems() with hand-written V1Job fixtures: Failed
status True/False/Unknown, BackoffLimitExceeded and DeadlineExceeded
reasons, stale job filtering, multi-job dedup, missing conditions,
Complete condition, and empty item list.
Cover interpretChildProcessExit() with exit code 0 (success), 1 and
other non-zero codes (now correctly errors, not silently accepted),
SIGINT, and the null/no-signal edge case.
No real kubectl/cluster or child_process mocking needed - both units
are pure functions. Wire `yarn test` into CI as its own step.
* [feat] refactor repo structure, upgrade dependencies, and refine job handling logic
* [chore] upgrade upload-artifact action to v4 in CI workflow
* [chore] upgrade dependencies and replace "pkg" with scoped "@yao-pkg/pkg"
* [chore] update download-artifact action to v4 in CI workflow
* [fix] correct exit code in job-failed test case
* [fix] refine job-failed test case logic for accurate detection
---------
Co-authored-by: Claude <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
2 participants
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.
No description provided.