Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions docker/entrypoints/celery_chatbot.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@ do
echo "Waiting for server volume..."
done

# Same variable Django routes the chat task with (settings.CHATBOT_QUEUE): the two must name the
# same queue, or turns are published where no worker is listening.
chatbot_queue="${CHATBOT_QUEUE:-chatbot}"

if [ "$AWS_SQS" = "True" ]
then
queues="chatbot.fifo,config.fifo"
queues="${chatbot_queue}.fifo,config.fifo"
else
queues="chatbot,broadcast,config"
queues="${chatbot_queue},broadcast,config"
fi

# Concurrency is intentionally low (-c 2): a chat turn is a long, LLM-bound ReAct run
Expand Down
3 changes: 3 additions & 0 deletions docker/env_file_app_template
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,6 @@ OLLAMA_KEEP_ALIVE=-1
CHATBOT_MESSAGE_RETENTION_DAYS=90
CHATBOT_RATE_LIMIT=5
CHATBOT_RATE_LIMIT_WINDOW=60
# Celery queue carrying the chat turns. Read by both the task routing and the dedicated chatbot
# worker, so changing it moves both ends together. Default: chatbot
CHATBOT_QUEUE=chatbot
8 changes: 7 additions & 1 deletion intel_owl/settings/celery.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@
BROADCAST_QUEUE = "broadcast"
CONFIG_QUEUE = "config"

CHATBOT_QUEUE = "chatbot"
# The dedicated chatbot worker consumes this same variable (docker/entrypoints/celery_chatbot.sh),
# so the queue the chat task is published to and the queue that worker drains cannot drift apart.
# `or` instead of a default argument: an empty value falls back like the shell's
# ${CHATBOT_QUEUE:-chatbot}, so both ends agree on unset *and* blank. Read from the environment
# only (not intel_owl.secrets, which would also consult AWS Secrets Manager) precisely because the
# worker entrypoint is a shell script: a value only Django could resolve would re-create the drift.
CHATBOT_QUEUE = get_secret("CHATBOT_QUEUE") or "chatbot"

CELERY_QUEUES = get_secret("CELERY_QUEUES", DEFAULT_QUEUE).split(",")
for queue in [DEFAULT_QUEUE, CONFIG_QUEUE, CHATBOT_QUEUE]:
Expand Down
4 changes: 3 additions & 1 deletion intel_owl/settings/chatbot.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
# chatbot is already opt-in (the separate ollama compose override), so an operator running it has
# accepted the model's memory cost. Constrained deploys can set a duration ("5m") or "0" (unload now).
OLLAMA_KEEP_ALIVE = secrets.get_secret("OLLAMA_KEEP_ALIVE", "-1")
CHATBOT_QUEUE = secrets.get_secret("CHATBOT_QUEUE", "chatbot")
# CHATBOT_QUEUE is defined in .celery, next to the other queue names and the CELERY_QUEUES loop
# that registers it. Defining it here too would shadow that one depending on the wildcard import
# order in settings/__init__.py.
CHATBOT_MESSAGE_RETENTION_DAYS = int(secrets.get_secret("CHATBOT_MESSAGE_RETENTION_DAYS", 90))

# Per-user rate limiting (messages / minute). Shared between REST and WebSocket;
Expand Down
125 changes: 125 additions & 0 deletions tests/intel_owl/test_settings_chatbot_queue.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# This file is a part of IntelOwl https://github.com/intelowlproject/IntelOwl
# See the file 'LICENSE' for copying permission.

import json
import os
import re
import subprocess
import sys
from pathlib import Path
from typing import Dict, Optional

from django.test import SimpleTestCase

REPO_ROOT = Path(__file__).resolve().parents[2]
WORKER_ENTRYPOINT = REPO_ROOT / "docker" / "entrypoints" / "celery_chatbot.sh"
# A cold django.setup() imports every plugin module, so this is generous on purpose: it is a guard
# against a hung interpreter, not a latency assertion, and a slow CI box must not flake on it.
PROBE_TIMEOUT_SECONDS = 300

# Settings are read at import time, so the effect of an environment variable can only be observed
# in a freshly booted interpreter: override_settings would assign the value we are trying to prove
# is computed, and reloading the settings package in-process corrupts it for every other test.
# This probe reports what a real deployment would end up with, from the setting down to the queue
# the chat task is actually routed to. The task name is read from the task itself so a drift
# between the route key and the real name surfaces here too.
SETTINGS_PROBE = """
import json

import django

django.setup()

from api_app.chatbot_manager.tasks import process_chat_message
from django.conf import settings
from intel_owl.celery import app, get_queue_name

print(
json.dumps(
{
"chatbot_queue": settings.CHATBOT_QUEUE,
"celery_queues": settings.CELERY_QUEUES,
"routed_queue": app.conf.task_routes[process_chat_message.name]["queue"],
"declared_queues": [queue.name for queue in app.conf.task_queues],
"expected_queue_name": get_queue_name(settings.CHATBOT_QUEUE),
}
)
)
"""


class ChatbotQueueSettingTestCase(SimpleTestCase):
"""The chatbot queue name must be configurable end to end: whatever the environment says has
to reach ``settings.CHATBOT_QUEUE`` *and* the Celery route of the chat task, otherwise turns
are published to a queue no worker drains."""

def _boot_settings(self, chatbot_queue: Optional[str] = None) -> Dict:
"""Boot Django in a subprocess with CHATBOT_QUEUE set to *chatbot_queue* (unset when None)
and return the probe's report. Fails the test if the interpreter does not exit cleanly."""
env = os.environ.copy()
env["DJANGO_SETTINGS_MODULE"] = "intel_owl.settings"
if chatbot_queue is None:
env.pop("CHATBOT_QUEUE", None)
else:
env["CHATBOT_QUEUE"] = chatbot_queue
result = subprocess.run(
[sys.executable, "-c", SETTINGS_PROBE],
capture_output=True,
cwd=REPO_ROOT,
env=env,
text=True,
timeout=PROBE_TIMEOUT_SECONDS,
check=False,
)
self.assertEqual(
result.returncode,
0,
msg=f"settings probe crashed\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}",
)
# django.setup() is free to log on the way up, so only the last line is the payload
return json.loads(result.stdout.strip().splitlines()[-1])

def test_custom_chatbot_queue_reaches_settings_and_task_routing(self):
booted = self._boot_settings(chatbot_queue="my_custom_queue")

self.assertEqual(booted["chatbot_queue"], "my_custom_queue")
self.assertIn("my_custom_queue", booted["celery_queues"])
# get_queue_name appends .fifo under SQS, so compare against what the deployment derives
self.assertEqual(booted["routed_queue"], booted["expected_queue_name"])
self.assertIn(
booted["expected_queue_name"],
booted["declared_queues"],
msg="the chat task is routed to a queue that is never declared",
)

def test_chatbot_queue_falls_back_to_the_default_when_unset_or_blank(self):
for chatbot_queue in (None, ""):
with self.subTest(chatbot_queue=chatbot_queue):
booted = self._boot_settings(chatbot_queue=chatbot_queue)

self.assertEqual(booted["chatbot_queue"], "chatbot")
self.assertIn("chatbot", booted["celery_queues"])

def test_worker_entrypoint_derives_its_queue_from_the_setting(self):
"""Django publishes the chat task and the dedicated worker consumes it, each reading
CHATBOT_QUEUE on its own side. Re-hardcoding either one, or letting the two defaults drift,
silently sends turns to a queue nobody drains -- the same failure this setting already had,
one layer down. Only the shell can be checked statically, so it is checked here."""
script = WORKER_ENTRYPOINT.read_text()

shell_default = re.search(r"\$\{CHATBOT_QUEUE:-([^}]+)\}", script)
self.assertIsNotNone(
shell_default,
msg="the worker entrypoint no longer derives its queue from CHATBOT_QUEUE",
)
self.assertEqual(
shell_default.group(1),
self._boot_settings()["chatbot_queue"],
msg="the entrypoint's default queue differs from the one the settings fall back to",
)
for hardcoded in ('queues="chatbot', "chatbot.fifo"):
self.assertNotIn(
hardcoded,
script,
msg=f"{hardcoded!r} is hardcoded again in the worker entrypoint",
)
76 changes: 76 additions & 0 deletions tests/intel_owl/test_settings_shadowing.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# This file is a part of IntelOwl https://github.com/intelowlproject/IntelOwl
# See the file 'LICENSE' for copying permission.

import ast
from collections import defaultdict
from pathlib import Path
from typing import Dict, Iterator, List, Set, Tuple

from django.test import SimpleTestCase

SETTINGS_DIR = Path(__file__).resolve().parents[2] / "intel_owl" / "settings"
SETTINGS_INIT = SETTINGS_DIR / "__init__.py"


def _assigned_names(node: ast.AST) -> Iterator[Tuple[str, int]]:
"""Names bound by a module-level assignment, including inside if/try blocks (where settings are
conditionally defined). Function and class bodies are skipped: their locals are not settings."""
for child in ast.iter_child_nodes(node):
if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):
continue
if isinstance(child, ast.Assign):
for target in child.targets:
if isinstance(target, ast.Name):
yield target.id, child.lineno
elif isinstance(child, ast.AnnAssign) and isinstance(child.target, ast.Name):
yield child.target.id, child.lineno
yield from _assigned_names(child)


def _wildcard_imported_modules() -> List[str]:
"""The submodule names __init__.py pulls in with `from .x import *`, in source order."""
tree = ast.parse(SETTINGS_INIT.read_text())
return [
node.module
for node in ast.walk(tree)
if isinstance(node, ast.ImportFrom) and any(alias.name == "*" for alias in node.names)
]


class SettingsSingleSourceTestCase(SimpleTestCase):
"""intel_owl/settings/__init__.py pulls every submodule in with a wildcard import, so a name
assigned in two of them silently resolves to whichever module is imported last. Re-ordering
those lines then changes the deployment's behaviour, which is exactly how CHATBOT_QUEUE ended
up ignoring its environment variable. One assignment per setting removes the ordering
dependency altogether."""

def test_no_setting_is_assigned_in_two_wildcard_imported_modules(self):
# __init__.py assigns settings of its own (INSTALLED_APPS, TEST_RUNNER) before the wildcard
# block, and a submodule shadowing one of those is the same bug in the other direction
sources: Dict[str, Path] = {SETTINGS_INIT.name: SETTINGS_INIT}
for module in _wildcard_imported_modules():
module_path = SETTINGS_DIR / f"{module}.py"
self.assertTrue(
module_path.is_file(),
msg=f"cannot audit the wildcard import of .{module}: {module_path} is not a file",
)
sources[module_path.name] = module_path

origins: Dict[str, Set[str]] = defaultdict(set)
for name, path in sources.items():
for setting, lineno in _assigned_names(ast.parse(path.read_text())):
origins[setting].add(f"{name}:{lineno}")

shadowed = {
setting: sorted(locations)
for setting, locations in origins.items()
# locations within a single module are re-assignments, not shadowing
if len({location.split(":")[0] for location in locations}) > 1
}

self.assertEqual(
shadowed,
{},
msg="these settings are assigned in more than one module of intel_owl/settings, so "
"their value depends on the import order in intel_owl/settings/__init__.py",
)
Loading