Conversation
| def build_request_body( | ||
| cfg: LoadConfig, doc_b64: str, doc_id: str, case_id: str | ||
| ) -> dict: | ||
| callback_url = f"http://{cfg.callback_url_host}:{cfg.callback_port}/callback" |
There was a problem hiding this comment.
Bug: The load test driver sends http callbacks, but the server in non-debug mode only accepts https. This will cause all load test requests to fail.
Severity: HIGH
Suggested Fix
Update the load test driver in run_load.py to construct an https callback URL when targeting a non-debug server. Alternatively, configure the load test environment to run the server with debug = true, but this would not accurately reflect a production environment. The best fix is to align the driver with the server's production security requirements.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: tests/load/run_load.py#L339
Potential issue: The load test driver at `tests/load/run_load.py` constructs a callback
URL using the `http` scheme. However, the API server, when running in its default
non-debug mode (`debug = False`), is configured to only accept callbacks with the
`https` scheme. This validation occurs in `app/server/handlers/redaction.py`. As a
result, every request submitted by the load test to a production-like server will be
rejected with an HTTP 400 error, preventing the load test from running successfully and
measuring performance.
Did we get this right? 👍 / 👎 to inform future reviews.
| pool="prefork", | ||
| max_memory_per_child=config.queue.max_memory_per_child, |
There was a problem hiding this comment.
Bug: The max_memory_per_child argument passed to the Celery Worker is likely incorrect. The documented configuration setting is worker_max_memory_per_child, so the current argument may be silently ignored.
Severity: HIGH
Suggested Fix
Instead of passing the argument to the Worker constructor, configure the memory limit on the Celery app instance before starting the worker. Set queue.conf.worker_max_memory_per_child = config.queue.max_memory_per_child to ensure Celery correctly applies the setting.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: cli/cli.py#L162-L163
Potential issue: The Celery `Worker` is instantiated with the `max_memory_per_child`
keyword argument. However, the officially documented Celery configuration setting is
`worker_max_memory_per_child`. There is no evidence that Celery's `Worker` class
automatically maps the `max_memory_per_child` kwarg to the correct internal
configuration. As a result, this setting is likely to be silently ignored. This would
cause worker processes to have no memory limit, potentially leading to unbounded memory
growth and OOM kills under heavy load, which is the exact scenario this change was
intended to prevent.
Checking in experiments and infrastructure for measuring capacity in a containerized environment, specifically with respect to reported issues with the
BASE64attachment type.