Repository navigation
Conversation
A begin whose endTaskBlock was never reached left the TLS token set, so every later begin on the thread failed, and its TASK_BLOCK run kept suppressing wall-clock signals. A new begin now supersedes the pending interval: it clears the token and closes the run with the generation-checked exitBlockedRun(), counting TASK_BLOCK_RECOVERED_ABANDONED. Document the finally requirement and the supersede policy on beginTaskBlock.
isPlatformThread() returned true on non-HotSpot JVMs reporting JNI 19+ without probing, so virtual threads could enter task-block producer state. The hardcoded IsVirtualThread table offset is only trusted on HotSpot, so other VMs now report every thread as non-platform, which keeps task-block hooks disabled there. Skip the arming TaskBlock tests on those JVMs and add a test asserting begin returns 0 there.
computeMinTaskBlockTicks() yields 0 when TSC::frequency() is below 1 kHz, and a zero threshold let every block pass the 1 ms minimum. exceedsMinTaskBlockDuration() now rejects in that case, which the caller counts as TASK_BLOCK_SKIPPED_TOO_SHORT. Add test-only hooks to reset the cached threshold and replace the frequency source, and cover the lazy recompute path and the fail-closed path.
Add TaskBlockAssertions.assertContainsDuration() and bound the encoded duration in the paired-API and lightweight TaskBlock tests.
Add a test-only observer for the event recordUnownedBlockedTail() writes and a gtest that accumulates suppressed weight before the tail flush and checks the emitted weight matches it.
blockEnter0 returned 0 for traced threads without any counter, unlike beginTaskBlock0. Increment BLOCK_ENTER_SKIPPED_TRACE_CONTEXT on that path and test it.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
Abandoned TaskBlock recovery retains generation checks, and invalid duration thresholds fail closed. The completed static review reported no actionable correctness regression; runtime behavior was not exercised.
🤖 Bits Code Review · Commit b626adf
Contributor
CI Test ResultsRun: #40555987154 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-27 18:30:31 UTC |
Contributor
This branch has not been deployed
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.
What does this PR do?:
Implements the 8 findings from an agentic code review of #664 (TaskBlock event recording core), as a stacked fix branch so the fixes can be accepted directly instead of being re-derived from review comments. One commit per finding group, plus the review's second-round fixes.
Motivation:
The review of #664 surfaced one functional hazard and several contract/test gaps in the new TaskBlock recording path:
beginTaskBlock()without a matchingendTaskBlock()(e.g. an exception withoutfinally) stranded the thread-local token forever: every laterbeginTaskBlock()failed for the life of the thread, and the orphanedTASK_BLOCK-owned tracker run suppressed wall-clock signals for that thread indefinitely. A generation-checked recovery now supersedes an abandoned interval at the nextbeginTaskBlock()(TASK_BLOCK_RECOVERED_ABANDONED).JVMSupport::isPlatformThread()assumed any non-HotSpot JVM at JNI ≥ 19 has no virtual threads, admitting virtual threads into carrier-only state. It now fails closed: non-HotSpot JVMs classify no thread as a platform thread until each VM's JNI table is verified.task_block_threshold_invalidcounter instead of masquerading as "too short".blockEnter0silently disarmed owned-block suppression for traced threads; it now incrementsblock_enter_skipped_trace_context, symmetric withbeginTaskBlock0.durationencoding, the deferred-tailweight, and the duration-threshold fallback are now asserted; stale comments fixed.Additional Notes:
srs_ADHSJBhREPCPpU9ld3ea6fpV,srs_6btCf90wtiQrlxxlT8hiYKbF).ProfilerOwnedBlockHooks.blockEnterruns until each VM is verified. The J9/Zing-specific arming tests are skipped there accordingly.javaApi.cpp): the caller-visible contract ("a0return keeps the open interval") takes precedence, since native code cannot distinguish a nested live interval from an abandoned one.How to test the change?:
New coverage:
AbandonedTaskBlockTestand threshold tests intaskBlockRecorder_ut.cpp, the no-observer tail-write and non-decodable-token cases inwallTailRecording_ut.cpp,JavaProfilerTaskBlockNonHotspotTest, duration/weight assertions inJavaProfilerTaskBlockApiTest/JavaProfilerTaskBlockLightweightTest, andtraceContextRejectionKeepsOpenIntervalinJavaProfilerTaskBlockApiTest../.claude/commands/build-and-summarize :ddprof-lib:gtestDebug./.claude/commands/build-and-summarize :ddprof-test:testDebug -Ptests=JavaProfilerTaskBlockApiTest./.claude/commands/build-and-summarize :ddprof-test:testDebug -Ptests=UnfilteredWallPrecheckTestAll of the above pass on JDK 21 HotSpot, plus the full
gtestDebugsuite andspotlessCheck.For Datadog employees:
dd:platform-security-reviewskill, or file a request via the PSEC review form).bewairealso runs automatically on every PR.