diff --git a/src/praisonai-bot/praisonai_bot/bots/_reliability.py b/src/praisonai-bot/praisonai_bot/bots/_reliability.py index 3821a9ed3..e1fe1c673 100644 --- a/src/praisonai-bot/praisonai_bot/bots/_reliability.py +++ b/src/praisonai-bot/praisonai_bot/bots/_reliability.py @@ -3,24 +3,44 @@ The gateway ships strong reliability building blocks — a durable inbound journal (default-on at the session level), a durable outbound outbox, gateway --wide admission control, and graceful-shutdown draining — but the two strongest -lifecycle knobs (graceful drain and inbound admission) are individually opt-in. -An operator running the gateway the "obvious" way therefore silently gets a -no-backpressure deployment that cuts in-flight turns on restart. +-wide admission control, and graceful-shutdown draining. Historically the two +strongest lifecycle knobs (graceful drain and inbound admission) were +individually opt-in, so an operator running the gateway the "obvious" way +silently got a no-backpressure deployment that cut in-flight turns on restart. +As of Issue #3438 the *unset* posture is safe by default (bounded admission + +graceful drain, bind-aware); ``reliability="off"`` opts back into the old +immediate-teardown behaviour. This module resolves a single, discoverable ``reliability`` preset onto the already-existing :class:`BotOS` constructor arguments so the happy path is production-grade in one switch, while explicit fields still win. +Safe by default (Issue #3438) +----------------------------- +Leaving ``reliability`` unset (``None``) now resolves to a *safe* posture +instead of the old no-backpressure one: a bounded admission ceiling + fair +wait queue and a graceful-drain window, so an operator running the gateway +the "obvious" way gets backpressure and does not cut in-flight turns on a +restart. The posture is bind-aware — a gateway bound to a non-loopback +interface (an actual deployment) resolves to the full ``production`` window, +while a loopback bind keeps the same admission ceiling with a snappier drain. +Opting back into today's immediate-teardown behaviour is an explicit +``reliability="off"``. + Profiles -------- ``"production"`` Graceful drain (15s window), inbound admission with a CPU-scaled concurrency ceiling and a bounded fair wait queue. -``"default"`` / ``None`` - A sane, small graceful-drain window (5s) so a restart doesn't cut - in-flight turns, but no admission ceiling (unbounded, legacy dispatch). - Durable inbound journal remains on by default (session level). +``None`` (unset) + Safe by default: bounded admission ceiling + fair wait queue plus a + graceful-drain window, bind-aware (full ``production`` window when + externally bound, a snappy 5s drain on loopback). +``"default"`` + The explicit legacy posture: a sane, small graceful-drain window (5s) so + a restart doesn't cut in-flight turns, but no admission ceiling + (unbounded, legacy dispatch). Durable inbound journal remains on by + default (session level). ``"off"`` Today's immediate-teardown behaviour: no drain, no admission. @@ -33,6 +53,7 @@ from __future__ import annotations +import ipaddress import os from dataclasses import dataclass from typing import Optional @@ -51,6 +72,37 @@ _KNOWN_PROFILES = ("production", "default", "off") +# Non-IP hostnames that mean "not an actual external deployment". A bind to +# any of these (or to any loopback IP, detected numerically below) keeps the +# safe-by-default admission ceiling but with a snappy drain; anything else (a +# real interface) resolves to the full ``production`` window (Issue #3438). +_LOOPBACK_HOSTNAMES = frozenset({"", "localhost", "loopback"}) + + +def _is_externally_bound(bind_host: Optional[str]) -> bool: + """Whether *bind_host* looks like a real (non-loopback) deployment bind. + + ``0.0.0.0`` / ``::`` (bind-all) and any concrete non-loopback address count + as external; ``None`` (unknown) is treated as loopback so we never guess a + host is external without evidence. Loopback is detected numerically via + :mod:`ipaddress`, so every valid loopback form — ``127.0.0.2``, + ``127.255.255.255``, an expanded ``0:0:0:0:0:0:0:1`` — is recognised, not + just the canonical ``127.0.0.1`` / ``::1`` spellings. + """ + if bind_host is None: + return False + host = str(bind_host).strip().lower() + if host in _LOOPBACK_HOSTNAMES: + return False + # Strip an IPv6 zone id / brackets, then classify numerically. A bare + # hostname that isn't a literal IP (e.g. a DNS name) is treated as an + # external bind — we only special-case the known loopback names above. + candidate = host.strip("[]").split("%", 1)[0] + try: + return not ipaddress.ip_address(candidate).is_loopback + except ValueError: + return True + def _cpu_scaled_ceiling() -> int: """A conservative CPU-scaled default concurrency ceiling. @@ -112,6 +164,7 @@ def normalize_reliability(reliability: Optional[str]) -> Optional[str]: def resolve_reliability( reliability: Optional[str], *, + bind_host: Optional[str] = None, drain_timeout: Optional[float] = None, max_concurrent_runs: int = 0, queue_depth: int = 0, @@ -123,7 +176,11 @@ def resolve_reliability( Args: reliability: Profile name (``"production"`` | ``"default"`` | ``"off"``) - or ``None`` for the default posture. + or ``None`` for the safe-by-default posture (Issue #3438). + bind_host: The host the gateway binds to; informs the *unset* default + posture — a non-loopback bind (an actual deployment) resolves to + the full ``production`` window, loopback keeps a snappy drain. + Ignored once an explicit preset is chosen. drain_timeout: Explicit graceful-drain window; ``None`` means "let the preset decide". max_concurrent_runs: Explicit admission ceiling; a positive value wins @@ -141,6 +198,15 @@ def resolve_reliability( """ profile = normalize_reliability(reliability) + # Safe by default (Issue #3438): an unset posture (``None``) resolves to a + # backpressured deployment rather than the old no-admission one. A real + # (non-loopback) bind becomes the full ``production`` posture; loopback + # keeps the same admission ceiling with a snappier drain. An explicit + # ``"default"`` / ``"off"`` / ``"production"`` is respected as-is. + unset_default = profile is None + if unset_default: + profile = "production" if _is_externally_bound(bind_host) else "__safe__" + # Start from the caller's explicit values (these always win). resolved_drain = drain_timeout resolved_max = int(max_concurrent_runs or 0) @@ -157,10 +223,11 @@ def resolve_reliability( f"outbound_ordering must be 'strict' or 'best_effort', " f"got {outbound_ordering!r}" ) - # An explicit ordering always wins; otherwise only production upgrades to - # strict, keeping default/off backward compatible. + # An explicit ordering always wins; otherwise the production posture and + # the safe-by-default posture upgrade to strict per-lane FIFO, keeping the + # explicit ``default``/``off`` presets backward compatible (best-effort). resolved_ordering = outbound_ordering or ( - "strict" if profile == "production" else "best_effort" + "strict" if profile in ("production", "__safe__") else "best_effort" ) if profile == "off": @@ -176,9 +243,17 @@ def resolve_reliability( outbound_ordering=resolved_ordering, ) - if profile == "production": + if profile in ("production", "__safe__"): + # ``production`` and the loopback safe-default share the same admission + # ceiling + bounded fair queue; they differ only in the drain window + # (a real deployment gets the longer production window, loopback keeps + # a snappy restart). if resolved_drain is None: - resolved_drain = _PRODUCTION_DRAIN_SECONDS + resolved_drain = ( + _PRODUCTION_DRAIN_SECONDS + if profile == "production" + else _DEFAULT_DRAIN_SECONDS + ) if not explicit_admission: resolved_max = _cpu_scaled_ceiling() if resolved_queue <= 0: @@ -195,8 +270,8 @@ def resolve_reliability( outbound_ordering=resolved_ordering, ) - # profile in (None, "default"): a sane small drain window so a restart does - # not cut in-flight turns, but no admission ceiling by default. + # profile == "default" (explicit legacy posture): a sane small drain window + # so a restart does not cut in-flight turns, but no admission ceiling. if resolved_drain is None: resolved_drain = _DEFAULT_DRAIN_SECONDS return ResolvedReliability( diff --git a/src/praisonai-bot/praisonai_bot/cli/features/gateway.py b/src/praisonai-bot/praisonai_bot/cli/features/gateway.py index 9b377b1a6..70f2cbe17 100644 --- a/src/praisonai-bot/praisonai_bot/cli/features/gateway.py +++ b/src/praisonai-bot/praisonai_bot/cli/features/gateway.py @@ -398,34 +398,38 @@ def _commit_start_flags() -> None: # CLI admission-control flags also apply in no-config mode (#2454): # build a shared gate directly so `--max-concurrent-runs` is honoured # even without a gateway.yaml. A ``--reliability`` preset (#2531) can - # supply the admission ceiling too, so build the gate whenever either - # is given, letting the preset fill fields the explicit flags omit. - if max_concurrent_runs is not None or reliability is not None: - try: - from praisonai_bot.bots._admission import build_admission_gate - from praisonai_bot.bots._reliability import resolve_reliability - - # Pass the explicit ``--drain-timeout`` through so it still wins - # over the preset; capture the resolved window for shutdown so - # ``--reliability production`` actually drains (#2531). - _resolved = resolve_reliability( - reliability, - drain_timeout=drain_timeout, - max_concurrent_runs=max_concurrent_runs or 0, - queue_depth=queue_depth or 0, - overflow_policy=overflow_policy or "reject", - ) - resolved_drain_timeout = _resolved.drain_timeout - self._gateway._admission_gate = build_admission_gate( - max_concurrent_runs=_resolved.max_concurrent_runs, - queue_depth=_resolved.queue_depth, - overflow_policy=_resolved.overflow_policy, - ) - except Exception as e: - # Invalid admission-control config is unrecoverable until the - # operator fixes it; restarting won't help (#2437). - print(f"Error: invalid admission-control config: {e}") - return GATEWAY_FATAL_CONFIG_EXIT_CODE + # supply the admission ceiling too. As of #3438 the *unset* posture is + # safe by default (bind-aware admission ceiling + drain), so we always + # resolve reliability here — the resolver returns an admission ceiling + # unless the operator passes ``--reliability off``. + try: + from praisonai_bot.bots._admission import build_admission_gate + from praisonai_bot.bots._reliability import resolve_reliability + + # Pass the explicit ``--drain-timeout`` through so it still wins + # over the preset; capture the resolved window for shutdown so + # ``--reliability production`` actually drains (#2531). ``host`` + # informs the safe-by-default posture — a non-loopback bind + # resolves to the full production window (#3438). + _resolved = resolve_reliability( + reliability, + bind_host=host, + drain_timeout=drain_timeout, + max_concurrent_runs=max_concurrent_runs or 0, + queue_depth=queue_depth or 0, + overflow_policy=overflow_policy or "reject", + ) + resolved_drain_timeout = _resolved.drain_timeout + self._gateway._admission_gate = build_admission_gate( + max_concurrent_runs=_resolved.max_concurrent_runs, + queue_depth=_resolved.queue_depth, + overflow_policy=_resolved.overflow_policy, + ) + except Exception as e: + # Invalid admission-control config is unrecoverable until the + # operator fixes it; restarting won't help (#2437). + print(f"Error: invalid admission-control config: {e}") + return GATEWAY_FATAL_CONFIG_EXIT_CODE if agent_file: diff --git a/src/praisonai-bot/tests/unit/bots/test_outbox_ordering.py b/src/praisonai-bot/tests/unit/bots/test_outbox_ordering.py index d42a04fd1..a659c28a3 100644 --- a/src/praisonai-bot/tests/unit/bots/test_outbox_ordering.py +++ b/src/praisonai-bot/tests/unit/bots/test_outbox_ordering.py @@ -148,8 +148,14 @@ def test_reliability_production_selects_strict(): assert r.outbound_ordering == "strict" -def test_reliability_default_and_off_stay_best_effort(): - assert resolve_reliability(None).outbound_ordering == "best_effort" +def test_reliability_unset_default_is_strict(): + # Safe by default (#3438): the unset posture upgrades to strict per-lane + # FIFO along with its admission ceiling. + assert resolve_reliability(None).outbound_ordering == "strict" + + +def test_reliability_explicit_default_and_off_stay_best_effort(): + # The explicit legacy presets remain backward-compatible (best-effort). assert resolve_reliability("default").outbound_ordering == "best_effort" assert resolve_reliability("off").outbound_ordering == "best_effort" diff --git a/src/praisonai-bot/tests/unit/bots/test_reliability.py b/src/praisonai-bot/tests/unit/bots/test_reliability.py index 5aa9c6ae3..bc95075af 100644 --- a/src/praisonai-bot/tests/unit/bots/test_reliability.py +++ b/src/praisonai-bot/tests/unit/bots/test_reliability.py @@ -11,16 +11,49 @@ ) -def test_default_posture_applies_small_drain_no_admission(): - """Unset reliability gives a sane small drain window but no ceiling.""" +def test_unset_posture_is_safe_by_default(): + """Unset reliability is safe by default: admission ceiling + drain (#3438).""" r = resolve_reliability(None) + # Snappy drain on the (unknown → loopback) bind, but a real admission + # ceiling and bounded fair queue so a burst can't fan out unboundedly. assert r.drain_timeout == 5.0 - assert r.max_concurrent_runs == 0 - assert r.queue_depth == 0 + assert r.max_concurrent_runs > 0 + assert r.queue_depth > 0 + assert r.overflow_policy == "queue" + + +def test_unset_externally_bound_is_full_production(): + """An unset posture on a non-loopback bind resolves to production (#3438).""" + r = resolve_reliability(None, bind_host="0.0.0.0") + assert r.drain_timeout == 15.0 + assert r.max_concurrent_runs > 0 + assert r.queue_depth > 0 + assert r.overflow_policy == "queue" + assert r.outbound_ordering == "strict" + + +def test_unset_loopback_bind_stays_snappy(): + """Loopback binds keep the ceiling but a snappy drain window (#3438).""" + for host in ("127.0.0.1", "localhost", "::1", None): + r = resolve_reliability(None, bind_host=host) + assert r.drain_timeout == 5.0 + assert r.max_concurrent_runs > 0 + +def test_unset_noncanonical_loopback_bind_stays_snappy(): + """Any valid loopback form is recognised, not just 127.0.0.1/::1 (#3438).""" + for host in ("127.0.0.2", "127.255.255.255", "0:0:0:0:0:0:0:1", "[::1]"): + r = resolve_reliability(None, bind_host=host) + assert r.drain_timeout == 5.0, host + assert r.max_concurrent_runs > 0, host -def test_default_alias_matches_none(): - assert resolve_reliability("default") == resolve_reliability(None) + +def test_explicit_default_is_legacy_no_admission(): + """Explicit reliability='default' keeps the legacy no-ceiling posture.""" + r = resolve_reliability("default") + assert r.drain_timeout == 5.0 + assert r.max_concurrent_runs == 0 + assert r.queue_depth == 0 def test_production_enables_drain_admission_bounded_queue(): @@ -101,6 +134,16 @@ def test_botos_reliability_off_no_drain_no_gate(): assert os_._admission_gate is None +def test_botos_unset_reliability_is_safe_by_default(): + """BotOS() with no reliability arg is backpressured by default (#3438).""" + from praisonai_bot.bots.botos import BotOS + + os_ = BotOS(bots=[]) + assert os_._drain_timeout == 5.0 + assert os_._admission_gate is not None + assert os_._admission_gate.enabled + + def test_botos_explicit_drain_overrides_reliability(): from praisonai_bot.bots.botos import BotOS @@ -159,3 +202,13 @@ def test_cli_no_config_explicit_drain_overrides_reliability(): def test_cli_no_config_reliability_off_immediate_teardown(): """`--reliability off` (no config) tears down immediately (drain 0).""" assert _run_no_config_gateway_start(reliability="off") == 0.0 + + +def test_cli_no_config_unset_loopback_safe_default_drains(): + """No reliability + loopback bind (no config) drains with the safe window.""" + assert _run_no_config_gateway_start(host="127.0.0.1") == 5.0 + + +def test_cli_no_config_unset_external_bind_full_production_drain(): + """No reliability + a non-loopback bind gets the full production drain.""" + assert _run_no_config_gateway_start(host="0.0.0.0") == 15.0