Skip to content

Commit 2a38987

Browse files
committed
fix/batch-changes: ignore per-job env vars in cache key
sourcegraph/sourcegraph#12503 renders codingAgent steps server-side and declares two per-job env-var forwards on each desugared step's env (SRC_BATCHES_MODEL_PROVIDER_TOKEN and SRC_BATCHES_JOB_ID). Their values are different on every dequeue, so including them in cache-key inputs would bust the cache on every run. Strip them before hashing so cache keys stay stable across dequeues. The bare-string declarations still influence the key via the serialized step.Env. Mirrored on the Sourcegraph side.
1 parent ed8850d commit 2a38987

2 files changed

Lines changed: 84 additions & 0 deletions

File tree

lib/batches/execution/cache/cache.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,13 @@ func (key CacheKey) mountsMetadata() ([]MountMetadata, error) {
4747
return nil, nil
4848
}
4949

50+
// perJobEnvVars resolve to per-job values and must be stripped from
51+
// cache keys. Mirrored in sourcegraph/sourcegraph/lib/batches/execution/cache/cache.go.
52+
var perJobEnvVars = []string{
53+
"SRC_BATCHES_MODEL_PROVIDER_TOKEN",
54+
"SRC_BATCHES_JOB_ID",
55+
}
56+
5057
// resolveStepsEnvironment returns a slice of environments for each of the steps,
5158
// containing only the env vars that are actually used.
5259
func resolveStepsEnvironment(globalEnv []string, steps []batches.Step) ([]map[string]string, error) {
@@ -64,6 +71,9 @@ func resolveStepsEnvironment(globalEnv []string, steps []batches.Step) ([]map[st
6471
if err != nil {
6572
return nil, errors.Wrapf(err, "resolving environment for step %d", i)
6673
}
74+
for _, name := range perJobEnvVars {
75+
delete(env, name)
76+
}
6777
envs[i] = env
6878
}
6979
return envs, nil
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
package cache
2+
3+
import (
4+
"encoding/json"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
10+
"github.com/sourcegraph/sourcegraph/lib/batches"
11+
"github.com/sourcegraph/sourcegraph/lib/batches/env"
12+
)
13+
14+
var repo = batches.Repository{
15+
ID: "my-repo",
16+
Name: "github.com/sourcegraph/src-cli",
17+
BaseRef: "refs/heads/f00b4r",
18+
BaseRev: "c0mmit",
19+
FileMatches: []string{"baz.go"},
20+
}
21+
22+
// TestKeyer_Key_PerJobEnvVarsIgnored verifies that the per-job
23+
// codingAgent env vars (SRC_BATCHES_MODEL_PROVIDER_TOKEN and
24+
// SRC_BATCHES_JOB_ID) do not influence the cache key value, even though
25+
// they are declared as bare-string forwards on the step. Without this,
26+
// every dequeue would invalidate the cache because the Sourcegraph
27+
// server resolves those vars to one value (empty) and src-cli resolves
28+
// them to another (the per-job values exported into the executor
29+
// container env).
30+
func TestKeyer_Key_PerJobEnvVarsIgnored(t *testing.T) {
31+
var stepEnv env.Environment
32+
require.NoError(t, json.Unmarshal(
33+
[]byte(`["SRC_BATCHES_MODEL_PROVIDER_TOKEN", "SRC_BATCHES_JOB_ID", "USER_VAR"]`),
34+
&stepEnv,
35+
))
36+
37+
step := batches.Step{Run: "foo", Env: stepEnv}
38+
39+
serverKey := CacheKey{
40+
Repository: repo,
41+
Steps: []batches.Step{step},
42+
StepIndex: 0,
43+
// Server-side: vars unset → empty resolved values.
44+
GlobalEnv: []string{"USER_VAR=hello"},
45+
}
46+
srcCLIKey := CacheKey{
47+
Repository: repo,
48+
Steps: []batches.Step{step},
49+
StepIndex: 0,
50+
// src-cli side: vars present with per-job values.
51+
GlobalEnv: []string{
52+
"USER_VAR=hello",
53+
"SRC_BATCHES_MODEL_PROVIDER_TOKEN=ephemeral-job-token",
54+
"SRC_BATCHES_JOB_ID=42",
55+
},
56+
}
57+
58+
a, err := serverKey.Key()
59+
assert.NoError(t, err)
60+
b, err := srcCLIKey.Key()
61+
assert.NoError(t, err)
62+
assert.Equal(t, a, b, "excluded env vars must not affect the cache key")
63+
64+
// Changing a non-excluded user var still invalidates the key.
65+
userVarChanged := CacheKey{
66+
Repository: repo,
67+
Steps: []batches.Step{step},
68+
StepIndex: 0,
69+
GlobalEnv: []string{"USER_VAR=world"},
70+
}
71+
c, err := userVarChanged.Key()
72+
assert.NoError(t, err)
73+
assert.NotEqual(t, a, c, "non-excluded env vars must still affect the cache key")
74+
}

0 commit comments

Comments
 (0)