Skip to content

refactor(jobs): group Job's scalar state by lifetime - #9915

Open
boscocp wants to merge 1 commit into
esengine:main-v2from
boscocp:refactor/jobs-scalar-state
Open

refactor(jobs): group Job's scalar state by lifetime#9915
boscocp wants to merge 1 commit into
esengine:main-v2from
boscocp:refactor/jobs-scalar-state

Conversation

@boscocp

@boscocp boscocp commented Sep 7, 2026

Copy link
Copy Markdown

Summary

Job carried 18 scalar fields behind one mutex — six over repolint's
struct-state ceiling of 12, and recorded as debt in baseline.json. The
rule's point is that independent scalars multiply: resultRead, runReturned
and stalled alone describe eight states, most of which cannot legally happen,
and nothing in the type says which ones can.

Grouped by lifetime into two structs in a new internal/jobs/jobstate.go:

  • jobClockstartedAt/finishedAt/activityAt plus the stalled latch
    that activityAt drives. Keeping the latch next to the stamp it derives from
    is what stops the two from describing different jobs.
  • jobOutcome — the run's text, whether the run returned, and whether Output
    already surfaced the text. All three are written once as the job terminates
    and read together afterwards.

Both stay nested in Job under the same j.mu, and neither takes a lock of
its own, so the concurrency contract is unchanged. Job drops to 11 scalars
and the struct-state finding to zero.

No behavior change. Every renamed field is unexported, so the public surface
(View, Result, Status, the Manager methods) is byte-identical and
nothing outside internal/jobs changes. Two comments that named the old
fields follow the rename — OutputForSession's note on where a task's answer
lands, and the surfaced-once flag's own line, which also stops citing a buf
field the type has not had in some time.

What this deliberately leaves alone

The five artifact fields (artifactPath, artifactMetaPath,
artifactComplete, artifactErr, tombstone) are an obvious second group by
the same argument, and grouping them would take Job from 11 scalars to
about 6. I left them out on purpose: they are already visually blocked in the
struct, they are the ones a reader is most likely to be looking for by name,
and folding them in would roughly triple the diff for a finding that is
already at zero. Happy to do it as a follow-up if you want the whole struct
converted at once.

Rebased onto the jobs.go split

This branch was rebased onto current main-v2 after #9896/#9897 moved
StartForSession into internal/jobs/start.go and the runtime observers into
internal/jobs/runtime_state.go. The two changes are orthogonal — that work
extracted functions, this one regroups fields — but they overlap textually in
jobs.go, which is what conflicted.

The resolution takes main-v2's jobs.go wholesale and re-applies the field
regrouping on top, so five of the accesses now land in start.go instead
(j.outcome.returned, j.outcome.text, j.clock.finishedAt and the
jobClock literal in the job constructor). Job itself is unchanged from
main-v2 apart from the two grouped fields.

Why a separate file

jobs.go is 1956 lines against the 800-line ceiling, and REASONIX.md asks
for one responsibility per file. Declaring the two types inline would fit
inside the recorded file-size budget — that entry still reads 1271 from
when the file was 2071 lines, so there are ~124 lines of slack — but spending
that slack to grow the most over-budget file in the package works against the
ratchet. Putting them in their own 23-line file moves the other way:
jobs.go nets nine lines smaller (1956 -> 1947).

To be explicit, since an earlier revision of this description claimed
otherwise: on the current base, inline placement would not trip the
file-size gate. The separate file is a judgement call about which direction
the debt should move, not a hard constraint.

Baseline diff

Hand-edited rather than regenerated. Two logical changes: the struct-state
key is removed from internal/jobs/jobs.go, and limits.struct-state goes
115 -> 109, exactly the six this change removes. go run ./tools/repolint is
clean both before and after the baseline edit, so the ratchet only tightens.

-update would have swept in unrelated tightening across other files whose
debt shrank in already-merged work. That seemed worse for review than editing
the two numbers this change owns, but happy to regenerate wholesale if you
prefer.

The two comment corrections

  • Session.Rewrite's example reasons cited "compact_auto" and "snip". No
    caller passes either; the only reasons reaching it are rewind_truncate and
    rewind_restore (internal/control/rewind.go) and guardian_merge
    (internal/guardian/guardian.go) — re-verified against current main-v2.
    Both strings are still live in the cache-diagnostics vocabulary
    (internal/cli/run_metrics.go, cmd/e2ebench), which is presumably where
    the example drifted in from.
  • NeedsRewriteSave attributed the in-place rewrite to "compaction, prune",
    but compaction installs a projection and never touches Session.Messages
    (CompactNow: "canonical transcript untouched"). rewriteVersion is bumped
    from about a dozen paths, so I dropped the parenthetical rather than swap in
    another list that would go stale too.

I deliberately left SaveSnapshot's mention of compaction alone: that one is
about which save method to call, and the controller genuinely does call
SaveRewrite after a manual compaction.

Happy to split these two out if you would rather keep the diff to
internal/jobs.

Verification

Re-run after the rebase:

  • go run ./tools/repolint: clean, against the tightened baseline.
  • gofmt -l ., go vet ./..., go build ./...: all clean.
  • go test -race -run TestManagerConcurrentAccess -count=5 ./internal/jobs/:
    ok. This is the load-bearing check — the existing stress test drives every
    public Manager method from 24 goroutines, so a field that escaped j.mu
    during the regrouping would trip the race detector or the runtime's
    concurrent-map fatal.
  • go test -race ./internal/jobs/: ok, including the runtime_state and
    ownership_completion regression tests that arrived with the split.
  • golangci-lint run ./... at the pinned v2.12.2: 0 issues. Needed
    GOTOOLCHAIN=go1.26.6 locally, since on Go 1.27 the bundled staticcheck
    panics in buildir on the stdlib internal/poll — unrelated to this diff.
  • go test ./internal/agent/ ./internal/tool/builtin/ ./internal/boot/: ok.
  • The desktop module builds and its subpackages pass; its main package needs
    webkit2gtk-4.0, which this machine lacks. It consumes only the exported
    jobs API, so it cannot reach the renamed fields.

Documentation-impact: none - internal refactor plus four comment corrections; no
behavior, flag, or documented output changes.

@github-actions github-actions Bot added v2 Go rewrite (1.x) — main-v2 branch, active development agent Core agent loop (internal/agent, internal/control) labels Sep 7, 2026
Job carried 18 scalar fields behind one mutex, six over repolint's
struct-state ceiling of 12. Independent flags multiply into combinations no
type records as legal — resultRead, runReturned and stalled alone describe
eight states, most of which cannot happen — and a recorded debt means the
next boundary case can be fixed by adding a ninth.

Two groups by lifetime, in a new jobstate.go:

  jobClock    startedAt/finishedAt/activityAt plus the stalled latch that
              activityAt drives, so the latch cannot outlive its stamp.
  jobOutcome  the run's text, whether the run returned, and whether Output
              already surfaced the text — all written once as the job
              terminates and read together afterwards.

Both stay nested in Job under the same j.mu and neither takes a lock of its
own, so the concurrency contract is unchanged. Job drops to 11 scalars and
the struct-state finding to zero, so the baseline entry is removed rather
than raised.

The two types live in their own file because jobs.go is already 1956 lines
against the 800-line ceiling, and one responsibility per file is the rule.
Inline would still fit the recorded file-size budget -- that entry reads 1271,
from when the file was 2071 lines -- but spending that slack to grow the
package's most over-budget file works against the ratchet. A separate 23-line
file moves the other way: jobs.go nets nine lines smaller.

Every renamed field is unexported, so nothing outside internal/jobs changes.
The two comments naming the old fields follow the rename: OutputForSession's
note on where a task's answer lands, and the surfaced-once flag's own line,
which also stops citing a "buf" field the type has not had in some time.

Also corrects two comments in internal/agent that no longer describe the
code. Session.Rewrite's example reasons cited "compact_auto" and "snip",
which no caller passes — the only reasons reaching it are rewind_truncate,
rewind_restore and guardian_merge. NeedsRewriteSave attributed the in-place
rewrite to "compaction, prune", but compaction installs a projection and
never rewrites Session.Messages; rewriteVersion is bumped from a dozen other
paths, so the parenthetical is dropped rather than replaced with a new list.
@boscocp
boscocp force-pushed the refactor/jobs-scalar-state branch from 849e2ad to 572618e Compare September 9, 2026 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Core agent loop (internal/agent, internal/control) v2 Go rewrite (1.x) — main-v2 branch, active development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant