Let an extension run inside the supervisor - #33
Conversation
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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.
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 AI
This 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.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. 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 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. |
|
@greptileai review |
| broadcast_worker_count:, | ||
| reminder_scheduler_count: | ||
| ) | ||
| @components = @builders.map(&:call) |
There was a problem hiding this comment.
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 AI
This 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.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.
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 409 runs, 0 failures; standard, rubocop, rbs, and steep all pass. @greptileai review |
1 participant
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 startexists only to run flush engines that are already thread-shaped and already share the connection pool this gem requires.What changed
SolidObjects.configuration.register_componenttakes 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?, andstop— the contract the built-in roles already keep. A missing method raisesArgumentErrorwhile the application boots, rather than hanging a shutdown much later.A bug found on the way
replace_dead_rolesrebuilt a crashed component withcomponent.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
@componentsby other means has no builder and keeps the oldclass.newpath, which is what the existingSupervisorReplacementTestinjection relies on.Tests
test/unit/component_registry_test.rb— registration, per-supervisor instances,count:, and each contract violationtest/integration/supervisor_additional_components_test.rb— a registered component runs, shuts down, leaves the built-in process rows unchanged, honorscount:, and is replaced through its own factory after a crashFull
rakedefault 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.