Skip to content

Add test for TSHttpTxnServerAddrSet retry behavior (issue #12611)#12727

Closed
bryancall wants to merge 3 commits intoapache:masterfrom
bryancall:test-12611-TSHttpTxnServerAddrSet-retry
Closed

Add test for TSHttpTxnServerAddrSet retry behavior (issue #12611)#12727
bryancall wants to merge 3 commits intoapache:masterfrom
bryancall:test-12611-TSHttpTxnServerAddrSet-retry

Conversation

@bryancall
Copy link
Copy Markdown
Contributor

@bryancall bryancall commented Dec 3, 2025

This PR adds a test that demonstrates issue #12611: TSHttpTxnServerAddrSet() retry fails because the OS_DNS hook is not called again when connection fails.

Test Description

The test plugin (test_TSHttpTxnServerAddrSet_retry):

  • Hooks into TS_HTTP_OS_DNS_HOOK
  • On first call: Sets a non-routable address (192.0.2.1:80) that will fail to connect
  • On subsequent calls: Sets localhost (127.0.0.1:8080) that should work
  • Tracks and logs how many times the OS_DNS hook was called

Expected Results

On master (bug exists):

  • OS_DNS hook is called only ONCE
  • Connection fails with the bad address
  • Plugin never gets a chance to provide an alternative address
  • Test logs: BUG CONFIRMED: OS_DNS hook was only called ONCE

After fix is applied:

  • OS_DNS hook should be called multiple times on retry
  • Plugin can provide a different address
  • Test logs: SUCCESS: OS_DNS hook was called X times

Test Output on Master (Confirms Bug)

[TSHttpTxnServerAddrSet_retry] OS_DNS hook called, count=1
[TSHttpTxnServerAddrSet_retry] Attempt 1: Setting BAD address 192.0.2.1:80 (will fail)
[TSHttpTxnServerAddrSet_retry] Transaction closing. OS_DNS was called 1 time(s)
[TSHttpTxnServerAddrSet_retry] *** BUG CONFIRMED: OS_DNS hook was only called ONCE. ***

This test is a precursor to the fix PR for #12611.

@bryancall bryancall added the Bug label Dec 3, 2025
@bryancall bryancall self-assigned this Dec 3, 2025
@bryancall bryancall added this to the 10.2.0 milestone Dec 3, 2025
@bryancall bryancall marked this pull request as draft December 3, 2025 19:46
@bneradt
Copy link
Copy Markdown
Contributor

bneradt commented Dec 3, 2025

[approve ci debian fedora rocky ubuntu]

@bryancall bryancall requested a review from Copilot December 4, 2025 02:49
Copilot AI reviewed Dec 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a test case to demonstrate issue #12611, where the TS_HTTP_OS_DNS_HOOK is not invoked again when a connection retry occurs after using TSHttpTxnServerAddrSet(). The test includes a C++ plugin that sets different addresses on sequential OS_DNS hook calls and a Python test script that verifies the bug's existence by checking that the hook is only called once.

Key changes:

  • New C++ test plugin that tracks OS_DNS hook invocations and sets non-routable vs. routable addresses
  • Python test script that configures ATS with connection retry settings and validates log output
  • Build system update to compile the new test plugin

Reviewed changes

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

File Description
test_TSHttpTxnServerAddrSet_retry.test.py Python test framework that configures ATS with retry settings and validates that OS_DNS hook is only called once, confirming the bug
test_TSHttpTxnServerAddrSet_retry.cc C++ plugin that attempts to set different addresses on OS_DNS hook calls and reports how many times the hook was invoked
CMakeLists.txt Adds the new test plugin to the build system

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

Comment thread tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc Outdated
Comment thread tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc Outdated
Comment thread tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc
Comment thread tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc
Comment thread tests/gold_tests/pluginTest/tsapi/test_TSHttpTxnServerAddrSet_retry.cc Outdated
@bryancall
Add test for TSHttpTxnServerAddrSet retry behavior (issue apache#12611)
b593d32 
This test demonstrates that TSHttpTxnServerAddrSet() retry fails because the
OS_DNS hook is not called again when connection fails.

The test plugin:
- Sets a bad address (192.0.2.1) on first OS_DNS call - will fail to connect
- Should set a good address (127.0.0.1) on retry if OS_DNS is called again
- Uses per-transaction storage (TSUserArg) to track call count correctly
- Logs whether OS_DNS was called once (bug) or multiple times (correct)

Expected result on master (bug exists):
- OS_DNS hook called only ONCE
- Connection fails with bad address
- Plugin never gets chance to provide alternative address
- Test detects and reports: BUG CONFIRMED

This test should FAIL/detect bug on master branch.
Once the fix is applied, the test should PASS (OS_DNS called multiple times).
@bryancall bryancall force-pushed the test-12611-TSHttpTxnServerAddrSet-retry branch from 1f86ec7 to b593d32 Compare December 4, 2025 03:35
@bryancall bryancall requested a review from Copilot December 4, 2025 04:25
Copilot AI reviewed Dec 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

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

@bryancall
Fix test to check for SUCCESS instead of BUG CONFIRMED
e996ef2 
The test should:
- FAIL on master (bug exists, 'SUCCESS' message not present)
- PASS after fix is applied (OS_DNS called multiple times, 'SUCCESS' logged)

This makes it a proper regression test that will catch if the bug
is reintroduced.
@bryancall bryancall requested a review from Copilot December 4, 2025 05:26
Copilot AI reviewed Dec 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

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

@bryancall
Copy link
Copy Markdown
Contributor Author

bryancall commented Dec 4, 2025

[approve ci]

@bryancall
Override default error check in test to allow TSError() output
85584d2 
Use '=' instead of '+=' for the first diags.log check to override
the default 'should not contain errors' check. This allows the test
plugin to use TSError() for diagnostic output that we verify.
@bryancall bryancall force-pushed the test-12611-TSHttpTxnServerAddrSet-retry branch from b918a13 to 85584d2 Compare December 4, 2025 19:22
@bryancall bryancall mentioned this pull request Dec 4, 2025
Merged
@bryancall
Copy link
Copy Markdown
Contributor Author

bryancall commented Dec 4, 2025

[approve ci]

@bryancall bryancall removed this from the 10.2.0 milestone Dec 5, 2025
@bryancall
Copy link
Copy Markdown
Contributor Author

bryancall commented Dec 5, 2025

This PR was just for testing and seeing if the autest would fail correctly in CI. It was helpful in finding an issue with CI and not correctly failing. The new PR that includes the fix and this test is: #12733

@bryancall bryancall closed this Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

@bryancall bryancall

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bryancall @bneradt