Skip to content

Commit 08158e0

Browse files
BridgeARBridgeAR
committed
fix(test-optimization): release removed replay probes (#9336)
Removed probes stayed indexed by source location, causing later failures to reuse probe IDs that no longer existed. Bulk cleanup also split Windows drive paths at the volume separator.
1 parent 8debc3b commit 08158e0

8 files changed

Lines changed: 476 additions & 35 deletions

File tree

‎integration-tests/ci-visibility/dynamic-instrumentation/hold-probe-removal.js‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
'use strict'
2+
3+
const { once } = require('node:events')
4+
const { join } = require('node:path')
5+
const { isMainThread } = require('node:worker_threads')
6+
7+
const NativeMessageChannel = globalThis.MessageChannel
8+
const dynamicInstrumentationPath = join('ci-visibility', 'dynamic-instrumentation', 'index.js')
9+
10+
let firstProbeRemovalAcknowledged
11+
let heldProbeId
12+
let heldProbeRemovalReleased = false
13+
let postProbeRemoval
14+
let probeSetAfterRelease = false
15+
16+
if (isMainThread) {
17+
globalThis.MessageChannel = class extends NativeMessageChannel {
18+
constructor () {
19+
super()
20+
21+
if (!new Error().stack?.includes(dynamicInstrumentationPath)) return
22+
23+
const postMessage = this.port2.postMessage.bind(this.port2)
24+
25+
/**
26+
* @param {object|string} message
27+
*/
28+
this.port2.postMessage = (message) => {
29+
if (heldProbeId === undefined && typeof message === 'string') {
30+
heldProbeId = message
31+
postProbeRemoval = postMessage
32+
firstProbeRemovalAcknowledged = once(this.port2, 'message')
33+
} else {
34+
if (heldProbeRemovalReleased && typeof message !== 'string' && message.file && message.line) {
35+
probeSetAfterRelease = true
36+
}
37+
postMessage(message)
38+
}
39+
}
40+
}
41+
}
42+
}
43+
44+
function releaseHeldProbeRemoval () {
45+
if (heldProbeId === undefined) {
46+
throw new Error('Dynamic Instrumentation probe removal was not held')
47+
}
48+
heldProbeRemovalReleased = true
49+
postProbeRemoval(heldProbeId)
50+
}
51+
52+
function assertNoProbeSetAfterRelease () {
53+
if (probeSetAfterRelease) {
54+
throw new Error('Dynamic Instrumentation set a canceled probe')
55+
}
56+
}
57+
58+
function waitForFirstProbeRemoval () {
59+
if (!firstProbeRemovalAcknowledged) {
60+
throw new Error('Dynamic Instrumentation removal channel was not created')
61+
}
62+
return firstProbeRemovalAcknowledged
63+
}
64+
65+
module.exports = {
66+
assertNoProbeSetAfterRelease,
67+
releaseHeldProbeRemoval,
68+
waitForFirstProbeRemoval,
69+
}

‎integration-tests/ci-visibility/dynamic-instrumentation/test-cancel-pending-probe.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
5+
const sum = require('./dependency')
6+
const {
7+
assertNoProbeSetAfterRelease,
8+
releaseHeldProbeRemoval,
9+
waitForFirstProbeRemoval,
10+
} = require('./hold-probe-removal')
11+
12+
describe('dynamic-instrumentation', () => {
13+
it('exhausts the first retry', function () {
14+
assert.strictEqual(sum(11, 3), 14)
15+
})
16+
17+
it('exhausts a later retry from the same location', function () {
18+
assert.strictEqual(sum(11, 3), 14)
19+
})
20+
21+
it('does not reinstall the canceled probe', async function () {
22+
this.timeout(15_000)
23+
releaseHeldProbeRemoval()
24+
await waitForFirstProbeRemoval()
25+
assertNoProbeSetAfterRelease()
26+
})
27+
})

‎integration-tests/ci-visibility/dynamic-instrumentation/test-reinstall-probe.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
5+
const sum = require('./dependency')
6+
7+
let secondTestAttempt = 0
8+
9+
describe('dynamic-instrumentation', () => {
10+
it('exhausts retries for the first failure with DI', function () {
11+
assert.strictEqual(sum(11, 3), 14)
12+
})
13+
14+
it('retries a later failure from the same location with DI', function () {
15+
const input = secondTestAttempt++ < 2 ? 11 : 1
16+
assert.strictEqual(sum(input, 3), input + 3)
17+
})
18+
})

‎integration-tests/mocha/mocha.spec.js‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4541,6 +4541,129 @@ describe(`mocha@${MOCHA_VERSION}`, function () {
45414541
})
45424542
})
45434543

4544+
onlyLatestIt('reinstalls a probe for a later failure from the same location', async () => {
4545+
receiver.setSettings({
4546+
flaky_test_retries_enabled: true,
4547+
di_enabled: true,
4548+
})
4549+
4550+
const testNamesWithDebugInfo = new Set()
4551+
let testOutput = ''
4552+
4553+
const eventsPromise = receiver
4554+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
4555+
const events = payloads.flatMap(({ payload }) => payload.events)
4556+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
4557+
const retriedTests = tests.filter(
4558+
test => test.meta[TEST_RETRY_REASON] === TEST_RETRY_REASON_TYPES.atr
4559+
)
4560+
4561+
assert.strictEqual(retriedTests.length, 4)
4562+
for (const retriedTest of retriedTests) {
4563+
if (retriedTest.meta[DI_ERROR_DEBUG_INFO_CAPTURED] === 'true') {
4564+
testNamesWithDebugInfo.add(retriedTest.meta[TEST_NAME])
4565+
}
4566+
}
4567+
assert.strictEqual(testNamesWithDebugInfo.size, 2)
4568+
assert.ok(testNamesWithDebugInfo.has(
4569+
'dynamic-instrumentation exhausts retries for the first failure with DI'
4570+
))
4571+
assert.ok(testNamesWithDebugInfo.has(
4572+
'dynamic-instrumentation retries a later failure from the same location with DI'
4573+
))
4574+
})
4575+
4576+
childProcess = exec(
4577+
'node ./ci-visibility/run-mocha.js',
4578+
{
4579+
cwd,
4580+
env: {
4581+
...getCiVisAgentlessConfig(receiver.port),
4582+
TESTS_TO_RUN: JSON.stringify([
4583+
'./dynamic-instrumentation/test-reinstall-probe',
4584+
]),
4585+
DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '2',
4586+
_DD_TRACE_INTEGRATION_COVERAGE_DISABLE: '1',
4587+
},
4588+
}
4589+
)
4590+
4591+
childProcess.stdout?.on('data', (chunk) => {
4592+
testOutput += chunk.toString()
4593+
})
4594+
childProcess.stderr?.on('data', (chunk) => {
4595+
testOutput += chunk.toString()
4596+
})
4597+
const stdoutEndPromise = childProcess.stdout ? once(childProcess.stdout, 'end') : Promise.resolve()
4598+
const stderrEndPromise = childProcess.stderr ? once(childProcess.stderr, 'end') : Promise.resolve()
4599+
4600+
const [[exitCode]] = await Promise.all([
4601+
once(childProcess, 'exit'),
4602+
eventsPromise,
4603+
stdoutEndPromise,
4604+
stderrEndPromise,
4605+
])
4606+
assert.strictEqual(exitCode, 0, testOutput)
4607+
})
4608+
4609+
onlyLatestIt('cancels a queued probe while a prior removal is pending', async () => {
4610+
receiver.setSettings({
4611+
flaky_test_retries_enabled: true,
4612+
di_enabled: true,
4613+
})
4614+
4615+
let retriedTests = []
4616+
let testOutput = ''
4617+
4618+
const eventsPromise = receiver
4619+
.gatherPayloadsMaxTimeout(({ url }) => url.endsWith('/api/v2/citestcycle'), (payloads) => {
4620+
const events = payloads.flatMap(({ payload }) => payload.events)
4621+
const tests = events.filter(event => event.type === 'test').map(event => event.content)
4622+
retriedTests = tests.filter(
4623+
test => test.meta[TEST_RETRY_REASON] === TEST_RETRY_REASON_TYPES.atr
4624+
)
4625+
})
4626+
4627+
childProcess = exec(
4628+
runTestsCommand,
4629+
{
4630+
cwd,
4631+
env: {
4632+
...getCiVisAgentlessConfig(receiver.port),
4633+
NODE_OPTIONS: '-r ./ci-visibility/dynamic-instrumentation/hold-probe-removal -r dd-trace/ci/init',
4634+
TESTS_TO_RUN: JSON.stringify([
4635+
'./dynamic-instrumentation/test-cancel-pending-probe',
4636+
]),
4637+
DD_CIVISIBILITY_FLAKY_RETRY_COUNT: '1',
4638+
DD_TRACE_DEBUG: 'true',
4639+
_DD_TRACE_INTEGRATION_COVERAGE_DISABLE: '1',
4640+
},
4641+
}
4642+
)
4643+
4644+
childProcess.stdout?.on('data', (chunk) => {
4645+
testOutput += chunk.toString()
4646+
})
4647+
childProcess.stderr?.on('data', (chunk) => {
4648+
testOutput += chunk.toString()
4649+
})
4650+
const stdoutEndPromise = childProcess.stdout ? once(childProcess.stdout, 'end') : Promise.resolve()
4651+
const stderrEndPromise = childProcess.stderr ? once(childProcess.stderr, 'end') : Promise.resolve()
4652+
4653+
const [[exitCode]] = await Promise.all([
4654+
once(childProcess, 'exit'),
4655+
eventsPromise,
4656+
stdoutEndPromise,
4657+
stderrEndPromise,
4658+
])
4659+
assert.strictEqual(exitCode, 0, testOutput)
4660+
assert.strictEqual(testOutput.includes('Unknown probe id'), false)
4661+
assert.strictEqual(retriedTests.length, 2)
4662+
for (const retriedTest of retriedTests) {
4663+
assert.strictEqual(retriedTest.meta[TEST_STATUS], 'fail')
4664+
}
4665+
})
4666+
45444667
onlyLatestIt('drains in-flight dynamic instrumentation hits before the next retry', async () => {
45454668
receiver.setSettings({
45464669
flaky_test_retries_enabled: true,

0 commit comments

Comments
 (0)