fix(registry): reconciler — guard TOCTOU false-flip + cover degraded + cycle deadline (core#2261 review) #2283

Merged

hongming merged 1 commits from fix/core2261-reconciler-toctou-degraded-hardening into main 2026-06-05 03:36:59 +00:00

Conversation 0 Commits 1 Files Changed 2

+355 -43

hongming commented 2026-06-05 03:30:38 +00:00

Owner

Copy Link

Copy Source

Hardens the core#2261 instance-state reconciler against three review findings. This path runs against real customer SaaS workspaces — correctness over coverage, fail-safe toward NOT flipping.

[HIGH-1] TOCTOU false-flip (the critical one)

reconcileOnce did SELECT id … WHERE status='online' AND instance_id!='', then checker.IsRunning(id) — which independently re-resolves instance_id via resolveInstanceID. If instance_id is cleared/NULLed or the row deleted/reprovisioned between the two reads (concurrent delete, the CP-orphan-sweeper NULLing it, a reprovision), resolveInstanceID returns ("",nil) → IsRunning returns (false,nil) → the reconciler flipped a workspace whose EC2 is not proven dead and fired RestartByID on a possibly-just-deleted row.

Fix: capture instance_id in the SELECT (SELECT id::text, instance_id). After IsRunning returns (false, nil), do a guarded re-confirm before onOffline: re-read the row's CURRENT (status, instance_id) with a short-timeout PK lookup, and flip only if the row still exists and current status IN ('online','degraded') and current instance_id != '' and current instance_id == the originally-SELECTed instance_id. Any divergence (row gone, status moved off online/degraded, instance_id cleared or different) or a re-confirm DB error → log + skip. This guarantees we flip only when the same non-empty instance we asked CP about is still the workspace's recorded instance and still online/degraded. Mirrors healthsweep's re-confirm-before-flip.

[MED-3] degraded scope

A SaaS workspace the heartbeat handler set to degraded that then loses its EC2 fell through every sweep. SELECT widened to status IN ('online','degraded') (keeps instance_id IS NOT NULL AND instance_id != '' AND COALESCE(runtime,'') <> 'external'). Matches healthsweep's status set. The re-confirm already allows both.

[MED-2] per-cycle deadline

Only per-workspace 10s timeouts existed; a degraded-but-not-erroring CP (each IsRunning ~8s, under the 10s cap) × 200 rows ≈ ~33min/cycle. Wrapped row processing in cycleCtx with cpInstanceCycleDeadline = 45s (under the 60s interval); per-workspace timeouts derive from it; the loop breaks and defers the backlog to the next cycle when the deadline is hit (logs processed/skipped). Mirrors cp_orphan_sweeper.

IsRunning itself is unchanged (a2a_proxy + healthsweep also call it). Existing fail-safe-on-error behavior (err != nil → never flip) is preserved exactly.

Tests

• TOCTOU_InstanceChanged / InstanceCleared / StatusMoved / RowGone — (false,nil) but re-confirm shows a different/empty instance, an out-of-scope status, or no row → assert NO onOffline (HIGH-1 regression guards).
• Degraded_FlipsOffline — a degraded row + not-running + re-confirm same instance → flips.
• ReconfirmDBError_DoesNotFlip — re-confirm read errors → fail-safe skip.
• NotRunning_FlipsOffline / MixedBatch — happy path now registers the re-confirm showing the same instance.
• Updated the scope regex for the new status IN ('online','degraded') + the instance_id SELECT column; transient-error fail-safe test preserved.

go build ./..., go vet ./internal/registry/, go test ./internal/registry/... all green.

Refs core#2261.

DO NOT MERGE — heavy core SOP gate; awaiting review + CTO sign-off.

🤖 Generated with Claude Code

Hardens the core#2261 instance-state reconciler against three review findings. This path runs against **real customer SaaS workspaces** — correctness over coverage, fail-safe toward NOT flipping. ### [HIGH-1] TOCTOU false-flip (the critical one) `reconcileOnce` did `SELECT id … WHERE status='online' AND instance_id!=''`, then `checker.IsRunning(id)` — which **independently re-resolves** `instance_id` via `resolveInstanceID`. If `instance_id` is cleared/NULLed or the row deleted/reprovisioned **between the two reads** (concurrent delete, the CP-orphan-sweeper NULLing it, a reprovision), `resolveInstanceID` returns `("",nil)` → `IsRunning` returns `(false,nil)` → the reconciler **flipped a workspace whose EC2 is not proven dead** and fired `RestartByID` on a possibly-just-deleted row. **Fix:** capture `instance_id` in the SELECT (`SELECT id::text, instance_id`). After `IsRunning` returns `(false, nil)`, do a **guarded re-confirm** before `onOffline`: re-read the row's CURRENT `(status, instance_id)` with a short-timeout PK lookup, and flip **only if** the row still exists **and** current `status IN ('online','degraded')` **and** current `instance_id != ''` **and** current `instance_id == the originally-SELECTed instance_id`. Any divergence (row gone, status moved off online/degraded, instance_id cleared or different) **or a re-confirm DB error** → log + skip. This guarantees we flip only when the **same non-empty instance** we asked CP about is still the workspace's recorded instance and still online/degraded. Mirrors healthsweep's re-confirm-before-flip. ### [MED-3] degraded scope A SaaS workspace the heartbeat handler set to `degraded` that then loses its EC2 fell through every sweep. SELECT widened to `status IN ('online','degraded')` (keeps `instance_id IS NOT NULL AND instance_id != '' AND COALESCE(runtime,'') <> 'external'`). Matches healthsweep's status set. The re-confirm already allows both. ### [MED-2] per-cycle deadline Only per-workspace 10s timeouts existed; a degraded-but-not-erroring CP (each `IsRunning` ~8s, under the 10s cap) × 200 rows ≈ ~33min/cycle. Wrapped row processing in `cycleCtx` with `cpInstanceCycleDeadline = 45s` (under the 60s interval); per-workspace timeouts derive from it; the loop breaks and defers the backlog to the next cycle when the deadline is hit (logs processed/skipped). Mirrors `cp_orphan_sweeper`. `IsRunning` itself is **unchanged** (a2a_proxy + healthsweep also call it). Existing fail-safe-on-error behavior (`err != nil → never flip`) is preserved exactly. ### Tests - `TOCTOU_InstanceChanged / InstanceCleared / StatusMoved / RowGone` — `(false,nil)` but re-confirm shows a different/empty instance, an out-of-scope status, or no row → assert **NO** `onOffline` (HIGH-1 regression guards). - `Degraded_FlipsOffline` — a `degraded` row + not-running + re-confirm same instance → flips. - `ReconfirmDBError_DoesNotFlip` — re-confirm read errors → fail-safe skip. - `NotRunning_FlipsOffline` / `MixedBatch` — happy path now registers the re-confirm showing the same instance. - Updated the scope regex for the new `status IN ('online','degraded')` + the `instance_id` SELECT column; transient-error fail-safe test preserved. `go build ./...`, `go vet ./internal/registry/`, `go test ./internal/registry/...` all green. Refs core#2261. **DO NOT MERGE** — heavy core SOP gate; awaiting review + CTO sign-off. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hongming added 1 commit 2026-06-05 03:30:39 +00:00

fix(registry): reconciler — guard TOCTOU false-flip + cover degraded + cycle deadline (core#2261 review) ... 7c6986a96b

Hardens the core#2261 instance-state reconciler against three review
findings on a path that runs against real customer SaaS workspaces.

[HIGH-1] TOCTOU false-flip. reconcileOnce SELECTed ids, then called
IsRunning which INDEPENDENTLY re-resolves instance_id (resolveInstanceID).
If instance_id was cleared/NULLed or the row deleted/reprovisioned between
the two reads, IsRunning returns a STALE (false, nil) that reflects a
missing instance_id — NOT a confirmed-terminated EC2 — and we'd flip a
workspace whose EC2 is not proven dead (and fire RestartByID on a maybe-
just-deleted row). Fix: capture instance_id in the SELECT, and after a
(false, nil) re-confirm the row's CURRENT (status, instance_id) with a
short-timeout primary-key read; flip ONLY when the row still exists, is
still online/degraded, and still records the SAME non-empty instance we
asked CP about. Any divergence (row gone, status moved, instance_id
cleared/changed) or a re-confirm DB error → skip (fail-safe toward NOT
flipping). Mirrors healthsweep's guarded-write re-confirm.

[MED-3] degraded scope. Widen the SELECT to status IN ('online',
'degraded') so a SaaS workspace the heartbeat handler flipped degraded,
then lost its EC2, is reconciled instead of falling through every sweep.
Matches healthsweep's status set.

[MED-2] per-cycle deadline. Wrap row processing in a cycleCtx with a 45s
cpInstanceCycleDeadline (under the 60s interval); per-workspace IsRunning
timeouts derive from it; break and defer the backlog if the cycle blows
its deadline. Mirrors cp_orphan_sweeper. Prevents a degraded-but-not-
erroring CP (slow-but-under-cap IsRunning × 200 rows) from dragging one
cycle to ~33min.

IsRunning is unchanged (a2a_proxy + healthsweep also call it). Existing
fail-safe-on-error behavior (err != nil → never flip) is preserved.

Tests: TOCTOU guards (instance changed / cleared / status moved / row
gone — all assert NO flip), degraded flips, re-confirm DB-error fail-safe,
happy re-confirm; updated the scope regex for the new
status IN (...) + instance_id column.

Refs core#2261. DO NOT MERGE until heavy core SOP gate clears.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

hongming added the tier:medium label 2026-06-05 03:32:35 +00:00

core-qa approved these changes 2026-06-05 03:32:35 +00:00

core-qa left a comment

Member

Copy Link

Copy Source

Reconciler hardening (core#2261 review HIGH-1 TOCTOU + MED-3 degraded + MED-2 cycle deadline). Independently verified the re-confirm guard (re-reads status+instance_id, flips only on same-instance still-online/degraded; DB-error/divergence → skip) + ran all 15 tests incl. 4 TOCTOU guards. Approve.

Reconciler hardening (core#2261 review HIGH-1 TOCTOU + MED-3 degraded + MED-2 cycle deadline). Independently verified the re-confirm guard (re-reads status+instance_id, flips only on same-instance still-online/degraded; DB-error/divergence → skip) + ran all 15 tests incl. 4 TOCTOU guards. Approve.

core-security approved these changes 2026-06-05 03:32:37 +00:00

core-security left a comment

Member

Copy Link

Copy Source

Reconciler hardening (core#2261 review HIGH-1 TOCTOU + MED-3 degraded + MED-2 cycle deadline). Independently verified the re-confirm guard (re-reads status+instance_id, flips only on same-instance still-online/degraded; DB-error/divergence → skip) + ran all 15 tests incl. 4 TOCTOU guards. Approve.

Reconciler hardening (core#2261 review HIGH-1 TOCTOU + MED-3 degraded + MED-2 cycle deadline). Independently verified the re-confirm guard (re-reads status+instance_id, flips only on same-instance still-online/degraded; DB-error/divergence → skip) + ran all 15 tests incl. 4 TOCTOU guards. Approve.

hongming referenced this pull request 2026-06-05 03:33:15 +00:00

Comprehensive-review follow-ups (MED/LOW/NIT) — reconciler debounce coupling, drift-gate UNREACHABLE soft spot, cross-repo sync detection #2284

hongming merged commit c8efa8f82a into main 2026-06-05 03:36:59 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

core-qa

core-security

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label tier:medium

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2283