Skip to content

Commit dab2c61

Browse files
lostlevelslostlevels
authored
[BACK-3245] Cleanup device logs on context canceled and respond with appropriate error code when unable to retrieve device logs contents. (#786)
1 parent 134adc6 commit dab2c61

3 files changed

Lines changed: 94 additions & 33 deletions

File tree

‎blob/service/api/v1/v1.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -280,12 +280,11 @@ func (r *Router) GetDeviceLogsContent(res rest.ResponseWriter, req *rest.Request
280280
}
281281

282282
content, err := blobClient.GetDeviceLogsContent(req.Context(), *deviceLogMetadata.ID)
283-
if err != nil {
284-
responder.Error(http.StatusInternalServerError, err)
283+
if responder.RespondIfError(err) {
285284
return
286285
}
287286
if content == nil || content.Body == nil {
288-
responder.Error(http.StatusNotFound, request.ErrorResourceNotFoundWithID(deviceLogID))
287+
responder.RespondIfError(request.ErrorResourceNotFoundWithID(deviceLogID))
289288
return
290289
}
291290
defer content.Body.Close()

‎blob/service/api/v1/v1_test.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -520,6 +520,20 @@ var _ = Describe("V1", func() {
520520
errorsTest.ExpectErrorJSON(request.ErrorResourceNotFoundWithID(id), res.WriteInputs[0])
521521
})
522522

523+
It("responds with not found error when the client returns a blob but the blob has no content", func() {
524+
// This has happened when blob creation succeeded but the request context canceled before the contents finished uploading to S3 so there is a "stray" blob w/o content
525+
deviceLogsBlob := blobTest.RandomDeviceLogsBlob()
526+
client.GetDeviceLogsBlobOutputs = []blobTest.GetDeviceLogsBlobOutput{{Blob: deviceLogsBlob}}
527+
client.GetDeviceLogsContentOutputs = []blobTest.GetDeviceLogsContentOutput{{Error: request.ErrorResourceNotFoundWithID(*deviceLogsBlob.ID)}}
528+
res.WriteOutputs = []testRest.WriteOutput{{BytesWritten: 0, Error: nil}}
529+
530+
handlerFunc(res, req)
531+
Expect(res.WriteHeaderInputs).To(Equal([]int{http.StatusNotFound}))
532+
Expect(res.HeaderOutput).To(Equal(&http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}))
533+
Expect(res.WriteInputs).To(HaveLen(1))
534+
errorsTest.ExpectErrorJSON(request.ErrorResourceNotFoundWithID(*deviceLogsBlob.ID), res.WriteInputs[0])
535+
})
536+
523537
It("responds successfully with headers", func() {
524538
deviceLogsBlob := blobTest.RandomDeviceLogsBlob()
525539
content := blob.NewDeviceLogsContent()
@@ -606,6 +620,19 @@ var _ = Describe("V1", func() {
606620
errorsTest.ExpectErrorJSON(request.ErrorResourceNotFoundWithID(id), res.WriteInputs[0])
607621
})
608622

623+
It("responds with not found error when the client returns a blob but the blob has no content", func() {
624+
deviceLogsBlob := blobTest.RandomDeviceLogsBlob()
625+
deviceLogsBlob.UserID = pointer.FromString(userID)
626+
client.GetDeviceLogsBlobOutputs = []blobTest.GetDeviceLogsBlobOutput{{Blob: deviceLogsBlob}}
627+
client.GetDeviceLogsContentOutputs = []blobTest.GetDeviceLogsContentOutput{{Error: request.ErrorResourceNotFoundWithID(*deviceLogsBlob.ID)}}
628+
res.WriteOutputs = []testRest.WriteOutput{{BytesWritten: 0, Error: nil}}
629+
630+
handlerFunc(res, req)
631+
Expect(res.WriteHeaderInputs).To(Equal([]int{http.StatusNotFound}))
632+
Expect(res.HeaderOutput).To(Equal(&http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}))
633+
Expect(res.WriteInputs).To(HaveLen(1))
634+
errorsTest.ExpectErrorJSON(request.ErrorResourceNotFoundWithID(*deviceLogsBlob.ID), res.WriteInputs[0])
635+
})
609636
It("responds successfully with headers for user's own logs content", func() {
610637
deviceLogsBlob := blobTest.RandomDeviceLogsBlob()
611638
deviceLogsBlob.UserID = pointer.FromString(userID)

‎blob/service/client/client.go‎

Lines changed: 65 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"crypto/md5"
66
"encoding/base64"
77
"io"
8+
"time"
89

910
"github.com/tidepool-org/platform/blob"
1011
blobStoreStructured "github.com/tidepool-org/platform/blob/store/structured"
@@ -19,6 +20,11 @@ import (
1920
structureValidator "github.com/tidepool-org/platform/structure/validator"
2021
)
2122

23+
const (
24+
// arbritrary timeout value for cleanup operations such as deleting from S3 or Repo
25+
defaultCleanupTimeout = time.Second * 5
26+
)
27+
2228
type Provider interface {
2329
BlobStructuredStore() blobStoreStructured.Store
2430
BlobUnstructuredStore() blobStoreUnstructured.Store
@@ -70,31 +76,40 @@ func (c *Client) Create(ctx context.Context, userID string, content *blob.Conten
7076
options.MediaType = content.MediaType
7177
err = c.BlobUnstructuredStore().Put(ctx, userID, *result.ID, io.TeeReader(io.TeeReader(io.LimitReader(content.Body, blob.SizeMaximum+1), hasher), sizer), options)
7278
if err != nil {
73-
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
74-
logger.WithError(destroyErr).Error("Unable to destroy blob after failure to put blob content")
75-
}
79+
alwaysDo(ctx, defaultCleanupTimeout, func(ctx context.Context) error {
80+
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
81+
logger.WithError(destroyErr).Error("Unable to destroy blob after failure to put blob content")
82+
}
83+
return nil
84+
})
7685
return nil, err
7786
}
7887

7988
size := sizer.Size
8089
if size > blob.SizeMaximum {
81-
if _, deleteErr := c.BlobUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
82-
logger.WithError(deleteErr).Error("Unable to delete blob content exceeding maximum size")
83-
}
84-
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
85-
logger.WithError(destroyErr).Error("Unable to destroy blob exceeding maximum size")
86-
}
90+
alwaysDo(ctx, defaultCleanupTimeout, func(ctx context.Context) error {
91+
if _, deleteErr := c.BlobUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
92+
logger.WithError(deleteErr).Error("Unable to delete blob content exceeding maximum size")
93+
}
94+
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
95+
logger.WithError(destroyErr).Error("Unable to destroy blob exceeding maximum size")
96+
}
97+
return nil
98+
})
8799
return nil, request.ErrorResourceTooLarge()
88100
}
89101

90102
digestMD5 := base64.StdEncoding.EncodeToString(hasher.Sum(nil))
91103
if content.DigestMD5 != nil && *content.DigestMD5 != digestMD5 {
92-
if _, deleteErr := c.BlobUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
93-
logger.WithError(deleteErr).Error("Unable to delete blob content with incorrect MD5 digest")
94-
}
95-
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
96-
logger.WithError(destroyErr).Error("Unable to destroy blob with incorrect MD5 digest")
97-
}
104+
alwaysDo(ctx, defaultCleanupTimeout, func(ctx context.Context) error {
105+
if _, deleteErr := c.BlobUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
106+
logger.WithError(deleteErr).Error("Unable to delete blob content with incorrect MD5 digest")
107+
}
108+
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
109+
logger.WithError(destroyErr).Error("Unable to destroy blob with incorrect MD5 digest")
110+
}
111+
return nil
112+
})
98113
return nil, errors.WithSource(request.ErrorDigestsNotEqual(*content.DigestMD5, digestMD5), structure.NewPointerSource().WithReference("digestMD5"))
99114
}
100115

@@ -129,31 +144,41 @@ func (c *Client) CreateDeviceLogs(ctx context.Context, userID string, content *b
129144
options.MediaType = content.MediaType
130145
err = c.DeviceLogsUnstructuredStore().Put(ctx, userID, *result.ID, io.TeeReader(io.TeeReader(io.LimitReader(content.Body, blob.SizeMaximum+1), hasher), sizer), options)
131146
if err != nil {
132-
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
133-
logger.WithError(destroyErr).Error("Unable to destroy blob after failure to put blob content")
134-
}
147+
alwaysDo(ctx, defaultCleanupTimeout, func(ctx context.Context) error {
148+
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
149+
logger.WithError(destroyErr).Error("Unable to destroy blob after failure to put blob content")
150+
}
151+
return nil
152+
})
135153
return nil, err
136154
}
137155

138156
size := sizer.Size
139157
if size > blob.SizeMaximum {
140-
if _, deleteErr := c.DeviceLogsUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
141-
logger.WithError(deleteErr).Error("Unable to delete blob content exceeding maximum size")
142-
}
143-
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
144-
logger.WithError(destroyErr).Error("Unable to destroy blob exceeding maximum size")
145-
}
158+
alwaysDo(ctx, defaultCleanupTimeout, func(ctx context.Context) error {
159+
if _, deleteErr := c.DeviceLogsUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
160+
logger.WithError(deleteErr).Error("Unable to delete blob content exceeding maximum size")
161+
}
162+
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
163+
logger.WithError(destroyErr).Error("Unable to destroy blob exceeding maximum size")
164+
}
165+
return nil
166+
})
146167
return nil, request.ErrorResourceTooLarge()
147168
}
148169

149170
digestMD5 := base64.StdEncoding.EncodeToString(hasher.Sum(nil))
150171
if content.DigestMD5 != nil && *content.DigestMD5 != digestMD5 {
151-
if _, deleteErr := c.DeviceLogsUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
152-
logger.WithError(deleteErr).Error("Unable to delete blob content with incorrect MD5 digest")
153-
}
154-
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
155-
logger.WithError(destroyErr).Error("Unable to destroy blob with incorrect MD5 digest")
156-
}
172+
alwaysDo(ctx, defaultCleanupTimeout, func(ctx context.Context) error {
173+
if _, deleteErr := c.DeviceLogsUnstructuredStore().Delete(ctx, userID, *result.ID); deleteErr != nil {
174+
logger.WithError(deleteErr).Error("Unable to delete blob content with incorrect MD5 digest")
175+
}
176+
if _, destroyErr := repository.Destroy(ctx, *result.ID, nil); destroyErr != nil {
177+
logger.WithError(destroyErr).Error("Unable to destroy blob with incorrect MD5 digest")
178+
}
179+
return nil
180+
})
181+
157182
return nil, errors.WithSource(request.ErrorDigestsNotEqual(*content.DigestMD5, digestMD5), structure.NewPointerSource().WithReference("digestMD5"))
158183
}
159184

@@ -287,3 +312,13 @@ func (s *SizeWriter) Write(bites []byte) (int, error) {
287312
s.Size += length
288313
return length, nil
289314
}
315+
316+
// alwaysDo performs an action given a context even if the context is
317+
// canceled or timed out. This is used if we have any cleanup functions that we
318+
// still want to perform and passing the parent context would time out any
319+
// child contexts.
320+
func alwaysDo(ctx context.Context, timeout time.Duration, fn func(ctx context.Context) error) error {
321+
newContext, cancel := context.WithTimeout(context.WithoutCancel(ctx), timeout)
322+
defer cancel()
323+
return fn(newContext)
324+
}

0 commit comments

Comments
 (0)