Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .server-changes/fix-transcript-404-empty-reason-phrase.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
area: webapp
type: fix
---

Fix missing session transcripts returning a download error instead of a not-found response
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createServer, type Server } from "node:http";
import { createServer as createTcpServer, type Server as TcpServer } from "node:net";
import { once } from "node:events";
import express from "express";
import { sendRemixResponse } from "@remix-run/express/dist/server";
Expand Down Expand Up @@ -114,6 +115,34 @@ test("does not treat permission or service failures as an absent transcript", ()
).toBe(false);
expect(isTranscriptNotFound({ $metadata: { httpStatusCode: 503 } })).toBe(false);
expect(isTranscriptNotFound({ name: "NoSuchKey" })).toBe(true);
expect(isTranscriptNotFound({ status: 404, message: "Failed to download from object store: " })).toBe(
true
);
});

test("recognizes a 404 with an empty reason phrase as a missing transcript", async () => {
const server: TcpServer = createTcpServer((socket) => {
socket.once("data", () => {
socket.end("HTTP/1.1 404 \r\nContent-Length: 0\r\nConnection: close\r\n\r\n");
});
});
server.listen(0, "127.0.0.1");
await once(server, "listening");
const address = server.address();
if (!address || typeof address === "string") throw new Error("Expected TCP listener");
const baseUrl = `http://127.0.0.1:${address.port}`;
try {
const client = ObjectStoreClient.create({
baseUrl,
accessKeyId: "test",
secretAccessKey: "test",
service: "s3",
});
await expect(client.getObjectResponse(key)).rejects.toSatisfy(isTranscriptNotFound);
} finally {
server.close();
await once(server, "close");
}
});

minioTest(
Expand Down
10 changes: 9 additions & 1 deletion apps/webapp/app/services/realtime/transcriptDownload.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,16 @@ export function downloadTranscript(

export function isTranscriptNotFound(error: unknown): boolean {
if (!error || typeof error !== "object") return false;
const { name, $metadata } = error as { name?: string; $metadata?: { httpStatusCode?: number } };
const { name, $metadata, status } = error as {
name?: string;
$metadata?: { httpStatusCode?: number };
status?: unknown;
};
if (name === "NoSuchKey" || name === "NotFound" || $metadata?.httpStatusCode === 404) return true;
// The aws4fetch adapter reports the HTTP status on the error. Check it first:
// S3-compatible stores may return an empty reason phrase, so the message alone
// cannot distinguish a 404 from other failures.
if (status === 404) return true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔍 Message fallback can override a known status

When a download error has a non-404 status and a Not Found reason phrase, isTranscriptNotFound still returns true. The message fallback runs even when ObjectStoreDownloadError carries a definitive status.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

// The aws4fetch adapter currently reports the HTTP status text in its error.
return (
error instanceof Error &&
Expand Down
36 changes: 33 additions & 3 deletions apps/webapp/app/v3/objectStoreClient.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,24 @@ export class ObjectVersionChangedError extends Error {
}
}

/**
* A failed object-store download that preserves the HTTP status code.
* Callers must not rely on `statusText`: S3-compatible stores may return an
* empty reason phrase (e.g. `HTTP/1.1 404 `), which yields an empty message
* suffix and breaks message-based 404 detection.
*/
export class ObjectStoreDownloadError extends Error {
readonly status: number;
readonly statusText: string;

constructor(message: string, status: number, statusText: string) {
super(message);
this.name = "ObjectStoreDownloadError";
this.status = status;
this.statusText = statusText;
}
}

/** `Range` header value for a byte range or a suffix. */
function rangeHeader(range: { suffixLength: number } | { start: number; end: number }): string {
return "suffixLength" in range
Expand Down Expand Up @@ -120,7 +138,11 @@ class Aws4FetchClient implements IObjectStoreClient {
async getObject(key: string): Promise<string> {
const response = await this.awsClient.fetch(this.buildUrl(key));
if (!response.ok) {
throw new Error(`Failed to download from object store: ${response.statusText}`);
throw new ObjectStoreDownloadError(
`Failed to download from object store: ${response.statusText}`,
response.status,
response.statusText
);
}
return response.text();
}
Expand All @@ -135,7 +157,11 @@ class Aws4FetchClient implements IObjectStoreClient {
async getObjectResponse(key: string): Promise<Response> {
const response = await this.awsClient.fetch(this.buildUrl(key));
if (!response.ok) {
throw new Error(`Failed to download from object store: ${response.statusText}`);
throw new ObjectStoreDownloadError(
`Failed to download from object store: ${response.statusText}`,
response.status,
response.statusText
);
}
return response;
}
Expand All @@ -155,7 +181,11 @@ class Aws4FetchClient implements IObjectStoreClient {
throw new ObjectVersionChangedError(key);
}
if (!response.ok) {
throw new Error(`Failed to download range from object store: ${response.statusText}`);
throw new ObjectStoreDownloadError(
`Failed to download range from object store: ${response.statusText}`,
response.status,
response.statusText
);
}
const bytes = new Uint8Array(await response.arrayBuffer());
return {
Expand Down