diff --git a/CHANGELOG.md b/CHANGELOG.md index f4fd8b3..ea58be3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,16 @@ has a matching GitHub release whose body comes from its section below. (`squatters`, `unprotected`, `leases`; there is no `holders`). `berth events` is called with `--format json`, and a berth that answers nothing leaves its stderr in the report instead of a silent "returned nothing". +- A lease whose attached run process dies without the wrapper's disconnect + reaching the daemon (the run's tree force-killed, a crash, a power loss) + is no longer left granted with an empty PIDS list: the broker now checks + the attached process itself on every tick, and after it has been seen + gone on two consecutive ticks it releases the lease exactly as a + disconnect would, once the card shows nothing of the run left. A PID the + kernel reused counts as gone, a lease that names a container or still + counts a process on the card is left alone, and nothing is signalled. + The event log gets `lease.orphaned`, then `lease.released` and + `lease.freed` ([#104](https://github.com/T-Crypt/berth/issues/104)). ## [v0.3.2] - 2026-10-20 diff --git a/internal/broker/broker.go b/internal/broker/broker.go index 94ae1fd..87eb556 100644 --- a/internal/broker/broker.go +++ b/internal/broker/broker.go @@ -206,6 +206,10 @@ type lease struct { state state pid int pidStart uint64 + // goneTicks counts the consecutive ticks that saw the attached pid + // gone (no process with its start time); the reaper needs two, so a + // pid lookup hiccup on one tick cannot free a live lease. + goneTicks uint64 // container is the full ID of the runtime container the engine runs in // ("" for a host process), containerName the name the operator gave. container string @@ -698,29 +702,33 @@ func (b *Broker) Tick(ctx context.Context) { delete(l.gpu, id) } } - if l.state == parking { - b.parkingTick(key, l, now, read) - continue - } - if l.state != releasing && l.state != stuck { + if l.state == releasing || l.state == stuck { + if len(l.gpu) == 0 && read > l.releasedRead { + // Only a tick whose card read started after the release frees + // the lease: an earlier read knows nothing about a process that + // appeared after it. Read order, not the clock, decides. + b.log.Info("lease freed", "lease", key, "engine", l.engine) + b.counters.VRAMHandedOff += l.peak + b.foldFootprint(l, now) + b.emit(proto.Event{Kind: proto.EvFreed, Lease: key, Engine: l.engine, Bytes: l.peak}) + delete(b.leases, key) + } else if l.state == releasing && now.Sub(l.released) > b.cfg.ReleaseGrace { + // Never grant on process exit alone: the lease keeps blocking + // until the card shows its memory gone, and waiters are told why. + l.state = stuck + b.log.Warn("lease stuck: released but its processes still hold memory", "lease", key, "engine", l.engine, "holding", holding(l)) + b.counters.StuckCaught++ + b.emit(proto.Event{Kind: proto.EvStuck, Lease: key, Engine: l.engine, Bytes: l.used(), Detail: holding(l)}) + } continue } - if len(l.gpu) == 0 && read > l.releasedRead { - // Only a tick whose card read started after the release frees - // the lease: an earlier read knows nothing about a process that - // appeared after it. Read order, not the clock, decides. - b.log.Info("lease freed", "lease", key, "engine", l.engine) - b.counters.VRAMHandedOff += l.peak - b.foldFootprint(l, now) - b.emit(proto.Event{Kind: proto.EvFreed, Lease: key, Engine: l.engine, Bytes: l.peak}) - delete(b.leases, key) - } else if l.state == releasing && now.Sub(l.released) > b.cfg.ReleaseGrace { - // Never grant on process exit alone: the lease keeps blocking - // until the card shows its memory gone, and waiters are told why. - l.state = stuck - b.log.Warn("lease stuck: released but its processes still hold memory", "lease", key, "engine", l.engine, "holding", holding(l)) - b.counters.StuckCaught++ - b.emit(proto.Event{Kind: proto.EvStuck, Lease: key, Engine: l.engine, Bytes: l.used(), Detail: holding(l)}) + // The remaining granted leases (active, draining, parking, parked) + // get the reaper's pass: a run whose attached process is gone gives + // its lease back without the wrapper's disconnect. One the reaper + // released is on its way out, so it skips the park tick. + b.reapAttached(key, l, now) + if l.state == parking { + b.parkingTick(key, l, now, read) } } b.claimed = claimed @@ -1206,6 +1214,36 @@ func (l *Lease) Attach(pid int) { l.b.mu.Unlock() } +// reapAttached is the second, connection-independent signal that a run has +// ended: the wrapper's disconnect is the first, and it is the one that never +// arrives when the run's whole process tree is killed out from under it. +// The broker asks the process table itself. The attached process must have +// been seen gone on two consecutive ticks, so a pid lookup hiccup on one +// tick cannot free a live lease; a PID the kernel reused (same number, new +// start time) counts as gone. A lease that names a container is left alone +// (its processes belong to the container, not the PID), and so is one the +// card still counts a process under or that has a live descendant: its +// memory is not shown gone. The release then runs exactly as the wrapper's +// would. Nothing here signals a process. Called with b.mu held, from Tick. +func (b *Broker) reapAttached(key string, l *lease, now time.Time) { + if l.pid == 0 || l.pidStart == 0 { + l.goneTicks = 0 + return + } + if b.tree.StartTime(l.pid) == l.pidStart { + l.goneTicks = 0 + return + } + l.goneTicks++ + if l.goneTicks < 2 || l.container != "" || len(l.gpu) != 0 || len(l.desc) != 0 { + return + } + b.log.Warn("attached process is gone; releasing the lease", "lease", key, "engine", l.engine, "pid", l.pid) + b.emit(proto.Event{Kind: proto.EvOrphaned, Lease: key, Engine: l.engine, PID: l.pid, Detail: "attached process is gone"}) + l.goneTicks = 0 + b.beginRelease(l, now, true) +} + // beginRelease takes a granted lease off the grant path: releasing waits for // a tick whose card read happened after now to show none of its processes. // A lease that never recorded anything (no attached wrapper, nothing on the diff --git a/internal/broker/descend.go b/internal/broker/descend.go index d011cf8..390b5d3 100644 --- a/internal/broker/descend.go +++ b/internal/broker/descend.go @@ -16,17 +16,22 @@ import ( // recordDescendants reads the process table once and refreshes every // lease's descendant set: entries live until their PID plus start time no -// longer does. Called with b.mu held. +// longer does. Pruning runs whenever a set has entries, attached or not, so +// a set whose processes are all gone reads empty on the next tick; new +// entries are recorded only under a lease whose wrapper is still alive. +// With neither, the table is not read. Called with b.mu held. func (b *Broker) recordDescendants() { - attached := false + attached, recorded := false, false for _, l := range b.leases { + if len(l.desc) > 0 { + recorded = true + } if (l.state == active || l.state == draining) && l.pid > 0 && b.tree.StartTime(l.pid) == l.pidStart { attached = true - break } } - if !attached { + if !attached && !recorded { return } table := b.cfg.ProcTable() @@ -44,6 +49,9 @@ func (b *Broker) recordDescendants() { } } } + if !attached { + return + } kids := make(map[int][]procs.Proc, len(table)) for _, p := range table { kids[p.Parent] = append(kids[p.Parent], p) diff --git a/internal/broker/reap_test.go b/internal/broker/reap_test.go new file mode 100644 index 0000000..d97d788 --- /dev/null +++ b/internal/broker/reap_test.go @@ -0,0 +1,201 @@ +// Copyright 2026 Trevin Tindall (T-Crypt) +// SPDX-License-Identifier: Apache-2.0 + +package broker + +// What the reaper decides: a granted lease whose attached process is gone +// gives its berth back on the broker's own ticks, without the wrapper's +// disconnect. And what it leaves alone: an attached process that is still +// alive, and a lease that names a container. + +import ( + "context" + "testing" + + "github.com/T-Crypt/berth/internal/gpu" + "github.com/T-Crypt/berth/internal/procs" + "github.com/T-Crypt/berth/internal/proto" +) + +func eventCount(b *Broker, kind, lease string) int { + evs, _, _ := b.Events(0, false) + n := 0 + for _, e := range evs { + if e.Kind == kind && e.Lease == lease { + n++ + } + } + return n +} + +// A run whose tree is killed out from under it: the wrapper's disconnect +// never arrives, but the broker sees the attached process gone on its own +// ticks. One dead tick counts, two release the lease, and the next tick +// frees it, as a wrapper's release would. +func TestReaperFreesALeaseWhoseAttachedProcessDies(t *testing.T) { + tree := procs.Fake{Starts: map[int]uint64{100: 10}} + b, c, _ := setup(t, tree) + b.cfg.ProcTable = func() []procs.Proc { return nil } + l := must(t, acquire(b, Request{Engine: "a", Needs: 8 * GiB})) + l.Attach(100) + c.set(gpu.Proc{PID: 100, Name: "a-engine", Used: 8 * GiB}) + b.Tick(context.Background()) + if st := b.Status(); len(st.Leases) != 1 || st.Leases[0].State != "active" { + t.Fatalf("leases = %+v, want one active", st.Leases) + } + + // The run's tree is force-killed; the driver shows the memory free. + delete(tree.Starts, 100) + c.set() + b.Tick(context.Background()) + if st := b.Status(); len(st.Leases) != 1 || st.Leases[0].State != "active" { + t.Fatalf("one dead tick released the lease: %+v", st.Leases) + } + b.Tick(context.Background()) + if st := b.Status(); len(st.Leases) != 1 || st.Leases[0].State != "releasing" { + t.Fatalf("two dead ticks did not release the lease: %+v", st.Leases) + } + if n := eventCount(b, proto.EvOrphaned, "a-1"); n != 1 { + t.Fatalf("orphaned events = %d, want 1", n) + } + b.Tick(context.Background()) + if st := b.Status(); len(st.Leases) != 0 { + t.Fatalf("leases = %+v, want none once the card shows it free", st.Leases) + } + if n := eventCount(b, proto.EvFreed, "a-1"); n != 1 { + t.Fatalf("freed events = %d, want 1", n) + } +} + +// A PID the kernel handed to a stranger is not the engine: the identity is +// PID plus start time, so the reaper releases the lease while the +// stranger's memory stays a stowaway, never the lease's. +func TestReaperTreatsAReusedPIDAsGone(t *testing.T) { + tree := procs.Fake{Starts: map[int]uint64{100: 10}} + b, c, _ := setup(t, tree) + b.cfg.ProcTable = func() []procs.Proc { return nil } + l := must(t, acquire(b, Request{Engine: "a", Needs: 8 * GiB})) + l.Attach(100) + c.set(gpu.Proc{PID: 100, Name: "a-engine", Used: 8 * GiB}) + b.Tick(context.Background()) + + // The engine exits; the kernel hands the PID to something else. + tree.Starts[100] = 99 + c.set(gpu.Proc{PID: 100, Name: "stranger", Used: 2 * GiB}) + b.Tick(context.Background()) + b.Tick(context.Background()) + st := b.Status() + if len(st.Leases) != 1 || st.Leases[0].State != "releasing" || st.Leases[0].Used != 0 { + t.Fatalf("leases = %+v, want the orphaned lease released, holding nothing", st.Leases) + } + if len(st.Squatters) != 1 || st.Squatters[0].Name != "stranger" { + t.Fatalf("squatters = %+v, want the stranger on its own", st.Squatters) + } + if n := eventCount(b, proto.EvOrphaned, "a-1"); n != 1 { + t.Fatalf("orphaned events = %d, want 1", n) + } +} + +// A lease whose attached process is alive is never touched by the reaper, +// not even when one tick's pid lookup goes wrong: the count resets the +// tick the process is seen again. +func TestReaperLeavesALiveAttachedProcess(t *testing.T) { + tree := procs.Fake{Starts: map[int]uint64{100: 10}} + b, c, _ := setup(t, tree) + b.cfg.ProcTable = func() []procs.Proc { return nil } + l := must(t, acquire(b, Request{Engine: "a", Needs: 8 * GiB})) + l.Attach(100) + c.set(gpu.Proc{PID: 100, Name: "a-engine", Used: 8 * GiB}) + b.Tick(context.Background()) + + // A hiccup: one tick the lookup misses the process. + delete(tree.Starts, 100) + b.Tick(context.Background()) + tree.Starts[100] = 10 + for i := 0; i < 3; i++ { + b.Tick(context.Background()) + } + st := b.Status() + if len(st.Leases) != 1 || st.Leases[0].State != "active" { + t.Fatalf("the reaper touched a live lease: %+v", st.Leases) + } + if n := eventCount(b, proto.EvOrphaned, "a-1"); n != 0 { + t.Fatalf("orphaned events = %d, want none", n) + } +} + +// A lease that names a container is left to it: its processes belong to +// the container, not the attached PID, so a dead PID releases nothing. +func TestReaperLeavesAContainerLease(t *testing.T) { + tree := procs.Fake{Starts: map[int]uint64{100: 10}} + b, c, _ := setup(t, tree) + b.cfg.Containers = func(pid int) (string, bool) { return "", false } + b.cfg.ProcTable = func() []procs.Proc { return nil } + l := must(t, acquire(b, Request{Engine: "a", Container: "a", Needs: 8 * GiB})) + l.Attach(100) + l.AttachContainer(vllmID) + c.set(gpu.Proc{PID: 100, Name: "a-engine", Used: 8 * GiB}) + b.Tick(context.Background()) + + delete(tree.Starts, 100) + c.set() + b.Tick(context.Background()) + b.Tick(context.Background()) + st := b.Status() + if len(st.Leases) != 1 || st.Leases[0].State != "active" { + t.Fatalf("the reaper released a container lease: %+v", st.Leases) + } + if n := eventCount(b, proto.EvOrphaned, "a-1"); n != 0 { + t.Fatalf("orphaned events = %d, want none", n) + } +} + +// A child that outlives its wrapper still counts under the lease while it +// holds the card: the reaper waits for the child too, and releases once the +// card shows nothing of the run left. +func TestReaperWaitsForTheRunsChildren(t *testing.T) { + tree := procs.Fake{Parents: map[int]int{101: 100}, Starts: map[int]uint64{100: 10, 101: 11}} + b, c, _ := setup(t, tree) + table := []procs.Proc{ + {PID: 100, Parent: 1, Start: 10, Name: "berth"}, + {PID: 101, Parent: 100, Start: 11, Name: "a-engine"}, + } + b.cfg.ProcTable = func() []procs.Proc { return table } + l := must(t, acquire(b, Request{Engine: "a", Needs: 8 * GiB})) + l.Attach(100) + c.set(gpu.Proc{PID: 101, Name: "a-engine", Used: 8 * GiB}) + b.Tick(context.Background()) + if st := b.Status(); len(st.Leases) != 1 || st.Leases[0].Used != 8*GiB { + t.Fatalf("child not attributed to the lease: %+v", st.Leases) + } + + // The wrapper dies; the child keeps the card. + table = table[1:] + delete(tree.Starts, 100) + delete(tree.Parents, 100) + b.Tick(context.Background()) + b.Tick(context.Background()) + st := b.Status() + if len(st.Leases) != 1 || st.Leases[0].State != "active" || st.Leases[0].Used != 8*GiB { + t.Fatalf("the reaper released a lease whose child still holds the card: %+v", st.Leases) + } + + // The child goes too, and the card is empty: the record is pruned on + // the same tick, and the reaper, already past its two dead ticks, + // releases the run. + table = []procs.Proc{} + delete(tree.Starts, 101) + delete(tree.Parents, 101) + c.set() + b.Tick(context.Background()) + if st := b.Status(); len(st.Leases) != 1 || st.Leases[0].State != "releasing" { + t.Fatalf("the reaper did not release the emptied run: %+v", st.Leases) + } + if n := eventCount(b, proto.EvOrphaned, "a-1"); n != 1 { + t.Fatalf("orphaned events = %d, want 1", n) + } + b.Tick(context.Background()) + if st := b.Status(); len(st.Leases) != 0 { + t.Fatalf("leases = %+v, want none once the card shows it free", st.Leases) + } +} diff --git a/internal/cli/events.go b/internal/cli/events.go index f523268..93b9ae8 100644 --- a/internal/cli/events.go +++ b/internal/cli/events.go @@ -107,7 +107,7 @@ func eventKind(c *Context, kind string) string { switch kind { case proto.EvRefused, proto.EvStuck, proto.EvProbeFailed, proto.EvAlertRaised: return c.Style.Err(kind) - case proto.EvSquatter, proto.EvJuggle, proto.EvFootprint: + case proto.EvSquatter, proto.EvJuggle, proto.EvFootprint, proto.EvOrphaned: return c.Style.Warn(kind) case proto.EvGranted, proto.EvFreed, proto.EvReattached, proto.EvAlertCleared, proto.EvProbeRecovered, proto.EvSquatterGone: return c.Style.OK(kind) diff --git a/internal/harbor/web/util.js b/internal/harbor/web/util.js index bb5d19d..6faab94 100644 --- a/internal/harbor/web/util.js +++ b/internal/harbor/web/util.js @@ -91,6 +91,7 @@ export function kindWord(kind) { case "lease.granted": return "GRANTED"; case "lease.draining": return "DRAINING"; case "lease.released": return "RELEASED"; + case "lease.orphaned": return "ORPHANED"; case "lease.freed": return "FREED"; case "lease.stuck": return "AGROUND"; case "lease.refused": return "REFUSED"; @@ -164,6 +165,7 @@ const KIND_SI = { "lease.park_refused": "crit", "lease.park_evicted": "warn", "lease.released": "adrift", + "lease.orphaned": "adrift", "lease.freed": "adrift", "lease.stuck": "aground", "lease.juggle": "stale", diff --git a/internal/proto/proto.go b/internal/proto/proto.go index 1518518..c903c1a 100644 --- a/internal/proto/proto.go +++ b/internal/proto/proto.go @@ -129,6 +129,7 @@ const ( EvStuck = "lease.stuck" EvRefused = "lease.refused" EvReattached = "lease.reattached" + EvOrphaned = "lease.orphaned" // the attached process is gone and the broker released the lease itself EvSquatter = "squatter.appeared" EvSquatterGone = "squatter.gone" EvProbeFailed = "probe.failed"