fix: await object init outcome before reporting add/update success - #90
fix: await object init outcome before reporting add/update success#90ygd58 wants to merge 1 commit into
Conversation
…enlayerlabs#80) update_object_config's docs promise "a rejected init/1 rolls back to the old config" — that only held for SYNCHRONOUS rejections (the config_schema immutable-key gate). A handler whose init/1 returns {:error, _} was not caught: - do_update_object_config's remove+re-add calls do_add_object, which calls ObjectSupervisor.start_object. - start_object (and ObjectServer's own GenServer init/1) returns {:ok, pid} as soon as the PROCESS spawns. The handler's real init/1 runs afterward, asynchronously, via a deferred `send(self(), :init_object)` — so a slow or rejecting init/1 hadn't even run yet when start_object returned. - do_add_object treated {:ok, pid} as "config accepted" and reported success. The async init then failed, the object landed in :error state (handler_state nil), but the PATCH/POST already reported "updated"/ "added" and the overlay/snapshot recorded the new config — the object was silently dead while everything else said it was fine. Same bug for add_object (issue: "an added object whose init rejects currently reports :added"). Fix: added ObjectServer.await_init/3, a handshake callers can use to learn the REAL init outcome instead of trusting start_object's {:ok, pid}. - New init_waiters/init_error fields on the ObjectServer struct. - await_init/3 does a GenServer.call(:await_init); if init already resolved it replies immediately (:ok or {:error, init_error}), otherwise it queues the caller and a new resolve_init/2 helper — wired into every place both native-mode (handler.init/1) and process-mode (backend start) init can resolve, success or failure — releases all queued callers with the right answer once it does. - do_add_object now calls await_init right after start_object succeeds. On {:error, reason} it stops the half-initialized object, rolls back the topology edges, and returns {:error, {:object_init_failed, reason}} — same failure shape as any other add_object failure branch, so it's already handled by do_update_object_config's existing rollback-to-old- spec logic without touching that function. Fixes BOTH add_object and update_object_config's rollback from a single change point, since both go through do_add_object. - Added client-facing messages in swarm_controller.ex's format_error/1 for the new error shapes (object_init_failed, config_rejected, config_rejected_and_rollback_failed) instead of falling through to the generic "Internal error". Testing: Added tests/genswarms/objects/object_server_await_init_test.exs (unit, starts an ObjectServer directly) covering: successful init, rejected init, a slow init (asserts await_init actually blocks — elapsed time check, not just a return value), a caller asking AFTER resolution (via init_error, not just the waiter queue), and 5 concurrent callers all released with the same correct outcome. Added tests/genswarms_swarm_manager_object_init_reject_test.exs (integration, via SwarmManager) reproducing the issue's exact repro: an object handler whose init/1 rejects config[:bad] == true — PATCH is now correctly refused and the object stays alive on its old config; POST add_object is correctly refused and nothing is left registered. Also covers the non-rejecting path for both (no false-positive rejections). Couldn't run these through `mix test` — this sandbox's network policy has no route to hex.pm to fetch deps (phoenix, bandit, jason, telemetry, ... per mix.exs). Instead: syntax-checked all changed/new files via Code.string_to_quoted, then actually exercised the real, modified ObjectServer source standalone via Code.require_file (with a Registry started manually and a one-line :telemetry stub, since that's the only external dep the exercised code paths touch) — all 5 await_init unit scenarios pass against the fix and I confirmed they fail against pre-fix code (await_init/2 is undefined). Couldn't run the SwarmManager integration test the same way (too many interlinked GenServers/Registries to hand-bootstrap safely), so that file is syntax-checked only pending CI. Flagging this explicitly rather than overclaiming local coverage.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Fixes #80 —
update_object_config(andadd_object) reported success even when the object handler'sinit/1asynchronously rejected the config.Root cause
do_add_objecttreatsObjectSupervisor.start_object's{:ok, pid}as proof the config was accepted. But that only means the process spawned —ObjectServer.init/1defers the handler's realinit/1call viasend(self(), :init_object), which runs afterward, asynchronously. A rejecting (or slow)init/1hadn't even run yet whenstart_objectreturned.Net effect:
do_update_object_config's remove+re-add reported"updated", the overlay/snapshot recorded the new config, but the async init then failed and the object landed in:errorstate (handler_statenil) — silently dead while everything else said it was fine. Same bug foradd_object(issue: "an added object whose init rejects currently reports:added").Fix
Added
ObjectServer.await_init/3— a handshake callers use to learn the real init outcome instead of trustingstart_object's{:ok, pid}:init_waiters/init_errorfields on theObjectServerstruct.await_init/3does aGenServer.call(:await_init): replies immediately if init already resolved (:okor{:error, init_error}), otherwise queues the caller.resolve_init/2helper, wired into every place both native-mode (handler.init/1) and process-mode (backend start) init can resolve — releases all queued callers with the right answer.do_add_objectnow callsawait_initright afterstart_objectsucceeds. On failure it stops the half-initialized object, rolls back the topology edges, and returns{:error, {:object_init_failed, reason}}— the same failure shape as any otherdo_add_objectfailure branch, sodo_update_object_config's existing rollback-to-old-spec logic handles it without any changes to that function. This single change point fixes bothadd_objectandupdate_object_config's rollback, since both go throughdo_add_object.swarm_controller.ex'sformat_error/1for the new error shapes instead of falling through to generic "Internal error".Testing
test/genswarms/objects/object_server_await_init_test.exs(unit, starts anObjectServerdirectly): successful init, rejected init, a slow init (assertsawait_initactually blocks — elapsed-time check, not just a return value), a caller asking after resolution (viainit_error), and 5 concurrent callers all released with the same outcome.test/genswarms/swarm_manager_object_init_reject_test.exs(integration, viaSwarmManager): reproduces the issue's exact repro — a handler rejectingconfig[:bad] == true.PATCHis now correctly refused and the object stays alive on its old config;POST add_objectis correctly refused and nothing is left registered. Also covers the non-rejecting path for both, to guard against false-positive rejections.I could not run these through
mix test— this sandbox's network policy has no route tohex.pmto fetch deps (phoenix,bandit,jason,telemetry, ... permix.exs). Instead:Code.string_to_quoted.ObjectServersource standalone viaCode.require_file(Registry started manually, one-line:telemetrystub since that's the only external dep the exercised paths touch) — all 5await_initunit scenarios pass against the fix, and I confirmed they fail against the pre-fix code (await_init/2 is undefined).SwarmManagerintegration test the same way (too many interlinked GenServers/Registries to hand-wire safely), so that file is syntax-checked only pending CI.Flagging the coverage gap explicitly rather than overclaiming local test results — CI will be the first full run of the integration test.