Serialize simulation CSV writes behind a recorder (#10)#76
Open
ff225 wants to merge 1 commit into
Open
Conversation
The simulation CSV handle, its writer and its row counter were class-level attributes on RequestHandler, opened and closed from ASGI request threads while the background I/O thread wrote rows through them. Three races followed: - `close_simulation_csv` cleared `csv_file` while the I/O thread was inside `_write_csv_row`, so a queued row could reach a closed file and raise "I/O operation on closed file"; - `_write_csv_row` tested `csv_writer` and then used `csv_file`, with nothing keeping the two consistent across the gap; - `inference_counter += 1` ran on concurrent request threads, and `set_simulation_csv` reset it underneath them, so ids could collide. InferenceCycleRecorder owns all of it behind one lock: the check and the write happen together, a row that arrives after a close is dropped and reported rather than written, and row ids are handed out atomically. A row that fails to serialize is now logged and swallowed instead of propagating into the background I/O worker. The debug-JSON writing moves here too, since it shares the same background thread and per-server-start lifecycle. `RequestHandler.set_simulation_csv` / `close_simulation_csv` stay as delegating classmethods; `http_server` is unchanged. Second step of #45.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10. Second step of #45; unblocked now that #9 has landed.
Why
The simulation CSV handle, its writer and its row counter were class-level attributes on
RequestHandler, opened and closed from ASGI request threads while the background I/O thread wrote rows through them. Three concrete races:close_simulation_csvclearedcsv_filewhile the I/O thread was inside_write_csv_row— a queued row could reach a closed file and raiseI/O operation on closed filerequest_handler.py:450-454, 488-494_write_csv_rowtestedcsv_writer, then usedcsv_file; nothing kept the two consistent across the gaprequest_handler.py:452-454inference_counter += 1ran on concurrent request threads, andset_simulation_csvreset it underneath them, so ids could colliderequest_handler.py:482, 791What changed
New
src/server/communication/inference_recorder.py.InferenceCycleRecorderowns the CSV handle, the writer and the counter behind one lock: the check and the write happen together, a row arriving after a close is dropped and reported rather than written, and row ids are handed out atomically.Failures are contained. A row that fails to serialize is logged and swallowed instead of propagating into the background I/O worker. A failed open no longer leaves a half-open handle behind.
Debug-JSON writing moves here too, since it shares the same background thread and the same per-server-start lifecycle.
No external change.
RequestHandler.set_simulation_csv/close_simulation_csvremain as delegating classmethods, sohttp_server.py:272,289andscripts/simulation/simulation_runner.pyare untouched.Testing
254 passed, 1 skipped (fast suite + HTTP end-to-end).
inference_recorder.pyanddevice_state.pyboth at 100% coverage. Newtests/unit/test_inference_recorder.pyincludes two concurrency tests that reproduce the races above: 8 writer threads against a thread that repeatedly closes and reopens, and 8 threads racing for row ids. ruff clean.Note for the reviewer
The CSV schema declares
min_device_time,max_device_time,min_edge_timeandmax_edge_time, but the request handler has never supplied them —DictWriterfills them with the empty string, so those four columns are always blank in every simulation CSV produced so far. This PR preserves that behaviour rather than quietly changing the data (the values are all available at the call site if you want them filled — say the word and it is a three-line follow-up).