Fix spawn_on_env vars leaking from server to app #749
Merged
+16
−2
Conversation
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
| app.env.delete "VAR_FROM_BOOT" | ||
|
|
||
| # Verify the variable is properly cleared (not inherited from server) | ||
| assert_success %(bin/rails runner 'p Rails.configuration.x.var_from_boot.inspect'), stdout: "nil" |
Author
There was a problem hiding this comment.
Without the fix, this line fails:
Expected /nil/ to match "\"\\\"before\\\"\"\n".
Previously, variables specified by `spawn_on_env` would behave inconsistently when the Spring server was created with one of those variables set: ``` VAR_FROM_BOOT=before bin/rails ... ``` would cause future clients' apps to be booted with `VAR_FROM_BOOT=before` set, even if the client is created without that variable: ``` bin/rails ... ``` and additionally, Spring would hide that the app was _booted_ with that var set by _unsetting_ it when the client attached to its app (making it look like the correct ENV). The issue was caused by how `spawn_on_env` was passed from client to server to app: - The client would slice `spawn_on_env` keys from its `ENV`, meaning `bin/rails ...` would have an empty `spawn_on_env` hash. - The server (which has `VAR_FROM_BOOT` set), forks an app with `spawn_on_env` values set (none, because its an empty hash) - The forked app inherits its parent's `ENV`, so it boots with `VAR_FROM_BOOT` set - The client gets a forked app process, and cleans up environment variables from the server process This is specifically an issue for `spawn_on_env` because the goal is to have apps booted with different environments. This commit fixes the issue by always passing all `Spring.spawn_on_env` values from the client to the server (including `nil` ones) so that the forked app will have those values cleared if necessary. `compact` is added to the `SPRING_SPAWN_ENV` value because it is only used to display the `spawn_on_env` values in `spring status` and showing all of the `nil` values is noisy/unnecessary.
hmcguire-shopify
force-pushed
the
hm-ovztstypytzzqmqr
branch
from
8c6a2c7 to
694deb9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Previously, variables specified by
spawn_on_envwould behave inconsistently when the Spring server was created with one of those variables set:would cause future clients' apps to be booted with
VAR_FROM_BOOT=beforeset, even if the client is created without that variable:and additionally, Spring would hide that the app was booted with that var set by unsetting it when the client attached to its app (making it look like the correct ENV).
The issue was caused by how
spawn_on_envwas passed from client to server to app:spawn_on_envkeys from itsENV, meaningbin/rails ...would have an emptyspawn_on_envhash.VAR_FROM_BOOTset), forks an app withspawn_on_envvalues set (none, because its an empty hash)ENV, so it boots withVAR_FROM_BOOTsetThis is specifically an issue for
spawn_on_envbecause the goal is to have apps booted with different environments.This commit fixes the issue by always passing all
Spring.spawn_on_envvalues from the client to the server (includingnilones) so that the forked app will have those values cleared if necessary.compactis added to theSPRING_SPAWN_ENVvalue because it is only used to display thespawn_on_envvalues inspring statusand showing all of thenilvalues is noisy/unnecessary.