Skip to content

test case fixes for cancelling streams - #4975

Merged
akshaydeo merged 1 commit into
v2.0.0from
07-06-test_case_fixes_for_cancelling_streams
Jul 7, 2026
Merged

test case fixes for cancelling streams#4975
akshaydeo merged 1 commit into
v2.0.0from
07-06-test_case_fixes_for_cancelling_streams

Conversation

@akshaydeo

@akshaydeo akshaydeo commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Updates the stream cancellation E2E test to recognize cancelled as a valid terminal status for cancelled requests, introduced in #4930. Previously, only error (and success for non-streaming) were accepted as valid cancel outcomes. This aligns the test with the dedicated cancellation status while retaining backward compatibility with older builds that logged error.

Changes

  • statusIsCancelOutcome now accepts cancelled in addition to error for both streaming and non-streaming cancel paths
  • Updated inline comments to reflect that cancelled is the expected status on current builds, with error retained as a fallback for pre-fix: stats for cancelled requests #4930 builds
  • Removed a duplicate go.sum entry for github.com/tidwall/pretty v1.2.0

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

Run the stream cancellation E2E test against a current build and verify that cancelled requests with status=cancelled no longer produce unexpected cost-check failures.

node tests/e2e/api/runners/run-stream-cancellation.mjs

Expected: cost checks pass for both streaming and non-streaming cancel scenarios, with cancelled, error, or success (non-stream only) all accepted as valid outcomes.

Breaking changes

  • No

Related issues

Closes #4930

Security considerations

None.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d856bd58-8bc2-4744-894a-cfec6ed0f731

📥 Commits

Reviewing files that changed from the base of the PR and between 741b716 and c0ef15d.

📒 Files selected for processing (1)
  • tests/e2e/api/runners/run-stream-cancellation.mjs
📝 Walkthrough

Walkthrough

This PR updates the e2e test logic in run-stream-cancellation.mjs that determines valid cancellation log statuses. Streaming cancels now accept cancelled or error, and non-stream cancels accept cancelled, error, or success. Related comments were reworded accordingly.

Changes

Cancellation Log Status Update

Layer / File(s) Summary
Expanded cancel outcome checks
tests/e2e/api/runners/run-stream-cancellation.mjs
statusIsCancelOutcome now accepts cancelled or error for streaming cancels and cancelled, error, or success for non-stream cancels; cost-verification comments reworded to describe the expanded acceptable statuses.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

  • maximhq/bifrost#3591: Updates stream cancellation/remote close behavior that can alter cancellation status logging matched by this PR's assertions.
  • maximhq/bifrost#4930: Adds cancelled as a terminal status in log stats/aggregation logic, aligning with this PR's expanded status checks.

Suggested reviewers: danpiths

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The change only adjusts cancellation test expectations and does not add the File API support requested in #123. Implement the File API support in #123, including POST /v1/files and provider file-upload handling for the target workflows.
Out of Scope Changes check ⚠️ Warning The PR changes only cancellation test logic, which is unrelated to the linked File API support objective. Keep this PR focused on File API support, or split the cancellation test fixes into a separate change.
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title is concise and matches the main change: stream cancellation test expectations.
Description check ✅ Passed The description follows the template and covers summary, changes, testing, and issue linkage.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 07-06-test_case_fixes_for_cancelling_streams

Comment @coderabbitai help to get the list of available commands.

@akshaydeo
akshaydeo marked this pull request as ready for review July 6, 2026 22:13

akshaydeo commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@coderabbitai
coderabbitai Bot requested a review from danpiths July 6, 2026 22:14
@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Safe to merge — the change widens the set of accepted terminal statuses in a test helper with no production code impact.

The only changed file is an E2E test runner. The logic change is a two-line expansion of the accepted status set, the error and success fallbacks are preserved, and the backward-compatibility rationale is clearly documented in the updated comments. No production paths, core logic, or schema are touched.

No files require special attention.

Important Files Changed

Filename Overview
tests/e2e/api/runners/run-stream-cancellation.mjs Adds cancelled as a valid terminal status in statusIsCancelOutcome for both streaming and non-streaming cancel paths, retaining error for backward compatibility with pre-#4930 builds; updates inline comments accordingly.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pollLogRow result] --> B{row found?}
    B -- No --> C[costCheck = FAIL]
    B -- Yes --> D{statusIsCancelOutcome?}
    D -- No --> E[costCheck = FAIL\nstatus unexpected]
    D -- Yes --> F{nonStream?}
    
    subgraph statusIsCancelOutcome
        G[nonStream=true] --> H["cancelled | error | success"]
        I[nonStream=false] --> J["cancelled | error"]
    end

    F -- nonStream --> K{EARLY_USAGE_PROVIDER?}
    F -- stream --> K
    K -- Yes --> L{cost > 0 && tokens > 0?}
    L -- No --> M[costCheck = FAIL]
    L -- Yes --> N[costAccuracyVerdict]
    K -- No --> O{cost > 0?}
    O -- Yes --> N
    O -- No --> P[costCheck = PASS\ncost-presence not required]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[pollLogRow result] --> B{row found?}
    B -- No --> C[costCheck = FAIL]
    B -- Yes --> D{statusIsCancelOutcome?}
    D -- No --> E[costCheck = FAIL\nstatus unexpected]
    D -- Yes --> F{nonStream?}
    
    subgraph statusIsCancelOutcome
        G[nonStream=true] --> H["cancelled | error | success"]
        I[nonStream=false] --> J["cancelled | error"]
    end

    F -- nonStream --> K{EARLY_USAGE_PROVIDER?}
    F -- stream --> K
    K -- Yes --> L{cost > 0 && tokens > 0?}
    L -- No --> M[costCheck = FAIL]
    L -- Yes --> N[costAccuracyVerdict]
    K -- No --> O{cost > 0?}
    O -- Yes --> N
    O -- No --> P[costCheck = PASS\ncost-presence not required]
Loading

Reviews (2): Last reviewed commit: "test case fixes for cancelling streams" | Re-trigger Greptile

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 6, 2026
@akshaydeo akshaydeo mentioned this pull request Jul 6, 2026
18 tasks
@akshaydeo
akshaydeo dismissed coderabbitai[bot]’s stale review July 6, 2026 23:18

The merge-base changed after approval.

@akshaydeo akshaydeo mentioned this pull request Jul 6, 2026
18 tasks
@akshaydeo
akshaydeo force-pushed the 07-06-test_case_fixes_for_cancelling_streams branch from 741b716 to c0ef15d Compare July 7, 2026 00:19

akshaydeo commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jul 7, 12:21 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jul 7, 12:22 AM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo
akshaydeo merged commit 8338603 into v2.0.0 Jul 7, 2026
15 of 17 checks passed
@akshaydeo
akshaydeo deleted the 07-06-test_case_fixes_for_cancelling_streams branch July 7, 2026 00:22
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.

2 participants