Skip to content

Fix max_retries=0, wait timeouts, run_many retries and partial failures, input encoding - #89

Open
jadenfix wants to merge 8 commits into
roe-ai:mainfrom
jadenfix:fix/correctness
Open

jadenfix wants to merge 8 commits into
roe-ai:mainfrom
jadenfix:fix/correctness

Conversation

@jadenfix

@jadenfix jadenfix commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Small, independent fixes, one commit each so any can be dropped:

  • max_retries=0 was ignored. RoeConfig.from_env used max_retries or <env>, so 0 fell back to ROE_MAX_RETRIES / 3.
  • Job.wait / JobBatch.wait overran the timeout. They always slept a full interval after the deadline check, so wait(interval=60, timeout=1) raised after 60s. Now they sleep at most until the deadline and use time.monotonic().
  • FileUpload(path=...) agent inputs leaked a file descriptor. The handle opened by to_multipart_tuple() was never closed; read the bytes in a with block like plain path strings already are.
  • User-Agent said roe-python/0.1.0. Now uses roe.__version__.
  • run_many chunks were retried. The transport retried the batch POST on 5xx/408/429, so one chunk could create (and bill) its jobs up to 4 times. It now sends x-roe-skip-retry like the single-run helpers.
  • run_many lost job IDs when a later chunk failed. The exception now carries submitted_job_ids for the jobs earlier chunks already started.
  • Dict and list agent inputs were sent as Python repr ({'a': 'b'}). They're now JSON-encoded; other types are unchanged.
  • JobBatch.wait hung until its timeout (2h by default) when the status response left out a job (e.g. a deleted ID). It now raises NotFoundError naming the missing IDs on the first poll, like the Go SDK does.

Testing

  • New tests: test_config.py, test_job_wait.py (fake clock), test_inputs.py, test_auth.py, plus asserts in test_agents_wrapper_transport.py. Each fails before its fix and passes after.
  • uv run pytest, ruff check, ruff format --check.
  • New test files are added with git add -f because .gitignore contains tests/.

Testing against the live API requires a Roe API key, so these were verified with unit tests and local mock servers only.

Fixes #87. Part of #85 (only run_many; the create/upload POSTs are unchanged) and #86 (only dict/list encoding; plain-string file-path detection needs your call).

`max_retries or <env>` treated 0 as "not provided", so passing
max_retries=0 fell back to ROE_MAX_RETRIES or the default 3 and
retries could not be turned off from code.

Tested: new tests/unit/test_config.py fails before, passes after;
full suite passes.
Both loops checked the deadline and then always slept a full
`interval`, so wait(interval=60, timeout=1) raised after 60s. Sleep
at most until the deadline, and use time.monotonic() so wall-clock
changes don't affect the timeout.

Tested: tests/unit/test_job_wait.py (fake clock) fails before with
60s elapsed, passes after; full suite passes.
build_execution_multipart passed the handle from
FileUpload.to_multipart_tuple() to httpx, and nothing closed it, so
every run with a path-based FileUpload leaked a file descriptor. Read
the bytes inside a `with` block, the same way plain path strings are
already handled a few lines below.

Tested: tests/unit/test_inputs.py fails before (open BufferedReader
returned), passes after; full suite passes.
The header was hard-coded to roe-python/0.1.0 while the package is
1.1.15, so server-side logs can't tell SDK versions apart. Use
roe.__version__ (imported lazily to avoid a circular import).

Tested: tests/unit/test_auth.py fails before ('roe-python/0.1.0'),
passes after; full suite passes.
@greptile-apps

greptile-apps Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium impact] Fixes timeout logic, retry config, file handle leak, and User-Agent staleness.

Fix the extra poll after the deadline before merging; the upload memory increase is a non-blocking concern.

Fix All in Claude CodeFindings

  1. P1 Waiting continues after timeout ▶
  2. P2 Uploads hold entire files ▶

Summary

The PR fixes four SDK behaviors and adds focused unit tests:

  • Explicit zero retries stay at zero.
  • Job waits stop sleeping at their deadline.
  • Path-based uploads close their file after reading.
  • API requests use the installed package version.

Waiting can still start another request after its deadline. Path-based uploads also now hold entire files in memory.

Reviews (1) · Last reviewed commit: "Send the package version in the User-Age..." · Reviewed by Greptile

Comment thread src/roe/models/job.py
)

time.sleep(interval)
time.sleep(min(interval, remaining))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Waiting continues after timeout

Job.wait() and JobBatch.wait() still start another status request after sleeping to the deadline. With interval=60, timeout=1, a pending job sleeps for one second, then makes another request before checking the timeout. That request uses the client's separate timeout and retry settings, so waiting can still run well past one second.

Check the deadline before starting another poll, preserve raise_on_timeout=False, and add a test that rejects requests made after the deadline.

Knowledge Base Used: Agent job execution and files

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last poll at the deadline is intentional. main also polled once after its final sleep, and could do it well past the deadline (interval=60, timeout=1 polled at t=0 and t=60). Now that last poll lands exactly on the deadline and never after, so a job that finishes in the final window is returned instead of reported as a timeout, and there are no more requests than before.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s fair — I treated the timeout as a hard cutoff for starting another poll, while this change intentionally preserves the existing final poll and only prevents it from starting after the deadline. Sleeping for remaining moves that poll from potentially well past the deadline to the deadline boundary, so a completion observed by that poll is returned. I’ll withdraw the request to reject the deadline poll; the test should allow the final poll at the deadline and ensure no later poll is started.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

Comment thread src/roe/utils/inputs.py
with open(value.path, "rb") as fh:
files[key] = (
value.effective_filename,
fh.read(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Uploads hold entire files

FileUpload(path=...) now reads the entire file into memory and keeps those bytes until the request finishes. Previously this path passed an open file object to HTTPX. If callers upload several large files, they now need memory for their combined sizes before any upload starts.

Keep the file objects open through the request and close SDK-owned handles afterward, for example with an ExitStack, rather than fixing the leak by buffering every file.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plain path-string inputs in the same function are already read into memory with with open(...), so this makes FileUpload(path=...) consistent with them and fixes the leaked descriptor. Keeping SDK-owned handles open across the request would mean changing what build_execution_multipart returns so call_dynamic can tell them apart from caller-owned file_objs. I'd rather do that for both input styles in a follow-up than widen this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s fair. Since plain path-string inputs in build_execution_multipart already buffer file contents inside a with open(...) block, buffering FileUpload(path=...) is consistent with the function’s existing behavior and closes the SDK-owned descriptor before returning.

The memory tradeoff is real for large uploads, but addressing it would require an API/lifecycle change to carry handle ownership through call_dynamic, as you noted. I’m withdrawing this finding for the scope of this PR; a follow-up can make both path-based input styles stream with explicit handle ownership.

retrieve_status_many returns only the jobs the server knows about, and
JobBatch.wait skipped any it didn't get back. A deleted or mistyped job
ID therefore never reached a terminal state, and wait() polled until
its timeout (two hours by default) before raising TimeoutError. Raise
NotFoundError naming the missing IDs on the first poll instead, as the
Go SDK does. IDs are compared in canonical UUID form, so callers who
pass upper-case IDs see no change.

The existing batch timeout test built status items as dicts, whose IDs
JobBatch can't read, so it was passing through this same path; it now
uses real UUIDs and attribute-style items.

Tested: new test in tests/unit/test_job_wait.py fails before
(TimeoutError after the full 60s) and passes after; full suite, ruff
check and ruff format --check pass.
RoeRetryTransport retried the run_async_many POST on 5xx/408/429, which
can submit (and bill) a chunk up to four times. Send x-roe-skip-retry like
the single-run helpers do. Fixes roe-ai#85.
A failure on a later chunk discarded the IDs of jobs already started by
earlier chunks. Attach them to the exception as submitted_job_ids and
re-raise it unchanged. Fixes roe-ai#87.
Non-string inputs went through str(), so {"a": "b"} reached the server as
the Python repr {'a': 'b'}. JSON-encode dicts and lists; other types are
unchanged. Part of roe-ai#86.
@jadenfix jadenfix changed the title Fix max_retries=0, Job.wait timeout overrun, FileUpload handle leak, stale User-Agent Fix max_retries=0, wait timeouts, run_many retries and partial failures, input encoding Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

run_many loses job IDs when a later chunk fails

1 participant