Conversation
|
The created documentation from the pull request is available at: docu-html |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Lifecycle “conditional launching” feature-integration test scenario and a Python FIT suite that runs it against both the Rust and C++ scenario runners. It also introduces a Rust “lifecycle-only” Bazel binary variant intended to avoid pulling in persistency-related dependencies for this specific suite, and updates test harness/configuration to support selective scenario builds.
Changes:
- Added Lifecycle conditional-launching scenarios to the Rust and C++ FIT scenario trees.
- Added a Python requirements-based FIT suite for conditional launching (parametrized for Rust/C++) plus a shared lifecycle base scenario fixture.
- Added a Rust lifecycle-only Bazel scenario binary and updated FIT build selection/docs.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updates pytest configuration section to tool.pytest.ini_options. |
| feature_integration_tests/test_scenarios/rust/src/scenarios/mod.rs | Registers the new lifecycle scenario group in the Rust scenario tree. |
| feature_integration_tests/test_scenarios/rust/src/scenarios/lifecycle/mod.rs | Adds a lifecycle scenario group (Rust). |
| feature_integration_tests/test_scenarios/rust/src/scenarios/lifecycle/conditional_launching.rs | Implements the Rust conditional-launching scenario input validation/logging. |
| feature_integration_tests/test_scenarios/rust/src/main.rs | Adds lifecycle_only compilation mode and a reduced root group for lifecycle-only builds. |
| feature_integration_tests/test_scenarios/rust/BUILD | Adds rust_lifecycle_test_scenarios Bazel target using --cfg=lifecycle_only. |
| feature_integration_tests/test_scenarios/cpp/src/scenarios/mod.cpp | Registers a lifecycle scenario group (C++). |
| feature_integration_tests/test_scenarios/cpp/src/scenarios/lifecycle/conditional_launching.h | Declares the C++ conditional-launching scenario factory. |
| feature_integration_tests/test_scenarios/cpp/src/scenarios/lifecycle/conditional_launching.cpp | Implements the C++ conditional-launching scenario parsing/logging. |
| feature_integration_tests/test_cases/tests/lifecycle/test_conditional_launching.py | Adds Python FIT assertions for lifecycle conditional launching (Rust/C++). |
| feature_integration_tests/test_cases/lifecycle_scenario.py | Introduces a shared LifecycleScenario base class with a common temp_dir fixture. |
| feature_integration_tests/test_cases/conftest.py | Skips building unselected scenario variants based on the pytest mark expression. |
| feature_integration_tests/test_cases/BUILD | Adds lifecycle_scenario.py to FIT runfiles. |
| feature_integration_tests/README.md | Documents running lifecycle conditional-launching FITs and the lifecycle-only Rust target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Saumya-R
changed the title
816b818 to
1c7d861
Compare
PiotrKorkus
left a comment
PiotrKorkus
left a comment
There was a problem hiding this comment.
also remove duplicated dependency/ordering tests across daemon files
7bc8cd9 to
d1cddfa
Compare
3d81256 to
37ddbd4
Compare
c3c1d5e to
6b43eee
Compare
616c7ec to
e720430
Compare
fac28cc to
5f17da5
Compare
Copyright fixes added test case to reject unknown conditions fixed the readme added launch manager processes adding additional test cases for lifecycle removing the changes in pyproject and requirements txt added review comment fixes: - cleared the non-required. - removed the dead LIFECYCLE_TESTS_SUMMARY.md doc reference. - dropped the class-level 13-req partially_verifies blanket claim; each requirement is now tagged on the specific test that actually exercises it (add_test_properties moved onto individual methods) - deleted test_config_defines_startup_retry_policy Adding review comments added the following fixes : - The scenario now actually polls and checks each condition instead of printing it - Replaced plain std::cout text with the same structured JSON log shape the Rust tracing subscriber emits - TestConditionalLaunchingScenario now creates a real flag file, sets a real env var, and spawns a real sleep process - Added TestConditionalLaunchingScenarioTimesOutOnUnmetConditions, a negative test where none of the conditions are ever satisfied - removed test_startup_launches_supervised_apps and test_dependency_gates_rust_startup from test_process_launching_with_daemon.py - TestConditionalLaunchingBlocksOnMissingDependency spins up its own launch_manager with cpp withheld to prove real gating added TestConditionalLaunchingScenarioRejectsUnsupportedPrefix Added timing assertion to TestConditionalLaunchingScenarioRejectsUnsupportedPrefix changed the decorators removed duplicate parametrization added patches/lifecycle/001-forward-visibility-to-config-combiner.patch updated known_goods.json for patch review comments addressed: - Split lifecycle tests into a standalone fit_lifecycle_daemon bazel target with no longer routed by language marker - strengthened test to assert the other app's pid is untouched, proving retry recovery - Added signal_process() helper - Retagged to launch_support updated read me added the issue for persistency build added real retry exhaustion test case added a fix for fit_cpp_orch , added test cases for parallel launching scorebug issue resolved removed the manual marker assuming CI always has resources removed the organizational marker daemon and marker manual as watchdog detection should be part of every CI added daemon invocation review comment address resolving the merge issue
5f17da5 to
0cdb69f
Compare
| time.sleep(1.0) | ||
| if not daemon.is_running(): | ||
| logs = daemon.get_logs() | ||
| pytest.skip(f"launch_manager failed to start in this environment. Logs:\n{logs}") |
There was a problem hiding this comment.
A launch_manager that dies on startup is the regression this suite exists to catch, and pytest.skip makes it indistinguishable from a missing prerequisite. When it triggers, every test in fit_lifecycle_daemon and fit_lifecycle_retries skips and both Bazel targets report PASS; nothing asserts a nonzero collected count. _wait_for_apps a few lines down (:473) already does this the other way, with pytest.fail.
The binary is a declared Bazel data dep, so there is no "unavailable in this environment" case to guard. Suggest pytest.fail with the collected logs here and at :564. If an environment gate is genuinely needed, match a specific enumerated condition rather than "the daemon died".
| @@ -0,0 +1,10 @@ | |||
| diff --git a/scripts/config_mapping/config.bzl b/scripts/config_mapping/config.bzl | |||
There was a problem hiding this comment.
This patch is never registered, so it cannot apply. grep -rn "patches/lifecycle\|forward-visibility" over the branch returns no hits outside this file, and no MODULE.bazel file is in the diff.
The repo applies module patches through patch_strip + patches on the module's git_override. bazel_common/score_modules_target_sw.MODULE.bazel:71-75 gives score_lifecycle only module_name, commit and remote, while baselibs, communication, persistency and kyron (:60-69) all carry patch lists. Since the patch forwards visibility to the config-combiner rule, and the new fit_lifecycle_daemon/fit_lifecycle_retries targets put exactly those targets in data, the dependency appears to rely on it.
Either add patch_strip = 1 and patches = ["//patches/lifecycle:001-forward-visibility-to-config-combiner.patch"] to the score_lifecycle override, or drop patches/lifecycle/ if the build does not need it.
| f"Daemon logs:\n{daemon.get_logs()}\n" | ||
| f"Process snapshot:\n{process_snapshot}" | ||
| ) | ||
| except BaseException: |
There was a problem hiding this comment.
This handler removes the runtime directory but never stops the daemon spawned at :437 with start_new_session=True, so any failure after Popen leaves a detached launch_manager running with its bin_dir deleted underneath it, respawning supervised apps for the rest of the session.
It is reachable: :470 runs _run(["ps", "-eo", "pid,args"]) with check=True before daemon.stop() at :471, so a failing ps lands here; so does an interrupt during the time.sleep(1.0) at :460, since BaseException catches it. start_flaky_retry_daemon has the same shape at :565.
Suggest initialising daemon = None before the try and stopping it in the handler before _cleanup_runtime_root, plus gathering the ps snapshot with check=False so diagnostics cannot pre-empt teardown.
| def pid(self) -> int: | ||
| return self.process.pid | ||
|
|
||
| def stop(self) -> None: |
There was a problem hiding this comment.
stop() sends SIGTERM/SIGKILL to the daemon's process group, but launch_manager's children put themselves in their own groups (setpgid(0, getpid()) in ProcessLauncher::setSchedulingAndSecurity), so the signals never reach them. They inherit the stdout pipe's write end, so the collector thread stays blocked in for line in process.stdout, the join(timeout=1) at :321 expires silently, and process.stdout is never closed — the thread and the fd leak for the rest of the session.
stop_flaky_retry_daemon (:582-588) already needs an explicit pkill -f <app_path> after stop() for this reason; stop_launch_manager_daemon (:600-603) has no equivalent.
Suggest closing process.stdout once the process is confirmed dead and then joining, and giving stop_launch_manager_daemon the same explicit cleanup of the supervised apps before the runtime root is removed.
Separately, self.process.wait(timeout=5) at :319 can raise TimeoutExpired out of stop(). Both teardown helpers call stop() as their first unguarded statement, so that would skip the runtime-root cleanup and the pkill that follow it — worth moving those into a finally.
|
|
||
|
|
||
| _TARGET_ENV_MAP = { | ||
| "@score_lifecycle_health//score/launch_manager:launch_manager": "FIT_LAUNCH_MANAGER_PATH", |
There was a problem hiding this comment.
These labels use the old module name. score_lifecycle_health was renamed to score_lifecycle (bazel_common/score_modules_target_sw.MODULE.bazel:71; recorded in docs/s_core_v_1/releases/release_note_score_v0_9.rst:128), and this PR's own BUILD file uses the new name.
Under bazel test these strings are only dictionary keys, so the env vars mask it. But _resolve_target_path falls back to bazel build <label> when the env var is absent — the plain-pytest flow this PR documents in the README — and that fails on a repository that no longer exists. Affects :34-36, :50-52, :350, :352, :363, :415-419 and :517.
Either rename the labels, or drop the nested bazel build/cquery fallback and fail loudly when an env var is missing rather than re-entering Bazel from inside a Bazel test action.
| * | ||
| * @return String containing the number of seconds since the UNIX epoch. | ||
| */ | ||
| inline std::string unix_seconds_string() { |
There was a problem hiding this comment.
unix_seconds_string, log_info and format_double_python already exist in internals/persistency/kvs_build_helpers.h (:41, :64, :81), byte-identical apart from log_info here adding json_escape(target). Only json_escape is genuinely new. format_double_python also has no caller in this header — every call site in the tree resolves to kvs_build_helpers::format_double_python.
Two copies of the "emit Rust-tracing-shaped JSON" contract will drift, and have already: the persistency copy still interpolates target unescaped.
Suggest moving the three generic helpers into this header, having kvs_build_helpers.h include it and keep only the KVS-specific to_need_flag/create_kvs, so the log format is defined once for both feature areas.
| Does not wait for the app to reach Running: whether it ever does is exactly | ||
| what the calling test is checking. | ||
| """ | ||
| runtime_root = Path(tempfile.mkdtemp(prefix="lifecycle_fit_retries-", dir=_tmpdir_root())) |
There was a problem hiding this comment.
Roughly 45 lines here repeat start_launch_manager_daemon (:406-491) verbatim: the runtime-root mkdtemp, the work_dir/etc_dir/bin_dir creation, resolving and copying and chmodding launch_manager, _generate_runtime_config, the ECUCFG_ENV_VAR_ROOTFOLDER default, the whole Popen block, the nested _collect_output reader, the thread start, the time.sleep(1.0) and the startup check. The real differences are which binaries get staged and whether it waits for Running. stop_flaky_retry_daemon and stop_launch_manager_daemon are likewise the same two calls plus one pkill.
Suggest one _spawn_daemon(work_dir, etc_dir, staged_binaries, wait_for=...) that both entry points call, so the launch and teardown contract exists in one place rather than being copied again for the next lifecycle scenario.
| @@ -0,0 +1,100 @@ | |||
| { | |||
There was a problem hiding this comment.
diff -u lifecycle_daemon_config.json lifecycle_daemon_parallel_launch_config.json yields a single hunk: the removal of depends_on: ["cpp_supervised_app"] from rust_supervised_app. The other 99 lines are a copy. Seven tests assert against the base config's contents (sandbox uid/gid, process_arguments, environmental_variables) and none covers this copy, so the two can drift undetected.
The same applies to the retry pair: lifecycle_daemon_retry_recovers_config.json and ..._retry_exhausts_config.json differ only in process_arguments[1] ("2" vs "999") and a recovery_action block.
_generate_runtime_config already mutates the parsed config in Python (it rewrites bin_dir and process_arguments), so both differences could be render-time flags and two of these four files could go away.
| config_schema = _resolve_target_path( | ||
| "@score_lifecycle_health//score/launch_manager/src/daemon/src/configuration/config_schema:launch_manager.schema.json" | ||
| ) | ||
| subprocess.run( |
There was a problem hiding this comment.
capture_output=True together with check=True means the CalledProcessError carries only "Command '[...]' returned non-zero exit status N" — the schema-validation message from lifecycle_config that names the offending field is captured and then discarded. Same pattern for flatc at :370-381 and in _run at :61-69.
Since this is the step that turns the JSON configs into the daemon's runtime config, its diagnostics are what a failing test needs most.
Suggest check=False and raising a RuntimeError that includes returncode, stdout and stderr.
| import pytest | ||
| from daemon_helpers import ( | ||
| is_running, | ||
| launch_manager_daemon, |
There was a problem hiding this comment.
launch_manager_daemon is a pytest fixture defined in daemon_helpers.py, and this import exists only to register it — the name is never referenced as a symbol, and it survives lint only because F401 is in pyproject.toml:102's ignore list. test_process_launching_with_daemon.py:43 does the same, while test_retry_exhaustion.py:46,59 defines its daemons locally, so the PR ships three different fixture-provisioning styles.
The repo shares fixtures through a conftest.py or a scenario base class (persistency_scenario.py:122-150, fit_scenario.py:66-140), and this PR adds a LifecycleScenario base class without using it here.
Suggest moving the daemon fixtures into tests/lifecycle/conftest.py (added to the three lifecycle targets' data) and keeping daemon_helpers.py as fixture-free helpers.
This branch introduces lifecycle integration coverage for Launching Processes and Conditional Launching using a real launch_manager daemon setup, updates test infrastructure to reduce false negatives, and refreshes documentation/roadmap artifacts.
feat_req__lifecycle__parallel_launch_support,feat_req__lifecycle__process_launch_args,feat_req__lifecycle__uid_gid_support,feat_req__lifecycle__launch_priority_support,feat_req__lifecycle__scheduling_policy,feat_req__lifecycle__retries_configurable,feat_req__lifecycle__secpol_non_root,feat_req__lifecycle__monitor_abnormal_termTestProcessLaunchingWithDaemon)pytest.mark.manual)feat_req__lifecycle__liveliness_detection,feat_req__lifecycle__smart_watchdog_configTestHealthMonitoringWithDaemon)feat_req__lifecycle__launch_support,feat_req__lifecycle__waitfor_support,feat_req__lifecycle__cond_process_start,feat_req__lifecycle__dependency_check,feat_req__lifecycle__process_orderingTestConditionalLaunchingWithDaemon,TestConditionalLaunchingBlocksOnMissingDependency)feat_req__lifecycle__total_wait_time_support,feat_req__lifecycle__polling_interval,feat_req__lifecycle__path_condition_check,feat_req__lifecycle__env_variable_cond_check,feat_req__lifecycle__dependency_check,feat_req__lifecycle__validate_conditionsTestConditionalLaunchingScenario,TestConditionalLaunchingScenarioTimesOutOnUnmetConditions,TestConditionalLaunchingScenarioRejectsUnsupportedPrefix)