fix(sdk): stop idle ACP sessions re-scanning the session index every 2s (#4689) - #4730
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
760cc53 to
9f3f680
Compare
|
Status: all agent lanes green; one human step remains.
Unblock: an approving GitHub review on PR head — |
ecd351b to
59063ca
Compare
Owner evidence for exact head 59063ca (base 02c739e)Host protection (this lane). Runaway descendant PID 3036130 ( Guard drift classification (base control). Telegram daemon generation guard run at exact base 02c739e with identical env: base fails identically on both telegram Native-build timeout (run 32296952361). Cancellation was in Local verification on 59063ca. sdk-session-index 54p + sdk-session-router-authority 47p + guard suite 75p all pass; Review state. needs-human is intentional: awaiting @snowykr's exact-head approving review (write-eligible collaborator, not the PR author). No self-approval; merge happens only after independent approval + coherent green product CI. — gjc agent · owner lane |
|
Handoff note for the live PR lane (this issue-side lane is retiring as duplicate coverage; no further mutation from here). Evidence reconciliation — items produced here that may not be visible elsewhere:
— |
Infrastructure attribution for cancelled run 32301002198 (superseded by coherent green 32303072237)Exact head 59063ca, base 02c739e. Cancellation cause, from job step timestamps: every cancelled job ( Surviving same-head run 32303072237 is coherent product green: 43 success / 2 platform-skip / 1 failure, where the single failure is Remaining blocker (by design): independent exact-head approving review from @snowykr (requested). Upon that review the verdict flips to — gjc agent · owner lane |
59063ca to
e22630b
Compare
Notify
|
| variant | CPU for 15 idle ticks |
|---|---|
| base session-router/session-index (02c739e content) | 581 ms |
| fixed head e22630b | 4 ms |
~145× reduction on the identical workload; the fixed binary's idle daemon would sit near-idle on the router path. The daemon's Telegram getUpdates long-poll (25s server-side timeout, bounded jittered conflict backoff) is a network wait, not a CPU loop, and is unchanged by this PR.
Live-daemon remediation is owner-operational, not repo-side: PID 122912 must be rotated onto a fixed build (it is the active notification authority for the shared agent dir; a repo-lane kill would drop notifications). No code change in this PR is required for it.
— gjc agent · owner lane project-pr-4730-current-exact-1500 · gaebal-gajae
e22630b to
c7aac58
Compare
Rebase onto dev tip 883ab16 — new exact head c7aac58Dev advanced (PR #4733 webhook merge). Transplanted the single #4689 commit onto 883ab16 (CHANGELOG context conflict resolved by keeping both entries; also removed a stray resolution comment line the prior rebase had accidentally committed). New digest On-head verification: sdk-session-index 54p + sdk-session-router-authority 47p + coordinator-mcp-server 113p all pass; telegram daemon generation guard Remaining sole blocker: independent exact-head approving review from @snowykr. — gjc agent · owner lane |
c7aac58 to
b5ff1ec
Compare
Rebase onto dev tip d97b79e — exact head b5ff1ecDev advanced (PR #4612 agent/ai stream fix; disjoint from this PR's sdk files — clean rebase, zero conflicts). Pushed with On-head verification (b5ff1ec): sdk-session-index 54p + sdk-session-router-authority 47p pass; telegram daemon generation guard Fresh CI for b5ff1ec is queued (Dev CI 32335050827). Sole remaining gate: independent exact-head approval from @probepark. — gjc agent · owner lane |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
This is a focused fix for the idle-session index rescan/lock-contention problem, and the core change-stamp and forced-reconciliation design is sound. However, the patch currently expands an exported SDK contract, carries unrelated protected-manifest attestations, leaves a central lock-contention regression insufficiently asserted, and does not fence the persistent Telegram daemon rollout.
Findings / Required Changes
-
[P2] Keep the maintenance heartbeat out of the public attachment contract
packages/coding-agent/src/sdk/router/session-router.ts:63-64makessendMaintenance(leaseId)a required member of the exportedSessionAttachmentinterface. This is a structural breaking change for downstream TypeScript providers and test doubles that implement the documentedsend()-based attachment capability. Keep this maintenance-only operation behind a non-exported internal capability, or explicitly make it a documented, versioned SDK contract change with a compatible migration path. -
[P2] Split unrelated protected-authority manifest attestations from this CPU fix
scripts/telegram-daemon-generation-manifest.json:526-527,652updates protected hashes forcreateNotificationsExtension,exactUnlinkNotificationFile, andcrates/pi-natives/src/path_identity.rs, even though their sources are not part of this PR. The PR description identifies this as pre-existing base drift. Regenerate and review that drift separately, then keep only manifest changes attributable to this router/daemon change here. -
[P2] Assert that unchanged polling does not acquire the global session-index lock
packages/coding-agent/test/sdk-session-index.test.ts:1305-1315verifies that five unchanged polls perform no log reads, but does not observe lock acquisition. A regression that moves the unchanged path back insidewithFileLock()without callingreadFile()would pass this test while restoring the exact machine-global contention this PR fixes. Use the existing scopedFileLockTestHooks.afterParentMkdirmechanism to count the session-index lock attempts after baseline initialization and assert zero alongside the no-read assertion. -
[P2] Fence the Telegram daemon when rolling out the changed Router behavior
packages/coding-agent/src/sdk/bus/chat-daemon-control.ts:129-130advances Discord/Slack generations but leaves the persistent Telegram daemon atDAEMON_GENERATION 170/SERVING_EPOCH 87, although it constructs the changedSessionRouter. Generation alone does not force replacement; serving-epoch compatibility does. An already-running Telegram daemon can therefore retain the old hot polling loop after upgrade. Advance the Telegram generation and serving epoch, document the rollout boundary, regenerate the manifest, and add the shared Router polling dependency to the guard inventory. -
[P3] Record the Discord/Slack generation rationale in the authoritative history
packages/coding-agent/src/sdk/bus/chat-daemon-control.ts:129-130changes Discord65 → 66and Slack68 → 69without a matching generation-history entry. Add one stating that these generations fence the SessionRouter idle-poll/change-stamp rollout so future compatibility reviews can determine why replacement is required.
CI / Verification
- Reviewed the changed test suite and GitHub-provided checks for exact head
b5ff1eca192f2c751713de83e7fdc14e40fb0c52; no PR code was executed locally. - Successful exact-head evidence included the Telegram daemon generation guard, CLI smoke, Windows
dev:doctor/session-path regression, native build, local public surfaces, and completed PR contract/static/runtime/read checks. - Human-gated PR-contract failures were excluded as directed.
- At review time, the exact-head Dev CI dynamic matrix was still queued, including affected SDK tests, coding-agent type/root checks, package shard, production-host isolation, broker/notification tests, TS builds, and cargo builds. The CI result was therefore not final.
Axis Coverage
| Axis | Result | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | Changes requested | Public SDK contract expansion and unrelated protected-manifest scope checked. |
| A2. Architecture / Correctness / Failure | Approved | Change stamps, reconciliation escalation, lifecycle/race paths, and maintenance-frame behavior checked; no A2 defect found. |
| A3. Security / Privacy / Trust | Approved | Authority, stale transport, maintenance-frame, path, token, and trust-boundary paths checked; no A3 defect found. |
| A4. Verification / Tests / CI | Changes requested | Changed tests and exact-head GitHub artifacts checked; lock-contention assertion is incomplete and CI remained queued. |
| A5. Context / Compatibility / Platform | Changes requested | Callers, public exports, daemon generations, generated artifacts, package/platform surfaces checked. |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review at exact head b5ff1eca — approved.
what changes
The old router did a locked open() plus refresh() and a full session projection every 2 seconds per attached router, and ACP lease heartbeats forced that same path every 5 seconds. Now an unchanged tick costs two index-file stats; projection waits for a 30-second sweep; lease maintenance sends a fixed-shape frame without authority reconciliation.
the thing I check on a stop-doing-work fix
A fix that stops periodic work must not stop necessary work. I traced each thing the scan was responsible for:
- Endpoint rotation — index appends wake it immediately; also revalidated by force and by the retained sweep.
- Broker/WebSocket replacement — the transport reconnect path is retained and unchanged ticks still call
connect(), so revival stays on the 2-second cadence. - Dead PID and stale heartbeat — covered by the 30-second sweep (dead PID, 2×60s heartbeat expiry).
- Dispatch — forces the full body, so idle never delays real work.
So detection is bounded rather than lost, which was the question.
the idle/work race is closed
Attach-throw latches an immediate retry, and force escalation plus queued follow-up closes the idle-versus-dispatch race. Nothing goes idle holding work.
the index reader is careful
SessionIndex.refreshIfChanged() preserves the ENOENT distinction, reclassifies changes under the index lock, tails cooperative append-only growth, fully replays truncation/rewrite/snapshot/rotation shapes, never fast-paths a corrupt suffix, and replays after same-instance compaction. That is the part where a cheap-tick optimization usually goes wrong, and it does not here.
sendMaintenance(leaseId) is a narrow fixed provider-heartbeat capability: it checks attachment currency, stamps endpoint/connection authority, and cannot carry command traffic.
minor — the idle-sweep test does not sweep anything interesting
test/sdk-session-router-authority.test.ts:1815-1889 proves the reconcile body and listing run on the sweep, but never mutates endpoint authority, PID liveness or heartbeat age to assert retirement/replacement. The behavior is supported by the explicit-reconcile and reconnect tests plus the implementation, so this is a coverage gap rather than broken behavior. Worth adding timer-driven dead/stale and endpoint-revalidation cases.
coverage otherwise
New index tests pin unchanged polling, append/removal, same-instance rotation, corrupt suffix, same-size rewrite, snapshot-only replacement, and append/compact interleaving. New router tests pin idle gating, sweep execution, queued-dispatch escalation, failed-attach retry and maintenance frame shape. Retained tests still pin endpoint replacement, rotation during validation, endpoint disappearance, token/URL rotation and reconnect handshakes. Typed SessionAttachment fixtures were updated for the new required method.
Reviewed by @probepark — method: detached worktree at b5ff1eca, enumerated what the removed per-tick work was responsible for detecting and located each replacement path, read refreshIfChanged for error-distinction and partial-write handling, checked the idle-versus-dispatch escalation, and inspected sendMaintenance for capability scope. Tests not executed.
gajae.pr-review-verdict.v1 merge-approved sha256:e974af968f5228a538c44d39bb2204e011147855d878e12aac307901d10794fe reviewer:human reviewer-id:probepark evidence:exact-head-b5ff1eca-idle-ticks-cheapened-with-rotation-replacement-and-staleness-detection-all-retained
Issue-lane #4689 verification — root cause and fix effect independently confirmed at exact head
|
Corrected live state + verified resolution for both blockers (coordination, not a push)Superseding my earlier note: Also correcting the record for #4689 readers: the issue comment citing PR head I did not push to your branch. Everything below was verified in a throwaway detached worktree so you can apply it directly. Blocker 1 — conflict is one semantic hunk, and "take theirs" is wrongAgainst
Both must survive: async reconcile(options: { waitForReplay?: boolean } = {}): Promise<void> {
const waitForReplay = options.waitForReplay ?? true;
// Explicit callers always force the full body (#4689): the idle gate must
// never make an explicit reconcile a no-op.
await this.#serialReconcile(this.#runEpoch, !waitForReplay, true);
if (!waitForReplay) return;Verified on the resolved tree: Blocker 2 — @snowykr's finding 3 is correct, and here is the discriminating testConfirmed by reading I proved the gap is real by mutation, not by argument. I injected exactly that regression (unchanged path re-enters The existing read-only assertion does not fail on that mutant. Reverted source → 56/56 pass. Patch (+36 lines, let lockAttempts = 0;
FileLockTestHooks.afterParentMkdir = () => { lockAttempts++; };
try {
for (let i = 0; i < 5; i++) expect(await index.refreshIfChanged()).toBe(false);
expect(logReads).toBe(0);
expect(lockAttempts).toBe(0);
...
} finally {
FileLockTestHooks.afterParentMkdir = undefined;
spy.mockRestore();
}Why the fix is worth landing (independent numbers, exact head
|
Rebase target moved again — resolution + zero-lock test re-verified on
|
b5ff1ec to
f1d95c0
Compare
Rebased onto current dev + all @snowykr findings addressed — new exact head
|
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Delta review at exact head eb6df36a — approved.
The approved idle no-rescan contract remains intact. Startup now opens SessionIndex once before forced reconciliation so cold routers recover compacted snapshot authority; periodic idle ticks still use lock-free stamp checks and do not reopen/rescan. Cold refresh replays when no cursor exists and tails only established state.
ACP maintenance capability is rejected before provider activation, preventing lease/resource acquisition on an attachment that cannot renew. Endpoint replacement rejection still retires and closes the attachment exactly once. No recurring scan, stale mutation, wrong authority, regression, or leak found.
Reviewed by @probepark — finite exact-object/blob review from approved 10354955.
gajae.pr-review-verdict.v1 merge-approved sha256:bc7c2dbbf1a84e091fe8b93c2f77ba7bb63af022f07cf832defda2bd67d07170 reviewer:human reviewer-id:probepark evidence:exact-head-eb6df36a-cold-compacted-index-and-preactivation-capability-fixes
eb6df36 to
1c0f45d
Compare
Current-base reconciliation completeCurrent PR head: 1c0f45d Re-fetched and inspected all 69 commits that landed on dev since the prior base. The material overlap was confined to rollout/documentation surfaces: current dev independently moved Telegram generation 171→172 while keeping epoch 87, and updated the registry assertion/manifest; the PR's approved idle-poll rollout then correctly advanced the serving epoch to 88. Autorouting/session changes in dev did not overlap the PR's runtime implementation blobs. Rebased the approved replacement onto current dev and resolved the sole test conflict by preserving the epoch-88 idle-poll fence. Approved runtime blobs remain byte-identical to the prior approved head for session-index, session-router, ACP adapter, chat-daemon control, and Telegram contract; only the rebase-integrated rollout/test/manifest context changed. The changelog has exactly one complete #4689 entry. Validation at this current-base head: focused session/index/router/ACP/reconnect/registry tests pass; SDK/coordinator suite passes 399 tests; coding-agent package check passes; state-writer, virtual-integration, daemon-generation, native build, canonical build, and contract tests pass. One SDK host-wiring full-suite timeout reproduced green on the targeted retry and full suite retry; it was a transient test timeout, not a source failure. The old probepark approval is stale after this push. Please perform a delta re-review at 1c0f45d, especially the four overlapping rollout files and the epoch-88 fence. Exact-head CI is restarting; the verdict remains needs-human until current-head independent approval arrives. — |
Every attached session's SessionRouter ran a 2s reconcile that re-acquired the machine-global index lock and re-read, re-parsed, and re-checksummed the whole session index, then re-projected every historical row — O(total index history) per tick per live session, forever. A fresh ACP session on an aged agent dir burned a sustained 30-70% of a core while completely idle (#4689), and concurrent pollers degraded session creation through lock contention. The index now keeps a change stamp over the log and snapshot files, so a polling reader proves "nothing changed" with two stats and reloads append-only changes through the tail reader. The router's idle tick is the stamp check plus a transport-revive no-op; the full attach/retire body runs only on index changes, pending adoptions, or a 30s liveness sweep (inside the index's own 2x60s heartbeat-freshness window). Measured idle CPU on a 15k-row index drops from a sustained ~30% to ~1% mean. Lore-id: 4689a1c2 Constraint: prompt latency must not regress -- the pre-send reconcile keeps exact authority revalidation on the send path Constraint: transport revival latency must stay at the 2s tick -- revive runs even on gate-skipped ticks Rejected: incremental projection maintenance | far larger change to security-sensitive projection code for the same idle win Rejected: slowing the 2s timer wholesale | would delay transport revival and external-change detection Confidence: high Scope-risk: narrow Reversibility: revertible Directive: authority write paths (append/unregister) must keep exact locked replay; the stamp fast path is for polling reads only Tested: 190 targeted sdk/index/router/acp tests, package typecheck, live FIFO ACP repro with /proc CPU sampling before/after Not-tested: macOS host (issue platform); behavior verified on Linux x86-64
…lling The idle-CPU fix removes a machine-global session-index lock acquisition from every 2s router tick, but three review findings left it unsafe to land. The existing regression only asserted that an unchanged poll performs no reads, so a change that put the unchanged path back inside withFileLock() without reading would pass while restoring the exact lock contention that caused the reported starvation -- the assertion now counts lock acquisitions directly, with a companion test proving a changed index still takes the lock so the zero-lock claim cannot go vacuous. Both new assertions were mutation-verified: injecting that regression fails them while the old read-only assertion passes it. A running chat daemon constructs its own SessionRouter, so generation alone does not force replacement -- an already-running Telegram daemon could keep serving the old hot polling loop across upgrade. The serving epoch advances with the generation to make replacement mandatory, and the Discord/Slack generation rationale is recorded in the authoritative history so future compatibility reviews can see why. Lore-id: 4689f3nc Constraint: sendMaintenance stays a required SessionAttachment member -- an optional capability would silently fall back to send() and restore the 5s heartbeat-forced locked rescan Constraint: unrelated manifest drift (createNotificationsExtension, path_identity.rs) is pre-existing on clean dev and the guard fails closed on it, so it cannot be split out without a red gate Rejected: make sendMaintenance optional | fail-open fallback reintroduces the regression Rejected: bump generation without serving epoch | generation alone does not force daemon replacement Confidence: high Scope-risk: medium Reversibility: safe Tested: mutation-proof that the zero-lock assertion catches a withFileLock re-entry regression Tested: telegram generation guard exit 0 against dev base 88ff56b Not-tested: macOS absolute idle-CPU percentages (Linux host reproduces the mechanism, not the reporter's 5-63%)
…tion Independent review found the cheap idle path could miss a durable change: the stamp compared size, mtimeMs and ctimeMs, and timestamps are not a durable-equality proof. Measured on ext4, ~98% of back-to-back same-size writes report identical mtimeMs AND ctimeMs, so a same-size snapshot replacement inside one filesystem tick was indistinguishable from no change -- and because the unlocked pre-check returns early, none of the locked reclassification safeguards could recover it. A stale projection would then persist until the next detectable mutation or a restart. The fix costs one field. The cooperative writer protocol only ever appends (which grows size) or rename-replaces (replaceAtomically, used for snapshots and rotation), and rename always installs a new inode: 0/3000 inode collisions measured against 2809/3000 timestamp collisions for the same operation. Anchoring on size+inode makes the fast path sound for every mutation the protocol can produce, and the timestamps remain as an extra signal for foreign in-place edits. Lore-id: 4689ino1 Constraint: the idle path must stay lock-free -- fixing this by taking the index lock would restore the contention #4689 exists to remove Rejected: durable monotonic generation record | one extra durable write per mutation to prove what the inode already proves Rejected: high-resolution mtimeNs | not exposed by the fs stat surface in use, and still not injective Confidence: high Scope-risk: low Reversibility: safe Tested: mutation-proof -- dropping the inode comparison makes the new collision test report a real change as unchanged Tested: 374 focused tests across index/router/adapter/daemon suites Not-tested: non-ext4 filesystems (test self-skips when the host reports high-resolution timestamps)
…l-closed Review of the previous head found the idle-poll optimization had leaked into paths that carry authority, and that its own regression coverage did not reach the routes it protects. A forced pass backs a dispatch, adoption, explicit reconcile, or start. Routing those through the unlocked change stamp meant the stat could not order itself against a writer committing an unregister/re-registration before the caller read the projection, so a forced request could go out through an attachment that was no longer authoritative. Forced passes now take the locked authority read; only timer ticks keep the stamp cut, so the idle-CPU win is untouched. Lease renewal had the same shape: it trusted the in-memory attachment, and because the idle path can defer projection to the 30s sweep, a replaced endpoint could keep receiving heartbeat renewals through that window. Renewal now revalidates durable endpoint authority and fails closed. sendMaintenance becomes optional on the exported capability so existing consumer-provided attachments stay source- and runtime-compatible, but the caller fails closed when it is absent rather than falling back to send() -- a fallback would restore the 5s heartbeat-forced locked rescan. The stamp keeps the inode as bigint so a 64-bit inode or Windows file id above MAX_SAFE_INTEGER cannot collapse onto another id. Lore-id: 4689auth Constraint: only timer ticks may use the unlocked stamp -- every authority-bearing caller must serialize on the index lock Rejected: keep sendMaintenance required | breaks exported SessionAttachment implementations including resolveAttachment callbacks Rejected: optional with send() fallback | silently reintroduces the 5s locked rescan this fix removes Confidence: high Scope-risk: medium Reversibility: safe Tested: mutation-proof -- reverting the heartbeat to send() fails both new ACP heartbeat tests Tested: idle sweep now asserts dead-pid and not-live retirement, not just that the body ran Tested: 393 focused + 370 broader SDK tests, tsc clean Not-tested: non-ext4 filesystems for the inode-collision path
The previous commit added inode identity to the change stamp and to the unlocked "definitely unchanged" cut, but the locked classifier that chooses tail-vs-replay still decided on size and timestamps alone. So a log that grew while the snapshot was replaced by a different file of the same size with matching timestamps was classified as append-only: the reader tailed from its last offset and cached a stale compacted projection, then served it as current until another mutation or a restart. Serving stale authority as fresh is the exact failure this work exists to prevent, and the evidence to prevent it was already being collected -- the decision in between just never consumed it. Tailing now additionally requires the same log inode and the same snapshot inode. Verified at the predicate level: with the guards the grew-plus-swapped case replays, without them it tails. Lore-id: 4689tail Constraint: tailing is only sound for the same file grown in place -- a new inode on either file must force replay Confidence: high Scope-risk: low Reversibility: safe Tested: predicate-level proof that the grew-plus-inode-swapped snapshot case tails without the guards and replays with them Tested: combined-case regression through the real reader with a valid same-size signed snapshot Tested: 378 focused tests, tsc clean
Fixing the lease-renewal authority hole in the previous commit reached for per-session heartbeat that restores exactly the locked full index scan this work exists to remove -- the fix for one review finding reintroduced the original defect. The heartbeat never needed index re-projection. It only needs to know that this endpoint record still carries the authority the attachment was published with, and the endpoint file proves that by itself: mtime fence, body identity (sessionId/pid/url/token/stale), then a re-stat to catch a write racing the read. That is bounded and lock-free, so renewal still fails closed on a replaced endpoint without paying for the index. The changelog also still advertised the pre-rebase generation numbers. Lore-id: 4689hbaf Constraint: nothing on the 5s heartbeat may enter the machine-global index lock Rejected: keep #readEndpoint and accept the refresh | that is the #4689 regression, just moved to a different timer Confidence: high Scope-risk: low Reversibility: safe Tested: mutation-proof -- routing renewal back through #readEndpoint fails the new locked-path assertion Tested: 491 tests across index/router/adapter/daemon/host suites, tsc clean
Routing forced passes through the locked read correctly serializes them against concurrent writers, but calling open() as well re-entered the shared open-group state that live attachments and replay cursors depend on. Exact-head CI caught it: socket-loss resume stopped replaying from the last acknowledged event, and a replay refused on a live socket lost its cursor position. refresh() is what takes the machine-global index lock, so it alone provides the ordering the authority path needs. open() is idempotent group setup that start() already performed, and repeating it per dispatch buys nothing. Lore-id: 4689open Constraint: forced passes must still take the index lock -- only the redundant open() is dropped Confidence: high Scope-risk: low Reversibility: safe Tested: chat-daemon-session-reconnect 25/25 (was 2 failing on CI), 491 tests across the SDK suites, tsc clean
Review found the bounded heartbeat authority check compared only sessionId, pid, stale, url, token and mtime. None of those is a replacement-safe identity: a rename-replace that preserves the body and lands in the same filesystem tick keeps every one of them, so a superseded endpoint could still have leases renewed against it for the whole sweep interval. Renewal now compares the endpoint inode captured when the attachment was published, plus the indexed generation. The inode is recorded at publish time and dropped on detach, so the check stays bounded and lock-free -- the point of the previous commit -- while gaining an identity a same-tick replacement cannot forge. The ACP adapter also rejected a missing sendMaintenance only when the heartbeat eventually fired, which meant a custom attachment silently stopped renewing leases. Unsupported attachments are now rejected at setup with an explicit migration message. Lore-id: 4689ino2 Constraint: renewal must remain lock-free -- identity comes from a stat, never from the index lock Rejected: compare inode before/after the body read only | both reads happen after the replacement, so they agree and prove nothing Rejected: leave the setup check to the first heartbeat | a lease that stops renewing silently is the failure mode being fixed Confidence: high Scope-risk: low Reversibility: safe Tested: mutation-proof -- without inode identity a rename-replaced endpoint renews; with it renewal fails closed Tested: 497 tests across index/router/adapter/daemon/host/stale-attachment suites, tsc clean
Review of the previous head found the published-inode cache was dropped only in #retireAttachment, so replacement, stop, and failed-publication teardown leaked entries on a long-lived router accumulating rehosts. Cleanup moves into dispose(), which every teardown path already routes through. The two verification gaps were real. The renewal regression did not assert the metadata collision it depends on, so the mtime fence could have rejected the replacement before the inode check ran and the test would still pass with the inode comparison removed; it now asserts that the replaced file carries the indexed mtime and a different inode. The setup-time capability rejection had no coverage at all; an attachment without sendMaintenance is now asserted to fail start() with a migration error and to leave no lease behind. Lore-id: 4689disp Confidence: high Scope-risk: low Reversibility: safe Tested: 498 tests across index/router/adapter/daemon/host/stale-attachment suites, tsc clean Not-tested: contractVersion 52 claim from review -- guard and manifest both declare 51 on this head and on dev, and the guard verifies clean
Review found the published inode was captured at registration, well after the endpoint authority proof. A rename-replace landing in that window would be recorded as the trusted identity -- the replacement this identity exists to detect could install itself as the baseline. Capture now happens adjacent to the #readEndpoint proof and is carried forward, so nothing re-stats the file later and trusts what it finds. The maintenance capability guard also only covered start() and the heartbeat send. acceptAttachment is the single admission point for the replacement and ready handoffs, so the assertion moves there: a capability-less attachment can no longer take over live leases and quietly stop renewing them. Lore-id: 4689atom Constraint: identity must be proven with authority, never re-observed afterwards Confidence: high Scope-risk: low Reversibility: safe Tested: mutation-proof -- removing the handoff guard fails the new replacement-refusal test Tested: 499 tests across nine SDK suites, tsc clean
… every ready path Review showed the inode was still sampled outside #readEndpoint: the authority proof returned endpoint data, then #attach separately stat'd a mutable path, so an identical rename between the two would install the replacement's inode as the trusted baseline. Identity is now captured inside #readEndpoint, compared across its own body read, recorded for the exact endpoint path, and re-checked at publication; stat failure fails closed instead of collapsing to "absent". attachmentReady's same-object branch also never reached the guarded acceptAttachment, so a current attachment lacking the maintenance capability could still activate providers and take leases it can never renew. The assertion moves ahead of the branch. The capability pin now builds an adapter whose current attachment is the capability-less one, so it exercises the same-object branch instead of re-testing the different-object guard, and asserts zero register_provider traffic. Lore-id: 4689pin2 Constraint: identity must be proven by the authority read itself, never re-observed later Confidence: high Scope-risk: low Reversibility: safe Tested: mutation-proof -- removing the pre-branch guard fails the same-object capability pin Tested: 499 tests across nine SDK suites, tsc clean Not-tested: an attach-window endpoint replacement pin -- reconciliation re-reads authority after the replacement and converges on the successor, so the scenario could not be made to discriminate; the non-discriminating draft was removed rather than kept as false evidence
…pares
Three times now identity evidence was added without reaching the check
that needed it: first the inode was missing from the locked append-only
classifier, then it was sampled outside the authority read, and then the
authority read proved an inode that #publishAttachment never consulted.
Publication compared url/token/pid, so an identical-byte rename during
the pre-publication hooks matched every field and was accepted at the
real commit point while the proof sat unread in a path-keyed side map.
The side map is gone. #readProvenEndpoint returns { endpoint, ino }, so
the proof travels with the value and the commit point compares the same
inode the authority read proved. Proof and consumer are now one value and
cannot drift apart, which is what ends this class of gap rather than
moving it one step further along.
Rejection paths also leaked: the transport is connected by then, so an
undefined or mismatched inode returned without closing it. All rejections
now dispose handlers and close the client.
The new router pin exercises the validation-to-publication window
directly by renaming inside onAttachment, and it discriminates: removing
the commit-point inode comparison fails it. The capability pin now
configures a provider so its zero-registration assertion carries weight
instead of being zero either way.
Lore-id: 4689one
Constraint: identity proof and the decision consuming it must be the same value, never two observations
Rejected: keep the path-keyed side map | that indirection is exactly how the proof and its consumer drifted apart
Confidence: high
Scope-risk: medium
Reversibility: safe
Tested: mutation-proof -- removing the commit-point inode comparison fails the new publication-window pin
Tested: mutation-proof -- removing the pre-branch capability guard fails the same-object pin
Tested: 500 tests across nine SDK suites, tsc clean
The pin let the in-hook rename adopt a fresh mtime and re-declared it as indexed authority, so #readProvenEndpoint rejected on the mtime fence before reaching the inode comparison. It passed for a reason adjacent to the one it names, and would have kept passing with the identity check deleted -- the one property a regression pin has to have. The indexed mtime is now fixed for the whole test and preserved across the rename, so the replacement is indistinguishable from the original on every field except the inode, and the test asserts exactly that. Lore-id: 4689pin3 Confidence: high Scope-risk: low Reversibility: safe Tested: mutation-proof -- removing the commit-point inode comparison now fails this pin (it did not before)
A forced startup refresh tailed only the empty log on a cold SessionIndex, so a compacted snapshot could leave a new router blind until another append. Seed the reader once at startup and make the locked refresh total for cold readers. Move ACP capability rejection ahead of provider activation, and pin publication rollback client closure. Lore-id: 4689startup Constraint: idle timer ticks must remain lock-free when the index is unchanged Constraint: forced authority reads must remain serialized under the session-index lock Confidence: high Scope-risk: medium Reversibility: safe Tested: sdk-session-index cold refresh and 60-test suite; sdk-session-router-authority targeted pins; ACP capability tests; coding-agent typecheck
1c0f45d to
6666591
Compare
Newest-base refreshCurrent PR head: 6666591 Dev advanced once more after the prior rebase with a single test-only commit widening the descendant-reap wait in packages/coding-agent/test/tools/bash-resource-lifecycle.test.ts. Rebased onto that exact tip; no PR runtime file overlap or semantic change occurred. The overlap test passes all 9 tests, the five approved runtime blobs remain byte-identical to the old approved head, and the current diff remains 23 files with the same canonical digest. This push invalidates the prior current-base approval and CI. Please re-review the exact head, with attention to the unchanged epoch-88 rollout fence and the clean newest-base ancestry. The verdict remains needs-human until authenticated approval targets 6666591. — |
Exact-head delta approval requestedThe current pushed head is 6666591 on current dev 235aee5. The approved runtime blobs are unchanged byte-for-byte from the previously approved implementation; only current-dev ancestry and test/rollout context were reconciled. Product checks are green or skipped, and the independent architecture review is APPROVE with no P1/P2 blockers. Please bind the maintainer delta approval to 6666591. Prior approval at eb6df36 is stale by commit identity after the required current-base rebase. — |
Fix-forward: deterministic reconnect settlement coverageCurrent PR head: 32a5149 The failed affected-path job Fix-forward made the regression coverage deterministic instead of weakening the assertion: the Slack retry-budget test and Discord ambiguous-publication test now await the router's The runtime implementation blobs remain unchanged from the approved implementation. This push invalidates prior approvals and CI; please perform a fresh exact-head delta review at 32a5149. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Delta review at exact head 32a5149a — approved. Prior approval remains valid.
All approved router, SessionIndex, ACP adapter, authority, lifecycle, and reconnect regression blobs are identical. The sole current semantic delta is test-harness synchronization: reconnect cases now wait for the existing frame-settlement callback instead of racing cursor advancement through timing sleeps.
No production scan, stale mutation, authority, lifecycle, resource, or durability behavior changed. Harness waiters remain local and bounded by existing test timeouts.
Reviewed by @probepark — exact blob/direct-child reaffirmation from approved eb6df36a.
gajae.pr-review-verdict.v1 merge-approved sha256:d987b76999a8a211b93b0c661d95b5e95281e40cf6e49e6764436b561e48314c reviewer:human reviewer-id:probepark evidence:exact-head-32a5149a-test-synchronization-only
Merged and issue resolvedPR #4730 merged into Post-merge The canonical checkout — |
3 participants
What
Fixes #4689 by removing the unconditional 2-second idle ACP session-index rescan. Timer ticks now use the lock-free change stamp and only changed or liveness-sensitive work performs reconciliation; forced dispatches retain locked authority ordering. ACP maintenance heartbeats emit their fixed frame without re-entering the index path.
The hardened implementation carries log/snapshot inode identity through the locked append-only classifier, binds lease renewal to the published endpoint inode, captures endpoint identity inside the authority proof, guards every attachment handoff before provider activation, closes rejected clients, and makes the publication-window regression pin discriminate on inode identity. Startup now performs the one-time index open/replay boundary so a cold router sees compacted snapshot state before its forced refresh.
Why
An idle attached session previously re-acquired the machine-global session-index lock and re-read/re-projected the full index every 2 seconds, causing sustained CPU and lock contention on aged agent directories. The startup replay correction closes the companion cold-reader regression found during exact-current-dev review.
Testing
bun test packages/coding-agent/test/sdk-session-index.test.ts packages/coding-agent/test/sdk-session-router-authority.test.ts packages/coding-agent/test/sdk-acp-adapter.test.ts packages/coding-agent/test/sdk-acp-provider-reconnect.test.ts packages/coding-agent/test/sdk-acp-stale-attachment-4356.test.ts— 137 passed before the final lint-only cleanup; the cleanup changes only optional-chain/unused-variable diagnostics and was formatted with Biome.bun --cwd=packages/coding-agent run check:types— passed.bun run check:tools— passed with no errors after the final cleanup.bun run check:rs— passed.bun --cwd=packages/natives run check— passed.git diff --check origin/dev...HEAD— passed.Review remediation
The semantic PR commits were transplanted with original authorship onto current
dev, then rebased to basef2734ae811692ceafb67328a6339f7df6f85fca6. Exact-current-dev review found and fixed the cold compacted-snapshot startup regression, added a discriminating client-close assertion, moved ACP capability rejection before provider activation, and cleaned two source diagnostics, regenerating the daemon manifest attestation for the final source hash. The priorcb106140approval is stale after these content changes; an exact-head adversarial reaffirmation is requested fromprobepark.Risk classification
low-riskregression-riskhigh-riskGJC verdict
devbun checkpassesf2734ae811692ceafb67328a6339f7df6f85fca6and head480abc3c1842852220f5129658c5ad15cd9cc678.