Bug: Prevent nil pointer panic in deployment metadata updates

[anubhavsingh2106]

Bug: Prevent nil pointer panic in deployment metadata updates

Description

UpdateSharedMetadata() and UpdatePluginMetadata() in
pkg/datastore/deploymentstore.go can panic when called with a
deployment whose MetadataV2 field is nil.

Both methods access fields of MetadataV2 without first checking whether
the pointer has been initialized.

Affected Functions

• UpdateSharedMetadata()
• UpdatePluginMetadata()

File:

pkg/datastore/deploymentstore.go

Current Behavior

When MetadataV2 == nil, calling UpdateSharedMetadata() results in a
nil pointer dereference while accessing:

d.MetadataV2.Shared

[omlahore]

I'd like to work on this. PR is up.

Traced how the nil gets there: pipedv0's trigger (pkg/app/piped/trigger/deployment.go) builds a Deployment without MetadataV2, and the deployment collection's Factory() decodes into &model.Deployment{}, so any stored document lacking the field arrives nil and both methods dereference it straight away.

[Warashi]

@anubhavsingh2106 @omlahore
Hi, thank you for raising this issue, and picking.
I think this is expected to be unreachable because it's not expected that pipedv1 will pick up pipedv0's deployment.
If there's a failure caused by this in production, could you please provide the full process to reproduce it?

[anubhavsingh2106]

@Warashi Thanks for pointing this out. I dug deeper into the scenario and was able to reproduce the panic using the actual MySQL datastore rather than a mocked Deployment.

The reproduction flow is:

1. Create a valid Deployment without MetadataV2.
2. Persist it using DeploymentStore.Add(), going through the real MySQL serialization/storage path.
3. Load the same Deployment back using DeploymentStore.Get() and verify that MetadataV2 is still nil.
4. Call UpdateSharedMetadata() on the persisted Deployment.

With the original implementation, this reaches d.MetadataV2.Shared while MetadataV2 is nil, resulting in an actual nil pointer dereference. The stack trace points directly to deploymentstore.go where MetadataV2.Shared is accessed.

After initializing MetadataV2 when it is nil, I ran the same integration test again and it passed.

So I agree that a defensive nil check shouldn't be added without a concrete use case. In this case, however, the integration test demonstrates that MetadataV2 == nil can be a valid persisted state and that the object can reach this update path, causing a production-relevant runtime panic.

I can also share the integration test/reproduction steps and stack trace if that would help with verification.

[anubhavsingh2106]

@Warashi Just to add, I’ve already opened another PR for this issue, and the same question was raised there as well regarding whether this state can actually occur in practice.

I investigated it in that PR using the real MySQL datastore and was able to reproduce the panic with a persisted Deployment where MetadataV2 is nil. I’ve described the complete reproduction flow and the resulting stack trace above.

So this isn't based only on a hypothetical nil state or a mocked object; I was able to reproduce it through the actual persistence path.

[Warashi]

@anubhavsingh2106
Thank you.
I want to know how to reproduce this issue with real piped data, not with data modified by hand.
It's an unexpected state, so I want to resolve it, not just use a workaround.

[omlahore]

@Warashi The nil state is not hand-made data. It is what pipedv0 itself writes, and nothing currently stops pipedv1 from picking those deployments up.

1. pipedv0 writes the nil

pkg/app/piped/trigger/deployment.go:74 builds the deployment with Metadata: metadata and never sets MetadataV2. pkg/app/pipedv1/trigger/deployment.go:99 is the only place in the tree that populates MetadataV2.

The control plane stores it as given. CreateDeployment passes req.Deployment straight to deploymentStore.Add (pkg/app/server/grpcapi/piped_api.go:401), and Add only backfills CreatedAt, UpdatedAt and stage AvailableOperation. Validate() accepts the nil, because the generated DeploymentMetadata.validate returns early on a nil receiver (pkg/model/deployment.pb.validate.go:1116).

It survives the read back. MySQL serialises with encoding/json (pkg/datastore/mysql/model_wrapper.go:113) and Firestore uses DataTo, both of which round-trip a nil message pointer as nil.

A test against the real v0 builder, no hand-written struct:

d, err := buildDeployment(app, "main", git.Commit{Hash: "0123456789abcdef"}, "commander",
    model.SyncStrategy_AUTO, "", time.Now(), nil, "", 0)
require.NoError(t, err)

assert.Nil(t, d.MetadataV2)   // passes
assert.NotNil(t, d.Metadata)  // passes

encoded, _ := json.Marshal(d)
var loaded model.Deployment
json.Unmarshal(encoded, &loaded)
assert.Nil(t, loaded.MetadataV2) // passes

So every row pipedv0 triggered has MetadataV2 == nil. That is ordinary production data.

2. Nothing enforces "pipedv1 will not pick up pipedv0's deployment"

ListNotCompletedDeployments (pkg/app/server/grpcapi/piped_api.go:353) filters on exactly two things: PipedId equals the caller, and Status is in the not-completed set. There is no field distinguishing a v0-triggered deployment from a v1-triggered one.

Downstream of that there is no filter either. The piped-side lister buckets purely by status (pkg/app/pipedv1/apistore/deploymentstore/store.go:117), syncPlanners and syncSchedulers take whatever comes back, and both startNewPlanner (controller.go:434) and startNewScheduler (controller.go:578) call c.metadataStoreRegistry.Register(d) unconditionally. validateDeploymentBelongsToPiped (piped_api.go:1205) checks only the piped ID as well.

Add already carries the comment "To make compatibility with pipedv0 and pipedv1 on the UI. TODO: Remove this block after ending support of pipedv0", so both shapes are expected to sit in one datastore while v0 is supported.

3. Reproduction

1. Run pipedv0 with piped ID X. Trigger a deployment for an application whose pipeline has a SCRIPT_RUN stage. It is persisted with MetadataV2 nil and reaches PLANNED.
2. Upgrade that piped to pipedv1 in place, same piped ID and same token, while the deployment is still PLANNED or RUNNING.
3. pipedv1 calls ListNotCompletedDeployments, and the v0 deployment comes back because the piped ID and status both match.
4. syncSchedulers hands it to startNewScheduler, which registers it in the metadata store registry.
5. The scheduler resolves the stage's plugin by stage name alone (scheduler.go:572, GetPluginClientByStageName). SCRIPT_RUN is pkg/model/stage.go:32 and the pipedv1 scriptrun plugin registers the same string, so it resolves.
6. executeScriptRun calls PutDeploymentPluginMetadata as its first action, before it even looks at opts.Run (pkg/app/pipedv1/plugin/scriptrun/plugin.go:104).
7. That goes registry.go:119 to pluginPutMulti to SaveDeploymentPluginMetadata (metadatastore/store.go:116) to UpdatePluginMetadata, which does d.MetadataV2.Plugins on the nil pointer (deploymentstore.go:264).

Step 6 is the part that makes this not hypothetical: the scriptrun plugin writes plugin metadata on every single SCRIPT_RUN stage, unconditionally. The ECS plugin does the same in its canary and traffic-routing paths.

4. Two corrections to the issue as filed

UpdateSharedMetadata is not reachable today. Nothing calls the SaveDeploymentSharedMetadata RPC, and the plugin SDK exposes only GetDeploymentSharedMetadata, with no Put counterpart. UpdatePluginMetadata is the one that can actually be hit.

The blast radius is larger than a failed request. The piped API server runs with log, piped token auth, request validation and optionally Prometheus interceptors (cmd/controlplane/server.go:218). None of them recovers, and grpc-go does not recover handler panics, so this takes the control plane process down for every piped connected to it.

5. On the fix

I agree a nil check on its own is a workaround. The invariant you named is the right one, it just is not enforced anywhere. Enforcing it means either filtering v0-triggered deployments out of ListNotCompletedDeployments or rejecting them in the pipedv1 controller, keyed off the piped's reported version or the same v0 marker Add already uses. Happy to send that instead if you prefer it over the guard.

[Warashi]

Upgrade that piped to pipedv1 in place, same piped ID and same token, while the deployment is still PLANNED or RUNNING.

I think this is an unexpected operation.
When you are upgrading piped to pipedv1, you should ensure there are no ongoing deployments, because upgrading causes downtime.

[omlahore]

Fair, I shouldn't have hung it on upgrading mid-deployment.

But stopping the piped doesn't finish its deployments. On a termination signal the v0 scheduler returns without reporting the deployment completed (pkg/app/piped/controller/scheduler.go:424), so the row stays PLANNED or RUNNING. That looks deliberate, pipedv1/controller/controller.go:198 says "after piped is restarted all running deployments need to be loaded firstly".

And the migration guide's only precondition is "stop your existing Piped process to avoid conflicts, as both versions use the same piped-id". It even suggests triggering a v0 deployment right before the switch to verify config compatibility.

There's no cleanup for stale rows anywhere in pkg/app/ops either, so one left behind by a v0 piped that got OOM-killed months ago is still sitting there when v1 first starts with that piped id. That's the case I care about, the operator didn't do anything wrong at switch time.

Guard in ListNotCompletedDeployments, or in the v1 controller?