Skip to content

🤖 perf: cut SSH stream startup remote I/O - #3805

Open
ammar-agent wants to merge 1 commit into
mainfrom
stream-startup-c4c0
Open

🤖 perf: cut SSH stream startup remote I/O#3805
ammar-agent wants to merge 1 commit into
mainfrom
stream-startup-c4c0

Conversation

@ammar-agent

@ammar-agent ammar-agent commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Cut SSH stream-startup latency by eliminating duplicate AGENTS.md scans and overlapping independent pre-stream context work. Net LoC is negative versus main.

Background

Slow stream starts on SSH showed most time in buildStreamSystemContextMs, with a second AGENTS.md pass for tool instructions. Each remote probe is ~80–100ms, so sequential context assembly added multi-second delay before the provider stream started.

Implementation

  • Load agent body, subagent discovery, skills discovery, and instruction sources concurrently in buildStreamSystemContext.
  • Return instructionSources and reuse them via toolInstructionsFromSources (no second remote AGENTS.md scan).
  • Parallelize instruction-set base/mux probes and global+context instruction loads.
  • Overlap post-context MCP/temp-dir/session-usage setup in aiService.

Intentionally not included after review thrash: full-lifetime remote exec session semaphores / SSH2 connection accounting (risked starving long-lived MCP servers and interactive PTYs).

Validation

  • bun test on stream context / system message / instruction files / aiService / agent defs / skills
  • make typecheck
  • eslint on touched files

Risks

  • Low/medium: concurrent discovery/instruction loading must preserve existing precedence. Covered by existing unit tests; behavior is parallelization + reuse, not policy changes.

Generated with mux • Model: xai:grok-4.5 • Thinking: high • Cost: $3.88

@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa99f9e64c

ℹ️ 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".

Comment thread src/node/services/agentSkills/agentSkillsService.ts Outdated
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Addressed the P2 by capping remote agent/skill descriptor reads with a sliding-window concurrency helper (mapWithConcurrency, limit 8). Local FS discovery remains unbounded parallel.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2ee14f5ba

ℹ️ 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".

Comment thread src/node/services/streamContextBuilder.ts
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Addressed the shared-cap concern by adding a per-RemoteRuntime exec concurrency semaphore (limit 8) held for the full session lifetime (spawn → exit). That bounds total SSH/Docker channels opened by concurrent stream-startup discovery, instruction probes, and path resolution, independent of higher-level Promise.all fan-out. The earlier per-root discovery helpers remain as soft structuring on top of this hard transport-level limit.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 505db0d947

ℹ️ 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".

Comment thread src/node/runtime/RemoteRuntime.ts Outdated
Comment thread src/node/runtime/RemoteRuntime.ts Outdated
Comment thread src/node/utils/main/instructionFiles.ts Outdated
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Addressed the three P2s:

  1. Shared connection-keyed capgetRemoteExecConnectionKey() on SSH runtimes keys the limiter by host/port/identity (shared across pooled workspaces). Docker/devcontainer stay instance-scoped via WeakMap keys.
  2. Abortable/deadline-aware acquireAsyncSemaphore.acquire now accepts abortSignal + deadlineMs; RemoteRuntime.exec starts the timeout clock before queue wait and rechecks abort after acquiring.
  3. Instruction probe then read — base-file priority now parallel-probes existence (stat/fileExists) and fully reads only the highest-priority existing candidate, so large lower-priority AGENT.md/CLAUDE.md cannot stall startup.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57019cdb15

ℹ️ 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".

Comment thread src/node/services/agentSkills/agentSkillsService.ts Outdated
Comment thread src/node/runtime/remoteExecConcurrency.ts Outdated
Comment thread src/node/utils/main/instructionFiles.ts Outdated
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Addressed the latest three P2s:

  1. Skill precedence short-circuit — list roots concurrently, then read descriptors in scan order and skip already-won names before opening SKILL.md.
  2. Weak instance limiters — Docker/devcontainer use WeakMap<object, AsyncSemaphore> via getRemoteExecConcurrencyLimiterForRuntime; only long-lived SSH connection keys stay in a process Map.
  3. Instruction candidate fallback — if the probed winner fails to read, continue to the next existing candidate instead of returning empty.

@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Rebased onto latest main and resolved the aiService.ts conflict by keeping the parallel startup path while using capabilityModelString for tool-instruction extraction.

@ammar-agent
ammar-agent force-pushed the stream-startup-c4c0 branch from 5199d7c to 06ebb80 Compare August 5, 2026 22:25

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06ebb809f0

ℹ️ 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".

Comment thread src/node/runtime/RemoteRuntime.ts Outdated
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Addressed the remaining-deadline concern: SSH2Transport.spawnRemoteProcess now uses deadlineMs - now for connection acquisition (same pattern as OpenSSH), so a nearly exhausted timeout after semaphore queue wait cannot spend another full connect timeout.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 24b4f0d9fb

ℹ️ 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".

Comment thread src/node/runtime/transports/SSH2Transport.ts
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Addressed: SSH2 connection acquisition now passes the remaining deadline as both timeoutMs and maxWaitMs, matching OpenSSHTransport, so pool backoff/retries cannot exceed the leftover command budget.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 442b36e785

ℹ️ 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".

Comment thread src/node/runtime/transports/SSH2Transport.ts
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Addressed: SSH2ConnectionPool.waitForAbortable is now deadline-aware. Callers joining an existing singleflight connect wait only up to their remaining maxWaitMs, so a short-budget stream cannot hang on a longer peer connection attempt.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd23e8a33d

ℹ️ 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".

Comment thread src/node/runtime/RemoteRuntime.ts Outdated
Comment thread src/node/runtime/SSHRuntime.ts Outdated
@ammar-agent
ammar-agent force-pushed the stream-startup-c4c0 branch from bd23e8a to 49cb541 Compare August 5, 2026 22:55
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Simplified the PR to the categorical startup wins only (parallel context loads + AGENTS.md reuse + overlapped post-context setup). Removed the contested full-lifetime remote session semaphore / SSH2 accounting layers after they ballooned scope and risked starving MCP/PTY sessions. Net LoC is negative vs main.

@ammar-agent

Copy link
Copy Markdown
Collaborator Author

Resolved the two leftover threads about RemoteRuntime exec-session budgeting / PTY accounting as no longer applicable: that full-lifetime session-limiter approach was removed from this PR after review thrash. The PR now only overlaps independent stream-context loads and reuses AGENTS.md sources for tool instructions.

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 49cb541cc6

ℹ️ 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".

Reuse AGENTS.md sources for tool instructions and parallelize independent
stream-context/instruction loads so pre-stream preparation is not serialized
across remote round-trips.
@ammar-agent

Copy link
Copy Markdown
Collaborator Author

@codex review

Rebased onto latest main and kept the PR as a single net-negative LoC commit focused on overlapping stream-context loads + AGENTS.md reuse.

@ammar-agent
ammar-agent force-pushed the stream-startup-c4c0 branch from f2b0e9f to 8115ac4 Compare August 5, 2026 23:14
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 8115ac4edb

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant