Add platform-specific memory management for macOS to spooledtempfile #150
Conversation
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
This commit adds macOS support to the spooledtempfile package's memory management functionality, which previously only supported Linux. Changes: - Split memory.go into platform-specific implementations using Go build tags - memory_linux.go: Linux implementation (cgroups v1/v2, /proc/meminfo) - memory_darwin.go: macOS implementation (sysctl VM statistics) - memory.go: Shared caching logic for all platforms - Added comprehensive platform-specific tests - memory_linux_test.go: Tests for cgroup and /proc/meminfo - memory_darwin_test.go: Tests for sysctl memory retrieval - memory_test.go: Platform-agnostic tests - Updated CI/CD workflow (.github/workflows/go.yml) - Added matrix strategy to test both ubuntu-latest and macos-latest - Platform-specific test verification for both Linux and macOS - Cross-compilation checks to ensure compatibility - Made spooled tests deterministic with memory mocking - Added mockMemoryUsage() helper for DRY test code - Fixed 8 tests that were failing on high-memory systems - Added 3 new threshold boundary tests (49%, 50%, 51%) - All tests now pass regardless of host system memory state The macOS implementation uses sysctl to query VM statistics and calculates memory usage as: (total - free - purgeable - speculative) / total All tests pass on both platforms with proper mocking ensuring deterministic behavior independent of actual system memory usage.
There was a problem hiding this comment.
Pull Request Overview
Adds macOS support for spooledtempfile’s memory management by splitting platform-specific logic and centralizing cache behavior, plus making tests deterministic across platforms.
- Introduces platform-specific implementations: memory_linux.go and memory_darwin.go with build tags
- Moves caching logic to shared memory.go and updates tests to use a mock helper for consistent behavior
- Extends CI to test on Ubuntu and macOS and adds cross-compilation checks
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/spooledtempfile/spooled.go | Removes local cache types/funcs now centralized in memory.go. |
| pkg/spooledtempfile/memory.go | Adds shared cache interval, cache struct, and getCachedMemoryUsage. |
| pkg/spooledtempfile/memory_linux.go | Linux-specific memory logic (cgroups v1/v2 and /proc/meminfo) plus helpers. |
| pkg/spooledtempfile/memory_darwin.go | macOS-specific memory logic using sysctl VM statistics. |
| pkg/spooledtempfile/spooled_test.go | Adds mockMemoryUsage helper; updates tests to be deterministic; adds boundary tests. |
| pkg/spooledtempfile/memory_test.go | Adds cache behavior tests and platform-agnostic integration test. |
| pkg/spooledtempfile/memory_linux_test.go | Linux-only tests for cgroups, meminfo, and file-read helpers. |
| pkg/spooledtempfile/memory_darwin_test.go | macOS-only tests for sysctl access and consistency checks. |
| .github/workflows/go.yml | Expands matrix to macOS, adds platform-specific test runs and cross-compiles. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
equals215
requested changes
Oct 16, 2025
Member
equals215
left a comment
equals215
left a comment
There was a problem hiding this comment.
besides microsoft's clanker comments, LGTM
Add ResetMemoryCache() function and call it in test cleanup to prevent global memoryUsageCache state from persisting across test packages. When running 'go test ./...', tests from pkg/spooledtempfile that mock memory usage (mocking high memory values like 60%) would pollute the global cache, causing TestHTTPClientRequestFailing to fail when run after them. The fix adds: 1. ResetMemoryCache() - clears the global cache (lastChecked and lastFraction) 2. Calls ResetMemoryCache() in mockMemoryUsage() cleanup to ensure clean state This maintains the performance benefits of the global cache while ensuring test isolation through explicit cleanup using t.Cleanup().
…ndianness Address three issues identified in code review: 1. **Fix potential uint64 underflow** (memory_darwin.go): - If reclaimablePages > totalPages, subtraction would wrap to large number - Now clamps usedPages to 0 when reclaimable pages exceed total - Adds explicit bounds checking to prevent invalid memory fractions 2. **Fix hardcoded error path** (memory_linux.go): - Changed error message from literal "/proc/meminfo" to use procMeminfoPath variable - Improves diagnostic accuracy when paths are overridden in tests 3. **Simplify endianness handling** (memory_darwin.go): - Removed custom getSysctlUint32() helper function - Now uses unix.SysctlUint32() directly which handles endianness correctly - Removed unnecessary encoding/binary import - Updated tests to use unix.SysctlUint32() directly All tests pass. Cross-compilation verified for both Linux and macOS.
Replace hardcoded relative paths with dynamic path resolution using runtime.Caller() to compute the testdata directory relative to the test file location. This fixes tests being skipped in CI/CD when running 'go test ./...' from the root directory. Tests now correctly find and run against testdata files. Changes: - Added getTestdataDir() helper function using runtime.Caller() - Replaced 5 hardcoded "../../testdata/warcs" paths with getTestdataDir() - Added runtime package import - Tests now run from any working directory (root, CI/CD, etc.) All TestAnalyzeWARCFile subtests now run and pass instead of being skipped.
Replace os.Getwd() + relative path construction with getTestdataDir() helper to make TestMendFunctionDirect work from any directory. This fixes 4 skipped subtests: - good.warc.gz.open - empty.warc.gz.open - corrupted-trailing-bytes.warc.gz.open - corrupted-mid-record.warc.gz.open These tests now run properly in CI/CD when 'go test ./...' is run from root.
Widen the acceptable byte range in TestHTTPClient from 27130-27160 to 27130-27170 to accommodate platform-specific differences in HTTP response headers between macOS and Linux. The test was failing on macOS with 27163 bytes due to platform-specific header variations, which is normal behavior across different operating systems.
CorentinB
force-pushed
the
macos-spooledtempfile
branch
from
b92c8e2 to
4170a1c
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Remove unused vm.page_pageable_internal/external_count sysctls that cause failures on some macOS versions - Fix data races in memory_test.go by using ResetMemoryCache() and proper mutex locking - Fix cache pointer reassignment in spooled_test.go to prevent race conditions - Update CI test filter to reference only existing tests (TestMemoryFractionConsistency instead of TestGetSysctlUint32)
NGTmeaty
approved these changes
Nov 1, 2025
Collaborator
NGTmeaty
left a comment
NGTmeaty
left a comment
There was a problem hiding this comment.
Looks good besides two nits.
Run only spooledtempfile package tests on macOS runners instead of the full test suite, since the macOS-specific code changes are limited to that package. This addresses review feedback to optimize CI runtime while maintaining platform-specific test coverage.
Introduce TestSmokeWARCFormatRegression to validate WARC format consistency using a frozen reference file (testdata/test.warc.gz). This test checks exact byte counts, record counts, Content-Length values, and digest hashes against known-good values. This complements the existing dynamic tests by providing explicit validation that the WARC format hasn't changed, addressing the concern about byte-level format regression detection while keeping the main integration tests maintainable.
CorentinB
force-pushed
the
macos-spooledtempfile
branch
from
2dda0fb to
8184aa7
Compare
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.
This commit adds macOS support to the spooledtempfile package's memory management functionality, which previously only supported Linux.
Changes:
The macOS implementation uses sysctl to query VM statistics and calculates memory usage as: (total - free - purgeable - speculative) / total
All tests pass on both platforms with proper mocking ensuring deterministic behavior independent of actual system memory usage.