Skip to content

428 app_builder rework#429

Open
nphias wants to merge 1 commit intomainfrom
428-app-builder-refactor
Open

428 app_builder rework#429
nphias wants to merge 1 commit intomainfrom
428-app-builder-refactor

Conversation

@nphias
Copy link
Collaborator

@nphias nphias commented Mar 26, 2026

NOTE: This work lays the foundation for branch 412 (PR 418) which stacks on this branch

also this work refactored changes made by PR 419 to the app_builder
the init_runtime logic was moved to a new file under host/conductora/src/runtime/init_runtime.rs
using RuntimeInitiatorState in the app_builder

also refactored / reverted were last minute changes made to main for PR 422 that conflicted with this work:
9393fad

The App_builder is more coherent and orchestrates via the

  • ConfigManager
  • SetupManager

with some abstraction work on app_builder, I was able to:

  • remove any vendor specific code
  • remove string matches
  • remove business logic that belongs higher up the stack
  • remove default configuration logic that served no useful purpose

alongside this clean up of the app_builder other supporting files were added for better abstraction

  • a provider registry and catalog for better provider handling
  • reverse reference removed in the config files
  • better placement of setup code
  • clean up of storage.json - removed defaults .. fail startup on no config found
  • other helper files

and many other small improvements

@nphias nphias linked an issue Mar 26, 2026 that may be closed by this pull request
@nphias nphias changed the title app_builder rework 428 app_builder rework Mar 26, 2026
Copy link
Owner

@evomimic evomimic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Findings

Overall

This PR looks directionally good. The provider-oriented refactor seems like a solid step toward modular startup/setup and better support for upcoming multi-provider and recovery work.

The issues I found are both in startup failure handling rather than in the overall structure of the refactor. In other words, the main concern is not the new architecture itself, but that the setup orchestration currently has a couple of control-flow bugs around provider/plugin failure paths.

1. Continuing startup after provider setup failure

Severity: medium-high

run_complete_setup() continues startup after provider setup failure, which can leave the app in a partially initialized state. In setup_manager.rs, apply_setups() errors are logged but not returned, so the code still loads receptor configs, calls init_from_state(), and creates the window. That means a failed provider setup can still produce a launched app with missing receptors/runtime state and a misleading “setup completed successfully” path. This looks like a behavioral regression from the old flow as well, since provider setup is still treated as foundational work.

The likely fix is small to moderate: return early from run_complete_setup() when apply_setups() fails, and make sure the failure is surfaced consistently instead of just logged.

2. Waiting only for setup-completed and ignoring setup-failed

Severity: high for failure handling, but probably low-frequency in normal use

When it happens, it can hang startup indefinitely, which is a poor user experience and makes debugging harder.

Async provider startup waits only for ...setup-completed and ignores ...setup-failed, which can deadlock startup on plugin failure. In setup_manager.rs, the pending-event gate listens only for the success event, but the holochain plugin explicitly emits holochain://setup-failed on launch failure in lib.rs. If that failure path happens, run_complete_setup() never starts, so the app can stall indefinitely without a clear startup failure path.

The fix should also be fairly contained: either listen for both success and failure events, or redesign the gating so a failure event trips a terminal startup-failed path instead of leaving the pending set unresolved.

@evomimic
Copy link
Owner

What Does this PR Do?

The PR description is not very explicit about the primary purpose of this PR. I asked codex to take a stab at inferring its main thrust. @nphias : please correct any errors or omissions.

This PR is an architectural refactor of host startup and provider integration, making provider selection, setup, and runtime initialization more modular and more extensible for upcoming recovery and multi-provider work.

Concretely it,

  • replaces the old StorageConfig/app_config loading path with StorageManager and typed provider configs under providers
  • introduces a ProviderRegistry + ProviderIntegration abstraction so plugin application, setup, and window creation are selected by provider type instead of hard-coded match logic in app_builder.rs
  • moves the setup orchestration into setup_manager.rs, including runtime-provider selection, async setup event gating, provider setup fan-out, receptor loading, runtime init, and window creation
  • extracts MAP runtime initialization into init_runtime.rs, replacing the old app-builder-owned conductor-client path with a provider-supplied DanceInitiator
  • reorganizes provider-specific Holochain and Local setup code under setup/providers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

App builder refactor

2 participants