diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 541d5c1..4eec3bc 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -72,6 +72,12 @@ "description": "Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers", "source": "./plugins/trogonstack-protobuf", "category": "development" + }, + { + "name": "trogonstack-concurrency", + "description": "Concurrency skills for finding and fixing time-of-check to time-of-use races in files, databases, caches, authorization, and distributed stores", + "source": "./plugins/trogonstack-concurrency", + "category": "development" } ] } diff --git a/.cursor-plugin/marketplace.json b/.cursor-plugin/marketplace.json index c9a5026..869158d 100644 --- a/.cursor-plugin/marketplace.json +++ b/.cursor-plugin/marketplace.json @@ -61,6 +61,11 @@ "name": "trogonstack-protobuf", "source": "plugins/trogonstack-protobuf", "description": "Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers" + }, + { + "name": "trogonstack-concurrency", + "source": "plugins/trogonstack-concurrency", + "description": "Concurrency skills for finding and fixing time-of-check to time-of-use races in files, databases, caches, authorization, and distributed stores" } ] } diff --git a/.github/release-please-config.json b/.github/release-please-config.json index 9a292eb..4bacb18 100644 --- a/.github/release-please-config.json +++ b/.github/release-please-config.json @@ -66,6 +66,9 @@ }, "plugins/trogonstack-protobuf": { "component": "trogonstack-protobuf" + }, + "plugins/trogonstack-concurrency": { + "component": "trogonstack-concurrency" } }, "plugins": [ diff --git a/.github/release-please-manifest.json b/.github/release-please-manifest.json index 13c2ca7..7db71c3 100644 --- a/.github/release-please-manifest.json +++ b/.github/release-please-manifest.json @@ -9,5 +9,6 @@ "plugins/trogonstack-eda": "0.2.0", "plugins/trogonstack-product-requirements": "0.2.0", "plugins/trogonstack-elixir": "0.2.0", - "plugins/trogonstack-protobuf": "0.1.1" + "plugins/trogonstack-protobuf": "0.1.1", + "plugins/trogonstack-concurrency": "0.0.1" } diff --git a/plugins/trogonstack-concurrency/.claude-plugin/plugin.json b/plugins/trogonstack-concurrency/.claude-plugin/plugin.json new file mode 100644 index 0000000..56c55ff --- /dev/null +++ b/plugins/trogonstack-concurrency/.claude-plugin/plugin.json @@ -0,0 +1,9 @@ +{ + "name": "trogonstack-concurrency", + "description": "Concurrency skills for finding and fixing time-of-check to time-of-use races in files, databases, caches, authorization, and distributed stores", + "version": "0.0.1", + "author": { + "name": "TrogonStack", + "url": "https://github.com/TrogonStack" + } +} diff --git a/plugins/trogonstack-concurrency/.cursor-plugin/plugin.json b/plugins/trogonstack-concurrency/.cursor-plugin/plugin.json new file mode 100644 index 0000000..1a82c4b --- /dev/null +++ b/plugins/trogonstack-concurrency/.cursor-plugin/plugin.json @@ -0,0 +1,25 @@ +{ + "name": "trogonstack-concurrency", + "displayName": "TrogonStack Concurrency", + "description": "Concurrency skills for finding and fixing time-of-check to time-of-use races in files, databases, caches, authorization, and distributed stores", + "version": "0.0.1", + "author": { + "name": "TrogonStack" + }, + "homepage": "https://github.com/TrogonStack/agentskills/tree/main/plugins/trogonstack-concurrency", + "repository": "https://github.com/TrogonStack/agentskills", + "license": "MIT", + "keywords": [ + "trogonstack", + "concurrency", + "toctou", + "race-condition" + ], + "category": "developer-tools", + "tags": [ + "concurrency", + "toctou", + "race-condition" + ], + "skills": "./skills/" +} diff --git a/plugins/trogonstack-concurrency/README.md b/plugins/trogonstack-concurrency/README.md new file mode 100644 index 0000000..d993ec8 --- /dev/null +++ b/plugins/trogonstack-concurrency/README.md @@ -0,0 +1,7 @@ +# trogonstack-concurrency + +Concurrency skills for finding and fixing time-of-check to time-of-use races in files, databases, caches, authorization, and distributed stores. + +```bash +claude plugin install trogonstack-concurrency@trogonstack +``` diff --git a/plugins/trogonstack-concurrency/plugin.json b/plugins/trogonstack-concurrency/plugin.json new file mode 100644 index 0000000..d460f7f --- /dev/null +++ b/plugins/trogonstack-concurrency/plugin.json @@ -0,0 +1,19 @@ +{ + "$schema": "https://agent-plugins.org/schemas/1.0.0/plugin.schema.json", + "name": "trogonstack-concurrency", + "version": "0.0.1", + "description": "Concurrency skills for finding and fixing time-of-check to time-of-use races in files, databases, caches, authorization, and distributed stores", + "author": { + "name": "TrogonStack", + "url": "https://github.com/TrogonStack" + }, + "homepage": "https://github.com/TrogonStack/agentskills/tree/main/plugins/trogonstack-concurrency", + "repository": "https://github.com/TrogonStack/agentskills", + "license": "MIT", + "keywords": [ + "trogonstack", + "concurrency", + "toctou", + "race-condition" + ] +} diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/SKILL.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/SKILL.md new file mode 100644 index 0000000..982f236 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/SKILL.md @@ -0,0 +1,132 @@ +--- +name: concurrency-find-toctou +description: "Find, prove, and fix time-of-check to time-of-use (TOCTOU) races: code that checks a condition, then acts as if nothing changed in between. Covers check-then-act in files (exists then open), databases (SELECT then INSERT, find-or-create, read-modify-write), authorization (permission check, later effect), caches (get-or-compute), distributed stores (NATS KV revision, JetStream expected sequence, event-store expected version), and async code (check, await, write). Each finding names the check, the act, the window, who can interleave, the consequence, and the fix. Use when reviewing code for race conditions, duplicate inserts, lost updates, double charges, stale permission checks, flaky tests under parallel load, intermittent 'already exists' errors, when asked 'is this atomic', 'can two requests collide', or 'what if this runs twice', and before adding a mutex, lock, or retry loop. Do not use for: deadlocks or lock ordering, performance tuning, memory-model data races, event model or NATS subject design." +allowed-tools: + - AskUserQuestion + - Read + - Write + - Shell +--- + +# Find TOCTOU Races + +A check is true only at the instant it is made. Correctness has to come from an atomic primitive at the point of effect, never from the gap between the check and the act being short. + +## Core Principle + +Every `if exists / if allowed / if enough / if not yet` followed by a write is a bet that nothing else touched the same state in between. The bet is lost by another request, another process, another node, a retry, a redelivered message, or the same user clicking twice. A short window does not shrink the bug, it only lowers how often the test suite sees it. + +The fix is never "check faster" or "check again". The fix is to make the store that owns the state reject or absorb the second actor. Everything below follows from that. + +## Principles + +Apply them in order. A pair that fails an earlier principle is a finding; the later principles decide the fix and what the finding has to cite. Each one names the references that show it in practice; read them when the situation matches, not up front. + +### 1. A Read Is A Fact About The Past + +The moment a read returns, the value it reported is history. A condition evaluated on that value is a statement about a moment that has already passed, and nothing in the language makes the next line run in the same moment. Any check-then-act pair is therefore stale by construction unless something else holds the world still. + +Read [references/principles/read-is-the-past.md](references/principles/read-is-the-past.md) when deciding whether a pair is a candidate at all, or when someone argues the window is too small to matter. + +### 2. Only The Owner Of The State Can Decide + +The store that holds the state is the only actor that sees every writer. A decision made anywhere else is made on a copy: an application variable, a cache, a projection, a session. The atomic primitive has to come from the owner (a constraint, a conditional write, an exclusive create, a revision), and the caller's job is to ask the owner to decide. + +Read [references/principles/owner-decides.md](references/principles/owner-decides.md) when picking which layer the fix belongs in, or when a proposed fix adds a lock in the caller. + +### 3. The Write Must Carry Its Condition + +When the condition travels inside the write, the store evaluates it at the instant it applies the write, and the earlier check becomes a fast path or disappears. `WHERE status = 'pending'`, `ON CONFLICT`, `O_EXCL`, an expected revision, and an expected version are all the same move: the condition and the effect in one indivisible operation whose result tells the caller whether it won. + +Read [references/principles/write-carries-condition.md](references/principles/write-carries-condition.md) when shaping the fix, and the surface reference for how the store under review spells it. + +### 4. Serialization Is Proven By Naming The Serializer + +A pair is safe only when something already makes the actors take turns, and that something has a name: one writer by construction, a lock every writer takes, a synchronous section with no yield. "Unlikely", "rarely called", and "never seen it" are not names. A safe verdict without a named serializer is an unreviewed pair. + +Read [references/principles/serializer-is-named.md](references/principles/serializer-is-named.md) before marking any pair safe. + +### 5. A Lock Only Helps When The Read Happens Under It + +A lock serializes the actors that take it, for the span they hold it. A read outside the lock, a writer that skips the lock, or a lease that can expire mid-act all leave the race in place and add the cost of the lock on top. Reach for a lock only when the decision genuinely needs application code between the read and the write, and only when every writer can be made to take it. + +Read [references/principles/lock-moves-the-race.md](references/principles/lock-moves-the-race.md) when a lock or a lease is proposed or already present. + +### 6. Retry Belongs Around The Conditional Write + +A conditional write reports a conflict; a retry around it reloads, re-decides, and tries again against fresh state. A retry around a check-then-act repeats the race with the same stale read. Serializable isolation, optimistic versions, and expected revisions all require this loop or they turn a silent race into a user-facing error. + +Read [references/principles/retry-wraps-the-write.md](references/principles/retry-wraps-the-write.md) when a retry loop exists or the fix introduces one. + +### 7. Idempotence Absorbs What Ordering Cannot Prevent + +Redelivery, client retries, and double submits run the same act twice no matter how well the single run is guarded. When the second run produces the same end state and no second side effect, the race is tolerable even though it exists. Idempotence is a property of the effect, keyed by something deterministic, and the finding has to say which side effects it covers. + +Read [references/principles/idempotence-absorbs.md](references/principles/idempotence-absorbs.md) when the interleaver is a retry or a redelivery, or when the effect cannot be made conditional. + +## Procedure + +1. Locate every check-then-act pair: a read whose result decides a write, including pairs split across functions, an `await`, a queue hop, or two requests. +2. Describe the pair: the check, the act, the window, who can interleave, and the consequence. +3. Name the serializer, or name the principle the pair violates. A pair without a named serializer is a finding. +4. Pick the primitive from the owner of the state, with the write carrying its condition. Read [references/review/fix-catalog.md](references/review/fix-catalog.md) to choose, then the surface and library references for the spelling. +5. Write the finding in the format below, one per pair. Pairs judged safe are listed separately with their serializer. + +## Finding Format + +``` +### + +- Location: (check) and (act) +- Check: +- Act: +- Window: +- Interleaver: +- Consequence: +- Principle: +- Fix: +- Residual: +``` + +Read [references/review/finding-template.md](references/review/finding-template.md) for a fully worked example. + +## Surfaces + +Each surface file names the principles it instantiates, shows the smells in SQL or pseudocode, and points at the library files that spell the primitive. + +Read [references/surfaces/filesystem.md](references/surfaces/filesystem.md) when the check is `exists`, `stat`, `access`, or a path lookup, and the act opens, creates, renames, or deletes. + +Read [references/surfaces/database.md](references/surfaces/database.md) when the check is a `SELECT` and the act is an `INSERT`, `UPDATE`, or `DELETE`, including find-or-create, counters, balances, and status transitions. + +Read [references/surfaces/authorization.md](references/surfaces/authorization.md) when the check is a permission, role, ownership, or quota decision and the effect happens later, in another request, or in a job. + +Read [references/surfaces/cache.md](references/surfaces/cache.md) when the check is a cache miss and the act fills the cache or recomputes a value. + +Read [references/surfaces/distributed.md](references/surfaces/distributed.md) when the state lives in NATS KV, a JetStream stream, an event store, or behind an at-least-once consumer. + +Read [references/surfaces/in-process.md](references/surfaces/in-process.md) when the check and the act are in the same process but separated by an `await`, a message to another process, or a second call into the same server. + +## Systems + +Each system file states what the store itself guarantees, independent of the client library: its isolation or execution model, the writes that carry a condition, the locks it offers, and what does not serialize despite looking like it does. + +Read [references/systems/postgres.md](references/systems/postgres.md) when the state lives in PostgreSQL: isolation levels, `ON CONFLICT` arbiters, `FOR UPDATE` and `FOR NO KEY UPDATE`, advisory locks, `40001`. + +Read [references/systems/mysql.md](references/systems/mysql.md) when the state lives in MySQL or InnoDB: snapshot versus locking reads, gap locks on missing rows, `ON DUPLICATE KEY UPDATE`, affected rows versus found rows, `GET_LOCK`. + +Read [references/systems/sqlite.md](references/systems/sqlite.md) when the state lives in SQLite: one writer, `BEGIN IMMEDIATE` versus deferred, `SQLITE_BUSY` after a read, WAL mode, `:memory:` per connection. + +Read [references/systems/redis.md](references/systems/redis.md) when the state lives in Redis: single-command atomicity, `SET NX PX`, `WATCH` and `EXEC`, scripts, leases versus locks, failover, cluster slots. + +Read [references/systems/nats.md](references/systems/nats.md) when the state lives in NATS: expected sequence headers, `Nats-Msg-Id`, KV revisions, streams as claims, at-least-once consumers and their configuration. + +## Libraries + +After the surface and system references, read the file for the library under review when one exists. Each covers one library, organized by the surfaces it touches. Redis and NATS clients have no separate file; their spellings live at the end of the system file. + +- TypeScript: [node-fs](references/typescript/node-fs.md), [node-postgres](references/typescript/node-postgres.md), [prisma](references/typescript/prisma.md), [drizzle](references/typescript/drizzle.md), [event-loop](references/typescript/event-loop.md). +- Rust: [std-fs](references/rust/std-fs.md), [sqlx](references/rust/sqlx.md), [tokio](references/rust/tokio.md), [moka-dashmap](references/rust/moka-dashmap.md), [atomics](references/rust/atomics.md). +- Elixir: [file](references/elixir/file.md), [ecto](references/elixir/ecto.md), [ets](references/elixir/ets.md), [genserver](references/elixir/genserver.md), [registry](references/elixir/registry.md). +- Go: [os](references/go/os.md), [database-sql](references/go/database-sql.md), [sync](references/go/sync.md), [singleflight](references/go/singleflight.md). +- Python: [os](references/python/os.md), [psycopg](references/python/psycopg.md), [sqlalchemy](references/python/sqlalchemy.md), [asyncio](references/python/asyncio.md). +- C: [posix-fs](references/c/posix-fs.md), [atomics](references/c/atomics.md). diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/c/atomics.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/c/atomics.md new file mode 100644 index 0000000..12dd1c1 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/c/atomics.md @@ -0,0 +1,29 @@ +# C Atomics And pthreads + +Spells scalar compare-and-swap and the mutex discipline from [in-process.md](../surfaces/in-process.md). + +## Compare-And-Swap Loop + +```c +#include + +_Atomic uint64_t counter; + +uint64_t cur = atomic_load(&counter); +for (;;) { + if (cur >= limit) return -1; + if (atomic_compare_exchange_weak(&counter, &cur, cur + 1)) break; +} +``` + +`atomic_compare_exchange_weak` reloads `cur` with the observed value on failure, which is what the loop needs. `atomic_load` then `atomic_store` is the race; `atomic_fetch_add` alone cannot enforce the limit because it has no condition. + +GCC and Clang builtins: `__atomic_compare_exchange_n(&counter, &cur, cur + 1, true, __ATOMIC_ACQ_REL, __ATOMIC_ACQUIRE)`. + +## Mutex Discipline + +A `pthread_mutex_t` held across both the check and the write is correct when every writer to that state takes the same mutex. A check outside the mutex followed by a locked write is the race, and so is a double-checked read of a non-atomic field without a fence. + +## Where Atomics Stop + +An atomic protects one word. A decision that reads two atomics and writes one has the same gap as any two reads; use a mutex over the pair or pack the state into one word. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/c/posix-fs.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/c/posix-fs.md new file mode 100644 index 0000000..0a49d9c --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/c/posix-fs.md @@ -0,0 +1,41 @@ +# POSIX Filesystem In C + +Spells the filesystem primitives from [filesystem.md](../surfaces/filesystem.md). + +## Exclusive Create + +```c +int fd = open(path, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0600); +if (fd < 0) { + if (errno == EEXIST) { /* someone else created it */ } + else return -1; +} +``` + +`mkdir` fails with `EEXIST` the same way; `access` or `stat` before either is the race. + +## Open Then Verify + +Verify the descriptor you hold, instead of `stat` then `open`. + +```c +int fd = open(path, O_RDONLY | O_NOFOLLOW | O_CLOEXEC); +if (fd < 0) return -1; +struct stat st; +if (fstat(fd, &st) < 0 || !S_ISREG(st.st_mode) || st.st_uid != getuid()) { + close(fd); + return -1; +} +``` + +## Untrusted Trees + +Open the directory once and use `openat(dirfd, "child", O_NOFOLLOW | O_CLOEXEC)` for each child, so a swapped parent component cannot redirect the walk. On Linux, `openat2` with `RESOLVE_BENEATH` refuses every escape in one call. + +## Temp File And Atomic Replace + +`int fd = mkstemp(template);` where `template` ends in `XXXXXX` and lives in the target's directory. Write to that descriptor, `fsync(fd)`, `close(fd)`, then `rename(tmp, target)`. Building a name and calling `access` first is the race. + +## Idempotent Delete + +Call `unlink` and accept `ENOENT`. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/ecto.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/ecto.md new file mode 100644 index 0000000..baf2bca --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/ecto.md @@ -0,0 +1,87 @@ +# Ecto + +Spells the database primitives from [database.md](../surfaces/database.md) and the transactional dedupe from [distributed.md](../surfaces/distributed.md). + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how Ecto spells it. + +## Find Or Create + +The upsert returns the id on both paths. `unique_constraint/3` in a changeset only translates the database error into a changeset error; without the unique index in a migration it prevents nothing. + +```elixir +Repo.insert(changeset, + on_conflict: [set: [email: email]], + conflict_target: :email, + returning: true +) +``` + +`Repo.get_by(User, email: email) || Repo.insert(...)` is the race. + +## Conditional Transition + +`update_all` with the expected state in the query; the count is the verdict. + +```elixir +from(o in Order, where: o.id == ^id and o.status == "pending") +|> Repo.update_all(set: [status: "paid", paid_at: DateTime.utc_now()]) +|> case do + {1, _} -> :ok + {0, _} -> {:error, :already_moved} +end +``` + +`Repo.get!` then `Repo.update` through a changeset is the race, because the changeset's `WHERE` is the primary key only. + +## Balance Debit In One Statement + +```elixir +from(a in Account, where: a.id == ^id and a.balance >= ^amount) +|> Repo.update_all(inc: [balance: -amount]) +|> case do + {1, _} -> :ok + {0, _} -> {:error, :insufficient_funds_or_missing} +end +``` + +The count is the verdict. Zero rows means the balance was too low or the account is gone, and the caller decides which by reloading, not by assuming. + +## Optimistic Version And Row Lock + +`optimistic_lock(changeset, :version)` adds `WHERE version = ^current` and raises `Ecto.StaleEntryError` when the row moved; the caller reloads and retries, bounded. + +Row lock inside `Repo.transaction/1` (`Repo.transact/2` on Ecto versions that deprecate it; the lock scope is the same): + +```elixir +Repo.transaction(fn -> + sub = Repo.one!(from(s in Subscription, where: s.id == ^id, lock: "FOR UPDATE")) + # decide + Repo.update!(Subscription.changeset(sub, attrs)) +end) +``` + +Every other writer to that row must take the same lock. + +## Dedupe Record And Effect In One Transaction + +The dedupe row goes in as a changeset that declares the unique constraint; inserting a bare struct raises `Ecto.ConstraintError` on the violation instead of returning the error tuple the `case` matches on. + +```elixir +seen = + %ProcessedMessage{} + |> Ecto.Changeset.change(msg_id: msg_id) + |> Ecto.Changeset.unique_constraint(:msg_id) + +Ecto.Multi.new() +|> Ecto.Multi.insert(:seen, seen) +|> Ecto.Multi.update_all(:debit, debit_query, []) +|> Repo.transaction() +|> case do + {:ok, _} -> :ok + {:error, :seen, %{errors: [msg_id: _]}, _} -> :already_done +end +``` + +## Advisory Lock + +`Repo.query!("SELECT pg_advisory_xact_lock(hashtext($1))", ["sessions:#{user_id}"])` as the first statement inside `Repo.transaction/1` serializes a cross-row invariant on that key; the lock releases at commit. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/ets.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/ets.md new file mode 100644 index 0000000..f807fbc --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/ets.md @@ -0,0 +1,41 @@ +# ETS + +Spells insert-if-absent, atomic counters, and compare-and-swap from [in-process.md](../surfaces/in-process.md) and [cache.md](../surfaces/cache.md). ETS has no owning server to serialize a lookup and an insert, so only its single-call atomic operations count as conditional writes. + +## Insert If Absent + +`insert_new/2` returns `false` to the loser. + +```elixir +case :ets.insert_new(table, {key, value}) do + true -> :claimed + false -> :taken +end +``` + +`:ets.lookup(table, key) == []` followed by `:ets.insert/2` is the race. + +## Atomic Counter With Default + +`update_counter/4` with a default creates the row if missing and increments in one call. + +```elixir +:ets.update_counter(table, key, {2, 1}, {key, 0}) +``` + +A limit cannot be enforced inside `update_counter`; it only clamps with a threshold tuple (`{2, 1, limit, limit}`), so a caller that must know whether it won checks the returned value against the limit and decrements on loss, or uses `select_replace` below. + +## Compare-And-Swap With select_replace + +`select_replace/2` replaces only the rows that match the spec, and returns how many it replaced. + +```elixir +case :ets.select_replace(table, [{{key, old}, [], [{{key, new}}]}]) do + 1 -> :ok + 0 -> :stale +end +``` + +## Plain Integers + +`:atomics.compare_exchange/4` on an `:atomics` array is the word-level compare-and-swap; `:atomics.get/2` then `:atomics.put/3` is the race. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/file.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/file.md new file mode 100644 index 0000000..ab8b521 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/file.md @@ -0,0 +1,40 @@ +# Elixir `File` + +Spells the filesystem primitives from [filesystem.md](../surfaces/filesystem.md). + +## Exclusive Create + +`:exclusive` is `O_CREAT | O_EXCL`; `{:error, :eexist}` is the answer. + +```elixir +case File.open(path, [:write, :exclusive]) do + {:ok, io} -> + IO.binwrite(io, data) + File.close(io) + {:error, :eexist} -> + :exists +end +``` + +`File.mkdir/1` returns `{:error, :eexist}` the same way; `File.mkdir_p/1` is the idempotent form. `File.exists?/1` before either is the race. + +## Atomic Replace + +Write to a sibling temp path created with `:exclusive`, then `File.rename/2` onto the target. Keep the temp file in the target's directory so the rename stays on one filesystem. + +```elixir +tmp = "#{target}.#{System.unique_integer([:positive])}.tmp" +{:ok, io} = File.open(tmp, [:write, :exclusive]) +IO.binwrite(io, data) +:ok = :file.sync(io) +File.close(io) +File.rename(tmp, target) +``` + +## Open Then Verify + +`File` does not expose `O_NOFOLLOW`, so `File.lstat/1` followed by `File.open/2` is still two moments. When the directory is writable by other users, keep the file inside a directory only this process can write, or open it from a NIF that passes the flag. + +## Idempotent Delete + +`File.rm/1` returns `{:error, :enoent}` instead of raising; match on it rather than checking `File.exists?/1` first. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/genserver.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/genserver.md new file mode 100644 index 0000000..cd8f72d --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/genserver.md @@ -0,0 +1,26 @@ +# GenServer + +Spells "two calls into one server" from [in-process.md](../surfaces/in-process.md). A GenServer serializes each message, not a sequence of messages from one caller. + +## One Message Carries The Decision + +```elixir +GenServer.call(server, {:put_new, key, value}) + +def handle_call({:put_new, key, value}, _from, state) do + case Map.fetch(state, key) do + {:ok, existing} -> {:reply, {:exists, existing}, state} + :error -> {:reply, {:ok, value}, Map.put(state, key, value)} + end +end +``` + +`GenServer.call(server, {:get, key})` followed by `GenServer.call(server, {:put, key, value})` is the race: another caller's `put` lands between the two. + +## Yielding Inside The Handler + +A handler that calls out to another process, a database, or the network while deciding is still serialized: the server processes nothing else until the handler returns. That is correct and also a throughput choice the finding should note. A handler that replies early with `{:noreply, ...}` and finishes the decision in a later message has reopened the gap. + +## One Process Per Entity + +A process registered under the entity id that handles every write to that entity serializes those writes by construction. The finding cites the registry key and confirms no other code path (a migration, an admin task, a second service) writes the same state outside the process; see [registry.md](registry.md) for the claim and [serializer-is-named.md](../principles/serializer-is-named.md) for what the citation must contain. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/registry.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/registry.md new file mode 100644 index 0000000..500ba9c --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/elixir/registry.md @@ -0,0 +1,22 @@ +# Registry + +Spells "claim then act" from [in-process.md](../surfaces/in-process.md). With `keys: :unique`, the register call is the claim; it succeeds for exactly one process and tells the others who won. + +## Register As The Claim + +```elixir +case Registry.register(MyRegistry, key, nil) do + {:ok, _owner} -> :claimed + {:error, {:already_registered, pid}} -> {:taken, pid} +end +``` + +`Registry.lookup(MyRegistry, key) == []` followed by `Registry.register/3` is the race. + +## Starting The Owner + +Starting a process under a `:via` name has the same shape built in: `GenServer.start_link(mod, arg, name: {:via, Registry, {MyRegistry, key}})` returns `{:error, {:already_started, pid}}` to the loser. A `DynamicSupervisor.start_child` with that name is the idiomatic find-or-start; `Registry.lookup` then `start_child` is the race. + +## Across Nodes + +`Registry` is local to one node. Two nodes can each register the same key locally. When one owner per key must hold across the cluster, the claim has to come from a store that spans nodes (a database unique row, a NATS KV `create`), and the finding says which. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/database-sql.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/database-sql.md new file mode 100644 index 0000000..8e43fb4 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/database-sql.md @@ -0,0 +1,52 @@ +# Go `database/sql` + +Spells the database primitives from [database.md](../surfaces/database.md). The SQL carries the condition; the package exposes `RETURNING` through `QueryRowContext` and the rowcount through `RowsAffected`. + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how `database/sql` spells it. On MySQL, read [references/systems/mysql.md](../systems/mysql.md) instead. + +## Upsert With Identity On Both Paths + +```go +var id int64 +err := db.QueryRowContext(ctx, + `INSERT INTO users (email) VALUES ($1) + ON CONFLICT (email) DO UPDATE SET email = EXCLUDED.email + RETURNING id`, email).Scan(&id) +``` + +## Conditional Update And Rowcount + +```go +res, err := db.ExecContext(ctx, + `UPDATE accounts SET balance = balance - $1 WHERE id = $2 AND balance >= $1`, + amount, id) +if err != nil { return err } +if n, _ := res.RowsAffected(); n == 0 { + return ErrInsufficientFunds +} +``` + +The same shape covers `WHERE status = 'pending'`, `WHERE version = $3`, and `WHERE owner_id = $3`. + +## Row Lock + +Every statement of the decision runs on the `*sql.Tx`; a `db.` call inside the transaction uses another connection and is outside the lock. + +```go +tx, err := db.BeginTx(ctx, nil) +if err != nil { return err } +defer tx.Rollback() +var sub Subscription +if err := tx.QueryRowContext(ctx, "SELECT ... FROM subscriptions WHERE id = $1 FOR UPDATE", id).Scan(&sub.Fields...); err != nil { + return err +} +// decide +if _, err := tx.ExecContext(ctx, "UPDATE subscriptions SET ... WHERE id = $1", id); err != nil { + return err +} +return tx.Commit() +``` + +## Serializable Retry + +`db.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelSerializable})` and a bounded loop that retries the whole transaction when the driver error's SQLSTATE is `40001` or `40P01` (`pgconn.PgError.Code` with pgx, `pq.Error.Code` with lib/pq). Retry `23505` only when the code can show the duplicate came from a value it chose after a read; otherwise it is a persistent error. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/os.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/os.md new file mode 100644 index 0000000..ae81e6a --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/os.md @@ -0,0 +1,45 @@ +# Go `os` + +Spells the filesystem primitives from [filesystem.md](../surfaces/filesystem.md). + +## Exclusive Create + +```go +f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) +if errors.Is(err, fs.ErrExist) { + // someone else created it +} else if err != nil { + return err +} +``` + +`os.Mkdir` fails with `fs.ErrExist` the same way; `os.MkdirAll` is the idempotent form. `os.Stat` before either is the race. + +## Atomic Replace + +```go +tmp, err := os.CreateTemp(filepath.Dir(target), ".tmp-*") +if err != nil { return err } +defer os.Remove(tmp.Name()) +if _, err := tmp.Write(data); err != nil { return err } +if err := tmp.Sync(); err != nil { return err } +if err := tmp.Close(); err != nil { return err } +return os.Rename(tmp.Name(), target) +``` + +`os.CreateTemp` creates exclusively with a unique name; building a name and checking `os.Stat` first is the race. + +## Open Then Verify + +`os.OpenFile` with `syscall.O_NOFOLLOW` rejects a symlink at the final component; then `f.Stat()` describes the descriptor, not the path. + +```go +f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW, 0) +if err != nil { return err } +st, err := f.Stat() +if err != nil || !st.Mode().IsRegular() { f.Close(); return ErrForbidden } +``` + +## Untrusted Trees And Idempotent Delete + +`os.Root` (Go 1.24+) resolves paths relative to an opened directory and refuses escapes through symlinks, which is the `openat` discipline. `os.Remove` then `errors.Is(err, fs.ErrNotExist)` is the idempotent delete. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/singleflight.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/singleflight.md new file mode 100644 index 0000000..4da33d9 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/singleflight.md @@ -0,0 +1,27 @@ +# singleflight + +Spells in-process single-flight from [cache.md](../surfaces/cache.md) with `golang.org/x/sync/singleflight`. + +## One Computation Per Key + +Concurrent callers for the same key share one `fn` call and receive the same result. + +```go +var group singleflight.Group + +v, err, shared := group.Do(key, func() (any, error) { + value, err := computeExpensive(ctx, key) + if err == nil { + cache.Set(key, value, ttl) + } + return value, err +}) +``` + +`cache.Get(key)` miss followed by `computeExpensive` and `cache.Set` is the stampede. + +## What It Does Not Cover + +- It dedupes within one process. Across a fleet the lease in [redis.md](../systems/redis.md) bounds the stampede. +- The `ctx` captured by the closure belongs to the first caller. If that caller cancels, every joined caller gets its error. Use `DoChan` and a detached context when callers can cancel independently. +- An error is shared too. A failed computation is handed to every joined caller; nothing is cached, so the next wave recomputes. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/sync.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/sync.md new file mode 100644 index 0000000..1ef0413 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/go/sync.md @@ -0,0 +1,41 @@ +# Go `sync` And `sync/atomic` + +Spells the in-process primitives from [in-process.md](../surfaces/in-process.md). + +## Hold The Mutex Across Check And Store + +```go +mu.Lock() +defer mu.Unlock() +if _, ok := m[key]; !ok { + m[key] = build(key) +} +``` + +Releasing between the check and the store, or checking under `RLock` and storing under `Lock`, reopens the race: two goroutines both miss under the read lock and both insert under the write lock. + +## sync.Map.LoadOrStore + +Publishes exactly one value per key and tells the loser which one won. + +```go +actual, loaded := m.LoadOrStore(key, candidate) +``` + +`candidate` is evaluated before the call, so `LoadOrStore` does not dedupe construction; pair it with [singleflight.md](singleflight.md) when building the value is expensive. `m.Load` then `m.Store` is the race. + +## One-Time Initialization + +`sync.Once` and `sync.OnceValue` run the initializer once and block concurrent callers until it finishes. A `var initialized bool` checked and set across a goroutine boundary is the race. + +## Compare-And-Swap + +```go +for { + cur := counter.Load() + if cur >= limit { return ErrExhausted } + if counter.CompareAndSwap(cur, cur+1) { break } +} +``` + +`counter.Add(1)` alone cannot enforce the limit because it has no condition; `Load` then `Store` is the race. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/idempotence-absorbs.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/idempotence-absorbs.md new file mode 100644 index 0000000..985101a --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/idempotence-absorbs.md @@ -0,0 +1,32 @@ +# Idempotence Absorbs What Ordering Cannot Prevent + +When running the act twice produces the same end state and no second side effect, a race on that act is tolerable even though it exists. + +## Why It Holds + +Some interleavers cannot be ordered away. A message redelivered after a crash, a client that retries a timed-out request, a user who double-submits a form: each one runs the whole act again, with the guards intact, because from the system's point of view it is a new request. No constraint on the first run prevents the second run. The only defense is that the second run changes nothing. + +Idempotence is a property of the effect, not of the request. It needs a deterministic key that both runs share (a client-generated id, a message id, a natural key) and an effect that is absolute rather than relative (set to a value, upsert by key, create-if-absent) or that is guarded by a record of the key. + +## What Idempotence Looks Like + +- An upsert keyed by a client-generated id. The second run matches the same row. +- `SET` of an absolute value, `mkdir -p`, `rm -f`. The second run finds the state already there. +- A `processed_messages` row inserted in the same transaction as the effect, with a unique constraint on the message id; the violation means "already done" and the transaction rolls back the duplicate effect. +- A payment with an idempotency key the provider enforces. +- A `Nats-Msg-Id` inside the stream's duplicate window, which absorbs a retried publish. + +## What Idempotence Does Not Cover + +- A counter increment, an appended log line, a sent email, a charge without an idempotency key. Each second run adds one more. +- A dedupe record written after the effect in a separate transaction. The crash between them leaves the effect done and the record missing, and the redelivery runs the effect again. +- A `Nats-Msg-Id` against two different decisions on the same aggregate. It absorbs the same publish repeated; it does not order two publishers who both read the same state ([write-carries-condition.md](write-carries-condition.md) does that). +- Any effect in a non-transactional store with no deterministic key. Say so in the finding rather than claiming idempotence. + +## What The Reviewer Must Cite + +The key both runs share, why the effect is absolute or guarded by that key, and a list of the side effects on the path with confirmation that none fires on the second run. For consumers, whether the dedupe record and the effect commit together, and the `ack_wait` and `max_deliver` that bound how many second runs can happen. + +## The Wrong Fix It Rules Out + +"We check `processed.contains(id)` first." That is a check-then-act on the dedupe set, and two instances handling the same redelivery both pass it. The record of the id has to be the conditional write itself, inside the same transaction as the effect. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/lock-moves-the-race.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/lock-moves-the-race.md new file mode 100644 index 0000000..1fb5600 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/lock-moves-the-race.md @@ -0,0 +1,30 @@ +# A Lock Only Helps When The Read Happens Under It + +A lock serializes the actors that take it, for the span they hold it. Everything outside that span, and every actor that does not take it, is unaffected. + +## Why It Holds + +A lock does not protect data. It protects a critical section, and only against other holders of the same lock. The guarantee it gives is exactly "no other holder runs between my acquire and my release". So the read has to be inside that span, the write has to be inside that span, and every writer to the same state has to be a holder. Remove any one of those and the lock is paying its cost for nothing. + +## When A Lock Is The Answer + +The decision genuinely needs application code between the read and the write, cannot be folded into one conditional statement, and every writer to that state can be made to take the same lock. A row lock (`SELECT ... FOR UPDATE`), an advisory lock on a key, and a single process that owns the state all satisfy that. The finding must list the other writers and confirm each one takes the lock. + +The choice between a lock and a conditional write is the choice between pessimistic and optimistic. Pessimistic costs waiting on every call and never surfaces a conflict. Optimistic costs nothing when conflicts are rare and pushes the conflict to the caller as a retry ([retry-wraps-the-write.md](retry-wraps-the-write.md)). Pick pessimistic when the decision is expensive, conflicts are common, or a retry would repeat a side effect that is not idempotent. + +## When A Lock Moves The Race + +- **In-process lock, shared store.** A mutex guards nothing against a second process, worker, or node. If the state lives in a shared store, the mutex is decoration. +- **Read outside, lock inside.** "Read, then lock, then write" protects the write from other writers but not the decision from staleness. The read has to happen under the lock. +- **Lock, release, re-lock.** Checking under one acquire and writing under a second one is two critical sections with a gap. On an async runtime this is the common shape when a guard cannot be held across an `await`. +- **A writer that skips the lock.** A migration script, an admin endpoint, a second service, or a background job that writes the same row without `FOR UPDATE` defeats every caller that did take it. +- **A lease.** A distributed lock with an expiry (`SET NX PX`, a leader election) is itself a check-then-act: the holder checks it has the lease, then acts, and the lease can expire in between. It bounds how often the race happens; it does not remove it. Fence the act with a token the owning store verifies (a version, a revision, a lease number stored with the write) or accept the window and state it. +- **`FOR UPDATE` on a missing row.** Locking a row that does not exist locks nothing, so a row lock never solves find-or-create. The unique constraint is the primitive. + +## What The Reviewer Must Cite + +The lock, the span it is held for, that the read is inside the span, and the list of writers with confirmation that each takes it. For a lease, the expiry and what fences the act after expiry. + +## The Wrong Fix It Rules Out + +Adding a mutex, a semaphore, or a distributed lock as the first response to a race on shared state. Before any lock, ask whether the owner of the state can carry the condition in the write ([write-carries-condition.md](write-carries-condition.md)); most races end there with no lock at all. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/owner-decides.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/owner-decides.md new file mode 100644 index 0000000..289967d --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/owner-decides.md @@ -0,0 +1,25 @@ +# Only The Owner Of The State Can Decide + +The store that holds the state is the only actor that sees every writer, so it is the only actor that can evaluate a condition and apply a write as one event. + +## Why It Holds + +A caller sees its own reads and its own writes. It does not see the other request on the other node, the worker draining the same queue, or the admin revoking the grant. The database, the filesystem, the KV bucket, and the registry each sit at the point where all of those writers converge, and each one offers a primitive that decides and acts in a single step: a unique constraint, an exclusive create, a conditional update, a revision-guarded put, a register call that returns the winner. + +Any decision made outside the owner is made on a copy. An application variable, a cache entry, a projection, a session's permission set, and a GenServer's in-memory map are all copies unless that process is by construction the only writer, in which case it is the owner. + +## What Violating It Looks Like + +- An in-process mutex around a database read-then-write. The mutex serializes this process; the other replica never took it. +- A cached count or balance consulted before a write to the real store. The cache is a copy and the write to the store carries no condition. +- A permission set loaded at login and trusted for the session. The owner of the grant is the authorization source, not the session. +- A distributed lock taken in the caller so that the caller can read, decide, and write. The lock is a second store with its own check-then-act (see [lock-moves-the-race.md](lock-moves-the-race.md)). +- A `find_or_create` helper in application code that is "careful". Carefulness in the caller cannot see the other caller. + +## What The Reviewer Must Cite + +Which store owns the state the decision is about, and whether the write to that store carries the condition or relies on the caller's earlier read. When the owner is a process (one GenServer per entity, one consumer per partition), cite what makes it the sole writer. + +## The Wrong Fix It Rules Out + +Adding coordination in the caller: a mutex, a semaphore, a "lock table" in application memory, a leader that is assumed to be unique. These help only when every writer is in the same process and takes the same lock, which is the narrow case covered in [serializer-is-named.md](serializer-is-named.md). For shared state the fix moves into the store, spelled as in [write-carries-condition.md](write-carries-condition.md). diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/read-is-the-past.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/read-is-the-past.md new file mode 100644 index 0000000..7262224 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/read-is-the-past.md @@ -0,0 +1,29 @@ +# A Read Is A Fact About The Past + +The value a read returns describes the state at some instant before the read completed, and the condition evaluated on it is true of that instant only. + +## Why It Holds + +Nothing in a language, a driver, or a runtime makes the line after a read execute in the same moment as the read. Between them sit scheduler preemption, an `await`, a network round trip, a transaction boundary, a queue, or simply another core running another request. The world keeps moving while the caller holds a snapshot and reasons about it as if it were live. + +The width of the gap changes how often the race fires, not whether it exists. A gap of microseconds still loses under load, under a retry storm, under a replay, or under a double-click, and those are exactly the conditions production produces and tests do not. + +## What Violating It Looks Like + +- `exists(path)` then `open(path)`: the path can be swapped for a symlink in between. +- `SELECT` then `INSERT`: two transactions both see nothing and both insert. +- `can(user, action)` then `await render()` then write: the grant is revoked during the render. +- `cache.get` misses, then every concurrent caller recomputes. +- `kv.get` then `kv.put`: the put overwrites a write that landed in between. +- `if not busy: busy = true` with an `await` between the read and the assignment. +- Two `GenServer.call`s, one to read and one to write: each call is serialized, the pair is not. + +The shape is always the same: a read, a decision on its result, a write that assumes the result still holds. + +## What The Reviewer Must Cite + +The read, the write it gates, and what runs between them. A pair where nothing runs between them on a single-threaded loop is the one case where the read is still the present; the finding must say so explicitly and name the function that contains no yield. + +## The Wrong Fix It Rules Out + +Checking again right before the write. Two checks have two windows. Narrowing the window by moving the check closer to the write changes the odds and nothing else. The fix is in [owner-decides.md](owner-decides.md) and [write-carries-condition.md](write-carries-condition.md): stop deciding on the snapshot and let the store decide at the instant of the write. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/retry-wraps-the-write.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/retry-wraps-the-write.md new file mode 100644 index 0000000..185df83 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/retry-wraps-the-write.md @@ -0,0 +1,30 @@ +# Retry Belongs Around The Conditional Write + +A retry is correct only when what it repeats is a conditional write that reports a conflict, and the loop reloads before it decides again. + +## Why It Holds + +A conditional write that loses tells the caller the world moved. The right response is to look at the world as it is now, decide again, and try again; that is a retry loop whose body contains the read, the decision, and the conditional write. The loop converges because each iteration either wins against current state or learns that it has to re-decide. + +A retry around a check-then-act does none of that. It re-runs the same stale shape, and on each iteration the window is open again. It can even make things worse: a retry storm is exactly the load pattern under which small windows lose. + +## Where The Loop Is Mandatory + +- **Optimistic version.** `UPDATE ... WHERE version = $n` returning zero rows means reload and re-decide. Without the loop the caller surfaces "conflict" to a user who did nothing wrong. +- **Expected revision or sequence.** `kv.update` with a stale revision, a JetStream publish with a stale expected sequence, an event-store append with a stale expected version. Same loop, bounded. +- **`SERIALIZABLE` isolation.** The database aborts one transaction with SQLSTATE `40001`. It is correct only when every caller retries the whole transaction on that code, and on `40P01`; without the loop it has turned a silent race into a user-facing error. A `23505` is retried only when the caller can show it was a race on a value chosen from a prior read. +- **Exclusive create as find-or-create.** `create` fails because the key exists, so the fallback is `update` with the revision just learned, which can itself lose and loop. + +## Where The Loop Is Wrong + +- Around `SELECT` then `INSERT` without a constraint. Each iteration can still double-insert. +- Around a lease acquisition that then reads and writes unconditionally. The lease is the check; the write is still unconditional. +- Unbounded. A loop that cannot give up hides a permanently contended key or a bug in the condition. Bound the attempts and report the exhaustion. + +## What The Reviewer Must Cite + +What the loop body repeats, whether the body reloads before deciding, what conflict signal it branches on (rowcount, error code, `false`), and the bound. + +## The Wrong Fix It Rules Out + +"Retry on failure" as a wrapper around the existing check-then-act. The retry has to wrap the conditional write from [write-carries-condition.md](write-carries-condition.md), and when the act is not idempotent the loop must not repeat the side effect ([idempotence-absorbs.md](idempotence-absorbs.md)). diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/serializer-is-named.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/serializer-is-named.md new file mode 100644 index 0000000..19c31a2 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/serializer-is-named.md @@ -0,0 +1,48 @@ +# Serialization Is Proven By Naming The Serializer + +A check-then-act pair is safe only when something already makes the actors take turns, and the reviewer can point at that something by name. + +## Why It Holds + +The race exists whenever two actors can reach the act with the same stale read. The only ways that cannot happen are that there is exactly one actor by construction, or that the actors are forced through a single door before they read. Both are concrete mechanisms with a name in the code or the configuration. Anything that is not such a mechanism is a probability, and a probability is a race with a smaller number attached. + +"The window is small", "this is rarely called", and "we have never seen it" describe odds. The reviewer who marks a pair safe owes the same evidence as the reviewer who marks it unsafe. + +## Accepted Serializers + +Each one with what must be cited: + +- **Partition key ownership.** Messages for a key always land on the same consumer instance, such as a Kafka partition keyed by the aggregate id. Cite the partitioner, the subject-to-consumer mapping, and the config that keeps one instance per partition. A NATS queue group is not this: it balances without affinity. +- **One process per entity.** A single process registered under the entity id handles every write to that entity. Cite the registry, the unique key, and that no other code path writes the same state outside that process. +- **Single consumer on the subject.** A JetStream consumer with `max_ack_pending: 1`, or a single-instance worker by deployment. Cite the consumer config and the deployment constraint, and note that redelivery after `ack_wait` re-enters the same consumer, which is safe only if the effect is idempotent or the dedupe is transactional ([idempotence-absorbs.md](idempotence-absorbs.md)). +- **Already inside a lock.** The enclosing transaction holds `FOR UPDATE` on the row, or an advisory lock on the key, before the check runs. Cite the lock statement and confirm every other writer to that state takes the same lock ([lock-moves-the-race.md](lock-moves-the-race.md)). +- **Synchronous section.** On a single-threaded event loop with no `await`, `yield`, or callback between the check and the act. Cite the function and state that it contains no yield point. + +## The Write Already Carries The Condition + +The pair is also safe when the act is itself conditional and the earlier check is only a fast path ([write-carries-condition.md](write-carries-condition.md)): + +- A unique index exists on the key and the insert handles the violation. Cite the index from the migration, not from a changeset helper. +- The update has `WHERE status = ...`, `WHERE version = ...`, or `WHERE owner_id = ...` and the code branches on the rowcount. Cite the statement. +- The KV update passes the revision, the publish carries the expected sequence header, the append carries the expected version. Cite the call. + +In that case the finding says "redundant check, write is conditional" rather than reporting a race. + +## What Is Not A Serializer + +- A comment saying the function is not reentrant. +- A mutex in one service when another service writes the same table. +- A low request rate. Rates change, and retries and replays cluster. +- A check repeated right before the write. Two checks have two windows. +- A test that passes. Races are the one bug class a passing test says nothing about. + +## What The Safe Verdict Must Contain + +``` +- Location: +- Pair: then +- Serialized by: +- Other writers: +``` + +A safe verdict without a named serializer is an unreviewed pair, not a safe one. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/write-carries-condition.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/write-carries-condition.md new file mode 100644 index 0000000..6a5494a --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/principles/write-carries-condition.md @@ -0,0 +1,38 @@ +# The Write Must Carry Its Condition + +When the condition is part of the write, the store evaluates it at the instant it applies the write, and the result of the write tells the caller whether it won. + +## Why It Holds + +A write with a condition inside it is one operation from the store's point of view. There is no instant at which the condition has been checked but the write has not happened, so there is nothing for another actor to slip into. The earlier read, if it survives at all, becomes a fast path that saves work on the common case and decides nothing. + +The condition can be spelled many ways and they are the same move: + +| Spelling | Where | +| --- | --- | +| `O_CREAT \| O_EXCL` | filesystem create | +| unique index plus `INSERT ... ON CONFLICT` | find-or-create | +| `UPDATE ... WHERE status = 'pending'`, check rowcount | state transition | +| `UPDATE ... SET x = x - $1 WHERE x >= $1` | counter, balance, quota | +| `UPDATE ... WHERE version = $n` | optimistic version | +| `UPDATE ... WHERE owner_id = $u` | authorization folded into the effect | +| `SET key v NX` | cache or lock claim | +| `kv.update(key, value, revision)` | NATS KV | +| `Nats-Expected-Last-Subject-Sequence` header | JetStream append | +| `append(stream, events, expected_version)` | event store | +| `insert_new`, `LoadOrStore`, `compare_exchange` | in-process table or scalar | +| `Registry.register` returning the winner | process ownership | + +## What Violating It Looks Like + +The write is unconditional and the condition lives in an earlier `if`. The tell is a plain `INSERT`, a plain `UPDATE ... WHERE id = $1`, a plain `put`, a plain `set`, or a plain `insert` that follows a read of the same key. + +A weaker form: the write carries a condition but the code never reads the result. A rowcount of zero, a `false` from `insert_new`, a `null` from `SET NX`, and a revision-mismatch error are the store saying "you lost"; ignoring them turns a correct primitive back into a silent race. + +## What The Reviewer Must Cite + +The write statement or call, whether the condition is inside it, and where the code branches on the result. When the condition is inside the write and the result is handled, the pair is safe and the statement is the evidence. The earlier check can then be reported as redundant rather than as a race. + +## The Wrong Fix It Rules Out + +A transaction around the check and the write. Under `READ COMMITTED` and under InnoDB's default `REPEATABLE READ`, two transactions can both read nothing and both insert; the transaction gives atomicity of the pair's effects, not exclusion of other readers. Only a constraint, a conditional write, a lock taken before the read, or `SERIALIZABLE` with a retry loop makes one of them lose. The surface files show the spelling for each store. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/asyncio.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/asyncio.md new file mode 100644 index 0000000..8bfd929 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/asyncio.md @@ -0,0 +1,38 @@ +# asyncio + +Spells the in-process primitives from [in-process.md](../surfaces/in-process.md) and the single-flight from [cache.md](../surfaces/cache.md). The event loop runs one task at a time, so a pair with no `await` between check and act is atomic, and that fact is the serializer the finding names. + +## Store The Future, Not The Result + +```python +inflight: dict[str, asyncio.Future] = {} + +async def get_or_compute(key: str): + cached = await cache.get(key) + if cached is not None: + return cached + fut = inflight.get(key) + if fut is None: + fut = asyncio.ensure_future(_compute_and_store(key)) + inflight[key] = fut + fut.add_done_callback(lambda _: inflight.pop(key, None)) + return await asyncio.shield(fut) +``` + +The dict read and write have no `await` between them. The `shield` matters: cancelling a task that is awaiting a future cancels that future too, so without it one caller's timeout would cancel the shared computation for every other waiter and leave the cache empty. With it the cancelled caller still gets `CancelledError`, and the computation finishes for the rest. `if key not in connections: connections[key] = await connect(key)` is the race. + +## Hold The Lock Across The Decision + +When the decision itself must await, an `asyncio.Lock` may be held across it: + +```python +async with lock: + if key not in connections: + connections[key] = await connect(key) +``` + +That serializes every caller through one lock, which is a throughput choice the finding should note. Checking outside the lock and inserting inside it is the race. + +## Threads + +A `threading.Lock` around both the check and the write. `dict.setdefault(key, value)` is atomic under the GIL for a value already built, but the value expression is evaluated before the call, so it does not dedupe construction. Free-threaded builds keep `setdefault` atomic per the dict's internal lock; a check followed by `d[key] = ...` is the race on every build. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/os.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/os.md new file mode 100644 index 0000000..e8f4097 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/os.md @@ -0,0 +1,50 @@ +# Python `os` And `tempfile` + +Spells the filesystem primitives from [filesystem.md](../surfaces/filesystem.md). + +## Exclusive Create + +The `"x"` mode is `O_CREAT | O_EXCL`; `FileExistsError` is the answer. + +```python +try: + with open(path, "x") as f: + f.write(data) +except FileExistsError: + pass # someone else created it +``` + +The lower-level form is `os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)`. `os.mkdir` raises `FileExistsError` the same way; `os.makedirs(path, exist_ok=True)` is the idempotent form. `os.path.exists` before either is the race. + +## Atomic Replace + +`tempfile.mkstemp` creates exclusively with a unique name in the target's directory; `os.replace` is the atomic rename that also overwrites on Windows. + +```python +import os, tempfile + +fd, tmp = tempfile.mkstemp(dir=os.path.dirname(target)) +with os.fdopen(fd, "wb") as f: + f.write(data) + f.flush() + os.fsync(f.fileno()) +os.replace(tmp, target) +``` + +## Open Then Verify + +```python +import os, stat + +fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW) +st = os.fstat(fd) +if not stat.S_ISREG(st.st_mode) or st.st_uid != os.getuid(): + os.close(fd) + raise PermissionError(path) +``` + +`os.stat(path)` followed by `open(path)` is the race. + +## Untrusted Trees And Idempotent Delete + +`os.open(child, flags, dir_fd=dirfd)` is `openat`; resolve each child relative to the directory descriptor instead of rebuilding the full path. `Path(path).unlink(missing_ok=True)` is the idempotent delete. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/psycopg.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/psycopg.md new file mode 100644 index 0000000..15c7406 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/psycopg.md @@ -0,0 +1,48 @@ +# psycopg + +Spells the database primitives from [database.md](../surfaces/database.md) with psycopg 3. The SQL carries the condition; the cursor exposes `RETURNING` rows and `rowcount`. + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how psycopg spells it. + +## Upsert With Identity On Both Paths + +```python +cur.execute( + """INSERT INTO users (email) VALUES (%s) + ON CONFLICT (email) DO UPDATE SET email = EXCLUDED.email + RETURNING id""", + (email,), +) +user_id = cur.fetchone()[0] +``` + +## Conditional Update And Rowcount + +```python +cur.execute( + "UPDATE accounts SET balance = balance - %s WHERE id = %s AND balance >= %s", + (amount, account_id, amount), +) +if cur.rowcount == 0: + raise InsufficientFunds() +``` + +The same shape covers `WHERE status = 'pending'`, `WHERE version = %s`, and `WHERE owner_id = %s`. + +## Row Lock + +Every statement of the decision runs on the same connection inside one transaction; psycopg 3 opens a transaction on the first statement and `with conn.transaction():` commits at the end of the block when no transaction was already open. Inside an open transaction the block is only a savepoint, so the lock lives until the outer transaction ends. + +```python +with conn.transaction(): + cur.execute("SELECT * FROM subscriptions WHERE id = %s FOR UPDATE", (sub_id,)) + sub = cur.fetchone() + # decide + cur.execute("UPDATE subscriptions SET ... WHERE id = %s", (sub_id,)) +``` + +A connection in autocommit mode commits after the `SELECT`, which releases the lock before the decision. + +## Serializable Retry + +`conn.isolation_level = psycopg.IsolationLevel.SERIALIZABLE`, set while no transaction is active because the setting is rejected mid-transaction, and a bounded loop that retries the whole transaction on `psycopg.errors.SerializationFailure` (`40001`) and `psycopg.errors.DeadlockDetected` (`40P01`). `UniqueViolation` is retried only when the code can show the duplicate came from a value it chose after a read. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/sqlalchemy.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/sqlalchemy.md new file mode 100644 index 0000000..db9df7f --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/python/sqlalchemy.md @@ -0,0 +1,58 @@ +# SQLAlchemy + +Spells the database primitives from [database.md](../surfaces/database.md) with SQLAlchemy 2 and the Postgres dialect. + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how SQLAlchemy spells it. On MySQL, read [references/systems/mysql.md](../systems/mysql.md) instead. + +## Upsert With Identity On Both Paths + +```python +from sqlalchemy.dialects.postgresql import insert + +stmt = ( + insert(User) + .values(email=email) + .on_conflict_do_update(index_elements=[User.email], set_={"email": email}) + .returning(User.id) +) +user_id = session.execute(stmt).scalar_one() +``` + +`session.scalar(select(User).where(User.email == email))` then `session.add(User(email=email))` is the race. + +## Conditional Update And Rowcount + +```python +from sqlalchemy import update + +result = session.execute( + update(Order) + .where(Order.id == order_id, Order.status == "pending") + .values(status="paid") +) +if result.rowcount == 0: + raise AlreadyMoved() +``` + +Loading the ORM object, checking `order.status`, setting the attribute, and calling `commit()` is the race: the unit-of-work `UPDATE` carries only the primary key. + +## Optimistic Version + +`__mapper_args__ = {"version_id_col": version}` makes every unit-of-work `UPDATE` carry `WHERE version = :current` and raise `StaleDataError` when zero rows match; the caller reloads and retries, bounded. + +## Row Lock + +```python +sub = session.execute( + select(Subscription).where(Subscription.id == sub_id).with_for_update() +).scalar_one() +# decide +sub.plan = new_plan +session.commit() +``` + +Every other writer to that row must lock the same way. + +## Serializable Retry + +`create_engine(url, isolation_level="SERIALIZABLE")` or `connection.execution_options(isolation_level="SERIALIZABLE")`, and a bounded loop that retries the whole transaction on `OperationalError` whose `orig` carries SQLSTATE `40001` or `40P01` (`orig.sqlstate` on psycopg 3, `orig.pgcode` on psycopg2 and asyncpg). A `23505` inside `IntegrityError` is retried only when the code can show the duplicate came from a value it chose after a read. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/review/finding-template.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/review/finding-template.md new file mode 100644 index 0000000..788c4be --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/review/finding-template.md @@ -0,0 +1,69 @@ +# Finding Template + +One finding per check-then-act pair. The format forces the reviewer to name the interleaver and the consequence; a finding that cannot name both is not yet a finding. + +## Format + +``` +### + +- Location: (check) and (act) +- Check: +- Act: +- Window: +- Interleaver: +- Principle: +- Consequence: +- Fix: +- Residual: +``` + +Pairs judged safe go in a separate list using the safe-verdict format in [serializer-is-named.md](../principles/serializer-is-named.md). + +## Worked Example + +The code under review: + +```ts +// src/billing/redeem.ts +export async function redeem(userId: string, code: string) { + const coupon = await db.oneOrNone( + "SELECT id, uses, max_uses FROM coupons WHERE code = $1", + [code], + ); + if (!coupon) throw new NotFound(); + if (coupon.uses >= coupon.max_uses) throw new Exhausted(); // line 9 + + await payments.applyDiscount(userId, coupon.id); // line 11 + + await db.none( + "UPDATE coupons SET uses = uses + 1 WHERE id = $1", + [coupon.id], + ); // line 16 +} +``` + +The finding: + +``` +### Coupon can be redeemed past max_uses + +- Location: src/billing/redeem.ts:9 (check) and src/billing/redeem.ts:16 (act) +- Check: SELECT uses, max_uses for the code; proceed only if uses < max_uses +- Act: UPDATE uses = uses + 1, after a discount has already been applied +- Window: the SELECT, the applyDiscount network call, then the UPDATE; tens to hundreds of milliseconds +- Interleaver: any concurrent redeem of the same code, including the same user double-submitting the form or retrying a timed-out request. Each reads uses = max_uses - 1, passes the check, applies a discount, and increments. +- Principle: the write must carry its condition. The UPDATE at line 16 is unconditional; the condition lives only in the application check at line 9. +- Consequence: N concurrent requests at the last use all receive the discount; uses ends at max_uses + N - 1. Money is given away and the stored count no longer matches reality. +- Fix: claim the use before applying the discount, with the condition inside the statement: + + UPDATE coupons + SET uses = uses + 1 + WHERE id = $1 AND uses < max_uses + RETURNING uses; + + Zero rows means exhausted; throw before touching payments. Add CHECK (uses <= max_uses) as a backstop. The order becomes claim, then discount, then (on discount failure) a compensating UPDATE uses = uses - 1. +- Residual: a crash between the claim and the discount leaves a consumed use with no discount applied. Acceptable if the compensation runs from a reconciliation job; otherwise wrap the claim and a pending-discount row in one transaction and apply the discount from an outbox. +``` + +What makes this finding complete: the interleaver names a realistic actor (double submit, retry), the principle is named so the fix follows from it, the consequence is stated in domain terms (money, wrong count), the fix is a conditional write at the store that owns the count, and the residual is honest about what the fix moves rather than removes. The example is TypeScript; the finding format is the same for every language, and the fix is spelled for the driver in use by the matching library file. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/review/fix-catalog.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/review/fix-catalog.md new file mode 100644 index 0000000..c5b26a8 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/review/fix-catalog.md @@ -0,0 +1,45 @@ +# Fix Catalog + +Pick the primitive by the store that owns the state, not by the language the check is written in. Each row names the smell, the principle the smell violates, the primitive that makes the second actor lose, and the surface file that shows the shape. The library files spell the primitive in the library under review. + +Principle abbreviations: Condition is [the write must carry its condition](../principles/write-carries-condition.md); Owner is [only the owner of the state can decide](../principles/owner-decides.md); Lock is [a lock only helps when the read happens under it](../principles/lock-moves-the-race.md); Retry is [retry belongs around the conditional write](../principles/retry-wraps-the-write.md); Idempotence is [idempotence absorbs what ordering cannot prevent](../principles/idempotence-absorbs.md). + +| Surface | Smell | Principle | Atomic primitive | Reference | +| --- | --- | --- | --- | --- | +| Filesystem | exists then create | Condition | exclusive create (`O_CREAT \| O_EXCL`) | [filesystem](../surfaces/filesystem.md) | +| Filesystem | stat then open | Condition | open with no-follow, then `fstat` the descriptor | [filesystem](../surfaces/filesystem.md) | +| Filesystem | build name, check free, create temp | Condition | atomic unique temp creation (`mkstemp` family) | [filesystem](../surfaces/filesystem.md) | +| Filesystem | write in place | Condition | write temp in same dir, `fsync`, `rename` | [filesystem](../surfaces/filesystem.md) | +| Filesystem | exists then unlink or mkdir | Idempotence | call it and handle `ENOENT` / `EEXIST` | [filesystem](../surfaces/filesystem.md) | +| Database | SELECT then INSERT (find-or-create) | Condition | unique index plus `INSERT ... ON CONFLICT` | [database](../surfaces/database.md) | +| Database | read balance then write balance | Condition | `UPDATE ... SET x = x - $1 WHERE x >= $1`, check rowcount, `CHECK` constraint | [database](../surfaces/database.md) | +| Database | read status then set status | Condition | `UPDATE ... WHERE status = 'expected'`, check rowcount | [database](../surfaces/database.md) | +| Database | read several columns then write | Condition, Retry | version column, `WHERE version = $n`, retry on zero rows | [database](../surfaces/database.md) | +| Database | decide in app code from row data | Lock | `SELECT ... FOR UPDATE` inside the transaction, every writer locks | [database](../surfaces/database.md) | +| Database | count then insert (per-key limit) | Lock | `pg_advisory_xact_lock(hashtext(key))` inside the transaction | [database](../surfaces/database.md) | +| Database | complex multi-row invariant | Retry | `SERIALIZABLE` plus whole-transaction retry on `40001` and `40P01` | [database](../surfaces/database.md) | +| Authorization | check, await, write | Condition | fold the condition into the write (`WHERE owner_id = $u`, membership join) | [authorization](../surfaces/authorization.md) | +| Authorization | check on preview, write on confirm | Owner | re-check on the confirm request | [authorization](../surfaces/authorization.md) | +| Authorization | check, enqueue, worker writes | Owner | re-check in the worker, or a signed short-lived capability | [authorization](../surfaces/authorization.md) | +| Authorization | cached ACL | Owner | per-write check, or TTL the domain accepts and the finding states | [authorization](../surfaces/authorization.md) | +| Cache | miss then compute (one process) | Condition | single-flight on an in-flight map | [cache](../surfaces/cache.md) | +| Cache | miss then compute (fleet) | Lock | `SET lock:key token NX PX ttl`, holder computes, compare-and-delete release | [cache](../surfaces/cache.md) | +| Cache | not exists then set | Condition | `SET key v NX`, `add`, insert-if-absent | [cache](../surfaces/cache.md) | +| Cache | cached negative outlives the create | Owner | invalidate on create, short negative TTL, or write-through | [cache](../surfaces/cache.md) | +| Cache | decide from a cached count or permission | Owner | the write carries its own condition against the owning store | [cache](../surfaces/cache.md) | +| NATS KV | get then put | Condition, Retry | `update` with the read revision; `create` for first write | [distributed](../surfaces/distributed.md) | +| JetStream | read last seq then publish | Condition, Retry | `Nats-Expected-Last-Subject-Sequence` (or `-Last-Sequence`) header | [distributed](../surfaces/distributed.md) | +| JetStream | retried publish | Idempotence | `Nats-Msg-Id` within the duplicate window | [distributed](../surfaces/distributed.md) | +| Event store | load then append | Condition, Retry | append with `expected_version` equal to the loaded version | [distributed](../surfaces/distributed.md) | +| Consumer | contains(id) then apply then record | Idempotence | insert the id (unique) and the effect in one transaction | [distributed](../surfaces/distributed.md) | +| Consumer | effect in a non-transactional store | Idempotence | idempotent effect keyed by a deterministic id | [distributed](../surfaces/distributed.md) | +| Outbox | read unsent, publish, mark sent | Condition | claim rows with an expiring `UPDATE ... WHERE claimed_until < now() RETURNING`, publish with `Nats-Msg-Id` = row id, reclaim after expiry | [distributed](../surfaces/distributed.md) | +| In-process | check, yield, set | Condition | publish the in-flight claim before the first yield | [in-process](../surfaces/in-process.md) | +| In-process | lock, release, yield, lock, insert | Lock | hold an async-aware lock across the decision, or a per-key entry API | [in-process](../surfaces/in-process.md) | +| In-process | two calls into one server | Owner | one call whose handler checks and writes | [in-process](../surfaces/in-process.md) | +| In-process | lookup then insert in a shared table | Condition | insert-if-absent, atomic counter, compare-and-swap | [in-process](../surfaces/in-process.md) | +| In-process | lookup owner then register | Condition | the register call is the claim | [in-process](../surfaces/in-process.md) | + +## Choosing Between Optimistic And Pessimistic + +Optimistic (version, revision, expected sequence) costs nothing when conflicts are rare and pushes the conflict to the caller as a retry. Pessimistic (row lock, advisory lock, single owning process) costs waiting on every call and never surfaces a conflict. Pick optimistic when writes to the same key are rare or the caller can re-decide cheaply. Pick pessimistic when the decision is expensive, conflicts are common, or a retry would repeat a side effect that is not idempotent. The cases where a lock is the answer and the cases where it only moves the race are in [lock-moves-the-race.md](../principles/lock-moves-the-race.md). diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/atomics.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/atomics.md new file mode 100644 index 0000000..51c9bee --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/atomics.md @@ -0,0 +1,30 @@ +# Rust Atomics + +Spells scalar compare-and-swap from [in-process.md](../surfaces/in-process.md). The loop is the retry from [retry-wraps-the-write.md](../principles/retry-wraps-the-write.md) in its smallest form: reload, re-decide, conditional write. + +## Compare-And-Swap Loop + +```rust +use std::sync::atomic::{AtomicU64, Ordering}; + +loop { + let current = counter.load(Ordering::Acquire); + if current >= limit { return Err(Error::Exhausted); } + if counter + .compare_exchange(current, current + 1, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + { + break; + } +} +``` + +`counter.load()` then `counter.store(current + 1)` is the race; `fetch_add` alone cannot enforce the limit because it has no condition. + +## fetch_update + +`AtomicU64::fetch_update(AcqRel, Acquire, |cur| (cur < limit).then_some(cur + 1))` is the same loop with the decision as a closure and `Err(cur)` when the closure declines. + +## Where Atomics Stop + +An atomic protects one word. A decision that reads two atomics and writes one has the same gap as any two reads; use a mutex over the pair or fold the state into one word. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/moka-dashmap.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/moka-dashmap.md new file mode 100644 index 0000000..3125ab8 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/moka-dashmap.md @@ -0,0 +1,41 @@ +# moka And DashMap + +Spells single-flight and per-key atomic entry from [cache.md](../surfaces/cache.md) and [in-process.md](../surfaces/in-process.md). + +## Single-Flight With moka + +`moka::future::Cache::get_with` computes once per key and hands the same value to every concurrent caller; `try_get_with` does the same for a fallible initializer and does not cache the error. + +```rust +let value = cache + .get_with(key.clone(), async { compute_expensive(&key).await }) + .await; +``` + +`cache.get(&key)` then `cache.insert(key, compute().await)` is the stampede. + +## Per-Key Entry With DashMap + +`entry(key).or_insert_with(...)` is atomic per key for synchronous construction. The shard lock is held while the closure runs, so the closure must not await or call back into the same map. + +```rust +let conn = map.entry(host.clone()).or_insert_with(|| connect_blocking(&host)).clone(); +``` + +`map.get(&host)` then `map.insert(host, ...)` is two operations with a gap, even though each one is internally locked. + +## Insert-If-Absent As The Claim + +For async construction, store a shareable in-flight future through `or_insert_with` and await it after the entry guard is dropped. `futures::FutureExt::shared` makes one future awaitable by every caller; a `JoinHandle` cannot be cloned, so it cannot play this role. + +```rust +use futures::FutureExt; + +let pending = map + .entry(key.clone()) + .or_insert_with(|| build(key.clone()).boxed().shared()) + .clone(); +let value = pending.await; +``` + +Holding the `Entry` across the await keeps the shard locked and stalls every other key in that shard. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/sqlx.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/sqlx.md new file mode 100644 index 0000000..5324673 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/sqlx.md @@ -0,0 +1,56 @@ +# sqlx + +Spells the database primitives from [database.md](../surfaces/database.md). The SQL carries the condition; sqlx exposes `RETURNING` rows and `rows_affected()`. + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how sqlx spells it. + +## Upsert With Identity On Both Paths + +```rust +let id: i64 = sqlx::query_scalar( + "INSERT INTO users (email) VALUES ($1) + ON CONFLICT (email) DO UPDATE SET email = EXCLUDED.email + RETURNING id", +) +.bind(&email) +.fetch_one(&pool) +.await?; +``` + +## Conditional Update And Rowcount + +```rust +let done = sqlx::query( + "UPDATE accounts SET balance = balance - $1 WHERE id = $2 AND balance >= $1", +) +.bind(amount) +.bind(id) +.execute(&pool) +.await?; +if done.rows_affected() == 0 { + return Err(Error::InsufficientFunds); +} +``` + +The same shape covers `WHERE status = 'pending'`, `WHERE version = $3`, and `WHERE owner_id = $3`. + +## Row Lock + +Run every statement of the decision on the `Transaction`, not on the pool; a pool call inside the transaction uses another connection and is outside the lock. + +```rust +let mut tx = pool.begin().await?; +let row = sqlx::query_as::<_, Subscription>("SELECT * FROM subscriptions WHERE id = $1 FOR UPDATE") + .bind(id) + .fetch_one(&mut *tx) + .await?; +// decide +sqlx::query("UPDATE subscriptions SET ... WHERE id = $1").bind(id).execute(&mut *tx).await?; +tx.commit().await?; +``` + +Every other writer to that row must lock the same way. + +## Serializable Retry + +Set the isolation with `sqlx::query("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE")` as the first statement on the transaction, and retry the whole transaction, bounded, when the error's SQLSTATE (`e.as_database_error().and_then(|d| d.code())`) is `40001` or `40P01`. Retry `23505` only when the code can show the duplicate came from a value it chose after a read; otherwise it is a persistent error. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/std-fs.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/std-fs.md new file mode 100644 index 0000000..f6a754a --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/std-fs.md @@ -0,0 +1,51 @@ +# Rust `std::fs` + +Spells the filesystem primitives from [filesystem.md](../surfaces/filesystem.md) with the standard library and the `tempfile` crate. + +## Exclusive Create + +`create_new(true)` is `O_CREAT | O_EXCL`; `AlreadyExists` is the answer. + +```rust +use std::fs::OpenOptions; +use std::io::ErrorKind; + +match OpenOptions::new().write(true).create_new(true).open(&path) { + Ok(file) => write_all(file, data)?, + Err(e) if e.kind() == ErrorKind::AlreadyExists => { /* someone else created it */ } + Err(e) => return Err(e.into()), +} +``` + +`std::fs::create_dir` fails with `AlreadyExists` the same way; `create_dir_all` is the idempotent form. + +## Atomic Replace + +Create the temp file in the target's directory so `persist` is a rename on the same filesystem. + +```rust +use std::io::Write; +use tempfile::NamedTempFile; + +let dir = target.parent().unwrap(); +let mut tmp = NamedTempFile::new_in(dir)?; +tmp.write_all(data)?; +tmp.as_file().sync_all()?; +tmp.persist(&target)?; +``` + +## Open Then Verify + +On Unix, `OpenOptionsExt::custom_flags(libc::O_NOFOLLOW)` rejects a symlink at the final component; then `file.metadata()` describes the descriptor you hold, not the path. + +```rust +use std::os::unix::fs::OpenOptionsExt; + +let file = OpenOptions::new().read(true).custom_flags(libc::O_NOFOLLOW).open(&path)?; +let meta = file.metadata()?; +if !meta.is_file() { return Err(Error::Forbidden); } +``` + +## Untrusted Trees And Idempotent Delete + +The standard library has no `openat`; use `cap-std` or `rustix` to resolve children relative to an open directory. `std::fs::remove_file` returns `NotFound` rather than raising; match on it instead of checking `exists` first. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/tokio.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/tokio.md new file mode 100644 index 0000000..8b6cdfe --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/rust/tokio.md @@ -0,0 +1,37 @@ +# tokio + +Spells the async lock and one-time initialization primitives from [in-process.md](../surfaces/in-process.md). + +## Hold The Lock Across The Decision + +A `std::sync::Mutex` guard held across an `.await` makes the future `!Send`, so `tokio::spawn` rejects it, and on a current-thread runtime it deadlocks the moment another task needs the lock. Dropping the guard before the await reopens the race. Switch to `tokio::sync::Mutex` and hold it across the whole decision: + +```rust +let mut state = state.lock().await; +if !state.contains_key(&key) { + let value = build(&key).await; + state.insert(key, value); +} +``` + +That serializes every caller through one lock, which is a throughput choice the finding should note. When per-key atomicity is enough, prefer [moka-dashmap.md](moka-dashmap.md). + +## Lock, Release, Await, Re-Lock + +```rust +let present = state.lock().await.contains_key(&key); // guard dropped at end of statement +if !present { + let value = build(&key).await; + state.lock().await.insert(key, value); // second critical section +} +``` + +Two tasks both see absent and both insert. The first guard is dropped at the semicolon; that drop is the yield point the finding cites. + +## One-Time Initialization + +`tokio::sync::OnceCell::get_or_init` runs the async initializer once and hands every concurrent caller the same value. `std::sync::OnceLock::get_or_init` is the synchronous form. A `Option` behind a mutex with `if none { set }` across an await is the race. + +## Publishing The In-Flight Work + +The Rust spelling of "store the promise, not the result" is a shared future inserted under the key before any await: `DashMap::entry(key).or_insert_with(|| build(key).boxed().shared())`, shown in [moka-dashmap.md](moka-dashmap.md). A `JoinHandle` cannot be cloned, so it cannot be the thing later callers join. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/authorization.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/authorization.md new file mode 100644 index 0000000..41b86bf --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/authorization.md @@ -0,0 +1,66 @@ +# Authorization Surfaces + +A permission check answers "may this principal do this, now". Any distance between that answer and the effect is a window in which the grant can be revoked, the ownership transferred, or the quota consumed by a sibling request. The check belongs at the point of effect, and ideally inside the same statement as the effect. + +## Principles At Work + +- [A read is a fact about the past](../principles/read-is-the-past.md): a grant loaded at login, at preview, or at enqueue time describes that moment. +- [The write must carry its condition](../principles/write-carries-condition.md): `WHERE owner_id = $u`, a membership join, a conditional quota decrement. +- [Only the owner of the state can decide](../principles/owner-decides.md): the worker, not the enqueuer, is at the point of effect, so the worker re-checks. +- [Idempotence absorbs what ordering cannot prevent](../principles/idempotence-absorbs.md): a replayed job must not re-apply an effect its first run already made. + +## Check, Await, Act + +Avoid: + +``` +if can(user, "edit", doc_id): + rendered = render(body) # slow, yields + UPDATE documents SET body = rendered WHERE id = doc_id +``` + +During `render`, the document can be transferred to another owner or the user removed from the team. Prefer scoping the write by the authorization condition: + +```sql +UPDATE documents +SET body = $1 +WHERE id = $2 + AND owner_id = $3; +-- rowcount 0: not found or not owned, treat both as forbidden +``` + +When the rule is a membership rather than a column, join it into the same statement: + +```sql +UPDATE documents d +SET body = $1 +FROM team_members m +WHERE d.id = $2 + AND m.team_id = d.team_id + AND m.user_id = $3 + AND m.role IN ('editor', 'owner'); +``` + +## Preview Then Confirm + +A two-step flow (one request shows the action and checks permission, a later request performs it) is two moments. The performing request must check again. The same holds for a confirmation dialog and for a wizard that checks on its first step and writes on its last. + +## Queue Hop + +A job enqueued after a permission check runs later, possibly much later, possibly on a replay. The worker must re-check at execution time against current state. If the worker cannot reach the authorization source, the job carries a short-lived signed capability (principal, action, resource, expiry) that the worker verifies, and expiry bounds the window. + +## Rehydrated Sessions And Cached ACLs + +A permission set loaded at login and cached in the session is a check made once and used for hours. Treat each privileged write as its own check against the source of truth, or cache with a TTL short enough that the domain accepts the window and the finding states that TTL. + +## Quotas And Limits + +"If count < limit then create" is a counter race wearing an authorization hat. Fold it into the write (see the cross-row invariants section of [database.md](database.md)) or into a conditional counter decrement, never into a separate read. + +## What The Finding Must Cite + +The check, the effect, what runs between them, and which actor can change the grant in that window (an admin, a transfer, a concurrent request on the same quota). State whether the effect's statement already encodes the condition; if it does, the pair is safe and the statement is the evidence. + +## Library Files + +The fix is the conditional statement from [database.md](database.md). The database library files show how the rowcount is read back: [node-postgres](../typescript/node-postgres.md), [prisma](../typescript/prisma.md), [drizzle](../typescript/drizzle.md), [sqlx](../rust/sqlx.md), [ecto](../elixir/ecto.md), [database-sql](../go/database-sql.md), [psycopg](../python/psycopg.md), [sqlalchemy](../python/sqlalchemy.md). diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/cache.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/cache.md new file mode 100644 index 0000000..52d35bf --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/cache.md @@ -0,0 +1,84 @@ +# Cache Surfaces + +A cache miss is a check. Filling the cache is the act. Between them, every other caller that misses on the same key does the same work, and the last writer wins regardless of which value was freshest. + +## Principles At Work + +- [Only the owner of the state can decide](../principles/owner-decides.md): a cached count or permission is a copy; the write must still carry its condition against the store that owns the data. +- [The write must carry its condition](../principles/write-carries-condition.md): `SET NX`, `add`, insert-if-absent, the in-flight claim published before the first yield. +- [A lock only helps when the read happens under it](../principles/lock-moves-the-race.md): the `SET NX PX` lease bounds the stampede and is itself a check-then-act. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): single-flight is safe only because the map read and write have no yield between them, and the finding says so. + +## Get Or Compute Stampede + +Avoid: + +``` +value = cache.get(key) +if value is missing: + value = compute_expensive(key) + cache.set(key, value, ttl) +return value +``` + +When the key expires under load, every concurrent request recomputes. Prefer single-flight inside the process, so one computation serves all concurrent callers for that key: + +``` +inflight: map key -> pending result + +value = cache.get(key) +if value present: return value + +pending = inflight.get(key) +if pending is missing: + pending = start(compute_expensive(key) then cache.set(key, result, ttl)) + inflight.set(key, pending) # no yield between get and set +return await pending # remove from inflight when settled +``` + +The `inflight.get` and `inflight.set` must run with no yield between them; that is what makes the dedupe atomic. + +## Cross-Process Stampede + +Single-flight covers one process. Across a fleet, take a short lease on the key and let only the holder recompute: + +``` +SET lock:{key} {token} NX PX 5000 +``` + +A reply of `OK` means this caller computes and then writes the value. Anyone else either waits briefly and re-reads, or serves the stale value if one exists (stale-while-revalidate). Release the lease only if the token still matches, in a Lua script, so a slow holder does not delete a successor's lease: + +```lua +if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) +end +return 0 +``` + +The lease bounds the stampede; it does not guarantee a single computation. A holder that exceeds the lease lets a second holder in. The finding must say whether double computation is merely wasteful or actually wrong. + +## Check Then Set + +Avoid `if not exists(key) then set(key, v)`. Prefer the conditional write the store already offers: `SET key v NX` in Redis, `add` in Memcached, an insert-if-absent in an in-memory table. The reply tells you whether you won. + +## Negative Caching + +Caching "not found" for a key that is then created elsewhere leaves the negative entry serving a lie for its full TTL. Either invalidate on the create path (the creator knows the key), keep negative TTLs short enough that the domain accepts the window, or write-through on create. The finding states which one is in place or which is missing. + +## Cache As Source Of Truth + +Any decision that reads a cached count, balance, or permission and then writes to the real store has moved the check onto a copy. The cache can inform the fast path, but the write must still carry its own condition against the store that owns the data. + +## What The Finding Must Cite + +The key, the TTL, whether the fill path dedupes in-process and across processes, and whether a double fill produces a wrong value or just wasted work. + +## Library Files + +For what the store itself guarantees, read [redis](../systems/redis.md) first; the lease and release spellings for ioredis, Redix, go-redis, and redis-py live at the end of that file. + +- TypeScript: [event-loop](../typescript/event-loop.md) for single-flight +- Rust: [moka-dashmap](../rust/moka-dashmap.md) +- Elixir: [ets](../elixir/ets.md) +- Go: [singleflight](../go/singleflight.md), [sync](../go/sync.md) +- Python: [asyncio](../python/asyncio.md) diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/database.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/database.md new file mode 100644 index 0000000..f296643 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/database.md @@ -0,0 +1,126 @@ +# Database Surfaces + +A row read in one statement says nothing about the row when the next statement runs. The database is the only actor that can see every writer, so the condition has to be expressed where the database enforces it: a constraint, a conditional `WHERE`, a row lock, or an expected version. + +A transaction on its own does not help. Under `READ COMMITTED` (the Postgres default) and under InnoDB's default `REPEATABLE READ`, two transactions can both `SELECT` nothing and both `INSERT`. Only a unique constraint or a lock makes one of them lose. + +## Principles At Work + +- [Only the owner of the state can decide](../principles/owner-decides.md): the database sees every writer; application code sees one snapshot. +- [The write must carry its condition](../principles/write-carries-condition.md): `ON CONFLICT`, `WHERE status = ...`, `WHERE balance >= ...`, `WHERE version = ...`. +- [A lock only helps when the read happens under it](../principles/lock-moves-the-race.md): `FOR UPDATE` and advisory locks, taken before the read, by every writer. +- [Retry belongs around the conditional write](../principles/retry-wraps-the-write.md): the zero-rowcount branch and the `40001` retry loop. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): a safe verdict names the unique index from the migration, not from an ORM helper. + +## Find Or Create + +Avoid: + +```sql +SELECT id FROM users WHERE email = $1; +-- none found +INSERT INTO users (email) VALUES ($1); +``` + +Prefer a unique index and an upsert, and read the identity back in the same statement: + +```sql +INSERT INTO users (email) VALUES ($1) +ON CONFLICT (email) DO UPDATE SET email = EXCLUDED.email +RETURNING id; +``` + +`DO UPDATE` on a no-op column makes `RETURNING` fire on both paths. `DO NOTHING` returns no row on conflict, so a follow-up `SELECT` is needed, which is acceptable because the row now certainly exists. MySQL spells it `INSERT ... ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id)`. + +An ORM helper that translates the unique violation into a validation error prevents nothing without the unique index in the schema. + +`SELECT ... FOR UPDATE` does not solve find-or-create. Locking a row that does not exist locks nothing, and two inserters still race. The constraint is the primitive. + +## Read, Modify, Write + +Avoid: + +``` +row = SELECT balance FROM accounts WHERE id = $1 +if row.balance >= amount: + UPDATE accounts SET balance = row.balance - amount WHERE id = $1 +``` + +Two debits read the same balance and both succeed. Prefer the condition and the arithmetic inside one statement, and treat zero rows as the failure: + +```sql +UPDATE accounts +SET balance = balance - $1 +WHERE id = $2 AND balance >= $1; +-- rowcount 0 means insufficient funds or missing account +``` + +A `CHECK (balance >= 0)` constraint is the backstop that catches any writer that forgets the `WHERE`. + +## Status Transitions + +Avoid `SELECT status` then `UPDATE status`. Prefer: + +```sql +UPDATE orders +SET status = 'paid', paid_at = now() +WHERE id = $1 AND status = 'pending'; +``` + +Zero rows means another worker already moved it. Whether that is an error or a no-op depends on the domain; the finding must say which. + +## Optimistic Versioning + +When the act depends on several columns read earlier, carry a version column and update only if it is unchanged: + +```sql +UPDATE documents +SET body = $1, version = version + 1 +WHERE id = $2 AND version = $3; +``` + +Zero rows means stale read: reload, re-decide, retry or surface the conflict. + +## Pessimistic Row Lock + +When the decision needs the row's current data and cannot be folded into one statement, lock the row for the length of the transaction: + +```sql +BEGIN; +SELECT * FROM subscriptions WHERE id = $1 FOR UPDATE; +-- decide in application code +UPDATE subscriptions SET ... WHERE id = $1; +COMMIT; +``` + +Everything that mutates the same row must also take the lock, or the lock protects nothing. + +## Cross-Row Invariants + +"At most N active sessions per user" cannot be a unique constraint. Serialize the invariant with an advisory lock scoped to the key, inside the transaction: + +```sql +SELECT pg_advisory_xact_lock(hashtext('sessions:' || $1)); +SELECT count(*) FROM sessions WHERE user_id = $1 AND active; +INSERT INTO sessions ...; +``` + +The lock releases at commit. Hash collisions only make the lock coarser, never unsafe. + +## Serializable Isolation + +`SERIALIZABLE` makes the database detect the interleaving and abort one transaction with SQLSTATE `40001`. It is correct only when every caller retries the whole transaction on that code and on `40P01`. Without the retry loop it has turned a silent race into a user-facing error. `23505` and `23P01` are retried only when the caller can show the conflict was a race on a value it chose from a prior read, because the same codes also mean a persistent error. + +## What The Finding Must Cite + +The `SELECT`, the write it gates, the isolation level in effect, and whether a constraint or lock already exists for that key. Name the constraint or index when claiming a pair is safe. + +## Library Files + +For what the store itself guarantees, read [postgres](../systems/postgres.md), [mysql](../systems/mysql.md), or [sqlite](../systems/sqlite.md) first. + +- TypeScript: [node-postgres](../typescript/node-postgres.md), [prisma](../typescript/prisma.md), [drizzle](../typescript/drizzle.md) +- Rust: [sqlx](../rust/sqlx.md) +- Elixir: [ecto](../elixir/ecto.md) +- Go: [database-sql](../go/database-sql.md) +- Python: [psycopg](../python/psycopg.md), [sqlalchemy](../python/sqlalchemy.md) diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/distributed.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/distributed.md new file mode 100644 index 0000000..fc94c11 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/distributed.md @@ -0,0 +1,111 @@ +# Distributed Store Surfaces + +Across nodes there is no shared memory and no shared lock that costs nothing. What the stores offer instead is a conditional write: "apply this only if the last thing you saw is still the last thing". Every check-then-act against a distributed store must be rewritten as one of these conditional writes, or it is a lost update waiting for load. + +## Principles At Work + +- [The write must carry its condition](../principles/write-carries-condition.md): the KV revision, the expected-sequence header, the expected version on append. +- [Retry belongs around the conditional write](../principles/retry-wraps-the-write.md): a revision or sequence mismatch means reload, re-decide, retry, bounded. +- [Idempotence absorbs what ordering cannot prevent](../principles/idempotence-absorbs.md): redelivery is a second run with the guards intact; only a transactional dedupe or an idempotent effect survives it. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): a single consumer or a partition owner is a serializer only when the config is cited and redelivery is accounted for. + +## NATS KV + +Avoid reading a key, deciding, and writing back with a plain put: + +``` +entry = kv.get("config.limits") +next = bump(entry.value) +kv.put("config.limits", next) # overwrites whatever landed in between +``` + +Prefer the revision-guarded update. Every KV entry carries a revision; `update` fails when the key's current revision differs from the one you pass: + +``` +entry = kv.get("config.limits") +next = bump(entry.value) +kv.update("config.limits", next, expected_revision = entry.revision) +# on revision mismatch: re-get, re-decide, retry +``` + +For first-time creation use `create`, which fails if the key already holds a live value. `create`, then `update` on the error path, is the correct find-or-create for KV. A plain `put` is only safe when the writer does not care what it overwrites. + +## JetStream Publish + +A stream append can be made conditional through headers on the publish: + +- `Nats-Expected-Last-Sequence`: the stream's last sequence must equal this value. +- `Nats-Expected-Last-Subject-Sequence`: the last sequence on this message's subject must equal this value. This is the per-aggregate expected version when each aggregate publishes to its own subject. +- `Nats-Expected-Last-Msg-Id`: the last message's `Nats-Msg-Id` must equal this value. +- `Nats-Expected-Stream`: the message must land on this stream, which catches subject overlap misconfiguration. + +A mismatch is rejected by the server and surfaces as a publish error. The publisher reloads from the subject, re-decides, and retries. Reading the subject's last sequence and then publishing without the header is the TOCTOU. + +`Nats-Msg-Id` on its own deduplicates publishes with the same id inside the stream's duplicate window. That protects against a retried publish, not against two different decisions racing on the same aggregate. + +## Event Store Append + +The same shape with explicit vocabulary. Load the stream, fold to current state, decide, append with the expected version equal to the version you loaded: + +``` +events, version = store.read(stream) +state = fold(events) +new_events = decide(state, command) +store.append(stream, new_events, expected_version = version) +# on wrong-expected-version: reload and retry, bounded +``` + +Appending with "any" expected version, or with a version from a cached projection, is a check made against a copy. + +## At-Least-Once Consumers + +A consumer that processes a message and then acks can crash between the effect and the ack. The message is redelivered, to this instance or another, and the effect runs twice. An ack-wait that is shorter than processing time produces the same redelivery with no crash at all. + +Avoid: + +``` +if not processed.contains(msg.id): + apply(msg) + processed.insert(msg.id) + msg.ack() +``` + +Two instances handling the same redelivered message both pass the `contains` check. Prefer recording the message id in the same transaction as the effect, with a unique constraint on the id, and treating the constraint violation as "already done": + +```sql +BEGIN; +INSERT INTO processed_messages (msg_id) VALUES ($1); -- unique; violation means done +UPDATE accounts SET balance = balance - $2 WHERE id = $3; +COMMIT; +``` + +When the effect is not in a transactional store, make the effect itself idempotent with a deterministic key (an upsert on a client-generated id, a payment with an idempotency key the provider enforces), or accept duplicates and say so in the finding. + +Check the consumer configuration while here: `ack_wait` must exceed the worst-case processing time, and `max_deliver` bounds how many times the double effect can happen before the message parks. + +## Outbox And Relay + +A relay that reads unsent rows, publishes them, and marks them sent has the same two-instance problem. A permanent claim (`SET claimed_by = $1 WHERE claimed_by IS NULL`) fixes the race between relays and creates a new one with the crash: a relay that dies after the claim and before the publish leaves a row that no relay will ever pick up again. The claim has to expire, and the publish has to be safe to repeat. + +```sql +UPDATE outbox +SET claimed_by = $1, claimed_until = now() + interval '30 seconds' +WHERE id = ANY($2) + AND sent_at IS NULL + AND (claimed_until IS NULL OR claimed_until < now()) +RETURNING id, payload; +``` + +The relay publishes only the rows the `UPDATE` returned, with `Nats-Msg-Id` set to the outbox row id, then marks them with `UPDATE outbox SET sent_at = now() WHERE id = $1 AND claimed_by = $2`. A crashed relay's rows become claimable again when `claimed_until` passes; the next relay republishes them with the same `Nats-Msg-Id`, and the stream drops the duplicate if the first publish did land. The claim window has to exceed the relay's worst-case publish time, or a slow relay and a reclaiming relay both publish, which the same message id absorbs inside the stream's duplicate window and nothing absorbs after it. + +`SELECT ... FOR UPDATE SKIP LOCKED` inside one transaction that also marks the rows sent is the other shape: the lock is released by the crash itself, so there is nothing to reclaim. It holds a transaction open across the publish, which is the cost. + +The residual either way is the stream's duplicate window: a reclaim that happens after it expires publishes the row twice, so consumers still dedupe on the message id. + +## What The Finding Must Cite + +The store, the read that produced the decision, the write that followed, and whether the write carried the expected revision, sequence, or version. For consumers, the ack mode, `ack_wait`, and whether the dedupe record and the effect commit together. + +## Library Files + +For what the store itself guarantees, read [nats](../systems/nats.md) first; the publish, KV, and consumer spellings for nats.js, async-nats, nats.go, nats-py, and the Elixir clients live at the end of that file. The transactional dedupe on the consumer side is in the database library file for the language, for example [ecto](../elixir/ecto.md). diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/filesystem.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/filesystem.md new file mode 100644 index 0000000..3dbcedc --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/filesystem.md @@ -0,0 +1,86 @@ +# Filesystem Surfaces + +A path is a name, not a file. Anything checked by name (`exists`, `stat`, `access`, `is_symlink`) can point at something else by the time the path is opened. The fix is to let the open itself carry the condition, then inspect the descriptor you actually got. + +## Principles At Work + +- [A read is a fact about the past](../principles/read-is-the-past.md): `stat` describes the path at the moment of the call, not at the moment of the `open`. +- [Only the owner of the state can decide](../principles/owner-decides.md): the kernel owns the namespace, so only a flag the kernel evaluates at open time (`O_EXCL`, `O_NOFOLLOW`) can decide. +- [The write must carry its condition](../principles/write-carries-condition.md): exclusive create, no-follow open, atomic rename. +- [Idempotence absorbs what ordering cannot prevent](../principles/idempotence-absorbs.md): delete and `mkdir` are made idempotent by handling `ENOENT` and `EEXIST` instead of checking first. + +## Exists Then Create + +Avoid: + +``` +if not exists(path): + write(path, data) +``` + +Two processes both see "missing" and both write; the second silently overwrites the first. Prefer an exclusive create and treat the failure as the answer: + +``` +try: + create_exclusive(path, data) # O_CREAT | O_EXCL +except AlreadyExists: + decide what "someone else created it" means here +``` + +The same applies to directories. `mkdir` already fails with `EEXIST`; checking first adds a race and removes nothing. + +## Stat Then Open + +Avoid: + +``` +st = stat(path) +if is_regular(st) and owner(st) == me: + fd = open(path) +``` + +Between `stat` and `open` the path can be replaced by a symlink to a file you must not read. Prefer opening first and checking what you hold: + +``` +fd = open(path, NOFOLLOW) +st = fstat(fd) +if not is_regular(st) or owner(st) != me: + close(fd); fail +``` + +`fstat` describes the open descriptor, which cannot be swapped. `NOFOLLOW` rejects a symlink at the final component. + +## Walking Untrusted Directory Trees + +Each component of a path can be swapped while you walk it. Open the directory once and resolve children relative to that descriptor (`openat`) rather than rebuilding the full path per step. + +## Temp Files + +Avoid building a temp name, checking it is free, and then creating it. Use a primitive that creates atomically with a unique name (`mkstemp` and its library equivalents), or create inside a private directory made with `mkdtemp`. + +## Replace A File Atomically + +Avoid writing directly into the target; readers can observe a partial file, and a crash leaves it truncated. Prefer write-to-temp in the same directory, `fsync`, then `rename` onto the target. `rename` is atomic within one filesystem on POSIX, so readers see either the old file or the new one. + +``` +tmp = create_exclusive(target + "." + unique_suffix) +write(tmp, data); fsync(tmp); close(tmp) +rename(tmp, target) +``` + +## Check Then Delete Or Move + +`if exists then unlink` and `if exists then rename` have the same shape. Call the operation and handle `ENOENT`. + +## What The Finding Must Cite + +Name the path-based check, the open or create that follows, and whether the directory is writable by anyone other than the process owner. A path in a private directory that only this process writes is still a bug if the process can run twice; say which is the case. The principle violated is almost always "the write must carry its condition". + +## Library Files + +- TypeScript: [node-fs](../typescript/node-fs.md) +- Rust: [std-fs](../rust/std-fs.md) +- Elixir: [file](../elixir/file.md) +- Go: [os](../go/os.md) +- Python: [os](../python/os.md) +- C: [posix-fs](../c/posix-fs.md) diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/in-process.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/in-process.md new file mode 100644 index 0000000..6f57812 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/surfaces/in-process.md @@ -0,0 +1,88 @@ +# In-Process Surfaces + +Inside one process the interleaver is another task on the same event loop, another thread, or another process in the same VM. The gap that lets it in is any point where control yields: an `await`, a message round trip, a lock released and re-taken, a second call into the same server. A check and an act with a yield between them are two separate moments. + +## Principles At Work + +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): a synchronous section with no yield is a serializer, and the finding names the function and states there is no yield in it. +- [The write must carry its condition](../principles/write-carries-condition.md): insert-if-absent, `LoadOrStore`, compare-and-swap, the register call as the claim, the single message that carries the decision. +- [A lock only helps when the read happens under it](../principles/lock-moves-the-race.md): lock, release, await, re-lock is two critical sections with a gap. +- [Only the owner of the state can decide](../principles/owner-decides.md): the server process that owns the map decides inside one handler, not the caller across two calls. + +## Check, Yield, Set + +Avoid: + +``` +if not connections.has(host): + conn = await connect(host) + connections.set(host, conn) +``` + +Every caller that arrives during `connect` sees the map empty and opens its own connection; the last one wins and the rest leak. Prefer publishing the claim before the first yield, so later callers find it: + +``` +pending = connections.get(host) +if pending is missing: + pending = start(connect(host)) # the in-flight work, not its result + connections.set(host, pending) # no yield between get and set + on failure: connections.delete(host) +conn = await pending +``` + +The rule for a single-threaded event loop: everything between the check and the write must be synchronous. If it is, the pair is atomic. If there is a yield, it is not. A boolean guard set synchronously (`if busy: return; busy = true`) is safe for the same reason. + +## Lock, Release, Yield, Lock Again + +Avoid: + +``` +with lock: present = map.contains(key) +if not present: + value = await build(key) + with lock: map.insert(key, value) +``` + +The lock is released after the check, so two tasks both miss and both insert. Prefer one of: + +- Hold the lock across the whole decision, using a lock type that may be held across a yield. This serializes all callers through the lock, which is a throughput choice the finding should note. +- Use an entry API that is atomic per key: insert-if-absent, get-or-insert-with, or a cache with built-in single-flight for async construction. +- For a scalar, compare-and-swap in a loop. + +A lock type that must not be held across a yield (a plain OS mutex in an async runtime) has to be dropped before the yield, which puts the race back. Switch the lock type or the data structure instead. + +## Two Calls Into One Server + +Avoid: + +``` +case server.call(get, key): + missing -> server.call(put, key, value) + existing -> existing +``` + +Each call is serialized by the server, but the pair is not. Another caller can `put` between them. Prefer one message that carries the decision, so the server does the check and the act inside a single handler: + +``` +server.call(put_new, key, value) +# handler: if key present reply exists, else insert and reply ok +``` + +The same shape applies to a shared in-memory table that has no server: use its insert-if-absent, atomic counter, or compare-and-swap primitive instead of lookup then insert. + +## Claim Then Act + +"Check that no process owns this key, then register as owner" is lookup-then-insert on a registry. Prefer the registry's own register call as the claim: it succeeds for exactly one caller and tells the others who won. + +## What The Finding Must Cite + +The exact yield point between the check and the act (the `await`, the second call, the lock release), and which other task or process can run at that point. A pair with no yield is atomic only when the serializer is a single thread of execution, such as one event loop or one GenServer, and the finding names that thread. Under OS threads another thread interleaves without any yield, so the pair needs a named lock or an atomic primitive. + +## Library Files + +- TypeScript: [event-loop](../typescript/event-loop.md) +- Rust: [tokio](../rust/tokio.md), [moka-dashmap](../rust/moka-dashmap.md), [atomics](../rust/atomics.md) +- Elixir: [genserver](../elixir/genserver.md), [registry](../elixir/registry.md), [ets](../elixir/ets.md) +- Go: [sync](../go/sync.md), [singleflight](../go/singleflight.md) +- Python: [asyncio](../python/asyncio.md) +- C: [atomics](../c/atomics.md) diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/mysql.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/mysql.md new file mode 100644 index 0000000..cd7a908 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/mysql.md @@ -0,0 +1,85 @@ +# MySQL (InnoDB) + +InnoDB has a plain `SELECT`, which reads a consistent snapshot and takes no locks, and a locking `SELECT`, which reads the latest committed row and holds it, and both can appear in the same transaction. A check made with the plain kind protects nothing. Most TOCTOU findings on MySQL come down to a snapshot read gating a write, and most fixes come down to either a unique key or turning the read into a locking read. + +## Principles At Work + +- [Only the owner of the state can decide](../principles/owner-decides.md): the unique key and the row lock are the only things that see every writer. +- [The write must carry its condition](../principles/write-carries-condition.md): `ON DUPLICATE KEY UPDATE`, `WHERE status = 'pending'`, `WHERE version = ?`. +- [A lock only helps when the read happens under it](../principles/lock-moves-the-race.md): `FOR UPDATE` on the read, not a plain `SELECT` followed by an `UPDATE`. +- [Retry belongs around the conditional write](../principles/retry-wraps-the-write.md): error 1213 (deadlock) rolls the transaction back and the whole transaction is retried, not the statement. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): cite the `UNIQUE KEY` from `SHOW CREATE TABLE`, or the locking read. + +## Snapshot Reads Versus Locking Reads + +The default isolation level is `REPEATABLE READ`. The first consistent read in a transaction establishes the snapshot; every later plain `SELECT` sees that snapshot regardless of what commits. A plain `SELECT` is therefore a read of the past by design. + +`SELECT ... FOR UPDATE` and `SELECT ... FOR SHARE` (`LOCK IN SHARE MODE` before 8.0) read the latest committed version and lock what they read until commit. `UPDATE` and `DELETE` behave like locking reads: they see the latest committed rows that match. This is why `UPDATE ... WHERE balance >= ?` is safe and `SELECT balance` then `UPDATE` is not, exactly as in Postgres. + +Under `SERIALIZABLE`, InnoDB rewrites every plain `SELECT` as `SELECT ... FOR SHARE` when autocommit is off. The read is then held, but it is held as a shared lock, and two transactions that both share-lock a row and both try to update it deadlock. Pick `FOR UPDATE` explicitly instead of relying on the level. + +## Gap Locks Change What A Missing Row Means + +Under `REPEATABLE READ`, a locking read on an index range also locks the gaps between index records (next-key locking). `SELECT ... WHERE email = ? FOR UPDATE` on a missing email locks the gap where that email would be inserted, so a second transaction's `INSERT` blocks. This is the opposite of Postgres, and it is where "FOR UPDATE then INSERT" findings on MySQL need care. + +The gap lock is shared. Two transactions can both take it for the same missing key, then both attempt the `INSERT`. Each insert needs an insert intention lock that the other's gap lock blocks, and InnoDB reports a deadlock (error 1213) to one of them and rolls that transaction back. The outcome is safe only if the caller retries the whole transaction on 1213. The unique key plus `ON DUPLICATE KEY UPDATE` avoids both the gap lock and the deadlock, and is the better primitive for find-or-create. + +Under `READ COMMITTED`, gap locking is disabled except for foreign key and duplicate key checks. A locking read on a missing row then locks nothing, which matches Postgres and makes the unique key mandatory. + +## Constraint-Backed Writes + +```sql +INSERT INTO users (email) VALUES (?) +ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id); +SELECT LAST_INSERT_ID(); +``` + +There is no `RETURNING` in MySQL (MariaDB added one; confirm against the server in use). The `LAST_INSERT_ID(expr)` form makes the function return the existing row's id on the update path, so one round trip gives the identity either way. The affected-rows count is 1 for an insert, 2 for an update, 0 when the update changed nothing. + +`INSERT IGNORE` converts the duplicate key error into a warning, and also converts data truncation, `NOT NULL` violations (it inserts the column default), and invalid enum values into warnings. It is too blunt for a guard; use `ON DUPLICATE KEY UPDATE` with a no-op assignment. + +`REPLACE` is a delete followed by an insert. It fires `DELETE` triggers, cascades foreign keys that reference the row, and allocates a new auto-increment id. Anything holding the old id is now stale. + +## Conditional Updates + +```sql +UPDATE orders SET status = 'paid' WHERE id = ? AND status = 'pending'; +UPDATE accounts SET balance = balance - ? WHERE id = ? AND balance >= ?; +UPDATE documents SET body = ?, version = version + 1 WHERE id = ? AND version = ?; +``` + +`ROW_COUNT()` and the driver's affected-rows value are the verdict. By default MySQL reports rows changed, not rows matched; an `UPDATE` that matches a row and sets it to the values it already has reports 0. Drivers expose a found-rows flag (`CLIENT_FOUND_ROWS`) that switches the count to rows matched. A conditional update whose assignment can be a no-op on the current value, such as re-setting the same status, reads as "lost" without the flag. The finding cites which count the driver returns. + +## Row Locks And Queues + +```sql +START TRANSACTION; +SELECT * FROM jobs WHERE status = 'queued' ORDER BY id LIMIT 1 FOR UPDATE SKIP LOCKED; +UPDATE jobs SET status = 'running', worker = ? WHERE id = ?; +COMMIT; +``` + +`SKIP LOCKED` and `NOWAIT` arrived in 8.0. Before that, the queue pattern is the conditional update alone: `UPDATE jobs SET status = 'running', worker = ? WHERE id = ? AND status = 'queued'`, with the rowcount deciding who owns the job. + +Lock waits time out with error 1205 after `innodb_lock_wait_timeout`, which rolls back only the statement unless `innodb_rollback_on_timeout` is on; the transaction is still open and holds its other locks. Deadlock (1213) rolls back the whole transaction. The retry handler needs to know which happened. + +## Named Locks + +```sql +SELECT GET_LOCK('sessions:42', 5); +SELECT count(*) FROM sessions WHERE user_id = 42 AND active; +INSERT INTO sessions ...; +SELECT RELEASE_LOCK('sessions:42'); +``` + +`GET_LOCK` is session scoped, not transaction scoped. It survives `COMMIT` and `ROLLBACK` and is released only by `RELEASE_LOCK`, `RELEASE_ALL_LOCKS`, or the session ending. Inside a connection pool a lock that is not released on every exit path is held by whichever caller gets the connection next. The name is limited to 64 characters. Since 5.7 a session may hold several named locks at once; earlier versions release the previous one on each `GET_LOCK`. + +## What The Finding Must Cite + +The isolation level, whether the check was a consistent read or a locking read, the unique key or lock the act relies on, how affected rows is counted by the driver, and where 1213 and 1205 are handled. + +## Library Files + +- Go: [database-sql](../go/database-sql.md) +- Python: [sqlalchemy](../python/sqlalchemy.md) +- The other database library files target Postgres. Their conditional update and version column shapes transfer unchanged; their `RETURNING` and `ON CONFLICT` shapes do not. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/nats.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/nats.md new file mode 100644 index 0000000..b779d48 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/nats.md @@ -0,0 +1,83 @@ +# NATS And JetStream + +Core NATS delivers a message at most once and remembers nothing. JetStream persists messages in streams and adds what a check-then-act needs: a sequence number per stream and per subject that a publish can be conditioned on, and a message id that the stream deduplicates for a window. Everything conditional in NATS is a sequence expectation or a message id, and every consumer is at-least-once, so the receiving side has to absorb a second delivery. + +## Principles At Work + +- [A read is a fact about the past](../principles/read-is-the-past.md): a request-reply answer, a KV `get`, and a watcher's last update are all older than the next publish. +- [The write must carry its condition](../principles/write-carries-condition.md): the expected sequence header, the KV revision, the expected stream. +- [Retry belongs around the conditional write](../principles/retry-wraps-the-write.md): a wrong last sequence rejection means reload the subject, re-decide, publish again. +- [Idempotence absorbs what ordering cannot prevent](../principles/idempotence-absorbs.md): `Nats-Msg-Id` for the publish retry, a transactional dedupe or an idempotent effect for the consumer redelivery. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): a work queue stream, a single pull consumer, or a KV key is a serializer only when the config is cited and redelivery is accounted for. + +## Core NATS + +A core subscriber that is slow or absent loses messages; a publish has no acknowledgement. Ordering holds per publisher per subject and nowhere else. A service that answers a request with "yes, slot available" has told the caller about the past, and the caller's next publish is a decision made on that past. Core NATS offers no primitive to close the window. Either the state moves into JetStream, or the responder itself owns the state and makes the decision inside the reply handler as a single process. + +## Conditional Publish + +A JetStream publish is rejected when a header's expectation does not hold: + +- `Nats-Expected-Stream`: the subject must map to this stream. It catches a subject landing on the wrong stream after a configuration change. +- `Nats-Expected-Last-Sequence`: the stream's last sequence must equal the value. This serializes against every publisher on the stream, which is rarely what an aggregate wants. +- `Nats-Expected-Last-Subject-Sequence`: the last sequence on this message's subject must equal the value. With one subject per aggregate this is the expected version. The value `0` means the subject has no messages, which makes it the create guard. +- `Nats-Expected-Last-Msg-Id`: the last stored message's `Nats-Msg-Id` must equal the value. + +The server answers a mismatch with a wrong last sequence error in the publish ack, and nothing is stored. Reading the last sequence of a subject, deciding, and publishing without the header is the TOCTOU; the header moves the comparison into the server, where it is atomic with the append. + +`Nats-Msg-Id` makes the stream drop a second publish with the same id inside the stream's duplicate window (two minutes by default, set per stream) and answer with an ack flagged as a duplicate. It protects a retried publish, not two different decisions on the same aggregate, and it is time bounded: a retry after the window stores the message twice. + +## Key-Value + +A KV bucket is a stream with one subject per key and a per-key history limit. Every entry carries the revision, which is the stream sequence of the message that wrote it: + +- `create(key, value)` publishes with expected last subject sequence `0` and fails when the key holds a live value. Clients retry a create against a key whose last operation is a delete by reusing the delete's revision, so a create after a delete succeeds; confirm the behaviour in the client in use. +- `update(key, value, revision)` publishes with the expected last subject sequence set to the revision. A mismatch is the signal to `get` again and re-decide. +- `put` has no condition and overwrites whatever landed in between. It is correct only when the writer does not care what it replaces. +- `delete` and `purge` accept a last revision in the clients that expose it, which guards against deleting a value someone just replaced. +- A watcher first delivers the current values (unless asked for updates only) and then live changes. Each delivery is a read of the past, and a write decided from a watched value still carries the revision. + +History is bounded by the bucket's `history` setting; `get` returns the latest, and the revision in hand is the only thing that proves the latest is still latest. + +## Streams As Claims + +A stream configured with `max_msgs_per_subject = 1`, `discard = new`, and `discard_new_per_subject = true` rejects a second publish to a subject that already holds a message. Publishing to `claims.` is then a claim that exactly one publisher wins, with no header to carry. The discard-per-subject option arrived in nats-server 2.9.1 and is rejected unless `discard = new` and `max_msgs_per_subject` is set. + +The object store has no conditional put. Two writers to the same object name are last-writer-wins, and a KV entry beside the object is the claim. + +## Consumers Are At-Least-Once + +A message is redelivered when `ack_wait` passes without an ack, when the consumer `nak`s it, and when the ack itself is lost on the way to the server. Every effect a consumer makes can therefore run twice, on this instance or another one, with all the guards inside the handler intact. A check against a local `processed` set does not survive the second instance. The consumer either records the message id in the same transaction as the effect, with a unique constraint on the id, or makes the effect idempotent on a deterministic key, or accepts the duplicate and says so. + +What the consumer configuration contributes: + +- `ack_policy`: `explicit` acks one message, `all` acks every message up to this one, `none` never redelivers and is at-most-once. +- `ack_wait`: must exceed the slowest handler, or the server redelivers to another instance while the first is still working. `in_progress` extends the wait for one message while the handler keeps going. +- `max_ack_pending`: bounds how many messages are in flight at once, which is how many duplicates a crash can produce. +- `nak` with a delay schedules the redelivery instead of letting it happen immediately; `term` stops redelivery of a message that can never succeed. +- `max_deliver`: caps redelivery; after it the message is parked, and an advisory is published. A handler whose effect is not idempotent has run up to this many times. +- A synchronous ack (an ack that waits for the server's confirmation) closes the lost-ack case; a fire-and-forget ack does not. + +`NumDelivered` in the message metadata is greater than one on a redelivery, which is useful for logging and never a substitute for the dedupe. + +## Work Queues And Ordered Consumers + +A stream with `retention = workqueue` deletes a message once it is acked and allows one consumer per overlapping filter subject. That is one delivery per message among competing workers, not exactly-once: a worker that times out hands the message to the next worker, and both effects can land. + +An ordered consumer is ephemeral, acks nothing, and recreates itself from the last known sequence when it detects a gap. It is for reading a stream in order, and a decision made from an ordered consumer's position is a read whose publish still needs the expected sequence header. + +A pull consumer's `fetch` hands the caller a batch that is ack-pending from the moment it is delivered. A batch that is processed slowly runs out of `ack_wait` on its tail while the head is still being handled, and the tail is redelivered elsewhere. Pull in batches no larger than the handler can finish inside one `ack_wait`. + +## What The Finding Must Cite + +For a publish: the subject, the sequence or revision the decision was made from, and the header that carries it. For a consumer: the ack policy, `ack_wait`, `max_deliver`, whether the ack is synchronous, and whether the dedupe record commits with the effect. + +## Client Spellings + +Every client sets the same headers and sends the same revision-guarded KV requests, so the finding cites the header and the revision or sequence it carries, not the client. No client can make the consumer's effect transactional: record the message id and the effect in one database transaction through the database library in use, then ack, and confirm `ack_wait` exceeds worst-case processing time and `max_deliver` is set. + +- nats.js: `js.publish(subject, data, { msgID, expect: { lastSubjectSequence, lastSequence, lastMsgID, streamName } })`. KV: `kv.create(key, data)`, `kv.update(key, data, revision)`, which throws when the revision is 0 or less, so never pass a default; `kv.delete(key, { previousSeq })` and `kv.purge(key, { previousSeq })` guard the removal. +- async-nats: `async_nats::jetstream::message::PublishMessage::build().payload(bytes).message_id(id).expected_last_subject_sequence(seq)`, with `.expected_last_sequence(seq)`, `.expected_last_message_id(id)`, and `.expected_stream(name)` for the other headers. Send with `js.send_publish(subject, publish).await?.await?`; the first `await` sends and the second awaits the ack, which is where the mismatch surfaces. `jetstream::context::Publish` is the deprecated name of the same builder. KV: `kv.entry(key)` returns the revision, then `kv.create(key, bytes)`, `kv.update(key, bytes, revision)`, `kv.delete_expect_revision(key, Some(rev))`, and `kv.purge_expect_revision(key, Some(rev))`. +- nats.go, module `github.com/nats-io/nats.go` with the `jetstream` subpackage (there is no v2 module): `js.Publish(ctx, subject, data, jetstream.WithMsgID(id), jetstream.WithExpectLastSequencePerSubject(seq))`, with `WithExpectLastSequence`, `WithExpectLastMsgID`, and `WithExpectStream` for the other headers. KV: `kv.Create(ctx, key, value)`, `kv.Update(ctx, key, value, revision)`, `kv.Delete(ctx, key, jetstream.LastRevision(rev))`, `kv.Purge(ctx, key, jetstream.LastRevision(rev))`. `Create` on a key whose last entry is a delete or purge retries internally as an `Update` against that revision. +- nats-py: `js.publish(subject, payload, headers={...}, stream=...)` takes the raw headers and has no typed expectation options, so spell `Nats-Msg-Id` and `Nats-Expected-Last-Subject-Sequence` (as a string) in the dict. KV: `kv.create(key, value)` and `kv.update(key, value, last=revision)`; `last` must always be passed, because the default sends expected sequence 0, which is create-if-absent and fails on an existing key instead of guarding the update. `kv.purge(key)` takes no revision guard. +- Elixir: the clients differ in how they expose headers and KV, so cite the exact call from the library in use. Gnat accepts headers on publish through the `headers:` option of `Gnat.pub/4` (confirm against the version in use); a KV update with no revision-guarded call in the client is the underlying JetStream publish with `Nats-Expected-Last-Subject-Sequence` set to the entry's revision. The transactional dedupe is in [ecto](../elixir/ecto.md). diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/postgres.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/postgres.md new file mode 100644 index 0000000..e1fcef6 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/postgres.md @@ -0,0 +1,108 @@ +# PostgreSQL + +Postgres decides every race with a unique or exclusion constraint, a row lock, an advisory lock, or a serialization failure. Application code never sees the other writer; the server does. A finding on Postgres names which of these the write relies on, and the isolation level the transaction actually runs under, because the same SQL means different things under each. + +## Principles At Work + +- [Only the owner of the state can decide](../principles/owner-decides.md): the constraint and the lock live in the catalog, not in the ORM. +- [The write must carry its condition](../principles/write-carries-condition.md): `ON CONFLICT`, `WHERE version = $n`, `WHERE status = 'pending'`. +- [A lock only helps when the read happens under it](../principles/lock-moves-the-race.md): `FOR UPDATE` before the read, in every writer, inside the transaction. +- [Retry belongs around the conditional write](../principles/retry-wraps-the-write.md): SQLSTATE `40001` and the zero-rowcount branch. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): cite the index from the migration, or the lock call. + +## What Each Isolation Level Does To Check-Then-Act + +`READ COMMITTED` is the default. Every statement takes a fresh snapshot, so a `SELECT` followed by an `UPDATE` in the same transaction can see two different worlds. The saving property: when an `UPDATE` or `DELETE` reaches a row that another committed transaction changed after the snapshot, it waits, re-reads the new row version, and re-evaluates its `WHERE`. That is why `UPDATE ... WHERE balance >= $1` is safe here while `SELECT balance` then `UPDATE` is not. + +`REPEATABLE READ` takes one snapshot at the first statement. Reads never change inside the transaction. An `UPDATE` that reaches a row changed by a concurrent committed transaction does not re-evaluate; it fails with `40001` ("could not serialize access due to concurrent update"). First updater wins, the loser must retry the whole transaction. Two transactions that both `SELECT` nothing and both `INSERT` do not conflict here; only a unique index stops the second. + +`SERIALIZABLE` adds predicate tracking on top. Two transactions whose reads and writes would not be equivalent to some serial order abort one of them with `40001` ("could not serialize access due to read/write dependencies"). It catches the `SELECT count(*)` then `INSERT` invariant that no constraint can express. It is correct only when every caller retries on `40001`; without the loop it has turned a silent race into a user-facing error. + +The retry contract is narrower than one code. `40001` is always a whole-transaction retry, including the logic that chose the SQL and the values. `40P01` (deadlock) is a retry in nearly every case. `23505` (unique violation) and `23P01` (exclusion violation) are retries only when the application can prove the conflict was a race on a value it chose from a prior read, such as picking the next free key; the same codes also mean a persistent error, and a blind retry loops forever on it. + +## Constraint-Backed Writes + +```sql +INSERT INTO users (email) VALUES ($1) +ON CONFLICT (email) DO UPDATE SET email = EXCLUDED.email +RETURNING id; +``` + +`ON CONFLICT` needs an arbiter: a unique index, a unique constraint, or an exclusion constraint matching the conflict target. Without one the statement is rejected at parse time, which is a useful property; the arbiter cannot be forgotten. `RETURNING` fires only for rows the statement inserted or updated, so `DO NOTHING` returns no row on conflict and the identity needs a follow-up `SELECT`, which is safe because the row now exists. + +`INSERT ... SELECT ... WHERE NOT EXISTS (...)` is the same check-then-act in one statement. Both transactions evaluate `NOT EXISTS` against their own snapshot, both insert. The unique index is still the primitive; the subquery is a hint for the reader, not a guard. + +"At most one active per user" is a partial unique index, and the conflict target has to repeat its predicate: + +```sql +CREATE UNIQUE INDEX one_active_sub ON subscriptions (user_id) WHERE status = 'active'; + +INSERT INTO subscriptions (user_id, status) VALUES ($1, 'active') +ON CONFLICT (user_id) WHERE status = 'active' DO NOTHING; +``` + +"No two bookings overlap" is an exclusion constraint, which the planner enforces like a unique index: + +```sql +ALTER TABLE bookings ADD CONSTRAINT no_overlap + EXCLUDE USING gist (room_id WITH =, during WITH &&); +``` + +`=` on a scalar inside a GiST exclusion needs the `btree_gist` extension. + +A `DEFERRABLE INITIALLY DEFERRED` unique constraint moves the violation to `COMMIT`, and `ON CONFLICT` refuses to use a deferrable constraint as an arbiter. The write still loses, but the error arrives from `COMMIT`, after the application thinks the insert succeeded. A finding on a deferrable constraint cites where the commit error is handled. + +## Conditional Updates + +```sql +UPDATE orders SET status = 'paid' WHERE id = $1 AND status = 'pending'; +UPDATE accounts SET balance = balance - $1 WHERE id = $2 AND balance >= $1; +UPDATE documents SET body = $1, version = version + 1 WHERE id = $2 AND version = $3; +``` + +The command tag's rowcount is the verdict. Zero rows under `READ COMMITTED` means the re-evaluated `WHERE` was false on the current row version; the other writer has already won. The system column `xmin` can stand in for a version column: it changes on every update to the row, compare it for equality only, never for ordering, and most ORMs do not expose it, so an explicit `version` column is the portable choice. + +A `CHECK (balance >= 0)` is the backstop for any writer that omits the `WHERE`. It turns a wrong balance into `23514` rather than a silent negative. + +## Row Locks + +```sql +BEGIN; +SELECT * FROM subscriptions WHERE id = $1 FOR UPDATE; +UPDATE subscriptions SET ... WHERE id = $1; +COMMIT; +``` + +`FOR UPDATE` blocks other `FOR UPDATE`, `FOR NO KEY UPDATE`, `FOR SHARE`, `UPDATE`, and `DELETE` on the row until commit. `FOR NO KEY UPDATE` is the right strength when the row is referenced by foreign keys from other tables and the update does not touch the key, because plain `FOR UPDATE` also blocks the `FOR KEY SHARE` that foreign key checks take, which blocks inserts into child tables that reference the row until the lock is released. `SKIP LOCKED` makes a queue table hand each row to one worker; `NOWAIT` turns waiting into an error. + +Postgres has no gap or predicate locks outside `SERIALIZABLE`. `SELECT ... WHERE email = $1 FOR UPDATE` on a missing row locks nothing, and two inserters proceed. Locking does not solve find-or-create; the unique index does. + +## Advisory Locks + +```sql +SELECT pg_advisory_xact_lock(hashtext('sessions:' || $1)); +SELECT count(*) FROM sessions WHERE user_id = $1 AND active; +INSERT INTO sessions ...; +``` + +`pg_advisory_xact_lock` holds until commit or rollback and takes a `bigint` or a pair of `int`. The session-scoped `pg_advisory_lock` outlives the transaction and must be released explicitly, which makes it the wrong tool inside a connection pool. Hash collisions make the lock coarser, never unsafe. Every writer of the invariant has to take the same lock, or the lock protects nothing. + +## What Does Not Serialize + +A trigger runs inside the triggering statement, under its snapshot. A `BEFORE INSERT` trigger that counts rows and raises when over quota reads a snapshot that another transaction's uncommitted insert is invisible to. The trigger has to take the advisory lock or rely on a constraint. + +Data-modifying CTEs in one statement all run against the same snapshot and cannot see each other's rows. `WITH ins AS (INSERT ... ON CONFLICT DO NOTHING RETURNING id) SELECT id FROM ins UNION ALL SELECT id FROM users WHERE email = $1` works only because the arbiter is a unique index; the `SELECT` branch is not a check that protects the insert. + +A transaction on its own serializes nothing. It is the frame inside which the constraint, the lock, or the `40001` can act. + +## What The Finding Must Cite + +The isolation level the connection actually uses, the SQL of the check and the act, the arbiter or lock the act relies on, and where `40001` and `40P01` are retried, whether `23505` and `23P01` are retried and on what evidence, and where the zero-rowcount branch is handled. + +## Library Files + +- TypeScript: [node-postgres](../typescript/node-postgres.md), [prisma](../typescript/prisma.md), [drizzle](../typescript/drizzle.md) +- Rust: [sqlx](../rust/sqlx.md) +- Elixir: [ecto](../elixir/ecto.md) +- Go: [database-sql](../go/database-sql.md) +- Python: [psycopg](../python/psycopg.md), [sqlalchemy](../python/sqlalchemy.md) diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/redis.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/redis.md new file mode 100644 index 0000000..b91f4f4 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/redis.md @@ -0,0 +1,91 @@ +# Redis + +Redis executes commands one at a time on a single thread, so every single command is atomic, and every sequence of commands from one client is not. A check-then-act on Redis is any place where the client issues a read, decides, and issues a write as a second round trip. The fix is always to move the decision into one command, one script, or one `WATCH`-guarded transaction. + +## Principles At Work + +- [Only the owner of the state can decide](../principles/owner-decides.md): the server evaluates `NX`, `INCR`, and the script; the client only observes. +- [The write must carry its condition](../principles/write-carries-condition.md): `SET NX`, `HSETNX`, `WATCH` plus `EXEC`, a script that compares before it deletes. +- [Retry belongs around the conditional write](../principles/retry-wraps-the-write.md): a `nil` from `SET NX` or from `EXEC` is the signal, and the loop is bounded. +- [Idempotence absorbs what ordering cannot prevent](../principles/idempotence-absorbs.md): a lease can expire while its holder still runs; only a fencing token the resource checks turns the lease into a lock. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): one command, one script, or one `MULTI` block, named by the reviewer. + +## Single Commands That Carry Their Condition + +``` +SET lock:order:42 NX PX 30000 -- nil when already held +SET cache:user:7 NX EX 300 -- fill only if absent +INCR rate:ip:10.0.0.1 -- returns the new value; the check is the compare on the return +HSETNX sessions -- field-level create +GETDEL job:claim:9 -- read and remove in one step +``` + +`SET` with `NX` and an expiry in one command replaced the `SETNX` then `EXPIRE` pair. The pair is a TOCTOU in the other direction: a crash between the two leaves a key with no expiry, and the lease never releases. `SET ... GET` returns the previous value in the same step. That is an atomic swap, not a compare-and-swap: nothing is compared, and the write lands whatever the old value was. Compare-and-swap on a string is `SET key value IFEQ expected` (8.4 and later; `IFNE`, `IFDEQ`, and `IFDNE` arrived with it, confirm against the version in use), or on older servers a `WATCH` sequence or a script. Combining `GET` with `NX` is allowed from 7.0. + +`INCR` is the right counter and the right rate limiter. `GET` then `SET value+1` loses increments under any load. + +## WATCH, MULTI, EXEC + +`MULTI` queues commands and `EXEC` runs them back to back with nothing interleaved. On its own that is atomicity, not a condition: the queued commands cannot look at a value and branch on it. `WATCH` adds the condition: + +``` +WATCH account:42 +GET account:42 -- decide on the client +MULTI +SET account:42 +EXEC -- nil if account:42 changed since WATCH +``` + +`WATCH` is optimistic. It blocks nobody; it makes `EXEC` return `nil` when a watched key was written by anyone after the `WATCH`. The caller retries from `WATCH`. A `WATCH` on one connection does not guard an `EXEC` on another, so pooled clients have to pin the connection for the whole sequence, and a pool that hands back the connection between `WATCH` and `EXEC` has silently removed the guard. `MULTI` does not roll back: a command that fails inside `EXEC` fails alone and the others still apply. + +## Scripts + +A Lua script via `EVAL` (or a loaded function via `FCALL` from 7.0) runs with no other command interleaved. It is the general read-modify-write: + +```lua +-- release only if this caller still holds the lease +if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) +end +return 0 +``` + +The unguarded `DEL` is the classic finding: the lease expired, another holder acquired it, and the first holder deletes the second holder's lease. The compare and the delete have to be in the script. The same shape extends a lease (`GET` compare then `PEXPIRE`) and implements a token bucket (`GET`, arithmetic, `SET`). Scripts block the server for their whole run, so a script that loops over many keys is a latency problem, not a safety one. + +## Leases Are Not Locks + +A `SET NX PX` lease expires on a timer. The holder can be paused by garbage collection, a network partition, or a slow disk past the expiry and keep acting while another holder owns the lease. Redis cannot know. The protected resource has to reject stale holders: hand out a monotonic fencing token (`INCR lease:token` taken with the lease), pass it with every write to the resource, and have the resource refuse tokens lower than the last one it accepted. When the resource cannot check a token, the finding says the lease reduces duplicates rather than prevents them. + +Redlock across several independent instances addresses losing the lease on failover, not the paused-holder case; the fencing token is still required. + +## Replication And Failover + +Replication is asynchronous. A `SET NX` acknowledged by the primary can be lost when the primary fails before the replica receives it, and the replica that is promoted hands the same lease to someone else. `WAIT ` blocks until the replicas acknowledge and narrows the window; it does not close it, because the primary can still fail after the acknowledgement is sent. A finding that calls a Redis lease the serializer says whether failover is in scope. + +## Cluster + +Multi-key commands, `MULTI` blocks, and scripts require every key in the same hash slot, otherwise the server answers `CROSSSLOT`. Keys that must change together carry a hash tag so they map to one slot: + +``` +SET {order:42}:lock NX PX 30000 +HSET {order:42}:state status paid +``` + +A script that reads `{order:42}:lock` and writes `{order:42}:state` is atomic. Spread across slots, the same logic is two round trips with a window between them. + +## Reads That Are Not Snapshots + +`KEYS` blocks the server, and the list it returns is stale the moment it is returned; acting on each key afterwards is check-then-act per key. `SCAN` never blocks, but it is a cursor over a changing keyspace: it can return a key twice and can miss a key added during the scan. Neither is a basis for a decision about the set as a whole. Pipelining sends commands without waiting for replies and saves round trips; it does not stop other clients' commands from landing between them. Keyspace notifications are fire-and-forget pub/sub, and a subscriber that missed one has no way to know. + +## What The Finding Must Cite + +The commands issued between the read and the write, whether they are one command, one script, or one `WATCH` sequence on one connection, the expiry on any lease, whether the resource checks a fencing token, and whether failover is accepted as a residual. + +## Client Spellings + +Every client passes the command through unchanged, and the server evaluates `NX`, the `WATCH` check, and the script atomically, so the finding cites the command and the connection it ran on, not the client. The compare-and-delete release script above is the same text in every client. + +- ioredis: `redis.set(key, token, "PX", 5000, "NX")` returns `"OK"` for the winner and `null` for everyone else. Scripts go through `redis.defineCommand(name, { numberOfKeys, lua })`. For a `WATCH`, `MULTI`, `EXEC` sequence use a connection the sequence owns, for example one from `redis.duplicate()`; the ioredis docs do not document `WATCH`, so cite the connection the code actually uses. +- Redix: `Redix.command(conn, ["SET", key, token, "NX", "PX", "5000"])` returns `{:ok, "OK"}` for the winner and `{:ok, nil}` otherwise; pass every argument as a string. Scripts are `["EVAL", script, "1", key, token]`. The Redix docs say to use an isolated connection for a manual `MULTI` and `EXEC`, and `Redix.transaction_pipeline/3` cannot hold a `WATCH` across a read, so a compare-and-set needs a manual sequence on one dedicated connection. +- go-redis: `rdb.SetNX(ctx, key, token, 5*time.Second).Result()` returns `true` only for the winner. `rdb.SetArgs(ctx, key, value, redis.SetArgs{Mode: "NX", TTL: ttl})` returns a `*StatusCmd`; what a lost race returns from it is not confirmed in the docs, so prefer `SetNX` when the reply is the decision. Scripts are `redis.NewScript(src).Run(ctx, rdb, []string{key}, token)`. Compare-and-set is `rdb.Watch(ctx, fn, keys...)` with `tx.TxPipelined` inside `fn`, retried on `errors.Is(err, redis.TxFailedErr)`. +- redis-py: `r.set(key, token, nx=True, px=5000)` returns `True` for the winner and `None` otherwise, not `False`. Scripts are `r.register_script(src)`, called as `script(keys=[...], args=[...])`. Compare-and-set is `pipe = r.pipeline()`, `pipe.watch(key)`, the read with `pipe.get(key)` immediately after, `pipe.multi()`, the writes, then `pipe.execute()`, retried on `redis.WatchError`; `r.transaction(func, *keys)` wraps that loop. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/sqlite.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/sqlite.md new file mode 100644 index 0000000..65d2439 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/systems/sqlite.md @@ -0,0 +1,74 @@ +# SQLite + +SQLite allows one writer at a time per database file, and the lock that enforces it is the serializer for everything. The TOCTOU specific to SQLite is a transaction that reads first and writes second: the read ran without the write lock, and by the time the write asks for it the snapshot can be stale, which SQLite reports as `SQLITE_BUSY` rather than silently overwriting. + +## Principles At Work + +- [A lock only helps when the read happens under it](../principles/lock-moves-the-race.md): `BEGIN IMMEDIATE` takes the write lock before the first read. +- [The write must carry its condition](../principles/write-carries-condition.md): `INSERT ... ON CONFLICT`, `UPDATE ... WHERE`, `changes()`. +- [Retry belongs around the conditional write](../principles/retry-wraps-the-write.md): `SQLITE_BUSY` on a deferred transaction means start over, not wait. +- [Serialization is proven by naming the serializer](../principles/serializer-is-named.md): the file lock on the database, which only holds when every writer is a connection to the same file. + +## Deferred Versus Immediate + +A plain `BEGIN` (deferred) takes no lock. The first `SELECT` takes a read lock and establishes the snapshot. The first `INSERT`, `UPDATE`, or `DELETE` tries to upgrade to the write lock. If another connection has written since this connection's snapshot, the upgrade cannot proceed: the reads already made would be from before that write. In WAL mode SQLite returns `SQLITE_BUSY_SNAPSHOT` immediately, without invoking the busy handler, because waiting cannot make a stale snapshot fresh. In rollback-journal mode the result is `SQLITE_BUSY`, and in the case where waiting would deadlock the busy handler is also skipped. + +```sql +BEGIN; +SELECT balance FROM accounts WHERE id = 1; +UPDATE accounts SET balance = balance - 10 WHERE id = 1; -- may fail with SQLITE_BUSY after the read +COMMIT; +``` + +`BEGIN IMMEDIATE` takes the write lock up front. Every read inside it is a read under the lock, there is no upgrade step, and `busy_timeout` applies to the `BEGIN` itself, where waiting does help: + +```sql +PRAGMA busy_timeout = 5000; +BEGIN IMMEDIATE; +SELECT balance FROM accounts WHERE id = 1; +UPDATE accounts SET balance = balance - 10 WHERE id = 1; +COMMIT; +``` + +Any transaction that reads and then writes is `BEGIN IMMEDIATE`. A deferred transaction is for reads only. The retry on `SQLITE_BUSY` from a deferred transaction has to restart from `BEGIN`, because the decision was made on the stale snapshot. + +## WAL Mode + +`PRAGMA journal_mode = WAL` lets readers proceed while a writer is active and lets the writer proceed while readers are active. It does not allow a second writer. Readers see the snapshot as of their first read; a long-lived read transaction holds the WAL from checkpointing but never blocks a write. The one-writer rule and the `BEGIN IMMEDIATE` advice are unchanged. + +## Constraint-Backed Writes + +```sql +INSERT INTO users (email) VALUES (?) +ON CONFLICT (email) DO UPDATE SET email = excluded.email +RETURNING id; +``` + +`ON CONFLICT ... DO UPDATE` arrived in 3.24 and `RETURNING` in 3.35; confirm against the library version in use, since embedded SQLite is often older than the system one. `INSERT OR IGNORE` is the older spelling of `DO NOTHING` and applies to every `UNIQUE`, `NOT NULL`, `CHECK`, and `PRIMARY KEY` violation on the row, not only the key you meant. `INSERT OR REPLACE` deletes the conflicting row first, and its delete triggers fire only when `recursive_triggers` is on. + +Because only one writer exists, the unique constraint never has two inserters racing inside the engine. It still matters: it is what makes the second `INSERT` fail rather than duplicate when two transactions run one after the other and both decided from the same stale read. + +## Conditional Updates + +```sql +UPDATE orders SET status = 'paid' WHERE id = ? AND status = 'pending'; +SELECT changes(); +``` + +`changes()` and the driver's affected-rows count report rows modified by the last statement. Inside `BEGIN IMMEDIATE` the conditional `WHERE` is evaluated under the write lock, so zero rows means a committed earlier transaction already moved the row, never a concurrent one. + +## Where The Serializer Lives + +The write lock is a lock on the database file, taken through the operating system's file locking (or the WAL shared-memory file). It serializes every connection in every process on the same machine. It does not serialize across a network filesystem reliably; SQLite's own documentation warns that NFS and SMB locking is unreliable, and a database on a shared mount has no serializer a reviewer can cite. + +`:memory:` is a database per connection. A pool of connections opened on `:memory:` is a pool of unrelated empty databases, and tests on it never exercise the write lock. `file::memory:?cache=shared` makes connections in one process share the database, under a different locking scheme that reports `SQLITE_LOCKED` instead of `SQLITE_BUSY` and does not invoke the busy handler. + +Serialized threading mode (`SQLITE_THREADSAFE=1`) is a mutex around the connection handle. It makes concurrent calls on one connection not crash; it does not make two statements from two threads a transaction. + +## What The Finding Must Cite + +Whether the transaction began deferred or immediate, the journal mode, the `busy_timeout`, whether the `SQLITE_BUSY` handler restarts the transaction or only the statement, and for tests whether the database under test is a file or `:memory:`. + +## Library Files + +The database library files in this skill target Postgres. The conditional update, version column, and `ON CONFLICT` shapes transfer to SQLite; the `BEGIN IMMEDIATE` discipline is the SQLite-specific addition, and each library spells it differently (a connection option, an explicit `execute("BEGIN IMMEDIATE")`, or a transaction mode argument), so confirm against the driver in use. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/drizzle.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/drizzle.md new file mode 100644 index 0000000..417d0f7 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/drizzle.md @@ -0,0 +1,53 @@ +# Drizzle + +Spells the database primitives from [database.md](../surfaces/database.md) with Drizzle ORM on Postgres. The unique index must be declared in the table schema for `onConflict` to have a target. + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how Drizzle spells it. + +## Find Or Create + +```ts +const [user] = await db + .insert(users) + .values({ email }) + .onConflictDoUpdate({ target: users.email, set: { email } }) + .returning({ id: users.id }); +``` + +`onConflictDoNothing()` returns no row on conflict, so a follow-up `select` is needed. + +## Conditional Transition With Returning + +Drizzle does not expose a rowcount uniformly across drivers, so use `returning` and branch on the array length. + +```ts +const moved = await db + .update(orders) + .set({ status: "paid" }) + .where(and(eq(orders.id, id), eq(orders.status, "pending"))) + .returning({ id: orders.id }); +if (moved.length === 0) throw new AlreadyMoved(); +``` + +## Counter And Balance + +```ts +const debited = await db + .update(accounts) + .set({ balance: sql`${accounts.balance} - ${amount}` }) + .where(and(eq(accounts.id, id), gte(accounts.balance, amount))) + .returning({ id: accounts.id }); +if (debited.length === 0) throw new InsufficientFunds(); +``` + +## Row Lock + +```ts +await db.transaction(async (tx) => { + const [row] = await tx.select().from(subscriptions).where(eq(subscriptions.id, id)).for("update"); + // decide + await tx.update(subscriptions).set({ ... }).where(eq(subscriptions.id, id)); +}); +``` + +Every statement of the decision runs on `tx`; a `db.` call inside the callback uses a different connection and is outside the lock. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/event-loop.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/event-loop.md new file mode 100644 index 0000000..5087526 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/event-loop.md @@ -0,0 +1,61 @@ +# Node Event Loop + +Spells the in-process primitives from [in-process.md](../surfaces/in-process.md) and the single-flight from [cache.md](../surfaces/cache.md). The event loop is single threaded, so a pair with no `await` between check and act is atomic, and that fact is the serializer the finding names. + +## Store The Promise, Not The Result + +Publish the in-flight work before the first `await`, so later callers join it. + +```ts +const connections = new Map>(); + +function getConnection(host: string): Promise { + let pending = connections.get(host); + if (!pending) { + pending = connect(host).catch((err) => { + connections.delete(host); + throw err; + }); + connections.set(host, pending); + } + return pending; +} +``` + +`if (!connections.has(host)) { const c = await connect(host); connections.set(host, c); }` is the race. + +## Single-Flight Over A Cache + +```ts +const inflight = new Map>(); + +async function getOrCompute(key: string): Promise { + const cached = await cache.get(key); + if (cached !== undefined) return cached; + + let p = inflight.get(key); + if (!p) { + p = computeExpensive(key) + .then(async (v) => { await cache.set(key, v, ttl); return v; }) + .finally(() => inflight.delete(key)); + inflight.set(key, p); + } + return p; +} +``` + +The `inflight.get` and `inflight.set` have no `await` between them. The `cache.get` before them does yield, which is fine: it only decides whether to enter the synchronous section. + +## Guard Flag + +Safe only when nothing yields between reading and setting it. + +```ts +if (this.flushing) return; +this.flushing = true; +try { await this.flush(); } finally { this.flushing = false; } +``` + +## Worker Threads + +Workers share nothing except `SharedArrayBuffer`. For a counter there, `Atomics.compareExchange` in a loop is the compare-and-swap; a plain read then `Atomics.store` is the race. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/node-fs.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/node-fs.md new file mode 100644 index 0000000..d06fbd2 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/node-fs.md @@ -0,0 +1,49 @@ +# Node `fs` + +Spells the filesystem primitives from [filesystem.md](../surfaces/filesystem.md). Read the surface file first for the smell and the interleaver. + +## Exclusive Create + +The `wx` flag is `O_CREAT | O_EXCL`; `EEXIST` is the answer, not an error to swallow silently. + +```ts +import { promises as fs } from "node:fs"; + +try { + await fs.writeFile(path, data, { flag: "wx" }); +} catch (err) { + if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err; + // someone else created it +} +``` + +`fs.mkdir(path)` without `recursive` fails with `EEXIST` the same way; `fs.mkdir(path, { recursive: true })` is the idempotent form. + +## Atomic Replace + +Create a sibling temp name exclusively, write, `sync`, then `rename`. The temp file must sit in the target's directory so the rename stays on one filesystem. + +```ts +import { randomUUID } from "node:crypto"; + +const tmp = `${target}.${process.pid}.${randomUUID()}`; +const handle = await fs.open(tmp, "wx"); +await handle.writeFile(data); +await handle.sync(); +await handle.close(); +await fs.rename(tmp, target); +``` + +## Open Then Verify + +Node exposes `O_NOFOLLOW` through `fs.constants`. Open with it, then `fstat` the handle instead of `stat` on the path. + +```ts +const handle = await fs.open(path, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW); +const st = await handle.stat(); +if (!st.isFile()) { await handle.close(); throw new Forbidden(); } +``` + +## Idempotent Delete And Private Temp Dirs + +`fs.rm(path, { force: true })` swallows `ENOENT`. `fs.mkdtemp(prefix)` creates a private directory atomically; create files inside it rather than guessing free names in a shared directory. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/node-postgres.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/node-postgres.md new file mode 100644 index 0000000..ca6eaa8 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/node-postgres.md @@ -0,0 +1,60 @@ +# node-postgres + +Spells the database primitives from [database.md](../surfaces/database.md) with the `pg` client. The SQL carries the condition; the client only exposes the result. + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how node-postgres spells it. + +## Upsert With Identity On Both Paths + +```ts +const { rows } = await client.query( + `INSERT INTO users (email) VALUES ($1) + ON CONFLICT (email) DO UPDATE SET email = EXCLUDED.email + RETURNING id`, + [email], +); +const userId = rows[0].id; +``` + +## Conditional Update And Rowcount + +`rowCount` is the verdict. Zero means another writer won; the code must branch on it. + +```ts +const res = await client.query( + `UPDATE accounts SET balance = balance - $1 WHERE id = $2 AND balance >= $1`, + [amount, id], +); +if (res.rowCount === 0) throw new InsufficientFunds(); +``` + +The same shape covers status transitions (`WHERE status = 'pending'`), optimistic versions (`WHERE version = $3`), and authorization folded into the write (`WHERE owner_id = $3`). + +## Row Lock + +Take the lock on a dedicated client inside a transaction, not on the pool, so the lock and the later write share a connection. + +```ts +const client = await pool.connect(); +try { + await client.query("BEGIN"); + const { rows } = await client.query( + "SELECT * FROM subscriptions WHERE id = $1 FOR UPDATE", + [id], + ); + // decide + await client.query("UPDATE subscriptions SET ... WHERE id = $1", [id]); + await client.query("COMMIT"); +} catch (err) { + await client.query("ROLLBACK"); + throw err; +} finally { + client.release(); +} +``` + +Running `BEGIN` and `FOR UPDATE` through `pool.query` sends each statement on whichever connection is free, so the lock is taken on one connection and the write runs on another. + +## Serializable Retry + +A transaction at `SERIALIZABLE` surfaces the conflict as an error whose `code` is `40001`. Wrap the whole transaction in a bounded loop that retries on `40001` and `40P01`. Retry `23505` only when the code can show the duplicate came from a value it chose after a read; otherwise it is a persistent error. diff --git a/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/prisma.md b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/prisma.md new file mode 100644 index 0000000..d24bed6 --- /dev/null +++ b/plugins/trogonstack-concurrency/skills/concurrency-find-toctou/references/typescript/prisma.md @@ -0,0 +1,53 @@ +# Prisma + +Spells the database primitives from [database.md](../surfaces/database.md) with Prisma Client. The constraint still has to exist in the schema as `@unique` or `@@unique`; Prisma does not add one for `upsert`. + +Read [references/systems/postgres.md](../systems/postgres.md) for what the store guarantees; this file covers how Prisma spells it. + +## Find Or Create + +`upsert` against a unique field is the find-or-create. It runs as one `INSERT ... ON CONFLICT` only when the `update` clause is not empty, `where` names one unique field, `create` carries the same value, and neither branch nests a query (Prisma 4.6 and later on PostgreSQL, CockroachDB, and SQLite). With `update: {}` Prisma runs the find and the create as separate statements, and the race surfaces as `P2002`. + +```ts +const user = await prisma.user.upsert({ + where: { email }, + update: { email }, + create: { email }, +}); +``` + +`findUnique` then `create` is the race, and so is `upsert` with an empty `update`. + +## Conditional Transition + +`update` takes a unique field plus any other column in `where` (Prisma 4.5 and later) and throws `P2025` when no row matches. `updateMany` takes the same filter and returns `count`, which is the cleaner signal. + +```ts +const { count } = await prisma.order.updateMany({ + where: { id, status: "PENDING" }, + data: { status: "PAID", paidAt: new Date() }, +}); +if (count === 0) throw new AlreadyMoved(); +``` + +`prisma.order.update({ where: { id } })` after a `findUnique` that checked the status is the race. The status belongs in the `where` of the write, whichever call makes it. + +## Counter And Balance + +Use the atomic number operations with the condition in `where`: + +```ts +const { count } = await prisma.account.updateMany({ + where: { id, balance: { gte: amount } }, + data: { balance: { decrement: amount } }, +}); +if (count === 0) throw new InsufficientFunds(); +``` + +## Row Lock And Advisory Lock + +Prisma Client documents no query-builder spelling for `FOR UPDATE` or `pg_advisory_xact_lock`. Use `$queryRaw` inside an interactive transaction (`prisma.$transaction(async (tx) => { ... })`) and run every statement of the decision on `tx`, not on `prisma`. + +## Serializable Retry + +`prisma.$transaction(fn, { isolationLevel: Prisma.TransactionIsolationLevel.Serializable })` surfaces a write conflict or deadlock as `PrismaClientKnownRequestError` with code `P2034`. Wrap in a bounded retry on that code. This is the Prisma 6 shape; the next major changes the transaction API, so confirm against the version in use.