Repository navigation
feat(reborn): add process lifecycle substrate - #3017
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several core infrastructure crates for the IronClaw Reborn project, covering approval resolution, authorization, event logging, extension management, scoped filesystem access, and process lifecycle tracking. Feedback identifies critical performance bottlenecks in the database-backed filesystem and authorization service due to inefficient record scanning. Reliability issues were noted regarding the process manager's reliance on volatile in-memory state for durable resource reservations and the use of synchronous I/O in async contexts. Additionally, better error reporting for background task failures was recommended to improve system observability.
| } | ||
|
|
||
| async fn all_paths(&self) -> Result<Vec<(VirtualPath, u64, FileType)>, FilesystemError> { | ||
| let client = self.client().await?; |
There was a problem hiding this comment.
The all_paths helper (and its counterpart in LibSqlRootFilesystem at line 1397) performs a full table scan of root_filesystem_entries. This is used by list_dir and stat to resolve directory contents and metadata. As the number of files grows, this will lead to severe performance degradation and high memory usage. The database queries should be optimized to fetch only relevant rows using prefix matching (e.g., WHERE path LIKE '/dir/%') to move the logic to the database layer and prevent performance bottlenecks.
References
- Use targeted database queries to fetch specific records instead of loading all records and filtering in the application to prevent performance bottlenecks.
- When application logic becomes complex or inefficient, consider moving it to the database layer (e.g., a dedicated SQL query) to improve performance.
| .ok_or(ProcessError::ResourceReservationNotOwned { | ||
| process_id, | ||
| reservation_id: record_reservation_id, | ||
| })?; | ||
| if Some(reservation_id) != record_reservation_id { | ||
| self.owned_reservations |
There was a problem hiding this comment.
ResourceManagedProcessStore relies on an in-memory owned_reservations map to track active resource reservations. This map is lost when the application restarts. If the underlying ProcessStore is durable (like FilesystemProcessStore), processes that were running before the restart will still exist in the store with a valid resource_reservation_id, but any attempt to complete, fail, or kill them will result in a ResourceReservationNotOwned error because the entry is missing from the in-memory map. The store should trust the resource_reservation_id present in the persisted ProcessRecord as a fallback.
let reservation_id = self
.owned_reservations
.lock()
.unwrap()
.remove(&ProcessKey::new(scope, process_id))
.or(record_reservation_id)
.ok_or(ProcessError::ResourceReservationNotOwned {
process_id,
reservation_id: record_reservation_id,
})?;| async fn active_leases_for_context(&self, context: &ExecutionContext) -> Vec<CapabilityLease> { | ||
| self.leases_for_scope(&context.resource_scope) | ||
| .await | ||
| .into_iter() | ||
| .filter(|lease| lease_is_authorizing(lease, context)) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
active_leases_for_context is implemented by calling leases_for_scope, which scans all historical invocation directories for a user to find active leases. This operation is performed during every capability dispatch. As the number of invocations grows, this will become a major performance bottleneck. Since lease_is_authorizing only accepts leases matching the current invocation_id, the implementation should directly access the specific invocation's directory instead of loading all records and filtering in the application.
async fn active_leases_for_context(&self, context: &ExecutionContext) -> Vec<CapabilityLease> {
let Ok(root) = lease_invocation_root(&context.resource_scope) else {
return Vec::new();
};
let Ok(files) = self.list_lease_files(&root).await else {
return Vec::new();
};
let mut leases = Vec::new();
for path in files {
if let Ok(lease) = self.read_lease_file(&path).await {
if lease_is_authorizing(&lease, context) {
leases.push(lease);
}
}
}
leases
}References
- Use targeted database queries or direct resource access to fetch specific records instead of loading all records and filtering in the application to prevent performance bottlenecks.
|
|
||
| #[async_trait] | ||
| impl RootFilesystem for LocalFilesystem { | ||
| async fn read_file(&self, path: &VirtualPath) -> Result<Vec<u8>, FilesystemError> { |
There was a problem hiding this comment.
LocalFilesystem implements the RootFilesystem trait using synchronous std::fs operations (e.g., std::fs::read, std::fs::write, std::fs::read_dir). Since these methods are called within an async context, they will block the Tokio executor threads. This can lead to thread pool starvation and poor performance under high I/O load. These operations should be performed using tokio::fs or wrapped in tokio::task::spawn_blocking.
| if let Ok(record) = store.complete(&scope, process_id).await | ||
| && let Some(result_store) = &result_store | ||
| { | ||
| let _ = result_store | ||
| .complete(&record.scope, record.process_id, result.output) | ||
| .await; | ||
| } | ||
| } | ||
| Err(error) => { | ||
| if let Ok(record) = store.fail(&scope, process_id, error.kind).await |
There was a problem hiding this comment.
The background task spawned in spawn silently ignores failures when updating the process status in the ProcessStore. If the store update fails, the process will remain in the Running state indefinitely. These failures should be surfaced via tracing::warn! to ensure system observability, and if spawn_blocking is used, the JoinError should be logged to distinguish between panics and cancellations.
References
- Errors in background persistence or status updates should be surfaced via tracing::warn! to ensure reliability and observability.
- When handling errors from tokio::task::spawn_blocking, log the JoinError to capture debugging information and distinguish between panics and cancellations.
ce8173b to
d471de7
Compare
|
Addressed all 9 findings from the paranoid review in
Diff: 6 files, +342 / −49. |
Carves the 1953-line lib.rs into 7 focused modules so each file fits
in one tool-read and an agent can grep-jump straight to the right
concern:
- types.rs data types, errors, traits, shared helpers
- cancellation.rs token + registry
- host.rs ProcessHost + Subscription
- memory_store.rs InMemory{Store,ResultStore}
- filesystem_store.rs Filesystem* + path/serde helpers
- wrappers.rs Eventing + ResourceManaged decorators
- services.rs ProcessServices + BackgroundProcessManager
lib.rs is now a thin module-decl + re-export hub so the public API
surface is visible at a glance.
No behavior change. Verified:
- cargo fmt
- cargo check -p ironclaw_processes
- cargo clippy -p ironclaw_processes --all-targets -- -D warnings
- cargo test -p ironclaw_processes (43/43 pass)
- M1 + L4: BackgroundFailure/Stage + with_error_handler so swallowed store/result-store errors in spawned tasks are now observable; covered by new FailingProcessResultStore test. - M2: invert write order in BackgroundProcessManager::spawn (result store first, then lifecycle status) so terminal status implies result is persisted. Added pre-check on store.get to skip writes when the process was already terminalized externally (e.g. by host.kill), avoiding overwriting a kill record. - M3: drop blanket impl<T: ProcessStore> ProcessManager for T so raw stores no longer impersonate a manager. - M4: doc note on spawn re detached-task orphan risk + TODO for startup reconciliation. - L1: ReservationDropGuard RAII guard around inner.start in ResourceManagedProcessStore::start; reservations released on panic. - L2: doc orphan-blob expectation on FilesystemProcessResultStore ::complete. - L3: doc single-instance invariant on FilesystemProcessStore::new and from_arc. - N1: doc the notify-then-check ordering in ProcessCancellationToken::cancelled.
[skip-regression-check] The actual fix commit (fc96d65) added a new integration test in crates/ironclaw_processes/tests/process_store_contract.rs, but the regression-check script only matches `^tests/` (repo-root) and misses crate-level integration tests. Skip explicitly via marker.
2506385 to
be69726
Compare
Refs: #3145, #3080, #3017, #3087. Decisions: host-runtime now wraps process stores with ProcessObligationLifecycleStore so spawn-phase resource reservations are reconciled on success or released on failure/kill, and staged network/secret handoffs are discarded at terminal lifecycle. Process-start failure remains CapabilityHost abort-owned. Files changed: ironclaw_host_runtime lib exports, obligations lifecycle/store cleanup, HostRuntimeServices process graph wiring, host_runtime_services_contract tests. Notes: full host-runtime tests need CARGO_BUILD_JOBS=1 in this environment to avoid linker OOM.
* RALPH: complete issue 3145 background obligation lifecycle Refs: #3145, #3080, #3017, #3087. Decisions: host-runtime now wraps process stores with ProcessObligationLifecycleStore so spawn-phase resource reservations are reconciled on success or released on failure/kill, and staged network/secret handoffs are discarded at terminal lifecycle. Process-start failure remains CapabilityHost abort-owned. Files changed: ironclaw_host_runtime lib exports, obligations lifecycle/store cleanup, HostRuntimeServices process graph wiring, host_runtime_services_contract tests. Notes: full host-runtime tests need CARGO_BUILD_JOBS=1 in this environment to avoid linker OOM. * Fix process obligation cleanup lifecycle * Fix stale reservation lifecycle cleanup * Fix background obligation cleanup failures * fix(reborn): harden process obligation cleanup * fix(host-runtime): preserve cancel side effects on cleanup failure * fix(reborn): enforce single active process handoff * fix: address review findings (iteration 1)
Carves out the Reborn process lifecycle substrate in ironclaw_processes with scoped process/result stores, ProcessHost, BackgroundProcessManager, cooperative cancellation, event/resource decorators, and contract tests.
…#3161) * RALPH: complete issue 3145 background obligation lifecycle Refs: nearai#3145, nearai#3080, nearai#3017, nearai#3087. Decisions: host-runtime now wraps process stores with ProcessObligationLifecycleStore so spawn-phase resource reservations are reconciled on success or released on failure/kill, and staged network/secret handoffs are discarded at terminal lifecycle. Process-start failure remains CapabilityHost abort-owned. Files changed: ironclaw_host_runtime lib exports, obligations lifecycle/store cleanup, HostRuntimeServices process graph wiring, host_runtime_services_contract tests. Notes: full host-runtime tests need CARGO_BUILD_JOBS=1 in this environment to avoid linker OOM. * Fix process obligation cleanup lifecycle * Fix stale reservation lifecycle cleanup * Fix background obligation cleanup failures * fix(reborn): harden process obligation cleanup * fix(host-runtime): preserve cancel side effects on cleanup failure * fix(reborn): enforce single active process handoff * fix: address review findings (iteration 1)
Summary
Carves the process lifecycle/result/output substrate from the Reborn stack.
Adds
crates/ironclaw_processeswith:ProcessStore,ProcessResultStore,ProcessManager,ProcessHost, andProcessExecutorBackgroundProcessManagerStacking note
This PR is draft/stacked because it is intended to land after the current Reborn substrate/control stack:
After those merge into
reborn-integration, rebase this branch so the diff narrows to only:Scope boundary
This PR intentionally does not include:
ironclaw_capabilities/CapabilityHostExposure checklist
TDD note
Copied the process contract tests first against a RED stub and confirmed
cargo test -p ironclaw_processesfailed due missing process types before porting the implementation.Verification
Passed:
Boundary grep returned no forbidden normal Reborn dependencies.
Refs #2987.