Skip to content

fix: make SEA binary cache writes concurrency-safe #358

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

Open
abhinavkr26104 wants to merge 1 commit into
base: main
Choose a base branch
from
+48 −4
Open

fix: make SEA binary cache writes concurrency-safe #358

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
20 changes: 16 additions & 4 deletions src/stagehand/_custom/sea_binary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

import os
import sys
import hashlib
import platform
import tempfile
import importlib.resources as importlib_resources
from pathlib import Path
from contextlib import suppress
Expand Down Expand Up @@ -71,9 +71,21 @@ def _copy_to_cache(*, src: Path, filename: str, version: str) -> Path:
return dst

data = src.read_bytes()
tmp = cache_root / f".{filename}.{hashlib.sha256(data).hexdigest()}.tmp"
tmp.write_bytes(data)
tmp.replace(dst)
with tempfile.NamedTemporaryFile(dir=cache_root, prefix=f".{filename}.", suffix=".tmp", delete=False) as file:
file.write(data)

@cubic-dev-ai cubic-dev-ai Bot Aug 15, 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.

P2: The new cache-write path renames the temp file into place without flushing or fsyncing the data first (file.write(data) then tmp.replace(dst)). On a crash between the rename and the data reaching disk, dst can exist with empty/partial contents, and because both the if dst.exists(): return dst fast path and the OSError fallback treat any existing dst as complete, that corrupted binary is never repopulated and breaks every subsequent run. Flush (file.flush()) and, ideally, os.fsync(file.fileno()) the temp file before the atomic replace (and fsync the directory after on POSIX) so the published cache entry is durable.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/stagehand/_custom/sea_binary.py, line 75:

<comment>The new cache-write path renames the temp file into place without flushing or fsyncing the data first (`file.write(data)` then `tmp.replace(dst)`). On a crash between the rename and the data reaching disk, `dst` can exist with empty/partial contents, and because both the `if dst.exists(): return dst` fast path and the OSError fallback treat any existing dst as complete, that corrupted binary is never repopulated and breaks every subsequent run. Flush (`file.flush()`) and, ideally, `os.fsync(file.fileno())` the temp file before the atomic replace (and fsync the directory after on POSIX) so the published cache entry is durable.</comment>

<file context>
@@ -71,9 +71,21 @@ def _copy_to_cache(*, src: Path, filename: str, version: str) -> Path:
-    tmp.write_bytes(data)
-    tmp.replace(dst)
+    with tempfile.NamedTemporaryFile(dir=cache_root, prefix=f".{filename}.", suffix=".tmp", delete=False) as file:
+        file.write(data)
+        tmp = Path(file.name)
+
</file context>

tmp = Path(file.name)

try:
try:
tmp.replace(dst)
except OSError:
# Another process may have populated the cache first. Its atomic
# replace guarantees that an existing destination is complete.
if not dst.exists():
raise
finally:
tmp.unlink(missing_ok=True)

_ensure_executable(dst)
return dst

Expand Down
32 changes: 32 additions & 0 deletions tests/test_sea_binary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import importlib.util
from pathlib import Path
from concurrent.futures import ThreadPoolExecutor

import pytest

Expand All @@ -23,6 +24,37 @@ def _load_download_binary_module():
download_binary = _load_download_binary_module()


def test_copy_to_cache_reuses_existing_binary(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
source = tmp_path / "source"
source.write_bytes(b"new")
cached = tmp_path / "cache" / "test" / "stagehand-test"
cached.parent.mkdir(parents=True)
cached.write_bytes(b"cached")
monkeypatch.setattr(sea_binary, "_cache_dir", lambda: tmp_path / "cache")

result = sea_binary._copy_to_cache(src=source, filename="stagehand-test", version="test")

assert result == cached
assert result.read_bytes() == b"cached"


def test_copy_to_cache_is_safe_when_called_concurrently(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
source = tmp_path / "source"
source.write_bytes(b"binary" * 1_000_000)
monkeypatch.setattr(sea_binary, "_cache_dir", lambda: tmp_path / "cache")

def copy(_index: int) -> Path:
return sea_binary._copy_to_cache(src=source, filename="stagehand-test", version="test")

with ThreadPoolExecutor(max_workers=16) as executor:
results = list(executor.map(copy, range(16)))

expected = tmp_path / "cache" / "test" / "stagehand-test"
assert results == [expected] * 16
assert expected.read_bytes() == source.read_bytes()
assert list(expected.parent.glob("*.tmp")) == []


def test_resolve_binary_path_defaults_cache_version_to_package_version(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
Expand Down