From bea9d634545cc203fdcce11aff986efb27098048 Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Sun, 21 Jun 2026 17:08:36 +0200 Subject: [PATCH] fix: supervise the cowboy listener instead of starting it as a side effect nova_sup started cowboy via cowboy:start_clear/start_tls inside init/1 and discarded the result. A failed bind (e.g. eaddrinuse) was only logged, so application:start(nova) still returned ok with no working listener and requests silently hit whatever already held the port. Build the listener with ranch:child_spec/5 and put it in the supervision tree. A bind failure now fails init/1 and surfaces through application:start/1, and the listener is restart-managed like any other child. The option transforms mirror cowboy:start_clear/start_tls (2.15) so buffer/connection_type behaviour is identical. Adds nova_sup_tests covering the child-spec shape and that a busy port fails the start instead of being swallowed. --- src/nova_sup.erl | 124 ++++++++++++++++++++++------------------ test/nova_sup_tests.erl | 50 ++++++++++++++++ 2 files changed, 118 insertions(+), 56 deletions(-) create mode 100644 test/nova_sup_tests.erl diff --git a/src/nova_sup.erl b/src/nova_sup.erl index 09731ef..78131af 100644 --- a/src/nova_sup.erl +++ b/src/nova_sup.erl @@ -13,6 +13,10 @@ %% Supervisor callbacks -export([init/1]). +-ifdef(TEST). +-export([listener_child_spec/1, clear_child_spec/3, tls_child_spec/3]). +-endif. + -include_lib("kernel/include/logger.hrl"). -include("../include/nova.hrl"). @@ -81,10 +85,13 @@ init([]) -> _ -> Children0 end, - setup_cowboy(Configuration), - + UseStacktrace = application:get_env(nova, use_stacktrace, false), + persistent_term:put(nova_use_stacktrace, UseStacktrace), - {ok, {SupFlags, Children}}. + %% The cowboy/ranch listener is a supervised child rather than a side + %% effect, so a failed bind (e.g. eaddrinuse) fails init/1 and surfaces + %% through application:start/1 instead of being logged and ignored. + {ok, {SupFlags, Children ++ cowboy_childspecs(Configuration)}}. %%%=================================================================== %%% Internal functions @@ -103,25 +110,18 @@ child(Id, Type, Mod) -> child(Id, Mod) -> child(Id, worker, Mod). -setup_cowboy(Configuration) -> - case start_cowboy(Configuration) of - {ok, App, Host, Port} -> - Host0 = inet:ntoa(Host), - CowboyVersion = get_version(cowboy), - NovaVersion = get_version(nova), - UseStacktrace = application:get_env(nova, use_stacktrace, false), - persistent_term:put(nova_use_stacktrace, UseStacktrace), - ?LOG_NOTICE(#{msg => <<"Nova is running">>, - url => unicode:characters_to_binary(io_lib:format("http://~s:~B", [Host0, Port])), - cowboy_version => CowboyVersion, nova_version => NovaVersion, app => App}); - {error, Error} -> - ?LOG_ERROR(#{msg => <<"Cowboy could not start">>, reason => Error}) - end. - --spec start_cowboy(Configuration :: map()) -> - {ok, BootstrapApp :: atom(), Host :: string() | {integer(), integer(), integer(), integer()}, - Port :: integer()} | {error, Reason :: any()}. -start_cowboy(Configuration) -> +cowboy_childspecs(Configuration) -> + {ChildSpec, App, Host, Port} = listener_child_spec(Configuration), + Host0 = inet:ntoa(Host), + ?LOG_NOTICE(#{msg => <<"Nova is running">>, + url => unicode:characters_to_binary(io_lib:format("http://~s:~B", [Host0, Port])), + cowboy_version => get_version(cowboy), nova_version => get_version(nova), app => App}), + [ChildSpec]. + +-spec listener_child_spec(Configuration :: map()) -> + {supervisor:child_spec(), BootstrapApp :: atom(), + Host :: string() | {integer(), integer(), integer(), integer()}, Port :: integer()}. +listener_child_spec(Configuration) -> Middlewares = [ nova_router, %% Lookup routes nova_plugin_handler, %% Handle pre-request plugins @@ -166,16 +166,8 @@ start_cowboy(Configuration) -> case maps:get(use_ssl, Configuration, false) of false -> Port = maps:get(port, Configuration, ?NOVA_STD_PORT), - case cowboy:start_clear( - ?NOVA_LISTENER, - [{port, Port}, - {ip, Host}], - CowboyOptions2) of - {ok, _Pid} -> - {ok, BootstrapApp, Host, Port}; - Error -> - Error - end; + ChildSpec = clear_child_spec(?NOVA_LISTENER, [{port, Port}, {ip, Host}], CowboyOptions2), + {ChildSpec, BootstrapApp, Host, Port}; _ -> case maps:get(ca_cert, Configuration, undefined) of undefined -> @@ -183,37 +175,57 @@ start_cowboy(Configuration) -> SSLOptions = maps:get(ssl_options, Configuration, #{}), TransportOpts = maps:put(port, Port, SSLOptions), TransportOpts1 = maps:put(ip, Host, TransportOpts), - - case cowboy:start_tls( - ?NOVA_LISTENER, maps:to_list(TransportOpts1), CowboyOptions2) of - {ok, _Pid} -> - ?LOG_NOTICE(#{msg => <<"Nova starting SSL">>, port => Port}), - {ok, BootstrapApp, Host, Port}; - Error -> - ?LOG_ERROR(#{msg => <<"Could not start cowboy with SSL">>, reason => Error}), - Error - end; + ?LOG_NOTICE(#{msg => <<"Nova starting SSL">>, port => Port}), + ChildSpec = tls_child_spec(?NOVA_LISTENER, maps:to_list(TransportOpts1), CowboyOptions2), + {ChildSpec, BootstrapApp, Host, Port}; CACert -> Cert = maps:get(cert, Configuration), Port = maps:get(ssl_port, Configuration, ?NOVA_STD_SSL_PORT), ?LOG_DEPRECATED(<<"0.10.3">>, <<"Use of use_ssl is deprecated, use ssl instead">>), - case cowboy:start_tls( - ?NOVA_LISTENER, [ - {port, Port}, - {ip, Host}, - {certfile, Cert}, - {cacertfile, CACert} - ], - CowboyOptions2) of - {ok, _Pid} -> - ?LOG_NOTICE(#{msg => <<"Nova starting SSL">>, port => Port}), - {ok, BootstrapApp, Host, Port}; - Error -> - Error - end + ?LOG_NOTICE(#{msg => <<"Nova starting SSL">>, port => Port}), + ChildSpec = tls_child_spec(?NOVA_LISTENER, + [{port, Port}, {ip, Host}, + {certfile, Cert}, {cacertfile, CACert}], + CowboyOptions2), + {ChildSpec, BootstrapApp, Host, Port} end end. +%% These mirror cowboy:start_clear/3 and cowboy:start_tls/3 (cowboy 2.15) but +%% yield a supervisor:child_spec/0 via ranch:child_spec/5 so the listener lives +%% in nova's supervision tree instead of being started as a side effect. Keep +%% the option transforms in listener_opts/2 in sync if the cowboy pin changes. +clear_child_spec(Ref, TransOpts0, ProtoOpts0) -> + {TransOpts, ProtoOpts} = listener_opts(TransOpts0, ProtoOpts0), + ranch:child_spec(Ref, ranch_tcp, TransOpts, cowboy_clear, ProtoOpts). + +tls_child_spec(Ref, TransOpts0, ProtoOpts0) -> + {TransOpts, ProtoOpts} = listener_opts(TransOpts0, ProtoOpts0), + ranch:child_spec(Ref, ranch_ssl, TransOpts, cowboy_tls, ProtoOpts). + +listener_opts(TransOpts0, ProtoOpts0) -> + TransOpts1 = ranch:normalize_opts(TransOpts0), + {TransOpts2, DynamicBuffer} = ensure_dynamic_buffer(TransOpts1, ProtoOpts0), + {TransOpts, ConnectionType} = ensure_connection_type(TransOpts2), + {TransOpts, ProtoOpts0#{connection_type => ConnectionType, dynamic_buffer => DynamicBuffer}}. + +ensure_connection_type(TransOpts = #{connection_type := ConnectionType}) -> + {TransOpts, ConnectionType}; +ensure_connection_type(TransOpts) -> + {TransOpts#{connection_type => supervisor}, supervisor}. + +ensure_dynamic_buffer(TransOpts, #{dynamic_buffer := DynamicBuffer}) -> + {TransOpts, DynamicBuffer}; +ensure_dynamic_buffer(TransOpts = #{socket_opts := SocketOpts}, _) -> + case proplists:get_value(buffer, SocketOpts, undefined) of + undefined -> + {TransOpts#{socket_opts => [{buffer, 512} | SocketOpts]}, {512, 131072}}; + _ -> + {TransOpts, false} + end; +ensure_dynamic_buffer(TransOpts, _) -> + {TransOpts, false}. + get_version(Application) -> diff --git a/test/nova_sup_tests.erl b/test/nova_sup_tests.erl new file mode 100644 index 0000000..48d60d0 --- /dev/null +++ b/test/nova_sup_tests.erl @@ -0,0 +1,50 @@ +-module(nova_sup_tests). +-include_lib("eunit/include/eunit.hrl"). + +%% The listener is a supervisor child spec, not a side effect. +child_spec_is_supervised_test() -> + Spec = nova_sup:clear_child_spec(shape_ref, [{port, 0}], #{}), + ?assertEqual({ranch_embedded_sup, shape_ref}, maps:get(id, Spec)), + ?assertEqual(supervisor, maps:get(type, Spec)), + {ranch_embedded_sup, start_link, [shape_ref, ranch_tcp, TransOpts, cowboy_clear, ProtoOpts]} = + maps:get(start, Spec), + ?assertEqual(supervisor, maps:get(connection_type, TransOpts)), + ?assertEqual(supervisor, maps:get(connection_type, ProtoOpts)). + +tls_child_spec_uses_ssl_transport_test() -> + Spec = nova_sup:tls_child_spec(tls_ref, [{port, 0}], #{}), + {ranch_embedded_sup, start_link, [tls_ref, ranch_ssl, _, cowboy_tls, _]} = maps:get(start, Spec). + +listener_lifecycle_test_() -> + {setup, + fun() -> + {ok, Apps} = application:ensure_all_started(cowboy), + Apps + end, + fun(Apps) -> [application:stop(A) || A <- lists:reverse(Apps)] end, + [ + fun binds_on_free_port/0, + fun fails_loudly_on_busy_port/0 + ]}. + +%% A free port yields a live listener process. +binds_on_free_port() -> + Spec = nova_sup:clear_child_spec(free_ref, [{port, 0}, {ip, {127, 0, 0, 1}}], #{}), + {M, F, A} = maps:get(start, Spec), + Result = erlang:apply(M, F, A), + ?assertMatch({ok, _}, Result), + {ok, Pid} = Result, + ?assert(is_process_alive(Pid)), + unlink(Pid), + exit(Pid, shutdown). + +%% A busy port fails the start instead of being swallowed. +fails_loudly_on_busy_port() -> + {ok, LSock} = gen_tcp:listen(0, [{ip, {127, 0, 0, 1}}]), + {ok, Port} = inet:port(LSock), + Spec = nova_sup:clear_child_spec(busy_ref, [{port, Port}, {ip, {127, 0, 0, 1}}], #{}), + {M, F, A} = maps:get(start, Spec), + process_flag(trap_exit, true), + Result = erlang:apply(M, F, A), + ?assertMatch({error, _}, Result), + gen_tcp:close(LSock).