fix(tests): retain exact SDK broker fixture owners#2265
Conversation
fc49053 to
2314089
Compare
![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: fc490531ef
ℹ️ 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".
| } | ||
|
|
||
| function linkWorkspacePackages(scopeDir: string, packages: LinkedWorkspacePackage[]): string[] { | ||
| function linkWorkspacePackages(scopeDir: string, packages: LinkedWorkspacePackage[]): void { |
There was a problem hiding this comment.
When createHarnessCliEnv(fakeRepo, ...) is used by the existing packages/coding-agent/test/harness-control-plane/cli.test.ts cleanup-contract test, this helper now only creates links in the temporary NODE_PATH tree and no longer creates/records the repoRoot/node_modules/@gajae-code/* symlinks the test immediately lstats. That makes the unchanged test fail with ENOENT and also removes the fallback source-package resolution path for harness CLI subprocesses in uninstalled worktrees; please restore the repo-node_modules link/marker behavior or update all callers/tests to the new contract.
Useful? React with 👍 / 👎.
| const started = await withFixtureBrokerEnvironment(() => | ||
| startFixtureBrokerWithLeaseForTest({ agentDir, env: linked.env }), | ||
| ); | ||
| const rootCleanup = createFixtureRootCleanup(agentDir, agentDir, started.lease); |
There was a problem hiding this comment.
For harness tests that call createHarnessCliEnvWithFixtureBroker(repoRoot, root), the afterEach paths no longer remove root directly and rely on this fixture cleanup. Registering agentDir as the cleanup root only deletes <root>/agent, leaving the fake tmux logs/server state and root-registry under <root> after every successful test; pass the outer fixtureRoot here so the root-wide cleanup actually removes the owned harness tree once the broker lease is closed.
Useful? React with 👍 / 👎.
Terminal hostile review — CLEAR / APPROVEExact head:
Current exact-head evidence: 136 focused tests pass with 609 assertions; coding-agent check/type/canonicalization passes; repository build passed; signed commit verified. Independent hostile Architect verdict: CLEAR / APPROVE, blockers: none. CI is still settling; merge remains gated on every exact-head required check reaching terminal success. — |
|
Exact-head CI found one platform-specific blocker on Fixed in signed commit — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66cdd1c8c2
ℹ️ 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".
| broker-fixture-lease: | ||
| name: Broker fixture lease regression / ${{ matrix.platform }} |
There was a problem hiding this comment.
This added matrix job is not included in the stable Affected path validation aggregate, whose needs and result checks only cover affected-plan, affected-native, and affected-shards. In the branch-protection setup described by this workflow comment, a Linux/macOS/Windows broker-fixture regression can fail without failing the required aggregate status, so the new coverage may be bypassed unless this job is added to the aggregate dependency/result check or otherwise made required.
Useful? React with 👍 / 👎.
| key: "acp-subprocess", | ||
| requiredOwner: "runtime-and-broker", | ||
| shutdown: () => teardown(oracle), | ||
| dispose: () => oracle.drainStderr(), |
There was a problem hiding this comment.
When teardown(oracle) hits its documented failure path where the ACP subprocess is still alive after SIGKILL, cleanupFixtureRoot still runs the dispose phase after the failed shutdown, and this callback awaits stderrDrain, which only resolves once the child's stderr fd closes. In that stuck-child scenario the cleanup hangs until the test timeout instead of surfacing the teardown error and preserving the root; make this drain conditional on confirmed exit or add a bounded wait.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4df72e1f64
ℹ️ 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".
| await cleanupFixtureRoot(cleanup); | ||
| authStorage.close(); |
There was a problem hiding this comment.
When this test runs on Windows (the new broker-fixture matrix includes windows-latest), cleanupFixtureRoot(cleanup) recursively removes cwd, but authStorage is still open on cwd/testauth.db until the next line. Windows commonly refuses to delete open SQLite/database files, so cleanup can fail and retain the broker/root; close or register authStorage before removing the fixture root.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bddc3d441
ℹ️ 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 agentDir = path.join(repo, ".gjc", "agent"); | ||
| const stateRoot = path.join(repo, ".gjc", "state"); | ||
| const environment = createFixtureBrokerEnvironment(repo, agentDir); | ||
| const fixtureSessionDir = path.join(agentDir, "sessions", "-"); |
There was a problem hiding this comment.
When invokeScenario reaches session.resume or session.fork, the lifecycle broker validates the supplied transcript by inventorying SessionManager.getDefaultSessionDir(cwd, agentDir) in validateSavedTranscript, but this fixture now writes the source session under the hard-coded agentDir/sessions/-. For the temp repos created here, the default directory is an encoded -tmp-... path, so the saved session is absent from the broker inventory and the shipped mcp/daemon lifecycle topology tests fail with invalid_input instead of exercising resume/fork.
Useful? React with 👍 / 👎.
|
Successor repair pushed at |
|
Exact-head repair pushed as signed commit Failure classification and repair:
Validated local evidence on the successor diff:
The earlier local shard timeout cascade was environment/process contamination: another live runner was concurrently executing PR remains open and unmerged pending exact-head adversarial review and terminal green required checks. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db8be54afd
ℹ️ 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".
|
|
||
| export function createFixtureBrokerEnvironment(root: string, agentDir: string): NodeJS.ProcessEnv { | ||
| const environment: NodeJS.ProcessEnv = { | ||
| HOME: root, |
There was a problem hiding this comment.
When these fixture environments are used on Windows, setting only HOME does not pin os.homedir() for the spawned broker/session-host processes; Bun/Node use USERPROFILE on win32 (docs), and otherwise fall back to the real user profile. Any fixture child path that expands ~ or uses os.homedir() can therefore read or write outside the owned fixture root, defeating the cleanup/isolation this helper is adding; set USERPROFILE (and preferably HOMEDRIVE/HOMEPATH) to the fixture root as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68d945afab
ℹ️ 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".
| while !self.accept_task.is_finished() || !self.state.connections.lock().is_empty() { | ||
| tokio::task::yield_now().await; |
There was a problem hiding this comment.
When a TCP client is accepted but stalls before completing the WebSocket handshake, accept_loop has already spawned handle_conn, but that task does not insert into state.connections until after accept_hdr_async_with_config and the hello write succeed. In that state this loop can see the aborted accept task as finished and an empty connection map, so stop_and_wait() returns while an accepted socket task is still alive, breaking the teardown boundary that the JS cleanup now relies on. Track spawned connection tasks (or insert/mark them before the handshake) instead of using only the post-handshake connection map.
Useful? React with 👍 / 👎.
68d945a to
43f9282
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f92829b3
ℹ️ 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".
| } | ||
| for (const [id, pending] of this.#pending) | ||
| this.#settlePending(id, pending, new SdkClientError("connection_closed", "SDK client closed")); | ||
| await Promise.all([...transports].map(incarnation => this.#closeTransport(incarnation))); |
There was a problem hiding this comment.
With this change close() can remain pending until #closeTransport receives the socket close event, but #closed is set before that await. If a second teardown path calls and awaits close() while the first close is still in progress, it returns immediately and may remove the state/root while the WebSocket is still open; keep and return a shared close promise until the transport wait finishes.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
| if (Number(socket.readyState) === WebSocket.CLOSED) resolve(); | ||
| await promise; |
There was a problem hiding this comment.
When the peer stops completing the close handshake, for example a half-open or stalled SDK client, #closeTransport now waits solely for a close event with no timeout or error fallback. Any caller that awaits SdkClient.close() during shutdown can hang indefinitely; add a bounded fallback or abort path so close remains a deterministic cleanup operation.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| cancel.cancel(); | ||
| while connections.join_next().await.is_some() {} |
There was a problem hiding this comment.
In shutdown, the accept loop now waits for every connection task without a cap. If a client has stopped reading while a connection task is already awaiting send_msg or drains queued direct frames after stop(), cancellation cannot interrupt that await, so stop_and_wait() can hang forever instead of releasing the fixture/state root; abort or time-bound remaining connection tasks after cancellation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d3fdd2ad
ℹ️ 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 (ownerReleaseFailures.length > 0) { | ||
| throw new AggregateError(ownerReleaseFailures, `SDK notification runtime ${id} owner release failed.`); |
There was a problem hiding this comment.
When rt.host.stop(), snapshot close, or rt.server.stopAndWait() throws during a fixture session_shutdown, this new throw correctly fails the cleanup once, but stopSession already deleted the runtime from runtimes before these awaits. A subsequent cleanupFixtureRoot retry calls the same shutdown handler, sees no runtime, and marks the phase verified without retrying the failed host/server release, so the fixture root can be removed while those owners are still open; preserve the failed runtime or keep a retryable release handle until all owner-release phases verify.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd5ada2602
ℹ️ 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".
| let serverStopped = false; | ||
| try { | ||
| rt.server.stop(); | ||
| await rt.server.stopAndWait(); |
There was a problem hiding this comment.
With the new cleanupRetries path, when the first teardown attempt succeeds in rt.host.stop() but this stopAndWait() call throws, the retained retry no longer has a started host: the next rt.host.stop() returns "already", so recordStop(... hostStopped: hostStopped && serverStopped) never marks rollback.hostStopped true even after the server retry succeeds. Lifecycle cleanup proof explicitly requires rollback.hostStopped in packages/coding-agent/src/sdk/broker/lifecycle.ts, so a transient server-stop failure can leave an otherwise completed retry reported as incomplete; carry forward the successful host-stop state or treat "already" on a cleanup retry as released.
Useful? React with 👍 / 👎.
|
Owner-direct recovery update at
Hostile review requested: please attack teardown ordering, retry-authority retention, exact-path cache ownership, root recreation, cross-platform behavior, and any operator-owned state/process risk before merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fc1c1f1be
ℹ️ 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".
| expectedRuntime?: SessionRuntime, | ||
| ): Promise<boolean> { | ||
| const requestedRuntime = runtimes.get(id); | ||
| const requestedRuntime = runtimes.get(id) ?? cleanupRetries.get(id); |
There was a problem hiding this comment.
When a teardown failure stores the old runtime in cleanupRetries, a subsequent session_start for the same session id can still create a replacement because startup only checks runtimes; after that, this lookup always selects the new runtimes entry and the retained failed runtime is no longer reachable by an ordinary retry. In that restart-before-retry scenario the fixture/root cleanup can stop the replacement and proceed while the original failed owner remains open, so the retry map needs to fence startup or address the retained runtime before any replacement with the same id.
Useful? React with 👍 / 👎.
2fc1c1f to
2ed46e7
Compare
|
Rebased the repair onto current Post-rebase proof:
Hostile review remains requested against the rebased head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ed46e7368
ℹ️ 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".
| let serverStopped = false; | ||
| try { | ||
| rt.server.stop(); | ||
| await rt.server.stopAndWait(); |
There was a problem hiding this comment.
When a checkout or installed package loads a stale @gajae-code/natives addon with the same package version but built before NotificationServer.stopAndWait existed, startup still passes assertNativeRuntimeCompatibility because native-runtime-compatibility.ts only checks registerArbitratedAsk/retireIfUnclaimed. The first shutdown then reaches this call and throws TypeError: rt.server.stopAndWait is not a function, which turns ordinary session cleanup into a retained owner-release failure; include stopAndWait in the runtime capability check or keep a safe fallback to stop().
Useful? React with 👍 / 👎.
edae6bb to
9fef6c5
Compare
|
Fresh hostile exact-head review requested for Windows CI evidence addressed:
Fresh-head verification:
Please review exact head only and attack directory-vs-file fsync classification, interrupted cleanup, retry authority, root recreation, and operator-state safety. |
|
@codex review Hostile exact-head review requested for The previous win32 head still failed because Attack whether process isolation hides a real owner leak, whether exact session/model-cache/broker authority still closes before root removal, and whether any operator-owned state can be touched. Do not rely on reviews of older commits. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
40e1a8b to
de55d42
Compare
|
Rebased PR #2265 onto current Supersession check:
Semantic rebase resolutions preserved both sides:
Validation on this exact tree:
@codex review Hostile exact-head review requested for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de55d4273f
ℹ️ 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".
| HOME: root, | ||
| XDG_CONFIG_HOME: path.join(root, "config"), | ||
| GJC_AGENT_DIR: agentDir, | ||
| GJC_CODING_AGENT_DIR: agentDir, | ||
| PI_CODING_AGENT_DIR: agentDir, |
There was a problem hiding this comment.
For Windows fixture children, setting only HOME does not isolate code that calls os.homedir(): Node/Bun use USERPROFILE when present, otherwise the current user's profile directory. The broker/CLI children created with this env import code such as packages/utils/src/env.ts, which eagerly reads path.join(os.homedir(), ".env"), so the new Windows fixture matrix can still read the runner/operator profile instead of the temp root. Please set Windows home/data variables such as USERPROFILE, LOCALAPPDATA/APPDATA, and preferably HOMEDRIVE/HOMEPATH to paths under root rather than relying on HOME alone.
Useful? React with 👍 / 👎.
| cleanupRetries.set(id, rt); | ||
| throw new AggregateError(ownerReleaseFailures, `SDK notification runtime ${id} owner release failed.`); | ||
| } | ||
| cleanupRetries.delete(id); |
There was a problem hiding this comment.
When owner cleanup fails, the old runtime is stored in cleanupRetries; however, if the same session id is started again before that retry succeeds, stopSession() will pick the new runtime from runtimes and this unconditional delete removes the old failed runtime's retry authority after the new runtime stops successfully. In that same-id restart scenario the original leaked owners are no longer reachable for retry, so this should only delete when cleanupRetries.get(id) === rt (and/or block startup until the retained cleanup completes).
Useful? React with 👍 / 👎.
|
Head Diagnosis:
Minimal repair:
Exact local validation:
@codex review Review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98b9230156
ℹ️ 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".
|
|
||
| // Failed terminal teardown remains fenced from normal runtime lookup while the | ||
| // exact runtime object retains authority for an explicit idempotent retry. | ||
| const cleanupRetries = new Map<string, SessionRuntime>(); |
There was a problem hiding this comment.
When a session shutdown fails after runtimes.delete(id), this map is the only remaining owner of the failed runtime, but startSession still checks only sessionStartPromises/runtimes before creating a replacement for the same session id. In that retry/restart scenario the old server/host remains alive in cleanupRetries, the new runtime shadows it because stopSession reads runtimes.get(id) ?? cleanupRetries.get(id), and the exact failed cleanup owner can no longer be retried. Treat cleanupRetries as a startup fence or drain it before allowing a replacement runtime.
Useful? React with 👍 / 👎.
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (process.platform !== "win32" || (code !== "EPERM" && code !== "EACCES")) throw error; |
There was a problem hiding this comment.
syncFile is used for both the temp broker.json file and the parent directory, so this Windows EPERM/EACCES exception now also treats a failed temp-file sync as success. If a permissions/AV/share-mode failure happens while syncing the just-written file, writeBrokerDiscovery will still rename and publish a discovery record whose contents were not durably flushed; only the directory durability barrier should be optional on Windows, e.g. by splitting the helper or passing an explicit directory-sync flag.
Useful? React with 👍 / 👎.
|
Exact head Deduplicated validity matrix
Exact Windows CI facts folded in
Successor verification
@codex review Review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 177ea77869
ℹ️ 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 timeoutMs = this.#remainingTimeout(); | ||
| if (timeoutMs <= 0) throw this.#deadlineError(); |
There was a problem hiding this comment.
When a client is constructed with an absolute deadline and an active socket is still open after that deadline elapses, close() now computes timeoutMs as 0 and throws before it ever calls socket.close(). This makes common cleanup paths like finally { await client.close(); } after a timed-out request leave the WebSocket open with its client-side listeners removed, so the transport can leak until process exit; close should still attempt teardown and use a separate close grace/default instead of the expired operation deadline.
Useful? React with 👍 / 👎.
Reconstructs the minimal still-needed exact-owner SDK broker fixture teardown fix onto current origin/dev, resuming PR #2265. The detached SDK broker spawned for a fixture is now reaped by targeting the exact owned child (pid + incarnation), with a bounded SIGTERM -> SIGKILL -> verified-exit reap and a single-use fixture lease, so a fixture never orphans its broker owner or leaks its root. Core: - src/sdk/broker/ensure.ts: exact-owner reap (reapSpawnedBroker), pid+incarnation owner fencing, single fixture lease. - src/sdk/broker/discovery.ts: broker process incarnation. - src/sdk/bus/index.ts: retained terminal-cleanup proof for idempotent owner release across retries. - src/sdk/bus/native-runtime-compatibility.ts + crates (gjc-sdk server, pi-natives): bounded stop_and_wait/stopAndWait joining half-open and established connections idempotently. - ai/model-cache.ts: close the shared cache only when it owns the exact db path. bridge-client: idempotent close awaiting exact socket owners. - test/helpers/fixture-broker-cleanup.ts (new): root-wide phased fixture teardown (shutdown/dispose, lease close, root removal + verified absence). Wired into sdk-production-host and sdk-lifecycle-fixture. Supersession audit vs origin/dev (dropped from the original PR): - workflow-gate-broker.ts + test: Bun Windows fsync EPERM now handled by #2309. - .github/workflows/dev-ci.yml + dev-ci-notification-atomicity CI hunk + build-sdk-package-smoke TUI-pack: CI/workaround churn. - sdk-public-surface manifest + CHANGELOG entries: unrelated churn. - #2292 is already in this fix's base (built upon, not superseded); #2305 (Discord) touches no file in this set. Repro (#2246): sandboxed operator-safe harness (owned pid only) shows the detached fixture broker alive + root present before teardown and reaped + root absent after fixtureRootCleanup.
177ea77 to
c871404
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c87140419e
ℹ️ 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".
| await retireLiveFixtureOwner(); | ||
| await cliEnv.cleanup(); |
There was a problem hiding this comment.
When retireLiveFixtureOwner() throws (for example, the harness owner remains live after the bounded retirement wait), this afterEach exits before cliEnv.cleanup() closes the fixture broker lease created in beforeEach. That leaves the pre-started broker child running after the test has already failed; preserve the diagnostic root if needed, but close the broker lease in a finally/aggregate cleanup path so the failure does not orphan the fixture broker.
Useful? React with 👍 / 👎.
| await retireLiveFixtureOwner(); | ||
| sdkServer.stop(true); | ||
| await cliEnv.cleanup(); |
There was a problem hiding this comment.
If retireLiveFixtureOwner() fails here (e.g. harness retire fails or the owner is still live), control jumps to the catch block, which only stops the fake SDK server and never runs cliEnv.cleanup(). Since beforeEach now starts a fixture broker with an exact lease, that failure path leaks the broker process; close the fixture lease in a finally path even when preserving the harness roots for inspection.
Useful? React with 👍 / 👎.
Reconstructs the minimal still-needed exact-owner SDK broker fixture teardown fix onto current origin/dev, resuming PR #2265. The detached SDK broker spawned for a fixture is now reaped by targeting the exact owned child (pid + incarnation), with a bounded SIGTERM -> SIGKILL -> verified-exit reap and a single-use fixture lease, so a fixture never orphans its broker owner or leaks its root. Core: - src/sdk/broker/ensure.ts: exact-owner reap (reapSpawnedBroker), pid+incarnation owner fencing, single fixture lease. - src/sdk/broker/discovery.ts: broker process incarnation; temp broker.json fsync is fail-closed (syncFile) while only the directory barrier (syncDirectory) tolerates win32 EPERM/EACCES. - src/sdk/bus/index.ts: retained terminal-cleanup proof; cleanupRetries fences same-id startup so a replacement never shadows an unreaped owner. - src/sdk/bus/native-runtime-compatibility.ts + crates (gjc-sdk server, pi-natives): bounded stop_and_wait/stopAndWait joining half-open and established connections idempotently. - ai/model-cache.ts: close the shared cache only when it owns the exact db path. bridge-client: idempotent close() that always issues socket.close() under a dedicated close grace (never gated on an expired request deadline), so a transport is never leaked on close. - test/helpers/fixture-broker-cleanup.ts (new): root-wide phased fixture teardown wired into sdk-production-host and sdk-lifecycle-fixture. Exact-head hostile-review repairs: - bridge-client close() no longer throws on an elapsed operation deadline before socket.close(); dedicated close grace + leak regression test. - Regression tests assert temp broker.json fsync stays fail-closed (even for EPERM) and the directory-barrier tolerance is win32-scoped only. - Confirmed cleanupRetries already fences same-id startup (owner-release proof preserved across retry). Supersession audit vs origin/dev (dropped from the original PR): - workflow-gate-broker.ts + test: Bun Windows fsync EPERM now via #2309. - .github/workflows/dev-ci.yml + dev-ci-notification-atomicity CI hunk + build-sdk-package-smoke TUI-pack: CI/workaround churn. - sdk-public-surface manifest + CHANGELOG entries: unrelated churn. - #2292 is already in this fix's base (built upon); #2305 (Discord) touches no file in this set. Repro (#2246): sandboxed operator-safe harness (owned pid only) shows the detached fixture broker alive + root present before teardown and reaped + root absent after fixtureRootCleanup.
c871404 to
4eab6fa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eab6fa03a
ℹ️ 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".
| observed, | ||
| stop: async () => { | ||
| const fixtureEnv = createFixtureBrokerEnvironment(agentDir, agentDir); | ||
| return withFixtureBrokerEnvironment(async () => { |
There was a problem hiding this comment.
When this helper is used, withFixtureBrokerEnvironment clears GJC_SDK_DISABLE while createAgentSession runs. In that environment createAgentSession still appends the built-in SDK/notifications extension for top-level sessions (packages/coding-agent/src/sdk/session.ts around the shouldHostSdk(...) branch), so the explicit factory below is registered a second time. The later auto runtime can overwrite the endpoint file, causing callers to connect to an uninstrumented host with no onSdkRequest observations while two servers run for one session; keep the env change scoped to broker startup or restore the old disable guard around session creation.
Useful? React with 👍 / 👎.
Reconstructs the minimal still-needed exact-owner SDK broker fixture teardown fix onto current origin/dev, resuming PR #2265. The detached SDK broker spawned for a fixture is now reaped by targeting the exact owned child (pid + incarnation), with a bounded SIGTERM -> SIGKILL -> verified-exit reap and a single-use fixture lease, so a fixture never orphans its broker owner or leaks its root. Core: - src/sdk/broker/ensure.ts: exact-owner reap (reapSpawnedBroker), pid+incarnation owner fencing, single fixture lease. - src/sdk/broker/discovery.ts: broker process incarnation; temp broker.json fsync is fail-closed (syncFile) while only the directory barrier (syncDirectory) tolerates win32 EPERM/EACCES. - src/sdk/bus/index.ts: retained terminal-cleanup proof; cleanupRetries fences same-id startup so a replacement never shadows an unreaped owner. - src/sdk/bus/native-runtime-compatibility.ts + crates (gjc-sdk server, pi-natives): bounded stop_and_wait/stopAndWait joining half-open and established connections idempotently. - ai/model-cache.ts: close the shared cache only when it owns the exact db path. bridge-client: idempotent close() that always issues socket.close() under a dedicated close grace (never gated on an expired request deadline), so a transport is never leaked on close. - test/helpers/fixture-broker-cleanup.ts (new): root-wide phased fixture teardown wired into sdk-production-host and sdk-lifecycle-fixture. - scripts/build-sdk-package-smoke.ts: pack the matching local @gajae-code/tui so the SDK package smoke exercises the workspace release boundary instead of falling back to an older registry tui missing the SecretInput export (fixes bridge-client-sdk-package-smoke; pre-existing on dev). Exact-head hostile-review repairs: - bridge-client close() no longer throws on an elapsed operation deadline before socket.close(); dedicated close grace + leak regression test. - Regression tests assert temp broker.json fsync stays fail-closed (even for EPERM) and the directory-barrier tolerance is win32-scoped only. - Confirmed cleanupRetries already fences same-id startup (owner-release proof preserved across retry). - Restored the SDK package-smoke TUI-pack fix after CI proved the smoke regresses without it. Supersession audit vs origin/dev (dropped from the original PR): - workflow-gate-broker.ts + test: Bun Windows fsync EPERM now via #2309. - .github/workflows/dev-ci.yml + dev-ci-notification-atomicity CI hunk: CI topology churn coupled to the dropped workflow. - sdk-public-surface manifest + CHANGELOG entries: unrelated churn. - #2292 is already in this fix's base (built upon); #2305 (Discord) touches no file in this set. Repro (#2246): sandboxed operator-safe harness (owned pid only) shows the detached fixture broker alive + root present before teardown and reaped + root absent after fixtureRootCleanup.
4eab6fa to
216a8c6
Compare
Reconstructs the minimal still-needed exact-owner SDK broker fixture teardown fix onto current origin/dev, resuming PR #2265. The detached SDK broker spawned for a fixture is now reaped by targeting the exact owned child (pid + incarnation), with a bounded SIGTERM -> SIGKILL -> verified-exit reap and a single-use fixture lease, so a fixture never orphans its broker owner or leaks its root. Core: - src/sdk/broker/ensure.ts: exact-owner reap (reapSpawnedBroker), pid+incarnation owner fencing, single fixture lease. - src/sdk/broker/discovery.ts: broker process incarnation; temp broker.json fsync is fail-closed (syncFile) while only the directory barrier (syncDirectory) tolerates win32 EPERM/EACCES. - src/sdk/bus/index.ts: retained terminal-cleanup proof; cleanupRetries fences same-id startup so a replacement never shadows an unreaped owner. - src/sdk/bus/native-runtime-compatibility.ts + crates (gjc-sdk server, pi-natives): bounded stop_and_wait/stopAndWait joining half-open and established connections idempotently. - ai/model-cache.ts: close the shared cache only when it owns the exact db path. bridge-client: idempotent close() that always issues socket.close() under a dedicated close grace (never gated on an expired request deadline), so a transport is never leaked on close. - test/helpers/fixture-broker-cleanup.ts (new): root-wide phased fixture teardown wired into sdk-production-host and sdk-lifecycle-fixture. - scripts/build-sdk-package-smoke.ts: pack the matching local @gajae-code/tui so the SDK package smoke exercises the workspace release boundary instead of falling back to an older registry tui missing the SecretInput export (fixes bridge-client-sdk-package-smoke; pre-existing on dev). Exact-head hostile-review repairs: - bridge-client close() no longer throws on an elapsed operation deadline before socket.close(); dedicated close grace + leak regression test. - Regression tests assert temp broker.json fsync stays fail-closed (even for EPERM) and the directory-barrier tolerance is win32-scoped only. - Confirmed cleanupRetries already fences same-id startup (owner-release proof preserved across retry). - Restored the SDK package-smoke TUI-pack fix after CI proved the smoke regresses without it. Supersession audit vs origin/dev (dropped from the original PR): - workflow-gate-broker.ts + test: Bun Windows fsync EPERM now via #2309. - .github/workflows/dev-ci.yml + dev-ci-notification-atomicity CI hunk: CI topology churn coupled to the dropped workflow. - sdk-public-surface manifest + CHANGELOG entries: unrelated churn. - #2292 is already in this fix's base (built upon); #2305 (Discord) touches no file in this set. Repro (#2246): sandboxed operator-safe harness (owned pid only) shows the detached fixture broker alive + root present before teardown and reaped + root absent after fixtureRootCleanup.
216a8c6 to
3690da4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3690da430e
ℹ️ 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".
| "TEMP", | ||
| "TMP", | ||
| "TMPDIR", |
There was a problem hiding this comment.
When fixture children run code that uses os.tmpdir() (for example task/worktree temporary files or SDK snapshot spills), these allowlisted runner TEMP/TMP/TMPDIR values keep temp writes under the real runner temp directory instead of the fixture root that cleanupFixtureRoot removes. That defeats the exact-owner cleanup contract and can leave files outside the preserved/removed root; set the temp variables to a root-owned temp directory rather than forwarding the host values.
Useful? React with 👍 / 👎.
Reconstructs the minimal still-needed exact-owner SDK broker fixture teardown fix onto current origin/dev, resuming PR #2265. The detached SDK broker spawned for a fixture is now reaped by targeting the exact owned child (pid + incarnation), with a bounded SIGTERM -> SIGKILL -> verified-exit reap and a single-use fixture lease, so a fixture never orphans its broker owner or leaks its root. Core: - src/sdk/broker/ensure.ts: exact-owner reap (reapSpawnedBroker), pid+incarnation owner fencing, single fixture lease. - src/sdk/broker/discovery.ts: broker process incarnation; temp broker.json fsync is fail-closed (syncFile) while only the directory barrier (syncDirectory) tolerates win32 EPERM/EACCES. - src/sdk/bus/index.ts: retained terminal-cleanup proof; cleanupRetries fences same-id startup so a replacement never shadows an unreaped owner. - src/sdk/bus/native-runtime-compatibility.ts + crates (gjc-sdk server, pi-natives): bounded stop_and_wait/stopAndWait joining half-open and established connections idempotently. - ai/model-cache.ts: close the shared cache only when it owns the exact db path. bridge-client: idempotent close() that always issues socket.close() under a dedicated close grace (never gated on an expired request deadline), so a transport is never leaked on close. - test/helpers/fixture-broker-cleanup.ts (new): root-wide phased fixture teardown wired into sdk-production-host and sdk-lifecycle-fixture; child temp dirs (TMPDIR/TMP/TEMP) are pinned inside the owned root so os.tmpdir() writes never escape or survive fixture cleanup. - scripts/build-sdk-package-smoke.ts: pack the matching local @gajae-code/tui so the SDK package smoke exercises the workspace release boundary instead of an older registry tui missing the SecretInput export (fixes bridge-client-sdk-package-smoke; pre-existing on dev). Exact-head hostile-review repairs: - bridge-client close() no longer throws on an elapsed operation deadline before socket.close(); dedicated close grace + leak regression test. - Regression tests assert temp broker.json fsync stays fail-closed (even for EPERM) and the directory-barrier tolerance is win32-scoped only. - sdk-production-host suppresses the session's auto-added SDK host during construction (GJC_SDK_DISABLE=1 scoped to createAgentSession) so ONLY the fixture's explicit instrumented notifications extension hosts; regression asserts exactly one server + one endpoint (no duplicate auto-host). - Fixture temp vars pinned to a root-owned dir with a no-external-residue regression. - Confirmed cleanupRetries already fences same-id startup; restored the SDK package-smoke TUI-pack + deterministic sdk-public-surface manifest. Supersession audit vs origin/dev (dropped from the original PR): - workflow-gate-broker.ts + test: Bun Windows fsync EPERM now via #2309. - .github/workflows/dev-ci.yml + dev-ci-notification-atomicity CI hunk: CI topology churn coupled to the dropped workflow. - CHANGELOG entries: unrelated churn. - #2292 is already in this fix's base (built upon); #2305 (Discord) touches no file in this set. Repro (#2246): sandboxed operator-safe harness (owned pid only) shows the detached fixture broker alive + root present before teardown and reaped + root absent after fixtureRootCleanup.
3690da4 to
80682e8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80682e8338
ℹ️ 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".
| // endpoint and onSdkRequest instrumentation is never overwritten. | ||
| const priorSdkDisable = process.env.GJC_SDK_DISABLE; | ||
| process.env.GJC_SDK_DISABLE = "1"; | ||
| let session: Awaited<ReturnType<typeof createAgentSession>>["session"]; |
There was a problem hiding this comment.
The repo-level AGENTS.md Code quality section says, "Never use ReturnType<>; write the actual type name." This newly added helper type uses ReturnType<typeof createAgentSession> for the session object, so it violates the documented contributor contract and spreads a banned type idiom; please use the explicit session type instead.
Useful? React with 👍 / 👎.
1 participant
Summary
Rejected designs
Verification
bun run --cwd packages/coding-agent check: passbun run build: passfc490531efad20a49f254761fac837d6ccaf793eCloses #2246.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]