FE-1218: Fix Petrinaut optimization streaming backpressure and cancellation latency#9057
FE-1218: Fix Petrinaut optimization streaming backpressure and cancellation latency#9057kube wants to merge 1 commit into
Conversation
…lation latency NodeAPI (apps/hash-api): - Race the backpressure wait against drain, close, and the request lifecycle abort signal, removing the losing listeners via an internal AbortController so stalled writes cannot leak listeners, strand a rejected promise, or hold an optimization slot past the timeouts. - Mark terminal events as committed at write hand-off so an abort during the final backpressured write cannot append a second, contradictory terminal event. - Write the failure-path terminal event without awaiting backpressure so teardown never blocks on a stalled client. - Skip transport heartbeats while the response buffer needs draining. Petrinaut Optimizer (apps/petrinaut-opt): - PetrinautModel.close(graceful=False) signals the CLI process group immediately instead of waiting up to five seconds for stdin EOF; the graceful EOF wait is now reserved for studies that completed normally. All failure, timeout, and cancellation paths terminate promptly. - Share one idempotent per-run cleanup between the stream wrapper and a StreamingResponse background task, so a client that aborts before the body is ever pulled cannot leak the admission slot or a live CLI. - Close an abandoned CLI via an initializer done-callback when a second cancellation races the init-cancel recovery. - Emit Retry-After on the 429 study-limit response and declare it in the OpenAPI spec. Tests cover drain-first, close-first, client abort, idle timeout during backpressure, single-terminal-event, heartbeat suppression, slot release, prompt process-group termination, and 429/Retry-After propagation across NodeAPI, the optimizer client, and the Python service.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
github-actions
Bot
added
area/apps > hash*
![semgrep-code-hashintel[bot]](https://avatars.githubusercontent.com/u/22116112?s=60&v=4){kind=small}
| if (response.destroyed || response.writableEnded) { | ||
| throw new Error("The optimization client disconnected"); | ||
| } | ||
| if (response.write(`${JSON.stringify(event)}\n`)) { |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
Detected directly writing to a Response object from user-defined input. This bypasses any HTML escaping and may expose your application to a Cross-Site-scripting (XSS) vulnerability. Instead, use 'resp.render()' to render safely escaped HTML.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L269 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 269] request.body</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L269 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 269] input</a>"]
v3["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L283 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 283] await</a>"]
v4["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L283 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 283] upstreamEvents</a>"]
v5["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L300 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 300] forwardOptimizationEvents</a>"]
v6["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L199 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 199] events</a>"]
v7["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L204 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 204] of</a>"]
v8["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L204 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 204] event</a>"]
v9["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L205 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 205] writeOptimizationEvent</a>"]
v10["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L146 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 146] event</a>"]
end
v2 --> v3
v3 --> v4
v4 --> v5
v5 --> v6
v6 --> v7
v7 --> v8
v8 --> v9
v9 --> v10
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/hashintel/hash/blob/7e9f6ef6fa70dca4121ed983cf00994511ca5f4d/apps/hash-api/src/petrinaut-optimizer/create-petrinaut-optimization-handler.ts#L151 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 151] `${JSON.stringify(event)}\n</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by direct-response-write.
You can view more details about this finding in the Semgrep AppSec Platform.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (response.destroyed || response.writableEnded) { | ||
| throw new Error("The optimization client disconnected"); | ||
| } | ||
| if (response.write(`${JSON.stringify(event)}\n`)) { |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9057 +/- ##
=======================================
Coverage 59.22% 59.23%
=======================================
Files 1391 1391
Lines 135140 135153 +13
Branches 6223 6226 +3
=======================================
+ Hits 80032 80053 +21
+ Misses 54152 54142 -10
- Partials 956 958 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview hash-api NDJSON proxy now waits on backpressure with a race among petrinaut-opt adds Tests cover backpressure teardown, slot release, timeouts, single terminal events, prompt CLI shutdown, and 429 header propagation. Reviewed by Cursor Bugbot for commit 7e9f6ef. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7e9f6ef. Configure here.
| with suppress(Exception): | ||
| await asyncio.to_thread(optimizer.pn_model.close, graceful=False) | ||
| finally: | ||
| await _release_optimization_slot(app) |
There was a problem hiding this comment.
Cleanup flag set before work finishes
High Severity
cleaned_up is set to True before CLI close and slot release finish. If the first cleanup call is cancelled after that assignment, the background retry sees the flag and returns immediately, so _release_optimization_slot never runs and an admission slot stays occupied until process restart.
Reviewed by Cursor Bugbot for commit 7e9f6ef. Configure here.
| # No-op when the stream already closed the CLI; prompt when the | ||
| # stream generators never ran at all. | ||
| with suppress(Exception): | ||
| await asyncio.to_thread(optimizer.pn_model.close, graceful=False) |
There was a problem hiding this comment.
Cleanup swallows cancellation on 3.10
Medium Severity
suppress(Exception) wraps the awaited CLI close in cleanup, but on Python 3.10 asyncio.CancelledError still subclasses Exception. Cancellation during teardown is swallowed instead of propagating, which breaks disconnect/cancel handling on a supported runtime.
Reviewed by Cursor Bugbot for commit 7e9f6ef. Configure here.
3 participants
🌟 What is the purpose of this PR?
Hardens the Petrinaut optimization streaming and cancellation path from #9040 so a slow or disconnecting client cannot leak NodeAPI listeners, promises, or optimizer capacity, and so cancelling a busy study terminates its CLI promptly. Also restores and regression-tests propagation of the optimizer's
429/Retry-Afterthrough NodeAPI.🔗 Related links
🔍 What does this change?
create-petrinaut-optimization-handler.ts): races the backpressure wait againstdrain,close, and the request abort signal, removing the losing listeners via an internalAbortController— no accumulated listeners, no orphaned rejected promise, no slot held past the timeouts.petrinaut_client.py):close(graceful=False)signals the CLI process group immediately instead of waiting up to 5s for stdin EOF a mid-trial CLI never observes; the graceful EOF wait is reserved for studies that completed normally. An idempotent per-run cleanup shared between the stream wrapper and aStreamingResponsebackground task covers a client that aborts before the body is pulled.Retry-Afteron the429study-limit response (declared in the OpenAPI spec); the client preserves status +Retry-After, and NodeAPI passes them through.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
network erroron long optimization streams — the evidence points to a max-connection-duration reset at an intermediate proxy/LB, upstream of NodeAPI. This PR removes NodeAPI as a suspect and guarantees a downstream reset frees capacity and kills the CLI promptly; the confirming logs land in FE-1221: Add structured correlated logging across the Petrinaut optimization pipeline #9059.🛡 What tests cover this?
New/updated unit tests in
apps/hash-api,apps/petrinaut-opt, and@local/petrinaut-optimizer-clientcovering: drain-first, close-first, client abort, idle timeout during backpressure, single-terminal-event, heartbeat suppression under backpressure, prompt process-group termination (fake and real subprocess), slot released exactly once (including never-started stream generators and double cancellation), and429/Retry-Afterpropagation end to end.❓ How to test this?
cd apps/petrinaut-opt && uv run pytestturbo run test:unit --filter '@apps/hash-api' --filter '@local/petrinaut-optimizer-client'