Skip to content

timeouts.total_seconds is documented and set in every example config, but never enforced #140

Description

@shrdgn

What's wrong

TimeoutsConfig.total_seconds (openfusion/config.py:268, default 300s) is a first-class config field that every example config sets explicitly:

$ grep -A3 '^timeouts:' examples/*.yaml.example
examples/bench-vote.yaml.example:  member_seconds: 90 / judge_seconds: 120 / total_seconds: 240
examples/dev.yaml.example:         member_seconds: 60 / judge_seconds: 90  / total_seconds: 180
examples/draco.yaml.example:       member_seconds: 240 / judge_seconds: 180 / total_seconds: 350
...

It reads as an overall per-request wall-clock cap — the name and the fact that every example sets a value alongside member_seconds/judge_seconds implies it bounds the sum. But grepping the whole openfusion/ package, it's read nowhere outside config.py itself:

$ grep -rn "total_seconds" openfusion/*.py
openfusion/config.py:268:    total_seconds: float = Field(default=300.0, gt=0)

Only member_seconds and judge_seconds are ever consulted (panel.py, stream.py, pipeline.py). Nothing bounds the aggregate duration of a request. A panel member's _call_member gets a fresh full member_seconds deadline, can retry up to 3× with exponential backoff on 429s within that deadline, and the judge synthesis timeout runs independently after — so total latency can legitimately run well past what an operator configured total_seconds to believe was the ceiling.

Why it matters

  • An operator who sets total_seconds to bound worst-case request latency (e.g. for an SLA, or a reverse proxy's own upstream timeout) gets no such guarantee.
  • It's actively misleading in at least one shipped example: examples/draco.yaml.example sets member_seconds: 240, judge_seconds: 180, total_seconds: 350 — i.e. total_seconds (350) is less than member_seconds + judge_seconds (420). If this field were naively enforced as a hard asyncio.wait_for wrapping the whole request, that config would start failing legitimate slow-but-successful requests. That inconsistency is itself evidence operators (including this repo's own example authors) don't have a working mental model of what the field currently does, because right now it does nothing.

Why I didn't just fix it in this pass

Enforcing this correctly needs a design decision, not just wiring up asyncio.wait_for:

  • Streaming requests (the common case) can't be wrapped with a single asyncio.wait_for around the whole call — the response is a lazily-consumed async generator, so the deadline has to be enforced inside the generator (e.g. extending _stream_with_cancellation's existing disconnect-watcher-task pattern in server.py with a sibling timeout watcher), including deciding what the client sees when a request is cut short mid-stream (silent truncation vs. an SSE error chunk).
  • Existing example configs are inconsistent with a literal sum-based reading of the field (see draco.yaml.example above) — those need auditing/fixing before enforcement ships, or the bench CI workflow that exercises draco.yaml.example could start failing.
  • It's unclear whether the intended semantics are a hard cancel (abort and error) or a soft/logged warning (finish the request but flag it) — that's a product call.

Suggested next step

Someone with product context should decide the intended semantics, then:

  1. Fix examples/*.yaml.example so total_seconds >= member_seconds + judge_seconds (with headroom for retries) wherever it currently isn't (at minimum draco.yaml.example).
  2. Enforce the deadline for non-streaming responses (buffer_synthesis/buffer_vote/buffer_ranked/buffer_pipeline in stream.py/pipeline.py) via asyncio.wait_for, mapping a timeout to a proper error response.
  3. Enforce it for streaming responses via a timeout-watcher task alongside the existing _watch_disconnect in server.py's _stream_with_cancellation, with a defined client-facing behavior on trip (e.g. an SSE error chunk before [DONE]).
  4. Add tests covering both paths, and a bench/dev-config smoke check that no example config configures an impossible-to-satisfy total_seconds.

Happy to pick this up in a follow-up PR once the semantics are confirmed.


Found via an automated repo-review scheduled task, cross-checked by hand against the current main (no matches for total_seconds outside config.py; example config values confirmed via grep).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions