Do not let a docs-only merge cancel a code deploy - #258
Conversation
ReviewGood fix for a real production gap, and the accompanying test suite is thorough (docs-only, code, mixed, empty-diff, and drift-detection cases). Three things worth addressing before merge: 1.
|
A documentation merge landing seconds after a code merge cancelled that code deploy. The rollout saw a newer master commit and declined, while the docs push started no rollout of its own because deploy.yml ignores those paths. The result was a green deploy job and production still running the previous image, with nothing reporting the gap. It stranded #251 for half an hour on 2026-09-10 and was found only by checking the live digest by hand. The supersede check now asks whether the newer commits could change the image at all, and defers only when they could. It fails closed: if the newer commits cannot be fetched or inspected, the answer is the previous behaviour. The tests cover both directions, an empty diff, and drift between run.py's list and deploy.yml's paths-ignore, which would otherwise be silent.
Three .pyc files rode along with the deploy-supersede fix. They are build output of the very scripts beside them, they are platform- and version-specific (cpython-313), and #261 already established that generated artifacts do not belong in the working tree. .gitignore now says so, which is what stops the next run of the tests re-adding them.
4e978d6 to
721eccb
Compare
|
Verified and rebased onto current master. This is live right now, not hypothetical. #262 merged at 09:48Z, its The superseding commits were two docs-only pushes at 09:56Z and 10:01Z. Production is still on Can-fail proof re-run by me on the rebased branch: forcing One change on top: three The 'merge when convenient rather than mid-backfill' caveat is satisfied: |
ReviewSolid, well-scoped fix. The root cause ( Things done well:
One suggestion (non-blocking): the Minor/no action needed: the No security or performance concerns — this only adds one extra shallow ( 🤖 Generated with Claude Code |
#262 went live at 10:36Z once #258's merge pushed a non-ignored path — a docs merge deploys nothing, so stranded code needs a code push to land. At 31 minutes: 8 obsolete base files proven reproduced (no futile rebuild minted), 19 not (minted as before), base_generation_unverified = 15 retries over those 19 files. rollup_published_empty_over_full_base stayed 0 and rollup_median_contiguous_days stayed 30 — the silent-short-publish failure mode this change risked did not occur.
* Review group commit in production and record what it exposed Group commit works: journal_hold avg 5,452 -> 3,093 us and the lock's duty cycle 18.5% -> 14.5% at a comparable process age (NOT against the 29.6% from the 19h process — a mature process is not the comparator for a 44-minute one). PR #263 is separately confirmed by journal_stats_publishes at 0.80/s against a ~1/s target. The cost did not disappear, it became measurable. performed 30,075 + coalesced 30,506 = 60,581 waiters exactly, 11.4 fsync cycles/s is one per 88 ms, and 11.4 x 88 ms is the whole second — the commit pipeline runs at ~100% duty, so every arrival queues. block.journal_commit_wait averages 362 ms with a 10.7 s max, on the pre-ack path. The ~2x coalescing is OPTIMAL for arrival-rate x fsync-duration, so batching harder buys nothing; the lever is cost per commit. Three findings recorded: the deploy pipeline stranding merged code (observed live on #262, second instance after #251, fixed by #258); unbounded growth of Complete task history driving compact() under the global mutex; and the no-op rollup skip being defeated by restart frequency, which raises the value of the content-fingerprint tag follow-up. * Record #262's production verification and how #258 unstranded it #262 went live at 10:36Z once #258's merge pushed a non-ignored path — a docs merge deploys nothing, so stranded code needs a code push to land. At 31 minutes: 8 obsolete base files proven reproduced (no futile rebuild minted), 19 not (minted as before), base_generation_unverified = 15 retries over those 19 files. rollup_published_empty_over_full_base stayed 0 and rollup_median_contiguous_days stayed 30 — the silent-short-publish failure mode this change risked did not occur.
1 participant
CI/deploy tooling only. No crate source change — nothing here can alter the running binary's behaviour.
The bug, observed in production
#251 merged at 01:49 and its Build and Deploy run went green, yet production kept the previous image. The log:
A newer master commit superseded this rollout; production is unchanged.The superseding commit was #254 — documentation only.run.pycompares HEAD againstrefs/heads/masterand defers whenever they differ. That is right for a real code push, but a docs commit cannot change the image; docs andbench/are in deploy.yml'spaths-ignoreprecisely for that reason, so they never start a rollout of their own — and they cancel the one in flight. Silent in both directions: green job, no run for the docs push, nothing reporting that merged code is not running. It stranded #251 for half an hour and was caught only by checking the live digest by hand.The change
The supersede check now asks whether the newer commits could change the image at all, and defers only when they could. It fails closed: if the newer commits cannot be fetched or inspected, it returns the previous behaviour.
Verification
only_undeployableforced to the old behaviour, all four docs-only cases fail; restored, all tests pass.test_ignore_list_matches_the_workflowfails ifrun.py's list and deploy.yml'spaths-ignoreever drift — otherwise that drift would be silent, and the workflow would skip a path this still defers on.make ci-signoffbeside the existingtest_lease.py.Parsed with a regex rather than
yaml.safe_loadso the check needs no third-party module.Suggest merging when convenient rather than mid-backfill — merging restarts production, and the Sep 1–7 index backfill is currently converging.