feat(servermonitor): dedupe sub-process report, add cursor + in-memory merge - #33
Merged
Merged
Conversation
…y merge The "Slow Sub-Processes" section of the profile summary previously printed one line per scan, so a process alive across many scan ticks was repeated dozens of times, and a process that finished between two scans was missed entirely. Worse, a process that outlived the profile window was not attributable to that profile at all. Three changes, per-pid: 1. Deduplicate per pid across scans. Each entry now carries first/last observation time plus the estimated lifetime (observed interval + age delta), instead of a single snapshot age. 2. Cumulative report cursor. The high-water mark (subProcessLogCursor) is persisted in the profile's .json metadata sidecar. The first report covers the profile's time window; each subsequent report shows only what was observed since the previous report, so long-running external processes are still attributed to the profile that triggered it. 3. In-memory observation registry. MonitoringService now tracks each pid's first/last-seen and max age across scans (pruned after 2 consecutive missed scans, so one transient /proc read failure does not drop a process). The report merges this live view with the persistent subprocesses.jsonl log, covering processes still observable right now — including ones that started after the profile window ended. Verified: mvn -pl src compile, ant -f src/build.xml compile, TestSubProcessLog (8), TestAsyncProfilerWrapper (5) all pass. Co-Authored-By: Claude Code <noreply@anthropic.com>
szhatchenko
force-pushed
the
feature/subprocess-report-dedup-cursor-v2
branch
from
August 31, 2026 11:33
389330e to
ae26f26
Compare
Contributor
Author
Review fixes — addressed in amended commit
|
…ecar, half-open interval Address review of the sub-process dedup/cursor feature: 1. (blocker) appendSubProcessSummary returned early when >20 slow entries were shown, skipping persistSubProcessCursor — so a busy system repeated the same report forever. Now breaks out of the loop so the cursor is always advanced after a report is generated. 2. (high) A malformed-but-present metadata sidecar (meta==null) was being atomically replaced with a cursor-only JSON, destroying startTime/endTime and other fields. persistSubProcessCursor now skips the write when the sidecar exists but failed to parse. 3. Make the follow-up cursor interval half-open (cursor, now] so an observation exactly at the cursor is reported once: the summary passes cursor+1 to the (inclusive) log reader, and the in-memory overlap filter excludes lastSeen <= qStart. Javadocs updated to match. 4. Remove the dead firstAgeSec field from SubProcess/SubProcessObservation (stored and propagated but never read after the lifetime fix). 5. Add testCursorIntervalIsHalfOpen (log reader + in-memory boundary) and a malformed-sidecar case to the cursor persistence test. Co-Authored-By: Claude <noreply@anthropic.com>
…ater mark
Address the concurrency review of the sub-process cursor feature:
1. (high) Two concurrent appendSubProcessSummary() calls for the same profile
could read the same cursor, generate overlapping reports, and persist their
report-time values out of order — moving the cursor backwards and re-reporting
a range. Fix:
- serialize the read -> generate -> persist sequence per profile with a
lock keyed by (sidecar path, base name);
- persistSubProcessCursor now re-reads the on-disk cursor and refuses to
write a value that would not advance it (monotonic high-water guard), as
defense in depth against any caller writing outside the lock.
2. Document that the cursor is a true high-water mark: the monitor appends the
log synchronously in the same checkSubProcesses() call that records the
observation timestamp, so every log line with timestamp <= now is on disk
before a report generated at `now` reads it.
3. Soften the getObservedSubProcesses "still alive" doc: the in-memory registry
is an approximation (pruned after SUB_PROCESS_MISS_GRACE missed scans), not a
live guarantee.
7. Log a WARNING (once per request) when the cursor is skipped due to a malformed
sidecar, so an admin can tell why the cursor is not advancing.
Add testCursorNeverMovesBackwards: an out-of-order persist (300 then 200) must
not regress the persisted cursor, and a newer value still advances it.
Co-Authored-By: Claude <noreply@anthropic.com>
…d lock map, real concurrency test Address the review of the sub-process cursor high-water guard: 1. (high) readSubProcessCursor conflated "absent cursor" (0) with "file exists but malformed" (also 0), so the high-water guard could treat a malformed sidecar as cursor 0 and clobber it. It now returns Long: 0 for absent, the stored value if parseable, null if exists-but-unreadable. persistSubProcessCursor skips the write on null, mirroring the existing meta == null guard, so the malformed file is left intact even if it appeared/changed between the summary's read and the persist. 2. (medium) The per-profile lock map was an unbounded ConcurrentHashMap that retained a lock per profile forever. It is now reference-counted: the last thread to leave the synchronized block removes its entry, so the map does not grow with the number of profiles. The lock key uses canonicalPath so the same profile reached via different path spellings/symlinks shares one lock. 3. Move the plugin check INSIDE the per-profile lock: the early return was previously before the lock was taken, so the lock only covered the happy path. The whole method body is now serialized per profile. 4. (low) Correct the high-water Javadoc: it is guaranteed because (a) the scan timestamp is taken before the append, and (b) both append and read are serialized on MonitoringService.subProcessLogLock, so a read sees a log closed under "append completed" — a line cannot land behind the cursor with timestamp <= now. Add a real concurrency regression test (testConcurrentReportsAreSerializedPerProfile) driving appendSubProcessSummary through the actual lock via a small in-lock test hook; asserts same-profile reports never overlap in the lock and the registry is empty afterward. Co-Authored-By: Claude <noreply@anthropic.com>
…profile lock registry The reference-counted sub-process report lock registry only counted threads that had entered the monitor (holders), so an entry could be evicted while a thread still blocked on its monitor was about to enter. A later thread would then create a second lock object for the same profile, and two reports for the same profile could run concurrently — defeating the per-profile serialization that keeps the high-water cursor from regressing. The fix acquires the registry reference (users++) *before* entering the monitor via an atomic compute, so a waiting thread keeps the entry alive, and releases it (users--, evict at 0) in a finally after the monitor is released. The field is renamed holders -> users to reflect that it counts waiters too. Also: - log the (rare) I/O failure in readSubProcessCursor at FINE level - document in checkSubProcesses that the append and the log reader both take subProcessLogLock, which is what makes the high-water cursor invariant hold - rewrite the concurrency regression test to drive the full happy path (a real MonitoringService via a plugin test hook) and to use latches instead of timing sleeps so the eviction race is created deterministically Co-Authored-By: Claude <noreply@anthropic.com>
…e the concurrency test a real seam Two substantive issues from the latest review: 1. The report cursor's high-water invariant was not actually guaranteed. The scan's log-line timestamp was captured *before* the monitor took subProcessLogLock, so a line appended after a report's read could carry a timestamp <= the report's now and land behind the cursor. Capture the timestamp *inside* subProcessLogLock (in the same critical section that writes the line), and rewrite the Javadoc to state the two conditions that must hold together (timestamp under the lock + append/read on the same lock). Split appendSubProcessLog into a production entry point that captures the time under the lock and a Locked core that the test calls with a pinned timestamp. 2. The concurrency regression test did not actually create the eviction race: the in-lock hook cannot observe a thread that has acquired the registry entry but is still waiting on the monitor, and the counter it used reset to zero between reports so B always re-entered with cur==1. Add a beforeEnter seam (a Consumer<Object> called between acquireSubProcessReportLock and the monitor) so the test can (a) count distinct lock objects per profile and (b) order a waiting thread. The test now deterministically stages A-holds / B-waiting / A-releases / C-arrives, asserts C saw the same lock object as B, and uses bounded waits throughout. Verified the test FAILS against the old holders-only registry (distinct objects == 2) and passes against the waiter-counting one. Also: - Move the plugin test seam off the global ServerMonitorPlugin singleton onto a per-instance field on SupportServlet (setServerMonitorPluginForTest), so the test no longer mutates process-wide state. - Distinguish malformed JSON (concise FINE, no stack trace) from a genuine I/O/security failure (FINE with stack trace) in readSubProcessCursor. Co-Authored-By: Claude <noreply@anthropic.com>
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
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
The "Slow Sub-Processes (during profile window)" section of the profile summary
previously printed one line per scan, so a process alive across many scan ticks was
repeated dozens of times, a process that finished between two scans was missed
entirely, and a process that outlived the profile window was not attributable to that
profile at all.
This PR makes the section per-pid and cumulative:
Deduplicate per pid across scans. Each entry now carries first/last observation
time plus an estimated lifetime (observed interval + age delta) instead of a single
snapshot age.
Cumulative report cursor. A high-water mark (
subProcessLogCursor) is persistedin the profile's
.jsonmetadata sidecar. The first report covers the profile's timewindow; each subsequent report shows only what was observed since the previous
report, so long-running external processes are still attributed to the profile that
triggered them. The cursor survives server restarts (worst case: a few lines shown
twice, harmless because entries are deduplicated per pid).
In-memory observation registry.
MonitoringServicenow tracks each pid'sfirst/last-seen and max age across scans (pruned after 2 consecutive missed scans, so
one transient
/procread failure does not drop a process). The report merges thislive view with the persistent
subprocesses.jsonllog, covering processes stillobservable right now — including ones that started after the profile window ended.
Example new report line
Files
MonitoringService.java—SubProcessObservationregistry,updateSubProcessObservations(),getObservedSubProcesses()SubProcessMonitor.java— first/last-seen + age fields,estimatedLifetimeSec()SupportServlet.java—appendSubProcessSummary()rewrite (dedup + cursor + memory merge),persistSubProcessCursor(),SubProcessEntryVerification
mvn -pl src compile— cleanant -f src/build.xml compile— BUILD SUCCESSFUL (the unchecked-operations note onSupportServletis pre-existing)TestSubProcessLog— OK (8 tests)TestAsyncProfilerWrapper— OK (5 tests)Single commit on top of
main(replaces the superseded #32, which was mistakenly based on a gxp_dev-side cherry-pick branch and therefore carried 50 unrelated commits).Co-Authored-By: Claude Code noreply@anthropic.com