Skip to content

fix(test): feed read_bounded_json's stream contract in the slot-close race stub - #8534

Closed
CrysisDeu wants to merge 1 commit into
mainfrom
fix/slot-close-race-req-stub
Closed

fix(test): feed read_bounded_json's stream contract in the slot-close race stub#8534
CrysisDeu wants to merge 1 commit into
mainfrom
fix/slot-close-race-req-stub

Conversation

@CrysisDeu

Copy link
Copy Markdown
Collaborator

Summary

Unblocker for #8518: test/test_slot_close_recreation_race.py fails on every PR merge commit — worker crashes on the Windows shard, AttributeError: '_Req' object has no attribute 'can_read_body' plus timeouts on Linux — reddening Backend Tests (Windows) (4), Backend Tests (3.12, 4) and the Coverage Gate on every open PR's merge ref.

Closes #8518

Root cause

Two independently-green PRs crossed on main: the race-test module landed with a _Req stub that mocks json(), and the tranche-3 read_bounded_json sweep (#7308, commit 9d3d279ac) converted api_chat_slot_delete / api_chat_slots_cleanup — the handlers this module drives — to read their body through read_bounded_json(request, allow_absent=True), which touches request.can_read_body (_shared.py:142) before ever calling json(). Each PR's own merge ref predated the other, so neither CI run could see the collision.

Fix

read_bounded_json's own docstring names the stand-in contract: "must feed content/content_length rather than mocking json". The stub now carries exactly the surface the helper touches — can_read_body, content_length, charset, and a content stream whose iter_chunked serves the serialized body — so it satisfies both the allow_absent short-circuit (bodiless request → {}) and the capped stream-read path (explicit body= → served in chunks). json() is kept for direct callers.

Test-file-only change; no runtime code touched.

What was tested

  • flake8, isort --check-only, diff-scoped black gate: pass. Test execution is delegated to CI per host policy (the failing jobs themselves are the verification: this module is the only red).

Pattern harvest

Rule candidate: an aiohttp request stand-in that mocks json() without implementing can_read_body/content breaks silently when its handler is later swept onto read_bounded_json. _shared.py's docstring already states the contract; a shared test/ fixture (one canonical FakeRequest) would make the next sweep collision impossible instead of documented.

Why no screenshot: backend test-stub change only; nothing renders.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 8f912641bd40bfe0329136f5dcec1e5437935843 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Minimal test-only unblocker that fits the stub to read_bounded_json's documented contract; the systemic fix (shared FakeRequest fixture) is correctly deferred.

[DESIGN-REVIEWED] 8f91264

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8f912641bd40bfe0329136f5dcec1e5437935843 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8f91264

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 8f912641bd40bfe0329136f5dcec1e5437935843: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 8f912641bd40bfe0329136f5dcec1e5437935843 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 8f91264

Verdict parsed from the review's SHA-scoped output markers for commit 8f912641bd40bfe0329136f5dcec1e5437935843.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 8f912641bd40bfe0329136f5dcec1e5437935843: <one-sentence reason>

@chenmingwei23

Copy link
Copy Markdown
Contributor

Heads-up so you do not spend a rebase on this: the same fix landed in #8536 twelve minutes ago (merged 20:47:24Z), which is why this PR just went mergeable: false / dirty. Both diffs were additive to test/test_slot_close_recreation_race.py and yours was opened eleven minutes first, so the conflict is purely an artifact of merge order rather than anything wrong with your change.

can_read_body is now a property at line 118 on main. Worth a look at what landed before you decide what to do here: if #8536 covers the same contract gap, this is redundant; if your +37 covers something it does not — the stream contract your title mentions is a plausible candidate — then the remaining delta is worth rebasing down to just that part rather than dropping it.

Context on why it mattered to more people than you: that fixture gap was reddening Backend Tests (Windows) (4) and Backend Tests (3.12, 4) on seven unrelated open PRs at once, plus Coverage Gate downstream of them, because pull_request CI tests the merge with main and so every branch inherited it. Details and the measurements are in #8517. Thanks for turning it around fast.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

🤖 Kiro Crew Auto-Pipeline [operator: CrysisDeu#0c98c3a2]

Review-ready. All owned checks green: the three verification targets this fix exists for — Backend Tests (Windows) (4), Backend Tests (3.12, 4), Coverage Gate — all pass on this merge ref (they fail on every other PR's), plus all 4 Windows shards, all 4 Linux shards, frontend tests, lint (green on rerun after an infra cancellation), and all five AI review lanes PASS.

The only red is E2E (stub ACP backend, offline), which is the other main breakage (tracked in #8526, fork.spec assistant-more-actions, regression window 824ab57e8f5596230f) — unrelated to this test-stub change and red on main's own CI at 701f8f981.

Merging this unblocks the Backend Tests (Windows) (4) / (3.12, 4) / Coverage Gate reds currently inherited by every open PR (including #8519 and #8510).

@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

🤖 Kiro Crew Auto-Pipeline [operator: CrysisDeu#0c98c3a2]

Closing as superseded: #8536 (merged, 5bc2fc796) fixes the same _Req stub contract with equivalent coverage — main's own CI now shows all four Windows shards green at that commit, which is exactly the outcome this PR existed to produce. The conflict this PR reports is with that fix itself, so rebasing it would leave an empty diff.

Thanks to the #8536 author for landing the fix. This branch's verification run remains useful evidence: the same stub-contract change turned Backend Tests (Windows) (1-4), Backend Tests (3.12, 1-4) and Coverage Gate green on its merge ref (run 33910161397) while every other open PR stayed red, independently confirming the root cause recorded in #8518.

@CrysisDeu CrysisDeu closed this Sep 4, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 4, 2026
@bolichen97
bolichen97 deleted the fix/slot-close-race-req-stub branch September 6, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_slot_close_recreation_race fails on every PR merge commit (worker crash on Windows, timeouts + stub AttributeError on Linux)

2 participants