amadan
offline move: the report refresh never renews the tombstone (fix round 1, finding 2)
Astra's review, MEDIUM. The 15 s refresh ran the sealer's step, which reaches offlineKeepStop, lease.Reassert and RenewLease: every refresh pushed the tombstone's expiry out another seven days. The refresh now calls offlineSealerRefresh (sealed to landed, sealer only): it reads the seal and the lease, treats a tombstone that is not this move's live one as the lost stop it always is, checks the source positions against the seal, and writes the sealer's report with the expiry exactly as read. It never calls Reassert or RenewLease. The sealer's ordinary pass still re-asserts once a pass, as designed, so the plan's bound holds across a long move: no write ever sets an expiry more than 7 days past the pass that wrote it, and an abandoned move's tombstone lapses 7 days after its last pass. Tests: three refreshes leave lock.json byte for byte and report its expiry; an expired, missing or replaced tombstone makes the refresh report the lost stop and write no lease. Routing the refresh through the old step fails the first. Census: envelope internal/host only.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 files changed,
+139
−3
docs/superpowers/notes/census-proposal-moves-offline-fixes.md+9 −0internal/host/movesoffline.go+5 −2internal/host/movesoffline_seal.go+51 −0internal/host/movesoffline_seal_test.go+72 −0internal/provision/testdata/plan14-root-reads-moves-offline-fixes.tsv+2 −1
diff --git a/docs/superpowers/notes/census-proposal-moves-offline-fixes.md b/docs/superpowers/notes/census-proposal-moves-offline-fixes.md| index 033cc16d1..67ba505c7 100644 |
| --- a/docs/superpowers/notes/census-proposal-moves-offline-fixes.md |
| +++ b/docs/superpowers/notes/census-proposal-moves-offline-fixes.md |
| @@ -59,6 +59,15 @@ The schedule runner is built only by the agent (`armSchedule`, from |
| Envelopes re-pinned: `internal/host`, `internal/schedule`. |
| +### Fix round 1 (Astra's review) |
| + |
| +| finding | unit | change | executing identity | disposition | |
| +|---|---|---|---|---| |
| +| 1 | `internal/schedule/runner.go` `Runner.Tick` | re-pinned: its due query moved into `Runner.dueRows` | the agent's schedule runner | re-frozen, as above | |
| +| 1 | `internal/schedule/runner.go` `Runner.dueRows` | new: held hosts left out by the query when `Skip` is set, and hosts `Skip` names paged past before the limit (one row read per host in the batch) | the agent's schedule runner | accepted legacy, J1 (as `Runner.Tick`) | |
| +| 2 | none | the refresh calls `offlineSealerRefresh` (seal, lease and source reports read, only the report written, the lease never), no root read of its own | the agent | envelope `internal/host` only | |
| + |
| + |
| ## The seal |
| The accepted-legacy rows add frozen declarations, so the legacy seal moves, |
diff --git a/internal/host/movesoffline.go b/internal/host/movesoffline.go| index 9138cfdc5..d272036c8 100644 |
| --- a/internal/host/movesoffline.go |
| +++ b/internal/host/movesoffline.go |
| @@ -359,6 +359,8 @@ const offlineReassertEvery = 15 * time.Second |
| // flip and open judge by age. The seal itself (sealing) and the target's |
| // landing are not repeated here: they are long, and they run on the pass a |
| // door's serial bump starts. From open on no door reads a report by age. |
| +// The sealer only re-reads and reports (offlineSealerRefresh): its ordinary |
| +// pass re-asserts the tombstone, and the refresh never does. |
| func offlineReassertRoles(m *provision.MoveState, source, sealer bool) (asSource, asSealer bool) { |
| switch m.State { |
| case provision.OfflineStopping, provision.OfflineSealing: |
| @@ -373,7 +375,8 @@ func offlineReassertRoles(m *provision.MoveState, source, sealer bool) (asSource |
| // the view holds in which this box's report is judged by age, it reads the |
| // record again and runs this box's source or sealer step for it, in |
| // offlineRound's order, under the pass lock. Each step is an ensure that ends |
| -// by rewriting the box's report (offlineSourceStop, offlineKeepStop). With no |
| +// by rewriting the box's report (offlineSourceStop, and offlineSealerRefresh, |
| +// which checks the tombstone but never re-asserts it). With no |
| // such move in flight it reads nothing at all. It returns how many records it |
| // acted on. |
| func (w *movesWorker) offlineReassert(ctx context.Context, s release.Store) int { |
| @@ -424,7 +427,7 @@ func (w *movesWorker) offlineReassert(ctx context.Context, s release.Store) int |
| errs = append(errs, w.offlineSourceStep(ctx, s, *rec)) |
| } |
| if asSealer { |
| - errs = append(errs, w.offlineSealerStep(ctx, s, *rec)) |
| + errs = append(errs, w.offlineSealerRefresh(ctx, s, *rec)) |
| } |
| if err := errors.Join(errs...); err != nil { |
| w.logTrouble(host, err) |
diff --git a/internal/host/movesoffline_seal.go b/internal/host/movesoffline_seal.go| index cbfb60cde..a0056e640 100644 |
| --- a/internal/host/movesoffline_seal.go |
| +++ b/internal/host/movesoffline_seal.go |
| @@ -967,6 +967,57 @@ func (w *movesWorker) offlineKeepStop(ctx context.Context, s release.Store, rec |
| return writeOfflineReport(ctx, s, living) |
| } |
| +// offlineSealerRefresh is the sealer's half of the report refresh |
| +// (offlineReassert), from offline-sealed to offline-landed: offlineKeepStop's |
| +// checks without its write to the lease. It reads the seal and the lease, a |
| +// tombstone that is not this move's live one is the lost stop it always is, |
| +// it checks the source positions against the seal, and it rewrites the |
| +// living report with the tombstone's owner and expiry exactly as read. It |
| +// never re-asserts the tombstone (fix round 1, Astra's finding 2: a refresh |
| +// every 15 s through offlineKeepStop pushed the expiry out seven days each |
| +// time); the sealer's ordinary pass keeps doing that, once a pass. |
| +func (w *movesWorker) offlineSealerRefresh(ctx context.Context, s release.Store, rec provision.Record) error { |
| + cfg := w.cfg |
| + m, host := rec.Move, rec.Host |
| + if !m.Offline() || cfg.Box == "" || m.Sealer != cfg.Box { |
| + return nil |
| + } |
| + switch m.State { |
| + case provision.OfflineSealed, provision.OfflineLanding, provision.OfflineLanded: |
| + default: |
| + return nil |
| + } |
| + raw, err := w.offlineSealRecord(ctx, s, rec) |
| + if err != nil { |
| + return err |
| + } |
| + seal, err := decodeSeal(raw, host, m.ID) |
| + if err != nil { |
| + return fmt.Errorf("offline move %s: %s: %w; it is never rewritten, so abort the move", m.ID, host, err) |
| + } |
| + st, err := w.offlineLock(ctx, host) |
| + if err != nil { |
| + return err |
| + } |
| + if !st.mine(m.ID) { |
| + return w.offlineLeaseLost(ctx, s, rec, seal, st) |
| + } |
| + if seal != nil { |
| + if err := w.offlineCheckPositions(ctx, s, rec, seal); err != nil { |
| + return err |
| + } |
| + } |
| + living := moves.OfflineReport{Op: m.ID, Host: host, Box: cfg.Box, Role: moves.OfflineRoleSealer} |
| + if seal != nil { |
| + living = *seal |
| + } |
| + living.Phase = moves.OfflinePhaseSealed |
| + living.At = cfg.now().UTC() |
| + living.TombstoneOwner = st.owner |
| + living.TombstoneUntil = st.expires.UTC() |
| + return writeOfflineReport(ctx, s, living) |
| +} |
| + |
| // offlineCheckPositions is the backstop behind the holds and the fence: |
| // every source report written since the seal (at or after its At) must be |
| // at or below the sealed final position. One past it is a box that wrote |
diff --git a/internal/host/movesoffline_seal_test.go b/internal/host/movesoffline_seal_test.go| index 02158838d..15e7d04ba 100644 |
| --- a/internal/host/movesoffline_seal_test.go |
| +++ b/internal/host/movesoffline_seal_test.go |
| @@ -1403,3 +1403,75 @@ func TestOfflineSealLostStopOnASourcePastTheSealedPosition(t *testing.T) { |
| t.Fatalf("the tombstone is %s", owner) |
| } |
| } |
| + |
| +// Fix round 1, Astra's finding 2: the report refresh (offlineReassert) is |
| +// report-only for the sealer. Repeated refreshes rewrite the sealer's report |
| +// with the tombstone's expiry as read and leave the lock object byte for |
| +// byte; an expired, missing or replaced tombstone makes the refresh report |
| +// the lost stop, never a fresh report, and the lock is not written. |
| +func TestOfflineSealerRefreshNeverRenewsTheTombstone(t *testing.T) { |
| + sw := newSealWorld(t) |
| + ie := sw.ie |
| + sw.instance(ie) |
| + sw.startAndSeal("ie-a", ie) |
| + sw.mustPass(ie) // seals, takes the tombstone, records sealed |
| + if st := sw.record().Move.State; st != provision.OfflineSealed { |
| + t.Fatalf("fixture: state %s", st) |
| + } |
| + sw.mustPass(ie) // the view learns sealed, as every pass refreshes it |
| + ctx := context.Background() |
| + key := sealPrefix + "/" + offHost + "/lock.json" |
| + ours, ok := sw.fake.Current(sealIE, key) |
| + if !ok { |
| + t.Fatal("fixture: no tombstone") |
| + } |
| + _, expires, _ := sw.lock() |
| + for i := range 3 { |
| + time.Sleep(20 * time.Millisecond) // a renewal would write a later expiry |
| + sw.advance(15 * time.Second) |
| + if n := ie.w.offlineReassert(ctx, sw.s); n != 1 { |
| + t.Fatalf("refresh %d acted on %d records", i, n) |
| + } |
| + if now, _ := sw.fake.Current(sealIE, key); !bytes.Equal(now, ours) { |
| + t.Fatalf("refresh %d rewrote the tombstone:\nbefore %s\nafter %s", i, ours, now) |
| + } |
| + r := sw.report(moves.OfflineRoleSealer, "ie-a") |
| + if r.Phase != moves.OfflinePhaseSealed || !r.At.Equal(sw.clock()) || !r.TombstoneUntil.Equal(expires.UTC()) || r.TombstoneOwner != moves.TombstoneOwner(offID) { |
| + t.Fatalf("refresh %d: report %+v, want sealed at %v with the expiry %v as read", i, r, sw.clock(), expires) |
| + } |
| + } |
| + |
| + lockBody := func(owner string, exp time.Time) []byte { |
| + b, err := json.Marshal(litestream.Lease{Generation: 1, Owner: owner, ExpiresAt: exp}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return b |
| + } |
| + for _, c := range []struct { |
| + name string |
| + damage func() |
| + }{ |
| + {"expired", func() { sw.fake.Put(sealIE, key, lockBody(moves.TombstoneOwner(offID), time.Now().Add(-time.Second))) }}, |
| + {"missing", func() { sw.fake.BareDelete(sealIE, key) }}, |
| + {"replaced", func() { sw.fake.Put(sealIE, key, lockBody(moves.TombstoneOwner(offID2), time.Now().Add(time.Hour))) }}, |
| + } { |
| + sw.fake.Put(sealIE, key, ours) |
| + sw.advance(15 * time.Second) |
| + ie.w.offlineReassert(ctx, sw.s) |
| + if r := sw.report(moves.OfflineRoleSealer, "ie-a"); r.Phase != moves.OfflinePhaseSealed { |
| + t.Fatalf("%s: before the damage the report is %s", c.name, r.Phase) |
| + } |
| + c.damage() |
| + before := sw.fake.Snapshot() |
| + sw.advance(15 * time.Second) |
| + ie.w.offlineReassert(ctx, sw.s) |
| + if sw.fake.Snapshot() != before { |
| + t.Fatalf("%s: the refresh wrote the lease", c.name) |
| + } |
| + r := sw.report(moves.OfflineRoleSealer, "ie-a") |
| + if r.Phase != moves.OfflinePhaseFailed || r.Items["lease"].OK || !strings.Contains(r.Items["lease"].Reason, "lost stop") || r.TombstoneOwner != "" { |
| + t.Fatalf("%s: report %+v, want the lost stop", c.name, r) |
| + } |
| + } |
| +} |
diff --git a/internal/provision/testdata/plan14-root-reads-moves-offline-fixes.tsv b/internal/provision/testdata/plan14-root-reads-moves-offline-fixes.tsv| index 37a9f1480..f59af42a9 100644 |
| --- a/internal/provision/testdata/plan14-root-reads-moves-offline-fixes.tsv |
| +++ b/internal/provision/testdata/plan14-root-reads-moves-offline-fixes.tsv |
| @@ -4,9 +4,10 @@ |
| # Defect 7 (fresh reports at every door step): envelope only. The source and sealer re-assert (movesWorker.offlineReassert, offlineReassertLoop) reads no root input of its own: it reads the record from the deployment store and calls the steps census-ruling-moves-offline already classified. The console's stale-report refusal reads nothing new. internal/console and internal/host re-pinned; the seal stands. |
| # Defect 8 (the source's scheduler wrote the moved host's env file after open): Runner.Tick and newScheduleDeps re-frozen at their dispositions, scheduleSkip new (J1), Runner.fire re-pinned at its class and task (hint, E2: overlay e's reclassification); internal/host and internal/schedule re-pinned. The seal moves. |
| # Fix round 1, finding 1 (Astra: skipped timers could starve other hosts): Runner.Tick re-pinned at its re-frozen disposition (its due query moved into dueRows); Runner.dueRows new, accepted legacy (J1, as Runner.Tick): held hosts excluded in the query and hosts Skip names paged past before the limit; internal/schedule re-pinned. The seal moves. |
| +# Fix round 1, finding 2 (Astra: the report refresh extended the tombstone): envelope only. The refresh now calls movesWorker.offlineSealerRefresh, which reads the seal, the lease (offlineLock) and the source reports and writes only the sealer's report, never the lease; it has no root read of its own. internal/host re-pinned; the seal stands. |
| @coverage internal/appconfig 2c32778cc777f026f1dd7b6404b11e3fabf1b6bd4912799d6ef26bfb59d42a07 |
| @coverage internal/console 7b015747c5ef8a3ee7473ad63f7f99200ce14c63c1b7a47945c212ad9239756b |
| -@coverage internal/host d1a6f949230f617898b8f308ee1f458b7ab3f396f8095885fb997ead8728f455 |
| +@coverage internal/host 9bdce80d912f31b185c3dfd74667bf8fa1caab32ac700b72d03d0a6dae45c28e |
| @coverage internal/schedule a76efa679b9089eff4f6dae8e6f0adc5d92de628e66e16d4a00c81ba70e086ff |
| @legacy 783751049991bdd17a140bba6ce9a3049a4075b1170e4882c7207b20bed24b41 internal/appconfig/syncexec.go SyncExecWithCallbacks legacy accepted-legacy accepted-legacy;rev3=PROPOSED-moves-offline-fixes;basis=route-and-input (J1,J4, main's disposition); the agent (/opt/carlos/carlos agent, carlos-edge.service, user carlos), through syncExecSerialized: main's body, with a nested row whose write found no instance directory named and skipped (errInstanceDirAbsent, WhenInstanceDirAbsent) instead of failing the pass; no new read site file:Stat@28119,registry:App@10156,registry:App@10371,registry:App@15749,registry:App@28004,registry:Environment@10164,registry:Environment@10379,registry:Environment@15757,registry:Environment@28012,registry:Host@1031,registry:Host@1460,registry:Host@15584,registry:Host@15682,registry:Host@16514,registry:Host@16865,registry:Host@17800,registry:Host@18341,registry:Host@21268,registry:Host@21485,registry:Host@22043,registry:Host@22233,registry:Host@22242,registry:Host@22261,registry:Host@25162,registry:Host@25779,registry:Host@25831,registry:Host@28513,registry:Host@831,registry:Host@902,registry:Host@996,whole:Route@10049,whole:Route@10110,whole:Route@10151,whole:Route@10268,whole:Route@10302,whole:Route@10366,whole:Route@1412,whole:Route@1441,whole:Route@15193,whole:Route@15243,whole:Route@15744,whole:Route@24758,whole:Route@25172,whole:Route@25876,whole:Route@27903,whole:Route@27936,whole:Route@27999,whole:Route@28100,whole:Route@28750,whole:Route@28823,whole:Route@693,whole:Route@721,whole:Route@750,whole:Route@776,whole:Route@9984 8d58864244e2a823103fab97653d48b7d586843eee5fcb5848194447c2f0210f |
| internal/appconfig/syncexec.go SyncExecWithCallbacks legacy accepted-legacy accepted-legacy;rev3=PROPOSED-moves-offline-fixes;basis=route-and-input (J1,J4, main's disposition); the agent (/opt/carlos/carlos agent, carlos-edge.service, user carlos), through syncExecSerialized: main's body, with a nested row whose write found no instance directory named and skipped (errInstanceDirAbsent, WhenInstanceDirAbsent) instead of failing the pass; no new read site file:Stat@28119,registry:App@10156,registry:App@10371,registry:App@15749,registry:App@28004,registry:Environment@10164,registry:Environment@10379,registry:Environment@15757,registry:Environment@28012,registry:Host@1031,registry:Host@1460,registry:Host@15584,registry:Host@15682,registry:Host@16514,registry:Host@16865,registry:Host@17800,registry:Host@18341,registry:Host@21268,registry:Host@21485,registry:Host@22043,registry:Host@22233,registry:Host@22242,registry:Host@22261,registry:Host@25162,registry:Host@25779,registry:Host@25831,registry:Host@28513,registry:Host@831,registry:Host@902,registry:Host@996,whole:Route@10049,whole:Route@10110,whole:Route@10151,whole:Route@10268,whole:Route@10302,whole:Route@10366,whole:Route@1412,whole:Route@1441,whole:Route@15193,whole:Route@15243,whole:Route@15744,whole:Route@24758,whole:Route@25172,whole:Route@25876,whole:Route@27903,whole:Route@27936,whole:Route@27999,whole:Route@28100,whole:Route@28750,whole:Route@28823,whole:Route@693,whole:Route@721,whole:Route@750,whole:Route@776,whole:Route@9984 8d58864244e2a823103fab97653d48b7d586843eee5fcb5848194447c2f0210f |