Skip to content

fix: harden MCP session cleanup, extend deploy timeout, and add experiment tooling#33

Merged
yoavkatz merged 4 commits into
rossoctl:mainfrom
yoavkatz:fix/mcp-session-cleanup-timeout
Jul 21, 2026
Merged

fix: harden MCP session cleanup, extend deploy timeout, and add experiment tooling#33
yoavkatz merged 4 commits into
rossoctl:mainfrom
yoavkatz:fix/mcp-session-cleanup-timeout

Conversation

@yoavkatz

@yoavkatz yoavkatz commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR bundles four sets of changes from the fork:

Session cleanup + deploy timeout (c9a36bd)

  • runner: catch delete_session errors inside the MCP.DeleteSession child span (success and failure paths) so a residual cleanup failure — e.g. the MCP sidecar reaped after an agent timeout — never flips a completed session's span to ERROR or propagates to the outer failure handler.
  • mcp_client: match benign "already gone" markers case-insensitively and recognize "no longer alive" as a clean no-op on session delete.
  • deploy-benchmark: extend rollout stabilization timeout 120s → 360s.
  • analyze_traces: collapse the per-config experiment comparison into a single aggregated table with dynamic column widths.

Experiment tooling (6a52068)

  • Add --experiment flag support.
  • Add delete-all-deployments.sh script.

Action timeout + fail-fast (f0ce136, fef2443)

  • deploy-benchmark: add --action-timeout flag and extend MCP wait time.
  • Fail fast on deployment errors; add pyyaml dependency.

OpenShift MLflow + time-window trace fetch (2afc116)

  • Rework analyze-run.sh to reach MLflow via kubectl port-forward for both --kind and --openshift, mirroring evaluate-benchmark.sh's OTEL collector approach.
  • Add --openshift DOMAIN / --kind cluster modes with per-mode defaults (namespace, service, port, TLS, workspace, auth mode, experiment id).
  • Add --auth-mode (secret | oc-token); oc-token uses oc whoami -t.
  • Replace --limit trace count with --window (e.g. 3h, 90m, 2d); downloader pages newest-first and stops at the first out-of-window trace.
  • Skip trivially short traces (MIN_DURATION_S) before fetching their spans.
  • Send x-mlflow-workspace header and allow insecure TLS for port-forwarded reencrypt HTTPS endpoints (RHOAI).
  • Verify kubectl context matches the cluster mode via libsh helper.
  • Harden secret-mode token flow against set -e command-substitution aborts.

Verification

  • Syntax-checked the changed Python files.
  • Confirmed avg guards empty lists and each metric row keeps its own > 0 division guard.
  • analyze-run.sh tested against both Kind and OpenShift (RHOAI) MLflow endpoints.

Assisted-By: Claude Code

yoavkatz and others added 3 commits July 20, 2026 17:16
Add --experiment NAME flag to deploy-agent.sh, deploy-benchmark.sh, deploy-and-evaluate.sh,
and evaluate-benchmark.sh so parallel experiments get distinct pod/service names (e.g.
exgentic-a2a-tool-calling-gsm8k-exp1 vs …-exp2).

Reorder steps in deploy-agent.sh and deploy-benchmark.sh so resource limits and rollout
stabilization happen before the health-check wait, avoiding a race where the pod restarts
mid-check.

Add delete-all-deployments.sh to bulk-delete all agent and benchmark deployments in a
namespace via the Kagenti API.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Yoav Katz <katz@il.ibm.com>
- runner: catch delete_session errors inside the MCP.DeleteSession child
  span so a residual cleanup failure (e.g. sidecar reaped after an agent
  timeout) never flips a completed session's span to ERROR
- mcp_client: match benign "already gone" markers case-insensitively and
  recognize "no longer alive" as a clean no-op on session delete
- deploy-benchmark: extend rollout stabilization timeout 120s -> 360s
- analyze_traces: aggregate experiment comparison into a single table
  with dynamic column widths

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Yoav Katz <katz@il.ibm.com>
Rework analyze-run.sh to reach MLflow via kubectl port-forward for both
--kind and --openshift, mirroring evaluate-benchmark.sh's OTEL collector
approach. MLflow location, TLS, workspace, auth mode, and experiment id
now default per cluster mode, with env/flag overrides.

- Add --openshift DOMAIN / --kind cluster modes with per-mode defaults
  (namespace, service, port, TLS, workspace, auth mode, experiment id)
- Add --auth-mode (secret | oc-token); oc-token uses `oc whoami -t`
- Replace --limit trace count with --window (e.g. 3h, 90m, 2d); the
  downloader pages newest-first and stops at the first out-of-window trace
- Skip trivially short traces (MIN_DURATION_S) before fetching their spans
- Send x-mlflow-workspace header and allow insecure TLS for port-forwarded
  reencrypt HTTPS endpoints (RHOAI)
- Verify kubectl context matches the cluster mode via libsh helper
- Harden secret-mode token flow against set -e command-substitution aborts

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Yoav Katz <katz@il.ibm.com>
@yoavkatz
yoavkatz force-pushed the fix/mcp-session-cleanup-timeout branch from 90174fa to f2579bd Compare July 20, 2026 14:17
@kellyaa

kellyaa commented Jul 20, 2026

Copy link
Copy Markdown
Member

Ran this end-to-end against my kind cluster and an OpenShift cluster, both local scripts and in-cluster Job. The core code changes work. Summary below; happy to share full logs if useful.

Coverage

Local scripts In-cluster Job
kind ✅ gsm8k 86.7%, tau2 66.7% ✅ gsm8k 86.7%, tau2 66.7% (matches local)
OpenShift ✅ gsm8k 93.3%, tau2 73.3% ❌ Dockerfile blocker, see below (not from this PR)

How I built the runner image

Followed the README's "In-Cluster Execution (Kubernetes Job)" section. From exgentic_a2a_runner/:

docker build -t ghcr.io/kellyaa/workload-harness/workload-harness:0.2 .
docker push ghcr.io/kellyaa/workload-harness/workload-harness:0.2

Then patched k8s/job.yamlimage: to point at that tag (per README Step 1's "Replace ... with your own registry path if needed"), and kubectl apply -f k8s/job.yaml from Step 4.

Session-cleanup fix — solid evidence

The "no longer alive" marker fired exactly 15 times per tau2 run (once per session at teardown, when the MCP sidecar goes away). Every occurrence was matched by the new pattern, logged as WARNING already cleaned up, and immediately followed by Session <id> deleted successfully.

$ grep -c "no longer alive" pr33-{A3,B3,D2}-tau2.log
pr33-A3-tau2.log:15   # kind, local scripts
pr33-B3-tau2.log:15   # OpenShift, local scripts
pr33-D2-tau2.log:15   # kind, in-cluster Job

Zero tracebacks, zero propagated exceptions, zero outer-span errors across all three runs. This is exactly what the PR is meant to do.

Deploy timeout, --experiment, delete-all-deployments.sh

  • The 360s stabilization timeout is comfortable — I never hit it under normal operation.
  • --experiment cleanly threads through to service names and MLflow tags. Verified in F1/F4 output.
  • New delete-all-deployments.sh handles both --kind and --openshift <domain> correct

analyze-run.sh matrix

  • --kind --window 3h — aggregated per-config sections + individual traces table rende
  • --kind --window 30m — paging correctly stops at the window edge (1 trace vs 34 with 3h).
  • --openshift --auth-mode oc-token --window 3h — bearer flow works, pulled 45 traces
  • ✅ Context-mismatch safety — kind-kagenti context + --openshift → exits 1 with a clear error, does not run.
  • ⚠️ --openshift --auth-mode secret — looks for mlflow-oauth-secret in redhat-ods-appt secret doesn't exist on the cluster (there are mlflow-oauth-proxy-*` variants butnot this one). Might be a cluster setup gap on my end, or the script needs an override.

Minor things I hit

PR gap: --action-timeout isn't wired end-to-end. deploy-benchmark.sh accepts the new --action-timeout SECONDS flag, but deploy-and-evaluate.sh doesn't accept or forward it. So invoking the wrapper drops the flag silently.
Pre-existing, not from this PR, but relevant when testing:
1. Runner image is OpenShift-incompatible. In-cluster Job on OCP failed because uv ruo rewrite /app/.venv/(removes.gitignore, rebuilds venv). Under the restricted-v2SCC, the random UID can't write there. Setting HOME=/tmp UV_CACHE_DIR=/tmp/uv-cachegets past the cache miss but not the.venv mutation. Fix would be either standard OCP hardening in the Dockerfile (chown -R 0:0 /app && chmod -R g+rwX /app) or invoking exgentic-a2a-runnerfrom/app/.venv/biuv run. On kind the Job worked perfectly. 2. **Namespace hardcoded to team1in three places.**deploy-agent.sh:209, deploy-bencbenchmark.sh:184/186/399all bake inteam1. I wanted to run Phase B in team2to leaveexisting OCP workloads alone, and needed one-line edits to make these honor${NAMESPACE:-team1}. Happy to send a small follow-up PR — should be non-invasive since the default is unchanged. 3. **README ↔ code drift I hit while testing cold-turkey:** - README §"Installation" (line 191) says update-secrets.sh"Creates/updates hf-secretken if not set)". The actual script *skips* ifHF_TOKEN is unset. On kind that leaveshf-secretmissing entirely, and the MCP pod forgsm8kfails withCreateContainerConfigError: secret "hf-secret" not found`. Either the README is wrong or the script's supposed to create a dummy — worth reconciling.

  • OPENAI_API_BASE is documented on line 79 ("point OPENAI_API_BASE at your endpoint") but listed on line 201 as "optional". In practice with a proxy-key model, it's mandatory: unset it and every LLM call returns Incorrect API key even after update-secrets.sh patches the key correctly. Justuired when using an OpenAI-compatible proxy (which the default openai/Azure/gpt-4.1model does)" would prevent this footgun.

Would I merge as-is

Yes, from the perspective of what this PR claims to do — the session-cleanup hardening, --experiment support, deploy timeout, and analyze-run rework all work as designed. The --action-timeout wiring in the wrapper is the one small PR gap; the rest of the findings are separate issues that predate

…ride

Resolve conflicts in deploy-agent.sh and deploy-benchmark.sh by combining
the experiment-suffix block from this branch with NAMESPACE="${NAMESPACE:-team1}"
from rossoctl/main (PR rossoctl#35), so both features coexist.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@yoavkatz

Copy link
Copy Markdown
Contributor Author

Merged and created issues for remaining issues .

@yoavkatz
yoavkatz merged commit 49dd34d into rossoctl:main Jul 21, 2026
1 check passed
@github-project-automation github-project-automation Bot moved this from New/ToDo to Done in Rossoctl Issue Prioritization Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants