Skip to content

Prevent competing processes from opening one container - #29

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

Prevent competing processes from opening one container#29
mccoyspace wants to merge 1 commit into
sqliteai:mainfrom
mccoyspace:pr/posix-model-lock

Conversation

@mccoyspace

Copy link
Copy Markdown

Problem

Two WASTE processes can open the same large container and each begin
model-sized allocation before either discovers the resulting memory pressure.
On a unified-memory workstation this can turn an accidental second launch into
heavy reclaim or paging. Multiple contexts deliberately opened by one process
should continue to work.

Change

  • Take a non-blocking advisory POSIX flock on the container directory before
    planning or model-sized allocation.
  • Share a device/inode-keyed, reference-counted ownership entry between
    contexts in the same process.
  • Return WASTE_E_BUSY to a competing process.
  • 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.
  • Add waste_cfg.allow_concurrent_open, --allow-concurrent-open, and the
    corresponding server option for intentional multi-process hosts.
  • Leave Windows lifecycle behavior unchanged.

The lock is advisory and coordinates cooperating WASTE processes. It is not a
filesystem lease or security boundary.

Correctness

The focused process test covers:

  • same-process reference sharing and last-close release;
  • competing exec, explicit opt-out, and fork handling;
  • under-budget, malformed-plan, and partial-load cleanup paths; and
  • close-on-exec behavior.

Current upstream-base validation:

  • macOS: 31 passed, 0 failed, 13 skipped; server suite 168 checks;
  • Linux/AArch64 on an NVIDIA DGX Spark: 30 passed, 0 failed, 13 skipped;
    server suite 168 checks, including the v0.6.6 CPU-binding test.

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. The
new status and appended waste_cfg field require pre-1.0 binary clients to
rebuild; the in-tree CLI and Python ctypes layout change together. Windows
ignores the opt-out because the ownership mechanism is POSIX-only.

Rollback

--allow-concurrent-open preserves intentional multi-process behavior.
Reverting this one commit removes the default lock and public status.

Take an advisory POSIX lock before model planning and allocation, share it across same-process contexts, and release it on every open failure and the final close. Expose an explicit opt-out through the C API, CLI, and server while leaving Windows behavior unchanged.

@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.

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