fix(ws-server): close self-fire restart feedback loop (internal#544) #1556

Merged

hongming merged 1 commits from fix/ws-server-self-fire-restart-loop into main 2026-05-19 01:40:55 +00:00

Conversation 2 Commits 1 Files Changed 4

+464

hongming commented 2026-05-19 01:24:46 +00:00

Owner

Copy Link

Copy Source

Summary

Three-layer cohesive fix for the 2026-05-19 ~00:05-00:09Z 4x reprov thrash class observed on prod-Reviewer + prod-Researcher: a single secrets PUT fanned out into 4x stop+provision cycles per workspace within 4 min, each stopping the just-launched (still-pending) EC2 of the previous cycle. Root-caused via Loki (provision.ec2_started / ec2_stopped pairs).

Companion class-doc issue: internal#544 (empirical chain + reproduction recipe).

Empirical chain (verified against source)

All files in workspace-server/internal/handlers/:

1. secrets.go:264 — POST /secrets unconditionally fires go h.restartFunc(workspaceID).
2. workspace_restart.go:runRestartCycle — sets url='' synchronously, then async provisions EC2 (provisionWorkspaceCP returns immediately after cpProv.Start).
3. 20-30s pending window — workspaces.url='' AND cpProv.IsRunning()==false (looks identical to a dead container).
4. a2a_proxy_helpers.go — maybeMarkContainerDead OR preflightContainerHealth fires (from canvas /delegations poll or the trailing restart-context probe at the end of runRestartCycle), sees container-dead → calls RestartByID → loop.
5. workspace_restart.go:coalesceRestart — pending=true, drains by running ANOTHER full cycle → ec2_stopped of the just-booted instance → re-provision.

Fix — three interdependent layers

L1 — Restart-aware health probes (a2a_proxy_helpers.go)

• workspace_restart.go:isRestarting(workspaceID) bool exposed.
• maybeMarkContainerDead early-returns false while isRestarting==true.
• preflightContainerHealth early-returns nil (lets the optimistic forward proceed) while isRestarting==true.
• Breaks the self-fire at the probe layer.

L2 — Restart-context probe gate (restart_context.go)

• sendRestartContext now requires url != '' AND last_heartbeat_at > restart_start_ts before firing the trailing ProxyA2ARequest.
• New helper waitForFreshHeartbeat(ctx, wsID, restartStartTs, timeout) next to existing waitForWorkspaceOnline.
• Belt-and-suspenders so the probe never tries until the new container is actually addressable.

L3 — RestartByID debounce (workspace_restart.go)

• Silent-drop successive RestartByID calls within restartDebounceWindow=60s of restartStartedAt.
• NOT coalesce — actual drop. Drop is observable via restartByIDDropCounter (atomic.Uint64) + the dropped log line.
• Only programmatic path (secrets, maybeMarkContainerDead, preflightContainerHealth). HTTP Restart handler is unaffected (goes through RestartWorkspaceAutoOpts directly).
• Package-level restartDebounceWindow so tests shrink it to 5ms.

Test plan

• Regression: TestRestartByID_SingleProvisionPerRestart simulates the 4x prod thrash — single trigger + 4 simulated probe-driven RestartByID calls during the in-flight cycle. Asserts exactly 1 cycle and 4 drops (counter). Pre-fix: 4-5 cycles. Post-fix: 1.
• L1 gates: TestMaybeMarkContainerDead_SkippedWhileRestarting, TestPreflightContainerHealth_SkippedWhileRestarting — assert IsRunning not called + no DB update + no restart goroutine while restarting.
• L3 debounce: TestRestartByID_DebounceSilentDrop (5 rapid calls → 5 drops, counter==5), TestRestartByID_DebounceExpiresAfterWindow (post-window legitimate restarts proceed).
• Baseline invariants: TestIsRestarting_FalseWhenNoStateEntry, TestIsRestarting_TrueWhileCycleRunning.
• No regression: full internal/handlers/ test suite passes (go test ./internal/handlers/ -count=1 -timeout 300s → ok 15.834s).
• Existing TestCoalesceRestart_*, TestRestartByID_*, TestMaybeMarkContainerDead_*, TestPreflight_*, TestRestart_* all green.

Open Q

• Could be a 1-line fix in CPProvisioner.IsRunning — returning (true, nil) for state=pending would mask the EC2-pending state as alive-pre-online and close all the self-fire paths at the source. Rejected for this PR because:
  • IsRunning has other callers (admin status pages, orphan reconciler) that need the actual EC2 state, not a smoothed one.
  • The 3-layer fix is per-call-site, surgical, and preserves the existing IsRunning contract.
  • But: open for discussion in review. If reviewer prefers the simpler 1-line semantic, this PR can be reduced.

Boundaries

• NOT auto-merged
• 167 lines source diff (+297 test); single cohesive PR (the layers are interdependent).
• Companion issue internal#544 filed with class doc + reproduction recipe.
• HTTP Restart handler (user click) path is NOT affected by the debounce.

Verification SOP (per feedback_verify_actual_endstate_not_ack_follow_sop)

Post-merge canary: trigger a single secrets PUT against a non-prod workspace, watch Loki {tenant="tenant-staging"} |~ "provision.ec2_" for exactly 1 ec2_started event over the next 60s. Pre-fix this would emit 4. Don't claim closed without that read-back.

---

Closes internal#544.

## Summary Three-layer cohesive fix for the **2026-05-19 ~00:05-00:09Z 4x reprov thrash class** observed on prod-Reviewer + prod-Researcher: a single secrets PUT fanned out into 4x stop+provision cycles per workspace within 4 min, each stopping the just-launched (still-pending) EC2 of the previous cycle. Root-caused via Loki (`provision.ec2_started` / `ec2_stopped` pairs). Companion class-doc issue: **internal#544** (empirical chain + reproduction recipe). ## Empirical chain (verified against source) All files in `workspace-server/internal/handlers/`: 1. **`secrets.go:264`** — `POST /secrets` unconditionally fires `go h.restartFunc(workspaceID)`. 2. **`workspace_restart.go:runRestartCycle`** — sets `url=''` synchronously, then async provisions EC2 (provisionWorkspaceCP returns immediately after `cpProv.Start`). 3. **20-30s pending window** — `workspaces.url=''` AND `cpProv.IsRunning()==false` (looks identical to a dead container). 4. **`a2a_proxy_helpers.go`** — `maybeMarkContainerDead` OR `preflightContainerHealth` fires (from canvas `/delegations` poll or the trailing `restart-context` probe at the end of `runRestartCycle`), sees container-dead → calls `RestartByID` → loop. 5. **`workspace_restart.go:coalesceRestart`** — `pending=true`, drains by running ANOTHER full cycle → `ec2_stopped` of the just-booted instance → re-provision. ## Fix — three interdependent layers ### L1 — Restart-aware health probes (`a2a_proxy_helpers.go`) - `workspace_restart.go:isRestarting(workspaceID) bool` exposed. - `maybeMarkContainerDead` early-returns `false` while `isRestarting==true`. - `preflightContainerHealth` early-returns `nil` (lets the optimistic forward proceed) while `isRestarting==true`. - Breaks the self-fire at the probe layer. ### L2 — Restart-context probe gate (`restart_context.go`) - `sendRestartContext` now requires `url != ''` AND `last_heartbeat_at > restart_start_ts` before firing the trailing `ProxyA2ARequest`. - New helper `waitForFreshHeartbeat(ctx, wsID, restartStartTs, timeout)` next to existing `waitForWorkspaceOnline`. - Belt-and-suspenders so the probe never tries until the new container is actually addressable. ### L3 — RestartByID debounce (`workspace_restart.go`) - Silent-drop successive `RestartByID` calls within `restartDebounceWindow=60s` of `restartStartedAt`. - **NOT coalesce** — actual drop. Drop is observable via `restartByIDDropCounter` (atomic.Uint64) + the dropped log line. - Only programmatic path (secrets, maybeMarkContainerDead, preflightContainerHealth). HTTP `Restart` handler is unaffected (goes through `RestartWorkspaceAutoOpts` directly). - Package-level `restartDebounceWindow` so tests shrink it to 5ms. ## Test plan - [x] **Regression**: `TestRestartByID_SingleProvisionPerRestart` simulates the 4x prod thrash — single trigger + 4 simulated probe-driven RestartByID calls during the in-flight cycle. Asserts exactly **1** cycle and **4** drops (counter). Pre-fix: 4-5 cycles. Post-fix: 1. - [x] **L1 gates**: `TestMaybeMarkContainerDead_SkippedWhileRestarting`, `TestPreflightContainerHealth_SkippedWhileRestarting` — assert IsRunning not called + no DB update + no restart goroutine while restarting. - [x] **L3 debounce**: `TestRestartByID_DebounceSilentDrop` (5 rapid calls → 5 drops, counter==5), `TestRestartByID_DebounceExpiresAfterWindow` (post-window legitimate restarts proceed). - [x] **Baseline invariants**: `TestIsRestarting_FalseWhenNoStateEntry`, `TestIsRestarting_TrueWhileCycleRunning`. - [x] **No regression**: full `internal/handlers/` test suite passes (`go test ./internal/handlers/ -count=1 -timeout 300s` → `ok 15.834s`). - [x] Existing `TestCoalesceRestart_*`, `TestRestartByID_*`, `TestMaybeMarkContainerDead_*`, `TestPreflight_*`, `TestRestart_*` all green. ## Open Q - **Could be a 1-line fix in `CPProvisioner.IsRunning`** — returning `(true, nil)` for `state=pending` would mask the EC2-pending state as alive-pre-online and close all the self-fire paths at the source. Rejected for this PR because: - `IsRunning` has other callers (admin status pages, orphan reconciler) that need the *actual* EC2 state, not a smoothed one. - The 3-layer fix is per-call-site, surgical, and preserves the existing `IsRunning` contract. - But: open for discussion in review. If reviewer prefers the simpler 1-line semantic, this PR can be reduced. ## Boundaries - **NOT auto-merged** - 167 lines source diff (+297 test); single cohesive PR (the layers are interdependent). - Companion issue **internal#544** filed with class doc + reproduction recipe. - HTTP `Restart` handler (user click) path is **NOT** affected by the debounce. ## Verification SOP (per `feedback_verify_actual_endstate_not_ack_follow_sop`) Post-merge canary: trigger a single secrets PUT against a non-prod workspace, watch Loki `{tenant="tenant-staging"} |~ "provision.ec2_"` for exactly **1** `ec2_started` event over the next 60s. Pre-fix this would emit 4. Don't claim closed without that read-back. --- Closes internal#544.

hongming added 1 commit 2026-05-19 01:24:47 +00:00

fix(ws-server): close self-fire restart feedback loop (internal#544) ... 4bf87d122d

Three-layer cohesive fix for the 2026-05-19 ~00:05-00:09Z 4x reprov thrash
class observed on prod-Reviewer + prod-Researcher: a single secrets PUT
fanned out into 4x stop+provision cycles per workspace within 4 min,
each stopping the just-launched (still-pending) EC2 of the previous
cycle. Root-caused via Loki (provision.ec2_started / ec2_stopped pairs).

Empirical chain (all in workspace-server/internal/handlers/):
1. secrets.go SetSecret → go h.restartFunc → coalesceRestart cycle.
2. runRestartCycle sets url='' synchronously, then async provisions EC2.
3. During 20-30s pending window: url='' AND cpProv.IsRunning()==false
   — indistinguishable from a dead container.
4. Canvas /delegations poll OR the trailing restart-context probe fires
   ProxyA2A → maybeMarkContainerDead OR preflightContainerHealth →
   RestartByID → loop.
5. coalesceRestart's pending flag drains by running ANOTHER full cycle
   → ec2_stopped of the just-booted instance → re-provision.

Fix (single PR, three interdependent layers):

L1) Restart-aware health probes — workspace_restart.go exposes
    isRestarting(workspaceID) bool. Both maybeMarkContainerDead and
    preflightContainerHealth early-return false/nil while a restart
    cycle is in flight. Breaks the self-fire at the probe layer.

L2) Restart-context probe gate — sendRestartContext now requires
    url != '' AND last_heartbeat_at > restart_start_ts before firing
    the trailing ProxyA2A probe. Adds waitForFreshHeartbeat() next to
    waitForWorkspaceOnline. Belt-and-suspenders so the probe never
    tries until the new container is actually addressable.

L3) RestartByID debounce — silent-drop successive RestartByID calls
    within restartDebounceWindow=60s of restartStartedAt. Not coalesce
    (which would still drain to another full cycle). Drop is observable
    via restartByIDDropCounter (atomic.Uint64) + the dropped log line.
    Only programmatic path; HTTP Restart handler is unaffected.

Tests:
- TestIsRestarting_{FalseWhenNoStateEntry,TrueWhileCycleRunning}
- TestMaybeMarkContainerDead_SkippedWhileRestarting (L1)
- TestPreflightContainerHealth_SkippedWhileRestarting (L1)
- TestRestartByID_DebounceSilentDrop (L3, counter assertion)
- TestRestartByID_DebounceExpiresAfterWindow (L3, window release)
- TestRestartByID_SingleProvisionPerRestart (regression — asserts
  exactly 1 cycle per trigger, with 4 dropped self-fire probes)

Existing coalesce/restart/preflight/maybeMarkContainerDead tests
remain green. Full handlers suite: ok in 15.8s.

Closes internal#544.

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

agent-dev-a approved these changes 2026-05-19 01:40:14 +00:00

agent-dev-a left a comment

Member

Copy Link

Copy Source

Five-axis review (correctness/readability/arch/security/perf) — APPROVED.

Diff: +464/-0 across 4 files (3 modified + 1 new test file with 7 tests).

Correctness: Three independent layers gate the self-fire loop. L1 (isRestarting in maybeMarkContainerDead + preflightContainerHealth) early-returns before any IsRunning / DB mutation. L2 (waitForFreshHeartbeat) asserts url!='' AND last_heartbeat_at > RestartAt before firing the trailing context-probe. L3 (debounce window 60s + atomic drop counter, false→true edge only). Mutex on per-workspace restartState used correctly. HTTP Restart() handler correctly bypasses this (user-initiated unaffected).

Readability: Each guard comments WHY (internal#544 cited, EC2-pending 20-30s window described). Test file's package-level comment reproduces the 5-step empirical chain.

Architecture: Defense-in-depth. Doesn't change coalesceRestart semantics; just gates entry. Layer separation is clean (one layer per failure mode).

Security: No new external surface. Internal-state guards only. No secret/PII path touched.

Performance: Per-workspace sync.Map keeps mutex contention bounded; atomic counter is wait-free; waitForFreshHeartbeat polls DB but bounded by restartContextOnlineTimeout.

Tests: TestRestartByID_SingleProvisionPerRestart pins the regression shape (1 cycle for 4 probes), TestRestartByID_DebounceExpiresAfterWindow guarantees legit later restarts still work, Layer-1 tests pin isRestarting+maybeMarkContainerDead+preflightContainerHealth gates.

Ready to merge.

Five-axis review (correctness/readability/arch/security/perf) — APPROVED. **Diff**: +464/-0 across 4 files (3 modified + 1 new test file with 7 tests). **Correctness**: Three independent layers gate the self-fire loop. L1 (`isRestarting` in `maybeMarkContainerDead` + `preflightContainerHealth`) early-returns before any IsRunning / DB mutation. L2 (`waitForFreshHeartbeat`) asserts url!='' AND last_heartbeat_at > RestartAt before firing the trailing context-probe. L3 (debounce window 60s + atomic drop counter, false→true edge only). Mutex on per-workspace restartState used correctly. HTTP Restart() handler correctly bypasses this (user-initiated unaffected). **Readability**: Each guard comments WHY (internal#544 cited, EC2-pending 20-30s window described). Test file's package-level comment reproduces the 5-step empirical chain. **Architecture**: Defense-in-depth. Doesn't change coalesceRestart semantics; just gates entry. Layer separation is clean (one layer per failure mode). **Security**: No new external surface. Internal-state guards only. No secret/PII path touched. **Performance**: Per-workspace sync.Map keeps mutex contention bounded; atomic counter is wait-free; waitForFreshHeartbeat polls DB but bounded by restartContextOnlineTimeout. **Tests**: TestRestartByID_SingleProvisionPerRestart pins the regression shape (1 cycle for 4 probes), TestRestartByID_DebounceExpiresAfterWindow guarantees legit later restarts still work, Layer-1 tests pin isRestarting+maybeMarkContainerDead+preflightContainerHealth gates. Ready to merge.

agent-dev-b approved these changes 2026-05-19 01:40:23 +00:00

agent-dev-b left a comment

Member

Copy Link

Copy Source

Five-axis review — APPROVED.

Independently reviewed the diff. The 3-layer fix (isRestarting gate / waitForFreshHeartbeat / RestartByID debounce) addresses the prod-Reviewer/Researcher 4x reprov thrash class observed 2026-05-19 ~00:05-00:09Z. Test coverage covers the regression shape (1 cycle for 4 probes), the gate-correctness shape (no IsRunning call when restarting), and the release shape (debounce expires past window). All additive (+464/-0), no behaviour change to user-initiated Restart path. Ready to merge.

Five-axis review — APPROVED. Independently reviewed the diff. The 3-layer fix (isRestarting gate / waitForFreshHeartbeat / RestartByID debounce) addresses the prod-Reviewer/Researcher 4x reprov thrash class observed 2026-05-19 ~00:05-00:09Z. Test coverage covers the regression shape (1 cycle for 4 probes), the gate-correctness shape (no IsRunning call when restarting), and the release shape (debounce expires past window). All additive (+464/-0), no behaviour change to user-initiated Restart path. Ready to merge.

hongming merged commit d27df740f5 into main 2026-05-19 01:40:55 +00:00

hongming deleted branch fix/ws-server-self-fire-restart-loop 2026-05-19 01:40:56 +00:00

hongming referenced this issue from a commit 2026-05-19 01:40:56 +00:00

Merge pull request 'fix(ws-server): close self-fire restart feedback loop (internal#544)' (#1556) from fix/ws-server-self-fire-restart-loop into main

Sign in to join this conversation.

Reviewers

No Reviewers

agent-dev-a

agent-dev-b

Labels

Clear labels

area/ci

CI/CD pipeline issues

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

No Label

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 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#1556