Skip to content

feat(templates): E2B-style display names with CubeMaster name cache#607

Closed
HuChundong wants to merge 1 commit into
TencentCloud:masterfrom
HuChundong:feat/template-name-e2b-align
Closed

feat(templates): E2B-style display names with CubeMaster name cache#607
HuChundong wants to merge 1 commit into
TencentCloud:masterfrom
HuChundong:feat/template-name-e2b-align

Conversation

@HuChundong

@HuChundong HuChundong commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add optional human-readable template name (CubeMaster display_name) with cluster-wide case-insensitive uniqueness (20260623051112_template_display_name_index.sql).
  • Web: list/show display names; PATCH rename on template detail; optional name on create.
  • API: POST /sandboxes and template paths accept display name or tpl-* id; CubeAPI resolves name→id once at entry, then uses existing tpl-id paths.
  • CubeMaster: GET /cube/template/lookup, POST /cube/template/display-name; sync name reservation at submit (ReserveTemplateDisplayNameForCreate → 409); release name after failed builds.
  • Name cache (CubeMaster): display_nametemplate_id cached in templatecenter (300s TTL, 4096 entries, singleflight); invalidated on rename/delete/create. CubeAPI is a thin client with no local cache.

Architecture

Layer Approach
CubeAPI Thin resolver; delegates all name lookups to CubeMaster
CubeMaster Owns display_name, lookup cache, and invalidation
Uniqueness Global per cluster (no tenant/team namespace today)

Compared to E2B infra: E2B uses team-scoped aliases reserved in RegisterBuild; we use global display_name_key + sync submit check. We release names on build failure (E2B keeps aliases until template delete).

Test plan

  • go test ./pkg/templatecenter/...
  • go test ./pkg/service/httpservice/cube/...
  • cargo test -p cube-api
  • go test ./pkg/base/dao/migrate/ -run TestMigrationFilenames
  • Apply migration 20260623051112_template_display_name_index on staging (run audit queries in migration header first)
  • Create template with name → create sandbox by name when READY
  • Duplicate name at submit → 409; failed build → name reusable

Migration note

This migration may clear legacy tpl-/snap- prefixed names and dedupe display names before adding the unique index. Run preflight audit queries on staging before production.

Assisted-by: Cursor:Composer

@kinwin-ustc

kinwin-ustc commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

You can check if PR #612 meets your requirements? The scale of the changes is much smaller

@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from ff87c16 to 2ed302c Compare June 22, 2026 08:28
Comment thread CubeAPI/src/services/templates.rs Outdated

match self
.fetch_resolved_template(&template_ref, &template_id)
.await

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant second fetch_resolved_template call

update_template_name already calls fetch_resolved_template on line 144 to get the existing template metadata (including the old display_name for cache invalidation). After the successful update, it calls fetch_resolved_template again on line 164 solely to build the response. This adds an unnecessary cross-service HTTP roundtrip + DB query.

The response could be constructed from the already-fetched existing record (which was returned just 20 lines earlier), substituting the new name for display_name. Consider eliminating this second fetch to reduce latency — especially since the CubeMaster PATCH update already returned a success envelope, so the template definition is expected to still exist.

.await?;
let existing = self
.fetch_resolved_template(&template_ref, &template_id)
.await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra fetch_resolved_template purely for display_name cache invalidation

delete_template calls fetch_resolved_template solely to retrieve the old display_name for cache invalidation (line 228: for display_name in template_names(&existing.display_name)). This triggers an HTTP roundtrip to CubeMaster + a DB query just to get a single string field that's already available in CubeMaster's response to the lookup endpoint.

Consider having GET /cube/template/lookup also return display_name alongside template_id, so the single lookup call serves both the ID resolution and the cache invalidation data.

return normalized, nil
}

func mapDefinitionCreateDuplicateError(err error, displayName string) error {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raw database errors propagated to API responses

mapDefinitionCreateDuplicateError returns err unmodified when the error message does not contain "1062" or "Duplicate entry". This means transient MySQL errors (e.g. deadlocks, connection timeouts, schema mismatches) flow through to the API response's RetMsg field verbatim (via handleTemplateLookupAction line 118 and handleTemplateDisplayNameAction line 168).

While these are authenticated endpoints, exposing raw DB error texts can reveal internal schema details, engine versions, and connection strings. Consider wrapping non-duplicate errors into a generic internal-error message before returning to clients, while logging the raw error server-side.

Comment thread CubeAPI/src/services/template_names.rs Outdated
error::{AppError, AppResult},
};

const CACHE_TTL: Duration = Duration::from_secs(60);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache TTL of 60 seconds is unnecessarily short

Template display names are effectively immutable after creation — they only change through the explicit update_template_name endpoint, which triggers targeted cache invalidation. Every 60s cache miss forces a cross-service HTTP call to CubeMaster's /cube/template/lookup endpoint (which queries the DB).

Since inline invalidation already handles renames on the originating replica, a TTL of 300-600s would dramatically reduce CubeMaster load without increasing the staleness window beyond what the existing invalidation already provides. Multi-replica deployments already tolerate up to one TTL window of staleness; 60s is unnecessarily tight for a mapping that changes at human timescales.

Comment thread CubeAPI/src/services/template_names.rs Outdated
//

//! Template name → templateID resolution with an in-memory cache.
//!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module doc comment omits several routes using name resolution

The doc comment lists POST /sandboxes, GET/PATCH/DELETE /templates/{templateID}, and compat-adopt as the only routes with name resolution. However, the TemplateService also resolves names for POST /templates/:templateID (rebuild), POST /templates/:templateID/builds/:buildID (start build), and GET /templates/:templateID/builds/:buildID/status (get build status). A developer reading this comment would miss roughly half the affected endpoints. Recommend updating the comment to reflect the full set of routes.

@HuChundong

Copy link
Copy Markdown
Contributor Author

You can check if PR #612 meets your requirements? The scale of the changes is much smaller

These two PRs overlap slightly, but they have completely different priorities. This PR focuses on syncing the E2B API, making template-based sandbox creation the core feature. Displaying names on the WebUI serves little purpose if those names can't actually be used to spin up sandboxes.

@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from 2ed302c to 214dc12 Compare June 22, 2026 08:52
@HuChundong
HuChundong marked this pull request as draft June 22, 2026 08:55
@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from 214dc12 to f801c11 Compare June 22, 2026 09:06
@HuChundong
HuChundong marked this pull request as ready for review June 22, 2026 09:16
@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from 19ceabf to b297013 Compare June 22, 2026 09:25

@HuChundong HuChundong left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review of b297013 — see inline notes for actionable items.

Comment thread CubeAPI/src/services/template_names.rs Outdated
});
}
let key = reference.to_ascii_lowercase();
if let Some(template_id) = self.cache_get(&key).await {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-replica stale cache on destructive paths

get_template_build_status re-validates cache hits via fetch_resolved_template, but create_sandbox, delete_template, and update_template_name trust cached name→ID mappings for up to 300s on replicas that did not handle the rename/delete.

After a name is freed or reassigned, other CubeAPI replicas can still route POST /sandboxes or DELETE/PATCH to the old template. Consider reusing the build-status revalidation pattern (or a lightweight lookup) on cache hits for mutating endpoints.

Comment thread CubeAPI/src/services/templates.rs Outdated
.template_names
.resolve_template_ref(&template_ref)
.await?;
let names_for_invalidation = if TemplateNameCache::needs_resolution(&template_ref) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Service-layer tests missing for name-resolution orchestration

template_names.rs has solid unit tests, but update_template_name, delete_template (name vs tpl-id branches), and sandbox create-by-name paths in this module are untested.

A few focused tests with a mock CubeMasterClient would guard cache invalidation and the delete-by-name skip-fetch behavior added in this PR.


CALL cubemaster_assert_table_exists('t_cube_template_definition');

-- Drop invalid reserved-prefix names on templates (snapshots keep their names).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration preflight clears legacy names irreversibly

Lines 14–37 blank out reserved-prefix names and duplicate display names before adding the unique index. This is reasonable for greenfield but can surprise production upgrades.

Please confirm staging/release notes call this out explicitly (PR body mentions it — worth a pre-migration audit query for affected rows).

@HuChundong

Copy link
Copy Markdown
Contributor Author

Code review (b297013)

Overall the PR is in good shape after the review fixes: name validation is centralized in CubeMaster, internal DB errors are masked, redundant fetches were trimmed, and test coverage on the Master side is solid.

Worth addressing / confirming

  1. Multi-replica cache staleness (medium) — Process-local 300s cache is only invalidated on the writing replica. Mutating paths (POST /sandboxes, DELETE/PATCH /templates/{name}) do not re-validate cache hits, unlike get_template_build_status. Document as a known limitation or add revalidation on cache hits for destructive routes.

  2. CubeAPI service tests (medium) — Add mock-based tests for update_template_name / delete_template orchestration and cache invalidation; unit tests in template_names.rs do not cover these flows.

  3. Migration 0010 preflight (low) — Preflight UPDATEs silently clear invalid/duplicate legacy display names. Ensure staging migration checklist includes an audit query before apply.

Look good

  • DB uniqueness via display_name_key + placeholder reservation at submit
  • Generic internal errors in Master handlers
  • OpenAPI / module docs aligned with name-resolution routes
  • CubeMaster unit + HTTP handler tests for lookup/rename paths

No blocking issues found for merge after acknowledging multi-replica cache behavior in release notes or follow-up.

@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from 9438441 to 4483cba Compare June 22, 2026 10:32
Comment thread CubeAPI/src/services/template_names.rs Outdated
Comment on lines +149 to +156
async fn lookup_and_cache(&self, reference: &str, key: &str) -> AppResult<String> {
let lock = {
let mut locks = self.lookup_locks.write().await;
locks
.entry(key.to_string())
.or_insert_with(|| Arc::new(Mutex::new(())))
.clone()
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential memory leak: lookup_locks entries are never removed. Every unique name ever resolved adds an Arc<Mutex<()>> to the lookup_locks map, but no code cleans them up — not when cache entries are evicted (TTL or capacity), not on invalidation. Over a long-running process with many distinct template names, this grows unbounded, violating the MAX_CACHE_ENTRIES contract on self.entries.

Suggestion: Either clean up lookup_locks entries alongside cache eviction (cache_set / invalidate_name), or switch to a fixed-size striped lock pool (e.g., 64 or 256 slots via usize::from_str_bytes hash) to bound memory and reduce write-lock contention on the lookup_locks RwLock.

Comment thread CubeAPI/src/services/template_names.rs Outdated
Comment on lines +82 to +95
/// Resolve a display name for mutating API paths, always refreshing via lookup.
pub async fn resolve_template_ref_for_mutation(&self, reference: &str) -> AppResult<String> {
let reference = reference.trim();
if reference.is_empty() {
return Err(AppError::BadRequest(
"template reference is empty".to_string(),
));
}
if !Self::needs_resolution(reference) {
return Ok(reference.to_string());
}
let key = reference.to_ascii_lowercase();
self.lookup_and_cache(reference, &key).await
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc/behavior mismatch: The doc comment says "always refreshing via lookup," but the implementation calls lookup_and_cache, which checks the cache first and returns a cached value if still valid (300s TTL). Mutating paths (delete, rebuild, adopt-baseline) can therefore operate on stale template_id mappings for up to 300s after a name is reassigned on another replica.

Suggestion: Either bypass the cache for mutation paths (clear entry before lookup), or update the doc comment to accurately describe the caching behavior (e.g., "uses cached resolution when available; cache TTL bounds cross-replica staleness").

}},
}
}
normalized, _ := templatecenter.NormalizeTemplateDisplayName(name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error silently discarded: NormalizeTemplateDisplayName can return an error (e.g., name exceeds 256 chars after lookup succeeded, or contains unexpected characters), but the error is discarded via _. The response will have a valid template_id but an empty/incorrect display_name, which is misleading for API consumers.

Suggestion: Log the error or fall back to the original name:

normalized := name
if n, err := templatecenter.NormalizeTemplateDisplayName(name); err == nil {
    normalized = n
} else {
    log.G(ctx).Warnf("lookup returned valid template but name normalization failed: %v", err)
}

@cubesandboxbot

cubesandboxbot Bot commented Jun 22, 2026

Copy link
Copy Markdown

PR #607 Review: E2B-style display names with CubeMaster name cache

This is a well-structured, layered feature addition. The architecture of thin CubeAPI client → CubeMaster-owned cache → DB unique index is clean, and the cache design with singleflight coalescing and per-name write locks is thoughtfully done. The following findings are the ones I consider most noteworthy after reviewing all five dimensions.


Authorization gap on template mutation (Medium severity)

None of the new endpoints (PATCH /templates/:id, DELETE /templates/:id, POST /templates/:id rebuild, POST /templates/:id/adopt-baseline, or GET /templates/lookup) perform any resource-level authorization beyond authentication. Any authenticated user can rename, delete, or rebuild any template by ID or by display name. In shared deployments this is a privilege escalation path. Consider adding ownership metadata (e.g., creator_id) and checking it on mutating paths.

Relevant: CubeAPI/src/routes.rs:186-222, CubeMaster/pkg/service/httpservice/cube/template.go:406-459


openapi.yml contradiction: POST /templates claims 409 but never returns one

The POST /templates endpoint always returns HTTP 202 (accepted) immediately. Display name uniqueness is enforced during the async build job, so a name conflict produces a build failure — not an HTTP 409. The name field description correctly says "Duplicate names fail during the build," but the 409 response block at openapi.yml:315-320 contradicts this. Remove the 409 block or document that conflicts surface via the async build status.


openapi.yml: "ambiguous" mapped to wrong status code

The 400 response descriptions on lookup_template_name, get_template, update_template, and adopt_template_compat_baseline read "Invalid or ambiguous template name." However, the Go backend maps ErrTemplateNameAmbiguous to error code 130404 (HTTP 404), not 130400 (HTTP 400). Only ErrTemplateNameInvalid maps to 400. Change the 400 description to "Invalid template name (charset, length, or prefix violation)" and move "ambiguous" into the 404 description.


Missing error code constants in Rust

The magic integers 130404, 130409, 130400 at CubeAPI/src/services/templates.rs:94-102 duplicate Go-side ErrorCode constants. If CubeMaster error codes change, the Rust side silently breaks. Define named constants in cubemaster/mod.rs.


Redundant fetch_resolved_template in update_template_name

update_template_name at templates.rs:242 fetches the existing template via an extra HTTP round trip to CubeMaster purely to build the response, then issues a second call to update. The update endpoint on CubeMaster could return the updated fields directly, eliminating the extra RTT (5-50ms latency per rename).


Global FIFO mutex contention under burst load

The display name cache eviction at cache.go:274-366 uses a single sync.Mutex to serialize all FIFO list operations, with O(n) removal via slice reconstruction. Under a burst of cache misses (cold start), this becomes a serialization point. Consider replacing the FIFO slices with container/list for O(1) removal, or using go-cache's OnEvicted callback to track eviction order. The FIFO/go-cache dual-management also risks the FIFO list drifting from actual cache state when entries expire via TTL.


Per-name lock is process-local in multi-replica deployments

withTemplateWriteLock(displayNameLockKey(...)) at template_display_name.go:221 and :343 uses process-local sync.RWMutex. In multi-replica CubeMaster deployments, two replicas processing the same name simultaneously will both pass the availability check and rely solely on the DB unique index to catch duplicates. The DB catches it cleanly (and mapDefinitionCreateDuplicateError translates the error), but callers that check availability before creating may see a surprising 409. Document that the per-name lock is best-effort across replicas.


Lookup endpoint silently confirms reserved-prefix input

needs_resolution at templates.rs:39 returns false for tpl-/snap- prefixed strings. The lookup_template_name handler returns HTTP 200 with the input as template_id without verifying the template exists. The client-side checkDisplayName interprets any 200 from /templates/lookup as "name taken," producing a false-positive warning for names like "tpl-foo". Return 400 for reserved prefixes instead.


getCachedTemplateIDByDisplayName checks not-found before positive cache

At cache.go:232, the negative cache is checked before the positive cache. If a stale not-found entry exists alongside a valid positive entry, the not-found wins — producing a false-negative lookup. setTemplateDisplayNameCache correctly clears the not-found cache when setting a positive entry, but the FIFO eviction order could remove the positive entry first while the not-found entry persists for its 30s TTL. Swap the check order to check the positive cache first.


Test coverage gaps

  • ValidateTemplateDisplayNameAvailable (4 code paths) has zero direct tests
  • FindTemplateIDByDisplayNameFresh is untested in Go (only cached path is tested)
  • UpdateDefinitionDisplayName logic is untested (only cache side-effect is tested)
  • HTTP handler tests miss fresh parameter, ErrTemplateNameInvalid, and the display-name handler's success/not-found/invalid-name responses
  • removeFIFOKey for not-found entries and the FIFO-empty fallback are untested

Positive notes

  • NormalizeTemplateDisplayName enforces a strict ASCII allowlist (letters, digits, hyphen, underscore) with length and prefix validation — a well-designed validation function.
  • Singleflight pattern (templateFetchGroup.Do) correctly coalesces concurrent lookups and uses separate group keys for fresh vs cached paths.
  • Cache invalidation in invalidateTemplateDisplayNameCache correctly clears both positive and not-found caches synchronously.
  • DB generated column approach (display_name_key as LOWER(display_name)) ensures NULL entries for unnamed templates, so the unique index only applies to named templates — a clean design.
  • Failed name reclamation in assertDisplayNameAvailableForCreate automatically clears FAILED template names, preventing name squatting.
  • Thorough Rust integration tests with mock HTTP servers covering resolve path selection, error mapping, and cache skip behavior.

Comment thread CubeAPI/src/services/template_names.rs Outdated
Comment on lines +149 to +156
async fn lookup_and_cache(&self, reference: &str, key: &str) -> AppResult<String> {
let lock = {
let mut locks = self.lookup_locks.write().await;
locks
.entry(key.to_string())
.or_insert_with(|| Arc::new(Mutex::new(())))
.clone()
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential memory leak: lookup_locks entries are never removed. Every unique name ever resolved adds an Arc<Mutex<()>> to the lookup_locks map, but no code cleans them up — not when cache entries are evicted (TTL or capacity), not on invalidation. Over a long-running process with many distinct template names, this grows unbounded, violating the MAX_CACHE_ENTRIES contract on self.entries.

Suggestion: Either clean up lookup_locks entries alongside cache eviction (cache_set / invalidate_name), or switch to a fixed-size striped lock pool (e.g., 64 or 256 slots via usize::from_str_bytes hash) to bound memory and reduce write-lock contention on the lookup_locks RwLock.

Comment thread CubeAPI/src/services/template_names.rs Outdated
Comment on lines +82 to +95
/// Resolve a display name for mutating API paths, always refreshing via lookup.
pub async fn resolve_template_ref_for_mutation(&self, reference: &str) -> AppResult<String> {
let reference = reference.trim();
if reference.is_empty() {
return Err(AppError::BadRequest(
"template reference is empty".to_string(),
));
}
if !Self::needs_resolution(reference) {
return Ok(reference.to_string());
}
let key = reference.to_ascii_lowercase();
self.lookup_and_cache(reference, &key).await
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc/behavior mismatch: The doc comment says "always refreshing via lookup," but the implementation calls lookup_and_cache, which checks the cache first and returns a cached value if still valid (300s TTL). Mutating paths (delete, rebuild, adopt-baseline) can therefore operate on stale template_id mappings for up to 300s after a name is reassigned on another replica.

Suggestion: Either bypass the cache for mutation paths (clear entry before lookup), or update the doc comment to accurately describe the caching behavior (e.g., "uses cached resolution when available; cache TTL bounds cross-replica staleness").

}},
}
}
normalized, _ := templatecenter.NormalizeTemplateDisplayName(name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error silently discarded: NormalizeTemplateDisplayName can return an error (e.g., name exceeds 256 chars after lookup succeeded, or contains unexpected characters), but the error is discarded via _. The response will have a valid template_id but an empty/incorrect display_name, which is misleading for API consumers.

Suggestion: Log the error or fall back to the original name:

normalized := name
if n, err := templatecenter.NormalizeTemplateDisplayName(name); err == nil {
    normalized = n
} else {
    log.G(ctx).Warnf("lookup returned valid template but name normalization failed: %v", err)
}

@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from 4483cba to 52079e1 Compare June 22, 2026 10:55
Comment thread CubeAPI/src/services/templates.rs Outdated
CubeMasterError::Http(e) if e.is_timeout() || e.is_connect() => {
AppError::ServiceUnavailable(format!("CubeMaster unavailable: {e}"))
}
CubeMasterError::Http(e) => AppError::BadGateway(format!("CubeMaster request failed: {e}")),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: When CubeMaster returns an unexpected transport error, map_resolve_err formats messages like "CubeMaster unavailable: {e}"/"CubeMaster request failed: {e}" where {e} is the full reqwest::Error Display string. This can include the internal CubeMaster URL (e.g., http://cubemaster.internal:8080/...) and connection details, which are forwarded to the API caller as 502/503 responses.

Suggestion: Log the full error server-side and return a sanitized message like "upstream service unavailable" to avoid leaking internal network topology.


// ReleaseTemplateDisplayNameAfterBuildFailure clears display_name on a template
// whose image build failed so the name can be reused on a later create.
func ReleaseTemplateDisplayNameAfterBuildFailure(ctx context.Context, templateID string) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: ReleaseTemplateDisplayNameAfterBuildFailure is called on every build failure to release the display name so it can be reused (from failTemplateImageJob in job_repo.go line 150). If this function breaks, failed templates permanently lock their display names, preventing users from retrying under the same name.

This function has no unit tests despite non-trivial logic: it skips on empty templateID, tolerates ErrTemplateNotFound, and logs on other errors. None of these paths are exercised.

Suggestion: Add a unit test covering the success path, the empty-ID skip, and the ErrTemplateNotFound tolerance.

}
normalized, err := NormalizeTemplateDisplayName(req.DisplayName)
if err != nil {
log.G(context.Background()).Warnf("template image job display_name normalize failed: %v", err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: displayNameFromTemplateImageJob (line 382) takes no context.Context parameter and uses context.Background() for logging (lines 388, 393). This severs distributed trace correlation — any log line emitted here has no link to the originating request.

Every other public function in this file accepts a context.Context. Adding one here and plumbing it from callers (job_repo.go line 157, image_job_runner.go lines 211-214) would be a small change with significant observability benefit.

Suggestion: Add ctx context.Context parameter and propagate from callers.

Comment thread CubeAPI/src/models/mod.rs

/// Body for PATCH /templates/:templateID (update display name).
#[derive(Debug, Deserialize, ToSchema)]
pub struct UpdateTemplateRequest {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: UpdateTemplateRequest.name is a bare String with no validation. Other request structs in this file (e.g. NewSandbox) use #[derive(Validate)] with constraints. The documented constraints (max 256 chars, ASCII letters/digits/hyphen/underscore only, no tpl-/snap- prefix) are enforced only by CubeMaster, not at the CubeAPI boundary.

This is a defense-in-depth gap: invalid names pass through CubeAPI to CubeMaster before being rejected, costing a network round-trip. A CubeMaster regression in error code mapping could produce unpredictable HTTP status codes instead of the documented 400.

Suggestion: Add #[derive(Validate)] with length and regex constraints matching the documented rules, or add a manual validator. The #[validate] derive on CreateTemplateRequest (line 601) is also never called — consider fixing both.

Comment thread CubeAPI/src/services/templates.rs Outdated
CubeMasterError::Http(e) if e.is_timeout() || e.is_connect() => {
AppError::ServiceUnavailable(format!("CubeMaster unavailable: {e}"))
}
CubeMasterError::Http(e) => AppError::BadGateway(format!("CubeMaster request failed: {e}")),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: When CubeMaster returns an unexpected transport error, map_resolve_err formats messages like "CubeMaster unavailable: {e}"/"CubeMaster request failed: {e}" where {e} is the full reqwest::Error Display string. This can include the internal CubeMaster URL (e.g., http://cubemaster.internal:8080/...) and connection details, which are forwarded to the API caller as 502/503 responses.

Suggestion: Log the full error server-side and return a sanitized message like "upstream service unavailable" to avoid leaking internal network topology.


// ReleaseTemplateDisplayNameAfterBuildFailure clears display_name on a template
// whose image build failed so the name can be reused on a later create.
func ReleaseTemplateDisplayNameAfterBuildFailure(ctx context.Context, templateID string) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: ReleaseTemplateDisplayNameAfterBuildFailure is called on build failure to release the display name so it can be reused (called from failTemplateImageJob in job_repo.go line 150). If this function breaks, failed templates permanently lock their display names, and users cannot retry under the same name.

This function has no unit tests despite non-trivial logic: it skips on empty templateID, tolerates ErrTemplateNotFound, and logs on other errors. None of these paths are exercised.

Suggestion: Add a unit test covering the success path, the empty-ID skip, and the ErrTemplateNotFound tolerance.

@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from 6983aa7 to 25a5804 Compare June 23, 2026 12:16
@HuChundong
HuChundong marked this pull request as ready for review June 23, 2026 12:35
@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch 3 times, most recently from 9764b1a to 91bd0af Compare July 3, 2026 03:43
Comment thread CubeAPI/src/services/templates.rs Outdated

/// Validate an optional or required E2B-style template display name at the CubeAPI
/// boundary (mirrors CubeMaster NormalizeTemplateDisplayName rules).
fn validate_template_display_name(name: &str, required: bool) -> AppResult<String> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: Duplicate validation logic — may diverge from CubeMaster

This function mirrors NormalizeTemplateDisplayName in Go (template_display_name.go:2979) but uses Rust's is_ascii_alphanumeric vs Go's manual ASCII range checks. If the two versions diverge, a name accepted by CubeAPI will be rejected by CubeMaster (producing a confusing 500 for the client).

Recommend either making CubeAPI a thin pass-through (delegate validation to CubeMaster) or sharing generated code.

return nil
}

func failTemplateImageJob(ctx context.Context, jobID, templateID string, values map[string]any) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Unnecessary DB write on build failure when no display name set

ReleaseTemplateDisplayNameAfterBuildFailure fires for every build that reaches replica-creation with template_status: StatusFailed. When no display name was ever set, this executes UPDATE ... SET display_name = '' on an already-empty column. Consider short-circuiting when the display name is empty.

Note: the early failure paths (pulling, unpacking, distributing) at lines 37, 48, 78, 103, 146 don't include template_status: StatusFailed, so they don't trigger this — correct behavior.

templateDisplayNameCache.Set(key, templateID, templateDisplayNameCacheTTL)
}

func evictOneDisplayNameCacheEntry() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: Non-deterministic eviction — O(n) on every cache miss when at capacity

templateDisplayNameCache.Items() iterates the internal map in non-deterministic Go map order. On every cache miss when the cache is at capacity, this loop may repeatedly evict and re-evict the same keys. With 4096 entries, this degrades from O(1) to O(n).

Consider maintaining a simple FIFO ring buffer of keys alongside the map, or using go-cache's own TTL eviction and removing the manual cap check.

Comment thread CubeAPI/src/models/mod.rs Outdated
pub name: String,
}

fn validate_update_template_name(name: &str) -> Result<(), validator::ValidationError> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: Third copy of the same validation logic

This is the third copy of display name validation (also in templates.rs:113 and Go's NormalizeTemplateDisplayName). If Rust's is_ascii_alphanumeric and Go's manual ASCII range checks diverge, names accepted here will be rejected by CubeMaster (500 error for the client). Recommend consolidating: only the CubeMaster side should be authoritative; the Rust side should pass through or share generated code.

@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from 91bd0af to b9fc856 Compare July 3, 2026 07:48
@ls-ggg

ls-ggg commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR! Sorry for the wait, please rebase

@ls-ggg ls-ggg added the test-needed Awaiting test info label Jul 10, 2026
Add cluster-wide template display names (CubeMaster display_name) so
sandboxes and APIs can reference templates by human-readable name.
CubeMaster owns lookup, reservation at definition creation, bounded
positive/negative caches with invalidation, and a DB migration with
dedupe; CubeAPI exposes thin name resolution and GET /templates/lookup;
Web adds create hints, list/detail rename, and i18n.

Rebased onto upstream/master (lifecycle/env_vars merge in sandboxes).
Review fixes: fresh lookup on mutating paths, UpdateTemplateRequest
validation, service mock tests, display-name lock cleanup on eviction.

Assisted-by: Cursor:Composer
Signed-off-by: carmake <gycm520@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@HuChundong
HuChundong force-pushed the feat/template-name-e2b-align branch from b9fc856 to 338f955 Compare July 14, 2026 04:13
fn map_resolve_err(reference: &str) -> impl FnOnce(CubeMasterError) -> AppError {
let reference = reference.to_string();
move |err| match err {
CubeMasterError::Api { ret_code, .. } if ret_code == 130404 => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic error codes should be named constants
Hard-coding 130404, 130409, 130400 ties the Rust client to Go-side error codes. If CubeMaster ever changes these, the Rust side silently breaks. Define named constants in the cubemaster module (e.g., const CM_NOT_FOUND: i32 = 130404) to document the intent and isolate against changes. Same applies to the map_err function further down.

return Err(AppError::BadRequest("name is required".to_string()));
}

let existing = self

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary sequential round trip
update_template_name fetches the existing template detail via fetch_resolved_template (line 242) solely to build the response, then separately calls update_template_display_name (line 246). This adds one full HTTP round trip to CubeMaster that could be eliminated. Consider having the update endpoint on CubeMaster return the updated definition fields, or running the fetch concurrently with the update.

return def.DisplayName
}

func assertDisplayNameAvailableForCreate(ctx context.Context, normalized, templateID string) error {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lock contract is enforcement-free
The comment says "Caller (withDisplayNameCreateLock) already holds the per-name write lock," but Go has no way to enforce this at compile time. If this function is called without the lock, clearDefinitionDisplayNameLocked may deadlock or corrupt state. Consider adding a sync.Locker parameter so callers must explicitly pass the lock, or inline this into withDisplayNameCreateLock since it only has one caller.

return strings.ToLower(strings.TrimSpace(name))
}

func getCachedTemplateIDByDisplayName(key string) (string, bool) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCachedTemplateIDByDisplayName checks not-found before positive — reversed priority?
This function checks templateDisplayNameNotFoundCache first (line 236), then the positive cache. If a name is both in the not-found cache (stale) and the positive cache, the not-found wins and the caller gets ErrTemplateNameNotFound despite a positive entry existing. setTemplateDisplayNameCache (line 280) correctly clears the not-found cache when setting a positive entry, so stale not-found entries shouldn't persist — but if the FIFO fallback evicts a positive entry first while a not-found entry remains, an active name could be falsely reported as "not found" until the not-found TTL (30s) expires.

@ls-ggg

ls-ggg commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Could you take a look at #749? That change seems a lot cleaner

@ls-ggg ls-ggg added the duplicate This issue or pull request already exists label Jul 15, 2026
@ls-ggg ls-ggg closed this Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists test-needed Awaiting test info

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants