Repository navigation
fix(sdk-broker): stop per-poll heartbeat checkpoint + full index replay in readiness wait - #6278
Conversation
… index on every readiness poll
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR changes readiness polling to use the existing change-aware session-index refresh instead of checkpointing heartbeats and replaying the full index on each poll. Independent reviews found no verified actionable defects; the readiness identity, endpoint, terminal-state, and native-process checks remain intact.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
The external G1 spawn-failure-rate measurement remains pending until deployment to the target fleet. The new operation-count regression test is Linux-only; exact-head CI ran it on Ubuntu, but that is not cross-platform runtime evidence.
CI / Verification
GitHub Actions Dev CI run 37130902395 is bound to reviewed head 6db191d5344d8e69cef6e9a05f56547463301fa6 and succeeded. The targeted sdk-broker-lifecycle-e2e.test.ts shard, affected-path evidence/aggregate validation, and virtual integration validation passed. The changed test verifies that readiness polling does not repeatedly checkpoint heartbeats or invoke full index refreshes. No local tests or project gates were run during this review; the post-deploy fleet metric is not established by CI.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The change matches the scoped polling-cost claim; readiness predicates and persisted/API contracts are unchanged. No .gjc intent projection was available. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Change-aware refresh retains locked reload on detected index changes; missing authority remains fail-closed and the wait retries within its existing deadline. |
| A3 — Security / Privacy / Trust | APPROVED |
No trust-boundary or authorization checks were weakened; endpoint, process-incarnation, and readiness-event verification remain. |
| A4 — Verification / Tests / CI | APPROVED |
The regression test is selected on the exact head and the required affected-test aggregate and virtual integration checks passed. The fleet metric remains a post-deploy measurement. |
| A5 — Context / Compatibility / Platform | APPROVED |
The implementation reuses SessionIndex.refreshIfChanged(); persisted formats and public interfaces are unchanged. The added test's platform evidence is Linux-only. |
Limitations
The review did not execute local tests or gates. CI provides exact-head Linux test evidence, but not Windows/macOS execution or the pending post-deploy fleet measurement.
|
Merged into dev as
— |
Why
gjc ACP spawn failures (hermes-ops G1: SPAWN_FAILED 3/56 = 5.4% over 2h, target <= 2%). All samples were on one host (studio),
did not become ready ... child=alive(terminal_uncertain): the child host registered ~4.5s after spawn and stayed alive, but the broker gave up on readiness.Root cause:
waitForReadypolls every 50ms and each poll callscurrentReadyAuthority, which ranbroker.heartbeatSessions()(machine-global session-index lock + live-heartbeat checkpoint) and an unconditionalbroker.index.refresh()(full replay). On a host with a large index (studio: 6779 sessions, checkpoint 150-168ms, index-lock occupancy 15% vs 1.2% on a smaller host) plus 3 concurrent launches, the per-poll lock/replay cost alone pushes host_registered -> ready past the readiness budget (studio p90 7.2s, failures 18.8-38.7s; other hosts p90 0.7s). 2-day readiness failures: studio 73 / work 7 / home2 1.What
currentReadyAuthoritynow callsbroker.index.refreshIfChanged()(change-stamp fast path, locked re-classification only when the index actually changed) instead ofheartbeatSessions()+index.refresh()on every poll.packages/coding-agent/changelog.d/broker-readiness-poll-index-cost.md.Local tests (command + result)
bun test test/sdk-broker-lifecycle-e2e.test.ts -t "readiness polling avoids"-> 1 pass (on tank, linux)bun run --workspaces --if-present check:types-> rc 0bun scripts/telegram-daemon-generation-guard.ts $(git merge-base origin/dev HEAD) HEAD-> "v52 no protected changes"bun --cwd=packages/coding-agent run checkandrun lintpass (2 pre-existing warnings intest/sdk-diagnostics-entry.test.ts).Local CI gate (prepush)
bun run --workspaces --if-present check:typesrc=0bun scripts/telegram-daemon-generation-guard.ts ...rc=0Needs e2e
DRY=1 python3 /opt/data/scripts/gjc-acp-guard.py(SPAWN_FAILED <= 2% over 2h, gjc provider).Acceptance
lifecycle.ts::currentReadyAuthority)lifecycle.tsl.~5395refreshIfChanged()sdk-broker-lifecycle-e2e.test.ts"readiness polling avoids ..." (fixture fixed in 7486cb1: valid endpoint + owned lifecycle marker + injected incarnation, so the poll reaches the index lookup)Risk
regression-risklow-riskhigh-riskAgent
prompt_failed(-32603, "Agent run failed after execution started", phase post_start), no commits.Open questions
lifecycle.ts/sdk-broker-lifecycle-e2e.test.ts): hunks do not overlap (fix(sdk-broker): stop per-poll heartbeat checkpoint + full index replay in readiness wait #6278 touchescurrentReadyAuthority~l.5392 and a new test near l.555; fix(sdk): retire stale lifecycle ready markers so session.resume does not EEXIST (#6261) #6265 touches l.1667-1841, l.6650 and other test lines). GitHub reports MERGEABLE against dev; whichever lands second may need a trivial rebase.