Skip to content

Commit f00f424

Browse files
committed
fix(pam): stop retrying rejected recording chunks
Chunk reconciliation retried every failed upload on a five-minute tick with no give-up, so a chunk the platform can only ever reject was re-POSTed for the life of the gateway. One stuck queue produced 22,511 errors in three hours in production, and the burst also saturated the write rate limit. Treat 400, 403 and 404 as permanent and drop the chunk; keep 401, 429, 5xx and network failures retriable. A failed removal is reported rather than assumed, since such a chunk stays queued and keeps failing.
1 parent af33991 commit f00f424

3 files changed

Lines changed: 238 additions & 0 deletions

File tree

packages/pam/session/chunk_uploader.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"crypto/sha256"
1010
"encoding/base64"
1111
"encoding/json"
12+
"errors"
1213
"fmt"
1314
"io"
1415
"net/http"
@@ -263,6 +264,41 @@ func (cu *ChunkUploader) EncryptAndQueueChunk(
263264
return pc, nil
264265
}
265266

267+
// Reconciliation never gives up, so a chunk that can only ever be rejected retries forever.
268+
func isPermanentUploadFailure(err error) bool {
269+
var apiErr *api.APIError
270+
if !errors.As(err, &apiErr) {
271+
return false
272+
}
273+
switch apiErr.StatusCode {
274+
case http.StatusBadRequest, http.StatusForbidden, http.StatusNotFound:
275+
return true
276+
default:
277+
return false
278+
}
279+
}
280+
281+
// flushSession has already advanced past it, so keeping the file only feeds the retry loop.
282+
func (cu *ChunkUploader) dropIfPermanent(sessionID string, pc *pendingChunk, err error) {
283+
if !isPermanentUploadFailure(err) {
284+
return
285+
}
286+
287+
entry := log.Error().
288+
Err(err).
289+
Str("sessionId", sessionID).
290+
Int("chunkIndex", pc.ChunkIndex).
291+
Int("ciphertextBytes", len(pc.Ciphertext))
292+
293+
if rmErr := os.Remove(chunkPendingFile(sessionID, pc.ChunkIndex)); rmErr != nil {
294+
entry.AnErr("removeError", rmErr).
295+
Msg("Recording chunk permanently rejected but could not be removed; it stays queued and will keep failing")
296+
return
297+
}
298+
299+
entry.Msg("Recording chunk permanently rejected by platform; dropped instead of retrying")
300+
}
301+
266302
func (cu *ChunkUploader) UploadChunk(sessionID string, pc *pendingChunk) error {
267303
secrets := cu.credentialsManager.GetRecordingSecrets(sessionID)
268304
if secrets == nil {
@@ -281,6 +317,7 @@ func (cu *ChunkUploader) UploadChunk(sessionID string, pc *pendingChunk) error {
281317
},
282318
)
283319
if err != nil {
320+
cu.dropIfPermanent(sessionID, pc, err)
284321
return fmt.Errorf("presigned PUT mint failed: %w", err)
285322
}
286323
if err := s3PutCiphertext(presigned.URL, pc.Ciphertext); err != nil {
@@ -305,6 +342,7 @@ func (cu *ChunkUploader) UploadChunk(sessionID string, pc *pendingChunk) error {
305342
}
306343

307344
if err := api.CallPAMSessionChunkMetadata(cu.httpClient, sessionID, secrets.UploadToken, metadataReq); err != nil {
345+
cu.dropIfPermanent(sessionID, pc, err)
308346
return fmt.Errorf("chunk metadata POST failed: %w", err)
309347
}
310348

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
package session
2+
3+
import (
4+
"net/http"
5+
"net/http/httptest"
6+
"os"
7+
"testing"
8+
9+
"github.com/Infisical/infisical-merge/packages/config"
10+
"github.com/go-resty/resty/v2"
11+
)
12+
13+
// Uses a real HTTP server so the status code travels the same path it does in production.
14+
func newChunkUploaderAgainst(t *testing.T, status int, body string) (*ChunkUploader, *int) {
15+
t.Helper()
16+
17+
hits := 0
18+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
19+
hits += 1
20+
w.Header().Set("Content-Type", "application/json")
21+
w.WriteHeader(status)
22+
_, _ = w.Write([]byte(body))
23+
}))
24+
t.Cleanup(srv.Close)
25+
26+
orig := config.INFISICAL_URL
27+
config.INFISICAL_URL = srv.URL + "/api"
28+
t.Cleanup(func() { config.INFISICAL_URL = orig })
29+
30+
cm := newTestCredentialsManager(t)
31+
cm.recordingSecrets["sess"] = &PAMRecordingSecrets{
32+
SessionKey: make([]byte, 32),
33+
UploadToken: "fake-token-for-test",
34+
StorageBackend: storageBackendPostgres,
35+
ProjectId: "proj",
36+
SessionId: "sess",
37+
}
38+
39+
return NewChunkUploader(resty.New(), cm), &hits
40+
}
41+
42+
func queuedChunk(t *testing.T, sessionID string) *pendingChunk {
43+
t.Helper()
44+
pc := &pendingChunk{
45+
ChunkIndex: 0,
46+
StartElapsedMs: 0,
47+
EndElapsedMs: 10,
48+
IV: make([]byte, 12),
49+
Ciphertext: []byte("ciphertext"),
50+
Sha256: make([]byte, 32),
51+
StorageBackend: storageBackendPostgres,
52+
}
53+
if err := writePendingChunk(sessionID, pc); err != nil {
54+
t.Fatal(err)
55+
}
56+
return pc
57+
}
58+
59+
func TestUploadChunk_RealInvalidUploadTokenIsDropped(t *testing.T) {
60+
setupTestDir(t)
61+
62+
cu, hits := newChunkUploaderAgainst(t, http.StatusBadRequest,
63+
`{"reqId":"req-x","statusCode":400,"message":"Invalid upload token","error":"PamUploadTokenHashMismatch"}`)
64+
pc := queuedChunk(t, "sess")
65+
66+
if err := cu.UploadChunk("sess", pc); err == nil {
67+
t.Fatal("expected UploadChunk to surface the 400")
68+
}
69+
if *hits != 1 {
70+
t.Errorf("server saw %d requests, want 1", *hits)
71+
}
72+
if _, err := os.Stat(chunkPendingFile("sess", 0)); !os.IsNotExist(err) {
73+
t.Error("a 400 is permanent: the chunk must be dropped, not left to retry forever")
74+
}
75+
}
76+
77+
func TestUploadChunk_RealRateLimitIsRetained(t *testing.T) {
78+
setupTestDir(t)
79+
80+
cu, _ := newChunkUploaderAgainst(t, http.StatusTooManyRequests,
81+
`{"reqId":"req-y","statusCode":429,"message":"Rate limit exceeded. Please try again in 14 seconds"}`)
82+
pc := queuedChunk(t, "sess")
83+
84+
if err := cu.UploadChunk("sess", pc); err == nil {
85+
t.Fatal("expected UploadChunk to surface the 429")
86+
}
87+
if _, err := os.Stat(chunkPendingFile("sess", 0)); err != nil {
88+
t.Errorf("a 429 is transient: the chunk must stay queued for retry: %v", err)
89+
}
90+
}
91+
92+
func TestUploadChunk_RealServerErrorIsRetained(t *testing.T) {
93+
setupTestDir(t)
94+
95+
cu, _ := newChunkUploaderAgainst(t, http.StatusInternalServerError, `{"statusCode":500,"message":"boom"}`)
96+
pc := queuedChunk(t, "sess")
97+
98+
if err := cu.UploadChunk("sess", pc); err == nil {
99+
t.Fatal("expected UploadChunk to surface the 500")
100+
}
101+
if _, err := os.Stat(chunkPendingFile("sess", 0)); err != nil {
102+
t.Errorf("a 500 is transient: the chunk must stay queued for retry: %v", err)
103+
}
104+
}
105+
106+
func TestReconcileSession_DrainsPermanentlyRejectedQueue(t *testing.T) {
107+
setupTestDir(t)
108+
109+
cu, hits := newChunkUploaderAgainst(t, http.StatusBadRequest,
110+
`{"statusCode":400,"message":"Invalid upload token","error":"PamUploadTokenHashMismatch"}`)
111+
112+
const queued = 5
113+
for i := 0; i < queued; i += 1 {
114+
pc := &pendingChunk{
115+
ChunkIndex: i,
116+
IV: make([]byte, 12),
117+
Ciphertext: []byte("ciphertext"),
118+
Sha256: make([]byte, 32),
119+
StorageBackend: storageBackendPostgres,
120+
}
121+
if err := writePendingChunk("sess", pc); err != nil {
122+
t.Fatal(err)
123+
}
124+
}
125+
126+
cu.ReconcileSession("sess")
127+
if *hits != queued {
128+
t.Errorf("first pass made %d requests, want %d", *hits, queued)
129+
}
130+
131+
before := *hits
132+
cu.ReconcileSession("sess")
133+
if *hits != before {
134+
t.Errorf("second pass made %d more requests; the queue was not drained", *hits-before)
135+
}
136+
137+
if cu.HasPendingChunks("sess") {
138+
t.Error("queue still holds permanently rejected chunks")
139+
}
140+
}

packages/pam/session/chunk_uploader_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@ import (
55
"crypto/cipher"
66
"crypto/sha256"
77
"encoding/json"
8+
"errors"
89
"fmt"
10+
"net/http"
911
"os"
1012
"path/filepath"
1113
"testing"
1214
"time"
15+
16+
"github.com/Infisical/infisical-merge/packages/api"
1317
)
1418

1519
func setupTestDir(t *testing.T) {
@@ -346,6 +350,62 @@ func TestDeletePendingChunk(t *testing.T) {
346350
}
347351
}
348352

353+
func TestIsPermanentUploadFailure(t *testing.T) {
354+
cases := []struct {
355+
name string
356+
err error
357+
want bool
358+
}{
359+
{"body too large is retryable after re-chunking", &api.APIError{StatusCode: http.StatusRequestEntityTooLarge}, false},
360+
{"bad request", &api.APIError{StatusCode: http.StatusBadRequest}, true},
361+
{"forbidden gateway", &api.APIError{StatusCode: http.StatusForbidden}, true},
362+
{"session gone", &api.APIError{StatusCode: http.StatusNotFound}, true},
363+
{"rate limited", &api.APIError{StatusCode: http.StatusTooManyRequests}, false},
364+
{"server error", &api.APIError{StatusCode: http.StatusInternalServerError}, false},
365+
{"unauthorized retries after token refresh", &api.APIError{StatusCode: http.StatusUnauthorized}, false},
366+
{"network error", errors.New("dial tcp: connection refused"), false},
367+
{"wrapped bad request", fmt.Errorf("chunk metadata POST failed: %w", &api.APIError{StatusCode: http.StatusBadRequest}), true},
368+
}
369+
for _, tc := range cases {
370+
t.Run(tc.name, func(t *testing.T) {
371+
if got := isPermanentUploadFailure(tc.err); got != tc.want {
372+
t.Errorf("isPermanentUploadFailure() = %v, want %v", got, tc.want)
373+
}
374+
})
375+
}
376+
}
377+
378+
func TestDropIfPermanent(t *testing.T) {
379+
setupTestDir(t)
380+
cu := newTestChunkUploader(t, newTestCredentialsManager(t))
381+
382+
pc := &pendingChunk{
383+
ChunkIndex: 0,
384+
IV: make([]byte, 12),
385+
Ciphertext: []byte("ct"),
386+
Sha256: make([]byte, 32),
387+
StorageBackend: "postgres",
388+
}
389+
390+
sid := "drop-permanent"
391+
if err := writePendingChunk(sid, pc); err != nil {
392+
t.Fatal(err)
393+
}
394+
cu.dropIfPermanent(sid, pc, fmt.Errorf("wrapped: %w", &api.APIError{StatusCode: http.StatusBadRequest}))
395+
if _, err := os.Stat(chunkPendingFile(sid, 0)); !os.IsNotExist(err) {
396+
t.Error("permanently rejected chunk should have been dropped from the queue")
397+
}
398+
399+
sid = "keep-transient"
400+
if err := writePendingChunk(sid, pc); err != nil {
401+
t.Fatal(err)
402+
}
403+
cu.dropIfPermanent(sid, pc, &api.APIError{StatusCode: http.StatusTooManyRequests})
404+
if _, err := os.Stat(chunkPendingFile(sid, 0)); err != nil {
405+
t.Errorf("rate-limited chunk must stay queued for retry: %v", err)
406+
}
407+
}
408+
349409
func TestPendingChunkJsonStability(t *testing.T) {
350410
pc := &pendingChunk{
351411
ChunkIndex: 2,

0 commit comments

Comments
 (0)