Skip to content

execute_spec.rs sets a process-wide timeout for four tests and twenty-one inherit it #473

Description

@vladimirrott

crates/sysknife-daemon/tests/execute_spec.rs sets a process-wide environment
variable in four tests, in a binary that holds twenty-one. The other seventeen
inherit whatever the last setter left, which is the defect #356 describes one
crate over.

$ git grep -n 'env::set_var' crates/sysknife-daemon/tests/execute_spec.rs
393:    std::env::set_var("SYSKNIFE_ACTION_TIMEOUT_SECS", "1");
530:    std::env::set_var("SYSKNIFE_ACTION_TIMEOUT_SECS", "1");
579:    std::env::set_var("SYSKNIFE_ACTION_TIMEOUT_SECS", "1");
619:    std::env::set_var("SYSKNIFE_ACTION_TIMEOUT_SECS", "1");
$ grep -cE '^\s*#\[(test|tokio::test)\]' crates/sysknife-daemon/tests/execute_spec.rs
21

Production reads it at execution time, so a neighbouring test's action inherits
the one-second ceiling:

$ sed -n '194,199p' crates/sysknife-daemon/src/executor.rs
/// `SYSKNIFE_ACTION_TIMEOUT_SECS` when an action on a slow link needs longer.
    match std::env::var("SYSKNIFE_ACTION_TIMEOUT_SECS") {

Why it matters

Every other file in the workspace that mutates the environment takes ENV_LOCK
first. Two did not:

$ comm -23 <(git grep -l 'env::set_var' -- 'apps/**/*.rs' 'crates/**/*.rs' | sort) \
           <(git grep -l 'ENV_LOCK'     -- 'apps/**/*.rs' 'crates/**/*.rs' | sort)
apps/sysknife-cli/src/mcp_server.rs
crates/sysknife-daemon/tests/execute_spec.rs

#470 removes the first by passing the socket explicitly instead of locking. This
is the other one, and it is the whole of the remaining class.

It reads differently from #356 when it fires. All four setters write the same
"1", so they do not fight each other; what breaks is one of the seventeen
tests that never asked for a one-second ceiling, timing out on a slow runner
for no reason its own source explains. That is worse than a loud failure,
because the obvious reading is that the runner was slow.

Scope

  • Remove the four mutations. Prefer passing the timeout explicitly, the way
    fix(mcp): isolate socket integration tests from process environment #470 passed the socket, over taking ENV_LOCK: a lock serialises the suite
    and leaves the shared global in place, and the next test to want a different
    value is back here.
  • If the execution path has no seam for that today, say so in the PR and add
    one rather than reaching for the lock. executor.rs:198 is the only reader.
  • Say in the PR whether any of the seventeen has ever failed this way. The
    honest answer may be no; a latent race is still worth closing, and knowing
    which it is changes how the fix should be argued.

Tests first

Show the race before you fix it. Run the binary repeatedly under ordinary
cargo test and count failures, the way #470 measured fourteen of forty; if
the rate is zero, say so and prove the coupling by pinning the variable to a
value that must break a specific test, then removing the mutation and showing
it green. A fix with no red before it is a claim.

Difficulty

medium. Four deletions and a seam, and the judgement about the seam is the
work.

Getting started

CONTRIBUTING.md
has the build and test commands. No CLA and no copyright waiver. The project is MIT.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinghelp wantedExtra attention is neededmediumDifficulty: needs familiarity with one subsystem

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions