review(substrate): close 2 MEDIUM concurrency gaps + trap-clean tempfiles (follow-up to #200)#202
Merged
Merged
Conversation
…iles (follow-up to #200) Post-merge review of #200 surfaced two race-condition gaps in the substrate's S3 concurrency story and a tempfile-cleanup hole in publish-catalog.sh. This change closes all three. **MEDIUM** — `pipeline/refresh-index.mjs` ETag-CAS retry was reusing a stale serialized body. Brief §3.4 prescribes "each writer lists by-repo/<R>/ to compute the latest, then PUTs index.json with conditional If-Match on its ETag, retrying on 412 conflict" — re-listing per attempt. The previous implementation listed once at script start, serialized once, then on 412 only re-read the ETag and retried with the same body. That gave PUT-level CAS (writer A's bytes won't clobber B's if B PUT between A's list and A's PUT) but NOT logical CAS — A's eventual successful retry would still carry A's view of the world, silently overwriting whatever B added. Fix: extract the listing + enumeration + serialization into a `recompute()` function and call it at the top of each retry attempt. The first attempt uses `initial = recompute()` from the regular execution path; conflict retries call `recompute()` again to incorporate the winning writer's additions before the next PUT. **MEDIUM** — `pipeline/refresh-index.mjs` first-publish race had no protection. When `headETag` returns null (no existing index.json), `putBytes` previously omitted any precondition — two concurrent first-publishers would both succeed, one silently losing. Fix: pass `--if-none-match '*'` whenever `ifMatch` is null, so even the create-vs-create race triggers a 412 and routes through the same retry path. **LOW** — `pipeline/publish-catalog.sh` tempfile cleanup was not exception-safe. The script creates `/tmp/publish-catalog-*.$$.*` tempfiles for validation status, per-catalog metadata, and the latest.json staging buffer. Under `set -eu`, any failure (validation, upload, jq) caused an early exit that bypassed the explicit `rm -f` calls. Fix: add a `trap '...rm -f /tmp/publish-catalog-*.$$.*...' EXIT` at the top of the script so cleanup runs unconditionally. The explicit `rm -f` calls remain as belt-and-suspenders on the happy path. Verified: 97 extractor tests + 26 canonical + 124 integration + 54 gojq-parity + 58 substrate = 359 tests pass (was 357; +2 for byte-stability check on `recompute()` and a tempfile-leak regression test on `publish-catalog.sh`). shellcheck clean on all four substrate shell scripts. Refs #152, #153, #200.
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
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.
Post-merge review of #200 caught two race-condition gaps in the substrate's S3 concurrency story plus a tempfile-cleanup hole. This change closes all three with regression tests.
MEDIUM —
pipeline/refresh-index.mjsretry reused stale serialized bodyBrief §3.4: "each writer lists
by-repo/<R>/to compute the latest, then PUTsindex.jsonwith conditionalIf-Matchon its ETag, retrying on 412 conflict." The previous implementation listed once at script start, serialized once, then on 412 only re-read the ETag — never re-listed. Result: byte-level CAS but not logical CAS. Writer A's eventual successful retry would carry A's view of the world, silently overwriting whatever writer B added between A's list and A's PUT.Fix: extract the list / enumerate / serialize pipeline into a
recompute()function. The first attempt usesinitial = recompute(); conflict retries callrecompute()again so the loser's eventual successful PUT carries the winner's view, not the loser's stale serialization.MEDIUM —
pipeline/refresh-index.mjsfirst-publish race had no protectionWhen
headETagreturned null (no existingindex.json),putBytesomitted any precondition. Two concurrent first-publishers both succeeded; one silently lost.Fix: pass
--if-none-match '*'wheneverifMatchis null, so even the create-vs-create race triggers a 412 and routes through the same retry path.LOW —
pipeline/publish-catalog.shtempfile cleanup not exception-safeThree
/tmp/publish-catalog-*.$$.*tempfiles were only removed on the happy path. Underset -eu, any failure mid-pipeline (validation, upload, jq) bypassed the explicitrm -fcalls.Fix: add a
trap '...rm -f ... EXIT'at the top of the script. Explicitrm -fcalls remain as happy-path belt-and-suspenders.Test plan
recompute()and a tempfile-leak regression check onpublish-catalog.sh)shellcheckclean on all four substrate shell scriptsRefs #152, #153, #200.