Merged
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughDocumentation for the start_directory schema was changed to state the default is "/app". Code in the deployment launcher, build service, and exec proxy now sets StartDirectory to "/app" when the configuration provides an empty value. A unit test was renamed and its assertion updated to expect the "/app" default. No exported signatures were modified. Comment |
The WORKDIR fix (f24f27d) removed the runtime /app default for StartDirectory from the deployment launcher, exec proxy, and build server. That works great for freshly deployed apps, but existing app versions built before that change have no start_directory stored in their config. Without the runtime default to fall back on, those apps boot with CWD=/ and immediately crash. Restore the /app default in all three places so existing apps keep working. The sandbox controller's conditional WithProcessCwd is left alone — it's correct behavior. A follow-up boot migration can backfill start_directory on existing configs, after which the launcher and exec proxy defaults become removable.
phinze
force-pushed
the
phinze/restore-app-default-start-directory
branch
from
dfbae3d to
a87c05f
Compare
Contributor
Author
|
Merging this right away to get this fix out there. |
phinze
added a commit
that referenced
this pull request
Feb 18, 2026
The previous PR (#621) restored /app defaults in the launcher and build server, but those only take effect when creating new pool specs. Existing pools persisted in etcd still have no directory field in their sandbox spec, so the sandbox controller skips WithProcessCwd entirely and the container boots with CWD=/. Add the /app default here in the sandbox controller as a safety net. When co.Directory is populated (new pools, WORKDIR apps), it's used as-is. When it's empty (pre-fix pools), we fall back to /app.
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
1 participant
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.
The WORKDIR fix (f24f27d) did the right thing for new deploys — detect the image's WORKDIR at build time and store it in the config. But it also removed the runtime
/appfallback from the deployment launcher, exec proxy, and build server. That's a problem for every existing app version that was built before the fix: they have nostart_directoryin their config at all, so they end up booting with CWD=/instead of/appand immediately crash.This restores the
/appdefault in all three places. The build server defaults new builds to/appwhen no WORKDIR is detected, and the launcher and exec proxy fall back to/appwhen the stored config is empty. The sandbox controller's conditionalWithProcessCwdis left as-is since it's the correct behavior.The launcher and exec proxy defaults are backward-compat scaffolding — once we run a boot migration to backfill
start_directoryon existing configs, those can be cleaned up.Fixes MIR-727