fix: release upstream response body before sending error on status/header failure by DominicBM · Pull Request #27 · dpla/thumbnail-api

DominicBM · 2026-04-15T14:36:13Z

Summary

Cancel the upstream response body (fire-and-forget) when okStatus or okHeaders checks fail, so the connection is released back to the pool even on error paths
The cancel is intentionally not awaited — there's no need to block sendError() on it, and this avoids adding latency to the 404 error response

Test plan

Request an image whose upstream returns a non-200 ok response (e.g. 201) — verify the 404 is returned promptly
Request an image whose upstream returns a bad Content-Type — verify the 404 is returned promptly
Verify no connection pool exhaustion under concurrent load (connections returned after error paths)

🤖 Generated with Claude Code

Summary

This PR ensures upstream response bodies are explicitly cancelled when status or header validation fail in the image proxy flow so the underlying connection is released back to the pool. A new private helper releaseUpstreamBody(response: Response) is added; it calls response.body?.cancel?.()?.catch(() => {}) and is invoked (fire-and-forget, intentionally not awaited) on failure paths where translated upstream status is not okStatus and where okHeaders validation fails. The S3-serving path's translated-status failure branch was also updated to cancel the upstream response body.

The cancellation is deliberately not awaited to avoid adding latency to error responses (e.g., prompt 404s).

Changes

Modified file: src/ThumbnailApi.ts — added private helper releaseUpstreamBody and calls on status/header failure paths. (+7/-0)
Nature of change: Bug fix addressing connection pool resource management on error paths

Impact Assessment

No manual pipeline trigger or ECS redeployment required beyond a normal deployment of the updated service image.
No environment variables or AWS Secrets Manager keys added, removed, or renamed.
No database migrations.
No changes to shared infrastructure (CodePipeline, CodeBuild, ECS task definitions, IAM policies).
No modifications to public-facing API response shapes or endpoints.
No security implications introduced by this change.

Test Plan

Request an image whose upstream returns a non-200 translated status (e.g., 201) — verify a prompt 404 is returned.
Request an image whose upstream returns a bad Content-Type — verify a prompt 404 is returned.
Verify no connection pool exhaustion under concurrent load (connections returned after error paths).

Notes:

PR description indicates it was generated with Claude Code.

coderabbitai · 2026-04-15T14:36:23Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 27fe7c6c-52b7-42d4-b6a1-09e843f05d18

📥 Commits

Reviewing files that changed from the base of the PR and between 908feb9 and f31d6e9.

📒 Files selected for processing (1)

src/ThumbnailApi.ts

Walkthrough

Cancel the upstream response body in proxyItemFromContributor for three additional error paths (translated upstream status not OK in proxying and S3-serving paths, and invalid upstream headers) by calling a helper that suppresses cancellation errors, then return 404 responses.

Changes

Cohort / File(s)	Summary
Response body cleanup `src/ThumbnailApi.ts`	Added a private helper `releaseUpstreamBody(response: Response)` that calls `response.body?.cancel?.()` while suppressing errors. This helper is invoked in additional early-return error branches: when translated upstream status is not `okStatus` (both proxying and S3-serving paths) and when upstream response headers fail validation, before returning 404 responses.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: releasing upstream response body before sending errors on status/header failures, which matches the changeset's primary objective.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch lessons-audit-fixes

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

…ader failure Body cancellation on okStatus and okHeaders failures is fire-and-forget — no need to await before calling sendError, which avoids adding latency to the error response path while still releasing the connection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

DominicBM · 2026-04-15T22:54:06Z

@coderabbitai full review

coderabbitai · 2026-04-15T22:54:13Z

✅ Actions performed

Full review triggered.

coderabbitai

🧹 Nitpick comments (1)

src/ThumbnailApi.ts (1)

99-113: Consider deduplicating body-cancel logic into a small helper.

Same cleanup snippet appears in two branches; extracting it reduces drift risk if this behavior changes later.

♻️ Optional refactor

+    const releaseUpstreamBody = (response: Response): void => {
+      void response.body?.cancel?.().catch(() => {});
+    };
+
     if (!this.responseHelper.okStatus(status)) {
       const error = new Error(`Status ${String(status)} from upstream.`);
-      remoteImageResponse.body?.cancel?.().catch(() => {});
+      releaseUpstreamBody(remoteImageResponse);
       this.sendError(expressResponse, itemId, 404, error);
       return;
     }
@@
     if (!this.responseHelper.okHeaders(remoteImageResponse.headers)) {
       const error = new Error(`Got bad headers from upstream.`);
-      remoteImageResponse.body?.cancel?.().catch(() => {});
+      releaseUpstreamBody(remoteImageResponse);
       this.sendError(expressResponse, itemId, 404, error);
       return;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ThumbnailApi.ts` around lines 99 - 113, Extract the repeated body
cancel-and-ignore-errors snippet into a small helper on ThumbnailApi (e.g., a
private method like cancelRemoteBody(remoteImageResponse) or a utility function)
and call it from both branches instead of repeating
remoteImageResponse.body?.cancel?.().catch(() => {}); update the two places that
currently run on failed okStatus and failed okHeaders to invoke this helper
before calling this.sendError(expressResponse, itemId, 404, error); keep use of
optional chaining and swallow errors in the helper to preserve existing
behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/ThumbnailApi.ts`:
- Around line 99-113: Extract the repeated body cancel-and-ignore-errors snippet
into a small helper on ThumbnailApi (e.g., a private method like
cancelRemoteBody(remoteImageResponse) or a utility function) and call it from
both branches instead of repeating remoteImageResponse.body?.cancel?.().catch(()
=> {}); update the two places that currently run on failed okStatus and failed
okHeaders to invoke this helper before calling this.sendError(expressResponse,
itemId, 404, error); keep use of optional chaining and swallow errors in the
helper to preserve existing behavior.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 92eebe58-652f-43c7-b225-812db75d376a

📥 Commits

Reviewing files that changed from the base of the PR and between 908feb9 and 37a2b75.

📒 Files selected for processing (1)

src/ThumbnailApi.ts

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

DominicBM · 2026-04-16T00:23:14Z

@coderabbitai full review

coderabbitai · 2026-04-16T00:23:21Z

✅ Actions performed

Full review triggered.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ThumbnailApi.ts`:
- Around line 174-176: The non-OK S3 response path in serveItemFromS3 currently
throws/returns without releasing the upstream response body; update
serveItemFromS3 to call the centralized releaseUpstreamBody(response) before
exiting on a non-OK status so the response.body is cancelled and pooled
connections are freed; locate the status check in serveItemFromS3 and insert a
call to releaseUpstreamBody(response) (using the existing private
releaseUpstreamBody method) immediately prior to any throw/return on error.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 532bcdfc-d75f-4a70-a06e-37de8b61fd01

📥 Commits

Reviewing files that changed from the base of the PR and between 37a2b75 and 535ebc8.

📒 Files selected for processing (1)

src/ThumbnailApi.ts

DominicBM · 2026-04-16T00:31:13Z

@coderabbitai full review

coderabbitai · 2026-04-16T00:31:20Z

✅ Actions performed

Full review triggered.

…ypeError body?.cancel?.() returns undefined when body is null (e.g. a no-body upstream response), and calling .catch() on undefined throws TypeError. Using ?.catch() safely short-circuits to undefined in that case while still suppressing Promise rejections when cancel returns a Promise. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

DominicBM · 2026-04-16T01:44:36Z

@coderabbitai full review

coderabbitai · 2026-04-16T01:44:43Z

✅ Actions performed

Full review triggered.

coderabbitai

♻️ Duplicate comments (1)

src/ThumbnailApi.ts (1)

174-176: ⚠️ Potential issue | 🟠 Major

Helper is centralized, but one upstream error path still skips it (S3 status failure).

serveItemFromS3 still returns on non-OK status without releasing the upstream body (Line 142–145), so pooled connections can remain occupied under load.

Suggested fix

     if (!this.responseHelper.okStatus(status)) {
       const error = new Error(`Status ${String(status)} from upstream.`);
+      this.releaseUpstreamBody(response);
       this.sendError(expressResponse, itemId, 404, error);
       return;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ThumbnailApi.ts` around lines 174 - 176, serveItemFromS3 currently
returns early when the S3 response status is not OK without calling the
centralized releaseUpstreamBody helper, which can leave pooled connections
occupied; update serveItemFromS3 to call releaseUpstreamBody(response)
immediately before any early return on non-OK Response (and in any S3
error/early-exit paths) so response.body is properly cancelled; reference the
serveItemFromS3 function and the releaseUpstreamBody(Response) helper when
making the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/ThumbnailApi.ts`:
- Around line 174-176: serveItemFromS3 currently returns early when the S3
response status is not OK without calling the centralized releaseUpstreamBody
helper, which can leave pooled connections occupied; update serveItemFromS3 to
call releaseUpstreamBody(response) immediately before any early return on non-OK
Response (and in any S3 error/early-exit paths) so response.body is properly
cancelled; reference the serveItemFromS3 function and the
releaseUpstreamBody(Response) helper when making the change.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 90643b4a-79a2-47f9-92d1-1417f56731d1

📥 Commits

Reviewing files that changed from the base of the PR and between 908feb9 and 8db4551.

📒 Files selected for processing (1)

src/ThumbnailApi.ts

Apply the same releaseUpstreamBody call to the serveItemFromS3 error path that was already added to proxyItemFromContributor, so connections are returned to the pool on S3 upstream failures too. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

DominicBM · 2026-04-16T03:32:03Z

@coderabbitai full review

coderabbitai · 2026-04-16T03:32:09Z

✅ Actions performed

Full review triggered.

DominicBM force-pushed the lessons-audit-fixes branch from 5f23f82 to 37a2b75 Compare April 15, 2026 22:05

coderabbitai Bot reviewed Apr 15, 2026

View reviewed changes

refactor: extract releaseUpstreamBody helper to remove duplication

535ebc8

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

coderabbitai Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread src/ThumbnailApi.ts

coderabbitai Bot reviewed Apr 16, 2026

View reviewed changes

DominicBM merged commit 289e872 into main Apr 16, 2026
6 checks passed

DominicBM deleted the lessons-audit-fixes branch April 16, 2026 03:45

coderabbitai Bot mentioned this pull request Apr 18, 2026

Potential fixes for 5 code quality findings #28

Merged

This was referenced May 8, 2026

Fix minor code quality issues (AI findings) #30

Merged

Fix: Headers not forwarded to clients, incorrect Content-Type #31

Open

Conversation

DominicBM commented Apr 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary

Changes

Impact Assessment

Test Plan

Uh oh!

coderabbitai Bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

DominicBM commented Apr 15, 2026

Uh oh!

coderabbitai Bot commented Apr 15, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

DominicBM commented Apr 16, 2026

Uh oh!

coderabbitai Bot commented Apr 16, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DominicBM commented Apr 16, 2026

Uh oh!

coderabbitai Bot commented Apr 16, 2026

Uh oh!

DominicBM commented Apr 16, 2026

Uh oh!

coderabbitai Bot commented Apr 16, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

DominicBM commented Apr 16, 2026

Uh oh!

coderabbitai Bot commented Apr 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

DominicBM commented Apr 15, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 15, 2026 •

edited

Loading