Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Jul 29, 2025

What do these changes do?

Despite previous attempts to mitigate aiohttp.http_exceptions.LineTooLong at the web.HTTPException level, this error still appears sporadically in production logs. Unfortunately, current logging lacks the necessary context to trace the root cause.

Log analysis from test deployments suggests the issue may originate from exceptions raised by the HTTP client communicating with director-v2. This PR reinforces that interaction and introduces refactorings to reduce the likelihood of malformed responses triggering the error.

Changes:

  • ♻️ Avoid setting the reason field in aiohttp.web.HTTPException (gets used as HTTP status line); use text instead.
  • ♻️ Refactor exception context payloads:
    • Remove reason key to prevent leakage into HTTP status lines.
    • Rename reason to details for clarity and to prevent misuse.
  • ♻️ Strengthen error handling around HTTP client interactions with director-v2.

Related issue/s

How to test

Dev-ops

@pcrespov pcrespov self-assigned this Jul 29, 2025
@pcrespov pcrespov added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Jul 29, 2025
@codecov
Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 79.54545% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.93%. Comparing base (28845dd) to head (eee4314).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8174      +/-   ##
==========================================
- Coverage   89.71%   87.93%   -1.79%     
==========================================
  Files        1709     1476     -233     
  Lines       66465    60655    -5810     
  Branches      825      631     -194     
==========================================
- Hits        59632    53338    -6294     
- Misses       6617     7095     +478     
- Partials      216      222       +6     
Flag Coverage Δ
integrationtests 64.13% <72.72%> (+0.02%) ⬆️
unittests 86.29% <77.27%> (-1.93%) ⬇️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.37% <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.10% <ø> (+0.05%) ⬆️
agent 93.81% <ø> (ø)
api_server 93.08% <ø> (ø)
autoscaling 95.88% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 92.37% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.14% <ø> (ø)
director_v2 91.04% <ø> (ø)
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.08% <ø> (ø)
efs_guardian 89.76% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.60% <ø> (ø)
resource_usage_tracker 92.39% <ø> (-0.16%) ⬇️
storage 86.39% <ø> (-0.34%) ⬇️
webclient ∅ <ø> (∅)
webserver 88.07% <79.54%> (-0.01%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28845dd...eee4314. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov changed the title WIP: Is7979/status line too long ♻️🐛 Fix: Prevent aiohttp.http_exceptions.LineTooLong and improve diagnostics Jul 29, 2025
@pcrespov pcrespov marked this pull request as ready for review July 29, 2025 14:35
@pcrespov pcrespov added this to the Engage milestone Jul 29, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors exception handling to prevent aiohttp.http_exceptions.LineTooLong errors by avoiding inappropriate use of the reason field in HTTP status lines. The PR systematically replaces reason fields with details across exception classes and improves error context to prevent malformed HTTP responses.

  • Rename exception context fields from reason to details to prevent leakage into HTTP status lines
  • Update HTTP exception handling to use text instead of reason field
  • Strengthen error handling around HTTP client interactions with director-v2

Reviewed Changes

Copilot reviewed 46 out of 46 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
exception_handling/_factory.py Update default exception handler template to use text instead of reason
rest_middlewares.py Fix HTTP status setting to use proper reason parameter
director_v2/_client_base.py Strengthen director-v2 client error handling with better context
Multiple error classes Rename reason field to details in exception message templates
login/_controller/rest/auth.py Replace reason parameter with text in HTTP exceptions

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks for the effort 👍

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

fun! thanks

@pcrespov pcrespov enabled auto-merge (squash) July 29, 2025 16:06
@pcrespov
Copy link
Member Author

@Mergifyio queue

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Jul 29, 2025
@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
      • any of: [🛡 GitHub branch protection]
        • check-success = SonarCloud Code Analysis
        • check-neutral = SonarCloud Code Analysis
        • check-skipped = SonarCloud Code Analysis

@sonarqubecloud
Copy link

@pcrespov pcrespov merged commit 68679ab into ITISFoundation:master Jul 29, 2025
95 of 96 checks passed
@pcrespov pcrespov deleted the is7979/status-line-too-long branch July 30, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

http_exceptions.LineTooLong

4 participants