Conversation
|
Thanks for reporting this, @rfoust. To help us track it down, could you share a few more details?
If you can attach logs (Help → Support → File an Issue), that would be especially helpful. — AetherClaude (automated agent for AetherSDR) 🤖 aethersdr-agent · cost: $4.0525 · model: claude-opus-5 |
aethersdr-agent
Bot
added
the
awaiting-response
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
Yes. #5664 asks for five things and the diff delivers all five: atomic whole-document replacement (writeJsonAtomically, QSaveFile with setDirectWriteFallback(false) — the right call, since a fallback direct write is exactly the truncation the issue is about); surfaced open/write/commit failures (reportPersistenceFailure → qCWarning(lcAx25) + activity); acknowledgement only after commit (cmdRead, cmdKill, finishCompose all build a candidate vector, save it, and adopt it only on success); m_nextId consumed only on a committed write; and the prior document preserved on failure.
I checked the "keep in-memory consistent with the last successful write" requirement specifically: m_messages is now assigned only via std::move(updated) after a true return in all three mutators — there is no path left that mutates m_messages and then saves. m_heard and m_callers are still mutated before their save and keep the mutation on failure, but the issue scopes that requirement to message data / read flags / next ID, and those two stores are self-healing (the next successful save carries the pending entry), so I do not read that as unaddressed.
Socket disclosure: the added test binds nothing. It is pure file I/O plus the existing in-memory onAirFrame/Frame::encode() injection seam — no QTcpServer/QUdpSocket/QLocalServer, no peer process, no Fake* radio. Nothing to record beyond that.
2. Scope
| File | What it changes | Claimed by the issue? | Verdict |
|---|---|---|---|
src/core/pms/PmsMailbox.cpp |
QSaveFile helper; save* return bool and take the candidate by value; ensureStorageDir returns its result; transactional read/kill/compose; reportPersistenceFailure |
Yes — all five requirements | In scope |
src/core/pms/PmsMailbox.h |
Signature changes only, all under private: (verified at PmsMailbox.h:150) — no new public/protocol/settings surface |
Yes | In scope |
tests/pms_mailbox_test.cpp |
One new test on the already-registered pms_mailbox_test target |
Yes — issue asks for failure/recovery/restart/atomicity coverage | In scope |
No CHANGELOG.md edit, no new settings key, no new RadioCapabilities bool, no build-config churn. The only thing in the diff no requirement strictly explains is the brace addition around m_callers.remove(...) in recordCaller (PmsMailbox.cpp:410-412) — one line of style, not worth a row.
3. Blockers
None.
4. Nits (non-blocking)
- A caller can be trapped in compose mode by a persistent storage failure — inline at
PmsMailbox.cpp:771. Retaining the draft is the right trade over the old falseSAVED, but there is no abort verb:finishCompose(false)has no caller anywhere in the file (so"Message aborted."is dead onmaintoo), andonLinkDatacallstouchSession()on every byte, so the 10-minute idle timer re-arms with each retry. Bounded only by the caller sending DISC. No data is lost either way. - The destructor's save can now emit a signal — inline at
PmsMailbox.cpp:103.saveHeardwasconstand silent before this PR; it can now reachemit activity(...), andm_pmsis a child ofAx25HfPacketDecodeDialog(Ax25HfPacketDecodeDialog.cpp:852, connected toappendSystemLineat :1253), so on the failure path the slot runs after the parent's derived destructor has completed. m_draft = candidate;atPmsMailbox.cpp:776is a dead store —m_draftis fully reset bycmdSendBeginbefore it is next read. Harmless; it just reads as if it mattered.- Sibling left behind, out of scope:
src/core/tnc/HeardList.cpp:196has the identical defect class —WriteOnly | Truncatewith thef.write(...)result discarded. It is the TNC's own heard list, not one of the three PMS stores #5664 names, so this is a completeness note rather than something to bundle here.
5. What I tried to break
- "Atomic replacement" — I checked the fixture actually exercises the path it claims. The test makes the target path a directory. Qt's
QSaveFile::open()has an explicitexistingFile.isDir()→WriteErrorbranch before it creates its temp file, so the failure is real and not an accident of permissions. And the hard-link assertions would fail against the unfixedQFiletruncate (the link would observe the new bytes), so they are load-bearing rather than self-confirming. - I tried to find a surviving path that mutates state before the save. Grepped every
m_messageswrite andsaveMessagescall site in the head checkout:cmdRead,cmdKill,finishComposeandloadAllare the only ones, and the first three all adopt viastd::moveafter atrue.updated.remove(i)/updated[i]index a copy ofm_messagestaken in the same scope, so the indices cannot skew. - I tried to break the ID accounting.
finishComposewritesm_nextId + 1while assigningcandidate.id = m_nextId, and bumps the member only after commit; the test'snextId == 3assertion after two commits and one failure would catch a consumed ID.loadAlladditionally re-derivesm_nextId = max(nextId, id+1), so even a stale header self-corrects. - Multi-instance interference in the new test. The
callerWriter/callerFailurescopes construct secondPmsMailboxobjects against the sameAETHER_PMS_DIRwhilepmsis still connected; I traced their destructors (saveHeardonly) against the order of thedurableHeard/durableCallerscaptures and found no fixture that reads a file another instance rewrites between capture and assertion. - Could not check, and saying so:
pms_mailbox_testappears in noctest -Rfilter in.github/workflows/ci.yml, so the five green checks prove the new test compiles and links on Linux/macOS/Windows — not that it passes. In particular thestd::filesystem::create_hard_linkfixture is unexercised by CI and will hard-fail (not skip) on a filesystem without hard-link support. Worth one local Windows run before merge. - All findings above are reasoned from the head checkout at
/tmp/aetherclaude/pr-5666; I ran no build and executed no test.
6. Recommendation
Approve with nits. The fix matches the issue requirement-for-requirement, the seam it chose (candidate-vector → save → adopt) is the right one, it adds no public surface, and the test is adversarial enough to fail against the unfixed code. The compose-trap and destructor-emit items are worth a follow-up commit but neither breaks a user or violates canon. Next step: confirm the new test passes on Windows locally, then merge.
Nice work — the setDirectWriteFallback(false) comment in particular explains why rather than what, which is exactly the house style.
🤖 aethersdr-agent · cost: $8.8480 · model: claude-opus-5
| QVector<Message> updated = m_messages; | ||
| updated.append(candidate); | ||
| if (!saveMessages(updated, m_nextId + 1)) { | ||
| reply(QStringLiteral("*** MESSAGE NOT SAVED; storage error. Draft retained: send /EX to retry.")); |
There was a problem hiding this comment.
A persistent storage failure leaves the caller with no way out of compose mode.
On this early return m_compose stays Compose::Body and m_draftLines is retained — deliberately, and the message text is the right thing to keep. But there is no abort verb to pair with it: finishCompose(false) has no caller anywhere in this file (grepped the head checkout — only the two finishCompose(true) sites at :483 and :546), so "Message aborted." is unreachable on main as well. Every line the caller types that is not /EX is appended to the draft body by processLine — including a B they meant as bye, which then rides along in the message once storage recovers.
The idle timer does not bound it either: onLinkData calls touchSession() on every inbound byte (:453), so a caller retrying /EX keeps re-arming the 10-minute clock indefinitely. The only exit is a DISC from their side.
Not a blocker — no data is lost, and this is still strictly better than the old unconditional SAVED. But a one-line escape would close it, e.g. accepting /ABORT alongside /EX in the Compose::Body branch at :544, or dropping compose state after the second consecutive failure.
| { | ||
| if (m_loaded) | ||
| saveHeard(); | ||
| saveHeard(m_heard); |
There was a problem hiding this comment.
This save can now emit a signal during destruction, which it could not before.
saveHeard was void and const and wrote nothing on failure; it now reaches reportPersistenceFailure → emit activity(message) (:924). m_pms is parented to the dialog (src/gui/Ax25HfPacketDecodeDialog.cpp:852) with activity wired straight to appendSystemLine (:1253), and child deletion happens inside ~QObject — i.e. after ~Ax25HfPacketDecodeDialog's own body has finished. So on the failure path at shutdown the slot runs against a dialog whose derived members are already gone.
Only reachable when the heard-list write fails at teardown, so this is a nit rather than a blocker. The cheapest fix is to keep the destructor silent — e.g. a m_shuttingDown flag that reportPersistenceFailure checks before emitting, leaving the qCWarning in place.
rfoust
removed
priority: high
K5PTB
left a comment
K5PTB
left a comment
There was a problem hiding this comment.
Issue fit
Yes. #5664 asks for five things, and each maps to a hunk:
- atomic whole-document replacement (
QSaveFilewithsetDirectWriteFallback(false)); - surfaced failures (
reportPersistenceFailure); - acknowledgement only after commit (read, kill and compose build a candidate, save it, then adopt it);
- a message ID consumed only on commit;
- the previous document preserved.
I confirmed the transactional part with mutants (below). One regression came with it: the new failure reporting can now fire from a destructor, and in the app's own ownership shape that aborts.
Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/pms/PmsMailbox.cpp |
writeJsonAtomically, bool saves over candidates, transactional read/kill/compose, reportPersistenceFailure |
yes | in scope |
src/core/pms/PmsMailbox.h |
private signature changes only | yes | in scope |
tests/pms_mailbox_test.cpp |
one new case on the registered target | yes | in scope |
Everything in the diff is explained by the issue: no CHANGELOG.md, no settings key, no public surface. Sockets: none; the test uses the existing in-memory frame injection. pms_mailbox_test matches no ci.yml -R filter, so the green checks compiled it and did not run it. It first runs in full-suite.yml.
Blockers
1. ~PmsMailbox can now emit activity into its already-destroyed parent dialog, and Qt aborts. (inline PmsMailbox.cpp:100)
Before this PR, saveHeard() was silent on failure. Now the destructor's save reaches emit activity(...). In the app, m_pms is a child of Ax25HfPacketDecodeDialog (Ax25HfPacketDecodeDialog.cpp:852), wired directly to the derived member appendSystemLine (:1253). The dialog's destructor never disconnects it, and QWidget::~QWidget deletes children before ~QObject disconnects anything. So a heard-list write that fails at shutdown calls a member function on a dialog whose derived part is gone — the storage-failure path this PR exists to make safe.
Reproduced with a review-only probe using the same shape: a QWidget subclass, a PmsMailbox child, activity connected to a derived member slot, and heard.json blocked by a directory:
~Dlg body done
aether.ax25: PMS could not save heard stations: Filename refers to a directory
ASSERT failure in QWidget: "Called object is not of the correct type (class destructor may have already run)", file .../QtCore.framework/Headers/qobjectdefs_impl.h, line 107
exit=134
That is with assertions enabled in the local Qt build (Homebrew, macOS). Without them, the same call runs a member function on a destroyed object. The one-line suggestion inline blocks signals for the destructor's save only, and keeps the qCWarning. With it applied, the probe reports calls into destroyed derived dialog: 0 and exits 0, and pms_mailbox_test still passes.
Nits (non-blocking)
- The test never reaches the direct-write fallback it disables (inline
PmsMailbox.cpp:40). WithsetDirectWriteFallback(true), the whole suite still passes: a directory at the target path failsQSaveFile::openbefore any fallback applies. A read-only store directory is the fixture that exercises it (inline atpms_mailbox_test.cpp:666). - The recovered compose is not checked for the retained body (inline
pms_mailbox_test.cpp:625). Clearingm_draftLineson the failure path still passes, because an empty-body message also readsSAVEDwith count 2. - I agree with the earlier review's compose-trap note (no abort verb while storage stays broken). It's not repeated inline.
What I tried to break
-
Built
pms_mailbox_testat302c6688(Debug, macOS,-j4): all pass. -
Mutants. Caught:
- skipping
commit(): 23 failures, including all three hard-link snapshot checks, so atomic replacement is really exercised; - adopting the kill before its save;
- adopting the read flag on failure;
- consuming an ID on a failed compose;
- not emitting
activity; - the heard save ignoring failure.
Survived:
- the fallback flag (nit 1);
- dropping the draft on failure (nit 2);
write() < 0in place of the full-length check, which can't be triggered without an injection seam, so not a finding.
A plain-
QFilewriter mutant didn't compile (nocommit()) and was not run. - skipping
-
Suggested tests, checked both ways:
- the read-only-directory case passes at the head, and fails 3 checks under the fallback mutant;
- the body check (
body == "body", since the first compose line is the subject) passes at the head and fails under the draft-dropping mutant.
-
Heard-save cadence.
recordHeardsaves only for a new station, so a persistently failing store cannot flood the activity log at packet rate. -
Interaction with #5659, which also edits
PmsMailboxand this test target:git merge-treeshows no conflict between the two heads. -
Not driven in the app: the bridge has no PMS frame or storage-fault injection, and teardown ordering is what the probe covers.
Recommendation
Request changes — one line. The persistence fix is right and well tested. Blocker 1 is a crash the PR introduces on the failure path it is fixing, and the validated suggestion closes it. The two test additions are optional, but each closes a mutant that currently survives.
| PmsMailbox::~PmsMailbox() | ||
| { | ||
| if (m_loaded) | ||
| saveHeard(); | ||
| saveHeard(m_heard); | ||
| } |
There was a problem hiding this comment.
Blocker 1. This save can now emit activity(...) during destruction. In Ax25HfPacketDecodeDialog, that signal goes to a derived member slot after the dialog's derived destructor has run (children are deleted in ~QWidget), and Qt aborts: Called object is not of the correct type (class destructor may have already run). That's reproduced in the probe described in the review body. Blocking signals for this one save keeps the qCWarning and removes the call. Applied locally, the probe exits 0 and pms_mailbox_test passes.
| PmsMailbox::~PmsMailbox() | |
| { | |
| if (m_loaded) | |
| saveHeard(); | |
| saveHeard(m_heard); | |
| } | |
| PmsMailbox::~PmsMailbox() | |
| { | |
| // Report nothing from teardown: a parent widget's derived part may already be | |
| // gone (QWidget deletes children before QObject disconnects them). | |
| const QSignalBlocker quiet(this); | |
| if (m_loaded) | |
| saveHeard(m_heard); | |
| } |
| QSaveFile file(path); | ||
| // Never fall back to writing the target directly: a failed replacement must | ||
| // leave the last complete mailbox snapshot available to the next startup. | ||
| file.setDirectWriteFallback(false); |
There was a problem hiding this comment.
Nit. This line is the heart of the fix, and nothing in the test fails if it flips to true: I ran that mutant, and the full suite passes. The directory-at-path fixture makes QSaveFile::open fail on isDir() before any fallback applies. The fallback only engages when the temp file can't be created, which in practice means a non-writable directory — exactly the case where it would truncate the target in place. See the test comment at the end of testPersistenceIsAtomicAndTransactional for a fixture that catches it.
| PmsMailbox restarted; | ||
| restarted.setListenCallsign(QStringLiteral("N0PMS-1")); | ||
| restarted.setEnabled(true); | ||
| CHECK(restarted.messageCount() == 1, "restart recovers the last committed mailbox snapshot"); |
There was a problem hiding this comment.
Nit — a fixture for the direct-write fallback. POSIX only, since Windows directory ACLs don't map onto these permission bits. It passes at this head, and fails three checks with setDirectWriteFallback(true) (both new checks here, plus the body check on the other comment):
#ifndef Q_OS_WIN
{
const QByteArray durable = readFile(messages);
QFile::setPermissions(store, QFileDevice::ReadOwner | QFileDevice::ExeOwner);
session.send(QByteArrayLiteral("K 2\r"));
const QString reply = session.drainText();
QFile::setPermissions(store, QFileDevice::ReadOwner | QFileDevice::WriteOwner | QFileDevice::ExeOwner);
CHECK(reply.contains(QLatin1String("not killed")), "read-only store directory: kill reports failure");
CHECK(readFile(messages) == durable, "read-only store directory: target is not rewritten in place");
}
#endif|
|
||
| session.send(QByteArrayLiteral("/EX\r")); | ||
| CHECK(session.drainText().contains(QLatin1String("SAVED")), | ||
| "retained draft can be saved after storage recovers"); |
There was a problem hiding this comment.
Nit. This proves a message was saved, not the retained draft. Clearing m_draftLines on the failure path still passes, because an empty-body message also reads SAVED with count 2. One check pins it; the first compose line is the subject, so the body is "body". It passes at this head and fails under that mutant:
CHECK(readJsonObject(messages).value(QStringLiteral("messages")).toArray().at(1).toObject()
.value(QStringLiteral("body")).toString() == QStringLiteral("body"),
"recovered message carries the retained draft body");
PMS previously replied
MESSAGE n SAVED.andMessage n killed.even whenmessages.jsoncould not be written. Its messages, callers, and heard stores also used unchecked truncating writes, risking loss of the previous complete document.This change writes each JSON document with
QSaveFile, disables direct-write fallback, and checks the complete write and commit. Message additions, deletions, read flags, and the next ID enter the live mailbox only after persistence succeeds. Failed compose retains the draft for/EXretry; failed delete/read-state updates return an error. All three stores report failures through mailbox activity andlcAx25. The JSON format and heard/caller save cadence are unchanged.Fixes #5664.
Validation
pms_mailbox_testwith injected AX.25 commands: save/read/delete failures, draft retry, ID stability, read recovery, restart, and failure reporting for all three stores.upstream/mainPMS sources (87b80c65d): 16 assertions failed, including old-snapshot preservation and false success. Restored the fix and passed.link status→pms.messages=1,callerConnected=false;get radio→connected=false;get transmit→transmitting=false. PMS sent zero I-frames. Bridgeclose MainWindowsucceeded and the launched process exited 0. No live radio was connected.The agent automation bridge has no PMS frame-injection or persistence-failure verb. Failure semantics are demonstrated by the injected-frame regression test; native reload/status evidence is reported separately. No live radio or RF transmission is used.
Generated with OpenAI Codex.