Phase 3 DoD Gaps - Fix Plan¶
Goal: Complete Phase 3 Definition of Done by implementing missing lockfile coordination, reference-counted shutdown, and checksum verification.
Issues to Fix:
1. Multi-process coordination missing (DoD line 1033)
2. Reference-counted shutdown missing (DoD line 1034)
3. verify_checksum dead code - downloads unverified
File Structure¶
Modified:
- opensysml/binary.py - integrate checksum verification into download flow
- opensysml/connection.py - add lockfile coordination + reference counting
- tests/test_binary.py - verify checksums enforced
- tests/test_connection.py - test multi-process scenarios
- tests/test_lifecycle.py - verify reference-counted shutdown
- setup.py / pyproject.toml - add filelock dependency
Created: - None (all changes to existing files)
Task 1: Add Lockfile Coordination¶
Files:
- Modify: opensysml/connection.py
- Modify: setup.py, pyproject.toml
- Test: tests/test_connection.py
Objective: Prevent race condition when multiple processes try to auto-start service simultaneously.
Design:
- Lockfile path: ~/.opensysml/sysml-grpc.lock
- Use filelock library (cross-platform)
- Lock acquired before checking if service running
- PID written to lockfile for reference tracking
- Lock released after service confirmed healthy
Implementation¶
- [ ] Step 1: Add filelock dependency
Add to setup.py:
Add to pyproject.toml:
- [ ] Step 2: Install filelock
Run: pip install filelock
Expected: Package installed successfully
- [ ] Step 3: Write failing test for lockfile acquisition
# tests/test_connection.py
from filelock import FileLock
import os
def test_ensure_service_uses_lockfile():
"""Test that _ensure_service acquires lockfile before starting service."""
with patch('opensysml.connection.ensure_binary') as mock_ensure:
mock_ensure.return_value = '/path/to/sysml-grpc'
with patch('opensysml.connection._probe_service', return_value=False):
with patch('subprocess.Popen') as mock_popen:
mock_popen.return_value = Mock(pid=12345)
# Mock time.sleep to skip retries
with patch('time.sleep'):
with patch('opensysml.connection._probe_service', side_effect=[False, True]):
conn = Connection(auto_start=True)
# Verify lockfile was created
lockfile_path = os.path.expanduser('~/.opensysml/sysml-grpc.lock')
assert os.path.exists(lockfile_path)
def test_concurrent_ensure_service_blocks():
"""Test that second process blocks while first starts service."""
lockfile_path = os.path.expanduser('~/.opensysml/sysml-grpc.lock')
# Simulate first process holding lock
lock1 = FileLock(lockfile_path, timeout=0.1)
lock1.acquire()
try:
# Second process should timeout trying to acquire
with pytest.raises(TimeoutError):
with patch('opensysml.connection.ensure_binary', return_value='/path/to/binary'):
with patch('opensysml.connection._probe_service', return_value=False):
Connection(auto_start=True)
finally:
lock1.release()
- [ ] Step 4: Run tests to verify they fail
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/test_connection.py::test_ensure_service_uses_lockfile -v
Expected: FAIL with "FileNotFoundError: lockfile_path"
- [ ] Step 5: Implement lockfile coordination in _ensure_service
# opensysml/connection.py
from filelock import FileLock, Timeout
import os
def _get_lockfile_path():
"""Get path to service lockfile."""
opensysml_dir = os.path.expanduser('~/.opensysml')
os.makedirs(opensysml_dir, exist_ok=True)
return os.path.join(opensysml_dir, 'sysml-grpc.lock')
def _get_pidfile_path():
"""Get path to service PID file."""
opensysml_dir = os.path.expanduser('~/.opensysml')
return os.path.join(opensysml_dir, 'sysml-grpc.pid')
def _ensure_service(self):
"""Ensure sysml-grpc service is running, with lockfile coordination.
Uses filelock to coordinate between multiple Python processes.
If service already running, returns immediately.
Otherwise, acquires lock and starts service.
"""
lockfile_path = _get_lockfile_path()
lock = FileLock(lockfile_path, timeout=30)
try:
with lock:
# Check if service already running (another process may have started it)
if self._probe_service(self.host, self.port):
return
# Get binary path
binary_path = ensure_binary()
if not os.path.exists(binary_path):
raise RuntimeError(f"Binary not found after download: {binary_path}")
# Start service
process = subprocess.Popen(
[binary_path, '-port', str(self.port)],
start_new_session=True,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL
)
self._service_process = process
# Write PID to file for reference counting
pidfile_path = _get_pidfile_path()
with open(pidfile_path, 'w') as f:
f.write(f"{process.pid}\n")
# Wait for service to become healthy
max_retries = 5
retry_delay = 0.5
startup_timeout = 10.0
for attempt in range(max_retries):
time.sleep(retry_delay)
if self._probe_service(self.host, self.port, timeout=2.0):
# Register cleanup
atexit.register(self._cleanup_service)
return
# Service didn't start in time
self._cleanup_service()
raise RuntimeError(f"Service failed to start within {startup_timeout}s")
except Timeout:
raise RuntimeError(
f"Timeout acquiring service lockfile after 30s. "
f"Another process may be starting the service."
)
- [ ] Step 6: Run tests to verify they pass
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/test_connection.py::test_ensure_service_uses_lockfile tests/test_connection.py::test_concurrent_ensure_service_blocks -v
Expected: PASS (2 tests)
- [ ] Step 7: Commit lockfile coordination
git add opensysml/connection.py tests/test_connection.py setup.py pyproject.toml
git commit -m "feat(connection): add lockfile coordination for multi-process service startup"
Task 2: Implement Reference-Counted Shutdown¶
Files:
- Modify: opensysml/connection.py
- Modify: tests/test_lifecycle.py
Objective: Track how many processes are using the service, shut down only when last process exits.
Design:
- Reference count file: ~/.opensysml/sysml-grpc.refcount
- Atomic increment on service start
- Atomic decrement on cleanup
- Terminate service when refcount reaches 0
- Use lockfile for atomic refcount operations
Implementation¶
- [ ] Step 1: Write failing test for reference counting
# tests/test_lifecycle.py
def test_service_shuts_down_when_last_process_exits():
"""Test that service terminates when reference count reaches 0."""
import time
# First connection increments refcount to 1
with patch('opensysml.binary.ensure_binary', return_value=get_binary_path()):
conn1 = Connection(auto_start=True)
# Get PID
pidfile = os.path.expanduser('~/.opensysml/sysml-grpc.pid')
with open(pidfile) as f:
pid = int(f.read().strip())
# Service should be running
assert psutil.Process(pid).is_running()
# Second connection increments to 2
conn2 = Connection(auto_start=True)
# Close first connection (refcount -> 1)
conn1.close()
time.sleep(0.5)
# Service should still be running
assert psutil.Process(pid).is_running()
# Close second connection (refcount -> 0)
conn2.close()
time.sleep(0.5)
# Service should be terminated
assert not psutil.Process(pid).is_running()
- [ ] Step 2: Run test to verify it fails
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/test_lifecycle.py::test_service_shuts_down_when_last_process_exits -v
Expected: FAIL (service doesn't shut down)
- [ ] Step 3: Implement reference counting
# opensysml/connection.py
def _get_refcount_path():
"""Get path to service reference count file."""
opensysml_dir = os.path.expanduser('~/.opensysml')
return os.path.join(opensysml_dir, 'sysml-grpc.refcount')
def _increment_refcount():
"""Atomically increment service reference count.
Returns:
int: New reference count
"""
lockfile_path = _get_lockfile_path()
lock = FileLock(lockfile_path, timeout=5)
with lock:
refcount_path = _get_refcount_path()
# Read current count
if os.path.exists(refcount_path):
with open(refcount_path, 'r') as f:
count = int(f.read().strip())
else:
count = 0
# Increment
count += 1
# Write back
with open(refcount_path, 'w') as f:
f.write(str(count))
return count
def _decrement_refcount():
"""Atomically decrement service reference count.
Returns:
int: New reference count (0 if file doesn't exist)
"""
lockfile_path = _get_lockfile_path()
lock = FileLock(lockfile_path, timeout=5)
with lock:
refcount_path = _get_refcount_path()
if not os.path.exists(refcount_path):
return 0
# Read current count
with open(refcount_path, 'r') as f:
count = int(f.read().strip())
# Decrement
count = max(0, count - 1)
# Write back
if count > 0:
with open(refcount_path, 'w') as f:
f.write(str(count))
else:
# Remove file when count reaches 0
os.remove(refcount_path)
return count
# Update _ensure_service to increment refcount
def _ensure_service(self):
"""Ensure sysml-grpc service is running, with lockfile coordination."""
# ... existing lockfile logic ...
# After service confirmed healthy:
_increment_refcount()
atexit.register(self._cleanup_service)
return
# Update _cleanup_service to check refcount
def _cleanup_service(self):
"""Clean up service process with reference counting.
Only terminates service if reference count reaches 0.
"""
# Decrement refcount
new_count = _decrement_refcount()
if new_count == 0:
# Last connection - shut down service
pidfile_path = _get_pidfile_path()
if os.path.exists(pidfile_path):
with open(pidfile_path, 'r') as f:
pid = int(f.read().strip())
try:
import psutil
process = psutil.Process(pid)
process.terminate()
process.wait(timeout=5)
except (psutil.NoSuchProcess, psutil.TimeoutExpired):
# Process already dead or timeout - force kill
try:
process.kill()
except psutil.NoSuchProcess:
pass
# Clean up PID file
os.remove(pidfile_path)
# Clean up instance state
if self._service_process:
self._service_process = None
- [ ] Step 4: Add psutil dependency
Add to setup.py and pyproject.toml:
install_requires=[
"grpcio>=1.83.0",
"protobuf>=7.35.1",
"filelock>=3.0.0",
"psutil>=5.9.0", # Add this
]
- [ ] Step 5: Install psutil
Run: pip install psutil
Expected: Package installed successfully
- [ ] Step 6: Run test to verify it passes
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/test_lifecycle.py::test_service_shuts_down_when_last_process_exits -v
Expected: PASS
- [ ] Step 7: Update existing persistence tests
The tests test_service_survives_connection_close and test_service_persists_across_multiple_loads currently verify service persists. Update them to use multiple connections so refcount stays > 0.
# tests/test_lifecycle.py - update test_service_survives_connection_close
def test_service_survives_connection_close():
"""Test that service persists when other connections still active."""
# ... existing setup ...
# Create TWO connections to keep refcount > 0
conn1 = Connection(auto_start=False, host='localhost', port=50051)
conn2 = Connection(auto_start=False, host='localhost', port=50051)
# Close first connection
conn1.close()
time.sleep(0.5)
# Service should still be running (conn2 keeps it alive)
response = conn2.stub.GetDiagnostics(sysml_pb2.DiagnosticsRequest(model_hash="test"))
assert response is not None # Service responds
conn2.close()
- [ ] Step 8: Run full test suite
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/ -v
Expected: All tests pass
- [ ] Step 9: Commit reference counting
git add opensysml/connection.py tests/test_lifecycle.py setup.py pyproject.toml
git commit -m "feat(connection): add reference-counted service shutdown"
Task 3: Integrate Checksum Verification¶
Files:
- Modify: opensysml/binary.py
- Modify: tests/test_binary.py
Objective: Verify downloaded binaries against checksums before using them.
Design:
- Checksum file: https://github.com/Open-MBEE/OpenSysML/releases/download/{version}/{binary_name}.sha256
- Download checksum file alongside binary
- Verify before marking download complete
- Fail if checksum mismatch
Implementation¶
- [ ] Step 1: Write failing test for checksum enforcement
# tests/test_binary.py
def test_download_binary_verifies_checksum():
"""Test that download_binary fetches and verifies checksum."""
version = 'v0.1.0'
github_repo = 'Open-MBEE/OpenSysML'
# Mock binary download
mock_binary_data = b'fake binary content'
actual_checksum = hashlib.sha256(mock_binary_data).hexdigest()
# Mock checksum file download
mock_checksum_data = f"{actual_checksum} sysml-grpc-linux-amd64\n".encode()
with patch('urllib.request.urlopen') as mock_urlopen:
# First call: checksum file
# Second call: binary
mock_urlopen.side_effect = [
Mock(__enter__=Mock(return_value=Mock(read=Mock(return_value=mock_checksum_data)))),
Mock(__enter__=Mock(return_value=Mock(read=Mock(return_value=mock_binary_data))))
]
with patch('opensysml.binary.detect_platform', return_value=('linux', 'amd64')):
result = download_binary(version, github_repo)
# Should have called urlopen twice (checksum + binary)
assert mock_urlopen.call_count == 2
# Verify checksum URL was constructed correctly
checksum_url = mock_urlopen.call_args_list[0][0][0]
assert '.sha256' in checksum_url
def test_download_binary_fails_on_checksum_mismatch():
"""Test that download fails if checksum doesn't match."""
version = 'v0.1.0'
github_repo = 'Open-MBEE/OpenSysML'
# Mock binary download
mock_binary_data = b'fake binary content'
# Wrong checksum
wrong_checksum = 'deadbeef' * 8
mock_checksum_data = f"{wrong_checksum} sysml-grpc-linux-amd64\n".encode()
with patch('urllib.request.urlopen') as mock_urlopen:
mock_urlopen.side_effect = [
Mock(__enter__=Mock(return_value=Mock(read=Mock(return_value=mock_checksum_data)))),
Mock(__enter__=Mock(return_value=Mock(read=Mock(return_value=mock_binary_data))))
]
with patch('opensysml.binary.detect_platform', return_value=('linux', 'amd64')):
with pytest.raises(RuntimeError, match="Checksum mismatch"):
download_binary(version, github_repo)
- [ ] Step 2: Run tests to verify they fail
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/test_binary.py::test_download_binary_verifies_checksum tests/test_binary.py::test_download_binary_fails_on_checksum_mismatch -v
Expected: FAIL (checksum not downloaded/verified)
- [ ] Step 3: Update download_binary to fetch and verify checksum
# opensysml/binary.py
import hashlib
def download_binary(version='v0.1.0', github_repo='Open-MBEE/OpenSysML'):
"""Download sysml-grpc binary from GitHub releases with checksum verification.
Args:
version (str): Release version tag (e.g., 'v0.1.0')
github_repo (str): GitHub repository (owner/repo)
Raises:
RuntimeError: If download fails or checksum mismatch
"""
if version == 'latest':
raise NotImplementedError(
"version='latest' not yet supported. "
"Specify explicit version tag like 'v0.1.0'"
)
goos, goarch = detect_platform()
binary_name = f'sysml-grpc-{goos}-{goarch}'
if goos == 'windows':
binary_name += '.exe'
# Construct URLs
base_url = f'https://github.com/{github_repo}/releases/download/{version}'
binary_url = f'{base_url}/{binary_name}'
checksum_url = f'{base_url}/{binary_name}.sha256'
binary_path = get_binary_path()
os.makedirs(os.path.dirname(binary_path), exist_ok=True)
try:
# Download checksum file first
import urllib.request
with urllib.request.urlopen(checksum_url) as response:
checksum_content = response.read().decode('utf-8')
# Parse checksum (format: "hexdigest filename\n")
expected_checksum = checksum_content.split()[0]
# Download binary
with urllib.request.urlopen(binary_url) as response:
binary_data = response.read()
# Write to temporary file first
temp_path = binary_path + '.tmp'
with open(temp_path, 'wb') as f:
f.write(binary_data)
# Verify checksum
if not verify_checksum(temp_path, expected_checksum):
os.remove(temp_path)
raise RuntimeError(
f"Checksum mismatch for {binary_name}. "
f"Expected {expected_checksum}, but download does not match. "
f"Binary may be corrupted or tampered with."
)
# Checksum valid - move to final location
os.rename(temp_path, binary_path)
# Make executable
os.chmod(binary_path, 0o755)
return binary_path
except urllib.error.URLError as e:
raise RuntimeError(f"Failed to download binary from {binary_url}: {e}")
- [ ] Step 4: Run tests to verify they pass
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/test_binary.py::test_download_binary_verifies_checksum tests/test_binary.py::test_download_binary_fails_on_checksum_mismatch -v
Expected: PASS (2 tests)
- [ ] Step 5: Verify verify_checksum is no longer dead code
Run: grep -n "verify_checksum" opensysml/binary.py
Expected: Should show both definition (line ~102) and call site (in download_binary)
- [ ] Step 6: Run full test suite
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/ -v
Expected: All tests pass
- [ ] Step 7: Commit checksum verification
git add opensysml/binary.py tests/test_binary.py
git commit -m "feat(binary): add checksum verification for downloaded binaries"
Final Verification¶
- [ ] Run full test suite
Run: PYTHONPATH=/home/han/IdeaProjects/OpenSysML pytest tests/ -v
Expected: All tests pass
- [ ] Run Go tests
Run: go test ./...
Expected: All tests pass
- [ ] Verify Phase 3 DoD complete
Check each DoD item:
- [x] import opensysml auto-downloads binary on first use
- [x] model = opensysml.load("A1.sysml") works without manual service start
- [x] Multiple Python processes can import concurrently (lockfile prevents conflicts)
- [x] Service shuts down when last process exits (reference counting)
- [x] Tests pass: pytest tests/test_binary.py tests/test_lifecycle.py
- [x] Checksums verified for security
- [ ] Manual test: Multi-process scenario
# test_concurrent.py
import opensysml
import multiprocessing
import time
def worker(file_path, worker_id):
print(f"Worker {worker_id} starting...")
model = opensysml.load(file_path)
print(f"Worker {worker_id} loaded: {model.root.name}")
time.sleep(2) # Hold connection
print(f"Worker {worker_id} done")
if __name__ == '__main__':
# Start 3 processes concurrently
processes = []
for i in range(3):
p = multiprocessing.Process(target=worker, args=("A1.sysml", i))
processes.append(p)
p.start()
# Wait for all to complete
for p in processes:
p.join()
print("All workers completed")
# Service should still be running (refcount = 3 -> 0)
Run: python test_concurrent.py
Expected:
- All 3 workers load successfully
- No port conflicts
- Service shuts down after all workers exit
Definition of Done¶
All items must be ✅:
- [ ] Lockfile coordination implemented and tested
- [ ] Reference-counted shutdown implemented and tested
- [ ] Checksum verification integrated into download_binary
- [ ] All unit tests pass (pytest tests/)
- [ ] All integration tests pass (pytest -m integration)
- [ ] Go tests pass (go test ./...)
- [ ] Manual multi-process test succeeds
- [ ] No orphaned sysml-grpc processes after tests
- [ ] Phase 3 DoD checklist 100% complete