fix(sdk): run initial attachment replay off the reconcile tail (#4527) - #4542
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 705be10399
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!(await this.#deliverRecoveredFrames(attached))) return; | ||
| await this.#replayAttachment(attached, attached.cursor.seq); | ||
| }); | ||
| return true; |
There was a problem hiding this comment.
When a periodic tick has already published a rehosted attachment, this return leaves its readyTail replay pending. If a caller subsequently invokes the public reconcile(), the non-deferred pass sees the attachment as resumable and returns immediately without joining that tail, so deterministic callers can proceed before recovered and replayed frames have been delivered—unlike the previous synchronous behavior. A non-deferred reconcile should await any outstanding attachment readyTail.
Useful? React with 👍 / 👎.
705be10 to
c311779
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c311779b5a
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fs.mkdirSync(endpointDir, { recursive: true }); | ||
| const sessionId = "wedge"; | ||
| const endpointFile = path.join(endpointDir, `${sessionId}.json`); | ||
| fs.writeFileSync(endpointFile, JSON.stringify({ sessionId, url: "ws://wedge.test", token: "v1", pid: 42 })); |
There was a problem hiding this comment.
The new regression test introduces fs.writeFileSync here and again during the generation bump, although the repository contract explicitly requires Bun.file()/Bun.write() and forbids writeFileSync; convert these writes to await Bun.write(...) so the added test follows the workspace's Bun filesystem convention.
AGENTS.md reference: AGENTS.md:L124-L126
Useful? React with 👍 / 👎.
47a3514 to
550b937
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 550b937050
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // was awaited inside the serialized reconcile tail, so every later tick | ||
| // froze until the replay budget expired; publications died while leases | ||
| // and inbound stayed green (#4527). | ||
| const repo = fs.mkdtempSync(path.join(os.tmpdir(), "gjc-router-4527-")); |
There was a problem hiding this comment.
Although the fixture writes now use Bun.write, the newly added setup still uses fs.mkdtempSync here and fs.mkdirSync below. The repository contract requires node:fs/promises for directory operations, so make this setup asynchronous with await fs.mkdtemp(...) and await fs.mkdir(...).
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
|
@probepark Exact-head review requested for The original patch was preserved, then fix-forwarded for both automated findings: explicit Required disposition is an authenticated exact-head approval or a concrete blocker. — |
550b937 to
fde1bfb
Compare
|
@probepark Review request refreshed to exact head — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fde1bfb426
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (await this.#attach(session, runEpoch, undefined, false, false, deferReplay)) | ||
| attachedIds.add(session.sessionId); |
There was a problem hiding this comment.
When a rehost is first observed by SessionRouter.request() or an attachment's send() before the next 2-second timer tick, those paths call #serialReconcile(runEpoch) with the default deferReplay=false (lines 555 and 1036). This propagation therefore still makes #publishAttachment await every replacement's slow replay inside the shared reconcile tail, so subsequent periodic ticks and sends across the fleet queue behind it and reproduce the outbound freeze this change is intended to eliminate. Operational reconciles triggered by request/send need the deferred mode too, while only bootstrap and the explicitly synchronous public reconcile() should drain the ready tails.
Useful? React with 👍 / 👎.
fde1bfb to
1b46578
Compare
|
@probepark Exact-head review refreshed to — |
|
@HaD0Yun Could you provide an authenticated exact-head review for — |
|
@IYENTeam Authenticated exact-head review requested for — |
1b46578 to
81de09c
Compare
|
@probepark @HaD0Yun @IYENTeam Final exact-head review target is — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81de09cc7d
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reconcile(): Promise<void> { | ||
| return this.#serialReconcile(this.#runEpoch); | ||
| async reconcile(): Promise<void> { | ||
| await this.#serialReconcile(this.#runEpoch); |
There was a problem hiding this comment.
When reconcile() is the first operation to observe a rehost—such as the production coordinator call in packages/coding-agent/src/coordinator-mcp/server.ts:2873—this non-deferred call still makes #publishAttachment await the replacement's slow event_replay inside #reconcileTail. Periodic ticks and request/send reconciles then queue behind it, recreating the fleet-wide freeze this change is intended to prevent; pass deferReplay=true here and retain synchronous behavior by awaiting the attachment ready tails afterward.
Useful? React with 👍 / 👎.
A session-host rehost re-attaches every session in one periodic reconcile pass, and each attachment's initial event_replay was awaited inside the serialized reconcileTail. One slow replay therefore held every later tick and the sends funneling through them for up to its full retry budget, so leases and inbound polling stayed green while outbound publications froze fleet-wide until daemon restart and backlog replay. The periodic reconcile timer now passes deferReplay=true through on the attachment's ready tail — the same surface the reconnect path already uses. start() and the public reconcile() keep synchronous replay so bootstrap callers and deterministic tests are unchanged. Replay ordering (barrier-held live frames drain after ordered replay events), generation fences, cross-session isolation, and provider hooks are unchanged. Verified the wedge reproducer (generation bump + never-settling replay) times out on the pre-fix router and passes after the change. Lore-id: b3053f34 Constraint: no arbitrary sleeps, retry, or timeout inflation; no restart workaround Constraint: preserve replay ordering, generation fences, cross-session isolation, provider behavior Rejected: awaiting replay only outside reconcile | start() drains tails for bootstrap callers instead, keeping the production path non-blocking Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 106 tests across router/daemon/telegram/control-frames/reconnect suites Not-tested: live multi-host Telegram fleet under real rehost
81de09c to
185897c
Compare
|
@probepark @HaD0Yun @IYENTeam Exact-head target updated to — |
|
Exact head All automated correctness blockers found on prior heads have been fix-forwarded. Merge remains held only for an authenticated approval from a write-authorized independent reviewer on this exact head; after that approval the verdict can be changed to — |
|
Independent issue-disposition verification for #4527 against exact head Issue acceptance contract vs this PR:
Check state at exact head: all matrix jobs SUCCESS/SKIPPED except the sole Disposition: issue #4527 remains open and held to this exact head. On authenticated approval, merge, and merged-history proof, the issue will be closed with signed evidence. No gaps found that require a second PR. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve - the replay no longer wedges the reconcile tail
Reviewed 185897cf5. session-router.ts +39, test +121.
The initial attachment replay ran on the reconcile tail, so a wedged replay blocked periodic
reconcile from ever converging - the #4527 symptom. Moving it off the tail decouples the two.
Differential
# base e6f8c926d, with this head's test file applied
(fail) SessionRouter dispatch authority > periodic reconcile converges while rehosted attachment's replay is wedged (#4527) [508.19ms]
39 pass 1 fail
# head 185897cf5
40 pass 0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
The test name states the invariant and it fails without the fix - a wedged replay must not stop
reconcile from converging. That is the right shape for a liveness bug.
merge-approved.
Reviewed by @probepark - method: fresh-worktree run and package typecheck at the exact head, base run with the head's test file to prove the differential.
|
Post-merge architecture review follow-ups (non-blocking, from issue #4527 disposition boundary review at merged dev tip
These are recorded for the fix owner; none block the #4527 disposition (merged contract verified green: authority suite 40/40, 155 assertions at the merge commit). — |
Refreshing only the protected declaration digests would let Discord and Slack daemons started before #4542 retain the older attachment contract. Bump both provider generations and regenerate their semantic attestations so stale owners are replaced. Lore-id: 4f2ca8d1 Constraint: preserve the existing #4563 manifest repair and update the same PR branch Rejected: digest-only refresh | does not invalidate pre-#4542 daemon owners Confidence: high Scope-risk: narrow Reversibility: revert-safe Tested: telegram daemon generation guard, daemon-control replacement tests, coding-agent check
|
Post-merge reconciliation at merged dev tip
Continuing active ownership until #4563/#4564 land and dev CI is green. — |
…gests chore(guard): refresh discord/slack session-router digests after #4542
…han-Heo#4542 PR Yeachan-Heo#4542 changed SessionRouter.#attach and #publishAttachment without refreshing the discord/slack semantic manifest digests, so the current-tree manifest check fails on dev after Yeachan-Heo#4558's merge surfaced it (dev CI 31820749528 Telegram daemon generation guard). The telegram digests all match. Refreshed through the canonical --write-manifest generator; --fix-generations confirms no generation bump is required (digest attestations are exempt from the policy bump). Lore-id: 6e3a1c04 Constraint: digest-only refresh, no generation bump, no source change Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/telegram-daemon-generation-guard.test.ts (75 pass); --validate-current-tree pass at merged dev head Not-tested: none
2 participants
Summary
After session-host rehost, the periodic reconcile re-attaches every session in one pass. Each attachment's initial
event_replaywas awaited inside the serialized#reconcileTail, so one slow replay (up to its full retry budget ≈40s) wedged every later tick and the sends funneling through them. Leases and inbound polling stayed green while outbound publications froze fleet-wide until daemon restart and backlog replay.Root cause
#publishAttachmentL1166 awaited#replayAttachmentsynchronously inside the reconcile path:With
ATTACH_CONCURRENCY=4and a 13-session fleet, one reconcile cycle blocked for ~170s. The 2s periodic timer chained behind the wedged tail. Publications froze becausebarrier.heldcaptured live frames during replay, and a failed replay setbarrier.failedwhich silently dropped all subsequent frames.Fix
The periodic reconcile timer now passes
deferReplay=truethrough#serialReconcile → #reconcile → attachWorker → #attach → #publishAttachment, which publishes immediately and runs initial replay on the attachment'sreadyTail— the same surface#reinitializeAttachmentalready uses for the reconnect path.start()and the publicreconcile()keep synchronous replay so bootstrap callers and deterministic tests are unchanged.What's preserved
#drainHeldFramesruns at end of#replayAttachment)barrier.heldset beforepublished=trueandpublication.resolve()readyTailonAttachmentReadystill awaited synchronously before replayTest evidence
bun run checkpasses.bun run lintpasses.Closes #4527
gajae.pr-review-verdict.v1 merge-approved sha256:a6cf95bb6d2c3b1fdfd134d3b97a7a7b423f893d25237eec041bb686320c3100 reviewer:human reviewer-id:probepark evidence:base 39pass-1fail (#4527 wedge) -> head 40pass-0fail; check exit0