Skip to content

fix(ruby): require shared replay reservations#235

Open
EfeDurmaz16 wants to merge 11 commits into
fix/rust-security-hardeningfrom
fix/ruby-replay-store-capability
Open

fix(ruby): require shared replay reservations#235
EfeDurmaz16 wants to merge 11 commits into
fix/rust-security-hardeningfrom
fix/ruby-replay-store-capability

Conversation

@EfeDurmaz16

Copy link
Copy Markdown
Collaborator

Require an explicit durable-and-shared replay-store capability outside localnet. FileStore remains suitable for single-process examples but is rejected for multi-worker production because its mutex is not cross-process.

Tests:

  • mise exec ruby@3.3 -- bundle exec rake test (484 runs, 1461 assertions)

@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR enforces that non-localnet MPP servers supply a replay store that is both durable and cross-process shared, closing a correctness gap where a FileStore (in-process mutex only) was previously accepted for devnet/mainnet. The harness gains a new HarnessReplayStore backed by flock + atomic rename to satisfy the new contract in CI.

  • runtime.rb replaces durable_replay_store? with durable_shared_replay_store?, which now checks shared? == true, verifies put_if_absent is overridden from the base class, and validates arity. The FileStore is correctly rejected for non-localnet by this change.
  • harness/ruby-server/server.rb switches from a per-PID FileStore to a scenario-scoped HarnessReplayStore for non-localnet runs (or MemoryStore on localnet), requiring MPP_HARNESS_REPLAY_STORE_PATH to be set explicitly for devnet/mainnet harness invocations.
  • Test coverage is thorough: the new dev_store_warning_test.rb cases cover truthy-but-not-true capability markers, missing put_if_absent, wrong-arity overrides, and explicit durable-shared acceptance; harness_replay_store_test.rb exercises cross-process atomicity via fork.

Confidence Score: 5/5

Safe to merge; the validation logic, harness store, and test suite are all consistent and the previously noted arity-check edge case is tracked in an open thread.

The change is purely additive in the enforcement direction — it makes it harder to misconfigure a production store, not easier. The new HarnessReplayStore uses well-understood POSIX primitives (flock + atomic rename) and the cross-process test exercises it with eight concurrent forked workers. The one known limitation (arity -1 false-positive for single-optional-arg methods) is already documented in a previous review thread.

runtime.rb — the accepts_two_positional_arguments? helper, as discussed in the existing review thread.

Important Files Changed

Filename Overview
ruby/lib/pay_kit/protocols/mpp/runtime.rb Core validation logic extended to require shared? == true and arity check on put_if_absent; an arity false-positive for single-optional-arg methods was noted in a previous review thread.
harness/ruby-server/harness_replay_store.rb New flock + atomic-rename store correctly satisfies durable? and shared? for cross-process test workers; implementation is sound.
harness/ruby-server/server.rb Switches non-localnet harness to HarnessReplayStore gated on MPP_HARNESS_REPLAY_STORE_PATH; exits with status 2 if path is missing, preventing silent misconfiguration.
harness/test/e2e.test.ts Injects a per-run, per-scenario replay store path; REPLAY_STORE_RUN_ID combines PID + timestamp + random to prevent cross-invocation collisions.
ruby/lib/pay_kit/protocols/mpp/store.rb Adds shared? (false by default) to Store base and FileStore; correctly documents that FileStore is not cross-process safe.
ruby/test/pay_kit/protocols/mpp/harness_replay_store_test.rb Fork-based concurrency test verifies exactly one winner on a shared key across 8 workers and persisted-restart semantics.
ruby/test/pay_kit/protocols/mpp/dev_store_warning_test.rb Comprehensive new test matrix covers FileStore rejection, truthy-but-not-true markers, missing put_if_absent, wrong arity, and acceptance of a well-formed durable+shared store.
ruby/test/pay_kit/dispatcher_test.rb Updated to use MemoryStore with singleton-method overrides for durable? and shared?, properly exercising the duck-typed validation path.
ruby/test/pay_kit/middleware_test.rb Migrated from FileStore to singleton-decorated MemoryStore; put_if_absent idempotency assertions preserved.
ruby/test/pay_kit/harness_adapter_test.rb New integration tests verify that devnet mode boots with an explicit store path and exits non-zero without one.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["PayKit::Protocols::Mpp.create(replay_store:)"] --> B{replay_store == DEV_ONLY_MEMORY_STORE?}
    B -- Yes --> C{localnet?}
    C -- No --> ERR1["ConfigurationError\n'durable replay_store shared\nacross workers required'"]
    C -- Yes --> WARN["warn: volatile MemoryStore\n(dev only)"] --> MEM["replay_store = MemoryStore.new"]
    B -- No --> D{replay_store nil?}
    D -- Yes --> ERR2["ConfigurationError\n'nil is not a valid store'"]
    D -- No --> E{localnet?}
    E -- Yes --> OK["Server::Charge.new(...)"]
    E -- No --> F["durable_shared_replay_store?(store)"]
    F --> G{durable? == true}
    G -- No --> ERR3["ConfigurationError\n'durable replay_store shared\nacross workers required'"]
    G -- Yes --> H{shared? == true}
    H -- No --> ERR3
    H -- Yes --> I{put_if_absent overridden\nfrom base Store?}
    I -- No --> ERR3
    I -- Yes --> J{accepts_two_positional_args?}
    J -- No --> ERR3
    J -- Yes --> OK
    MEM --> OK
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["PayKit::Protocols::Mpp.create(replay_store:)"] --> B{replay_store == DEV_ONLY_MEMORY_STORE?}
    B -- Yes --> C{localnet?}
    C -- No --> ERR1["ConfigurationError\n'durable replay_store shared\nacross workers required'"]
    C -- Yes --> WARN["warn: volatile MemoryStore\n(dev only)"] --> MEM["replay_store = MemoryStore.new"]
    B -- No --> D{replay_store nil?}
    D -- Yes --> ERR2["ConfigurationError\n'nil is not a valid store'"]
    D -- No --> E{localnet?}
    E -- Yes --> OK["Server::Charge.new(...)"]
    E -- No --> F["durable_shared_replay_store?(store)"]
    F --> G{durable? == true}
    G -- No --> ERR3["ConfigurationError\n'durable replay_store shared\nacross workers required'"]
    G -- Yes --> H{shared? == true}
    H -- No --> ERR3
    H -- Yes --> I{put_if_absent overridden\nfrom base Store?}
    I -- No --> ERR3
    I -- Yes --> J{accepts_two_positional_args?}
    J -- No --> ERR3
    J -- Yes --> OK
    MEM --> OK
Loading

Reviews (10): Last reviewed commit: "Merge remote-tracking branch 'origin/fix..." | Re-trigger Greptile

@EfeDurmaz16

Copy link
Copy Markdown
Collaborator Author

@greptile-apps please review.

@EfeDurmaz16
EfeDurmaz16 requested a review from lgalabru July 10, 2026 13:42
@EfeDurmaz16

Copy link
Copy Markdown
Collaborator Author

@greptile-apps please re-review exact head a20b530. This hardens the harness replay store with cross-process atomic replacement and tightens durable/shared store capability validation.

EfeDurmaz16 added a commit that referenced this pull request Jul 14, 2026
…hygiene gates

The ruby native-guard probe follows #235's durable_shared_replay_store?
predicate rename (the guard itself is unchanged and stronger), and the
missing-ATA hygiene gate matches the reconciled deliver-or-reject step name
while still asserting the step stays blocking with the red-fault env.
@EfeDurmaz16
EfeDurmaz16 changed the base branch from split/pr216-ci-harness-mega to fix/rust-security-hardening July 15, 2026 11:17
Comment on lines +108 to 111
def self.accepts_two_positional_arguments?(method)
arity = method.arity
arity == 2 || (arity.negative? && (-arity - 1) <= 2)
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Arity check has a false-positive for single-optional-arg signatures

Method#arity reports -1 for both def put_if_absent(*args) (unlimited) and def put_if_absent(key = nil) (maximum 1 arg). The formula -(-1) - 1 = 0 ≤ 2 passes both, but calling the second with two positional arguments raises ArgumentError (wrong number of arguments (given 2, expected 0..1)) at runtime — meaning a store that declares durable? and shared? but accidentally implements put_if_absent with one optional arg passes validation and then crashes on the first real payment attempt. Method#parameters provides exact per-parameter types and lets you check both minimum and maximum positional capacity without this ambiguity.

Suggested change
def self.accepts_two_positional_arguments?(method)
arity = method.arity
arity == 2 || (arity.negative? && (-arity - 1) <= 2)
end
def self.accepts_two_positional_arguments?(method)
params = method.parameters
positional_req = params.count { |type, _| type == :req }
positional_opt = params.count { |type, _| type == :opt }
has_splat = params.any? { |type, _| type == :rest }
max_positional = has_splat ? Float::INFINITY : positional_req + positional_opt
positional_req <= 2 && max_positional >= 2
end

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