fix: contexts read binary-detection + transient-disconnect worker health (6003e3bb, ab8a68f0)#2263
Draft
federicodeponte wants to merge 2 commits into
Draft
fix: contexts read binary-detection + transient-disconnect worker health (6003e3bb, ab8a68f0)#2263federicodeponte wants to merge 2 commits into
federicodeponte wants to merge 2 commits into
Conversation
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
Fixes two bugs surfaced by live testing of an agent-mode worker: feedback tickets
6003e3bb(CLI contexts read) andab8a68f0(transient-disconnect worker health + agent-mode latency). Two commits, one per ticket. Backend changes are inapps/api(in-boundary — same floom monorepo); rollout of the API + any env flags is a separate ops step via theworkeros-cloudpinned engine submodule, out of this repo.Ticket 6003e3bb —
floom contexts readprinted "Binary file." for UTF-8 JSON/textRoot cause:
contextsReadCommand(apps/mcp/src/commands/contexts.ts) calledclient.requestJson()expecting a JSON wrapper{content, is_binary, ...}, but the API endpointGET /contexts/{name}/files/{path}(apps/api/routers/contexts.py::get_context_file) returns the raw file bytes (served inline with the mime-type for text,Content-Disposition: attachmentfor binary/active-markup).parseResponsethen JSON-parses the body — a.jsonfile becomes a parsed object with no top-levelcontentstring, other text becomes{text}— sotypeof file.content === "string"was always false and every text file fell through to the "Binary file." branch. This was broken for all text files (json, md, yaml, txt), not just JSON.Fix:
contextsReadCommandnow mirrors the MCP server's already-correctreadContextFile(apps/mcp/src/server.ts): fetch the context detail (GET /contexts/{name}) for the server-computedis_binarymetadata (the single source of truth —is_binary_filereturns False for all text including active-markup); if truly binary, show the notice + download URL; otherwise fetch the raw bytes via a newrequestText()client helper (apps/mcp/src/lib/api.ts) and print the UTF-8 text. A missing file now returns a clear non-zero error.Files:
apps/mcp/src/commands/contexts.ts,apps/mcp/src/lib/api.ts,apps/mcp/test/contexts-read.test.js(new regression tests: text JSON prints content, markdown prints content, genuinely-binary shows the notice and does not dump bytes, missing file errors).Tests:
npm run buildclean;npm test174/174 pass (independently re-run, includes the 3 new tests).Ticket ab8a68f0 — transient "Server disconnected" left the worker in needs_attention; agent-mode latency
(a) Transient disconnect must not flip worker health (fixed)
Root cause: two issues. (1) Transport "Server disconnected" errors were bucketed into the broad catch-all
agent_runtime_errorinagent_driver.py, conflating pure infra blips with genuine agent/worker bugs. (2)_resolve_worker_status(apps/api/services/worker_serialize.py) downgraded HEALTHY -> NEEDS_ATTENTION whenever the last run FAILED, with no exemption for transient/retryable infra failures that are being retried or later succeed — so a single disconnect flipped the worker to needs_attention even though the agent had already reached its correct outcome.Fix:
agent_runtime_disconnected):agent_driver.pynow walks the exception cause chain and recognizes httpx/httpcore/openai/litellm transport drops (RemoteProtocolError, ConnectError, ReadError, "server disconnected"/"connection reset"/…) before the generic catch-all. Added to the transient retry set and torun_metricsas categorynetwork.apps/api/services/retry_classification.py(single source of truth;run_service.pyre-imports under its existing private names — no behavior change there). DRY._resolve_worker_statusnow takeslast_run_error_codeand does not downgrade to NEEDS_ATTENTION when the last failure is a transient-infra code. Threaded through both call sites (list path viaworker_listing.py, detail path via_build_worker_detail). Genuine failures (permanent codes, schema/validation, missing secret/connection, real agent bugs) still downgrade exactly as before.recent_runs[0]), so a successful retry clears health naturally; while the retry is queued, the transient parent is exempt.overview.pyfailure-cluster derivation now skips a lone (failure_count == 1) transient-infra failure so a single blip is not surfaced as a worker-health problem; genuine clusters still surface.Files:
apps/api/runner_sandbox/agent_driver.py,apps/api/run_service.py,apps/api/services/retry_classification.py(new),apps/api/services/run_metrics.py,apps/api/services/worker_serialize.py,apps/api/routers/worker_listing.py,apps/api/routers/overview.py,apps/api/tests/test_worker_health_transient_disconnect.py(new, 8 tests).Tests:
py_compileclean on all 8 changed Python files; the newtest_worker_health_transient_disconnect.py8/8 pass (independently re-run).test_monitoring_apis.py53 pass / 2 fail andtest_1080_...overviewpass — the 2 failures areModuleNotFoundError: croniter/agents, a pre-existing environment dependency gap that fails identically on cleanmain(verified in a scratch worktree), not caused by this change. This dev host has noapps/api/venv, so full-suite runs are gated by those missing optional deps.(b) Agent-mode latency (~3-5 min for trivial decisions) — investigation + low-risk fix + proposal
Investigation (grounded in code): agent-mode runs the LLM/tool loop in
AgentDriver._run_agent_innerand do not enter the script driver's sandbox acquisition (E2BSandboxDriver._run_in_sandbox). An E2B sandbox is created only when the agent invokesrun_command, viaAgentDriver._run_command_e2b. So a true no-op run ("found 0 inbound, decided not to send") already avoids sandbox cold-start entirely — the dominant latency for trivial decisions is the model loop, MCP connection, and remote tool calls, not sandbox spin-up. (Conclusion from code-path analysis; production timings were not available on this host.)Implemented (low-risk, gated off by default): wired the agent-mode
run_commandE2B path through the existing warm sandbox pool (_warm_pool_lease/_warm_pool_return), which previously only benefited the script driver. It is gated behindWORKEROS_E2B_WARM_POOL_ENABLED(default off, so production default behavior is unchanged), uses a distinctagent-command:pool key namespace (agent command sandboxes have a different directory lifecycle than script sandboxes and must not share entries), and adds_reset_agent_command_sandboxto clear per-call files before a sandbox is returned to the pool. Verified all imported warm-pool helpers exist with matching signatures.Proposed (not implemented — needs ops + measurement):
WORKEROS_E2B_WARM_POOL_ENABLED=1, startWORKEROS_E2B_WARM_POOL_SIZE_PER_KEY=1, keepWORKEROS_E2B_WARM_POOL_MAX_AGE_SECONDS=900(consider 300-900 after measuring utilization/cost); raise size to 2 only for concurrent same-key command workloads. Rollout is via theworkeros-cloudengine submodule + deployed API config, outside this repo._run_agent_innerbefore the full agent loop that exposes only tools with verified read-only metadata (withholdingrun_commandand all side-effecting MCP/connection tools) and returns immediately when the structured decision is "no action required"; if action is required, pass the decision transcript into the existing full loop. Launch in shadow mode first and measure context-staging, MCP-connect, model-call, tool-call, and E2B timings separately. Primary risks: incorrect side-effect classification, false no-op decisions, missing read-only metadata, and adding a second model call to actionable runs.🤖 Generated with Claude Code