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/transcript-download-empty-reason-404.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
area: webapp
type: fix
---

Return a not-found response when a missing transcript receives an object-store 404 without a reason phrase

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.

🔍 Release note describes infrastructure, not user behavior

The changelog note names the object store and HTTP reason phrase. The release-note guidance calls for describing the visible change without internal infrastructure.

Devin Review


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

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { createServer, type Server } from "node:http";
import { once } from "node:events";
import { createServer as createTcpServer } from "node:net";
import express from "express";
import { sendRemixResponse } from "@remix-run/express/dist/server";
import { expect, test } from "vitest";
Expand Down Expand Up @@ -116,6 +117,34 @@ test("does not treat permission or service failures as an absent transcript", ()
expect(isTranscriptNotFound({ name: "NoSuchKey" })).toBe(true);
});

test("recognizes a missing transcript when the object store omits the reason phrase", async () => {
const server = 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");

try {
const client = ObjectStoreClient.create({
baseUrl: `http://127.0.0.1:${address.port}`,
accessKeyId: "test-access-key",
secretAccessKey: "test-secret-key",
service: "s3",
});

// @crumbs Exercise the adapter with a valid 404 response whose statusText is empty.
await expect(client.getObjectResponse(key)).rejects.toSatisfy(isTranscriptNotFound);
} finally {
await new Promise<void>((resolve, reject) =>
server.close((error) => (error ? reject(error) : resolve()))
);
}
});

minioTest(
"HEAD distinguishes permission failures from missing objects",
async ({ minioConfig }) => {
Expand Down
16 changes: 14 additions & 2 deletions apps/webapp/app/services/realtime/transcriptDownload.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,20 @@ 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 } };
if (name === "NoSuchKey" || name === "NotFound" || $metadata?.httpStatusCode === 404) return true;
const { name, status, $metadata } = error as {
name?: string;
status?: number;
$metadata?: { httpStatusCode?: number };
};
// @crumbs Prefer status codes because HTTP reason phrases are optional.
if (
name === "NoSuchKey" ||
name === "NotFound" ||
status === 404 ||
$metadata?.httpStatusCode === 404
) {
return true;
}
// The aws4fetch adapter currently reports the HTTP status text in its error.
return (
error instanceof Error &&
Expand Down
6 changes: 5 additions & 1 deletion apps/webapp/app/v3/objectStoreClient.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,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}`);
// @crumbs Preserve the numeric status because statusText may be empty for valid responses.
throw Object.assign(
new Error(`Failed to download from object store: ${response.statusText}`),
{ status: response.status }
);
}
return response;
}
Expand Down