fix: remaining adversarial-review findings (M4/M5/M6/M7, S2/S3, A4, F2, P2, AU6/AU7, R8, U6)#69
Open
russellromney wants to merge 7 commits into
Open
fix: remaining adversarial-review findings (M4/M5/M6/M7, S2/S3, A4, F2, P2, AU6/AU7, R8, U6)#69russellromney wants to merge 7 commits into
russellromney wants to merge 7 commits into
Conversation
- The read cache wrote every ref into the cache even when the durable store skipped it as older, so a write that lost the ordering check could still be served from cache for the TTL window. Invalidate the cache entry on save instead; the next load reads the kept value through. - The pack HashingWriter hashed the whole buffer before a possibly-short inner write, so write_all's retry re-hashed the unwritten remainder — a wrong pack trailer that index-pack rejects for blobs whose compressed form spans a short write. Hash only the bytes the inner writer accepted. Refs: ADVERSARIAL_REVIEW_2026-06-25.md M5, P2 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eject mismatch (S2, S3, M7) - Local retention only protected a best-effort side list, and on a local-only backend the durability check is a no-op — so a stale/incomplete list could delete a still-referenced artifact (its only copy). Retention now also protects the set the live refs actually point at, computed each run from the ref store (the reachable walk is shared with remote GC). - Remote GC continued past a manifest read error, leaving that manifest's chunks out of the reachable set — a brief failure could delete live objects. Fail the whole pass instead. - A network queue paired with a per-host file metadata store silently lost refs (server and workers each read their own files). Refuse that combination at startup with a clear message. Refs: ADVERSARIAL_REVIEW_2026-06-25.md S2, S3, M7 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…A4, M6) - A push that arrived while the prior build for the same key was already claimed (and had already fetched) was coalesced onto that in-flight build, so the newer commit wasn't built until the next push. Coalescing now targets only queued jobs, and the unique index is relaxed to one queued job per key so a claimed build and a fresh queued one can coexist — the newer commit gets its own job and builds next. - MySQL key columns are VARCHAR (the composite PK can't be TEXT); an over-long repo/branch/queue key would silently truncate and collide. Reject it with a clear error instead. Refs: ADVERSARIAL_REVIEW_2026-06-25.md A4, M6 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (F2, U6) - The no-dictionary frame decompress used unbounded `decode_all`, so a corrupt or hostile frame could expand without limit before the length check. Cap it at the frame's declared raw length, like the dictionary branch. - The directory-creation helper probed with `exists()` before `create_dir`, which races concurrent producers (one wins, the rest hit EEXIST). Use the atomic create-then-tolerate-AlreadyExists form. Refs: ADVERSARIAL_REVIEW_2026-06-25.md F2, U6 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…0 token (AU6, R8, AU7) - The direct-artifact redirect signed URLs with a flat 15-minute TTL, ignoring repo visibility. Use the same shorter TTL for private repos as the ref path. - The HTTP client builder fell back to a header-less client on a build error, silently dropping the auth headers so every request went out unauthenticated. Fail fast instead. - The token file was written with the default umask then chmod'd 0o600 (briefly world-readable) and the chmod error was ignored. Create it 0o600 up front and surface a permission error. Refs: ADVERSARIAL_REVIEW_2026-06-25.md AU6, R8, AU7 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (M7) Refusing to start broke valid single-host farm-out (a networked queue with the server and workers sharing one filesystem and its file metadata). We can't tell shared-filesystem from multi-host here, so warn loudly instead of bailing — the goal is to end the silent footgun, not block a working setup. Refs: ADVERSARIAL_REVIEW_2026-06-25.md M7 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
"A newer sync never loses" ordered by synced_at — the builder's per-host wall clock — so cross-host clock skew or two writes in the same second could pick the wrong winner. Order by the commit's history depth instead (its `generation`, from `git rev-list --count`): recency follows the commit's place in git history, which is the same for every builder, so skew can't reorder it. - RefInfo carries `generation`; the build computes it once via `git rev-list --count` (the commit-graph we write makes it cheap), set wherever synced_at is. - The compare (shared `should_replace_ref` + every SQL `save_ordered`) now: same commit wins; else higher-or-equal generation wins; else (a side without a generation) fall back to synced_at. `refs` tables gain a `generation` column (with a best-effort migration); the comparison stays a single atomic upsert. - synced_at is kept as the fallback for refs/repos without a generation. Known limitation (documented at the compare): a force-push that rewinds a branch to an older commit has a lower generation, so it lags until the next forward push (a same-commit re-sync still updates). That is the cost of ordering by history rather than by observation time. Tests: higher generation wins over a newer wall clock (file store + the SQL lifecycle across sqlite/pg/mysql). Refs: ADVERSARIAL_REVIEW_2026-06-25.md M4 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
russellromney
changed the title
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.
Works through the remaining tractable findings from
ADVERSARIAL_REVIEW_2026-06-25.md, after #62–#65 merged. Audited each against currentmainfirst (the report's line refs were two main-versions stale); all below were still open. Themed commits; fullcargo test+clippy -D warnings+fmtgreen, with regression tests for the key fixes.Fixed
generation, fromgit rev-list --count) instead of the builder's wall clock, so cross-host clock skew can't reorder. Falls back tosynced_atfor refs without it. (Known, documented limit: a force-push rewind to an older commit lags until the next forward push.) (tests)HashingWriterre-hashed the remainder on a short write → bad pack trailer for >256 KiB-compressing blobs. (test)exists()-then-create_dir; use the atomic form.0o600up front, surface errors.Deferred (with reasons)
🤖 Generated with Claude Code