🐛 Recover from a server that no longer exists

Fixes #69, which regressed when the read was extracted into
fetchServerResourceState: it reports diagnostics, and a diagnostic can only fail
the read, so the 404 handling added in #71 was lost. A server deleted outside of
Terraform failed every plan until the state was edited by hand.

fetchServerResourceState now returns an error, and errServerNotFound for a 404,
which Read turns back into removing the resource from state. Create and Update
treat it like any other read failure, since the server is meant to exist there.
Delete treats a 404 as the success it describes, for the paths that reach it
without a refresh first: `-refresh=false`, or a server deleted in between.

Also fixes the advanced features conversion overwriting the diagnostics gathered
before it.

TestServerResourceDeletedOutOfBand covers the read: it deletes the server
through the API and refreshes, which fails against the code before this commit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

internal/provider/server_resource.go

@@ -3,6 +3,7 @@ package provider
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"slices"
@@ -772,8 +773,9 @@ func (r *serverResource) Create(ctx context.Context, req resource.CreateRequest,
 
 	// One extra read to check the final state of enabled_advanced_features, needed because
 	// some flags (like "cloud-init") are not set until the server is fully created. See #13
-	diag := r.fetchServerResourceState(ctx, &data)
-	resp.Diagnostics.Append(diag...)
+	if err := r.fetchServerResourceState(ctx, &data); err != nil {
+		resp.Diagnostics.AddError("Error reading server", err.Error())
+	}
 
 	// Save data into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
@@ -791,8 +793,15 @@ func (r *serverResource) Read(ctx context.Context, req resource.ReadRequest, res
 
 	// Read API call logic
 	tflog.Debug(ctx, fmt.Sprintf("Reading server: id=%s, name=%s", data.Id.String(), data.Name.ValueString()))
-	diag := r.fetchServerResourceState(ctx, &data)
-	resp.Diagnostics.Append(diag...)
+	if err := r.fetchServerResourceState(ctx, &data); err != nil {
+		if errors.Is(err, errServerNotFound) {
+			tflog.Warn(ctx, fmt.Sprintf("Server not found, removing from state: id=%s, name=%s", data.Id.String(), data.Name.ValueString()))
+			resp.State.RemoveResource(ctx)
+			return
+		}
+		resp.Diagnostics.AddError("Error reading server", err.Error())
+		return
+	}
 
 	// Get user data script
 	userDataResp, err := r.bc.client.GetServersServerIdUserDataWithResponse(ctx, data.Id.ValueInt64())
@@ -841,8 +850,10 @@ func (r *serverResource) Update(ctx context.Context, req resource.UpdateRequest,
 			return
 		}
 
-		diag := r.fetchServerResourceState(ctx, &state)
-		resp.Diagnostics.Append(diag...)
+		if err := r.fetchServerResourceState(ctx, &state); err != nil {
+			resp.Diagnostics.AddError("Error reading server", err.Error())
+			return
+		}
 
 		// Save updated data into Terraform state
 		resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
@@ -915,8 +926,10 @@ func (r *serverResource) Update(ctx context.Context, req resource.UpdateRequest,
 	if !plan.VpcIpv4Address.Equal(state.VpcIpv4Address) && !plan.VpcIpv4Address.IsNull() && !plan.VpcIpv4Address.IsUnknown() {
 		// If VPC was just changed, IP address needs to be fetched so it can be sent in the request
 		if refreshNeeded {
-			diag := r.fetchServerResourceState(ctx, &state)
-			resp.Diagnostics.Append(diag...)
+			if err := r.fetchServerResourceState(ctx, &state); err != nil {
+				resp.Diagnostics.AddError("Error reading server", err.Error())
+				return
+			}
 			resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
 			if resp.Diagnostics.HasError() {
 				return
@@ -1306,6 +1319,11 @@ func (r *serverResource) Delete(ctx context.Context, req resource.DeleteRequest,
 		return
 	}
 
+	if serverResp.StatusCode() == http.StatusNotFound {
+		tflog.Warn(ctx, fmt.Sprintf("Server already gone, nothing to delete: id=%s, name=%s", data.Id.String(), data.Name.ValueString()))
+		return
+	}
+
 	if serverResp.StatusCode() != http.StatusNoContent {
 		resp.Diagnostics.AddError(
 			"Unexpected HTTP status code deleting server",
@@ -1473,25 +1491,25 @@ func (r *serverResource) updateSeparatePrivateNetworkInterface(
 	return nil
 }
 
-func (r *serverResource) fetchServerResourceState(ctx context.Context, state *serverResourceModel) diag.Diagnostics {
+// errServerNotFound reports that Binary Lane no longer has the server, so that Read can drop it
+// from state instead of failing (see #69).
+var errServerNotFound = errors.New("server not found")
+
+// fetchServerResourceState reads the server and maps it into state, returning errServerNotFound
+// if the server no longer exists.
+func (r *serverResource) fetchServerResourceState(ctx context.Context, state *serverResourceModel) error {
 	var diags diag.Diagnostics
 
 	serverResp, err := r.bc.client.GetServersServerIdWithResponse(ctx, state.Id.ValueInt64())
 	if err != nil {
-		return diag.Diagnostics{
-			diag.NewErrorDiagnostic(
-				fmt.Sprintf("Error reading server: id=%s, name=%s", state.Id.String(), state.Name.ValueString()),
-				err.Error(),
-			),
-		}
+		return fmt.Errorf("error reading server: id=%s, name=%s, error: %w", state.Id, state.Name.ValueString(), err)
+	}
+	if serverResp.StatusCode() == http.StatusNotFound {
+		return errServerNotFound
 	}
 	if serverResp.StatusCode() != http.StatusOK {
-		return diag.Diagnostics{
-			diag.NewErrorDiagnostic(
-				fmt.Sprintf("Unexpected HTTP status code %s reading server: name=%s, id=%s", serverResp.Status(), state.Name.ValueString(), state.Id.String()),
-				string(serverResp.Body),
-			),
-		}
+		return fmt.Errorf("unexpected HTTP status code %s reading server: id=%s, name=%s, details: %s",
+			serverResp.Status(), state.Id, state.Name.ValueString(), serverResp.Body)
 	}
 
 	state.Id = types.Int64Value(serverResp.JSON200.Server.Id)
@@ -1524,7 +1542,7 @@ func (r *serverResource) fetchServerResourceState(ctx context.Context, state *se
 	}
 
 	advFeat := serverResp.JSON200.Server.AdvancedFeatures.EnabledAdvancedFeatures
-	state.AdvancedFeatures, diags = resources.NewAdvancedFeaturesValue(
+	advFeatValue, advFeatDiags := resources.NewAdvancedFeaturesValue(
 		resources.AdvancedFeaturesValue{}.AttributeTypes(ctx),
 		map[string]attr.Value{
 			"emulated_hyperv":  types.BoolValue(slices.Contains(advFeat, "emulated-hyperv")),
@@ -1538,7 +1556,9 @@ func (r *serverResource) fetchServerResourceState(ctx context.Context, state *se
 			"qemu_guest_agent": types.BoolValue(slices.Contains(advFeat, "qemu-guest-agent")),
 			"uefi_boot":        types.BoolValue(slices.Contains(advFeat, "uefi-boot")),
 		})
-	if diags.HasError() {
+	diags.Append(advFeatDiags...)
+	state.AdvancedFeatures = advFeatValue
+	if advFeatDiags.HasError() {
 		state.AdvancedFeatures = resources.NewAdvancedFeaturesValueUnknown()
 	}
 
@@ -1600,7 +1620,11 @@ func (r *serverResource) fetchServerResourceState(ctx context.Context, state *se
 		state.PrivateIpv6Addresses = tfPrivateIpv6Addresses
 	}
 
-	return diags
+	if diags.HasError() {
+		return errFromDiagnostics(diags)
+	}
+
+	return nil
 }
 
 func isAdvFeatChanged(config *resources.AdvancedFeaturesValue, data *resources.AdvancedFeaturesValue) bool {

internal/provider/server_resource_test.go

@@ -8,6 +8,7 @@ import (
 	"log"
 	"net/http"
 	"regexp"
+	"strconv"
 	"strings"
 	"terraform-provider-binarylane/internal/binarylane"
 	"testing"
@@ -284,6 +285,72 @@ EOT
 	})
 }
 
+// TestServerResourceDeletedOutOfBand covers a server that no longer exists at Binary Lane: the
+// next refresh drops it from state so a plan can re-create it, rather than failing with the 404
+// and leaving the state to be edited by hand (#69).
+func TestServerResourceDeletedOutOfBand(t *testing.T) {
+	// Must assign a password to the server or Binary Lane will send emails
+	password := GenerateTestPassword(t)
+
+	var serverId string
+
+	resource.ParallelTest(t, resource.TestCase{
+		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
+		Steps: []resource.TestStep{
+			{
+				Config: providerConfig + `
+resource "binarylane_server" "test" {
+	name              = "tf-test-server-out-of-band"
+	// TEMPORARY: "per" is intermittently returning "Unable to find a suitable host for the
+	// requested server configuration"
+	region            = "mel"
+	image             = "debian-12"
+	size              = "std-min"
+	public_ipv4_count = 0
+	password          = "` + password + `"
+}
+`,
+				Check: resource.TestCheckResourceAttrWith("binarylane_server.test", "id", func(value string) error {
+					serverId = value
+					return nil
+				}),
+			},
+			{
+				PreConfig:          func() { deleteServerOutOfBand(t, serverId) },
+				RefreshState:       true,
+				ExpectNonEmptyPlan: true, // the server is gone from state, so the plan re-creates it
+			},
+		},
+	})
+}
+
+// deleteServerOutOfBand deletes a server through the API, as if someone had removed it outside of
+// Terraform.
+func deleteServerOutOfBand(t *testing.T, serverId string) {
+	t.Helper()
+
+	client, err := binarylane.NewClientWithDefaultConfig()
+	if err != nil {
+		t.Fatalf("Error creating Binary Lane API client: %s", err)
+	}
+
+	id, err := strconv.ParseInt(serverId, 10, 64)
+	if err != nil {
+		t.Fatalf("Error parsing server id %q: %s", serverId, err)
+	}
+
+	reason := "Terraform deletion"
+	deleteResp, err := client.DeleteServersServerIdWithResponse(context.Background(), id, &binarylane.DeleteServersServerIdParams{
+		Reason: &reason,
+	})
+	if err != nil {
+		t.Fatalf("Error deleting server %d out of band: %s", id, err)
+	}
+	if deleteResp.StatusCode() != http.StatusNoContent {
+		t.Fatalf("Unexpected status %d deleting server %d out of band: %s", deleteResp.StatusCode(), id, deleteResp.Body)
+	}
+}
+
 func TestServerResourceRename(t *testing.T) {
 	// Must assign a password to the server or Binary Lane will send emails
 	password := GenerateTestPassword(t)

internal/provider/util.go

@@ -2,11 +2,14 @@ package provider
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"slices"
+	"strings"
 
 	"github.com/hashicorp/terraform-plugin-framework/attr"
 	d_schema "github.com/hashicorp/terraform-plugin-framework/datasource/schema"
+	"github.com/hashicorp/terraform-plugin-framework/diag"
 	r_schema "github.com/hashicorp/terraform-plugin-framework/resource/schema"
 	"github.com/hashicorp/terraform-plugin-framework/types"
 )
@@ -266,3 +269,13 @@ func listContainsUnknown(ctx context.Context, list types.List) bool {
 func Pointer[T any](d T) *T {
 	return &d
 }
+
+// errFromDiagnostics turns error diagnostics into an error, for helpers that report errors rather
+// than diagnostics.
+func errFromDiagnostics(diags diag.Diagnostics) error {
+	messages := make([]string, 0, len(diags))
+	for _, d := range diags.Errors() {
+		messages = append(messages, fmt.Sprintf("%s: %s", d.Summary(), d.Detail()))
+	}
+	return errors.New(strings.Join(messages, "; "))
+}