test(integration): cover health-check withdrawal and recovery in all 3 runtimes - #1485
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdded UC41 integration fixtures and tests for Python, TypeScript, and Java. The scenarios verify health-check withdrawal, consumer failover, provider recovery, same-process recovery, and degraded handling of throwing checks. ChangesUC41 health-check withdrawal integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderA
participant Registry
participant Consumer
participant ProviderB
ProviderA->>Registry: publish health status
Registry->>Consumer: resolve provider dependency
ProviderA->>Registry: withdraw after unhealthy check
Registry->>Consumer: route request to ProviderB
ProviderA->>Registry: re-register after recovery
Registry->>Consumer: route request back to ProviderA
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
dhyansraj
force-pushed
the
test/1480-health-check-integration
branch
from
August 5, 2026 18:05
fa44d7c to
5375605
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
tests/integration/suites/uc41_health_check_withdrawal/tc01_python_withdraw_failover_recover/test.yaml (1)
178-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBound the
meshctl callpolls by elapsed time, not iteration count.Each iteration costs one
meshctl callplus a 1s sleep, so 60 iterations can exceed the 90s step timeout. On a failing run the harness kills the step before the loop printsBASELINE: TIMEOUT, and the same applies to the failover and failback loops. The assertions still fail, but the intended verdict line is missing from the report. An elapsed-time deadline keeps the diagnostic line inside the step budget.♻️ Proposed fix: deadline-bounded poll
- for i in $(seq 1 60); do + DEADLINE=$(( $(date +%s) + 75 )) + i=0 + while [ "$(date +%s)" -lt "$DEADLINE" ]; do + i=$((i + 1)) R=$(meshctl call hc-consumer-py:who_served '{}' 2>&1 || true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/suites/uc41_health_check_withdrawal/tc01_python_withdraw_failover_recover/test.yaml` around lines 178 - 207, Replace the fixed iteration limit in the baseline polling loop around meshctl call with an elapsed-time deadline that accounts for each call and sleep, ensuring BASELINE: TIMEOUT is printed before the 90-second step timeout. Apply the same deadline-based polling approach to the corresponding failover and failback loops, preserving their existing success, failure, and timeout verdicts.tests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades/test.yaml (1)
53-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the shared fleet bring-up into a suite routine.
Lines 53-158 repeat the tc01 bring-up almost verbatim, including the two ordering gates and the fleet-ready poll. The ordering gate is load-bearing for both tests, so a future fix must be applied twice.
routines.yamlalready holds the suite-level registry and cleanup routines, so astart_python_hc_fleetroutine there would let both test files call one definition. Six cases across three runtimes make this duplication grow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades/test.yaml` around lines 53 - 158, Move the shared Python health-check fleet setup from this test and the matching tc01 flow into a suite-level start_python_hc_fleet routine in routines.yaml, including artifact copying, dependency installation, provider/consumer startup, both health gates, and the fleet-ready poll. Replace the duplicated inline steps in both test files with calls to that routine while preserving their existing ordering and readiness behavior.tests/integration/suites/uc41_health_check_withdrawal/tc05_java_withdraw_failover_recover/test.yaml (1)
209-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetry the
/livezprobe once before counting a failure.The assertion at line 354 requires
LIVEZ_FAILURES: 0. Onecurl --max-time 2timeout under parallel container load then fails the run, even though the JVM stayed alive. A single immediate retry keeps the "withdrawn is not dead" invariant and removes that flake source.♻️ Proposed retry around the liveness probe
LIVEZ_FAIL=0 + probe_livez() { + curl -sf --max-time 2 http://localhost:3431/livez > /dev/null 2>&1 && return 0 + sleep 1 + curl -sf --max-time 2 http://localhost:3431/livez > /dev/null 2>&1 + } for i in $(seq 1 60); do - curl -sf --max-time 2 http://localhost:3431/livez > /dev/null 2>&1 || LIVEZ_FAIL=$((LIVEZ_FAIL + 1)) + probe_livez || LIVEZ_FAIL=$((LIVEZ_FAIL + 1)) BODY=$(curl -s --max-time 3 http://localhost:8000/agents || echo '{}') A=$(echo "$BODY" | jq -r '.agents[]? | select(.name=="hc-provider-a-java") | .status') B=$(echo "$BODY" | jq -r '.agents[]? | select(.name=="hc-provider-b-java") | .status') echo "t=${i}s A=$A B=$B livez_failures=$LIVEZ_FAIL" if [ "$A" = "unhealthy" ] && [ "$B" = "healthy" ]; then - curl -sf --max-time 2 http://localhost:3431/livez > /dev/null 2>&1 || LIVEZ_FAIL=$((LIVEZ_FAIL + 1)) + probe_livez || LIVEZ_FAIL=$((LIVEZ_FAIL + 1)) echo "WITHDRAWAL: OK after ~${i}s"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/suites/uc41_health_check_withdrawal/tc05_java_withdraw_failover_recover/test.yaml` around lines 209 - 231, Update the `/livez` checks in the Phase B shell command to retry each probe once immediately before incrementing `LIVEZ_FAIL`. Apply this to both the per-iteration probe and the final confirmation probe, preserving the existing `LIVEZ_FAILURES` accounting and withdrawal success condition.tests/integration/suites/uc41_health_check_withdrawal/tc06_java_throwing_check_degrades/test.yaml (1)
241-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPoll for the routing verdict instead of calling once.
The consumer returns
served_by: "UNRESOLVED"during any transient resolution gap. This step takes one sample, so assertion 283 (contains 'hc-provider-a-java') fails on a gap that the feature under test allows. Poll for the A verdict and emit a distinct failure line, so a permanent gap still fails the run. Keep provider B out of the captured output on the success path, because assertion 285 asserts its absence.♻️ Proposed bounded poll for `still_on_a`
- name: "The consumer must still be routed to provider A" handler: shell workdir: /workspace - command: meshctl call hc-consumer-java:whoServed '{}' 2>&1 || echo 'CALL_FAILED' + command: | + for i in $(seq 1 20); do + R=$(meshctl call hc-consumer-java:whoServed '{}' 2>&1 || true) + case "$R" in + *hc-provider-a-java*) echo "STILL_ON_A: OK after ~${i}s -> $R"; exit 0 ;; + *hc-provider-b-java*) echo "STILL_ON_A: FAILED_OVER (answer named the survivor provider)"; exit 0 ;; + esac + echo "t=${i}s unresolved -> $R" + sleep 1 + done + echo "STILL_ON_A: TIMEOUT" capture: still_on_a timeout: 60With this loop, tighten the two assertions to the explicit verdicts:
- expr: "${captured.still_on_a} contains 'STILL_ON_A: OK'" message: "RESOLVABLE: a degraded provider must remain selectable — the consumer must still be routed to A" - expr: "${captured.still_on_a} not contains 'STILL_ON_A: FAILED_OVER'" message: "RESOLVABLE: the consumer must NOT have failed over to B — that would mean the throwing check withdrew A"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/suites/uc41_health_check_withdrawal/tc06_java_throwing_check_degrades/test.yaml` around lines 241 - 246, Replace the single `meshctl call` command in the `still_on_a` step with a bounded polling loop that retries until the response confirms provider A, emits `STILL_ON_A: OK` on success, and emits `STILL_ON_A: FAILED_OVER` for a permanent failure or provider-B result. Keep provider B absent from successful captured output, and update assertions 283 and 285 to check these explicit verdict markers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/package.json`:
- Around line 14-16: Update the dependencies in package.json for the
ts-hc-provider-b artifact to declare zod directly alongside `@mcpmesh/sdk`,
matching the version required by the existing src/index.ts import and avoiding
reliance on transitive hoisting.
In `@tests/integration/suites/uc41_health_check_withdrawal/routines.yaml`:
- Around line 70-87: Update the readiness loop around the health endpoint check
and health-monitor log grep so a successful /health response does not
immediately fail when the timing log line has not yet been written. Continue
retrying within the existing 30-second loop until both registry readiness and
the expected “timeout: 5s, interval: 2s” log entry are observed; only emit the
failure diagnostics and exit after the loop expires.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades/test.yaml`:
- Around line 215-219: Update the THROW_TICKS command in the health-check test
so grep’s zero-match exit status does not append a second fallback value. Ensure
N always contains exactly one integer, while still returning 0 when the log is
missing, so the numeric comparison and reported verdict remain valid.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc04_typescript_throwing_check_degrades/test.yaml`:
- Around line 239-245: Update the log assertions in the test’s degrade_log
capture to extract `[mesh-health]` verdicts and require at least one `DEGRADED`
classification, rather than only checking thrown and non-UNHEALTHY lines. Apply
the same assertion to the corresponding block near the second reported location,
while preserving the existing diagnostic output.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc06_java_throwing_check_degrades/test.yaml`:
- Around line 227-229: The grep count fallback emits duplicate values when there
are no matches. In
tests/integration/suites/uc41_health_check_withdrawal/tc06_java_throwing_check_degrades/test.yaml
lines 227-229, remove the fallback echo, default N to 0 with parameter
expansion, and preserve the existing THROW_TICKS verdict logic. Apply the same
variable-based count and defaulting in
tests/integration/suites/uc41_health_check_withdrawal/tc05_java_withdraw_failover_recover/test.yaml
line 259 so FAIL_TICKS receives a single numeric value.
---
Nitpick comments:
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc01_python_withdraw_failover_recover/test.yaml`:
- Around line 178-207: Replace the fixed iteration limit in the baseline polling
loop around meshctl call with an elapsed-time deadline that accounts for each
call and sleep, ensuring BASELINE: TIMEOUT is printed before the 90-second step
timeout. Apply the same deadline-based polling approach to the corresponding
failover and failback loops, preserving their existing success, failure, and
timeout verdicts.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades/test.yaml`:
- Around line 53-158: Move the shared Python health-check fleet setup from this
test and the matching tc01 flow into a suite-level start_python_hc_fleet routine
in routines.yaml, including artifact copying, dependency installation,
provider/consumer startup, both health gates, and the fleet-ready poll. Replace
the duplicated inline steps in both test files with calls to that routine while
preserving their existing ordering and readiness behavior.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc05_java_withdraw_failover_recover/test.yaml`:
- Around line 209-231: Update the `/livez` checks in the Phase B shell command
to retry each probe once immediately before incrementing `LIVEZ_FAIL`. Apply
this to both the per-iteration probe and the final confirmation probe,
preserving the existing `LIVEZ_FAILURES` accounting and withdrawal success
condition.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc06_java_throwing_check_degrades/test.yaml`:
- Around line 241-246: Replace the single `meshctl call` command in the
`still_on_a` step with a bounded polling loop that retries until the response
confirms provider A, emits `STILL_ON_A: OK` on success, and emits `STILL_ON_A:
FAILED_OVER` for a permanent failure or provider-B result. Keep provider B
absent from successful captured output, and update assertions 283 and 285 to
check these explicit verdict markers.
🪄 Autofix
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 Plus
Run ID: b69884bd-72f4-43ac-acc0-d9d821e87fec
📒 Files selected for processing (28)
tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/pom.xmltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/src/main/java/com/example/hcconsumer/HcConsumerApplication.javatests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/src/main/resources/application.ymltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/pom.xmltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/src/main/java/com/example/hcprovidera/HcProviderAApplication.javatests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/src/main/resources/application.ymltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/pom.xmltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/src/main/java/com/example/hcproviderb/HcProviderBApplication.javatests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/src/main/resources/application.ymltests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-consumer/main.pytests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.pytests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-b/main.pytests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/package.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/src/index.tstests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/tsconfig.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/package.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/src/index.tstests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/tsconfig.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/package.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/src/index.tstests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/tsconfig.jsontests/integration/suites/uc41_health_check_withdrawal/routines.yamltests/integration/suites/uc41_health_check_withdrawal/tc01_python_withdraw_failover_recover/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc03_typescript_withdraw_failover_recover/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc04_typescript_throwing_check_degrades/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc05_java_withdraw_failover_recover/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc06_java_throwing_check_degrades/test.yaml
…ry in all 3 runtimes The feature shipped in Python (#1473), Java (#1475) and TypeScript (#1481) with no integration coverage anywhere. Everything asserted was a unit test with a stubbed publish or a Rust test that a command was enqueued; nothing exercised the chain the feature exists for — verdict, heartbeat suppression, registry withdrawal, consumer failover, and recovery through the 410 Gone re-register. uc41 adds six cases: withdrawal-through-recovery and a throwing-check negative, per runtime. Withdrawal and recovery are one timeline rather than two cases because the load-bearing claim is the SAME pid across both the outage and the restore, which a test that redoes the withdrawal cannot make. Java reads its pid from inside the process, since meshctl's pid file names the mvn wrapper and a restarted JVM underneath it would look identical from outside. The negative is the case that matters. A suite that only tests withdrawal passes just as happily on a runtime that withdraws on ANY non-healthy verdict, which is the bug — degraded must keep heartbeating. It watches the invocation log to prove the check kept running, rather than inferring it from the absence of a withdrawal. On TypeScript the obvious assertion is blind. health-check.ts prints "threw — reporting degraded" hardcoded beside the return, so it appears identically whether the verdict is degraded or healthy, and TS exposes no HTTP surface for the verdict (#1478). Under a runtime neutered to call a throw healthy, the thrown-line count, the degrade-line count and the no-unhealthy check all still pass; only the core's "Health status changed: Healthy -> Degraded" — the one signal computed from the verdict rather than printed next to it — catches it. tc04 asserts on that. Nothing sleeps through a state transition. Phases gate on the registry's actual state and on the consumer naming the other provider, with elapsed-time deadlines sized inside their step timeouts so the TIMEOUT verdict stays reachable. The registry runs at HEALTH_CHECK_INTERVAL=2 and DEFAULT_TIMEOUT_THRESHOLD=5, and the routine retries both readiness and the health-monitor timing line within one window before failing — asserting the tuning took, without failing on the race where /health answers before the log line flushes. /livez is curled once a second across the whole outage, retried once per probe so a GC pause on the heaviest fleet does not read as a dead process. Every case was proven red against a neutered runtime: a fail branch that returns healthy fails the three withdrawal cases; a throw branch that returns unhealthy fails the three negatives; and a throw branch that returns healthy while the refresh loop dies also fails all three — the agent stays resolvable there, which is what a naive negative test scores as a pass. 6 passed in 88s at --parallel 6. Total integration suite 559 -> 565. Closes #1480 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hsym5TX4sFTLUv9LrxgSjq
dhyansraj
force-pushed
the
test/1480-health-check-integration
branch
from
August 5, 2026 19:10
5375605 to
04dc982
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.py`:
- Around line 43-50: Update _read_flag to catch only FileNotFoundError and
return "ok" for a missing flag file; allow the "ok" value explicitly while
preserving supported fault states, and reject unknown or invalid flag contents
instead of mapping them to the healthy result.
- Around line 53-63: Update _trace to expose trace-file readiness or write
failures without altering the health result, and update
tc02_python_throwing_check_degrades to require successful trace setup and writes
before validating repeated flag=throw entries. Ensure the scenario fails when
hc-invocations.log is missing, empty, unwritable, or cannot record the required
invocations.
- Line 31: Update the artifact’s datetime usage to avoid an undeclared Python
3.11 dependency: either declare Python 3.11+ for this artifact, or replace the
UTC import and usage with the compatible timezone.utc approach while preserving
the existing timestamp behavior.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc01_python_withdraw_failover_recover/test.yaml`:
- Around line 308-325: Update the Phase C recovery check around the polling loop
to observe and assert the actual failover sequence: provider A must receive HEAD
/heartbeat with a 410 Gone response, followed by a POST re-registration from the
same process, before accepting its registry status as healthy. Use an existing
request trace, registry event log, or test hook, and retain the current timeout
and failure behavior.
🪄 Autofix
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 Plus
Run ID: 3454973e-4135-4712-b5de-8284483a4277
📒 Files selected for processing (31)
tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/pom.xmltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/src/main/java/com/example/hcconsumer/HcConsumerApplication.javatests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/src/main/resources/application.ymltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/pom.xmltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/src/main/java/com/example/hcprovidera/HcProviderAApplication.javatests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/src/main/resources/application.ymltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/pom.xmltests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/src/main/java/com/example/hcproviderb/HcProviderBApplication.javatests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/src/main/resources/application.ymltests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-consumer/main.pytests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-consumer/requirements.txttests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.pytests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/requirements.txttests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-b/main.pytests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-b/requirements.txttests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/package.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/src/index.tstests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/tsconfig.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/package.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/src/index.tstests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/tsconfig.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/package.jsontests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/src/index.tstests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/tsconfig.jsontests/integration/suites/uc41_health_check_withdrawal/routines.yamltests/integration/suites/uc41_health_check_withdrawal/tc01_python_withdraw_failover_recover/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc03_typescript_withdraw_failover_recover/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc04_typescript_throwing_check_degrades/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc05_java_withdraw_failover_recover/test.yamltests/integration/suites/uc41_health_check_withdrawal/tc06_java_throwing_check_degrades/test.yaml
🚧 Files skipped from review as they are similar to previous changes (24)
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/tsconfig.json
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/src/main/resources/application.yml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/tsconfig.json
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/tsconfig.json
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/src/main/resources/application.yml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/package.json
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/src/main/resources/application.yml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/pom.xml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-b/main.py
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/src/main/java/com/example/hcconsumer/HcConsumerApplication.java
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-consumer/main.py
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/pom.xml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-b/src/index.ts
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/src/index.ts
- tests/integration/suites/uc41_health_check_withdrawal/tc03_typescript_withdraw_failover_recover/test.yaml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-b/src/main/java/com/example/hcproviderb/HcProviderBApplication.java
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/src/index.ts
- tests/integration/suites/uc41_health_check_withdrawal/tc04_typescript_throwing_check_degrades/test.yaml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-consumer/package.json
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-consumer/pom.xml
- tests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades/test.yaml
- tests/integration/suites/uc41_health_check_withdrawal/tc05_java_withdraw_failover_recover/test.yaml
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/java-hc-provider-a/src/main/java/com/example/hcprovidera/HcProviderAApplication.java
- tests/integration/suites/uc41_health_check_withdrawal/artifacts/ts-hc-provider-a/package.json
| """ | ||
|
|
||
| import os | ||
| from datetime import UTC, datetime |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while IFS= read -r file; do
rg -n 'python|requires-python|3\.10|3\.11|3\.12' "$file" || true
done < <(
fd --hidden --no-ignore --type f \
--glob 'Dockerfile*' \
--glob 'pyproject.toml' \
--glob '.python-version' \
--glob '.tool-versions' \
--glob 'requirements.txt' .
)Repository: dhyansraj/mcp-mesh
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant tracked files:\n'
git ls-files | sed -n '/python|Dockerfile|pyproject|requirements|\.python-version|\.tool-versions/i{p;q}' | sort
printf '\nSearch python runtime references in tracked files:\n'
git ls-files -z | while IFS= read -r -d '' file; do
if rg -n -i 'python|requires-python|3\.10|3\.11|3\.12' "$file" >/tmp/matches.txt 2>/dev/null; then
cat /tmp/matches.txt
fi
done
printf '\nTarget file header:\n'
sed -n '1,55p' tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.pyRepository: dhyansraj/mcp-mesh
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.py'
printf 'Target file header:\n'
sed -n '1,60p' "$file"
printf '\nTarget artifact Dockerfile:\n'
if [ -f "$file" ]; then
dir="$(dirname "$file")"
sed -n '1,80p' "$dir/Dockerfile"
else
printf 'missing target file\n'
fi
printf '\nREADME runtime declarations around target artifact:\n'
if [ -f "$dir/README.md" ]; then
sed -n '1,35p' "$dir/README.md"
fiRepository: dhyansraj/mcp-mesh
Length of output: 2734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
dir='tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a'
printf 'Files in target artifact directory:\n'
find "$dir" -maxdepth 2 -print | sort
printf '\nTarget suite README runtime/runtime lines:\n'
rg -n 'python|3\.10|3\.11|3\.12|runtime|version|requirements|docker|Dockerfile' tests/integration/suites/uc41_health_check_withdrawal tests/integration/suites/uc41_health_check_withdrawal | head -120Repository: dhyansraj/mcp-mesh
Length of output: 19192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'requirements files for this suite/artifact:\n'
for f in tests/integration/suites/uc41_health_check_withdrawal/requirements.txt \
tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/requirements.txt \
tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-consumer/requirements.txt \
tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-b/requirements.txt; do
if [ -f "$f" ]; then
printf '\n%s:\n' "$f"
sed -n '1,120p' "$f"
else
printf 'missing: %s\n' "$f"
fi
done
printf '\nPython 3.11 compatibility of datetime.UTC import:\n'
python3 - <<'PY'
import ast, sys, tokenize
source = open("tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.py", encoding="utf-8").read()
tree = ast.parse(source)
imports = []
for node in tree.body:
if isinstance(node, ast.ImportFrom):
imports.append((node.module, [alias.name for alias in node.names]))
print("imports:", imports)
print("python:", sys.version.split()[0])
try:
from datetime import UTC, datetime
print("import_utc_works:", UTC, type(UTC).__name__, datetime.now(UTC).__class__.__name__)
except Exception as e:
print("import_utc_error:", type(e).__name__, e)
PYRepository: dhyansraj/mcp-mesh
Length of output: 1152
Document Python 3.11+ for this artifact or use timezone.utc.
datetime.UTC is only available in Python 3.11+, and this artifact does not declare a runtime or require Python 3.11+. Add a Python 3.11+/runtime declaration, or replace from datetime import UTC, datetime with from datetime import datetime, timezone and datetime.now(timezone.utc).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.py`
at line 31, Update the artifact’s datetime usage to avoid an undeclared Python
3.11 dependency: either declare Python 3.11+ for this artifact, or replace the
UTC import and usage with the compatible timezone.utc approach while preserving
the existing timestamp behavior.
Comment on lines
+43
to
+50
| def _read_flag() -> str: | ||
| """Current fault state. A missing file means healthy, so the agent boots | ||
| green without the test having to seed anything.""" | ||
| try: | ||
| with open(FLAG_FILE) as handle: | ||
| return handle.read().strip().lower() or "ok" | ||
| except OSError: | ||
| return "ok" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail closed for invalid and unreadable health flags.
The docstring defines only a missing file as healthy. except OSError also maps permission, directory, and I/O errors to "ok". The final branch maps every unknown value to the healthy result. A bad HC_FLAG_FILE or a flag typo can make a withdrawal case exercise the healthy path. Catch only FileNotFoundError, allow "ok" explicitly, and reject unknown values.
Proposed fix
def _read_flag() -> str:
try:
with open(FLAG_FILE) as handle:
return handle.read().strip().lower() or "ok"
- except OSError:
+ except FileNotFoundError:
return "ok"
async def vendor_health() -> dict:
flag = _read_flag()
+ if flag not in {"ok", "fail", "throw"}:
+ raise ValueError(f"unknown health flag: {flag!r}")Also applies to: 66-87
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 46-46: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(FLAG_FILE)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.py`
around lines 43 - 50, Update _read_flag to catch only FileNotFoundError and
return "ok" for a missing flag file; allow the "ok" value explicitly while
preserving supported fault states, and reject unknown or invalid flag contents
instead of mapping them to the healthy result.
Comment on lines
+53
to
+63
| def _trace(flag: str, verdict: str) -> None: | ||
| """Append one line per invocation. Best-effort: a trace write that fails | ||
| must never be the reason the health check reports something different.""" | ||
| try: | ||
| with open(TRACE_FILE, "a") as handle: | ||
| handle.write( | ||
| f"{datetime.now(UTC).isoformat()} agent={AGENT_NAME} " | ||
| f"flag={flag} verdict={verdict}\n" | ||
| ) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
scenario="$(fd --type f --glob 'test.yaml' \
tests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades |
head -n 1)"
test -n "$scenario"
rg -n -C 3 \
'HC_TRACE_FILE|hc-invocations\.log|flag=throw|verdict=raised' \
"$scenario"Repository: dhyansraj/mcp-mesh
Length of output: 1123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the provider and relevant test suite files.
fd -a '^(main\.py|health-flag|test\.yaml)$' tests/integration/suites/uc41_health_check_withdrawal \
| rg 'py-hc-provider-a/main\.py|tc02_python_throwing_check_degrades/test\.yaml|health-flag' || true
provider="$(fd --type f --glob 'main.py' tests/integration/suites/uc41_health_check_withdrawal/py-hc-provider-a | head -n 1)"
scenario="$(fd --type f --glob 'test.yaml' tests/integration/suites/uc41_health_check_withdrawal/tc02_python_throwing_check_degrades | head -n 1)"
health_flag="$(fd --type f --glob 'health-flag' tests/integration/suites/uc41_health_check_withdrawal/py-hc-provider-a | head -n 1)"
printf 'provider=%s\nscenario=%s\nhealth_flag=%s\n' "$provider" "$scenario" "$health_flag"
printf '\n--- provider header ---\n'
sed -n '1,140p' "$provider"
printf '\n--- test assertion block ---\n'
sed -n '200,240p' "$scenario"Repository: dhyansraj/mcp-mesh
Length of output: 541
Require hc-invocations.log for the throwing-check evidence.
tc02_python_throwing_check_degrades only checks that flag=throw appears at least 3 times in /workspace/hc-invocations.log; it does not fail when the file is missing, empty, or unwritable. Keep trace I/O independent from the health result, but make missing trace readiness or trace write failures fail this scenario so it cannot pass without proving repeated throwing checks.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 56-56: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(TRACE_FILE, "a")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/integration/suites/uc41_health_check_withdrawal/artifacts/py-hc-provider-a/main.py`
around lines 53 - 63, Update _trace to expose trace-file readiness or write
failures without altering the health result, and update
tc02_python_throwing_check_degrades to require successful trace setup and writes
before validating repeated flag=throw entries. Ensure the scenario fails when
hc-invocations.log is missing, empty, unwritable, or cannot record the required
invocations.
Comment on lines
+308
to
+325
| # Going back to `healthy` in the registry is not a formality: per #955 a HEAD | ||
| # heartbeat from an agent whose row is `unhealthy` is answered 410 Gone | ||
| # precisely so a bare ping cannot revive it. The only route back to healthy | ||
| # is a full POST re-register, so this poll turning green IS the 410 path. | ||
| - name: "Phase C: poll until the registry restores A (410 Gone -> POST re-register)" | ||
| handler: shell | ||
| workdir: /workspace | ||
| command: | | ||
| for i in $(seq 1 60); do | ||
| BODY=$(curl -s --max-time 3 http://localhost:8000/agents || echo '{}') | ||
| A=$(echo "$BODY" | jq -r '.agents[]? | select(.name=="hc-provider-a-py") | .status') | ||
| B=$(echo "$BODY" | jq -r '.agents[]? | select(.name=="hc-provider-b-py") | .status') | ||
| echo "t=${i}s A=$A B=$B" | ||
| if [ "$A" = "healthy" ]; then echo "RECOVERY_REGISTRY: OK after ~${i}s"; exit 0; fi | ||
| sleep 1 | ||
| done | ||
| echo "RECOVERY_REGISTRY: TIMEOUT" | ||
| capture: recovery_registry |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Assert the 410 Gone recovery branch.
Lines 316-325 only prove that provider A becomes healthy again. A direct status update or another recovery path would also satisfy this gate. The test does not prove that a heartbeat received 410 Gone and that the same process then issued POST re-registration.
Add an assertion from a request trace, registry event log, or test hook that observes both the HEAD /heartbeat 410 response and the following registration request before accepting recovery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/integration/suites/uc41_health_check_withdrawal/tc01_python_withdraw_failover_recover/test.yaml`
around lines 308 - 325, Update the Phase C recovery check around the polling
loop to observe and assert the actual failover sequence: provider A must receive
HEAD /heartbeat with a 410 Gone response, followed by a POST re-registration
from the same process, before accepting its registry status as healthy. Use an
existing request trace, registry event log, or test hook, and retain the current
timeout and failure behavior.
dhyansraj
added a commit
that referenced
this pull request
Aug 6, 2026
## Summary Release **v3.5.2**. Version bump across 486 files, both lockfiles refreshed, and the release notes. A provider withdraws itself from dependency resolution while a health check it declares reports unhealthy, and returns automatically once the check passes — in all three runtimes, with no pod restart. v3.5.1's notes said Java and TypeScript readiness could not yet reflect a dependency outage; this closes that. It also fixes **a live defect in 3.5.1**: Python `@mesh.route` and A2A agents serve none of the probe endpoints the agent chart points at, so a Python gateway 404s every probe and is restart-looped by the kubelet. ## Bump verification The over-match guard runs inline now, but it is as much under test as the diff, so I cross-checked it independently against my own criteria: - **702 changed lines carrying 3.5.2**; inline guard reports all provably mesh-owned - **Independent scan: 22 suspects**, every one in the categories known to be legitimately unanchorable — mesh's own workflow version inputs, docs prose, tutorial `CHART_VERSION`, `mesh/__init__.py`, and test config. **No third-party coordinate moved.** - `Cargo.lock` — `Locking 1 package`, one-line diff, no crates advanced (not `generate-lockfile`, which re-resolves the graph) - `Chart.lock` — six `file://../mcp-mesh-*` sub-charts plus digest; no bitnami version moved despite `helm dependency update` re-resolving - `constraints.txt` — **untouched**, correctly; a dependency move is its own PR - All six `check_release_lockfiles.py` checks pass Both runs of the bump produced byte-identical results, which is itself worth something. ## What is in it Twelve changes since v3.5.1. Nine of the underlying issues did not exist at the start of this work — they came out of reviewing the first three. - Health-check withdrawal in Python (#1473), Java (#1475) and TypeScript (#1481); `degraded` keeps heartbeating, with Python's malformed-return case aligned (#1477) - Route and A2A agents never withdraw — including, since #1489, a Java gateway's `/ready` - Python route/A2A agents now serve `/livez`, `/ready`, `/health` (#1494) — the 3.5.1 defect above - Python honours `MCP_MESH_HEALTH_CHECK_TTL` (#1493), so cadence is tunable from Helm values rather than baked into the image - TypeScript `/ready` and `/health` reflect the verdict (#1487) - Scaffold vendor gate resolves by vendor, not substring (#1484), so a `bedrock/` or `vertex_ai/` model no longer gets a probe for credentials it lacks - Generated helm values wire only usable credentials (#1487, #1496); compose healthchecks probe `/livez` (#1495) - Cross-runtime integration coverage for withdrawal, failover and recovery (#1485) ## Release notes accuracy The parked draft was written eight PRs ago and treated as a starting point, not a source of truth. Three stale claims were found and corrected: two issue numbers cited where PR numbers belonged, and one statement — that a gateway's verdict "is never published to the runtime whatever the check reports" — that #1489 had since disproved. The compose bullet deliberately makes **no restart claim**. I originally wrote one into issue #1490 and it is wrong: Docker standalone does not restart a container for being unhealthy. Tested, disproved, corrected on the issue and recorded in #1495's guard test. ## Test plan - [x] `scripts/bump_version.py` inline over-match and coverage guards both clean - [x] Independent over-match scan: 22 suspects, all verified legitimate - [x] `check_release_lockfiles.py` — six checks green - [x] `constraints.txt` confirmed unchanged - [x] Release-notes link placement: `[Full Changelog]` directly above `## v3.5.2`, `[Unreleased changes]` at `v3.5.2...HEAD`, no stacking, exactly one `## v3.5.2` - [ ] CI green before tagging
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Health-check withdrawal shipped in Python (#1473), Java (#1475) and TypeScript (#1481) with no integration coverage in any runtime. Everything asserted was either a unit test with a stubbed publish or a Rust test that a command was enqueued. Nothing exercised the chain the feature exists for:
uc41adds six cases — withdrawal-through-recovery and a throwing-check negative, per runtime.Six cases, not nine. Withdrawal and recovery are one continuous timeline with separately-asserted phases, because the load-bearing claim is the same pid across both the outage and the restore — a test that redoes the withdrawal from scratch cannot make it. Java reads its pid from inside the process (
ProcessHandle.current().pid()): meshctl's pid file names themvn spring-boot:runwrapper, so a restarted JVM under a surviving wrapper would look identical from outside.The negative is the case that matters. A suite that only tests withdrawal passes just as happily against a runtime that withdraws on any non-healthy verdict — which is the bug, since
degradedmust keep heartbeating. It watches the invocation log to prove the check kept running, rather than inferring that from the absence of a withdrawal.Review notes
Nothing sleeps through a state transition (the #1459 lesson). Phases gate on the registry's actual state and on the consumer naming the other provider.
/livezis curled once per second across the whole outage withLIVEZ_FAILURES: 0asserted, rather than sampled at two convenient moments — "withdrawn, not dead" gets continuous coverage.The registry runs at
HEALTH_CHECK_INTERVAL=2/DEFAULT_TIMEOUT_THRESHOLD=5, turning a ~30s withdrawal into ~7s. The routine greps the registry's own startup line and exits 1 if the tuning did not take, so a renamed env var fails loudly instead of degrading into confusing 60s poll timeouts.Every case proven red against a neutered runtime, with the neuters applied to the artifacts rather than the test files:
failbranch returns healthy (≡ pre-#1472)throwbranch returns unhealthy (≡ a throw withdraws)WITHDRAWN at ~7sthrowreturns healthy and tracing stops (≡ refresh loop died)The third is the important one: the agent is not withdrawn — the outcome a naive negative test scores as a pass — and all three still fail.
Two things found while building this, worth recording:
tsuite-mesh:localimage on the cluster predated all three health-check merges, so nothing had ever been validated against a build containing the feature. Rebuilt before any run here.--parallel 6, the assumption that provider A wins the initial resolution only holds once both providers exist — B sometimes registered first and the test silently lost the property it is built on. Fixed with explicit ordering gates; a general trap for any multi-provider case added to this suite later.Scoped to the mesh machinery only. The scaffold's emitted vendor probe is not covered — it hardcodes its API host with no base-URL override, so it cannot be pointed at a stub without a template change. Deferred to #1483, noted in
routines.yaml.TypeScript's
/healthis FastMCP's built-in and carries no verdict (#1478), so tc03/tc04 read it from the[mesh-health]log line, with a header note marking the assertion to move once #1478 lands.Closes #1480
Test plan
tsuite run --suite-path tests/src-tests— 12/12, 376s,tsuite-mesh:localrebuilt fromfa6d2e7a2tsuite run --suite-path tests/integration --uc uc41_health_check_withdrawal --parallel 6— 6 passed, 0 failed, 136.5sSummary by CodeRabbit
New Features
Tests