Skip to content

fix(tools): report deeply nested progress snapshots as usage errors - #12602

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
xXPBSXx:tools/progress-compare-recursion
Oct 29, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
xXPBSXx:tools/progress-compare-recursion

Conversation

@xXPBSXx

@xXPBSXx xXPBSXx commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

What

progress.py --compare reads the two snapshots with json.loads and reports an unreadable one as a usage error, but only for OSError and ValueError. A snapshot nested deeper than the interpreter's recursion limit raises RecursionError, which is neither, so the tool printed a traceback and exited with 1 instead of the usage error and exit code 2 that every other bad snapshot gets. This catches RecursionError there as well, the way performance_compare does after #12266, and adds a test that feeds a deeply nested file in either position.

Tested

  • Windows 10 Pro, Python 3.12.10: the new test fails on main 2199ee0d47 with RecursionError and passes with this change (head 3d92bfa50d); all 47 tests of tools/tests/test_progress.py pass.
  • Ubuntu 24.04 under WSL2 (a virtualised kernel, not bare-metal), Python 3.12.3: the same test fails on main 2199ee0d47 and passes with this change; 47 of 47 pass.
  • check_conventions --base upstream/main passes.
  • Not run: macOS.

Overlap

Four open PRs change the same files: #12492 and #10992 (both tools/progress.py and tools/tests/test_progress.py), #12588 (those two and docs/dev/PROGRESS.md) and #12540 (tools/tests/test_progress.py). I took the changed files of each of the 1346 open PRs open at the first pass from git diff --name-only against its merge base, and of the 47 opened since from the pull request file lists. A git merge-tree of each of the four against this branch is clean, and so is each of them alone against main 319621ba2e, so none is a real pair and none needs a Depends on. No open or merged PR or issue deals with RecursionError in progress.py; #12355 and #12501 change progress_history.py.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

AI-assisted (Claude Code).

@adriadam10 adriadam10 self-assigned this Oct 28, 2026

@rooklz rooklz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First macOS run (arm64, head 3d92bfa50d applied to main 3a2b919816). The code change is correct, but the new test fails on Python 3.14 and later.

Results of python3 -m unittest test_progress

Python head main progress.py + new test
3.9.6 new test OK (1 unrelated hardlink_to error, already on main) RED, 2 RecursionError
3.12.13 / 3.13.13 49/49 OK RED, 2 RecursionError
3.14.8 / 3.15.1 FAIL ×2: 'recursion' not found in '... base snapshot.json: snapshot must be an object' —

Python 3.14 changed the C JSON decoder so it checks for real stack exhaustion instead of sys.getrecursionlimit(). On 3.14/3.15 here, json.loads("[" * 100000 + "]" * 100000) parses. The input then reaches validate_snapshot and is reported as "snapshot must be an object". That is already rc 2 with no traceback, so the tool behaves correctly, but the assertIn("recursion", ...) assertion fails. At 10^6 levels both versions raise RecursionError, with different messages: 3.13 says maximum recursion depth exceeded while decoding a JSON array… and 3.14 says Stack overflow (used 16352 kB) while decoding a JSON array…. A fixed depth or a fixed message therefore can't be portable. CI's Ubuntu python3 is 3.12, so CI won't show this. Contributors on Homebrew or a current python.org install will.

tools/tests/test_performance_compare.py:33 (excessive_json_value, #12266) already solves this by probing for a depth that actually raises. Proposed test-only diff:

--- a/tools/tests/test_progress.py
+++ b/tools/tests/test_progress.py
@@ -10,6 +10,17 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
 import progress
 
 
+def excessive_nesting():
+    depth = sys.getrecursionlimit() + 64
+    for _ in range(8):
+        try:
+            json.loads("[" * depth + "]" * depth)
+        except RecursionError:
+            return "[" * (depth + 128) + "]" * (depth + 128)
+        depth *= 2
+    raise AssertionError("JSON decoder accepted every bounded nesting probe")
+
+
 class ProgressSnapshotTests(unittest.TestCase):
     def test_library_counters_reject_invalid_types_and_negative_values(self):
         for field in ("done", "todo"):
@@ -65,7 +76,7 @@ class ProgressSnapshotTests(unittest.TestCase):
                 paths = [Path(directory) / "base snapshot.json", Path(directory) / "head snapshot.json"]
                 for path in paths:
                     path.write_text(json.dumps(self.snapshot()), encoding="utf-8")
-                paths[position].write_text("[" * 100000 + "]" * 100000, encoding="utf-8")
+                paths[position].write_text(excessive_nesting(), encoding="utf-8")
                 stdout, stderr = io.StringIO(), io.StringIO()
                 with patch("sys.stdout", stdout), patch("sys.stderr", stderr):
                     with self.assertRaises(SystemExit) as raised:
@@ -73,7 +84,8 @@ class ProgressSnapshotTests(unittest.TestCase):
                 self.assertEqual(raised.exception.code, 2)
                 self.assertEqual(stdout.getvalue(), "")
                 self.assertIn(str(paths[position]), stderr.getvalue())
-                self.assertIn("recursion", stderr.getvalue())
+                self.assertNotIn("must be an object", stderr.getvalue())
+                self.assertNotIn("Traceback", stderr.getvalue())
 
     def test_non_object_snapshots_are_usage_errors(self):
         for snapshot in (None, [], "progress", 7, True):

The "must be an object" check makes sure the input fails while decoding and not in validate_snapshot. Without it, a decoder that accepts the input would pass the test vacuously.

With the diff applied: ProgressSnapshotTests OK on 3.9.6, 3.12, 3.13, 3.14 and 3.15. Main progress.py + the fixed test is RED (2 errors) on 3.9.6, 3.13, 3.14 and 3.15. The drop-RecursionError mutant is killed on 3.14. The test takes 0.09 s. check_conventions --base upstream/main is clean.

AI-assisted review (Claude Code).

xXPBSXx and others added 2 commits October 29, 2026 01:56
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…thon

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@adriadam10
adriadam10 force-pushed the tools/progress-compare-recursion branch from 3d92bfa to c76feb5 Compare October 29, 2026 00:59
@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Windows run of this head (c76feb5836), merged onto main 52c09d567d (clean merge, local only). Windows 11 Enterprise 10.0.22621, Python 3.15.1, python -m unittest tools/tests/test_progress.py; this machine can create symbolic links.

  • This PR: 49 tests pass, 3 of 3 runs.
  • Negative control: with tools/progress.py put back to main's version and this PR's tests kept, the new test test_deeply_nested_snapshots_are_usage_errors fails. The other test definitions this diff adds or moves pass with main's code as well: test_non_object_snapshots_are_usage_errors.

AI-assisted: yes (Claude Code).

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed c76feb5836f72811f8b3afb7f53e647ec2960882 against 3a2b91981651569163768854a1fcdc7193b6667b. Both progress snapshot inputs translate decoder recursion failures into path-specific CLI usage errors before comparison/report generation. No actionable finding in this scoped review.

Windows/Python 3.11 validation: head 49 passed, 0 skipped; baseline 48 passed, 0 skipped. The deep-snapshot controls raise RecursionError on old production and pass on the head. Input/report preservation is covered; no change to scanner counting or title behavior is claimed.

@adriadam10
adriadam10 merged commit fdd1753 into boykopovar:main Oct 29, 2026
13 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the RecursionError catch for both --compare inputs and the reworked test (depth probed on the running Python); confirmed by @JJenkx on Windows/Python 3.15.1 (49 pass, negative control fails) and @THEMANnt on 3.11; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants