Skip to content

test(qa): use full-turn budget for native stop recovery#99656

Merged
RomneyDa merged 3 commits into
mainfrom
dallin/fix-native-active-session-flake
Jul 3, 2026
Merged

test(qa): use full-turn budget for native stop recovery#99656
RomneyDa merged 3 commits into
mainfrom
dallin/fix-native-active-session-flake

Conversation

@RomneyDa

@RomneyDa RomneyDa commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes #99655.

The native command session-target scenario sends recovery immediately after /stop acknowledges. That acknowledgement proves command handling, but the aborted turn can still own the session lane while it unwinds. Exact QA artifacts recorded 16586ms and 18578ms session-lane waits, so the recovery marker arrived after the scenario's fixed 15000ms outbound timeout.

Why This Change Was Made

The scenario now applies the repository's existing full-turn timeout policy to recovery:

  • keeps the 5000ms active-run assertion unchanged;
  • keeps native /stop and the abort acknowledgement assertion unchanged;
  • keeps immediate recovery injection, so the scenario still proves queued next-turn recovery;
  • waits for the recovery marker with liveTurnTimeoutMs(env, 30000).

This is not a sleep. Mock providers preserve the 30000ms fallback, while live providers raise it to their model-specific full-turn floor. The discarded sessions.list.hasActiveRun idle poll was not valid because that projection clears before the command lane is released.

PR #99368 separately owns suppression of optional QA child startup warmups. This PR does not change gateway startup behavior or JSONL parsing.

User Impact

QA Smoke CI retains the same native command targeting, abort acknowledgement, and recovery assertions without failing when normal abort cleanup consumes part of the next turn's queue budget.

Evidence

  • PR fix(qa): prevent qa smoke ci timeouts under gateway concurrency #99368 run 28639086776, job 84931413935, artifact 8058095433: recovery waited 18578ms behind the session lane after successful /stop acknowledgement.
  • PR test(qa): use full-turn budget for native stop recovery #99656 run 28684909956, job 85075721090, artifact 8075313865: hasActiveRun cleared before recovery was injected, but recovery still waited 16586ms for the lane and arrived after the 15000ms assertion expired.
  • extensions/qa-lab/src/live-timeout.test.ts proves mock lanes preserve the caller fallback and live lanes use provider/model floors.
  • node scripts/run-vitest.mjs extensions/qa-lab/src/scenario-catalog.test.ts — 32/32 tests passed.
  • Four simultaneous exact Crabline Telegram workers — 4/4 passed; every recorder was parse-clean and contained both abort and recovery sends.
  • Fresh artifact-aware autoreview — clean, no accepted/actionable findings.
  • Blacksmith Testbox changed gate — provider blacksmith-testbox, ID tbx_01kwn0fe5vm4gevwq0c3vks9r9, exit 0.
  • git diff --check — passed.

@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 6:49 PM ET / 22:49 UTC.

Summary
The PR changes the native command session-target QA scenario recovery wait from a fixed 15000ms timeout to liveTurnTimeoutMs(env, 30000).

PR surface: Other +1. Total +1 across 1 file.

Reproducibility: yes. source-level. Current main still waits only 15000ms for the recovery marker after /stop, and the linked issue and PR provide exact CI artifact timings where recovery waited longer than that; I did not rerun the flaky live QA lane in this read-only review.

Review metrics: 1 noteworthy metric.

  • QA timeout policy changed: 1 changed: recovery wait 15000ms -> liveTurnTimeoutMs(env, 30000). This is the behavioral change that affects CI flake detection and the scenario's worst-case wait time.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #99655
Summary: This PR is the open candidate fix for the canonical QA native stop recovery timeout issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] Merging intentionally extends this QA Smoke scenario's recovery wait to the provider full-turn budget, which can increase worst-case failure latency for this specific automation path.

Maintainer options:

  1. Accept the bounded QA timeout tradeoff (recommended)
    Given the narrow diff, matching helper contract, and PR evidence, maintainers can accept the longer recovery wait as the intended flake fix.
  2. Request one more exact QA Smoke proof
    If maintainers want more confidence, rerun the exact native-command-session-target QA Smoke path under contention before landing.

Next step before merge

  • [P2] This maintainer-labeled PR has no narrow repair finding; the remaining action is maintainer review or landing after accepting the QA automation timeout tradeoff.

Security
Cleared: The diff only changes a QA scenario timeout expression and does not touch dependencies, workflows, credentials, package metadata, generated code, or other supply-chain surfaces.

Review details

Best possible solution:

Land this scenario-only timeout change after maintainer review, keeping the /stop acknowledgement assertion fixed while giving queued recovery the existing full-turn QA budget.

Do we have a high-confidence way to reproduce the issue?

Yes, source-level. Current main still waits only 15000ms for the recovery marker after /stop, and the linked issue and PR provide exact CI artifact timings where recovery waited longer than that; I did not rerun the flaky live QA lane in this read-only review.

Is this the best way to solve the issue?

Yes. The patch is the narrowest maintainable fix because it applies the existing QA full-turn timeout policy at the recovery wait without changing production native-command routing, adding sleeps, or relying on the non-canonical active-run projection.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against fbdfd1962258.

Label changes

Label justifications:

  • P2: The PR targets a bounded QA Smoke false-negative in one native-command scenario with no indicated production runtime regression.
  • merge-risk: 🚨 automation: The diff intentionally changes QA scenario timeout behavior, which can affect CI duration and pass/fail behavior even when correct.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate is not applicable because this is a maintainer-authored PR; the PR body still includes QA artifact, Crabline, Testbox, and CI evidence.
Evidence reviewed

PR surface:

Other +1. Total +1 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 2 1 +1
Total 1 2 1 +1

What I checked:

Likely related people:

  • RomneyDa: Authored and merged the PR that introduced the transport-portable native-command QA scenario, authored the linked issue, and authored this focused candidate fix. (role: recent QA native-command scenario owner; confidence: high; commits: e701dc76b060, b191c2434f87, 1cab479f7814; files: qa/scenarios/channels/native-command-session-target.yaml, extensions/qa-lab/src/scenario-flow-runner.ts, extensions/qa-lab/src/crabline-transport.ts)
  • gumadeiras: Commit history for extensions/qa-lab/src/live-timeout.ts points to the QA provider registry work that owns the provider-backed timeout helper this PR reuses. (role: adjacent QA timeout/provider helper introducer; confidence: medium; commits: bb7e9823a886; files: extensions/qa-lab/src/live-timeout.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Closed
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 3, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 3, 2026
@RomneyDa RomneyDa changed the title test(qa): stabilize native command active-session scenario test(qa): use full-turn budget for native stop recovery Jul 3, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Merged
@clawsweeper clawsweeper Bot removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 3, 2026
@RomneyDa

RomneyDa commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 3, 2026
@RomneyDa
RomneyDa merged commit 96b0297 into main Jul 3, 2026
94 of 101 checks passed
@RomneyDa
RomneyDa deleted the dallin/fix-native-active-session-flake branch July 3, 2026 22:56
@RomneyDa RomneyDa mentioned this pull request Jul 4, 2026
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 4, 2026
@RomneyDa
test(qa): use full-turn budget for native stop recovery (openclaw#99656)
528c1c4 
* test(qa): warm native command session scenario

* test(qa): wait for native abort cleanup

* test(qa): use full-turn budget for native stop recovery
@steipete steipete mentioned this pull request Jul 5, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: qa-lab maintainer Maintainer-authored PR merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. P2 Normal backlog priority with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: QA native stop recovery can outwait fixed turn timeout

1 participant

@RomneyDa