Week 18: Four Small Pieces and the Seams Between Them

Four pull requests opened this week, on the 28th, 29th, 29th and 30th, and none of them is more than about a hundred lines. Autograde settings, a
GradingResult model, AutogradeJob, and GradeScaleMapper. Each one is easy to review on its own, and almost every review comment
was about the gap between two of them rather than about the code inside one. A small piece is easy to get right. The seam between two
small pieces belongs to nobody until someone writes it down.

Settings that nothing reads

#7926 adds three per assignment settings to the form: allow partial credit, a maximum number of attempts, and whether to reveal test
cases before submission. Three columns, a form block, 82 added lines. The reviewer's comment was the one I should have written myself:

these three settings get saved but nothing reads them yet (not on master, not in #7836/#7906/#7907), so an instructor can set max
attempts to 1 and students still get unlimited runs. i'd add each column in the pr that enforces it, or hide this block until then.

A control that saves and does nothing is worse than a missing control. An instructor sets attempts to one, sees "saved", and believes
something is being enforced. I split the settings out to keep the job pull request small, and the split quietly shipped a promise before
the thing that keeps it.

I did not want to fold the columns back into the job's pull request, because that undoes the point of splitting. The commit that followed
gates the whole block behind a Flipper flag, so the form does not show it unless autograding is switched on for the
deployment. That keeps the pieces separate and keeps the promise honest: until the enforcing pull requests merge, nobody can make it.
The alternative of adding each column in the pull request that reads it was the reviewer's first suggestion and is the better long term
rule. It would have meant re-cutting the stack, and I would rather say that than pretend the flag is the same thing.

A default that defeats its own constraint

GradingResult stores the outcome of one grading attempt: a copy of the suite it ran against, the per test breakdown, and a score
validated to sit between 0 and 100 when present. The reason for the copy is that an instructor can edit a suite after students have been
graded, and a result that only points at the live suite would show a grade next to a test it was never run against.

CodeRabbit found a flaw in the migration that I would not have found by testing through the model:

if a database insert omits suite_snapshot, this default stores {} instead of rejecting the result. Rails insert_all skips model
validations, so such a result can be persisted without the suite context needed to interpret it after the live suite changes.

I had written the column as null: false, default: {}, and the default is exactly what lets a bad row in.
Validations live in the model and insert_all does not call the model, so the only guard that applies to every write path is the one in
the database, and default: {} switches that guard off. Dropping the default means an insert that forgets the snapshot fails loudly at
the database, which is the whole reason the column exists.

The job, and the four questions CodeRabbit asked it

AutogradeJob is the piece that finally connects submission to grade. It is triggered after the deadline job forks a student's project
into a submission, and only when the assignment has a testbench. It runs the runner and stores a GradingResult:

def perform(project_id)
  project = Project.find_by(id: project_id)
  assignment = project&.assignment
  testbench = assignment&.testbench
  return unless testbench

  max = assignment.max_attempts
  return if max && GradingResult.where(project: project).count >= max

  begin
    result = Autograder::Runner.call(project: project, testbench: testbench)
    breakdown = result.groups
    score = (result.score * 100).round(2)
  rescue Autograder::Runner::RunnerError
    breakdown = []
    score = nil
  end

  GradingResult.create!(project: project, suite_snapshot: testbench.data,
                        breakdown: breakdown, score: score)
end

I chose a background job over running the grade in the request because the runner call is a network request with real latency and
timeouts, and a submission should not wait on it. The review found four different ways the seams in this short method fail.

It enqueues before the data exists. The deadline job calls this from inside assignment.with_lock. With default settings Rails
enqueues immediately, so a Sidekiq worker can start before the transaction commits, Project.find_by returns nothing, and the first
line quietly returns. The submission is never graded and nothing says why. The fix is to enqueue after commit, either through
enqueue_after_transaction_commit on the job or an after commit callback.

An outage uses up an attempt. If the runner is down, the rescue stores a result with a nil score. With max_attempts at one,
that row is counted by the next run's limit check, so a student who submitted once and hit an outage can never be graded. A failed run
is not an attempt the student made.

It can run twice. Sidekiq can execute the same job more than once. Two executions can both pass the count check before either has
written a row, so one submission ends up with two results and exceeds its attempt limit. The count and the write are not one atomic
step.

It ignores a setting I had just added. The reviewer noticed that with partial_credit: false from #7926, one passing case out of
two still stores 50 percent. The job was written before the setting existed, and neither pull request mentions the other. The fix
landed in a follow up commit on the 12th, a few lines that turn a partial score into all or nothing.

Two of those four are problems with the job. The other two, the outage and the partial credit rule, are problems with what the job
assumes the neighbouring pieces mean. That ratio is the point of this week.

Making the raw number mean something

AutogradeJob stores a fraction between 0 and 1, and a stored 0.75 does not say whether the assignment is graded on a percentage, a
letter scale or not at all. GradeScaleMapper converts it:

LETTER_THRESHOLDS = [[90, "A"], [80, "B"], [70, "C"], [60, "D"], [50, "E"]].freeze

def self.call(assignment, fraction)
  return unless mappable?(assignment)

  percent = (fraction * 100).round.clamp(0, 100)
  case scale_for(assignment)
  when "letter" then LETTER_THRESHOLDS.find { |min, _| percent >= min }&.last || "F"
  else percent.to_s
  end
end

def self.scale_for(assignment)
  assignment.lti_integrated? ? "percent" : assignment.grading_scale
end

An LTI integrated assignment is always treated as a percentage, whatever its grading_scale says, because that is the constraint the
LMS side already imposes. A grade that would have to be a letter in one system and a number in the other is a bug waiting for a
gradebook. no_scale returns nil, and the job checks GradeScaleMapper.mappable? before it runs the runner at all, since there is
no point spending a simulator call on an assignment that will not be graded.

I considered storing the mapped grade on the result row instead of computing it. I kept the raw fraction because the scale is an
assignment setting that an instructor can change, and the stored data should not go stale when they do.

The runner's fixes

The two configuration findings from last week landed on the 29th. The default URL is gone, so a missing SIMULATOR_RUNNER_URL raises
KeyError instead of posting to localhost, and the request now carries a bearer token from SIMULATOR_RUNNER_TOKEN:

HTTP.timeout(**TIMEOUTS)
    .auth("Bearer #{ENV.fetch('SIMULATOR_RUNNER_TOKEN')}")
    .post(endpoint, json: { circuit: circuit, testbench: testbench.data })

The response validation findings, the null body and the string "false", are still open.

Where the project stands

The pipeline now exists end to end as code: a suite is authored, a submission triggers a job, the job calls the runner, the result is
stored with the suite it ran against, and the score can be expressed on the scale the assignment uses. It does not yet exist as
something I would trust, because the seams I listed are mostly still open: enqueue after commit, outage versus attempt, idempotency.
Those are not hard fixes. They are the fixes you only find by reading the code next to its neighbours.

Next week: a student sees the result, and the first question is how much of the suite they are allowed to see.