Should I refactor the code that is marked as “don't change”?

Yes, you should refactor the code before you add the other features.

The trouble with comments like these is that they depend on particular circumstances of the environment in which the code base is running. The timeout programmed at a specific point may have been truly necessary when it was programmed.

But there are any number of things that might change this equation: hardware changes, OS changes, unrelated changes in another system component, changes in typical data volumes, you name it. There is no guarantee that in the present day it is still necessary, or that it is still sufficient (the integrity of the thing it was supposed to protect might have been broken for a long time - without proper regression tests you might never notice). This is why programming a fixed delay in order to allow another component to terminate is almost always incorrect and works only accidentally.

In your case, you don't know the original circumstances, and neither can you ask the original authors. (Presumably you also don't have proper regression/integration testing, or you could simply go ahead and let your tests tell you whether you broke something.)

This might look like an argument for not changing anything out of caution; but you say there will have to be major changes anyway, so the danger of upsetting the delicate balance that used to be achieved in this spot is already there. It is much better to upset the apple cart now, when the only thing you're doing is the refactoring, and be certain that if things break it was the refactoring that caused it, than to wait until you are making additional changes simultaneously and never be sure.

My question is: should I refactor the code when I encounter such warnings from the authors

No, or at least not yet. You imply that the level of automated testing is very low. You need tests before you can refactor with confidence.

for now we are no longer able to add any feature without breaking something else

Right now you need to focus on increasing stability, not refactoring. You might do refactoring as part of stability increase, but it is a tool to achieve your real goal - a stable codebase.

It sounds like this has become a legacy codebase, so you need to treat it a little differently.

Start by adding characterization tests. Don't worry about any spec, just add a test that asserts on the current behavior. This will help prevent new work from breaking existing features.

Every time you fix a bug, add a test case that proves the bug is fixed. This prevents direct regressions.

When you add a new feature, add at least some basic testing that the new feature works as intended.

Perhaps get a copy of "Working Effectively with Legacy Code"?

I was given a few months to refactor existing code.

Start by getting test coverage up. Start with areas that break the most. Start with areas that change the most. Then once you have identified bad designs, replace them one by one.

You almost never one to do one huge refactor, but instead a constant flow of small refactors every week. One big refactor has a huge risk of breaking things, and it's hard to test well.

Remember the G. K. Chesterton's fence: do not take down a fence obstructing a road until you understand why it was built.

You can find the author(s) of the code and the comments in question, and consult them to obtain the understanding. You can look at the commit messages, email threads, or docs if they exist. Then you'll either be able to refactor the marked fragment, or will write down your knowledge in the comments so that the next person to maintain this code could make a more informed decision.

Before that, I'd not touch the code which is marked "do not touch".

Probably not a good idea to refactor code with such warnings, if things work OK as they are.

But if you must refactor...

First, write some unit tests and integration tests to test the conditions that the warnings are warning you about. Try to mimic production-like conditions as much as possible. This means mirroring the production database to a test server, running the same other services on the machine, etc... whatever you can do. Then attempt your refactoring (on an isolated branch, of course). Then run your tests on your new code to see if you can get the same results as with the original. If you can, then it is probably OK to implement your refactoring in production. But be ready to roll back the changes if things go wrong.

Code with comments like the ones you showed would be top on my list of things to refactor if, and only if I have any reason to do so. The point is, that the code stinks so badly, you even smell it through the comments. It's pointless to try to frickle any new functionality into this code, such code needs to die as soon as it needs to change.

Also note that the comments are not helpful in the least: They only give warning and no reason. Yes, they are the warning signs around a tank of sharks. But if you want to do anything within the vicinity, there is little points to try to swim with the sharks. Imho, you should get rid of those sharks first.

That said, you absolutely need good test cases before you can dare to work on such code. Once you have those test cases, be sure to understand every tiny little bit you are changing, to ensure that you are really not changing behavior. Make it your top priority to keep all the behavioral peculiarities of the code until you can prove that they are without any effect. Remember, you are dealing with sharks - you must be very careful.

So, in the case of the time out: Leave it in until you understand precisely what the code is waiting for, and then fix the reason why the time out was introduced before you proceed to remove it.

Also, be sure that your boss understands what an endeavor it is that you are embarking on, and why it is needed. And if they say no, don't do it. You absolutely need their support in this.

I say go ahead and change it. Believe it or not, not every coder is a genius and a warning like this means it could be a spot for improvement. If you find that the author was right, you could (gasp) DOCUMENT or EXPLAIN the reason for the warning.

Yes, absolutely. These are clear indications that the person who wrote this code was unsatisfied with the code and likely poked at it until they got it to happen to work. It's possible they didn't understand what the real issues were or, worse, did understand them and were too lazy to fix them.

However, it's a warning that a lot of effort will be needed to fix them and that such fixes will have risks associated with them.

Ideally, you'll be able to figure out what the issue was and fix it properly. For example:

Time out set to give Module A some time to do stuff. If its not timed like this, it will break.

This strongly suggests that Module A doesn't properly indicate when it's ready to be used or when it has finished processing. Perhaps the person who wrote this didn't want to bother fixing Module A or couldn't for some reason. This looks like a disaster waiting to happen because it suggests a timing dependency being dealt with by luck rather than proper sequencing. If I saw this, I would very much want to fix it.

Do not change this. Trust me, you will break things.

This doesn't tell you much. It would depend on what the code was doing. It could mean that it has what appear to be obvious optimizations that, for one reason or another, will actually break the code. For example, a loop might happen to leave a variable at a particular value which another piece of code depends on. Or a variable might be tested in another thread and changing the order of variable updates might break that other code.

I know using setTimeout is not a good practice, but in this case I had to use it.

This looks like an easy one. You should be able to see what setTimeout is doing and perhaps find a better way to do it.

That said, if these kinds of fixes are outside the scope of your refactor, these are indications that trying to refactor inside this code may significantly increase the scope of your effort.

At a minimum, look closely at the code affected and see if you can at least improve the comment to the point that it more clearly explains what the issue is. That might save the next person from facing the same mystery you face.

I am expanding my comments into an answer because I think some aspects of the specific problem are either being overlooked, or used to draw the wrong conclusions.

At this point, the question of whether to refactor is premature (even though it will probably be answered by a specific form of 'yes').

The central issue here is that (as noted in some answers) the comments you quote strongly indicate that the code has race conditions or other concurrency/synchronization problems, such as discussed here. These are particularly difficult problems, for several reasons. Firstly, as you have found, seemingly unrelated changes can trigger the problem (other bugs can also have this effect, but concurrency errors almost always do.) Secondly, they are very hard to diagnose: the bug often manifests itself in a place that is distant in time or code from the cause, and anything you do to diagnose it may cause it to go away (Heisenbugs). Thirdly, concurrency bugs are very hard to find in testing. Partly, that is because of the combinatorial explosion: it is bad enough for sequential code, but adding the possible interleavings of concurrent execution blows it up to the point where the sequential problem becomes insignificant in comparison. In addition, even a good test case may only trigger the problem occasionally - Nancy Leveson calculated that one of the lethal bugs in the Therac 25 occurred in 1 of about 350 runs, but if you do not know what the bug is, or even that there is one, you do not know how many repetitions are effective. In addition, only automated testing is feasible at this scale, and it is possible that the test driver imposes subtle timing constraints such that it will never actually trigger the bug (Heisenbugs again).

There are some tools for concurrency testing in some environments, such as Helgrind for code using POSIX pthreads, but we do not know the specifics here. Testing should be supplemented with static analysis (or is that the other way round?), if there are suitable tools for your environment.

To add to the difficulty, compilers (and even the processors, at runtime) are often free to reorganize the code in ways that sometimes make reasoning about its thread-safety very counter-intuitive (perhaps the best-known case is the double-checked lock idiom, though some environments (Java, C++...) have been modified to ameliorate it.)

This code may have one simple problem that is causing all the symptoms, but it is more likely that you have a systemic problem that could bring your plans to add new features to a halt. I hope I have convinced you that you might have a serious problem on your hands, possibly even an existential threat to your product, and the first thing to do is to find out what is happening. If this does reveal concurrency problems, I strongly advise you to fix them first, before you even ask the question of whether you should do more general refactoring, and before you try to add more features.