Skip to content

fix: Close remaining RFC 0008 wrap-action gaps in hook scoping, cleanup, and preflight #277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwiebe merged 1 commit into from
Jul 23, 2026
Merged

fix: Close remaining RFC 0008 wrap-action gaps in hook scoping, cleanup, and preflight #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
301 changes: 199 additions & 102 deletions crates/openjd-cli/src/run/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,53 +306,6 @@ pub async fn execute(args: RunArgs) -> Result<(), Box<dyn std::error::Error>> {
// parameters and fail with "Undefined variable".
let env_template_symtab = openjd_model::build_symbol_table(&param_values)
.map_err(|e| format!("Failed to build environment template symbol table: {e}"))?;
for et in &env_templates {
println!("{}\t", fmt_elapsed(&session_start));
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
println!(
"{}\t--------- Entering Environment: {}",
fmt_elapsed(&session_start),
et.environment.name
);
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
let job_env = openjd_model::convert_environment_with_symtab(
&et.environment,
Some(&env_template_symtab),
);
let _out = session
.enter_environment(&job_env, None, None, None)
.await
.map_err(|e| format!("Environment template setup failed: {e}"))?;
}
// Enter job environments
if let Some(job_envs) = &job.job_environments {
for env in job_envs {
println!("{}\t", fmt_elapsed(&session_start));
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
println!(
"{}\t--------- Entering Environment: {}",
fmt_elapsed(&session_start),
env.name
);
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
let _out = session
.enter_environment(env, None, None, None)
.await
.map_err(|e| format!("Environment setup failed: {e}"))?;
}
}

let mut tasks_run: usize = 0;
let mut session_failed = false;
Expand Down Expand Up @@ -403,6 +356,182 @@ pub async fn execute(args: RunArgs) -> Result<(), Box<dyn std::error::Error>> {
StepDependencyGraph::new(&job)?.topo_sorted()?
};

// RFC 0008 preflight: "If more than one Environment in the session
// stack defines any wrap hook, the session is invalid and the
// scheduler must reject it before entering any Environment." The model
// validator enforces this within one template; here we check the
// stacks this run will actually build — external env templates + job
// environments + each selected step's step environments — before
// entering anything. (The session's own enter-time check remains as
// defense in depth for library callers.)
{

@jericht jericht Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this execute function is getting a bit long at 800+ lines, might be worth simplifying it later on

let mut base_wrap_envs: Vec<&str> = Vec::new();
for et in &env_templates {
if et
.environment
.script
.as_ref()
.is_some_and(|s| s.actions.has_any_wrap_hook())
{
base_wrap_envs.push(et.environment.name.as_str());
}
}
if let Some(job_envs) = &job.job_environments {
for env in job_envs {
if env
.script
.as_ref()
.is_some_and(|s| s.actions.has_any_wrap_hook())
{
base_wrap_envs.push(env.name.as_str());
}
}
}
let reject = |names: Vec<&str>| -> Box<dyn std::error::Error> {
format!(
"RFC 0008: a session may have at most one Environment defining wrap hooks \
(onWrapEnvEnter / onWrapTaskRun / onWrapEnvExit). Found {}: {}.",
names.len(),
names.join(", ")
)
.into()
};
if base_wrap_envs.len() > 1 {
return Err(reject(base_wrap_envs));
}
for &step_idx in &steps_to_run {
let step = &job.steps[step_idx];
let mut stack_wrap_envs = base_wrap_envs.clone();
if let Some(step_envs) = &step.step_environments {
for env in step_envs {
if env
.script
.as_ref()
.is_some_and(|s| s.actions.has_any_wrap_hook())
{
stack_wrap_envs.push(env.name.as_str());
}
}
}
if stack_wrap_envs.len() > 1 {
return Err(reject(stack_wrap_envs));
}
}
}

// Environments entered so far, as (identifier, name, step symtab), in
// enter order. Cleanup exits these in LIFO order — including an
// environment whose enter action FAILED. OpenJD's cleanup guarantee
// (How Jobs Are Run, extended by RFC 0008 "Lifecycle and cleanup
// guarantees") is that every environment the session entered or
// attempted to enter has its onExit run — through the wrap layer when
// one is active — before the session ends. The third element is the
// step's resolved symtab for step-scoped environments (`None` for job
// and template environments).
type EnvSymtab = Option<openjd_expr::SerializedSymbolTable>;
let mut entered_envs: Vec<(String, String, EnvSymtab)> = Vec::new();

// Enter an environment, recording it for cleanup. On failure, report
// the failing action's exit code (the conformance suite attributes the
// first non-zero `exited with code:` line to the failure) and record
// the environment anyway when the session kept it on its stack, so
// cleanup still exits it.
macro_rules! enter_env {
($env:expr, $name:expr, $symtab:expr) => {{
println!("{}\t", fmt_elapsed(&session_start));
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
println!(
"{}\t--------- Entering Environment: {}",
fmt_elapsed(&session_start),
$name
);
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
let symtab: EnvSymtab = $symtab;
match session
.enter_environment($env, symtab.as_ref(), None, None)
.await
{
Ok(id) => entered_envs.push((id, $name.to_string(), symtab)),
Err(e) => {
eprintln!("ERROR: Environment setup failed: {e}");
if let Some(code) = session.action_status().and_then(|s| s.exit_code) {
println!(
"{}\tProcess exited with code: {}",
fmt_elapsed(&session_start),
code
);
}
// A failed enter action leaves the environment on the
// session stack; a rejection before entry (e.g. the
// RFC 0008 single-wrap-layer rule) does not.
if let Some(id) = session.environments_entered().last() {
if !entered_envs.iter().any(|(eid, _, _)| eid == id) {
entered_envs.push((id.clone(), $name.to_string(), symtab));
}
}
session_failed = true;
}
}
}};
}

// Exit recorded environments in LIFO order until only `$baseline`
// remain. Runs the exits even after failures — the cleanup guarantee.
macro_rules! exit_envs_down_to {
($baseline:expr) => {{
while entered_envs.len() > $baseline {
let (id, name, symtab) = entered_envs.pop().expect("guarded by len");
println!("{}\t", fmt_elapsed(&session_start));
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
println!(
"{}\t--------- Exiting Environment: {}",
fmt_elapsed(&session_start),
name
);
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
if let Err(e) = session
.exit_environment(&id, symtab.as_ref(), true, None)
.await
{
eprintln!("ERROR: Environment teardown failed: {e}");
session_failed = true;
}
}
}};
}

// Enter environment template environments, then job environments.
for et in &env_templates {
if session_failed {
break;
}
let job_env = openjd_model::convert_environment_with_symtab(
&et.environment,
Some(&env_template_symtab),
);
enter_env!(&job_env, &et.environment.name, None);
}
if let Some(job_envs) = &job.job_environments {
for env in job_envs {
if session_failed {
break;
}
enter_env!(env, &env.name, None);
}
}

// Execute steps
for &step_idx in &steps_to_run {
if session_failed || interrupted.load(Ordering::SeqCst) {
Expand All @@ -418,15 +547,26 @@ pub async fn execute(args: RunArgs) -> Result<(), Box<dyn std::error::Error>> {

let step_symtab = step.resolved_symtab.as_ref();

// Enter step environments
// The job/template environments stay entered across steps; step
// environments stack on top and are unwound to this baseline at
// the end of the step (or on failure).
let step_env_baseline = entered_envs.len();

// Enter step environments. A failure marks the session failed and
// skips the tasks, but the unwind below still exits everything
// this step entered (cleanup guarantee).
if let Some(step_envs) = &step.step_environments {
for env in step_envs {
let _out = session
.enter_environment(env, step_symtab, None, None)
.await
.map_err(|e| format!("Step environment setup failed: {e}"))?;
if session_failed {
break;
}
enter_env!(env, &env.name, step.resolved_symtab.clone());
}
}
if session_failed {
exit_envs_down_to!(step_env_baseline);
break;
}

// Build iterator
let has_params;
Expand Down Expand Up @@ -681,58 +821,15 @@ pub async fn execute(args: RunArgs) -> Result<(), Box<dyn std::error::Error>> {
}
}

// Exit step environments in reverse
if let Some(step_envs) = &step.step_environments {
for _env in step_envs.iter().rev() {
if let Some(id) = session.environments_entered().last().cloned() {
if let Ok(_out) = session.exit_environment(&id, step_symtab, true, None).await {
}
}
}
}
// Exit step environments in reverse (down to the pre-step baseline).
exit_envs_down_to!(step_env_baseline);
}

// Exit job environments in reverse
if let Some(job_envs) = &job.job_environments {
for env in job_envs.iter().rev() {
println!("{}\t", fmt_elapsed(&session_start));
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
println!(
"{}\t--------- Exiting Environment: {}",
fmt_elapsed(&session_start),
env.name
);
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
if let Some(id) = session.environments_entered().last().cloned() {
if let Ok(_out) = session.exit_environment(&id, None, true, None).await {}
}
}
}
for et in env_templates.iter().rev() {
println!("{}\t", fmt_elapsed(&session_start));
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
println!(
"{}\t--------- Exiting Environment: {}",
fmt_elapsed(&session_start),
et.environment.name
);
println!(
"{}\t==============================================",
fmt_elapsed(&session_start)
);
if let Some(id) = session.environments_entered().last().cloned() {
if let Ok(_out) = session.exit_environment(&id, None, true, None).await {}
}
}
// Exit every remaining entered environment in LIFO order. This runs
// even when an enter action failed — the cleanup guarantee (How Jobs
// Are Run; RFC 0008 for wrapped exits) is that every environment
// entered or attempted has its onExit run before the session ends.
exit_envs_down_to!(0);

@github-actions github-actions Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This final cleanup (and the per-step exit_envs_down_to!(step_env_baseline)) is bypassed when run_task returns an Err rather than a failed ActionState. The three task call sites still use .map_err(...)? (lines 624, 706, 763), so a session-level error from run_task returns early from execute() and skips all environment exits — the same cleanup-bypass class this PR fixes for the enter path.

This matters here specifically because the scope-separation work newly introduces run_task error modes in the wrap path: if a wrap environment’s let bindings or WrappedAction.* resolution fails, seed_wrapped_action_symbols returns SessionError::FormatString, which propagates out of run_task as Err. In that case a wrapper whose onEnter already started a container would never get its onWrapEnvExit/onExit run — the very guarantee the PR is trying to establish end-to-end.

Consider treating a run_task Err the same as a failed enter: log it, set session_failed = true, and fall through to cleanup instead of ?-propagating.

@mwiebe mwiebe Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #276 is covering this already. Let's look at it in that pr.


println!("{}\t", fmt_elapsed(&session_start));
if session_failed {
Expand Down
36 changes: 36 additions & 0 deletions crates/openjd-cli/tests/cli_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3362,4 +3362,40 @@ mod wrap_actions {
"no task may run in a session with two wrap layers. stdout: {stdout}"
);
}

/// RFC 0008 "Lifecycle and cleanup guarantees", scenario row 1: a
/// failed `onWrapEnvEnter` for an inner environment still runs that
/// environment's `onWrapEnvExit` and the wrapping environment's own
/// `onExit` before the run ends, and the failing hook's exit code is
/// the one surfaced for the failure.
#[test]
fn test_run_failed_wrap_enter_still_runs_wrap_exit_and_own_exit() {
let tdir = templates_dir();
let (code, stdout, stderr) = run_cli(&[
"run",
tdir.join("wrap_job_with_inner_env.yaml").to_str().unwrap(),
"--env",
tdir.join("wrap_env_failing_enter.yaml").to_str().unwrap(),
]);
assert_ne!(code, 0, "the failed enter must fail the run");
let combined = format!("{stdout}\n{stderr}");
for expected in [
"WrapEnv Own Enter",
"Wrap Enter Failing",
"Process exited with code: 7",
"Wrap Exit Ran for InnerEnv",
"WrapEnv Own Exit",
] {
assert!(
combined.contains(expected),
"expected '{expected}' in output; got:\n{combined}"
);
}
for forbidden in ["TaskBody Ran", "Inner Enter Body", "Inner Exit Body"] {
assert!(
!combined.contains(forbidden),
"'{forbidden}' must not run after the failed enter; got:\n{combined}"
);
}
}
}
Loading
Loading