)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7d9034d8d764a4a65d6b34ee3fd2ea2fd4108d18","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1c8a51e7_7dc08bd0","updated":"2026-09-21 21:50:16.000000000","message":"Threw an idea to (maybe?) simplify in https://review.opendev.org/c/openstack/swift/+/1006659\n\nI\u0027m still a little worried about fragments that are an exact multiple of `fragment_size` -- we might need to start doing our own byte-counting as we process responses.","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy","status":"I\u0027m a Software Engineer at Red Hat and I love Open Source and connect with people! Feel free to DM through IRC, I\u0027ll be delighted to chat"},"change_message_id":"79fa5b5a32d4d8d5ca9d6c0d4a1c5044fe3fac82","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"89f54ab2_a4d2fa83","in_reply_to":"1c8a51e7_7dc08bd0","updated":"2026-09-22 11:02:51.000000000","message":"Thanks Tim. The boundary question turned out to be a real bug, so patch set 2 is bigger than a cleanup.\n\nI squashed your 1006659, then added byte-counting on top of it, because the exact-multiple case defeats patch set 1 and 1006659 both. Details inline. What changed:\n\n- The check is inline in `_get_one_fragment()` as you suggested, but it records the mismatch on the response instead of raising. See the inline reply for why.\n- `_handle_fragment_response()` stashes the donor\u0027s Content-Length next to the hasher, and the check fires as soon as `frag_bytes_read` reaches it, rather than waiting for a short read.\n- A donor that stops short of its Content-Length isn\u0027t checked. No Content-Length at all falls back to checking at EOF.\n- New test for an archive that ends exactly on a `fragment_size` boundary. The existing mismatch test now also asserts that an incomplete fragment reached the wire, not just that an exception came out.\n\nI checked this on a SAIO with a 4+2 policy, by corrupting two donor archives in the payload of their last fragment and deleting a third frag. On patch set 1 the 2 MiB object came back with frag#0 rebuilt from a corrupt donor and committed  and then frag#2 rebuilt from that corrupt frag#0 in the same pass. One bit flip, two bad fragments, each with an ETag matching its own contents, so nothing will flag them later. On patch set 2 the frag isn\u0027t written and ssync aborts.\n\nOne thing I left out: a rebuild that trips this check isn\u0027t retried with a different donor set. With one corrupt donor out of five, a retry would recover the fragment instead of failing. That changes when rebuilds succeed rather than whether they\u0027re correct, so I think it belongs in its own patch. Happy to write it if you agree.","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"}],"swift/obj/reconstructor.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7d9034d8d764a4a65d6b34ee3fd2ea2fd4108d18","unresolved":true,"context_lines":[{"line_number":729,"context_line":"                if not chunk:"},{"line_number":730,"context_line":"                    # this donor has sent its whole fragment archive,"},{"line_number":731,"context_line":"                    # so its hash is complete and can be verified"},{"line_number":732,"context_line":"                    resp.frag_complete \u003d True"},{"line_number":733,"context_line":"                    break"},{"line_number":734,"context_line":"                remaining_bytes -\u003d len(chunk)"},{"line_number":735,"context_line":"                resp.frag_hasher.update(chunk)"}],"source_content_type":"text/x-python","patch_set":1,"id":"29ed352f_ef30481a","line":732,"updated":"2026-09-21 21:50:16.000000000","message":"We can\u0027t just do a `if resp.frag_hasher.hexdigest() !\u003d resp.frag_etag` check here? Why the new function?","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy","status":"I\u0027m a Software Engineer at Red Hat and I love Open Source and connect with people! Feel free to DM through IRC, I\u0027ll be delighted to chat"},"change_message_id":"79fa5b5a32d4d8d5ca9d6c0d4a1c5044fe3fac82","unresolved":true,"context_lines":[{"line_number":729,"context_line":"                if not chunk:"},{"line_number":730,"context_line":"                    # this donor has sent its whole fragment archive,"},{"line_number":731,"context_line":"                    # so its hash is complete and can be verified"},{"line_number":732,"context_line":"                    resp.frag_complete \u003d True"},{"line_number":733,"context_line":"                    break"},{"line_number":734,"context_line":"                remaining_bytes -\u003d len(chunk)"},{"line_number":735,"context_line":"                resp.frag_hasher.update(chunk)"}],"source_content_type":"text/x-python","patch_set":1,"id":"4f283613_02806a38","line":732,"in_reply_to":"29ed352f_ef30481a","updated":"2026-09-22 11:02:51.000000000","message":"You\u0027re right, and patch set 2 does it inline. One thing came up on the way there that\u0027s worth mentioning.\n\nThe reason it was a separate function was the blanket `except (Exception, Timeout)` a few lines down. A raise inside `_get_one_fragment()` comes back to the consumer inside that try, and the handler turns it into a break, a silent end of stream, which is the thing this patch is trying to remove. A dedicated exception caught ahead of it fixes that, which is what 1006659 does. So I squashed it and ran the result on a SAIO.\n\nThat\u0027s where I hit a problem. `_get_one_fragment()` runs in a GreenPile greenthread. Eventlet passes the exception to whoever is waiting on the result and also re-raises it; `hub.fire_timers()` catches that second copy and `squelch_timer_exception()` prints it. So every corrupt donor produced this:\n\n```\n object-reconstructor-6010: STDERR: Traceback (most recent call last):\n   File \".../eventlet/hubs/hub.py\", line 471, in fire_timers\n   File \".../eventlet/greenthread.py\", line 272, in main\n   File \".../swift/obj/reconstructor.py\", line 786, in _get_one_fragment\n swift.obj.reconstructor.ETagMismatchException: Invalid ETag for frag#2 ...\n```\n\nfollowed by the same message logged properly, and then again from ssync. Patch set 1 didn\u0027t do this, because its raise was in the consumer. A condition we handle on purpose showing up as a traceback in the reconstructor log seemed like a bad trade for the tidier code.\n\nSo patch set 2 keeps the check inline where you wanted it, but records the message on the response instead of raising. The consumer reads the records once the pile completes, before it rebuilds anything. Nothing goes on the wire between those two points, so the ordering is the same. With no raise crossing a greenthread boundary, the helper function and the exception class both go away.\n\nThe unit tests don\u0027t catch this, for what it\u0027s worth, the hub isn\u0027t involved when the pile is driven synchronously under test. It only showed up on a real cluster.","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7d9034d8d764a4a65d6b34ee3fd2ea2fd4108d18","unresolved":true,"context_lines":[{"line_number":791,"context_line":"                # ends. A donor reaches EOF during the read that"},{"line_number":792,"context_line":"                # produced the payload we are about to rebuild from,"},{"line_number":793,"context_line":"                # and once the rebuilt fragment has been yielded ssync"},{"line_number":794,"context_line":"                # has already put it on the wire -- the receiver"},{"line_number":795,"context_line":"                # finalises on byte count and is not waiting for"},{"line_number":796,"context_line":"                # anything further from the sender."},{"line_number":797,"context_line":"                _verify_fragment_etags()"},{"line_number":798,"context_line":"                if not all(fragment_payload):"}],"source_content_type":"text/x-python","patch_set":1,"id":"d817de45_099156d0","line":795,"range":{"start_line":794,"start_character":52,"end_line":795,"end_character":41},"updated":"2026-09-21 21:50:16.000000000","message":"So... what if the fragment ends on a `fragment_size` boundary?","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy","status":"I\u0027m a Software Engineer at Red Hat and I love Open Source and connect with people! Feel free to DM through IRC, I\u0027ll be delighted to chat"},"change_message_id":"79fa5b5a32d4d8d5ca9d6c0d4a1c5044fe3fac82","unresolved":true,"context_lines":[{"line_number":791,"context_line":"                # ends. A donor reaches EOF during the read that"},{"line_number":792,"context_line":"                # produced the payload we are about to rebuild from,"},{"line_number":793,"context_line":"                # and once the rebuilt fragment has been yielded ssync"},{"line_number":794,"context_line":"                # has already put it on the wire -- the receiver"},{"line_number":795,"context_line":"                # finalises on byte count and is not waiting for"},{"line_number":796,"context_line":"                # anything further from the sender."},{"line_number":797,"context_line":"                _verify_fragment_etags()"},{"line_number":798,"context_line":"                if not all(fragment_payload):"}],"source_content_type":"text/x-python","patch_set":1,"id":"996e8543_1c0e21fb","line":795,"range":{"start_line":794,"start_character":52,"end_line":795,"end_character":41},"in_reply_to":"d817de45_099156d0","updated":"2026-09-22 11:02:51.000000000","message":"You\u0027re right, and neither patch set 1 nor 1006659 handles it.\n\n`_get_one_fragment()` fills `remaining_bytes` exactly and then exits on the while remaining_bytes condition, so there\u0027s no short read on that pass. EOF turns up on the next pass, after the last rebuilt fragment has been yielded, after ssync has written content_length bytes, and the receiver finalises on byte count. So the fragment is already committed by the time the exception fires.\n\nIt isn\u0027t rare, either. The archive is an exact multiple of `fragment_size` whenever the object is an exact multiple of `ec_segment_size`, so every 1 MiB object under the default segment size.\n\nPatch set 2 does the byte-counting you suggested. `_handle_fragment_response()` stashes the donor\u0027s Content-Length next to the hasher, `_get_one_fragment()` counts what it has hashed, and the check fires when `frag_bytes_read` reaches `frag_length`, inside the pass that produced the payload we\u0027re about to rebuild from.\n\nTwo cases I left unchecked on purpose:\n\n- A donor that hits EOF short of its Content-Length. Its hash only covers part of the archive, so a mismatch there would be misleading, and a truncated stream is already caught by the receiver\u0027s content-length check. `test_reconstruct_fa_short_donor_no_etag_error` covers this.\n- No Content-Length at all falls back to checking at EOF, i.e. patch set 1 behaviour.\n\nThe new test is `test_reconstruct_fa_donor_frag_etag_mismatch_on_boundary`, with the object sized to exactly two segments and a bit flipped in the donor\u0027s last fragment. Worth saying why it asserts what it does: patch set 1 still raises on this input, just too late, so a test that only checks the exception type passes on the broken code. What fails on patch set 1 is the byte count, the test collects what the reader emitted and asserts it\u0027s less than a whole archive, i.e. that the receiver never got something it could finalise. I added the same assertion to the existing mismatch test.","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"}],"test/unit/obj/test_reconstructor.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7d9034d8d764a4a65d6b34ee3fd2ea2fd4108d18","unresolved":true,"context_lines":[{"line_number":147,"context_line":"    frag_index \u003d struct.unpack(\u0027h\u0027, metadata[:2])[0]"},{"line_number":148,"context_line":"    return {"},{"line_number":149,"context_line":"        \u0027X-Object-Sysmeta-Ec-Frag-Index\u0027: frag_index,"},{"line_number":150,"context_line":"    }"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"@patch_policies([StoragePolicy(0, name\u003d\u0027zero\u0027, is_default\u003dTrue),"}],"source_content_type":"text/x-python","patch_set":1,"id":"0d5e5f4f_ee62d482","line":150,"updated":"2026-09-21 21:50:16.000000000","message":"Off-topic: I wonder if this should have `Etag` included, too.","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy","status":"I\u0027m a Software Engineer at Red Hat and I love Open Source and connect with people! Feel free to DM through IRC, I\u0027ll be delighted to chat"},"change_message_id":"79fa5b5a32d4d8d5ca9d6c0d4a1c5044fe3fac82","unresolved":true,"context_lines":[{"line_number":147,"context_line":"    frag_index \u003d struct.unpack(\u0027h\u0027, metadata[:2])[0]"},{"line_number":148,"context_line":"    return {"},{"line_number":149,"context_line":"        \u0027X-Object-Sysmeta-Ec-Frag-Index\u0027: frag_index,"},{"line_number":150,"context_line":"    }"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"@patch_policies([StoragePolicy(0, name\u003d\u0027zero\u0027, is_default\u003dTrue),"}],"source_content_type":"text/x-python","patch_set":1,"id":"5de00f26_c512f3e9","line":150,"in_reply_to":"0d5e5f4f_ee62d482","updated":"2026-09-22 11:02:51.000000000","message":"I looked at this too, and I don\u0027t think it\u0027s needed.\n\n`FakeConn.getheaders()` synthesises etag from the body it\u0027s actually serving whenever the test doesn\u0027t supply one. So every mocked reconstruct test already sends a correct per-fragment ETag, and they\u0027re all exercising the new verification on the happy path. That\u0027s part of why this patch landed green without touching them.\n\nAdding Etag to `get_header_frag_index()` would only change behaviour for a test that serves a body different from the headers it was built from, which is what the corruption tests do by hand anyway.\n\nHappy to add it if you\u0027d rather have it pinned explicitly than rely on `FakeConn\u0027s` default.","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7d9034d8d764a4a65d6b34ee3fd2ea2fd4108d18","unresolved":true,"context_lines":[{"line_number":6805,"context_line":""},{"line_number":6806,"context_line":"    def test_reconstruct_fa_no_donor_etag_header_still_rebuilds(self):"},{"line_number":6807,"context_line":"        # An object server that sends no ETag header cannot be"},{"line_number":6808,"context_line":"        # verified, but must not break the rebuild."},{"line_number":6809,"context_line":"        def drop_etag(i, body, headers):"},{"line_number":6810,"context_line":"            # HeaderKeyDict drops keys whose value is None"},{"line_number":6811,"context_line":"            headers[\u0027Etag\u0027] \u003d None"}],"source_content_type":"text/x-python","patch_set":1,"id":"3246a45f_45337fcd","line":6808,"updated":"2026-09-21 21:50:16.000000000","message":"I\u0027m torn sure about this -- every object-server GET response should include an Etag header -- if it doesn\u0027t, should we really still trust the data?\n\nOTOH, it\u0027s no worse than pre-upgrade behavior...","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy","status":"I\u0027m a Software Engineer at Red Hat and I love Open Source and connect with people! Feel free to DM through IRC, I\u0027ll be delighted to chat"},"change_message_id":"79fa5b5a32d4d8d5ca9d6c0d4a1c5044fe3fac82","unresolved":true,"context_lines":[{"line_number":6805,"context_line":""},{"line_number":6806,"context_line":"    def test_reconstruct_fa_no_donor_etag_header_still_rebuilds(self):"},{"line_number":6807,"context_line":"        # An object server that sends no ETag header cannot be"},{"line_number":6808,"context_line":"        # verified, but must not break the rebuild."},{"line_number":6809,"context_line":"        def drop_etag(i, body, headers):"},{"line_number":6810,"context_line":"            # HeaderKeyDict drops keys whose value is None"},{"line_number":6811,"context_line":"            headers[\u0027Etag\u0027] \u003d None"}],"source_content_type":"text/x-python","patch_set":1,"id":"ec6818f9_c76bfd32","line":6808,"in_reply_to":"3246a45f_45337fcd","updated":"2026-09-22 11:02:51.000000000","message":"I\u0027d keep tolerating it in this patch, but I\u0027m not certain either — push back if you disagree.\n\nRefusing turns a hypothetical into a real failure. A donor with no ETag becomes an unusable response, and a rebuild that would have worked fails with \"Unable to get enough responses\". Meanwhile the object server has always set response.etag \u003d metadata[\u0027ETag\u0027] on GET, so there\u0027s no released Swift where the header is legitimately missing. Being strict costs availability in the situation we can\u0027t foresee and gains nothing in the ones we can.\n\nThere\u0027s a fair argument for symmetry, since _handle_fragment_response already rejects a response missing X-Object-Sysmeta-Ec-Etag. But that header is needed to rebuild correctly — it says which version of the object the frags belong to, while the per-frag ETag is a verification aid. Different trade-off.\n\nIf you\u0027d rather be strict, or just want a warning logged so the condition is visible rather than silent, I think that\u0027s its own change: it alters when rebuilds fail. Happy to write it.","commit_id":"b5c27b87e3797db730e0cda57a253517af76bff1"}]}