Python Debug & Fix

The reasoning

Look at the two outputs: the surviving rows and their total are identical on both sides. That is the point. The bug is not a wrong number, it is that a caller reading "loaded 2 rows, total 40" has no way to know that three rows were offered. A downstream dashboard showing 40 instead of 60 looks like a business change, not a defect.

`except Exception` also catches far more than a parse failure. A `KeyError` from a renamed column, a `MemoryError`, a `TypeError` from a bug you introduced in the loop body — every one of them becomes "skip this row". The blast radius grows every time someone adds a line inside the `try`.

The fix has three parts and all three matter. Narrow the exception to the one you actually expect (`ValueError` from `int()`), so an unexpected failure still propagates. Collect the rejects with enough context to act on — the line and the message. And decide explicitly what a non-zero reject count means: fail the job, quarantine the rows to a dead-letter location, or accept it under a documented threshold.

The interview answer worth giving is the last of those. "Skip bad rows" is not a policy; "reject rows that fail to parse, write them to a rejects table, and fail the run if rejects exceed 1% of the batch" is. The code should make the threshold visible rather than burying it in a `pass`.

The reasoning

The handler catches `Exception`, which includes everything the body can raise. The network call succeeded; the failure came from the data — a non-numeric amount in the payload. The log line then actively misleads whoever investigates, pointing them at the vendor rather than at the parsing code.

Returning 0 compounds it. A page that failed and a page with no revenue are now indistinguishable, so the daily total is quietly short and nothing in the pipeline registers an error. This is the same "None means two things" problem in numeric form.

The fix is to shrink the `try` to the single call whose failure it describes, and to name the exception precisely — `OSError` covers the socket-level failures, or the client library's own exception type if it has one. Everything after that call runs outside the handler, so a data problem raises a data error, with a traceback pointing at the line that could not parse.

The general rule worth stating: a `try` block should contain exactly the operation the `except` clause knows how to handle, and no more. Every extra line inside it is another failure mode being relabelled. In review, a `try` covering more than about three lines is worth questioning.

The reasoning

Interpolating `str(error)` keeps the words and throws away everything programmatic. `__cause__` is None, so nothing downstream can ask "was this really a parse failure?" — an `except ValueError` two frames up will not match, and a retry policy that treats parse errors and network errors differently has nothing left to branch on.

The traceback is the bigger loss in practice. Without the chain, the printed traceback starts at the `raise` inside the wrapper. The line that actually failed — the `int()` call, three functions down — does not appear. You know the load failed; you do not know where.

`raise NewError(...) from original` sets `__cause__` and makes Python print both tracebacks joined by "The above exception was the direct cause of the following exception". You get your domain-level message *and* the original site. Note that a bare `raise` inside an `except` block re-raises the original untouched, which is what you want when you only meant to log and continue.

The third form is `from None`, which deliberately suppresses the context. That is right when the original is noise the caller should not see — a `KeyError` from an internal dict that you are turning into a clean `MissingRegion` — and wrong everywhere else. The point is that all three are choices, and the buggy version made one by accident.

The reasoning

There are two conflations stacked here. The function maps a missing key to None, and the caller's truthiness test maps both None and `[]` to "no data". Either alone would be recoverable; together, three genuinely different states — has rows, configured but empty, not configured — become two.

The operational consequence is the one to lead with. "us returned no data" is something an on-call engineer might reasonably ignore for a quiet region. "apac is not in the config" is a deployment problem that will keep producing zero rows until someone notices, and the longer it hides the more backfill it costs.

The fix raises for the absent case, because it is genuinely exceptional and the caller must decide what to do about it. A sentinel would work too — a distinct `MISSING = object()` — but an exception is harder to ignore by accident, which is the property you want. `from None` suppresses the internal `KeyError`, since the caller does not care that the config happened to be a dict.

The general rule: reserve None for one meaning per function and say which in the type hint. `Optional[list[Row]]` returning None for "unknown region" is defensible only if documented; returning `list[Row]` and raising for the unknown case needs no documentation at all, which is why it is usually better.

The reasoning

The failure is in the gap between "the server committed" and "the client heard about it". From the client's side a lost acknowledgement is indistinguishable from a write that never happened, so it cannot decide correctness by looking at the error. Three attempts, three commits, six rows.

The fix is to make the operation naturally repeatable rather than to make the retry smarter. Writing into a dict keyed by `(batch_id, row_id)` means the second and third attempts overwrite the entries the first one made — the end state is the same whether the write ran once or ten times. In a real sink that is a MERGE on a natural key, an idempotency key on the API request, or writing to a partition path that a re-run replaces wholesale.

The `batch_id` matters as much as the row key. Without it, a genuine second delivery of the same row ids — a corrected batch — would be indistinguishable from a retry. With it, retries collapse and legitimate re-deliveries stay separate, which is the distinction the sink actually needs.

When the operation genuinely cannot be made idempotent — charging a card, sending an email — the answer changes: record intent before acting, in a durable store, and check that record before retrying. That turns "did it happen?" from an unanswerable question into a lookup. Saying this in an interview signals you have thought about the case where the neat fix is unavailable.

The reasoning

A retry is a bet that the failure was transient. That bet is good for a 500, a 503, a timeout or a dropped connection, and it is guaranteed to lose for a 400 or a 422 — the server has evaluated the request and rejected it, and the identical request will be rejected identically.

The cost is more than wasted seconds. Under a broad outage every client retries every request, which is exactly when the upstream service can least afford five times the traffic; that is the retry storm that turns a partial outage into a total one. And the real error — a malformed date parameter, here — is reported five failures later than it could have been.

429 is the interesting exception: it is a 4xx and it *is* retryable, but only after waiting, and ideally for the duration the `Retry-After` header specifies. 408 and 425 are similar. So the rule is not "retry 5xx only" but "retry the statuses the protocol says are transient", which is worth saying precisely.

The rest of a production retry policy: exponential backoff so attempts spread out rather than clustering, jitter so a fleet of clients does not synchronise, a cap on total elapsed time rather than only on attempt count, and a circuit breaker that stops trying when a service is clearly down. `tenacity` implements all of it; the point of the question is knowing what to configure.

The reasoning

The retry loop assumes its inputs are unchanged between attempts. That holds for a list and fails for anything single-pass — a generator, a file handle, a socket, a database cursor. The first `list(rows)` drained it; the exception came afterwards; the second call to `list()` on the same object returned an empty list, and the function cheerfully reported success.

Note how quiet the failure is. No exception, no warning, and a return value of 0 that a caller could easily interpret as "there was nothing to send". If the sink is append-only, this is a batch silently lost — worse than the duplicate-write bug, because a duplicate is at least visible.

The fix is one line: materialise before the loop, so the retryable operation is a pure function of a fixed list. This also makes the memory cost explicit, which is the right trade — you cannot retry what you have not kept, so a retryable send is inherently bounded by what you can hold.

When the batch is too large to hold, the structure has to change rather than the loop. Retry at a coarser granularity where the source can be replayed — re-read the file from the offset, re-query with the same cursor — or checkpoint what has been acknowledged and resume from there. The general principle: a retry needs a replayable input, and if you cannot replay it you need a checkpoint instead.

The reasoning

The inner `for _ in range(3)` breaks on success and otherwise ends normally. Ending normally is the failure case, and nothing distinguishes it, so the outer loop carries on. The result is a partial dataset presented as a complete one — the worst outcome available, because downstream consumers have no signal that anything is wrong.

`for ... else` is the idiomatic fix and reads badly until you learn it: the `else` runs when the loop finished *without* breaking. Here that means every attempt failed, so it raises with the page number and the attempt count. If the construct still feels obscure, a `success = False` flag checked after the loop is equally correct and more obvious to a reader.

The design question underneath is what "partial" should mean for this job. Failing the whole run is right when downstream aggregates would be wrong without page 2. Continuing and reporting is right when pages are independent and a later backfill can fill the gap — but then the count of failures must be part of the job's output, and the run must be marked degraded rather than successful.

Whichever you choose, the invariant is that the caller can tell. A job that returns rows and a status is honest; a job that returns rows only is asking every consumer to trust that nothing went wrong. This is the same failure as `bare-except`, one level up.

The reasoning

CSV is not "text separated by commas". It is a format with quoting rules: a field may be wrapped in double quotes, in which case it may contain commas, newlines and doubled-up quotes. `str.split` implements none of that, so `"Acme, Inc."` becomes two fields and every subsequent index shifts by one.

The code hid it in a way worth noticing: `parts[-1]` for the amount kept working, because the extra field was inserted in the middle and the amount stayed last. So the total is still right and only the name is corrupt — a single wrong column is much easier to miss in review than a crash. Had the parser used `parts[2]`, it would at least have raised or produced obvious nonsense.

`csv.reader` handles the rules and costs nothing to adopt; it takes any iterable of lines, so it works on a file handle, a `StringIO`, or a network stream. Embedded newlines are the argument that ends the discussion — a quoted field can contain a line break, which means iterating the file line by line is *itself* wrong, and only a real parser can know where a record ends.

Two related details for real files: open with `newline=""` so the module handles line endings itself, and pass an explicit `encoding`. And if the file is large and well-formed, `pyarrow.csv` or DuckDB will parse it in C, correctly, faster, with types inferred — which is usually the better answer than either version here.

The reasoning

UTF-8 encodes `ü` as `0xC3 0xBC`. latin-1 is a single-byte encoding covering all 256 values, so it decodes those two bytes as `Ã` and `¼`. The result is a string that is longer than it should be, sorts wrongly, and does not compare equal to the same city name from any other source — so joins and lookups silently miss.

The absence of an error is the whole problem. Because latin-1 has no invalid byte sequences, `decode("latin-1")` *never* raises, which is why it gets used as a "safe" fallback. It is the opposite of safe: it guarantees you get a string, and guarantees nothing about whether it is the right one. Decoding as UTF-8 raises `UnicodeDecodeError` on genuinely non-UTF-8 input, which is the loud failure you want.

The fix is to declare the encoding rather than assume it, and to let it fail when the assumption is wrong. Where the source really is inconsistent, `errors="replace"` makes the damage visible as `�` characters, and `chardet`/`charset-normalizer` can guess — but guessing belongs in a one-off investigation, not in a daily pipeline. The durable fix is to get the encoding into the data contract.

Two practical notes. Always pass `encoding=` explicitly when opening files: the default is locale-dependent, so the same code decodes differently on a developer laptop and a container. And normalise Unicode at ingest with `unicodedata.normalize("NFC", s)`, because `ü` has two valid representations — precomposed and `u` plus a combining diaeresis — that look identical and do not compare equal.

The reasoning

Iterating a file yields lines and advances a position that never resets on its own. `iter(handle) is handle`, so there is no fresh cursor for the second loop. The count is correct because it ran first; the sum sees an empty sequence and returns the identity value, 0, without any indication that it read nothing.

The zero is the dangerous part. `sum` of nothing is 0, `max` of nothing raises but `max(..., default=0)` does not, and `list` of nothing is `[]`. A pipeline that computes several statistics over one handle will get the first one right and quietly zero the rest.

One pass is the correct fix here, and it generalises: any time you find yourself iterating the same source twice, ask whether the two computations can share a loop. It is faster, it works on a stream of any size, and it removes the ordering dependency entirely.

`handle.seek(0)` works for a real file on disk and is the wrong habit to build, because the identical code fails on a network response, a `stdin` pipe, a decompression wrapper or an S3 streaming body — none of which are seekable. If you genuinely need multiple passes over a non-seekable source, materialise it deliberately or write it to a temporary file, and make that cost visible.

The reasoning

The JSON spec has no notion of a non-string key. `json.dumps` silently coerces `int`, `float`, `bool` and `None` keys to their string forms rather than raising — a convenience that makes the write succeed and the read wrong. The values keep their types; only the keys change, which is why the dict still looks broadly right when printed.

The lookup then fails in the quietest possible way. `restored.get(1)` returns None, which a caller may well interpret as "no entry for this id" rather than "the entire table is keyed differently now". If the code used `restored[1]` it would at least raise `KeyError`.

The immediate fix is to restore the type on load — `{int(k): v for k, v in loaded.items()}` — and to do it in one place, at the deserialisation boundary, rather than at each lookup. If keys can be more than one type, that is a sign the structure should be a list of records with an explicit key field instead of a dict.

The general lesson is that JSON is a lossy encoding for Python values, and keys are only the first case. `tuple` becomes `list`, so a tuple key fails outright and a tuple value comes back as something that no longer hashes. `Decimal` raises unless you supply a default, and encoding it as a float reintroduces the money problem. `datetime` is not supported at all. For anything where types matter, Parquet, Avro or msgpack preserve them; JSON is for interchange with things that cannot do better.

The reasoning

`pool.submit` returns a `Future` immediately and runs the callable on a worker thread. If the callable raises, the executor catches it and stores it on the future — it must, because there is no call stack to propagate into. The exception surfaces only when something calls `.result()` or `.exception()` on that future.

Here the return value of `submit` was discarded, so the futures were unreachable and the stored exceptions went with them. Exiting the `with` block calls `shutdown(wait=True)`, which waits for the work to *finish* — it does not inspect outcomes, which is why the block completes cleanly on a failed batch.

The fix is to keep the futures and resolve each one. `pool.map` behaves differently and is worth knowing about: it re-raises on iteration, so failures do surface — but at the first failing element, in input order, which means later results are never reached. `as_completed` plus a per-future `try` gives you every outcome and is the right shape when partial success needs reporting.

The same trap exists in asyncio. A task created with `asyncio.create_task` and never awaited swallows its exception in the same way, and you get at most a "Task exception was never retrieved" warning at garbage-collection time. In both worlds the rule is identical: something must consume the result of every unit of work you start.

The reasoning

`pool.map` preserves input order; `as_completed` deliberately does not — yielding results as they finish is the entire reason to use it. The moment you zip its output against the inputs, you have assumed an ordering it explicitly does not provide.

Here the pages are submitted as 3, 2, 1 and page 1 is fastest, so `bodies` comes back as `body-1, body-2, body-3` and zipping with `[3, 2, 1]` attaches page 1's body to page 3. The dictionary has the right keys, the right number of entries and plausible values, which is why nothing downstream complains.

In testing it usually looks correct, and that is the trap. With fast local stubs the tasks often complete in submission order, so the pairing accidentally lines up; the scrambling appears under real latency, in production, intermittently. Any test that would catch it has to deliberately vary the durations.

The fix is the standard idiom: build `{pool.submit(fn, item): item for item in items}` and look the input up from the future when it completes. That keeps the association explicit rather than positional — the same principle as using a dict index instead of relying on two lists staying aligned.

The reasoning

Under threads the module-level dict is genuinely one object, so the original code worked — at the cost of needing a lock. Under processes each worker is a separate interpreter with its own memory; with the spawn start method it re-imports the module from scratch, so `TOTALS` is freshly initialised to zero in every child and the parent's copy is never touched.

Nothing raises, because every individual operation is valid. The workers really did count the rows, in their own address spaces, and then exited. The parent prints the value it started with. This is the same class of failure as the futures bug — work happened and its result was discarded — but here the discarding is a property of the memory model rather than an oversight.

The fix makes the worker a pure function of its argument that returns its result, and lets `Pool.map` collect the return values. That is not a workaround; it is the shape multiprocessing requires, and it happens to be the shape that is easiest to test — you can call `count([1, 2])` directly with no pool at all.

When shared mutable state is genuinely needed there are `Manager().dict()` (a proxy, with a socket round trip per access), `Value`/`Array` for simple numbers, and `shared_memory` for large buffers. All three cost more than returning a value, and all three reintroduce the locking question, so reach for them only when the data is too large to copy.

The reasoning

By default `gather` re-raises the first exception immediately. The remaining tasks are not cancelled — they keep running in the background — but the `await` has already returned control to the `except` block, so their results have nowhere to go. From the caller's point of view the whole batch produced nothing, and page 1's work was done and thrown away.

`return_exceptions=True` changes the contract: `gather` waits for every task to settle and returns a list positionally aligned with the inputs, where each element is either a result or an exception object. You then partition that list, which gives you both the pages you got and a precise account of the ones you did not.

The positional alignment is the part people miss and the reason this is better than `as_completed` here — the index in the returned list matches the index of the coroutine you passed in, so associating a failure with its page needs no extra bookkeeping.

The design question is what to do with the partition. Failing the run is right when the downstream aggregate needs every page; recording the failures and continuing is right when pages are independent and a backfill can fill the gap. Either way the job must not report plain success — which is the same conclusion as the exhausted-retries question, arrived at from the async side.

The reasoning

Python's truthiness rules make `0`, `0.0`, `""`, `[]`, `{}` and `False` all falsy. In business data every one of those is a legitimate value: a zero-value order, a customer who left the note blank, a batch with no tags. Treating them as missing rejects real rows.

This is the direct Python analogue of the SQL question about `COUNT(column)` versus `COUNT(*)` — the distinction between "no value" and "a value that happens to be empty" is the same distinction, and getting it wrong changes the numbers rather than crashing.

The consequence runs in both directions. Here it rejects valid rows, so the loaded total is short. The reverse appears in defaulting code: `amount = row.get("amount") or 0` silently rewrites a legitimate 0 as 0 (harmless) but also rewrites a legitimate `0.0` price or an empty-but-meaningful string. `x if x is not None else default` is the version that means what it says.

Two related habits worth naming. Distinguish "key absent" from "value is None" — `"amount" not in row` and `row["amount"] is None` are different data-quality problems with different causes. And do the check once at the parsing boundary rather than at every use, so there is one place that decides what missing means for this dataset.

The reasoning

The comprehension has no notion of "latest". It inserts `id: row` for every row and each repeat overwrites the previous value, so the survivor is whichever occurrence came last in iteration order. That is an accident of how the file was written, how the query sorted, or which partition was read first.

It fails silently and inconsistently, which is the worst combination. Change the upstream sort order and the answer changes; re-run the same job on a re-generated file and it may change again. Nothing in the code or the output records that a choice was even made.

The fix states the policy: keep the row with the greatest `updated_at`. That makes the result independent of input order, which is also what makes the job idempotent — re-running it on the same data produces the same answer, which is the property backfills depend on. Note the tie-break question the fix leaves open: with equal timestamps this keeps the first seen, and if that matters you need a second key.

It is worth saying out loud that this is a window function in disguise — `ROW_NUMBER() OVER (PARTITION BY id ORDER BY updated_at DESC)` is the same operation, and if the data is already in a warehouse that is where it belongs. Doing it in Python is right when the data is in flight; doing it in Python *because* you did not think of the SQL is not.

The reasoning

The generator is consumed by the validation pass. `[r for r in rows if r["id"] < 0]` walks it to the end, so `len(list(rows))` afterwards is 0. The check is not wrong about the rows it saw — it is that the rows it saw are now gone.

Worse, the check would report "passed" even on data full of negatives, as long as something consumed the generator first. A check whose result does not depend on the data is not a check. The six months of green is evidence of nothing, which is the reason to be actively suspicious of a gate that has never fired.

The fix materialises once and uses the same list for both the check and the write, so the validation is provably about the rows that get written. That is the property you actually want from a quality gate: it must be evaluated over the same data the consumer receives, not over a separate pass that might see something different.

The general practice this argues for is to give every check a deliberate negative test — feed it data you know is bad and confirm it fails. That is cheap, it is the only way to distinguish "no problems" from "no detection", and it would have caught this on day one. In dbt or Great Expectations terms: assert that your assertion can fail.

The reasoning

The filtered version divides 1 paid by 2 known statuses and reports 50%. The unfiltered version divides 1 by 4 and reports 25%. Both are arithmetically correct; they answer different questions, and only one of them is the question the metric name claims to answer.

The perverse incentive is what makes this dangerous. If an upstream bug starts nulling out statuses, the known-status denominator shrinks, and the reported paid rate climbs. The metric moves in the opposite direction to the problem, so the dashboard looks better precisely when the pipeline is degrading.

The fix keeps the full denominator and reports the unknown count alongside. That makes the ambiguity visible rather than resolving it silently: a consumer can see 25% paid with 2 of 4 unknown and draw their own conclusion, which they could not do from 50% alone. Publishing the coverage next to the rate is the general pattern.

This is the same reasoning as `COUNT(column)` versus `COUNT(*)` in SQL, and it generalises to any ratio: state what the denominator is in the metric's name, and if unknowns are excluded, say so — `paid_rate_of_known_status` is ugly and honest. Where the exclusion is genuinely right, it should be a documented decision rather than a `WHERE` clause someone added to make a division work.

The reasoning

**What a decorator actually does.** `@retry` above a `def` means `f = retry(f)`. The name is now bound to whatever `retry` returned — usually a closure called `wrapper` — and that object has its own `__name__`, `__doc__` and `__module__`. Anything that introspects the function sees the wrapper.

**Where it shows up.** Tracebacks, `logger.info(func.__name__)`, `help()`, API frameworks that route on the function name, and metrics keyed on it. All of them start describing the decorator, and every decorated function in the codebase looks identical.

**The fix.** `@functools.wraps(fn)` on the inner function copies `__name__`, `__doc__`, `__module__`, `__qualname__` and `__dict__`, and sets `__wrapped__` so tools can still reach the original. It is one line and there is no reason ever to omit it.

**The related defect.** A wrapper written as `def wrapper(row)` instead of `def wrapper(*args, **kwargs)` only works for single-argument functions, and fails with a confusing `TypeError` the first time somebody decorates something else. Forward everything unless you are deliberately narrowing the signature.

The reasoning

**Why it happens.** Default arguments are evaluated once, when the `def` statement runs, not on each call. `def collect(rows, acc=[])` therefore has one list attached to the function object, shared by every call that does not supply its own.

**Why it looks intermittent.** A script exits and forgets, so tests pass and a local run looks fine. A scheduled worker keeps the module alive for days, so the list accumulates across every batch in that process — and the counts grow, which reads as a data problem rather than a code one.

**The fix, and the half people get wrong.** Default to `None` and build the real default inside: `acc = [] if acc is None else acc`. Test the sentinel with `is None`, not with `if not acc` — an empty list the caller deliberately passed is falsy, and truthiness would silently discard their object and hand back your own.

**Where else it hides.** The same rule catches `def f(t=datetime.now())`, which freezes the timestamp at import so every row gets the time the process started. And the class-level version: `class Batch: rows = []` gives every instance the same list, because the attribute belongs to the class.

The reasoning

**Cause one: the representation.** `0.1 + 0.1 + 0.1 == 0.3` is `False`, because binary floating point cannot represent a tenth. Over a million rows those errors accumulate into something a reconciliation can see, and no amount of rounding at the end recovers it.

**Cause two: the rounding mode.** Python's `round()` is round-half-to-even, so `round(0.5)` is `0` and `round(2.5)` is `2`. Warehouses commonly round half-up. Half the ties therefore go the other way, which is a penny per affected row and never reconciles.

**And the one that looks like the tie rule and is not.** `round(2.675, 2)` gives `2.67` because `2.675` is stored slightly below the tie. The digit was gone before rounding started, which is why the fix is the representation rather than the mode.

**The fix.** Parse money with `Decimal` from the **string** the source gave you — `Decimal(0.1)` imports the float's error rather than escaping it — and round with `quantize(Decimal('0.01'), rounding=ROUND_HALF_UP)`, naming the mode even when it is the default.

**And check the total.** Splitting 100.00 three ways and rounding each share gives 99.99. The parts have to sum back to the whole, and allocating the remainder deliberately is a decision somebody has to make rather than a rounding artefact to absorb.