fix: pass pagination limit to log-reading callables to prevent OOM#860
Open
JeremiahChurch wants to merge 1 commit intopfrest:masterfrom
Open
fix: pass pagination limit to log-reading callables to prevent OOM#860JeremiahChurch wants to merge 1 commit intopfrest:masterfrom
JeremiahChurch wants to merge 1 commit intopfrest:masterfrom
Conversation
Log endpoints load ALL log files (current + every rotated/compressed copy) into memory before Model::read_all() applies pagination via array_slice(). Even ?limit=1 loads the entire dataset, causing PHP memory exhaustion (512MB) on systems with moderate log volumes. This passes the pagination limit from Model::read_all() through get_internal_objects() to log-reading callables so they stop early. LogFileModelTraits changes: - read_log() accepts optional $limit parameter - When limit > 0, reads newest files first and stops when satisfied - Plain files: reverse-reads from EOF via fseek - O(limit) memory - Compressed files: streams via PHP stream wrappers (compress.zlib://, compress.bzip2://) and popen (xz) with a ring buffer - O(limit) memory for all formats - gather_log_filepaths() uses glob($base.'.*') to avoid matching unrelated files, sorts newest-first by rotation number - Unbounded reads (limit=0) preserve existing behavior exactly Model.inc changes: - get_internal_objects() accepts optional $limit hint - Forwards limit to callables via cached reflection check (backward-compatible with callables that don't accept it) - read_all() passes limit+offset down; skips optimization when reverse=true since that needs the full dataset All 6 log model callables updated to accept and forward limit. 10 new test cases covering bounded reads, edge cases, mixed compression, and unrelated file filtering. Tested on live pfSense (FreeBSD 15.0, 28MB filter.log + 31 rotated bz2 files): memory dropped from 512MB+ (OOM) to 22MB, response time 1ms for ?limit=10. Fixes pfrest#806
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.
Summary
Fixes #806. Passes the pagination
limitfromModel::read_all()down throughget_internal_objects()to log-reading callables so they can stop reading early instead of loading entire log histories into memory.LogFileModelTraits::read_log()gains optional$limitparameter. When set, reads newest files first using memory-efficient readers and stops once enough lines are collectedfseek- O(limit) memorycompress.zlib://,compress.bzip2://) andpopen(xz) with a ring buffer - O(limit) memory for all formatsgather_log_filepaths()now usesglob($base.'.*')to avoid matching unrelated files (e.g.filter.logrotate), sorts newest-first by rotation number extracted from the suffix after the base pathModel::get_internal_objects()accepts optional$limithint, forwards to callables via cached reflection (backward-compatible with callables that don't accept it)Model::read_all()passeslimit + offsetdown; skips the optimization when$reverse=truesince that needs the full dataset$limit=0) follow the exact same code path as beforeTested on live pfSense (FreeBSD 15.0, REST API v2.5)
28MB
filter.log+ 31 rotated.bz2files:?limit=50memory?limit=10response timeFiles changed (9 files, +624 -48)
LogFileModelTraits.inc- core fix: bounded reading with tail/streaming readersModel.inc- plumbing: forward limit hint throughget_internal_objects()to callablesFirewallLog.inc,SystemLog.inc,DHCPLog.inc,AuthLog.inc,OpenVPNLog.inc,RESTAPILog.inc- trivial: addint $limit = 0parameter to each callableAPIModelTraitsLogFileModelTraitsTestCase.inc- 10 new test casesTest plan
LogFileModelTraitstests pass unchanged (unbounded reads)Model::read_all(limit: 10)on FirewallLog returns correct results without OOMread_log()still returns all entries (backward compat)