docs: ADR-0021 — no blocking work on a runtime worker thread - #78
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ADR 0021 for blocking-I/O rules and dedicated-thread SQLite actors. It records follow-up issues, updates ADR navigation, and adds ChangesBlocking I/O policy and SQLite actor roadmap
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 088dcaeea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| constraints rather than a guarantee — no I/O, no unbounded iteration, and nothing that grows | ||
| with request volume, inside the critical section. Sections that outgrow those constraints stop |
There was a problem hiding this comment.
When recurring alerts have distinct subjects, AlertDispatcher::dispatch inserts them into the unbounded last_sent HashMap while holding its standard mutex (daemon/src/alerts.rs:247-250); the Nostr abort tracker likewise retains and extends a request-sized vector under its lock (daemon/src/nostr_engine.rs:1838-1841). These are among the sections expressly endorsed just above, but they already violate the stated prohibitions on allocation and request-volume growth, and none of the four delivery beads changes them. Closing those beads would therefore leave the ADR's contract false; either narrow these constraints or include bounding/converting these sections in the tracked work.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/adr/0021-no-blocking-work-on-runtime-threads.md (1)
74-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the dependency facts used by this decision.
The ADR makes version- and implementation-specific claims about
tokio-rusqlite,deadpool-sqlite, andsqlx. Record the evaluated versions and the source or lockfile used for the channel and dependency-graph conclusions. Recheck this section when those versions change.Also applies to: 130-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0021-no-blocking-work-on-runtime-threads.md` around lines 74 - 77, Update the ADR’s dependency-facts section to record the evaluated versions of tokio-rusqlite, deadpool-sqlite, and sqlx, along with the source or lockfile used to verify their channel-based implementations and dependency graphs. State that these claims must be rechecked whenever the referenced versions change, including the related discussion at the additional marked section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.beads/issues.jsonl:
- Line 154: The shutdown requirement in issue lnrent-68f uses the superseded
“bounded, off-worker shutdown join” design. Replace that wording with a
requirement for a completion signal that the shutdown path can await and
abandon, or explicitly require following the final shutdown mechanism
implemented by lnrent-skk; remove the specific join reference while preserving
bounded, non-blocking worker shutdown behavior for both index actors.
In `@docs/adr/0021-no-blocking-work-on-runtime-threads.md`:
- Around line 212-214: Update the SQL-site inventory command in the ADR to
detect unqualified rusqlite imports and constructors, including patterns such as
use rusqlite::Connection and Connection::open alongside the existing reference
and transaction searches. Ensure the inventory covers cases like
daemon/src/backup.rs without replacing the documented scope with unrelated
searches.
- Around line 105-107: Update obligation 1 in the ADR to define the guarantee
from successful completion of Store::run’s self.tx.send(job).await, rather than
from invocation. Explicitly distinguish cancellation before send completion,
where no job delivery is required, from cancellation after send completion,
where the queued job must still be processed.
---
Nitpick comments:
In `@docs/adr/0021-no-blocking-work-on-runtime-threads.md`:
- Around line 74-77: Update the ADR’s dependency-facts section to record the
evaluated versions of tokio-rusqlite, deadpool-sqlite, and sqlx, along with the
source or lockfile used to verify their channel-based implementations and
dependency graphs. State that these claims must be rechecked whenever the
referenced versions change, including the related discussion at the additional
marked section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77e5e8a6-76bc-426a-9f61-0d800f2e3f97
📒 Files selected for processing (4)
.beads/issues.jsonlREADME.mddocs/adr/0021-no-blocking-work-on-runtime-threads.mdflake.nix
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
088dcae to
f253445
Compare
|
Round 1 addressed — all four findings verified against the tree before fixing, none rejected. Head is now codex P2 (mutex exception vs. the existing maps) — correct, and the sharper version of it is that the ADR named CodeRabbit, Major (enqueue boundary) — correct. Obligation 1 said a cancelled caller "leaves the job queued", which only holds after CodeRabbit, Major (inventory command undercounts) — correct, and CodeRabbit, Minor (stale join cross-reference) — correct. Context: this diff already went through an 11-pass codex + Claude Fable review loop before the PR opened (5 P1 / 33 P2 / 6 P3 fixed, 3 simplified, 2 cut). The loop was stopped deliberately on a fractal-tail signal rather than a clean pass — the remaining findings were about my corrections rather than the decision — so a fresh set of eyes finding four real items is the expected and useful outcome, not a surprise. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f25344506c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 1. **Enqueued work is not silently dropped.** The guarantee starts at *successful enqueue*: | ||
| `Store::run` returns from `tx.send(job).await` with `Ok`, then awaits a oneshot, so a caller | ||
| cancelled at that second await leaves a job already in the queue that must still run. A | ||
| caller cancelled *before* the send completes has no such job and is owed nothing — that | ||
| boundary is deliberate, and stating it is what makes the obligation testable. Closing the | ||
| senders lets `blocking_recv` return `None` once the queue drains. |
There was a problem hiding this comment.
When one database job stalls and another job has already been successfully enqueued behind it, the bounded shutdown described on lines 121-135 exits after logging the abandoned queue, so the queued job cannot “still run” as required here. The delivery beads inherit both obligations, leaving an implementation unable to satisfy the stated contract; scope the run-to-completion guarantee to a normal drain and explicitly define the timeout path as an acknowledged abandonment, including a test for a successfully enqueued job behind the stalled one.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
f253445 to
b4a8d4b
Compare
|
Round 2 — head is now codex P2 (accepted-job guarantee vs. bounded shutdown) — FIXED. Correct, and it's a contradiction I introduced in round 1: tightening obligation 1 to "must still run" put it in direct conflict with obligation 2's bounded exit, since a job enqueued behind a stalled one cannot satisfy both. Rather than weaken either obligation, the ADR now states the resolution explicitly: bounded exit wins, obligation 1 describes the normal drain only, and the timeout path is an acknowledged data-loss event that must log the abandoned queue depth so the loss is announced rather than inferred. CodeRabbit Major (SQL inventory undercounts) — REJECTED, already fixed in round 1. The comment re-anchored to Run against the file the finding names, it catches both lines the old pattern missed:
I verified this against the tree rather than assuming staleness, because "the bot re-posted an old finding" is a convenient conclusion and worth being suspicious of. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4a8d4ba4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| One narrow exception is deliberate and named here so the rule and the beads cannot disagree: | ||
| **the graceful-shutdown unlink.** `ipc.rs:471` removes the socket before in-flight handlers | ||
| drain at `:487`, so it does run while requests are in flight. It stays blocking — a single | ||
| bounded `unlink` on a path already committed to shutting down, where there is no latency left |
There was a problem hiding this comment.
When the socket resides on a stalled FUSE/network filesystem or the underlying filesystem is wedged, std::fs::remove_file can block indefinitely; because ipc.rs:471 executes it synchronously on a Tokio worker, the task cannot observe cancellation and runtime teardown can hang despite SHUTDOWN_DRAIN. Calling the unlink bounded therefore contradicts the ADR's bounded-shutdown obligation—offload it behind a cancellable wait, or explicitly acknowledge that shutdown is unbounded on this path.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.beads/issues.jsonl:
- Line 155: Remove the earlier statement in the “Sanctioned boundaries” guidance
that permits allowing load_operator_recipe in place. Keep the later directive
requiring load_operator_recipe and its Recipe::load_all work to be offloaded, so
the bead contains no contradictory exemption.
In `@docs/adr/0021-no-blocking-work-on-runtime-threads.md`:
- Around line 128-130: Qualify the SQLite recovery statement in this ADR by
limiting WAL plus synchronous=FULL to SQLite’s documented commit durability and
recovery guarantees under supported filesystem assumptions. Reword the
corresponding claims at the referenced passages so they do not imply protection
against I/O, filesystem, or storage failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da7a3705-d205-4b55-85ec-063ac32430cf
📒 Files selected for processing (4)
.beads/issues.jsonlREADME.mddocs/adr/0021-no-blocking-work-on-runtime-threads.mdflake.nix
🚧 Files skipped from review as they are similar to previous changes (2)
- flake.nix
- README.md
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
b4a8d4b to
88eedaa
Compare
|
Round 3 — head codex P2 (shutdown unlink is not bounded) — FIXED, and it's the best catch of the three rounds. Correct: Resolved by dropping the false label rather than adding machinery: the residual is now accepted and stated, with the reasoning (the socket lives in the data dir, so a filesystem wedged enough to hang this unlink has already made the daemon unable to commit anything, and a clean exit for an unrecoverable process isn't worth an offload). CodeRabbit Minor (contradictory CodeRabbit Major (WAL durability overstated) — FIXED. Correct. "WAL plus CodeRabbit Major (SQL inventory undercounts) — REJECTED, third posting, unchanged. The comment cites "Lines [212]-[214]" and quotes the Against the file the finding names, it matches both lines the old pattern missed — Two of these three rounds have turned up defects introduced by my own previous fix, which is the expected shape this late in a review — but each has been real, and an ADR that contradicts itself would hand the implementer impossible obligations. Worth the rounds. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88eedaa4bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| daemon's filesystem work is genuinely pre-runtime (`config.rs`), offline (`backup.rs`, which | ||
| refuses to run against a live daemon), or one-shot startup, and converting those buys |
There was a problem hiding this comment.
When implementing lnrent-njv, this rationale can cause config.rs blocking calls to be exempted as if no runtime existed, but only load_raw_config precedes build_runtime (main.rs:243-254); prepare_data_dir and bootstrap_headless_with_store execute inside async run_daemon (main.rs:410-415). The ADR itself calls this distinction load-bearing, so describe this category as pre-serving/one-shot startup, or restrict the config.rs parenthetical to load_raw_config, to keep the exemption rationale consistent with the enforcement audit.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0021-no-blocking-work-on-runtime-threads.md`:
- Around line 184-187: Update the rejection rationale around the configuration
filesystem work to say “pre-serving” instead of “pre-runtime,” accurately
distinguishing config::load_raw_config from config::prepare_data_dir and
config::bootstrap_headless_with_store, which run inside run_daemon on runtime
workers.
- Around line 92-95: Update ADR 0021’s evaluation notes or add an appendix
documenting the exact evaluated versions of tokio-rusqlite, rusqlite,
libsqlite3-sys, deadpool-sqlite, and sqlx, including dependency-graph evidence
from Cargo.toml and Cargo.lock so the SQLite runtime claims remain reproducible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec43069a-a10c-43a3-bf56-188e6a88a73c
📒 Files selected for processing (4)
.beads/issues.jsonlREADME.mddocs/adr/0021-no-blocking-work-on-runtime-threads.mdflake.nix
🚧 Files skipped from review as they are similar to previous changes (3)
- flake.nix
- README.md
- .beads/issues.jsonl
| *"Blocking"* — this is not "all code is async", which is unachievable: sqlite has no async | ||
| implementation in Rust. Every crate advertising one (`tokio-rusqlite`, `deadpool-sqlite`, | ||
| `sqlx`'s sqlite driver) is a thread-and-channel wrapper around the same blocking calls. The | ||
| achievable rule is about *where* blocking runs, not whether it exists. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while IFS= read -r file; do
rg -n -C 2 'tokio-rusqlite|rusqlite|libsqlite3-sys|deadpool-sqlite|sqlx' "$file" || true
done < <(fd -a -t f '^(Cargo\.toml|Cargo\.lock)$' .)
if command -v cargo >/dev/null 2>&1; then
cargo tree --workspace --edges normal,build |
rg -n 'tokio-rusqlite|rusqlite|libsqlite3-sys|deadpool-sqlite|sqlx' || true
fiRepository: douglaz/lnrent
Length of output: 144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ADR relevant lines =="
sed -n '80,105p' docs/adr/0021-no-blocking-work-on-runtime-threads.md 2>/dev/null || true
sed -n '150,180p' docs/adr/0021-no-blocking-work-on-runtime-threads.md 2>/dev/null || true
echo
echo "== Locate manifests =="
fd -a -t f '^(Cargo\.toml|Cargo\.lock)$' . || true
echo
echo "== Dependency mentions in manifests =="
while IFS= read -r file; do
echo "--- $file ---"
rg -n -C 2 'tokio-rusqlite|rusqlite|libsqlite3-sys|deadpool-sqlite|sqlx' "$file" || true
done < <(fd -a -t f '^(Cargo\.toml|Cargo\.lock)$' .)
echo
echo "== Rust tooling availability =="
command -v cargo || true
cargo --version || true
rustc --version || true
echo
echo "== Targeted lockfile entries (if Cargo.lock exists) =="
file="$(fd '^Cargo\.lock$' . | head -n 1)"
if [ -n "${file:-}" ]; then
awk '
/^\[\[package\]\]$/ { name=""; version=""; source=""; deps="" }
/^name = / { gsub(/^name = "/,""); gsub(/"$/,""); name=$0 }
/^version = / { gsub(/^version = "/,""); gsub(/"$/,""); version=$0 }
/^source = / { gsub(/^source = "/,""); gsub(/"$/,""); source=$0 }
/^name = "rusqlite|name = "tokio-rusqlite|name = "libsqlite3-sys|name = "deadpool-sqlite|name = "sqlx/ {
p=1; printf "\npackage: %s version: %s\n", name, version; if (source) printf "source: %s\n", source
}
p && /^dependencies = \[/ { depblock=1 }
p && depblock && /^\[/ { depblock=0; print }
p && depblock { print }
p && /^name = / { p=0 }
END { if (p) print "package:" name " version:" version }
' "$file"
fiRepository: douglaz/lnrent
Length of output: 144
Include the exact dependency versions evaluated for this ADR.
The ADR makes SQLite runtime claims that depend on tokio-rusqlite, rusqlite, libsqlite3-sys, deadpool-sqlite, and sqlx versions. Add the evaluated versions and dependency graph evidence from Cargo.toml/Cargo.lock, e.g. in an appendix or evaluation notes, so future maintainers can repeat these checks against the same dependency set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/0021-no-blocking-work-on-runtime-threads.md` around lines 92 - 95,
Update ADR 0021’s evaluation notes or add an appendix documenting the exact
evaluated versions of tokio-rusqlite, rusqlite, libsqlite3-sys, deadpool-sqlite,
and sqlx, including dependency-graph evidence from Cargo.toml and Cargo.lock so
the SQLite runtime claims remain reproducible.
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
88eedaa to
9a02105
Compare
|
Round 4 — head codex P2 + CodeRabbit Major — same finding, and the first time the two bots have agreed on this PR. FIXED. Line 185 still called Fixed at the source and then swept: CodeRabbit Minor (add an appendix of evaluated dependency versions) — CUT, over-specification. The ADR already cites every version its claims rest on, inline where the claim is made: CodeRabbit Major (SQL inventory undercounts) — REJECTED, fourth identical posting. Same text, same stale "Lines [212]-[214]" citation, same already-replaced pattern. Current state, unchanged since round 2: It matches both Round-by-round this is converging: 4 real → 1 real + 1 stale → 3 real + 1 stale → 1 real + 1 cut + 1 stale. The single real finding this round was a wording inconsistency, not a defect in the decision, which has been stable since well before the PR opened. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a021059b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| not allowed. The genuinely exempt set is the two `config.rs` calls plus `prepare_data_dir`: | ||
| fixed, single-shot work that does not grow with anything. |
There was a problem hiding this comment.
When lnrent-njv applies the denylist, this declares the config entry points exempt as fixed work, but bootstrap_headless_with_store reaches read_secret_file_bytes, whose read_to_end is unbounded (config.rs:1707-1747). That is the same data-scaled blocking work that lines 62-68 use to put Recipe::load_all in scope, so the implementer cannot consistently decide which startup calls must be offloaded. Either exempt all pre-serving work regardless of size or require these unbounded config reads to be bounded/offloaded too.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.beads/issues.jsonl (2)
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd current/target versions for the web-only dependency row.
Every other row in lnrent-hrm's upgrade table states a current and target version (for example
rusqlite 0.31 -> 0.40.1), but the last row only names the crates:getrandom/gloo-net/gloo-timers (web buyer only)Without version numbers, whoever picks up this bead cannot tell what the target state is for these three crates, and the acceptance criterion "Each bump is a reviewable step" is ambiguous for this row specifically. State the current and target versions, matching the format of the other rows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.beads/issues.jsonl at line 158, Update the web-only dependency row in the lnrent-hrm upgrade table to include current and target versions for getrandom, gloo-net, and gloo-timers, using the same current-to-target format as the other dependency rows. Keep the web buyer-only scope and crate grouping unchanged.
154-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate lnrent-skk's shutdown acceptance criteria into one final list.
The shutdown-mechanism guidance for lnrent-skk goes through four layered corrections: the round-8 addendum ("joins each actor thread"), the round-10 correction ("supersedes the round-8 addendum," proposes
spawn_blocking+timeout), the round-11 correction ("superseding BOTH addenda above," refutes round 10's own proposal, proposes a completion signal instead), and a final PR#78round-2 refinement that adds the "healthy job enqueued behind a stalled one" requirement without an explicit "Acceptance:" header.Each correction is internally consistent when read in sequence, but a reader who stops at the round-11 "Acceptance (supersedes both earlier shutdown lines)" block will miss the PR
#78refinement that the stalled-job test must also enqueue a healthy job behind the stalled one and assert the logged abandoned-queue-depth. Add a single, final "Acceptance criteria (shutdown)" block near the end of the description that states the current, complete set of requirements, so an implementer does not need to trace four rounds of "supersedes" language to find the true bar.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.beads/issues.jsonl at line 154, Consolidate the shutdown requirements in the issue description into one final “Acceptance criteria (shutdown)” block near the end, covering the current completion-signal mechanism, bounded shutdown without unbounded Tokio-worker blocking, preservation of enqueued jobs, and the PR `#78` refinement requiring the stalled-job test to enqueue a healthy job behind the stalled one and assert the logged abandoned-queue depth. Remove or supersede the layered earlier shutdown guidance so readers are not required to reconcile conflicting rounds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0021-no-blocking-work-on-runtime-threads.md`:
- Around line 99-102: Update the SQLite connection architecture statement to
limit the async-only contract to serving/runtime paths, explicitly excluding the
synchronous offline backup entry point in backup.rs. Preserve the
dedicated-thread and bounded-channel requirements for the state DB, lnv2 index,
and phoenixd index.
- Around line 65-70: Clarify the exempt call sites in the ADR by explicitly
naming config::load_raw_config, config::prepare_data_dir, and
config::bootstrap_headless_with_store. State that only config::load_raw_config
runs pre-runtime, while config::prepare_data_dir and
config::bootstrap_headless_with_store run pre-serving, and remove the ambiguous
“two config.rs calls plus prepare_data_dir” phrasing.
- Around line 263-265: Update the ADR text describing the denylist near the
“std::thread::sleep” examples to replace “future blocking primitives” with
wording that limits coverage to explicitly listed blocking primitives,
accurately reflecting the configured clippy.toml denylist.
---
Nitpick comments:
In @.beads/issues.jsonl:
- Line 158: Update the web-only dependency row in the lnrent-hrm upgrade table
to include current and target versions for getrandom, gloo-net, and gloo-timers,
using the same current-to-target format as the other dependency rows. Keep the
web buyer-only scope and crate grouping unchanged.
- Line 154: Consolidate the shutdown requirements in the issue description into
one final “Acceptance criteria (shutdown)” block near the end, covering the
current completion-signal mechanism, bounded shutdown without unbounded
Tokio-worker blocking, preservation of enqueued jobs, and the PR `#78` refinement
requiring the stalled-job test to enqueue a healthy job behind the stalled one
and assert the logged abandoned-queue depth. Remove or supersede the layered
earlier shutdown guidance so readers are not required to reconcile conflicting
rounds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88bc29ad-46a8-4e4b-911c-6598e0f67ae3
📒 Files selected for processing (4)
.beads/issues.jsonlREADME.mddocs/adr/0021-no-blocking-work-on-runtime-threads.mdflake.nix
🚧 Files skipped from review as they are similar to previous changes (2)
- flake.nix
- README.md
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
9a02105 to
23f36cf
Compare
|
Round 5 — head codex P2 + CodeRabbit Major, both on line 70 — FIXED by cutting, not patching. codex is right: The fix is structural rather than another qualifier. The ADR now states the criterion and stops classifying sites: fixed-cost + pre-serving is exemptible; recurring, data-scaled, or reachable-once-serving must be offloaded. Site-by-site classification moves to The reason is on the record in the ADR itself: four review rounds produced four different wrong classifications from that section — CodeRabbit Major (async-only contract vs. the backup door) — FIXED. Correct contradiction: the pattern section said "the only public surface is CodeRabbit Minor (denylist coverage overclaim) — FIXED. "future blocking primitives" implied coverage the mechanism cannot give. Now "any other explicitly listed blocking primitive". Rejected, both repeats: the SQL-inventory finding (fifth identical posting, still citing pre-fix "Lines [212]-[214]"; the one Round tally: 4 real → 1 → 3 → 1 → 4. The count went back up, and every one traced to the same block, which is why it's gone rather than fixed. If the next round is clean on that section I'd suggest landing: what's left is mechanism that only code and tests can settle, and it's already delegated to |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23f36cf9e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unbounded, which for a `synchronous=FULL` money DB converts a slow fsync into unbounded | ||
| queue growth with no backpressure signal, where `mpsc::channel(64)` makes the caller wait. |
There was a problem hiding this comment.
When the actor queue fills behind a slow fsync, this does not actually bound pending memory at the current IPC ingress: serve_with_shutdown accepts every same-UID connection and immediately adds a handler to conns (ipc.rs:424-449) without a semaphore, so each additional mutating request can retain its Job while awaiting tx.send. The 64-slot channel therefore moves excess work into an unbounded set of waiting tasks rather than providing the claimed protection against unbounded growth; carry the bound through request admission, cap in-flight IPC handlers, or narrow this rationale to actor-local queue growth.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
23f36cf to
b799234
Compare
|
Round 6 — head codex P2 (the bound doesn't reach IPC admission) — FIXED, and it correctly punctured a claim I'd made twice. Verified: Both occurrences narrowed to what's actually true — the bound makes pressure visible at the call site and bounds the actor's queue, rather than bounding total pending memory. This matters beyond wording because that claim was one of three reasons given for rejecting Capping IPC admission is deliberately not opened here, and the ADR says why rather than leaving it implied: the socket is owner-only and Rejected: the SQL-inventory finding (sixth identical posting, unchanged text, still citing pre-fix "Lines [212]-[214]") and the dependency-version appendix (third posting; cut in round 4 — the ADR cites every version inline and The section I cut last round produced zero findings this round — that was the convergence signal I was watching for. Round tally: 4 → 1 → 3 → 1 → 4 → 1. This is the point to land, and I'd rather say so than keep spending rounds. Six rounds have moved the document from wrong to narrow; what remains is mechanism — the shutdown drain, the denylist's real coverage, the site classification — which is delegated to |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7992341df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| still depends on `rusqlite` directly, and there is no `clippy.toml`. The four beads that | ||
| deliver it are `lnrent-skk` (store actor), `lnrent-68f` (index actors), `lnrent-njv` (lint | ||
| denylist) and `lnrent-7dw` (crate boundary). Read every "does" and "is" below as "will" until |
There was a problem hiding this comment.
When ADR-0021 gains, splits, or replaces a delivery bead, this hand-maintained count and list can remain unchanged and incorrectly report the decision as built once only these four beads close. The delivery set is already derivable from .beads by querying references to ADR-0021, so name that source of truth rather than duplicating its current membership here.
AGENTS.md reference: AGENTS.md:L171-L173
Useful? React with 👍 / 👎.
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
|
Round 7 — head codex P2 (frozen delivery-bead list) — FIXED. Correct, and consistent with every count finding on this PR: the Status header hand-maintained "the four beads that deliver it are skk / 68f / njv / 7dw", which silently rots the moment a bead splits or another is added — and would then report the decision as built once only those four close. Replaced with the derivation, verified to return all four before committing: br list --limit 0 --json -a | jq -r '.[] | select(.status!="closed")
| select((.description // "") + (.title // "") | test("ADR-0021")) | "\(.id) \(.title)"'I checked that Swept the rest of the ADR for the same defect: the remaining Rejected, all repeats: the SQL-inventory finding (seventh posting, unchanged, still citing pre-fix line numbers), the dependency-version appendix (fourth; cut in round 4 as over-specification), and the denylist-coverage wording CodeRabbit itself marked Round tally: 4 → 1 → 3 → 1 → 4 → 1 → 1. Seven rounds, CI green on every one. |
b799234 to
d147dff
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d147dffd0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - `alerts.rs:248` inserts into an unbounded `last_sent` map keyed by `(kind, subject)`. | ||
| - `nostr_engine.rs:1839` does a `retain()` scan over a vector sized by in-flight requests. | ||
|
|
||
| None of the four delivery beads changes them, and none of the three is a live hazard at current |
There was a problem hiding this comment.
Although lines 7–13 now derive the outstanding delivery set, fresh evidence after the prior comment is that this separate hard-coded count remains. If an ADR-0021 delivery bead is split or added, “four” can silently become false and misstate whether the grandfathered mutex sections are covered; refer to the derived set without freezing its size.
AGENTS.md reference: AGENTS.md:L171-L173
Useful? React with 👍 / 👎.
| {"id":"lnrent-3zt","title":"web buyer e2e: the request-kind op assertion cannot fail the run (try/catch then exit 0)","description":"## The gap\nThe web buyer e2e's request-kind op step cannot fail the run. `clients/web/e2e/web-buyer-e2e.mjs`\nwraps the whole op sequence in a `try`, sets `opOk = false` in the `catch`, logs\n\"request-kind op step skipped/soft-failed\", and then reaches `process.exit(0)` regardless. The only\nhard failure in the script is a page exception.\n\nSo if `op.request` -> `op.result` regresses in the SPA — ops never listed, the click does nothing,\nthe result never rendered — CI stays green and the PASS line simply omits the op clause.\n\n## Why it matters\n`docs/specs/web-wasm-buyer.md` makes this an acceptance requirement: the e2e must assert that a\nrequest-kind operation runs and displays `op.result.data`. A check that logs and exits 0 does not\nassert anything. It is worse than no check, because the PASS line reads as coverage.\n\nFound 2026-07-31 by a codex review of the docs cleanup, which correctly refused the claim that the\nmissing CSP (lnrent-3ma) was that spec's ONLY outstanding requirement.\n\n## Scope\n- Make the op step fatal: on failure, exit non-zero with which sub-step failed.\n- Decide deliberately whether the step may be SKIPPED when `#ops-section` is absent (a recipe with\n no declared operations is legitimate) versus soft-failing when the section exists but the flow\n breaks. Those are different conditions and only the second should be fatal.\n- Keep the failure message specific enough to diagnose from CI logs alone.\n\n## Acceptance\n- A deliberately broken op flow makes the e2e exit non-zero in CI.\n- A recipe with no operations still passes, by an explicit skip path, not by swallowing an error.\n- web-wasm-buyer.md's status is updated once this and lnrent-3ma are both closed.\n\n## The predicate is ALSO vacuous — making the catch fatal is NOT enough\nCaught by a second codex pass, 2026-07-31. Even with the `try`/`catch` removed, the wait that\nclaims to prove the op returned cannot fail:\n\n```\nawait waitFor(ws, `(document.getElementById('ops-list')?.textContent||'').length > 0 && ...`,\n 20000, 'op.result shown');\n```\n\n`#ops-list` is the element holding the operation BUTTONS, so its `textContent` is already non-empty\nthe moment the list renders — before any operation is invoked. The predicate is therefore true on\nentry and the 'op.result shown' label is unearned. The actual result is written elsewhere: the\nhandler sets `resultEl.textContent = JSON.stringify(result.data, null, 2)`\n(`clients/web/static/app.js`), a sibling `<pre>`, and clears it on error.\n\nSo the fix has TWO parts, and shipping only the first leaves the check just as green on regression:\n1. make the failure fatal (exit non-zero, naming the failed sub-step), AND\n2. replace the predicate with an assertion on the RENDERED RESULT element — that it becomes\n non-empty and parses as the op's `data` — not on the list that was already populated.\n\nA regression test for the test itself is worth it here: break the op path deliberately and confirm\nthe e2e goes red. This bead exists because a check that logs and exits 0 read as coverage for\nhowever long it has been in CI.","status":"open","priority":2,"issue_type":"bug","created_at":"2026-07-31T18:35:17.259726659Z","created_by":"master","updated_at":"2026-07-31T20:22:41.427423490Z","source_repo":"lnrent","source_repo_path":"/home/master/projects/lnrent","compaction_level":0,"original_size":0} | ||
| {"id":"lnrent-skk","title":"store actor runs blocking sqlite on a tokio worker thread (ADR-0021)","description":"The store actor (ADR-0001's sole writer) runs SYNCHRONOUS rusqlite inside a plain\n`tokio::spawn` task (daemon/src/store.rs:743). While a job executes, it occupies a tokio\nworker thread for the duration of the SQL. ADR-0021 fixes the rule: once the daemon is\nserving, no blocking I/O and no unbounded-duration work runs on a runtime worker. (NOT \"no blocking call\" -- ADR-0021 explicitly rejects that phrasing and permits short in-memory std::sync::Mutex sections.)\n\nOwn an OS thread instead of a runtime task:\n\n std::thread::spawn(move || {\n let mut conn = conn;\n while let Some(job) = rx.blocking_recv() { job(&mut conn); }\n });\n\nKEEP the bounded `mpsc::channel::<Job>(64)`. The bound is load-bearing: with\n`synchronous=FULL`, an unbounded queue turns a slow fsync into unbounded memory growth and\nunbounded latency with no signal, whereas `tx.send(job).await` makes the caller wait. (One of\nthree reasons ADR-0021 rejected adopting tokio-rusqlite, whose channel is\n`crossbeam_channel::unbounded`.)\n\nMOVING THE LOOP IS NOT ENOUGH -- caught in review. `Store::open_spawn` (store.rs:770)\nevaluates `open(path)?` BEFORE spawning, and `open()` does `Connection::open`, the PRAGMA\nbatch, `quick_check` and migrations (store.rs:600-640). That work runs on the CALLER's\nthread; the production call site is config.rs:1009. So the connection must be created and\ninitialized INSIDE the actor thread, with its initialization result returned to the caller\nasynchronously (a oneshot carrying `Result`), or the bead ships with the blocking part\nuntouched and only the cheap part fixed.\n\nVerified safe to move off the runtime: the job closure is\n`Box::new(move |conn| { let _ = rtx.send(f(conn)); })` -- pure rusqlite plus a\n`oneshot::Sender::send`, neither of which needs a tokio context. Only `rx.recv().await` did,\nand `blocking_recv()` is its off-runtime equivalent.\n\nAcceptance:\n- Store::spawn owns a std::thread; no SQL executes on a tokio worker.\n- Connection OPEN + PRAGMAs + quick_check + migrations also happen on that thread, and their\n failure still surfaces to the caller as an error (the y4m.3 empty-file and quick_check\n gates must keep failing startup loudly).\n- The 64-slot bound is unchanged and a test proves backpressure still applies.\n- Existing store tests pass unchanged, including under #[tokio::test] (current_thread).\n- Panic behaviour is unchanged: a panicking job still ends the actor and subsequent sends\n fail with \"store actor stopped\".\n\nKEEP THE JoinHandle AND DRAIN ON SHUTDOWN (review, round 8). The naive\n`std::thread::spawn(...)` discards its handle, and a detached thread does NOT keep the process\nalive -- so daemon exit can terminate the actor mid-job while work is still queued. The window\nis real: `Store::run` (store.rs:1069-1078) sends the job and then awaits the oneshot, so a\ncancelled caller leaves its job queued behind it.\n\nRequired shape: retain the `JoinHandle`, and on shutdown close the sender, let the actor drain\nthe queue to completion, then join the thread before the process exits. Do this for all three\nactors (state DB + both indexes, see lnrent-68f).\n\nThis is not a regression the bead introduces -- today's `tokio::spawn` actor dies with the\nruntime just as abruptly -- but the bead is where the thread's lifetime becomes explicit, so\nit is where the drain belongs. sqlite's WAL + `synchronous=FULL` means an interrupted job\ncannot corrupt the DB; what it can do is silently drop queued work a caller was told to expect.\n\nAcceptance addendum:\n- Shutdown closes the channel, drains queued jobs, and joins each actor thread.\n- A test proves a job sent immediately before shutdown still commits.\n\nTHE JOIN MUST BE BOUNDED AND OFF-WORKER (review, round 10 -- correcting round 8's addendum\nabove, which said \"join the thread\" without either qualifier). A bare `handle.join()` awaited\nfrom async shutdown blocks a tokio worker for an unbounded time (forbidden by this very ADR)\nand contradicts `SHUTDOWN_GRACE` at daemon/src/supervisor.rs:95, whose doc comment says\noutright that \"a stuck loop must not hang process exit\". A stalled fsync would hang the daemon\nforever -- the round-8 addendum traded one defect for two.\n\nRequired: drop the senders, then await the join inside `spawn_blocking` under a\n`tokio::time::timeout` aligned with SHUTDOWN_GRACE. On timeout, log the abandoned queue depth\nand exit anyway; WAL + `synchronous=FULL` makes the DB crash-safe, and bounded exit outranks a\ndrained queue.\n\nAcceptance (supersedes the round-8 line \"joins each actor thread\"):\n- Shutdown closes the channel, drains, and joins OFF-WORKER under a bound.\n- A stalled-job test proves exit still happens within the bound.\n\nSHUTDOWN: THIS BEAD OWNS THE MECHANISM (review, round 11 -- superseding BOTH addenda above).\nThree drafts were specified in the ADR and all three were refuted:\n 1. bare `handle.join()` -- unbounded, and blocks a tokio worker.\n 2. `timeout(spawn_blocking(|| handle.join()))` -- a timed-out blocking task cannot be\n abandoned, and dropping the runtime at main.rs:258 waits for the blocking pool, so the\n process still hangs.\n 3. (whatever is chosen) -- must be TESTED, not argued.\n\nThe obligations, which do not change:\n a. Queued jobs are not silently dropped: closing the senders drains the queue.\n b. Shutdown stays bounded -- SHUTDOWN_GRACE (supervisor.rs:95) says a stuck loop must not\n hang process exit.\n c. The wait must not block a tokio worker for an unbounded time.\n\nA completion SIGNAL the shutdown path can await and abandon (rather than any join) looks like\nthe shape that satisfies all three, since a detached std::thread does not hold up process exit\n-- but verify it, including that runtime teardown does not wait on anything left behind.\n\nAcceptance (supersedes both earlier shutdown lines):\n- A stalled-job test: with a job wedged, the process still exits within the bound.\n- A drain test: a job sent immediately before shutdown still commits.\n\nTHE TWO OBLIGATIONS COLLIDE -- TEST THE COLLISION (codex review, PR #78 round 2). Obligation\n(a) says enqueued jobs run to completion; obligation (b) says shutdown exits on the bound. When\none job stalls, a healthy job enqueued BEHIND it cannot satisfy both. ADR-0021 resolves it:\nbounded exit WINS, and the timeout path is an acknowledged data-loss event -- log the abandoned\nqueue depth so it is visible rather than inferred.\n\nThe test that matters is therefore not just \"a stalled job still lets us exit\". It is:\n- enqueue a healthy job BEHIND a stalled one,\n- confirm the process still exits within the bound,\n- confirm the abandoned queue depth is logged (>= 1), so the loss is announced.\nA test that only wedges a single job passes without ever exercising the collision.","status":"open","priority":2,"issue_type":"task","created_at":"2026-08-01T18:32:54.493796994Z","created_by":"master","updated_at":"2026-08-02T17:01:17.439986189Z","source_repo":"lnrent","source_repo_path":"/home/master/projects/lnrent","compaction_level":0,"original_size":0} | ||
| {"id":"lnrent-68f","title":"payment-backend index sqlite is called directly from async fns, with no boundary (ADR-0021)","description":"ADR-0021 requires every blocking resource to sit behind an async API. The two payment\nbackend indexes do NOT: `Arc<Mutex<Connection>>` is called directly from async fns via ~20\nfree functions (`idx_get_by_external`, `idx_insert`, `idx_mark_canceled`, ... in\ndaemon/src/lnv2_backend.rs:1463+ and daemon/src/phoenixd_backend.rs:1686+).\n\nThese are a WORSE violation than the store was. The store at least had a boundary -- a\nchannel in, a oneshot out, an async-only surface -- and was merely mounted on the wrong kind\nof thread. The index helpers have no boundary at all: a raw blocking sqlite call inside an\nasync fn, which is precisely the surface where a later edit adds an `.await` and something\nsubtle breaks.\n\nApply the same sanctioned pattern: a dedicated-thread actor per connection (they stay THREE\nseparate connections -- lnv2_index.db lives inside the federation data-dir and the two indexes are backed up by\nDIFFERENT mechanisms -- backup.rs:253 `vacuum_if_present` captures ONLY the phoenixd index,\nwhile the lnv2 index rides inside the opaque fedimint subtree byte copy at backup.rs:237. Two capture mechanisms is exactly why folding them into the state DB would entangle backup/restore layout with a concurrency fix).\n\nDepends on the store actor bead so the pattern exists to reuse.\n\nAcceptance:\n- No `Mutex<Connection>` remains; idx_* are async and reach sqlite only via the actor.\n- The comments asserting \"the lock never crosses an `.await`\" are deleted along with the\n locks they describe.\n- The existing index GC `spawn_blocking` (lnv2_backend.rs:588) is folded into the actor --\n it is the one long index operation and no longer needs a special case.\n- Money-path behaviour is unchanged: create-once, the pay maps, and recovery all keep their\n current semantics. This is a transport change, not a semantics change.\n\nDO NOT collapse the chunked GC into a single actor job (review, round 4). The current reaper\ntakes and RELEASES the index mutex per 512-row batch on purpose -- lnv2_backend.rs:1763 says\nso verbatim (\"Chunking releases the sole index mutex between batches on a flooded DB\"), with\nthe lock acquired inside the loop. Folding both GC loops into one job would hold every queued\ninvoice/pay operation behind the entire backlog on a flooded DB, which is a money-path\nLIVENESS change, not the transport-only change this bead claims. Each batch must remain a\nseparate actor job (or otherwise yield between batches) so ordinary money operations keep\ninterleaving.\n\nINITIALISE INSIDE THE ACTOR THREAD, exactly as lnrent-skk requires for the state DB (review,\nround 10). Both index constructors do blocking sqlite work on the `run_daemon` worker today:\n`Connection::open(paths.index_db)` + `execute_batch(INDEX_SCHEMA)` at lnv2_backend.rs:1924-1926,\nand the same plus `prepare_private_file` at phoenixd_backend.rs:786-790. Moving only the query\nloop into an actor would leave these untouched, and all four ADR-0021 beads could close with\nblocking startup SQL still on a worker.\n\nShutdown: track WHATEVER MECHANISM lnrent-skk lands on -- do not name one here. An earlier\n\nAcceptance addendum:\n- Index open + schema init happen on the actor thread; failures still surface to the caller.\ndraft of this line said \"the same bounded, off-worker shutdown join skk specifies\", which skk\nitself walked back: its round-11 revision supersedes every join-based draft in favour of a\ncompletion signal the shutdown path can await and abandon, and its acceptance criteria no\nlonger mention a join at all. An implementer reading this bead in isolation would have copied\nthe design skk discarded for blocking a tokio worker unboundedly.\n\nThe obligations are the stable part: enqueued jobs are not silently dropped, shutdown stays\nbounded, and the wait never blocks a tokio worker unboundedly. Both index actors inherit\nskk's final mechanism, whatever it turns out to be.","status":"open","priority":2,"issue_type":"task","created_at":"2026-08-01T18:32:54.567435752Z","created_by":"master","updated_at":"2026-08-02T14:50:26.194647583Z","source_repo":"lnrent","source_repo_path":"/home/master/projects/lnrent","compaction_level":0,"original_size":0,"dependencies":[{"issue_id":"lnrent-68f","depends_on_id":"lnrent-skk","type":"blocks","created_at":"2026-08-01T18:32:54.861342548Z","created_by":"master","metadata":"{}","thread_id":""}]} | ||
| {"id":"lnrent-njv","title":"clippy.toml denylist for blocking primitives, with a proven failure path (ADR-0021)","description":"ADR-0021's rule is only real if a violation fails a build. Add `clippy.toml` with\n`disallowed-methods` covering the blocking primitives, denied by CI's existing `-D warnings`.\n\nMEASURED, not assumed: `clippy::disallowed_methods` is warn-by-default once configured\n(verified on rustc 1.96.0 with a throwaway crate -- it fires on the call and is silenced by an\nexplicit `#[allow(clippy::disallowed_methods)]`). No lint-level plumbing beyond the config\nfile.\n\nKNOW WHAT THIS DOES NOT DO. `disallowed-methods` matches the exact paths listed and nothing\nelse: an unlisted blocking call (`std::net::TcpStream::connect`,\n`std::sync::mpsc::Receiver::recv`, a future crate's blocking API) still compiles silently.\nDemonstrating one listed violation proves the lint is LIVE, not that every violation fails the\nbuild. Do not describe this bead's output as closing the category -- it closes an audited\nlist. Say so in the PR.\n\nCandidate denylist: EVERY `rusqlite::Connection` constructor. Do NOT enumerate them from\nmemory -- an earlier draft of this bead said \"four\" and rusqlite 0.31 has SIX (`open`,\n`open_in_memory`, `open_with_flags`, `open_with_flags_and_vfs`, `open_in_memory_with_flags`,\n`open_in_memory_with_flags_and_vfs`). Derive the list with `grep -E \"pub fn open\" src/lib.rs\nin the vendored rusqlite source, and re-derive it after the 0.40 bump. Plus `std::thread::sleep` and the `std::fs` entry points. Keep the list scoped: an `#[allow]` that becomes routine stops being a signal.\n\nSanctioned boundaries needing an explicit `#[allow]`, each a deliberate marker: the\nstore/index actors; `config.rs` -- but ONLY `load_raw_config` (main.rs:243) is pre-runtime; `prepare_data_dir` (:410) and `bootstrap_headless_with_store` (:415) run INSIDE run_daemon and are exempt as PRE-SERVING, not as pre-runtime. Copy that reason correctly into the `#[allow]`;\n`backup.rs` (offline CLI); and path\nsetup at startup. The IPC SOCKET BIND is NOT in this group -- supervisor.rs:927-932 restarts\nserve_with_shutdown in a backoff loop, so bind_owner_only recurs on a live daemon and must be\nOFFLOADED, not allowed. `load_operator_recipe` (main.rs:451) must ALSO be offloaded, not allowed -- see the unbounded-scan note below. The only sites that may be allowed in place are the fixed, single-shot ones: config.rs's three entry points and the offline backup CLI.\n\nCRITICAL -- this is exactly the class from `checks-that-pass-without-proving`: PROVE the lint\nfails. Add a deliberate violation, confirm CI goes red, then remove it. A denylist that\nsilently matches nothing is worse than none, because it reads as enforcement.\n\nAcceptance:\n- clippy.toml exists; `cargo clippy --workspace --all-targets -- -D warnings` is green.\n- The failure path is demonstrated in the PR, not asserted.\n- Every `#[allow(clippy::disallowed_methods)]` carries a one-line reason.\n\nTHIS BEAD OWNS THE AUDIT. ADR-0021 deliberately stopped enumerating exempt sites after four\nreview rounds each found another live blocking path its \"exhaustive\" list had missed. Derive\nthe inventory here; do not inherit a list from the ADR.\n\nThe audit must cover at least these, all confirmed live-while-serving:\n- `std::fs::*` entry points (the obvious set).\n- `std::path::Path` METADATA methods -- `exists`, `metadata`, `is_file`, `is_dir`,\n `read_dir`, `canonicalize`. These are blocking syscalls that match NEITHER an `fs::` grep\n NOR a `std::fs` denylist. Confirmed live: `hook.exists()` at daemon/src/preflight.rs:650,\n reached by a normal `Request::Preflight` (served at ipc.rs:816).\n- The supervised IPC rebind. `supervisor.rs:927` is a restart loop that re-invokes the task\n factory at :932, so `bind_owner_only` (ipc.rs:406) and its filesystem work RECUR during\n recovery while Nostr and maintenance stay live -- it is not a one-shot startup cost, and\n an earlier draft of the ADR wrongly classified it as one.\n- `ipc.rs:471`'s shutdown unlink, which runs before in-flight handlers drain at :487.\n- NOT this bead: the store crate's synchronous backup entry point. lnrent-7dw CREATES that\n helper, so 7dw OWNS adding its own exact path to this denylist. Deliberately not a blocker\n on this bead -- a cheap, valuable lint should not wait behind a crate split.\n\nACCEPTANCE IS NOT SYMMETRIC (review, round 6). \"OFFLOAD or EXEMPT-WITH-REASON\" for every\nsite would let an implementer `#[allow]` the three LIVE paths above and close this bead with ADR-0021's rule left false. So: sites reachable while serving MUST be offloaded, not exempted -- `hook.exists()` becomes an async existence check, and the supervised rebind runs off-worker. Only `ipc.rs:471`'s shutdown unlink may be exempted, and NOT because it is bounded -- it is not: `remove_file` on a wedged or networked FS can block indefinitely and cannot observe cancellation. It is exempted as an ACCEPTED RESIDUAL (a filesystem wedged enough to hang it has already made the daemon unable to commit), and its `#[allow]` must say that rather than claiming a bound. Startup-only sites may be exempted with a reason. An audit that silently omits a category is the same defect\nas a lint that matches nothing, and an acceptance criterion satisfiable by annotation alone is the same defect one level up.\n\nDENY THE OUTER BACKUP API, NOT JUST THE INNER HELPER (review, round 8). `pub mod backup`\n(daemon/src/lib.rs:8) exposes `pub fn backup(..)` (daemon/src/backup.rs:183) and its restore\ncounterpart. Denying only the store crate's sync helper does nothing at that layer: the\nhelper call sits INSIDE backup.rs behind its `#[allow]`, so async code calling\n`lnrentd::backup::backup(..)` performs `VACUUM INTO` plus synchronous filesystem work on a\nworker with no lint firing. The public wrapper launders the disallowed work.\n\nEither put the outer `backup::backup` / `backup::restore` paths on the denylist and `#[allow]`\nonly the synchronous CLI edge that legitimately calls them, or reduce their visibility to\n`pub(crate)` plus a single sanctioned entry point. Whichever is chosen, the deliberate door\nmust be ONE named place, not a public module surface.\n\nTHE CONSTRUCTOR LIST CANNOT BE PROVEN COMPLETE -- SAY SO (review, round 9). This bead has now\nsupplied a wrong enumeration THREE times: first \"four\" constructors (there are six `open*`),\nthen a derivation `grep -E \"pub fn open\"` that finds those six and MISSES two more --\n`Connection::from_handle` and `Connection::from_handle_owned` are `pub unsafe fn` and do not\nstart with `open` (rusqlite 0.31 src/lib.rs:947, :979). Eight total.\n\nCorrect derivation for 0.31, and RE-RUN it after the 0.40 bump:\n\n grep -nE '^\\s*pub (unsafe )?fn (open|from_handle)' <rusqlite>/src/lib.rs\n\nBut do not write \"every constructor is covered\" in the PR. A path-matching denylist is closed\nover the paths you listed and nothing else; a future rusqlite can add a ninth and the lint\nstays green. State the covered set explicitly and note that completeness rests on re-deriving\nat each version bump -- claiming more is the exact defect this bead exists to prevent, and\nthree wrong enumerations in one bead is the evidence.\n\nAlso offload `load_operator_recipe` (main.rs:451) rather than allowing it: `Recipe::load_all`\n(recipe.rs:153-166) is unbounded in both directory count and manifest size, so it is\ndata-scaled work, not the fixed single-shot startup cost the exemption is for.\n\nWRAPPER LAUNDERING IS A GENERAL HOLE, NOT A LIST (review, round 11). Once a primitive inside a\nsanctioned helper carries `#[allow]`, ANY public wrapper around it is callable from async code\nwith no disallowed method at the call site: `lnrentd::backup::backup`, `Recipe::load`,\n`Recipe::load_all`, `config::prepare_data_dir` are the ones found so far, and enumerating them\nhere would repeat this bead's three-strikes history with the rusqlite constructors.\n\nState the principle in the PR instead: an `#[allow]` on a primitive is only safe if the\nenclosing function is not reachable from a serving path. So for each `#[allow]` added, either\n(a) the enclosing function is private / crate-visible and provably called only pre-serving or\noffline, or (b) the enclosing PUBLIC function goes on the denylist too, allowed at its single\nsanctioned edge. Reducing visibility is usually cheaper than another denylist entry, and unlike\na path list it cannot silently miss a wrapper someone adds later.\n\nALSO ASSESS THE THREE GRANDFATHERED MUTEX SECTIONS (codex review, PR #78). ADR-0021 permits\nshort in-memory `std::sync::Mutex` sections under constraints -- no I/O, no unbounded\niteration, nothing growing with request volume -- and names three existing sections that do\nNOT meet them:\n- relay_status.rs:77 -- clones the whole relay vector under the lock.\n- alerts.rs:248 -- inserts into an unbounded `last_sent` map keyed by (kind, subject).\n- nostr_engine.rs:1839 -- `retain()` scan over a vector sized by in-flight requests.\nNone is a live hazard at current scale, and none of the four delivery beads changes them. For\neach: bound it, or record why it is tolerated with the scale at which it stops being tolerable.\nLeaving them unassessed would let these beads close with the ADR's own exception list\ncontaining its counterexamples.\n\nTHIS BEAD OWNS THE CLASSIFICATION, NOT JUST THE LINT (PR #78 round 5). ADR-0021 now states only\nthe CRITERION -- fixed-cost + pre-serving is exemptible; recurring, data-scaled, or\nreachable-once-serving must be offloaded -- because four review rounds each produced a different\nwrong site classification from the ADR. Derive the sites here; do not inherit a list.\n\nKnown misfilings from those rounds, as INPUTS not a complete set:\n- `load_operator_recipe` (main.rs:451) was filed as bounded; `Recipe::load_all`\n (recipe.rs:153-166) walks read_dir with no count limit and read_to_strings every manifest.\n- The IPC bind (ipc.rs:406) was filed as one-shot startup; supervisor.rs:927-932 restarts\n serve_with_shutdown in a backoff loop, so it rebinds on a LIVE daemon.\n- `bootstrap_headless_with_store` (main.rs:415) was filed as fixed-cost; it reaches\n `read_secret_file_bytes`, whose `file.read_to_end(&mut bytes)` (config.rs:1746) is unbounded.\n Either bound that read (a secret file has a sane max size) or offload the call.\nThe exemption must be applied consistently: if an unbounded read is tolerated at startup, say\nwhy in the `#[allow]`; do not exempt one unbounded startup read while offloading another.","status":"open","priority":2,"issue_type":"task","created_at":"2026-08-01T18:32:54.615703187Z","created_by":"master","updated_at":"2026-08-03T19:25:15.160470784Z","source_repo":"lnrent","source_repo_path":"/home/master/projects/lnrent","compaction_level":0,"original_size":0} |
There was a problem hiding this comment.
When implementing lnrent-njv, the final amendment permits an unbounded startup read to remain on the runtime worker if its #[allow] explains why, contradicting both the immediately preceding requirement to bound or offload this read_to_end and ADR-0021's fixed-cost-plus-pre-serving exemption criterion. Because amendments are the guidance implementers are told to prioritize, this can let the bead close while data-scaled blocking work still violates the decision; remove the tolerance alternative and require bounding or offloading.
AGENTS.md reference: AGENTS.md:L63-L65
Useful? React with 👍 / 👎.
Operator directive: keep everything async, because mixing sync and async is what
lets a later edit silently park a runtime worker. This records the rule, the
sanctioned pattern, and seven beads that deliver it. No code changes.
The achievable rule is narrower than "100% async" and the ADR says so: sqlite has
no async implementation in Rust — every crate advertising one (tokio-rusqlite,
deadpool-sqlite, sqlx) is a thread-and-channel wrapper over the same blocking
calls. So the rule is "once serving, no blocking I/O and no unbounded-duration
work on a tokio worker", with in-memory mutex sections a named exception carrying
constraints rather than a duration guarantee.
tokio-rusqlite was evaluated and REJECTED as a dependency while its design was
adopted: its implementation is exactly the OS-thread-plus-channel actor proposed
here. It requires rusqlite ^0.37 against our 0.31, and libsqlite3-sys declares
links = "sqlite3", so cargo forbids coexistence — adopting it forces a rusqlite
major bump as a side effect of a concurrency fix. Its channel is also unbounded,
which for a synchronous=FULL money DB turns a slow fsync into unbounded queue
growth with no backpressure.
Enforcement is two mechanisms because neither suffices. The obvious one — re-export
Transaction, withhold Connection — was REFUTED with a build probe: an associated-type
projection reaches every constructor from a crate with no dependency on the sqlite
crate at all:
type C = <store::Transaction<'static> as std::ops::Deref>::Target;
let _ = C::open("/tmp/x.db"); // compiles
So the boundary is a Txn newtype that does not Deref to Connection, plus a
clippy.toml denylist for what no crate boundary can catch. The ADR states plainly
what that does NOT close: disallowed-methods matches listed paths only.
Beads: lnrent-skk (store actor onto its own thread), lnrent-68f (index actors),
lnrent-njv (denylist + the blocking-call audit), lnrent-7dw (crate boundary),
lnrent-hrm (dependency upgrades), lnrent-7w1 (hkdf/sha2 under a funded key),
lnrent-73r (what fedimint v0.11.1's graph pins).
flake.nix gains cargo-outdated; README's ADR range stops being hand-maintained.
Addresses PR #78 bot review, round 1 — all four findings verified before fixing:
- codex P2: the mutex exception named alerts.rs / nostr_engine.rs as endorsed
while stating constraints they violate (alerts.rs:248 inserts into an unbounded
map under the lock; nostr_engine.rs:1839 retains over a request-sized vector).
The ADR now lists all three non-conforming sections explicitly as grandfathered
rather than conforming, and lnrent-njv owns assessing them. An exception list
that quietly contains its own counterexamples is a false contract.
- CodeRabbit (Major): obligation 1's delivery guarantee did not state where it
begins. It starts at SUCCESSFUL enqueue — a caller cancelled before
tx.send().await returns is owed nothing.
- CodeRabbit (Major): the SQL-site inventory command was anchored on `rusqlite::`
and so missed unqualified use after `use rusqlite::Connection` — backup.rs:619
is exactly that. Fixed, and marked a lower bound for sizing rather than proof of
coverage, since four derivation commands in this ADR's history were each wrong.
- CodeRabbit (Minor): lnrent-68f still cited skk's "bounded, off-worker join",
which skk's own round-11 revision superseded with a completion signal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
d147dff to
34a0b0d
Compare
|
Round 8 — head codex P2 (frozen delivery-bead count) — FIXED, and the sweep found two more than were reported. Correct: round 7 removed the frozen list from the Status header and left a frozen count thirty lines below ("None of the four delivery beads changes them"). Half-fixing is the recurring failure on this PR, so I swept instead of patching the cited line — the same frozen count also sat in codex P2 (njv permitted the unbounded read to stay) — FIXED, and this one could have shipped a real violation. The tolerance alternative is gone: "tolerated with a reason" is now explicitly not an option for an unbounded read, because the criterion exempts fixed-cost pre-serving work only. A read that scales with file size gets bounded or offloaded, never Worth noting these two are the same defect from opposite directions: one left a stale number after fixing the list, the other left a permissive escape hatch after adding a requirement. Both are corrections that didn't delete what they superseded — which is now the single most repeated mistake across this PR's eight rounds, and the reason I sweep rather than patch the cited line. Round tally: 4 → 1 → 3 → 1 → 4 → 1 → 1 → 2. CI green on every round so far. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Records the decision to keep blocking work off tokio's worker threads, the one sanctioned pattern for reaching sqlite, and seven beads that deliver it. No code changes — this is the decision record plus its work graph.
The achievable rule is narrower than "100% async", and the ADR says so up front: sqlite has no async implementation in Rust, so the rule is "once serving, no blocking I/O and no unbounded-duration work runs on a tokio worker", with in-memory mutex sections as a named exception carrying constraints rather than a duration guarantee.
The ADR opens with Status: DECIDED, NOT YET BUILT and names the delivering beads, because none of it is in the tree yet.
Two things worth reviewing closely
tokio-rusqlitewas evaluated and rejected as a dependency while its design was adopted. Its implementation is the OS-thread-plus-channel actor proposed here. Rejected because it requiresrusqlite ^0.37against our0.31andlibsqlite3-sysdeclareslinks = "sqlite3"— cargo forbids coexistence, so adopting it forces a rusqlite major bump as a side effect of a concurrency fix. Its channel is alsounbounded, which for asynchronous=FULLmoney DB converts a slow fsync into unbounded queue growth with no backpressure.The obvious enforcement design was refuted with a build probe. Re-exporting
Transactionwhile withholdingConnectiondoes not work — an associated-type projection reaches every constructor from a crate with no dependency on the sqlite crate at all:Hence a
Txnnewtype that does notDereftoConnection, plus aclippy.tomldenylist for what no crate boundary can catch. The ADR states plainly what that does not close:disallowed-methodsmatches listed paths only.Beads
lnrent-skklnrent-68flnrent-njvlnrent-7dwDerefnewtypelnrent-hrmlnrent-7w1lnrent-73rTrade-offs accepted
join();timeout(spawn_blocking(|| join())), which can't be abandoned and which runtime teardown then waits on). The ADR states three obligations and records the failed drafts;lnrent-skkowns choosing and testing one.lnrent-njvowns the audit, derived rather than recalled.lnrent-73ralso owns a doc repair it uncovered:daemon/Cargo.toml:98-99and ADR-0018 both state that no fork commit touches a compiled crate.cargo tree -i fedimint-tpeshows otherwise. The defensible claim is narrower (the changed function is uncalled), and this does not weaken the standing decision to keep the fork.Test plan
nix developevaluates with thecargo-outdatedaddition;cargo outdatedrunsfile:linecitation in the ADR and beads verified against the treeDerefbypass claim proven by a three-crate build, not assertedclippy::disallowed_methodsandawait_holding_lockconfirmed warn-by-default on this toolchainNo code changed, so the money-path suites are unaffected; CI runs them regardless.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EzBbjDBkhCjbEefQrHWdGq
Summary by CodeRabbit