Skip to content

fix(seinetwork): recover the genesis ceremony when a founding validator is lost mid-plan - #545

Merged
bdchatham merged 5 commits into
mainfrom
devin/1789079303-mid-ceremony-validator-loss
Sep 11, 2026
Merged

bdchatham merged 5 commits into
mainfrom
devin/1789079303-mid-ceremony-validator-loss

Conversation

@bdchatham

@bdchatham bdchatham commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes PLT-1242 (found while deflaking PLT-1241 / #543). A founding validator SeiNode deleted while the genesis ceremony plan is active wedges the SeiNetwork forever:

  • reconcileSeiNodes defers creates while PlanInProgress=True (deliberate — the ceremony stamps peers on the children);
  • collect-and-set-peers / await-nodes-running Get the founding set by the names frozen into the plan and error on the missing one, so the plan just retries;
  • needsGenesisPlan returns false while Status.Plan != nil, so nothing ever rebuilds it.

Fix, in reconcilePlan before driving an active plan:

if network.Status.Plan != nil && Plan.Phase == TaskPlanActive {
    if validatorLost(network) {           // len(Status.IncumbentNodes) < Spec.Replicas
        if err := r.abandonPlanForLostValidator(ctx, network); err != nil { return ctrl.Result{}, err }
        return planner.ResultRequeueImmediate, nil
    }
    return r.drivePlan(ctx, network)
}

abandonPlanForLostValidator deletes every surviving founding child this network minted, clears Status.Plan, sets PlanInProgress=False/ValidatorLost and GenesisCeremonyComplete=False/ValidatorLost, and emits a Warning event. The next reconciles recreate the whole set through the normal ensureSeiNode path and needsGenesisPlan rebuilds the ceremony once every replica exists again.

Why the whole set and not just the lost node: the sidecar gates assemble-genesis and configure-genesis on marker files in the data PVC. A survivor that kept its PVC would keep the genesis minted from the dead node's gentx (and the assembler would skip reassembly), so the rebuilt ceremony would either split the set across two genesis hashes or latch GenesisCeremonyComplete=True over a validator set containing a key nobody holds. Deleting the survivors makes the SeiNode finalizer remove their PVCs — markers included — so every founding node re-derives from the new set. A set the ceremony has not finished minting holds no chain state, so an empty restart is the only convergent outcome. Each survivor delete emits Warning/FoundingSetTornDown (distinct from the scale-down SeiNodeDeleted). ValidatorLost is sticky in setGenesisCeremonyCondition like CeremonyFailed but is intentionally not mapped to GroupPhaseFailed: it self-heals.

Adopted sets are never torn down. A Retain teardown orphans children with their consensus identities and chain data; a same-named SeiNetwork recreated over them adopts them and runs a (marker-no-op) ceremony over established validators. A loss under that plan must not cascade. The positive signal is age: minted children are created after the network, adopted children predate it. If any survivor has CreationTimestamp.Before(network.CreationTimestamp) the plan is abandoned, nothing is deleted, and GenesisCeremonyComplete latches True/AdoptedSet so needsGenesisPlan never rebuilds a ceremony over the adopted set plus a marker-less replacement (which could reassemble and republish genesis over the live chain's). The gate reopens and the lost node is recreated through the plain replacement path.

populateIncumbentNodes now skips children with a DeletionTimestamp. A child held in Terminating by its finalizer is not a ceremony participant: counting it let the planner rebuild the ceremony over a node on its way out, which the network then had to abandon a second time.

The check is count-based rather than name-based because replicas are immutable once the ceremony starts and creates are gated under the plan, so "fewer incumbents than replicas under an active plan" can only mean a founding child vanished.

Tests

  • TestReconcilePlan_ValidatorLostMidCeremony_AbandonsPlan (unit): survivors deleted, plan cleared, both conditions ValidatorLost, immediate requeue, seed keeps the reason.
  • TestReconcilePlan_ValidatorLostOverAdoptedSet_KeepsSurvivors (unit): survivors older than the network are kept, plan abandoned, GenesisCeremonyComplete=True/AdoptedSet latched.
  • TestPopulateIncumbentNodes_ExcludesTerminatingChildren (unit).
  • TestGenesisCeremony_ValidatorLostMidCeremony_Recovers (envtest): deletes -1 under an active plan and waits for the ValidatorLost event, both children recreated with new UIDs, and GenesisCeremonyComplete=True over the new set. Two test-harness enablers: StubSidecarClient.SetCompleteAfter (locked setter; the instant stub otherwise finishes the ceremony within one reconcile lap so there is no window to delete under) and a testStub handle in the suite. The test strips finalizers and deletes both data PVCs up front, standing in for the GC envtest doesn't run — otherwise a recreated child's init plan refuses to adopt the orphaned claim. It therefore proves recreation + rebuilt ceremony, not marker clearing; that rests on the SeiNode finalizer's deleteNodeDataPVC (TestNodeDeletion_SnapshotNode_WithoutRetain_DeletesPVC).

Verified locally: seinetwork + planner unit tests, full internal/controller/seinetwork/envtest package ×3 consecutive, golangci-lint on the package (0 issues).

Not in scope: the SeiNode-level behavior when a replacement lands on a retained PVC (data PVC ... already exists and is not owned by SeiNode) — that is Spec 004 territory.

Link to Devin session: https://app.devin.ai/sessions/b2e0897e975b458598ae62a68176b7a3
Open in Devin Desktop: https://app.devin.ai/desktop/session/b2e0897e975b458598ae62a68176b7a3?variant=devin
Requested by: @bdchatham

…or is lost mid-plan

A SeiNode deleted while the ceremony plan is active could never come back:
reconcileSeiNodes defers creates under PlanInProgress, the ceremony's tasks
Get the founding set by name and retry forever, and needsGenesisPlan never
rebuilds while Status.Plan is set. reconcilePlan now abandons the active
plan when fewer incumbents than replicas remain (PlanInProgress=False and
GenesisCeremonyComplete=False, both ValidatorLost, plus a Warning event),
letting the child be recreated and the ceremony rebuilt over the whole set.

PLT-1242

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@cursor

cursor Bot commented Sep 10, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes genesis ceremony and plan abandonment logic, including destructive deletes of surviving founding nodes and divergent handling for adopted vs minted validators—core network bootstrap behavior.

Overview
Fixes a wedge where deleting a founding SeiNode during an active genesis ceremony left PlanInProgress set and the plan retrying forever against a missing child (creates are gated until the plan ends).

reconcilePlan now detects validatorLost (fewer IncumbentNodes than Replicas under an active plan) and runs abandonPlanForLostValidator: clear the plan, reopen node creation, emit ValidatorLost / FoundingSetTornDown events, and set status reasons ValidatorLost or AdoptedSet.

For minted founding children, surviving nodes are deleted so PVC markers reset and the whole set can be recreated with a fresh ceremony (avoids split genesis from stale sidecar markers). For adopted validators (older than the network), survivors are kept, GenesisCeremonyComplete latches True/AdoptedSet, and only the missing node is replaced.

populateIncumbentNodes no longer counts Terminating children, so loss is detected as soon as delete lands. setGenesisCeremonyCondition treats ValidatorLost like CeremonyFailed until a new plan starts.

Tests add unit coverage for both branches plus an envtest recovery scenario; the sidecar stub gains SetCompleteAfter to hold the plan open mid-ceremony.

Reviewed by Cursor Bugbot for commit 27910d0. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread internal/controller/seinetwork/plan.go

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request.


// Drive active plan.
if network.Status.Plan != nil && network.Status.Plan.Phase == seiv1alpha1.TaskPlanActive {
if validatorLost(network) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker — This fires on an active ceremony plan at any task index, but the ceremony is not restartable from the middle.

The sidecar gates both halves of genesis on markers in the node's data PVC: assemble-genesis returns immediately when .sei-sidecar-assemble-done exists (sidecar/tasks/assemble_genesis.go:132), and each node's configure-genesis returns immediately when .sei-sidecar-genesis-done exists (sidecar/tasks/genesis.go:93). The assembler is the lexicographically-first child (cmd/main.go:237-240), and children only reach Running after assemble has published the final genesis — so the whole collect-and-set-peers / await-nodes-running window (the long one) sits after the assemble marker is written.

With replicas=2 and -1 lost during await-nodes-running: the plan is abandoned, -1 is recreated with a fresh identity, the ceremony is rebuilt, and its assemble-genesis hits the marker on the surviving -0 and does nothing. S3 still holds the genesis minted from the dead -1's gentx. The replacement downloads it, marks ready, goes Running; await-nodes-running only checks PhaseRunning, so the plan Completes and GenesisCeremonyComplete latches True. The genesis validator set now contains a consensus key nobody holds — 50% voting power, never >=2/3 — so the chain never produces a block while the SeiNetwork reports healthy. Nothing catches it: the controller consumes no AssembleGenesisResult/genesisHash.

Mirror case if the lost node is the assembler: the fresh -0 reassembles a new genesis while the surviving -1 keeps the old one behind its own .sei-sidecar-genesis-done.

Today's behaviour is a wedge an operator can see; this trades it for a network that claims to be fine and can never produce a block. Scope the guard to the window where restarting is sound (before assemble-genesis completes), or make the rebuild invalidate the survivors' assemble/genesis markers so every founding node re-derives from the new set.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and fixed in ee9e9d4 by taking the second option: the rebuild invalidates the survivors' markers by deleting the survivors. abandonPlanForLostValidator now deletes every remaining founding child alongside clearing the plan; the SeiNode finalizer removes each data PVC (and with it .sei-sidecar-assemble-done / .sei-sidecar-genesis-done), the set is recreated from scratch, and the rebuilt ceremony assembles genesis from the new identities — at any task index, both when the lost node is the assembler and when it is not. Nothing of the chain exists before GenesisCeremonyComplete, so an empty restart is the only convergent outcome.

Also from this: populateIncumbentNodes no longer counts a Terminating child as an incumbent — otherwise the ceremony could be rebuilt over a node still held by its finalizer, which the network then had to abandon again.

On the non-blockers: validatorLost is intentionally scoped by the reconcilePlan structure (only ceremony plans exist at network level); I'll leave the GenesisCeremonyComplete != True assertion and the abandoned-plan ID/task-index in the event message for a follow-up unless you want them here.

Coverage note: the envtest still holds the plan open at assemble-genesis (the stub sidecar has no per-task delay), but the recovery path is now identical regardless of which task was current, and the unit test asserts the survivor deletes.

seidroid[bot]
seidroid Bot previously requested changes Sep 10, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review found something blocking. The findings are on the lines they are about, and the summary is in this tool's comment on this pull request.

@seidroid

seidroid Bot commented Sep 10, 2026

Copy link
Copy Markdown

1. Blocking

internal/controller/seinetwork/plan.go:28 — abandon-and-rebuild is only sound while the ceremony is still in its first task; after assemble-genesis it silently produces a dead chain.

The guard fires on any active ceremony plan regardless of which task is current, but the ceremony is not re-runnable from the middle. Two sidecar-side idempotency markers make the rebuilt ceremony a partial no-op:

  • sidecar/tasks/assemble_genesis.go:132assemble-genesis returns immediately if .sei-sidecar-assemble-done exists in the assembler's SEI_HOME (the data PVC).
  • sidecar/tasks/genesis.go:93 — each node's configure-genesis returns immediately if .sei-sidecar-genesis-done exists.

The assembler is the lexicographically-first child (cmd/main.go:237-240), and children only reach Running after assemble has published the final genesis — so the entire collect-and-set-peers / await-nodes-running window (the long one; await retries up to groupAssemblyMaxRetries = 180) sits after the marker is written.

Concretely, with replicas: 2 and -1 lost during await-nodes-running: the plan is abandoned, -1 is recreated with a fresh identity, the ceremony is rebuilt, and its assemble-genesis hits the marker on the surviving -0 and does nothing. S3 still holds the genesis minted from the dead -1's gentx. The replacement -1 downloads that genesis, marks ready, goes Running; await-nodes-running only checks PhaseRunning, so the plan Completes and GenesisCeremonyComplete latches True. The genesis validator set now contains a consensus key nobody holds — 50% voting power, never ≥2/3 — so the chain never produces a block while the SeiNetwork reports healthy. The controller consumes no AssembleGenesisResult/genesisHash today, so nothing catches the stale assembly.

The mirror case is no better: if the lost node is the assembler, the fresh -0 reassembles a new genesis while the surviving -1 keeps the old one behind its own .sei-sidecar-genesis-done.

Today's behaviour is a wedged network that an operator can see. This trades it for a network that says it is fine and can never produce a block. The guard needs to be scoped to the window where restarting is actually sound (before assemble-genesis completes), or the rebuild has to invalidate the survivors' genesis/assemble markers so every founding node re-derives from the new set.

Missing coverage for the window where the recovery is unsound. TestGenesisCeremony_ValidatorLostMidCeremony_Recovers holds the first sidecar task open with SetCompleteAfter(5 * time.Second) and deletes the child there, so it only exercises loss during assemble-genesis — the one window where restarting is safe. TestReconcilePlan_ValidatorLostMidCeremony_AbandonsPlan uses a synthetic plan with no task state at all. Nothing covers loss after assemble completes, which is both the longer window and the one that breaks.

2. Non-blocking

  • validatorLost (plan.go:119) is a bare len(IncumbentNodes) < Spec.Replicas over any active network plan; "this is the ceremony" lives only in the doc comment. It holds today — planner.ForGroup (internal/planner/planner.go:95) builds only the genesis planner and spec.replicas is CEL-immutable — but it will misfire silently the day a second GroupPlanner is added. Asserting the ceremony directly (e.g. GenesisCeremonyComplete != True) would make the guard carry its own precondition. (Raised by codex, in a stronger form that does not hold; see summary.)
  • abandonPlanForLostValidator drops Status.Plan without recording the abandoned plan's ID or how far it got; the event message carries only the incumbent/replica counts. Given the blocker above, "which task was current" is exactly the fact that decides whether the restart was recoverable, and it is the one thing the operator loses.
  • Nit, envtest/stubs.go:85 — the SetCompleteAfter comment says the delay applies "for tasks submitted from now on", but GetTask reads s.CompleteAfter at poll time and compares it against each record's submittedAt, so a change applies retroactively to already-submitted tasks; the cleanup reset to 0 instantly completes anything still pending.

3. Summary

The diagnosis in the PR body is right and the mechanism — abandon the plan so the PlanInProgress gate reopens, keep the ceremony incomplete so the planner rebuilds it — is the right shape for a loss during assemble-genesis. But it is applied to the whole plan, and the ceremony is not restartable past its first task: the sidecar's assemble and configure-genesis markers make the rebuilt ceremony skip the work that would actually incorporate the replacement's fresh identity, converting a visible wedge into a network that reports GenesisCeremonyComplete=True over a genesis whose validator set includes a key nobody holds. Both new tests land inside the one window where the recovery is sound, so this is not visible from the suite. Of the prior readings, codex contributed one finding: that validatorLost applies to every active plan and so could abandon "a later rollout/maintenance plan". I dropped the claim as stated — ForGroup builds no deployment planner (internal/planner/planner.go:95 documents this explicitly) and spec.replicas is CEL-immutable, so there is no other network-level plan and no way for the count to drift legitimately — but kept its underlying worry, that the guard does not assert the precondition it relies on, as a non-blocker credited to it.

seidroid review · decision request_changes · session ce752d5077234f3cad86e73ba8fe0e07 · turn resp_claude_57f31f0d363d6335b49cb7d8b06ede43 · item 2e2fa03b671d5420b2feedd0c3167b67

Findings: 2 blocking | 3 non-blocking | 1 posted inline | 1 pre-existing

…r is lost mid-ceremony

Abandoning the plan alone left survivors on the genesis assembled from the
lost node's gentx: the sidecar's assemble and configure-genesis markers make
the rebuilt ceremony skip that work, so the set would split across two
genesis hashes (or latch complete over a validator key nobody holds).
Deleting every remaining founding child recreates the set with fresh PVCs
and identities, and the rebuilt ceremony derives genesis from the new set.

Terminating children are no longer counted as incumbents, so the loss is
detected as soon as the delete lands and no ceremony is built over a node
still held by its finalizer.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request.


// Drive active plan.
if network.Status.Plan != nil && network.Status.Plan.Phase == seiv1alpha1.TaskPlanActive {
if validatorLost(network) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker — Tearing down the whole set is the right call for the case this PR targets, and it does close the stale-genesis hole: handleNodeDeletion calls deleteNodeDataPVC (internal/controller/node/controller.go:446), so the survivors lose .sei-sidecar-assemble-done / .sei-sidecar-genesis-done and every founding node re-derives from the new set.

What changed is the blast radius. validatorLost is still len(Status.IncumbentNodes) < Spec.Replicas under any active plan, and its justification — "Nothing of the chain exists yet, so restarting from an empty set is the only convergent outcome" — is asserted in a comment and never checked. It is false in a flow this repo supports:

  1. Retain teardown: orphanChildSeiNodes strips the owner ref, children keep running with their data PVCs.
  2. The operator recreates the same-named SeiNetwork over them — ensureSeiNode's adoption branch exists for exactly this.
  3. The new network's status is empty, so GenesisCeremonyComplete seeds False/NotStarted; once the children are adopted, needsGenesisPlan builds a genesis ceremony plan over established, block-producing validators.
  4. Any incumbent shortfall while that plan is active — the event class this PR exists to handle — now deletes every remaining child, and each SeiNode finalizer deletes its data PVC.

That destroys "a ceremony-generated consensus identity that cannot be regenerated" — this controller's own words, in the handleDeletion comment that biases the unknown-policy branch toward retaining precisely because the two outcomes are not symmetric. This adds a path that cascades without the operator choosing it. The window in step 4 is short (markers make assemble/configure no-ops and the children are already Running), but a short window is not a guard for an irreversible fleet-wide delete.

Before deleting the survivors, assert positively that they are children this ceremony minted and that hold no chain state — e.g. record the founding set on the plan at startPlan and tear down only nodes in that set, rather than everything listChildSeiNodes returns. Where the assertion does not hold, abandon the plan and surface ValidatorLost without deleting. Note the retain annotations cannot serve as that signal: ensureSeiNode replaces existing.Annotations wholesale with seiNodeAnnotations(network) on the adopting pass, so they are gone by the time this guard runs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the Retain-then-recreate adoption flow does put a ceremony plan over established validators, and the count alone did not distinguish it. Fixed in ee10989.

The positive signal is age: a child this network minted was created after the network (ensureSeiNode runs only once the network exists), while an adopted child predates it. abandonPlanForLostValidator now requires every survivor to satisfy !node.CreationTimestamp.Before(&network.CreationTimestamp) before deleting anything. If any survivor predates the network the set is treated as adopted: the plan is abandoned and ValidatorLost surfaced (message says the adopted validators are kept), nothing is deleted, and the gate reopens so the lost node is recreated and joins the existing chain via the retained genesis. I went with the timestamp rather than recording the founding set on the plan because the adopted names are the founding names — the plan would have listed them too — and it needs no API change.

Unit coverage: TestReconcilePlan_ValidatorLostOverAdoptedSet_KeepsSurvivors (adopted survivors intact, plan cleared, condition set) alongside the existing minted-set teardown test.

Non-blockers picked up in the same commit: a distinct Warning/FoundingSetTornDown event per deleted survivor instead of the scale-down SeiNodeDeleted; the abandoned plan ID in the condition/event message; survivorsDeleted now counts actual deletes; the SetCompleteAfter comment now states the delay is read at poll time and applies retroactively; and the envtest comment states it proves recreation, not marker-clearing (that rests on the SeiNode finalizer's deleteNodeDataPVC, covered by TestNodeDeletion_SnapshotNode_WithoutRetain_DeletesPVC). Task index of the abandoned plan is not recorded in status — the log line and the plan ID are the pointer; happy to add it if you want it in the condition message.

@seidroid

seidroid Bot commented Sep 10, 2026

Copy link
Copy Markdown

1. Blocking

internal/controller/seinetwork/plan.go:29 — the recovery now deletes every child SeiNode and its data PVC, but the trigger is still a bare count that never asserts the set holds no chain state.

The teardown is the right call for the case the PR is written for, and it does close the stale-genesis hole from the previous revision: handleNodeDeletion explicitly calls deleteNodeDataPVC (internal/controller/node/controller.go:446), so the survivors lose the .sei-sidecar-assemble-done / .sei-sidecar-genesis-done markers and every founding node genuinely re-derives from the new set.

What changed is the blast radius. validatorLost is still len(Status.IncumbentNodes) < Spec.Replicas under any active plan, and the justification for that — "Nothing of the chain exists yet, so restarting from an empty set is the only convergent outcome" — is an assumption the code states in a comment and never checks. It is false in a flow this repository documents and supports:

  1. Retain teardown: orphanChildSeiNodes strips the owner ref and leaves the children running with their data PVCs.
  2. The operator recreates the same-named SeiNetwork on top of them — ensureSeiNode's adoption branch exists specifically for this ("a Retain teardown orphans children deliberately, and the next run recreates the same-named SeiNetwork on top of them").
  3. The new network's status is empty, so GenesisCeremonyComplete seeds False/NotStarted; once the children are adopted, IncumbentNodes is full and needsGenesisPlan builds a genesis ceremony plan over established, block-producing validators.
  4. Any incumbent shortfall while that plan is active — the exact event class this PR exists to handle — now deletes every remaining child, and each SeiNode finalizer deletes its data PVC.

That destroys "a ceremony-generated consensus identity that cannot be regenerated" — this controller's own words, in the handleDeletion comment that deliberately biases the unknown-policy branch toward retaining rather than cascading, precisely because the two outcomes are not symmetric. This change adds a path that cascades without the operator ever choosing it.

The window in step 4 is short (the markers make assemble-genesis and configure-genesis no-ops and the children are already Running, so the re-ceremony completes in a few laps), but a short window is not a guard for an irreversible fleet-wide delete.

Before deleting the survivors, the code needs a positive assertion that they are children this ceremony minted and that hold no chain state — for example, recording the founding set on the plan at startPlan and tearing down only nodes in that set that this network created, rather than everything listChildSeiNodes returns. Where the assertion does not hold, abandon the plan and surface ValidatorLost without deleting, and let a human decide. Note that the retain annotations are not usable as that signal: ensureSeiNode replaces existing.Annotations wholesale with seiNodeAnnotations(network) on the adopting pass, so they are gone by the time the guard runs.

2. Non-blocking

  • The envtest cannot demonstrate the mechanism the fix rests on: it strips the SeiNode finalizers and hand-deletes both data PVCs up front, so the controller's deletes never run deleteNodeDataPVC and the marker-clearing is stubbed out in both directions. It proves the set is recreated with fresh UIDs, not that a real cluster's teardown clears the markers. That is an envtest limitation (no GC), not a defect — worth saying in the test comment so the next reader does not over-trust it.
  • Each survivor's deletion is recorded as Normal/SeiNodeDeleted, the same reason scaleDown uses for routine excess-replica removal. CLAUDE.md treats condition/event reasons as the runbook and alerting API; an irreversible teardown of the whole founding set reading identically to a scale-down costs an operator the one signal that would distinguish them. The Warning/ValidatorLost event does carry the story, which is why this is not blocking.
  • abandonPlanForLostValidator drops Status.Plan without recording the abandoned plan's ID or which task was current. Given the action is now destructive, "what was torn down and how far had it got" is the first thing an operator will want and the one thing the status no longer holds.
  • Nit, plan.go:154 — the survivorsDeleted log field is len(survivors), the length of the listed set, which counts children the loop skipped as already terminating. An operator-facing count of an irreversible action that can overstate what it did.
  • Nit, envtest/stubs.go:85 (unchanged in this revision) — the SetCompleteAfter comment says the delay applies "for tasks submitted from now on", but GetTask reads s.CompleteAfter at poll time and compares it against each record's submittedAt, so a change applies retroactively to already-submitted tasks; the cleanup reset to 0 instantly completes anything still pending.

3. Summary

The change moved substantially: abandonPlanForLostValidator now tears down the whole founding set rather than only clearing the plan, and populateIncumbentNodes excludes terminating children so a loss is detected when the delete lands rather than when the finalizer releases. Both blocking findings from my earlier review are addressed — the teardown wipes the sidecar's assemble/genesis markers with the PVCs, so the rebuilt ceremony can no longer complete over a stale genesis, and that also removes the distinct post-assemble-genesis failure window the tests did not cover. What the revision introduces is a new one: the trigger for an irreversible, fleet-wide delete is still an unqualified count, and the Retain-teardown-then-recreate flow the controller explicitly supports puts a genesis ceremony plan over established validators, where a shortfall now destroys consensus identities this repository says cannot be regenerated. Of the prior readings, codex contributed one finding, and I kept its conclusion while correcting its mechanism: there is still no "later rollout/maintenance plan" to misfire on — planner.ForGroup builds only the genesis planner and spec.replicas remains CEL-immutable — but its claim that this can delete the entire surviving node set and cause an established-network outage with data loss does now hold, by the adoption route above, and it is credited in the blocker.

seidroid review · decision request_changes · session ce752d5077234f3cad86e73ba8fe0e07 · turn resp_claude_4d0f5f18ba57762a9471e8fcc7e2c0bc · item 164d97da209b57c8b020f6ba5572cc97

Findings: 1 blocking | 5 non-blocking | 1 posted inline | 1 pre-existing

seidroid[bot]
seidroid Bot previously requested changes Sep 10, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review found something blocking. The findings are on the lines they are about, and the summary is in this tool's comment on this pull request.

…-ceremony loss

The whole-set teardown assumed every survivor was minted by this ceremony. A
same-named SeiNetwork recreated over children a Retain teardown released
adopts them and runs a marker-no-op ceremony over established validators; a
loss under that plan must not cascade into deleting identities that cannot be
regenerated. Survivors that predate the network are adopted: the plan is
abandoned and ValidatorLost surfaced, but nothing is deleted.

Also: a distinct Warning/FoundingSetTornDown event per deleted survivor, the
abandoned plan ID in the condition/event message, and an accurate
survivorsDeleted log count.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread internal/controller/seinetwork/plan.go

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request.

Comment thread internal/controller/seinetwork/plan.go Outdated
msg = fmt.Sprintf("%d of %d founding validators present; plan %s abandoned, the set is torn down and the genesis ceremony restarts once it is recreated",
len(network.Status.IncumbentNodes), network.Spec.Replicas, network.Status.Plan.ID)
} else {
msg = fmt.Sprintf("%d of %d validators present; plan %s abandoned, adopted validators are kept and the missing node is recreated",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker — The guard is the right instinct and the comment states the right intent — "the plan is abandoned, the loss is surfaced, and the lost node is recreated to join the existing chain." The code does the first two, then does more than the third, because both branches fall through to the same two lines: GenesisCeremonyComplete = False/ValidatorLost and Status.Plan = nil. That is exactly what needsGenesisPlan (internal/planner/planner.go) reads as "build the ceremony again" — it returns false only for nil-plan-plus-True, and False/ValidatorLost is not True.

On an adopted (Retain-teardown-then-recreated) set the sequence continues past the abandon:

  1. PlanInProgress drops, so reconcileSeiNodes recreates the lost child — with a fresh data PVC, its predecessor's having been removed by the SeiNode finalizer.
  2. That child's plan is buildGenesisPlan, so it mints a new identity and upload-genesis-artifacts writes a new gentx to S3 under the same deterministic node name, replacing the dead validator's.
  3. Once all replicas exist the ceremony is rebuilt over the mixed set. The assembler is the lexicographically-first child (cmd/main.go sorts by name, takes [0]).
    • Lost node was ordinal 0: the replacement is the assembler and has no .sei-sidecar-assemble-done, so assemble-genesis runs for real — pulls every gentx, assembles a different genesis, and uploads it back to S3 over the live chain's published genesis.json. The adopted survivors are marker-guarded on configure-genesis and keep the original, so the set splits across two genesis hashes and the canonical artifact misdirects any future bootstrap.
    • Any other ordinal: the assembler is an adopted survivor, assemble no-ops, and the replacement installs the existing genesis, which does not contain its key — the chain permanently loses that slot's voting power.
  4. await-nodes-running only checks PhaseRunning, so the plan completes and GenesisCeremonyComplete latches True: the network reports the ceremony succeeded either way.

None of this was reachable before — the loss under an active plan wedged the plan, so no ceremony was ever rebuilt over an established set containing a marker-less member. Abandoning the plan is what opens it.

In the adopted case the ceremony must not be rebuilt at all: make needsGenesisPlan false (latch GenesisCeremonyComplete=True under a distinct reason, or gate the rebuild explicitly) so only the plain replacement path runs — what this function's own comment describes. Whether a replacement can meaningfully rejoin a minted validator set is a separate question the PR scopes out, and that is fine; re-running assembly over a live chain is not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right — the adopted branch abandoned the plan but left the rebuild open. Fixed in 0deac0f: the adopted branch now latches GenesisCeremonyComplete=True/AdoptedSet (new stable reason) instead of False/ValidatorLost, so needsGenesisPlan stays false and only the plain replacement path runs; PlanInProgress still drops with ValidatorLost. setGenesisCeremonyCondition's True-latch leaves it alone (asserted in TestReconcilePlan_ValidatorLostOverAdoptedSet_KeepsSurvivors, which now also checks Status=True/AdoptedSet).

Non-blockers: both unit tests now use real, well-separated timestamps (network an hour old, minted children a minute after it) so they no longer pin "equal ⇒ tear down"; the envtest comment cites TestNodeDeletion_SnapshotNode_WithoutRetain_DeletesPVC by name — it asserts the PVC is gone after SeiNode delete, which is deleteNodeDataPVC's effect even if the function isn't referenced literally; and the condition message now reports the actual number of survivors deleted, so a zero-survivor loss reads as such. I kept the timestamp proxy over recording the founding set on the plan: the adopted names are the founding names, so a name list would not separate the two cases, and it fails safe (any single older survivor selects the non-destructive branch).

seidroid[bot]
seidroid Bot previously requested changes Sep 10, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review found something blocking. The findings are on the lines they are about, and the summary is in this tool's comment on this pull request.

@seidroid

seidroid Bot commented Sep 10, 2026

Copy link
Copy Markdown

1. Blocking

internal/controller/seinetwork/plan.go:181 — the adopted branch correctly refuses to delete, but still clears the way for a genesis ceremony to be rebuilt and re-run over an established chain.

The guard itself is the right instinct and the comment states the right intent: "the plan is abandoned, the loss is surfaced, and the lost node is recreated to join the existing chain." The code does the first two and then does considerably more than the third, because both branches land on the same two lines — GenesisCeremonyComplete = False/ValidatorLost and Status.Plan = nil. That combination is exactly what needsGenesisPlan (internal/planner/planner.go) reads as "build the ceremony again": it returns false only for a nil-plan-plus-True condition, and False/ValidatorLost is not True.

So on an adopted (Retain-teardown-then-recreated) set the sequence continues past the abandon:

  1. PlanInProgress drops, so reconcileSeiNodes recreates the lost child — with a fresh data PVC, since its predecessor's was removed by the SeiNode finalizer.
  2. That child's own plan is buildGenesisPlan, so it mints a new identity and upload-genesis-artifacts writes a new gentx to S3 under the same deterministic node name, replacing the dead validator's.
  3. Once all replicas exist, the network rebuilds the ceremony over the mixed set. The assembler is the lexicographically-first child (cmd/main.go sorts by name and takes [0]).
    • If the lost node was ordinal 0, the replacement is the assembler and has no .sei-sidecar-assemble-done, so assemble-genesis runs for real: it pulls every node's gentx, assembles a different genesis, and uploads it back to S3, overwriting the live chain's published genesis.json. The adopted survivors are marker-guarded on configure-genesis and keep the original, so the set splits across two genesis hashes and the canonical artifact now misdirects any future bootstrap.
    • For any other ordinal, the assembler is an adopted survivor and assemble no-ops, so the replacement installs the existing genesis — which does not contain its key. The chain permanently loses that validator slot's voting power.
  4. await-nodes-running only checks PhaseRunning, so the plan completes and GenesisCeremonyComplete latches True either way: the network reports the ceremony succeeded.

None of this was reachable before this change — the loss under an active plan wedged the plan, so no ceremony was ever rebuilt over an established set containing a marker-less member. Abandoning the plan is what opens it.

For the adopted case the ceremony must not be rebuilt at all: make needsGenesisPlan false (latch GenesisCeremonyComplete=True under a distinct reason, or gate the rebuild explicitly) so only the plain replacement path runs, which is what the function's own comment describes. Whether a replacement can meaningfully rejoin a minted validator set is a separate question the PR scopes out, and that is fine — re-running assembly over a live chain is not.

2. Non-blocking

  • The minted/adopted split rests on survivors[i].CreationTimestamp.Before(&network.CreationTimestamp). metav1.Time is second-granular once round-tripped through the API server, so the predicate has a one-second ambiguity band in which "not before" is read as minted — the destructive branch. It fails safe in the realistic direction (any one older survivor short-circuits to adopted, and retained children predate a recreated network by a network lifetime), which is why this is not blocking, but it is a proxy rather than a provenance record, and TestReconcilePlan_ValidatorLostMidCeremony_AbandonsPlan builds both objects with zero timestamps, so what it actually pins is "equal timestamps ⇒ tear down". Recording the founding set on the plan at startPlan and tearing down only those names would be exact; note seiNodeAnnotations returns nil today, so there is no existing marker to lean on. (Raised by codex, in a stronger form — see summary.)
  • The new envtest comment says the PVC teardown "rests on the SeiNode finalizer deleting the data PVC, which is exercised by the node controller's own tests." deleteNodeDataPVC has no test reference anywhere in the repository — it appears only in internal/controller/node/controller.go. The comment is now the thing a future reader will trust about that coverage.
  • minted initialises to true and an empty survivors slice never falsifies it, so a loss that takes every child at once reports "the set is torn down" with deleted at 0.

3. Summary

The revision adds an adoption guard: abandonPlanForLostValidator now deletes the survivors only when every one of them postdates the network, and otherwise keeps them, with a dedicated unit test for the adopted case. That addresses the blocker I raised last round — retained consensus identities are no longer cascaded — and every non-blocking point from that review is addressed too: the teardown gets its own Warning/FoundingSetTornDown event reason distinct from the scale-down SeiNodeDeleted, the plan ID is now in the condition message and the per-node events, survivorsDeleted counts actual deletes, the envtest comment records what its GC stand-in does and does not prove, and the SetCompleteAfter doc comment is corrected. What is new is that the adopted branch stops at abandoning the plan: because it leaves GenesisCeremonyComplete=False, the ceremony is rebuilt and re-run over the established set, and where the lost node is the assembler that re-uploads a divergent genesis over the live chain's. Of the prior readings, codex contributed one finding, and I kept half of it: its conclusion that the adoption test needs an explicit provenance signal rather than a timestamp comparison is fair and is recorded as a non-blocker, but its stated failure — a retained validator sharing a creation second with the recreated network — I dropped, since that requires the original creation, the Retain teardown and the recreate to all land inside one wall-clock second, and any single older survivor short-circuits the check to the safe branch regardless.

seidroid review · decision request_changes · session ce752d5077234f3cad86e73ba8fe0e07 · turn resp_claude_bc8d1f968d6a60d98703f924b61b37dd · item 35288866579a5ab1999cb9e4379f94b5

Findings: 1 blocking | 3 non-blocking | 1 posted inline | 1 pre-existing

…d of rebuilding over an adopted set

Abandoning the plan with GenesisCeremonyComplete=False let needsGenesisPlan
rebuild the ceremony over adopted validators plus a marker-less replacement,
which could reassemble and republish genesis over the live chain's. The
adopted branch now latches the condition True under AdoptedSet so only the
plain replacement path runs.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0deac0f. Configure here.

Comment thread internal/controller/seinetwork/plan.go

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request.

Comment thread internal/controller/seinetwork/plan.go
@seidroid
seidroid Bot dismissed stale reviews from themself September 11, 2026 00:12

Superseded: the latest review found nothing blocking in this change.

@seidroid

seidroid Bot commented Sep 11, 2026

Copy link
Copy Markdown

1. Blocking

Nothing.

2. Non-blocking

  • internal/controller/seinetwork/plan.go:158 — the minted/adopted split rests on survivors[i].CreationTimestamp.Before(&network.CreationTimestamp), and metav1.Time is second-granular once round-tripped through the API server. Before is strict, and minted initialises to true, so an equal-second tie resolves to the destructive branch. The reachable exposure is degenerate — a retained survivor only collides if the previous ceremony's child creation, the Retain teardown and the recreate all land in one wall-clock second, and a single strictly-older survivor short-circuits the loop to the safe branch — which is why this is not blocking. Worth saying explicitly for whoever revisits it: the boundary cannot simply be moved. Minted children are created by the first reconcile after the network, routinely in the same second, so treating equal as adopted would misclassify the common minted path and silently restore the original wedge. The signal is ambiguous at exactly the resolution where the two cases overlap; the exact fix is provenance, e.g. recording the founding set on the plan at startPlan and deleting only those names. (Raised by codex, in a stronger form — see summary.)
  • The adopted branch's condition message says "the missing node is recreated", and the replacement does come back through ensureSeiNode — but it mints a fresh consensus identity and the live chain's genesis already names the old key, so it rejoins without a validator slot and the set is permanently short that voting power. That is the pre-existing limitation the PR scopes out as Spec 004 territory, not a defect here; it is worth naming in the message because True/AdoptedSet plus NodesReady=AllNodesReady is what a runbook will read, and neither says the set came back degraded.

3. Summary

The adopted branch now latches GenesisCeremonyComplete=True/AdoptedSet rather than False/ValidatorLost, which is what closes my last blocker: needsGenesisPlan reads the True and never rebuilds a ceremony over an adopted set, so the path where a marker-less replacement becomes the assembler and republishes a divergent genesis over the live chain's is gone, with TestReconcilePlan_ValidatorLostOverAdoptedSet_KeepsSurvivors asserting both the kept survivors and the latch. Every earlier finding is now addressed across the series — the whole-set teardown with PVC-backed marker clearing, the adoption guard that keeps retained identities, the distinct Warning/FoundingSetTornDown reason, the plan ID in the condition and events, the accurate deleted count, the corrected SetCompleteAfter comment, the minted test no longer resting on zero timestamps, and the envtest comment now citing a node-controller test that does exist (internal/controller/node/reconciler_test.go:357). What remains is a design note about the timestamp proxy and an expectation-setting line in the adopted message, neither of which blocks. Of the prior readings, codex contributed one finding, and I kept the half that holds: its point that adoption needs explicit provenance rather than a timestamp comparison is right and is recorded as a non-blocker on the line, but its stated failure — retained children sharing a creation second with a recreated network — needs that whole cycle inside one second and fails safe on any older survivor, and its implied remedy of moving the boundary would break the common minted case, so I did not carry it as blocking.

seidroid review · decision approve · session ce752d5077234f3cad86e73ba8fe0e07 · turn resp_claude_be1e39458335fdcbb66d0ef70966f086 · item 60c200f022405078b9956b48c105fdcc

Findings: 0 blocking | 2 non-blocking | 1 posted inline | 2 pre-existing

… degraded adopted-set outcome

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@bdchatham
bdchatham merged commit 7da9946 into main Sep 11, 2026
17 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant