feat(sensor-server): make collection interval configurable by hardikkaurani · Pull Request #1054 · intel/pcm

hardikkaurani · 2026-09-30T11:46:23Z

Summary

Adds a configurable collection interval to pcm-sensor-server.

Changes

Add -i / --interval to configure the collection interval.
Preserve the existing 1 second default.
Validate the configured interval before starting the server.
Propagate the interval through HTTP and HTTPS server construction.
Replace the uninterruptible periodic sleep with an interruptible condition-variable wait so shutdown does not depend on the configured interval.
Preserve fixed-rate scheduling with overrun recovery to avoid catch-up busy loops.

Validation

git diff --check passes.
Source-level call-site and lifecycle analysis completed.
Automated compilation and PCM tests could not be executed locally because the available Windows environment does not have the required cmake/g++ toolchain and WSL is unavailable.

Compatibility

Existing constructor call sites remain source-compatible through default interval arguments, preserving the existing 1 second behavior when no interval is supplied.

Fixes #693

Fixes intel#693 This commit introduces a new command-line option -i or --interval to pcm-sensor-server, allowing users to specify the metric collection interval in seconds (defaulting to 1.0 seconds). This addresses the issue where the hardcoded 1-second delay was too frequent for some long-running use cases. Changes: - Added -i|--interval <seconds> option to argument parser. - Modified PeriodicCounterFetcher to accept an interval. - Replaced hardcoded std::chrono::seconds(1) with std::chrono::milliseconds based on the provided interval. - Updated HTTPServer and HTTPSServer constructors to propagate the interval.

- Replaced int-based milliseconds cast with double duration - Added busy-loop protection if collection overruns interval - Hardened CLI parser to reject NaN, Infinity, < 1ms, and > 1 year - Addressed maintainer review feedback during forensic audit

- Replaced std::this_thread::sleep_until with std::condition_variable - Ensures that HTTPServer::stop() immediately wakes the worker thread - Prevents shutdown delays when long collection intervals are configured

hardikkaurani · 2026-09-30T14:47:53Z

@rdementiev This PR implements configurable collection time for the PCM sensor server as requested in #693. Please review when you have a chance!

Copilot

Copilot review overview

🟡 Changes recommended

Shutdown synchronization, overrun recovery, and /persecond/X semantics need correction.

Review effort: Balanced
Findings: 1 · 2

Open (3)

Protect exit_ updates with mtx_ to prevent lost shutdown notifications · New
Preserve persecond endpoint time semantics when changing sample interval · New
Advance missed deadlines to prevent fetcher busy-looping · New

What changed in this PR

Adds configurable sampling intervals to pcm-sensor-server while retaining the one-second default.

Changes:

Adds and validates -i/--interval.
Propagates intervals through HTTP/HTTPS servers.
Adds interruptible, fixed-rate collection waits.

File	Description
`src/pcm-sensor-server.cpp`	Implements configurable collection scheduling and shutdown notification.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…ersecond/X semantics, and overrun recovery - Synchronize exit_ state update with mtx_ before cv_.notify_all() in PeriodicCounterFetcher::stop() to prevent lost wakeups - Advance overrun deadline to next future interval instead of current time to avoid busy-looping - Store sample timestamps in HTTPServer and dynamically size history buffer to preserve /persecond/X elapsed-time semantics across configurable intervals

…tion for all intervals - Remove arbitrary 3600-sample cap so history can retain full 29s across all supported intervals - Calculate maxAggregators_ as max(30, ceil(29 / interval) + 1) - Refine getAggregators wait predicate and search loop to safely support any index2

hardikkaurani · 2026-10-01T11:34:10Z

@rdementi I’ve gone through the three Copilot findings and addressed them in commit dec98f2.

The shutdown path now protects the exit_ update with the same mutex used by the condition-variable wait, so the notification cannot be missed.

I also updated the /persecond/X handling to work based on the actual sample timestamps rather than assuming that one sample always represents one second. The history size is now derived from the configured interval while keeping the existing behavior for the default 1-second interval.

For the scheduling logic, I added overrun recovery so that if a collection takes longer than the configured interval, the next wait is moved to a future deadline instead of repeatedly hitting an already-expired deadline.

I also checked the current validation status. AppVeyor Build #54820182 is passing with 0 warnings/errors, while the GitHub Actions workflows are currently waiting for the repository’s required approval/action.

The implementation is contained in src/pcm-sensor-server.cpp.

When you get a chance, could you please take a look at the updated changes? Thanks!

Copilot

Copilot review overview

🟡 Changes recommended

Coarse intervals break per-second endpoint semantics, while high-frequency settings risk excessive memory and processing overhead.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

Unbounded memory growth from high-frequency aggregator retention · New
Front insertion makes high-frequency collection O(history capacity) · New
Add regression tests for history selection and shutdown behavior · New

Resolved since last review (3)

Protect exit_ updates with mtx_ to prevent lost shutdown notifications
Advance missed deadlines to prevent fetcher busy-looping
Preserve persecond endpoint time semantics when changing sample interval

- Replace std::vector with std::deque for O(1) front insertion and back eviction - Implement two-tier history retention: recent high-resolution queue and coarse 1-second queue - Strictly bound total retained Aggregator objects to at most 60 across all collection intervals - Preserve 29-second elapsed-time contract and timestamp-based selection for /persecond/X - Add regression tests covering fractional interval selection, retention bounds, shutdown wakeup, and default behavior

hardikkaurani · 2026-10-01T18:02:33Z

@rdementi I’ve addressed the three findings from the latest Copilot review in the latest commit.

The history handling is now bounded for high-frequency collection intervals, so the number of retained AggregatorRecord objects no longer grows with the collection frequency. I also switched the history containers from std::vector to std::deque to avoid the overhead of shifting elements on each new sample.

I’ve also added regression tests covering the history selection, bounded retention, fractional intervals, and shutdown behavior.

Could you please take another look when you get a chance? Thanks!

Copilot

Copilot review overview

🟡 Changes recommended

History accuracy, shutdown safety, and test isolation need correction before approval.

Review effort: Balanced
Findings: 3 · 1 · 1

Open (5)

Coarse queue is too small for the 29-second history window · New
Signal handler can deadlock and uses unsafe shutdown operations · New
Tests leak port 80 listeners across runs · New
Fetcher may be deleted while stop() accesses its condition variable · New
Add regression tests for history selection and shutdown behavior

Resolved since last review (2)

Unbounded memory growth from high-frequency aggregator retention
Front insertion makes high-frequency collection O(history capacity)

hardikkaurani · 2026-10-02T10:07:15Z

@rdementi I've addressed all the remaining review findings in the latest update (a1a39c9) :

• Subsecond History Coverage: Expanded coarseQueue_ capacity to 60 (maxCoarseSamples_), ensuring the full 29-second history window is reliably satisfied across all subsecond intervals (including 0.75s and 0.4s) while strictly maintaining bounded memory ($\le 90$ total snapshots).

• Async-Signal-Safe Teardown: Updated POSIX SignalHandler::handleSignal() to record state via volatile sig_atomic_t and wake accept() safely, moving all shutdown operations (stop(), condition-variable wakeups, worker thread draining, and PMU cleanup) into normal execution context to eliminate mutex deadlock risks.

• Fetcher Lifecycle & Shutdown Handshake: Wrapped PeriodicCounterFetcher synchronization in reference-counted SharedState and added a deterministic doneCv completion wait in HTTPServer::stop(), removing sleep heuristics and preventing shutdown use-after-free.

• Test Isolation & Listener Cleanup: Added a socket-free construction path (createSocket = false) for the history unit test suite and verified socket cleanup in Server::~Server() and HTTPServer::stop(), avoiding port 80 binding and socket reuse conflicts.

• CI & Review Status: AppVeyor CI is passing cleanly and all review threads have been audited and resolved.

Whenever you have a moment, please take a look. Thanks!

hardikkaurani · 2026-10-16T16:30:42Z

Hi @rdementi,

Friendly follow-up when you have a moment to take a look. All previous review findings were fully addressed in commit a1a39c9 and CI is passing cleanly.

Please let me know if there are any specific changes or adjustments you'd like me to make. Thanks!

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

…rval validation, and signal safety

…afe idempotency

… self-join deadlock

…-worker deadlock Do not invoke emptyThreadPool() from inside the std::call_once cleanup body in HTTPServer::stop(). Instead, limit the once-protected server teardown to setting stopped_, notifying agVectorCV_, closing serverSocket_, and stopping PeriodicCounterFetcher. Drain and join the ThreadPool only from a non-worker control context via a dedicated once-flag, and add ThreadPool::isCurrentThreadWorker() to reliably detect caller thread pool membership. Add regression test TEST 12 verifying concurrent worker stop calls complete without deadlock or cross-worker join cycles.

hardikkaurani · 2026-10-18T07:37:16Z

Hi @rdementi,

I've resolved the shutdown deadlock and cross-worker synchronization in HTTPServer::stop() with commit 4011021:

Decoupled Server Teardown from ThreadPool Draining:
- stopOnceFlag_ now strictly handles server/resource shutdown (setting atomic stopped_, notifying agVectorCV_, closing serverSocket_, stopping PeriodicCounterFetcher, and clearing server/fetcher state). emptyThreadPool() is no longer invoked inside this body, allowing stopOnceFlag_ to complete without waiting on any worker threads.
- Thread pool draining is separated into a dedicated once-protected path (drainPoolOnceFlag_) executed only from non-worker control threads.
Safe Worker Detection (ThreadPool::isCurrentThreadWorker):
- Added ThreadPool::isCurrentThreadWorker() backed by zero-overhead thread-local storage (isWorker_).
- Worker callers perform the fast, non-blocking shutdown request and return immediately without joining peer workers or blocking each other.
- External control threads (main(), signal handlers, or server destructor) safely perform emptyThreadPool(). Detaching arbitrary workers has been removed in favor of clean joins.
Regression Tests:
- Retained the single-worker stop test (TEST 11).
- Added TEST 12 (ConcurrentWorkersStopDoesNotDeadlock) testing concurrent stop() invocations from 4 synchronized ThreadPool workers, verifying that all workers resolve in under 5 seconds without deadlock or std::system_error, and that sequential shutdown remains idempotent.

AppVeyor build 54918647 passed cleanly.

Whenever you have a chance, please take another look. Thanks!

hardikkaurani added 3 commits September 30, 2026 16:55

fix(pcm-sensor-server): make PeriodicCounterFetcher interruptible

3a0ac0f

- Replaced std::this_thread::sleep_until with std::condition_variable - Ensures that HTTPServer::stop() immediately wakes the worker thread - Prevents shutdown delays when long collection intervals are configured

rdementi requested a balanced review from Copilot October 1, 2026 09:05

Copilot started reviewing on behalf of rdementi October 1, 2026 09:06

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread src/pcm-sensor-server.cpp Outdated

Comment thread src/pcm-sensor-server.cpp Outdated

Comment thread src/pcm-sensor-server.cpp Outdated

hardikkaurani added 2 commits October 1, 2026 15:50

rdementi requested a balanced review from Copilot October 1, 2026 12:29

Copilot started reviewing on behalf of rdementi October 1, 2026 12:30

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread src/pcm-sensor-server.cpp Outdated

Comment thread src/pcm-sensor-server.cpp Outdated

Comment thread src/pcm-sensor-server.cpp Outdated

hardikkaurani closed this Oct 1, 2026

hardikkaurani deleted the feat/sensor-server-configurable-interval branch October 1, 2026 14:25

hardikkaurani restored the feat/sensor-server-configurable-interval branch October 1, 2026 14:26

hardikkaurani reopened this Oct 1, 2026

rdementi requested a balanced review from Copilot October 2, 2026 07:30

Copilot started reviewing on behalf of rdementi October 2, 2026 07:31

View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread src/pcm-sensor-server.cpp Outdated

Comment thread src/pcm-sensor-server.cpp Outdated

Comment thread tests/utests/pcm-sensor-server-history-utest.cpp Outdated

Comment thread src/pcm-sensor-server.cpp Outdated

fix(sensor-server): address review findings

a1a39c9

hardikkaurani requested a balanced review from Copilot October 18, 2026 04:21

Copilot AI reviewed Oct 18, 2026

hardikkaurani added 3 commits October 18, 2026 12:17

fix(sensor-server): address stopped_ synchronization, /persecond inte…

e3925a4

…rval validation, and signal safety

fix(sensor-server): serialize stop() with std::call_once for thread-s…

ce17e28

…afe idempotency

fix(threadpool): exclude calling thread in emptyThreadPool to prevent…

834cb1d

… self-join deadlock

Conversation

hardikkaurani commented Sep 30, 2026

Summary

Changes

Validation

Compatibility

Uh oh!

hardikkaurani commented Sep 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hardikkaurani commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hardikkaurani commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hardikkaurani commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hardikkaurani commented Oct 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

hardikkaurani commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hardikkaurani commented Oct 1, 2026 •

edited

Loading

hardikkaurani commented Oct 1, 2026 •

edited

Loading

hardikkaurani commented Oct 2, 2026 •

edited

Loading

hardikkaurani commented Oct 18, 2026 •

edited

Loading