fix(CubeMaster/templatecenter): serialize concurrent rootfs builds for the same artifact_id

[dushulin]

Motivation

Quick-succession cubemastercli tpl create-from-image calls against the same OCI image spec race inside ensureRootfsArtifact: both goroutines resolve to the same artifact_id, share the same workDir/ext4Path, and one's defer cleanupIntermediateArtifacts wipes the ext4 file out from under the other. The surviving caller reaches distributeRootfsArtifact with a partial record (ext4_size_bytes=0, empty download_token, empty master_node_ip); buildDownloadURL falls back
to os.Hostname(), and cubelet rejects the pull with invalid size:0 — the template is marked FAILED with no clear signal of the underlying race.

Changes

• artifactBuildLocks sync.Map (keyed by artifact_id) serializes the full find-or-build flow in ensureRootfsArtifact. Granularity is artifact_id only — concurrent builds for different image specs still run in parallel.
• Defense-in-depth guard at the top of distributeRootfsArtifact: refuse to push a CreateImage when the artifact record is obviously incomplete, returning a diagnostic error instead of relying on cubelet's opaque invalid size:0.

Verification

• go build ./CubeMaster/pkg/templatecenter/... clean
• go vet ./CubeMaster/pkg/templatecenter/... clean
• Reproduced on an internal deployment before the fix; after the fix, concurrent submits serialize as expected and no partial artifact reaches cubelet.

[fslongjin]

/cubebot review

[github-actions]

Claude finished @fslongjin's task in 2m 23s —— View job

---

PR Review: fix(CubeMaster/templatecenter): serialize concurrent rootfs builds for the same artifact_id

• Gather context: read the changed file and diff
• Analyze the race condition fix and provide review feedback

---

Overall assessment: This is a well-designed fix for a real race condition. The root cause analysis is thorough, the locking strategy is sound, and the defense-in-depth guard is a good safety net. Below are a few observations.

What works well

• Per-artifactID locking via sync.Map is idiomatic Go and correctly serializes only competing builds for the same image spec. Different images still build in parallel.
• The LoadOrStore pattern at template_image.go:1244 correctly handles concurrent initialization of the mutex.
• The defense-in-depth validation in distributeRootfsArtifact (lines 1702-1711) provides a clear diagnostic error that would be much easier to debug than cubelet's opaque invalid size:0.
• The lock scope is correct — it covers the entire find-or-build flow in ensureRootfsArtifact, so the second caller will find the ready artifact after the first releases the lock.

Issues and suggestions

1. artifactBuildLocks entries are never cleaned up (minor)

artifactBuildLocks grows monotonically — once an artifactID key is stored, it's never deleted. For a long-running cubemaster, this accumulates orphaned *sync.Mutex entries. The number is bounded by distinct image specs, which is likely manageable, but it's worth noting. A simple fix would be to Delete the entry after the lock is released (the second caller would re-create it if needed):

muV, _ := artifactBuildLocks.LoadOrStore(artifactID, &sync.Mutex{})
mu := muV.(*sync.Mutex)
mu.Lock()
defer func() {
    mu.Unlock()
    artifactBuildLocks.Delete(artifactID)
}()

This is safe because: if another goroutine is already waiting on mu.Lock(), it holds a reference to the same mutex and will proceed. If a new goroutine arrives after the Delete, LoadOrStore creates a fresh mutex — which is fine since no one else holds it.

Fix this →

2. Missing Ext4SHA256 in the defense-in-depth check (minor)

The validation at line 1705 checks Ext4SizeBytes, DownloadToken, and MasterNodeIP, but not Ext4SHA256. If size > 0 but SHA256 is empty, cubelet would receive a corrupt image spec. Consider adding:

artifact.Status != ArtifactStatusReady ||
    artifact.Ext4SizeBytes == 0 ||
    artifact.Ext4SHA256 == "" ||
    strings.TrimSpace(artifact.DownloadToken) == "" ||
    strings.TrimSpace(artifact.MasterNodeIP) == ""

Fix this →

3. No test for concurrent build serialization (suggestion)

The PR description mentions go build and go vet pass, but there's no unit test verifying that concurrent calls to ensureRootfsArtifact for the same artifactID are properly serialized. Given the subtlety of the race, a targeted test would prevent regressions:

func TestEnsureRootfsArtifact_ConcurrentSerialization(t *testing.T) {
    // Launch N goroutines calling ensureRootfsArtifact for the same artifactID
    // Verify only one build actually occurs and all callers succeed
}

This is a suggestion rather than a blocking issue — I understand the build infrastructure and test setup may make this non-trivial.

4. Pre-existing concern: redo cleanup not under lock (observation, not a PR issue)

In runRedoTemplateImageJob (lines 1057-1066), the cleanup of the previous artifact happens before calling ensureRootfsArtifact, so it's not protected by the per-artifactID lock. If two redo jobs for the same artifactID run concurrently, both could attempt cleanup. This appears safe today since the cleanup operations are idempotent, but it's worth being aware of.

Summary

The fix correctly addresses the race condition. The two minor suggestions (cleaning up artifactBuildLocks entries and adding Ext4SHA256 to the validation) would make it more robust. Consider adding a concurrency test if feasible.

---

[chenhengqi]

The second request will be blocked here. Can we return early here?

[dushulin]

Requests that do not currently obtain a lock will be stuck in the first state of the state machine (BUILDING_EXT4). After the job with the lock is completed, other tasks will instantly enter the next state. I understand what you mean is that when you find this situation, do the next job directly throw an exception at the api level to the cli and end the job?

[dushulin]

I think the advantage of this is that it is not intrusive to the state machine or the cli, but can cause too many coroutines in extreme cases