Repository navigation
perf: the setup pass runs its stages in subshells, not a bash each - #678
Merged
Merged
Conversation
Every new non-interactive bash reads $BASH_ENV, and kinisi_ros's image points it at its whole ROS bashrc: about 1.1s a read inside a devpod ssh session. A cold launch's setup trip started one bash per stage on top of its own, so the trip took about 11s for 6.7s of work. The stages now run as `( eval '<script>' )`. eval keeps what the nested bash gave each stage: the script parses when the stage runs, so a parse error fails its own stage only, and exit, set -u and set -C end at the closing parenthesis. The stage inherits the environment the pass already loaded, which is the one it was reading anyway. Measured over six cold kinisi_ros launches on fresh branches, alternating builds: the setup trip went from 10.90s-11.14s to 6.66s-6.90s.
Reviewer's GuideThe PR changes setup-stage execution from one Sequence diagram for the optimized setup passsequenceDiagram
participant Devpod
participant PassShell
participant Stage
participant BashEnv
Devpod->>PassShell: devpod ssh --command setup script
PassShell->>BashEnv: Read BASH_ENV once
loop Each setup stage
PassShell->>Stage: ( eval '<stage script>' )
Stage-->>PassShell: Stage outcome
end
PassShell-->>Devpod: Probe results
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="rust/devlaunch-core/src/flows/provision.rs" line_range="1910" />
<code_context>
+/// parenthesis. `the_pass_reads_the_images_bash_env_once_however_many_stages_it_runs`
+/// pins the count.
+fn in_subshell(script: &str) -> String {
+ format!("( eval {} )", quote(script))
+}
+
</code_context>
<issue_to_address>
**Unexported config path gets seeded**
When a login profile assigns `CLAUDE_CONFIG_DIR` without exporting it, the subshell inherits the login shell’s non-exported `CLAUDE_CONFIG_DIR`, so `onboarding_script` and `trust_script` write to that directory. Claude Code runs as a child process and does not inherit the unexported variable, so it reads its default directory instead; the user sees onboarding or trust prompts despite the stages reporting success.
Make the stages use only the exported `CLAUDE_CONFIG_DIR` value, matching the environment Claude Code receives.
Also at `rust/devlaunch-core/src/flows/provision.rs:2081`, `rust/devlaunch-core/src/flows/provision.rs:2092`.
</issue_to_address>| /// parenthesis. `the_pass_reads_the_images_bash_env_once_however_many_stages_it_runs` | ||
| /// pins the count. | ||
| fn in_subshell(script: &str) -> String { | ||
| format!("( eval {} )", quote(script)) |
There was a problem hiding this comment.
🟡 Medium · Unexported config path gets seeded
When a login profile assigns CLAUDE_CONFIG_DIR without exporting it, the subshell inherits the login shell’s non-exported CLAUDE_CONFIG_DIR, so onboarding_script and trust_script write to that directory. Claude Code runs as a child process and does not inherit the unexported variable, so it reads its default directory instead; the user sees onboarding or trust prompts despite the stages reporting success.
Make the stages use only the exported CLAUDE_CONFIG_DIR value, matching the environment Claude Code receives.
Also at rust/devlaunch-core/src/flows/provision.rs:2081, rust/devlaunch-core/src/flows/provision.rs:2092.
Prompt for AI agents
In `rust/devlaunch-core/src/flows/provision.rs` at line 1910:
**Unexported config path gets seeded**
When a login profile assigns `CLAUDE_CONFIG_DIR` without exporting it, the subshell inherits the login shell’s non-exported `CLAUDE_CONFIG_DIR`, so `onboarding_script` and `trust_script` write to that directory. Claude Code runs as a child process and does not inherit the unexported variable, so it reads its default directory instead; the user sees onboarding or trust prompts despite the stages reporting success.
Make the stages use only the exported `CLAUDE_CONFIG_DIR` value, matching the environment Claude Code receives.
Also at `rust/devlaunch-core/src/flows/provision.rs:2081`, `rust/devlaunch-core/src/flows/provision.rs:2092`.
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
# Conflicts: # CHANGELOG.md
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A cold launch of a large ROS repository on a new branch takes about 100s. This PR removes about 4.2s of that. The 4.2s is in the part that dl owns: the setup trip.
What this changes
bash. The new helperin_subshellinrust/devlaunch-core/src/flows/provision.rswrites a stage as( eval '<script>' ). Before, it wrotebash -c '<script>'. Six stages use it: session-name, zellij, codex, title, onboarding and trust.$BASH_ENV. That image setsBASH_ENVto the user's~/.bashrcinside adevpod sshsession, and that file loads the full ROS environment. One read costs about 1.1s. A default cold launch ran four stages in a new bash, so it read the file five times. Now it reads the file one time.evalkeeps the isolation that the nested bash gave each stage:exit, aset -uor aset -Cstops at the closing parenthesis.the_pass_reads_the_images_bash_env_once_however_many_stages_it_runs(core).rust/dl/tests/setup_pass_shells.rs(thedlbinary, a cold launch through adevpodthat runsssh --commandfor real).a_stage_that_will_not_parse_fails_alone_and_the_pass_goes_on.a_stage_that_sets_u_and_c_keeps_them_to_itself.a_stage_that_calls_exit_ends_only_itself.the_setup_pass_records_trust_for_the_workspace_it_stands_in. This test runs the trust stage's Python merge through the real pass.( eval … ).docs/performance.mdhas a new section "One shell for the setup pass".CHANGELOG.mdhas an entry.Where this fits
The script that the setup trip sends, for one stage:
How I checked it
How I found it. I timed four cold launches of that repository with
DEVLAUNCH_TIMING=json(95s to 127s). Most of the time is indevpod up: the image's on_create script (about 23s) and the dotfiles step (about 34s). dl does not own those. In the part that dl owns, the setup trip took 10.85s to 10.91s every time. Adevpod ssh <ws> --command truetrip takes 4.4s. Inside the container:bash -c truetakes 0.49s withdocker exec, and 1.07s inside adevpod sshsession.bash -lc truetakes 1.05s withdocker exec, and 2.2s inside adevpod sshsession.BASH_ENV, set to the user's~/.bashrc, in the session environment.Prototype. I replayed the exact setup command that dl sent, on the same container, with a shim that changes
bash -cto( eval … ). The output was the same, byte for byte. The trip time went from 10.95s, 11.03s and 11.41s to 6.56s, 6.76s and 6.78s.Before and after, end to end. I ran six cold launches. Each launch used a new branch. The runs alternated between the released
dl 0.59.4(the same commit as this PR's base) and a release build of this branch:The lend trip and the attach trip changed a lot from run to run (6.5s to 14.0s), so the totals are noisy. The setup trip is not noisy. In a container that this PR's build made, the hostname, the session name, the title and the PATH line were all in place.
Tests.
BASH_ENV was read 5 times. It passes now.the setup pass read BASH_ENV 5 timeswhen I putbash -cback inin_subshell. It passes with the fix.eval, the script pasted withouteval, andbash -c.cargo test --workspace --no-fail-fast,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --check,prekon the changed files, and the doc guards (test_docs_prose.py,test_bench_doc.py,test_citations_resolve.py).origin/main, so this PR does not cause them. All five are about commits pushed by URL, or a remote that cannot be reached. Four are inflows::lifecycle, and one is inrust/dl/tests/lifecycle.rs:a_host_that_prunes_on_fetch_does_not_turn_a_merged_branch_into_unpushed_worka_remote_that_cannot_be_reached_leaves_the_refusal_standinga_site_pushed_by_url_does_not_stop_a_plain_rmcommits_pushed_by_url_do_not_stop_a_plain_rma_commit_pushed_by_url_does_not_stop_rmI did not test an image with no
$BASH_ENV. There, the saving is the cost of a bash start, which is small. A warm launch makes no setup trip, so this PR does not change it.Merge danger
Door: two-way. A plain revert puts
bash -cback.Blast radius: small. If the change is wrong, a cold launch can set the hostname, title, session name or Claude onboarding incorrectly in a new container.
dl <ws> recreateruns the pass again. The stage outcomes stay in the same probe protocol, so a stage that fails still says so.Review notes
set -unow applies to each stage, and a readonly profile variable fails a stage that assigns it. No current stage breaks: each stage assigns a variable before it reads it, or reads it as${X:-}. For example, a profilezellij()wrapper with nozellijbinary makescommand -v zellijsucceed, and the zellij stage then skips its install. The review judged this unlikely. Thein_subshelldoc comment records these rules. Functions from$BASH_ENVreached the stages before this change too.pixi global sync(17.5s), and the dotfiles bootstrap runspixi global syncagain (13.5s) pluschezmoi apply(14s). Bothpixi global syncruns report "unchanged".🤖 Generated with Claude Code
Summary by Sourcery
Reduce cold-launch overhead by running setup stages in isolated subshells instead of starting a new Bash process for each stage.
Enhancements:
Documentation:
Tests:
Chores: