feat: Refactor agent and hub sync for better readability#208
Open
maximbigler wants to merge 43 commits into
Open
feat: Refactor agent and hub sync for better readability#208maximbigler wants to merge 43 commits into
maximbigler wants to merge 43 commits into
Conversation
6a3927e to
2b3b396
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
8 tasks
This comment has been minimized.
This comment has been minimized.
alex289
reviewed
Jun 27, 2026
alex289
reviewed
Jun 29, 2026
alex289
reviewed
Jun 29, 2026
alex289
reviewed
Jun 29, 2026
alex289
reviewed
Jun 29, 2026
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
alex289
reviewed
Jun 29, 2026
alex289
reviewed
Jun 29, 2026
…r.go Co-authored-by: Alex <me@alexanderkonietzko.com>
…rcaCD/orca-cd into refactor/deploy-state-machine
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Alex <me@alexanderkonietzko.com>
…rcaCD/orca-cd into refactor/deploy-state-machine
alex289
approved these changes
Jul 2, 2026
timokoessler
reviewed
Jul 3, 2026
| ALTER TABLE applications ADD COLUMN name_hash TEXT NOT NULL DEFAULT ''; | ||
|
|
||
| -- Partial index excludes legacy rows (empty hash, pre-backfill) so they don't | ||
| -- collide on (agent_id, ''). Names were globally unique under the old rule, so |
Member
There was a problem hiding this comment.
We did not check this before no?
timokoessler
reviewed
Jul 3, 2026
| // nonce, so we match on the name_hash blind index instead of decrypting every | ||
| // row. excludeID skips a record (the one being updated). Uniqueness is scoped per | ||
| // agent — the same name may exist on different agents. | ||
| func applicationNameTaken(ctx context.Context, name, agentID, excludeID string) (bool, error) { |
Member
There was a problem hiding this comment.
In theory we could just run the insert and catch duplicate key error like done elsewhere?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of change
Description
Fixes #172
Fixes #211
Refactors the repository-sync and application-deploy flow in
internal/hub/applicationsto make it readable and to remove duplicated/divergent logic. No new feature; behavior is preserved except for the intentional changes called out below.Application-level deploy state machine
updateApplicationStatusas the single chokepoint for every application status write + SSE publish; the fourmark*helpers are now thin wrappers over it. Fixes a bug wheremarkDeploymentInProgresspublished an SSE event twice on a DB error.applyDeployResult(+deployNotifications) — one shared interpreter of an agent's deploy outcome (err/ nil result /!Success/ success), replacing two divergent copies that previously lived in the sync path and the manual-deploy path.Deployer.StartDeployintodispatchDeploy(transport-only) andStartDeploy(dispatch + mark in-progress). The sync path usesdispatchDeployviaDeployAndWaitbecauseprocessSyncJobalready owns the in-progress transition, so each path now marks the application in-progress exactly once instead of twice.processSyncJobto use linear control flow instead of thesuccess bool+deferpattern.context.Background()instead of the job context. A deploy that hit the 3-minute job timeout previously wrote its failure status on an already-cancelled context, which could leave the application stuck displayingSyncing.Unified sync entry points
SyncApplications, the single path that all four sync triggers funnel through (polling, manual repo sync, webhooks, GitHub Actions). It performs consistent repository-status bookkeeping (Syncing → Success/Failed).CommitResolverwithStaticCommit(webhooks and GitHub Actions already carry the pushed SHA) andLatestCommit(polling and commit-less generic webhooks), separating "where the commit comes from" from the shared sync logic.SyncRepository,WebhookHandler,handleGenericWebhook, and the GitHub Actions handler now delegate toSyncApplications; the bespokeenqueueGenericAppshelper and the inline repository-status updates were removed.Documentation
SyncStatusreflects whether the sync was dispatched (commits resolved, jobs enqueued), not whether every application finished deploying — per-application progress lives on eachApplication.Additional context
Intentional behavior changes (please review)
OutOfSynconly, instead of alsoUnhealthy— matching the manual-deploy path. A dropped connection does not mean the running containers are unhealthy.Syncing → Success/Failedrepository bookkeeping like the poller. Previously webhooks markedSuccessonly (neverFailed) and GitHub Actions did no repository-status bookkeeping at all. For these synchronous handlers theSyncingstate is effectively instantaneous; it is only visibly observable on the polling path during the network commit lookup.Failedif commit resolution fails, instead of silently enqueuing an empty commit.Out of scope (planned follow-up)
Removing the
Default*package-level globals in favor of dependency injection, which also requires reordering theserver.gobootstrap. Deliberately left for a separate PR to keep this diff focused and reviewable.Testing
applications,routes, andwebsocketsuites pass;gofmt/go vetclean. The only failing tests are the Docker-daemon-dependent ones ininternal/agent/docker, which this branch does not touch.