Bind standalone Airflow to loopback by default and stop the whole process group - #2222
Open
jlaneve wants to merge 2 commits into
Open
Bind standalone Airflow to loopback by default and stop the whole process group#2222jlaneve wants to merge 2 commits into
jlaneve wants to merge 2 commits into
Conversation
Standalone mode sets SIMPLE_AUTH_MANAGER_ALL_ADMINS=True while Airflow's api-server/webserver default to binding 0.0.0.0, so a local dev instance was reachable from other machines on the network. BuildEnv now defaults AIRFLOW__API__HOST (Airflow 3) and AIRFLOW__WEBSERVER__WEB_SERVER_HOST (Airflow 2) to 127.0.0.1. A user's own setting, from .env or the inherited environment, still wins; only the default changed. StopProcess polled the master PID, which often exits on SIGTERM before its scheduler/api-server/triggerer children, so those children survived astro dev stop. It now polls the process group and only returns once the whole group is gone, escalating to SIGKILL on timeout as before. The signal helpers are now vars so tests can stub them; the stop path is covered without real processes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P6DEdUpFBFaAiPKEu81Wti
Review found that airflowrt.BuildEnv and airflowrt.StopProcess have no production callers: astro dev start/stop run (*Standalone).buildEnv and (*Standalone).Stop in airflow/standalone.go, which still bound 0.0.0.0 and still polled only the master PID. This wires the fixes into those paths. airflowrt now exports ApplyLoopbackDefaults, which both its own BuildEnv and standalone.go's buildEnv call, so the two host keys are defined once. It also exports ProcessGroupAlive; standalone.go's Stop polls it instead of re-reading the master PID. The master is the group leader (Start sets Setpgid), so kill(-pid, 0) is a valid group probe. Live-path tests: buildEnv loopback defaults plus .env and inherited-env overrides, and a Stop test that starts a real process group whose master dies on SIGTERM while a TERM-ignoring child lives on — Stop must outwait the child. Also trims repeated comments in the airflowrt seam files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P6DEdUpFBFaAiPKEu81Wti
Coverage Report for CI Build 29872953320Coverage increased (+0.01%) to 43.894%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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.
What changed
Loopback binding. Standalone mode runs Airflow with
SIMPLE_AUTH_MANAGER_ALL_ADMINS=True, and Airflow's api-server (Airflow 3) and webserver (Airflow 2) default to binding0.0.0.0. That combination made a local dev instance reachable from other machines on the network. A newairflowrt.ApplyLoopbackDefaultsdefaultsAIRFLOW__API__HOSTandAIRFLOW__WEBSERVER__WEB_SERVER_HOSTto127.0.0.1; bothairflowrt.BuildEnvand the live path,(*Standalone).buildEnvinairflow/standalone.go, call it. Only the default changed: a user's own setting, from the.envfile or the inherited environment, still wins. Airflow ignores whichever key doesn't apply to the running major.Stop the whole process group. Both
airflowrt.StopProcessand the live path,(*Standalone).Stop, polled the master PID, which often exits on SIGTERM before its scheduler/api-server/triggerer children, so those children outliveastro dev stop. Both now poll the process group (newairflowrt.ProcessGroupAlive) and return only once the whole group is gone, still escalating to SIGKILL on timeout. The master is the group leader —Startlaunches it withSetpgid— sokill(-pid, 0)is a valid group probe. On Windows, where Unix process groups don't apply, the airflowrt fallback checks the single PID — children a dead master leaves behind can still linger there. (v1's standalone mode is a stub on Windows, so its live path is unaffected.)Review found that
airflowrt.BuildEnvandairflowrt.StopProcesshave no production callers in this repo —astro devruns thestandalone.gopaths — so the fix lands in both places, keeping the shared module and the live path in agreement.Tests
.envand from the inherited environment win — asserted for bothairflowrt.BuildEnvand(*Standalone).buildEnv.airflowrt.StopProcesscovered without real processes: the signal helpers are package vars the tests stub, so the graceful-wait and force-kill paths run in milliseconds.(*Standalone).Stopcovered with a real process group whose master dies on SIGTERM while a TERM-ignoring child lives on ~2s; Stop must outwait the child and leave the group fully dead.go build/go vet/go testinpkg/airflowrt(including aGOOS=windowscross-compile),go build ./...andgo test ./airflow/...at the root,golangci-linton both (no new findings).Breaking changes
None. Defaults changed; explicit user overrides still win. The airflowrt additions (
ApplyLoopbackDefaults,ProcessGroupAlive) are new exports, no signatures changed — the module stays backward compatible.🤖 Generated with Claude Code
https://claude.ai/code/session_01P6DEdUpFBFaAiPKEu81Wti