From 2280f02becb2c5a71c170c6c8ac4f99f703bfb82 Mon Sep 17 00:00:00 2001 From: Moko Consulting Date: Sat, 18 Jul 2026 16:08:20 -0500 Subject: [PATCH 1/2] =?UTF-8?q?feat(release):=20server-side=20packaging=20?= =?UTF-8?q?=E2=80=94=20attach=20full-repo=20+=20entry=5Fpoint=20subtree=20?= =?UTF-8?q?zips=20on=20release=20[#809]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add GenerateReleaseArtifacts in services/release/packaging.go, invoked from CreateRelease and the UpdateRelease publish path (drafts skipped). It streams a full-repository zip (-.zip) and, when repo metadata declares a non-root entry_point, an entry_point subtree zip (--source.zip) via gitrepo.CreateArchive through an io.Pipe into attachment_service.NewAttachment, so archives are never buffered in memory. The helper is idempotent (existing artifacts of the same name are replaced) and runs before GenerateReleaseChecksums so each zip receives a .sha256 sidecar automatically. Refs #809 #812 EPIC #367 Authored-by: Moko Consulting --- services/release/packaging.go | 155 ++++++++++++++++++++++++++++++++++ services/release/release.go | 25 +++++- 2 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 services/release/packaging.go diff --git a/services/release/packaging.go b/services/release/packaging.go new file mode 100644 index 0000000000..f81275e088 --- /dev/null +++ b/services/release/packaging.go @@ -0,0 +1,155 @@ +// Copyright 2026 Moko Consulting +// SPDX-License-Identifier: GPL-3.0-or-later + +package release + +import ( + "context" + "fmt" + "io" + "path" + "strings" + + repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGIT/models/repo" + "code.mokoconsulting.tech/MokoConsulting/MokoGIT/modules/gitrepo" + "code.mokoconsulting.tech/MokoConsulting/MokoGIT/modules/log" + attachment_service "code.mokoconsulting.tech/MokoConsulting/MokoGIT/services/attachment" +) + +// isRootEntryPoint reports whether an entry point refers to the repository root +// (and therefore does not warrant a separate subtree archive). +func isRootEntryPoint(entryPoint string) bool { + switch strings.TrimSpace(entryPoint) { + case "", ".", "/", "./": + return true + default: + return false + } +} + +// deleteExistingArtifacts removes any pre-existing release attachments whose +// names match the supplied set. This makes GenerateReleaseArtifacts idempotent: +// re-running it (for example, on a draft→publish UpdateRelease) replaces the +// previously generated zips rather than accumulating duplicates. +func deleteExistingArtifacts(ctx context.Context, rel *repo_model.Release, names map[string]struct{}) error { + if err := repo_model.GetReleaseAttachments(ctx, rel); err != nil { + return fmt.Errorf("GetReleaseAttachments: %w", err) + } + for _, a := range rel.Attachments { + if _, ok := names[a.Name]; !ok { + continue + } + if err := repo_model.DeleteAttachment(ctx, a, true); err != nil { + // Non-fatal: log and continue so a stale artifact doesn't abort packaging. + log.Warn("Failed to delete old release artifact %s: %v", a.Name, err) + } + } + return nil +} + +// attachArchive streams a `git archive` of the given commit/paths directly into +// a new release attachment via an io.Pipe, so the full zip is never buffered in +// memory. paths == nil archives the whole repository; a single-element slice +// archives that subtree. +func attachArchive(ctx context.Context, rel *repo_model.Release, name, commitID string, paths []string) error { + pr, pw := io.Pipe() + + go func() { + // "zip" is a valid archive format for gitrepo.CreateArchive (it only + // rejects "unknown"). usePrefix=false keeps entries at the archive root. + err := gitrepo.CreateArchive(ctx, rel.Repo, "zip", pw, false, commitID, paths) + // Closing the writer with err (nil on success) propagates any archive + // failure to the reader side so NewAttachment aborts instead of storing + // a truncated zip. + _ = pw.CloseWithError(err) + }() + + attach := &repo_model.Attachment{ + RepoID: rel.RepoID, + ReleaseID: rel.ID, + Name: name, + } + + // size == -1: the archive is streamed with an unknown length, matching the + // unknown-size upload path already used by the attachment service. + if _, err := attachment_service.NewAttachment(ctx, attach, pr, -1); err != nil { + _ = pr.CloseWithError(err) + return fmt.Errorf("NewAttachment %s: %w", name, err) + } + return nil +} + +// GenerateReleaseArtifacts packages a published release into release +// attachments: a full-repository zip always, plus an entry-point subtree zip +// when the repository metadata declares a non-root build entry point. +// +// It is idempotent — existing artifacts with the same names are replaced — and +// is intended to run BEFORE GenerateReleaseChecksums so the subsequent checksum +// pass mints a .sha256 sidecar for each generated zip automatically. +// +// Following the convention of GenerateReleaseChecksums, a failure to produce an +// individual artifact is logged and does not abort the release; only setup +// failures (metadata lookup, attachment enumeration) are returned. +func GenerateReleaseArtifacts(ctx context.Context, rel *repo_model.Release) error { + if rel.Repo == nil { + if err := rel.LoadAttributes(ctx); err != nil { + return fmt.Errorf("LoadAttributes: %w", err) + } + } + + commitID := rel.Sha1 + if commitID == "" { + // No resolved commit (e.g. a draft with no tag) — nothing to archive. + return nil + } + + version := rel.TagName + repoName := rel.Repo.Name + + fullName := fmt.Sprintf("%s-%s.zip", repoName, version) + sourceName := fmt.Sprintf("%s-%s-source.zip", repoName, version) + + // Resolve the build entry point from repo metadata, if any. + var entryPoint string + meta, err := repo_model.GetRepoMetadata(ctx, rel.RepoID) + if err != nil { + log.Warn("GenerateReleaseArtifacts: GetRepoMetadata for repo %d: %v", rel.RepoID, err) + } else if meta != nil { + entryPoint = meta.EntryPoint + } + wantSource := !isRootEntryPoint(entryPoint) + + // Idempotent replace: delete any prior copies of the artifacts we're about + // to (re)generate before attaching the new ones. + targetNames := map[string]struct{}{fullName: {}} + if wantSource { + targetNames[sourceName] = struct{}{} + } + if err := deleteExistingArtifacts(ctx, rel, targetNames); err != nil { + return err + } + + created := 0 + + // Full-repository archive (paths == nil). + if err := attachArchive(ctx, rel, fullName, commitID, nil); err != nil { + log.Error("GenerateReleaseArtifacts: full-repo zip for release %s (repo %d): %v", version, rel.RepoID, err) + } else { + created++ + } + + // Entry-point subtree archive. + if wantSource { + subPath := path.Clean(strings.TrimSuffix(strings.TrimSpace(entryPoint), "/")) + if err := attachArchive(ctx, rel, sourceName, commitID, []string{subPath}); err != nil { + log.Error("GenerateReleaseArtifacts: subtree zip (%s) for release %s (repo %d): %v", subPath, version, rel.RepoID, err) + } else { + created++ + } + } + + if created > 0 { + log.Info("Generated %d release artifact(s) for release %s (repo %d)", created, version, rel.RepoID) + } + return nil +} diff --git a/services/release/release.go b/services/release/release.go index fc71bca91c..964979066e 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -254,8 +254,17 @@ func CreateRelease(gitRepo *git.Repository, rel *repo_model.Release, attachmentU return err } + // Server-side packaging: attach full-repo + entry_point subtree zips. + // Skip drafts (mirrors the notify guard below); run before the checksum + // pass so the generated zips receive .sha256 sidecars automatically. + if !rel.IsDraft { + if err := GenerateReleaseArtifacts(gitRepo.Ctx, rel); err != nil { + log.Error("GenerateReleaseArtifacts for %s: %v", rel.TagName, err) + } + } + // Generate SHA256 checksums for all release attachments - if len(attachmentUUIDs) > 0 { + if len(attachmentUUIDs) > 0 || !rel.IsDraft { if err := GenerateReleaseChecksums(gitRepo.Ctx, rel); err != nil { log.Error("GenerateReleaseChecksums for %s: %v", rel.TagName, err) } @@ -415,8 +424,18 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo } } - // Regenerate checksums when attachments change - if len(addAttachmentUUIDs) > 0 || len(delAttachmentUUIDs) > 0 { + // Server-side packaging on publish: (re)attach full-repo + entry_point + // subtree zips. Skip drafts (mirrors the notify guard below). The helper is + // idempotent, so a draft→publish transition replaces any prior artifacts. + // Run before the checksum pass so the zips receive .sha256 sidecars. + if !rel.IsDraft { + if err := GenerateReleaseArtifacts(ctx, rel); err != nil { + log.Error("GenerateReleaseArtifacts for %s: %v", rel.TagName, err) + } + } + + // Regenerate checksums when attachments change or artifacts were generated + if len(addAttachmentUUIDs) > 0 || len(delAttachmentUUIDs) > 0 || !rel.IsDraft { if err := GenerateReleaseChecksums(ctx, rel); err != nil { log.Error("GenerateReleaseChecksums for %s: %v", rel.TagName, err) } -- 2.52.0 From 19d498f6c3b9be44f379f0a661ed71a87bc5e626 Mon Sep 17 00:00:00 2001 From: Moko Consulting Date: Sat, 18 Jul 2026 16:39:19 -0500 Subject: [PATCH 2/2] fix(release): mark generated artifacts, serialize + reorder attach to prevent data loss [#809] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address data-safety review of the server-side packaging hook: - DATA LOSS (HIGH): tag generated zips with UploaderID = user_model.ActionsUserID (-2), a built-in bot sentinel that cannot collide with human uploads (which always use a positive doer ID). Only attachments carrying this marker are ever deleted, so a user-uploaded asset sharing a generated name is never destroyed. - CONCURRENCY (HIGH): serialize the per-release delete+attach sequence with globallock.LockAndDo, keyed release_packaging_, matching the forge's existing working-lock convention. Attachments are re-read inside the lock. - DELETE-THEN-FAIL (LOW): reorder to attach-new-then-delete-old — the fresh archive is stored under the canonical name first and stale generated copies are removed only on success, so a failed regeneration never nets asset loss. - FILENAME (LOW): sanitize the tag segment (replace path separators) so a tag like release/1.0 yields a clean asset name. - ENTRY_POINT (LOW): normalizeEntryPoint trims a leading slash (git archive rejects absolute paths) and treats a now-empty result as root/skip. - CHECKSUM re-hash cost (MEDIUM): documented as a follow-up at the checksum call sites in release.go. The verified-correct io.Pipe streaming is unchanged. Refs #809 #812 EPIC #367 Authored-by: Moko Consulting --- services/release/packaging.go | 190 ++++++++++++++++++++++++---------- services/release/release.go | 10 +- 2 files changed, 146 insertions(+), 54 deletions(-) diff --git a/services/release/packaging.go b/services/release/packaging.go index f81275e088..94d5eab28c 100644 --- a/services/release/packaging.go +++ b/services/release/packaging.go @@ -11,46 +11,89 @@ import ( "strings" repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGIT/models/repo" + user_model "code.mokoconsulting.tech/MokoConsulting/MokoGIT/models/user" "code.mokoconsulting.tech/MokoConsulting/MokoGIT/modules/gitrepo" + "code.mokoconsulting.tech/MokoConsulting/MokoGIT/modules/globallock" "code.mokoconsulting.tech/MokoConsulting/MokoGIT/modules/log" attachment_service "code.mokoconsulting.tech/MokoConsulting/MokoGIT/services/attachment" ) +// generatedArtifactUploaderID marks a release attachment as server-generated by +// the packaging hook rather than uploaded by a human. Real uploads always set +// UploaderID to a positive doer ID (see routers/**/release_attachment.go), so +// this negative built-in bot id can never collide with a user asset. Only +// attachments carrying this marker are ever deleted by the packaging hook, which +// prevents it from destroying a user-uploaded asset that happens to share a name +// with a generated zip. +const generatedArtifactUploaderID = user_model.ActionsUserID + +// getReleasePackagingLockKey builds the global-lock key that serializes the +// delete+attach sequence for a single release, following the forge convention +// (see getPullWorkingLockKey / getWikiWorkingLockKey). +func getReleasePackagingLockKey(relID int64) string { + return fmt.Sprintf("release_packaging_%d", relID) +} + // isRootEntryPoint reports whether an entry point refers to the repository root // (and therefore does not warrant a separate subtree archive). func isRootEntryPoint(entryPoint string) bool { - switch strings.TrimSpace(entryPoint) { - case "", ".", "/", "./": + switch normalizeEntryPoint(entryPoint) { + case "", ".": return true default: return false } } -// deleteExistingArtifacts removes any pre-existing release attachments whose -// names match the supplied set. This makes GenerateReleaseArtifacts idempotent: -// re-running it (for example, on a draft→publish UpdateRelease) replaces the -// previously generated zips rather than accumulating duplicates. -func deleteExistingArtifacts(ctx context.Context, rel *repo_model.Release, names map[string]struct{}) error { - if err := repo_model.GetReleaseAttachments(ctx, rel); err != nil { - return fmt.Errorf("GetReleaseAttachments: %w", err) +// normalizeEntryPoint reduces an entry point to a repo-relative, cleaned path +// suitable for `git archive `. It trims surrounding whitespace, +// strips a leading slash (git archive rejects absolute paths), drops a trailing +// slash, and cleans the result. A root-equivalent entry point normalizes to "". +func normalizeEntryPoint(entryPoint string) string { + ep := strings.TrimSpace(entryPoint) + ep = strings.TrimPrefix(ep, "/") + ep = strings.TrimSuffix(ep, "/") + if ep == "" { + return "" } + ep = path.Clean(ep) + if ep == "." || ep == "/" { + return "" + } + return ep +} + +// sanitizeNameSegment makes a string safe and tidy for use inside an attachment +// display name. Attachment storage paths are UUID-based (see +// AttachmentRelativePath), so there is no traversal risk — this only keeps the +// display name clean, e.g. a tag "release/1.0" would otherwise render as +// "repo-release/1.0.zip". +func sanitizeNameSegment(s string) string { + return strings.NewReplacer( + "/", "-", + "\\", "-", + "\x00", "-", + ).Replace(s) +} + +// findGeneratedArtifacts returns the existing, server-generated attachments on +// the release with the given name. User-uploaded assets (any UploaderID other +// than the marker) are never included, so they can never be touched. +func findGeneratedArtifacts(rel *repo_model.Release, name string) []*repo_model.Attachment { + var out []*repo_model.Attachment for _, a := range rel.Attachments { - if _, ok := names[a.Name]; !ok { - continue - } - if err := repo_model.DeleteAttachment(ctx, a, true); err != nil { - // Non-fatal: log and continue so a stale artifact doesn't abort packaging. - log.Warn("Failed to delete old release artifact %s: %v", a.Name, err) + if a.Name == name && a.UploaderID == generatedArtifactUploaderID { + out = append(out, a) } } - return nil + return out } // attachArchive streams a `git archive` of the given commit/paths directly into // a new release attachment via an io.Pipe, so the full zip is never buffered in // memory. paths == nil archives the whole repository; a single-element slice -// archives that subtree. +// archives that subtree. The attachment is tagged with the generated-artifact +// marker so a later run can safely recognize and replace it. func attachArchive(ctx context.Context, rel *repo_model.Release, name, commitID string, paths []string) error { pr, pw := io.Pipe() @@ -65,9 +108,10 @@ func attachArchive(ctx context.Context, rel *repo_model.Release, name, commitID }() attach := &repo_model.Attachment{ - RepoID: rel.RepoID, - ReleaseID: rel.ID, - Name: name, + RepoID: rel.RepoID, + ReleaseID: rel.ID, + UploaderID: generatedArtifactUploaderID, + Name: name, } // size == -1: the archive is streamed with an unknown length, matching the @@ -79,17 +123,56 @@ func attachArchive(ctx context.Context, rel *repo_model.Release, name, commitID return nil } +// regenerateArtifact attaches a fresh archive under the canonical name and only +// then, on success, removes any previously generated copies of that name +// (attach-new-then-delete-old). This ordering guarantees a failed regeneration +// never nets asset loss: if the archive fails, the previous artifact is left +// intact and nothing is deleted. On success the stale generated copies are +// removed, leaving exactly one generated artifact under the name. +// +// Only artifacts carrying the generated-artifact marker are considered for +// deletion, so a user-uploaded asset of the same name is never removed. The +// caller must hold the per-release packaging lock so the brief window in which +// two same-named generated copies coexist is never observed concurrently. +func regenerateArtifact(ctx context.Context, rel *repo_model.Release, name, commitID string, paths []string) error { + // Snapshot prior generated copies BEFORE attaching the new one, so the + // post-attach cleanup removes only the old rows, not the fresh insert. + stale := findGeneratedArtifacts(rel, name) + + if err := attachArchive(ctx, rel, name, commitID, paths); err != nil { + // Attach failed — leave any existing artifact untouched. No net loss. + return err + } + + // New artifact is stored under the canonical name; drop the stale copies. + for _, old := range stale { + if err := repo_model.DeleteAttachment(ctx, old, true); err != nil { + // Non-fatal: log and continue. Worst case a superseded copy lingers, + // but the fresh artifact is already in place under the real name. + log.Warn("Failed to delete superseded release artifact %s (uuid %s): %v", old.Name, old.UUID, err) + } + } + return nil +} + // GenerateReleaseArtifacts packages a published release into release // attachments: a full-repository zip always, plus an entry-point subtree zip // when the repository metadata declares a non-root build entry point. // -// It is idempotent — existing artifacts with the same names are replaced — and -// is intended to run BEFORE GenerateReleaseChecksums so the subsequent checksum -// pass mints a .sha256 sidecar for each generated zip automatically. +// It is idempotent and safe on re-run: only server-generated artifacts (tagged +// with the generated-artifact marker) are ever replaced, so a user-uploaded +// asset sharing a name is never destroyed. The delete+attach sequence for the +// release is serialized under a global lock, and each artifact is attached +// before its stale copy is removed, so concurrent runs and mid-run failures can +// never net an asset loss. +// +// It is intended to run BEFORE GenerateReleaseChecksums so the subsequent +// checksum pass mints a .sha256 sidecar for each generated zip automatically. // // Following the convention of GenerateReleaseChecksums, a failure to produce an // individual artifact is logged and does not abort the release; only setup -// failures (metadata lookup, attachment enumeration) are returned. +// failures (metadata lookup, attachment enumeration, lock acquisition) are +// returned. func GenerateReleaseArtifacts(ctx context.Context, rel *repo_model.Release) error { if rel.Repo == nil { if err := rel.LoadAttributes(ctx); err != nil { @@ -103,7 +186,7 @@ func GenerateReleaseArtifacts(ctx context.Context, rel *repo_model.Release) erro return nil } - version := rel.TagName + version := sanitizeNameSegment(rel.TagName) repoName := rel.Repo.Name fullName := fmt.Sprintf("%s-%s.zip", repoName, version) @@ -119,37 +202,40 @@ func GenerateReleaseArtifacts(ctx context.Context, rel *repo_model.Release) erro } wantSource := !isRootEntryPoint(entryPoint) - // Idempotent replace: delete any prior copies of the artifacts we're about - // to (re)generate before attaching the new ones. - targetNames := map[string]struct{}{fullName: {}} - if wantSource { - targetNames[sourceName] = struct{}{} - } - if err := deleteExistingArtifacts(ctx, rel, targetNames); err != nil { - return err - } + // Serialize the delete+attach sequence per-release so concurrent + // CreateRelease/UpdateRelease calls on the same release cannot interleave + // (which could duplicate zips or delete a freshly attached artifact). + return globallock.LockAndDo(ctx, getReleasePackagingLockKey(rel.ID), func(ctx context.Context) error { + // Re-load attachments inside the lock so replace decisions see a + // consistent view (including any generated artifacts from a prior run). + if err := repo_model.GetReleaseAttachments(ctx, rel); err != nil { + return fmt.Errorf("GetReleaseAttachments: %w", err) + } - created := 0 + created := 0 - // Full-repository archive (paths == nil). - if err := attachArchive(ctx, rel, fullName, commitID, nil); err != nil { - log.Error("GenerateReleaseArtifacts: full-repo zip for release %s (repo %d): %v", version, rel.RepoID, err) - } else { - created++ - } - - // Entry-point subtree archive. - if wantSource { - subPath := path.Clean(strings.TrimSuffix(strings.TrimSpace(entryPoint), "/")) - if err := attachArchive(ctx, rel, sourceName, commitID, []string{subPath}); err != nil { - log.Error("GenerateReleaseArtifacts: subtree zip (%s) for release %s (repo %d): %v", subPath, version, rel.RepoID, err) + // Full-repository archive (paths == nil). + if err := regenerateArtifact(ctx, rel, fullName, commitID, nil); err != nil { + log.Error("GenerateReleaseArtifacts: full-repo zip for release %s (repo %d): %v", version, rel.RepoID, err) } else { created++ } - } - if created > 0 { - log.Info("Generated %d release artifact(s) for release %s (repo %d)", created, version, rel.RepoID) - } - return nil + // Entry-point subtree archive. The stale-snapshot is taken from the same + // in-lock attachment read above; the full-repo archive uses a different + // name, so no re-read is needed here. + if wantSource { + subPath := normalizeEntryPoint(entryPoint) + if err := regenerateArtifact(ctx, rel, sourceName, commitID, []string{subPath}); err != nil { + log.Error("GenerateReleaseArtifacts: subtree zip (%s) for release %s (repo %d): %v", subPath, version, rel.RepoID, err) + } else { + created++ + } + } + + if created > 0 { + log.Info("Generated %d release artifact(s) for release %s (repo %d)", created, version, rel.RepoID) + } + return nil + }) } diff --git a/services/release/release.go b/services/release/release.go index 964979066e..19a5f1f7a0 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -263,7 +263,10 @@ func CreateRelease(gitRepo *git.Repository, rel *repo_model.Release, attachmentU } } - // Generate SHA256 checksums for all release attachments + // Generate SHA256 checksums for all release attachments. + // FOLLOW-UP (#809): GenerateReleaseChecksums re-hashes every attachment on + // the release, so publishing a release with many large assets re-reads them + // all. Consider hashing only newly added/changed attachments. if len(attachmentUUIDs) > 0 || !rel.IsDraft { if err := GenerateReleaseChecksums(gitRepo.Ctx, rel); err != nil { log.Error("GenerateReleaseChecksums for %s: %v", rel.TagName, err) @@ -434,7 +437,10 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo } } - // Regenerate checksums when attachments change or artifacts were generated + // Regenerate checksums when attachments change or artifacts were generated. + // FOLLOW-UP (#809): GenerateReleaseChecksums re-hashes every attachment on + // the release, so a non-draft edit re-reads them all even if only one asset + // changed. Consider hashing only newly added/changed attachments. if len(addAttachmentUUIDs) > 0 || len(delAttachmentUUIDs) > 0 || !rel.IsDraft { if err := GenerateReleaseChecksums(ctx, rel); err != nil { log.Error("GenerateReleaseChecksums for %s: %v", rel.TagName, err) -- 2.52.0