Skip to content

Implementation Plan: Fix xdist Isolation — Hardcoded /dev/shm and /tmp Paths (groupB)#563

Merged
Trecek merged 1 commit intointegrationfrom
test-audit/552/groupB
Mar 28, 2026
Merged

Implementation Plan: Fix xdist Isolation — Hardcoded /dev/shm and /tmp Paths (groupB)#563
Trecek merged 1 commit intointegrationfrom
test-audit/552/groupB

Conversation

@Trecek
Copy link
Copy Markdown
Collaborator

@Trecek Trecek commented Mar 28, 2026

Summary

Five audit findings (1.22, 1.24, 1.26, 1.27, 3.7) across three test files share a single root cause: hardcoded shared filesystem paths (/dev/shm/autoskillit-sessions, /tmp, /tmp/proj) used as test working directories or ephemeral roots. Under pytest-xdist -n 4, these shared paths become a race-condition surface. The fix redirects each path to a tmp_path-scoped location per the canonical xdist isolation patterns documented in tests/CLAUDE.md.

Architecture Impact

Concurrency Diagram

%%{init: {'flowchart': {'nodeSpacing': 40, 'rankSpacing': 50, 'curve': 'basis'}}}%%
flowchart TB
    classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;
    classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff;
    classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff;
    classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff;
    classDef output fill:#00695c,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff;
    classDef gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000;

    START([task test-all])

    subgraph XDist ["PYTEST-XDIST ORCHESTRATOR"]
        RUNNER["pytest -n 4<br/>━━━━━━━━━━<br/>Spawns 4 worker<br/>processes"]
        QUEUE["WorkSteal Queue<br/>━━━━━━━━━━<br/>Distributes tests<br/>across workers"]
    end

    subgraph Workers ["PARALLEL WORKERS (×4 processes)"]
        direction LR
        W1["Worker 1<br/>━━━━━━━━━━<br/>tmp_path: unique<br/>per worker"]
        W2["Worker 2<br/>━━━━━━━━━━<br/>tmp_path: unique<br/>per worker"]
        W3["Worker 3<br/>━━━━━━━━━━<br/>tmp_path: unique<br/>per worker"]
        W4["Worker 4<br/>━━━━━━━━━━<br/>tmp_path: unique<br/>per worker"]
    end

    subgraph ModifiedTests ["● MODIFIED TESTS"]
        SS["● test_session_skills.py<br/>━━━━━━━━━━<br/>monkeypatch.setattr(<br/>ss._CANDIDATE_ROOTS, [tmp_path])"]
        HL["● test_headless.py<br/>━━━━━━━━━━<br/>cwd=str(tmp_path)<br/>instead of cwd='/tmp'"]
        AD["● test_headless_add_dirs.py<br/>━━━━━━━━━━<br/>proj=tmp_path/'proj'<br/>instead of '/tmp/proj'"]
    end

    subgraph Isolation ["ISOLATION AFTER FIX"]
        MP["monkeypatch scope<br/>━━━━━━━━━━<br/>Overrides _CANDIDATE_ROOTS<br/>Auto-restores on teardown"]
        TP["tmp_path / worker<br/>━━━━━━━━━━<br/>Unique dir per test<br/>No cross-worker writes"]
        CAND["_CANDIDATE_ROOTS → [tmp_path]<br/>━━━━━━━━━━<br/>resolve_ephemeral_root()<br/>returns tmp_path/autoskillit-sessions"]
    end

    SHARED["/dev/shm/autoskillit-sessions<br/>━━━━━━━━━━<br/>BEFORE FIX: global shared path<br/>Race: session name collisions<br/>cleanup_stale() interference"]

    PASS([PASS — no xdist races])

    START --> RUNNER
    RUNNER --> QUEUE
    QUEUE --> W1 & W2 & W3 & W4

    W1 & W2 & W3 & W4 -->|"each worker<br/>runs subset"| SS
    W1 & W2 & W3 & W4 --> HL
    W1 & W2 & W3 & W4 --> AD

    SS -.->|"before fix:<br/>race condition"| SHARED
    HL -.->|"before fix:<br/>race condition"| SHARED

    SS --> MP
    MP --> CAND
    CAND --> TP
    HL --> TP
    AD --> TP

    TP --> PASS

    class START,PASS terminal;
    class RUNNER phase;
    class QUEUE stateNode;
    class W1,W2,W3,W4 newComponent;
    class SS,HL,AD handler;
    class MP detector;
    class TP,CAND output;
    class SHARED gap;
Loading

Color Legend:

Color Category Description
Dark Blue Terminal task test-all entry and PASS result
Purple Orchestrator pytest-xdist runner
Dark Teal State WorkSteal queue, _CANDIDATE_ROOTS resolution
Green Workers / Isolation Per-worker processes; tmp_path isolation
Orange Modified Tests ● Test files changed by this PR
Red Synchronization monkeypatch scope boundary
Amber Gap (before fix) Shared /dev/shm path — race surface

Implementation Plan

Plan file: /home/talon/projects/autoskillit-runs/impl-groupB-20260328-084620-500771/.autoskillit/temp/make-plan/fix_xdist_isolation_hardcoded_paths_plan_2026-03-28_085500.md

🤖 Generated with Claude Code via AutoSkillit

Token Usage Summary

Step input output cached count time
group 2.7k 25.0k 721.4k 1 9m 3s
plan 462 84.7k 14.8M 9 41m 2s
verify 98 55.7k 3.3M 6 18m 12s
implement 536 45.2k 10.3M 9 25m 25s
fix 60 10.0k 1.6M 2 11m 53s
Total 3.8k 220.7k 30.7M 1h 45m

… isolation

Replace shared /dev/shm/autoskillit-sessions, /tmp, and /tmp/proj paths in four
tests with per-test tmp_path fixtures to eliminate race conditions under pytest-xdist -n 4.
Adds monkeypatch.setattr(ss, "_CANDIDATE_ROOTS", [tmp_path]) in test_session_skills.py
so resolve_ephemeral_root() resolves to an isolated directory.

Fixes audit findings 1.22, 1.24, 1.26, 1.27, 3.7.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
) -> None:
import autoskillit.workspace.session_skills as ss

monkeypatch.setattr(ss, "_CANDIDATE_ROOTS", [tmp_path])
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[warning] cohesion: monkeypatch.setattr(ss, '_CANDIDATE_ROOTS', [tmp_path]) injects tmp_path directly as a candidate root rather than a subdirectory. Both tests in test_headless_add_dirs.py construct tmp_path / 'proj' via mkdir(). The convention for how tmp_path is used (raw vs. subdirectory) is inconsistent across the three files in this diff. Consider using tmp_path / 'root' with mkdir() for consistency.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Investigated — this is intentional. resolve_ephemeral_root() at session_skills.py:54 appends 'autoskillit-sessions' to each candidate root, so tmp_path as a candidate root means all I/O lands in tmp_path/autoskillit-sessions/ — correctly isolated. test_resolve_ephemeral_root_fallback (line 37) also passes tmp_path directly without issue. The _CANDIDATE_ROOTS pattern (base dirs) and the cwd argument (project dirs) are different abstractions; surface consistency across them would be misleading.


ctx = make_ctx(runner=mock_runner)
await run_headless_core("/autoskillit:investigate foo", "/tmp/proj", ctx, add_dirs=())
proj = tmp_path / "proj"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[info] cohesion: Creates proj = tmp_path / 'proj' with explicit mkdir(). Pattern is consistent within the file; note that test_headless.py passes str(tmp_path) directly as cwd without a subdirectory — minor inconsistency across the diff.


with pytest.raises(RuntimeError, match="simulated crash"):
await run_headless_core("/investigate test", cwd="/tmp", ctx=tool_ctx)
await run_headless_core("/investigate test", cwd=str(tmp_path), ctx=tool_ctx)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[info] cohesion: Passes str(tmp_path) directly as cwd without creating a subdirectory (e.g. tmp_path / 'proj'), while tests in test_headless_add_dirs.py create an explicit proj subdirectory. The cwd construction pattern differs across the diff.

Copy link
Copy Markdown
Collaborator Author

@Trecek Trecek left a comment

Choose a reason for hiding this comment

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

AutoSkillit review found 1 blocking issue. See inline comments.

1 warning (changes_requested) and 2 info findings. The actionable finding is a cohesion inconsistency in how tmp_path is used across the three files — test_session_skills.py injects tmp_path directly as a _CANDIDATE_ROOTS entry while both add_dirs tests use tmp_path / 'proj' with mkdir().

@Trecek Trecek added this pull request to the merge queue Mar 28, 2026
Merged via the queue into integration with commit 456e886 Mar 28, 2026
2 checks passed
@Trecek Trecek deleted the test-audit/552/groupB branch March 28, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant