From 515ff0f2842506fee5f43d53fba00c95e1ad25a6 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sat, 24 Oct 2026 05:08:56 -0700 Subject: [PATCH] fix(server): report which S3 call failed in storage error events (#17424) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The server keeps attachments and other files in a storage provider. Hosted instances use the S3 provider. > - Sentry received `SignatureDoesNotMatch` errors from the S3 provider on two staging hosts. > - The events have only AWS SDK stack frames. They do not show which S3 call failed, the HTTP status, or the S3 request ID. > - Without these facts, we cannot find the cause or make a safe fix. > - This pull request records a small, fixed-shape failure record on the S3 error and sends it with the Sentry event. > - The benefit is that the next event shows the failed operation and the provider request ID, and no private data goes to Sentry. ## Linked Issues or Issue Description **What happened?** The S3 storage provider threw `SignatureDoesNotMatch` four times on staging. The Sentry events do not identify the S3 operation or the caller. **Expected behavior** A storage provider failure in Sentry shows which storage call failed and the provider's correlation fields. **Steps to reproduce** 1. Configure the S3 storage provider against an endpoint that rejects the request signature. 2. Read, write, check, or delete a stored file. 3. Look at the Sentry event. It has no first-party stack frame and no operation name. No public GitHub issue exists. I searched open and closed pull requests for `SignatureDoesNotMatch` and storage failure diagnostics. No pull request covers this. ## What Changed - Add `server/src/storage/failure-diagnostics.ts`. It keeps a frozen, fixed-shape record for each failed storage error in a `WeakMap`. - Add `server/src/storage/s3-failure.ts`. It wraps each S3 call and records the operation, error code, HTTP status, request ID, and SDK attempt count. It reads only data properties. A value that does not match its bounded format becomes `unknown` or `null`. - Wrap `PutObject`, `GetObject`, `HeadObject`, `DeleteObject`, `CreateMultipartUpload`, `UploadPart`, and `CompleteMultipartUpload`. - In `captureException`, add the `storage_operation` and `storage_error_code` tags and a `storage_failure` context to the event of that error only. The original error and its default grouping do not change. - Document the new context in `doc/observability.md`. ## Verification - `pnpm exec vitest run src/__tests__/storage-s3-failure-diagnostics.test.ts src/__tests__/sentry.test.ts` (in `server/`): 37 passed, 5 skipped. The skipped tests are the existing real-SDK tests, which need an environment flag. - `pnpm exec vitest run src/__tests__/storage-s3-multipart.test.ts src/__tests__/storage-local-provider.test.ts`: 8 passed. - The new tests use a local HTTP server that returns a real S3 `SignatureDoesNotMatch` XML body with a `StringToSign` field. The tests make sure that the record has the operation, code, status and request ID, and that it does not contain the private string. - I typechecked the changed source files with a scoped `tsc` project. They have no errors. I could not run the full `pnpm -r typecheck` and `pnpm build` in my sandbox: it has a 4 GiB memory limit and no `cargo` for the runner binary. CI runs the full checks. ## Risks - Low risk. The change adds a `try`/`catch` that records data and throws the same error again. Storage behavior, not-found handling, and HTTP responses do not change. - Secret-sensitive routes replace the error before reporting, so those events do not get the record. This is intended. - The record never contains the bucket, object key, endpoint, credentials, error message, or provider response body. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic. Used through Claude Code with tool use (shell, file edit) and extended reasoning. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 --- doc/observability.md | 14 +++ server/src/__tests__/sentry.test.ts | 25 ++++ .../storage-s3-failure-diagnostics.test.ts | 112 ++++++++++++++++++ server/src/sentry.ts | 10 ++ server/src/storage/failure-diagnostics.ts | 34 ++++++ server/src/storage/s3-failure.ts | 46 +++++++ server/src/storage/s3-multipart.ts | 7 +- server/src/storage/s3-provider.ts | 17 +-- 8 files changed, 254 insertions(+), 11 deletions(-) create mode 100644 server/src/__tests__/storage-s3-failure-diagnostics.test.ts create mode 100644 server/src/storage/failure-diagnostics.ts create mode 100644 server/src/storage/s3-failure.ts diff --git a/doc/observability.md b/doc/observability.md index 618f14305fe..ae4deb44c38 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -1036,6 +1036,20 @@ receives a plain error with the existing generic message and default grouping. The context does not change the HTTP response, authentication, cookies, cache, deadline, or single-fetch behavior, and does not carry into unrelated events. +A file storage provider failure has an event-local `storage_failure` context +and `storage_operation` and `storage_error_code` tags. The S3 provider records +the failed call (`put_object`, `get_object`, `head_object`, `delete_object`, +`create_multipart_upload`, `upload_part`, or `complete_multipart_upload`), +the provider's error code, the HTTP status, the provider-issued request ID, +and the SDK attempt count. Values come from the SDK error's own data +properties. A value that does not match its bounded format becomes `unknown` +or `null`. The record never includes the bucket, object key, endpoint, +credentials, error message, or provider response body (such as an S3 +`StringToSign`). Sentry receives the original error with default grouping, +so existing issues keep collecting events. The record does not change +storage behavior, not-found handling, or HTTP responses. It is attached only +to the error that the failed call threw. + An unconfirmed adapter Stop timeout has an event-local `adapter_stop` context: the run UUID, built-in adapter type, native/legacy runtime mode, configured wait duration, and whether abort was requested. Invalid identities become diff --git a/server/src/__tests__/sentry.test.ts b/server/src/__tests__/sentry.test.ts index 865ae21fc4c..8771bf694d1 100644 --- a/server/src/__tests__/sentry.test.ts +++ b/server/src/__tests__/sentry.test.ts @@ -150,6 +150,31 @@ describe("captureException", () => { expect(sdk.captureException.mock.calls[1]).toEqual([unrelated]); }); + it("adds the normalized storage failure to the original error's event only", async () => { + process.env[BACKEND_DSN_ENV] = "https://fixture@example.com/1"; + const sdk = mockSentryPackage(); + const { captureException, sentryReady } = await importFreshSentry(); + const { recordS3Failure } = await import("../storage/s3-failure.js"); + await sentryReady; + const error = Object.assign(new Error("The request signature we calculated does not match"), { + name: "SignatureDoesNotMatch", + $metadata: { httpStatusCode: 403, requestId: "req-4G-1", attempts: 1, extendedRequestId: "private host id" }, + StringToSign: "private string to sign", + }); + recordS3Failure(error, "get_object"); + captureException(error); + const unrelated = new Error("unrelated"); + captureException(unrelated); + expect(sdk.captureException.mock.calls[0]).toEqual([ + error, + { tags: { storage_operation: "get_object", storage_error_code: "SignatureDoesNotMatch" }, fingerprint: ["{{ default }}"], contexts: { + storage_failure: { provider: "s3", operation: "get_object", errorCode: "SignatureDoesNotMatch", httpStatus: 403, requestId: "req-4G-1", attempts: 1 }, + } }, + ]); + expect(JSON.stringify(sdk.captureException.mock.calls[0]![1])).not.toContain("private"); + expect(sdk.captureException.mock.calls[1]).toEqual([unrelated]); + }); + it("adds bounded Stop timeout context only to that event", async () => { process.env[BACKEND_DSN_ENV] = "https://fixture@example.com/1"; const sdk = mockSentryPackage(); diff --git a/server/src/__tests__/storage-s3-failure-diagnostics.test.ts b/server/src/__tests__/storage-s3-failure-diagnostics.test.ts new file mode 100644 index 00000000000..ef92ef8879b --- /dev/null +++ b/server/src/__tests__/storage-s3-failure-diagnostics.test.ts @@ -0,0 +1,112 @@ +import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; +import type { AddressInfo } from "node:net"; +import { Readable } from "node:stream"; +import { CreateMultipartUploadCommand, S3Client, UploadPartCommand } from "@aws-sdk/client-s3"; +import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; +import { storageFailureDiagnosticsFor } from "../storage/failure-diagnostics.js"; +import { createS3StorageProvider } from "../storage/s3-provider.js"; +import { putS3Multipart } from "../storage/s3-multipart.js"; + +const SIGNATURE_MISMATCH = ` +SignatureDoesNotMatchThe request signature we calculated does not match the signature you provided. Check your key and signing method.private string to signreq-4G-1`; + +let handler: (req: IncomingMessage, res: ServerResponse) => void = (_req, res) => res.end(); +const server = createServer((req, res) => { + req.resume(); + req.on("end", () => handler(req, res)); +}); +let endpoint = ""; +const savedEnv = { ...process.env }; + +beforeAll(async () => { + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + endpoint = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; + process.env.AWS_ACCESS_KEY_ID = "fixture-access-key"; + process.env.AWS_SECRET_ACCESS_KEY = "fixture-secret-key"; + delete process.env.AWS_SESSION_TOKEN; + delete process.env.AWS_PROFILE; +}); + +afterAll(async () => { + process.env = savedEnv; + await new Promise((resolve) => server.close(() => resolve())); +}); + +function provider() { + return createS3StorageProvider({ bucket: "bucket", region: "us-east-1", endpoint, forcePathStyle: true }); +} + +function rejectSignature(req: IncomingMessage, res: ServerResponse) { + res.writeHead(403, { "content-type": "application/xml", "x-amz-request-id": "req-4G-1" }); + res.end(req.method === "HEAD" ? undefined : SIGNATURE_MISMATCH); +} + +describe("S3 storage failure diagnostics", () => { + it("records the failed operation and provider correlation fields on the original SDK error", async () => { + handler = rejectSignature; + const error = await provider().getObject({ objectKey: "company/asset.png" }).catch((err: unknown) => err); + + expect(error).toMatchObject({ name: "SignatureDoesNotMatch" }); + expect(storageFailureDiagnosticsFor(error)).toEqual({ + provider: "s3", + operation: "get_object", + errorCode: "SignatureDoesNotMatch", + httpStatus: 403, + requestId: "req-4G-1", + attempts: 1, + }); + expect(JSON.stringify(storageFailureDiagnosticsFor(error))).not.toContain("private"); + }); + + it("names each single-request operation", async () => { + handler = rejectSignature; + const s3 = provider(); + const put = await s3.putObject({ objectKey: "k", body: Buffer.from("abc"), contentType: "text/plain", contentLength: 3 }).catch((err: unknown) => err); + const head = await s3.headObject({ objectKey: "k" }).catch((err: unknown) => err); + const remove = await s3.deleteObject({ objectKey: "k" }).catch((err: unknown) => err); + + expect(storageFailureDiagnosticsFor(put)).toMatchObject({ operation: "put_object", errorCode: "SignatureDoesNotMatch", httpStatus: 403 }); + // A HEAD response has no body, so the SDK can only report the status. + expect(storageFailureDiagnosticsFor(head)).toMatchObject({ operation: "head_object", httpStatus: 403, requestId: "req-4G-1" }); + expect(storageFailureDiagnosticsFor(remove)).toMatchObject({ operation: "delete_object", errorCode: "SignatureDoesNotMatch" }); + }); + + it("keeps not-found handling unchanged", async () => { + handler = (_req, res) => { + res.writeHead(404, { "content-type": "application/xml" }); + res.end(`NoSuchKeymissing`); + }; + const s3 = provider(); + + await expect(s3.getObject({ objectKey: "missing" })).rejects.toMatchObject({ status: 404 }); + await expect(s3.headObject({ objectKey: "missing" })).resolves.toEqual({ exists: false }); + }); + + it("names the multipart step that failed and ignores unsafe provider fields", async () => { + const client = new S3Client({ region: "us-east-1" }); + const failure = Object.assign(new Error("private provider message"), { + name: "Signature mismatch ", + $metadata: { httpStatusCode: 403, requestId: "private request id with spaces", attempts: 2 }, + }); + const send = vi.spyOn(client, "send").mockImplementation(async (command: unknown) => { + if (command instanceof CreateMultipartUploadCommand) return { UploadId: "upload" }; + if (command instanceof UploadPartCommand) throw failure; + return {}; + }); + try { + const body = Readable.from([Buffer.from("abc")]); + await expect(putS3Multipart(client, "bucket", "key", { objectKey: "key", contentType: "text/plain", contentLength: 3, body })).rejects.toBe(failure); + expect(storageFailureDiagnosticsFor(failure)).toEqual({ + provider: "s3", + operation: "upload_part", + errorCode: "unknown", + httpStatus: 403, + requestId: null, + attempts: 2, + }); + } finally { + send.mockRestore(); + client.destroy(); + } + }); +}); diff --git a/server/src/sentry.ts b/server/src/sentry.ts index 09870d9814a..1808cc9291c 100644 --- a/server/src/sentry.ts +++ b/server/src/sentry.ts @@ -58,6 +58,7 @@ import os from "node:os"; import { AdapterStopTimeoutError } from "./services/adapter-stop-timeout.js"; import { CloudPortfolioError } from "./services/cloud-portfolio-error.js"; +import { storageFailureDiagnosticsFor } from "./storage/failure-diagnostics.js"; import type { RunFailureDiagnostics } from "./services/run-failure-diagnostics.js"; import { readBuildCommit } from "./build-commit.js"; import { checkExactPeerVersions } from "./peer-version-check.js"; @@ -115,6 +116,7 @@ export function captureException(error: unknown, reference?: SentryRunReference) ...(reference.companyId ? { company_id: reference.companyId } : {}), ...(reference.agentId ? { agent_id: reference.agentId } : {}), } : {}; + const storageFailure = storageFailureDiagnosticsFor(error); if (error instanceof AdapterStopTimeoutError) { // Event-local, fixed-shape context: no ambient scope or raw error fields. const exception = new Error(error.message); @@ -132,6 +134,14 @@ export function captureException(error: unknown, reference?: SentryRunReference) contexts: { cloud_portfolio: { ...CloudPortfolioError.diagnosticsFor(error) } }, fingerprint: ["{{ default }}"], }); + } else if (storageFailure) { + // The original error keeps its default grouping; only the normalized + // record is added, never the provider's request or response. + sentryHandle.captureException(error, { + tags: { ...tags, storage_operation: storageFailure.operation, storage_error_code: storageFailure.errorCode }, + contexts: { storage_failure: { ...storageFailure } }, + fingerprint: ["{{ default }}"], + }); } else if (reference) { sentryHandle.captureException(error, { tags, contexts: {}, fingerprint: ["{{ default }}"] }); } else { diff --git a/server/src/storage/failure-diagnostics.ts b/server/src/storage/failure-diagnostics.ts new file mode 100644 index 00000000000..5e150cbe5dc --- /dev/null +++ b/server/src/storage/failure-diagnostics.ts @@ -0,0 +1,34 @@ +// Server-only, fixed-shape storage failure details for error reporting. The +// provider that owns the protocol normalizes them; reporting reads only this +// record and never the provider's error payload. + +export type StorageFailureOperation = + | "put_object" + | "get_object" + | "head_object" + | "delete_object" + | "create_multipart_upload" + | "upload_part" + | "complete_multipart_upload"; + +export interface StorageFailureDiagnostics { + provider: "s3"; + operation: StorageFailureOperation; + errorCode: string; + httpStatus: number | null; + requestId: string | null; + attempts: number | null; +} + +const recorded = new WeakMap>(); + +/** Keep the first record: it names the call that actually failed. */ +export function recordStorageFailure(error: unknown, diagnostics: StorageFailureDiagnostics): void { + if (!error || typeof error !== "object" || recorded.has(error)) return; + recorded.set(error, Object.freeze({ ...diagnostics })); +} + +export function storageFailureDiagnosticsFor(error: unknown): Readonly | null { + if (!error || typeof error !== "object") return null; + return recorded.get(error) ?? null; +} diff --git a/server/src/storage/s3-failure.ts b/server/src/storage/s3-failure.ts new file mode 100644 index 00000000000..3160b8fd3ec --- /dev/null +++ b/server/src/storage/s3-failure.ts @@ -0,0 +1,46 @@ +import { recordStorageFailure, type StorageFailureOperation } from "./failure-diagnostics.js"; + +const ERROR_CODE = /^[A-Za-z][A-Za-z0-9._-]{0,63}$/; +const REQUEST_ID = /^[A-Za-z0-9+/=._-]{1,128}$/; + +// Inspect data properties only. An SDK error can carry getters, request +// objects or provider XML; none of those are diagnostic payloads. +function ownValue(value: unknown, key: string): unknown { + if (!value || typeof value !== "object") return undefined; + try { + const descriptor = Object.getOwnPropertyDescriptor(value, key); + return descriptor && "value" in descriptor ? descriptor.value : undefined; + } catch { + return undefined; + } +} + +function matching(value: unknown, pattern: RegExp): string | null { + return typeof value === "string" && pattern.test(value) ? value : null; +} + +function integerIn(value: unknown, min: number, max: number): number | null { + return Number.isInteger(value) && (value as number) >= min && (value as number) <= max ? value as number : null; +} + +/** Record which S3 call failed and the provider's own correlation fields. */ +export function recordS3Failure(error: unknown, operation: StorageFailureOperation): void { + const metadata = ownValue(error, "$metadata"); + recordStorageFailure(error, { + provider: "s3", + operation, + errorCode: matching(ownValue(error, "name"), ERROR_CODE) ?? matching(ownValue(error, "code"), ERROR_CODE) ?? "unknown", + httpStatus: integerIn(ownValue(metadata, "httpStatusCode"), 100, 599), + requestId: matching(ownValue(metadata, "requestId"), REQUEST_ID), + attempts: integerIn(ownValue(metadata, "attempts"), 1, 100), + }); +} + +export async function withS3FailureDiagnostics(operation: StorageFailureOperation, send: () => Promise): Promise { + try { + return await send(); + } catch (error) { + recordS3Failure(error, operation); + throw error; + } +} diff --git a/server/src/storage/s3-multipart.ts b/server/src/storage/s3-multipart.ts index 123852c8a09..2b8e0d951d4 100644 --- a/server/src/storage/s3-multipart.ts +++ b/server/src/storage/s3-multipart.ts @@ -1,6 +1,7 @@ import { AbortMultipartUploadCommand, CompleteMultipartUploadCommand, CreateMultipartUploadCommand, UploadPartCommand, type S3Client } from "@aws-sdk/client-s3"; import { Readable } from "node:stream"; import type { PutObjectInput } from "./types.js"; +import { withS3FailureDiagnostics } from "./s3-failure.js"; /** Each part is streamed too: multipart must not become another whole-file * buffer or a fixed part-count ceiling below the storage provider's limits. */ @@ -12,7 +13,7 @@ export async function putS3Multipart(client: S3Client, bucket: string, key: stri // Scale part sizes to the S3 part-count limit while retaining stream backpressure. const partSize = Math.max(8 * 1024 * 1024, Math.ceil(input.contentLength / 10_000)); try { - uploadId = (await client.send(new CreateMultipartUploadCommand({ Bucket: bucket, Key: key, ContentType: input.contentType }))).UploadId; + uploadId = (await withS3FailureDiagnostics("create_multipart_upload", () => client.send(new CreateMultipartUploadCommand({ Bucket: bucket, Key: key, ContentType: input.contentType })))).UploadId; if (!uploadId) throw new Error("S3 did not return a multipart upload id"); const parts: { ETag: string; PartNumber: number }[] = []; for (let offset = 0; offset < input.contentLength;) { @@ -34,14 +35,14 @@ export async function putS3Multipart(client: S3Client, bucket: string, key: stri } })()); try { - const result = await client.send(new UploadPartCommand({ Bucket: bucket, Key: key, UploadId: uploadId, PartNumber: parts.length + 1, Body: body, ContentLength: length })); + const result = await withS3FailureDiagnostics("upload_part", () => client.send(new UploadPartCommand({ Bucket: bucket, Key: key, UploadId: uploadId, PartNumber: parts.length + 1, Body: body, ContentLength: length }))); if (remaining || !result.ETag) throw new Error("S3 upload part is incomplete"); parts.push({ ETag: result.ETag, PartNumber: parts.length + 1 }); } finally { body.destroy(); } offset += length; } if (pending.length || !(await iterator.next()).done) throw new Error("S3 upload source exceeds its declared size"); - await client.send(new CompleteMultipartUploadCommand({ Bucket: bucket, Key: key, UploadId: uploadId, MultipartUpload: { Parts: parts } })); + await withS3FailureDiagnostics("complete_multipart_upload", () => client.send(new CompleteMultipartUploadCommand({ Bucket: bucket, Key: key, UploadId: uploadId, MultipartUpload: { Parts: parts } }))); } catch (error) { if (uploadId) await client.send(new AbortMultipartUploadCommand({ Bucket: bucket, Key: key, UploadId: uploadId })).catch(() => {}); throw error; diff --git a/server/src/storage/s3-provider.ts b/server/src/storage/s3-provider.ts index 4939bb9b25f..b96ff003be8 100644 --- a/server/src/storage/s3-provider.ts +++ b/server/src/storage/s3-provider.ts @@ -6,6 +6,7 @@ import { PutObjectCommand, } from "@aws-sdk/client-s3"; import { putS3Multipart } from "./s3-multipart.js"; +import { withS3FailureDiagnostics } from "./s3-failure.js"; import { addAbortSignal, Readable } from "node:stream"; import type { StorageProvider, GetObjectResult, HeadObjectResult } from "./types.js"; import { notFound, unprocessable } from "../errors.js"; @@ -86,7 +87,7 @@ export function createS3StorageProvider(config: S3ProviderConfig): StorageProvid await putS3Multipart(client, bucket, key, input); return; } - await client.send( + await withS3FailureDiagnostics("put_object", () => client.send( new PutObjectCommand({ Bucket: bucket, Key: key, @@ -94,20 +95,20 @@ export function createS3StorageProvider(config: S3ProviderConfig): StorageProvid ContentType: input.contentType, ContentLength: input.contentLength, }), - ); + )); }, async getObject(input): Promise { const key = buildKey(prefix, input.objectKey); try { - const output = await client.send( + const output = await withS3FailureDiagnostics("get_object", () => client.send( new GetObjectCommand({ Bucket: bucket, Key: key, Range: input.range ? `bytes=${input.range.start}-${input.range.end}` : undefined, }), { abortSignal: input.signal }, - ); + )); const stream = await toReadableStream(output.Body); if (input.signal) addAbortSignal(input.signal, stream); @@ -128,13 +129,13 @@ export function createS3StorageProvider(config: S3ProviderConfig): StorageProvid async headObject(input): Promise { const key = buildKey(prefix, input.objectKey); try { - const output = await client.send( + const output = await withS3FailureDiagnostics("head_object", () => client.send( new HeadObjectCommand({ Bucket: bucket, Key: key, }), { abortSignal: input.signal }, - ); + )); return { exists: true, @@ -152,12 +153,12 @@ export function createS3StorageProvider(config: S3ProviderConfig): StorageProvid async deleteObject(input): Promise { const key = buildKey(prefix, input.objectKey); - await client.send( + await withS3FailureDiagnostics("delete_object", () => client.send( new DeleteObjectCommand({ Bucket: bucket, Key: key, }), - ); + )); }, }; }