[Micro66]

Summary by CodeRabbit

• New Features

  • Configurable validation timing via environment variables
  • Validation submissions queued in background and acknowledged immediately
  • Per-shell resolution of shell type and base image for bots
  • Option to retain failed validation containers for troubleshooting

• Bug Fixes

  • Timeouts now mark validations as submitted/in-progress (allowing polling) instead of immediate failure
  • Cleanup of validation containers scheduled asynchronously with optional delay

• Tests

  • Added tests for validation container retention, background submission, and shell-info resolution

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors image validation to use asynchronous background submission, introduces environment-driven validation timing and retry constants, preserves failed validation containers optionally, schedules delayed container cleanup, and expands shell resolution to return both shell_type and base_image.

Changes

Cohort / File(s)	Summary
Validation API & Timing backend/app/api/endpoints/adapter/shells.py	Added env-driven constants (VALIDATION_CONTAINER_CLEANUP_DELAY_SECONDS, VALIDATION_SUBMIT_TIMEOUT_SECONDS) and changed submission timeout usage and HTTP timeout error handling to return a non-final "submitted" state instead of a completed error.
Shell Resolution & Request Builder backend/app/services/execution/request_builder.py	Replaced single cached shell type with per-shell cached _cached_shell_info keyed by (user_id, namespace, name). Added _resolve_shell_info to return shell_type and base_image; _build_bot_config now uses resolved base_image.
Docker Executor: Validation Behavior executor_manager/executors/docker/executor.py	Added _should_keep_failed_validation_container toggle and logic to retain failed validation containers when enabled. Adjusted initial dispatch timeouts/retries for validation tasks, changed health-check cleanup behavior, and modified heartbeat env var injection for validation tasks.
Executor Manager Router & Backgrounding executor_manager/routers/routers.py	Changed /images/validate to build an OpenAI-Responses-style task payload with metadata.callback_url, compute dynamic callback URL from CALLBACK_HOST/CALLBACK_PORT, register validation tasks in _validation_task_registry, enqueue background submission via asyncio.create_task, and added _run_validation_task_in_background for offloading via asyncio.to_thread.
Tests — Docker Executor executor_manager/tests/executors/test_docker_executor.py	Added tests for preserving failed validation containers under VALIDATION_KEEP_FAILED_CONTAINER=true and for validation-specific initial-dispatch timeout/retry env vars being passed through.
Tests — Validation Router executor_manager/tests/routers/test_validate_image.py	Added async tests asserting immediate "submitted" response, callback URL preservation (HTTPS), background offload via asyncio.to_thread, registry cleanup on background failure, and that process_tasks is invoked by the background worker.
Tests — Request Builder backend/tests/services/execution/test_request_builder_user_mcp.py	Added test ensuring _resolve_shell_info caching is isolated per shell reference and returns distinct shell_type/base_image per shell.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant APIEndpoint as API:/images/validate
    participant Registry
    participant BGWorker as Background<br/>Task
    participant TaskProcessor
    participant DockerExec as Docker<br/>Executor
    participant Shell as Shell CRD

    Client->>APIEndpoint: POST /images/validate
    APIEndpoint->>Registry: register validation task
    APIEndpoint->>BGWorker: enqueue (asyncio.create_task)
    APIEndpoint-->>Client: Immediate response {status:"submitted", validation_task_id}

    BGWorker->>TaskProcessor: to_thread(process_tasks, [validation_task])
    TaskProcessor->>DockerExec: execute validation task
    DockerExec->>Shell: resolve shell info (shell_type, base_image)
    DockerExec->>DockerExec: apply validation timeouts/retries
    alt Container fails & VALIDATION_KEEP_FAILED_CONTAINER true
        DockerExec-->>TaskProcessor: preserve container (no rm)
    else
        DockerExec->>DockerExec: cleanup container
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 A hop, a queue, a backgrounded song,
Tasks no longer wait too long,
Timers and callbacks stitched with care,
Failed shells kept for a curious stare,
I nibble logs and bound along!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title is in Chinese and addresses custom image usage and validation issues, which aligns with the core changes across multiple files (shell info caching, validation timeout handling, and container cleanup logic).
Docstring Coverage	✅ Passed	Docstring coverage is 80.65% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (3)

backend/app/services/execution/request_builder.py (2)

61-64: Remove dead _cached_shell_type variable.

_cached_shell_type is no longer used since _resolve_shell_type now delegates to _resolve_shell_info, which uses _cached_shell_info instead.

🧹 Proposed fix to remove dead code

         self.db = db
-        # Cache shell_type to avoid repeated database queries
-        self._cached_shell_type: str | None = None
         # Cache shell info (shell_type + base_image) to avoid repeated database queries
         self._cached_shell_info: dict | None = None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/execution/request_builder.py` around lines 61 - 64,
Remove the dead field _cached_shell_type from the RequestBuilder (or the class
that defines it) because _resolve_shell_type now delegates to
_resolve_shell_info and caching is handled by _cached_shell_info; delete the
declaration "self._cached_shell_type: str | None = None" and any references to
_cached_shell_type, leaving _cached_shell_info and _resolve_shell_info intact so
caching logic is centralized.

---

693-707: Redundant variable initialization.

The shell = None on line 694 is immediately overwritten by the query result on lines 697-707.

🧹 Proposed fix

         shell_ref = bot_crd.spec.shellRef
-        shell = None

         # 1. Query user's private shell first
         shell = (

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/services/execution/request_builder.py` around lines 693 - 707,
Remove the redundant initialization "shell = None" before the DB query: the
variable is immediately assigned by the result of self.db.query(Kind)...first()
(referenced as shell_ref and the subsequent query on Kind), so delete the
standalone "shell = None" line and keep the query-assignment as the single
initialization; ensure no other code between uses shell before that query.

executor_manager/routers/routers.py (1)

953-959: Consider using Redis to store validation task metadata for multi-worker deployments.

The _validation_task_registry is an in-memory dict that only works when /images/validate and /callback requests hit the same process. For single-worker deployments (current setup), this works fine. However, if the executor_manager scales to multiple worker processes or replicated instances, the callback handler may fail to locate the validation metadata. To future-proof the implementation for multi-worker deployments, migrate this correlation to Redis or another shared store. Redis is already configured in the environment (REDIS_URL).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@executor_manager/routers/routers.py` around lines 953 - 959, The current
implementation stores per-validation metadata in the in-memory dict
_validation_task_registry keyed by validation_task_id, which will break when
/images/validate and /callback requests land on different processes; move this
correlation into Redis (using the existing REDIS_URL) by writing the same
metadata (validation_id, shell_type, image, created_at) to a Redis key when you
create the entry in the code path that sets _validation_task_registry (refer to
_validation_task_registry and validation_task_id), and update the callback
handler to read and delete that Redis key (instead of or in addition to
_validation_task_registry) when processing the terminal callback so multi-worker
instances can locate the metadata reliably.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/app/api/endpoints/adapter/shells.py`:
- Around line 996-997: The cleanup coroutine started with
asyncio.create_task(_cleanup_validation_container(request.executor_name)) is not
stored and may be garbage-collected before it runs; change this to create the
task and store a strong reference (e.g., add it to a lifecycle-managed set like
self._pending_cleanup_tasks) and register a done callback to remove it when
finished (use task.add_done_callback to pop from the set), doing the same for
the other places that spawn this cleanup (the similar block around the other
validation/cleanup calls). Ensure you reference the same task-handling pattern
used elsewhere (e.g., self._refresh_task) and keep the task referenced until
completion.

In `@backend/app/services/execution/request_builder.py`:
- Around line 676-678: The cache self._cached_shell_info is global across the
loop in _build_bot_config so multiple bots in the same team end up sharing the
first bot's shell_info; update the logic so shell_info is resolved
per-member—either make the cache keyed by the bot identity (e.g., bot.name +
bot.namespace) and use that key when reading/writing self._cached_shell_info, or
clear/avoid using the cached value inside the member iteration in
_build_bot_config (and any helper like _resolve_shell_info) so each bot gets its
own resolved shell_info; ensure all reads/writes reference the new per-bot key
or that caching is skipped during member processing.

In `@executor_manager/executors/docker/executor.py`:
- Around line 770-777: The cleanup call to self.subprocess.run(["docker", "rm",
"-f", executor_name], ...) currently swallows all failures; change the logic in
the cleanup routine (where the subprocess.run for docker rm -f is invoked) to
capture the CompletedProcess result or catch the exception and log an error
including executor_name and the stdout/stderr/exception details, and ensure
non-zero return codes are treated as failures (log and/or re-raise as
appropriate) instead of being silently ignored.

In `@executor_manager/routers/routers.py`:
- Around line 907-917: The current logic that trims "http(s)://" from
callback_host and then hardcodes "http://" for callback_url breaks HTTPS and
mangles IPv6; update the logic around callback_host/callback_port/callback_url
to parse the environment value with urllib.parse.urlparse (use get_host_ip() as
fallback), preserve the parsed.scheme if present (default to "http" only if no
scheme), use parsed.hostname (and parsed.port unless CALLBACK_PORT env
overrides) to avoid splitting on ":" (which breaks IPv6), and reconstruct
callback_url using the preserved scheme and properly bracketed IPv6 host and
final port; update references to callback_host, callback_port, and callback_url
accordingly.
- Around line 836-849: When process_tasks() fails in
_run_validation_task_in_background the code currently pops
_validation_task_registry and logs an error, leaving Redis stuck in "submitted";
change the except block to first write a terminal failed validation status (use
the existing _update_validation_status_from_callback or explicitly set status:
"failed", valid: false in Redis) before removing the registry entry, and apply
the same change to the other identical error-handling block referenced in the
review. Also address the other review notes: stop building callback URLs with
rsplit(":", 1) (use urllib.parse / yarl or proper URL/host handling to preserve
IPv6 addresses) and replace the process-local _validation_task_registry with
shared storage (Redis or DB) so request and callback correlation survives across
workers/replicas.

---

Nitpick comments:
In `@backend/app/services/execution/request_builder.py`:
- Around line 61-64: Remove the dead field _cached_shell_type from the
RequestBuilder (or the class that defines it) because _resolve_shell_type now
delegates to _resolve_shell_info and caching is handled by _cached_shell_info;
delete the declaration "self._cached_shell_type: str | None = None" and any
references to _cached_shell_type, leaving _cached_shell_info and
_resolve_shell_info intact so caching logic is centralized.
- Around line 693-707: Remove the redundant initialization "shell = None" before
the DB query: the variable is immediately assigned by the result of
self.db.query(Kind)...first() (referenced as shell_ref and the subsequent query
on Kind), so delete the standalone "shell = None" line and keep the
query-assignment as the single initialization; ensure no other code between uses
shell before that query.

In `@executor_manager/routers/routers.py`:
- Around line 953-959: The current implementation stores per-validation metadata
in the in-memory dict _validation_task_registry keyed by validation_task_id,
which will break when /images/validate and /callback requests land on different
processes; move this correlation into Redis (using the existing REDIS_URL) by
writing the same metadata (validation_id, shell_type, image, created_at) to a
Redis key when you create the entry in the code path that sets
_validation_task_registry (refer to _validation_task_registry and
validation_task_id), and update the callback handler to read and delete that
Redis key (instead of or in addition to _validation_task_registry) when
processing the terminal callback so multi-worker instances can locate the
metadata reliably.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3528e097-43c9-4bec-9550-fea6dd648a13

📥 Commits

Reviewing files that changed from the base of the PR and between 935c93b and ca54d3a.

📒 Files selected for processing (6)

• backend/app/api/endpoints/adapter/shells.py
• backend/app/services/execution/request_builder.py
• executor_manager/executors/docker/executor.py
• executor_manager/routers/routers.py
• executor_manager/tests/executors/test_docker_executor.py
• executor_manager/tests/routers/test_validate_image.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n backend/app/api/endpoints/adapter/shells.py | sed -n '990,1035p'

Repository: wecode-ai/Wegent

Length of output: 2093

---

🏁 Script executed:

# Get the full context around the flagged lines to understand the function and cleanup mechanism
head -n 1050 backend/app/api/endpoints/adapter/shells.py | tail -n +980

Repository: wecode-ai/Wegent

Length of output: 2748

---

🏁 Script executed:

# Search for the _cleanup_validation_container function to understand its implementation
rg -n "_cleanup_validation_container" backend/app/api/endpoints/adapter/shells.py -A 15

Repository: wecode-ai/Wegent

Length of output: 1108

---

🏁 Script executed:

# Search for VALIDATION_CONTAINER_CLEANUP_DELAY_SECONDS to understand how delays are configured
rg -n "VALIDATION_CONTAINER_CLEANUP_DELAY_SECONDS" backend/

Repository: wecode-ai/Wegent

Length of output: 603

---

🏁 Script executed:

# Check if cleanup tasks are stored or tracked anywhere else in the file
rg -n "cleanup.*task\|pending.*task\|_cleanup" backend/app/api/endpoints/adapter/shells.py

Repository: wecode-ai/Wegent

Length of output: 42

---

🏁 Script executed:

# Check if there's any task tracking or background task management in this file
rg -n "background.*task\|task.*set\|pending.*task\|_cleanup" backend/app/api/endpoints/adapter/shells.py -B 2 -A 2

Repository: wecode-ai/Wegent

Length of output: 42

---

🏁 Script executed:

# Check imports at top of file to understand what's available
head -n 50 backend/app/api/endpoints/adapter/shells.py

Repository: wecode-ai/Wegent

Length of output: 1529

---

🏁 Script executed:

# Search for similar fire-and-forget patterns in the codebase to see if this is a consistent pattern
rg "asyncio\.create_task" backend/ -n | head -20

Repository: wecode-ai/Wegent

Length of output: 2112

---

Track the delayed cleanup task instead of dropping it.

This coroutine can spend time inside asyncio.sleep() with no strong reference. If the task is garbage collected or loses its reference, the cleanup will never execute and validation containers will remain on the executor. Keep pending cleanup tasks in a set or another lifecycle-managed runner, similar to patterns used elsewhere in the codebase like self._refresh_task = asyncio.create_task(...).

Also applies to lines 1022-1028.

🧰 Tools

🪛 Ruff (0.15.7)

[warning] 997-997: Store a reference to the return value of asyncio.create_task

(RUF006)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/app/api/endpoints/adapter/shells.py` around lines 996 - 997, The
cleanup coroutine started with
asyncio.create_task(_cleanup_validation_container(request.executor_name)) is not
stored and may be garbage-collected before it runs; change this to create the
task and store a strong reference (e.g., add it to a lifecycle-managed set like
self._pending_cleanup_tasks) and register a done callback to remove it when
finished (use task.add_done_callback to pop from the set), doing the same for
the other places that spawn this cleanup (the similar block around the other
validation/cleanup calls). Ensure you reference the same task-handling pattern
used elsewhere (e.g., self._refresh_task) and keep the task referenced until
completion.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Log failed cleanup of exited validation containers.

docker rm -f is invoked without checking the return code, and the except block suppresses every failure. A cleanup regression here will silently leave stale validation containers behind.

🧰 Tools

🪛 Ruff (0.15.7)

[error] 776-777: try-except-pass detected, consider logging the exception

(S110)

---

[warning] 776-776: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@executor_manager/executors/docker/executor.py` around lines 770 - 777, The
cleanup call to self.subprocess.run(["docker", "rm", "-f", executor_name], ...)
currently swallows all failures; change the logic in the cleanup routine (where
the subprocess.run for docker rm -f is invoked) to capture the CompletedProcess
result or catch the exception and log an error including executor_name and the
stdout/stderr/exception details, and ensure non-zero return codes are treated as
failures (log and/or re-raise as appropriate) instead of being silently ignored.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check the file structure and examine the specific lines mentioned
cd /repo || exit 1
find . -name "routers.py" -path "*/executor_manager/*" -type f | head -5

Repository: wecode-ai/Wegent

Length of output: 115

---

🏁 Script executed:

# Find and read the relevant file
find . -name "routers.py" -path "*executor_manager*routers*" -type f

Repository: wecode-ai/Wegent

Length of output: 97

---

🏁 Script executed:

# Get file size and content around the specified lines
file_path=$(find . -name "routers.py" -path "*executor_manager*routers*" -type f | head -1)
if [ -n "$file_path" ]; then
  echo "File: $file_path"
  wc -l "$file_path"
  echo "=== Lines 836-849 ==="
  sed -n '836,849p' "$file_path"
  echo "=== Lines 907-917 ==="
  sed -n '907,917p' "$file_path"
  echo "=== Lines 953-965 ==="
  sed -n '953,965p' "$file_path"
fi

Repository: wecode-ai/Wegent

Length of output: 1935

---

🏁 Script executed:

# Search for validation status updates and Redis interactions
rg "validation.*status|update.*status" ./executor_manager/routers/routers.py -A 3

Repository: wecode-ai/Wegent

Length of output: 2041

---

🏁 Script executed:

# Look for _validation_task_registry usage
rg "_validation_task_registry" ./executor_manager/routers/routers.py -B 2 -A 2

Repository: wecode-ai/Wegent

Length of output: 1680

---

🏁 Script executed:

# Find the callback endpoint handler
rg "executor-manager/callback|callback.*endpoint" ./executor_manager/routers/routers.py -A 10

Repository: wecode-ai/Wegent

Length of output: 1226

---

🏁 Script executed:

# Search for Redis or status update operations related to validation
rg "redis.*validation|validation.*redis" ./executor_manager/routers/routers.py -B 2 -A 5

Repository: wecode-ai/Wegent

Length of output: 42

---

🏁 Script executed:

# Find where validation status is initially created in Redis
rg "validation.*=.*submitted|status.*=.*submitted" ./executor_manager/routers/routers.py -B 3 -A 3

Repository: wecode-ai/Wegent

Length of output: 42

---

🏁 Script executed:

# Look for any other places where validation status is updated outside the callback path
rg "update.*validation.*status|validation.*status.*update" ./executor_manager/routers/routers.py -B 2 -A 4

Repository: wecode-ai/Wegent

Length of output: 1427

---

🏁 Script executed:

# Check if there's any mechanism to report failed validation outside of callback
rg "failed.*validation|validation.*failed" ./executor_manager/routers/routers.py -B 2 -A 2

Repository: wecode-ai/Wegent

Length of output: 42

---

🏁 Script executed:

# Look for where validation status is first created when task is registered
rg "validation.*status|create.*validation" ./executor_manager/routers/routers.py -B 5 -A 5 | head -60

Repository: wecode-ai/Wegent

Length of output: 2243

---

🏁 Script executed:

# Check the entire function that contains the registry population to see initial status
sed -n '890,975p' ./executor_manager/routers/routers.py

Repository: wecode-ai/Wegent

Length of output: 3483

---

Report failed validation when background submission fails.

If process_tasks() raises an exception, the registry entry is deleted immediately and only an error is logged. This leaves the backend validation status stuck at "submitted" since _update_validation_status_from_callback() is the only mechanism that updates Redis to a terminal state. Before deleting the registry entry, issue a failed validation status update (status: failed, valid: false) or keep the entry intact so the callback handler can report completion if the task was queued despite the exception.

Also applies to: 963-965

Additional concerns:

• Lines 907-917: The callback URL construction using rsplit(":", 1) will break IPv6 addresses (e.g., [::1] becomes [:: after stripping the last segment). Use proper URL parsing.
• Lines 953-959: Module-level registry _validation_task_registry is process-local and breaks when request and callback land on different workers/replicas. Use shared storage (Redis/database) for validation correlation.

🧰 Tools

🪛 Ruff (0.15.7)

[warning] 845-845: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@executor_manager/routers/routers.py` around lines 836 - 849, When
process_tasks() fails in _run_validation_task_in_background the code currently
pops _validation_task_registry and logs an error, leaving Redis stuck in
"submitted"; change the except block to first write a terminal failed validation
status (use the existing _update_validation_status_from_callback or explicitly
set status: "failed", valid: false in Redis) before removing the registry entry,
and apply the same change to the other identical error-handling block referenced
in the review. Also address the other review notes: stop building callback URLs
with rsplit(":", 1) (use urllib.parse / yarl or proper URL/host handling to
preserve IPv6 addresses) and replace the process-local _validation_task_registry
with shared storage (Redis or DB) so request and callback correlation survives
across workers/replicas.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

executor_manager/routers/routers.py (1)

837-850: ⚠️ Potential issue | 🟠 Major

Failed background submission leaves Redis validation status stuck at "submitted".

If process_tasks() raises an exception, the registry entry is deleted but Redis validation status (initialized by the backend at "submitted") is never updated to a terminal state. Frontend polling will wait indefinitely.

Before removing the registry entry, call the backend's validation-status endpoint to report failure:

🐛 Proposed fix

 async def _run_validation_task_in_background(
     validation_task: Dict[str, Any], validation_task_id: int, image: str
 ) -> None:
     """Run validation task submission in background to avoid blocking HTTP handlers."""
     try:
         await asyncio.to_thread(task_processor.process_tasks, [validation_task])
         logger.info(
             f"Validation task submitted: task_id={validation_task_id}, image={image}"
         )
     except Exception as e:
-        # Clean up registry entry if background submission fails.
-        _validation_task_registry.pop(validation_task_id, None)
+        # Clean up registry entry and report failure to backend.
+        validation_meta = _validation_task_registry.pop(validation_task_id, None)
+        if validation_meta and validation_meta.get("validation_id"):
+            try:
+                task_api_domain = os.getenv("TASK_API_DOMAIN", "http://localhost:8000")
+                update_url = (
+                    f"{task_api_domain}/api/shells/validation-status/"
+                    f"{validation_meta['validation_id']}"
+                )
+                async with traced_async_client(timeout=10.0) as client:
+                    await client.post(update_url, json={
+                        "status": "completed",
+                        "stage": "Submission failed",
+                        "progress": 100,
+                        "valid": False,
+                        "errorMessage": f"Failed to submit validation task: {e}",
+                    })
+            except Exception as update_err:
+                logger.error(f"Failed to report validation failure: {update_err}")
         logger.error(f"Failed to submit validation task for {image}: {e}")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@executor_manager/routers/routers.py` around lines 837 - 850, On exception in
_run_validation_task_in_background, before popping _validation_task_registry,
call the backend validation-status endpoint to mark the validation as
terminal/failed (include validation_task_id and error details) so frontend
polling stops; implement this by invoking the backend client/HTTP call from the
exception handler (around task_processor.process_tasks), catch and log any
errors from that backend call via logger.error, then finally remove the registry
entry with _validation_task_registry.pop(validation_task_id, None) to ensure
cleanup even if the status update fails.

🧹 Nitpick comments (1)

executor_manager/routers/routers.py (1)

964-967: Store the task reference to prevent potential garbage collection.

Fire-and-forget asyncio.create_task() without storing the reference can lead to the task being garbage-collected before completion in edge cases, and any unhandled exceptions are silently lost.

♻️ Proposed fix

     # Submit in background to keep this endpoint responsive.
-    asyncio.create_task(
+    # Store reference to prevent GC and enable exception logging
+    background_task = asyncio.create_task(
         _run_validation_task_in_background(validation_task, validation_task_id, image)
     )
+    background_task.add_done_callback(
+        lambda t: t.exception() if not t.cancelled() and t.exception() else None
+    )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@executor_manager/routers/routers.py` around lines 964 - 967, The
fire-and-forget call to asyncio.create_task(...) that starts
_run_validation_task_in_background(validation_task, validation_task_id, image)
should keep a reference to the created Task to avoid GC and lost exceptions;
change the call to store the returned Task (e.g., assign to a local variable or
add it to a module-level set/list like background_tasks) and attach error
handling (either wrap _run_validation_task_in_background with try/except or add
a done callback that logs exceptions) so validation_task_id/image are available
for context when logging failures.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@executor_manager/routers/routers.py`:
- Around line 906-919: The current callback URL reconstruction (using
callback_host, callback_port, urlparse, callback_scheme, callback_hostname and
callback_effective_port) breaks for IPv6 literals because urlparse().hostname
strips the surrounding brackets; update the logic that builds callback_url to
detect when callback_hostname is an IPv6 address (contains ':'), wrap it in
square brackets when constructing the host portion, and then assemble the final
URL with callback_scheme and callback_effective_port; ensure values coming from
get_host_ip() or env CALLBACK_HOST are handled similarly so IPv6 addresses
become http://[::1]:8001/... instead of http://::1:8001/....

---

Duplicate comments:
In `@executor_manager/routers/routers.py`:
- Around line 837-850: On exception in _run_validation_task_in_background,
before popping _validation_task_registry, call the backend validation-status
endpoint to mark the validation as terminal/failed (include validation_task_id
and error details) so frontend polling stops; implement this by invoking the
backend client/HTTP call from the exception handler (around
task_processor.process_tasks), catch and log any errors from that backend call
via logger.error, then finally remove the registry entry with
_validation_task_registry.pop(validation_task_id, None) to ensure cleanup even
if the status update fails.

---

Nitpick comments:
In `@executor_manager/routers/routers.py`:
- Around line 964-967: The fire-and-forget call to asyncio.create_task(...) that
starts _run_validation_task_in_background(validation_task, validation_task_id,
image) should keep a reference to the created Task to avoid GC and lost
exceptions; change the call to store the returned Task (e.g., assign to a local
variable or add it to a module-level set/list like background_tasks) and attach
error handling (either wrap _run_validation_task_in_background with try/except
or add a done callback that logs exceptions) so validation_task_id/image are
available for context when logging failures.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8dfb0f2e-5b32-499c-98c0-1722f9bff196

📥 Commits

Reviewing files that changed from the base of the PR and between ca54d3a and d08b604.

📒 Files selected for processing (4)

• backend/app/services/execution/request_builder.py
• backend/tests/services/execution/test_request_builder_user_mcp.py
• executor_manager/routers/routers.py
• executor_manager/tests/routers/test_validate_image.py

🚧 Files skipped from review as they are similar to previous changes (1)

• executor_manager/tests/routers/test_validate_image.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major

IPv6 addresses produce malformed URLs.

urlparse().hostname strips brackets from IPv6 literals (e.g., [::1] → ::1), but the reconstruction doesn't add them back. This produces invalid URLs like http://::1:8001 instead of http://[::1]:8001.

🐛 Proposed fix to bracket IPv6 addresses

     callback_host = os.getenv("CALLBACK_HOST", get_host_ip())
     callback_port = os.getenv("CALLBACK_PORT", "8001")
     parsed = urlparse(
         callback_host if "://" in callback_host else f"http://{callback_host}"
     )
     callback_scheme = parsed.scheme or "http"
     callback_hostname = parsed.hostname or callback_host
     callback_effective_port = parsed.port or callback_port
+    # Bracket IPv6 addresses for valid URL format
+    if ":" in callback_hostname:
+        callback_hostname = f"[{callback_hostname}]"
     callback_url = (
         f"{callback_scheme}://{callback_hostname}:{callback_effective_port}"
         f"/executor-manager/callback"
     )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@executor_manager/routers/routers.py` around lines 906 - 919, The current
callback URL reconstruction (using callback_host, callback_port, urlparse,
callback_scheme, callback_hostname and callback_effective_port) breaks for IPv6
literals because urlparse().hostname strips the surrounding brackets; update the
logic that builds callback_url to detect when callback_hostname is an IPv6
address (contains ':'), wrap it in square brackets when constructing the host
portion, and then assemble the final URL with callback_scheme and
callback_effective_port; ensure values coming from get_host_ip() or env
CALLBACK_HOST are handled similarly so IPv6 addresses become
http://[::1]:8001/... instead of http://::1:8001/....