Skip to content

feat: a prerequisite stage, and the EventLog corruption it uncovered - #49

Closed
sotashimozono wants to merge 3 commits into
feat/deadline-attempts-blaswarnfrom
feat/prerequisite-stage
Closed

sotashimozono wants to merge 3 commits into
feat/deadline-attempts-blaswarnfrom
feat/prerequisite-stage

Conversation

@sotashimozono

Copy link
Copy Markdown
Member

Closes #46.

Stacked on #48. Merge that first.

The prerequisite stage

run! locks the KEY, so work shared between keys has nowhere to live but inside work_fn, where it has no protection at all: every worker that wants a setup not yet on disk builds it itself.

run_loop!(work_fn, vault, keys;
          prerequisite = Prerequisite(prep_fn, prep_vault, derived_keys))

The setup becomes its own key space and gets the same locking, resume and provenance as any other stage.

Measured, 8 concurrent processes

16 keys sharing 2 setups (8 keys per fibre), gated so the processes start together:

setup builds
check-then-build inside work_fn 16
prerequisite stage 2

(With a 2-key fibre the same experiment gives 2 against 1: the duplication ceiling is fibre width, which is why the wide fixture is the one worth quoting.)

The correctness argument is the stronger one

work_fn's wall clock included the setup for one key in each fibre and excluded it for the other ~199. A cost claim built on that number depended on scheduling luck. With the setup in its own stage its cost is recorded in its own payload, and the dependent timings are clean.

It is a barrier, not a work loop

That is the whole difference from run_loop!, which stops after max_empty_rounds empty rounds. A round that finds every remaining key held by a sibling sleeps and goes again, because the dependent stage cannot start until the setup exists. Terminates on: all done; no progress and nothing held by a sibling; stop_flag; deadline.

If the prerequisite does not complete, the dependent stage does not start. Running it anyway would spend the allocation on keys whose setup is known to be missing, which is what Preflight already exists to prevent one layer up.

Not a DAG. SweepRunner does not know which dependent key needs which prerequisite key; that is resolved inside work_fn. This is "all of the prerequisite, then all of the dependents", and the docstring says so.

run_loop! now returns (; ran, rounds, done, stopped_by, prerequisite) instead of nothing.

EventLog corruption, found on the way

The :key_acquired event from #48 roughly doubled the event rate and turned a rare failure in test_run_keylock.jl into a frequent one. It is not a flake and it is not new:

4 EventLog objects on one path, 300 events each, 4 threads
  before: 947 of 1200 lines, 82 malformed
  after:  1200 of 1200,      0 malformed

test_run_keylock.jl, 10 runs each
  base branch (no key_acquired):  2/10 tore a line
  with key_acquired:              3/10
  with this fix:                  0/10

run! builds a fresh EventLog on every call, so four concurrent masters in one process hold four objects pointing at one file and four different ReentrantLocks. The documented "serialized through an internal ReentrantLock" was false in exactly the configuration the repo's own tests use, and events were being lost, not merely spliced.

Two fixes, both needed:

  • the lock is now held per path, process-wide, so several EventLogs on one file share it;
  • the write goes to an unbuffered append descriptor. open(path, "a") gives an IOStream, which flushes on its own boundaries, so write(io, line) was never the single syscall the O_APPEND guarantee is about. The docstring claimed it was.

The regression test covers both halves: threads within one process, and four separate processes appending to one file.

Verification

All 20 test files under test/, green. New: test_prerequisite.jl (33 assertions), test_eventlog_shared_path.jl (9).

The prerequisite tests carry their controls:

  • a testset asserts the setup is built once per setup key; the next one asserts that without the prerequisite the same fixture does duplicate, so the first is not passing for having nothing to prevent;
  • the barrier test holds one setup key with a live sibling lock that is never released and asserts the run waited (pre.waited >= 1, elapsed past the poll) and was ended by the deadline, rather than returning early the way run_loop! would.

Patch bump, 0.6.2 -> 0.6.3.

🤖 Generated with Claude Code

Closes #46.

## Prerequisite

`run!` locks the KEY, so work shared BETWEEN keys has nowhere to live but inside `work_fn`, where
it has no protection at all: every worker that wants a setup not yet on disk builds it itself.

    run_loop!(work_fn, vault, keys;
              prerequisite = Prerequisite(prep_fn, prep_vault, derived_keys))

The setup becomes its own key space and gets the same locking, resume and provenance as any other
stage, so its cost is recorded in its own payload instead of landing on whichever dependent key
happened to run first. That is the stronger half of #46: `work_fn`'s wall-clock included the setup
for one key in each fibre and excluded it for the other ~199, so a cost claim built on it depended
on scheduling luck.

Measured here, 8 concurrent processes over 16 keys sharing 2 setups (8 keys per fibre), gated so
they start together:

| | setup builds |
|---|---|
| check-then-build inside work_fn | 16 |
| prerequisite stage | 2 |

`run_prerequisite!` is a BARRIER, not a work loop, and that is the whole difference from
`run_loop!`. A round that finds every remaining key held by a sibling SLEEPS and goes again rather
than counting an empty round; the dependent stage cannot start until the setup exists. It
terminates on: all done; no progress AND nothing held by a sibling; the stop flag; the deadline.

If the prerequisite does not complete, the dependent stage does not start at all. Running it anyway
would spend the allocation on keys whose setup is known to be missing, which is what `Preflight`
already exists to prevent one layer up.

SweepRunner does NOT know which dependent key needs which prerequisite key: the dependency is
resolved inside `work_fn`. This is "all of the prerequisite, then all of the dependents", not a
DAG, and the docstring says so.

`run_loop!` now returns `(; ran, rounds, done, stopped_by, prerequisite)` instead of `nothing`.

## EventLog corruption, found on the way

The new `:key_acquired` event from #44 roughly doubled the event rate and turned a rare failure in
`test_run_keylock.jl` into a frequent one. It is not a flake and it is not mine:

    4 EventLog objects on one path, 300 events each, 4 threads
      before: 947 of 1200 lines, 82 malformed
      after:  1200 of 1200, 0 malformed

    test_run_keylock.jl, 10 runs
      base branch:        2/10 tore a line
      with key_acquired:  3/10
      with this fix:      0/10

`run!` builds a fresh `EventLog` on every call, so four concurrent masters in one process hold four
objects pointing at one file and four DIFFERENT `ReentrantLock`s. The documented "serialized
through an internal ReentrantLock" was therefore false in exactly the configuration the repo's own
tests use. Two fixes, both needed:

- the lock is now held per PATH, process-wide, so several `EventLog`s on one file share it;
- the write goes to an UNBUFFERED append descriptor. `open(path, "a")` gives an `IOStream`, which
  flushes on its own boundaries, so `write(io, line)` was never the single syscall the O_APPEND
  guarantee is about. The docstring claimed it was.

The regression test covers both halves: threads within one process, and four separate processes
appending to one file.

Verified locally: all 20 test files under test/, green.

Stacked on #48. Merge that first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the enhancement New feature or request label Sep 15, 2026
The per-path lock landed in the previous commit but did not reach the workers, which is where the
concurrency actually is. `run!` serialises the `EventLog` to every worker, and deserialization
rebuilds the struct WITHOUT running the constructor, so the `lock` field that arrives there is a
fresh private lock that serialises nothing against its siblings on that worker.

Measured: `deserialize(serialize(log)).lock === log.lock` is false, and `=== _path_lock(path)` is
also false.

`log_event` now looks the lock up by path on every call, so it is correct however the object got
there. The test asserts the field really does not survive serialization, then writes 200 events
from each of the original and the deserialized log concurrently and requires all 400 lines back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

📚 Docs preview: https://qatlashub.github.io/SweepRunner.jl/previews/PR49/

(updates on each push to this PR)

The EventLog deserialization test used Serialization without it being declared, which works in a
dev environment that happens to have it and fails in the isolated test env CI builds:
`ArgumentError: Package Serialization not found in current path` on shard s1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sotashimozono

Copy link
Copy Markdown
Member Author

Folded into #51.

The stack was one patch bump per pull request, each a single step above its own base, which is what version-check asks of a pull request. Merging them in sequence would have published a patch version to General for each one, including one whose content is test: Aqua. Collapsing the stack onto main ships it as one step instead.

Every commit from this branch is in #51 unchanged, so nothing here is lost; this body stays readable as the per-change account. The branch is kept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant