Conversation
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
The PR adds a build lifecycle coordinator that permits, queues, watchdogs, and drains Graphile schema builds, then threads the shared coordinator through the cache, middleware, and server so builds are fenced before shutdown and bounded under load. A single high-confidence issue surfaced: a routine disposal failure now latches the coordinator into a permanent
Reviewed commit: 51f923f |
There was a problem hiding this comment.
This change introduces a GraphileBuildCoordinator that serializes, bounds, and drains Graphile schema builds across the graphile-cache, GraphQL server, and explorer, with a new error-code registry and env config plumbing.
Key findings
- 🟡 Disposal failure permanently bricks build coordination — graphile-cache.ts:276
Implements F21 from constructive-io/constructive-planning#1693, stacked on F20 (#1854) and F19 (#1853), above F17 (#1852) and F04 (#1747).
Concurrent distinct schema misses now enter one process-wide FIFO coordinator before capacity admission. One active permit covers eviction waits, preset allocation, schema/readiness work, publication, and cleanup; the waiting queue defaults to 16. Exact-key joins consume no additional slot. A synchronous admission check rejects overflow before allocating flight records, including a burst of 100 distinct calls in one JavaScript turn.
The watchdog (default 300000 ms) fences publication and promptly rejects active/queued callers while retaining the permit and any reservation until actual work and cleanup settle. Watchdog expiry, failed cleanup, and drain timeout permanently latch the coordinator unavailable. JavaScript work cannot be cancelled safely here; recovery requires process restart. A disappearing cache/disposal record does not prove a failed release reclaimed its resources.
Shutdown fences builds before HTTP close waits on those callers. One configurable deadline (default 30000 ms) covers actual coordinator work and flight settlement. A false drain result becomes a canonical timeout error, and pools are not closed. Once actual builds drain, resident cleanup failures remain observable and pool cleanup is still attempted; failures from both owners are aggregated. Healthy closed coordinators reopen only after the prior tasks and preparation scopes drain. Full request/WebSocket retirement remains outside this build-coordination slice.
Typed graphile.build options and strict GRAPHILE_BUILD_* environment overrides expose queueMax/watchdogMs/shutdownTimeoutMs. Node timer overflow values are rejected; absent overrides preserve configuration and do not claim explicit policy ownership.
The stack reuses the request-context owner for API/pool/transaction settings and the canonical error owner for causes, codes, and HTTP status. Build refusals retain their 503 response; unknown failures expose a generic 500. Cache environment parsing and precedence belong to graphql-env, and the existing LRU enforces the resolved runtime limits.
Validation on the updated stack:
Extended performance stress tests were not repeated in this follow-up; #1747 records the earlier measurements and upstream subscriber-listener limitation. The changes preserve public release boundaries and do not claim immediate completion of upstream background cleanup.
All newly authored commits use the requested personal Git author and committer identity. Existing PR branches are updated with fast-forward merges and retain the dependency stack.