diff --git a/CLAUDE.md b/CLAUDE.md index 79539ca..4d53379 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -3436,3 +3436,110 @@ three, and `api.CameraState` is the one function that decides them: - **An unparseable `last_seen_at` is stale**, not connected. It should be impossible, which is precisely why it must not fall through to the state that says everything is fine. + +## A demo on somebody else's Mac found four things, all of them silent + +Three failures in one afternoon on a colleague's machine, plus one the fixing +uncovered. Every one produced a message that was true and useless. + +### behavision-setup chose the Python least likely to work + +`findPython` walked `3.14, 3.13, 3.12, 3.11, 3.10` and took the first hit — a +floor with **no ceiling**, which is exactly backwards. The newest Python on a +machine is the one least likely to have binary wheels for anything. It picked +3.14, pip found no numpy wheel for cp314 (`numpy<2.0` caps the resolver at +1.26.4, whose newest is cp312), fell back to building numpy from source and +produced `ERROR: Unknown compiler(s)`; once the operator had installed Xcode's +command line tools to get past that, ten minutes of compiling ended in +` is intended only for ARM and AArch64 targets`. + +Two screens of C compiler output on a shop counter, for a version choice this +program made silently. `maxMinor` refuses in one line before anything is +downloaded, and **"too new" is a different message from "too old"** — telling +somebody holding Python 3.14 that no Python was found sends them to install a +newer one, which is the direction that just failed. It is a *wheel-availability* +ceiling, not a language one: onnxruntime is the binding dependency today +(cp314 is its newest), numpy publishes further ahead, and opencv ships a +stable-ABI wheel that covers everything. + +### `numpy<2.0` was the cap; OpenCV was the hazard + +Widening to `<3.0` needed proof, and the proof found something else. Nine runs +of the detector guard per combination, one machine, one sitting: + +``` +numpy 1.26 / cv2 4.11 9 passed, 0 crashed +numpy 2.0 / cv2 4.11 8 passed, 1 crashed +numpy 1.26 / cv2 4.14 3 passed, 6 crashed +numpy 2.0 / cv2 4.14 2 passed, 7 crashed +``` + +**numpy is not the variable; OpenCV is** — the third row is numpy 1.26. The +crash was `test_a_shared_detector_really_does_race`, which races a shared +`cv2.FaceDetectorYN` on purpose to prove the per-camera rule. That is undefined +behaviour in C++: 4.11 usually turned it into an exception, 4.14 usually turns +it into a **segfault**, and 4.11 crashing once says the hazard was always there +and 4.11 merely survived it. + +It never reached the product — `Engine._build_worker` builds a detector per +camera, which is the rule and is what the second test guards. What it reached +was the suite: two runs in three died with **no failing assertion in them**, +turning "we upgraded OpenCV" into the hardest kind of CI failure to read. The +race now runs in a **subprocess**, so a segfault is an observed outcome rather +than the end of the run, and one clean attempt proves nothing — the premise +holds if *any* of several attempts misbehaves. With that fixed the suite is +226 passed / 2 skipped on numpy 2.0.2, five runs out of five. + +`opencv-python` stays capped below 5. Everything above was measured on 4.x, and +an uncapped `>=4.8.1` means every NEW install silently gets a major release +this project has never run a real camera through while every existing one keeps +4.11. + +### One MQTT client id for a whole shop, so two PCs fought over it + +`behavision--` is the same string on every computer claimed to +one site. MQTT requires client ids to be unique and a broker enforces it by +disconnecting the older session when a new one arrives with the same id, so the +colleague's Mac and the shop's own till took turns kicking each other off: + +``` +broker connected / broker connection lost: EOF / broker connected / EOF / ... +``` + +**The damage is not confined to the new machine.** The shop's till is the other +half of that loop, so somebody signing in on a laptop to look at the product +stops a live shop delivering visits — and from each end it reads as an unstable +network, because nothing says otherwise. + +`Config.MQTTClientID()` appends a per-installation id, minted on first load and +written back so an existing install gets one without anybody doing anything. +The site stays in the name because that is what a broker log is read *by*. A +config that could not be written falls back to a per-run id rather than a +shared one: the right failure is a new name in the log after a restart, not the +collision this exists to end. + +### "no such file or directory" for an engine nobody had installed + +Pressing Start with no engine went straight to the supervisor, which reported +what `exec` reported: + +``` +engine failed to start: fork/exec /private/var/folders/c2/.../AppTranslocation/ +500A5354-.../d/Behavision.app/Contents/MacOS/engine/behavision: +no such file or directory +``` + +Every word true, none of it saying *run the setup tool*. The startup path did +have that sentence — in a log file nobody on a shop counter opens. +`App.engineMissing()` is now the one function the startup path, the Start +button and the status panel all consult, so three surfaces cannot give three +accounts of one fact. + +It also names **App Translocation**, which is in that path and is unguessable. +macOS quarantines a downloaded app it cannot verify and runs it from a randomly +named read-only copy, so every relative path resolves inside that copy — which +is why the engine folder appears missing from a bundle that plainly contains +one, and why installing into it would not survive a restart. Fixed by dragging +the app to Applications; saying nothing leaves somebody re-running a setup tool +that cannot win. The product is unsigned, so this is the *normal* first-run +state on every Mac, not an edge case. diff --git a/agent/cmd/behavision-setup/main.go b/agent/cmd/behavision-setup/main.go index a8119f1..2ab5f22 100644 --- a/agent/cmd/behavision-setup/main.go +++ b/agent/cmd/behavision-setup/main.go @@ -46,6 +46,61 @@ import ( // otherwise arrive as a syntax error deep inside a dependency. const minMinor = 10 +// maxMinor is a WHEEL-availability ceiling, not a language one, and it is the +// reason this constant exists at all. +// +// findPython used to take the newest interpreter it could find, with a floor +// and no ceiling - which is precisely backwards, because the newest Python is +// the one least likely to have binary wheels for anything. Measured on a +// second Mac: it chose Python 3.14, pip found no numpy wheel for cp314, fell +// back to building numpy from source, and produced +// +// ERROR: Unknown compiler(s): [['cc'], ['gcc'], ['clang'], ...] +// +// then, once the operator installed Xcode's command line tools to get past +// that, ten minutes of compiling ending in +// +// arm_neon.h:28:2: error: " is intended only for ARM and +// AArch64 targets" +// +// Two screens of C compiler output, on a shop counter, for a version choice +// made silently by this program. Refusing in one line, before anything is +// downloaded, is the whole of the fix. +// +// Raise it when the dependency set has wheels for the next version. Today +// onnxruntime is the binding one (cp314 is its newest); numpy publishes +// further ahead, and opencv-python ships a stable-ABI wheel that covers +// everything. `pip download --only-binary=:all: -r requirements.txt` against +// a candidate interpreter is the check. +const maxMinor = 14 + +// The three answers a candidate interpreter can get. Three, not two: a +// version that is too new and one that is too old need opposite actions from +// the operator, and collapsing them tells somebody holding Python 3.14 to go +// and install a newer Python. +const ( + verdictOK = "ok" + verdictTooOld = "old" + verdictTooNew = "new" + verdictUnknown = "unparseable" +) + +func pythonVerdict(major, minor int, parsed bool) string { + switch { + case !parsed: + return verdictUnknown + case major != 3: + // Python 4 is not a version this has been tried against, and 2 is + // long gone. Neither is a thing to guess about. + return verdictTooNew + case minor < minMinor: + return verdictTooOld + case minor > maxMinor: + return verdictTooNew + } + return verdictOK +} + func main() { if err := run(); err != nil { fmt.Fprintf(os.Stderr, "\n Setup did not finish: %v\n\n", err) @@ -267,7 +322,13 @@ func findPython() (string, string, error) { // `python3` therefore told a Mac with Python 3.12 sitting on it to go and // install Python - measured on this machine, which has 3.12 under // ~/.local/opt and reported "Found, but too old: python3 3.9". - versions := []string{"3.14", "3.13", "3.12", "3.11", "3.10"} + // Newest first WITHIN the supported range. Newest overall is what broke + // this; a version nobody has built wheels for is not a better choice than + // one that works. + var versions []string + for v := maxMinor; v >= minMinor; v-- { + versions = append(versions, fmt.Sprintf("3.%d", v)) + } for _, v := range versions { cands = append(cands, cand{"python" + v, nil}) } @@ -290,7 +351,7 @@ func findPython() (string, string, error) { } } - var tried []string + var tried, tooNew []string for _, c := range cands { exe := c.exe if filepath.IsAbs(exe) { @@ -314,7 +375,18 @@ func findPython() (string, string, error) { } ver := strings.TrimSpace(string(out)) tried = append(tried, c.exe+" "+ver) - if major, minor, ok := parseVer(ver); ok && (major > 3 || (major == 3 && minor >= minMinor)) { + major, minor, parsed := parseVer(ver) + switch verdict := pythonVerdict(major, minor, parsed); verdict { + case verdictTooNew: + // Recorded separately: "too new" and "too old" need opposite + // actions, and a single "found, but unsuitable" list sends + // somebody to upgrade a Python that is already past the problem. + tooNew = append(tooNew, c.exe+" "+ver) + continue + case verdictTooOld, verdictUnknown: + continue + } + { full := exe if len(c.args) > 0 { full = exe + " " + strings.Join(c.args, " ") @@ -327,7 +399,30 @@ func findPython() (string, string, error) { // python.exe to PATH" on a Windows installer page reads as software that // does not know where it is running, which is exactly the moment somebody // stops trusting the rest of what it says. - msg := "no Python 3.10 or newer was found on this computer.\n\n" + // Only a too-new Python is a different problem with a different fix, and + // saying "no Python was found" to somebody looking at Python 3.14 is the + // kind of message that makes people stop believing the next one. + if len(tooNew) > 0 && len(tried) == 0 { + // Built as a value and wrapped, not written as an fmt.Errorf literal: + // this is a paragraph shown to an operator, and a linter that wants + // error strings to be lower-case fragments is right about errors + // programs read and wrong about the ones people do. + tooNewMsg := fmt.Sprintf( + "this computer has %s, which is newer than Behavision supports.\n\n"+ + " Some of the libraries the engine needs have no build for it\n"+ + " yet, so installing would fail part-way through.\n\n"+ + " Install Python 3.%d and run this again:\n"+ + " macOS: brew install python@3.%d\n"+ + " or https://www.python.org/downloads/macos/\n"+ + " Windows: https://www.python.org/downloads/windows/\n\n"+ + " Both versions can sit on the machine together; this picks\n"+ + " the one it can use.", + strings.Join(tooNew, ", "), maxMinor, maxMinor) + return "", "", errors.New(tooNewMsg) + } + + msg := fmt.Sprintf("no Python between 3.%d and 3.%d was found on this computer.\n\n", + minMinor, maxMinor) if runtime.GOOS == "windows" { msg += " Install it from https://www.python.org/downloads/windows/\n" + " and tick \"Add python.exe to PATH\" on the first screen,\n" + @@ -339,6 +434,9 @@ func findPython() (string, string, error) { if len(tried) > 0 { msg += "\n\n Found, but too old: " + strings.Join(tried, ", ") } + if len(tooNew) > 0 { + msg += "\n\n Found, but too new: " + strings.Join(tooNew, ", ") + } return "", "", errors.New(msg) } diff --git a/agent/cmd/behavision-setup/python_test.go b/agent/cmd/behavision-setup/python_test.go new file mode 100644 index 0000000..e2395aa --- /dev/null +++ b/agent/cmd/behavision-setup/python_test.go @@ -0,0 +1,66 @@ +package main + +import "testing" + +// The choice this program makes silently, and got wrong. +// +// findPython took the newest interpreter on the machine, with a floor and no +// ceiling - backwards, because the newest Python is the one least likely to +// have binary wheels. On a Mac holding Python 3.14 it chose 3.14, pip found +// no numpy wheel for cp314, fell back to a source build and produced two +// screens of clang errors ending in " is intended only for ARM +// and AArch64 targets". The operator's machine was fine; the version was not. +func TestTooNewIsRefusedRatherThanCompiled(t *testing.T) { + if got := pythonVerdict(3, maxMinor+1, true); got != verdictTooNew { + t.Errorf("3.%d = %q, want %q - picking it means a source build", + maxMinor+1, got, verdictTooNew) + } + if got := pythonVerdict(3, maxMinor, true); got != verdictOK { + t.Errorf("3.%d = %q, want %q - the ceiling is inclusive", maxMinor, got, verdictOK) + } +} + +// Too old and too new must stay different answers. Telling somebody holding +// Python 3.14 that no Python was found, or that theirs is too old, sends them +// to install a newer one - which is the direction that already failed. +func TestOldAndNewAreDifferentAnswers(t *testing.T) { + old := pythonVerdict(3, minMinor-1, true) + fresh := pythonVerdict(3, maxMinor+1, true) + if old == fresh { + t.Fatalf("3.%d and 3.%d both reported %q", minMinor-1, maxMinor+1, old) + } + if old != verdictTooOld { + t.Errorf("3.%d = %q, want %q", minMinor-1, old, verdictTooOld) + } +} + +// Every version in the range is accepted, so the window this program claims +// to support is the one it actually uses. +func TestTheWholeSupportedRangeIsAccepted(t *testing.T) { + for m := minMinor; m <= maxMinor; m++ { + if got := pythonVerdict(3, m, true); got != verdictOK { + t.Errorf("3.%d = %q, want %q", m, got, verdictOK) + } + } + if minMinor > maxMinor { + t.Fatal("the supported range is empty; nothing would ever be chosen") + } +} + +// A major version nobody has tested against is not something to guess at, and +// an unreadable version string is not a working interpreter. +func TestUnknownVersionsAreNotAccepted(t *testing.T) { + for _, c := range []struct { + name string + major, minor int + parsed bool + }{ + {"python 4", 4, 0, true}, + {"python 2", 2, 7, true}, + {"unparseable", 0, 0, false}, + } { + if got := pythonVerdict(c.major, c.minor, c.parsed); got == verdictOK { + t.Errorf("%s was accepted", c.name) + } + } +} diff --git a/agent/main.go b/agent/main.go index 62c5898..cb600ff 100644 --- a/agent/main.go +++ b/agent/main.go @@ -325,7 +325,7 @@ func cmdRun() error { cfg.ClientID, cfg.SiteID, cfg.BrokerURL) client, err := mqtt.NewClient(mqtt.ClientOptions{ BrokerURL: cfg.BrokerURL, - ClientID: "behavision-" + cfg.ClientID + "-" + cfg.SiteID, + ClientID: cfg.MQTTClientID(), Username: cfg.BrokerUsername, Password: cfg.BrokerPassword, CAFile: cfg.BrokerCAFile, Log: logger, }) diff --git a/agent/pkg/cameras/live.go b/agent/pkg/cameras/live.go index dec4e59..d05586a 100644 --- a/agent/pkg/cameras/live.go +++ b/agent/pkg/cameras/live.go @@ -30,12 +30,12 @@ import ( // argument, and it is why the wanted-check comes first and the push stops the // moment the server says the last viewer has gone. type Live struct { - Engine *EngineClient - Cloud *CloudClient - Log *log.Logger - FPS float64 - Width int - Quality int + Engine *EngineClient + Cloud *CloudClient + Log *log.Logger + FPS float64 + Width int + Quality int } // Defaults, measured against the office camera rather than guessed. diff --git a/agent/pkg/config/config.go b/agent/pkg/config/config.go index 92c0772..e592739 100644 --- a/agent/pkg/config/config.go +++ b/agent/pkg/config/config.go @@ -8,7 +8,9 @@ package config import ( + "crypto/rand" "encoding/base64" + "encoding/hex" "encoding/json" "fmt" "os" @@ -83,6 +85,10 @@ type Config struct { // Queue. SpoolMax int `json:"spool_max"` + // InstallID distinguishes THIS installation from every other one claimed + // to the same site. See MQTTClientID. + InstallID string `json:"install_id,omitempty"` + path string } @@ -124,6 +130,14 @@ func Load(path string) (Config, error) { return cfg, fmt.Errorf("config %s: %w", path, err) } cfg.path = path + // Minted on first load and written back, so an installation that predates + // this field gets one without anybody doing anything. Best effort: a + // read-only config still yields a working id for this run, it is simply + // not the same one next time. + if cfg.InstallID == "" { + cfg.InstallID = newInstallID() + _ = cfg.Save(path) + } for _, field := range []*string{&cfg.BrokerPassword, &cfg.APIPassword, &cfg.SessionToken, &cfg.SessionRefresh, &cfg.AgentToken} { plain, err := reveal(*field) @@ -210,3 +224,41 @@ func reveal(stored string) (string, error) { } return string(plain), nil } + +// MQTTClientID names this INSTALLATION, not this site. +// +// It was `behavision--`, which is the same string on every +// computer claimed to one shop. MQTT requires client ids to be unique and a +// broker enforces it by disconnecting the older session when a new one +// arrives with the same id - so two machines on one site take turns kicking +// each other off, forever. Measured on a second Mac claimed to a live shop: +// +// broker connected / broker connection lost: EOF / broker connected / ... +// +// The damage is not confined to the new machine. The shop's own till is the +// other half of that loop, so somebody signing in on a laptop to look at the +// product stops the shop delivering visits - and nothing at either end says +// why, because from each side it reads as an unstable network. +// +// The site stays in the id because it is what a broker log is read by, and +// the random half is short for the same reason. `CleanSession(true)` means +// there is no session state for a changed id to strand. +func (c Config) MQTTClientID() string { + id := c.InstallID + if id == "" { + // A config that could not be written still has to produce a UNIQUE + // id, or this falls straight back into the collision it exists to + // prevent. Per-run is the right failure: the connection works and the + // only cost is a new name in the broker's log after a restart. + id = newInstallID() + } + return "behavision-" + c.ClientID + "-" + c.SiteID + "-" + id +} + +func newInstallID() string { + b := make([]byte, 4) + if _, err := rand.Read(b); err != nil { + return "x" + } + return hex.EncodeToString(b) +} diff --git a/agent/pkg/config/installid_test.go b/agent/pkg/config/installid_test.go new file mode 100644 index 0000000..a60ae97 --- /dev/null +++ b/agent/pkg/config/installid_test.go @@ -0,0 +1,88 @@ +package config + +import ( + "path/filepath" + "strings" + "testing" +) + +// The bug this exists to prevent, measured on a second Mac claimed to a live +// shop: MQTT requires client ids to be unique, and a broker enforces it by +// disconnecting the older session when a new one arrives with the same id. The +// id was `behavision--` - identical on every computer claimed to +// one shop - so the two took turns kicking each other off: +// +// broker connected / broker connection lost: EOF / broker connected / ... +// +// The damage is not confined to the new machine. The shop's own till is the +// other half of that loop, so somebody signing in on a laptop to look at the +// product stops the shop delivering visits. +func TestTwoInstallsOnOneSiteGetDifferentClientIDs(t *testing.T) { + dir := t.TempDir() + one := writeClaimed(t, filepath.Join(dir, "a.json")) + two := writeClaimed(t, filepath.Join(dir, "b.json")) + + if one.MQTTClientID() == two.MQTTClientID() { + t.Fatalf("both installs answered to %q; the broker will disconnect one "+ + "whenever the other connects", one.MQTTClientID()) + } +} + +// And the same install keeps its name across restarts, or a broker log is a +// list of strangers and nobody can tell one till from a stream of new ones. +func TestOneInstallKeepsItsClientIDAcrossRestarts(t *testing.T) { + path := filepath.Join(t.TempDir(), "agent.json") + first := writeClaimed(t, path) + + again, err := Load(path) + if err != nil { + t.Fatalf("reload: %v", err) + } + if got, want := again.MQTTClientID(), first.MQTTClientID(); got != want { + t.Errorf("after a restart the id was %q, want %q", got, want) + } +} + +// The site stays in the id: it is what somebody reading a broker log is +// reading FOR, and an opaque random string would make every connection +// anonymous. +func TestTheClientIDStillNamesTheShop(t *testing.T) { + c := Config{ClientID: "tenext-retail", SiteID: "chennai", InstallID: "abcd1234"} + id := c.MQTTClientID() + for _, want := range []string{"tenext-retail", "chennai", "abcd1234"} { + if !strings.Contains(id, want) { + t.Errorf("client id %q does not contain %q", id, want) + } + } +} + +// A config that could not be written still has to produce a UNIQUE id, or a +// read-only install falls straight back into the collision. Per-run is the +// right failure: the connection works, and the only cost is a new name in the +// broker's log after a restart. +func TestAnUnsavedConfigStillGetsAUniqueID(t *testing.T) { + a := Config{ClientID: "c", SiteID: "s"} + b := Config{ClientID: "c", SiteID: "s"} + if a.MQTTClientID() == b.MQTTClientID() { + t.Fatal("two configs with no install id produced the same client id") + } +} + +func writeClaimed(t *testing.T, path string) Config { + t.Helper() + cfg := Defaults() + cfg.ClientID, cfg.SiteID = "tenext-retail", "chennai" + if err := cfg.Save(path); err != nil { + t.Fatalf("save: %v", err) + } + // Loading is what mints the id, so an installation that predates the + // field gets one without anybody doing anything. + got, err := Load(path) + if err != nil { + t.Fatalf("load: %v", err) + } + if got.InstallID == "" { + t.Fatal("loading a config without an install id did not mint one") + } + return got +} diff --git a/desktop/app.go b/desktop/app.go index 8c1f746..c7834a1 100644 --- a/desktop/app.go +++ b/desktop/app.go @@ -44,6 +44,9 @@ type App struct { broker *agentmqtt.Client stopBridge func() hookURL string + // The resolved engine command, so engineMissing() and the supervisor are + // never looking at two different paths. + engineExe string // Relays camera feeds to the webview so the engine's credential never has // to travel in an src, which a Chromium webview would strip anyway. proxy *streamProxy @@ -109,6 +112,9 @@ func (a *App) startup(ctx context.Context) { if exe != "" && !filepath.IsAbs(exe) { exe = filepath.Join(agentpaths.InstallRoot(), exe) } + a.mu.Lock() + a.engineExe = exe + a.mu.Unlock() logFile, _ := agentengine.LogFile(agentpaths.EngineLog()) a.sup = agentengine.New(agentengine.Options{ Command: func(c context.Context) *exec.Cmd { @@ -154,13 +160,55 @@ func (a *App) startup(ctx context.Context) { // not run yet, starting the supervisor would loop on a missing executable // with nothing useful to say. The Start button still exists for the one // case where somebody has deliberately stopped it. - if _, err := os.Stat(exe); err == nil { + if why := a.engineMissing(); why == "" { a.sup.Start() } else { - log.Printf("engine not installed yet (%s); run behavision-setup, then Start", exe) + log.Printf("%s (looked for %s)", why, exe) } } +// engineMissing says, in a sentence somebody can act on, why recognition +// cannot start here - or "" when it can. +// +// It exists because the answer was only ever given at startup, to a log file +// nobody on a shop counter opens. Pressing Start went straight to the +// supervisor, which reported what exec reported: +// +// engine failed to start: fork/exec /private/var/folders/c2/.../ +// AppTranslocation/500A5354-.../d/Behavision.app/Contents/MacOS/engine/ +// behavision: no such file or directory +// +// Every word of that is true and none of it says "run the setup tool". One +// function, consulted by the startup path, the Start button and the status +// panel, so the three cannot give three different accounts of one fact. +func (a *App) engineMissing() string { + a.mu.RLock() + exe := a.engineExe + a.mu.RUnlock() + if exe == "" { + return "The recognition engine is not set up on this computer yet." + } + if _, err := os.Stat(exe); err == nil { + return "" + } + msg := "The recognition engine is not installed on this computer yet. " + + "Run behavision-setup from the folder you unzipped, then press Start." + // macOS quarantines a downloaded app it cannot verify and runs it from a + // randomly named READ-ONLY copy - App Translocation. Every relative path + // then resolves inside that copy, which is why the engine folder appears + // to be missing from a bundle that plainly contains one, and why an + // install into it would not survive a restart. Detectable, unguessable, + // and fixed by one drag; saying nothing leaves somebody re-running a + // setup tool that cannot win. + if strings.Contains(exe, "/AppTranslocation/") { + msg = "macOS is running Behavision from a temporary read-only copy, " + + "because it was opened straight from Downloads. Move Behavision " + + "to your Applications folder and open it from there, then run " + + "behavision-setup." + } + return msg +} + // webhookURL is the loopback address the bridge is listening on, or empty // before it has started. func (a *App) webhookURL() string { @@ -242,7 +290,7 @@ func (a *App) startPipeline(ctx context.Context) { } client, err := agentmqtt.NewClient(agentmqtt.ClientOptions{ BrokerURL: a.cfg.BrokerURL, - ClientID: "behavision-" + a.cfg.ClientID + "-" + a.cfg.SiteID, + ClientID: a.cfg.MQTTClientID(), Username: a.cfg.BrokerUsername, Password: a.cfg.BrokerPassword, CAFile: a.cfg.BrokerCAFile, Log: logger, }) @@ -595,6 +643,11 @@ func (a *App) EngineStatus() EngineStatus { if err != nil { out.Error = err.Error() } + // The supervisor's own error is an exec failure; this replaces it with + // the reason, which is the part that tells somebody what to do. + if why := a.engineMissing(); why != "" { + out.Error = why + } ctx, cancel := context.WithTimeout(a.ctx, 4*time.Second) defer cancel() // A running process is not a working engine: on a memory-starved box the @@ -609,6 +662,13 @@ func (a *App) EngineStatus() EngineStatus { } func (a *App) StartEngine() EngineStatus { + // Refused rather than attempted. Handing a missing path to the supervisor + // produces a retry loop and an exec error for a message. + if why := a.engineMissing(); why != "" { + st := a.EngineStatus() + st.Error = why + return st + } if a.sup != nil { a.sup.Start() } diff --git a/pyproject.toml b/pyproject.toml index 08b251e..7cc1720 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,8 +4,9 @@ version = "1.1.0" description = "Production face recognition over RTSP" requires-python = ">=3.10" dependencies = [ - "numpy>=1.26,<2.0", - "opencv-python>=4.8.1", + # See requirements.txt for why numpy is uncapped and opencv is not. + "numpy>=1.26,<3.0", + "opencv-python>=4.8.1,<5", "onnxruntime>=1.16", "fastapi>=0.110", "uvicorn>=0.29", diff --git a/requirements.txt b/requirements.txt index 6551568..b2b72c6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,31 @@ -numpy>=1.26,<2.0 -opencv-python>=4.8.1 +# numpy 2 is allowed, and that is what lets this install on a current Python. +# `<2.0` capped the resolver at numpy 1.26.4, whose newest wheel is cp312, so +# on a Mac with Python 3.14 pip fell back to BUILDING numpy from source and +# died in clang - two screens of C compiler output on a shop counter, for a +# version choice made silently by behavision-setup. +# +# Measured before changing it, nine runs of the detector guard per combination +# on one machine: +# +# numpy 1.26 / cv2 4.11 9 passed, 0 crashed +# numpy 2.0 / cv2 4.11 8 passed, 1 crashed +# numpy 1.26 / cv2 4.14 3 passed, 6 crashed +# numpy 2.0 / cv2 4.14 2 passed, 7 crashed +# +# numpy is not the variable there; OpenCV is. The crash was a test racing a +# shared cv2.FaceDetectorYN on purpose - undefined behaviour in C++, which 4.11 +# usually turned into an exception and 4.14 usually turns into a segfault. The +# product never shares one (Engine._build_worker builds a detector per camera), +# and the test now runs that race out of process. With that fixed, the whole +# suite is 226 passed / 2 skipped on numpy 2.0.2, five runs out of five. +# +# opencv IS capped, and the two are not the same call. Everything above was +# measured on 4.x; OpenCV 5.0 is a major release this project has never run a +# real camera or an emotion model through, and an uncapped `>=4.8.1` means +# every NEW install silently gets it while every existing one keeps 4.11. Lift +# it after running a camera on 5.x, not before. +numpy>=1.26,<3.0 +opencv-python>=4.8.1,<5 onnxruntime>=1.16 fastapi>=0.110 uvicorn>=0.29 diff --git a/tests/test_detector_concurrency.py b/tests/test_detector_concurrency.py index dd11659..9e31612 100644 --- a/tests/test_detector_concurrency.py +++ b/tests/test_detector_concurrency.py @@ -1,7 +1,32 @@ """cv2.FaceDetectorYN caches its input size and is not thread-safe, so camera workers must not share one. Skipped when the model is absent, matching the faiss-optional pattern in test_index.py — the suite stays runnable with no -models installed.""" +models installed. + +The premise is proved in a SUBPROCESS, and that is the whole lesson of this +file. The first version raced a shared detector in-process and asserted that +an exception came back, because on OpenCV 4.11 one usually did. It is a data +race in C++: what it produces is undefined, and on 4.14 what it mostly +produces is a segmentation fault. Measured, nine runs each, same machine: + + numpy 1.26 / cv2 4.11 9 passed, 0 crashed + numpy 2.0 / cv2 4.11 8 passed, 1 crashed + numpy 1.26 / cv2 4.14 3 passed, 6 crashed + numpy 2.0 / cv2 4.14 2 passed, 7 crashed + +numpy is not the variable; OpenCV is, and 4.11 crashing once says the hazard +was always there and 4.11 merely survived it. A test that takes the whole +suite down two runs in three is worse than no test: it turns "we upgraded +OpenCV" into a CI failure with no failing assertion in it, which is the +hardest kind to read. + +None of this reaches the product. `Engine._build_worker` constructs a +FaceDetector per camera, which is what the rule says and what the second test +here guards. +""" +import subprocess +import sys +import textwrap import threading from pathlib import Path @@ -11,10 +36,16 @@ import pytest from behavision.config import Config from behavision.detection import YUNET_FILENAME, FaceDetector -MODELS = Path(__file__).resolve().parent.parent / "models" +ROOT = Path(__file__).resolve().parent.parent +MODELS = ROOT / "models" pytestmark = pytest.mark.skipif(not (MODELS / YUNET_FILENAME).exists(), reason="YuNet model not installed") +# A shared detector fails probabilistically, so one clean attempt proves +# nothing. Several do: the premise holds if ANY attempt misbehaves, and only +# an unbroken run of clean ones is evidence it has stopped being true. +ATTEMPTS = 6 + def _detector(): d = Config().detection @@ -44,11 +75,37 @@ def _race(det_a, det_b): return errors +_SHARED_RACE = textwrap.dedent(""" + import sys + sys.path.insert(0, {root!r}) + from tests.test_detector_concurrency import _detector, _race + shared = _detector() + print("raced" if _race(shared, shared) else "clean") +""") + + +def _shared_race_outcome(): + """Run one shared-detector race out of process. + + Returns "raced" (an exception came back), "crashed" (the process died, + which is the same premise arriving by a blunter route) or "clean". + """ + proc = subprocess.run([sys.executable, "-c", _SHARED_RACE.format(root=str(ROOT))], + capture_output=True, text=True, timeout=120) + if proc.returncode != 0: + return "crashed" + return proc.stdout.strip().splitlines()[-1] if proc.stdout.strip() else "clean" + + def test_a_shared_detector_really_does_race(): """Guards the premise: if this ever stops failing, the test below is proving nothing and the per-camera split can be revisited.""" - shared = _detector() - assert _race(shared, shared), "expected a shared detector to race" + seen = [_shared_race_outcome() for _ in range(ATTEMPTS)] + assert any(o != "clean" for o in seen), ( + f"a shared detector survived {ATTEMPTS} races ({seen}) - if that is " + "reproducible, cv2.FaceDetectorYN may have become thread-safe and the " + "per-camera rule in CLAUDE.md can be revisited" + ) def test_per_camera_detectors_do_not_race():