fix(operator): never resurrect a package entry on interrupt completion (#426) - #435
Conversation
An interrupt Job is package-agnostic — one per node, stage and interrupt type, deduped across packages — so its completion also has to promote siblings left at (interrupt, skipped). shouldRecordCompletion's interrupt branch authorizes the write on the strength of such a sibling, before looking at the Job's own package at all. recordJobCompletion then took that as license for its own package too: HandleCompletePod's interrupt branch only promotes and reports no update, so the fallback Upsert always ran, and Upsert creates. A rerun, reset or finalizer-driven uninstall that removed the entry while the interrupt Job was completing therefore got it back at (interrupt, complete) — and with the entry present and complete, the rerun predicate keeps the Job, so the stage never runs again. The rerun the user asked for silently does nothing until the failure TTL. The self-write is now gated on entryAwaitsCompletion (present, at this stage, not complete), extracted so shouldRecordCompletion's non-interrupt tail and this guard cannot drift. Promotion is untouched. State is re-read after HandleCompletePod, since promotion can move this package's own entry. Closes #426. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
github-actions
Bot
added
component/operator
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe controller now uses Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage Report for CI Build 31542544617Warning No base build found for commit Coverage: 79.074%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
…n event Follow-ups from review of the previous commit. A deleted NodeWright reads as (nil, nil) from GetSkyhook, and wrapper.Convert dereferences it, so HandleCompletePod's interrupt branch panicked when the CR was removed while a completed interrupt Job was still unprocessed — the same window the entry guard is about. The uninstall branch two blocks down already nil-checks; this one now does too. Pre-existing, but it is the same failure family, and the new spec panics without the guard. The success event could also lie once the self-write is gated: an interrupt Job that reaches the recorder purely on a sibling's promotion left skyhookNode.Changed() true, so the operator announced "Package [x:1.0.0] state complete" for a package whose entry a reset had just cleared. The event now follows what was actually written. Also folds shouldRecordPodErroring onto entryAwaitsCompletion — it was a third verbatim copy of the same predicate in the same package — and notes on recordJobErroring why its erroring-exclusion is deliberately not the same helper. Specs added for the CR-gone panic, the upgrade branch's RemoveState path (previously untested), and no-regression from a later stage on the interrupt path, which is the direction only the new guard covers. The design doc now records that the create-nothing rule binds the Job path too, not just the Pod watch. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
github-actions
Bot
added
the
doc
|
Ran an independent Opus review over the first commit. Verdict was that the fix is sound — it traced the guard across every stage and both Job kinds, and separately confirmed the two things I was least sure of: It found three things worth acting on, now in 1. A nil-CR panic in the branch this PR hardens. 2. The success event could lie — introduced by my change. Once the self-write is gated, an interrupt Job reaching the recorder purely on a sibling's promotion still had 3. The "cannot drift" extraction was 2/3 done. Two nits taken as written rather than as code: One flagged trade left alone: if the Job's own package is the one at (interrupt, Specs added for the CR-gone panic, the upgrade branch (previously untested), and no-regression from a later stage on the interrupt path — the direction only the new guard covers. Suite 306/306, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Line 131: Update the prose around the Job path and interrupt Jobs to use the
grammatical phrase “write a package entry that node state says is gone” and
replace “on the strength of” with “because,” without changing the described
invariant or behavior.
In `@operator/internal/controller/job_controller_test.go`:
- Line 470: Strengthen the assertion in the completion-path test around
getNodeState so it verifies both StagePostInterrupt and StateComplete, or
compares the expected complete PackageStatus, ensuring the retained stage cannot
mask an incorrect state.
In `@operator/internal/controller/pod_controller.go`:
- Line 245: Update the completion check around entryAwaitsCompletion so error
recording uses an error-specific predicate that excludes StateErroring, matching
recordJobErroring and preserving idempotency for re-served terminal events.
Restore or extract an entryAwaitsErroring helper and use it for this return path
without changing non-error completion eligibility.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 959e3670-78a0-4538-9bb2-5ee348825ed6
📒 Files selected for processing (4)
docs/designs/2026-07-10-package-execution-as-jobs.mdoperator/internal/controller/job_controller.gooperator/internal/controller/job_controller_test.gooperator/internal/controller/pod_controller.go
Review pointed out that reusing a predicate called entryAwaitsCompletion for the Pod watch's erroring write reads as the wrong thing, which is the same complaint that renamed isParkedJob. It is now entryOpenAtStage — present, at this stage, not complete — which is what both callers actually require, with the doc comment saying so and recording why an already-erroring entry is deliberately still open (a rising restart count must land; an identical write is dropped by the Changed() check). Also tightens the no-regression spec to assert State as well as Stage, and fixes a garbled sentence in the design doc. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/internal/controller/job_controller.go (1)
339-377: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd recorder coverage for interrupt completion.
HandleCompletePodreturnsfalsefor interrupt completion, including sibling promotion. Assert that the removed-self case emits only the generic interrupt event, not a package-specific completion event.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/internal/controller/job_controller.go` around lines 339 - 377, Add recorder coverage for the interrupt-completion path in HandleCompletePod where the package entry was removed after sibling promotion. Assert that it emits only the generic interrupt completion event and does not emit the package-specific JobComplete event.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/designs/2026-07-10-package-execution-as-jobs.md`:
- Around line 131-132: Revise the prose around the Job path restriction to say
it cannot write an entry node state that the package says is gone, explicitly
including “package.” Replace the idiomatic “This bites” phrasing and the
indirect “on the strength of” wording with direct, precise language while
preserving the existing explanation and behavior.
In `@operator/internal/controller/job_controller_test.go`:
- Around line 470-472: Update the reconciliation test assertions around
JobReconcile to verify sibling promotion: use sibling.GetUniqueName() to assert
its entry reaches StateComplete, while retaining the existing assertions that
pkgRef remains at StagePostInterrupt and StateComplete. This must distinguish
actual reconciliation behavior from pkgRef’s pre-existing state.
---
Outside diff comments:
In `@operator/internal/controller/job_controller.go`:
- Around line 339-377: Add recorder coverage for the interrupt-completion path
in HandleCompletePod where the package entry was removed after sibling
promotion. Assert that it emits only the generic interrupt completion event and
does not emit the package-specific JobComplete event.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 36eb7fa6-640a-45fc-bd0a-485aa4c2d018
📒 Files selected for processing (4)
docs/designs/2026-07-10-package-execution-as-jobs.mdoperator/internal/controller/job_controller.gooperator/internal/controller/job_controller_test.gooperator/internal/controller/pod_controller.go
… path The spec seeded the entry at (post-interrupt, complete) and asserted it was still there, which a reconcile that did nothing at all would also satisfy. It now asserts the sibling was promoted and the Job carries the state-recorded marker, so the interrupt completion path demonstrably ran while the entry was left alone. Also inserts the relative pronoun the design doc sentence was missing. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
4 participants
Important
Base this work on
feature/package-as-jobs, notmain.The Jobs migration (#223) is integrating on that branch.
maindoes not yet executepackages as Jobs, so a PR opened against
mainwill be missing the code this dependson.
feature/package-as-jobsmerges tomainonce #305 lands.That branch squash-merges, so if your branch is stacked on another PR in this epic,
replay only your own commits when restacking:
git rebase --onto origin/feature/package-as-jobs <last-inherited-commit>Closes #426. Part of #223, based on
feature/package-as-jobs(notmain).#426 was single-reporter and never cross-verified, so this starts with a failing spec rather than a fix.
Reproduced first
promotes the sibling without resurrecting a package whose entry was removedfails on the parent commit: node state comes back holdingtuning|1.0.0after the entry was removed.What it is
An interrupt Job is package-agnostic — one per (node, stage, interrupt type), deduped across packages — so its completion also has to promote siblings sitting at (interrupt,
skipped).shouldRecordCompletion's interrupt branch authorizes the write on the strength of such a sibling, before looking at this Job's own package:recordJobCompletionthen treated that as license for its own package too.HandleCompletePod's interrupt branch only callsProgressSkippedand reportsupdated == false, so the fallbackUpsertalways ran — andUpsertcreates. Apackage rerun, a reset, or a finalizer-driven uninstall that removed the entry while the interrupt Job was completing got it back at (interrupt,complete).The second-order effect is the damaging one: with the entry present and complete,
shouldDeleteFinishedJob'srecordedDonearm keeps the Job, so the stage never re-runs. The rerun silently does nothing until the failure TTL expires.The fix
The self-write is gated on
entryAwaitsCompletion— present, still at this Job's stage, not already complete — extracted soshouldRecordCompletion's non-interrupt tail and this guard cannot drift apart. Promotion is untouched: the sibling still lands atcompletein the same spec.Node state is re-read after
HandleCompletePodrather than before, since promotion can move this package's own entry.This is the same invariant the Pod watch already states for itself (
shouldRecordPodErroring: "this watch is not an authority that may create or resurrect a node-state entry") — the Job path is the completion authority, but "authority" was never meant to include creating an entry that state says is gone.Considered and rejected
Moving the presence check above the interrupt branch — the one-line version — also drops the sibling promotion whenever this Job's own package was removed, which can leave packages at (interrupt,
skipped) until another interrupt cycle. That trades a resurrection for a possible stall.Verification
Controller suite 303/303 (302 before, +1 new),
make lint0 issues,go build ./...clean. No chainsaw fixture touches this path. Not run: e2e (no cluster locally) — CI covers it.