Skip to content

fix(service): reconcile re-inspects a container before reaping its row (UC-20) - #360

Merged
sumansaurabh merged 2 commits into
mainfrom
fix/reconcile-reinspect-before-reap
Jul 23, 2026
Merged

fix(service): reconcile re-inspects a container before reaping its row (UC-20)#360
sumansaurabh merged 2 commits into
mainfrom
fix/reconcile-reinspect-before-reap

Conversation

@sumansaurabh

Copy link
Copy Markdown
Contributor

What

Reconcile's "container gone" branch (internal/service/service.go) deletes the sandbox store row and its cluster FSM placement whenever a sandbox ID is absent from a single bulk ListManaged() snapshot. That snapshot is one racy read per tick — the containerd driver's ListManaged silently skips a container whose per-item Inspect errors at that instant (mid-adopt / mid-start; lifecycle.go does if err != nil { continue }). A create that is merely still settling is then indistinguishable from a durably-dead container, so a healthy sandbox gets reaped.

Observed live (UC-20, cluster-mixed-benchmark-with-obs): node A creates+owns sb-… (POST 201), its own 5-min reconcile tick logs audit reconcile destroyed ~180ms later, and the follow-up snapshot — routed by the domain LB to node B — returns 404 because A already deleted the row + placement.

flowchart TD
    C[create returns 201 on owner node A] --> R[reconcile tick: bulk ListManaged]
    R --> M{sb.ID in snapshot?}
    M -- yes --> OK[steady-state update]
    M -- no --> G{"BEFORE: reap immediately<br/>AFTER: re-Inspect the container"}
    G -- "AFTER: driver resolves it (alive)" --> SP[spare row; next pass adopts]
    G -- "gone (error / empty identity)" --> D[reap: delete row + FSM placement]
Loading

Fix

reconcileGoneContainerConfirmed re-Inspects the specific container before the destructive teardown. A container the driver still resolves (no error, non-empty runtime identity) proves the bulk snapshot was stale → spare the row. A genuinely gone container (Inspect error / empty identity) still reaps, so orphan-row cleanup and host_port freeing are unchanged.

pr-review.md call-outs

  • Idempotency (§1): Unchanged and strengthened. Reconcile stays idempotent; the guard only makes it more conservative (re-verify before delete). No new host-port-pool walk, no new resource allocation.
  • Sandbox boot-path latency (§2): No impact. The change is entirely inside Reconcile, a background 5-min sweep. CreateSandbox and its callees are untouched. First-call case: N/A.
  • Failure-path consistency (§4): Reduces spurious deletes. On the spare path the branch just continues — no partial caddy/store writes. This directly addresses the "reconcile as routine cleanup" hazard by not leaning on a single racy signal to destroy state.
  • TCP host-port pool & L4 (§5): Not touched. The reap path (which cascades exposed_ports and frees host_port) still runs for genuinely-gone containers; TestReconcileDestroyedRowFreesHostPort and TestDestroyEventFreesHostPort still pass.
  • Cluster-mode correctness (§6): The reap calls deleteSelfOwnedClusterPlacement; the guard makes placement deletion fire only after a targeted Inspect confirms the container is gone — strictly fewer spurious FSM placement deletions. No split-brain / replay implications (this only makes local reap more conservative; it never writes to the FSM). Single-node unaffected: runtimeForSandbox + Inspect behave the same, and the Noop cluster's deleteSelfOwnedClusterPlacement is already a no-op.

Test plan

  • New regression TestReconcileReInspectsBeforeReap (internal/service/reconcile_destroy_test.go): re-inspectable container → spared; not-found → reaped. Verified fails without the guard (reaped on a stale snapshot) and passes with it.
  • go test ./internal/service/... green (42s); go vet + go build ./... + gofmt clean.

Known limitation

This closes the mechanism where the bulk ListManaged transiently missed a container that was actually alive. If the deeper trigger is a genuine container removal ~180ms post-create (the ordering evidence is consistent with that too), re-Inspect also fails and the row still (correctly) reaps — UC-20 would not fully close and the true culprit would remain. Distinguishing needs an instrumented live run (log every containerd container-delete with its caller); tracked separately, not in this PR.

🤖 Generated with Claude Code

…w (UC-20)

Reconcile's "container gone" branch deletes the sandbox store row AND its
cluster FSM placement whenever a sandbox ID is absent from a single bulk
ListManaged() snapshot. That snapshot is one racy read per tick: the
containerd driver's ListManaged silently skips a container whose per-item
Inspect errors at that instant (mid-adopt / mid-start — lifecycle.go's
`if err != nil { continue }`). A create that is merely still settling is
then indistinguishable from a durably-dead container, so reconcile reaps a
healthy sandbox — dropping its row and placement, which surfaced as an
intermittent cross-node snapshot 404 (UC-20: create+own on node A, snapshot
routed to node B → 404 because A's reconcile already deleted it).

Fix: reconcileGoneContainerConfirmed re-Inspects the specific container
before the destructive teardown. A container the driver still resolves
(no error, non-empty runtime identity) proves the bulk snapshot was stale,
so the row is spared and the next steady-state pass adopts it. A genuinely
gone container (Inspect error / empty identity) still reaps, so orphan-row
cleanup and host_port freeing are unchanged.

Regression: TestReconcileReInspectsBeforeReap (verified fails without the
guard — reaped on a stale snapshot — and passes with it).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 23, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
❌ Deployment failed
View logs
microvm 3e085bc Jul 23 2026, 12:54 AM

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…nches

Adds a direct unit test for the guard's nil-row and unregistered-runtime-
driver paths, which the Reconcile-level regression can't reach. Brings the
helper to 100% and lifts codecov/patch above the project target.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@sumansaurabh
sumansaurabh merged commit b621a2d into main Jul 23, 2026
18 of 20 checks passed
@sumansaurabh
sumansaurabh deleted the fix/reconcile-reinspect-before-reap branch July 23, 2026 01:40
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