Skip to content

test(gateway): preserve held-work close failures - #139593

Merged
steipete merged 1 commit into
mainfrom
codex/held-work-observer-current
Sep 6, 2026
Merged

test(gateway): preserve held-work close failures#139593
steipete merged 1 commit into
mainfrom
codex/held-work-observer-current

Conversation

@steipete

@steipete steipete commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Related: #139576

What Problem This Solves

Resolves a problem where the Gateway lifetime test observer could report work as held after public close had already completed, or replace a native close rejection with an assertion error.

Why This Change Was Made

Race the original closing promise at the final observation boundary and require public close to remain pending. Keep the existing bounded entry wait, native drain observation, connection-release checks, and cleanup ownership unchanged.

User Impact

No production behavior changes. Developers get an accurate failure at the held-work boundary and retain the original close error. The change is one net test-support line; no baseline or timeout changes.

Evidence

Six temporary controls exercise the real AsyncWorkScope and Vitest spy contract. On current main, early public close and rejection identity fail (two failures, four passes); the candidate passes all six. Scoped type-aware lint and independent P0–P2 review pass. The controls are retained proof, with no new maintained test file or production seam.

All four existing Gateway lifetime callers and their work-scope owner are byte-identical to the previously passed 15-case real-Gateway batches. Exact-head CI remains required before landing. No user-facing UI, provider API, storage, or runtime change is made.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: XS maintainer Maintainer-authored PR labels Sep 6, 2026
@steipete
steipete marked this pull request as ready for review September 6, 2026 01:21
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 9:26 PM ET / September 6, 2026, 01:26 UTC.

ClawSweeper review

What this changes

The Gateway shutdown test helper preserves close errors and rejects premature close completion while work remains held.

Merge readiness

✅ Ready for maintainer review

Keep open: current main still has the observer gap, and the focused patch addresses it without an identified correctness defect. The maintainer-authored PR is also protected from automatic closure.

Priority: P3
Reviewed head: 304dcd1208a118f0d6cb93185ac60c40d393cd00

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A narrow, maintainable correction with relevant reported controls and no identified blocking defect.
Proof confidence 🌊 off-meta tidepool Not applicable: The maintainer-authored test-support change is exempt from the external contributor runtime-proof gate; the body reports targeted before/after controls, without claiming a fresh execution of the unchanged Gateway callers.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The maintainer-authored test-support change is exempt from the external contributor runtime-proof gate; the body reports targeted before/after controls, without claiming a fresh execution of the unchanged Gateway callers.
Evidence reviewed 6 items Introduced patch verified: The pinned merge-base-to-head diff changes only the final observation boundary: race the original closing promise and assert that public close remains pending.
Still necessary on fetched main: Fetched main still awaits only nextTurn and never asserts closed is false at the final boundary. An already-rejected close can therefore be replaced by a later assertion failure, and an early successful close is not independently rejected.
Caller and cleanup contract: The request, health, startup-presentation, and operator-observer lifetime tests pass public close promises into the helper while holding work; their finally blocks release and join the original operations. The helper retains native drain observation, bounded entry waiting, and caller-owned cleanup.
Findings None None.
Security None None.

How this fits together

Gateway lifetime tests hold asynchronous work while shutting down a real test server. This shared helper checks that connections have released and shutdown remains pending until the held work finishes.

flowchart TD
  A[Gateway lifetime tests] --> B[Hold asynchronous work]
  B --> C[Start Gateway shutdown]
  C --> D[Observe owner drain and connection release]
  D --> E[Check public close outcome]
  E --> F[Preserve rejection or reject early completion]
  E --> G[Confirm shutdown remains pending]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Tests +1. Total +1 across 1 file.

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

Review metrics

None.

Technical review

Best possible solution:

Keep the shared observer strict about public shutdown completion while preserving the original rejection and existing cleanup ownership.

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

Yes, from source: supply an already-rejected close promise or an early successful close while the observed drain remains pending. The current-main helper can mask the rejection or miss premature completion; no tests were executed during this read-only review.

Is this the best way to solve the issue?

Yes. Racing the original promise and checking its settlement state repairs the shared observation boundary without replacing native drain behavior or increasing timeouts.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against abd18df28d85.

Labels

Label changes:

  • add P3: This improves test failure accuracy without changing production behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored test-support change is exempt from the external contributor runtime-proof gate; the body reports targeted before/after controls, without claiming a fresh execution of the unchanged Gateway callers.

Label justifications:

  • P3: This improves test failure accuracy without changing production behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored test-support change is exempt from the external contributor runtime-proof gate; the body reports targeted before/after controls, without claiming a fresh execution of the unchanged Gateway callers.

Evidence

What I checked:

  • Introduced patch verified: The pinned merge-base-to-head diff changes only the final observation boundary: race the original closing promise and assert that public close remains pending. (src/gateway/server-held-work.test-support.ts:55, 304dcd1208a1)
  • Still necessary on fetched main: Fetched main still awaits only nextTurn and never asserts closed is false at the final boundary. An already-rejected close can therefore be replaced by a later assertion failure, and an early successful close is not independently rejected. (src/gateway/server-held-work.test-support.ts:55, abd18df28d85)
  • Caller and cleanup contract: The request, health, startup-presentation, and operator-observer lifetime tests pass public close promises into the helper while holding work; their finally blocks release and join the original operations. The helper retains native drain observation, bounded entry waiting, and caller-owned cleanup. (src/gateway/server.request-lifetime.test.ts:147, 304dcd1208a1)
  • Production lifetime owner: AsyncWorkScope.drain waits for tracked descendants; GatewayConnectionWork also releases registered connections before awaiting that drain. The PR modifies neither owner nor authorization, persistence, dependencies, or runtime shutdown behavior. (src/gateway/server-connection-work.ts:32, 304dcd1208a1)
  • Prior merged work and routing: Provided GitHub context identifies test(gateway): observe native drain without a lint exception #139576 as merged at e927f02. Local history records Peter Steinberger on that helper change and earlier lifetime work. Exact line introduction could not be established because the required parent tree object was unavailable. (src/gateway/server-held-work.test-support.ts, e927f02952be)
  • Validation evidence and release context: The captured PR body reports six temporary controls: two fail before the patch and all six pass afterward, covering early close and rejection identity. It also reports prior 15-case Gateway batches with unchanged callers and owners; these are contributor-reported results, not reviewer executions. The helper is absent from the latest supplied release tag. (src/gateway/server-held-work.test-support.ts, 3928bad9badf)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

@steipete
steipete merged commit e100074 into main Sep 6, 2026
170 checks passed
@steipete
steipete deleted the codex/held-work-observer-current branch September 6, 2026 01:35
@steipete

steipete commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

vincentkoc added a commit that referenced this pull request Sep 6, 2026
@vincentkoc
chore: merge current main into release validation repair
a8a396a 
* origin/main: (302 commits)
  test(gateway): preserve held-work close failures (#139593)
  refactor(exec): share package argument scanning (#139560)
  fix(audit): preserve sequence range validation before numeric decoding (#139587)
  fix(transcripts): retain capture ownership through failed cleanup (#139572)
  refactor(models): share selector reference traversal (#139571)
  perf(agents): classify allowlists without intermediate sets (#138392)
  chore(release): update appcast for 2026.9.2 (#139575)
  fix(ci): synchronize cloud retries and board widget input (#139506)
  fix(plugins): explain SDK incompatibilities after core updates (#139458)
  fix(voice-call): retain installed WebSocket behavior under Bun (#139567)
  test(text): consolidate literal sanitizer cases (#139579)
  test(gateway): observe native drain without a lint exception (#139576)
  test(memory): consolidate visibility fixtures without policy defaults (#139569)
  fix(ui): show shimmer skeletons while loading Control UI (#139525)
  test(qa): make cleanup child logging deterministic (#139566)
  test(cli): keep Unix socket fixtures within path limits (#139565)
  fix: avoid slow incognito key checks in large session stores (#139528)
  feat(macos): open Gateway windows from the Gateways menu (#139522)
  test(workshop): isolate marker casing fixture identities (#139554)
  fix(process): require cleanup evidence before releasing owned work (#139517)
  ...

# Conflicts:
#	scripts/release-check.ts
vincentkoc added a commit that referenced this pull request Sep 6, 2026
@vincentkoc
chore(ci): merge current main for landing
99378d8 
* commit '047587a542ce205ee78354fbd88ba8ec05c07844': (45 commits)
  chore(ui): refresh control ui locales (#139481)
  fix(slack): keep prototype-named emoji shortcodes intact (#136374)
  fix(gateway): deliver upgrade rejection responses under Bun (#139516)
  refactor(test): simplify catalog guards and fix native report fixture (#139496)
  fix(macos): prevent dashboard fixture loopback timeouts (#139556)
  fix(matrix): preserve selected reply targets inside threads (#139479)
  test(gateway): preserve held-work close failures (#139593)
  refactor(exec): share package argument scanning (#139560)
  fix(audit): preserve sequence range validation before numeric decoding (#139587)
  fix(transcripts): retain capture ownership through failed cleanup (#139572)
  refactor(models): share selector reference traversal (#139571)
  perf(agents): classify allowlists without intermediate sets (#138392)
  chore(release): update appcast for 2026.9.2 (#139575)
  fix(ci): synchronize cloud retries and board widget input (#139506)
  fix(plugins): explain SDK incompatibilities after core updates (#139458)
  fix(voice-call): retain installed WebSocket behavior under Bun (#139567)
  test(text): consolidate literal sanitizer cases (#139579)
  test(gateway): observe native drain without a lint exception (#139576)
  test(memory): consolidate visibility fixtures without policy defaults (#139569)
  fix(ui): show shimmer skeletons while loading Control UI (#139525)
  ...
@github-actions github-actions Bot mentioned this pull request Sep 6, 2026
Open
This was referenced Sep 6, 2026
Merged
Open
vincentkoc added a commit that referenced this pull request Sep 6, 2026
@vincentkoc
chore(codex): merge current main into PR 135167
43ba7f0 
* commit 'refs/openclaw/tasks/pr-135167-main': (2467 commits)
  refactor(ios): reuse shared integer conversion (#139581)
  fix(sessions): prevent database lock failures during reclamation (#139469)
  perf(test): reuse transforms within owned scheduler slots (#139553)
  test(agents): share sparse transcript input fixtures (#139589)
  chore(ui): refresh control ui locales (#139481)
  fix(slack): keep prototype-named emoji shortcodes intact (#136374)
  fix(gateway): deliver upgrade rejection responses under Bun (#139516)
  refactor(test): simplify catalog guards and fix native report fixture (#139496)
  fix(macos): prevent dashboard fixture loopback timeouts (#139556)
  fix(matrix): preserve selected reply targets inside threads (#139479)
  test(gateway): preserve held-work close failures (#139593)
  refactor(exec): share package argument scanning (#139560)
  fix(audit): preserve sequence range validation before numeric decoding (#139587)
  fix(transcripts): retain capture ownership through failed cleanup (#139572)
  refactor(models): share selector reference traversal (#139571)
  perf(agents): classify allowlists without intermediate sets (#138392)
  chore(release): update appcast for 2026.9.2 (#139575)
  fix(ci): synchronize cloud retries and board widget input (#139506)
  fix(plugins): explain SDK incompatibilities after core updates (#139458)
  fix(voice-call): retain installed WebSocket behavior under Bun (#139567)
  ...
@steipete steipete mentioned this pull request Sep 6, 2026
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 6, 2026
@steipete
test(gateway): preserve held-work close failures (openclaw#139593)
1b4d308
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

gateway Gateway runtime maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary 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.

1 participant

@steipete