From 4eb049578e41f35d5e60ee264b83267696bccf76 Mon Sep 17 00:00:00 2001 From: T-Crypt <132922589+T-Crypt@users.noreply.github.com> Date: Wed, 21 Oct 2026 07:29:39 -0600 Subject: [PATCH 1/2] fix(broker): free a lease whose attached process is gone (#104) The run's disconnect was the only signal that a lease ended. When the whole run tree is killed and the disconnect never reaches the daemon, the lease stayed granted with no process for as long as anyone looked. Each tick now checks the attached process by PID and start time. Seen gone on two ticks in a row, with no container named and nothing of the run still counted on the card or alive as a descendant, the lease is released the way a disconnect releases it, with a lease.orphaned event first. A reused PID counts as gone. Nothing is signalled. Descendant sets are now pruned while any has entries, so a dead run's children clear from its lease; with no attached run and no entries the process table is still not read. Fixes #104. --- CHANGELOG.md | 13 +++ internal/broker/broker.go | 80 ++++++++++---- internal/broker/descend.go | 16 ++- internal/broker/reap_test.go | 201 +++++++++++++++++++++++++++++++++++ internal/cli/events.go | 2 +- internal/harbor/web/util.js | 2 + internal/proto/proto.go | 1 + 7 files changed, 289 insertions(+), 26 deletions(-) create mode 100644 internal/broker/reap_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 43083c5..4c8f9f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,19 @@ has a matching GitHub release whose body comes from its section below. one WARN line. A relative path fails `berth config check`, and `berth prefetch MODEL` makes the same ask by hand. +### Fixed + +- 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 Fixes from the first Windows testers, and four contributions. On Windows, the Epic Games 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" From 8bd40b341f4d4942dd52f2875399b325ab051d1e Mon Sep 17 00:00:00 2001 From: T-Crypt <132922589+T-Crypt@users.noreply.github.com> Date: Wed, 21 Oct 2026 07:45:21 -0600 Subject: [PATCH 2/2] ci: rerun after merging main