test(meshjob): gate tc16 on real job status instead of a fixed 8s sleep - #1459
Merged
Conversation
Closes #1458 Both tc16 variants slept a flat 8 seconds and then asserted the job was completed. Under --parallel 8 the long-running job outlives that budget, so the bystanders read status=working and assertion 4 fails. Observed in 2 of 3 full parallel runs; passes scoped, which is what made it look flaky rather than under-specified. The Java twin already had this gate, which is why it never failed. Python and TypeScript were the outliers, so this brings all three into line rather than introducing a new pattern. Polls via the CONSUMER, not a bystander: the bystanders reading status without auth is the assertion under test, so polling through one would pre-empt it. Also distinguishes a broken job from a slow one. Previously a genuinely failed job produced the identical message as a slow one -- does not contain "completed" -- with the case timeout cutting off any diagnostics. The gate now exits immediately on failed/cancelled with the response body, dumps the last status and provider log tail on timeout, and the case timeout moves 120 -> 180 so that output can land. Full suite at --parallel 8: 559 passed, 0 failed, 9 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hsym5TX4sFTLUv9LrxgSjq
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Python and TypeScript MeshJob status tests replace fixed 8-second waits with consumer-side polling gates. They detect completion, failed or cancelled jobs, and timeout diagnostics. The Python test timeout increases from 120 to 180 seconds. ChangesMeshJob status polling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
added a commit
that referenced
this pull request
Aug 5, 2026
…3 runtimes (#1485) ## 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: ``` verdict → heartbeat suppression → registry withdrawal → consumer failover → recovery via 410 Gone ``` `uc41` adds 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 the `mvn spring-boot:run` wrapper, 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 `degraded` must 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. `/livez` is curled once per second across the whole outage with `LIVEZ_FAILURES: 0` asserted, 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: | neuter | result | |---|---| | `fail` branch returns healthy (≡ pre-#1472) | 3 withdrawal cases red | | `throw` branch returns unhealthy (≡ a throw withdraws) | 3 negatives red, `WITHDRAWN at ~7s` | | `throw` returns healthy and tracing stops (≡ refresh loop died) | 3 negatives red | 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: - The `tsuite-mesh:local` image 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. - Under `--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 `/health` is 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 - [x] `tsuite run --suite-path tests/src-tests` — 12/12, 376s, `tsuite-mesh:local` rebuilt from `fa6d2e7a2` - [x] `tsuite run --suite-path tests/integration --uc uc41_health_check_withdrawal --parallel 6` — **6 passed, 0 failed, 136.5s** - [x] Full integration suite green across four consecutive runs; 559 → 565 tests - [x] Each case verified red under three separate runtime neuters, all reverted byte-identically <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Python, TypeScript, and Java health-check consumer and provider test applications. - Added healthy, unhealthy, and error health states with provider identity reporting. - Added automatic provider failover and same-process recovery validation. - Added consumer responses identifying the active provider or reporting unavailable dependencies. - **Tests** - Added integration coverage for withdrawal, failover, recovery, and degraded health behavior across all three languages. - Added validation that liveness remains available during withdrawal and that throwing checks degrade without removing providers. - Added registry readiness, timing, diagnostics, and cleanup validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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
Both tc16 variants slept a flat 8 seconds waiting for a long-running job, then asserted it was
completed. Under--parallel 8the job outlives that budget and the bystanders readstatus=working:Failed in 2 of 3 full parallel runs; passes scoped — which is what made it look flaky rather than under-specified.
The Java twin already had this exact gate (
uc23/tc16~line 141), which is precisely why it never failed. Python and TypeScript were the outliers, so this brings all three into line rather than introducing a new pattern.Two things beyond "wait longer"
It polls via the consumer, not a bystander. The bystanders reading job status without auth is the assertion under test — polling through one would pre-empt the very thing being proven. The comment says so, so a future editor doesn't "simplify" it.
It distinguishes a broken job from a slow one. Previously, a job that genuinely
failedproduced the identical message as one that was merely slow —does not contain "completed"— with the case timeout cutting off any diagnostics before they landed. The gate now exits immediately onfailed/cancelledwith the response body, dumps the last status and the provider log tail on timeout, and the case timeout moves 120 → 180 so that output can actually surface.That second point is why this isn't just a flake fix: the old failure mode was uninformative by construction.
Verification
Full suite at
--parallel 8: 559 passed, 0 failed, 9 skipped.Assertions 4 and 5 are unchanged — they are the point of the test.
Follow-up, not in this PR
46 steps across the meshjob suites use
handler: wait, several standing in for real readiness conditions. tsuite 0.7.0 ships aprobehandler (interval,timeout,until,success_threshold,on_failure) that the suite uses nowhere today. Worth migrating as one reviewed change —on_failurealone would have short-circuited two separate investigations today by attachingmeshctl logs <agent> | tail -50to the failure output.Boundary for that work: probe belongs on infrastructure readiness — agent serving HTTP, registry answering, job reaching terminal state — and never on dependency or provider resolution. The settling window (#1193) exists so integration tests don't wait on DI, and a probe waiting for the mesh to converge would have hidden #1456 entirely. That test failing is what surfaced a real runtime race.
Closes #1458
Summary by CodeRabbit