fix: tell a stopped run apart from one the service killed - #14407
fix: tell a stopped run apart from one the service killed#14407ogabrielluiz wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
|
✅ Migration Validation Passed All migrations follow the Expand-Contract pattern correctly. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/flow-span-protocol #14407 +/- ##
===========================================================
+ Coverage 61.50% 62.03% +0.53%
===========================================================
Files 2406 2416 +10
Lines 240085 242243 +2158
Branches 36189 36190 +1
===========================================================
+ Hits 147654 150286 +2632
+ Misses 90496 90022 -474
Partials 1935 1935
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
be57c49 to
6a7b0f9
Compare
897d15a to
e2b1752
Compare
e2b1752 to
f94bce7
Compare
d330b30 to
1d6c5ee
Compare
asyncio delivers a user pressing stop and a server-imposed execution ceiling as the same CancelledError, and the flow span reported both as cancelled with OTel status UNSET. That kept every timeout out of error-rate alerting, while the client was served a terminal error and the job row was written FAILED. The span was the only place claiming nothing had gone wrong. The two are now separate outcomes. A user stop stays cancelled with status UNSET, because a withdrawn request is not a service fault. Anything else that cancels a run reports aborted with span status ERROR, which is what the rest of the system already says about it. The discriminator already existed: the producers stamp a marker on the exception args, and the job service reads it to choose CANCELLED over FAILED. It was a bare string repeated at five sites, so the span could not have trusted it. It is one constant now, and it lives in lfx.constants rather than next to the graph exceptions: importing lfx.graph.exceptions from a langflow service module runs lfx/graph/__init__, which imports Graph, and that made langflow.main fail to import at all. lfx.constants imports nothing but pathlib.
f94bce7 to
7b9466f
Compare
Stacked on #14403 and merges after it. The diff below is against that branch, not
release-1.12.0, and green CI here only proves this change against its parent.What
asynciodelivers two very different events as the sameCancelledError, and the flow span reported both the same way:asyncio.wait_foraround the v2 build driver, a2a, or the agentic assistant), a worker shutdown, a parent task being torn downBoth came out as
status="cancelled"with OTel span status UNSET. So every timeout stayed out of error-rate alerting, while the client was served a terminal error and the job row was written FAILED. The span was the only place in the system claiming nothing had gone wrong.They are separate outcomes now:
statuscancelledabortederror.type=CancelledErrorA withdrawn request is genuinely not a service fault, so the stop button keeps its UNSET. Everything else now agrees with what the client and the job row already say.
One constant instead of five strings
The discriminator already existed: the producers stamp a marker on the exception args and the job service reads it to choose CANCELLED over FAILED. It was a bare
"LANGFLOW_USER_CANCELLED"repeated at five sites, which is why the span could not have trusted it. It is one constant now, imported by both the producers and the span.It lives in
lfx.constants, not next to the graph exceptions where it more naturally belongs. Importinglfx.graph.exceptionsfrom a langflow service module runslfx/graph/__init__.py, which importsGraphfrom a half-initializedbase.py, andfrom langflow.main import create_appthen fails outright.lfx.constantsimports nothing butpathlib. I hit that while writing this and would rather leave the reason here than have someone move it back.Verification
New test drives a real
asyncio.wait_fortimeout rather than hand-raisingCancelledError, so it exercises the way the ceiling actually cancels a run. The user-stop test raises with the marker the producers stamp.Collapsing the split back to one value turns the timeout test red with
'cancelled' == 'aborted'.Suites: 12 flow-span tests, 1763 in
services/, 145 v2 background/cancellation tests, plus an explicitfrom langflow.main import create_appcheck for the import cycle above.Known gaps
abortedstill covers a few distinguishable things (ceiling, shutdown, teardown). They all mean "the service did not deliver this run", which is the distinction that matters for alerting, so I did not split further.