Skip to content

test(sdk): wait for the broker ensure past the runtime-image probe (#6122) - #6124

Merged
Yeachan-Heo merged 1 commit into
devfrom
fix/6122-heartbeat-recovery-test
Sep 29, 2026
Merged

Yeachan-Heo merged 1 commit into
devfrom
fix/6122-heartbeat-recovery-test

Conversation

@Yeachan-Heo

Copy link
Copy Markdown
Owner

Summary

Dev shard-2 has been red since 35f28080. The failing test is four live SDK hosts recover broker index heartbeats without recreating sessions (failingEnsureCalls expected 1, received 0).

Root cause: since #6042, broker recovery first awaits the bounded runtime-image replacement probe, which is a real fs.stat, and only then calls ensureBroker. The test gave the recovery a single Bun.sleep(0) after firing the timer twice. On a loaded shard the first ensure lands later than that, so the test reads 0. The bench changes in #6117 only shifted shard load and ordering.

  • The test now polls, with a bound, until the first ensure lands. It then leaves a duplicate the same window to show up, and still asserts exactly one ensure for the duplicated timer fire.
  • Test-only change. Recovery behavior is unchanged.

Verification

  • Reproduction: delaying the probe by 20 ms makes the old test fail with Received: 0, the same failure as CI. The new test passes.
  • sdk-lifecycle-telegram-integration.test.ts: 6/0, three runs in a row. Coding-agent tsc is clean.

Risk

  • low-risk: test-only change

Fixes #6122

GJC verdict

gajae.pr-review-verdict.v1 needs-human sha256:81203aa4095448e41a40e270d363933f754610ed90ec01d1d5a8ba47d06634c2 reviewer:human reviewer-id:snowykr evidence:repro-delayed-probe;6-0-x3

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

…6122)

Since #6042, broker recovery awaits the bounded runtime-image replacement
probe (a real fs.stat) before ensuring the broker, so on a loaded CI shard
the first ensure lands after the single Bun.sleep(0) the test allowed and
failingEnsureCalls reads 0. Poll (bounded) for the first ensure, give a
duplicate the same window, then keep asserting exactly one ensure for the
duplicated timer fire. Reproduced by delaying the probe 20 ms: the old test
fails with Received 0, the new one passes.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T09:16:44.310153Z d8489dd PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@snowykr snowykr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

APPROVED

Summary

This test-only change replaces a one-tick scheduling assumption with bounded polling for the broker recovery ensure call, while retaining the duplicate-timer exactly-once assertion. The change is consistent with the asynchronous runtime-image probe and introduces no verified merge-blocking defects.

Findings / Required Changes

No blocking or actionable findings.

CI / Verification

The exact-head changed-file test check and coding-agent TypeScript build passed; virtual integration validation also passed. The merge-approval bootstrap failed as an approval/policy gate, not a product-test failure. Windows, WSL, and Darwin-specific optional checks were skipped. No local test execution was performed as part of this review.

Axis Coverage

Axis Verdict Coverage
A1 — Intent / Policy / Contract APPROVED Test-only adjustment matches intent; the exactly-once ensure assertion remains.
A2 — Architecture / Correctness / Failure APPROVED Bounded probe is armed before the test polling loop; duplicate callbacks remain covered, with no reachable timing defect established.
A3 — Security / Privacy / Trust APPROVED No production trust boundary, data handling, or authority changes.
A4 — Verification / Tests / CI APPROVED Changed-file test, TypeScript build, and virtual integration checks passed; approval bootstrap failure is policy-related.
A5 — Context / Compatibility / Platform APPROVED Change is isolated to a test and follows existing bounded polling patterns; no compatibility or reuse defect found.

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (head d8489dd, gajae-reviewer on behalf of probepark) — COMMENT, no verdict

CI: all planned checks green (test:packages/coding-agent/test/sdk-lifecycle-telegram-integration.test.ts pass, ts-build:ts:coding-agent pass); only Merge approval bootstrap is red, and that is the verdict gate.
Scope: +6 / -1, 1 file — packages/coding-agent/test (test-only)
Conventions: CHANGELOG n/a (test-only), no generated files, no labels
Notable:

  • packages/coding-agent/test/sdk-lifecycle-telegram-integration.test.ts:225 — the bounded poll (≤200×5 ms) until failingEnsureCalls > 0, followed by a fixed 10×5 ms window, keeps the single-flight assertion toBe(1) meaningful: a duplicate ensure from the second firstTimer.callback() still has 50 ms to show up after the first one lands. If the first ensure never lands, the loop exits after about 1 s and the assertion fails with 0, as before, so it cannot hang.
    Blocking: no blocking candidates found in the code.

Approve held: the CI plan did not run cd packages/coding-agent && bun run check (biome + check:types) on d8489dd. Run it at this head, or add it to the plan, and re-request review.

@Yeachan-Heo
Yeachan-Heo merged commit 448a182 into dev Sep 29, 2026
23 of 24 checks passed
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.

3 participants