amadan

carlos / platform Public

console: a panic in a detached --from copy aborts its seed, not the console (Plan 5 Task 5 re-review)

runFrom recovers a panic, logs its stack and aborts the seed with SD-N39,
before releasing the reservation; scratch is already removed. The
reservation's record write no longer takes the request's cancellation.
Two tests now say and assert exactly what they show.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude d4637d48e3a313c6e35381a3e5feaedb2200477a parent 12726f4
2 files changed, +53 −10
  • internal/console/apiinstanceseed.go +17 −1
  • internal/console/apiinstanceseed_test.go +36 −9

CI passed — run details

diff --git a/internal/console/apiinstanceseed.go b/internal/console/apiinstanceseed.go
index 35cb5f974..e35c53914 100644
--- a/internal/console/apiinstanceseed.go
+++ b/internal/console/apiinstanceseed.go
@@ -21,6 +21,7 @@ import (
"net/http"
"os"
"path/filepath"
+ "runtime/debug"
"strconv"
"strings"
"sync/atomic"
@@ -260,7 +261,10 @@ func (s *Server) beginFrom(w http.ResponseWriter, r *http.Request, c seedCreate,
rec := c.rec
rec.Seed = &provision.SeedSource{ID: id, Kind: seeds.KindReplica, From: source, State: seeds.StateReserving,
By: c.principal, At: time.Now().UTC()}
- writeCtx, writeCancel := context.WithTimeout(ctx, instanceStoreTimeout)
+ // Not the request's cancellation: a client leaving now must not turn a
+ // write that landed into an error, which would answer 502 and start no
+ // copy while the reserving record held the host.
+ writeCtx, writeCancel := context.WithTimeout(context.WithoutCancel(ctx), instanceStoreTimeout)
defer writeCancel()
if err := provision.CreateRecord(writeCtx, s.cfg.Releases, rec); err != nil {
if errors.Is(err, release.ErrCASConflict) {
@@ -304,6 +308,18 @@ func (s *Server) runFrom(j *fromCopy) {
}
s.bumpSerial(bg)
}
+ // Nothing above this copy contains a panic any more: net/http's
+ // handler recover covered the synchronous door, and a panic here (in
+ // the restore's decode of replica data, the check, the store walk, the
+ // upload) would end the whole console. It aborts this seed instead.
+ // Registered after release and Done, so it runs before them; scratch
+ // removal, registered later, has already run.
+ defer func() {
+ if p := recover(); p != nil {
+ log.Printf("console: from %s: panic in the copy of %s (seed %s): %v\n%s", source, host, j.id, p, debug.Stack())
+ abort(j.failed, "a panic")
+ }
+ }()
if c.m.PrivateStorage {
// Its refusals are written to a held answer nobody reads but this:
diff --git a/internal/console/apiinstanceseed_test.go b/internal/console/apiinstanceseed_test.go
index b4ffee8ce..c4bf66e96 100644
--- a/internal/console/apiinstanceseed_test.go
+++ b/internal/console/apiinstanceseed_test.go
@@ -527,9 +527,10 @@ func TestFromWatcherCancelsARestoreInFlight(t *testing.T) {
}
// Once the door has answered 202 the copy no longer depends on the
-// request: a client that goes away, and takes its connection with it,
-// leaves the copy to finish, ready or aborted, and to clean up after
-// itself either way.
+// request: the request's context is cancelled when the handler returns,
+// and the client then cancelling and closing its connection changes
+// nothing. The copy finishes, ready or aborted, and cleans up after itself
+// either way. (A disconnect before the 202 is a refusal like any other.)
func TestFromCopyOutlivesItsClient(t *testing.T) {
for _, c := range []struct {
name string
@@ -588,6 +589,27 @@ func TestFromCopyOutlivesItsClient(t *testing.T) {
}
}
+// A panic in the detached copy aborts its seed and leaves the console
+// serving: nothing above the copy's goroutine would contain it.
+func TestFromPanicAbortsTheSeedAndSparesTheConsole(t *testing.T) {
+ f := newFromFixture(t, 3)
+ oldRestore := restoreToTarget
+ defer func() { restoreToTarget = oldRestore }()
+ restoreToTarget = func(ctx context.Context, c *dbdoor.Client, parent string, target uint64) (*dbdoor.Restored, error) {
+ if _, err := os.MkdirTemp(parent, "carlos-db-"); err != nil { // scratch the door must clear
+ return nil, err
+ }
+ panic("a decoder fault")
+ }
+ requireAborted(t, f.copy(t, fromBody), fromFailed)
+ f.requireClean(t)
+ f.requireNothingUploaded(t)
+ // Still serving: the next copy runs to ready.
+ restoreToTarget = oldRestore
+ requireReady(t, f.copy(t, `{"host":"c2.shop.example.com","from":{"host":"main.shop.example.com"}}`))
+ f.requireClean(t)
+}
+
func TestFromUploadFailureAborts(t *testing.T) {
f := newFromFixture(t, 3)
f.fx.srv.cfg.SeedData = failingPuts{f.data}
@@ -712,17 +734,22 @@ func TestFromPrivateStorageRules(t *testing.T) {
// The store gates that write nothing refuse before the record is reserved
// (ruling R3), with the store plane's own answers.
func TestFromStoreGatesRefuseBeforeTheReservation(t *testing.T) {
- for name, unwire := range map[string]func(*Server){
- "no backend": func(s *Server) { s.cfg.StoreProvisioner = nil },
- "no sealing key": func(s *Server) { s.cfg.SecretsPub = "" },
+ for name, c := range map[string]struct {
+ unwire func(*Server)
+ want string
+ }{
+ "no backend": {func(s *Server) { s.cfg.StoreProvisioner = nil },
+ "object storage is not configured for this deployment (CARLOS_STORE_BACKEND), and shop gives every instance its own storage — no instance can be created until one is wired or the policy is turned off (carlos instances enable --private-storage=false)"},
+ "no sealing key": {func(s *Server) { s.cfg.SecretsPub = "" },
+ "this console has no secrets key (CARLOS_SECRETS_PUBKEY) — it cannot deliver a store credential, so it will not mint one"},
} {
t.Run(name, func(t *testing.T) {
f := newFromFixture(t, 3)
enableInstances(t, f.fx, fromApp, fromManifest(true))
- unwire(f.fx.srv)
+ c.unwire(f.fx.srv)
code, out := f.create(t, fromBody)
- if code < 500 || str(t, out, "error") == "" {
- t.Fatalf("= %d %v, want the store plane's refusal", code, out)
+ if code != http.StatusServiceUnavailable || str(t, out, "error") != c.want {
+ t.Fatalf("= %d %v, want 503 %q", code, out, c.want)
}
f.requireNoRecord(t, fromHost)
f.requireClean(t)