Week 17: A Client That Fails Loud

Two pull requests opened this week, the runner on the 23rd and the test case
editor on the 24th, and between them they taught the same lesson from opposite
directions. The runner taught me that a client for a service nobody has built
yet has to be stricter than a client for a service that exists, because
nothing downstream will catch what it lets through. The editor taught me that
a pull request which leans on an unmerged one fails in CI before a reviewer
reads a single line of it.

What the runner is, and what it refuses to be

Autograder::Runner is 159 lines across three files and it does one thing:
it posts a student's circuit and a test suite to an external simulator
service and turns the answer into a pass count. It does not touch the
database. It does not decide what a score means. It is the messenger between
Rails and the thing that can actually simulate a circuit, because the
simulator lives entirely in the browser and Rails has no way to run a circuit
by itself.

I considered a headless browser per submission and dropped it, since booting
a full frontend to grade one circuit is too heavy to do on every run. A
background queue was the other option I set aside. The job that calls the
runner is its own pull request, so the runner stays a plain object that
either returns a result or raises.

class RunnerError < StandardError; end
TIMEOUTS = { connect: 5, write: 10, read: 60 }.freeze

def run
  response = HTTP.timeout(**TIMEOUTS)
                 .post(endpoint, json: { circuit: circuit, testbench: testbench.data })
  raise RunnerError, "Runner returned #{response.status}" unless response.status.success?

  JSON.parse(response.body.to_s)
rescue HTTP::Error => e
  raise RunnerError, "Runner unreachable: #{e.message}"
rescue JSON::ParserError
  raise RunnerError, "Runner returned invalid JSON"
end

Every failure path ends in one exception class, so a caller has one thing to
rescue. That was the design. The reviews were about the places where it
quietly was not true.

A response that is valid JSON is not a valid response

The sharpest finding came from CodeRabbit, on how the answer gets scored:

a successful response containing null raises NoMethodError instead of
RunnerError. a response containing {"groups":[{}]} returns a zero
score, while a case with "passed":"false" counts as passed.

I had checked that the body parsed and that groups was a non empty array,
and stopped there. The scoring line counts a case as passed when
kase["passed"] is truthy, and in Ruby the string "false" is truthy. A
runner that serialised its booleans as strings would have turned every failing
case into a passing one and handed a student full marks. The finding's last
sentence is the rule I am taking from it: malformed responses must not become
grades.

The same review pointed at the timeouts. connect, write and read are
per operation limits. A service that keeps trickling bytes just inside the
sixty second read window can hold a Sidekiq worker for as long as it likes,
because nothing bounds the whole request. Per operation timeouts and a total
deadline are different protections and I had only written the first kind.

The fallback that would have posted student work to localhost

A human reviewer found the one I am least comfortable with. The endpoint was
built like this:

"#{ENV.fetch('SIMULATOR_RUNNER_URL', DEFAULT_URL)}/run"

with DEFAULT_URL set to the loopback address the development runner uses.
The comment on it:

with this fallback a prod box missing SIMULATOR_RUNNER_URL silently posts
student circuits to its own localhost and every run just shows as
"unreachable".

That is a configuration mistake that produces no error you would look at. The
symptom is an outage that looks like the simulator being down, and the cost is
student circuits going to a port on the same machine. A default is helpful in
development and dangerous in production, and .env.example already covers
development. The fix is to drop the default so a missing variable raises
KeyError on the first call instead of on the first complaint.

The same reviewer asked the next question before I did: the request carries no
authentication, so anything that can reach the runner can make it simulate
arbitrary circuits, and is a shared secret planned for when the service
lands? It was not planned. It is now, as a bearer token read from the
environment in the same strict way. CodeRabbit followed up with the logical
conclusion: a bearer token sent over plain http:// to a non loopback host is
sent in cleartext along with the circuit and the suite, so non loopback URLs
should have to be HTTPS.

The contract only one side has seen

The most useful comment was not about any line:

the response shape (groups[].cases[].passed) is only defined by this
client and the stubbed spec, there's no runner service in the repo or an
open pr yet. i'd land this with or after the service so the contract comes
from the side that actually simulates.

I wrote the specs against a stub that I also wrote, which means the test and
the code agree with each other and with nothing else. If the real service
names a field differently, every spec still passes and the first real run
fails. I cannot fix that inside this pull request. The honest options are to land
the runner together with the service, or to say plainly in the description
that the response shape is a proposal, and the second is cheaper to do this
week. It also means the strict response checks above matter more, because
they are the only thing standing between a wrong shape and a wrong grade.

One more review item belongs to the specs: they stubbed only the localhost
URL, so setting SIMULATOR_RUNNER_URL in a test environment would make the
request miss WebMock and fail. The endpoint in the spec should be derived from
the same configuration as the service.

The pull request that failed before anyone read it

The editor went up on the 24th as #7907, 523 lines over 13 files, and CI
failed with Missing model class Testbench. The cause was plain
once I looked. The editor edits a Testbench, and the model that defines it
is a separate pull request that was still open, so on a branch cut from master
the class does not exist. A reviewer's comment said the same thing and added
the second half: both pull requests declare the same has_one :testbench
line, so they would conflict the moment either merged.

The fix is the one the stack already implied, which is to build the editor on
top of the model's branch and keep a single declaration:

has_one :testbench, dependent: :destroy, autosave: true

I had split the work into small pull requests to keep each one reviewable,
and I had not paid for the split. A small pull request that cannot run alone
is not small for the reviewer, because they have to reconstruct what it
depends on. Stacking is honest about that and cutting from master hides it.
That cost is going to come back.

Where the project stands

The runner's configuration and authentication findings are settled in
principle: no silent default, a shared secret, one exception class. The
response validation findings are the open half, and they are the ones that
decide whether a grade can be trusted. The editor is stacked on the model it
depends on.

Next week: the runner's fixes land, and the three pieces that turn its answer
into a stored, scaled, rate limited grade start going up.