From b18dc6d352ee1516b16b8cf1945e34a9fe4e40a9 Mon Sep 17 00:00:00 2001 From: AdaAibaby Date: Wed, 16 Sep 2026 16:23:40 +0800 Subject: [PATCH] fix(api): compensate a create the node finished after the request was cancelled When a create request is cancelled or times out on the client side while the node-side create is still running, the node can finish creating the instance even though the gRPC create returns Canceled. The API classifies placement as timed out and returns the error without ever registering the sandbox, so the instance is left running on the node with no running-store record, no team index entry, and no catalog entry. The periodic reconcile reclaims it, but only after a full orphan grace period, and the create failure path itself does nothing. The compensation that removes a node instance only runs on the sandboxStore.Add failure branch, i.e. after a successful placement. The placement-failure branch never compensated. placeSandbox now remembers the node whose in-flight create was interrupted by the context being cancelled (a ResourceExhausted refusal never started a create, so it is skipped) and returns it as PlacementResult.InterruptedNode. On the placement-failure branch, CreateSandbox issues a best-effort kill of that exact (sandboxID, executionID) on that node, detached from the cancelled request context. It reuses the same node-side kill the orphan reconciler runs, just eagerly, so the leak window drops from an orphan grace period to ~0. killSandboxOnNode already treats NotFound as success, so if the node never actually completed the create the kill is a cheap no-op. Reconcile stays as the backstop. Fixes #3637 --- .../internal/orchestrator/create_instance.go | 17 +++++ .../internal/orchestrator/delete_instance.go | 34 +++++++++ .../placement/interrupted_create_test.go | 73 +++++++++++++++++++ .../orchestrator/placement/placement.go | 26 ++++++- 4 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 packages/api/internal/orchestrator/placement/interrupted_create_test.go diff --git a/packages/api/internal/orchestrator/create_instance.go b/packages/api/internal/orchestrator/create_instance.go index fdd4ee1726..98ae3971b7 100644 --- a/packages/api/internal/orchestrator/create_instance.go +++ b/packages/api/internal/orchestrator/create_instance.go @@ -418,6 +418,23 @@ func (o *Orchestrator) CreateSandbox( o.maybeRemapResumeOriginNode(ctx, snapshotSandboxID, team, sbxData.NodeID, placed.WarmedNode) } + // A create the request context cancelled mid-flight may still have + // completed on the node, leaving an instance the API never registered + // (no running-store record, no index, no catalog entry). Compensate + // immediately with a best-effort kill of this exact (id, execution) + // rather than waiting a full orphan grace period for reconcile to + // reclaim it. Detached from the cancelled request context. + if placed.InterruptedNode != nil { + o.compensateInterruptedCreate( + context.WithoutCancel(ctx), + placed.InterruptedNode, + sandboxID, + executionID, + sbxData.Build.Vcpu, + sbxData.Build.RamMb, + ) + } + return sandbox.Sandbox{}, placementAPIError(err) } diff --git a/packages/api/internal/orchestrator/delete_instance.go b/packages/api/internal/orchestrator/delete_instance.go index e44537c2eb..1f88f27de2 100644 --- a/packages/api/internal/orchestrator/delete_instance.go +++ b/packages/api/internal/orchestrator/delete_instance.go @@ -29,6 +29,10 @@ const refusalRetryAfter = 10 * time.Second const pauseTimeout = 80 * time.Second +// interruptedCreateKillTimeout bounds the best-effort kill of an instance a +// node may have created for a request that was cancelled before registration. +const interruptedCreateKillTimeout = 30 * time.Second + func (o *Orchestrator) RemoveSandbox(ctx context.Context, teamID uuid.UUID, sandboxID string, opts sandbox.RemoveOpts) error { ctx, span := tracer.Start(ctx, "remove-sandbox") defer span.End() @@ -395,6 +399,36 @@ func (o *Orchestrator) killOrphanSandbox(ctx context.Context, sbx sandbox.NodeSa } } +// compensateInterruptedCreate best-effort kills an instance a node may have +// created for a request whose context was cancelled before the API registered +// it. It runs the same node-side kill as the orphan reconciler, just eagerly on +// the failure path so the leak window is near-zero instead of an orphan grace +// period. A no-op on the node (NotFound) is handled by killSandboxOnNode, so a +// create the node never actually completed costs only a cheap delete RPC. The +// caller must pass a context detached from the cancelled request. +func (o *Orchestrator) compensateInterruptedCreate(ctx context.Context, node *nodemanager.Node, sandboxID, executionID string, vcpu, ramMB int64) { + ctx, cancel := context.WithTimeout(ctx, interruptedCreateKillTimeout) + defer cancel() + + nodeSbx := sandbox.NodeSandbox{ + SandboxID: sandboxID, + ExecutionID: executionID, + NodeID: node.ID, + ClusterID: node.ClusterID, + VCpu: vcpu, + RamMB: ramMB, + } + + if err := o.killSandboxOnNode(ctx, node, nodeSbx, sandbox.KillReasonOrphaned); err != nil { + logger.L().Error(ctx, "Failed to compensate interrupted sandbox create on node", + zap.Error(err), + logger.WithSandboxID(sandboxID), + logger.WithNodeID(node.ID), + zap.String("kill_reason", sandbox.KillReasonOrphaned.String()), + ) + } +} + func (o *Orchestrator) killSandboxOnNode( ctx context.Context, node *nodemanager.Node, diff --git a/packages/api/internal/orchestrator/placement/interrupted_create_test.go b/packages/api/internal/orchestrator/placement/interrupted_create_test.go new file mode 100644 index 0000000000..ffca5cc3eb --- /dev/null +++ b/packages/api/internal/orchestrator/placement/interrupted_create_test.go @@ -0,0 +1,73 @@ +package placement + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/e2b-dev/infra/packages/api/internal/api" + "github.com/e2b-dev/infra/packages/api/internal/orchestrator/nodemanager" +) + +// TestPlaceSandbox_InterruptedCreateReportsNode: when a node's SandboxCreate is +// interrupted by the request context being cancelled, that node is reported as +// InterruptedNode so the caller can compensate for an instance the node may +// have completed server-side (issue #3637). +func TestPlaceSandbox_InterruptedCreateReportsNode(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + node := nodemanager.NewTestNode("node1", api.NodeStatusReady, 0, 4) + // Cancels the request context, then returns as the node create would when + // the deadline lands mid-flight (the orchestrator collapses this to Internal). + node.SetSandboxClient(erroringClient(cancel, status.Error(codes.Internal, "context canceled"))) + + result, err := PlaceSandbox(ctx, failIfCalled(t), []*nodemanager.Node{node}, node, testSbxRequest("test-sandbox"), CPURequirement{}, false, nil) + + require.Error(t, err) + assert.True(t, result.TimedOut) + require.NotNil(t, result.InterruptedNode, "the interrupted node must be reported for compensation") + assert.Equal(t, node.ID, result.InterruptedNode.ID) +} + +// TestPlaceSandbox_ResourceExhaustedInterruptNotCompensated: a node that refused +// with ResourceExhausted never started a create, so even when the deadline +// lands on it, it must NOT be reported as InterruptedNode — killing it would be +// a pointless RPC against a node that holds nothing. +func TestPlaceSandbox_ResourceExhaustedInterruptNotCompensated(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + node := nodemanager.NewTestNode("node1", api.NodeStatusReady, 0, 4) + node.SetSandboxClient(erroringClient(cancel, status.Error(codes.ResourceExhausted, "no capacity"))) + + result, err := PlaceSandbox(ctx, failIfCalled(t), []*nodemanager.Node{node}, node, testSbxRequest("test-sandbox"), CPURequirement{}, false, nil) + + require.Error(t, err) + assert.True(t, result.TimedOut) + assert.Nil(t, result.InterruptedNode, "a ResourceExhausted refusal must not be compensated") +} + +// TestPlaceSandbox_HardFailureNoCancelNoInterruptedNode: a hard create failure +// while the context is still live is a genuine node failure, not an interrupt. +// It must not be reported for compensation (the node cleaned up itself). +func TestPlaceSandbox_HardFailureNoCancelNoInterruptedNode(t *testing.T) { + t.Parallel() + + node := nodemanager.NewTestNode("node1", api.NodeStatusReady, 0, 4, + nodemanager.WithSandboxCreateError(status.Error(codes.Internal, "create failed"))) + + result, err := PlaceSandbox(t.Context(), failIfCalled(t), []*nodemanager.Node{node}, node, testSbxRequest("test-sandbox"), CPURequirement{}, false, nil) + + require.Error(t, err) + assert.False(t, result.TimedOut) + assert.Nil(t, result.InterruptedNode) +} diff --git a/packages/api/internal/orchestrator/placement/placement.go b/packages/api/internal/orchestrator/placement/placement.go index 3ca5975c99..05d89057d7 100644 --- a/packages/api/internal/orchestrator/placement/placement.go +++ b/packages/api/internal/orchestrator/placement/placement.go @@ -26,6 +26,13 @@ type PlacementResult struct { WarmedNode *nodemanager.Node // TimedOut reports whether placement failed due to context cancellation/deadline. TimedOut bool + // InterruptedNode is the node whose in-flight SandboxCreate was interrupted + // by the request context being cancelled/timing out. The node may have + // completed the create server-side even though the RPC returned Canceled, so + // it can hold an instance the API never registered. Set only on such a + // failure; callers should issue a best-effort kill of the (sandboxID, + // executionID) on it to avoid leaving an orphan until reconcile reclaims it. + InterruptedNode *nodemanager.Node // Response is the successful create's RPC response; nil on failure. Response *orchestrator.SandboxCreateResponse } @@ -89,6 +96,11 @@ func placeSandbox( // First node that attempted the create (not a fast ResourceExhausted refusal). var firstTriedNode *nodemanager.Node + // Node whose in-flight SandboxCreate returned because the request context + // was cancelled/timed out. That node may still have completed the create + // server-side, so it is the one that can hold an unregistered instance. + var interruptedNode *nodemanager.Node + var lastCreateErr error // failed reports the warming node only when the failure was caused by the @@ -96,6 +108,9 @@ func placeSandbox( // failures (where the context is still live) carry no node, so callers never // pin a retry to a node that genuinely refused the sandbox. // + // It also surfaces the interrupted node so the caller can compensate for a + // create the node may have finished after the RPC was cancelled. + // // TODO [EN-1099]: We key off ctx.Err() rather than the gRPC status code because // the orchestrator currently collapses a timed-out resume into codes.Internal // (it folds the deadline cause into the message, not the code), @@ -105,7 +120,7 @@ func placeSandbox( return PlacementResult{}, err } - return PlacementResult{WarmedNode: firstTriedNode, TimedOut: true}, err + return PlacementResult{WarmedNode: firstTriedNode, InterruptedNode: interruptedNode, TimedOut: true}, err } attempt := 0 @@ -194,6 +209,15 @@ func placeSandbox( firstTriedNode = failedNode } + // If the request context was cancelled while this node's create was in + // flight, the node may have finished creating the instance even though + // the RPC returned. A ResourceExhausted refusal never started a create, + // so it cannot leak. Track the most recent such node as the compensation + // target; the loop exits on the next ctx.Done() check. + if ctx.Err() != nil && statusCode != codes.ResourceExhausted { + interruptedNode = failedNode + } + switch statusCode { case codes.ResourceExhausted: refusals++