Skip to content

Reduce backup upload buffer overhead - #2926

Draft
stopachka wants to merge 1 commit into
mainfrom
codex/backup-upload-buffers
Draft

stopachka wants to merge 1 commit into
mainfrom
codex/backup-upload-buffers

Conversation

@stopachka

@stopachka stopachka commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

This is a standalone efficiency optimization. It does not resolve the September 24 outage investigation and should not be deployed on the assumption that it prevents recurrence.

Backup uploads currently hand at most 8 KiB to the AWS SDK on each read, while SDK 2.46.2 allocates a 16 KiB backing array for every read. Multipart retries retain those arrays. A filled 32 MiB payload queue therefore retains roughly 64 MiB of backing arrays.

Fill the caller's buffer before returning from the backup pipe, except at EOF. The same queue then uses roughly 32 MiB of backing arrays. This changes only the backup reader; compression, object contents, multipart sizing, concurrency, retry behavior and publication order stay the same. Sparse namespaces can wait for more bytes, and finish when their writer closes.

Validation:

  • Five unit tests, 52 assertions: byte equality at segment/chunk boundaries, EOF, read overloads, errors, close, pipe cancellation, and the actual SDK's buffer allocations.
  • Six cases against isolated MinIO using the actual backup stream/completion functions: baseline and candidate small backups, baseline and candidate 37.2 MB compressed five-part backups, a consumed first-part response changed to 503 to trigger replay, and a stalled first part. Exact decompressed bytes, hashes, config, counts, object metadata and publication order matched. Database operations were mocked; this is not a full production snapshot/restore test.
  • Targeted clj-kondo and git diff --check passed. Independent review found no actionable regressions. Clojure CI also passed on e1f513f61: full lint, uberjar build and all five test shards.

This reduces Java heap allocations. With the fixed 90 GiB heap, the reduction need not translate into lower host RSS. The failed host exhausted root-volume read throughput while telemetry stopped, but retained evidence does not identify the initiating process or connect these buffers to the reads. Passing a full nightly backup would establish additional functional confidence; it would not by itself establish outage prevention. That requires attributing or reproducing the host stall, then showing that the proposed mitigation prevents it while the complete backup and concurrent request workload succeed.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: e47ddb23-3536-4aea-884c-30692eec1e53

📥 Commits

Reviewing files that changed from the base of the PR and between 94b167f and e1f513f.

📒 Files selected for processing (2)
  • server/src/instant/backup.clj
  • server/test/instant/backup_test.clj

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The backup upload wraps its pipe input stream with filling-input-stream. New tests check read behavior, error handling, cancellation, and the resulting SDK payload-buffer capacity.

Changes

Backup stream upload

Layer / File(s) Summary
Wrap and validate backup input reads
server/src/instant/backup.clj, server/test/instant/backup_test.clj
start-stream passes the pipe input stream through filling-input-stream. Tests cover read overloads, byte contents, errors, closure, cancellation, and SDK payload-buffer capacity.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: dwwoelfel

Merge Risk: ⚪ Minimal · up to e1f51

The changed backup reader has no identified issue that needs resolution before merge. The planned full-backup rollout check remains appropriate.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main change: reducing buffer overhead during backup uploads.
Description check ✅ Passed The description directly explains the buffer optimization, its scope, validation, limitations, and relationship to the outage investigation.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
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.

1 participant