[duclucky]

Summary

• snapshot frame-local values before scanning so trace/debug hooks cannot resize the live dictionary during iteration
• use tuple(f_locals.values()) to avoid allocating key/value pairs, while preserving the existing request and req fast paths
• add a FastAPI regression test that mutates frame.f_locals from a trace hook after iteration starts and restores the previous tracer

Fixes #89

Testing

• Python 3.10 on Linux: python -m pytest --ignore tests/smoketests -q (98 passed)
• Python 3.10 on Linux: backend=fastapi python -m pytest tests/smoketests/test_run_smoketest.py -q (6 passed)
• Python 3.12 on Windows: python -m pytest --ignore tests/smoketests --ignore tests/test_gunicorn_flask.py -q (95 passed; Gunicorn is covered by the Linux run)
• python -m flake8 json_logging tests --count --select=E9,F63,F7,F82 --show-source --statistics (passed)
• python -m build --no-isolation (sdist and wheel built)
• uv pip check --python .venv/Scripts/python.exe (passed)
• Regression check: removing the snapshot reproduces RuntimeError: dictionary changed size during iteration; restoring it passes

Summary by CodeRabbit

• Bug Fixes

  • Improved request inspection reliability when local variables change during stack lookup.
  • Avoided errors from changes to frame-local values while examining request data.

• Tests

  • Added regression coverage for request lookup when local variables are modified during runtime tracing.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: e115a1c1-4d4c-4a07-b3e1-7c5582d47245

📥 Commits

Reviewing files that changed from the base of the PR and between 71fe7d6 and ecf03e8.

📒 Files selected for processing: 2

• json_logging/util.py
• tests/test_fastapi.py

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📝 Walkthrough

Request stack inspection now iterates over a tuple snapshot of frame-local values. A FastAPI regression test checks behavior when tracing mutates those locals.

Changes: Request stack safety

Layer / File(s)	Summary
Snapshot frame locals and validate mutation handling json_logging/util.py, tests/test_fastapi.py	Request discovery iterates over a tuple snapshot of frame-local values. The regression test installs a tracer, mutates frame locals, checks that lookup returns None, restores the previous tracer, and verifies that the mutation occurred. The path-pattern matching expression is unchanged.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: suppagoddo, jhermann

Merge Risk: ⚪ Minimal · up to ecf03

The request-lookup fix and its regression test are ready to merge after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: fixing request lookup when frame locals mutate.
Linked Issues check		Issue #89 is closed and marked as historical context only. No active directly linked targets remain, so no linked-issue coding requirements apply. For context, the reviewed head changes `RequestUtil.g…
Out of Scope Changes check		The whole-PR diff from merge base 79d4d38 to head ecf03e8 changes three areas. (1) json_logging/util.py: the frame-local scan now iterates a snapshot tuple of values. This is the direct fix for #89.…
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.

Full details: Linked Issues check

Explanation

Issue #89 is closed and marked as historical context only. No active directly linked targets remain, so no linked-issue coding requirements apply. For context, the reviewed head changes RequestUtil.get_request_from_call_stack in json_logging/util.py. The loop now scans tuple(f_locals.values()) instead of iterating the live f_locals dictionary by key. This removes the RuntimeError: dictionary changed size during iteration path reported in #89. The request and req checks run before the loop and are unchanged. The loop still returns the first instance of class_type. Values under the request and req keys that do not match class_type produce the same result in the loop as before, because the same value is tested again and fails again.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff from merge base 79d4d38 to head ecf03e8 changes three areas. (1) json_logging/util.py: the frame-local scan now iterates a snapshot tuple of values. This is the direct fix for #89. (2) tests/test_fastapi.py: a regression test was added. The PR summary and raw summary state that it mutates frame.f_locals from a trace hook and restores the previous tracer. This supports the fix and is in scope. I could not read the full test text because the repository output was truncated, so I did not verify its exact contents. (3) json_logging/util.py: the file now ends with a newline. This is a formatting-only change and is not a concern. No unrelated functional change was found in the inspected diff.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Snapshot frame locals during request lookup to avoid mutation errors

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Snapshot frame.f_locals.items() before scanning to avoid dict-resize during iteration.
• Reuse snapshotted values during request-type checks for consistent inspection.
• Add FastAPI regression test that mutates frame.f_locals via sys.settrace().

Diagram

graph TD
  A["RequestUtil.get_request_from_call_stack()"] --> B{"Iterate frame locals"} --> C["Snapshot: tuple(f_locals.items())"] --> D["Type-check values"] --> E["Return request or None"]
  T["Trace hook (sys.settrace)"] --> F["Mutates frame.f_locals"]
  F --> C
  subgraph Legend
    direction LR
    _fn["Function"] ~~~ _dec{"Decision"} ~~~ _test["Test/Hook"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Iterate over list(f_locals) and re-index values

• ➕ Avoids 'dict changed size' by snapshotting keys only
• ➕ Slightly less memory than snapshotting (key, value) pairs
• ➖ Still reads values from the live dict, so values could change between key snapshot and lookup
• ➖ Extra dict lookup per key

2. Copy locals dict via f_locals.copy()

• ➕ Clear intent: stable snapshot of locals
• ➕ Provides both keys and values consistently
• ➖ Potentially higher overhead than tuple(items()) (creates a full dict)
• ➖ May differ in subtle ways if code relies on dict identity/ordering (unlikely here)

Recommendation: Keep the current approach (tuple(f_locals.items())) because it provides a stable, minimal snapshot (including values) and avoids additional live-dict reads during type checks. The added regression test using sys.settrace() is a strong guard against reintroducing the mutation-during-iteration failure mode.

Files changed (2) +39 / -4

Bug fix (1) +7 / -4

util.pySnapshot frame locals items to prevent mutation during stack inspection +7/-4

Snapshot frame locals items to prevent mutation during stack inspection

• Changes frame-local scanning to iterate over 'tuple(f_locals.items())' and reuse the snapshotted value when checking request types, preventing failures when tracing/debug hooks mutate 'f_locals' mid-iteration. Also normalizes the file to include a trailing newline.

json_logging/util.py

Tests (1) +32 / -0

test_fastapi.pyAdd regression test for frame locals mutation during request lookup +32/-0

Add regression test for frame locals mutation during request lookup

• Adds a FastAPI-focused regression test that installs a trace function via 'sys.settrace()' and mutates 'frame.f_locals' after iteration begins inside 'get_request_from_call_stack()'. Verifies the lookup completes without error and that the mutation path was exercised.

tests/test_fastapi.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Extra locals snapshot allocations 🐞 Bug ➹ Performance

Description

get_request_from_call_stack() now allocates tuple(f_locals.items()) for every inspected frame,
creating an O(n) tuple of 2-tuples per frame walked. This increases CPU/memory overhead for
correlation-id lookup during log formatting in FastAPI, where the library falls back to stack
scanning (no global request object).

Code

json_logging/util.py[190]

+            for key, value in tuple(f_locals.items()):

Evidence

The PR introduces a per-frame tuple(f_locals.items()) allocation during stack scanning. This scan
is triggered during log formatting when correlation-id is missing, and in FastAPI the library
explicitly cannot rely on a global request object, making stack scanning more common.

json_logging/util.py[180-200]
json_logging/formatters.py[141-154]
json_logging/util.py[124-152]
json_logging/framework/fastapi/implementation.py[61-69]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RequestUtil.get_request_from_call_stack()` snapshots `tuple(f_locals.items())`, which allocates a tuple plus a 2-tuple per local entry for every frame visited.

## Issue Context
This codepath is exercised from `JSONLogWebFormatter` via `request_util.get_correlation_id(within_formatter=True)`. For FastAPI, `support_global_request_object()` is `False`, so correlation-id lookup may frequently fall back to scanning the call stack.

## Fix Focus Areas
- json_logging/util.py[180-200]

### Suggested approach
Preserve mutation-safety but reduce allocations by snapshotting only values:
- Keep the existing fast-path checks for `request` and `req`.
- Replace `for key, value in tuple(f_locals.items()): ...` with `for value in tuple(f_locals.values()): ...` (and drop the key filter), since you only need to detect any local whose value is an instance of `class_type`.
- Ensure the regression test still passes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[llamapreview]

LlamaPReview — No blocking issues found

This PR safely fixes the RuntimeError by snapshotting frame locals before scanning, with a regression test that fails on the old code and no PR-caused correctness or security regressions found.

1 non-blocking finding retained — highest: Optional: per-frame tuple allocation in logging hot path.

Review details and evidence

Priority	File	Finding	Evidence
P2	json_logging/util.py	Optional: per-frame tuple allocation in logging hot path	needs verification

Finding details

P2 · Optional: per-frame tuple allocation in logging hot path

json_logging/util.py

The fix snapshots frame locals with tuple(f_locals.items()), adding a new O(n) allocation per scanned frame. The old loop already did O(n) work per frame, and typical frame-local counts are small; there is no evidence this is a material regression. Profiling of high-volume logging would determine if a follow-up optimization is warranted; this is not a merge blocker.

Owner action: Optionally profile high-volume logging to assess the tuple allocation cost; only optimize if a regression is measured.

Verification boundary: needs verification; scope: changed region.

Material unknowns

• Exact-head CI evidence for pytest/flake8/build is not independently present; only CodeRabbit status is available. Would confirm the change passes full CI rather than only statically analyzed; does not change the clear verdict.
  • Check: Confirm CI run results on the final commit before merge for completeness.

LlamaPReview checks

• Read the complete PR-head file json_logging/util.py.
• Read the complete PR-head file tests/test_fastapi.py.

---

LlamaPReview reviewed this pull request at its exact head commit. Inspect the source or share feedback.

[suppagoddo]

@duclucky - Can you please rebase ?

[suppagoddo]

@duclucky - Would it be possible for you to rebase and address the qodo-code-review ... ?

[suppagoddo]

Thanks for addressing the requested changes, it now looks good.