fix(objectcache): synchronize container status fields and add cloud metadata annotations

[matthyx]

Description

This PR addresses two improvements in pkg/objectcache:

1. Synchronize container status fields:
   Fixes a data race on WatchedContainerData.status and completionStatus. Both are string-typed fields read and written concurrently by multiple goroutines (the monitoring loop on ticks/termination and deleteContainer from the container callback). Without synchronization, concurrent writes can produce torn reads, yielding corrupted statuses when stamping container profiles. A dedicated sync.RWMutex (statusMu) protects these fields across GetStatus(), SetStatus(), GetCompletionStatus(), and SetCompletionStatus().

2. Cloud metadata annotations helper:
   Adds CloudMetadataAnnotations(cloudMetadata *armotypes.CloudMetadata) map[string]string, mapping host type, host ID, cluster name, cloud account ID, and region to standard annotations where storage reads them.

Extracted from entlein's hardened batch work.

Testing

• Unit tests TestCloudMetadataAnnotations and concurrent status access TestWatchedContainerData_StatusConcurrency run clean with go test -race ./pkg/objectcache/....

Summary by CodeRabbit

• Bug Fixes
  • Container statuses marked Completed, Failed, or otherwise terminal remain unchanged during monitoring, preventing them from reverting to Ready.
  • Cloud metadata annotations now include available host, cluster, cloud account, and region details; fields without values are omitted.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7ea43400-66de-485c-a112-839e710feb73

📥 Commits

Reviewing files that changed from the base of the PR and between 67e455e and bd948d0.

📒 Files selected for processing (5)

• pkg/containerprofilemanager/v1/lifecycle_control_test.go
• pkg/containerprofilemanager/v1/monitoring.go
• pkg/objectcache/cloud_metadata_test.go
• pkg/objectcache/shared_container_data.go
• pkg/objectcache/shared_container_data_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change synchronizes container status access, prevents monitoring from replacing terminal statuses with Ready, and adds a function that maps nonempty cloud metadata fields to annotation keys.

Changes

Terminal status protection

Layer / File(s)	Summary
Synchronized status updates pkg/objectcache/shared_container_data.go, pkg/objectcache/shared_container_data_test.go	Status and completion-status reads and writes use a mutex. Terminal statuses are identified, and SetReadyUnlessTerminal preserves them. Tests cover concurrent access and readiness updates.
Monitoring preserves terminal status pkg/containerprofilemanager/v1/monitoring.go, pkg/containerprofilemanager/v1/lifecycle_control_test.go	Monitoring sets Ready only when the container is not terminal. The test checks that a completed status remains unchanged after a ticker event.

Cloud metadata annotations

Layer / File(s)	Summary
Map cloud metadata to annotations pkg/objectcache/shared_container_data.go, pkg/objectcache/cloud_metadata_test.go	CloudMetadataAnnotations returns annotation values for nonempty metadata fields, or an empty map for nil metadata. Tests cover populated fields, empty fields, and nil metadata.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to bd948

The terminal-status regression test exercises the monitoring update as intended. No identified issue needs resolution before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to bd948

The change strengthens terminal-state protection without demonstrating a new access path or privilege increase. Remaining uncertainty concerns downstream persistence guarantees and consumers outside the reviewed code.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated effect is on watched-container readiness, finalization, and generated profile state. The existing host pseudo-container has a separate completion branch that continues periodic saving; the new ordinary readiness guard does not replace that branch.

Trust Boundaries and Controls

• observed — The readiness helper holds one write lock across the terminal-state check and assignment, preventing a concurrent terminal update from being overwritten within that operation. SetStatus remains an unconditional setter; terminal preservation is specifically the readiness helper's contract, not a universal restriction on all status writes.
• inferred — The supplied repository-local references identify only the unit test as a caller of CloudMetadataAnnotations. Inspected production profile construction still uses GetLabels, so the added helper does not establish newly deployed cloud-metadata exposure. Consumers outside this repository are not covered.

Resilience and Maintainability Implications

• observed — Monitoring checks delivered storage errors before ticker processing and lifecycle finalization. Deletion releases the entry lock before signalling, and also observes monitor completion while sending and waiting, preserving the inspected failure-containment and cleanup protocol.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the container status synchronization and cloud metadata annotation changes, which are the pull request’s primary objectives.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Performance Benchmark Results

Node-Agent Resource Usage

Metric	BEFORE	AFTER	Delta
Avg CPU (cores)	0.242	0.239	-1.0%
Peak CPU (cores)	0.256	0.249	-2.5%
Peak CPU p95 (cores)	0.256	0.248	-2.9%
Avg Memory (MiB)	379.914	317.160	-16.5%
Peak Memory (MiB)	387.402	324.414	-16.3%

Dedup Effectiveness

No data available.

[Copilot]

Copilot review overview

🟡 Changes recommended

A concurrent ticker can still overwrite a terminal container status with Ready.

Review effort: Balanced
Findings: 1

Open (1)

• Prevent stale ticks from regressing terminal container status · New

What changed in this PR

Adds synchronized status access and a helper for cloud metadata annotations in pkg/objectcache.

Changes:

• Protects container status fields with an RW mutex.
• Adds cloud metadata annotation mapping.
• Adds concurrency and metadata unit tests.

File	Description
pkg/​objectcache/​shared_container_data.go	Adds synchronization and metadata helper.
pkg/​objectcache/​shared_container_data_test.go	Tests concurrent status access.
pkg/​objectcache/​cloud_metadata_test.go	Tests annotation mapping.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟢 Approval recommended

The synchronization is consistently applied, the prior terminal-state race is resolved, and relevant behavior is tested.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Prevent stale ticks from regressing terminal container status

[github-actions]

Performance Benchmark Results

Node-Agent Resource Usage

Metric	BEFORE	AFTER	Delta
Avg CPU (cores)	0.000	0.000	N/A
Peak CPU (cores)	0.000	0.000	N/A
Peak CPU p95 (cores)	0.000	0.000	N/A
Avg Memory (MiB)	0.000	0.000	N/A
Peak Memory (MiB)	0.000	0.000	N/A

Dedup Effectiveness

No data available.

[github-actions]

Performance Benchmark Results

Node-Agent Resource Usage

Metric	BEFORE	AFTER	Delta
Avg CPU (cores)	0.128	0.144	+12.4%
Peak CPU (cores)	0.138	0.154	+11.2%
Peak CPU p95 (cores)	0.135	0.152	+12.2%
Avg Memory (MiB)	383.032	318.368	-16.9%
Peak Memory (MiB)	386.336	325.180	-15.8%

Dedup Effectiveness

No data available.