Skip to content

Refactor: eliminate racy process-env reads from build_netsuke_command #284

@coderabbitai

Description

@coderabbitai

Summary

build_netsuke_command in tests/bdd/steps/manifest_command.rs currently reads the process environment without holding EnvLock to obtain values for scenario-tracked variables as well as PATH and NETSUKE_NINJA. Because Rust test threads share a single process, these lockless reads race with concurrent VarGuard mutations from other test threads, risking non-deterministic test failures.

Naïvely adding EnvLock::acquire() inside build_netsuke_command is not safe: NinjaEnvGuard holds EnvLock for its entire lifetime, so calling build_netsuke_command from a scenario thread that already holds a NinjaEnvGuard would deadlock on the non-reentrant Mutex.

Root cause

world.env_vars stores key → old_value pairs — a restoration map used by TestWorld::drop. When build_netsuke_command needs to forward those variables to the child process, it iterates the keys and reads their current values back from the process environment via std::env::var() without a lock. The same applies to the PATH and NETSUKE_NINJA reads.

Desired outcome

Eliminate all unguarded process-env reads from build_netsuke_command. The desired forwarded values should be stored explicitly in TestWorld at the point they are set (i.e. in BDD step definitions), so that build_netsuke_command reads only from TestWorld state — never from the live process environment for scenario-controlled variables. The one remaining process-env read (for PATH) should be guarded by EnvLock.

Affected files

  • tests/bdd/fixtures/mod.rs
  • tests/bdd/steps/manifest_command.rs
  • tests/bdd/steps/advanced_usage.rs
  • Any other step modules that call track_env_var

Acceptance criteria

  • build_netsuke_command performs exactly one std::env::var_os call (for PATH), guarded by EnvLock::acquire().
  • All scenario-forwarded env vars (including NETSUKE_NINJA) are read from TestWorld state, not from the process environment.
  • All existing BDD scenarios and the unit tests in manifest_command.rs continue to pass.
  • No new unsafe blocks are introduced.

References

Raised by @leynos.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions