Learn

The fix reads the log one line at a time and still keeps every line

5 November 2026 · Code review: find it, then defend it

The interviewer shares a pull request titled "Stream the log so the slow-request report stops running out of memory". The old readlines() is gone, there is a for loop over the file, and the job passed on staging. It looks like the textbook fix. The question to ask is not "does it read less?" but "what does it still keep?".

The situation

Here is an original example. A nightly job reads a web server's access log, where each line ends with the response time in milliseconds, and writes the requests slower than one second to a report file. Last week the job ran out of memory. The change:

# before
def write_slow(log_path, out_path, limit_ms=1000):
    with open(log_path) as f:
        lines = f.readlines()
    slow = [line for line in lines if int(line.split()[-1]) > limit_ms]
    with open(out_path, "w") as out:
        out.writelines(slow)

# after
def write_slow(log_path, out_path, limit_ms=1000):
    slow = []
    with open(log_path) as f:
        for line in f:
            if int(line.split()[-1]) > limit_ms:
                slow.append(line)
    with open(out_path, "w") as out:
        out.writelines(slow)

The new test feeds three lines, GET /orders 200 120, GET /orders/7 200 1350 and POST /pay 504 3000, and checks that the report holds the last two. It passes on both versions.

What goes wrong, with the input that shows it

Reading is now one line at a time. Keeping is not. The list slow grows with every slow request, and nothing leaves it until the end. So the honest description of the change is: memory used to grow with the number of lines, and now it grows with the number of slow lines.

On an ordinary night that is a large improvement. If one request in a hundred is slow, a log of a million lines leaves ten thousand in the list. That is why staging was happy.

Now pick the input that matters: the night of an outage, when the database is struggling and nearly every request takes over a second. Then almost every line is slow, the list holds almost the whole log, and memory is back where it was before the change. The job fails on the one night its report is needed most, which is also the night the log is largest.

The fix is to write each slow line as soon as it is read, so nothing is kept between iterations:

def write_slow(log_path, out_path, limit_ms=1000):
    with open(log_path) as f, open(out_path, "w") as out:
        for line in f:
            if int(line.split()[-1]) > limit_ms:
                out.write(line)

Now argue the severity:

• Who gets hurt? The engineers investigating an incident, who reach for this report and find the job crashed.
• When does it show? Only when most requests are slow, so not in staging, not in a three-line test, and not on a quiet night.
• Can it be undone? Yes. No data is lost, the log is still there, and the job can be rerun with the fix.

That is a request for changes. The pull request claims to fix a crash, and it leaves the crash in place for the input where the report matters most.

There is a second thing you may notice: a blank line in the log makes line.split()[-1] raise IndexError. Both versions do that, so the diff did not introduce it. Say it, label it as an older bug, and keep it out of your verdict on this change.

What to say out loud

"The read is streamed now, but the list slow still grows with the input, just with the slow lines instead of all of them. On a normal night that is fine. On an outage night nearly every line is slow, so memory is back to what it was, and that is the night someone needs this report. I would request changes and write each slow line as it is read. One question for you: if the job dies halfway through, is a partial report acceptable, or should it write to a temporary file and rename it at the end? Separately, a blank line raises IndexError in both versions. That is an older bug, not something this diff caused, so I would file it on its own."

That answer names what grows, picks the input that makes it grow, ties the harm to a real person, and keeps an older bug from inflating the severity of a new one.

The exercise

1. Find a function in your own code that processes a file, a query result or a queue.
2. Write down every variable that survives from one item to the next.
3. For each one, write what it grows with: nothing, the number of distinct keys, or the number of items.
4. For anything that grows with the number of items, describe the day the input is at its worst, and decide whether the function survives it.
5. Say aloud in under thirty seconds: what is kept, what it grows with, the worst day, and what you would change.

Where this fits in Gadfly

Gadfly has four rooms: algorithms, system design, behavioral and code review. The code review room takes a class you wrote yourself, a connected GitHub repository, or a project from its catalog, and reviews it together with the transcript of how the code was produced. It does not execute the code, so finding the input that makes memory grow is reasoning you do and explain aloud. Code review is part of a session pack. You code in Java or Python, and the tutor speaks English.