feat: wire runner restart counters (RestartCount/RestartTime) (step 2) - #1333
Merged
Conversation
Owner
Author
|
PR #1333 Review — LGTM — makes |
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
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.
Summary
maybeRestartExitedContainersinrefresh.go) as the single place a non-root container restart happens. This step makes that pass the sole writer of the user-visibleRestartCount/RestartTimecounters and turnspopulateCellContainerStatusesinto a pure preserver of them.refresh.go): newstampContainerRestarthelper, called frommaybeRestartExitedContainersimmediately after a successful relaunch — bumpsRestartCountby exactly one over the preserved prior and stampsRestartTime = r.nowUTC().StartContainerre-derives statuses from the persisted (preserved) prior, so the bump lands on top of it;ReconcileCell'srestartFiredbranch then persists the incremented value via the existingpersistCellStatusGuardedpath. Single increment site ⇒ exact, monotonic count that survives ticks.helpers.go):populateCellContainerStatusesnow snapshotspriorRestartCount/priorRestartTime(same observe-and-preserve contract asCreatedAt/StartTime/ExitCode) and carries them across the unconditional overwrite. It never increments; a never-restarted container preserves zero from zero. The prior intentionally-zero block is gone.runner.go): therestartStatesdoc-comment previously flagged the user-visible counters as "runner: restart counters (RestartCount/RestartTime) (step 2) #1234's scope (documented intentionally-zero in chore(controller): document RestartCount/RestartTime as intentionally-zero pending #1151 #1303)"; updated to point atstampContainerRestart/ the populate preserve now that they're wired.Premise-rot note (salvageable — flagged per dev CLAUDE.md)
The issue's Proposal says to "Replace the
TODO(#1146)markers (helpers.go:864-865)". Those markers no longer exist: #1303 (the #1146 marker-documentation chore) landed first and replaced them withintentionally-zerocomments, which had since shifted to ~helpers.go:1057-1070. Per the issue's own "Ordering vs #1146" note ("if #1146's marker-documentation chore lands first, this step supersedes its comments"), I superseded the current intentionally-zero block rather than the originalTODO(#1146)text. Intent unchanged — these are the same lines, just relabeled and relocated by the intervening chore.Test plan
make test(full CI suite,test-root+test-kukebuild) — green,MAKE-TEST-EXIT=0, no FAIL lines.GOWORK=off go build ./internal/controller/runner/andgo vet— clean.internal/controller/runner/restart_on_exit_test.go:TestMaybeRestartExitedContainers_BumpsRestartCounterByOne— increment-by-one +RestartTimestamp on a fired restart; root container untouched (AC 1, 2).TestMaybeRestartExitedContainers_RestartCounterMonotonic— prior 4 → 5, no reset (AC 4).TestMaybeRestartExitedContainers_NoBumpWhenNoRestartFires— sole-writer contract: a no-fire tick leaves the counters exactly as preserved.TestPopulateCellContainerStatuses_PreservesRestartCounters— populate preserves across two passes, never increments (AC 3, 5).Acceptance criteria
RestartCountincrements by exactly one per reconciler-driven restart and survives subsequent reconciliation passes (increment in the restart pass + populate preserve).RestartTimereflects the wall-clock time of the most recent restart and survives subsequent passes.populateCellContainerStatusesnever increments — pure preserver; the intentionally-zero block (the relabeledTODO(#1146)markers — see premise-rot note) is gone.Closes #1234