Skip to content

Add opt-in exclusive model-container ownership - #29

Open
mccoyspace wants to merge 1 commit into
sqliteai:mainfrom
mccoyspace:pr/posix-model-lock
Open

Add opt-in exclusive model-container ownership#29
mccoyspace wants to merge 1 commit into
sqliteai:mainfrom
mccoyspace:pr/posix-model-lock

Conversation

@mccoyspace

@mccoyspace mccoyspace commented Aug 7, 2026

Copy link
Copy Markdown

Problem

A dedicated WASTE host may want one process to own a large model container so
that a second cooperating launch is refused before model-sized allocation.
This is host policy rather than a data-safety or RAM-accounting invariant:
containers are read-only, and container identity does not describe total
machine memory pressure.

Change

  • Add opt-in waste_cfg.exclusive_open and --exclusive-open flags for the
    CLI and server; ordinary concurrent opens remain the default.
  • Take a non-blocking advisory POSIX flock on the container directory before
    planning or model-sized allocation when exclusivity is requested.
  • Share a device/inode-keyed, reference-counted ownership entry between
    contexts in the same process.
  • Return WASTE_E_BUSY only for actual EWOULDBLOCK/EAGAIN contention.
    Directory-open, fstat, unsupported-flock, and other locking failures
    proceed without ownership, leaving model readability to the normal loader.
  • Release ownership on the last close and on planning, budget, and partial-load
    failures; use close-on-exec and clear inherited registry state after fork.
  • Leave Windows lifecycle behavior unchanged.

The lock is advisory between cooperating WASTE processes. It is not RAM
accounting, a filesystem lease, or a security boundary.

Correctness

The focused process test covers:

  • default concurrent behavior and opt-in exclusivity;
  • same-process reference sharing and last-close release;
  • competing exec, default bypass, and fork handling;
  • search-only-directory fail-open behavior;
  • under-budget, malformed-plan, and partial-load cleanup paths; and
  • close-on-exec behavior.

Current upstream-base validation on macOS:

  • tests/run.sh /nonexistent: 31 passed, 0 failed, 13 skipped;
  • server suite: 168 checks;
  • manual CLI and server contention diagnostics; and
  • default CLI open while an independent process holds the directory lock.

The lock is acquired once per context lifetime and is absent from the token
path, so no throughput claim is made.

Compatibility

There is no container-format, arithmetic, routing, state, or I/O change.
Concurrent opens remain the default. Version 0.6.7 and its CHANGELOG entry
record the appended waste_cfg field and new status; pre-1.0 binary clients
must rebuild, matching the existing ABI policy.

Rollback

Hosts that do not pass --exclusive-open retain the prior behavior. Reverting
this one commit removes the optional ownership mechanism and public status.

@marcobambini marcobambini left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the implementation is careful, and the test is the best
part of the PR: verifying ownership with an independent flock rather than
trusting the library's own return, and using exec instead of relying on
fork's copied registry, is exactly the right paranoia. I built the branch and
ran it: tests/run.sh /nonexistent gives 37 passed, 0 failed, 9 skipped on an
M-series macOS host, and test_lock passes.

Two things need to change before this can land: one defect, and the polarity
of the default.

The lock turns any locking failure into an open failure

In model_lock_acquire, only EWOULDBLOCK/EAGAIN becomes WASTE_E_BUSY.
A failing open() on the directory, a failing fstat(), and every other
flock errno become WASTE_E_IO, and the container cannot be opened at all:

$ chmod 0111 tiny.waste          # directory traversable but not readable
$ ./waste info tiny.waste
open: I/O error
$ ./waste info tiny.waste --allow-concurrent-open
WASTE 0.6.6 (container v0, backend NEON, crc32 armv8, arm64)   # and on main

A search-only model directory is a legitimate way to publish a shared
read-only tree, and it is the mild version of the problem. The serious one is
that flock is absent or returns ENOTSUP/EOPNOTSUPP/ENOLCK on several
FUSE mounts (third-party exFAT and NTFS drivers on macOS), on SMB, and on
some NFS configurations — which is precisely the kind of external disk this
project puts a 982 GB container on. The user gets I/O error and nothing
that points at the new flag.

The fix is small: treat contention as EWOULDBLOCK/EAGAIN only, and let
every other failure proceed without ownership instead of failing the open.
Whether the container is actually readable is still decided two lines later by
waste_plan_memory, with the same WASTE_E_IO that has always meant that.

The default should be inverted: opt-in exclusivity, not opt-out

The mechanism is fine. Having it on by default is the part I do not think
survives contact with how the engine is used.

It guards the wrong identity. The hazard you describe is RAM
oversubscription, and container identity is neither necessary nor sufficient
for it. Two processes opening different containers oversubscribe exactly as
badly and are untouched by this. Two processes opening a small container —
Kimi-Linear on a 128 GB workstation — do not oversubscribe at all, and are now
refused. The predicate that actually predicts the harm is resident bytes
against waste_usable_ram(), which waste_open already computes; the
container's inode is a proxy that is wrong in both directions.

It cannot hold the invariant it claims, so it should not be a default.
The lock is advisory and only binds cooperating WASTE processes. Any other
memory-hungry process on the machine, any WASTE process that passed the
opt-out, any second container — all unprotected. A default that stops the
tidy case and misses the untidy ones buys less than it costs.

There is no data hazard to justify it. A container is read-only, opened
O_DIRECT, never written by the engine. Nothing about two readers is unsafe;
the only cost is performance, on a machine the operator owns and can observe.
Refusing an operation the OS would happily allow, on an artifact where
concurrency is harmless, is a strong claim for a library to make on its host's
behalf — and CLAUDE.md's library-first rule puts exactly this class of
decision (like logging, signal handling and argument parsing) with the host.

The failure modes are asymmetric, and this inverts them the wrong way.
Today, a mistaken second load is visible and recoverable: throughput drops,
the operator notices, kills one. After this change, a legitimate second load
is invisible and blocking: a workflow that worked yesterday now stops with a
message about a flag nobody has heard of, and on a filesystem without flock
support it stops for a reason unrelated to concurrency at all. Trading a
self-announcing performance problem for a silent availability problem is a bad
trade even when the performance problem is real.

It breaks ordinary single-machine workflows. With the default on,
waste info MODEL, waste bench MODEL, and a second python3 -m serve MODEL
all fail while a server is up (verified: open: container is already open in another process). Anyone with one machine and one container hits this on the
first day — including inspecting the container that is currently being served,
which is a read-only question that never needed a model load to be exclusive.

And the polarity is a one-way door. Shipping opt-in and promoting it to
default later is a normal deprecation. Shipping default-on and retreating is a
behavioral break for every host that had been opening the same container more
than once.

Concretely, I would keep everything in this PR and change one thing: rename the
field to waste_cfg.exclusive_open with the sense reversed, and the flags to
--exclusive-open on the CLI and the server. Same code, same registry, same
refcounting, same test — test_lock only needs its allow arguments flipped.
Hosts that genuinely want single-owner semantics (a workstation daemon that
owns its model) set one field and get the behavior you built; everybody else
keeps working. If you want the RAM-pressure story addressed on its own terms
afterwards, the place for it is the budget arithmetic in waste_open, not the
container's inode — that would catch the different-container case this misses,
and it is worth a separate PR.

Also needed

  • Version bump and a CHANGELOG entry. allow_concurrent_open is appended
    to waste_cfg and there is a new enumerator, so the ABI moved. 0.6.6 set the
    precedent for cpu_list ("Callers must recompile against this header"), and
    per CLAUDE.md the changelog is updated in the same commit that bumps
    WASTE_VERSION_*. This case is worse than cpu_list: a binding built
    against 0.6.6's struct passes four bytes short, and waste_open reads the
    new field from past the end of the caller's object — garbage that happens to
    be non-zero silently disables the lock, so the misread fails quiet and in
    the direction that defeats the feature. The serve/engine.py mirror is
    updated correctly.

  • tests/run.sh discards the diagnostics. ./test_lock ... 2>/dev/null
    throws away exactly the FAIL line N: ... lines the test emits, so a CI
    failure is one line with no assertion behind it. The else branch also
    reports FAIL when make_test_container.py is what failed — a missing
    prerequisite, which this suite reports as SKIP by convention.

  • Nothing suggests the opt-out on WASTE_E_BUSY. Both cli/main.c and
    serve/__main__.py already have the block that turns an opaque status into
    advice (the --cpus pair); this is the case that needs it most.

Minor

  • pthread_atfork handlers cannot be unregistered, and libwaste is loaded as a
    shared object through ctypes. A host that ever dlcloses it will jump into
    unmapped memory on the next fork. Worth a note next to model_lock_init.
  • flock treats two open file descriptions in one process as independent, so a
    process can exclude itself in narrow cases where the (dev, ino) identity
    changes underneath it. The registry covers the normal path; this deserves a
    comment rather than code.
  • child_probe execs argv[0], which is fine from run.sh and fragile if the
    binary is ever invoked through PATH.

What is right

I checked every return in waste_open after acquisition — there are exactly
three (plan, budget, model load) and all three release. (dev, ino) identity
instead of path strings, refcounting that preserves the documented
multiple-context behavior, close-on-exec with the fcntl fallback, and the
lock kept out of the token path are all the right calls. The comments explain
the failure that motivates them, which is the house style.

@marcobambini

Copy link
Copy Markdown
Member

Follow-up to the review: the RAM half of this now has its own home —
#31, "Two auto-budget opens on one machine each size against the whole
machine".

Tracing it back, the second-open case is your observation. #14's correction
— that the original 80.64 GiB paging sample came from a second WASTE model
open after budget selection — is where it enters the record, and #15
deliberately scoped itself to pressure already present at waste_open. So the
gap this PR is aimed at is real and was left open on purpose; it is the key
that I do not think holds, not the concern.

#31 sets out what is already settled (capacity via min(physical, cgroup);
instantaneous MemAvailable/memory.current refuted in src/memory.c), the
question 0.6.5's changelog left open in as many words — whether pressure
should trim the working-set multiplier rather than the ceiling — and gates in
cheapest-first order, starting with actually measuring two concurrent K3 opens
on a 64 GB machine, which has never been done on purpose. There is a table
there showing why the inode is wrong on three of four configurations,
including the different-container case a lock cannot see.

If you want to take gate 1 or 2 on the GN100, that is the part of this whose
answer nobody currently has. Nothing in #31 blocks this PR: the errno fix and
the inverted default stand on their own.

Take a non-blocking advisory POSIX lock before model planning and allocation when the host requests exclusivity. Share it across same-process contexts and release it on every open failure and the final close. Fail open when locking is unavailable, while returning WASTE_E_BUSY for actual contention. Expose the opt-in through the C API, CLI, and server, with focused lifecycle and fallback tests.
@mccoyspace
mccoyspace force-pushed the pr/posix-model-lock branch from 293d06e to bfd78c8 Compare August 10, 2026 04:17
@mccoyspace mccoyspace changed the title Prevent competing processes from opening one container Add opt-in exclusive model-container ownership Aug 10, 2026
@mccoyspace

Copy link
Copy Markdown
Author

Thanks for the detailed review. I agree with the distinction between optional
container ownership and RAM accounting, and have revised the single commit at
bfd78c8 accordingly:

  • inverted the API and flags to opt-in exclusive_open / --exclusive-open;
  • made every non-contention locking failure proceed without ownership, with a
    search-only-directory regression case;
  • bumped to 0.6.7 and added the ABI/behavior CHANGELOG entry;
  • retained test_lock diagnostics and report synthetic-container failure as
    SKIP;
  • added actionable WASTE_E_BUSY guidance in both CLI and server;
  • added the pthread_atfork/dlclose and changing-identity comments; and
  • changed the child probe to execlp for PATH invocation.

Current Mac validation is 31 passed, 0 failed, 13 skipped, including the
focused lock test, plus all 168 server checks. I also manually verified the
CLI/server contention advice and that a default open still succeeds while an
independent process holds the directory lock.

I have kept the RAM-budget question in #31 separate. We can add the GN100 gate
1/2 measurements there later without coupling them to this PR.

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.

2 participants