review: pod store split
This commit is contained in:
parent
9db8cdc7f8
commit
be5bbf3117
|
|
@ -0,0 +1,45 @@
|
|||
# Review R2: session-pod-state-boundary
|
||||
|
||||
Verdict: approve
|
||||
|
||||
## Conceptual summary
|
||||
|
||||
Commit `d2e8087` addresses the two prior blocking issues without reintroducing session-log scope authority. The restore path now reconciles missing/unreachable delegated children inside `Pod::restore_from_manifest` before returning a usable `Pod`, and `pod_registry::reclaim_delegated_scope` now removes the parent's delegated deny layer even when the child allocation is already absent.
|
||||
|
||||
## Findings
|
||||
|
||||
No blocking issues found in the reviewed delta.
|
||||
|
||||
The specific R1 blockers are resolved:
|
||||
|
||||
- `crates/pod-registry/src/mutate.rs:181-224` now removes matching `parent_alloc.scope_deny` entries unconditionally for delegated write rules, before optionally removing/reparenting an existing child allocation. This covers the missing-child allocation case and remains idempotent for absent deny entries.
|
||||
- `crates/pod-registry/src/mutate.rs:524-551` adds direct coverage for reclaiming parent deny when the child allocation is missing.
|
||||
- `crates/pod/src/pod.rs:4056` calls `pod.reconcile_restored_delegations().await?` from `Pod::restore_from_manifest` before returning the restored `Pod`; `restore_from_pod_metadata` still delegates through `restore_from_manifest`, so name-based restore gets the same enforcement.
|
||||
- `crates/pod/src/pod.rs:4061-4108` performs reachability checks, reclaims runtime lock state, updates in-memory scope, moves reclaimed children into `pod-store`, and queues a notification via `push_notify` before the restored Pod can be used for a model request.
|
||||
- Grep review did not find reintroduced `pod.scope` / effective-scope session authority. Remaining `LogEntry::Extension` uses are metrics/memory or generic replay handling, not Pod scope snapshots.
|
||||
|
||||
## Non-blocking notes
|
||||
|
||||
- `PodController::spawn` still has a secondary `SpawnedPodRegistry::load_from_pod_state_with_reclaim` reconciliation/notification path. With the constructor-level reconciliation, restored Pods should normally arrive there already cleaned up; the remaining path is still useful for registry construction and non-restore cases, but future cleanup could avoid duplicate conceptual ownership.
|
||||
- As noted in R1, `pod-store::PodMetadataStore::update_by_name` remains read-modify-write internally. That is acceptable for this ticket's filesystem backend and covered field-preservation semantics, but it is not a transactional concurrency primitive.
|
||||
|
||||
## Validation run
|
||||
|
||||
Inspected the `d2e8087` delta and the current relevant files:
|
||||
|
||||
- `crates/pod-registry/src/mutate.rs`
|
||||
- `crates/pod/src/pod.rs`
|
||||
- `crates/pod/src/controller.rs`
|
||||
- `crates/pod/tests/pod_comm_tools_test.rs`
|
||||
- `crates/pod/tests/restore_test.rs`
|
||||
|
||||
Commands run from `/home/hare/Projects/insomnia/.worktree/session-pod-state-boundary`:
|
||||
|
||||
```text
|
||||
cargo test -p pod-registry reclaim_delegated_scope
|
||||
cargo test -p pod --test pod_comm_tools_test load_from_pod_state_reclaims_missing_child_scope_and_records_history
|
||||
cargo test -p pod --test restore_test
|
||||
git diff --check HEAD~3..HEAD
|
||||
```
|
||||
|
||||
All commands passed.
|
||||
|
|
@ -0,0 +1,51 @@
|
|||
# Review: session-pod-state-boundary
|
||||
|
||||
Verdict: blocking
|
||||
|
||||
## Conceptual summary
|
||||
|
||||
The crate split is mostly in the intended shape: `pod-store` now owns the Pod metadata types, validation, trait, and filesystem-backed store; `session-store` no longer exports Pod metadata; normal construction uses separate session and Pod-state roots; TUI discovery goes through `PodMetadataStore`; and the old `{sessions_root}/pods` layout is not used as an authority.
|
||||
|
||||
The late scope-authority change is only partially satisfied. `pod.scope` session-log authority appears removed, and restore derives initial deny rules from `pod-store` `spawned_children`, but the restore reconciliation path has a runtime-state hole for missing children. That violates the ticket's requirement to reclaim delegated scope for missing/stopped/unreachable children before resumed model use.
|
||||
|
||||
## Blocking issues
|
||||
|
||||
1. **Restore reconciliation leaves stale runtime deny rules when the missing child has no live lock allocation.**
|
||||
|
||||
- `crates/pod/src/pod.rs:4597-4616` derives restored parent scope by adding deny rules for every outstanding `metadata.spawned_children` write delegation.
|
||||
- `crates/pod/src/pod.rs:3960-3967` installs the restored top-level Pod into the runtime registry with those deny rules.
|
||||
- `crates/pod/src/spawn/registry.rs:131-159` then detects unreachable children and calls `pod_registry::reclaim_delegated_scope(...)` via `reclaim_record(...)`.
|
||||
- But `crates/pod-registry/src/mutate.rs:202-215` removes the parent's `scope_deny` entries only when the child allocation currently exists (`if child_exists { ... remove parent deny ... }`). If the child is missing from the live lock registry—which is exactly one of the required restore-reconciliation cases—the restored parent allocation keeps the delegated write deny in `pods.json` even though `pod-store` has moved the child to `reclaimed_children` and the in-memory `SharedScope` has removed the deny.
|
||||
|
||||
This makes runtime lock state diverge from `pod-store`/in-memory scope after restore. Future spawn/delegation checks that consult the lock allocation will still see the reclaimed write scope as denied. The existing test `load_from_pod_state_reclaims_pruned_child_scope_and_records_history` only covers the case where a stale child allocation exists, so it misses the required "missing child" case. The reclaim operation needs to remove the parent's matching delegated deny independently of whether the child allocation still exists, while still being idempotent.
|
||||
|
||||
2. **Restore reconciliation is not structurally part of `Pod::restore_*`; it depends on `PodController::spawn`.**
|
||||
|
||||
`Pod::restore_from_manifest` / `Pod::restore_from_pod_metadata` construct a restored Pod with scope derived from outstanding `pod-store` delegations, but the pruning/reclaim and notification are performed later in `PodController::spawn` (`crates/pod/src/controller.rs:160-174`). Normal CLI startup goes through the controller, but the public restore constructors can return a Pod that can be used directly without the required reconciliation notification. If direct use is intentionally unsupported, the API should make that boundary explicit; otherwise the reconciliation should be moved into or made mandatory by the restore path so "before any model request observes resumed state" is enforced by construction.
|
||||
|
||||
## Non-blocking notes
|
||||
|
||||
- `pod-store::PodMetadataStore::update_by_name` is still implemented as a read-modify-write default method (`crates/pod-store/src/lib.rs:133-145`). The call sites no longer manually preserve unrelated fields, and the field-specific methods are test-covered, so this is acceptable for the current filesystem backend. If concurrent metadata writers become possible, this API will need a stronger atomicity story.
|
||||
- `pod-store` depends on `session-store` for `SessionId`/`SegmentId`. The ticket allowed a narrow dependency for IDs; the crate docs keep the replay boundary clear.
|
||||
- `crates/pod/src/main.rs:230-235` derives the Pod-state root from `paths::data_dir()` rather than from custom `--store` when data dir is available. That keeps the new top-level layout, but users of a custom session store do not have a matching custom Pod store flag yet.
|
||||
|
||||
## Validation run
|
||||
|
||||
Read/inspected:
|
||||
|
||||
- Ticket: `work-items/open/20260529-205844-session-pod-state-boundary/item.md`
|
||||
- Implementation diff/stat for commits `2117381..e10b4ad`
|
||||
- Key files: `crates/pod-store/src/lib.rs`, `crates/session-store/src/lib.rs`, `crates/session-store/src/fs_store.rs`, `crates/pod/src/pod.rs`, `crates/pod/src/controller.rs`, `crates/pod/src/spawn/registry.rs`, `crates/pod-registry/src/mutate.rs`, `crates/tui/src/pod_list.rs`, `crates/pod/src/main.rs`
|
||||
|
||||
Commands run from `/home/hare/Projects/insomnia/.worktree/session-pod-state-boundary`:
|
||||
|
||||
```text
|
||||
cargo test -p pod-store
|
||||
cargo test -p pod --test pod_comm_tools_test load_from_pod_state_reclaims_pruned_child_scope_and_records_history
|
||||
cargo test -p session-store
|
||||
cargo test -p tui pod_list
|
||||
git diff --check HEAD~2..HEAD
|
||||
./tickets.sh doctor
|
||||
```
|
||||
|
||||
All commands passed. Full command output was saved by the shell tool at `/run/user/1000/insomnia/review-session-pod-state-boundary/bash-output/bash-uDx0E4.log`.
|
||||
|
|
@ -5,3 +5,15 @@
|
|||
Created by tickets.sh create.
|
||||
|
||||
---
|
||||
|
||||
<!-- event: review author: review-session-pod-state-boundary at: 2026-05-29T23:04:00Z -->
|
||||
|
||||
## External review
|
||||
|
||||
Initial review found blocking issues in restore reconciliation: missing child allocations left stale runtime deny entries, and reconciliation was not enforced at the public restore boundary. The coder fixed these in commit `d2e8087`; second review approved the implementation.
|
||||
|
||||
Artifacts:
|
||||
- `artifacts/review.md`
|
||||
- `artifacts/review-r2.md`
|
||||
|
||||
---
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user