Skip to content

fix(pam): stop retrying rejected recording chunks - #392

Open
bernie-g wants to merge 1 commit into
mainfrom
bernie/pam-463-investigate-session-recording-endpoint-error-spike
Open

fix(pam): stop retrying rejected recording chunks#392
bernie-g wants to merge 1 commit into
mainfrom
bernie/pam-463-investigate-session-recording-endpoint-error-spike

Conversation

@bernie-g

@bernie-g bernie-g commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description 📣

Chunk reconciliation retried every failed upload on a 5-minute tick with no give-up, so a chunk the platform rejects for a reason that cannot change 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.

Fixes the gateway half of PAM-463; the platform half is in Infisical/infisical#8043. This is the only change that clears an already-stuck queue, so it needs to reach affected gateways for the error rate to drop.

Type ✨

  • Bug fix

Tests 🛠️

Four new tests drive UploadChunk against a real httptest server with config.INFISICAL_URL overridden, so the status code travels the genuine production path (resty response → NewAPIErrorWithResponsefmt.Errorf("%w")errors.As) rather than a synthetic error:

  • a real 400 Invalid upload token drops the chunk
  • a real 429 and a real 500 both keep it queued
  • a queue of five permanently rejected chunks drains in one reconcile pass and makes zero requests on the second pass

The second commit adds singleflight around session credential fetches. Recording secrets are minted on a session's first fetch, and the cache check released its lock before the API call, so two connections on one session both fetched and both minted, leaving this gateway holding a token the platform had discarded. Covered by a test that fires 16 concurrent callers and asserts one fetch, run under -race.

go test ./packages/pam/... 
go test ./packages/pam/session/ -run "Real|DrainsPermanently|Cached|Singleflight" -race -v

Also built the binary from this branch and ran it as a real gateway against a local platform: it enrolls, connects to the relay, heartbeats, and starts the session uploader with no errors.


@linear

linear Bot commented Sep 8, 2026

Copy link
Copy Markdown

PAM-463

@infisical-review-police

Copy link
Copy Markdown

💬 Discussion in Slack: #pr-review-cli-392-fix-pam-stop-retrying-rejected-recording-chunks

Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel.

@gitguardian

gitguardian Bot commented Sep 8, 2026

Copy link
Copy Markdown

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Comment thread packages/pam/session/chunk_uploader.go Outdated
@veria-ai

veria-ai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 3 · PR risk: 0/10

@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

The PR should not merge until recoverable invalid-token responses stop causing irreversible loss of queued PAM recording data.

Findings

  1. P1 Recoverable Chunks Are Deleted
  2. P2 Failed Deletions Stay Queued
  3. P2 Test Bypasses Production Fetch

Summary

  • Classifies several API response statuses as permanent and deletes their pending recording chunks.
  • Deduplicates concurrent credential fetches for the same session.
  • Adds real-HTTP chunk upload coverage and unit tests for failure classification and singleflight behavior.
  • The permanent-failure classification currently discards a recoverable stale-token chunk, and the new concurrency test bypasses the production method it is intended to protect.

Comment thread packages/pam/session/chunk_uploader.go Outdated
Comment thread packages/pam/session/chunk_uploader.go Outdated
Comment thread packages/pam/session/credentials_test.go Outdated
Comment thread packages/pam/session/chunk_uploader.go Outdated
Comment thread packages/pam/session/chunk_uploader.go
@bernie-g
bernie-g requested a review from lb-vn September 8, 2026 19:11
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.
@bernie-g
bernie-g force-pushed the bernie/pam-463-investigate-session-recording-endpoint-error-spike branch from 9a100f7 to f00f424 Compare September 8, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant