Skip to content

aimdb-sync: drop try_set, finish the doc pass #212

Description

@lxsaah

Revised 2026-09-16 against f1fea6c, after the aimdb-sync rework
(#226#239) landed under this issue. Line references refreshed throughout.
Decisions taken since the last revision:

  • §2 (pre-resolve the producer) is dropped. It would reverse the
    lazy-producer contract refactor(sync): make the fork check unavoidable by construction #235 established. Its one leftover, a doc item, moved
    to §3.
  • §1 must merge before aimdb-sync 0.6.0 is published. SetTimeout and
    try_set_value never shipped, so removing them is free until then.
  • No new measurement. The figures in fork.rs go the way of the other
    claims (§4).
  • # Panics stays per method, and now covers attach() too (§3).

New since the last revision: tests/fork_safety_test.rs uses try_set; the
README also documents set_with_timeout and several DbError variants that no
longer exist.

Context

#208 removed the internal channel bridge in aimdb-sync: blocking calls now go
straight to the runtime through the block_on seam, and SyncProducer holds
a Weak<Runtime> plus the record key, resolving the record on every set().

Two things were deliberately left out of that PR as follow-up work: pre-resolving
the producer, and dropping try_set now that no buffer can refuse a value. The
first has since been dropped (§2). The documentation items the review raised are
collected here too — along with the crate README, which the review never opened
and which still describes the deleted bridge.

The sections are commit-sized and listed in the order they should land: §1
deletes surface whose wording §3 would otherwise have to fix, and §4 rewrites the
one file no one has looked at since the bridge existed. §5 is optional and must
not hold the branch. Line references are as of f1fea6c.

This issue is self-contained. Every claim it touches is removed, not
remeasured — there is no benchmark work queued behind it and none is required to
close it. See §4.

1. Drop try_set

AimDb::produce is infallible on the push (aimdb-core/src/builder.rs:988-990)
and every buffer today overwrites rather than refusing, so set() and
try_set() are the same operation with different error mapping.

Timing: merge before aimdb-sync 0.6.0 is published (crates.io is at 0.5.0).
Of the three things removed, only try_set shipped in 0.5.0 — it returned
DbResult then. SyncError::SetTimeout and try_set_value are new in the
unreleased 0.6.0, so until it is published, removing them breaks no one.

  • Remove SyncProducer::try_set (producer.rs:147-188) and try_set_value
    (producer.rs:243-246).
  • Remove SyncError::SetTimeout (error.rs:36-38), its kind() arm
    (error.rs:76) and its test assertion (error.rs:121).
  • Drop the try_set example from SyncProducer's type docs (producer.rs:31-35),
    the bullets at lib.rs:16, lib.rs:146 and lib.rs:160-161, and the feature
    line in examples/sync-api-demo/src/main.rs:171.
  • Tests: test_runtime_shutdown_error_non_blocking
    (tests/integration_test.rs:268) loses its producer half (:277);
    integration_test.rs:185 switches to set;
    try_set_value_is_non_blocking_and_produces (tests/settable_integration.rs:67)
    goes with try_set_value; tests/fork_safety_test.rs drops refused_try
    (:55-58, used at :70, and "the two publishes" at :60).
  • CHANGELOG: record the removal of try_set under [Unreleased] → Changed (breaking). Record nothing for the other two — instead drop try_set_value
    from the unreleased Added entry (CHANGELOG.md:138), and try_set() from the
    fork-check entry (CHANGELOG.md:87).
  • fork.rs:18 justifies pthread_atfork with a try_set timing. §4 rewrites
    that paragraph.
  • aimdb-sync/README.md:146-148,274,287 documents try_set against the
    pre-Simplify the implementation of aimdb-sync #208 DbError::SetTimeout. Leave it to §4 — the whole file needs
    rewriting, not three line fixes.

This also removes an incidental cost: try_set calls db.producer(&self.key)?
per call (producer.rs:183), which takes impl Into and so clones the key every
time.

TryProduceError::Full stays in core; only the facade wrapper goes. No
buffer anywhere in the tree overrides WriteHandle::try_push
(aimdb-core/src/buffer/traits.rs:170) — the only overrides are the two test
doubles in that file's own mod tests (:444, :455) — so Full is
unreachable at every layer today, not just this one. The core variants stay
because they are the extension point a bounded, non-overwriting buffer would be
built against, and their docs should be corrected to say so
(aimdb-core/src/typed_api.rs:163-180 still describes backpressure). The facade
wrapper is the opposite trade: it duplicates set() for as long as no such
buffer exists, and re-adding it if one lands is cheap.

2. Pre-resolve the producer — dropped

Decided against on 2026-09-16. Since this issue was filed, #235 made lazy
creation the crate's contract:

  • handle.producer() touches neither the database nor the runtime thread
    (handle.rs:344-357).
  • test_error_propagation (tests/integration_test.rs:407) pins that an
    unregistered key yields a producer and set() reports it.
  • The unreleased CHANGELOG records that a forked child's refusal lands on the
    first set() (CHANGELOG.md:85-87).

Pre-resolving would reverse all three, for less than it used to buy: set() must
still go through Runtime::db() for the fork and shutdown checks, so only the
keyed lookup would go. The one leftover is documentation — producer() cannot
fail and does not say so — and that is now in §3.

3. Documentation left over from the #208 review

set() describes behavior it no longer has

aimdb-sync/src/producer.rs:112-121 still promises blocking, guaranteed eventual
delivery, and a buffer-full error. None of the three can happen. set() checks
the runtime (RuntimeShutdown, ForkedChild), looks the record up
(Db(RecordKeyNotFound), Db(TypeMismatch)), and pushes, which cannot fail.
Same wording survives at producer.rs:28 and producer.rs:138.

The matching bullet in aimdb-sync/src/lib.rs:15 ("Blocking send, waits if
channel is full") is the last mention of the bridge's channel in the crate's
docs. lib.rs:157-159 needs the same fix: the error half is right, the blocking
half is not.

producer() cannot fail, and does not say so

AimDbHandle::producer() (handle.rs:320-343) returns SyncResult but always
succeeds — see §2. Its rustdoc should say so and name where key, type and fork
errors surface instead: the first set(). Today only SyncProducer::check()'s
doc hints at it.

# Panics on the blocking entry points

get and get_with_timeout call Handle::block_on (consumer.rs:125, :170);
get_latest and get_latest_with_timeout reach it through get_catch_up. It
panics with "Cannot start a runtime from within a runtime" if the calling
thread is already driving tasks. Both attach() methods have the same panic:
they wait for startup with blocking_recv (handle.rs:177), which panics inside
a runtime too (AimDbBuilderSyncExt::attach, handle.rs:53;
AimDbSyncExt::attach, handle.rs:88).

The crate now states this once, at crate level (lib.rs:204-207). Give each of
those six methods its own # Panics section as well, consistent with that note.
set() no longer enters the runtime, so producers need none.

Whether get() should also try reader.try_recv() first and enter block_on
only on BufferEmpty is a separate question and explicitly not part of this
issue; the docs are correct under either outcome, since such a fast path narrows
when the panic fires without removing it.

Consumer error docs

  • get_latest_with_timeout's # Errors (consumer.rs:280-283) omits
    SyncError::Db, which get_latest lists (consumer.rs:231-235).
  • Neither method says that BufferLagged is skipped rather than returned —
    which is the behavior Simplify the implementation of aimdb-sync #208 introduced.
  • get_latest_with_timeout's # Arguments (consumer.rs:278) says the timeout
    bounds the first value; add that the drain after it is unbounded. In practice
    the drain outruns a hot producer (a ring read is cheaper than a keyed produce),
    so this is a doc fix, not a code fix.

CHANGELOG: lag now reaches callers

Dropping the forwarder changed observable behavior, not just the API: lag now
surfaces to callers of get() / get_with_timeout() / try_get() as
SyncError::Db(DbError::BufferLagged { .. }), where the old forwarding task
swallowed it. The rustdoc says so (consumer.rs:99-100); the CHANGELOG's #200
entry (CHANGELOG.md:155) does not. Add one bullet there, same unreleased 0.6.0
section.

Small stuff

  • Typos: "occuring" (consumer.rs:233, consumer.rs:319,
    tests/integration_test.rs:361), "occured" (consumer.rs:235,
    consumer.rs:348), "succesfully" (consumer.rs:263).
  • consumer.rs:169: the async { ... .await } wrapper is redundant —
    block_on(tokio::time::timeout(...)) works directly.
  • handle.rs:370 uses # Errors (wrapped in SyncError::Db); the rest of the
    crate uses a plain # Errors.

4. Rewrite aimdb-sync/README.md and clear the last performance claims

The #208 review never touched this file, and neither did the rework since.
It still documents the channel bridge as the crate's architecture, and API that
no longer compiles:

  • Lines 26-30 — a Channel Bridge (tokio::sync::mpsc + std::sync::mpsc) box
    in the architecture diagram, the defect the reviewer caught and had fixed in
    lib.rs.
  • Lines 136, 387set_with_timeout, removed in aimdb-sync: collapse the bridge onto the block_on seam #200 (CHANGELOG.md:156).
  • Lines 146-148try_set ("Channel full or error"); goes with §1.
  • Lines 317-328 — a Channel Capacity section for producer_with_capacity /
    consumer_with_capacity, removed in aimdb-sync: collapse the bridge onto the block_on seam #200 (CHANGELOG.md:157).
  • Lines 274-290DbError::SetTimeout, DbError::RuntimeShutdown,
    DbError::RecordNotFound, DbError::AttachFailed. None exist; the facade's
    own failures are SyncError variants.
  • Lines 450-452 — Optimization Tips: "Use try_* methods" (wrong for
    producers once §1 lands), "Channel Capacity", and runtime_threads, which is
    not a setting.

The performance claims are the part that matters

Four claims are left in the crate. All four go in this pass:

  • README.md:444"Channel crossing adds ~1-10μs latency".
  • README.md:446"Memory: One tokio::mpsc channel per producer, one
    std::mpsc channel per consumer"
    .
  • src/lib.rs:135"Latency: Excellent for <50ms target, not suitable
    for hard low-latency requirements"
    . It is the only bullet under
    ## Performance (lib.rs:133), so the heading goes with it.
  • src/fork.rs:18-21"Measured: try_set is 121 ns and
    std::process::id() is 321 ns"
    . Added by feat(sync): tell a forked child the truth about its inherited handles #230, after this issue was filed.
    It cites no benchmark, and it names try_set. Keep the reasoning — a relaxed
    atomic load per publish instead of a pid read — and drop the figures.

The first two describe a bridge that no longer exists; the other two cite no
benchmark. This is the same family as the ~100–500μs per operation claim #208
removed from the crate docs, and they go the same way: delete them rather than
correct them.

There is no number to replace them with — aimdb-bench has no aimdb-sync
target — and none should be estimated. Building one is out of scope here and
is not queued behind this issue
; the crate is expected to ship claim-free.
If a figure ever returns to these docs it must cite the benchmark that produced
it. Do not carry a placeholder forward.

Rewrite the README against the current design: the block_on seam, set()
pushing straight into the record's buffer, SyncConsumer holding a Reader
and blocking only when a read has to wait, the &mut self / no-Clone consumer
signature, SyncError, and the shutdown contract the CHANGELOG already records.
The Optimization Tips section should survive only where its advice is still true.

5. Optional: cover the mid-drain lag path

drain_remaining (consumer.rs:343) skips BufferLagged and keeps draining
(:347) — the fix for the review finding that a lag mid-drain used to end the
loop early and return a value that wasn't the latest. It still has no test: the
two get_latest tests (tests/integration_test.rs:341, :383) produce every
value before reading, so a lag can only be hit in get_catch_up. Triggering it
needs a producer overrunning the reader between the first read and the drain,
which is hard to make deterministic — worth doing only if it can be done without
a racy test. Do not hold the branch for it.

Acceptance criteria

  1. try_set, try_set_value and SyncError::SetTimeout are gone, merged before
    aimdb-sync 0.6.0 is published. The CHANGELOG records the removal of
    try_set and no longer mentions the other two.
  2. AimDbHandle::producer()'s rustdoc says creation cannot fail and that key,
    type and fork errors surface on set(). test_error_propagation and
    test_runtime_shutdown_error pass unchanged.
  3. No producer doc describes blocking, a channel, or a buffer-full error.
    get, get_with_timeout, get_latest, get_latest_with_timeout and both
    attach() methods have # Panics.
  4. The CHANGELOG's [Unreleased] section records that BufferLagged now reaches
    callers of get/get_with_timeout/try_get.
  5. grep -i channel aimdb-sync/README.md returns nothing outside the shutdown
    plumbing, no removed API (set_with_timeout, *_with_capacity, try_set)
    is documented as callable, and no latency or memory-footprint claim or
    figure remains anywhere in the crate
    README.md:444,446, src/lib.rs:135
    and src/fork.rs:18-21 included.
  6. make check green, including the --no-default-features no_std build that
    Add std feature gate and no_std clip #205 established.

Revised 2026-08-15. Folded in the src/lib.rs:133 <50ms latency claim
(previously tracked nowhere) and the CHANGELOG note about BufferLagged
reaching callers (raised in the #208 review, never recorded). Measuring
aimdb-sync overhead was considered and deferred indefinitely — no bench
target is planned, so this issue removes the claims outright and closes on its
own.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions