From a932b976f7f70cd0004afe577b3513d6837bc29d Mon Sep 17 00:00:00 2001 From: Tachikoma Date: Sun, 2 Aug 2026 19:00:22 -0400 Subject: [PATCH 1/2] Move outstanding work from TODO.md into the issue tracker Fourteen issues on source.starfort.tech/andrew/libakerror, labelled by kind and blast radius and milestoned by what they can land in: 2.0.x for anything that breaks no ABI, 2.1.0 for additive surface, 3.0.0 for the handler-ladder rewrite. Everything filed carries status::grooming. Three of the fourteen were not in this file at all. They are defects in this library that were written down in a consumer's TODO instead: IGNORE() leaking a context (#14), the coverage target not being namespaced when embedded (#15), and no akerrorConfigVersion.cmake being installed (#16). libakstdlib carries a workaround for each. That is the finding worth keeping -- a library whose consumers record its defects in their own files cannot see them, and the workaround gets written once per consumer. TODO.md keeps the reasoning: why the handler ladder is a major-version change, why a copied akerr_ErrorContext is a trap in two specific fields, why validating more inputs lowers branch coverage by construction, why the mutation score is a floor, and why a registered name is returned by pointer. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- TODO.md | 230 +++++++++++++++++----------------------------- tests/MUTATION.md | 5 +- 3 files changed, 89 insertions(+), 148 deletions(-) diff --git a/README.md b/README.md index 9758ccc..9af9e16 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ This library has 6 guiding principles: | [docs/thread-safety.md](docs/thread-safety.md) | What thread safety here covers, what it does not, and how to hand an error to another thread | | [docs/building.md](docs/building.md) | Configure options, the generated header, and building without stdlib | | [UPGRADING.md](UPGRADING.md) | What changed in 1.0.0, 2.0.0 and 2.0.1, and how to migrate | -| [TODO.md](TODO.md) | Known defects, ordered by blast radius | +| [TODO.md](TODO.md) | The reasoning behind decisions and measurements. **Outstanding work is in [the issue tracker](https://source.starfort.tech/andrew/libakerror/issues).** | # Installation diff --git a/TODO.md b/TODO.md index 5d9f9f0..1525717 100644 --- a/TODO.md +++ b/TODO.md @@ -1,161 +1,103 @@ -# TODO +# Record -Working notes for `libakerror`. Outstanding items only. +**Outstanding work is in the issue tracker, not in this file:** + -## 1. Only ThreadSanitizer is wired into CI, not ASan/UBSan +Issues are labelled by kind and blast radius, and milestoned by what they can land +in: `2.0.x` for anything that breaks no ABI, `2.1.0` for additive surface, `3.0.0` +for the handler-ladder rewrite and the contract changes that go with it. Everything +filed carries `status::grooming` until it has been through grooming. -`AKERR_SANITIZE` builds the library and the tests with any sanitizer list, and -CI runs `-DAKERR_SANITIZE=thread` through `scripts/thread_test.sh`. Nothing runs -`address,undefined` yet, and that is the one that covers the original -motivation: mutation testing caught an out-of-bounds probe in the status-name -hash table that the suite could not, because the failure mode was a write into -adjacent BSS, which does not crash. Sharpening one test closed that instance; -ASan would catch the whole class regardless of how sharp the assertions are. +What stays here is the reasoning a tracker has no place for. -The machinery is in place — this is one more job in -`.gitea/workflows/ci.yaml` running -`cmake -S . -B build/asan -DAKERR_SANITIZE=address,undefined`. Left separate -because ASan and TSan cannot be combined in one build. +## Three defects were recorded only downstream -## 2. `HANDLE`-level status aliasing is still undetectable +Found while moving this file and `libakstdlib`'s into their trackers. Each is a +defect **in this library**, each was written down in a *consumer's* TODO file, and +none had an entry here: -Two components can compile the same integer into a `case` label without ever -reserving a range or registering a name, and nothing sees it. Ownership -enforcement covers *naming*, which is the part the library mediates; the `case` -label never reaches it. +| Issue | What it is | What it costs downstream | +|---|---|---| +| #14 | `IGNORE()` logs a context and never releases it | `libakstdlib`'s `aksl_tree_iterate` open-codes log-then-release by hand; every `IGNORE()` in `libakgl`'s `CLEANUP` blocks is a leaked slot | +| #15 | The `coverage` target is not namespaced when embedded, though `mutation` is | `libakstdlib` shadows `add_custom_target` to embed this library at all | +| #16 | No `akerrorConfigVersion.cmake` is installed | No consumer can ask `find_dependency(akerror)` for a version floor | -Closing this needs the `if`/`else if` handler ladder — rewriting -`PROCESS`/`HANDLE`/`HANDLE_GROUP`/`HANDLE_DEFAULT`/`FINISH` so status matching -is not restricted to integer constant expressions. That would also allow -matching on ranges or predicates, and would let a handler resolve a code through -its owner. It touches the most load-bearing code in the library and every -consumer's error handling at once, so it wants its own change. +**That is the finding worth keeping, more than the three defects.** A library whose +consumers record its defects in their own files has no way to see them: each +consumer knows one, nobody sees the pattern, and the workaround gets written three +times. The tracker is now the place, and a consumer filing upstream costs them one +issue instead of one workaround. -Note it would *not* by itself fix the "don't use `CATCH` or `FAIL_*_BREAK` -inside a loop" hazard: that comes from exiting via `break`, not from `switch`. +## Why the handler ladder is a `3.0.0` change and not a patch -## 3. No registry introspection +`PROCESS`/`HANDLE`/`HANDLE_GROUP`/`HANDLE_DEFAULT`/`FINISH` compile to a `switch`, +which restricts status matching to integer constant expressions. That is what makes +`HANDLE`-level aliasing undetectable (#4): two components can compile the same +integer into a `case` label without reserving a range or registering a name, and +**ownership enforcement never sees it, because it covers naming and the `case` label +never reaches the library.** -There is no way to ask who owns a status, or to enumerate reservations. The -"coordinate ranges at the dependency-stack level" advice in UPGRADING.md -therefore has no tooling behind it. +Rewriting it as an `if`/`else if` ladder would allow matching on ranges or +predicates and let a handler resolve a code through its owner. It also touches **the +most load-bearing code in the library and every consumer's error handling at once.** -A read-only accessor plus a dump through `akerr_log_method` would let a startup -self-check or a CI job print the whole map for a linked stack. Cheap, additive, -and the natural next step for multi-component adoption. +One thing it would *not* fix, recorded so nobody expects it to: the "don't use +`CATCH` or `FAIL_*_BREAK` inside a loop" hazard comes from exiting via `break`, not +from `switch`. -## 4. No way to release a reservation +## Why a copied `akerr_ErrorContext` is a trap -A plugin host that `dlopen`s many distinct plugins over a process lifetime -accumulates ranges until the table fills. Reloading the *same* plugin is fine — -an identical repeat by the same owner is idempotent. +Recorded because the struct looks copyable and is not, and #11 exists only if a +consumer ever needs it. -## 5. Renaming a status is not safe against a concurrent lookup +- **`stacktracebufptr` is self-referential.** `akerr_ErrorContext c = *src;` leaves + the copy's cursor pointing into the source's buffer, so the copy logs correctly + and then **corrupts a slot it does not own** the first time anything appends. +- **`arrayid` is restored after the wipe** in `akerr_release_error()` + (`src/error.c:395-398`), so a copied id makes the destination **impersonate the + source's slot for the life of the process.** + +If it is ever built, the shape takes the destination as a parameter rather than +allocating it: an allocating copy could fail on pool exhaustion, and **reporting +that failure needs a pool slot**, so it would have to abort — a third `exit()` site +in a library that deliberately has two. + +## Why validating more inputs lowers branch coverage + +Branch coverage on `src/error.c` sits just above its 50% gate, and the gate is set +where it is on purpose. + +Every `FAIL_*` site carries about **six** branch outcomes of error-construction +machinery (`ENSURE_ERROR_READY`, `AKERR_STACKTRACE_APPEND`) that only run when that +specific failure fires. **Every `PASS` site around a call that cannot fail carries +about twenty-five.** + +So adding a defensive check lowers the ratio by construction. **Before adding one, +expect to add a test that drives it**, as `tests/err_copy_string.c` does. + +## Why the mutation score is a floor + +`scripts/mutation_test.py` configures each mutant with the default CMake options, +so **a mutant that only breaks under concurrency is judged by a suite running +without ThreadSanitizer.** + +Measured: deleting the pool's `akerr_mutex_lock()` call survives the run, **even +though it is a real race.** Rebuilt and run directly, that mutant fails +`tests/err_threads_pool.c` in 4 of 10 runs, and fails under `scripts/thread_test.sh` +in 5 of 5. + +**81.2% is therefore a floor for that category, not a verdict.** #10 is the +passthrough that would fix the measurement, and it belongs behind a flag: a +TSan-instrumented suite per mutant costs roughly 6s instead of 0.4s. + +## Why a registered name is returned by pointer `akerr_name_for_status(status, NULL)` returns a pointer into the registry rather -than a copy, which is what makes it usable from inside `FAIL` — it needs no -buffer and no error context of its own. Registering a *second* name for a status -that already has one (`tests/err_name_ownership.c` covers that it is allowed) -overwrites that buffer in place, so a thread reading the name at that moment can -see a torn string. Every other registry operation is serialized; this one cannot -be, because the reader is outside the lock by the time it reads the characters. +than a copy, **which is what makes it usable from inside `FAIL`** — it needs no +buffer and no error context of its own. -Documented in docs/thread-safety.md and UPGRADING.md as "register names during -initialization". Closing it properly means making a registered name immutable — -either refusing a rename outright (a behavior change, and -`tests/err_name_ownership.c` asserts the current contract), or copying names -into a bump-allocated arena and publishing the pointer with a release store, so -a rename allocates new storage instead of rewriting live storage. The arena is -the better answer; it costs a second capacity limit and its exhaustion path. - -## 6. Deprecate the two-argument name-registration path - -`akerr_name_for_status(status, name)` cannot identify its caller, so it can only -check that *some* reservation covers the status, not that the caller owns it. It -exists for migration. Once consumers have moved to -`akerr_register_status_name()`, make the set path a no-op or remove it and leave -`akerr_name_for_status()` as pure lookup. - -## 7. `akerr_init()`'s own reservation failure is untested - -`tests/err_library_status_fatal.c` covers the terminal path in -`__akerr_name_library_status()` by naming a status the library does not own. The -band reservation in `akerr_init()` has no such handle: it can only fail in a -build whose tables are too small for the library's own entries, and both sizes -are `PRIVATE` to the library target, so a test executable cannot set them. - -Closing it means a second library target built with tiny tables plus a -`WILL_FAIL` test linked against it. Nothing in the CMake does that yet: the -sanitizer and coverage options vary the *flags* of the one library target, not -its compile definitions. - -Related: branch coverage on `src/error.c` now sits just above its 50% gate. -Every `FAIL_*` site carries about six branch outcomes of error-construction -machinery (`ENSURE_ERROR_READY`, `AKERR_STACKTRACE_APPEND`) that only run when -that specific failure fires, and every `PASS` site around a call that cannot -fail carries about twenty-five. Validating more inputs therefore lowers the -ratio by construction. Before adding defensive checks, expect to add a test that -drives them, as `tests/err_copy_string.c` does. - -## 8. Mutation testing judges concurrency mutants without a sanitizer - -`scripts/mutation_test.py` configures each mutant build with the default CMake -options, so a mutant that only breaks under concurrency is judged by a suite -running without ThreadSanitizer. Deleting the pool's `akerr_mutex_lock()` call -survives the run even though it is a real race: rebuilt and run directly, that -mutant fails `tests/err_threads_pool.c` in 4 of 10 runs, and fails under -`scripts/thread_test.sh` in 5 of 5. So 81.2% is a floor for that category, not a -verdict. - -Closing it means a `--cmake-arg` passthrough on the harness so the mutant build -can be configured with `-DAKERR_SANITIZE=thread`. The whole run then costs a -TSan-instrumented suite per mutant (roughly 6s instead of 0.4s), so it belongs -behind a flag rather than in the default target or in CI. - -## 9. No way to keep an error context and report it at the same time - -A context can be handed to another thread and released there -- -`docs/thread-safety.md` now documents that pattern and -`tests/err_threads_handoff.c` proves it -- but it is a *move*. A thread that -wants to both keep its error and report it upward has to read the fields out -into its own record, and it loses the stack trace doing so, because -`stacktracebuf` is the one thing that cannot be usefully summarized. - -Copying the struct is not a workaround. `stacktracebufptr` is self-referential -(`include/akerror.tmpl.h`), so `akerr_ErrorContext c = *src;` leaves the copy's -cursor pointing into the source's buffer -- the copy logs correctly and then -corrupts a slot it does not own the first time anything appends to it. `arrayid` -is restored after the wipe in `akerr_release_error()` (`src/error.c:395-398`), so -a copied id makes the destination impersonate the source's slot for the life of -the process. - -If this is ever worth an API, the shape is: - -```c -akerr_ErrorContext AKERR_NOIGNORE *akerr_copy_error(akerr_ErrorContext *source, - akerr_ErrorContext *destination); -``` - -taking the destination as a parameter rather than allocating it. An allocating -copy could fail on pool exhaustion, and reporting *that* failure needs a pool -slot, so it would have to abort -- adding a third `exit()` site to a library that -deliberately has two. Caller-allocates puts the pool pressure where it can be -managed. The copy must repair four fields: `arrayid` (the destination's own), -`refcount` (set to 1, never inherited), `handled` (false -- a copy is a fresh -obligation, or `FINISH_NORETURN` on the receiving side drops it silently), and -`stacktracebufptr` (re-anchored to the destination's buffer at the *same offset*, -so a later append continues the trace instead of overwriting it). - -Not worth building yet: no consumer needs it. The trigger is a consumer that -needs a worker's stack *trace*, not just its status and message, at the join -point -- `libakstdlib`'s planned `pthread_*` wrappers are the likely first. - -## Unrelated pre-existing issues - -- The `AKERR_USE_STDLIB=OFF` build does not compile at all: `bool`, `PATH_MAX` - and `NULL` are used unconditionally but only included under the stdlib branch. - `docs/building.md`'s dependency list states what a replacement must provide, - but the header still needs its includes untangled for that configuration to - work. -- `CMakeLists.txt` sets `main_lib_dest` from `MY_LIBRARY_VERSION`, which is never - defined and never read. Dead line. +The cost is that renaming a status is not safe against a concurrent lookup (#7): +every other registry operation is serialized, and this one cannot be, because **the +reader is outside the lock by the time it reads the characters.** Documented in +`docs/thread-safety.md` and `UPGRADING.md` as "register names during +initialization". diff --git a/tests/MUTATION.md b/tests/MUTATION.md index 953fe58..5362d74 100644 --- a/tests/MUTATION.md +++ b/tests/MUTATION.md @@ -149,9 +149,8 @@ Findings surfaced by mutation testing: * **Open:** the harness builds every mutant with the default CMake options, so a mutant that only breaks under concurrency is judged by a suite running without ThreadSanitizer. Mutating under `-DAKERR_SANITIZE=thread` would close that, - and needs a way to pass CMake options through to the mutant build. See - "Mutation testing judges concurrency mutants without a sanitizer" in - `TODO.md`. + and needs a way to pass CMake options through to the mutant build. That is + issue #10; `TODO.md` records why the score is a floor rather than a verdict. * **Superseded:** status names now use a private sparse registry, so the old public `AKERR_MAX_ERR_VALUE` ceiling and its consumer ABI mismatch no longer -- 2.43.0 From 651770f08fe08b41743660ac79bc9ea9b0f4e0be Mon Sep 17 00:00:00 2001 From: Tachikoma Date: Sun, 2 Aug 2026 19:24:49 -0400 Subject: [PATCH 2/2] Point AGENTS.md at the issue tracker for outstanding work The agent instructions now say where new work goes -- an issue on the forge, carrying status::grooming until its scope is settled -- and that TODO.md is the record: why the handler ladder is a major-version change, why a copied akerr_ErrorContext is a trap, why validating more inputs lowers branch coverage, and why the mutation score is a floor. The instruction that matters most here is about consumers. Three defects in this library -- IGNORE() leaking a context, the un-namespaced coverage target, and the missing akerrorConfigVersion.cmake -- were written down in libakstdlib's notes and never here, so each was worked around once per consumer and nobody saw the pattern. Filing upstream should cost a consumer one issue instead of one workaround. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0ed2b95..6919a09 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -133,11 +133,31 @@ anything that touches the pool, the registry, initialization, or the lock. Recent commits use short, imperative, sentence-case subjects, for example `Fix refcount leak and stack-trace buffer overflow`. Keep commits focused and describe the observable behavior changed. Pull requests should include a brief -summary, tests run, and any compatibility impact for public macros, generated -headers, installation paths, or CMake/pkg-config consumers. +summary, tests run, any compatibility impact for public macros, generated +headers, installation paths, or CMake/pkg-config consumers, and a link to the +issue they close. ## Agent-Specific Instructions +**Outstanding work goes in the issue tracker, not in a file.** Open an issue at + — `tea issues create +--repo andrew/libakerror` — naming the file and line, the functional +consequence, and what closing it would touch. Label it by kind and blast radius +and leave `status::grooming` on it until its scope and approach are settled. +**Do not add outstanding items to `TODO.md`**: that file is the record of why +the handler ladder is a major-version change, why a copied `akerr_ErrorContext` +is a trap, why validating more inputs lowers branch coverage, and why the +mutation score is a floor. A description of work still to do goes stale the +moment somebody does it. + +**This library's defects are most often found by its consumers, so make filing +them cheap.** Three defects in this library — `IGNORE()` leaking a context, the +un-namespaced `coverage` target, and the missing `akerrorConfigVersion.cmake` — +were written down in `libakstdlib`'s own notes and never here, so each was +worked around once per consumer and nobody saw the pattern. A consumer filing +upstream should cost them one issue instead of one workaround. `libakgl`, +`libakstdlib` and `akbasic` are all on the same forge. + Do not overwrite uncommitted user changes. Avoid editing generated files in `build/`; update `include/akerror.tmpl.h`, `src/error.c`, CMake files, tests, or scripts instead. -- 2.43.0