Skip to content

Record a Comunica timeout as a timeout, not a pass - #35

Open
ecrum19 wants to merge 1 commit into
mainfrom
fix/comunica-timeout-pass
Open

ecrum19 wants to merge 1 commit into
mainfrom
fix/comunica-timeout-pass

Conversation

@ecrum19

@ecrum19 ecrum19 commented Oct 11, 2026

Copy link
Copy Markdown
Owner

Problem

When a Comunica endpoint's own per-query timeout fires, it has already sent HTTP 200 and the start of the result. It then ends the response with !TIMEDOUT! and kills its worker (HttpServiceSparqlEndpoint.runWorker, connection.end('!TIMEDOUT!')). The validator trusted the 200 and recorded a pass.

This is visible in the v3.3.1 provenance rerun on vcf-bench-3 (16_scale_retrieval/r1000000__hdt__hdt__core__r1):

query recorded what happened
q05 PASS, exit 0, 3600.06 s the body ends "bindings": [ followed by !TIMEDOUT!
q06–q13 EXECUTION_FAILED in < 1 ms each Connection refused: the replacement worker had not bound yet

The run as a whole was not reported as passing, because q05's body failed to parse at the comparison step. But the per-query record (benchmark.json, benchmark.csv, query-executions.json) says q05 passed, and one timeout cost eight more queries.

Fix

  • The marker means a timeout. A response ending in !TIMEDOUT! is recorded with exit 124 and EXECUTION_FAILED, the same as a client-side timeout. --stop-after-query-timeout therefore applies to it too. The truncated body is kept as evidence.
  • Exit 0 alone no longer counts as a pass, for any engine. _envelope requires a well-formed SPARQL Results JSON result: a results.bindings list or an ASK boolean. This also covers QLever and anything else that returns a 200 with a broken body.
  • The endpoint is restarted after a timeout, whether the timeout came from the server or the client, so one timeout costs one query. The restart is reported as endpointRestartSeconds on the next query and kept out of its wallSeconds. It happens only when another query is about to run, so --stop-after-query-timeout never pays for it. If the restart fails, that query records endpointRestartError and the remaining queries fail immediately; there is no repeated retry.

Tests

test/test_query_timeout_marker_unit.py (12 tests) replays the recorded q05 body byte for byte against a fake Comunica that behaves like the real one: its worker dies on a timeout and refuses connections until it is restarted. The tests include a full run_validation run with q04–q06 in which q05 times out.

  • Against main: 18 failures and errors. The end-to-end test reproduces the incident, recording q05 as PASS.
  • With the fix: the full unit suite passes (1223 tests, 2 skipped). CI's coverage commands also pass on a venv built the same way as the coverage job (pycottas==1.1.0, pyoxigraph 0.3.18), and every changed line in validation_runner.py is covered.

docs/validation.md documents the behaviour.

🤖 Generated with Claude Code

When a Comunica endpoint's own per-query timeout fires, it has already
sent HTTP 200 and the start of the result, so it ends the response with
"!TIMEDOUT!" and kills its worker. The validator trusted the 200: in the
vcf-bench-3 scale run (HG005, 1M records, HDT) q05 ran for 3600.06s and
was recorded as PASS. The replacement worker was not yet listening, so
q06-q13 were then refused within a millisecond each.

- A response ending in the marker is a timeout: exit 124,
  EXECUTION_FAILED. The truncated body is kept as evidence.
- No engine passes on exit 0 alone: the result must be SPARQL Results
  JSON (a bindings list or an ASK boolean). This covers QLever too.
- After a timeout (server- or client-side), the endpoint is restarted
  before the next query, so one timeout costs one query. The restart is
  reported as endpointRestartSeconds and kept out of wallSeconds. It is
  lazy, so --stop-after-query-timeout pays nothing for it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 99.25373% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
test/test_query_timeout_marker_unit.py 99.06% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

This branch has not been deployed

No deployments
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.

2 participants