Skip to content

Fix WebSocket disconnect during large manual log file uploads (#1062) - #343

Merged
rquidute merged 3 commits into
v2.15.1-developfrom
fix/1062-manual-log-upload-websocket-error
Jul 22, 2026
Merged

Fix WebSocket disconnect during large manual log file uploads (#1062)#343
rquidute merged 3 commits into
v2.15.1-developfrom
fix/1062-manual-log-upload-websocket-error

Conversation

@rquidute

@rquidute rquidute commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #1062: THCLI reports a WebSocket error after successfully uploading a manual test log (e.g. TC-CADMIN-1.17), even though the upload itself completed.

Root cause

ManualLogUploadStep.handle_uploaded_file() called logger.info() once per line of the uploaded file. Uploaded logs can have hundreds of thousands of lines (the reported log had ~191K). Each logger.info() call:

  • gets picked up by TestLogHandler, batched, and broadcast over the same main WebSocket the CLI is using (TestUIObserversocket_connection_manager.broadcast)
  • triggers a blocking session.commit() in TestDBObserver

This flood of broadcast + DB-commit work stalls the event loop long enough (~100s in the reported log) that the WebSocket's ping/pong keepalive isn't serviced in time, and the connection gets dropped without a proper close handshake — ConnectionClosedError: no close frame received or sent. The CLI's very next action (sending the prompt response after upload) hits the broken socket and surfaces as Unexpected error uploading file: ..., even though the file was already uploaded successfully.

Fix

  • Batch uploaded log lines into chunks of 500 before calling logger.info(), instead of one call per line — a large file now produces a handful of log/broadcast/DB-commit operations instead of hundreds of thousands.
  • Collapse the invalid-UTF-8 warning to a single message per upload instead of one per bad line.
  • Add app.uvicorn_worker.ExtendedTimeoutUvicornWorker, which raises ws_ping_timeout to 60s (matching the value already used by the dev-only start-reload.sh script), and wire it up as the default worker class in gunicorn/start.sh so production tolerates the same event-loop stalls as local dev.

Testing

  • Added unit tests covering: chunked batching behavior + line-order integrity, invalid-UTF-8 replacement/single-warning behavior, and unsupported content-type rejection.
  • Verified flake8/mypy/black/isort clean on the touched files.
  • Verified the chunking logic directly against the module (bypassing an unrelated, pre-existing local env issue where the full pytest suite requires the matter SDK checkout module not present in this sandbox).

Uploading a manual test log with hundreds of thousands of lines
(e.g. TC-CADMIN-1.17) called logger.info() once per line. Each call
triggered a broadcast over the same main WebSocket used by the CLI
and a synchronous DB commit, stalling the event loop long enough for
the WebSocket's ping/pong keepalive to time out mid-upload. The
connection was dropped with ConnectionClosedError, and the next
prompt-response send failed with 'Unexpected error uploading file'
even though the upload itself had already succeeded (issue #1062).

- Batch uploaded log lines into chunks of 500 before calling
  logger.info(), instead of one call per line, so a large file
  produces a handful of log/broadcast/DB-commit operations instead
  of hundreds of thousands.
- Collapse the invalid-UTF-8 warning to a single message per upload
  instead of one per bad line.
- Add app.uvicorn_worker.ExtendedTimeoutUvicornWorker, which raises
  ws_ping_timeout to 60s (matching the value already used by the
  dev-only start-reload.sh script), and wire it up as the default
  worker class in gunicorn/start.sh so production tolerates the same
  event-loop stalls as local dev.
- Add unit tests covering chunked logging, invalid UTF-8 handling,
  and unsupported content-type rejection.
@mergify

mergify Bot commented Jul 22, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request addresses websocket connection drops during large manual log uploads (GitHub issue #1062) by batching log lines into chunks of 500 before logging them, and by extending the uvicorn worker's websocket ping timeout to 60 seconds. Feedback suggests using .rstrip("\r\n") instead of .rstrip("\n") to properly handle Windows-style line endings and avoid formatting issues.

Comment thread app/test_engine/models/manual_test_case.py Outdated
.rstrip("\n") left a trailing \r on every line of a Windows-style
(CRLF) uploaded log, since the file is read in binary mode and split
only on \n. That stray \r would end up embedded in the joined chunk,
showing up as ^M characters or unexpected double newlines wherever
the chunk is logged or displayed.

Use .rstrip("\r\n") so both Unix and Windows line endings are
handled correctly. Add a regression test with a CRLF-terminated
upload.
@rquidute
rquidute requested a review from antonio-amjr July 22, 2026 18:19
mypy's structural check against the UploadFile protocol requires
file: BinaryIO exactly; a bare BytesIO() assignment left the
attribute inferred as BytesIO, which mypy treats as incompatible
with the protocol's BinaryIO annotation despite BytesIO satisfying
it at runtime. Annotate the attribute explicitly so
handle_uploaded_file(FakeUploadFile(...)) type-checks.
@rquidute
rquidute merged commit cb44885 into v2.15.1-develop Jul 22, 2026
7 of 8 checks passed
@rquidute
rquidute deleted the fix/1062-manual-log-upload-websocket-error branch July 22, 2026 19:17
rquidute added a commit that referenced this pull request Jul 22, 2026
…#343) (#344)

* Fix WebSocket disconnect during large manual log file uploads

Uploading a manual test log with hundreds of thousands of lines
(e.g. TC-CADMIN-1.17) called logger.info() once per line. Each call
triggered a broadcast over the same main WebSocket used by the CLI
and a synchronous DB commit, stalling the event loop long enough for
the WebSocket's ping/pong keepalive to time out mid-upload. The
connection was dropped with ConnectionClosedError, and the next
prompt-response send failed with 'Unexpected error uploading file'
even though the upload itself had already succeeded (issue #1062).

- Batch uploaded log lines into chunks of 500 before calling
  logger.info(), instead of one call per line, so a large file
  produces a handful of log/broadcast/DB-commit operations instead
  of hundreds of thousands.
- Collapse the invalid-UTF-8 warning to a single message per upload
  instead of one per bad line.
- Add app.uvicorn_worker.ExtendedTimeoutUvicornWorker, which raises
  ws_ping_timeout to 60s (matching the value already used by the
  dev-only start-reload.sh script), and wire it up as the default
  worker class in gunicorn/start.sh so production tolerates the same
  event-loop stalls as local dev.
- Add unit tests covering chunked logging, invalid UTF-8 handling,
  and unsupported content-type rejection.

* Address review: strip CRLF instead of LF only when batching log lines

.rstrip("\n") left a trailing \r on every line of a Windows-style
(CRLF) uploaded log, since the file is read in binary mode and split
only on \n. That stray \r would end up embedded in the joined chunk,
showing up as ^M characters or unexpected double newlines wherever
the chunk is logged or displayed.

Use .rstrip("\r\n") so both Unix and Windows line endings are
handled correctly. Add a regression test with a CRLF-terminated
upload.

* Fix CI mypy error: annotate FakeUploadFile.file as BinaryIO

mypy's structural check against the UploadFile protocol requires
file: BinaryIO exactly; a bare BytesIO() assignment left the
attribute inferred as BytesIO, which mypy treats as incompatible
with the protocol's BinaryIO annotation despite BytesIO satisfying
it at runtime. Annotate the attribute explicitly so
handle_uploaded_file(FakeUploadFile(...)) type-checks.
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