Fix two races in the notification popup and history handling (#6735)
* Replay the history a dismissal or a clear was still being written into The popup files a replay reads are written by a serialized queue of shell jobs, and the read ran as its own process alongside it. A dismissal issued a moment earlier could still be queued when the directory was read, leaving the notification out of the replay it was the newest entry of, and a clear issued a moment earlier could still be queued too, replaying entries it was about to remove. The read now waits for the queue to go idle, so the replay shows the history as of the moment it was asked for rather than whichever jobs happened to have landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Catch up on an update that arrived before its popup had a row Watching a notification for in-place updates starts the moment it is handed over, but the row those updates write to is inserted a tick later, deferred to keep a mid-incubation Repeater from being mutated underneath. A client fast enough to update inside that window found no row to write to, and a property that has already changed does not change again — so the toast and its file sat on the superseded content until something else moved. The row is now refreshed from the live notification once it exists. That reads the same object the signals would have, so an update that beat the insert is picked up and one that did not costs nothing: a refresh whose content matches the row it would write is dropped, which also collapses the several signals a single multi-property update emits into one rewrite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Hold queued file work behind the replay's read, not just ahead of it The read waited for everything queued before it, but nothing stopped the queue from running on while it worked. A clear or an archive issued during the read could delete or move files out from under awk mid-glob, so a replay could still show a partial history — some of what a clear was in the middle of emptying. The read is a barrier in both directions now: the queue holds until it exits, and it releases on exit rather than on output, so a read that comes back empty or fails cannot park the queue behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Queue the replay's read instead of waiting for the queue to empty Waiting for the queue to go idle before starting the read still let work overtake it. A clear or an archive enqueued after the replay was asked for, while the current job was running, was dequeued the moment that job exited — the read only starts once nothing is left — so the replay showed the state after those jobs, which is the race this was meant to close. Unbroken file traffic could postpone the read indefinitely for the same reason. The read is now an entry in that queue rather than a process running beside it. It takes its place in line behind the work queued before the request and ahead of everything queued after, so no later job can overtake it and no amount of traffic can push it back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
David Heinemeier Hansson
co-authored by
Claude Opus 5
Claude Opus 5
parent
fd23ca023a
commit
9b8bf1da71
@@ -92,6 +92,28 @@ function snapshotOf(notification, timestamp) {
|
||||
}
|
||||
}
|
||||
|
||||
// Everything the popup card draws, and therefore everything an in-place
|
||||
// update has to write through to the row and its file.
|
||||
var POPUP_ROLES = ["app", "appIcon", "summary", "body", "image", "glyph", "exec", "urgency", "expireTimeout"]
|
||||
|
||||
function popupRoles() {
|
||||
return POPUP_ROLES
|
||||
}
|
||||
|
||||
// Whether a refresh has anything to write. Each property a client updates
|
||||
// emits its own signal, and the catch-up refresh after a row is inserted
|
||||
// usually finds the object exactly as it was snapshotted — without this,
|
||||
// one update would rewrite the file several times over.
|
||||
function popupRowChanged(row, updated) {
|
||||
var current = row || {}
|
||||
var next = updated || {}
|
||||
for (var i = 0; i < POPUP_ROLES.length; i++) {
|
||||
var role = POPUP_ROLES[i]
|
||||
if (current[role] !== next[role]) return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// A client updating a notification through replaces_id keeps the identity of
|
||||
// the popup it took over: the file name is the timestamp and id the popup was
|
||||
// first persisted under, and the restore, replace and archive paths all key
|
||||
@@ -276,6 +298,8 @@ if (typeof module !== "undefined") {
|
||||
execFromHints: execFromHints,
|
||||
shouldRenderCompactGlyph: shouldRenderCompactGlyph,
|
||||
snapshotOf: snapshotOf,
|
||||
popupRoles: popupRoles,
|
||||
popupRowChanged: popupRowChanged,
|
||||
replacementSnapshot: replacementSnapshot,
|
||||
historyEntry: historyEntry,
|
||||
parseSettings: parseSettings,
|
||||
|
||||
@@ -183,6 +183,10 @@ Item {
|
||||
Qt.callLater(function() {
|
||||
removePopupsByOriginalId(snapshot.originalId, NotificationLogic.popupFileName(snapshot))
|
||||
popupModel.insert(0, snapshot)
|
||||
// An update that arrived while the insert was deferred found no row to
|
||||
// write to, and a property that already changed will not change again.
|
||||
// Reading the object once the row exists catches up on it.
|
||||
service.refreshPopup(notification, snapshot.originalId, snapshot.timestamp)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -223,18 +227,12 @@ Item {
|
||||
return
|
||||
}
|
||||
|
||||
var roles = NotificationLogic.popupRoles()
|
||||
for (var i = 0; i < popupModel.count; i++) {
|
||||
var row = popupModel.get(i)
|
||||
if (!row || row.originalId !== originalId || row.timestamp !== timestamp) continue
|
||||
popupModel.setProperty(i, "app", updated.app)
|
||||
popupModel.setProperty(i, "appIcon", updated.appIcon)
|
||||
popupModel.setProperty(i, "summary", updated.summary)
|
||||
popupModel.setProperty(i, "body", updated.body)
|
||||
popupModel.setProperty(i, "image", updated.image)
|
||||
popupModel.setProperty(i, "glyph", updated.glyph)
|
||||
popupModel.setProperty(i, "exec", updated.exec)
|
||||
popupModel.setProperty(i, "urgency", updated.urgency)
|
||||
popupModel.setProperty(i, "expireTimeout", updated.expireTimeout)
|
||||
if (!NotificationLogic.popupRowChanged(row, updated)) return
|
||||
for (var r = 0; r < roles.length; r++) popupModel.setProperty(i, roles[r], updated[roles[r]])
|
||||
// The file name is the timestamp and id this popup was persisted under,
|
||||
// so the rewrite lands on the same file: a restart restores the version
|
||||
// last shown, and so does the copy that ends up in history.
|
||||
@@ -391,17 +389,37 @@ Item {
|
||||
// match these rows against fresh notifications.
|
||||
property var restoredPopups: ({})
|
||||
|
||||
// Entries are either { command } for a file job or { read: true } for a
|
||||
// replay's directory read. Queueing the read rather than running it beside
|
||||
// the queue is what makes it a barrier: it takes its place in line, so the
|
||||
// history it sees is the one that existed when the replay was asked for.
|
||||
// Everything queued after it — a clear, an archive, a silenced write — waits
|
||||
// for it, and no amount of later traffic can push it back.
|
||||
property var popupFileQueue: []
|
||||
|
||||
function enqueuePopupFileJob(command) {
|
||||
popupFileQueue = popupFileQueue.concat([command])
|
||||
popupFileQueue = popupFileQueue.concat([{ command: command }])
|
||||
runNextPopupFileJob()
|
||||
}
|
||||
|
||||
function enqueueHistoryRead() {
|
||||
popupFileQueue = popupFileQueue.concat([{ read: true }])
|
||||
runNextPopupFileJob()
|
||||
}
|
||||
|
||||
function runNextPopupFileJob() {
|
||||
if (popupFileProc.running || popupFileQueue.length === 0) return
|
||||
popupFileProc.command = popupFileQueue[0]
|
||||
if (readHistoryProc.running || popupFileProc.running) return
|
||||
if (popupFileQueue.length === 0) return
|
||||
|
||||
var job = popupFileQueue[0]
|
||||
popupFileQueue = popupFileQueue.slice(1)
|
||||
|
||||
if (job.read) {
|
||||
startHistoryRead()
|
||||
return
|
||||
}
|
||||
|
||||
popupFileProc.command = job.command
|
||||
popupFileProc.running = true
|
||||
}
|
||||
|
||||
@@ -483,6 +501,9 @@ Item {
|
||||
Process {
|
||||
id: readHistoryProc
|
||||
running: false
|
||||
// Let the file queue go again, whatever the read did — a failed or empty
|
||||
// read must not leave archives and clears parked behind it forever.
|
||||
onExited: service.runNextPopupFileJob()
|
||||
stdout: StdioCollector {
|
||||
waitForEnd: true
|
||||
onStreamFinished: service.replayHistory(text)
|
||||
@@ -494,15 +515,26 @@ Item {
|
||||
// by then, so they're handed over in memory instead of being waited for.
|
||||
property var replayCarryOver: []
|
||||
|
||||
// Re-show what's in historyDir as toasts. Reading the directory is a
|
||||
// subprocess, so the replay lands in replayHistory a moment later.
|
||||
// Set from the moment a read is queued until it starts, so a second
|
||||
// showHistory while one is still waiting its turn doesn't queue another.
|
||||
property bool historyReadQueued: false
|
||||
|
||||
// Re-show what's in historyDir as toasts. The read goes through the file
|
||||
// queue and its own subprocess, so the replay lands in replayHistory once
|
||||
// the work queued ahead of it has finished.
|
||||
function showRecentHistory() {
|
||||
if (readHistoryProc.running) return "ok"
|
||||
if (readHistoryProc.running || service.historyReadQueued) return "ok"
|
||||
service.replayCarryOver = liveRowsForReplay()
|
||||
service.historyReadQueued = true
|
||||
enqueueHistoryRead()
|
||||
return "ok"
|
||||
}
|
||||
|
||||
function startHistoryRead() {
|
||||
service.historyReadQueued = false
|
||||
readHistoryProc.command = ["bash", "-c",
|
||||
"awk 1 \"$1\"/*.json 2>/dev/null || true", "--", historyDir]
|
||||
readHistoryProc.running = true
|
||||
return "ok"
|
||||
}
|
||||
|
||||
// Copy the on-screen rows out of the model. The placeholder from an earlier
|
||||
|
||||
@@ -170,6 +170,18 @@ assertEqual(
|
||||
'12345-12.json',
|
||||
'notifications keep the persisted file name across an in-place update'
|
||||
)
|
||||
assert(
|
||||
!notifications.popupRowChanged(replacement, replacement),
|
||||
'notifications skip a refresh that matches the row it would write'
|
||||
)
|
||||
assert(
|
||||
notifications.popupRowChanged(replacement, Object.assign({}, replacement, { body: 'message 3' })),
|
||||
'notifications refresh a row whose content moved on'
|
||||
)
|
||||
assert(
|
||||
!notifications.popupRowChanged(replacement, Object.assign({}, replacement, { timestamp: 999 })),
|
||||
'notifications ignore identity fields when deciding whether a refresh has work'
|
||||
)
|
||||
|
||||
const settings = notifications.parseSettings(JSON.stringify({ version: 3, dnd: true }))
|
||||
assertEqual(settings.dnd, true, 'notifications parse the persisted DND state')
|
||||
@@ -371,9 +383,33 @@ assert(
|
||||
'notifications service refreshes the popup from every property the card draws'
|
||||
)
|
||||
assert(
|
||||
/popupModel\.setProperty\(i, "summary", updated\.summary\)[\s\S]{0,600}?persistPopupFile\(updated\)/.test(serviceQml),
|
||||
/popupModel\.setProperty\(i, roles\[r\], updated\[roles\[r\]\]\)[\s\S]{0,600}?persistPopupFile\(updated\)/.test(serviceQml),
|
||||
'notifications service rewrites both the row and its file when a notification is updated in place'
|
||||
)
|
||||
assert(
|
||||
/if \(!NotificationLogic\.popupRowChanged\(row, updated\)\) return/.test(serviceQml),
|
||||
'notifications service leaves the row and its file alone when a refresh finds nothing changed'
|
||||
)
|
||||
assert(
|
||||
/popupModel\.insert\(0, snapshot\)[\s\S]{0,300}?service\.refreshPopup\(notification, snapshot\.originalId, snapshot\.timestamp\)/.test(serviceQml),
|
||||
'notifications service catches up on an update that beat the deferred row insert'
|
||||
)
|
||||
assert(
|
||||
/function showRecentHistory\(\)[\s\S]{0,300}?enqueueHistoryRead\(\)/.test(serviceQml),
|
||||
'notifications service reads history from its place in the file queue'
|
||||
)
|
||||
assert(
|
||||
/if \(job\.read\) \{\s*\n\s*startHistoryRead\(\)/.test(serviceQml),
|
||||
'notifications service runs the queued read when its turn comes'
|
||||
)
|
||||
assert(
|
||||
/function runNextPopupFileJob\(\) \{\s*\n\s*if \(readHistoryProc\.running \|\| popupFileProc\.running\) return/.test(serviceQml),
|
||||
'notifications service holds queued file work until a history read finishes'
|
||||
)
|
||||
assert(
|
||||
/id: readHistoryProc[\s\S]{0,300}?onExited: service\.runNextPopupFileJob\(\)/.test(serviceQml),
|
||||
'notifications service releases the file queue even when a history read comes back empty'
|
||||
)
|
||||
assert(
|
||||
/onSummaryChanged: cardSlot\.remainingLifetime = 1\.0/.test(serviceQml),
|
||||
'notifications service restarts the countdown when a toast is updated under it'
|
||||
|
||||
Reference in New Issue
Block a user