The machine reading is divided by the lanes the whole process is running, not one conversation's (#907)

cmd/aforge/chatv3_orchestrate.go

@@ -38,6 +38,17 @@ import (
 // in the one the person is looking at, or the room that draws its graph would
 // ask the wrong session for a snapshot.
 
+// v3MachineLanes is THE ACCOUNT OF THIS MACHINE'S RUNNING TASK LANES, and there
+// is one for the life of the process because there is one machine under it.
+// /new and /resume each build a conversation of their own, and the memory half
+// of the reading each one's admission governor takes is of this whole process
+// and everything it started — /proc cannot say which conversation started which
+// compiler. So what that reading is divided by has to be every conversation's
+// lanes and never one conversation's, or a neighbour's build becomes this
+// conversation's per-node weight for the rest of the session (internal/session's
+// task_pressure.go, issue #907).
+var v3MachineLanes = session.NewTaskLanes()
+
 // v3OpenSession builds one v3 conversation with its adaptive runner wired to
 // itself.
 //
@@ -47,6 +58,9 @@ import (
 // session that could start a run the surface cannot then find is worse than a
 // session that cannot start one.
 func v3OpenSession(cfg session.Config) (*session.Agent, error) {
+	// Every conversation this process opens counts its task lanes in one
+	// account, because they are all running on one machine.
+	cfg.TaskLanes = v3MachineLanes
 	cfg, runs := v3Adaptive(cfg)
 	agent, err := session.New(cfg)
 	if err != nil {

docs/changes/unreleased/947-governor-footprint.md

@@ -0,0 +1,18 @@
+---
+kind: fixed
+title: the machine reading is divided by the lanes the whole process is running, not one conversation's
+pr: 947
+surface: [engine]
+invalidates:
+  - "The admission governor used to divide its reading by ONE GRAPH'S count of running nodes, while the memory in that reading is the whole process tree's; a second conversation's build therefore became this conversation's per-node footprint, and that figure only rises. The count now covers every lane the reading does, kept in one `session.TaskLanes` the process hands to every conversation it opens (`Config.TaskLanes`, set by `cmd/aforge`'s `v3OpenSession`). A graph handed none is alone in its process and keeps an account of its own."
+  - "`TaskGraph.running` used to be incremented and decremented at five sites (the frontier's start, `handBackSlotLocked`, `park`, `unpark`, and `standingWideWork`'s `graph.running = 1`). There is now ONE door, `takeLaneLocked`/`giveLaneLocked`, it moves the process's account in the same breath, and `TestEveryLaneMovesThroughTheOneDoor` fails the build on a sixth site."
+  - "`docs/design/task-start/DESIGN.md` used to state the per-conversation attribution gap as a boundary owed to #907. It is closed, and the section now describes the account."
+---
+
+Two conversations share one process, and `/proc` cannot say which of them started
+which compiler. Divided by one graph's own lanes, a neighbour's build read here as
+that build over this conversation's node count — and a measured footprint only
+rises, so one such reading narrowed every later fan in this conversation for the
+rest of the session. The same gap ran the other way on the reservation, each graph
+reading the other's visible memory as covering part of its own. Both halves now
+read one count over the same population the reading covers.

docs/design/task-start/DESIGN.md

@@ -305,15 +305,38 @@ re-read every second.
 
 **Boundaries, stated rather than hidden.** The reading is `/proc`'s, so on macOS
 and Windows the governor still says "cannot say" and never holds, exactly as
-before. And each conversation's graph keeps its own governor and so its own
-reservation, while the visible half is this whole process: two conversations in
-one process fanning out at the same moment each read the other's visible work as
-covering part of its own reservation, and one reading taken during the other's
-build can raise this graph's measured footprint for the rest of the session,
-because that figure only rises. The floor on the reading itself still holds for
-both. The fix is one account for the whole process, which needs every
-`TaskGraph.running` mutation behind one door; it is
-[#907](https://github.com/Agent-Field/aforge-v2/issues/907), not a clamp here.
+before.
+
+**One account for the whole process (#907).** Each conversation keeps its own
+graph and its own governor, but the count those governors divide the reading by
+is **the process's**, not the graph's. It has to be: the visible half is a
+reading of the whole process tree, and /proc cannot say which conversation
+started which compiler. Divided by one graph's own lanes, a second
+conversation's build read here as that build over *this* conversation's node
+count — and the measured footprint only rises, so one such reading narrowed
+every later fan in this conversation for the rest of the session. The same gap
+ran the other way on the reservation, each graph reading the other's visible
+memory as covering part of its own. Both close with `session.TaskLanes`: one mutex and one int,
+written by every graph through the one door every `TaskGraph.running` mutation
+now goes through (`takeLaneLocked`/`giveLaneLocked`), read back by
+`TaskGraph.lanesTaken` for `observe` and by `holdOnStartingLocked` for `admits`.
+No clamp to a fraction of `MemTotal`, no timer decay and no per-graph
+correction.
+
+**Which graphs share one is said, not assumed.** `cmd/aforge`'s `v3OpenSession`
+— the one door every conversation is built through, launch, relaunch, `/new`
+and `/resume` — builds one account for the life of the process and puts it on
+every `session.Config` it hands out; `Agent.graph` and `standingWideWork` read
+it off the config. A graph handed none is **alone in its process** and keeps an
+account of its own (`newTaskGraph`), which is the truth about an embedder with
+one conversation and about every scripted graph in the tests. A package-level
+variable would have been the same claim made silently, and it is a claim a
+library cannot make for its caller: a test binary is one process and a hundred
+unrelated machines, and a lane one scene left running would have narrowed the
+next scene's fan.
+`TestEveryLaneMovesThroughTheOneDoor` (`go/ast`, so it is on the pull-request
+gate) is what keeps there being one door, and the two graphs of
+`task_pressure_account_test.go` are the scene itself.
 
 **Deleted.** `admissionGovernor.holds`, the once-per-pass `busy` bool and the
 `busy` parameter of `holdOnStartingLocked`; `TaskGraph.machineBusy`, which asked

internal/manual/chat/how-tasks-run.md

@@ -2415,7 +2415,7 @@ minutes later — so each task that starts **sets aside a footprint** of memory
 reading shows it, and the next one is judged against what is left. A footprint is the
 larger of two figures this machine gives: one core's share of its memory (`MemTotal` ÷
 cores, so 2 GiB on a 16 GiB eight-core laptop) and the most memory per task aforge has
-watched this session's tasks actually hold. A quiet machine therefore starts roughly **one
+watched a task actually hold. A quiet machine therefore starts roughly **one
 task per core's share of the memory above the floor**; the rest wait saying `machine busy`
 and start as the earlier ones finish or as a reading shows room. Nothing is counted twice —
 as a running task's memory appears in the reading, what is set aside for it falls by as
@@ -2424,6 +2424,12 @@ much.
 This gates **starts only**. Nothing already running is ever touched; pressure drains as
 running tasks finish.
 
+**Tasks in your other conversations count here.** aforge is one program, and the reading
+covers that whole program and everything it started — it cannot tell which conversation
+started which build. So every task aforge is running is set aside for, whichever
+conversation started it: tasks fanning out beside you leave less room here too, and a
+build running beside yours is never mistaken for what one of your own tasks weighs.
+
 **The honest caveat:** these two governors read `/proc`, so they only work on Linux. On
 macOS and Windows there is no `/proc`, the machine cannot say, and silence is never
 treated as a hold — those platforms get no pressure gating at all, and `task.max_load` and

internal/session/session.go

@@ -1703,6 +1703,20 @@ type Config struct {
 	// gets, which is what lets pressure drain instead of having to be relieved.
 	TaskMinFreeMB int
 
+	// TaskLanes is THE ACCOUNT OF THIS MACHINE'S RUNNING TASK LANES, shared by
+	// every conversation this process opens ([NewTaskLanes]). The memory half
+	// of the reading above is `treeResidentMB(os.Getpid())` — this process and
+	// every descendant it started — and /proc cannot say which conversation
+	// started which compiler, so the count that reading is divided by has to
+	// cover the same work: every lane the process is running, not one graph's
+	// (task_pressure.go's ONE ACCOUNT FOR THE WHOLE PROCESS, #907).
+	//
+	// The process's own door sets it once and hands the same pointer to every
+	// conversation (cmd/aforge). Left nil, a graph is ALONE IN ITS PROCESS and
+	// keeps an account of its own — which is the truth for an embedder with one
+	// conversation, and for every scripted graph in the tests.
+	TaskLanes *TaskLanes
+
 	// InTask marks this agent as ONE TASK NODE'S RUNNER (task_run.go) rather
 	// than the conversation. It changes exactly two things, and both are
 	// consequences of the same fact — there is nobody to talk to:

internal/session/standing_run.go

@@ -884,6 +884,9 @@ func standingWideWork(cfg Config, item standing.Item, brief string) (Config, *Ta
 	// this firing leaves behind lands.
 	graph.limit = cfg.TaskParallel
 	graph.governor = newAdmissionGovernor(cfg.TaskMaxLoad, cfg.TaskMinFreeMB)
+	if cfg.TaskLanes != nil {
+		graph.lanes = cfg.TaskLanes
+	}
 	graph.store = newTaskStore(taskCheckpointPath(cfg.SessionFile))
 	graph.run = graph.runOwned
 	graph.report = graph.reportHome
@@ -930,7 +933,12 @@ func standingWideWork(cfg Config, item standing.Item, brief string) (Config, *Ta
 	// here. A firing on a machine capped at one task at a time has no second
 	// pair of hands to give parts to, and [TaskGraph.freeHands] answers that
 	// correctly only if the work already under way is counted as under way.
-	graph.running = 1
+	// It goes through the same door every other start does, so the process's
+	// account sees a standing firing's lane exactly as it sees a task's — the
+	// machine carries this one no differently (task_pressure.go, #907). No lock
+	// is taken: the graph is still this function's, with nothing else able to
+	// reach it.
+	graph.takeLaneLocked()
 
 	cfg.tasker = graph
 	cfg.taskID = id

internal/session/task_lane_door_law_test.go

@@ -0,0 +1,122 @@
+package session
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// theLaneDoor is the pair of functions that move a lane, and the only place in
+// this package where either count of one may move. Both names are here rather
+// than in the assertion below so that the failure can say what the door is.
+var theLaneDoor = map[string]string{
+	"takeLaneLocked": "take",
+	"giveLaneLocked": "give",
+}
+
+// A LANE IS TAKEN AND HANDED BACK THROUGH ONE DOOR, AND THE DOOR KEEPS BOTH
+// COUNTS IN STEP.
+//
+// Two numbers say how much work is running: this graph's, which the person's
+// task.parallel cap is measured against, and the process's ([TaskLanes]),
+// which the machine reading is divided by — and the reading is of the whole
+// process tree, so a start written to one and not the other is exactly the
+// attribution gap #907 closed. It was open in the first place because there
+// were five places a lane moved and no one of them that could be made to tell
+// the account; this law is what keeps there being one.
+//
+// It reads the package's own source, so it is on the pull-request gate
+// (scripts/laws.sh) and decides in well under a second.
+func TestEveryLaneMovesThroughTheOneDoor(t *testing.T) {
+	files := token.NewFileSet()
+	entries, err := os.ReadDir(".")
+	if err != nil {
+		t.Fatalf("read the package: %v", err)
+	}
+	// counter is every function that moves a graph's own count of running
+	// lanes, and account every function that writes the process's.
+	counter, account := map[string]bool{}, map[string]bool{}
+	for _, entry := range entries {
+		name := entry.Name()
+		if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") {
+			continue
+		}
+		file, err := parser.ParseFile(files, filepath.Join(".", name), nil, 0)
+		if err != nil {
+			t.Fatalf("parse %s: %v", name, err)
+		}
+		for _, decl := range file.Decls {
+			fn, ok := decl.(*ast.FuncDecl)
+			if !ok || fn.Body == nil {
+				continue
+			}
+			where := fn.Name.Name
+			ast.Inspect(fn.Body, func(n ast.Node) bool {
+				switch node := n.(type) {
+				case *ast.IncDecStmt:
+					if isField(node.X, "running") {
+						counter[where] = true
+					}
+				case *ast.AssignStmt:
+					for index, lhs := range node.Lhs {
+						// A COUNT MOVES BY ARITHMETIC, and `a.running = true`
+						// elsewhere in this package is a different field on a
+						// different type: what is watched here is a number.
+						if !isField(lhs, "running") {
+							continue
+						}
+						if node.Tok != token.ASSIGN || (index < len(node.Rhs) && isNumber(node.Rhs[index])) {
+							counter[where] = true
+						}
+					}
+				case *ast.CallExpr:
+					if call, ok := node.Fun.(*ast.SelectorExpr); ok && isField(call.X, "lanes") {
+						if call.Sel.Name == "take" || call.Sel.Name == "give" {
+							account[where] = true
+						}
+					}
+				}
+				return true
+			})
+		}
+	}
+	for where := range counter {
+		if _, isDoor := theLaneDoor[where]; !isDoor {
+			t.Errorf("%s moves a graph's count of running lanes itself. Every lane goes through takeLaneLocked or giveLaneLocked, "+
+				"which are the only two places that can tell the process's account as well (task_pressure.go, #907).", where)
+		}
+	}
+	for where := range account {
+		if _, isDoor := theLaneDoor[where]; !isDoor {
+			t.Errorf("%s writes the process's lane account directly, outside the door that keeps it in step with the graph's own count", where)
+		}
+	}
+	// AND THE DOOR ITSELF DOES BOTH HALVES, which is the whole of what makes it
+	// a door: a take that forgot the account would pass every check above.
+	for door := range theLaneDoor {
+		if !counter[door] {
+			t.Errorf("%s no longer moves the graph's own count of running lanes", door)
+		}
+		if !account[door] {
+			t.Errorf("%s no longer tells the process's lane account, so the two counts can drift apart", door)
+		}
+	}
+}
+
+// isField says whether an expression is a selector onto the named field, which
+// is how both halves of the count are spelled at every site.
+func isField(expr ast.Expr, name string) bool {
+	selector, ok := expr.(*ast.SelectorExpr)
+	return ok && selector.Sel.Name == name
+}
+
+// isNumber says whether an expression is a literal number, which is how a
+// count is set outright rather than stepped.
+func isNumber(expr ast.Expr) bool {
+	lit, ok := expr.(*ast.BasicLit)
+	return ok && lit.Kind == token.INT
+}