405 map commands phase 22 implement integrationhub map command dispatcher#423
Conversation
evomimic
left a comment
There was a problem hiding this comment.
Findings
1. [P1] Finalized transactions are never removed from RuntimeSession
File: [host/crates/map_commands/src/dispatch/transaction_dispatch.rs]
Commit and LoadHolons both finalize the transaction context, but this dispatcher no longer calls RuntimeSession::remove_transaction(). That leaves committed transactions in active_transactions indefinitely, which both leaks the session map over time and changes post-commit behavior from “invalid/removed tx” to “still bound but closed” on later requests. The Phase 2.2 spec explicitly calls out session cleanup when transactions are finalized, and the helper is still present in .../dispatch/runtime_session.rs so this looks like an accidental regression from the previous implementation.
However, this issue brought to light a bigger issue that I would like to resolve in this PR (although it does constitute a scope expansion). See additional comment for details.
2. [P2] Manual open-transaction precheck masks richer lifecycle errors
File: host/crates/map_commands/src/dispatch/runtime.rs
The new requires_open_tx branch returns TransactionNotOpen for every non-open state before the shared transaction policy gates run. That flattens committed transactions into a generic error even though the core lifecycle matrix distinguishes TransactionAlreadyCommitted from TransactionNotOpen in .../core_shared_objects/transactions/transaction_context.rs. Because this check runs before ensure_host_mutation_entry_allowed() / begin_host_commit_ingress_guard(), mutating commands on a committed transaction now lose the more specific, deterministic error the rest of the stack uses.
Verification
I confirmed happ and host both build clean. All sweetests pass. npm start yields expected runtime behavior.
PR Note: Transaction Commit Semantics, Lifecycle Ownership, and Deferred CleanupAs noted in the review findings, this PR brought to light a few nuances that we should clean up while we are establishing a strong foundation to move forward from. SummaryThis PR updates transaction commit behavior to support multiple concurrent transactions and preserve short-term post-commit reference resolution. The key changes are:
Changes Introduced1. Commit no longer clears holon poolsOn successful commit, the following are no longer cleared:
Rationale Immediate teardown invalidates:
With concurrent transactions, this is no longer acceptable. Retaining these pools allows outstanding references to remain resolvable briefly after commit. 2. Commit is orchestrated by RuntimeSession (not TransactionContext alone)
pub enum TransactionAction {
Commit,
...
}However, introduce a Execution flow
Rationale Commit has system-level effects, including:
Therefore:
At the same time:
3. Commit remains a Transaction-scoped command (intentional decision)We explicitly retain Rationale
Design Principle
So:
4. TransactionContext is moved to an archive on commitOn successful commit:
Rationale This ensures:
5. Cleanup is deferred (out of scope)This PR does not implement resource reclamation. Future work will include:
Updated Transaction Lifecycle ModelTransactions now follow a three-stage lifecycle:
Implementation Notes
Design IntentThis change establishes:
It ensures that:
Lifecycle is owned by |
|
requested changes have been implemented:
Note on On separating |
|
The change so that RuntimeSession now owns commit orchestration via Note on clear_stageI'm confused by your note on clear_stage. The request was to change Note on separating map_commands into map_runtime + map_commandsThanks for the analysis. I agree the situation is a bit more complicated than I initially thought. The main thing I'm trying to ensure is that we not have wire types creep into command signatures. See follow-on comment for a more complete proposal. |
Follow-on proposal: restructure
|
- Remove clear_stage() from TransactionContext::commit(); nursery pools are no longer cleared on commit - Split CommandDescriptor::read_only() into transaction_read_only() (requires open tx) and holon_read_only() (allows committed tx) so all TransactionActions reject committed transactions while HolonAction reads remain accessible - Return TransactionAlreadyCommitted (not TransactionNotOpen) when commands target a committed transaction LookupFacade methods with assert_allowed for defense in depth - Update RuntimeSession::get_transaction to resolve from both active and archived pools - Rename dispatch files/functions to handler terminology per architect review (dispatch_inner → execute_bound_command, dispatch_command → route_command, dispatch → handle_ipc)
…port types are kept out of execution signatures
…ed current architecture
Implement Runtime command dispatcher with centralized lifecycle enforcement
Closes #405
Summary
CommandDescriptorwithMutationClassificationto declare lifecycle requirements (requires_open_tx,requires_commit_guard, mutation classification) per command variantRuntime::dispatch_inner(): open-transaction validation, commit guard acquisition, and host mutation entry checks — all descriptor-drivenNotImplemented)MapResult/MapResultWirevariants; addValue(BaseValue)and rename for clarityRequestOptions(gesture_id, gesture_label, snapshot_after) andGestureIdtoMapIpcRequestRequestIdto wrapMapIntegerinstead ofu64context: Arc<TransactionContext>toHolonCommandso holon writes go through mutation entry checksDeferred to follow-up issue (blocked on #412)
RuntimeSessionstoresClientSessionsnapshot_afterpolicy executed byRuntime::dispatch_inner()for mutating commandsTransactionAction::UndoandRedovariants implemented and wired