Skip to content

A floor break can merge with every check green: lowest-direct never runs on pull requests #245

Description

@lesnik512

#244 shipped a crash to PyPI in 1.8.0 and sat there through 1.9.0. Twelve checks passed on the pull
request that introduced it.

What happened

sentry_instrument.py passed sentry_logs_level=None unconditionally. That parameter arrived in
sentry-sdk 2.25.0; the declared floor is sentry-sdk>=2.1. Every Sentry-enabled service between
those versions failed at bootstrap with TypeError.

scripts/floor_smoke.py catches it. I confirmed that directly: with the pre-fix
sentry_instrument.py installed, floor_smoke.py free fails with exactly that TypeError, and it
passes with the fix.

So the guard existed and worked. It just never ran on the change that needed it.

Why

_checks.yml gates the job on inputs.lowest-direct, which ci.yml leaves at its false default
and only scheduled.yml turns on. The comment explains the reasoning:

Off for PRs: the job resolves direct dependencies at their floor and transitives at their newest,
so a release published upstream today can turn it red for reasons no PR here caused. scheduled.yml
turns it on, where that failure opens a tracking issue instead of blocking a merge.

That reasoning is sound and should not simply be reversed. It protects pull requests from unrelated
upstream drift, which is a real and recurring problem here (#232 was exactly that, from the newest
side).

But it also means a pull request can break a declared floor and merge with every check green,
which is what happened. The daily cadence added in #233 bounds the damage to about a day; it does not
prevent the release. 1.8.0 was tagged the same afternoon.

The tension

The floors job conflates two failure modes that deserve different handling:

  1. The diff broke a floor. The author's fault, deterministic, should block the merge.
  2. An upstream release broke a floor. Nobody's fault, non-deterministic, should open a tracking
    issue, which is what it does today.

Both look identical to CI right now, so the job is treated as category 2 for everything.

Options

  1. Run the floors job on pull requests too, and accept occasional unrelated red. Simplest. Costs
    the protection the current comment was written to provide, and a 25-job matrix on every PR.
  2. Run a narrowed floors job on pull requests — one interpreter, only the targets whose files the
    diff touches. Cheaper, and most floor breaks are introduced in the instrument or bootstrapper they
    belong to. Does not help when a shared instrument breaks a framework target's floor.
  3. Pin transitives for the PR run so only direct floors move, making category 2 much rarer. The
    floors job already resolves "direct at floor, transitive at newest", which is the part that drifts.
  4. Accept and rely on the daily run, and add a release gate: do not tag unless the most recent
    scheduled run is green. Cheap, and directly targets the outcome that actually hurt, which was
    publishing rather than merging.

Recommendation

4 combined with 2, if the narrowing is expressible. The thing that cost users here was not the merge,
it was the tag twenty minutes later. A release gate is a few lines and closes that hole completely;
narrowing the PR job is a bonus that catches it earlier.

Filed rather than fixed because the current behaviour is deliberate and documented, so changing it is
a decision rather than a repair. Related: #236, the same job's other gap, where the floors are
smoke-tested but never type-checked or unit-tested.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions