Add database handler abstractions, registry improvements, and test infrastructure - #233
Merged
Conversation
Managers now retry registry lock acquisition on contention (configurable timeout, default 60s) and release the lock via atexit, fixing identity loss after ungraceful container restarts. RegistryLockError is fatal. Nodes now resolve stable IDs from the registry at startup using the same IdentityResolver pattern, gaining stable identity across restarts instead of generating a fresh ULID each time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves registry-based identity stability and startup resilience by adding configurable retry behavior for registry lock contention, releasing locks on shutdown, and extending the same stable-ID resolution pattern to nodes.
Changes:
- Added
retry_timeoutsupport throughIdentityResolver→LocalRegistryManager.resolve()with retry-on-RegistryLockError. - Managers and nodes now release registry identity locks via
atexit, and nodes resolve stable IDs at startup (configurable viaNodeConfig). - Bumped project/package versions to
0.7.1and updated docs + unit tests accordingly.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/package.json | Bumps UI package version to 0.7.1. |
| src/madsci_workcell_manager/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_squid/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_resource_manager/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_node_module/tests/test_node_registry.py | Adds node identity-resolution unit tests via registry resolver mocking. |
| src/madsci_node_module/tests/conftest.py | Disables registry resolution by default in node tests to avoid shared-lock side effects. |
| src/madsci_node_module/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_node_module/madsci/node_module/abstract_node_module.py | Adds node registry identity resolution + atexit lock release. |
| src/madsci_location_manager/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_experiment_manager/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_experiment_application/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_event_manager/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_data_manager/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_common/tests/test_registry_resolution.py | Updates manager registry-resolution test expectations for retry_timeout. |
| src/madsci_common/tests/test_registry/test_local_registry.py | Adds tests for resolve() retry behavior under lock contention. |
| src/madsci_common/pyproject.toml | Bumps package version to 0.7.1. |
| src/madsci_common/madsci/common/types/node_types.py | Adds node registry-resolution settings fields. |
| src/madsci_common/madsci/common/types/manager_types.py | Adds manager registry_lock_timeout setting. |
| src/madsci_common/madsci/common/registry/local_registry.py | Implements retry loop in resolve() with retry_timeout. |
| src/madsci_common/madsci/common/registry/identity_resolver.py | Plumbs retry_timeout through resolver APIs. |
| src/madsci_common/madsci/common/manager_base.py | Adds manager retry timeout + fatal lock contention + atexit release. |
| src/madsci_client/pyproject.toml | Bumps package version to 0.7.1. |
| pyproject.toml | Bumps monorepo version to 0.7.1. |
| docs/api/madsci/common/types/node_types.md | Documents new NodeConfig registry fields. |
| docs/api/madsci/common/types/manager_types.md | Documents new ManagerSettings.registry_lock_timeout. |
| docs/api/madsci/common/registry/local_registry.md | Documents new retry_timeout parameter on LocalRegistryManager.resolve(). |
| docs/api/madsci/common/registry/index.md | Documents retry_timeout in resolver/registry APIs. |
| docs/api/madsci/common/registry/identity_resolver.md | Documents retry_timeout in IdentityResolver APIs. |
| docs/Configuration.md | Adds env var docs for *_REGISTRY_LOCK_TIMEOUT across managers. |
| docs/CHANGELOG.md | Adds 0.7.1 release notes for registry retry + shutdown release + node stability. |
| .env.example | Adds *_REGISTRY_LOCK_TIMEOUT examples for managers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dundant patches - Extract hardcoded 2.0s retry interval to RETRY_INTERVAL_SECONDS constant - Replace sleep-based test coordination with threading.Event for deterministic behavior - Remove redundant @patch decorators that targeted wrong import path in node registry tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add session-scoped autouse fixture that redirects MADSCI_REGISTRY_PATH to a temp file and patches enable_registry_resolution defaults to False on ManagerSettings, NodeConfig, and all their subclasses recursively. This prevents FileLock contention and heartbeat-thread spawning across 2612 tests. Key changes: - conftest.py: _patch_field_default_recursive() walks Pydantic v2 subclass trees (each has independent FieldInfo copies) - 18 test files: explicit enable_registry_resolution=False on all Settings/Config instantiations (belt-and-suspenders) - test_registry.py: use env var patching instead of _default_path mock - test_registry_resolution.py, test_node_registry.py: pass enable_registry_resolution=True explicitly in default-assertion tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch from `nbconvert --execute` to `papermill` for validating example notebooks, gaining per-cell timeouts (120s) and readable log output. Rename e2e_tests commands to validate_notebooks to clarify intent: these validate that example notebooks still work, not serve as e2e tests. - Add close() and __del__() to AbstractNode for registry lock cleanup - Add __del__() to AbstractManagerBase for GC-based identity release - Update node_notebook.ipynb to call close() before node reassignment - Rename Docker service e2e_test_runner -> notebook_validator - Rename GH Actions workflow and job to "Validate Example Notebooks" - Regenerate docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Introduce ABC-based database handler abstractions (MongoHandler, RedisHandler, PostgresHandler, MinioHandler) with real and in-memory implementations. Migrate all 6 managers to accept handler parameters, enabling fast in-memory testing without Docker containers. Key changes: - New db_handlers package with ABCs and implementations for all 4 DB types - All managers accept optional handler params (backward compatible) - All test suites migrated from pytest-mock-resources to in-memory handlers - LocalRunner uses handlers instead of raw in-memory clients - SQLiteHandler handles composite PK autoincrement via raw DDL fallback - InMemoryCollection gains projection support, replace_one(), client property - InMemoryRedisDict inherits MutableMapping for Pydantic compatibility - Fix timezone-naive datetime comparison in ResourceInterface lock checks - Fix DatabaseVersionChecker to only create SchemaVersionTable (not all tables) - Remove pytest-mock-resources dependency - 2677 tests passing (up from 2612), all without Docker Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The testcontainer-based integration fixtures now catch container startup failures and verify connectivity via ping() before yielding, converting unreachable containers into clean pytest skips instead of hard failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Root cause: PostgresContainer and MongoDbContainer verify readiness only inside the container (ExecWaitStrategy/log checks), but on macOS with Rancher Desktop, host-side port forwarding through the VM networking layer lags ~1-2s behind. Tests connecting immediately after container "ready" hit connection refused on the mapped host port. Fix: Add PortWaitStrategy(port).wait_until_ready() calls for Postgres and MongoDB fixtures to poll the host-side mapped port before yielding the handler. Redis and MinIO already verify host connectivity (Redis via client.ping() with retry, MinIO via HttpWaitStrategy). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…on-logging Replace the deprecated `LoggingHandler` from `opentelemetry.sdk._logs` with `LoggingInstrumentor` from `opentelemetry-instrumentation-logging`, which automatically bridges stdlib logging to OTEL log records via the global LoggerProvider. This eliminates 44 deprecation warnings across 5 test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Expand CHANGELOG to cover all branch changes (db handler abstractions, EventClient retry removal, OTEL logging migration, test infrastructure, node lifecycle management). Add NodeConfig/RestNodeConfig to pydantic-settings-export and regenerate Configuration.md and .env.example. Fix stale references to pytest-mock-resources and Docker-required tests across AGENTS.md, CONTRIBUTING.md, guides, and tutorials. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ngs, and safety fixes - Fix DataManager minio handler gap: consolidate all MinIO operations through MinioHandler abstraction, removing direct self.minio_client usage - Add EventClient send_failures OTEL counter and upgrade failure logging to error level with structured kwargs (event_type, event_id) - Add LoggingInstrumentor double-instrumentation guard (is_instrumented check) - Move EventManager pymongo imports behind TYPE_CHECKING / into methods - Use weakref-based atexit handlers in managers and nodes to allow GC, with _atexit_registered flag to prevent handler accumulation - Fix InMemoryMongoHandler _client = None when external database provided - Add TYPE_CHECKING-guarded return types for handler ABCs (Collection, Engine, MetaData, MutableMapping, ContextManager) - Emit DeprecationWarning for legacy constructor params (db_connection, db_client, redis_connection, mongo_connection) across all managers - Update CHANGELOG, data manager docs, auto-generated API docs, and PR body Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github.repository returns mixed-case (AD-SDL/MADSci) but Docker requires lowercase repository names. Add a step to lowercase IMAGE_NAME and DASHBOARD_IMAGE_NAME env vars before they're used in docker pull/tag commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
madsci.common.db_handlerspackage with ABCs (MongoHandler,RedisHandler,PostgresHandler,MinioHandler) and both real and in-memory implementations. All 6 database-backed managers accept injectable handler parameters, enabling dependency injection for testing.atexit(usingweakrefto avoid preventing GC). Nodes now resolve stable IDs from the registry at startup using the sameIdentityResolverpattern, with explicitclose()for notebook users.send_failuresOTEL counter and structured error logging for failed deliveries.LoggingHandlertoLoggingInstrumentorfromopentelemetry-instrumentation-logging, with double-instrumentation guard. Eliminates 44+ deprecation warnings.MinioHandlerabstraction; removed directself.minio_clientusage. Legacy raw clients are wrapped inRealMinioHandlerduring setup.db_connection,db_client,redis_connection,mongo_connection) now emitDeprecationWarningacross all 6 managers and 2 state handlers.pytest-mock-resourceswith in-memory handlers (no Docker required for most tests); replacednbconvertwithpapermill; testcontainer fixtures now skip gracefully when Docker is unavailable; addedPortWaitStrategyfor macOS host port-forwarding race.Test plan
just checks)pytest) — 2677 tests passingDeveloper Checklists
I have:
🤖 Generated with Claude Code