I am in the process of looking to test this locally -- I am brand new to the project so it may take a minute. Feedback is welcome!

So what was the result of the testing?

@CommanderStorm I'm doing this as a side-side-quest for my day job and have never touched this repo before so it's going to take a little to build, push, and use the docker container in our existing setup amongst my other priorities.

Hopefully a maintainer or others can weigh in on this PR in the interim.

Just read #275 again. I have the same concern as @steveiliop56 said. #275 (comment)

I have also checked the Node.js Doc. https://nodejs.org/api/http.html#new-agentoptions

agent.destroy()
It is usually not necessary to do this. However, if using an agent with keepAlive enabled, then it is best to explicitly shut down the agent when it is no longer needed.

keepAlive Default: false.

1. So we have never enabled keepAlive, why is destroy() still needed?
2. I think you have to prove that maybe using a minimal javascript script to compare both with/without reusing the http agent object, not (I think!).

And actually I am not so faimilar with http.Agent too. Recreating http.Agent looks more stateless to me I think. While reusing the same http.Agent, I have no idea if it would have any state changes inside the agent, and affect upcoming requests.

And since you mentioned Claude AI, Did the idea of reusing agent and agent.destroy() purely come up with Caude, not by your test?

If yes, I think we better stop here.

@louislam

So we have never enabled keepAlive, why is destroy() still needed?

Great catch. I was taking a belt & suspenders approach. That one's on me, I hadn't read the docs thoroughly enough. I was coming with a .NET mindset of disposing any IDisposable.

But from those same docs, right after you quoted:

Otherwise, sockets might stay open for quite a long time before the server terminates them.

So I'm thinking there could be something where enough sockets are kept open longer than they should be, causing an issue. This is a strange situation in that I'm attempting to resolve a performance-related symptom where it's difficult to diagnose the problem, so I'm looking for a non-harmful practice that could also possibly fix things, and this stuck out.

To be clear, I'm also shifting my thinking to agree that this is less about memory (and perhaps less about sockets) and more about pressure from object accumulation over a long time. I'm not seeing memory pressure on our container at all.

My new theory is that enough of these are being created that we see socket exhaustion faster than they're cleaned up, or that GC pauses due to object accumulation get longer. And that since it's efficient and fine to re-use them, nothing seems to be hurt by this change.

I did work with a colleague to create a small test harness (which didn't make sense to include in the larger codebase I don't think, though let me know if you want it). It just shows that we definitely don't accumulate things anymore with this PR. But I think that much is obvious from the code change so it probably doesn't make sense to include.

If you'd like, I can work to deploy this PR into our production environment and let it run for some time. That might be the ultimate proof -- putting it in a production scenario and seeing if the symptom is resolved.

I've deployed this to an experimental container under my dockerhub and we are now using it in our pre-prod and prod environments. I'll report back on findings. This can stay open in the interim.

Based on past experience it takes about 18 hours for the symptom to show up. I'll let this run for a few days and if symptoms don't show up I think we'll have a strong signal on this

Observations from production so far:

• We've received far less of the 48000ms timeout errors, but the ones we did receive took an interesting format. Rather than a continuous rolling issue as we previously saw, a one-time issue happened at 12:41am where 12 monitors had this issue at once, and then recovered 1 minute later. I didn't restart anything yet; looking to see if it happens again or if maybe that was a one-off fluke.
  • The end-user experience is definitely preferable to the current continual rolling false-positives.
• I've noticed a side effect of my change where in one instance, an agent that was being re-used "went bad" (looking to further diagnose). This agent was monitoring a web site in front of Azure app gateway. It received a 403 and was marked as down. The 403 is surprising which makes me think something about how it initially established the http connection with the Azure app Gateway went stale (possibly the keepAlive = false). When I paused the monitor and then resumed it, the problem cleared (presumably because the http agent was recreated). So I'll want to look into that as part of this PR, even if we proceed.

Additional observations from production:

• No other 48000ms timeout errors except for that one small batch that resolved itself yesterday
• No other issues with monitors "going back" or being unable to recover

So I do think we have a strong signal that this is, at a minimum, helping with the symptom. Going to keep monitoring / reporting for a few more days.

I've still not run into any more 48000ms timeout errors. I'm thinking this PR might be a fix for the majority of the symptoms. The lack of rolling false positives has been delightful.

I just pushed a change to destroy the HTTP agent whenever the monitor fails. This should alleviate the case in which a "stuck" agent causes an issue (such as my long-running agent causing an issue with the one web site that's behind an Azure app Gateway and "went bad" after a few days.)

I'll let that change marinate for few more days in our production environment before asking for another review here.

Could you run another new Uptime Kuma instance that without this patch for side-by-side comparison?

Since you mentioned socket exhaustion, did you see this by using command like netstat?

I will try to invite other people to test it.

If it is eventually true, I think we should transfer the issue to Node.js' repo too, as their doc has never mentioned this behavior.

Could you run another new Uptime Kuma instance that without this patch for side-by-side comparison?

You mean so that we can see one instance start failing while the other doesn't? I could look into that but there's a cost associated in our case. Not a bad idea though 👍 Coming up on a holiday in the US but I will look into that right after.

Previously very few days we would begin seeing 48000ms timeouts causing monitors to trip, and it would continue to increase in frequency until I restarted the container in Azure. Haven't seen that happening with this deployment.

Since you mentioned socket exhaustion, did you see this by using command like netstat?

This was a best guess around x number of HTTP agents being created. My thinking was that socket contention could be causing the issue, but I have no proof of that. All I have is a fix for the symptoms at this point, not certainty about the cause, which is unfortunate.

If it is eventually true, I think we should transfer the issue to Node.js' repo too, as their doc has never mentioned this behavior.

Not a bad idea, though I'm well out of my depth at that point.

My fix to destroy agents on failure didn't resolve the issue this PR introduced/uncovered around monitors failing and staying failed in certain situations (specifically ones that check a site that's behind an Azure app gateway). Still seeing that in production. I'll have to look into that a bit more, will likely revert that commit and try again.

Still resolves the 48000ms timeout errors so far though! Haven't seen them reappear.