Repository navigation
Conversation
|
Thank you, this is thorough, and the "Known limit" and design-choice sections make it much easier to review. CI is green on all three platforms. We're reviewing it now (the full suite plus live tests), and I'll come back here with findings and answers to Q1 and Q2. |
|
Thanks again, the review is done. The checks pass, and so does the full suite on your head and merged into current main: unit tests, 1. A notice can reach a later conversation that took the sender's name (
Names like 2. The backfill's "sent from this machine" rule has the same gap ( 3. A full table scan on every presence. 4. A notice can be pushed twice. Smaller: an empty first line is quoted as Tests we'd like added: a new session taking over a lapsed name, held mail expiring through Q2: relying on the |
From the review of reason-machines#153: - Mail sent or queued here keeps the sending session's key; a notice goes to the session under that name only if it still has that key. Notices to another machine can't check this without a wire change; a comment says so. - Mail from before notices is put down as sent here only if each other machine of the person's has said what sessions it has and none named it. - That search reads an index of just those rows, parses the presences once, and saves what it found in one go. - deliver_notices pushes only a notice still unread and not on its way. - An empty first line isn't quoted; dropping a session's mail is one transaction; the forget-grace override is set only for the e2e case that needs it. - Tests: a new session taking a lapsed name, held mail expiring, the ack seen flag, and the two above.
|
Thank you for the careful review and the repro. b8f3f4f and 438fd83 have the fixes. They are two new commits on top of be1a78d, with no rebase. For findings 2 and 4 I used your approach. I have one question on each of them, and you decide. 1. A notice reaches a later conversation with the same name
2. The backfill's "sent from this machine" rule
3. The full table scan on each presence
4. A notice pushed twice
Smaller items
New tests
For each new test, I reverted its fix in a scratch copy, and the test failed. Limits
Q1 and Q2
Checks
|
|
Re-review of 438fd83 is done, and it looks good. All six points are fixed, and each fix has a test that fails when it's reverted. The migration held up in our tests: upgrade from 0.12.13 with 100k rows, killing the daemon mid-backfill seven times, downgrade to 0.12.13 and upgrade again, with the integrity check clean each time. The suite passes on your head and merged into main ( Three small things, all optional, though we'd take them in this PR if you have time:
On your questions: waiting for presences after an upgrade is fine, since the last presences are saved from before the upgrade. The Before merge we'll run our live test with real agents on both sides, and confirm Q1 (the notice on rejected held mail) on our side. |
|
Thank you for the re-review and for the answers. I made all three changes in 68007c8, as one new commit on top of 438fd83, with no rebase. 1. Sessions known only by their process
2. A refusal lost in a race
3. Pre-upgrade local mail
Both new tests fail when I revert their fix. Checks
|
|
Sorry for the churn: main moved quite a bit in the last two days (v0.12.14 to v0.12.17, with changes in |
Whenever this machine drops a message nobody read (its session forgotten a day on, a held one turned down or a week old, one given up on in the outbox or refused after it was queued), its sender gets a short notice: who it was for, when it was sent, its first line, and that it wasn't delivered. A teammate's notice waits in the outbox for the machine the message came from; a notice is never about another notice. It is an ordinary message from the person's terminal, so older engines show it like any other.
Mail queued before the upgrade doesn't say which machine it came from. The daemon now finds it when it starts, from the team and each machine's last presence: the machine a terminal's address names, the sender's only one, the one whose presence names the session, or the one heard from last. Mail sent here is marked so from now on. A notice keeps the sender's machine on its address until it is known where it goes, so one for a terminal on another machine never lands in the terminal here.
A sender here is found whether or not the machine is in a team. Another machine is named only when the address names it, it is the sender's only one, or it is the only one whose presence names the session; otherwise the mail stays unknown and goes without a notice if dropped, rather than on a guess. Each new presence looks again while such mail could still be placed.
Each presence looks again while any mail from before notices is unknown, however the team has changed since. A sender of this person's no longer remembered here may have been a session here: the only other machine of theirs is no evidence it came from there.
… the rollback undoes it
From the review of reason-machines#153: - Mail sent or queued here keeps the sending session's key; a notice goes to the session under that name only if it still has that key. Notices to another machine can't check this without a wire change; a comment says so. - Mail from before notices is put down as sent here only if each other machine of the person's has said what sessions it has and none named it. - That search reads an index of just those rows, parses the presences once, and saves what it found in one go. - deliver_notices pushes only a notice still unread and not on its way. - An empty first line isn't quoted; dropping a session's mail is one transaction; the forget-grace override is set only for the e2e case that needs it. - Tests: a new session taking a lapsed name, held mail expiring, the ack seen flag, and the two above.
…son's names the session
…nder gave up on as it came still tells it
68007c8 to
3b12ab5
Compare
|
Thank you. I rebased the branch onto main at bcf045c. The new head is 3b12ab5. I kept all 9 commits and did not squash them. Conflicts
Fixes from your last review
One behavior change
Open question
Checks on 3b12ab5
|
|
Thank you for the careful rebase and the clear notes. On your two questions:
We'll re-review |
|
We re-reviewed
Tests (fmt and clippy pass):
A few small things we found. None of them block:
If you'd like to pick up the last two, great. Otherwise we'll do them in a follow-up. The new sender notices are a product change, so we're making a final call on them on our side before merging. We'll update you here soon. Thanks again for staying with this through two rebases. |
|
Thank you for the re-review and the detailed test results. I would like to do the last two items: the refusal reason in the notice, and no "Reply if useful" line on notices. I will wait for your final call on the sender notices first. Then I can add the two items to this PR, or I can open a follow-up PR. Please tell me which you prefer. |
|
Thanks! Adding them to this PR is best, so the review and the merge happen once. There's no need to start before our call on the notices. We'll update you here as soon as we've decided. |
Closes #137.
When a receiving machine drops a message that nobody read, the daemon now sends its sender a short notice. The cases with no notice are under "Known limit". This is my attempt at #137. If you'd like anything done differently, please tell me.
What changes
The notice is an ordinary message from
<person>/terminalon the dropping machine. Itsfrom_agentisreason-network, and itsreply_tois the dropped message id.The store writes the notice in the same SQLite transaction as the drop. When a drop makes a notice, the two commit together or not at all.
A new column,
messages.source, records where each message came from. It holds the endpoint id of a teammate machine, or an empty string for local mail. The migration adds the column when the store opens.Mail from before the upgrade has
sourceNULL. The daemon fills it in only on one of these kinds of evidence:An
@machinein the sender address limits the search to that machine. Otherwise the row stays NULL, and its drop sends no notice.The daemon looks for that evidence at start. It looks again on each incoming presence while the last search left an unread row unknown.
The agent view ends with one line about the last message in a batch. When that message is a notice, the line is "From Reason Network on 's machine."
No wire format change.
src/net.rschanges only ACK bookkeeping.src/team.rs,src/invite.rsandsrc/proto.rsdon't change.The notice text, with the time in UTC and the first line cut at 100 characters:
The six points from #137
When the notice goes out. Each path that deletes unread mail from an inbox goes through
Store::drop_unread:FORGET_MS). After a daemon start, forgetting waits 6 hours.Outbox mail goes through
Store::drop_outboxwhen it is more than 7 days old, or when the other machine refuses it after the send call returned.What it says. The recipient, the send time, the first line, and "Not delivered". Reason Network never forgets a mailbox (
terminal,listen --as), so the forgotten-session path never drops mailbox mail. Rejected or expired held mail in a mailbox gets a notice like other held mail.The sender may be offline. A notice for a teammate machine waits in the outbox like other mail. A notice for a sender here goes to that sender's inbox, if this machine still remembers the session. A notice about a notice never goes out, because
notice()returns nothing whenfrom_agentisreason-network. Each drop that makes a notice makes one. In a test, a burst of 50 drops gave 50 notices. Delivery is at least once, as it was before this change. A notice could show up twice if its delivery stays unconfirmed past the 90 s timeout.Older engines. The notice uses only existing message fields, so an older engine reads it as ordinary mail. I checked this on ac0168b with a 27252c2 binary on one side. The old engine received a notice without error. In a team with one engine of each version, mail went both ways. After a downgrade, the old engine opened upgraded databases with 10k and 100k rows, and the SQLite integrity check passed.
Tests. Unit tests cover each drop path, and
two_machines.pyhas a new case. The "Tests" section has the details.No new dependencies.
Cargo.tomlandCargo.lockdon't change.Tests
a_drop_whose_notice_cant_be_kept_keeps_its_message_and_changes_nothinginsrc/store.rs, checks the transaction. A temporary SQLite trigger makes each notice insert fail after SQLite writes the row. The test covers three drops. Unread mail from a teammate machine puts its notice in the outbox. Unread mail from a session here puts its notice in that session's inbox. Outbox mail puts its notice in the sender's inbox. For each drop, it checks that the drop returns the error, both tables stay the same, and the message is still there. Then it deletes the trigger and checks that the same drop succeeds. Without the transaction indrop_unreador indrop_outbox, the test fails.tests/e2e/two_machines.pyqueues two messages for an offline session and turns one into pre-upgrade mail (source NULL). Then it forgets the session while the sender machine is off. The sender gets both notices once, held by its hold policy like any mail from that teammate.REASON_NETWORK_FORGET_GRACE_MS, so the e2e can forget sessions right after start. Release builds ignore it.Results on macOS, on be1a78d. The commits after ac0168b add only the rollback test. The old-engine, mixed-team and downgrade checks in point 4 ran on ac0168b.
cargo fmt --check: passcargo clippy --all-targets --locked -- -D warnings: passcargo test --locked: 239 unit and 2 integration tests passcargo build && env -u CLAUDE_CODE_MESSAGING_SOCKET python3 tests/e2e/two_machines.py: 148/148 passI ran these only on macOS. For Linux and Windows, I'm relying on CI.
Known limit
Some mail from before the upgrade gets no notice. This covers mail with an ambiguous origin and no presence that names the session. It also covers mail from this person's own session that this machine no longer remembers, when no teammate machine's presence names it. I chose to skip these, because if I guess wrong, another conversation that reuses the name could get the first line.
An unknown unread row also keeps the retry flag on. The numbers below come from debug builds on a Mac mini (M4). They are not release guarantees.
I think this is acceptable for a first PR, but it's your call. This cost appears only when a machine holds a large backlog of unread mail from before the upgrade. I can't say that the cost is small for such a backlog. If you prefer, I can follow up with either of these:
Design choices I made
These are the choices I made. You know the project better than I do, and I'm happy to change either one.
Q1. Held mail that someone rejects later
I chose to send a notice when someone rejects held mail, but not when
send_messagealready returned a refusal.reason-network rejectdeleted held mail without telling the sender.send_messagereturns it, and no notice goes out. If not, the sender sees "queued", and the later refusal sends a notice.Does this match how you want held mail to behave? If not, I'm happy to change it.
Q2. The loop marker
I chose to rely on the
from_agent == "reason-network"check for this PR.from_agent, so the marker is not authenticated. A code comment at the first check says nothing may trust it.Would you prefer a stronger marker in a follow-up? If so, I'm happy to do it.