DevOps NetworkDevOpsNetwork
Join Free
Practical infra coding

Fix a broken log-rotation script

mediumDebug and review code

Problem statement

A cron job rotates app.log every night, keeping three old copies: after a rotation, app.log.1 should hold yesterday's log, app.log.2 the day before, app.log.3 the day before that, and the oldest copy is dropped. Someone noticed that app.log.2 is always missing and old logs are lost. Find the bugs and fix the script.

rotate.py (broken)

Python
import os
def rotate(path, keep=3):
for i in range(1, keep):
src = f"{path}.{i}"
dst = f"{path}.{i + 1}"
if os.path.exists(src):
os.replace(src, dst)
os.replace(path, f"{path}.1")
open(path, "w").close()

The test harness creates a directory where each file holds a generation label (app.log is the newest, gen4), calls rotate(path, keep=3), and lists the directory. It then calls rotate on a directory where app.log does not exist yet, which is what happens on a fresh host.

Symptom (actual output of the broken version):

TEXT
before:
app.log gen4
app.log.1 gen3
app.log.2 gen2
app.log.3 gen1
after rotate(keep=3):
app.log (empty)
app.log.1 gen4
app.log.3 gen3
missing app.log: FileNotFoundError

Expected after rotation: app.log.1 = gen4, app.log.2 = gen3, app.log.3 = gen2, and gen1 dropped.

Examples

Example 1

Input: The same harness with the fixed `rotate`

Output: before: app.log gen4 app.log.1 gen3 app.log.2 gen2 app.log.3 gen1 after rotate(keep=3): app.log (empty) app.log.1 gen4 app.log.2 gen3 app.log.3 gen2 missing app.log: ok, nothing to do

Hints

Approach

Optimal

Bug 1: the shift runs in the wrong direction. With range(1, keep) the loop first moves .1 (gen3) onto .2, overwriting gen2. The next iteration then moves .2, which now holds gen3, onto .3, overwriting gen1. Two generations are destroyed, one survives in the wrong slot, and .2 ends up missing. Shifting up has to start at the highest number: range(keep - 1, 0, -1) moves .2 to .3 first, then .1 to .2, so every file moves into a slot that is already free.

Bug 2: dropping the oldest copy is accidental. The broken script only "deletes" old logs by overwriting them. The fix removes app.log.{keep} explicitly before shifting, so the retention rule is visible in the code.

Bug 3: it crashes when there is nothing to rotate. os.replace(path, ...) raises FileNotFoundError on a host where the app has not written a log yet, and cron sends an error email every night. The fix returns early if the log is missing or empty (rotating an empty file only pushes a useful old log out of retention).

Bug 4: no input validation. keep=0 would still rotate into .1. The fix rejects keep < 1.

Why os.replace. On Windows os.rename fails if the target exists, while os.replace overwrites on every platform. The broken script already used os.replace, which is exactly why the data loss was silent everywhere instead of crashing.

What the fix does not cover. A process that still has app.log open keeps writing to the renamed file (app.log.1). The application must reopen its log (often on SIGHUP), or you use copy-then-truncate. See the FAQ.

ComplexityTime O(keep)Space O(1)
Python
import os
def rotate(path, keep=3):
if keep < 1:
raise ValueError("keep must be >= 1")
if not os.path.exists(path) or os.path.getsize(path) == 0:
return # nothing to rotate
oldest = f"{path}.{keep}"
if os.path.exists(oldest):
os.remove(oldest) # drop the oldest on purpose
for i in range(keep - 1, 0, -1): # newest-numbered first: .2->.3, then .1->.2
src = f"{path}.{i}"
if os.path.exists(src):
os.replace(src, f"{path}.{i + 1}")
os.replace(path, f"{path}.1")
open(path, "w").close() # fresh, empty log for the writer
# ---- test harness: build a log dir, rotate, show what is left ----
import tempfile
from pathlib import Path
def show(d):
for p in sorted(Path(d).iterdir()):
text = p.read_text().strip() or "(empty)"
print(f" {p.name:<10} {text}")
with tempfile.TemporaryDirectory() as d:
log = Path(d, "app.log")
for name, text in [("app.log", "gen4"), ("app.log.1", "gen3"),
("app.log.2", "gen2"), ("app.log.3", "gen1")]:
Path(d, name).write_text(text + "\n")
print("before:")
show(d)
rotate(str(log), keep=3)
print("after rotate(keep=3):")
show(d)
with tempfile.TemporaryDirectory() as d:
try:
rotate(str(Path(d, "app.log")), keep=3)
print("missing app.log: ok, nothing to do")
except Exception as e:
print(f"missing app.log: {type(e).__name__}")

Follow-up questions

  • Compress rotated files (app.log.2.gz) except the most recent one, like logrotate's delaycompress.
  • Rotate by size instead of by schedule: only rotate once app.log is over 100 MB.
  • Two copies of the cron job run at the same time. What breaks, and how do you prevent it? (a lock file with fcntl.flock)

Frequently asked questions

Log rotation scripts are classic hand-rolled ops tooling, and their bugs lose exactly the logs you need during an incident. The shift-order bug is a good review question because the code looks right at a glance and only fails when you trace it with real files.

Renaming a file does not affect processes that already have it open; they hold the file, not the name. The app has to be told to reopen its log (many daemons do this on SIGHUP), or the rotator copies the file and truncates the original in place (logrotate's copytruncate), which can lose lines written between the copy and the truncate.

On Linux hosts you usually should. The exercise is about reading code carefully, and the same shift pattern shows up in backup retention, release directories and numbered snapshots.