Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Confirm that we don't trivially leak scorecards in any kind of finalize #6288

Closed
wants to merge 1 commit into from
Closed

Confirm that we don't trivially leak scorecards in any kind of finalize #6288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions server/build_event_protocol/build_event_handler/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ go_test(
"//proto:invocation_status_go_proto",
"//proto:publish_build_event_go_proto",
"//server/backends/chunkstore",
"//server/backends/memory_metrics_collector",
"//server/eventlog",
"//server/tables",
"//server/testutil/testauth",
Expand Down
76 changes: 76 additions & 0 deletions server/build_event_protocol/build_event_handler/build_event_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"github.com/buildbuddy-io/buildbuddy/server/backends/chunkstore"
"github.com/buildbuddy-io/buildbuddy/server/backends/memory_metrics_collector"
"github.com/buildbuddy-io/buildbuddy/server/build_event_protocol/build_event_handler"
"github.com/buildbuddy-io/buildbuddy/server/eventlog"
"github.com/buildbuddy-io/buildbuddy/server/tables"
Expand Down Expand Up @@ -461,6 +462,9 @@ func TestHandleEventWithWorkspaceStatusBeforeStarted(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx := context.Background()
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -509,6 +513,10 @@ func TestHandleEventWithWorkspaceStatusBeforeStarted(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_COMPLETE_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
Copy link
Member

@bduffany bduffany Apr 3, 2024 •
edited

Choose a reason for hiding this comment

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

I think since we are creating "fake" builds by maually streaming BES events, we won't have any cache requests associated with the build, so this assertion would pass even if we didn't flush correctly (because the cache request data in the MetricsCollector will be empty)

The testenv should have a cache client available as te.GetPooledByteStreamClient(), so it might work to create a dedicated test that opens a BES stream, uploads a blob (cachetools.UploadBlob) then downloads the blob (with the invocation ID set in RequestMetadata, so that it gets associated with the BES stream - can use bazel_request.WithRequestMetadata for the DownloadBlob ctx), then making assertions on the counts before/after calling finalize

assert.NoError(t, err)
assert.Empty(t, metrics)
}

func TestHandleEventWithEnvAndMetadataRedaction(t *testing.T) {
Expand Down Expand Up @@ -607,6 +615,9 @@ func TestFinishedFinalizeWithCanceledContext(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx, cancel := context.WithCancel(context.Background())
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -649,12 +660,19 @@ func TestFinishedFinalizeWithCanceledContext(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_COMPLETE_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)
}

func TestFinishedFinalize(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx, cancel := context.WithCancel(context.Background())
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -695,12 +713,19 @@ func TestFinishedFinalize(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_COMPLETE_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)
}

func TestUnfinishedFinalizeWithCanceledContext(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx, cancel := context.WithCancel(context.Background())
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -738,12 +763,19 @@ func TestUnfinishedFinalizeWithCanceledContext(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_DISCONNECTED_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)
}

func TestUnfinishedFinalize(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx, cancel := context.WithCancel(context.Background())
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -779,12 +811,19 @@ func TestUnfinishedFinalize(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_DISCONNECTED_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)
}

func TestRetryOnComplete(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx := context.Background()
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -832,6 +871,10 @@ func TestRetryOnComplete(t *testing.T) {
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_COMPLETE_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)

exists, err := te.GetBlobstore().BlobExists(ctx, protofile.ChunkName(build_event_handler.GetStreamIdFromInvocationIdAndAttempt(testInvocationID, 1), 0))
assert.NoError(t, err)
assert.True(t, exists)
Expand Down Expand Up @@ -859,6 +902,9 @@ func TestRetryOnDisconnect(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx := context.Background()
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -901,6 +947,10 @@ func TestRetryOnDisconnect(t *testing.T) {
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_DISCONNECTED_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)

exists, err := te.GetBlobstore().BlobExists(ctx, protofile.ChunkName(build_event_handler.GetStreamIdFromInvocationIdAndAttempt(testInvocationID, 1), 0))
assert.NoError(t, err)
assert.True(t, exists)
Expand Down Expand Up @@ -951,6 +1001,10 @@ func TestRetryOnDisconnect(t *testing.T) {
assert.Equal(t, "def456", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_COMPLETE_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err = te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)

// Make sure the new protofile exists
exists, err = te.GetBlobstore().BlobExists(ctx, protofile.ChunkName(build_event_handler.GetStreamIdFromInvocationIdAndAttempt(testInvocationID, 2), 0))
assert.NoError(t, err)
Expand All @@ -966,6 +1020,9 @@ func TestRetryTwiceOnDisconnect(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx := context.Background()
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -1008,6 +1065,10 @@ func TestRetryTwiceOnDisconnect(t *testing.T) {
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_DISCONNECTED_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)

exists, err := te.GetBlobstore().BlobExists(ctx, protofile.ChunkName(build_event_handler.GetStreamIdFromInvocationIdAndAttempt(testInvocationID, 1), 0))
assert.NoError(t, err)
assert.True(t, exists)
Expand Down Expand Up @@ -1058,6 +1119,10 @@ func TestRetryTwiceOnDisconnect(t *testing.T) {
assert.Equal(t, "def456", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_DISCONNECTED_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err = te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)

exists, err = te.GetBlobstore().BlobExists(ctx, protofile.ChunkName(build_event_handler.GetStreamIdFromInvocationIdAndAttempt(testInvocationID, 2), 0))
assert.NoError(t, err)
assert.True(t, exists)
Expand Down Expand Up @@ -1113,6 +1178,10 @@ func TestRetryTwiceOnDisconnect(t *testing.T) {
assert.Equal(t, "000789", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_COMPLETE_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err = te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)

// Make sure all protofiles exist
exists, err = te.GetBlobstore().BlobExists(ctx, protofile.ChunkName(build_event_handler.GetStreamIdFromInvocationIdAndAttempt(testInvocationID, 1), 0))
assert.NoError(t, err)
Expand Down Expand Up @@ -1144,6 +1213,9 @@ func TestRetryOnOldDisconnect(t *testing.T) {
te := testenv.GetTestEnv(t)
auth := testauth.NewTestAuthenticator(testauth.TestUsers("USER1", "GROUP1"))
te.SetAuthenticator(auth)
mc, err := memory_metrics_collector.NewMemoryMetricsCollector()
assert.NoError(t, err)
te.SetMetricsCollector(mc)
ctx := context.Background()
testUUID, err := uuid.NewRandom()
assert.NoError(t, err)
Expand Down Expand Up @@ -1189,6 +1261,10 @@ func TestRetryOnOldDisconnect(t *testing.T) {
assert.Equal(t, "abc123", invocation.CommitSha)
assert.Equal(t, inspb.InvocationStatus_DISCONNECTED_INVOCATION_STATUS, invocation.InvocationStatus)

metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID)
assert.NoError(t, err)
assert.Empty(t, metrics)

exists, err := te.GetBlobstore().BlobExists(ctx, protofile.ChunkName(build_event_handler.GetStreamIdFromInvocationIdAndAttempt(testInvocationID, 1), 0))
assert.NoError(t, err)
assert.True(t, exists)
Expand Down