Bug: Extend error handling in backend_write_file()#983
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds fault-tolerant error handling to ChangesFile Write Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/executorlib/task_scheduler/file/backend.py`:
- Around line 59-64: The fallback writes the caught exception object
(serialize_error) directly, which may itself be non-serializable; change the
recovery payload to store a serializable string (e.g., traceback.format_exc() or
repr(serialize_error)) instead: in the except block where dump(...) is called
(references: serialize_error, file_name_out, runtime, dump), replace the "error"
value with a string representation of the exception (and optionally include a
short tag/context) so the dump always receives a serializable payload.
In `@tests/unit/task_scheduler/file/test_backend.py`:
- Around line 87-88: The test currently uses a broad with
self.assertRaises(Exception) around future_file_obj.result(), which can hide
other failures; change this to assert the specific exception the serialization
layer throws (e.g., the concrete exception class used when futures from
future_file_obj.result() fail — replace Exception with that type, or use the
stable project-specific base like SerializationError/ValueError/RuntimeError
used elsewhere), and optionally assert the error message contents to make the
expectation explicit; update the test in
tests/unit/task_scheduler/file/test_backend.py to call
self.assertRaises(ExpectedException) around future_file_obj.result() (and add an
assert on str(e) if a message check is needed).
- Around line 82-84: The test calls backend_write_file which first renames the
input "<task>_i.h5" and therefore must create that HDF5 input file before
invoking backend_write_file; update the test setup to create the expected input
HDF file (using the same cache_directory/name pattern used for file_name) so
backend_write_file can find and rename it, then proceed to call
backend_write_file and construct FutureItem as before (reference
backend_write_file and FutureItem to locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de89cb44-b544-46dc-9409-dd4771f602f2
📒 Files selected for processing (2)
src/executorlib/task_scheduler/file/backend.pytests/unit/task_scheduler/file/test_backend.py
| except Exception as serialize_error: | ||
| # Serialization failed — store the error so the job is not stuck | ||
| dump( | ||
| file_name=file_name_out + "_r.h5", | ||
| data_dict={"error": output["error"], "runtime": runtime}, | ||
| data_dict={"error": serialize_error, "runtime": runtime}, | ||
| ) |
There was a problem hiding this comment.
Make fallback error payload always serializable.
At Line 63, writing serialize_error directly can fail again for non-serializable exception objects, which breaks the new recovery path. Persist a string payload (repr/traceback) instead.
Proposed fix
+import traceback
@@
- except Exception as serialize_error:
+ except Exception as serialize_error:
# Serialization failed — store the error so the job is not stuck
dump(
file_name=file_name_out + "_r.h5",
- data_dict={"error": serialize_error, "runtime": runtime},
+ data_dict={
+ "error": f"{type(serialize_error).__name__}: {serialize_error}",
+ "error_traceback": traceback.format_exc(),
+ "runtime": runtime,
+ },
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as serialize_error: | |
| # Serialization failed — store the error so the job is not stuck | |
| dump( | |
| file_name=file_name_out + "_r.h5", | |
| data_dict={"error": output["error"], "runtime": runtime}, | |
| data_dict={"error": serialize_error, "runtime": runtime}, | |
| ) | |
| except Exception as serialize_error: | |
| # Serialization failed — store the error so the job is not stuck | |
| dump( | |
| file_name=file_name_out + "_r.h5", | |
| data_dict={ | |
| "error": f"{type(serialize_error).__name__}: {serialize_error}", | |
| "error_traceback": traceback.format_exc(), | |
| "runtime": runtime, | |
| }, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 59-59: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/executorlib/task_scheduler/file/backend.py` around lines 59 - 64, The
fallback writes the caught exception object (serialize_error) directly, which
may itself be non-serializable; change the recovery payload to store a
serializable string (e.g., traceback.format_exc() or repr(serialize_error))
instead: in the except block where dump(...) is called (references:
serialize_error, file_name_out, runtime, dump), replace the "error" value with a
string representation of the exception (and optionally include a short
tag/context) so the dump always receives a serializable payload.
| file_name = os.path.join(cache_directory, "test_file_i.h5") | ||
| backend_write_file(file_name=file_name, output={"result": Future()}, runtime=0.1) | ||
| future_file_obj = FutureItem( |
There was a problem hiding this comment.
This test does not set up the input file required by backend_write_file.
At Line 83, backend_write_file renames <task>_i.h5 first, so this test can fail with FileNotFoundError before hitting the serialization-error branch. Create the input HDF file in setup for this test.
Proposed fix
def test_backend_write_file_serialization_error(self):
cache_directory = os.path.abspath("executorlib_cache")
os.makedirs(cache_directory, exist_ok=True)
file_name = os.path.join(cache_directory, "test_file_i.h5")
+ dump(file_name=file_name, data_dict={"fn": my_funct, "args": [1], "kwargs": {"b": 2}})
backend_write_file(file_name=file_name, output={"result": Future()}, runtime=0.1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/task_scheduler/file/test_backend.py` around lines 82 - 84, The
test calls backend_write_file which first renames the input "<task>_i.h5" and
therefore must create that HDF5 input file before invoking backend_write_file;
update the test setup to create the expected input HDF file (using the same
cache_directory/name pattern used for file_name) so backend_write_file can find
and rename it, then proceed to call backend_write_file and construct FutureItem
as before (reference backend_write_file and FutureItem to locate the change).
| with self.assertRaises(Exception): | ||
| future_file_obj.result() |
There was a problem hiding this comment.
Avoid blind exception assertion in test.
At Line 87, assertRaises(Exception) is too broad and can mask unrelated regressions. Assert a specific expected exception type (or a stable base type used by your serialization layer).
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 87-87: Do not assert blind exception: Exception
(B017)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/task_scheduler/file/test_backend.py` around lines 87 - 88, The
test currently uses a broad with self.assertRaises(Exception) around
future_file_obj.result(), which can hide other failures; change this to assert
the specific exception the serialization layer throws (e.g., the concrete
exception class used when futures from future_file_obj.result() fail — replace
Exception with that type, or use the stable project-specific base like
SerializationError/ValueError/RuntimeError used elsewhere), and optionally
assert the error message contents to make the expectation explicit; update the
test in tests/unit/task_scheduler/file/test_backend.py to call
self.assertRaises(ExpectedException) around future_file_obj.result() (and add an
assert on str(e) if a message check is needed).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #983 +/- ##
=======================================
Coverage 94.15% 94.16%
=======================================
Files 39 39
Lines 2089 2092 +3
=======================================
+ Hits 1967 1970 +3
Misses 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ltalirz
left a comment
ltalirz
left a comment
There was a problem hiding this comment.
fantastic, thanks @jan-janssen !
Please let me know when a new release with this fix is out
2 participants
Summary by CodeRabbit
Bug Fixes
Tests