feat(storage): S3 manifest with CAS-coordinated writes and stale-refresh reads - #4
Merged
Merged
Conversation
…esh reads
Introduce a per-(tenant, signal) `manifest.json` at
`tenants/<tenant>/v2/logs/manifest.json` as the coordination surface
`docs/architecture.md` calls for. Writes go through one owner process
per manifest that coalesces concurrent segment registrations into one
S3 conditional PUT per flush window (default 10ms) or immediately at
batch cap 256. Reads are served from a lock-free ETS cache and, once
the cache entry ages past `refresh_stale_ms` (default 1s), refreshed
with a conditional GET so cross-node writes become visible without a
local write kicking the cache.
NIF (`native/pulso_object_store/src/lib.rs`):
* New `put_if_none_match`, `put_if_match`, `get_if_none_match`.
* `put` now returns `{:ok, etag}` so callers can prime a CAS cache
without a follow-up GET.
* Shared `stream_body_into` helper keeps GET at one full-payload
allocation streamed into a Rustler `NewBinary` — no Rust-side
intermediate.
* Error mapping extends to `:not_modified`, `:already_exists`,
`:precondition_failed`.
Elixir manifest layer (new modules under `Pulso.Storage.S3`):
* `Manifest` + `Segment` — compact JSON wire form (1-2 char keys),
invariant of segments sorted by `max_ts` desc, O(n) `merge/2`
with per-batch key dedup.
* `ManifestCache` — public ETS table with `read_concurrency: true`;
the query hot path does a single `:ets.lookup/2` with no message
passing.
* `ManifestOwner` — Registry-addressed GenServer that owns writes,
coalesces flushes, retries on `:precondition_failed`, and does
the first-write LIST rebuild when no manifest exists yet.
* `ManifestSupervision` — Registry + DynamicSupervisor + Cache,
wired into `Pulso.Application` only when the S3 adapter is active.
`Pulso.Storage.S3` rewired:
* `append/3` writes the segment then CAS-registers it via the
owner; returns `:ok` only after both land in S3.
* `query/2` reads the manifest from the cache (refreshing when
stale), prunes by time bounds, and fetches only the survivors.
Codex adversarial review addressed three concerns before merge:
* Cross-node cache staleness — added stale-refresh conditional GET.
* Rebuild used to include sidecar / non-`.ndjson` objects with nil
bounds, producing a manifest that failed to reload; rebuild now
skips any key that does not parse to integer time bounds.
* Owner mailbox was unbounded and callers hit raw `exit(:timeout)`;
`register_segments/5` now probes `:message_queue_len` and returns
`{:error, :owner_overloaded}` at the cap (default 512, config-
overridable), and `call_owner/3` translates the timeout `exit`
into `{:error, :timeout}` so the `@spec` holds in every failure.
Regression tests cover the manifest data model, conditional NIF ops,
end-to-end round-trips, write coalescing, cross-node stale refresh,
rebuild-time filtering of non-segment objects, and mailbox-cap
rejection.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pepicrft
marked this pull request as ready for review
September 25, 2026 17:46
…pers
CI's Credo job failed on three findings against the manifest owner:
* `cas_with_retry/3` was cyclomatic complexity 11 (max 9) because the
`:precondition_failed` and `:already_exists` arms each carried
their own reload-and-retry `case`. Extract `attempt_cas/2` and
`reload_and_retry/3` so the recovery paths share one helper and
the main function stays linear.
* `rebuild_from_prefix/1` was cyclomatic complexity 12 (max 9) and
nested a `case` four levels deep in the create-race branch.
Extract `publish_rebuilt_manifest/2`, `rebuild_segments/1`, and
`reload_after_create_race/2`; use `with` to flatten the successful
path.
* Two `Logger.warning/2` calls passed metadata keys (`tenant`,
`signal`, `reason`, `retries_left`) that are not declared in the
global Logger config, which Credo flags as a warning. Inline the
same fields into the message string so the log line stays useful
without polluting global metadata configuration.
No behavior change — the CAS retry semantics, rebuild filtering, and
error propagation are byte-for-byte the same. Full unit and integration
suites still pass locally (`mix test`, `PULSO_INTEGRATION=1 mix test
--only integration`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's Quokka job runs `mix format --check-formatted` and the two multi-line `Logger.warning(...)` blocks I added in the previous commit re-collapse onto a single line under the project's formatter config. Reformat so the working tree matches what CI expects. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Introduce a per-
(tenant, signal)manifest.jsonattenants/<tenant>/v2/logs/manifest.jsonas the coordination surface Pulso's architecture doc calls for. Every accepted append is registered in the manifest via a per-tenant owner process that coalesces concurrent segment registrations into a single S3 conditional PUT per flush window; every query reads the manifest from a lock-free ETS cache, prunes by time bounds, and fetches only the survivors.Concretely:
native/pulso_object_store/src/lib.rs): newput_if_none_match,put_if_match,get_if_none_match.putnow returns{:ok, etag}so callers can prime a CAS (compare-and-swap, Wikipedia) cache without a follow-up GET. A sharedstream_body_intohelper keeps every GET path at one full-payload allocation streamed straight into a RustlerNewBinary, no Rust-side intermediate. Error mapping now covers:not_modified,:already_exists,:precondition_failed.Pulso.Storage.S3.Manifest+Segment): compact JSON on the wire (1-2 character keys), an in-memory invariant of segments sorted bymax_tsdescending, and an O(n)merge/2with per-batch key dedup.Pulso.Storage.S3.ManifestCache): public ETS table withread_concurrency: true. Query path does a single:ets.lookup/2— no message passing, no locks under load.Pulso.Storage.S3.ManifestOwner): Registry-addressed GenServer that owns writes for one manifest, batches pending segments into one CAS per flush window (default 10ms) or immediately at batch cap (default 256), retries on:precondition_failedwith a reload-and-rebuild, and does a first-write LIST rebuild when no manifest exists yet.Pulso.Storage.S3.ManifestSupervision): Registry + DynamicSupervisor + Cache, wired intoPulso.Applicationonly when the S3 adapter is active.Pulso.Storage.S3):append/3writes the segment then CAS-registers it via the owner and returns:okonly after both durability points land.query/2reads the manifest from the cache and, when the cached entry ages pastrefresh_stale_ms(default 1s), refreshes it with a conditional GET so cross-node writes become visible without a local write kicking the cache.Why
The current storage adapter discovers segments by LIST-ing the tenant prefix on every query, which is fine for a few objects but does not scale.
docs/architecture.mdis explicit that S3 is the only shared coordination surface for Pulso and that manifests updated via conditional PUT are the load-bearing durability point. Landing the manifest layer first is what unlocks the follow-up work (Parquet segments, compaction, alerting, cross-node freshness gossip) without redesigning coordination each time.Optimization goals for the initial cut were minimizing copies at the NIF boundary, maximum write throughput per tenant, a lock-free query hot path, and a compact wire form.
Approach
The tradeoffs I made explicitly:
1 / flush_interval_msper tenant. Ingesters block inGenServer.call/3and get their reply once the CAS lands, so the ingester layer does not have to know about the CAS itself.read_concurrency: true. Every query is one lock-free lookup. Writes to the cache go only through the owner (single writer per tenant), so I do not pay forwrite_concurrency.manifest.jsonshape the architecture doc names.refresh_stale_ms. This is the bound on cross-node freshness in the absence of gossip.register_segments/5probes the owner's mailbox length before enqueuing and returns{:error, :owner_overloaded}at the cap rather than allowing unbounded mailbox growth; theGenServer.call/3timeout is translated into{:error, :timeout}so the typed spec holds in every failure mode.Impact
Pulso.ObjectStore.put/3now returns{:ok, etag}instead of:ok. Only callers inside this repo are affected and all are updated in this change.v2/logs/prefix. Non-.ndjsonobjects under the prefix are skipped at rebuild time.Pulso.Storage.S3config accepts two new optional keys —refresh_stale_ms(default 1000) for the cross-node freshness window, andmax_mailbox(default 512) for the backpressure cap.Validation
Codex adversarial review addressed three concrete findings before merge:
rebuild_from_prefixused to include every object under the signal prefix. A sidecar or malformed key became a segment withnilbounds, which the nextManifest.decode/1rejected, permanently breaking the tenant. Rebuild now skips any key that does not parse to integer time bounds.:precondition_failedretries, and callers hitexit({:timeout, {GenServer, :call, _}})instead of the specified error tuple.register_segments/5now probes the mailbox length and returns{:error, :owner_overloaded}at the cap; the timeoutexitis translated into{:error, :timeout}so the@specholds in every failure mode.Commands run locally:
mix precommit— clean; 84 unit tests pass.PULSO_INTEGRATION=1 mix test --only integration— clean; 25 integration tests pass against the local RustFS (RustFS is the S3-compatible object store the repo bundles viadocker-compose.yml).Regression tests were added for each Codex finding:
stale cache refreshes via conditional GET so cross-node writes become visible,rebuild skips non-segment objects (sidecars, junk under the prefix), andrejects appends with :owner_overloaded when the mailbox is at the cap.🤖 Generated with Claude Code