Skip to content

train: the start's claim is released on every exit that doesn't hand it to the worker - #58

Open
joelteply wants to merge 3 commits into
masterfrom
48029d24/train-claim-rollback
Open

joelteply wants to merge 3 commits into
masterfrom
48029d24/train-claim-rollback

Conversation

@joelteply

Copy link
Copy Markdown

Why

Continuum card 48029d24, found while reading the core's /train refusal contract for continuum ggml-org#5001.

server_trainer::start claims the run (running CAS to true) and releases the claim through refuse on every explicit refusal. But an exception thrown after the claim escaped with running still true. Two examples:

  • a json::type_error from a body field ("yield" is read with body.value("yield", true));
  • a std::system_error from std::thread.

ex_wrapper then answered 500, and /train kept naming the run "preparing". Every later start was refused as "a training run is already in progress" until the server restarted.

What

  • A claim_guard declared right after the claim releases it unless refuse or the hand-off to the worker has settled it. Releasing means state "error" under this run's out, then running = false.
  • The flag is explicit (settled), so the guard never releases a later run's claim.
  • It's declared before the unique_lock on mu, so unwinding drops that lock before the guard takes mu.

Test

test-chat --train-control (the existing CPU integration check) now also sends a start with "yield": "not a bool", which throws json::type_error after the claim. It asserts:

  • the status is "error" under that run's own out;
  • the next start is accepted.

Not built locally (IntelMac doesn't build the engine), so this PR's CI is the first build.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FRQWzgSfo79JtnwZywHKrE

…it to the worker, so an exception can't leave /train refusing every run until a restart

Continuum card 48029d24, found reading the core's refusal contract (ggml-org#5001).
server_trainer::start claims the run (running CAS to true) and releases the
claim through `refuse` on every explicit refusal. An exception thrown after the
claim escaped with running still true: a json::type_error from a body field
(e.g. "yield" read with body.value(.., true)), or std::system_error from
std::thread. ex_wrapper answered 500, /train named the run "preparing" forever,
and every later start was refused as "a training run is already in progress"
until the server restarted.

A claim_guard declared right after the claim releases it (state "error" under
this out, running=false) unless `refuse` or the hand-off to the worker settled
it. The explicit flag means it never releases a later run's claim. It's
declared before `lock`, so `lock` (on mu) is gone before the guard takes mu.

test-chat --train-control: a start with "yield": "not a bool" throws after the
claim. The status is then "error" under its own out, and the next start is
accepted.
…rd only after it released the claim

Codex's review on #58 (023a509), request changes:
- refuse set claim.settled first, before building the error state and
  clearing running. A throw there disarmed the rollback with running still
  true. settled is now set only after the release.
- The guard's destructor built and assigned allocating JSON while unwinding.
  A throw there would hit the implicit noexcept and terminate the serving
  process. The error state is now built BEFORE the claim (out is validated
  above it), and the destructor (explicitly noexcept) only moves it in, inside
  try/catch with the lock, then releases running unconditionally.
  Recording the diagnostic is best-effort; releasing the claim is not.
- test-chat --train-control also covers the refusal path: "examples": [] with
  no "text" is refused after the claim, leaves its error under its own out,
  and the next start is accepted.
… the file

ubuntu CI on 729d950: 'expected unqualified-id before &' at test-chat.cpp:7292.
In this file json is common_json, which has no nested exception type, and every
other catch here uses std::exception. nlohmann's type_error derives from it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant