fix(sdk): preserve discovery during concurrent launches - #4149
fix(sdk): preserve discovery during concurrent launches#4149Yeachan-Heo wants to merge 16 commits into
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: c7fcca798c
ℹ️ 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".
| } catch (error) { | ||
| const result = failLifecycleStartup("failed", error); | ||
| finishStartup(result); | ||
| await cleanupAbandonedStartup(); | ||
| return result; | ||
| logger.warn( | ||
| `notifications: provider daemon ownership unavailable; core SDK remains available: ${String(error)}`, | ||
| ); |
There was a problem hiding this comment.
When the initial Discord or Slack daemon ensure rejects, this catch preserves the core endpoint but also allows startSession to report success. The retained runtime then makes later reconciliation and /notify on take the already path; unlike Telegram, these providers have no separate preflight, so ensureConfiguredProviderDaemons is never called again. The command consequently reports notifications as enabled even though no provider daemon became available, leaving delivery broken for the session. Preserve the SDK runtime, but track adapter readiness and retry the failed provider before notification activation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved at 91a6f9a7ba966713a049e5f76feab3948b9541c4. Provider readiness is now separate from core SDK ownership; a failed configured-provider ensure remains retry, activation is gated on aggregate readiness, and /notify on re-runs the full configured-owner check without republishing the endpoint. The regression counts a second provider attempt and verifies unchanged endpoint content/mtime.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
c7fcca7 to
aa7edd8
Compare
Post-#4098 refactor final candidateExact base/head: The direct relay/serve transport deleted by #4098 remains deleted. Discovery identity regressions were relocated to The current head also repairs the shard-load maintenance status blocker: over-threshold pruning proves artifact availability before expensive hashing/planning, the fixture remains qualifying but smaller, and it asserts exactly one failed install attempt. Expected Focused cohort: 314 passed; six worktree API contracts 6/6; checks passed. Exact-head CI 31370995123 is queued/running. No merge. — |
47b24e4 to
99258ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99258ec103
ℹ️ 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".
| // Core SDK authority is published before optional notification adapters acquire | ||
| // daemon ownership. A blocked adapter must not make an interactive session | ||
| // undiscoverable or uncontrollable. | ||
| const endpoint = await sdkRuntime.startTransport(); |
There was a problem hiding this comment.
When Telegram ownership is blocked while Discord or Slack is also configured, ensureConfiguredDaemonOwners() returns blocked_identity_with_sibling, so this newly published standard endpoint is immediately torn down at lines 5420-5424 and replaced by an isolated chat endpoint. During the potentially slow provider-ensure window, direct discovery consumers such as SDK serve or MCP can select and connect to this transient endpoint, only to have their connection and in-flight requests invalidated by cleanupAbandonedStartup(). Determine whether the runtime must be isolated before exposing discovery, or otherwise avoid publishing an endpoint that this branch will abandon.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved at 91a6f9a7ba966713a049e5f76feab3948b9541c4. Discovery scope is fixed before publication. If Telegram ownership changes after a canonical endpoint is live, notification adapters fail closed and the canonical core endpoint/session-index locator remain intact; no cleanup/republication to chat scope occurs. Preflight-proven foreign owners still start directly in chat scope.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
99258ec to
5dafea1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dafea1cc2
ℹ️ 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".
| // The sidecar and raw monitor intentionally race to publish the first valid | ||
| // verdict. If the exact supervisor-exit path loses that race or times out | ||
| // under load, recover only from the same fully validated durable evidence. | ||
| return await waitForExpectedVerdict(identity, sleep, now); |
There was a problem hiding this comment.
When the supervisor observer reaches its FORCE_CLOSE_VERDICT_TIMEOUT_MS - 500 timeout without producing a verdict, this fallback calls waitForExpectedVerdict(), which creates a fresh 15-second deadline even though the close intent expires at the original 15-second deadline. Thus a failed gjc session force-close can block for roughly 29.5 seconds and spend the second interval polling for evidence whose authorization has already expired; pass the original deadline or only the remaining budget into the durable-evidence fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved at 91a6f9a7ba966713a049e5f76feab3948b9541c4. The immutable deadline used for expiresAt is passed to both the preferred observer and durable fallback. Even an injected observer that never settles is raced against the remaining original intent budget; the regression advances past expiry and proves zero fallback sleeps and no cleanup.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
BLOCK — two real holes, both in the exact behavior #4146 asks for. Rebases cleanly onto dev (3e902206b), no conflicts.
BLOCKER 1 — discovery publication is still non-atomic
packages/coding-agent/src/sdk/host/websocket-transport.ts:199-214 writes straight to the final endpoint path, then chmods it:
await filesystem.writeFile(endpointFile, JSON.stringify({ version: 1, sessionId, url, token, pid }), "utf8");
// …separately, afterwards:
await filesystem.chmod(endpointFile, 0o600);Two consequences:
- A concurrent reader can observe truncated or partial JSON mid-write — which is exactly the "discovery can disappear under concurrent launch" symptom this PR is meant to remove.
- The file carries the auth token and briefly exists at default creation permissions before the chmod lands. That is a small but real disclosure window on a multi-user box.
Fix: write a uniquely named temp file with mode 0600, then rename it over the final path. Rename is atomic on the same filesystem, so a reader sees either the old record or the complete new one, never a partial, and the token is never world-readable even momentarily.
The six-lane test does not catch this because it only reads after Promise.all(start()) resolves, and each lane uses a separate state root. It never reads during publication.
BLOCKER 2 — provider readiness failures can never retry
packages/coding-agent/src/sdk/bus/index.ts:5428-5432 swallows Discord/Slack daemon startup errors but keeps the runtime. Every subsequent start then short-circuits to "already" at :3750-3757, and activation proceeds at :5757-5766 without ever re-attempting provider ownership.
So the notification daemon is permanently dead for the life of the process, with no retry path. Isolating the failure from core SDK startup is right — #4146 explicitly requires that — but isolation became permanent suppression.
The test at packages/coding-agent/test/notifications-config.test.ts:1924-1977 only asserts the endpoint still exists. It never asserts a second daemon attempt or working delivery, so it passes on a runtime that will never notify again.
Fix: keep core startup isolated, but track notification readiness separately and retry before activation.
What checks out
- Session-index locking is properly bounded: 600 attempts × 100 ms (
session-index.ts:79-84), exhaustion throws clearly (file-lock.ts:305-318). Dead owners reclaimed immediately, indeterminate owners after the inherited 10s stale bound, live owners never time-reaped (file-lock.ts:228-239). It reuses the shared lock machinery, so it does not contradict #4015 — different resource, same primitives. - No identity collision risk:
Bun.randomUUIDv7()atsession-manager.ts:2208-2209. No timestamp- or PID-derived identity anywhere in the path. - Tests are load-bearing: production reverted → 139 pass / 9 fail; reapplied → 147 pass / 1 fail. The single failure (
settings-enabled subagent sessions…) reproduces unchanged on pristinedev, so it is not yours. - Neighbouring lifecycle/broker suites: 72 pass / 0 fail.
Coverage gap worth naming
Automated concurrency covers six child session-index contenders and six simultaneous transports — but not six real worktree sessions. #4146's acceptance asks that session.metadata and turn.prompt succeed for each of 6+ concurrently launched panes, and nothing here proves that. Given BLOCKER 1 is a during-publication race, a test that only reads after settle cannot close it either.
5dafea1 to
8d191e3
Compare
8d191e3 to
1b729db
Compare
The dev-shard regression surfaced by #4149 workflow-dispatch run 31360117739 shard-1 shows gjc gc --disk --prune reclaiming a blob as unreferenced_by_any_surviving_session while a session transcript keeps changing under the mark; the sweep must withhold with keep:withheld_evidence_incomplete: sessions_changed_during_mark instead. Reproduced on exact dev d73bc90 (2/25), #4149 base e5ff1c1 (1/25), and dev head 06bf6d2 (4/40 single-test, 2/15 full-file). gc-runtime.ts and the retention test are byte-identical between e5ff1c1 and dev HEAD, and the #4149 branch diff touches zero GC files: the race is pre-existing dev code introduced by #4037, not by #4149. The bundle is GPG-signed (key 6A8D48D0B4C7ACA36463DF217CCCF17C606579E8); verify with gpg --verify SHA256SUMS.asc SHA256SUMS && sha256sum -c SHA256SUMS. Lore-id: gc-mark-sweep-changing-transcript Confidence: high Scope-risk: narrow Reversibility: easy Tested: 4/40 2/25 1/25 2/15 failure-rate reproductions across dev head, exact dev, and #4149 base
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b729db0b3
ℹ️ 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".
| return await ensureTelegramOwner(settings); | ||
| const result = await ensureTelegramOwner(settings); | ||
| const runtime = runtimes.get(binding.sessionId); | ||
| if (runtime) runtime.notificationOwnerState = result === "ready" ? "ready" : "blocked"; |
There was a problem hiding this comment.
When Telegram and Discord or Slack are both configured, a startup can leave notificationOwnerState as retry because Telegram ownership succeeded but the sibling daemon ensure threw. During reconciliation, this Telegram-only preflight then overwrites the aggregate state to ready, so runtime.start() skips ensureConfiguredDaemonOwners() and activation reports notifications as running while the failed sibling daemon remains unavailable. Fresh evidence in this revision is this new assignment clearing the retry state based solely on Telegram readiness; preserve the sibling retry until every configured provider has been ensured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved at 91a6f9a7ba966713a049e5f76feab3948b9541c4. Telegram preflight no longer clears an aggregate retry. On a preflight-isolated chat endpoint, blocked_identity_with_sibling becomes ready only after the full configured-provider retry succeeds. The foreign-owner regression injects sibling failure then recovery, proves activation, and verifies the original chat endpoint content and mtime are unchanged.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
1b729db to
20381a0
Compare
The dev-shard regression surfaced by #4149 workflow-dispatch run 31360117739 shard-1 shows gjc gc --disk --prune reclaiming a blob as unreferenced_by_any_surviving_session while a session transcript keeps changing under the mark; the sweep must withhold with keep:withheld_evidence_incomplete: sessions_changed_during_mark instead. Reproduced on exact dev d73bc90 (2/25), #4149 base e5ff1c1 (1/25), and dev head 06bf6d2 (4/40 single-test, 2/15 full-file). gc-runtime.ts and the retention test are byte-identical between e5ff1c1 and dev HEAD, and the #4149 branch diff touches zero GC files: the race is pre-existing dev code introduced by #4037, not by #4149. The bundle is GPG-signed (key 6A8D48D0B4C7ACA36463DF217CCCF17C606579E8); verify with gpg --verify SHA256SUMS.asc SHA256SUMS && sha256sum -c SHA256SUMS. Lore-id: gc-mark-sweep-changing-transcript Confidence: high Scope-risk: narrow Reversibility: easy Tested: 4/40 2/25 1/25 2/15 failure-rate reproductions across dev head, exact dev, and #4149 base
20381a0 to
91a6f9a
Compare
The dev-shard regression surfaced by #4149 workflow-dispatch run 31360117739 shard-1 shows gjc gc --disk --prune reclaiming a blob as unreferenced_by_any_surviving_session while a session transcript keeps changing under the mark; the sweep must withhold with keep:withheld_evidence_incomplete: sessions_changed_during_mark instead. Reproduced on exact dev d73bc90 (2/25), #4149 base e5ff1c1 (1/25), and dev head 06bf6d2 (4/40 single-test, 2/15 full-file). gc-runtime.ts and the retention test are byte-identical between e5ff1c1 and dev HEAD, and the #4149 branch diff touches zero GC files: the race is pre-existing dev code introduced by #4037, not by #4149. The bundle is GPG-signed (key 6A8D48D0B4C7ACA36463DF217CCCF17C606579E8); verify with gpg --verify SHA256SUMS.asc SHA256SUMS && sha256sum -c SHA256SUMS. Lore-id: gc-mark-sweep-changing-transcript Confidence: high Scope-risk: narrow Reversibility: easy Tested: 4/40 2/25 1/25 2/15 failure-rate reproductions across dev head, exact dev, and #4149 base
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91a6f9a7ba
ℹ️ 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".
| const remainingMs = deadline - now().getTime(); | ||
| if (remainingMs <= 0) throw new Error("owner_term_verdict_timeout"); | ||
| const timeout = Promise.withResolvers<never>(); | ||
| const timer = setTimeout(() => timeout.reject(new Error("owner_term_verdict_timeout")), remainingMs); |
There was a problem hiding this comment.
In the exact-owner force-close observer path, this adds a hand-built setTimeout rejection promise instead of the repository-required Bun.sleep(ms) mechanism, bypassing the established Bun timing convention and the surrounding sleep abstraction. Race the verdict against Bun.sleep(remainingMs) (or pass the existing sleep dependency into this helper) rather than creating a timer promise.
AGENTS.md reference: AGENTS.md:L124-L128
Useful? React with 👍 / 👎.
The dev-shard regression surfaced by #4149 workflow-dispatch run 31360117739 shard-1 shows gjc gc --disk --prune reclaiming a blob as unreferenced_by_any_surviving_session while a session transcript keeps changing under the mark; the sweep must withhold with keep:withheld_evidence_incomplete: sessions_changed_during_mark instead. Reproduced on exact dev d73bc90 (2/25), #4149 base e5ff1c1 (1/25), and dev head 06bf6d2 (4/40 single-test, 2/15 full-file). gc-runtime.ts and the retention test are byte-identical between e5ff1c1 and dev HEAD, and the #4149 branch diff touches zero GC files: the race is pre-existing dev code introduced by #4037, not by #4149. The bundle is GPG-signed (key 6A8D48D0B4C7ACA36463DF217CCCF17C606579E8); verify with gpg --verify SHA256SUMS.asc SHA256SUMS && sha256sum -c SHA256SUMS. Lore-id: gc-mark-sweep-changing-transcript Confidence: high Scope-risk: narrow Reversibility: easy Tested: 4/40 2/25 1/25 2/15 failure-rate reproductions across dev head, exact dev, and #4149 base
b4e1adb to
ab558fb
Compare
8ef5f01 to
33709ac
Compare
ba6084d to
f706b6a
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Reviewed the stale delta through f706b6ad15455e26a9648e8d045112bebec4f8a8, concentrating on the changes after the prior human review.
Merge state
This branch is not review-ready against current dev: GitHub reports DIRTY / CONFLICTING, and the readable merge analysis shows conflicts in at least packages/coding-agent/CHANGELOG.md, packages/coding-agent/src/sdk/broker/session-index.ts, and packages/coding-agent/src/sdk/bus/index.ts. The PR also currently targets refactor/sdk-owned-session-lifecycle, while the repository contract requires PRs to target dev. Please rebase onto current dev, resolve the conflicts, and retarget the PR to dev. I reviewed only the portions that remain readable.
Findings
Major — shutdown can leak the newly registered Telegram root during startup
packages/coding-agent/src/sdk/bus/index.ts:5558-5583 keeps the token produced by ensureConfiguredDaemonOwners() only in the local registrationToken variable until the entire aggregate provider-readiness call finishes. Telegram registration can therefore succeed and mint replacement authority, then Discord/Slack readiness can remain pending or fail before the finally assigns initializedRuntime.notificationRootRegistration.
That interval is observable during shutdown. packages/coding-agent/src/sdk/bus/index.ts:6776-6817 intentionally does not join an in-flight sessionStartPromises entry; it calls stopSession() immediately. The release path at packages/coding-agent/src/sdk/bus/index.ts:3808-3820 unregisters only when rt.notificationRootRegistration is already populated. In this race it is still empty, so shutdown completes without releasing the root; the startup finally then stores the token on a runtime that has already been removed. Token fencing makes this worse in the correct way: an older token cannot clean up the newly minted registration, so the root is left live.
Record replacement authority in the onRegistered callback immediately, while checking that the runtime is still authoritative. If registration completes after fencing/removal, explicitly unregister that late token instead of attaching it to the dead runtime. Apply the same ownership rule to the retry callback at packages/coding-agent/src/sdk/bus/index.ts:5855-5878.
Minor — the newest ownership fix has no behavioral regression test
Commit f706b6ad changes registration-token retention, but its only change in packages/coding-agent/test/sdk-host-wiring.test.ts is whitespace at line 618. The nearby cleanup test at packages/coding-agent/test/sdk-host-wiring.test.ts:522-588 covers release after successful provider startup, while the retry fixture at packages/coding-agent/test/notifications-config.test.ts:2001-2101 stubs Telegram as blocked and therefore never mints a registration token. Neither exercises the contract the newest commit claims to fix.
Add an observable test where Telegram registration succeeds, a sibling provider is held pending or rejects, and shutdown occurs before aggregate readiness settles. Assert that the newest token is used and the roots registry is empty. Also cover a failed sibling-provider attempt followed by retry replacing the token and successful cleanup.
Prior findings
The earlier non-atomic publication issue is now addressed by mode-600 temp creation plus same-directory rename, and the provider retry path now preserves the published core endpoint. The new shutdown race above is in the token-retention follow-up added after that review.
Focused tests could not start in this checkout because workspace-linked modules such as @gajae-code/utils/native-process, @gajae-code/utils/shell-config, and @gajae-code/ai/core are unavailable; no install or project-wide gate was run.
gajae.pr-review-verdict.v1: needs-human
f706b6a to
255fb0c
Compare
Repair evidenceExact head: Resolved the startup/shutdown ownership race: Telegram root registration is retained synchronously in each registration callback only while its runtime remains authoritative. A callback completing after runtime fencing unregisters its token rather than attaching it to a removed runtime. The retry callback follows the same rule. Regression coverage:
Verification:
— |
255fb0c to
dd40cd5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd40cd521f
ℹ️ 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".
| } | ||
| const runtime = runtimes.get(binding.sessionId); | ||
| if (runtime) { | ||
| if (runtime.notificationOwnerState === "retry") { |
There was a problem hiding this comment.
When a retained default runtime is marked blocked by a Telegram ownership conflict and the configuration is subsequently changed to disable Telegram while keeping Discord or Slack enabled, reconciliation no longer runs the Telegram preflight and this branch retries ownership only for the retry state. The runtime therefore remains permanently blocked, activate() refuses to enable delivery, and /notify on cannot recover until the session restarts even though the remaining provider can be safely ensured. Re-evaluate configured-provider ownership for blocked runtimes when the blocking provider is no longer effective.
Useful? React with 👍 / 👎.
dd40cd5 to
a243345
Compare
The private bridge client is no longer packed, but the tarball smoke still referenced its unset path under set -u.\n\nLore-id: private-bridge-install-smoke\nConstraint: private bridge client must not be packed or installed\nConfidence: high\nScope-risk: narrow\nReversibility: easy\nTested: bash -n; no bridge_client_tgz reference; release-publish-order
Notification adapter ownership failures could tear down an otherwise healthy SDK host, while the session index abandoned contenders after five seconds and exact tmux close trusted only one competing verdict publisher. Publish core discovery first, extend the bounded index serialization budget with dead-owner recovery, bind discovery records to session identity, and recover exact-owner cleanup from validated durable verdict evidence. Lore-id: a4146c0d Constraint: core SDK control must remain independent of notification daemon ownership Constraint: concurrent session-index contenders must not steal a live owner Rejected: globally disable notifications | hides the production coupling instead of fixing it Rejected: unconditional tmux cleanup | weakens exact-owner fencing Confidence: high Scope-risk: wide Reversibility: clean Tested: 197 focused SDK, notification, session-index, and tmux tests Tested: coding-agent biome and TypeScript checks Tested: six concurrent isolated git worktree launch probes with unique usable endpoints Not-tested: full monorepo test suite
Filename-derived legacy discovery remains supported, but empty or path-like identifiers cannot address a session safely. Lock the parser boundary and add direct Telegram-ownership coverage proving canonical SDK discovery survives while the adapter stays blocked. Lore-id: b4146d1d Constraint: legacy minimal discovery records remain filename-authoritative Constraint: unusable or mismatched session identities fail closed Confidence: high Scope-risk: narrow Reversibility: clean Tested: notification and SDK serve discovery suites Tested: coding-agent Biome and TypeScript checks
Keep opaque legacy session identifiers usable, make the tmux durable-verdict fallback deterministic under test, and state the six-process test scope accurately. These refinements keep the post-#4098 delta narrow and mutation-sensitive. Lore-id: c4146e2e Constraint: PR #4098 remains the sole ownership architecture Constraint: legacy opaque session identifiers remain usable Confidence: high Scope-risk: narrow Reversibility: clean Tested: deterministic tmux fallback regression Not-tested: full focused cohort after this refinement
Publishing the core endpoint before optional notification ownership exposed an early-server callback that could enqueue activity before the canonical identity record. Record identity in the host before transport startup while keeping direct socket publication and notification ownership after the server is listening. Lore-id: d4146f3f Constraint: core endpoint publication must not wait for notification ownership Constraint: identity must precede every lifecycle event in replay Confidence: high Scope-risk: narrow Reversibility: clean Tested: startup ordering regression repeated 20 times Tested: 146 notification and SDK host wiring tests Tested: coding-agent Biome and TypeScript checks
Discovery records contained control tokens but were written directly to their final path before chmod, allowing partial reads and a permission window. Publish a complete mode-600 sibling through atomic rename, prove six endpoints accept metadata and prompt controls, and retain failed provider readiness as an independently retryable notification state. Lore-id: e4146040 Constraint: PR #4098 remains the sole lifecycle and attachment ownership architecture Constraint: discovery tokens must never exist outside mode 0600 Constraint: notification retry must reuse the core SDK runtime Confidence: high Scope-risk: wide Reversibility: clean Tested: 289 focused SDK, notification, session-index, tmux, and host tests Tested: atomic during-publication reader boundary and six metadata/prompt controls Tested: provider retry repeated 10 times Tested: coding-agent Biome and TypeScript checks
Telegram-only preflight could incorrectly clear an aggregate sibling-provider retry, late ownership conflicts could rotate an already-published core endpoint, and tmux fallback polling could outlive its authorization intent. Preserve aggregate readiness, keep the canonical endpoint stable on late conflicts, and bind all verdict polling to the original expiry. Lore-id: f4146151 Constraint: a published core endpoint is never invalidated to recover notification adapters Constraint: full configured-provider readiness precedes notification activation Constraint: verdict evidence cannot outlive its owner intent Confidence: high Scope-risk: wide Reversibility: clean Tested: 291 focused issue tests Tested: mixed readiness, late ownership, and intent-expiry races repeated 10 times Tested: coding-agent Biome and TypeScript checks
Telegram-only preflight could clear a pending sibling-provider retry on an isolated chat endpoint, while injected owner observers were not universally bounded by the original force-close intent. Preserve full configured-provider readiness and race every observer against the same immutable expiry. Lore-id: 04146262 Constraint: Telegram proof cannot substitute for Discord or Slack readiness Constraint: every owner observer shares the original intent deadline Confidence: high Scope-risk: narrow Reversibility: clean Tested: 291 focused issue tests Tested: foreign Telegram sibling retry and hung observer deadline repeated 10 times Tested: coding-agent Biome and TypeScript checks
Over-threshold maintenance could spend its test budget planning a large tool-output eviction after exact artifact persistence was already unavailable, allowing timeout cancellation to surface aborted instead of the fail-closed failed contract. Prove persistence first and keep the qualifying fixture compact enough for shard load. Lore-id: 14146773 Constraint: artifact persistence failure reports failed, never timeout-driven aborted Constraint: do not widen the 15-second contract timeout Confidence: high Scope-risk: narrow Reversibility: clean Tested: exact persistence row 10 times Tested: eight concurrent isolated test workers all passed under five seconds Tested: coding-agent Biome and TypeScript checks
PR #4098 removed the direct SDK serve transport suite. Keep issue #4146's filename/body identity, unsafe-name, stale tombstone, and opaque legacy-record coverage on the surviving SDK client surface instead of reviving the retired transport tests. Lore-id: 24146884 Constraint: do not restore removed direct relay architecture Constraint: legacy minimal discovery remains filename-authoritative Confidence: high Scope-risk: narrow Reversibility: clean Tested: 314 post-refactor focused tests Tested: coding-agent Biome and TypeScript checks
PR #4098 removed the direct sdk serve socket/stdio relay and prohibited unsafe machine adapter ingress, but the Python and operation-matrix fixtures still asserted the retired surface. Keep real-session cleanup on the supported authenticated WebSocket path, assert every prohibited adapter explicitly, and bind prompt diagnostics to an isolated lifecycle startup capability so concurrent shards cannot race an unowned host. Lore-id: 34146995 Constraint: do not restore direct socket or stdio relay commands Constraint: bash and session lifecycle controls remain prohibited on all adapters Constraint: prompt diagnostics await owned startup without widening timeouts Confidence: high Scope-risk: narrow Reversibility: clean Tested: Python real websocket cleanup Tested: 18 machine-adapter mutation rows Tested: coding-agent shard 13 -- 928 pass, 0 fail Tested: coding-agent shard 1 prompt diagnostics pass; remaining failures are base-owned model/tmux rows Tested: Python mypy and coding-agent checks
Over-threshold maintenance continued into digest planning or a full branch clone after exact artifact persistence was already unavailable or had failed. Under shard load those wasted paths reached the test cancellation boundary and surfaced aborted instead of the fail-closed failed contract. Return immediately, clean any earlier publications, and keep canonical output untouched. Lore-id: 44146006 Constraint: unavailable or failed exact persistence reports failed, never aborted Constraint: canonical tool output remains unchanged and no artifact survives Constraint: retain original 15s and 30s timeout contracts Confidence: high Scope-risk: narrow Reversibility: clean Tested: both failure rows 10 times each Tested: eight concurrent workers, both rows, all under ten seconds Tested: full maintenance file 19 pass Tested: coding-agent Biome and TypeScript checks
A failed exact artifact publication was indistinguishable from an unneeded prune, allowing mid-run maintenance to continue into promotion or compaction. Return an explicit persistence-failure result, stop the maintenance checkpoint with failed, and directly cover rollback after one saved artifact followed by a second publication failure. Lore-id: 54146117 Constraint: persistence failure cannot fall through to compacted or not-needed Constraint: remove staged eviction payloads while retaining monotonic ID claims Constraint: canonical outputs remain unchanged Confidence: high Scope-risk: narrow Reversibility: clean Tested: both persistence failure rows 10 times each Tested: eight concurrent workers in 2.6-2.9 seconds Tested: full maintenance file 19 pass Tested: coding-agent Biome and TypeScript checks
A terminal artifact-persistence failure may occur after an earlier eviction payload was staged. Report the staged count with zero realized savings so below-threshold maintenance still emits its rollback notice while mid-run maintenance terminates failed. Lore-id: 64146228 Constraint: rollback never reports reclaimed tokens Constraint: staged payload deletion remains observable Confidence: high Scope-risk: narrow Reversibility: clean Tested: cache-epoch staged rollback 10 times Tested: both mid-run persistence failures 10 times each
The rewritten #4098 base introduced per-session Telegram root registration tokens. Preserve the token across provider readiness failure and retry so cleanup keeps exact ownership while #4146's aggregate readiness state remains retryable and the core endpoint remains stable. Lore-id: 74146339 Constraint: registration tokens survive sibling-provider readiness failure Constraint: retry replaces runtime authority with the newest exact token Constraint: late ownership never rotates the canonical endpoint Confidence: high Scope-risk: narrow Reversibility: clean Tested: notifications and SDK host lifecycle 150 pass Tested: coding-agent Biome and TypeScript checks
Telegram registration can complete while a sibling provider is still pending. Retain the token at registration time so shutdown releases it, and release late callbacks instead of attaching them to a removed runtime. Lore-id: pr-4149-startup-root-release Confidence: high Scope-risk: narrow Reversibility: revert-commit Tested: bun test packages/coding-agent/test/sdk-host-wiring.test.ts; bun run check Constraint: preserve discovery while provider readiness is pending
a243345 to
4cc568e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cc568efdc
ℹ️ 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".
| void unregisterNotificationRoot({ settings, cwd, sessionId: id, registrationToken }).catch(error => | ||
| logger.warn(`notifications: late Telegram root unregister failed: ${String(error)}`), | ||
| ); |
There was a problem hiding this comment.
When session shutdown removes a runtime while registerNotificationRoot() is still completing, this late-callback branch becomes the only owner of the newly minted registration token. Because the unregister is fire-and-forget, shutdown can return before it settles, and any transient lock/I/O rejection is merely logged rather than retained in cleanupRetries; the .gjc roots registry can therefore keep a stale session/root registration indefinitely. Track and await this cleanup during shutdown, or retain it as retryable ownership evidence.
Useful? React with 👍 / 👎.
2 participants
Tracks #4146. This PR is a stacked post-#4098 regression delta, not an independent ownership-architecture landing.
Current relationship to #4098
Frozen base/head:
c911ffde92b8d43cf5c0675d29350230e84e488f→6f75ef392c7070972114f2672cb3993e2ec350da.#4098 remains the sole SessionLifecycleService/SessionRouter ownership inversion and now removes the direct attachment relay/serve transport. This stack keeps those surfaces deleted; discovery compatibility coverage moved to the surviving SDK client suite.
Delta
failedExact-head evidence
session.metadata6/6,turn.prompt6/6Stale-head CI attribution
Run 31368777253 on
91a6f9a7confirmed a real shard-load maintenance timeout. The current repair proves artifact availability before expensive over-threshold planning and shrinks the still-qualifying fixture; the outward status remainsfailed, the timeout remains 15s, and the exact row now passes under eight concurrent workers.No merge requested. #4098 must land first.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]