From 066a19052bde6e721a6d1e13a401a7b25634b35a Mon Sep 17 00:00:00 2001 From: cproudlock Date: Wed, 16 Sep 2026 08:57:04 -0400 Subject: [PATCH] Act on the wj architectural review: races, limits, pool and contract gaps Findings F2, F4, F5, F6, F7, F8, F9, F10, F11 and F13 from wj-architectural-review.md. F1/F1a (the non-sargable LOWER pattern) and F3 (the warranty sync holding a pooled connection) are being handled separately and are untouched here. - A collector check-in that raced another for the same PC lost everything: the unique index on the application fingerprint raised, nothing caught it, and the whole check-in rolled back, discarding state, events and hardware, answering the agent with form copy meant for a person. The insert now runs in a savepoint, reconciles what the other transaction already wrote, one row per savepoint so a second collision skips a row rather than the batch, and the check-in commits with a warning. This is the shape the delete side of the same function already had. - Enrollment is deliberately unauthenticated, so it now carries a per-address rate limit and a cap on pending requests, and the cap is a setting an admin can see and change. A flood can no longer bury the approval queue or grow the table without bound. - The fleet alert poll no longer formats debug strings on every request. - BusinessUnit is on the contract surface (0.34.0), so the endpoints plugin stops reaching into a core table with raw SQL. - A test walks every asset type descriptor and proves the endpoint it names really refuses a caller without the declared permission. - The plugin settings routes now need a permission; they were telling anyone which modules a site runs and where its components live. - The connection pool is sized explicitly with a short timeout, and the arithmetic against the server's connection ceiling is written next to the worker count instead of being left to defaults nobody chose. - Fleet alerts carry both the correct lowercase keys and the old camelCase pair. Trays older than 1.7.0 match keys case-sensitively and would have gone quiet on every PC not yet upgraded; the old pair goes when they are. - The dev server proxy points at localhost again, with environment variables for the remote-box workflow, and one developer's address is out of the server certificate page. --- .env.example | 13 ++ CONTRIBUTING.md | 37 +++ .../FleetAlertClient.cs | 8 +- docker-compose.yml | 25 ++ docs/deployment/LOCAL-DEV-SETUP.md | 10 + .../src/views/settings/ModelCatalogHub.vue | 6 +- frontend/vite.config.js | 28 ++- plugins/endpoints/AGENT-CONTRACT.md | 30 +++ plugins/endpoints/api/bootstrap.py | 35 ++- plugins/endpoints/api/enrollment.py | 117 +++++++++- plugins/endpoints/api/ratelimit.py | 52 +++++ plugins/endpoints/api/routes.py | 56 ++--- .../frontend/views/EnrollmentSettings.vue | 59 ++++- .../frontend/views/ServerCertSettings.vue | 19 +- .../frontend/views/enrollmentSettings.spec.js | 19 +- plugins/endpoints/manifest.json | 2 +- plugins/endpoints/plugin.py | 119 +++++++++- shopdb/__init__.py | 11 +- shopdb/api/__init__.py | 2 + shopdb/config.py | 42 ++++ shopdb/core/api/plugins.py | 12 +- shopdb/plugins/base.py | 15 +- .../test_asset_type_descriptor_enforcement.py | 79 +++++++ .../test_core/test_plugin_settings_routes.py | 25 +- tests/test_core/test_public_read_surface.py | 11 +- .../test_endpoints_app_conflict.py | 165 +++++++++++++ .../test_endpoints_credentials.py | 6 +- .../test_endpoints_enroll_limits.py | 220 ++++++++++++++++++ .../test_endpoints_fleet_alert_scheduling.py | 6 +- .../test_endpoints_fleet_alerts.py | 34 ++- 30 files changed, 1149 insertions(+), 114 deletions(-) create mode 100644 plugins/endpoints/api/ratelimit.py create mode 100644 tests/test_core/test_asset_type_descriptor_enforcement.py create mode 100644 tests/test_plugins/test_endpoints_app_conflict.py create mode 100644 tests/test_plugins/test_endpoints_enroll_limits.py diff --git a/.env.example b/.env.example index 891adb9a..649c0102 100644 --- a/.env.example +++ b/.env.example @@ -28,6 +28,19 @@ JWT_SECRET_KEY=change-this-to-another-secure-random-string # utf8mb4 too (see the DEPLOY wiki page). Both must be utf8mb4 to match the schema. DATABASE_URL=mysql+pymysql://shopdb:CHANGE_ME@db:3306/shopdb_flask?charset=utf8mb4 +# ---- Database connection pool (optional, defaults shown) ---- +# Per WORKER process, so the real ceiling is workers x (size + overflow). See +# the arithmetic beside --workers in docker-compose.yml before raising either: +# at the shipped 4 workers this is 4 x (10 + 10) = 80 against the mysql:8.4 +# default max_connections of 151. More workers needs a smaller pool here or a +# higher --max-connections on the db service. +# DB_POOL_TIMEOUT is the wait for a free connection. 5s fails fast and +# legibly; the SQLAlchemy default of 30s turns pool exhaustion into what looks +# like a dead database. +DB_POOL_SIZE=10 +DB_MAX_OVERFLOW=10 +DB_POOL_TIMEOUT=5 + # ---- CORS (required, no wildcards in production) ---- # Comma-separated list of explicit origins permitted to call the API. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5fddca00..26a0d944 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -156,6 +156,43 @@ When creating a new plugin: See the PLUGIN-QUICKSTART wiki page (when published) for the 30-minute walkthrough. +#### A plugin must not hold a DB session across external I/O + +Rule: finish with the database before you call anything outside the process - +an HTTP API (Dell warranty, ServiceNow, Zabbix, Azure/Intune), a subprocess, a +file on a network share - and open a fresh session afterwards to write the +result. + +Why it is a rule and not a preference. The app runs gevent workers, so a +request that blocks on a network call does not block its worker, and the call +can take minutes without anything complaining. What it DOES hold for those +minutes is a pooled database connection. Each worker owns a pool of +`DB_POOL_SIZE + DB_MAX_OVERFLOW` connections (see `shopdb/config.py` and the +arithmetic beside `--workers` in `docker-compose.yml`), and hundreds of +greenlets can enter the same slow path at once. Drain the pool and every other +request in that worker blocks on checkout and then fails - the whole site, +every plugin, while the CPU sits idle and MySQL reports itself healthy. That is +the worst diagnostic signature there is. + +The shape to write: + +```python +# Read what you need, then let the session go. +servicetags = [row.servicetag for row in query.all()] +db.session.remove() + +results = provider.bulk_lookup(servicetags) # slow, no connection held + +for servicetag, result in results.items(): # new session, short write + ... +db.session.commit() +``` + +Rules of thumb: no `requests`, `urllib`, `subprocess` or network-share access +between a query and its `commit()`; cap every external call with a timeout; and +if a sync is inherently long, chunk it so each chunk is its own +read / call / write cycle. + ## Testing Run tests with: diff --git a/agent/ShopDbEndpointAgent.Core/FleetAlertClient.cs b/agent/ShopDbEndpointAgent.Core/FleetAlertClient.cs index 7f047367..10672c0e 100644 --- a/agent/ShopDbEndpointAgent.Core/FleetAlertClient.cs +++ b/agent/ShopDbEndpointAgent.Core/FleetAlertClient.cs @@ -99,9 +99,9 @@ public sealed class FleetAlert private string title = ""; private string message = ""; - [JsonPropertyName("notificationId")] public int NotificationId { get; set; } + [JsonPropertyName("notificationid")] public int NotificationId { get; set; } [JsonPropertyName("type")] public string TypeName { get => typeName; set => typeName = value ?? ""; } - [JsonPropertyName("typeColor")] public string TypeColor { get => typeColor; set => typeColor = value ?? ""; } + [JsonPropertyName("typecolor")] public string TypeColor { get => typeColor; set => typeColor = value ?? ""; } [JsonPropertyName("title")] public string Title { get => title; set => title = value ?? ""; } [JsonPropertyName("message")] public string Message { get => message; set => message = value ?? ""; } [JsonPropertyName("timeout")] public int? Timeout { get; set; } @@ -126,7 +126,7 @@ public sealed class FleetAlertList : List } // Deserializes the data array one element at a time. An element that fails -// to bind, or has no usable notificationId, is recorded and skipped. +// to bind, or has no usable notificationid, is recorded and skipped. public sealed class TolerantFleetAlertListConverter : JsonConverter> { public override List? Read( @@ -146,7 +146,7 @@ public sealed class TolerantFleetAlertListConverter : JsonConverter(options); if (alert is null || alert.NotificationId <= 0) - list.Skipped.Add($"element {index}: missing notificationId"); + list.Skipped.Add($"element {index}: missing notificationid"); else list.Add(alert); } diff --git a/docker-compose.yml b/docker-compose.yml index 0fefd312..5407a3c4 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -167,6 +167,31 @@ services: # connections, is the binding resource. - --worker-class - gevent + # DATABASE CONNECTION ARITHMETIC. Read this before changing the number. + # + # workers x (pool_size + max_overflow) + migrate + admin vs MySQL + # max_connections + # + # Each worker process owns its own SQLAlchemy pool, so the ceiling is per + # worker multiplied by the worker count. With the defaults in + # shopdb/config.py (DB_POOL_SIZE 10, DB_MAX_OVERFLOW 10): + # + # 4 x (10 + 10) = 80 + # + the migrate service (a handful, and it has exited before api + # starts, so it only overlaps during a manual upgrade) + # + whatever a human has open (mysqldump, a client on the host) + # = about 90 against the mysql:8.4 default max_connections of 151. + # + # The `command:` on the db service above sets charset and collation only, + # so that 151 is the server default and nothing else in this repo relates + # the two numbers. Raise --workers to 8 and the ceiling is 160 before any + # overhead, which is ABOVE 151: MySQL then refuses connections, and the + # failure shows up as errors from the database rather than anything in the + # app. So change one of these and change the others deliberately: + # more workers means either a smaller pool or --max-connections on db. + # + # pool_timeout is 5s, not the 30s default, so exhaustion fails fast and + # legibly instead of hanging every request for half a minute. - --workers - "4" # Per-worker greenlet ceiling. 4 x 2000 is far above any realistic fleet; diff --git a/docs/deployment/LOCAL-DEV-SETUP.md b/docs/deployment/LOCAL-DEV-SETUP.md index 685f411f..a1e4c190 100644 --- a/docs/deployment/LOCAL-DEV-SETUP.md +++ b/docs/deployment/LOCAL-DEV-SETUP.md @@ -189,6 +189,16 @@ npm run dev Frontend dev server runs on http://localhost:5173 and proxies API requests to Flask on port 5001. +To point the proxy at a backend on another machine, set `VITE_DEV_API_TARGET` +in your shell instead of editing `vite.config.js`: + +```bash +VITE_DEV_API_TARGET=https://10.0.0.5:5001 npm run dev +``` + +Certificate verification is off for an https target because those boxes +normally use a self-signed cert; set `VITE_DEV_SECURE=true` to enforce it. + For production build (served by Flask): ```bash cd frontend diff --git a/frontend/src/views/settings/ModelCatalogHub.vue b/frontend/src/views/settings/ModelCatalogHub.vue index 53d31c04..6b0debde 100644 --- a/frontend/src/views/settings/ModelCatalogHub.vue +++ b/frontend/src/views/settings/ModelCatalogHub.vue @@ -48,7 +48,9 @@