Code Review and Working with Existing Codebases Questions

Direct answer

I'd write at least four tests, covering the success path, an HTTP error response, a malformed JSON body missing the expected key, and a network-level failure like a timeout, all with requests.get mocked so no test makes a real network call. Each test targets a distinct way this function can fail in production, not just variations on the happy path.

Structured elaboration

Approach. Mock requests.get so the function's own logic, not the network, is what's under test. For each test, build a fake response object with just enough behavior to drive the code path being tested (raise_for_status either does nothing or raises, json() returns a controlled payload), then assert on get_latest_image's return value or on the exception it raises.

Why these specific tests.

• Success proves the normal path works and the correct value is extracted from a realistic JSON payload.
• HTTP error (a 4xx or 5xx status) proves the function surfaces the failure via raise_for_status() rather than silently returning something wrong.
• Missing key in the response body proves that if the API's response shape doesn't match what the code expects, the caller gets a clear exception rather than a confusing downstream error somewhere else.
• Network-level failure (a timeout, a connection error) proves the function doesn't swallow or mask an infrastructure problem, which matters specifically because this function is used in a deployment script where a caller needs to know the difference between "the deploy image genuinely doesn't exist" and "we couldn't reach the registry at all."

Worked example

# deploy_utils.py
import requests

def get_latest_image(repo):
    r = requests.get(f'https://registry.example/api/{repo}/latest')
    r.raise_for_status()
    return r.json()['tag']

# test_deploy_utils.py
from unittest.mock import Mock, patch
import pytest
import requests
from deploy_utils import get_latest_image


def make_response(json_data=None, raise_error=None):
    resp = Mock()
    resp.raise_for_status = Mock(side_effect=raise_error) if raise_error else Mock()
    resp.json = Mock(return_value=json_data or {})
    return resp


def test_get_latest_image_returns_tag_on_success():
    resp = make_response(json_data={'tag': 'v1.2.3'})
    with patch('deploy_utils.requests.get', return_value=resp) as mock_get:
        result = get_latest_image('myapp')
    assert result == 'v1.2.3'
    mock_get.assert_called_once_with('https://registry.example/api/myapp/latest')


def test_get_latest_image_raises_on_http_error():
    resp = make_response(raise_error=requests.exceptions.HTTPError('404 Client Error'))
    with patch('deploy_utils.requests.get', return_value=resp):
        with pytest.raises(requests.exceptions.HTTPError):
            get_latest_image('missing-repo')


def test_get_latest_image_raises_keyerror_on_malformed_body():
    resp = make_response(json_data={'digest': 'sha256:abc'})
    with patch('deploy_utils.requests.get', return_value=resp):
        with pytest.raises(KeyError):
            get_latest_image('myapp')


def test_get_latest_image_propagates_network_timeout():
    with patch('deploy_utils.requests.get', side_effect=requests.exceptions.Timeout):
        with pytest.raises(requests.exceptions.Timeout):
            get_latest_image('myapp')

Actually run with pytest, output:

test_deploy_utils.py::test_get_latest_image_returns_tag_on_success PASSED
test_deploy_utils.py::test_get_latest_image_raises_on_http_error PASSED
test_deploy_utils.py::test_get_latest_image_raises_keyerror_on_malformed_body PASSED
test_deploy_utils.py::test_get_latest_image_propagates_network_timeout PASSED

4 passed

Complexity

This is straightforward, constant-time mocked I/O per test, no algorithmic complexity to speak of; the interesting design decision is which failure modes are worth a dedicated test, not runtime cost.

Edge cases

• A 500 server error takes the exact same code path as a 404, since both raise via raise_for_status(); one test covering "any HTTP error" is representative, a second status-specific test adds little.
• A response that's valid JSON but not a dict at all (a bare list, for example) would raise TypeError rather than KeyError when ['tag'] is applied; worth a fifth test if this API's contract is genuinely uncertain.
• Real production code often uses a requests.Session with a configured retry adapter rather than a bare requests.get; mocking at the requests.get level, as done here, doesn't exercise that retry behavior at all, which would need a different test approach.

Trade-offs and pitfalls

Mocking at the requests.get level is fast and has zero network flakiness, but it also means these tests can't catch a real integration problem, like the registry's actual response shape changing; a smaller number of separate integration tests against a real or realistic staging registry are worth having alongside these, not instead of them. A common pitfall is mocking so aggressively that the test asserts almost nothing about get_latest_image's own logic, for example forgetting to assert on the exact URL called, which would let a bug in the f-string (a wrong path, a typo) slip through unnoticed.

Direct answer

A rollback-safe rename plus backfill never touches the old column or existing readers directly. It adds the new column alongside the old one, writes to both while backfilling the new one in batches, verifies the backfilled data matches, only then switches reads over behind a flag, and keeps the old column around for a retention window so any step can be reversed just by flipping the flag back, not by undoing a destructive change.

Structured elaboration

Each phase below names what code review should specifically confirm before approving it, as a "Review check," plus what to monitor once it ships.

1. Schema change. Add the new column as nullable, with no constraints yet:

ALTER TABLE events ADD COLUMN new_name text NULL;

Review check: confirm this specific statement is additive only and backward-compatible, meaning every existing reader and writer keeps working unmodified the moment this ships, with zero application changes required yet.

2. Dual-write. Deploy an application change, behind a feature flag, that writes both the old and new column on every write to a row. Review check: is the write to both columns transactional or otherwise guaranteed consistent (not "write old, then separately and non-atomically write new"), and is the flag off by default so this ships dormant before anything depends on it?

3. Backfill. A batched, idempotent job fills in the new column for existing rows, only where it's still NULL, ordered by primary key, with a checkpoint so it can resume after an interruption instead of restarting from row one:

UPDATE events
SET new_name = old_name
WHERE id BETWEEN :batch_start AND :batch_end
  AND new_name IS NULL;

Review check: is progress persisted somewhere durable (not just in the running process's memory), and does re-running an already-completed batch do nothing (true idempotence), not create incorrect data? Monitoring: track batches completed, rows backfilled, replication lag, and write error rate on the table for the duration of the backfill, and pause automatically if replication lag crosses an agreed threshold.

4. Verification. Before trusting the backfill, sample a random set of rows and confirm new_name matches what old_name implies for each. Review check: is the sample size and comparison method actually specified in the PR, not just asserted as "we verified it"?

5. Read cutover. Only after verification passes, flip the flag so reads prefer the new column, falling back to the old one if the new one is somehow still empty for a given row. Review check: is the fallback logic actually tested, not just written? Monitoring: application error rate and query latency on this table specifically, right after the flag flips, since a regression here is the trigger for the rollback shown in the diagram below.

6. Cleanup. Once reads have run on the new column successfully for a defined retention window, make it NOT NULL, add any index it needs, and only then drop the old column, in a separate, later PR. Review check: is dropping the old column genuinely a separate step from everything above, so it can never accidentally ship bundled with a change that hasn't been verified yet?

flowchart LR
  A[Step 1: add new_name column, nullable] --> B[Step 2: dual write old_name plus new_name]
  B --> C[Step 3: backfill new_name in batches where NULL]
  C --> D{Verification: sampled row values match}
  D -- mismatch found --> C
  D -- fully verified --> E[Step 4: flip reads to new_name behind a flag]
  E --> F{Error rate normal after cutover}
  F -- regression --> G[Rollback: flip flag back to old_name, dual write stays intact]
  F -- healthy --> H[Step 5: make new_name NOT NULL, add index concurrently]
  H --> I[Step 6: drop old_name after a retention window]

Worked example

Renaming user_email to primary_email on a table with 40 million rows, backfilled in batches of 5,000 rows: that's 40,000,000 / 5,000 = 8,000 batches total. With a short pause between batches to keep replication lag bounded, the job runs as a background process over however long it takes to work through all 8,000 batches, checkpointing its position after each one so a restart resumes from the last completed batch instead of row one. Verification samples 10,000 random rows after the backfill reports complete and confirms primary_email equals user_email for every one of them before the flag is ever flipped to prefer reads from the new column.

Trade-offs and pitfalls

Every step here is reversible specifically because the old column and old read path stay intact until the very last, separate cleanup step, which is exactly what makes this slower and more code than a single rename statement; that trade is worth it for a heavily-used column and wrong for a rarely-touched internal table, where a single migration with a maintenance window might be simpler and perfectly safe. The most dangerous version of this pattern to review is one that quietly combines two of these steps, most often shipping the read cutover and the old-column drop in the same change, which collapses the rollback safety the whole design exists to provide.

Direct answer

None of the three tasks here is idempotent, meaning running the playbook a second time against a server that's already correctly configured should report no changes, but this one still reports changes, or worse, causes them, every single time. Each task needs to move from an imperative shell command to a declarative module that checks the current state before acting, and the fix should be validated by actually running the playbook twice and confirming the second run reports zero changes, not just by reading the code and assuming it's fine.

Structured elaboration

Task 1: command: apt-get install -y nginx. A raw shell command has no idea whether nginx is already installed; it just runs apt-get install every single time. That might be a no-op at the package-manager level, but Ansible itself has no way to know that and will always report this task as "changed," which defeats the entire point of using a configuration-management tool.

Task 2: the copy task for the config file. This one is closer to idempotent already, since Ansible's copy module compares the destination file's content against what's being written and only reports a change when the content actually differs. It's still incomplete though: no explicit file permissions or owner are set, and a config change should trigger a service reload, not happen silently with no connection to the next task.

Task 3: service: state: restarted. This always restarts the service on every single run, whether or not anything actually changed. It's the least idempotent line in the whole playbook: running this playbook nightly, for example on a schedule, would bounce nginx nightly for no reason at all.

The fix. Use Ansible's notify/handler pattern: the config-file task notifies a handler, and the handler, which reloads or restarts the service, only runs when that specific task actually reported a change. A no-op run then touches the service zero times.

Worked example

A corrected version of the playbook:

- hosts: web
  become: true
  tasks:
    - name: install nginx
      apt:
        name: nginx
        state: present

    - name: place nginx site config
      copy:
        content: "server { listen 80; }"
        dest: /etc/nginx/sites-available/default
        owner: root
        group: root
        mode: '0644'
      notify: reload nginx

  handlers:
    - name: reload nginx
      service:
        name: nginx
        state: reloaded

Why each change matters: the apt module is declarative, it checks the package's actual state first, so a rerun is cheap and honest about whether anything changed. notify plus a handler means the service only restarts, specifically via reloaded, which is less disruptive than a full restarted, exactly when the configuration actually changed, not on every run regardless of state.

How to test this in CI (continuous integration). Use Molecule, a testing framework built specifically for Ansible roles, to spin up an ephemeral container and converge (run) the playbook against it, then converge a SECOND time and assert the second run reports zero changed tasks, that's a direct, mechanical test of "is this actually idempotent," rather than trusting it by inspection. Add a verify step, using a tool like Testinfra, asserting the real end state: nginx is installed, the config file has the expected content, and the service is running. Wire this into the CI pipeline so a role that regresses on idempotency fails the build automatically, instead of being caught by a human rerunning it by hand much later.

Trade-offs and pitfalls

reloaded is gentler than restarted, but not every application supports a clean reload; some genuinely need a full restart to pick up certain kinds of configuration changes, so this substitution has to match how the real service actually behaves, not be applied blindly to every service task. Testing idempotency by running the playbook twice in CI adds real time to every pipeline run, a fair cost for something this cheap to verify and this easy to silently break without anyone noticing.

Direct answer

I would not try to find a production memory leak purely by reading code. I'd combine dynamic evidence, actually profiling the running agent under load, with a targeted code-review checklist for the patterns most likely to cause a slow, steady leak, and then lock in whatever I find with a CI check so the same class of bug can't silently come back.

Structured elaboration

Profilers and dynamic evidence. Before trusting any code-review guess, I'd reproduce the leak under a controlled, repeatable workload and watch memory over time: resident set size (RSS, the actual physical memory the process holds) climbing steadily and never coming back down after garbage collection runs is the signature of a real leak, as opposed to a memory spike that a full garbage collection cycle reclaims. For a compiled agent, a built-in heap profiler (Go's pprof, for example) can take a heap snapshot before and after a workload and show exactly which allocations are still retained; for an interpreted agent, an allocation tracer (Python's tracemalloc, for example) does the equivalent by snapshotting live allocations and diffing two points in time.

Code patterns to look for. The question names circular references specifically, and it's worth being precise about when that's actually the cause: in a language with a tracing garbage collector (which reclaims anything unreachable from a root, cycles included), a circular reference on its own is usually NOT what leaks memory, since the collector can free a cycle nobody outside it points to. What actually leaks in that kind of runtime is something still reachable that shouldn't be:

• Unbounded global caches or maps: a cache or dictionary that only ever grows, with no eviction policy or size cap, is one of the most common leak sources in a long-running service, since every entry is reachable from a live root for the life of the process.
• Leaked lightweight concurrent tasks (goroutines, in Go, or the equivalent worker/coroutine in another runtime): a task that's spawned per request or per event but never exits, most often because it's blocked forever waiting on a channel or queue that nothing will ever write to, keeps itself and everything it captured in its closure alive indefinitely. Every hung task is a small, permanent leak.
• Forgotten deregistration: an event listener, callback, or subscription that's added but never removed when the thing that registered it goes away, so a long-lived object (say, a connection pool) keeps growing a list of listeners that should have been cleaned up.
• Circular references DO matter directly in a reference-counted runtime (no tracing collector, or one that only handles simple cases), where two objects each holding a strong reference to the other can prevent the count from ever reaching zero; if the agent embeds any component like that, it's worth checking specifically.

CI checks and metrics that catch regressions early.

• A memory-regression test: run the agent against a fixed, deterministic synthetic workload (a set number of requests or events, not "for N minutes," so the result is reproducible), take a heap snapshot before and after, and fail the build if retained memory grows past a set threshold.
• A task-leak assertion: after the synthetic workload finishes and drains, assert that the number of live background tasks (goroutines, threads, whatever the runtime calls them) has returned to its known baseline count, not just "isn't growing forever."
• Production metrics and alerting: track RSS trend, heap object count, and live background-task count over time, and alert on sustained upward trend rather than a single spike, since a spike that recovers after garbage collection is expected behavior, not a leak.

Worked example

A concrete pattern this would catch: the monitoring agent spawns one background task per incoming metric batch to forward it to an upstream collector over HTTP, and that HTTP call has no timeout or deadline. If the upstream collector stops responding, every task blocked on that call never returns, and each one keeps its metric-batch buffer alive in its closure. Under normal conditions this is invisible, since tasks come and go quickly; the moment the upstream collector gets slow or unresponsive, the agent starts accumulating one leaked task (and its buffer) per batch, forever, which shows up as a slow, steady RSS climb that never plateaus.

The review-time catch: does every code path that spawns a background task per request or event pass it a context or deadline, so a hung downstream call can't block that task forever? The CI catch: a synthetic test that sends, say, 500 metric batches to a fake upstream collector that never responds, then asserts the live background-task count returns to its pre-test baseline (not zero, since some fixed background workers are expected) once the test workload finishes, rather than staying elevated by roughly 500.

Trade-offs and pitfalls

Continuous production profiling has real overhead, so it's usually run on-demand or sampled, not left on all the time. A CI memory-regression gate with an absolute byte threshold is prone to flaking across different CI runner hardware; a relative threshold (percent growth over baseline, measured on the same run) is more stable. The most common wrong turn is fixing the symptom instead of the cause: bounding a growing cache with a size limit stops the crash but, if entries are evicted on a schedule that doesn't match how they're actually used, can just delay the same leak rather than fix it, so the review should ask why the cache grows unbounded in the first place, not just cap it and move on.

Direct answer

I check the code for the things that turn a normal provisioning run into an expensive or throttled one: whether it respects the provider's API rate limits, whether it batches calls instead of making one API call per resource, whether retries use backoff instead of hammering a failing endpoint, whether resource sizes default to something reasonable, and whether re-running the script is safe (idempotent) rather than creating duplicate, billable resources.

Structured elaboration

• Rate-limit awareness. Why it matters: every cloud provider throttles API calls per account or per project, and hitting that limit turns a fast run into a slow one full of failed requests and retries. How I verify it: check whether the client respects the provider's documented per-second or per-minute limit (many SDKs expose this directly), and look for a test that simulates a throttled (HTTP 429) response and confirms the code backs off instead of hammering the endpoint again immediately.
• Batching. Why it matters: creating, updating, or tagging resources one API call at a time multiplies both latency and the chance of hitting a rate limit, when many providers support a bulk operation that does the same work in one call. How I verify it: look for a loop making one API call per resource where a bulk equivalent exists in the provider's API, and check whether the batch size used is close to the provider's documented maximum per call.
• Retry and backoff policy. Why it matters: a naive retry-immediately loop against a struggling API amplifies the problem instead of recovering from it, and racks up cost from repeated attempts. How I verify it: confirm retries use exponential backoff with jitter (randomized delay, to avoid many callers retrying in lockstep) and a hard cap on attempts.
• Resource size defaults. Why it matters: an oversized default (a large instance type where a small one would do) silently inflates the monthly bill for every resource created with that default; an undersized one hurts performance instead. How I verify it: check that instance/resource sizes come from an explicit, reviewed configuration rather than a hardcoded value buried in the script, and look for a cost-linting check (policy-as-code) that flags anything above an agreed tier.
• Idempotence. Why it matters: if re-running the script after a partial failure creates duplicate resources instead of recognizing what already exists, every retry becomes extra, unwanted cost. How I verify it: check that resources are created with a stable, deterministic identifier (a name or tag derived from the input, not a random one) so a second run can detect "this already exists" instead of blindly creating it again, and look for a test that runs the script twice and asserts the second run is a no-op.

Worked example

A provisioning script includes this loop, creating VMs one at a time with a hardcoded large default size:

for name in vm_names:
    client.create_instance(name=name, machine_type="n1-standard-8")

Review comments: no rate-limit handling (a burst of vm_names will start hitting 429s partway through with no backoff), no batching (most providers support a bulk-create call for exactly this case), an oversized hardcoded default (n1-standard-8 for every VM regardless of what it's actually for), and no idempotence check (running this twice after a partial failure creates duplicate VMs for any name that already succeeded). An improved version batches the creates, makes the size a required, reviewed parameter instead of a hardcoded default, and checks for an existing instance with the same name before creating a new one.

Trade-offs and pitfalls

Simulating rate-limit and failure conditions in CI (mocking a 429 response, for example) adds test complexity that a straight-line happy-path test doesn't need, but it's the only way to actually verify backoff behavior rather than assume it works. A common pitfall: treating idempotence as "the script doesn't crash on a second run" when the real bar is "the script doesn't create duplicate billable resources on a second run," which is a stricter and more important guarantee.