feat(lifecycle): add dormant PostgreSQL Docker lifecycle adapter - #181
Merged
Conversation
Implement the PR4 slice of SP-RESOURCE-LIFECYCLE: a lifecycle adapter that observes PostgreSQL Docker resources through fixed-argv commands, resolves read-only authority records, and journals immutable evidence. - Fixed-argv Docker observation with no shell; malformed output, nonzero exit, timeout, and identifier mismatch all map to UNVERIFIABLE with zero mutation. - Read-only tenant/project authority lookup emitting typed presence evidence; caller scope is never copied into the result. - Fsynced append-only JSONL journal that reloads immutable run and action records; recovery verbs delegate without provision or restore. The module stays unimported by the server, so no runtime behavior changes until the composition slice wires it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds a PostgreSQL Docker lifecycle adapter, authority-backed observation, recovery delegation, durable JSONL journaling, and quarantine-history activity preservation. ChangesPostgreSQL Docker lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalOwnershipAuthority
participant PostgresDockerLifecycleAdapter
participant Docker
participant JsonlLifecycleJournal
participant Provider
LocalOwnershipAuthority->>PostgresDockerLifecycleAdapter: lifecycle_records()
PostgresDockerLifecycleAdapter->>Docker: list and inspect container
Docker-->>PostgresDockerLifecycleAdapter: identity, labels, and state
PostgresDockerLifecycleAdapter->>JsonlLifecycleJournal: append lifecycle event
PostgresDockerLifecycleAdapter->>Provider: delegate lifecycle action
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odoo_forge_postgres_docker/lifecycle.py`:
- Around line 193-208: Update LifecycleJournal.append to acquire a process-safe
exclusive lock on the journal descriptor before any writes and retain it through
the complete write loop and fsync, releasing it during cleanup so each record
append is serialized. When O_CREAT creates the journal, also fsync
self.path.parent after the file creation to persist the directory entry.
- Around line 57-69: Validate timeout in
PostgresDockerLifecycleAdapter.__init__: reject non-positive or non-finite
values and any value exceeding the configured maximum finite limit before
assigning self._timeout. Reuse the existing timeout-limit configuration or
symbol if available, and add tests covering float("inf"), 0, and a value above
that limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7956615-9089-4a81-b3cd-4e494ee4cf76
📒 Files selected for processing (3)
src/odoo_forge_postgres_docker/authority.pysrc/odoo_forge_postgres_docker/lifecycle.pytests/adapters/test_postgres_docker_lifecycle.py
CodeQL py/side-effect-in-assert flagged the mock_calls expectation. The preceding assert also invoked adapter.cleanup() inside the assert itself, so under python -O the call would be stripped and the delegation expectation would no longer hold. Bind both results before asserting.
…able
Address two review findings on the lifecycle adapter.
The adapter accepted any timeout, so float("inf") let subprocess.run wait
without a deadline and defeated the bounded-timeout guarantee the Docker
boundary relies on. Reject non-finite, non-positive, and over-limit values
at construction, matching how LifecycleService already rejects a negative
max_cleanup_retries.
A journal record can span several os.write calls when a write is partial.
Under O_APPEND another process could interleave its own record between
those calls and corrupt both JSONL lines, so hold an exclusive lock across
the whole record through fsync. Syncing the file also left a newly created
journal's directory entry unrecoverable after a crash, losing the entire
audit trail; fsync the parent directory when the append creates the file.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odoo_forge_postgres_docker/lifecycle.py`:
- Around line 203-216: Update the journal-reading logic in events() to open the
journal through a descriptor, acquire fcntl.LOCK_SH before reading, and release
it afterward so reads cannot overlap the locked write sequence. Preserve
existing event parsing behavior, and add a concurrent reader/writer test
covering reads during append operations.
- Around line 219-222: Update the append flow around _fsync_directory and the
created condition so every successful append fsyncs self.path.parent before
returning, not only the process that created the file. Preserve the existing
journal locking and append behavior while removing the initialization-only
restriction.
In `@tests/adapters/test_postgres_docker_lifecycle.py`:
- Around line 146-188: Replace the syscall-order assertions in
test_journal_holds_an_exclusive_lock_from_first_write_through_fsync and
test_journal_fsyncs_parent_directory_only_when_it_creates_the_file with
observable journal behavior tests. Use concurrent append processes against
JsonlLifecycleJournal, then verify events() reloads complete, intact events;
move any crash-recovery scenario requiring integration infrastructure to a
separately marked, default-deselected integration test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 45a0651e-d00b-42b5-a079-309e79d0a67a
📒 Files selected for processing (2)
src/odoo_forge_postgres_docker/lifecycle.pytests/adapters/test_postgres_docker_lifecycle.py
…reads A non-creating append could return before the creating process synced the directory entry, leaving the audit trail unrecoverable after a crash. Sync on every append instead of only the first, and take a shared lock when reading so a concurrent append cannot hand back a truncated final line.
…istory Quarantine history carried the evidence digest and timestamp but not the baseline the expiration decision was made against, so the durable trail could not show why a resource was judged expired.
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
2 participants
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.
Closes #182
Summary
PR4 of the
SP-RESOURCE-LIFECYCLEchain (stacked tomain, after #177/#179/#180). Adds the live authority and journal boundary as a dormant adapter — the module is not imported by the server, so no runtime behavior changes until the composition slice (PR5) wires it.Implements tasks 4.1–4.4.
Changes
lifecycle.py): no shell, validated immutable identifiers, bounded timeout. Malformed output, nonzero exit, timeout, and identifier injection/mismatch all map toUNVERIFIABLEwith zero mutation.authority.py):LocalOwnershipAuthorityaccepts signed records carrying lifecycle metadata while preserving legacy records;lifecycle_records()stays read-only. The adapter coerces raw authority mappings into validated immutableLifecycleAuthorityRecordvalues and ignores missing/invalid/retired metadata fail-closed. Caller scope is never copied into the result.JsonlLifecycleJournal): fsynced append-only JSONL that reloads immutable run and action records. Recovery verbs delegate without provision or restore.Verification
Slice size: 398 changed lines across exactly 3 paths, within the 400-line review budget.
Scope boundary
Does not touch portfolio, wizard, data-environments, migrations, or backfill. No new flags. Rollback is reverting this commit; PR3 stays green and dormant.
Whole-change verification runs once after PR5 completes the remaining requirement (
Controlled operations and audit) and itsManual defaultscenario.Summary by CodeRabbit
New Features
Bug Fixes
Tests