Let an extension run inside the supervisor - #33
Conversation
Sorry, something went wrong.
An extension gem that needs a long running loop had to ship its own executable, so an operator ran and monitored a second process for work that belongs to the same runtime. Two delivery paths where one would do is two things to watch, and the second one is usually the one nobody watches. The configuration now takes a component registration. The supervisor runs a registered component beside the workers, the effect executors, the broadcast executors, and the reminder schedulers, under the same supervision, replacement, and shutdown timeout. The registration builds one instance at once and checks that it answers run, request_shutdown, stopped?, and stop. A missing method raises while the application boots, instead of hanging a shutdown later. Replacement now goes through the builder that made the component. The supervisor called component.class.new, which discards every constructor argument, so a component built with arguments came back with its defaults after a crash. A component placed by other means keeps the old behavior.
Greptile SummaryThe PR adds extension-defined supervisor components, validates their lifecycle contract during supervisor construction, preserves their factories for replacement, and cleans up components after partial initialization failures.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Register component factory] --> B[Supervisor collects builders]
B --> C[Build component]
C --> D[Validate lifecycle contract]
D -->|Valid| E[Run under supervisor]
E -->|Unexpected exit| C
C -->|Build failure| F[Stop components already built]
D -->|Contract failure| F
Reviews (3): Last reviewed commit: "fix: stop the components already built w..." | Re-trigger Greptile |
Sorry, something went wrong.
| raise ArgumentError, "register_component requires a block" unless factory | ||
| raise ArgumentError, "count must be positive" unless count.positive? | ||
|
|
||
| validate_component!(factory.call) |
There was a problem hiding this comment.
Validation instance escapes lifecycle cleanup
If an extension factory acquires resources or registers externally visible state during construction, register_component creates and discards a validation instance without invoking its lifecycle methods, leaving process rows, connections, threads, sockets, or callbacks behind before the supervisor starts.
Prompt To Fix With AIThis is a comment left during a code review.
Path: lib/solid_objects/configuration.rb
Line: 166
Comment:
**Validation instance escapes lifecycle cleanup**
If an extension factory acquires resources or registers externally visible state during construction, `register_component` creates and discards a validation instance without invoking its lifecycle methods, leaving process rows, connections, threads, sockets, or callbacks behind before the supervisor starts.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Sorry, something went wrong.
Registration built one instance to check the contract, then dropped it. That instance is never supervised and never stopped, so a constructor that opens a connection, starts a thread, or registers itself somewhere durable leaked one of each on every boot. It also forced a component to be constructible while the application boots, which a component that reaches the database cannot promise. The supervisor checks the contract on the instance it is about to run, so the check covers the object that matters and builds nothing extra.
|
Addressed the 4/5 blocker. Registration no longer constructs anything. register_component stores the factory and returns; the supervisor validates the contract on the instance it is about to run, in build_component. That removes the abandoned instance you flagged. It also removes a constraint I had not intended: the old code required a component to be constructible while the application boots, which SolidObjectsPro::FlushEngine cannot promise because it reaches the database. Building the Pro side is what surfaced it. New test asserts registration builds nothing, and the contract tests now assert at build time. 406 runs, 0 failures; standard, rubocop, rbs, and steep all pass. |
Sorry, something went wrong.
| broadcast_worker_count:, | ||
| reminder_scheduler_count: | ||
| ) | ||
| @components = @builders.map(&:call) |
There was a problem hiding this comment.
Partial initialization leaks components
When a registered factory raises or returns an invalid component after earlier components have been constructed, @builders.map(&:call) aborts initialization without calling stop on those earlier components, leaving their process records, connections, threads, or extension-owned state behind.
Prompt To Fix With AIThis is a comment left during a code review.
Path: lib/solid_objects/supervisor.rb
Line: 28
Comment:
**Partial initialization leaks components**
When a registered factory raises or returns an invalid component after earlier components have been constructed, `@builders.map(&:call)` aborts initialization without calling `stop` on those earlier components, leaving their process records, connections, threads, or extension-owned state behind.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Sorry, something went wrong.
The supervisor builds its components one after another. A factory that raised, or a component that failed the contract check, left every earlier component constructed and unreachable. Whatever those constructors took was never given back, and the process that failed to start kept holding it. Each already built component now receives stop. A failure inside that cleanup is instrumented rather than raised, so the failure that stopped the build is still the one the caller sees. The contract check moved into the same loop, after the component joins the list, so a component that fails the check is stopped with the rest.
|
Addressed the partial-initialization finding. Supervisor#build_all now stops every component it already built when a later builder raises or a later component fails the contract check, then re-raises. A failure inside that cleanup is instrumented as supervisor.component_cleanup_failed rather than raised, so the failure that stopped the build stays the one the caller sees. The contract check moved into the same loop, after the component joins the list, so a component that fails the check is stopped alongside the earlier ones rather than dropped on its own. Three tests cover it: a later factory that raises, a later component that breaks the contract, and a component whose stop itself raises (asserting the original error survives). 409 runs, 0 failures; standard, rubocop, rbs, and steep all pass. @greptileai review |
Sorry, something went wrong.
Why
An extension gem that needs a long-running loop has to ship its own executable, so an operator runs and monitors a second process for work that belongs to the same runtime. Two delivery paths where one would do means two things to watch, and the second is usually the one nobody watches.
Solid Objects Pro is the immediate case: bundle exec solid_objects_pro start exists only to run flush engines that are already thread-shaped and already share the connection pool this gem requires.
What changed
SolidObjects.configuration.register_component takes a block that builds a component. The supervisor runs it beside the workers, effect executors, broadcast executors, and reminder schedulers, under the same supervision, replacement, and shutdown timeout.
The registration builds one instance immediately and checks it answers run, request_shutdown, stopped?, and stop — the contract the built-in roles already keep. A missing method raises ArgumentError while the application boots, rather than hanging a shutdown much later.
A bug found on the way
replace_dead_roles rebuilt a crashed component with component.class.new, which discards every constructor argument. A component built with arguments came back with its defaults after a crash, silently. Each component now keeps the builder that made it, and replacement calls that builder.
The built-in roles take no constructor arguments, so their behavior is unchanged. A component placed into @components by other means has no builder and keeps the old class.new path, which is what the existing SupervisorReplacementTest injection relies on.
Tests
Full rake default passes: 405 runs, 0 failures, plus standard, rubocop, rbs, and steep.
Version
Minor bump to 0.11.0. The change is additive, and the replacement fix only affects components that were already being rebuilt incorrectly.