Skip to content

Remove explicit streamable HTTP timeouts#30

Merged
Kaiohz merged 1 commit into
mainfrom
fix/remove-transport-timeout
Jun 19, 2026
Merged

Remove explicit streamable HTTP timeouts#30
Kaiohz merged 1 commit into
mainfrom
fix/remove-transport-timeout

Conversation

@Kaiohz

@Kaiohz Kaiohz commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@Kaiohz Kaiohz merged commit d5f13b6 into main Jun 19, 2026
1 check passed
@Kaiohz

Kaiohz commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Code Review — PR #30

Scope : src/infrastructure/mcp/adapter.py — suppression des constantes _STREAMABLE_HTTP_TIMEOUT et _STREAMABLE_HTTP_SSE_READ_TIMEOUT, ainsi que des clés timeout / sse_read_timeout passées au server_config du transport streamable_http.

Analyse des changements

Le diff est minimal (1 fichier, -10 lignes) et cohérent dans son intention : revenir aux valeurs par défaut de la lib mcp / langchain-mcp-adapters au lieu de forcer des timeouts explicites.

👍 Points positifs

  • Diff chirurgical : un seul fichier touché, pas de changement de comportement collatéral.
  • Suppression de configuration implicite : on s'aligne sur la lib, donc moins de surface à maintenir quand la lib évolue.
  • Constantes devenues orphelines proprement retirées (pas de dead code laissé).

⚠️ Points d'attention

  1. Pas de description de PR — le diff supprime un commentaire qui justifiait explicitement les valeurs choisies :

    "Explicit values make client/server behaviour deterministic instead of relying on library defaults, and reduce the reconnect storms seen when the SSE GET stream drops between tool calls."
    "Keep aligned with the server (mcp-raganything / fastmcp) SDK version."

    Si on enlève les timeouts explicites, on perd cette protection. Il manque dans la PR :

    • la raison du retour aux défauts (bump de lib ? bug ? ),
    • la confirmation que les défauts actuels sont alignés avec mcp-raganything / fastmcp côté serveur.
  2. Risque de régression silencieuse — le commentaire original mentionne des "reconnect storms" observés quand le stream SSE GET lâchait entre deux tool calls. Sans timeouts explicites, on dépend des défauts de la lib :

    • httpx default timeout = 5s (très court pour des tool calls MCP un peu lents).
    • sse_read_timeout default = 30s (vs 10 min avant, donc plus restrictif).

    Si des tool calls RAG dépassent 30s côté mcp-raganything (multimodal, indexing), on risque des déconnexions en chaîne.

  3. Cohérence client/serveur non vérifiée — la version de langchain-mcp-adapters et de mcp n'est pas bumpée dans cette PR. Si on s'appuie sur les défauts, encore faut-il qu'ils matchent la version SDK côté serveur.

  4. Pas de test ajouté/modifié — même si la logique est triviale, un test qui vérifie l'absence des clés timeout / sse_read_timeout (ou la résilience à un tool call long) aurait valeur de garde-fou.

Score qualité du code

7 / 10

Le code en lui-même est propre et la suppression est cohérente. On perd 3 points sur :

  • absence de justification documentée dans la PR,
  • risque non discuté de régression sur les tool calls longs (RAG multimodal),
  • pas de test accompagnant le changement.

Suggestions d'amélioration

  1. Compléter la description de la PR avec : lib bumpée ? problème résolu ? valeur des défauts actuels ?
  2. Vérifier et documenter les défauts actuels de mcp SDK pour timeout et sse_read_timeout, et confirmer qu'ils matchent mcp-raganything / fastmcp.
  3. Ajouter un test d'intégration : appel MCP sur un tool dont la réponse met >30s, vérifier qu'on n'a pas de coupure.
  4. Si le but est juste de tester un bump de lib, envisager une PR séparée qui bump + ajuste les timeouts de manière ciblée plutôt que de tout supprimer.
  5. Si le problème venait d'un conflit avec la lib, garder les constantes mais les passer via un wrapper qui catch l'erreur "unknown kwarg" — au lieu de tout retirer.

/cc @SoluDevTech/maintainers

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.

1 participant