Skip to content

fix: one worker per session, and a queue that stops leaking its own answer - #20

Merged
imkp1 merged 2 commits into
mainfrom
fix/capture-race-and-cleanup
Aug 2, 2026
Merged

fix: one worker per session, and a queue that stops leaking its own answer#20
imkp1 merged 2 commits into
mainfrom
fix/capture-race-and-cleanup

Conversation

@imkp1

@imkp1 imkp1 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Eight defects found in a full read of the codebase, then the dead and duplicated code the review turned up on the way through. Releases as 0.1.0-rc7.

Every finding below was reproduced before it was fixed, and every removal was verified inert before it was removed.

The one a developer feels

has_session and begin_session are two statements, so two capture workers could both pass the first — SessionEnd firing twice, or a corpus run overlapping the live hook, is enough. Both then ran the whole four-call pipeline and both wrote. record_session no-ops for the loser, but add_seed and add_probe had no such refusal:

two workers past the guard  →  sessions: 1   seeds: 2   probes: 2

The same question asked twice, billed twice. begin_session now returns whether the caller owns the session, settled by the INSERT because that is the only step sqlite makes atomic, and the loser returns before the first model call. The conflict clause tests staleness as well as capturing, so all four cases separate: fresh insert wins, live marker loses, stale marker is taken over, terminal verdict loses. Taking over a dead worker's marker is unchanged — a claim with no way out jams a session forever, which is worse than the double capture it would prevent.

The rest

  • serve no longer sends the probe's topic. It states why the probe was raised, which is the bridge to its answer. SKILL.md was told to keep it out of the picker and the payload shipped it anyway. Absent now, the way the transcript is absent from verify's signature rather than its prompt.
  • pending: true on the full payload — it was null in one arm and missing in the other, and both surfaces branch on it.
  • settings.json is written atomically (temp + rename) and refused when unparseable. install used to raise a JSONDecodeError as a traceback on the first command a new user runs, having already written half the configuration. It now reads settings before writing the skill.
  • WAL and a 30s busy timeout, so serve stops queueing behind a detached worker's commit and timing out in front of the developer. grask.log rotates at 1 MB.
  • Stage 4's duration reaches the probe row on the path where its call failed, matching the cost that already did — the invariant verify folds the two together to preserve.
  • grask stats — the developer's own record. Every number grask kept, it kept for whoever tunes the pipeline; the person being asked could not see the questions they had answered without opening sqlite. Read-only, free, and no percentage: one probe cannot identify understanding.
  • CI measures coverage against a floor of 84%, with nothing excluded to flatter the number (it would read 91 without the two operator scripts).

Removed — nothing here changes what grask does

  • Turn.timestamp and its parser. Written by both read paths, read by nothing.
  • The reply length cap. Clipped at 4000 leaving the transcript and again at 2000 entering the prompt; only the second could ever bind, since clip(x, 4000)[:2000] == x[:2000] for every string. Edits keep theirs — nothing downstream clips those.
  • Interrogation.confidence. Could only ever be None. The column stays: older rows hold real numbers.
  • Dialogue.rendered_bytes and unprobed_seeds(within_days=…) — one never called, one never given anything but its default.

Unified

The option alphabet (declared twice, at two different lengths), the option cap (twice), the probe-expiry cutoff (four times), and "is the hook wired" (twice, with the two copies already disagreeing about which errors mean no).

Verification

440 passed (was 408)   coverage 84.62% ≥ floor 84
ruff: All checks passed        mypy --strict: no issues, 20 files
3.8 floor job: 440 passed

Also exercised by hand against a copy of a real 484-session database: stats reads back correctly, serve consumes nothing, and the legacy confidence column still holds its values.

🤖 Generated with Claude Code

imkp1 and others added 2 commits August 3, 2026 00:09
…nswer

Eight defects, then the dead code the review turned up on the way through.

The one a developer feels: `has_session` and `begin_session` are two
statements, so two capture workers could both pass the first — SessionEnd
firing twice, or a corpus run overlapping the live hook, is enough. Both then
ran the whole four-call pipeline and both wrote. `record_session` no-ops for
the loser, but `add_seed` and `add_probe` had no such refusal, so one session
produced two seeds and two probes: the same question asked twice, billed
twice. `begin_session` now returns whether the caller owns the session,
settled by the INSERT because that is the only step sqlite makes atomic, and
the loser returns before the first model call. Taking over a marker left by a
worker that died is unchanged — a claim with no way out jams a session
forever, which is worse than the double capture it would prevent.

The rest:

- `serve` no longer sends the probe's topic. It states why the probe was
  raised, which is the bridge to its answer; the skill was told to keep it
  out of the picker and the payload shipped it anyway. Absent now, the way
  the transcript is absent from `verify`'s signature rather than its prompt.
- `pending: true` on the full payload. It was null in one arm and missing in
  the other, and both surfaces branch on it.
- `settings.json` is written atomically and refused when unparseable.
  `install` used to raise a JSONDecodeError as a traceback on the first
  command a new user runs, having already written half the configuration.
- WAL and a 30s busy timeout, so `serve` stops queueing behind a detached
  worker's commit; `grask.log` rotates at 1 MB.
- Stage 4's duration reaches the probe row on the path where its call failed,
  matching the cost that already did.
- `grask stats`: the developer's own record. Every number grask kept, it kept
  for whoever tunes the pipeline; the person being asked could not see the
  questions they had answered without opening sqlite. No percentage — one
  probe cannot identify understanding.
- CI measures coverage against a floor of 84%, nothing excluded to flatter it.

Then the sweep, verified inert before removal rather than after: a `Turn`
timestamp written by both read paths and read by nothing, a reply cap that no
input length could reach because a tighter one sat downstream of it, an
always-None `confidence` field (the column stays — older rows hold real
numbers), and two unused knobs. Four duplicates collapsed: the option
alphabet was declared twice at two different lengths, the option cap twice,
the probe-expiry cutoff four times, and "is the hook wired" twice with the
two copies already disagreeing about which errors mean no.

440 tests, ruff and mypy --strict clean, and the 3.8 floor still passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@imkp1
imkp1 merged commit 12e6947 into main Aug 2, 2026
10 checks passed
@imkp1
imkp1 deleted the fix/capture-race-and-cleanup branch August 2, 2026 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant