carlos / platform Public

offline-move: console doors fix round 2 (keep the lock on a failed write, bound the write, seal box)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
claude agent, operated by paul@keymail.dev 345d071e9571bcde62ab903d10ad5fc80bf905d5 parent 968e606
2 files changed, +83 −11
  • internal/console/apiinstancemove_offline.go +41 −11
  • internal/console/apiinstancemove_offline_test.go +42 −0
diff --git a/internal/console/apiinstancemove_offline.go b/internal/console/apiinstancemove_offline.go
index 033ceb635..4d0813f9a 100644
--- a/internal/console/apiinstancemove_offline.go
+++ b/internal/console/apiinstancemove_offline.go
@@ -546,27 +546,57 @@ func (s *Server) handleAPIInstanceOfflineStart(w http.ResponseWriter, r *http.Re
writeRefusal(w, ref)
return
}
- got, ok := s.offlineWrite(w, r, d, start)
- if !ok {
- s.offlineReleaseLock(ctx, host, id)
+ // The write is bounded well under offlineLockGrace, so the lock cannot be
+ // judged stale while it is still in flight.
+ wctx, cancel := context.WithTimeout(ctx, offlineStartWrite)
+ defer cancel()
+ got, err := s.offlineApply(wctx, d, start)
+ if err != nil {
+ // A failed write does not prove nothing was written (a commit whose
+ // reply was lost). The lock is never released here: if the record
+ // carries this move the start succeeded, and if it does not the
+ // lock goes stale and frees itself.
+ rctx, rcancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second)
+ defer rcancel()
+ if cur, rerr := provision.GetRecord(rctx, s.cfg.Releases, d.acct.Sqid, d.app, host); rerr == nil && cur.Move.Offline() && cur.Move.ID == id {
+ got, err = cur, nil
+ }
+ }
+ if err != nil {
+ s.offlineWriteFailed(w, d, err)
return
}
+ s.bumpSerial(ctx)
writeJSON(w, http.StatusAccepted, s.offlineViewOf(ctx, got))
}
+// offlineStartWrite bounds the start door's record write.
+const offlineStartWrite = 30 * time.Second
+
+// offlineApply is one transition through the record writer.
+func (s *Server) offlineApply(ctx context.Context, d offlineDoor, mut func(*provision.Record) error) (*provision.Record, error) {
+ return provision.SetRecordMove(ctx, s.cfg.Releases, d.acct.Sqid, d.app, d.host, mut)
+}
+
+// offlineWriteFailed answers a transition's failure: a refusal with its
+// recorded sentence, anything else as unwritten.
+func (s *Server) offlineWriteFailed(w http.ResponseWriter, d offlineDoor, err error) {
+ if errors.Is(err, provision.ErrMoveConflict) {
+ jsonError(w, http.StatusConflict, offlineConflict(err))
+ return
+ }
+ log.Printf("console: offline move %s/%s/%s: %v", d.acct.Sqid, d.app, d.host, err)
+ jsonError(w, http.StatusBadGateway, "could not write the move. try again")
+}
+
// offlineWrite applies one transition and bumps the serial, or answers the
// refusal itself. When the move it leaves is cleared (finish, or the finish
// that ends an abort) it releases the lock.
func (s *Server) offlineWrite(w http.ResponseWriter, r *http.Request, d offlineDoor, mut func(*provision.Record) error) (*provision.Record, bool) {
ctx := r.Context()
- rec, err := provision.SetRecordMove(ctx, s.cfg.Releases, d.acct.Sqid, d.app, d.host, mut)
- if errors.Is(err, provision.ErrMoveConflict) {
- jsonError(w, http.StatusConflict, offlineConflict(err))
- return nil, false
- }
+ rec, err := s.offlineApply(ctx, d, mut)
if err != nil {
- log.Printf("console: offline move %s/%s/%s: %v", d.acct.Sqid, d.app, d.host, err)
- jsonError(w, http.StatusBadGateway, "could not write the move. try again")
+ s.offlineWriteFailed(w, d, err)
return nil, false
}
s.bumpSerial(ctx)
@@ -795,7 +825,7 @@ func (s *Server) offlineSealCheck(f offlineFleet, ev offlineEvidence, mv *provis
// byte for byte. The seal is written once, so the hash holds for the whole
// move whatever the sealer's living report does each pass.
func offlineSealHolds(ev offlineEvidence, mv *provision.MoveState, host string) bool {
- return ev.seal != nil && mv.Source != nil &&
+ return ev.seal != nil && ev.seal.Box == mv.Sealer && mv.Source != nil &&
offlineEvidenceMatches(mv.Source.Evidence, moves.OfflineSealKey(host, mv.ID), ev.seal.raw)
}
diff --git a/internal/console/apiinstancemove_offline_test.go b/internal/console/apiinstancemove_offline_test.go
index 7e453142f..6a991ff82 100644
--- a/internal/console/apiinstancemove_offline_test.go
+++ b/internal/console/apiinstancemove_offline_test.go
@@ -784,6 +784,10 @@ func TestOfflineFlipDoor(t *testing.T) {
bad = w.putSeal("ie-box", func(r *moves.OfflineReport) { r.TombstoneOwner = moves.TombstoneOwner("mv_other") })
w.rehash(bad)
w.refused(http.MethodPost, "/flip", "", http.StatusConflict, msgO25(provision.OfflineSealed))
+ // A seal written by a box other than the recorded sealer.
+ bad = w.putSeal("ie-x", nil)
+ w.rehash(bad)
+ w.refused(http.MethodPost, "/flip", "", http.StatusConflict, msgO25(provision.OfflineSealed))
offlinePut(t, w.fx.rel, sealKey, seal)
w.rehash(seal)
// A process left, a layout journal, a stale report.
@@ -1383,3 +1387,41 @@ func TestOfflineStartIgnoresOtherRecords(t *testing.T) {
offlinePut(t, w.fx.rel, provision.RecordKey("bab", "titogo", "junk.go.tito.io"), []byte("{not a record"))
w.start()
}
+
+// A failed record write does not prove nothing was written, so start never
+// releases its lock on one. A write that lands and then reports an error is a
+// start that succeeded; a write that does not land leaves the lock, which
+// goes stale by itself. Either way no other host starts meanwhile.
+func TestOfflineStartKeepsTheLockWhenTheRecordWriteFails(t *testing.T) {
+ for _, landed := range []bool{true, false} {
+ t.Run(fmt.Sprintf("landed=%v", landed), func(t *testing.T) {
+ w := newOfflineWorld(t, oneEach...)
+ if c, _, out := rawReq(t, http.MethodPost, instancesURL(w.fx, "bab", "titogo"), w.token, `{"host":"beta.go.tito.io","region":"ie"}`); c != http.StatusCreated {
+ t.Fatalf("create = %d %s", c, out)
+ }
+ w.fx.srv.cfg.Releases = &recordWriteFault{Store: w.fx.rel, key: provision.RecordKey("bab", "titogo", offlineHost), land: landed}
+ code, out := w.req(http.MethodPost, "", `{"region":"uk"}`)
+ if landed {
+ // The record carries the move: the start succeeded.
+ if code != http.StatusAccepted {
+ t.Fatalf("start = %d %s", code, out)
+ }
+ if v := w.view(code, out); v.State != provision.OfflineStopping || v.ID == "" {
+ t.Fatalf("view = %+v", v)
+ }
+ } else if code != http.StatusBadGateway || errorText(out) != "could not write the move. try again" {
+ t.Fatalf("start = %d %s", code, out)
+ }
+ a, ok := w.lock()
+ if !ok || a.Host != offlineHost {
+ t.Fatalf("lock = %+v %v, want it kept", a, ok)
+ }
+ // A second host's start is refused whichever way the write went.
+ bu := instancesURL(w.fx, "bab", "titogo") + "/beta.go.tito.io/move/offline"
+ c, _, o := rawReq(t, http.MethodPost, bu, w.token, `{"region":"uk"}`)
+ if c != http.StatusConflict || errorText(o) != offlineHost+" is moving offline. move one instance at a time" {
+ t.Fatalf("beta start = %d %s", c, o)
+ }
+ })
+ }
+}