fix(org): project.go review — orgID scoping, orphan/race/self-heal repo hardening, comments - #2827
Merged
Merged
Conversation
From the project.go review: - Move republishWorkerFiles' doc comment back above republishWorkerFiles; it had been left stranded above ensureWorkerRepo (an insertion artifact), so Go attached it to the wrong function. - Rewrite the repoEnsureMu ponytail note: the real ceiling is that the lock is process-local (a second API replica defeats it, and CreateGitRepo auto-increments the name so the loser silently makes <worker>-2). The upgrade path is a store-level uniqueness constraint, not per-org sharding. Comment-only; no behaviour change.
…owed errors
From the project.go review (behavioural findings that are fixable in this
adapter without expanding ProjectService or the git-repo schema):
- ensureWorkerRepo now gets the authoritative orgID (the org the project was
applied into) instead of projOrgID read back from a GetProject that may have
failed. An empty org made CreateGitRepo mint an org-less repo that
AttachRepoToProject then rejected ("must be in the same org"), leaking the
repo — the main trigger of the partial-failure orphan.
- PutProjectSecret / GetProject failures are now logged instead of silently
discarded (worker needs those secrets; a swallowed GetProject hid why a repo
got created).
- WhoAmI's error is surfaced in the returned message instead of being dropped.
Remaining review findings (orphan reclaim on attach failure, deleted-repo
self-heal, cross-process dedup via a store uniqueness constraint) need
interface/schema changes plus live activation testing; tracked as follow-up.
chocobar
changed the title
…edup, self-heal) Implements the remaining project.go review findings in the org runtime layer (no shared git-schema change), each covered by a unit test: - Orphan cleanup (#2): if AttachRepoToProject fails after CreateGitRepo, the just-created repo is deleted so a retry doesn't leak it (and doesn't create `<worker>-2` beside the orphan). New DeleteGitRepo on ProjectService. - Cross-process race dedup (#1): CreateGitRepo auto-increments the name on collision rather than erroring; a returned name != requested means another replica (which repoEnsureMu can't serialise) won the create race. Delete the duplicate and error so the caller retries instead of silently keeping `<worker>-2`. - Deleted-repo self-heal (#4): the fast path and the ensureWorkerRepo re-check now validate the repo still exists (new GetGitRepo + ErrRepoNotFound); a DefaultRepoID/state repo deleted out-of-band is re-provisioned instead of handed back dead. Transient (non-not-found) read errors never trigger a recreate, to avoid duplicates. ProjectService gains GetGitRepo / DeleteGitRepo, wired in the in-proc adapter to the existing get/delete git-repository handlers. Test fakes updated.
chocobar
changed the title
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
1 participant
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.
Addresses the full
project.goreview in the org runtime layer (no shared git-schema change). Every behavioural change has a unit test.Applied
Repo provisioning hardening
ensureWorkerReponow uses the authoritativeorgID, notprojOrgIDfrom a possibly-failedGetProject(an empty org made an org-less repo that then failed to attach → leak). Primary trigger of the orphan.AttachRepoToProjectfails afterCreateGitRepo, the just-created repo is deleted (newDeleteGitRepo) so a retry doesn't leak it / make<worker>-2.CreateGitRepoauto-increments on name collision instead of erroring; a returned name ≠ requested means another replica won the create race — delete the duplicate and retry rather than silently keep<worker>-2.ensureWorkerRepore-check now validate the repo still exists (newGetGitRepo+ErrRepoNotFound); aDefaultRepoID/state repo deleted out-of-band is re-provisioned, not handed back dead. Transient read errors never trigger a recreate.PutProjectSecret/GetProject/WhoAmIerrors.ProjectServicegainsGetGitRepo/DeleteGitRepo, wired in the in-proc adapter to existing handlers.Comment-only
republishWorkerFiles' doc comment back above its function.repoEnsureMuceiling note (process-local; a store uniqueness constraint is the real upgrade path, not per-org sharding).Testing
TestEnsureDeletesOrphanRepoOnAttachFailure,TestEnsureDeletesRacedDuplicateRepo,TestEnsureFastPathReprovisionsDeletedRepo, plus the existing suite — all green.GetGitRepoon every re-activation) verified via a real bot re-activation (repo present → returned → republished → ran). Fresh org create seeds CoS cleanly. The create path's repo-creation branch is deferred-gated behind runtime config, so it's covered by unit tests rather than a live create.A reviewer's "helix-specs vs main branch" finding was a false positive (
EnsureBranch(repoID, "main")passesmainas the base; target is the configuredhelix-specsbranch).🤖 Generated with Claude Code