Skip to content

Add RERUM fetch wrapper with timeout#41

Open
cubap wants to merge 1 commit intomainfrom
31-no-timeout-on-outgoing-fetch-to-rerum
Open

Add RERUM fetch wrapper with timeout#41
cubap wants to merge 1 commit intomainfrom
31-no-timeout-on-outgoing-fetch-to-rerum

Conversation

@cubap
Copy link
Copy Markdown
Member

@cubap cubap commented Apr 1, 2026

fixes #31
new file rerum.js which provides fetchRerum (bounded fetch using RERUM_FETCH_TIMEOUT_MS with AbortSignal.any and a 30s default) and error helpers (createRerumNetworkError / createRerumTimeoutError). Replace duplicated fetch/error-handling in routes (create, delete, overwrite, query, update) to use fetchRerum and the network-error helper for consistent 502/504 behavior and invalid-response checks. Add tests/rerum.testcases.md documenting timeout and upstream-failure scenarios. Update sample.env to note the default RERUM timeout configuration.

fixes #31
new file rerum.js which provides fetchRerum (bounded fetch using RERUM_FETCH_TIMEOUT_MS with AbortSignal.any and a 30s default) and error helpers (createRerumNetworkError / createRerumTimeoutError). Replace duplicated fetch/error-handling in routes (create, delete, overwrite, query, update) to use fetchRerum and the network-error helper for consistent 502/504 behavior and invalid-response checks. Add __tests__/rerum.testcases.md documenting timeout and upstream-failure scenarios. Update sample.env to note the default RERUM timeout configuration.
@cubap cubap linked an issue Apr 1, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Member Author

@cubap cubap left a comment

Choose a reason for hiding this comment

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

To note: These changes suggest creating tests that timeout, which is slow. There are no new tests built, but described instead. All errors should be thrown the same way. It is a separate file so as we implement it in new routes, we can cover it easily. Look at create.js for an example that shows special error handling, as most routes can just send it without catching.

@cubap cubap mentioned this pull request Apr 2, 2026
Copy link
Copy Markdown
Member

@thehabes thehabes left a comment

Choose a reason for hiding this comment

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

This looks good and function as expected. A couple of minor findings during static review that do not block this PR.

Static Review Comments

Branch: 31-no-timeout-on-outgoing-fetch-to-rerum
Review Date: 2026-04-06
Reviewer: Pair Static Review - Claude & @thehabes

Claude and Bryan make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.

Category Issues Found
🔴 Critical 0
🟠 Major 1
🟡 Minor 1
🔵 Suggestions 0

Critical issues 🔴

None found.


Major issues 🟠

Issue 1: sample.env uses wrong environment variable name

File: sample.env:19
Category: Configuration Bug (Introduced by PR)

Problem:
The sample.env entry uses the name DEFAULT_RERUM_TIMEOUT_MS, which is the internal JavaScript constant name in rerum.js. The code actually reads process.env.RERUM_FETCH_TIMEOUT_MS. If a developer copies sample.env and uncomments this line, the timeout configuration will be silently ignored and the default (30000ms) will always be used.

Current Code:

#DEFAULT_RERUM_TIMEOUT_MS = 30000

Suggested Fix:

#RERUM_FETCH_TIMEOUT_MS = 30000

How to Verify:

  1. Set RERUM_FETCH_TIMEOUT_MS=3000 in .env
  2. Start TinyPEN
  3. Simulate a slow RERUM response (> 3s)
  4. Confirm HTTP 504 arrives after ~3s

Minor issues 🟡

Issue 1: Missing JSDoc on getRerumTimeoutMs

File: rerum.js:3
Category: Code Hygiene (Introduced by PR)

Problem:
The project convention requires JSDoc style documentation (per CLAUDE.md). The other two helper functions (createRerumNetworkError, createRerumTimeoutError) have proper JSDoc, but getRerumTimeoutMs does not.

Current Code:

const getRerumTimeoutMs = () => {
  const timeoutMs = Number.parseInt(process.env.RERUM_FETCH_TIMEOUT_MS ?? `${DEFAULT_RERUM_TIMEOUT_MS}`, 10)
  return Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : DEFAULT_RERUM_TIMEOUT_MS
}

Suggested Fix:

/**
 * Read the RERUM fetch timeout from the environment, falling back to the default.
 *
 * @returns {number} Timeout in milliseconds (always a positive finite integer)
 */
const getRerumTimeoutMs = () => {
  const timeoutMs = Number.parseInt(process.env.RERUM_FETCH_TIMEOUT_MS ?? `${DEFAULT_RERUM_TIMEOUT_MS}`, 10)
  return Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : DEFAULT_RERUM_TIMEOUT_MS
}

Suggestions 🔵

None.


If there are significant code changes in response to this review please test those changes. Run the application manually and test or perform internal application tests when applicable.

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.

No timeout on outgoing fetch() to RERUM

2 participants