Skip to content

covratchet: ask the coverage question before the push, not 47 minutes later - #925

Merged
tannevaled merged 1 commit into
mainfrom
covratchet-require
Nov 13, 2026
Merged

covratchet: ask the coverage question before the push, not 47 minutes later#925
tannevaled merged 1 commit into
mainfrom
covratchet-require

Conversation

@tannevaled

@tannevaled tannevaled commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

covratchet: ask the coverage question before the push, not 47 minutes later

Why

The coverage ratchet is correct, and it caught the mistake. It caught it in
CI, on a lane that runs the whole suite, about 47 minutes after the push —
twice in one day, on the same two functions, because a commit written to
answer it did not move the measurement at all.

A gate that refuses after the spend teaches the lesson. One that refuses
before it prevents the mistake.

What -require does

The narrow question that is cheap locally: the functions this commit claims
to have covered — are they actually at 100%?

go test ./internal/vm/ -run FTP -coverprofile=/tmp/ftp.cov
go tool cover -func=/tmp/ftp.cov > /tmp/ftp.func
covratchet -profile /tmp/ftp.func -require ftpApplyOpts,ftpTimeoutErr
required 2 function(s), matched 2 record(s) in a profile of 7347

all required functions at 100%

A selective -run is sound here: a function at 100% under some tests is at
100% under all of them. It needs no record, so it runs on any profile.

⛔ Why it is a command and not a grep

This is also why parse now returns the whole profile and not only the sub-100
set. With the sub-100 set alone, "this function is fully covered" and "this
profile has never heard of that name" are the same observation — absence. A
typo, a renamed function, or a profile taken over the wrong package would every
one of them read as success, and the check would be worse than useless,
because it would be believed.

So a name that matches nothing fails, and says which:

$ covratchet -profile /tmp/ftp.func -require ftpApplyOpts,ftpTimeoutErrr
required 2 function(s), matched 1 record(s) in a profile of 7347

-- NOT IN THIS PROFILE -- the name matched nothing (1) --
   ftpTimeoutErrr

::error::1 name(s) matched nothing; this profile cannot answer for them

That is the go test -run failure mode exactly: exit 0 on a selection that
matched nothing. Every verdict prints its denominator — records matched out
of a profile of N — so a run that read nothing cannot pass for a clean one.

Verified

Against the real profile that produced yesterday's red, not a fixture: the two
functions report 100% and exit 0, and a single extra letter in one name
exits 1 and names it. Plus eight new unit cases covering both halves, including
file:name pointing at the wrong file — a right name in the wrong place is an
absence, not a pass.

Also

scripts/coverage/README.md, which did not exist. The ratchet was documented
nowhere in the repository, so N function(s) dropped below 100% named no page
to go to; now it explains what is gated (the set, not the total, and why a
percentage could not be calibrated), how to answer it locally, and what each
exit status means.

🤖 Generated with Claude Code

@tannevaled @claude
covratchet: ask the coverage question BEFORE the push, not 47 minutes…
ec439f0 
… later

The ratchet is correct and it caught the mistake. It caught it in CI, on a
lane that runs the whole suite, about 47 minutes after the push -- twice in
one day, on the SAME two functions, because a commit written to answer it
did not move the measurement at all. A gate that refuses after the spend
teaches the lesson. One that refuses before it prevents the mistake.

`-require` answers the narrow question that is cheap locally: the functions
this commit CLAIMS to have covered, are they actually at 100%?

    go test ./internal/vm/ -run FTP -coverprofile=/tmp/ftp.cov
    go tool cover -func=/tmp/ftp.cov > /tmp/ftp.func
    covratchet -profile /tmp/ftp.func -require ftpApplyOpts,ftpTimeoutErr

A selective -run is sound here: a function at 100% under SOME tests is at
100% under all of them. It needs no record, so it runs on any profile.

⛔ THE REASON IT IS A COMMAND AND NOT A GREP, and the reason parse now
returns the whole profile rather than only the sub-100 set: with the sub-100
set alone, "this function is fully covered" and "this profile has never
heard of that name" are the SAME observation -- absence. A typo, a renamed
function or a profile taken over the wrong package would every one of them
read as success, and the check would be worse than useless because it would
be believed. So a name that matches nothing FAILS, and says so:

    -- NOT IN THIS PROFILE -- the name matched nothing (1) --
       ftpTimeoutErrr

That is the `go test -run` failure mode exactly: exit 0 on a selection that
matched nothing. Every verdict prints its denominator -- records matched out
of a profile of N -- so a run that read nothing cannot pass for a clean one.

Verified against the real profile that produced yesterday's red: the two
functions report 100% and exit 0, while a single extra letter in one name
exits 1 and names it.

Also adds scripts/coverage/README.md, which did not exist: the ratchet was
documented nowhere, and "N function(s) dropped below 100%" named no page to
go to.

Co-Authored-By: Claude Opus 5 <[email protected]>
@tannevaled
tannevaled merged commit 0803929 into main Nov 13, 2026
26 checks passed
@tannevaled
tannevaled deleted the covratchet-require branch November 13, 2026 12:21
@tannevaled tannevaled mentioned this pull request Nov 13, 2026
Merged
tannevaled added a commit that referenced this pull request Nov 13, 2026
@tannevaled @claude
Net::FTP.default_passive, and the options a nil host was throwing away (
8c8ec0a 
#928)

* Net::FTP: the DATA connection releases the GVL too, and the backlog shrinks

#921 bounded the control connection and #924 released the GVL around it. Both
left the DATA connection holding it, and the gate #924 added said so in its
seeded backlog -- four entries I wrote down rather than fixed, so the gate
could land without a sprawling change.

This is the change. dialData, acceptData, sendData, retrlines and retrbinary
now run their blocking call inside ftpIO, and the four entries are DELETED from
testdata/gvl-blocking-backlog.txt. That list may only shrink, and an entry that
stops reporting fails the gate too, so a fix cannot leave its allowance behind.

⛔ acceptData is the one #916 is actually about. In ACTIVE mode the client
listens and the server dials back, so a Ruby program that serves itself has
both ends in one process -- and holding the GVL in that Accept parks every
other Ruby thread, INCLUDING the one that would make the call it is waiting
for. ruby/spec's net-ftp fixtures are exactly that shape, and their
spec_helper.rb puts Net::FTP into active mode (see #916's comment: rbgo has no
Net::FTP.default_passive, so today that helper is skipped in silence and the
suite never reaches this path at all).

Only the blocking call goes inside each closure. ftpConnErr raises, and a panic
crossing a released GVL would unwind without re-acquiring it, so every raise
stays outside.

⛔ NO BEHAVIOURAL TEST, AND THE REASON IS NOT LAZINESS. The in-repo FTP test
server is Go-side, so it cannot reproduce this deadlock at all: nothing about
rbgo's emulated GVL blocks a Go goroutine, and a test built on it would pass
whether or not the fix is present. A real regression test needs BOTH ends in
Ruby -- a dummy FTP server in a Ruby Thread, which is what ruby/spec's fixture
is -- and that is worth doing properly rather than approximating with something
that looks like proof and is not.

What does guard the property is the gate: TestEveryBlockingSocketCallReleasesTheGVL
fails in about a second if any of these five is unwrapped again, and the
backlog deletion means it also fails if one silently stops being reported.

The measurement that would show this moving -- the library/net-ftp sweep -- is
deferred: this machine has been carrying a 1-minute load average between 23 and
86, and a per-file timeout measured under that cannot tell a hang from a slow
file.

Refs #916

Co-Authored-By: Claude Opus 5 <[email protected]>

* Net::FTP.default_passive, and the options a nil host was throwing away

ruby/spec's library/net-ftp/spec_helper.rb — which all 56 files require —
opens with:

    if defined?(Net::FTP.default_passive)
      Net::FTP.default_passive = false
    end

rbgo had no such accessor, so the guard was skipped in SILENCE and the whole
suite ran against a connection rbgo kept in PASSIVE mode, while the fixture
server implements PORT/EPRT and has no `pasv` method at all. That is the
`451 Exception: undefined method 'pasv'` those specs report (#916).

Implemented against the reference, not against expectations: net-ftp 0.3.9
lib/net/ftp.rb:96 (`@@default_passive = true`), :159-167 (the accessors) and
:286-290 (`if options[:passive].nil?` → the default). The value lives in the
CLASS VARIABLE MRI uses, so Net::FTP.class_variable_get(:@@default_passive)
answers here as it does there and there is one place holding it, not two that
can disagree.

Two defects the reference comparison turned up that reading our own code would
not have:

  - ⛔ `Net::FTP.new(nil, passive: true)` answered FALSE. ftpInit returned
    before reading the options Hash whenever the host was nil, so EVERY option
    was dropped on that path — binary, timeouts, credentials, all of it. MRI
    applies the options first and connects separately, and ruby/spec drives
    exactly that shape: `@ftp.send(:initialize, nil, options)` then asserts
    `passive` (initialize_spec.rb:201-226). A mechanism wired at only some of
    its doors, again.

  - ⛔ `passive: nil` meant FALSE. ftpApplyOpts tested the KEY's presence, but
    MRI tests the VALUE: nil is "no opinion" and falls back to the default.
    Note this is the OPPOSITE of read_timeout in the same Hash three lines
    away, where nil IS a request (for no bound) — easy to conflate, so both
    now say which they are in a comment.

Tests: a 12-line table run through rbgo, each line annotated with the
reference line it comes from, plus ftpDefaultPassive's fallbacks (nil class,
absent or nil class variable → true), which no Ruby program can reach and
which decide the transfer mode of every connection.

⛔ Measured before pushing with the tool added in #925, which caught my own
gap: `covratchet -require ftpDefaultPassive,ftpApplyOpts,ftpInit` first
reported ftpDefaultPassive at 66.7%, and reports all three at 100% now.

Refs #916

Co-Authored-By: Claude Opus 5 <[email protected]>

---------

Co-authored-by: Claude Opus 5 <[email protected]>
tannevaled added a commit that referenced this pull request Nov 13, 2026
@tannevaled @claude
socket: write the class guard so it has no unreachable statement
04530e3 
The coverage ratchet named installSocketNonblock at 83.3%, newly below 100%.
The uncovered statement was the `continue`:

    cls, ok := vm.consts[name].(*RClass)
    if !ok {
        continue          // ← all six classes exist, so nothing reaches this
    }

All six socket classes are registered at startup, so that arm is unreachable on
this build and no test can honestly cover it. Writing the guard as

    if cls, ok := vm.consts[name].(*RClass); ok { … }

keeps exactly the same protection — a name that is not a class is skipped
rather than type-asserted into a panic — and has no statement on the false
path to explain away.

⛔ The alternative was a test that deletes a class from vm.consts to drive the
`continue`. That buys the percentage by asserting a state the VM never has, and
leaves the next reader wondering which of the two is the lie. Removing the dead
statement is cheaper and truer.

Measured before pushing, with covratchet -require (#925), which is what that
flag exists for after two commits in one day failed to move this same number:

    required 2 function(s), matched 2 record(s) in a profile of 7351
    all required functions at 100%

The ruby/spec ratchet is already green on this head — the nonblock commit
settled core/io/nonblock_spec.rb — so this is the last red.

Refs #931

Co-Authored-By: Claude Opus 5 <[email protected]>
tannevaled added a commit that referenced this pull request Nov 14, 2026
@tannevaled @claude
socket: open with a block must close, which is 27 of the hangs in #916 (
6b51deb 
#932)

* socket: `open` with a block must close, which is 27 of the hangs in #916

TCPSocket.open, TCPServer.open and Socket.open were bare aliases for .new, so
the block form closed nothing: the socket outlived the block and the peer
waited for an EOF that never came.

⛔ THAT IS THE WHOLE net-http COLUMN OF #916. ruby/spec's
library/net-http/http/fixtures/http_server.rb serves ONE client at a time and
shuts down by opening a NEW connection to write an unterminated "CLOSE":

    def close
      TCPSocket.open(ip, port) { |socket| socket.write "CLOSE" }
      @thread.join
    end

With that socket still open the server's `gets("\r\n\r\n")` never returns, the
listen loop never comes back to accept, and @thread.join never completes. All
27 library/net-http files end on that join.

Measured against the restored MRI oracle, the same ten-line reproduction:

    closed? after the block     MRI true    rbgo false
    the peer's gets             MRI "CLOSE" rbgo blocks for ever
    join                        MRI :joined rbgo hangs

and after this change rbgo prints the same seven lines as MRI, in the same
order, and exits 0.

⛔ IT IS NOT THE GVL, which is what I expected to find. A goroutine dump of a
hung get_spec.rb puts the server thread correctly INSIDE ioBlock with the GVL
released (readUntil -> bufio.ReadByte -> net.(*conn).Read) and the main thread
in threadJoin. The emulated GVL behaves; the socket was simply still open. Nor
does Net::HTTP leak its own connection: the reproduction shows "peer closed"
after the request under BOTH engines.

HOW IT CLOSES, and why there is no fourth spelling of close: it sends the
CLASS'S OWN `close` method, so TCPSocket, TCPServer and Socket each keep the
one close they already had -- each guarded by `s.closed`, hence idempotent, so
a block that closed the socket itself is not closed twice. The close runs in a
defer, so an exception or a break leaves the socket closed as well: this is an
ensure, not a tidy-up on the happy path.

The test asserts the contract on the socket the block was handed rather than
through a peer, so a regression FAILS it instead of HANGING it: the block's
value comes back (not the socket), closed? is true after a normal return and
after an exception, and `open` with no block still answers with an open
socket exactly as .new does.

Refs #931, #916

Co-Authored-By: Claude Opus 5 <[email protected]>

* socket: nonblock? on the socket classes — the vacuous pass the open fix exposed

The ruby/spec ratchet refused the previous commit with +27 examples overall
and ONE file down:

    passing examples: 23699 (baseline 23672, +27)
    -- REGRESSED (1) --
       core/io/nonblock_spec.rb: 3 passing, baseline 4 (-1)

⛔ The gate was right to refuse, and nothing had regressed. The example is

    TCPServer.open(0) { |socket| socket.nonblock?.should == true }

and while `open` ignored its block that body NEVER RAN: the example passed with
no assertion evaluated, and the baseline recorded that pass. Honouring the
block made it run, and it raised NoMethodError — TCPServer#nonblock? does not
exist. A test that had never run started running.

Measured both ways rather than assumed, because the +27 includes other work
merged into main today and the -1 could have been someone else's:

    core/io/nonblock_spec.rb, main ba89ba6:  pass=4 fail=0 error=0
    core/io/nonblock_spec.rb, this branch:    pass=3 fail=0 error=1
      NoMethodError  undefined method 'nonblock?' for an instance of TCPServer

So it was mine.

IO#nonblock? is installed on IO alone and reads an *IOObj field, which a socket
is not. This installs `nonblock?` on the socket classes FROM THE SAME
`io/nonblock` feature hook, so one place decides what that require adds and the
two cannot drift; the socket classes exist from startup, so they are there when
it fires.

The answer is `true` because it is true here: Go puts every socket descriptor
in non-blocking mode and polls it through the runtime, which is also why MRI
reports true for a socket by default.

⛔ `nonblock=` is deliberately absent. The blocking mode of these descriptors
belongs to the Go runtime, and a writer that accepted `false` and changed
nothing would be a more expensive lie than a missing method. MRI does allow it;
the divergence is stated here rather than papered over.

The file is back to 4 passing — but the fourth example now RUNS and asserts,
where on main it passes with nothing evaluated. Same number, different meaning.

Refs #931

Co-Authored-By: Claude Opus 5 <[email protected]>

* socket: write the class guard so it has no unreachable statement

The coverage ratchet named installSocketNonblock at 83.3%, newly below 100%.
The uncovered statement was the `continue`:

    cls, ok := vm.consts[name].(*RClass)
    if !ok {
        continue          // ← all six classes exist, so nothing reaches this
    }

All six socket classes are registered at startup, so that arm is unreachable on
this build and no test can honestly cover it. Writing the guard as

    if cls, ok := vm.consts[name].(*RClass); ok { … }

keeps exactly the same protection — a name that is not a class is skipped
rather than type-asserted into a panic — and has no statement on the false
path to explain away.

⛔ The alternative was a test that deletes a class from vm.consts to drive the
`continue`. That buys the percentage by asserting a state the VM never has, and
leaves the next reader wondering which of the two is the lie. Removing the dead
statement is cheaper and truer.

Measured before pushing, with covratchet -require (#925), which is what that
flag exists for after two commits in one day failed to move this same number:

    required 2 function(s), matched 2 record(s) in a profile of 7351
    all required functions at 100%

The ruby/spec ratchet is already green on this head — the nonblock commit
settled core/io/nonblock_spec.rb — so this is the last red.

Refs #931

Co-Authored-By: Claude Opus 5 <[email protected]>

---------

Co-authored-by: Claude Opus 5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@tannevaled