fix(benchmarks): use TCP probe for langflow_run_http_ready readiness - #12798
Conversation
Swap the `Application startup complete.` stdout marker for a TCP connect probe against 127.0.0.1:7860 so the scenario no longer races against langflow's structlog processor pipeline. The same approach is used by _langflow_no_change_restart_supervisor.py, whose header note already called out this class of failure. The scenario's thresholds.json entry is still the `mean_ms: 0` sentinel, so continue-on-error stays set for this matrix cell until a run-benchmark-snapshot captures a real baseline. Workflow comments and the generated thresholds.json `_note` are updated to reflect the fix.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
erichare
left a comment
There was a problem hiding this comment.
LGTM! Minor comments below, but approving
Verdict: Approve. Small, well-scoped fix with solid rationale. Brings langflow_run_http_ready into alignment with the already-proven pattern in _langflow_no_change_restart_supervisor.py.
What it does
Replaces stderr line-scanning for uvicorn's Application startup complete. marker (swallowed by langflow's structlog pipeline → sentinel mean_ms: 0) with a TCP connect probe on 127.0.0.1:7860, plus a daemon thread draining child stdout so CI logs still show boot progress.
Strengths
- Pattern consistency: identical readiness strategy to the sibling
_langflow_no_change_restart_supervisor.py, which even explicitly flagged this exact failure class in its header note. - Exit-code semantics preserved (0/2/3) via an
exit_codeaccumulator, with clearer stderr messages for each path. - No deadlock risk: drain thread is
daemon=Truewithjoin(timeout=2);stop_eventis belt-and-suspenders — closing child stdout on SIGTERM unblocks thefor line in streamloop anyway. - Workflow comment updates are thorough: the 5→10min timeout bump is justified (tight budget only made sense while scenario was sentinel-failing), and the ordering — land fix → capture baseline via
run-benchmark-snapshot→ drop fromcontinue-on-errorin follow-up — is sensible.
Minor notes (LOW, non-blocking)
- Port-already-in-use false positive: if anything else is bound to
127.0.0.1:7860,_tcp_readyreturns true immediately and the supervisor reports a near-zeroLANGFLOW_READY_MSwhile the child fails to bind. Pre-existing property of the sibling supervisor too; each matrix job runs on its own runner so not a practical concern, but worth remembering when someone tries to run two scenarios concurrently locally. - stdout buffering:
Popen(text=True)withoutbufsize=1means log lines may arrive in chunks. Purely a CI-log-visibility thing, not correctness. exit_code = 0initialization ([_langflow_supervisor.py:89](https://claude.ai/epitaxy/src/backend/tests/benchmarks/scenarios/_langflow_supervisor.py:89)) is redundant — all success paths setready_atand bypass thereturn exit_code or 3. Harmless.
No CRITICAL/HIGH issues. Test plan in PR body (ruff, test_driver_verify.py 4/4, stub-listener smoke) is appropriate for the scope.
Addresses two low-severity review notes on #12798. Pre-flight check before launching the child: if 127.0.0.1:7860 already accepts connections (dev server, leftover benchmark boot), fail fast with exit code 3 rather than race against the stale listener and emit a bogus near-zero LANGFLOW_READY_MS. Post-ready child-liveness check: after a successful TCP connect, verify the child is still running. If it already exited, the connect landed on someone else's listener and we refuse to record a measurement. Also drops the redundant exit_code accumulator — failure paths now return directly inside the try/finally; the finally block still runs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## cold-start/01-measurement-foundation #12798 +/- ##
=========================================================================
- Coverage 52.78% 41.91% -10.88%
=========================================================================
Files 2024 1953 -71
Lines 183804 180256 -3548
Branches 27344 10724 -16620
=========================================================================
- Hits 97018 75547 -21471
- Misses 85693 103542 +17849
- Partials 1093 1167 +74
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…sions Captured a real baseline for langflow_run_http_ready via snapshot-mode run 24784108652 on 12798/merge@93c8aa10: 22183.74ms mean, 246ms stddev over 5 runs. Updated thresholds.json, including a refreshed snapshot for the other scenarios on the same run. With the sentinel gone, dropped langflow_run_http_ready from every continue-on-error expression and from the regression-comment-skip condition, so the gate now enforces a real regression ceiling on that scenario. langflow_run_no_change_restart is retained at its 2026-04-20 baseline (11324.7ms) rather than the 0.85ms the current run produced — the low number is a known self_measuring dispatch bug, not a real measurement. Workflow comments updated to reflect this.
When a thresholds.json entry has runs=0 AND mean_ms<=0, treat it as a
placeholder for a scenario that has never been snapshotted: record the
current measurement for visibility but do not trip the gate. The next
run-benchmark-snapshot anchors the real baseline.
Previously, any baseline mean_ms<=0 tripped the gate unconditionally.
That meant a new scenario landing as a sentinel (mean_ms=0, runs=0,
the convention for "tracked but not yet anchored") would fail the
workflow on its very first run, forcing the contributor to either:
- snapshot on the PR branch (discouraged — authoritative baselines
should come from main), or
- add continue-on-error: true in the workflow matrix as a hack,
then remember to remove it in a followup PR after the baseline
lands.
Distinguishing runs=0 (unanchored) from runs>0 with mean_ms=0
(intentionally-zeroed Path-B sentinel) preserves the existing
sentinel-trip semantic for the latter case.
Cold Start Benchmark: regression detected (1 scenario(s) failed)Baseline ref:
Hyperfine JSON artifacts: see the Per to merge anyway, apply the Measurement mode: bytecode_compile_delta |
Cold Start Benchmark: regression detected (1 scenario(s) failed)Baseline ref:
Hyperfine JSON artifacts: see the Per to merge anyway, apply the Measurement mode: bytecode_compile_delta |
Cold Start Benchmark: regression detected (1 scenario(s) failed)Baseline ref:
Hyperfine JSON artifacts: see the Per to merge anyway, apply the Measurement mode: bytecode_compile_delta |
Cold Start Benchmark: regression detected (1 scenario(s) failed)Baseline ref:
Hyperfine JSON artifacts: see the Per to merge anyway, apply the Measurement mode: bytecode_compile_delta |
Cold Start Benchmark: regression detected (1 scenario(s) failed)Baseline ref:
Hyperfine JSON artifacts: see the Per to merge anyway, apply the Measurement mode: bytecode_compile_delta |
0f2b207
into
cold-start/01-measurement-foundation
Summary
Swaps the
langflow_run_http_readybenchmark scenario from scraping uvicorn'sApplication startup complete.stdout marker to a TCP connect probe against127.0.0.1:7860. The scenario was previously sentinel (mean_ms: 0,runs: 0) because the structlog processor pipeline swallows the marker line the supervisor was waiting for. TCP connect is a ground-truth readiness signal that does not depend on logging at all — the sibling_langflow_no_change_restart_supervisor.pyalready uses this approach and its own header note called out this exact class of failure.What changed
src/backend/tests/benchmarks/scenarios/_langflow_supervisor.py: replaced the stderr line-scan loop with asocket.create_connectionpoll (50 ms interval), added a background thread that drains the child's merged stdout so CI logs still show boot progress, and kept the same exit-code semantics (0ready,2timeout,3early exit).src/backend/tests/benchmarks/scenarios/langflow_run.py: updated the module docstring to reflect TCP-based readiness..github/workflows/cold-start-benchmark.yml: dropped stale references to the "structlog marker issue," bumped thelangflow_run_http_readyjob timeout from 5 min to 10 min now that it actually runs to completion, and rewrote the continue-on-error comment to say this flag stays set only until arun-benchmark-snapshotcaptures a real baseline intothresholds.json. Also dropped the stale sentinel caveat from the generated_note.Why
continue-on-errorstayssrc/backend/tests/benchmarks/thresholds.jsonstill carries{ mean_ms: 0, runs: 0 }for this scenario. The driver's verify gate treatsbaseline_ms <= 0as a sentinel-trip — any finite current mean would be flagged as an infinite regression. Once arun-benchmark-snapshotrun swaps the 0 for a real number, a follow-up PR can droplangflow_run_http_readyfrom the continue-on-error expression. Comments in the workflow file describe this ordering.Verification
uv run ruff checkanduv run ruff format --check— clean.cd src/backend && uv run pytest tests/benchmarks/tests/test_driver_verify.py— 4/4 pass.LANGFLOW_READY_MS. Ran it against a child that exits early — correctly returns 3.cold-start-benchmarkworkflow on this branch will exercise the scenario end-to-end; numbers will surface in the matrix job output.Stacked on
#12788 (
cold-start/05-service-init-container).