fix(datastore): prevent nil deployment metadata panic

[anubhavsingh2106]

What does this PR do?

Prevents UpdateSharedMetadata() and UpdatePluginMetadata() from
panicking when called on a deployment with a nil MetadataV2 field.

Both methods previously dereferenced d.MetadataV2 without checking
whether it had been initialized, which could result in a nil pointer
panic.

Changes

• Initialize MetadataV2 when it is nil before updating metadata.
• Add regression coverage for both metadata update methods.

Testing

• go test ./pkg/datastore/... -count=1 ✅
• git diff --check ✅
• gofmt ✅

Impact

Deployments with an uninitialized MetadataV2 can now safely update
shared or plugin metadata without causing a runtime panic.

Related Issue

Closes #7228

[netlify]

✅ Deploy Preview for pipecd-site ready!

Name	Link
🔨 Latest commit	ddb9cac
🔍 Latest deploy log	https://app.netlify.com/projects/pipecd-site/deploys/6a8bdc2facb79200084ccf7f
😎 Deploy Preview	https://deploy-preview-7229--pipecd-site.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[areebahmeddd]

these two tests are almost identical, you could pull the deployment fixture into a helper and table-drive the two cases, similar to TestMergeMetadata / TestListDeployments above 🤔

[anubhavsingh2106]

Thanks for pointing that out! I initially kept the tests separate to make it explicit that both UpdateSharedMetadata and UpdatePluginMetadata are covered independently. But I agree that there’s quite a bit of duplicated setup here. I’ll refactor the deployment fixture into a helper and table-drive these cases, following the existing test patterns. Thanks for the suggestion!

[anubhavsingh2106]

Made the changes, pls review when u get time.

[Copilot]

Pull request overview

This PR hardens the datastore deployment update paths by preventing nil-pointer panics when updating Deployment.MetadataV2 metadata, aligning behavior with the expectation that older/partially-initialized deployment records can still be updated safely.

Changes:

• Initialize d.MetadataV2 when nil in UpdateSharedMetadata() and UpdatePluginMetadata() to avoid dereferencing a nil pointer.
• Add regression tests covering both update methods when MetadataV2 starts as nil.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pkg/datastore/deploymentstore.go	Adds nil-guard initialization for MetadataV2 before updating shared/plugin metadata.
pkg/datastore/deploymentstore_test.go	Adds regression coverage ensuring shared/plugin metadata updates succeed when MetadataV2 is nil.

---

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

[armistcxy]

How could this case occur, can you show me one example where metadatav2 can be nil

It could help me much more if you can reproduce it @anubhavsingh2106

[anubhavsingh2106]

Yeah, I was able to reproduce this locally, @armistcxy . MetadataV2 can be nil when a Deployment is created or loaded without that field being initialized. I reproduced it by creating a valid deployment with MetadataV2 unset and then calling the metadata update methods.

Before the fix, both UpdateSharedMetadata() and UpdatePluginMetadata() panic when accessing d.MetadataV2.Shared / d.MetadataV2.Plugins. I’ve also added regression tests for this exact case in the PR to reproduce and prevent the panic. Thanks for pointing this out!

[armistcxy]

Hi, I need real proof for this one. Since I dont think that this case might happen, you can find where the metadatav2 be passed into or created for better proof

🤔 Defensive is good but I think that too much check for nil might make the function look "verbose"

[anubhavsingh2106]

@armistcxy, I dug a bit deeper into this. You're right that the current deployment creation path initializes MetadataV2, so newly created deployments shouldn't hit this.

The concern is with existing deployments loaded from the datastore — Update() operates on the already-persisted Deployment, so the update methods currently assume MetadataV2 is always present.

I haven't found a current creation path that explicitly produces a nil MetadataV2, so I agree the real-world reproduction isn't as clear as I initially thought. The regression test demonstrates the actual panic when the field is absent, but I don't want to overstate that this is definitely possible in current production data.

If MetadataV2 is guaranteed to be present for every persisted deployment, I'm happy to reconsider the nil check and keep the change minimal.

[armistcxy]

Sorry but I won't approve until you can produce a fail path

[anubhavsingh2106]

@armistcxy I dug deeper into this and was able to reproduce the panic through the actual MySQL datastore, rather than just with a mocked Deployment.

The flow I tested is:

Create a valid Deployment without setting MetadataV2.
Persist it through DeploymentStore.Add(), so it goes through the real MySQL serialization/storage path.
Load the same deployment back with DeploymentStore.Get() and verify that MetadataV2 is still nil.
Call UpdateSharedMetadata() on that persisted deployment.

With the original implementation, the test hits d.MetadataV2.Shared while MetadataV2 is nil and produces an actual nil pointer dereference:

panic: runtime error: invalid memory address or nil pointer dereference

The stack trace points directly to deploymentstore.go where MetadataV2.Shared is accessed.

After adding the nil initialization, I ran the exact same integration test again and it passes.

So I agree that a defensive nil check shouldn't be added without a real use case. In this case, though, the integration test demonstrates that MetadataV2 == nil is a valid persisted state and that it can reach this update path and cause a real runtime panic.