Skip to content

refactor(station): ask the producer whether publishing still works - #10

Merged
lxsaah merged 2 commits into
weather-station-cppfrom
feat/station-uses-producer-check
Aug 27, 2026
Merged

refactor(station): ask the producer whether publishing still works#10
lxsaah merged 2 commits into
weather-station-cppfrom
feat/station-uses-producer-check

Conversation

@lxsaah

@lxsaah lxsaah commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Why

StationHandle::is_closed kept its own copy of aimdb's fork generation and compared it against the current one:

self.closed.load(Ordering::Acquire) || aimdb_sync::fork::forked_since(self.made_in)

That needs aimdb_sync::fork to be public, and it is not — aimdb-sync made the module crate-private in aimdb#235. This crate does not currently compile against aimdb main, with three E0603s in weather-station/src/handle.rs.

What

It now asks the producer it publishes through:

self.closed.load(Ordering::Acquire) || self.temperature.check().is_err()

SyncProducer::check() is added upstream in aimdb-dev/aimdb#236 for exactly this. The made_in field goes with it, and the word "fork" survives here only in a sentence explaining what the state means.

Three things improve beyond compiling again

The station no longer knows what a fork is. It asks a question about publishing and gets an answer about publishing. The mechanism stays aimdb's business.

The answer cannot drift. There were two checks that happened to agree — this one, and the one inside set(). Now there is one, used by both.

It catches more. A handle dropped without shutdown() left closed false and the generation current, so is_closed() reported open while every publish failed with RuntimeShutdown. The producer catches that too.

What stays

closed: AtomicBool remains. While detach_timeout is joining, the Arc is briefly alive and the producer would still report usable, so the flag still covers that window.

The lock-ordering argument is unchanged — Weak::upgrade takes no lock, so a getter under the GIL still cannot block behind a shutdown. But three comments describing is_closed as "reads an atomic" would have read as stale to the next person checking that argument, so they now say "takes no lock".

Verification

  • weather-station: 16 tests green under both tokio-runtime,rustls,sync and tokio-runtime,sync; clippy clean on both.
  • weather-station-cpp and weather-station-py check and clippy clean.
  • make spike-cpp all green, including the round that matters:
after fork(), the child has no runtime thread
  ok    the parent keeps publishing across the fork — 2 of the parent's 2 readings arrived
  ok    a forked child is told the station is closed — is_closed() reported closed
  ok    a forked child's publish is refused, not silently dropped — threw
  ok    no phantom reading reached the broker

Merge order

Depends on aimdb-dev/aimdb#236. That lands first — this does not compile without it.

🤖 Generated with Claude Code

lxsaah and others added 2 commits August 27, 2026 18:44
`is_closed` kept its own copy of aimdb's fork generation and compared it against
the current one. That needed `aimdb_sync::fork` to be public, and it is not —
the module is crate-private upstream, so this crate did not compile against
aimdb main at all.

It now asks the producer it publishes through, via `SyncProducer::check()`,
added upstream for this. The `made_in` field goes with it.

Three things improve beyond compiling again. The station no longer knows what a
fork is; it asks a question about publishing and gets an answer about
publishing. The answer cannot drift from what a publish does, being the same
check rather than one running alongside it. And it now also catches a handle
dropped without `shutdown()`, where the counter comparison reported open while
every publish failed with `RuntimeShutdown`.

`closed` stays: while `detach_timeout` is joining, the `Arc` is briefly alive
and the producer would still report usable.

The lock-ordering argument is unchanged — `Weak::upgrade` takes no lock — but
three comments describing `is_closed` as "reads an atomic" would have read as
stale to the next person checking that argument, so they say "takes no lock".

Verified: `make spike-cpp` all green, including all four checks of the fork
round, and `is_closed()` still reports closed in the child.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The doc comments in the three files this PR touches had grown essayistic:
rationale restated at length, measurement anecdotes, and history about how
each layer arrived at its shape.

Trimmed to the non-obvious "why" a reader cannot get from the code — the
GIL lock ordering, why the shutdown guard drops before the join, why the
on_start gate exists, why `is_closed` asks the producer, why the C sink can
never be uninstalled. Comment-only; no behaviour, signatures, or intra-doc
links change, and rustfmt is clean on all three.

One correction along the way: the C door's module docs said a null pointer
and a dangling one were the detectable arguments. A dangling pointer is not
detectable; a non-UTF-8 `const char*` is, and the code checks it.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UcZHK1b9thLfB6NzeVxQdt
@lxsaah
lxsaah merged commit fafe259 into weather-station-cpp Aug 27, 2026
@lxsaah
lxsaah deleted the feat/station-uses-producer-check branch August 27, 2026 19:11
lxsaah pushed a commit that referenced this pull request Aug 28, 2026
This branch is nine commits behind `weather-station-cpp`, and one of them —
`fafe259` (#10) — tightened the doc comments throughout
`weather-station-py/src/lib.rs`. The previous commit was written against the
older text, so merging it would have reverted that work in every block it
touched, and left the blocks it did not touch looking like a revert too.

Re-applied the design-050 change on top of #10's version of the file instead.
No behaviour change from the previous commit: `PyLogger` and `init_logging` are
the same, and the module-doc note about the bridge is rewritten in #10's
register rather than the older one. The diff against #10 is now confined to the
imports, the module-doc tail and the bridge itself — `PyStation` is untouched.

#10's change to this crate was comment-only, so nothing here depended on it.

`make spike` still reports all checks passed; `make clippy`, `make test` and
`make fmt-check` clean.

Note for whoever merges: this branch is still missing the other eight commits
on `weather-station-cpp`, including the C door itself. Which way those branches
should converge is a topology decision, not one this commit makes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QgaHbjCPgkNS6Y34jxqVYH
lxsaah pushed a commit that referenced this pull request Aug 28, 2026
Brings the design-050 migration of the pyo3 door onto the branch that carries
both. `weather-station-cpp` was nine commits ahead, so its copy of the Python
door was still installing the interpreter's global `tracing` subscriber while
its `weather-station` had already moved to the `log` facade.

Three conflicts, all in files both sides edited:

- `weather-station-py/src/lib.rs` — cpp's side was #10's doc tightening, which
  the py branch had already been rebuilt on top of, so the migrated version is a
  strict superset. Took it whole; the result is byte-identical to the py
  branch's file.
- `weather-station/Cargo.toml`, `default` — kept `native-tls`. The py branch
  predates CR-11 and had no TLS backend to name; dropping it here would have
  left the default build without one.
- the same file's `log` feature comment — one word apart; kept "destination",
  which is the design's vocabulary.

One thing neither branch could have got right alone: `weather-station-py` takes
`weather-station` with `default-features = false`, and on the py branch that was
harmless because a TLS backend was not yet selectable — `rumqttc` named
`use-native-tls` directly. Merged, it means no backend is selected at all and
`mqtts://` would have failed at run time with nothing failing at build time.
Named `rustls` explicitly, for the reason CR-11 gives and which describes this
crate exactly: a build that must not hand its host process a second OpenSSL —
"an FFI layer shipped as a shared library, above all". The C door made the same
call for the same reason. `cargo tree -p weather-station-py` links no OpenSSL
(`openssl-probe` is `rustls-native-certs`' cert-store locator, not a link to
OpenSSL) and still contains neither `tracing` nor `tracing-subscriber`.

That is the one judgement call in this merge rather than a mechanical
resolution; `native-tls` is a one-word change if the wheel should keep it.

Verified on the merged tree: `make clippy`, `make test`, `make fmt-check` clean;
`make spike` and `make spike-cpp` both report all checks passed against a live
broker.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QgaHbjCPgkNS6Y34jxqVYH
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants