Skip to content

fix(ingest): send Content-Length on S3 PUTs so the WAL heartbeat lands - #936

Merged
Makisuo merged 1 commit into
mainfrom
fix/ingest-wal-heartbeat-content-length
Sep 18, 2026
Merged

Makisuo merged 1 commit into
mainfrom
fix/ingest-wal-heartbeat-content-length

Conversation

@Makisuo

@Makisuo Makisuo commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

What

The WAL owner heartbeat is an empty-body S3 PUT. hyper does not write Content-Length: 0 for an empty body, and S3 rejects that request with 411 MissingContentLength. Every task has logged "Failed to refresh the WAL owner heartbeat" once a minute since the S3 tier reached prod on 2026-09-04.

Consequence: no owners/<uuid> object ever existed, so stale_owners was always empty, recover_orphans never claimed anything, and segments shipped by a dying task sat under segments/<dead-owner>/ until the 7-day lifecycle deleted them. The 2026-09-18 05:47 task deaths lost their backlog this way.

Fix

  • S3Client::put sets content-length explicitly (signed with the rest of the headers).
  • The fake S3 in the telemetry tests now answers a PUT without Content-Length with 411, like S3 does. That is why the suite was green.
  • New test: a_heartbeat_lands_and_makes_its_owner_claimable_once_stale. I confirmed it fails without the fix.

Why now

This is the first step of moving the gateway to EC2 with its WAL on instance-store NVMe. Instance store is wiped when an instance is replaced, so the S3 tier has to work before the WAL moves there.

Verify after deploy

  • The "Failed to refresh the WAL owner heartbeat" WARNs stop.
  • wal/v1/owners/ in the WAL bucket holds one object per running task.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • Bug Fixes
    • S3 uploads now reliably support empty request bodies.
    • WAL owner heartbeats correctly create owner records and identify stale owners when heartbeats stop refreshing.

hyper omits Content-Length: 0 for an empty body and S3 answers that PUT
with 411 MissingContentLength. The owner heartbeat is an empty PUT, so it
has failed every minute since the S3 tier reached prod: no owner object
ever existed, no dead task was ever seen as stale, and every shipped
segment sat unclaimed until the 7-day lifecycle deleted it.

The fake S3 now refuses a PUT without Content-Length, as S3 does, and a
new test covers heartbeat -> stale owner discovery.
@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 9b090375-ac88-4050-b5d4-b9a2208d5e64

📥 Commits

Reviewing files that changed from the base of the PR and between b047d13 and 047f744.

📒 Files selected for processing (2)
  • apps/ingest/src/aws.rs
  • apps/ingest/src/telemetry.rs

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


📝 Walkthrough

Walkthrough

The change adds explicit Content-Length headers to S3 PUT requests, updates the fake S3 server to enforce this requirement, and adds a WAL heartbeat test for owner creation and stale detection.

Changes

S3 and WAL heartbeat

Layer / File(s) Summary
S3 PUT content length
apps/ingest/src/aws.rs, apps/ingest/src/telemetry.rs
S3Client::put sends the request body length, including 0 for empty bodies. The fake S3 server returns HTTP 411 when Content-Length is missing.
WAL heartbeat validation
apps/ingest/src/telemetry.rs
An asynchronous test verifies that a heartbeat creates the owner object and that the owner is stale after one hour without a refreshed heartbeat.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 047f7

The change correctly supplies Content-Length for heartbeat PUTs and tests owner creation and stale detection.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding Content-Length to S3 PUT requests to fix WAL heartbeat failures.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.98.0)

Clippy execution failed


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

@Makisuo
Makisuo merged commit f390ae8 into main Sep 18, 2026
35 checks passed
@Makisuo
Makisuo deleted the fix/ingest-wal-heartbeat-content-length branch September 18, 2026 23:50
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