Fix: 🐛 Zenodo release timeout issue#188
Conversation
|
Thank you for submitting this pull request! We appreciate your contribution to the project. Before we can merge it, we need to review the changes you've made to ensure they align with our code standards and meet the requirements of the project. We'll get back to you as soon as we can with feedback. Thanks again! |
Reviewer's GuideIntroduce network timeouts, heartbeat-based SSE connection keep-alive, and client-side reconciliation/pending UI to prevent Zenodo releases from appearing failed when long-running uploads or proxy drops occur; also refactor server-side publication error handling into a shared helper and tweak deployment proxy config to support long-lived streams. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for making updates to your pull request. Our team will take a look and provide feedback as soon as possible. Please wait for any GitHub Actions to complete before editing your pull request. If you have any additional questions or concerns, feel free to let us know. Thank you for your contributions! |
PR Summary
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
reconcilePublishStatus, repeated failures to fetch/release/zenodo/statusare silently ignored; consider logging or surfacing a more explicit error state so unexpected backend issues don’t leave the UI stuck in the genericpendingstate. - The fixed timeout values
ZENODO_API_TIMEOUT_MSandZENODO_UPLOAD_TIMEOUT_MSare hard-coded; consider making them configurable via environment variables so they can be tuned for different deployment environments or unusually large releases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `reconcilePublishStatus`, repeated failures to fetch `/release/zenodo/status` are silently ignored; consider logging or surfacing a more explicit error state so unexpected backend issues don’t leave the UI stuck in the generic `pending` state.
- The fixed timeout values `ZENODO_API_TIMEOUT_MS` and `ZENODO_UPLOAD_TIMEOUT_MS` are hard-coded; consider making them configurable via environment variables so they can be tuned for different deployment environments or unusually large releases.
## Individual Comments
### Comment 1
<location path="ui/server/services/archival/zenodo.ts" line_range="579" />
<code_context>
"Content-Type": "application/octet-stream",
},
method: "PUT",
+ signal: AbortSignal.timeout(ZENODO_UPLOAD_TIMEOUT_MS),
});
</code_context>
<issue_to_address>
**suggestion:** Consider handling fetch timeouts explicitly so users see a clearer error than a generic AbortError message.
`AbortSignal.timeout` prevents hanging, but when it fires `fetch` rejects with an `AbortError` whose message (e.g. "The operation was aborted") is not very helpful. Since `failPublication` displays `err.message`, users will see a vague error. Consider catching `AbortError` around these long-running requests (or in a shared wrapper) and rethrowing a clearer message like "Zenodo upload timed out after 5 minutes; please retry" so timeouts are distinguishable from other failures.
Suggested implementation:
```typescript
/**
body: body.toString(),
=======
// Request timeouts so a stalled network call can never hang the publication
// workflow indefinitely. Metadata/API calls are quick; file up/downloads and
// the repo zipball can be large, so they get a much longer budget.
const ZENODO_API_TIMEOUT_MS = 60_000; // metadata / publish / deposition / API calls
const ZENODO_UPLOAD_TIMEOUT_MS = 300_000; // file up/downloads & repo zipball
// ===== Error helper ==================================================
/**
body: body.toString(),
+
+/**
+ * Normalize errors from Zenodo calls so we can surface clearer messages to the user.
+ *
+ * In particular, when AbortSignal-based timeouts fire, fetch rejects with an AbortError
+ * whose default message ("The operation was aborted") is not helpful. Callers should
+ * pass the timeout and a short context string (e.g. "file upload") so we can generate
+ * a clearer message.
+ */
+export function normalizeZenodoError(
+ err: unknown,
+ timeoutMs?: number,
+ context?: string,
+): Error {
+ if (err instanceof Error && err.name === "AbortError" && timeoutMs && context) {
+ const minutes = Math.round(timeoutMs / 60_000);
+ return new Error(
+ `Zenodo ${context} timed out after ${minutes} minute${minutes === 1 ? "" : "s"}; please retry.`,
+ );
+ }
+
+ return err instanceof Error ? err : new Error("Zenodo request failed");
+}
```
To fully implement the suggestion, you should also:
1. **Wrap long-running fetches in try/catch and use `normalizeZenodoError`**. For example, where you perform metadata/API calls:
- Locate the `fetch` that uses `signal: AbortSignal.timeout(ZENODO_API_TIMEOUT_MS)` (the one that produces `res` just above the `if (!res.ok)` block).
- Wrap it as:
```ts
let res: Response;
try {
res = await fetch(url, {
// ...existing options...
signal: AbortSignal.timeout(ZENODO_API_TIMEOUT_MS),
});
} catch (err) {
throw normalizeZenodoError(err, ZENODO_API_TIMEOUT_MS, "API request");
}
```
2. **Do the same for file uploads/downloads** using `ZENODO_UPLOAD_TIMEOUT_MS`:
```ts
let res: Response;
try {
res = await fetch(uploadUrl, {
// ...existing options...
signal: AbortSignal.timeout(ZENODO_UPLOAD_TIMEOUT_MS),
});
} catch (err) {
throw normalizeZenodoError(err, ZENODO_UPLOAD_TIMEOUT_MS, "file upload");
}
```
3. **Ensure `failPublication` (or any shared error handler) still displays `err.message`**, but now the error has already been normalized by `normalizeZenodoError`, so users will see the clearer timeout-specific message instead of the generic `"The operation was aborted"`.
You may need to adjust import paths if `normalizeZenodoError` is moved to or used from other modules in your codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "Content-Type": "application/octet-stream", | ||
| }, | ||
| method: "PUT", | ||
| signal: AbortSignal.timeout(ZENODO_UPLOAD_TIMEOUT_MS), |
There was a problem hiding this comment.
suggestion: Consider handling fetch timeouts explicitly so users see a clearer error than a generic AbortError message.
AbortSignal.timeout prevents hanging, but when it fires fetch rejects with an AbortError whose message (e.g. "The operation was aborted") is not very helpful. Since failPublication displays err.message, users will see a vague error. Consider catching AbortError around these long-running requests (or in a shared wrapper) and rethrowing a clearer message like "Zenodo upload timed out after 5 minutes; please retry" so timeouts are distinguishable from other failures.
Suggested implementation:
/**
body: body.toString(),
=======
// Request timeouts so a stalled network call can never hang the publication
// workflow indefinitely. Metadata/API calls are quick; file up/downloads and
// the repo zipball can be large, so they get a much longer budget.
const ZENODO_API_TIMEOUT_MS = 60_000; // metadata / publish / deposition / API calls
const ZENODO_UPLOAD_TIMEOUT_MS = 300_000; // file up/downloads & repo zipball
// ===== Error helper ==================================================
/**
body: body.toString(),
+
+/**
+ * Normalize errors from Zenodo calls so we can surface clearer messages to the user.
+ *
+ * In particular, when AbortSignal-based timeouts fire, fetch rejects with an AbortError
+ * whose default message ("The operation was aborted") is not helpful. Callers should
+ * pass the timeout and a short context string (e.g. "file upload") so we can generate
+ * a clearer message.
+ */
+export function normalizeZenodoError(
+ err: unknown,
+ timeoutMs?: number,
+ context?: string,
+): Error {
+ if (err instanceof Error && err.name === "AbortError" && timeoutMs && context) {
+ const minutes = Math.round(timeoutMs / 60_000);
+ return new Error(
+ `Zenodo ${context} timed out after ${minutes} minute${minutes === 1 ? "" : "s"}; please retry.`,
+ );
+ }
+
+ return err instanceof Error ? err : new Error("Zenodo request failed");
+}To fully implement the suggestion, you should also:
- Wrap long-running fetches in try/catch and use
normalizeZenodoError. For example, where you perform metadata/API calls:- Locate the
fetchthat usessignal: AbortSignal.timeout(ZENODO_API_TIMEOUT_MS)(the one that producesresjust above theif (!res.ok)block). - Wrap it as:
let res: Response; try { res = await fetch(url, { // ...existing options... signal: AbortSignal.timeout(ZENODO_API_TIMEOUT_MS), }); } catch (err) { throw normalizeZenodoError(err, ZENODO_API_TIMEOUT_MS, "API request"); }
- Locate the
- Do the same for file uploads/downloads using
ZENODO_UPLOAD_TIMEOUT_MS:let res: Response; try { res = await fetch(uploadUrl, { // ...existing options... signal: AbortSignal.timeout(ZENODO_UPLOAD_TIMEOUT_MS), }); } catch (err) { throw normalizeZenodoError(err, ZENODO_UPLOAD_TIMEOUT_MS, "file upload"); }
- Ensure
failPublication(or any shared error handler) still displayserr.message, but now the error has already been normalized bynormalizeZenodoError, so users will see the clearer timeout-specific message instead of the generic"The operation was aborted".
You may need to adjust import paths if normalizeZenodoError is moved to or used from other modules in your codebase.
Summary by Sourcery
Harden the Zenodo release workflow against long-running network operations and dropped SSE connections, while improving client-side reconciliation of publish status and proxy configuration for streaming responses.
Bug Fixes:
Enhancements:
Deployment: