feat: wire multi-workflow mode into main server#124
Conversation
When -database-dsn is provided, the server starts in multi-workflow mode: - Connects to PostgreSQL and runs migrations - Creates WorkflowEngineManager for managing concurrent engines - Mounts the REST API at /api/v1/ on the same HTTP server - Bootstraps admin user when -admin-email and -admin-password are set - -config flag becomes a seed for the initial workflow - Without -database-dsn, single-config mode is preserved unchanged Closes #113 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements multi-workflow mode for the server by wiring together PostgreSQL-backed storage, engine management, and REST API when the -database-dsn flag is provided. The implementation replaces a TODO placeholder with full multi-workflow functionality while maintaining backward compatibility with the existing single-config mode.
Changes:
- Implements
runMultiWorkflow()to connect to PostgreSQL, run migrations, bootstrap admin users, create an engine manager with all plugins, mount the REST API, and optionally seed workflows from a YAML config file - Adds graceful shutdown handling for multi-workflow components (engine manager, API server, admin engine, PG store)
- Minor formatting fixes from
go fmtin three existing files
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| cmd/server/main.go | Implements multi-workflow mode with PostgreSQL backend, engine manager, REST API mounting, admin bootstrapping, and workflow seeding functionality; adds graceful shutdown and cleanup logic for multi-workflow mode |
| plugins/api/plugin.go | Formatting fix: realigns default constructor assignments for better readability |
| module/query_handler_test.go | Formatting fix: removes trailing blank line |
| module/command_handler_test.go | Formatting fix: removes trailing blank line |
| record := &evstore.WorkflowRecord{ | ||
| ID: uuid.New(), | ||
| Name: name, | ||
| Slug: slug, | ||
| Description: "Seeded from " + configPath, | ||
| ConfigYAML: string(yamlBytes), | ||
| Version: 1, | ||
| Status: evstore.WorkflowStatusDraft, | ||
| CreatedAt: now, | ||
| UpdatedAt: now, | ||
| } |
There was a problem hiding this comment.
The WorkflowRecord is missing required fields: ProjectID, CreatedBy, and UpdatedBy. These fields are NOT NULL in the database schema (see store/migrations/004_workflows.sql lines 4, 12-13) and have foreign key constraints. Creating a workflow without these fields will fail with a database constraint violation.
The seed workflow needs to either:
- Create or reference a project and user to populate these fields, or
- Modify the database schema to make these fields nullable/optional for seed workflows
| existing, err := users.GetByEmail(ctx, email) | ||
| if err == nil && existing != nil { | ||
| logger.Info("Admin user already exists", "email", email) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Error handling is incomplete. If GetByEmail returns an error other than "not found" (e.g., database connection error, timeout), the error is silently ignored and the function proceeds to create the admin user. This could lead to:
- Attempting to create duplicate users if the error was transient
- Masking real database connectivity issues
- Unexpected behavior during bootstrap
Consider explicitly checking for "not found" errors and returning other errors to the caller.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
* Initial plan * fix: address all review comments for multi-workflow mode - Extract defaultEnginePlugins() to eliminate plugin list duplication - Fix bootstrapAdmin to check ErrNotFound explicitly; return admin UUID - Fix seedWorkflow: handle List error, improve slug via slugify(), populate required WorkflowRecord fields (ProjectID, CreatedBy, UpdatedBy) using new ensureSystemProject() helper - Change JWT secret logger.Warn -> logger.Error - Use srvErrCh to propagate API server failures to initiate graceful shutdown - Fix URL display using net.SplitHostPort for non-localhost bind addresses - Fix misleading comment about admin infrastructure setup Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Summary
When
-database-dsnis provided, the server now starts in multi-workflow mode instead of falling back to single-config mode with a TODO warning.What changed
cmd/server/main.go: Replaced the TODO placeholder with a fullrunMultiWorkflow()implementation:-database-dsnflagstore.Migrator-admin-emailand-admin-passwordare providedWorkflowEngineManagerwith all engine plugins registeredapi.NewRouter()with all PG-backed stores and JWT config/api/v1/on the primary HTTP server (*addr)-configas seed flag: When both-database-dsnand-configare provided, the YAML config is imported as the initial workflow into the database (idempotent — skips if slug already exists)Backward compatibility: Without
-database-dsn, the server behaves exactly as before (single-config mode, no changes)Graceful shutdown:
WorkflowEngineManager.StopAll(), the API HTTP server, and the admin engine are all shut down cleanly on SIGINT/SIGTERMMinor formatting fixes
go fmtapplied to 3 pre-existing formatting issues inmodule/andplugins/api/Closes #113