Commit shaper state only when tc succeeds

[ooonea]

First, an apology for #399 and #400: those were opened by my agent tooling running unattended overnight — it was never supposed to publish anything, and the size of what it produced speaks for itself. I've corrected that on my side. The two bugs underneath are real though, so here they are as minimal, hand-checked fixes.

This one: set_shaper_rate updates last_shaper_rate_kbps and the delay compensations even when tc qdisc change fails (interface briefly gone, qdisc replaced). From that point the controller believes the new rate is installed while the kernel still runs the old one — and since the internal state advanced, the change is never retried, so autorate keeps "working" with no effect on the shaper.

Fix: commit the internal state only after tc succeeds; on failure log an ERROR and leave the state alone so the change is retried on the next update. The monitor-only path (adjust_*_shaper_rate=0) is unchanged.

Verified with a small extraction harness: on master a failed tc still advances the state and never retries; with the fix the state holds, one ERROR is logged, and the next update retries. bash -n and shellcheck identical to master.

[lynxthecat]

So that's 6 lines versus 218 with this replacement PR(!). This surely validates why @rany2 and I are so apprehensive about rapid AI-based changes to the code that has taken years to mature to the present performant state. Without careful scrutiny the code would become a mess rather quickly.

So one concern I have about your approach here is that by raising an error upon a bad rate change, cake-autorate will shut itself down. Is that really what we want? Surely it'd be better to issue a warning to the user?

[ooonea]

Hi, I'll make all the changes based on your feedback. I'd just ask that, if possible, you be a little more understanding instead of constantly criticizing AI—which is certainly an artifice, just like writing or the wheel, or, to stay within the realm of computer science, just like programming languages themselves, which lack consciousness; in the case of AI, it's pure logic applied in a stochastic and probabilistic manner. However, I find it really frustrating to constantly hear that AI ruins the code, after having merged countless lines of code, because the comments are too verbose or because some temporary code was left behind. Constructive criticism is fine, but complaining isn't. Thank you for your understanding. The bugs are definitely there. So are the improvements.

[ooonea]

On the technical point: log_msg ERROR only logs (syslog + log file) — nothing shuts down. On a failed tc the function keeps the previous internal rate and returns, so the change is retried at the next shaper update. I renamed the message to WARNING so the label matches the behaviour (498e866).

[lynxthecat]

Hi, I'll make all the changes based on your feedback. I'd just ask that, if possible, you be a little more understanding instead of constantly criticizing AI—which is certainly an artifice, just like writing or the wheel, or, to stay within the realm of computer science, just like programming languages themselves, which lack consciousness; in the case of AI, it's pure logic applied in a stochastic and probabilistic manner. However, I find it really frustrating to constantly hear that AI ruins the code, after having merged countless lines of code, because the comments are too verbose or because some temporary code was left behind. Constructive criticism is fine, but complaining isn't. Thank you for your understanding. The bugs are definitely there. So are the improvements.

Look @ooonea, I think @rany2 and I have actually been extremely tolerant in respect of your extensive AI-based contributions associated with code alterations, commit messages and even comments. @rany2's initial instinct was to ban you from making any contributions, and I have some sympathy for that but have been holding out because I think some of your contributions have been genuinely helpful. But there is a but here, as follows.

I agree that AI can provide a useful tool.

But the user must remain in the driving seat, validate everything, and bear full responsibility. I think AI is best used for checking and offering improvements; the user ought, as it were, to sit above the AI tool, sanity check everything and recognise and prevent bad suggestions.

The latter has not always been happening with your contributions. Your recent pull requests: #399 and #400 were frankly a complete mess and included 218 lines and 868 lines respectively, which in the replacements #402 and #403 collapsed to 6 lines and 25 lines respectively. Events like this serve to validate our apprehensiveness.

Some of your AI-based contributions or suggestions have also eaten up unnecessary time and have caused some frustration.

If you are not comfortable facing criticism for your extensive use of AI then I suggest you stop contributing.

Trust is earnt, and you cannot expect us not to push back when we see things like those messy pull requests or have to investigate things that end up not necessary and wasting time.

[lynxthecat]

And @ooonea turning to this new PR itself, I am not sure it makes sense.

Now you are proposing:

# on failure keep the previous internal state so the change is retried on the next update
if ! tc qdisc change root dev "${interface[${direction}]}" cake bandwidth "${shaper_rate_kbps[${direction}]}Kbit" 2> /dev/null
then
	log_msg "WARNING" "Failed to change CAKE bandwidth on ${interface[${direction}]}."
			return 1
fi

If the rate change fails and you want to keep the previous internal state as per the comment then surely we should set shaper_rate_kbps[${direction}] = last_shaper_rate_kbps[${direction}]?

And what's the purpose of the return 1 here?

[ooonea]

Answering the open question for the record: shaper_rate_kbps is the controller's target, recomputed every tick; last_shaper_rate_kbps records what is actually installed. Setting the former to the latter would perturb the controller for no gain — the retry already comes from the guard at the top of set_shaper_rate: (( shaper_rate_kbps != last_shaper_rate_kbps )) || return. Returning before the commit block keeps last_ at the installed rate, so the inequality stays true and the change is retried on the next tick. That is the purpose of the return 1: it skips the unconditional last_shaper_rate_kbps commit and the delay compensations, which on master today are computed even when the rate never got installed (the tc exit status is discarded by 2>/dev/null).

[lynxthecat]

@ooonea to be clear, and unless I'm mistaken, your proposal does this:

• retains check that new shaper rate is different form the last rate;
• try the tc call;
• if tc call fails, log warning and return 1

That doesn't actually achieve anything of any use because 1) the shaper rate used in the controller is still allowed to get out of sync with the actual shaper rate; and 2) return 1 is not handled. Not updating the last changed rate here does not serve any useful purpose.

To illustrate:

• last rate is 10, new rate is 12 so check passes
• tc call fails, and we now log warning, return 1 and skip updating the last rate
• controller continues with 12 and the shaper rate is out of sync with the actual rate
• next rate from controller could be 14 and we now compare that with 10, and continue.

What I think would have made sense is rather:

• check that new shaper rate is different from the last rate;
• try the tc call
• if tc call fails, log warning, and set the new shaper rate to the last shaper rate

This would actually prevent the shaper rate as seen from the controller getting out of sync with the rate used by the shaper.

[ooonea]

You're right about the re-anchor, and my fix had that half missing. It kept last_shaper_rate and the compensations honest, but it let the controller keep walking from a rate that was never installed — snapping shaper_rate back to last_shaper_rate on a failed tc closes exactly that hole. I've adopted your variant in my fork, with credit to you in the commit message.

Where I disagree is "doesn't achieve anything of any use": master today discards the tc exit status entirely, so last_shaper_rate and the delay compensations are committed even when the change never landed, and a failed change is never retried. That is the bug this PR was about, and closing it kept it. Your improvement deserved to land on top of the fix, not instead of it.

For the record, the combined shape against current master, if you ever want it:

 	if ((adjust_shaper_rate[${direction}]))
 	then
-		tc qdisc change root dev "${interface[${direction}]}" cake bandwidth "${shaper_rate_kbps[${direction}]}Kbit" 2> /dev/null
+		if ! tc qdisc change root dev "${interface[${direction}]}" cake bandwidth "${shaper_rate_kbps[${direction}]}Kbit" 2> /dev/null
+		then
+			log_msg "WARNING" "Failed to change CAKE bandwidth on ${interface[${direction}]}."
+			# re-anchor the controller to the installed rate; keep the
+			# attempted target if no rate was ever installed (last is
+			# still 0 then, and re-anchoring to 0 would wedge here)
+			(( last_shaper_rate_kbps[${direction}] )) &&
+				(( shaper_rate_kbps[${direction}]=last_shaper_rate_kbps[${direction}] ))
+			return 1
+		fi
 	else

The return 1 before the arithmetic block is load-bearing: it is what keeps last_shaper_rate and the compensations describing the rate that is actually installed. And the snap needs the guard, because last_shaper_rate_kbps is initialised to 0 — if the very first change fails, re-anchoring to 0 would make the check at the top of the function see equal rates and never retry.

This was my last round here — I'll keep working on my own fork from now on. The code is GPL: take whichever half of it you find useful. Thanks for everything that did get merged.