internal/planner: three documented invariants are unguarded by tests

Open
#392 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
65/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Quiet
Tech stack
go, kubernetes
Domain
devops, testing

Research direction

Start with internal/planner/doc.go and the CLAUDE.md Key Patterns, then inspect TestReconcile_CreatesPlanOnFirstRun and node_update_test.go. Add coverage for zero submissions on plan creation, one Status().Patch per reconcile path, and Running after terminal NodeUpdate failure. Run the internal/planner tests and confirm all three invariants are enforced.

Written by the indexing model from the issue text.

Description

Problem

Three internal/planner invariants are documented in doc.go (sei-k8s-controller#391) and listed in CLAUDE.md Key Patterns, but no test directly guards them — a regression of any of the three compiles, lints, and passes CI today:

  1. Atomic plan creation (persist-before-execute ordering). TestReconcile_CreatesPlanOnFirstRun asserts the plan is persisted and Active, but does not assert zero task submissions on the creating reconcile. Its comment even says "and executes the first task," which contradicts the planAlreadyActive gate (the creating reconcile requeues without executing). The ordering — plan visible before any side effect — is unverified.
  2. Single-patch model. Nothing asserts how many Status().Patch calls occur per reconcile path. A second status write per reconcile is legal Go and would pass; the single-snapshot/single-flush invariant is unverified.
  3. FailedPhase == "" means retry, not terminal. node_update_test.go asserts a NodeUpdate plan's FailedPhase is empty, but no test asserts node.Status.Phase remains Running after a NodeUpdate task fails terminally — the actual retry-not-terminal behavior is unverified.

Impact

These are correctness/idempotency invariants: plan-creation atomicity (external observers must see the plan before side effects), single-patch staleness safety (paired with the optimistic-lock rule), and NodeUpdate retry semantics. They are load-bearing per CLAUDE.md, but a silent regression would not be caught by the suite.

Relevant experts

  • kubernetes-specialist — controller-runtime test patterns, envtest harness.
  • idiomatic-reviewer — surfaced these during the /idiomatic pilot.

Proposed approach

  • Atomic creation: assert len(submitted) == 0 on the creating reconcile in TestReconcile_CreatesPlanOnFirstRun, and fix the misleading comment.
  • Single-patch: assert exactly one Status().Patch per reconcile path (a counting fake client, or a reconcile-level assertion).
  • Retry semantics: assert the node stays Running after a NodeUpdate task fails terminally.

Out of scope

  • The doc.go documentation change itself (sei-k8s-controller#391).
  • A stale-write race test for the optimistic-lock invariant — needs an envtest concurrency harness; separate and harder.

References

  • Surfaced by the Tide /idiomatic expert pilot: sei-protocol/Tide#126
  • Invariants documented in: sei-k8s-controller#391
Dominant language
Go
Stars
1
Forks
2
Avg merge
2h 29m
Merged PRs (30d)
56

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from sei-protocol/sei-k8s-controller

All issues in sei-protocol/sei-k8s-controller

Similar issues

More Go issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.