Skip to content

feat(firehose): honor S3 size and interval buffering hints - #2747

Merged
vieiralucas merged 9 commits into
faiscadev:mainfrom
cavanaug:feat/firehose-s3-buffering
Oct 18, 2026
Merged

vieiralucas merged 9 commits into
faiscadev:mainfrom
cavanaug:feat/firehose-s3-buffering

Conversation

@cavanaug

@cavanaug cavanaug commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Final increment of the Firehose S3 delivery series tracked in #2708. Stacked on GZIP (#2745) and Xerial Snappy (#2746). Until those merge, this PR's diff against main also contains their commits; the buffering changes are the commits from feat(firehose): honor S3 size and interval buffering hints onward.

  • Honor decoded-byte size and first-arrival interval buffering hints across producer calls, with effective defaults of 5 MiB / 300 seconds and paired validation of 1–128 MiB / 0–900 seconds (fixes the old 0 or 60-900 check, per Firehose: staged contributions for S3 output fidelity, GZIP/Snappy, and size/time buffering #2708 item 2).
  • Share one engine across public Firehose APIs, CloudWatch Logs subscriptions, and SES event destinations. Admit whole API batches without splitting records; empty blobs remain valid without creating empty objects unless delimiters are enabled. Exact byte concatenation is unchanged.
  • Capture destination/configuration/key at detachment, preserve FIFO retry ordering with 1–30 s exponential backoff, and guard commits against delete/reset/recreate races using runtime stream identities.
  • As on AWS for DirectPut streams, stop retrying a batch 24 hours after its oldest record arrived and discard it with a warning; this also bounds memory retained during prolonged destination failure.
  • Preserve upstream S3 versioning, default Object Lock retention, ownership, durability, default CRC64NVME checksums of the encoded bytes, and bucket-default encryption (including KMS), replication, and notifications in the guarded writer. The producer guard is taken before the S3 write lock and both are released before notification callbacks (documented and regression-tested).
  • Narrow, Firehose-owned lifecycle: one worker started by the server, reset discards pending buffers, and graceful shutdown closes admission after the HTTP server stops and performs one final drain with undelivered counts (Firehose: staged contributions for S3 output fidelity, GZIP/Snappy, and size/time buffering #2708 lifecycle guidance).
  • Reject record admission for AWS-valid but unimplemented settings — ZIP/HADOOP_SNAPPY, processing other than AppendDelimiterToRecord alone, format conversion, dynamic partitioning, prefix expressions, non-UTC time zones, explicit destination KMS, S3 backup, and non-S3 destinations — with HTTP 400 InvalidArgumentException naming the setting (Firehose: staged contributions for S3 output fidelity, GZIP/Snappy, and size/time buffering #2708 item 3). Create/UpdateDestination and CloudFormation keep accepting every modeled value; an unsupported update pauses already accepted active data until the configuration is supported again. Admission, scheduling, and the final drain share one capability check.
  • Enforce the model's S3 destination length limits (Prefix/ErrorOutputPrefix ≤ 1,024, CustomTimeZone ≤ 50, RoleARN 1–512, BucketARN 1–2,048) on create/update and CloudFormation, so unbounded values cannot be copied into every buffered batch and object key. ARN and time-zone patterns are not enforced. Values such as an empty RoleARN, accepted on main, now return InvalidArgumentException.
  • CloudFormation coerces quoted BufferingHints/RetryOptions integers and Enabled flags (as produced by Ref parameters and AWS's own examples), including inside S3BackupConfiguration.

Scope and compatibility

Acknowledgment now means accepted into process-local memory, not immediately visible in S3; poll S3 for delivery. A malformed bucket ARN, which main acknowledges and only logs, now fails record admission with InvalidArgumentException (Create/UpdateDestination still accept it). Pending buffers/retries are not durable across a crash; healthy graceful shutdown drains them. Within the 24-hour retention window there is no memory-budget backpressure.

Delivery does not assume RoleARN or evaluate IAM/bucket policies (unchanged from main; now documented), and Logs/SES may target another account's stream as before. A stream stored without an S3 destination (an S3 configuration missing RoleARN/BucketARN, which AWS rejects) still acknowledges records without writing them, as on main; documented rather than changed here.

The Logs-to-Firehose raw GZIP fix already landed in #2720. The test(logs) commit only adds multi-member payload and buffered-delivery coverage.

Review

Before publication the stack had an independent code/design review (engine concurrency and lifecycle; S3/CloudFormation/Logs/SES integration, tests, and docs) and a STRIDE security review. No Critical findings. Addressed here: stale docs, duplicated readiness checks that could let the drain spin, unbounded Prefix × retained batches, unlimited retries vs AWS's 24 hours, per-scan configuration copies under global locks, caller-controlled text (object keys, errors, stream names, ARNs) in log lines, now Debug-escaped, the CloudFormation quoted-hint regression, an unused S3 helper, and weak negative E2E waits (now proven against healthy control streams). Not changed (pre-existing, follow-ups): SES's delivery log can list records Firehose refused because the internal FirehoseDelivery hook returns no result; Firehose is not IAM-enforced.

Test plan

Verified on 6945abc71 (stacked on Snappy c5907ef3a, upstream 774c772d6):

  • 1,826 focused unit tests: CloudFormation (638), Firehose (161), Logs (364), S3 (663); 27 server reset tests.
  • 212 rebuilt-server SDK tests across Firehose buffering/codecs/persistence/validation, Logs, SES and SES→Firehose events, S3 checksums/persistence/SSE-KMS/write side effects; one Athena regression; 107 Firehose/SES/S3 conformance tests.
  • Workspace all-target Clippy with warnings denied, formatting, documentation counts, diff checks, and the E2E nextest partition check (2,673 tests, each covered exactly once).
  • Every commit in the stack compiles on its own (cargo check --workspace --all-targets), so the history bisects.
  • New behavior was observed failing before its fix (24-hour retention, destination string limits, log escaping, notification lock release).
  • The unrestricted cargo test --workspace (every E2E test, including container runtimes) was not rerun on this revision. On the GZIP predecessor it stopped on the unchanged, upstream-reproduced API Gateway fixed-port test (expected 502, got 504 at closed 127.0.0.1:54321 in this WSL environment). No exclusions were added, and the workspace gate is not claimed as passing.

Series context

Refs #2708; completes the four-increment plan. Branch feat/firehose-s3-buffering is stacked on feat/firehose-snappy. As each predecessor merges, I will rebase onto main so only the buffering commits remain, and revalidate.


Summary by cubic

Completes the Firehose S3 delivery series by adding buffered delivery that honors decoded-byte size and first-arrival interval hints from PutRecord/PutRecordBatch, CloudWatch Logs subscriptions, and SES event destinations, all through one shared engine. Delivery now retries up to 24 hours, rejects unsupported settings with HTTP 400, and enforces destination string limits.

New Features:

  • Buffering hints default to 5 MiB / 300 s and validate to 1–128 MiB / 0–900 s.
  • One engine serves the public API, Logs, and SES; batches are not split across calls.
  • Unsupported settings (e.g., ZIP, processing, dynamic partitioning, non-S3 destinations) reject record admission with InvalidArgumentException.
  • Failed batches retry in FIFO order every 5 seconds and stop after the oldest record exceeds 24 hours.
  • Destination SSE-KMS encryption is carried through buffered delivery and survives UpdateDestination.

Migration:

  • Acknowledgment now means accepted into process-local memory; poll S3 for delivery.
  • A malformed bucket ARN fails record admission (previously only logged).
  • Pending buffers are not durable across a crash; graceful shutdown drains them once.
  • CloudFormation coerces quoted BufferingHints/RetryOptions integers and fails the resource on unparseable or wrong-typed scalars.

Written for commit cc260a3. Summary will update on new commits.

View guided diff

cavanaug and others added 9 commits October 18, 2026 15:44
Buffer records from PutRecord/PutRecordBatch, CloudWatch Logs subscriptions, and SES event destinations in one Firehose-owned engine. Size (decoded bytes) and first-arrival interval hints decide when a batch is written; IntervalInSeconds accepts 0-900 and SizeInMBs 1-128. Detached batches keep their captured configuration and object key, retry in FIFO order with 1-30 s exponential backoff, and are guarded against delete/reset/recreate by runtime stream identities. The server starts one worker, reset discards pending buffers, and graceful shutdown closes admission and makes one final drain.

Unsupported delivery settings, explicit destination KMS, S3 backup, and non-S3 destinations reject record admission with HTTP 400 InvalidArgumentException naming the setting, while Create/UpdateDestination and CloudFormation keep accepting every modeled value; an unsupported update pauses already accepted active data. The raw GZIP Logs handoff landed in faiscadev#2720; this adds multi-member and buffered-delivery coverage for it.
As on AWS for DirectPut streams, a batch whose oldest record arrived more than 24 hours earlier is discarded with a warning on its next failed attempt, which also bounds retained memory during prolonged failure. Admission, scheduling, and the final drain now share one delivery-capability check so they cannot drift, the pause diagnostic latches on the stream version instead of copying the configuration on every scan, the final-attempt warning is logged only by the drain, and caller-supplied values are Debug-escaped in warnings.
Prefix and ErrorOutputPrefix allow 1,024 characters, CustomTimeZone 50, RoleARN 1-512, and BucketARN 1-2,048, the Firehose model's length limits, so unbounded values cannot be copied into every buffered batch and object key. Create/UpdateDestination and CloudFormation return InvalidArgumentException as for FileExtension. Also document that constructing the service does not start delivery.
Ref parameters resolve to strings and AWS examples quote numbers, so BufferingHints, RetryOptions.DurationInSeconds, and Enabled flags, including inside S3BackupConfiguration, are coerced before the strict API parser runs. Non-numeric strings still fail the resource.
Spell out that the producer guard is taken before the S3 write lock and both are released before notifications, drop the unguarded options helper that no longer has a production caller, and cover a commit whose account was removed after staging.
Assert the interval lower bound, prove worker liveness in negative waits with healthy control streams, cover SDK buffering-hint and Prefix limits on create and update, and show a Firehose service reset discards pending buffers.
…y trust model

The services index now matches the parity row and service page: unsupported settings fail record admission with HTTP 400, bucket-default encryption, replication, and notifications apply, and failed batches are retried for up to 24 hours. The service page also lists destination string limits, CloudFormation coercion, when a malformed bucket ARN is rejected, and that delivery does not evaluate RoleARN or IAM.
…etry every 5s

- Capture EncryptionConfiguration.KMSEncryptionConfig in the detached batch's
  DeliveryConfig and pass it to the guarded S3 writer, so buffered objects are
  written with SSE-KMS under AWSKMSKeyARN instead of the bucket default.
- Retry failed S3 batches every 5 seconds (AWS's documented S3 cadence) up to
  the 24-hour limit, instead of 1-30s exponential backoff.
- Drop remaining KMS admission-rejection docs/tests; ASCII punctuation in docs.
- CloudFormation fails the resource when a modeled boolean/integer/double in
  the destination block is unparseable or the wrong JSON type (integers are
  32-bit, no whitespace trimming), naming the full property path, instead of
  echoing values DescribeDeliveryStream cannot deserialize; merge overlapping
  coercion tests.
- Test that captured SSE-KMS reaches the S3 write and survives UpdateDestination.
- Fix the Firehose parity row ordering.
@vieiralucas
vieiralucas force-pushed the feat/firehose-s3-buffering branch from 6945abc to cc260a3 Compare October 18, 2026 18:51
@vieiralucas
vieiralucas merged commit c8b3207 into faiscadev:main Oct 18, 2026
158 checks passed
@vieiralucas

Copy link
Copy Markdown
Member

Thanks a lot for this, @cavanaug. Real size/interval buffering across producer calls, shared by the Firehose API, Logs subscriptions and SES events, with detach-time config capture, delete/reset/recreate guards, the 24-hour retry limit and a clean shutdown drain, is the piece that makes Firehose behave like the real thing end to end, and it closes out the #2708 plan. The engine design held up well under review. I pushed a few follow-ups before merging, so a couple of points in the PR description no longer apply:

  • Rebased onto main after feat(firehose): deliver GZIP-compressed S3 objects #2745/feat(firehose): deliver Xerial Snappy-compressed S3 objects #2746 landed. A destination KMSEncryptionConfig is now implemented, not rejected: the key is captured at detachment and buffered objects are written with SSE-KMS under AWSKMSKeyARN (with tests that the captured key reaches the S3 write and survives UpdateDestination).
  • Failed S3 batches now retry every 5 seconds, matching AWS's documented S3 cadence, up to the 24-hour limit (instead of 1-30s exponential backoff).
  • CloudFormation uses one recursive model-member coercion for quoted template scalars, and fails the resource (naming the property path) when a boolean/integer/double is unparseable, out of 32-bit range, or the wrong JSON type, instead of echoing values DescribeDeliveryStream cannot deserialize.
  • Doc wording fixes and ASCII punctuation.

Merged. Really appreciate the whole series.

@cavanaug

Copy link
Copy Markdown
Contributor Author

FYI. I didn’t benchmark this but I suspect that fakecloud firehose performance is now vastly improved with the buffering vs previous.

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.

2 participants