Sol/Fable Review of Durable Server#18
Open
jeregrine wants to merge 21 commits into
Open
Conversation
jeregrine
changed the title
DurableServer accepted any binary key, including __nodes/... paths reserved for LifecycleManager heartbeats. A child persisted at one of those paths was later decoded as heartbeat data; the refresh failed and could exhaust the supervisor's restart intensity. Validate keys before any start path reaches storage and reject both the __nodes root and descendants. Add a regression test proving a reserved key fails before a child can be started.
Auto-sync treated every storage error as transient. When another owner replaced the stored object, the stale process logged the CAS conflict, acknowledged its callback, and remained available to process more messages. Treat an auto-sync conflict as definitive ownership loss, matching explicit and periodic sync behavior. Centralize the fatal conflict path and add a regression test that advances the stored ETag behind an auto-sync process and verifies that its next mutation terminates it.
Crash handling fetched the latest object and reused its ETag without confirming that the stored metadata still belonged to the process being terminated. If another process had already claimed the key, the stale process could overwrite that owner's running status and crash history. Verify the persisted PID, node reference, and node identity against the current boot before updating metadata. Treat a mismatch as lost ownership, and add a regression test that installs a newer owner before crashing the stale process and verifies the newer metadata remains untouched.
Self-deletion previously ignored a conflict while persisting :deleting and then unconditionally removed the storage key. A stale process could therefore delete an object that a newer process had already claimed. Make the successful CAS write of the :deleting status the deletion linearization point. Only issue the storage delete after that write succeeds; a conflict leaves the newer owner's object untouched. Preserve the synced state internally so deletion failures are retained, and add a regression test that replaces the owner before the stale process terminates.
terminate_and_delete_child previously treated a process DOWN as proof of deletion. The DurableServer sent an in-progress message before terminate/2 touched storage, logged any backend error, exited, and caused both PID and key callers to receive :ok while the object remained. Carry the requester and operation reference into terminate/2, send the final persistence/deletion result before exit, and wait for both that result and process termination under one monotonic deadline. Propagate non-timeout failures through the key path, and add deterministic PID/key tests with a backend that rejects deletion.
Prefix ownership was stored in persistent_term before Supervisor.start_link. Nothing erased it after a normal stop or failed init, so a supervisor could never restart with the same storage prefix until the VM restarted. Add a unique local Registry and atomically register the prefix from the supervisor process during init. Registry removes the claim when that process exits; startup briefly waits for asynchronous Registry cleanup from a dead owner while still rejecting a live competing supervisor. Cover normal shutdown and failed initialization, and remove the test-only persistent_term cleanup workaround.
The DurableServer callback contract advertised normal GenServer timeout
actions and {:shutdown, :normal} stops, but callback normalization
rejected integer actions and omitted the wrapped normal clauses.
Following either documented form crashed the process as a bad callback
return or invalid stop reason.
Define the accepted callback action guard to include non-negative
integer timeouts for reply and noreply paths. Handle wrapped normal
stops as graceful persistence while preserving their exit reason. Add
call/noreply timeout coverage and verify wrapped normal stops persist
stopped_graceful.
A leader could dispatch its result and unregister after a caller observed it but before that caller registered as a waiter. Because the leader's caller process could remain alive, the waiter then slept until its full deadline and returned a timeout even though the operation had completed. Register the waiter and immediately re-read the owner registry as one handshake: wait only while the observed owner is still registered, follow a replacement owner, or retry immediately when the old owner has finished. Because the leader dispatches results before unregistering, a waiter that registers inside that narrow window still falls back to the timeout-then-retry path, so this removes the common interleaving rather than every possible one. Also limit Registry error recovery to the registration call so an ArgumentError raised by the guarded operation is not executed a second time. Add deterministic coverage for the stale-owner registration state.
Remote placement checked its deadline only between rounds, so one round could still make several sequential ERPC calls with fresh fixed 3-8 second timeouts after the caller's budget had expired. Thread the original monotonic deadline through every placement round, cap each ERPC and remote child timeout by the remaining budget, and stop before an expired attempt. A unit test verifies the per-node ERPC timeout is capped by the caller's remaining budget and collapses to zero once the deadline has passed.
Terminator applied graceful_shutdown_timeout_ms independently to every child. With limited concurrency, total shutdown time grew by the number of batches and routinely exceeded the parent supervisor timeout, leaving later children to be killed without coordinated persistence. Final sync failures were also counted as graceful completion. Create one monotonic deadline before stopping discovery and pass its remaining budget to discovery and every child task. Later batches kill immediately after the shared budget expires instead of receiving a fresh timeout. Compare lifecycle diagnostics around shutdown and warn when final persistence failed. Add regression coverage for four serialized slow children and for a final CAS failure.
Map-based max_children enforcement performed two unsynchronized Group count checks around startup. Concurrent callers could all observe an empty registry, pass both checks, and exceed total or per-module limits. Integer limits were left to DynamicSupervisor and leaked its raw :max_children error. Use ETS update_counter reservations as local capacity slots before spawning. A small reservation keeper transfers each slot from the starting caller to the child process and releases it on failure, caller death, or child termination. Bootstrap rechecks resource/draining state without rejecting its own reserved slot, current_capacity reads the exact slot counters, and integer limits use the same normalized total-limit path. Add concurrent slow-init coverage and assert the integer API returns the documented capacity error.
Heartbeat retries recognized only bare atom reasons. Object-store backends actually return Req response and exception structs, so a single retryable 503, timeout, refused connection, closed socket, or HTTP/2 unprocessed error immediately crashed the one-for-all supervisor tree instead of using the heartbeat deadline budget. Classify Req's transient response, transport, and HTTP/2 errors consistently with Req's retry policy while preserving immediate failure for permanent errors. Extend the in-memory backend with deterministic heartbeat failures and prove startup survives a 503 followed by a transport timeout before writing its heartbeat.
Persisted heartbeat timestamps are wall-clock values, so a peer whose clock is far ahead could remain healthy long after it died. The same wall clock also drove local retry and watchdog deadlines, allowing an NTP adjustment to extend or prematurely expire this node's safety window. Add a validated heartbeat_future_skew_tolerance_ms option (5 seconds by default) and require heartbeat ages to fall between that future bound and the relevant staleness threshold. Ignore out-of-bound records during cache/source merging without deleting a potentially live peer's data. Capture a matching monotonic instant when each heartbeat is created and use it for local write budgets, scheduling, and watchdog enforcement while retaining wall time on disk. Resolve the skew tolerance through a compatibility helper that uses the configured value for running supervisors and the documented default when no supervisor ETS config exists, so the pure restart-gate helpers keep supporting synthetic supervisor names and heartbeat tables. Cover both sides of the skew policy and the monotonic owner timestamp.
Discovery used Task.Supervisor.async/2, which linked each worker to the LifecycleManager. Because the manager does not trap exits, an exception killed it immediately and the existing monitored :DOWN recovery clause never ran; the one-for-all tree then restarted despite code intended to clear state and retry discovery. Start discovery with async_nolink/2 so failures arrive through the task monitor. The existing handler can now log the error, clear potentially stale skip/gate state, and schedule the next sweep. A fault-injecting backend raises on its first discovery listing and verifies a second attempt runs while the manager remains alive.
Promotion performed an unconditional put after the preferred backend returned not_found. If another writer created the preferred object while the fallback read was in flight, promotion overwrote that newer authoritative value with stale fallback data and returned the stale value as successful. Use the backend's atomic try_claim operation to promote only when the preferred key is still absent. If a concurrent create wins, re-read and return its preferred-backend body and ETag; surface only genuine claim failures as promotion errors. A barrier-controlled race covers both the old put path and new claim path, and existing EKV promotion/failure integration tests remain green.
Metadata was Base64-decoded with unrestricted binary_to_term/1. Anyone able to modify stored objects could intern arbitrary atoms, deserialize executable/opaque terms, or submit compressed and oversized ETF payloads before schema checks, risking VM memory exhaustion and unsafe data reaching lifecycle logic. Bound encoded and raw metadata size, reject compressed ETF, decode with binary_to_term/2's :safe option, recursively reject unsupported term kinds, and validate every retained Meta field before constructing the struct. Accept binary node references alongside current integer references and nil so previously persisted metadata with opaque incarnation references stays readable. Malformed inputs now become controlled ArgumentErrors that StoredState returns as errors, so discovery stays alive while callers receive a data error instead of a partially typed Meta struct. Tests cover valid legacy round trips, unknown-atom non-creation, function/compressed/malformed payload rejection, and invalid field types, and the corrupted-restart-metadata lifecycle test now asserts the controlled rejection instead of accepting corrupt strings.
IAM response bodies were passed directly to SweetXml.xpath/2, which implicitly parsed remote XML with its default DTD behavior. A compromised or malicious IAM endpoint could therefore attempt external-entity file reads or entity-expansion attacks in the application VM. Parse every IAM XML response exactly once with dtd: :none, quiet parsing, and a 1 MiB input limit before applying XPath. Normalize rejected or malformed documents to the existing XML/error paths. A temporary-file XXE regression proves a DTD payload is rejected while ordinary IAM XML remains queryable.
CreateAccessKey debug/error logging emitted the complete IAM XML body, including SecretAccessKey, and successful supervisor startup inspected the entire config. Custom backend state and arbitrary init_info therefore appeared in ordinary info logs; failed backend initialization could also echo raw options. Move access-key extraction into a tested parser that never logs its payload, remove credential IDs and raw IAM bodies from messages, and report only operation-level parse status. Supervisor startup now logs operational fields plus adapter module names while omitting backend state, object stores, circuit-breaker internals, and init_info. Backend validation errors likewise avoid echoing raw options. Capture-log tests use sentinel credentials to prove both paths are redacted at debug level.
Mint 1.9.2 accepts signed HTTP/1 chunk sizes, enabling response smuggling when a pooled connection crosses an intermediary with stricter parsing (CVE-2026-59249 / GHSA-x3x7-96vm-6h2w). Advance only the compatible transitive Mint lock entry to 1.9.3, which replaces Integer.parse/2 with a parser restricted to hexadecimal digits and also hardens undersized HTTP/2 control frames. Req and ReqS3 remain unchanged as requested. The full unit and available EKV/Mirror integration suites pass with the patched transport.
Document the user-facing changes from this series under an unreleased 0.1.5 heading: the new heartbeat skew tolerance option, capacity limit enforcement and error-shape changes, reserved key validation, callback contract fixes, deletion and shutdown semantics, prefix claim release, metadata decoding hardening, log redaction, and the Mint security bump.
jeregrine
force-pushed
the
js-p1-hardening
branch
from
July 16, 2026 15:36
528f51f to
b0562f0
Compare
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
1 participant
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.
first commit is just updating deps and fixing 1.20 type errors. Every commit after is individual fix of an issue with test.