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++