Skip to content

[Thinking, not ready] fix: Record task-emitted openjd_env macros for WrappedAction.Environment #367

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

Draft
leongdl wants to merge 3 commits into
base: mainline
Choose a base branch
from
Draft

[Thinking, not ready] fix: Record task-emitted openjd_env macros for WrappedAction.Environment #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54b83a3
fix: Record task-emitted openjd_env macros for WrappedAction.Environment
leongdl Sep 8, 2026
973a78d
test: Pin the snapshot read deterministically, and narrow the caught …
leongdl Sep 8, 2026
9529ab0
docs: Correct the wrapped/unwrapped equivalence claim on the SET branch
leongdl Sep 9, 2026
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
126 changes: 90 additions & 36 deletions src/openjd/sessions/_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,21 @@ class Session(object):
The only remover is an explicit ``openjd_unset_env``. openjd-rs holds the
same split -- its session-lifetime ``env_vars`` beside its per-environment
``created_env_vars`` -- and this mirrors it.

Written by every ``openjd_env`` macro, including one emitted by a task's
``onRun`` when no environment is running. Such a macro is recorded here and
nowhere else, so it reaches ``WrappedAction.Environment`` without altering
any child process environment.

openjd-cli 0.1.14 / openjd-sessions 0.5.5, the released openjd-rs lineage,
behaves the same way: ``apply_message`` writes the cumulative ``env_vars``
for every SetEnv regardless of which action is running, that map is what
``seed_wrapped_action_symbols`` receives, and ``evaluate_env_vars`` builds
process environments from ``created_env_vars`` alone. Read the released tag,
not ``main``: openjd-rs #362 repointed the wrap-hook seeding at
``live_session_env_vars()``, which drops task-emitted macros, so ``main``
currently fails the conformance fixture named below. That is tracked as an
openjd-rs regression, not a divergence to copy here.
"""

_wrap_env_file_records: dict[EnvironmentIdentifier, list["_FileRecord"]]
Expand Down Expand Up @@ -2106,8 +2121,16 @@ def _collect_session_env_list(self) -> list[str]:
``_session_env_vars`` is insertion-ordered and already holds the
effective value per name -- a later set overwrites in place, and an
explicit ``openjd_unset_env`` removes the name -- so this is a
formatting step only."""
return [f"{name}={value}" for name, value in self._session_env_vars.items()]
formatting step only.

Iterates a snapshot: the writer is ``_action_callback`` on the
LoggingSubprocess stdout thread, and iterating the live dict while that
thread inserts raises ``RuntimeError: dictionary keys changed during
iteration``. Reproduced before this snapshot was added. ``dict.copy()``
rather than a lock, because the writer runs on the thread that forwards
a running child's output and must not be able to block on this reader.
"""
return [f"{name}={value}" for name, value in self._session_env_vars.copy().items()]

@github-actions github-actions Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

_session_env_vars is now growable from arbitrary task stdout, and it has no bound. Before this PR only an environment-entry action could add to it — a template-controlled, small set. Now any task that prints openjd_env: lines in a loop appends session-lifetime entries that are never reclaimed (environment exit deliberately does not remove them, per the docstring above), and _collect_session_env_list() renders all of them into WrappedAction.Environment.

Two consequences:

  • Memory: a long-lived session with a chatty task grows this dict without limit. dict.copy() on every wrap-hook launch also copies it in full each time.
  • WrappedAction.Environment becomes a launch-failure vector: a hook that renders {{WrappedAction.Environment}} (as the RFC examples and the tests in this PR do) puts every entry into the child argv. Once the accumulated list crosses ARG_MAX (~128 KiB per arg / 2 MiB total on Linux), Popen fails with E2BIG and every subsequent wrapped task in the session fails to start — not just the task that emitted the macros. A single misbehaving task can now poison the rest of the session for wrapped execution.

The PR's own concurrency test writes 20 000 entries into this map, so the scale is not hypothetical. Worth considering a cap on the number of task-emitted entries (or on total serialized size) with a warning when it is hit, since unlike the environment-entry path there is no template-side limit on how many a task can emit.

@leongdl leongdl Sep 8, 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.

Correct in mechanism, and I am not fixing it here. Leaving the thread open, since the concern outlives this PR.

The unbounded growth and the E2BIG path are both real, and the consequence you name — one task poisoning wrapped execution for the rest of the session — is the part worth taking seriously.

Two corrections to the scope, though. First, the vector is not new in kind: an environment's onEnter script can already emit openjd_env: in a loop, and those exports go into this same map through the pre-existing in-environment branch, so an unbounded session-lifetime map is reachable on mainline today. What this PR changes is which action can trigger it. Second, released openjd-rs has the same unbounded cumulative map feeding the same symbol, so capping here alone would diverge.

One nit on the evidence: the concurrency test's 20 000 entries were written directly to the dict by the test, not produced through the macro path, so they show the reader's cost rather than a production growth rate. That test has since been replaced with a deterministic one for unrelated reasons, so the number is gone either way.

A cap is the right idea and I would rather it be designed once, for both paths and both implementations, with a defined behaviour when it is hit — silently dropping exports and failing a hook at ARG_MAX are both bad, and choosing between "reject the macro" and "truncate the symbol" is a spec question. That is a follow-up issue rather than something to bolt onto a regression fix. Happy to file it with this analysis if you want it tracked.


def _resolve_action_timeout(self, action: Any, symtab: SymbolTable) -> Optional[int]:
"""Return the wrapped action's timeout as an int (seconds), or
Expand Down Expand Up @@ -2404,25 +2427,43 @@ def _action_log_filter_callback(

elif kind == ActionMessageKind.ENV:
if self._running_environment_identifier is None:
# Ignore the message if we're not running an environment.
# No environment is running, so a task's onRun emitted this --
# directly, or through an RFC 0008 onWrapTaskRun hook standing in
# for one. RFC 0008 (§"Stdout forwarding and macro propagation")
# requires it in WrappedAction.Environment regardless: runtimes
# "MUST include ... every openjd_env-defined variable emitted by
# any earlier action in the same session -- regardless of whether
# that action ran normally or via a wrap hook".
#
# Per How-Jobs-Are-Run, `openjd_env` "can only be emitted by the
# Action for entering an Environment" — so a task's onRun cannot
# define a session variable, and neither can an RFC 0008
# onWrapTaskRun hook, which stands in for one. That keeps
# wrapping transparent: a task that prints an `openjd_env:` line
# behaves the same wrapped and unwrapped.
# Record it in the session-lifetime map only. _created_env_vars
# stays untouched, so no process environment changes: an
# unwrapped task's environment is exactly what it was before this
# branch existed. openjd-rs holds the same split, its cumulative
# env_vars feeding the wrap symbols while evaluate_env_vars builds
# process environments from created_env_vars alone.
#
# This is deliberate, not an oversight, and it is a known
# divergence from openjd-rs, which records such a variable in the
# map that feeds WrappedAction.Environment while still not
# applying it to any subprocess environment — advertising a
# variable the wrapped context does not have. The spec does not
# settle the case (it also says a wrap script MAY emit these
# macros directly) and no conformance fixture covers it; filed
# upstream. Logged at debug so an author chasing a silent no-op
# has something to find, without adding noise to every task log.
self._log_discarded_env_macro(kind, value)
# This is NOT full wrapped/unwrapped equivalence, and the earlier
# comment here overclaimed it. The two views can disagree, in this
# direction: a task exporting a name an entered environment also
# declares wins here, so a wrap hook is handed the task's value
# while an unwrapped task's real environment keeps the
# environment's. test_task_export_does_not_displace_an_environments
# _value_in_a_child pins both halves. That also means a task's
# stdout chooses names and values a later wrapped task's hook is
# handed -- reachable only through a hook that forwards the symbol,
# but new for tasks, and matching what the released openjd-rs
# cumulative map already did. Filed upstream for a spec ruling
# alongside the redacted-value and unbounded-growth cases; it is
# one question, whether a task may write this symbol at all.
if cancel_action_mark_failed:
# Malformed macro: value is the parse error, not a variable.
# Keep discarding it, and keep not failing the action -- only
# the in-environment path below does that.
self._log_discarded_env_macro(kind, value)
return
# Assert for the type checker; the type is guaranteed by the ActionMonitoringFilter
assert isinstance(value, dict)
self._session_env_vars[value["name"]] = value["value"]

@github-actions github-actions Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Security note on the widened SET branch: this path also receives openjd_redacted_env, because _handle_redacted_env re-dispatches as ActionMessageKind.ENV with a plain name/value dict (_action_filter.py:676). So a task printing openjd_redacted_env: SECRET=hunter2 now lands its cleartext value in _session_env_vars, and from there into WrappedAction.Environment, which a wrap hook interpolates into its own command/argv.

Before this change a task-emitted redacted export was dropped here, so this is a new exposure surface, not just a new symbol value. Two concrete leak channels worth checking before merging:

  1. argv is world-readable. A hook rendering {{WrappedAction.Environment}} puts the cleartext into the child process command line, visible via /proc/<pid>/cmdline to any process of the same uid (and to ps for the session user). The _redacted_values scrubber only covers the log stream, not process argv.
  2. _redacted_values only helps if the exact substring survives. apply_message_redaction does a literal find, so the value is scrubbed when a hook echoes it verbatim — but the "Running command ..." line in _subprocess.py:663 goes through redact_openjd_redacted_env_requests, which only redacts when the literal token openjd_redacted_env: appears in the command line. A hook command that embeds the value (not the token) gets no protection from that helper and relies entirely on the filter having already seen the macro. Since the value is recorded on the stdout thread and the hook is launched from the caller thread, that ordering holds for a later task but is worth confirming, especially for the newly-reachable task-emitted case.

test_task_emitted_redacted_env_is_listed_like_any_other_export already flags this as a deliberate choice and invites inversion — this comment is to make sure the argv-exposure half of the tradeoff is on the record, since the test comment only argues about log redaction ("log redaction is unaffected"). Excluding redacted names from _session_env_vars (or recording a placeholder) would keep the RFC 0008 MUST satisfied for ordinary exports without pushing secrets through argv.

@leongdl leongdl Sep 8, 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.

Correct, and the argv half is a fair addition to the record. Leaving this open as a maintainer decision; I have not changed the behaviour, for one reason worth checking before you rule on it.

The argv exposure is not new with this PR. An environment's openjd_redacted_env export has always been recorded into _session_env_vars by the in-environment branch at line 2446, which pre-dates this change, so it already reaches WrappedAction.Environment and already lands in a hook's argv. What this PR adds is the task-emitted path to the same destination. Excluding redacted values for tasks only would leave two macros with the same name behaving differently depending on which action printed them, which I think is worse than the exposure; excluding them for both paths is defensible but is a behaviour change to existing sessions and a divergence from released openjd-rs, where RedactedEnv also writes the cumulative map that seeds the symbol.

Your second point is the sharper one and I had not verified the ordering, so thank you: redact_openjd_redacted_env_requests only fires on the literal openjd_redacted_env: token, so a hook embedding the value gets nothing from it and depends entirely on the filter having already registered the value. For the newly-reachable case that ordering does hold — _redacted_values.add happens in _handle_redacted_env on the stdout thread strictly before the callback that records the variable, so by the time any later hook can render it the value is registered — but that is an implicit dependency, not a guarantee anything asserts.

Your placeholder suggestion is the option I would pick if maintainers want the exposure closed: it satisfies the RFC MUST for ordinary exports without putting cleartext in argv. test_task_emitted_redacted_env_is_listed_like_any_other_export is where to invert it, and it is one line.

return
if cancel_action_mark_failed:
# Assert for the type checker; the type is guaranteed by the ActionMonitoringFilter
Expand All @@ -2441,15 +2482,21 @@ def _action_log_filter_callback(
changes=[EnvironmentVariableSetChange(name=value["name"], value=value["value"])]
)
# Session-lifetime copy for WrappedAction.Environment; see
# _session_env_vars. Runs on the LoggingSubprocess IO thread, like
# the line above -- a plain dict assignment, so no new hazard.
# _session_env_vars. Runs on the LoggingSubprocess IO thread, so the
# reader iterates a snapshot; see _collect_session_env_list.
self._session_env_vars[value["name"]] = value["value"]
return
elif kind == ActionMessageKind.UNSET_ENV:
if self._running_environment_identifier is None:
# Ignore the message if we're not running an environment.
# See the ENV branch above for why this is deliberate.
self._log_discarded_env_macro(kind, value)
# See the ENV branch above. An unset is the one remover from the
# session-lifetime map, matching openjd-rs, where UnsetEnv erases
# the cumulative map whether or not an environment owns the name.
if cancel_action_mark_failed:
self._log_discarded_env_macro(kind, value)
return
# Assert for the type checker; the type is guaranteed by the ActionMonitoringFilter
assert isinstance(value, str)
self._session_env_vars.pop(value, None)

@github-actions github-actions Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A task-emitted openjd_unset_env now removes the name from _session_env_vars even when an entered environment still owns it in _created_env_vars. That makes the two views disagree in the direction that breaks wrapped/unwrapped equivalence — the opposite direction from the SET branch above.

Concretely (this is exactly what test_task_emitted_unset_removes_only_the_named_variable pins): env Setter exports TASKEMIT_DOOMED, a task prints openjd_unset_env: TASKEMIT_DOOMED, and afterwards:

  • an unwrapped task still gets TASKEMIT_DOOMED=doomed-value (the test asserts SAW=[doomed-value]), because _created_env_vars was untouched;
  • a wrapped task no longer sees it, because WrappedAction.Environment is where the hook gets the variable list from, and the name is gone.

So a task can silently strip a session variable from every subsequent wrapped task while leaving it present for every unwrapped one. The SET branch is careful to avoid precisely this class of divergence ("a task behaves the same wrapped or unwrapped"); the UNSET branch introduces it.

The parity argument in the comment also does not obviously carry over: openjd-rs erasing its cumulative map is consistent there only because that map is not the sole source of the wrapped environment in the same way. If the intent is "a task cannot mutate the effective environment", then the safer reading is that an out-of-environment unset should only be able to remove a name that was itself recorded out-of-environment — i.e. not one an entered environment still holds. Otherwise this needs to be called out as a deliberate, spec-visible asymmetry.

@leongdl leongdl Sep 8, 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.

Correct on the facts, deliberately unfixed. Leaving this thread open, because the asymmetry is real and worth a maintainer decision rather than my judgement.

Everything you describe is what happens, and my own test asserts the pair: the unwrapped reader still gets SAW=[doomed-value] while the wrapped hook no longer sees the name.

Where I would push back is on it being this PR's divergence. I checked the released Rust implementation, which is the parity target the whole change is measured against: in openjd-sessions 0.5.5, UnsetEnv does self.env_vars.remove(&key) unconditionally, before and independently of the per-environment write, and self.env_vars is what seeds WrappedAction.Environment. So a task-emitted unset removes the name from the wrap symbol while created_env_vars keeps it in the process environment — the same split, in the same direction. Your suggested alternative, restricting an out-of-environment unset to names recorded out-of-environment, would be a deliberate divergence from that.

So the two candidate readings are "match the reference implementation" and "preserve wrapped/unwrapped equivalence for unset", and they conflict. I have taken the first because parity is what this PR is for, and because the SET branch's equivalence claim is about process environments, which no unset here touches. But you are right that it needs to be visible rather than implied: the UNSET branch comment names the parity, and I would rather the maintainers rule on whether the spec should say what an out-of-environment unset may remove. Happy to invert it in this PR if the answer is the second reading.

return

if cancel_action_mark_failed:
Expand Down Expand Up @@ -2501,21 +2548,28 @@ def _cancel_running_action_as_failed(self) -> None:
self.cancel_action(mark_action_failed=True)

def _log_discarded_env_macro(self, kind: ActionMessageKind, value: Any) -> None:
"""Record, at debug level, that an environment-variable stdout macro was
ignored because the running Action is not an Environment's entry Action.

Debug rather than a warning on purpose: ``_reset_action_state`` clears
the running-environment identifier for every task, and this callback
cannot tell an RFC 0008 wrap hook from an ordinary task action — so a
warning here would fire for every existing job whose task happens to
print an ``openjd_env:`` line.
"""Record, at debug level, that a malformed environment-variable stdout
macro was ignored because no Environment's entry Action is running.

A well-formed macro from a task is kept (see :attr:`_session_env_vars`);
only one that failed to parse lands here, from ``openjd_env``,
``openjd_redacted_env`` (which the filter re-dispatches as ``ENV``) or
``openjd_unset_env``. In an environment, the same parse failure cancels
the action and marks it failed. Outside one there is no environment
action to fail, and this callback cannot tell an RFC 0008 wrap hook from
an ordinary task action, so failing every task that prints a malformed
macro would be a behaviour change for existing jobs. Debug keeps it
findable without that.

``value`` is the filter's parse-error message: both call sites are
reached only when ``cancel_action_mark_failed`` is set, and the filter
pairs that flag with a ``str`` payload, never the name/value dict.
"""
name = value.get("name") if isinstance(value, dict) else value
self._logger.debug(
"Ignoring %s for '%s': environment variables can only be defined by "
"the Action that enters an Environment.",
"Ignoring malformed %s macro (%s): no Environment entry Action is running, "
"so there is no action to fail.",
kind.name.lower(),
name,
value,
)

def _fail_action_before_start(self, message: str) -> None:
Expand Down
Loading
Loading