ascan: Pause timers when pausing scan

[kingthorin]

No description provided.

[Copilot]

Pull request overview

This pull request implements pause-aware timers for the active scanner, ensuring that paused time is excluded from elapsed time calculations. The changes address Issue 3455 by introducing a StopWatch-based timing mechanism that suspends/resumes when scans are paused/resumed, and propagating pause-aware timestamps throughout the scanner hierarchy (Scanner → HostProcess → Plugin).

Changes:

• Introduced StopWatch-based timing in Scanner with pause/resume support via ReentrantLock and Condition
• Updated timing methods across Scanner, HostProcess, AbstractPlugin, and PluginStats to use scanner-aligned Instant values
• Added comprehensive unit tests for pause timer functionality

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
ScannerPauseTimerUnitTest.java	New comprehensive test suite validating pause timer behavior, including elapsed time exclusion, waitIfPaused blocking, and state transitions
PluginStatsUnitTest.java	New test validating PluginStats timing with null Instant fallback
AbstractPluginUnitTest.java	Extended tests to verify plugin timing uses parent's effective instant
Scanner.java	Core timer implementation with StopWatch, pause/resume locking, and virtual time methods
PluginStats.java	Updated to accept Instant-based timing for pause-aware plugin statistics
HostProcess.java	Modified to use scanner's effective instant for pause-aware timing and duration checks
AbstractPlugin.java	Updated setTimeStarted/setTimeFinished to use scanner's effective instant
ScanProgressItem.java	Modified getElapsedTime to use scanner-aligned timing for running plugins
ActiveScan.java	Added state field management with synchronized blocks and virtual time method overrides

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The state field is declared as volatile and updated here without synchronization, while other state updates (in pauseScan, resumeScan, start, stopScan) are synchronized. This creates a potential race condition where the state could be read inconsistently. Either all state updates should be synchronized, or none should be (if volatile is sufficient). Given that other methods use synchronized blocks with compound checks (e.g., checking and updating state atomically), this update should also be synchronized.

Suggested change

	state = State.FINISHED;
	synchronized (this) {
	state = State.FINISHED;
	}

[Copilot]

Similar to the maxScanDurationInMins issue, this maxRuleDurationInMins check compares getNowInstant() (which excludes paused time) against plugin.getTimeStarted() (which is now set using getEffectiveInstant(), also excluding paused time). This means the duration check now excludes paused periods. While this may be intentional (only count active scanning time), it represents a behavior change from the previous implementation and should be clearly documented or reconsidered if wall-clock time was the intended metric.

[Copilot]

The StopWatch instance (watch) is accessed without holding pauseLock in getElapsedMillis() and getElapsedSeconds(), but Apache Commons Lang's StopWatch is not thread-safe. While most StopWatch operations in this class are protected by pauseLock, these read methods are not. This could lead to inconsistent reads or potential race conditions. Consider either protecting these methods with the lock or documenting why it's safe (e.g., if getDuration() is thread-safe for read operations).

Suggested change

	return watch.getDuration().toMillis();
	}
	public double getElapsedSeconds() {
	var duration = watch.getDuration();
	return duration.getSeconds() + (double) duration.getNano() / TimeUnit.SECONDS.toNanos(1);
	pauseLock.lock();
	try {
	return watch.getDuration().toMillis();
	} finally {
	pauseLock.unlock();
	}
	}
	public double getElapsedSeconds() {
	pauseLock.lock();
	try {
	var duration = watch.getDuration();
	return duration.getSeconds()
	+ (double) duration.getNano() / TimeUnit.SECONDS.toNanos(1);
	} finally {
	pauseLock.unlock();
	}

[Copilot]

The getEffectiveInstant() method is public but lacks JavaDoc documentation. Since this is a key method in the pause timer implementation that returns the current "effective" instant (start time plus elapsed active time), it should have documentation explaining what it returns, when it returns null, and how it relates to paused time being excluded.

[Copilot]

The lock is released in the finally block after setting pause to false, but watch.resume() is also called in the finally block. This creates a potential issue: if watch.resume() throws an exception, pauseLock.unlock() will not be called, leading to a deadlock. The unlock() call should be in its own try-finally block, or watch.resume() and pause = false should be inside the main try block.

Suggested change

	} finally {
	if (watch.isSuspended()) {
	watch.resume();
	}
	pause = false;
	if (watch.isSuspended()) {
	watch.resume();
	}
	pause = false;
	} finally {

[Copilot]

There are two different ways to determine the scan state: the state field (used in pauseScan, resumeScan, start, stopScan) and the getState() method (which derives state from timeStarted, isStop(), and isPaused()). These two approaches can diverge. For example, if the state field is RUNNING but isStop() returns true, getState() would return FINISHED while the field says RUNNING. Consider using only one approach throughout for consistency, or ensure they are always kept in sync.

[Copilot]

The elapsed time calculation uses getNowInstant() which includes paused time (it's just the scanner's effective instant). However, the maxScanDurationInMins feature should likely check wall-clock time, not active scan time. If a scan is paused for a long time, the elapsed calculation here would not include that paused time, meaning the scan could run longer than the configured maximum duration in terms of wall-clock time. Consider whether this is the intended behavior or if maxScanDurationInMins should use wall-clock time instead.

[Copilot]

The getEffectiveInstant() method is public but lacks JavaDoc documentation. This method is crucial for coordinating pause-aware timing across scanner components and should document its purpose, return value, and relationship to the parent scanner's effective instant.

[Copilot]

The getElapsedSeconds() method is public but lacks JavaDoc documentation. While getElapsedMillis() has a doc comment, getElapsedSeconds() should also be documented to explain that it returns the elapsed seconds excluding paused time, similar to getElapsedMillis().