Conversation
…(B7, B8) Any unexpected exception between sending a request and confirming it left the science container waiting for a confirmation that never came, and the accepted connection had no timeout, so a dead peer blocked the sidecar forever. The body after send() now runs in a try/finally that always confirms, the connection gets a timeout that defaults to twelve hours and is overridden by SCIENCE_CONTAINER_TIMEOUT, and an empty status is reported instead of being ignored. A science container failure now posts a failure record for the file and sends SIGTERM to the worker parent instead of exiting the pool child, which celery would have replaced with one holding the same dead connection, and the task rejects a message lost with the worker so it is redelivered. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
… (B20) put_file_complete ignored the response, so a rejected or failed update was logged as a success and the file was never retried, and it used a half second connect timeout with no read timeout, which made a slow app hang the transform forever. The response is now checked with raise_for_status and the call uses a finite (5, 60) connect and read timeout. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
… (B21) hash_path trimmed a long file name to exactly the 255 byte NAME_MAX limit, so when the sidecar later appended .parquet or .rntuple.root to it the conversion failed with a name too long error. The cap now reserves the length of the longest extension the sidecar can add. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
initialize_logging called basicConfig with force=True and then added its own stream handler, so the root logger ended up with two handlers and every message appeared twice, once unformatted. The basicConfig call is dropped and any handler already installed is removed before the formatted one is added. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
…orm (B23) A failed conversion left the source ROOT file, a stray temp.parquet, or a half written rntuple file in the scratch directory, and a science container failure left its partial output there for the next replica to trip over, so a retried file could fill the volume or upload garbage. Each of those paths now removes the file it is responsible for. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
…e (B25) The sidecar only created its listening socket after the capabilities file appeared, so a science container that started first had its connection refused, and watch.sh gave up after a single attempt one second in. The socket is now bound and listening as soon as init runs, with the wait for the peer moved into a separate accept call, and watch.sh retries its nc connection sixty times, overridable with SIDECAR_CONNECT_RETRIES. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
requests is imported by servicex_adapter at runtime but was only declared in the optional test group, so it was installed in the image by accident through another package. It moves to the main dependencies, and the lock file records requests and its transitive dependencies in the main group. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
put_file_complete was the only user of the unmaintained retry package, while the sidecar already depends on tenacity for the same job. The call site now uses tenacity's Retrying, which also covers the response check, so a failed status is retried like a connection error, and retry along with its decorator and py dependencies is dropped from the project and the lock file. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
upload_file built a new ObjectStoreManager for every file, which opened a fresh minio client and connection pool per upload even though init already keeps one in the object_store global. The global is used instead. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
The celery_app global was assigned in init and never read, the module level start_time was shadowed by the per file value inside transform_file, and the hard failure path tested transform_success twice in a row. All three are removed and the two blocks are merged. Refs ssl-hep#1539 Assisted-by: claude-code:claude-opus-5
ariostas
force-pushed
the
fix/sidecar-failure-handling
branch
from
September 9, 2026 14:10
459d0fd to
3d5f0f2
Compare
This branch has not been deployed
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.
🤖 AI text below 🤖
The transformer sidecar and the science container talk over a socket where every request has to be answered with a confirmation. Today any unexpected exception in the sidecar skips that confirmation and both sides sit there forever, and the one failure path that does not hang loses the message instead of reporting it. This PR makes those paths report a failure and keep the protocol in sync, tightens the startup handshake, and fixes a handful of smaller sidecar issues found alongside them.
How to review. Each finding is one commit; the Commits tab shows them in severity order. Each entry below links to its commit; tick approve or reject under it. To ask for a change instead, leave a review comment on the commit. Rejected commits will be dropped from the branch and this list will be updated to match.
a88d206· Always confirm the request in atry/finally, time out the socket, report a science container failure and take the whole worker down instead of the pool child. Both findings restructure the same loop, so they share one commit53bf213·raise_for_status()on the file-complete response and a finite(5, 60)timeoute7e37ce·hash_pathleaves room for the extension a sidecar conversion appends9d81033· Clear existing handlers instead ofbasicConfig(force=True), so every line is logged onceebd3a2e· Remove the source file,temp.parquet, a partial rntuple, and a partial science container outputd8ab94d· Bind and listen before reading the capabilities file, and retry thencconnection inwatch.shb01f74c·requestsmoves from the optional test group to the main dependencies459064e·retry_callreplaced by tenacity, which the sidecar already depends on814c631·upload_filereuses theObjectStoreManagerbuilt ininit()3d5f0f2· Unusedcelery_appglobal, a duplicatedstart_time, and two consecutive identicalif not transform_success:blocksSkipped
transformer_argument_parser.py, which is out of scope for this PR.Follow-ups noticed, out of scope
check=Trueon themvin the TopCPToolkit template, is the upstream trigger for B7: the science container reportssuccess.and the sidecar then raises on the missing file. It is fixed in the code generators PR, not here.transformer_sidecar/DockerfilepinsPOETRY_VERSION=2.1.1whilepoetry.lockis generated by Poetry 2.4.x, so the image installs from a lock file written by a newer Poetry than it runs.Part of #1539.