Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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. Comment |
There was a problem hiding this comment.
Verified on clean venvs, PR head vs base:
52a4b47 (PR) : 9 failed, 205 passed, 11 errors
f87e22e (base): 6 failed, 208 passed, 11 errors
Collection errors are identical on both sides, so the 3 new failures are this branch's (details inline). CI only runs CodeQL, so nothing surfaces on the PR.
The namespacing of GC / Redis / agents / volume / state file is right, and I checked that _cleanup_stale_containers only removes the explicit names it computes — a namespaced test GC cannot reach a real deploy's containers. Three things block: red tests, the dashboard stack still shared and torn down on every run, and a rebase onto main that is semantic (two new GC_CONTAINER_NAME call sites), not just the cli/ → packages/cli/ move.
One more for the description: this works around canyonos stop leaving the GC container and state file behind rather than changing stop. Defensible — but say so, and open a follow-up for it
| def gc_container_name(): | ||
| ns = os.environ.get("CANYONOS_NAMESPACE") | ||
| return f"canyonos-{ns}-global-controller" if ns else "canyonos-global-controller" |
There was a problem hiding this comment.
🔴 Blocking — removing GC_CONTAINER_NAME breaks 2 tests on this branch, and 2 more call sites on main.
tests/test_gc_container_name.py:77 and :112 still read init_cmd.GC_CONTAINER_NAME:
$ python -m pytest tests/test_gc_container_name.py -q
FAILED test_the_container_is_started_under_a_fixed_name
FAILED test_a_port_collision_retry_still_gets_the_name
E AttributeError: module 'canyonos.init' has no attribute 'GC_CONTAINER_NAME'
Both pass on the PR's base. CI only runs CodeQL, so this doesn't show up on the PR — full-suite delta vs base is 6 failures → 9, with collection errors identical on both sides.
Separately, main has since moved cli/ → packages/cli/ (#130) and switched two log-tail calls from the container id to the fixed name — packages/cli/canyonos/deploy.py:497 and logs.py:28 both do ["docker", "logs", "-f", GC_CONTAINER_NAME], with a comment at deploy.py:493-496 justifying the fixed name for concurrent-redeploy safety. Under a namespace both would tail the wrong container. So the rebase isn't just a path move: those two sites need gc_container_name() too, and the tests need gc_container_name() in place of the constant
| # Own namespace for the whole run (state file, GC/Redis/agent container | ||
| # names) so this never touches -- or gets confused by -- a real deploy's. |
There was a problem hiding this comment.
🔴 Blocking — the dashboard stack is not namespaced, so this claim doesn't hold: every canyonos test run destroys the real deploy's dashboard.
The GC, Redis, agents, volume and state file are namespaced correctly. The dashboard isn't. _deploy_locally calls run_deploy(serve=True, …) → _start_dashboard() → dashboard_stack.py:24 COMPOSE_PROJECT = "canyonos-dashboard" — one fixed compose project, no namespace anywhere in that file. Then this PR's unconditional quit_existing() (line 415) → run_quit() → quit.py:54 teardown_dashboard() → docker compose -p canyonos-dashboard down.
Two ways in, same result:
- Dashboard still running (deploy up, but
_refuse_if_deploy_runningonly looks at the GC):dashboard_stack.py:156web_port = _existing_dashboard_port() or _find_web_port(preferred_port)— the existing container wins, so thedashboard_portpinned at line 81 is ignored, andstart()doescompose rm -sf apiand re-creates the api against the test controller's Redis identity. - After
canyonos stop:_existing_dashboard_port()returnsNonefor a stoppedcanyonos-dashboard-web-1,compose up -drecreates it on the test's port, thendownremoves it.
The .env guard in _dashboard_compose_command doesn't help — this run's own _start_dashboard() → prepare() writes <cwd>/.env before teardown gets there.
To be fair on attribution: the passing path already did this on base (elif run.error is None: quit_existing()). This PR extends it to every run, and is the one promising "no risk of clobbering".
Two ways to close it — either derive the compose project from the namespace:
def compose_project():
ns = os.environ.get("CANYONOS_NAMESPACE")
return f"canyonos-{ns}-dashboard" if ns else "canyonos-dashboard"(and thread it through _compose_argv / _existing_dashboard_port), or pass serve=False from _deploy_locally unless the LLM-proxy path genuinely needs the dashboard up under test. The second is smaller; the first keeps the current behavior
| if run.deploy_started: | ||
| # Read the log before tearing anything down -- it's the only trace | ||
| # of a failure left once quit_existing() below removes the container. | ||
| try: | ||
| run.log_tail = _log_tail(load_state()["container_id"]) | ||
| container_live = True | ||
| except (FileNotFoundError, OSError): | ||
| pass | ||
| elif run.error is None: | ||
| # TODO: always tearing down trades away inspecting a live failed deploy -- rework once that's needed again. | ||
| quit_existing() |
There was a problem hiding this comment.
🔴 Blocking — the teardown contract changed but its test didn't, and the teardown isn't in the finally the docstring implies.
tests/test_canyonos_test.py:174 test_a_failed_deploy_keeps_the_container_and_reads_its_log locks in the old behavior:
> assert deployable["quit"] == 0
E assert 1 == 0
You've deliberately reversed that contract here (and in the module docstring at lines 9-10), which is fine — but the test needs to follow, not sit red.
The second half is the one that bites: this block lives in the try body, not in run_test's finally (430-435, which only restores the namespace and ui.set_quiet). The inner try catches KeyboardInterrupt and RuntimeError only, so anything else skips teardown — KeyError from _send_query's json.loads(...)["request_id"] on a 200 with an unexpected body, a corrupt state file in _refuse_if_deploy_running, a ruamel error in _force_local_providers.
Same shape existed on base, but the namespace makes the consequence worse: what leaks is now canyonos-test-global-controller + the canyonos-test-workspace volume + ~/.canyonos/test-state.json, and since the finally restores CANYONOS_NAMESPACE on the way out, canyonos quit can no longer see or remove any of it. Only the next canyonos test's run_init → quit_existing() reaches it.
Move the teardown into the finally so "every run tears its own deploy down, pass or fail" is actually true:
try:
...
return 0 if run.error is None else 1
finally:
if run.deploy_started:
try:
run.log_tail = _log_tail(load_state()["container_id"])
except (FileNotFoundError, OSError):
pass
quit_existing()
if original_namespace is None:
os.environ.pop("CANYONOS_NAMESPACE", None)
else:
os.environ[CANYONOS_NAMESPACE] = original_namespace
ui.set_quiet(False)(the log tail has to move with it, since it needs the container alive.) And please add a test for the failure-path teardown while you're rewriting the one above
| ssh_private_key_path: ${EC2_SSH_PRIVATE_KEY_PATH} | ||
|
|
||
|
|
||
| env_file: .env |
There was a problem hiding this comment.
🟠 Unrelated to CAN-347, and it turns a fresh clone of the example into a hard failure — split it out.
.env is gitignored (.gitignore:46); only .env.example ships. env_file.py::_resolve_project_env_file raises on a missing file, and it's called both at canyonos_core/cli.py:465-471 (→ sys.exit(1)) and in GlobalController.__init__:
ValueError: env_file does not exist: .../examples/portfolio/.env (from env_file: .env)
I'll grant that .env.example:1-2 already says "Copy to .env … Loaded by config/global_controller.yaml (env_file: .env)", so the line restores a documented contract rather than inventing one. But the failure mode is a raw ValueError with nothing pointing at the copy step, and sync.py is a bare docker cp <cwd>/. … so whatever .env is on disk goes in — including the one dashboard_stack.prepare() rewrites with CANYONOS_JWT_SECRET, CANYONOS_REDIS_HOST, CANYONOS_WEB_PORT, …, which this line now hands to every agent container via --env-file.
Own PR, with either a friendlier error or a README line for the copy step
| if agent.get("type") == "workflow": | ||
| agent["api_port"] = _free_test_port(reserved_ports) | ||
| agent["dashboard_port"] = _free_test_port(reserved_ports) |
There was a problem hiding this comment.
🟠 api_port and dashboard_port get pinned, redis_port doesn't — pick one story.
Every agent in the portfolio config carries redis_port: 6379, and the GC reaches its own Redis through the published host port (init.py sets CANYONOS_REDIS_HOST=host.docker.internal), so the namespaced canyonos-test-redis-localhost genuinely has to own 6379. If anything else holds it, _launch_redis_containers goes docker inspect (namespaced name, missing) → docker run -p 6379:6379 → logger.critical + sys.exit(1), and the user sees "The deploy stopped before the workflow came up."
I checked the normal flow: canyonos stop → /clean → SIGTERM → cleanup() → stop() does remove the Redis and agent containers, so the port is free and this isn't reachable today. Which raises the question of what pinning api_port buys either — the real deploy's ports are equally free after a stop, and a running one is already refused by _refuse_if_deploy_running.
Either pin all three (and the scan then has to skip 6379-range ports too), or drop the port rewriting and let _refuse_if_deploy_running be the guard. Half of it is the confusing option
| def _free_test_port(reserved): | ||
| """First free port at/after TEST_PORT_START not already claimed this run.""" | ||
| port = TEST_PORT_START | ||
| while port_in_use(port) or port in reserved: | ||
| port += 1 | ||
| reserved.add(port) | ||
| return port |
There was a problem hiding this comment.
🟡 Nit — no attempt cap, and connect-based probing reads a bound port as free.
dashboard_stack._find_web_port caps at max_attempts=50 and probes with bind; this one scans forever and probes with port_in_use (connect). A port that's bound but not yet listening — or bound on a non-loopback interface — reports as free here. Nothing holds the port between this probe and Docker's bind minutes later after the image build, so two concurrent canyonos test runs (or anything grabbing 9000 in between) collide; deploy.py:178-182 at least fails legibly when that happens.
Mirroring _find_web_port — bind probe + max_attempts + a clear RuntimeError when exhausted — would put the two on the same footing
| @@ -81,7 +90,7 @@ def verify_runtime(config_path, gc_port): | |||
| # Image and container names the local provider derives from the agent name. | |||
| image = f"canyonos-{name.lower()}" | |||
There was a problem hiding this comment.
🟡 Nit — image tags aren't namespaced, so a test run retags the real deploy's images.
Containers are namespaced now; the image canyonos-<agent> here and in Local/_runtime.py:60 isn't. canyonos test rebuilds from the test's synced workspace and overwrites the same tag a real deploy uses. Running containers hold the image id so they're unaffected, but the "never touches a real deploy's" wording at test.py:388-389 should say so, or the tag should carry the namespace too
| # injection. CANYONOS_LLM_STUB_TEXT (the `canyonos test` LLM stub) is reachable | ||
| # only via `canyonos test`, never a deploy's env_file. | ||
| _RESERVED_ENV_KEYS = frozenset({"CANYONOS_LLM_STUB_TEXT"}) | ||
| _RESERVED_ENV_KEYS = frozenset({"CANYONOS_LLM_STUB_TEXT", "CANYONOS_NAMESPACE"}) |
There was a problem hiding this comment.
🟡 Nit — the reservation only covers the GC's own env, not the agents'.
Adding CANYONOS_NAMESPACE here is the right call. But _RESERVED_ENV_KEYS is consulted only in _load_dotenv; env_file_args still hands the whole file to every agent via --env-file, so a CANYONOS_NAMESPACE= line in a user .env does reach the agent containers. Harmless today (agents don't compute container names), and the same gap already exists for CANYONOS_LLM_STUB_TEXT — just flagging that the "must never be settable from a user's .env" comment above overclaims. Pre-existing; a follow-up is fine
| def container_name(agent_name, replica_index): | ||
| return f"canyonos-{agent_name.lower()}-{replica_index}" | ||
| namespace = os.environ.get("CANYONOS_NAMESPACE") | ||
| prefix = f"canyonos-{namespace}" if namespace else "canyonos" | ||
| return f"{prefix}-{agent_name.lower()}-{replica_index}" | ||
|
|
||
|
|
||
| def redis_container_name(host): | ||
| """The single source of truth for this name -- every caller (the | ||
| controller that creates it, and every runtime that points an agent at | ||
| it) must agree, or agents connect to a Redis container that doesn't | ||
| exist under the name they were given.""" | ||
| namespace = os.environ.get("CANYONOS_NAMESPACE") | ||
| prefix = f"canyonos-{namespace}" if namespace else "canyonos" | ||
| return f"{prefix}-redis-{host.replace('.', '-')}" |
There was a problem hiding this comment.
🟡 Nit — no tests for the namespacing itself, and one caller still doesn't agree with the "single source of truth".
This PR adds five namespace-aware name functions (container_name, redis_container_name, gc_container_name, state_path, workspace_volume) and ships zero tests for them. A small parametrized test with CANYONOS_NAMESPACE set and unset would have caught the GC_CONTAINER_NAME breakage, and is the cheapest guard against the next caller drifting.
Speaking of which: EC2/_runtime.py:213 still hardcodes f"canyonos-redis-{host.replace('.', '-')}". Not a connectivity bug — EC2 agents get the instance IP as redis_host (:150), not the container name — but _cleanup_stale_containers (global_controller.py:179) now looks for the namespaced name, which the EC2 path never creates. Importing redis_container_name there finishes what the docstring promises.
| elif run.error is None: | ||
| # TODO: always tearing down trades away inspecting a live failed deploy -- rework once that's needed again. | ||
| quit_existing() | ||
| # else: failed before this run ever started its own deploy (e.g. bad |
There was a problem hiding this comment.
🟡 Nit — comments that no longer match the code.
- This
# else:now trails anifwith noelse. _Run.__init__(line 160-161) still says "the container worth keeping" — nothing is kept anymore.- Line 410-411: the log tail is now read on success too and lands in the
--jsonpayload aslog_tail. Gate it onrun.error is not Noneunless you want it there
2 participants
When canyonos deploy is ran and stopped, any subsequent canyonos tests will cause unexpected errors to the program. This is due to the fact that canyonos stop didn't remove the containers, just stopped them, unlike quit, resulting in canyonos test trying to access the containers and erroring because those containers are stopped.
This was a deeper issue with overlap between canyonos test and deploy, and I decided to try to completely decouple canyonos test deploys with the actual deploy path. This involves launching canyonos test with completely different containers, being prefixed with "test" and being located on the 9000 port range (not the best fix, but good for now). This allows test to occupy different containers from deploy, with no risk of clobbering/getting clobbered by anything.
I also set canyonos test to delete all test containers immediately upon exit, its a bandage solution in the sense of being unable to debug containers if it fails as they get shut down, but something i did for simplicity.