Skip to content

fix: address code-review findings (batch validation, stale-snapshot guard, rate limiter) - #15

Merged
mytrashcan merged 1 commit into
mainfrom
fix/review-findings-0825
Aug 24, 2026
Merged

mytrashcan merged 1 commit into
mainfrom
fix/review-findings-0825

Conversation

@mytrashcan

Copy link
Copy Markdown
Owner

Summary

Fixes the three SHOULD-FIX findings from the code review:

  1. TossMarketDataClient.fetchPrices no longer fails an entire batch on one bad symbol — invalid symbols are now filtered out with a warning log instead of throwing IllegalArgumentException for all 200 symbols. The upstream 200-symbol limit is still enforced against the valid remainder, and an all-invalid input short-circuits with no HTTP call.

  2. Stale-snapshot guard for recent-price averages — PriceSpikeRule (and the price leg of PriceVolumeSurgeRule) now excludes snapshots older than a configurable max-age (detection.price-spike.max-age, default 30m) from the "recent average". Prior-session leftovers can no longer produce false spike detections at market open; if every snapshot is stale, the rule stays silent.

  3. ApiRateLimitFilter hardening

    • Optional trusted reverse-proxy support: new marketguard.security.trusted-proxies property resolves the client from the X-Forwarded-For chain, skipping the configured number of trusted hops (falls back to the socket address when the header is missing/short or when 0 proxies are trusted, so spoofing cannot bypass the limit).
    • Replaced the globally synchronized LRU map with a per-key-locked ConcurrentHashMap, removing the global choke point on every /api/ request.
    • Overflow eviction now purges entries from already-rolled-over windows first, so active clients are not evicted mid-window by floods of one-shot clients.

Tests

  • New/updated tests for each fix: batch filtering + >200 rejection in TossMarketDataClientCharacterizationTest; staleness tests in PriceSpikeRuleTest and PriceVolumeSurgeRuleTest; XFF resolution, spoof-resistance, and short-chain fallback in ApiRateLimitFilterTest.
  • Full ./gradlew build passes (all tests green, -Werror, JaCoCo 65% coverage gate).

…uard, rate limiter)

- TossMarketDataClient.fetchPrices: filter-and-warn invalid symbols instead
  of throwing for the whole batch; still rejects >200 valid symbols.
- PriceSpikeRule / PriceVolumeSurgeRule: exclude snapshots older than a
  configurable max-age (detection.price-spike.max-age) from the recent-price
  average so prior-session leftovers cannot trigger false spikes at open.
- ApiRateLimitFilter: optional trusted X-Forwarded-For support via
  marketguard.security.trusted-proxies; replace globally synchronized LRU
  map with per-key locked ConcurrentHashMap and purge stale windows before
  evicting active clients.
- Tests added/updated for each fix.

Co-authored-by: ox-alpha <noreply@nousresearch.com>

@mytrashcan mytrashcan left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review

Verified the diff in an isolated clone; CI fully green (verify, CodeQL, Analyze x2), merge state CLEAN.

Verified OK

  • fetchPrices: invalid symbols filtered with warn log instead of killing the batch; >200 valid symbols still rejected; empty-after-filter short-circuits without an HTTP call. Matches the ETN resilience concern.
  • PriceSpikeRule/PriceVolumeSurgeRule: maxAge staleness guard (config detection.price-spike.max-age, default 30m) filters stale snapshots out of the recent-price average; all-stale history yields no detection. Constructor validation added.
  • ApiRateLimitFilter: optional trusted-proxy XFF resolution (hop-count from right, safe socket fallback prevents spoofing), per-key locked ConcurrentHashMap replacing the global synchronized LRU, overflow eviction purges stale windows before touching active ones.
  • Tests added/updated across all three areas; -Werror + coverage gates satisfied.

Nit / Optional

  • boolean[] allowed as a compute() out-param works but a small holder record would read cleaner — author's discretion.

No blocking or should-fix issues found.

Reviewed by Hermes Agent

@mytrashcan
mytrashcan merged commit 499fa1a into main Aug 24, 2026
4 checks passed
@mytrashcan
mytrashcan deleted the fix/review-findings-0825 branch August 24, 2026 17:37
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