Conversation
|
This review is now based on the post-419 architecture (per MAP Commands Spec), where MAP command ingress is moving to the Runtime layer and Review FindingsA lot of work went into PR 418. It delivers a substantial amount of new recovery/persistence infrastructure, including a recovery store implementation and SQLite-backed persistence built from scratch, and it arrived faster than I was expecting. The bulk of the code structure and supporting logic seems good to me. It's also pretty cool to see this level of resilience being added to Holons Core at this early stage! There are a few concerns I wanted to note and some changes I’m requesting. Dev-Mode EnhancementsThe introduction of dev-mode changes initially caught me off guard, since they are outside the core scope of Issue 412. In general, I would prefer not to expand a feature PR beyond the specific issue/spec it is meant to implement, especially when the additional work is not tied to a separately defined issue or spec. That said, at this point I do not think the dev-mode changes are a strong reason to block or split the PR. They are outside the core Issue 412 scope, but they still appear useful under the new architecture (despite the narrowed scope of the holochain receptor), and they are not a major source of the 418/419 merge-conflict surface. Architectural ConcernsCoupling of Recovery Persistence to "Provider"Recovery persistence appears to be coupled to provider setup, especially the Holochain provider path. That seems architecturally inverted. The need for snapshot persistence and crash recovery arises from IntegrationHub’s own transaction-staging responsibilities, not from any particular external provider. It is not a Holochain-specific concern. Recovery should therefore be owned at the transaction/runtime layer and backed by a persistence boundary, rather than being provisioned through provider-specific setup. SQLite Not Modeled as a ReceptorI expected recovery persistence to be introduced behind a receptor abstraction, consistent with the deployment architecture’s treatment of IntegrationHub/environment touch points. PR 418 instead embeds SQLite directly inside the host recovery subsystem. I can see the rationale for treating the recovery store as an internal persistence mechanism inside Since this introduces a new persistent local storage dependency (SQLite) and writes to the host filesystem, I think there is a strong argument that it belongs behind a receptor-style abstraction rather than being embedded directly in the recovery subsystem. At minimum, this highlights the need for clearer architectural guidance on what does and does not count as a receptor within the IntegrationHub. Recovery / Persistence Store ReviewTwo logic issues need to be fixed before merge.
RecommendationBefore resolving the remaining 418/419 merge conflicts, I think the recovery/persistence store should first be decoupled from provider-specific setup and elevated to its own receptor/provider. As noted above, the need for recovery arises from IntegrationHub’s own transaction-staging responsibilities. It is not a Holochain concern, and it is not logically owned by any particular provider. At the same time, recovery persistence introduces its own external boundary: a SQLite-backed store on the host filesystem. Given the way providers/receptors are used elsewhere in the architecture, that makes recovery persistence a better fit for its own receptor/provider abstraction than as a configurable option attached to other providers. My recommendation would be:
That seems like the cleanest path to reduce conflict surface, avoid locking in the wrong ownership model, and merge 418 without regression while preserving the architectural direction introduced by 419. Recovery / Persistence Store ReviewTwo logic issues need to be fixed before merge.
RecommendationBefore resolving the remaining 418/419 merge conflicts, I think the recovery/persistence store should first be decoupled from provider-specific setup and elevated to its own receptor/provider. The need for recovery arises from IntegrationHub’s own transaction-staging responsibilities. It is not a Holochain concern, and it is not logically owned by any particular provider. At the same time, recovery persistence introduces its own external boundary: a SQLite-backed store on the host filesystem. Given the way providers/receptors are used elsewhere in the architecture, that makes recovery persistence a better fit for its own receptor/provider abstraction than as a configurable option attached to other providers. My recommendation would be:
That seems like the cleanest path to reduce conflict surface, avoid locking in the wrong ownership model, and merge 418 without regression while preserving the architectural direction introduced by 419. |
|
Without looking at the changes and merge conflicts.. just going to register my first reaction before diving deeper into the perspective given. Receptors are by my initial design, (with the exception of the local receptor which we haven't built yet ) are holonic network storage options. Holochain being one of them. The recovery snapshot option is a core feature that is available to every provider. Providers being the storage configuration build that encodes into a BaseReceptor later to instanciate a real receptor (builder pattern) The recovery host crate encapsulates the storage implementation logic and vendor.. these can be swapped out anytime independently. For now we have chosen SQLite. SQLite is a storage crate .. it is not a receptor, there is no conceptual match there. The architecture is both UI and Receptor driven.. configuration, database creation and other startup pre-requisites are performed by the appbuilder setup .. recovery/snapshot database is optional per receptor type.. if its configured, the snapshot database is created on startup for that receptor. this is not a runtime operation.. its core startup operation including recovery from crash which restores all receptors with their respective holon cache of snaphots. I don't really agree with or perhaps fully understand this statement: With regards to the other two additional features that came with this PR namely logging and dev-mode , I agree they should have been separate issues.. with my frustration of needing a better build test and log experience, I bundled them in. I have made a separate branch off main to merge in just dev-mode+logging |
1474ba2 to
33cb383
Compare
### Please note this is branched off 421 not main.. merge 422 first before 418 |
|
Yeah first reaction to re-write later.. As I said before the recovery snapshots are available to all receptors not just holochain and bootstraped at startup . I have a notion of these receptors as network storage for actually writing holon data, all with git versioning ability. whereas other storage options in the hub design are more read only... And don't need transaction recovery I also see an architecture around the local receptor being the main holon storage point . And holochain, GitHub, radicle and other remote provider options being secondary sync points .. If everything goes through the local receptor it might be the case that we only need one transaction snapshot recovery database and can as you suggest move this out of the receptor configuration options With a 20 second startup time using the holochain provider in normal mode...I don't see that as practical option for the home/ root space. |
|
My goal is to get explicit architectural alignment so this PR can move to merge-ready. I know your previous note was an initial reaction; this follow-up is to ask for a clear agree/disagree on each point below.
If you disagree with any item, please call out the specific item number and your alternative wording so we can converge quickly. |
|
Here is what I see as the implementation implications of the architectural points listed in my previous comment. I'm providing them here:
Concrete Code Impact (Assuming Architectural Alignment)1. Model recovery persistence as its own provider-type (not a per-provider capability flag)Move from “snapshot recovery as an option on unrelated providers” to “recovery persistence as its own provider configuration type.” Primary files:
Expected outcome:
2. Remove conceptual coupling of recovery store to Holochain receptor setupAny wiring that makes recovery appear “for Holochain” should be removed. Primary files:
Expected outcome:
3. Keep provisioning at app-builder setup (not runtime command path), but via recovery provider pathProvision recovery DB once during startup/setup, driven by recovery provider config. Primary files:
Expected outcome:
4. Stop treating recovery as receptor-local payload on primary storage receptorsIf Primary files:
Expected outcome:
5. Keep
|
33cb383 to
58df61c
Compare
|
after the conversation yesterday i think we mostly agree on the way forward.. i have rebased off main and a few things were broken and added last minute with the 422 merge that conflicted with this PR ... i have reverted them as they regress my work for my app_builder cleanup commit. i will comment here more once i finish this |

### Please note this is branched off 421 not main.. merge 422 first before 418
TODO in follow-up issue:
A lot of new code was added and 50 plus existing files updated.
most requirements are met .. i would like to merge part 1 and create a refinements issue
for further final touches
Definition of Done (Section 7)
disable_undometadata behaviorsnapshot_afterpolicy hook (mock acceptable)recovery_session,recovery_checkpoint, indexes)export_staged/transient_holons→SerializableHolonPool)Gaps to Close
snapshot_afterpolicy hookpersist A→undo A→persist B→ assertcan_redo() == falsesnapshot.staged_holons/transient_holonsmatch originalrecover_latest()→ assert content matches