test(integration): remove flaky dependencies from the core suite (ENG-3999) - #222
Conversation
…-3999) The Core integration lane was red on roughly half the runs. Each failure is fixed at its root, with no blanket retries and no ignored failures. Self-hosted echo server replaces httpbin.org (~29 tests). httpbin rate-limits the shared Blaxel egress IPs and answers 503 in bursts, taking the whole proxy suite down with it. Tests now run one sandbox with a node echo server behind a public preview, set up in ~3s per pytest session. x-blaxel-request-id is now asserted on the response the client receives rather than upstream: the cluster gateway strips inbound x-blaxel-* headers by design, so it can never reach a Blaxel-hosted endpoint. This checks what the proxy actually guarantees to its caller. Cleanup is scoped to the run that created the resources. Every sandbox carries a run-id label and the session teardown only deletes its own, plus orphans older than two hours. Deleting by env=integration-test tore down sandboxes that concurrent CI runs were still using (ENG-3283). Two assertions could never pass: - drive.size: the control plane clears Spec.Size on create; the field is legacy and deliberately kept out of the OpenAPI spec. - extra_args nvme: no longer a valid key (iptables, nfs, tun), removed in ENG-3722. Tests move to tun, and now also cover key validation and the nfs+iptables conflict. Fixed sleeps before assertions are replaced by wait_until() with a deadline, and the preview poll loop retries on transient network errors instead of failing on the first ReadTimeout. Verified on workspace main: two consecutive full runs green, 246 passed / 21 skipped / 0 failed in 8m41 and 8m42. Proxy suite alone: 53 passed in 67s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🧪 Testing GuideWhat this PR addressesThe core integration test suite was red on ~50% of CI runs, blocking SDK merges. The root causes were:
Steps to reproduce the original issue
What to verify (expected behavior)
Note Posted by PR Testing Guide · Tag @mendral-app with feedback. |
|
✅ Linked to Linear issue ENG-3999 — status already In Progress, PR attached to the issue. Note Posted by Linear Issue Enforcer · Tag @mendral-app with feedback. |
🔀 Component Interaction DiagramHere's how the key components interact based on this PR's changes: sequenceDiagram
participant Runner as Test Runner
participant Helpers as helpers (run_id, wait_until)
participant Echo as Echo Server (sandbox)
participant Conftest as conftest (cleanup)
participant Proxy as Proxy Tests
participant Sandbox as Sandbox Tests
participant API as Blaxel API
Runner->>Helpers: generate run_id (UUID)
Runner->>API: create echo sandbox (cached per session)
API-->>Echo: Node.js HTTP server started
rect rgb(240, 248, 255)
note right of Proxy: Proxy Test Flow
Proxy->>API: create sandbox (labels: run_id, pytest)
Proxy->>Echo: echo_host() → get hostname
Proxy->>API: write_echo_url() → /tmp/echo-url
Proxy->>Echo: HTTP request via proxy (GET /headers, /get, etc.)
Echo-->>Proxy: response + X-Blaxel-Request-Id header
Proxy->>Proxy: parse_response_headers() → validate proxy stamp
end
rect rgb(245, 255, 245)
note right of Sandbox: Sandbox Tests (polling)
Sandbox->>API: trigger async operation (process, filesystem, session)
loop wait_until(predicate, timeout=10s)
Sandbox->>API: poll for condition
API-->>Sandbox: not ready / ready
end
end
rect rgb(255, 245, 238)
note right of Conftest: Session Cleanup
Conftest->>API: list all sandboxes
API-->>Conftest: sandbox list
Conftest->>Conftest: filter: run_id match OR stale orphan (>2h)
Conftest->>API: parallel delete matching sandboxes
end
Summary of the Flow
The core architectural change replaces external service dependencies and timing assumptions with deterministic, self-contained infrastructure — echo server for HTTP testing, Note Posted by PR Sequence Diagram · Tag @mendral-app with feedback. |
![mendral-app[bot]](https://avatars.githubusercontent.com/in/1975249?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # Unique per pytest process. CI runs of several PRs share one workspace, so the | ||
| # end-of-session cleanup must only delete what *this* run created -- deleting by | ||
| # ``env=integration-test`` alone tears down sandboxes a concurrent run is still | ||
| # using, which is a large part of the suite's cross-run flakiness. | ||
| run_id = uuid.uuid4().hex[:12] |
There was a problem hiding this comment.
🟡 Test cleanup deletes nothing when the suite is run in parallel
Each parallel test process generates its own random run tag (run_id = uuid.uuid4().hex[:12] at tests/helpers/utils.py:20), while the end-of-run cleanup only deletes resources tagged with the tag of the coordinating process, so nothing created by the parallel workers is ever cleaned up.
Impact: When tests run in parallel, every test sandbox is left running in the workspace instead of being deleted.
Why the master's run-id never matches a worker's
run_id is module-level state computed at import time, so with pytest-xdist each worker process imports tests.helpers separately and gets a different value. Sandboxes are created inside workers (with default_labels carrying the worker's run-id), but pytest_sessionfinish runs only on the master (tests/integration/core/conftest.py:43-44) and filters with labels.get("run-id") == run_id (tests/integration/core/conftest.py:52) using the master's own value. The fallback branch only matches resources older than 2 hours, so freshly created worker sandboxes survive. The previous implementation deleted by env=integration-test, which was worker-agnostic.
A fix is to derive the run id from a value shared across workers, e.g. propagate the master's id through an environment variable (os.environ.setdefault("BL_TEST_RUN_ID", uuid4().hex[:12])) so workers inherit it.
| # Unique per pytest process. CI runs of several PRs share one workspace, so the | |
| # end-of-session cleanup must only delete what *this* run created -- deleting by | |
| # ``env=integration-test`` alone tears down sandboxes a concurrent run is still | |
| # using, which is a large part of the suite's cross-run flakiness. | |
| run_id = uuid.uuid4().hex[:12] | |
| # Unique per pytest run. CI runs of several PRs share one workspace, so the | |
| # end-of-session cleanup must only delete what *this* run created -- deleting by | |
| # ``env=integration-test`` alone tears down sandboxes a concurrent run is still | |
| # using, which is a large part of the suite's cross-run flakiness. | |
| # Stored in the environment so pytest-xdist workers inherit the master's id. | |
| run_id = os.environ.setdefault("BL_TEST_RUN_ID", uuid.uuid4().hex[:12]) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| # Timestamps come back with nanosecond precision, which fromisoformat | ||
| # rejects on older Pythons -- truncate to microseconds. | ||
| head, _, tail = created_at.partition(".") | ||
| created = datetime.fromisoformat(f"{head}.{tail[:6]}+00:00" if tail else f"{head}+00:00") | ||
| except ValueError: | ||
| return False |
There was a problem hiding this comment.
🟡 Leftover test sandboxes from crashed runs are never cleaned up for common timestamp formats
The creation time of a leftover sandbox is reformatted by blindly appending a UTC offset (datetime.fromisoformat(...) at tests/integration/core/conftest.py:25) without removing the trailing Z, so any timestamp that is not fractional-with-at-least-six-digits fails to parse and the sandbox is silently treated as not old, meaning it is never deleted.
Impact: Sandboxes left behind by crashed test runs accumulate in the workspace forever instead of being swept after two hours.
Parsing failure cases
RFC3339 timestamps from the control plane commonly look like 2025-08-18T01:58:22Z (no fraction) or 2025-08-18T01:58:22.123Z (fewer than 6 fractional digits). In the first case tail is empty and the code builds "2025-08-18T01:58:22Z+00:00"; in the second tail[:6] keeps the Z, producing "...123Z+00:00". Both raise ValueError (Python 3.10 fromisoformat also rejects Z), and the except ValueError: return False branch makes _is_stale_orphan always return False. The rest of the codebase normalizes with .replace("Z", "+00:00") (e.g. src/blaxel/core/sandbox/types.py:37).
| try: | |
| # Timestamps come back with nanosecond precision, which fromisoformat | |
| # rejects on older Pythons -- truncate to microseconds. | |
| head, _, tail = created_at.partition(".") | |
| created = datetime.fromisoformat(f"{head}.{tail[:6]}+00:00" if tail else f"{head}+00:00") | |
| except ValueError: | |
| return False | |
| try: | |
| # Timestamps come back with nanosecond precision and a trailing ``Z``, | |
| # which fromisoformat rejects on older Pythons -- normalise both. | |
| stamp = created_at.strip().rstrip("Z") | |
| head, _, tail = stamp.partition(".") | |
| created = datetime.fromisoformat(f"{head}.{tail[:6]}+00:00" if tail else f"{head}+00:00") | |
| except ValueError: | |
| return False |
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| sandboxes = await SandboxInstance.list() | ||
| for sb in sandboxes: | ||
| labels = sb.metadata.labels | ||
| # Labels are stored in additional_properties of MetadataLabels object | ||
| if labels is not None: | ||
| props = getattr(labels, "additional_properties", {}) or {} | ||
| if props.get("env") == "integration-test": | ||
| try: | ||
| await sb.delete() | ||
| except Exception: | ||
| pass | ||
| except Exception as e: | ||
| print(f" Error listing sandboxes: {e}") | ||
|
|
||
| # Clean up volumes with test labels | ||
| try: | ||
| volumes = await VolumeInstance.list() | ||
| for vol in volumes: | ||
| labels = vol.metadata.labels if hasattr(vol, "metadata") and vol.metadata else None | ||
| # Labels are stored in additional_properties of MetadataLabels object | ||
| if labels is not None: | ||
| props = getattr(labels, "additional_properties", {}) or {} | ||
| if props.get("env") == "integration-test": | ||
| try: | ||
| await vol.delete() # type: ignore[attr-defined] | ||
| except Exception: | ||
| pass | ||
| except Exception as e: | ||
| print(f" Error listing volumes: {e}") | ||
| sandboxes = [] | ||
| await asyncio.gather( | ||
| *(sb.delete() for sb in sandboxes if is_ours(sb)), | ||
| return_exceptions=True, | ||
| ) |
There was a problem hiding this comment.
🟡 Cleanup only looks at the first 50 sandboxes in the workspace
The end-of-run cleanup inspects only the first page of the workspace listing (await SandboxInstance.list() at tests/integration/core/conftest.py:69, which returns at most 50 entries), so sandboxes created by this run that fall outside that page are never deleted.
Impact: In a busy shared workspace, test sandboxes silently survive the cleanup and keep consuming resources.
Pagination detail
SandboxInstance.list() returns an AsyncPaginatedList page with limit=50 by default (src/blaxel/core/sandbox/default/sandbox.py:379-381); it is list-like, so iterating it silently yields only the first page. Since orphaned resources accumulate in the shared workspace (the volume sweep was also removed in this PR), exceeding 50 sandboxes is likely. Use auto_paging_iter() to walk every page before filtering with is_ours.
| try: | |
| sandboxes = await SandboxInstance.list() | |
| for sb in sandboxes: | |
| labels = sb.metadata.labels | |
| # Labels are stored in additional_properties of MetadataLabels object | |
| if labels is not None: | |
| props = getattr(labels, "additional_properties", {}) or {} | |
| if props.get("env") == "integration-test": | |
| try: | |
| await sb.delete() | |
| except Exception: | |
| pass | |
| except Exception as e: | |
| print(f" Error listing sandboxes: {e}") | |
| # Clean up volumes with test labels | |
| try: | |
| volumes = await VolumeInstance.list() | |
| for vol in volumes: | |
| labels = vol.metadata.labels if hasattr(vol, "metadata") and vol.metadata else None | |
| # Labels are stored in additional_properties of MetadataLabels object | |
| if labels is not None: | |
| props = getattr(labels, "additional_properties", {}) or {} | |
| if props.get("env") == "integration-test": | |
| try: | |
| await vol.delete() # type: ignore[attr-defined] | |
| except Exception: | |
| pass | |
| except Exception as e: | |
| print(f" Error listing volumes: {e}") | |
| sandboxes = [] | |
| await asyncio.gather( | |
| *(sb.delete() for sb in sandboxes if is_ours(sb)), | |
| return_exceptions=True, | |
| ) | |
| try: | |
| page = await SandboxInstance.list() | |
| sandboxes = [sb async for sb in page.auto_paging_iter()] | |
| except Exception as e: | |
| print(f" Error listing sandboxes: {e}") | |
| sandboxes = [] | |
| await asyncio.gather( | |
| *(sb.delete() for sb in sandboxes if is_ours(sb)), | |
| return_exceptions=True, | |
| ) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| assert no_proxy_headers.get("x-proxy-compare") is None | ||
| assert no_proxy_headers.get("x-api-key") is None | ||
| assert no_proxy_headers.get("x-blaxel-request-id") is None | ||
| assert parse_response_headers(no_proxy_result["logs"]).get("x-blaxel-request-id") is None |
There was a problem hiding this comment.
🔍 Echo endpoint is now a Blaxel-hosted preview, which may itself stamp response headers
test_proxy_injects_headers_no_proxy_does_not asserts that the no-proxy sandbox's response contains no x-blaxel-request-id. Previously the target was httpbin.org (unrelated infrastructure); now it is a Blaxel preview served by the Blaxel edge. If the preview gateway ever stamps x-blaxel-request-id on responses it serves, this negative assertion becomes a false failure even though the proxy behaved correctly. Worth confirming with the platform team that only the egress proxy — not the preview edge — adds that response header.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Checked this against the gateway source rather than leaving it open.
The preview edge does not stamp x-blaxel-request-id on responses. In cluster-gateway, every insert_header("X-Blaxel-*") call is on the upstream request (src/proxy/upstream_handler.rs), never on the response. The only headers it touches on the response path are RESERVED_PLATFORM_RESPONSE_HEADERS (src/proxy/constants.rs) — X-Blaxel-Source, X-Blaxel-Error-Code, X-Blaxel-Dispatch-State — and those are stripped, not added.
The header comes solely from the MITM proxy, which sets it on the response it hands back to the caller (agent-proxy/internal/mitm/handler.go). So the negative assertion holds: a no-proxy sandbox hitting the preview sees no x-blaxel-request-id.
Confirmed empirically too — the full proxy suite passes, including this test.
Cleanup swept only the first page of sandboxes. SandboxInstance.list() returns one 50-entry page; the shared workspace currently holds 126, so more than half the sandboxes escaped the sweep. Walk every page with auto_paging_iter(). Stale-orphan detection never matched. The API returns RFC3339 timestamps with a trailing Z, sometimes without a fraction; appending the UTC offset without dropping the Z always raised, so every orphan was silently treated as fresh and nothing was ever swept. The run id now lives in the environment. With pytest-xdist each worker imports tests.helpers in its own process and would mint its own id, so the master's teardown -- the only one that runs -- would match nothing the workers created. Both predicates moved to tests/helpers so they can be unit tested: the Z-suffix bug shipped precisely because conftest internals had no test. Added tests/core/test_cleanup_helpers.py covering every timestamp shape the API returns, the age boundary, and the fail-safe paths. echo_url() now double-checks under an asyncio.Lock so two concurrent callers cannot each create a sandbox. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…x-integration-test-flakiness # Conflicts: # tests/helpers/utils.py # tests/integration/core/conftest.py # tests/integration/core/sandbox/test_extra_args.py
1 participant
The Core integration lane was red on about half the runs, blocking every SDK merge. Each failure is fixed at its root: no blanket retries, no ignored failures.
Verified on workspace
main: two consecutive full runs green — 246 passed / 21 skipped / 0 failed (8m41 and 8m42). Proxy suite alone: 53 passed in 67s.What was actually breaking
httpbin.orgreturning 503 in burstsrun-idlabelassert drive.size == 10extra_args: nvmetunwait_until()with a deadlineReadTimeoutinside a poll loopSelf-hosted echo server
httpbin.orgrate-limits the shared Blaxel egress IPs, so the whole proxy suite went down together whenever it answered 503. Tests now run their own echo server: one sandbox, a small node server, a public preview. Setup costs ~3s per pytest session, and from the sandbox's point of view it is still a real external HTTPS host, so the proxy and firewall paths are exercised exactly as before.One behaviour change worth flagging:
x-blaxel-request-idis now asserted on the response the client receives, not upstream. The cluster gateway strips inboundx-blaxel-*headers by design, so that header can never reach a Blaxel-hosted endpoint. Checking the response is what the proxy actually guarantees to its caller.Cleanup no longer breaks concurrent runs
Every sandbox carries a
run-idlabel and the session teardown deletes only its own, plus orphans older than two hours. Deleting everything labelledenv=integration-testtore down sandboxes that a concurrent CI run was still using — that is ENG-3283.Volumes are not swept:
VolumeInstance.list()returnsLiteVolumeMetadata, which carries no labels, so there is no way to tell ours apart without a GET per volume. Volume tests already delete what they create.Two assertions that could never pass
drive.size— the control plane setsSpec.Size = nilon create and deliberately keeps the field out of the OpenAPI spec (legacy).extra_args: {"nvme": "enabled"}— no longer a valid key (iptables,nfs,tun), removed in ENG-3722. Tests move totunand now also cover key validation and thenfs+iptablesconflict.Also removed the
_is_transient_httpbin_gateway_response+pytest.skipworkaround intest_comparison.py: it hid real failures and no longer has a reason to exist.Platform issues found along the way (not fixed here)
extra_args: {"iptables": "enabled"}and{"nfs": "enabled"}return500 DEPLOYMENT_FAILEDin every region, reproducible every time.tunworks. These select mk3.0 kernels, and ENG-4739 (removing generation 3.0) was cancelled, so they are meant to still be supported.VolumeInstance.list()does not expose labels, so orphaned test volumes accumulate (50 currently inmain).🤖 Generated with Claude Code
Note
Follow-up commit adds
asyncio.Locktoecho_url()(addressing previous review), fixes pagination in cleanup to walk all pages viaauto_paging_iter(), fixes RFC3339 Z-suffix timestamp parsing inis_stale_orphan, moves run-id to environment for pytest-xdist compatibility, and adds unit tests for the cleanup predicates.Written by Mendral for commit b944a46.