Repository navigation
fix(restart-intent): ack on consumption, not on write - #3191
john-the-dev wants to merge 9 commits into
Conversation
The bridge acked the owner the moment it WROTE state/core-restart-requested.json. Writing always succeeds, so on a host whose executor is gone the owner got "the app will relaunch the core in a few seconds" and nothing ever happened; the intent expired silently 10 minutes later. await_consumption() waits for the file to DISAPPEAR, which is the only implementation-agnostic evidence that something claimed it — consume_intent() deletes before acting. Probing for a named consumer (a running Sutando.app, a specific launchd label) answers "is THAT consumer here", not "will anything act", and returns the same False for a host whose executor is merely a different one. That distinction is what made the original report wrong. Reported in #3182 and corrected by the reporter in #3183, whose revised ask this implements: verify a live consumer before acknowledging. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking findings:
-
CI is red on the existing bridge seam. I reproduced
python3 tests/bridge-restart-intercept.test.py: it now waits through the real 12-second timeout for each successful write and fails both existing acknowledgement assertions. The GitHubpython standalone testsjob fails on the same file. Please stub/inject the consumption wait in that suite and cover both claimed and timeout acknowledgements without wall-clock sleeps. -
The three new
TestAwaitConsumptioncases are declared after theif __name__ == "__main__": unittest.main(...)call, so the documented direct invocation exits before that class is defined.python3 tests/core-restart-intent.test.pyreports 12 pre-existing tests, not the new cases; manually loading the class proves its three tests pass. Move the main guard to the end so the standalone CI runner actually exercises them. -
The timeout message says "nothing is going to happen", but the implementation deliberately leaves the intent on disk for a late executor. That can tell the owner no action will occur immediately before the pending restart/stop is consumed. Either cancel the intent on timeout or phrase this as "not picked up yet; it may still run before expiry."
Focused checks:
python3 tests/core-restart-intent.test.py: passes, but does not discover the new class- manual load of
TestAwaitConsumption: 3/3 pass python3 tests/bridge-restart-intercept.test.py: fails 2 checks- GitHub CI:
python standalone testsfailed on the same bridge test
Reviewed by Qingyun's Personal Codex.
| unittest.main(verbosity=2) | ||
|
|
||
|
|
||
| class TestAwaitConsumption(unittest.TestCase): |
There was a problem hiding this comment.
These cases are defined after unittest.main(), which exits during the documented direct invocation. Move the main guard below this class so the standalone runner discovers the new coverage.
| "stopped until you say `restart core`.") | ||
| else: | ||
| ack = (f"Wrote the {action} request, but **no executor picked it " | ||
| f"up** within 12s — so nothing is going to happen. The " |
There was a problem hiding this comment.
This certainty conflicts with leaving the intent on disk: a late executor can still consume it and perform the action before expiry. Either remove the pending intent or tell the owner it has not been claimed yet and may still run.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/core_restart_intent.pyLines 192-201 192 with _intent_lock(path):
193 try:
194 with open(path) as f:
195 raw = f.read()
! 196 except OSError:
! 197 return None
198 try:
199 os.unlink(path) # consume FIRST — crash mid-action must not replay
200 except OSError:
201 # FAIL CLOSED (qingyun review, #2408): if the file can't be |
…t, honest timeout ack Three findings from @qingyun-wu's review, all correct: 1. The new TestAwaitConsumption class sat AFTER the __main__ guard, so the documented invocation exited before defining it. Guard moved to EOF: `python3 tests/core-restart-intent.test.py` goes 12 -> 15 tests. The 12 were all pre-existing; my three never ran, and the PR body's "9 pre-existing + 3 new" was wrong on both halves. 2. bridge-restart-intercept.test.py drove the real 12s wait per successful write and asserted the old ack text. _handle_restart_command now takes wait_for_consumption (defaulting to await_consumption) so the suite injects the outcome; it covers BOTH the claimed and the timeout ack and runs in 0.33s. `tsc + tests (clean install)` was a roll-up of this same failure. 3. The timeout ack said "nothing is going to happen" while the code deliberately leaves the intent for a late executor. Reworded to "no executor picked it up within 12s ... may still run ... before the request expires", and the test now pins both that wording and the intent still being on disk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three findings were correct and are fixed in 1. The new tests never ran, and my evidence line was wrong. The 2. Bridge suite: wait is now injectable. 3. Timeout wording fixed. You're right that "nothing is going to happen" contradicted leaving the intent for a late executor. Now:
I chose the "not yet" phrasing over cancelling the intent on timeout, because cancelling would break the case this PR is really for: an executor that starts a few seconds late still does the right thing. Happy to flip it if you'd rather the ack be unconditionally final. Local at
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head ee49b99e.
The three prior findings are resolved: the direct intent suite now discovers 15 tests, the bridge suite injects both wait outcomes and passes, and the timeout copy accurately says the request may still run. Two blockers and one required live-path witness remain:
-
[P1]
src/core_restart_intent.py:100/src/discord-bridge.py:2999-3005— pathname disappearance cannot identify which destructive request was consumed.write_intentreplaces one shared file, while every waiter only checks whether that pathname is gone. If the owner sendsrestart coreand thenstop coreinside the 12-second window, the second write supersedes the first; the executor performs onlystop, but both waiters returnTrueand both messages say their different action was picked up. Exact-head production-function control:consumer_action=stop, restart_waiter_claimed=True, stop_waiter_claimed=True. Correlate consumption to the exact intent, or serialize/reject superseding requests, and add this overlapping-opposite-action regression before acknowledging either action. -
[P2]
src/discord-bridge.py:3000-3002still violates the two-line code-comment rule. The exact full-diff gate reportsprose-cap: src/discord-bridge.py:3000 comment block is 3 lines (cap 2). Keep the constraint in at most two lines and leave the narrative in the PR body. -
This changes a live bridge path for destructive restart/stop behavior, but the PR still explicitly has no post-restart Discord round trip. Please restart the branch-built bridge and exercise the no-consumer owner-command path, showing the actual timeout acknowledgement and surviving intent. The repository requires a real post-restart round trip for bridge/live delivery changes; harness-only proof is not sufficient.
Focused evidence at this head:
python3 tests/core-restart-intent.test.py: 15/15 passpython3 tests/bridge-restart-intercept.test.py: all checks pass in 0.33sgit diff --check: cleanscripts/review-checks.shover the full PR diff: fails only the comment block above- hosted Python standalone tests, clean-install roll-up, CLA, and other completed checks are green; coverage was still running at review time
Reviewed by Qingyun Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
The three prior blockers are resolved: the direct suite now runs 15 tests, the bridge seam passes quickly with both claimed/timeout branches, and the timeout acknowledgement is honest about a possible late claim.
One blocking repository gate remains on this head: bash scripts/review-checks.sh --diff /private/tmp/pr3191-current.diff fails because the comment block at src/discord-bridge.py:3000 is three lines while the prose cap is two. Please condense that comment, then the focused code/test delta is ready from my side.
Focused results:
python3 tests/core-restart-intent.test.py: 15/15 passpython3 tests/bridge-restart-intercept.test.py: pass- review checks: fail only on the three-line prose-cap block
Reviewed by Qingyun's Personal Codex.
| "It stays stopped until you say `restart core`.") | ||
| # Ack on CONSUMPTION, not on write (#3183): writing always succeeds, so | ||
| # acking here promised a restart on hosts whose executor was gone — | ||
| # silent for as long as nobody happened to notice. |
There was a problem hiding this comment.
Repository review checks cap code-comment blocks at two lines; this three-line block makes the required gate fail. Please condense it to two lines.
…entifies its request await_consumption infers "my request was taken" from the pathname disappearing. With a superseding write that inference is unsound: restart then stop inside the 12s window leaves one file, one executor action, and two waiters that both report their own (opposite) action as picked up. write_intent now claims the path with os.link, which fails atomically when one is already pending, and raises IntentPending naming the surviving action. A stale intent is still replaced, so an abandoned request cannot wedge the command. peek_intent reads without consuming. Also condenses the ack comment to the two-line cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both blockers addressed in P1 — consumption now identifies its requestYou were right that "the pathname is gone" cannot say which intent was consumed. Rather than correlate after the fact, I removed the ambiguity at the source: Your exact control, run against both commits: The bridge catches Two things I made sure not to break:
Regression coverage is the overlapping-opposite-action case you asked for, plus five siblings: (15 → 21. New: P2 — prose capCondensed. On the full PR diff, not just the delta:
P3 — the live witness, which I have NOT producedStating this plainly rather than burying it: there is still no live post-restart round trip in this PR. I did not run one because producing it means restarting the owner's core out from under an active session, and I don't think a PR of mine is sufficient reason to do that unasked. So the honest status is that the unit and seam coverage is complete and the live-path evidence is absent — not that it passed. I'll run it the moment the owner is up for a restart, and post the real before/after here. If you'd rather block on that, that's a reasonable call and I won't argue it. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head 6d9b0d75.
P1 — the stale-replacement path can still lose the only intent and falsely acknowledge consumption (src/core_restart_intent.py:102-123). Concurrent handlers in one bridge share the same {pid}.tmp; more fundamentally, two writers that both classify the existing path as stale can each unlink it without proving it is still the inode they inspected. I drove the production write_intent with two threads past the same stale observation. Writer A returned written; writer B then removed A's replacement and failed linking the already-removed shared temp; before any executor ran, peek_intent was None and await_consumption(..., timeout_sec=0) returned True. The bridge would therefore tell A “picked up by the executor” although no executor touched the file.
Please serialize the complete inspect/remove/create transition with a stable lock (and give each writer a genuinely unique temp), then add a deterministic two-writer stale-intent regression proving exactly one writer succeeds and its file remains until consume_intent claims it. The 21 focused intent tests, bridge interception suite, compilation, diff hygiene, and repository checks pass, but they only exercise sequential replacement. The author also correctly notes the real post-restart witness is still absent.
Verdict: not merge-ready until concurrent stale replacement is atomic and the live-path gate is satisfied.
…flagged The gate named 9 uncovered changed lines, all of them branches that decide whether the owner is told a request was queued — exactly the ones that must not be inferred. Adds: core-restart-intent: peek on absent/corrupt/unknown-action input, the racing refill, an undeletable stale intent, and a tmp-cleanup failure that must not turn a successful claim into an error. 21 -> 26 tests. bridge: the IntentPending ack, asserting it names the SURVIVING action and leaves the first intent on disk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…not a conflict Self-review with the lesson from the #726 finding: `except OSError` on the stale-clear covered two cases whose correct responses are opposite. If an executor consumes the stale intent between our peek and our unlink, the path is now FREE and we should claim it — but the bare OSError catch reported IntentPending, telling the owner a request is pending that nobody made. FileNotFoundError now falls through to the retry; a genuine unlink failure (permissions, read-only) still refuses. Control: with the FileNotFoundError branch removed the new test errors; with it, 27/27 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed
Also corrected the PR body: it still carried the retracted "Ran 12 tests (9 pre-existing + 3 new)" claim, and it cited a |
qingyun-wu
left a comment
There was a problem hiding this comment.
[P1] The concurrent stale-replacement blocker remains unresolved on this head. write_intent still performs peek_intent() and then blindly unlink(path) without serializing that inspect/remove/create transition or proving the pathname still names the stale inode it inspected. Writer A can remove the stale file and install its fresh intent while writer B is between peek and unlink; B then deletes A's live intent and replaces it (or loses the shared per-process temp). A's waiter observes disappearance and reports executor consumption even though no executor acted. The new FileNotFoundError branch only handles the path already being absent; it does not handle the path having been replaced before B's unlink. The temp name is also PID-only, so concurrent handlers in this bridge still share it.
Please use a stable writer lock around the full stale inspect/remove/claim transition and a genuinely unique temp per invocation, then add the requested deterministic two-writer stale regression proving exactly one request succeeds and remains until consume_intent removes it. The expanded 27-test intent suite and bridge interception suite pass locally, but they still exercise stale replacement sequentially. The author also confirms the required post-restart live witness remains outstanding.
Reviewed by Qingyun's Personal Codex.
qingyun review: exclusivity covered the superseding write but not concurrent STALE replacement. Two writers that both classify the existing file as stale each unlink it without proving it still names the inode they inspected, so the loser can delete the winner's freshly-installed intent — and the winner's waiter reads that disappearance as executor consumption, acking a restart nobody is performing. The peek/remove/claim sequence now runs under an flock on <path>.lock, which serializes across threads AND processes and is released by the kernel if the holder dies. The retry loop is gone: under the lock the path cannot be refilled between inspect and claim. The temp comes from mkstemp rather than a pid-named path, so two threads of one bridge can no longer share it. Tests: 27 -> 32. The concurrent cases are proven by control, not asserted — with the lock neutered, test_writer_blocks_while_the_transition_lock_is_held and the repeated race both FAIL; with it, 32/32 pass. An earlier draft of these tests passed either way and was rewritten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The fixThe peek/remove/claim sequence now runs under an with _writer_lock(path):
live = peek_intent(workspace)
if live is not None:
raise IntentPending(live["action"], float(live["requested_at"]))
try:
os.unlink(path)
except FileNotFoundError:
pass
...
os.link(tmp, path)
return pathThe part I got wrong firstMy initial regression was four concurrent-race tests. They passed — and they also passed with the lock neutered. A single race almost never lands in the peek/unlink window, so the tests asserted nothing. That is precisely the "passes either way" failure I flagged on your same-code test earlier today, committed by me, an hour later. Rewrote them around a control:
27 → 32 tests. Bridge suite passes, Still outstanding, unchangedThe live post-restart witness. The bridge on this host started One judgement call worth flagging rather than burying: the lock file |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head ad755373.
[P1] The new lock serializes writers, but the consumers can still delete a freshly installed request while acting on stale bytes. write_intent holds <path>.lock around its stale unlink/link transition (src/core_restart_intent.py:124-140), but both the Python consumer (:187-205) and the live Swift consumer (src/Sutando/main.swift:2571-2584) still do an unlocked read followed by pathname deletion. This ordering remains possible:
- a consumer opens and reads the old stale intent;
- the writer acquires its lock, removes that stale pathname, links the new intent, and returns success;
- the consumer removes the pathname—which now names the new intent—then rejects its already-read stale payload without acting.
A deterministic exact-head control using the production write_intent and consume_intent produced:
writer_path=<...>/core-restart-requested.json
consumer_action=None
intent_after_race=None
waiter_claimed=True
So the bridge can still tell the owner that the new restart/stop was picked up even though no executor performed it—the exact failure this PR is meant to eliminate. Please include consumers in the same claim protocol (including the Swift live consumer), or otherwise correlate deletion to the inode/intent that was actually read, and add this writer-vs-consumer stale replacement regression.
The writer-vs-writer repair itself is sound: unique mkstemp files plus the stable flock remove the prior collision, and the exact-head intent suite (32/32), bridge interception suite, Python compilation, diff hygiene, and repository review checks pass. The required real post-restart Discord round trip also remains outstanding, as the author notes.
Verdict: not merge-ready until the writer/consumer race is closed and the live-path witness is supplied.
Reviewed by Qingyun's Personal Codex.
…ters qingyun review: locking the writer alone left the writer-vs-CONSUMER race. A consumer reads the stale bytes, the writer then replaces the file under its lock, and the consumer deletes the pathname — which now names the NEW intent — before discarding its stale payload. Nobody acts, the intent is gone, and the writer's waiter reads that disappearance as consumption. consume_intent now holds the same lock across read+unlink, so anything that deletes the pathname has read what it is deleting. Renamed _writer_lock -> _intent_lock because it is the protocol's lock, not one participant's. A missing-file fast path returns before the lock: an intent that does not exist needs no serialization, and the lock's directory may not exist on a never-written workspace. Without it consume_intent raised FileNotFoundError where it used to return None — caught by an existing test, not by me. Tests 32 -> 34. Control: with the consumer's lock removed, test_consumer_blocks_while_the_intent_lock_is_held FAILS. The 60-round invariant test did not trip in the control, so the deterministic one is what holds this contract. NOT covered: the Swift consumer in src/Sutando/main.swift has the same read-then-delete shape and needs the same flock. It is not changed here — see the PR discussion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Python consumer now takes the same lock
One subtlety worth naming: a missing-file fast path returns before the lock. An intent that does not exist needs no serialization, and the lock file's directory may not exist on a never-written workspace. Without that, Control, and I want to be precise about which test does the work: The 60-round writer/consumer invariant test did not trip in that control. So the deterministic lock-held-externally test is what actually holds this contract; the race test is corroboration, not proof. Saying that explicitly because "34 tests pass" would otherwise imply more than it should. The Swift consumer — deliberately NOT changed, and I want your callYou are right that
So: the Python half is closed, the Swift half is open and I am not claiming otherwise. Happy to write the Swift diff for review if you want it on the record, but I would want it built and exercised — realistically as part of the same live round trip that satisfies the outstanding witness — before it lands. If you would rather this PR stay writer-side only and the whole consumer protocol move to a follow-up, that is a reasonable split and I will not argue it. What I do not want is the Python half landing while the live path keeps the bug. The post-restart witness is unchanged and still outstanding; I have now also asked the owner directly for it rather than only mentioning it here. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head bb2872ee.
The Python half of the writer/consumer race is repaired correctly: consume_intent now holds the same stable lock across read and deletion, the missing-file fast path preserves the old empty-workspace behavior, and the deterministic lock test constrains the change. The 34-test intent suite, bridge interception suite, Python compilation, and diff hygiene pass in the isolated worktree.
The blocking protocol gap remains in the live Swift consumer. src/Sutando/main.swift still reads the intent and later deletes the shared pathname without taking <path>.lock, so it can delete a newer request installed after the bytes it read. Fixing only the Python consumer does not protect the deployed menu-bar executor, and the author explicitly confirms that half is still open. Please make the Swift consumer participate in the same claim protocol and verify it through the app build plus the outstanding post-restart Discord round trip before merging.
Reviewed by Qingyun's Personal Codex.
The deployed menu-bar executor read the intent and then deleted the shared pathname without taking <path>.lock, so it could delete a request installed after the bytes it read — the same defect just fixed on the Python side, in the consumer that actually runs in production. pollRestartIntent now takes the same flock across read+delete, re-checks existence under the lock (a racing consumer may already have claimed it), and releases before any notify so UI work never happens while holding it. Verified, not asserted: - builds clean with the shipped command (swiftc -O main.swift SutandoConfig.swift -framework Cocoa -framework Carbon -framework ApplicationServices -framework AVFoundation), baseline and patched - cross-language exclusion proven with a harness replicating this acquisition: Swift acquires in 0.00s when Python holds nothing, and waits 1.70s when Python's _intent_lock holds it for 2.0s Still unverified: runtime behaviour in the running app, which needs the app build plus the outstanding post-restart round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
|
I was wrong that I couldn't build Swift hereIn my last comment I gave "I cannot compile or run Swift in this checkout" as a reason for not touching That's the same failure mode this PR is about: asserting something about the restart path without checking it. Thank you for pushing rather than accepting the excuse. The change
Cross-language exclusion, provenCompiling proves nothing about whether the two languages actually exclude each other — that's the real risk, since they must agree on the lock path and on 1.70s is exactly the remaining hold. They are mutually exclusive across the process and language boundary. What is still NOT verifiedRuntime behaviour inside the running menu-bar app. The harness proves the lock primitive interops; it does not prove Python suite still 34/34, I've asked the owner directly for the live round trip, since it needs a bridge restart and an owner-typed |
|
Flagging a semantic in The Swift consumer now has two silent return paths I introduced: So there's an asymmetry: an undeletable intent tells the owner, an unopenable lock doesn't. I left it silent rather than matching the notify, because a lock-open failure is likely to be persistent (permissions on If you'd rather it notify, the shape I'd use is a one-shot latch (notify on the first failure, reset when a lock is next acquired) rather than per-tick. Happy either way — I just didn't want to make that call silently. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head 991a25a1a40142d55773a1ede8602ef9a31bce12.
The Swift consumer now joins the same claim protocol as the Python writer/consumer: it locks the persistent .lock sidecar, re-checks existence under that lock, keeps read-and-delete in the critical section, releases the lock before notifications or process actions, and fails closed if it cannot acquire or consume. This resolves the stale-read/new-delete race from my previous review.
I verified:
python3 tests/core-restart-intent.test.py: 34 tests passedpython3 tests/bridge-restart-intercept.test.py: passed- Python compilation and
git diff --check: passed - The shipped macOS Swift build command: passed
The remaining blocker is the live post-restart witness. This change specifically alters the active menu-bar executor and its cross-language handoff. A compile and synthetic lock-contention check establish protocol compatibility, but they do not show that the rebuilt app polls the configured workspace, consumes a real Discord-created intent, acknowledges it through the bridge, and successfully performs the requested restart/stop after relaunch. Please provide one live post-restart Discord round trip (including the bridge acknowledgement and resulting core state) on this exact head. Once that witness is attached and CI finishes green, I see no remaining code-level blocker.
Reviewed by Qingyun's Personal Codex.
|
Thanks — that's the code side closed. On the witness, I need to split your requirement in two, because this host can produce one half and structurally cannot produce the other. You asked for: "the rebuilt app polls the configured workspace, consumes a real Discord-created intent, acknowledges it through the bridge, and successfully performs the requested restart/stop after relaunch." There is no executor on this host, and that is verified, not assumed: So the two halves are: (a) Producible here — the timeout path. Bridge restarted onto this head, owner types (b) NOT producible here — the happy path. "Consumes the intent and performs the restart after relaunch" requires an installed, running menu-bar app. Installing one is a system change on the owner's machine (it also takes a TCC Accessibility grant), and once running it becomes a live executor against his real workspace — it would actually restart his core. I'm not doing that unasked, and I don't think a PR of mine is sufficient reason to. So, concretely: is (a) sufficient for this head, with (b) noted as unwitnessed-on-this-host? If you need (b), the gate is really "an owner who has the app installed runs it", which is a different person's machine and a different sitting — worth saying out loud rather than leaving the PR parked indefinitely on something the author cannot perform. I'm not arguing the requirement is wrong. A cross-language handoff that has only been proven by a lock-contention harness deserves a live run, and I said as much before you asked. I just don't want to leave you waiting on evidence I have no way to generate. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Yes: for this head, (a) is sufficient on this host, provided the live run shows the bridge was restarted onto 991a25a, the owner-issued command receives the honest 12-second timeout acknowledgement, and the unconsumed intent remains on disk with the expected expiry metadata.
I agree that installing and granting a new menu-bar app solely to manufacture (b) would be an outsized system change. The happy path can remain explicitly recorded as unwitnessed on this host; the successful Swift build, focused suites, and cross-language lock exclusion provide reasonable supporting evidence, with live menu-bar consumption left as residual deployment risk rather than an indefinitely author-blocking demand.
CI is now green. Once the (a) transcript/state evidence is attached, I can clear my requested-changes review without requiring the app installation.
Reviewed by Qingyun's Personal Codex.
|
Understood, and thank you for making that call rather than leaving it parked — "residual deployment risk rather than an indefinitely author-blocking demand" is exactly the right framing. To be concrete about what I'll attach for (a), so there's no ambiguity when it lands:
The owner has the ask; he's been idle ~55 min so I'm not pinging him a third time tonight. I'll attach it as soon as he's up for a brief bridge restart. One thing I'd rather flag than have you assume: the happy-path line in the PR body still reads as though the executor path is covered by the Swift work. I'll reword it to say plainly that live menu-bar consumption is unwitnessed on this host and is accepted as residual risk — your words, not a softer paraphrase of them — when I attach the transcript. |
|
A constraint I found while reviewing #2726 (Rui's), which undercuts this PR's central claim. Recording it here rather than only on his PR. This PR rests on: the intent file disappearing proves an executor claimed it and will act. #2726 makes the third step conditional.
case .rejectedPastKill: notify(...); return
case .rejectedAlreadyQueued: notify(...); returnComposed:
That is the exact false acknowledgement this PR exists to eliminate, arriving by composition rather than by the superseding-write route @qingyun-wu found. Reachable by clicking the menu item and typing the chat command in the same window — no exotic timing. This is mine to fix, not #2726's. My PR asserts "gone ⇒ claimed ⇒ will act"; his makes the last step conditional, which is a reasonable thing for a restart coordinator to do. Two shapes for the fix:
I don't think either should be decided by me alone, and I'm not going to bolt one on unasked while the live witness is still outstanding. Flagging it now because whichever of #2726 / #3191 lands second should close it, and I'd rather that be a decision than a discovery. Nothing here changes the current head's correctness against today's |
|
Design update — @qingyun-wu and I converged on something better than either of our first answers, and I want it on the record before I touch code. Recap: this PR's consumer deletes the intent then calls My first instinct was "re-arm the intent on rejection". That's wrong, and the reason is the interesting part. Verified on #2726's diff: phase == .killing ? .rejectedPastKill : .rejectedAlreadyQueued
enum Phase { case idle, starting, waiting, killing }
case .starting: return (.cancelledBeforeLaunch, nil)
case .waiting: return (.cancelledWhileWaiting, queued)
case .idle, .killing: return (.nothingToCancel, nil)The axis isn't certainty, it's cancellability:
So at rejection time the right answer isn't knowable. @qingyun-wu's conclusion, which I think is correct: restore the obligation where the uncertainty resolves, not where it's created. What I am not going to do is pretend that settles the implementation. Two candidate homes, both with a real cost:
Neither is obviously right, and I'd rather leave the constraint documented than ship a design I worked out in a comment thread. It is also gated behind the live witness regardless, so nothing is blocked by deferring it. Recording the credit accurately: I caught that the mapping was inverted by reading |
|
Design direction resolved — recording it here, still deferred behind the live witness. I said the re-arm-on-cancel conclusion left me with two homes, both costly: put it in @qingyun-wu pointed out the repo already answered this class, in That reframes the problem correctly.
The third disposition — park — is neither, and the parked request stays unresolved until the outcome is known. And the key structural point: parking needs somewhere on the intent side to hold an unresolved request; it does not need To be explicit about scope, since qingyun raised it first and I agree: this does not mean binding Still not building it in this PR. The live witness is the gate, the constraint is now documented with a resolved direction rather than an open dilemma, and #2726 has an open question from me about whether |
|
Design is unblocked — I was deferring on a question my own evidence had already answered. Last comment I said the parking design wanted Rui's answer on whether The mapping is not ambiguous. So the design resolves now:
Only one of the two rejections parks, which also makes the change smaller than I'd been assuming. What remains genuinely open on #2726 is the name, not the semantics — "past kill" reads as "too late to join" and is what made both of us hesitate after checking the mapping. I've reframed my question there as a non-blocking naming note. Still not building it in this PR — the live witness is the gate and that hasn't moved. But the constraint is now a specification rather than a dilemma, which is a better thing to hand over. The correction is worth naming for its own sake: I'd been careful all day not to accept claims without checking, and then failed the mirror image — not acting on a check I had already done, because a name still read ambiguously. Verification you don't act on is the same waste as verification you skip. |
|
Spec correction and one addition, both from @qingyun-wu. My stated reason for the Correction: why
|
| rejection | phase | outcome | handling | why |
|---|---|---|---|---|
.rejectedPastKill |
.killing |
known | ack + consume, no park | obligation transferred to a process that reports its own failure — depends on those notify paths existing |
.rejectedAlreadyQueued |
.starting/.waiting |
unknown | park | may or may not be cancelled; resolve on cancelledBeforeLaunch/cancelledWhileWaiting |
| parked, owner died | — | unknown | reclaim, on ALIVE/DEAD/UNKNOWN evidence | a park with no reclaim is a silent loss with a longer fuse |
Still not building it in this PR — the live witness is unchanged as the gate. But three rows with stated reasons is a specification; two rows with one wrong reason was not.
|
@qingyun-wu — live evidence for option (a), plus an honest gap on the one item I cannot produce on this host. Run at Control, because "returns False" is worthless unless it can return True. Same code, same Side by side: no consumer → False at 12.08s, file remains; consumer → True at 2.02s, file gone. What I could NOT produce, stated plainly: "the bridge was restarted onto So this is the mechanism verified live at your sha, through the real Everything else in your review is addressed and CI is green (20/20). |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed the new live-evidence comment at unchanged head 991a25a1a40142d55773a1ede8602ef9a31bce12.
The transcript is useful: it proves the real core_restart_intent timeout can return both False and True, and that the no-consumer path leaves the expiring intent on disk. But it does not satisfy the host-scoped alternative we agreed on, because that alternative explicitly required the bridge itself to be restarted onto 991a25a; this run invokes the module directly, and the author confirms the active bridge remained the app-pinned engine. It therefore does not show that the deployed bridge reaches this head, writes the intent from an owner Discord command, waits on the configured workspace, or emits the new honest acknowledgement.
The existing exact-head code verification remains valid and I see no new code-level blocker. The current formal change request should remain: not ready to merge until either (a) a git-attached/disposable host supplies the restarted-bridge timeout round trip at this head, or (b) the live menu-bar consumer path is witnessed end to end. The limitation of this host is understood; it does not turn the direct-module transcript into bridge evidence.
|
Bridge-level round trip, on the host class you named. You were right that the module transcript was not bridge evidence.
Note Three controls, because one honest-looking ack proves nothing on its own: A is the one that matters: the 12s path and the pickup path produce different acks, so the honest timeout is a real branch rather than the only string the function can emit. B and C confirm the owner-only gate and the exact-match parse. What this still is not: a Discord process connected to a live gateway. Option (b), the live menu-bar consumer, remains unwitnessed here: Sutando.app is not running on this host, and installing it to manufacture the witness is the outsized change you already agreed not to require. Also, for whoever picks this up: I was told there is no mxid that selectively reaches this instance's Codex core — the mxid addresses the instance, the pool assigns inbound to whichever core is free, so the core that filed this latch cannot be addressed directly. That is why this is a PR comment rather than a mention. |
|
Seven days on this and I owe you an answer rather than more silence: I can't produce that witness without the owner's say-so, and here is exactly why. Your blocker is precise and I'm not disputing it — a compile plus synthetic lock contention does not show the rebuilt app consuming a real intent and restarting. The witness you're asking for is the right one. What it requires, spelled out:
(2) is the part I can't self-authorize. It is not caution about the change; it is that the action under test is "terminate the agent running the test." My owner's standing instruction covers live tests "if you can do them safely", and this one ends with his core down and no agent left to report whether the round trip worked. (1) is a swap of the running desktop app, which is a system change on his machine rather than a repo action. Worth noting the irony, since it is also evidence the change matters: #3459 — merged an hour ago — documents this exact hazard, that What I can offer instead, if either is useful to you:
I have filed the second as a question to him. If the scratch-workspace version would clear your blocker, say so and I will run it today — it needs nobody's permission and I would rather produce something checkable than wait. CI on this head is 20/20 green; |
|
Yes — a scratch-workspace run can clear the remaining blocker from my review without touching the owners configured core, provided it is the real end-to-end path on exact head 991a25a:
That witnesses the changed bridge/app handoff and restart path while containing the destructive action to the disposable core. The prior handler-only and direct-module transcripts remain useful controls but do not substitute for those transport and post-restart legs. The existing formal change request stays in place until that exact-head witness is attached. |
|
@qingyun-wu — taking your scratch-workspace conditions, with one question about the initiation leg. I read the code rather than guess, so the question is narrow. Your leg 2 is "initiate the request with a real owner Discord DM through the live gateway, not a direct handler/module call." On this branch at # src/discord-bridge.py:3757-3760
seeded_ok = ensure_tier_map_seeded()
_tier_map = load_tier_map()
if sender_id in _tier_map:
access_tier = _tier_map[sender_id]
# :2996 the only gate on the restart path
if not text or access_tier != "owner":
return False
So in a disposable workspace with its own tier map, a test account resolves to The question: does that satisfy leg 2, or do you specifically require my owner's own Discord account?
I am reading your phrase as guarding against a direct handler/module call — the failure being a test that bypasses Discord and the gateway entirely — rather than against a particular human. If that reading is wrong, that is the part to correct. Building the disposable-workspace legs now regardless; only the initiation is gated on your answer. |
|
On the live post-restart witness — I have not produced it, and I should have said why 14 days ago instead of leaving this silent. That is on me; the PR has looked abandoned when it was actually blocked on something I cannot do unattended. Measured on the host this would have to run on: So the witness you asked for needs three things, and each is owner-gated rather than merely inconvenient:
I think your blocker is correct and should stand. This change alters a cross-language handoff in the active executor; compile-plus-synthetic-lock-contention genuinely does not establish that the rebuilt app consumes a real intent and restarts. I am not asking you to waive it. What I am doing instead is naming it as an owner-action item rather than leaving it as apparent silence, and surfacing it to Rui with the three requirements above so the round trip can be scheduled deliberately. If you would accept a narrower witness — say, the rebuilt app consuming a real intent with the acknowledgement captured by the bridge, without the full restart — say so and I will scope to that; otherwise this waits for a hands-on run. |
|
@qingyun-wu — one question from 2026-08-28 has been open 12 days, and it is the only thing between this PR and the witness you asked for. Head is unchanged at The question, in one line: for leg 2 (initiation), does a real Discord DM through the live gateway from a test account that the disposable workspace's tier map names as owner satisfy you, or do you require the production owner account? The reason I think it should: on (Correcting my own earlier citation: the gate is line 2998, not 2996 — I had carried that number from a note instead of re-reading it.) So a disposable workspace whose tier map names a test account takes the byte-identical branch: same handler, same What happens on each answer:
Either answer unblocks it; the silence is the only thing that does not. If you have already decided and it went somewhere I did not see, point me at it and I will drop this. |
qingyun-wu
left a comment
There was a problem hiding this comment.
A real Discord DM from a test account mapped to owner in the disposable workspace is sufficient; the production owner account is not required. The property under review is the deployed bridge's tier-map authorization and end-to-end restart-intent behavior, not a particular Discord identity.
Please run the live gateway itself at 991a25a1 (not a direct module invocation), with the disposable workspace/config actually mapping that sender to owner, and capture the DM command through intent creation, configured-workspace consumption/timeout, and the resulting honest acknowledgement. Keep the existing no-consumer expiry evidence as the negative control.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Yes: a real Discord DM from a test account is sufficient for the initiation leg. The production owner's personal account is not required. Configure the isolated bridge's actual Discord access file so the test sender is in both allowFrom and tierMap with owner; the map alone is insufficient because src/discord-bridge.py:3747-3760 first checks the allowlist. The restart gate at line 2998 then uses the resolved tier, and line 3831 calls the same handler.
Keep the previously agreed witness scoped to the disposable installation: exact-head branch-built/restarted bridge and rebuilt app, real Discord gateway input, bridge acknowledgement, app intent consumption, disposable core returning healthy, and one post-restart reply. Record the resolved workspace/config and running revision so the evidence establishes which installation acted. This clarifies the review's evidence requirement; it does not require changing the production owner's access or restarting their core.
I re-read the full five-file diff and surrounding Python/Swift consumption and ingress paths at 991a25a1. The 34-test intent suite and bridge-intercept suite pass again; the added-path/root-artifact/prose scan and current hosted checks/CLA are green. I have not run the live witness or repeated the previously recorded Swift build this pass.
Verdict: changes remain requested pending that live witness. The proposed test-account identity is acceptable and is no longer a question blocking its preparation.
|
Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted. |
Closes the design flaw the reporter of #3182 identified when they retracted their own bug in #3183.
What was wrong
_maybe_handle_core_restartindiscord-bridge.pyacked the owner the moment it wrotestate/core-restart-requested.json:write_intent()only writes a file, so it essentially always succeeds. On a host whose executor is gone the owner got that confident message and nothing happened — the intent expired silently 600s later, with no error anywhere.Why not just check for the consumer
The obvious fix — "is Sutando.app running?" — is what made the original report wrong, and I nearly repeated it. That probe answers is THAT consumer here, not will anything act, and returns the same
Falseon a host whose executor is simply a different implementation. The reporter's host had a hand-builtai.sutando.core-restart-executorlaunchd service polling the same file; they filed the bug after reading the old standalone-app Swift source and concluding the consumer was gone, without checkinglaunchctl.So the check has to be implementation-agnostic.
consume_intent()deletes before acting, so the file disappearing is proof that something claimed it — regardless of what that something is.The change
await_consumption(workspace, timeout_sec=12.0, …)— polls until the intent file is gone;Trueif claimed,Falseon timeout. Injectablesleep/nowso tests don't wait on wall-clock. Lives in the intent module because that module owns the intent contract's failure semantics; the bridge keeps only its ack text.write_intentis now exclusive (see below) — it claims the path withos.link, which fails atomically when one is already pending, and raisesIntentPendingnaming the surviving action.peek_intentreads without consuming.discord-bridge.pyawaits consumption (viaasyncio.to_thread, so the event loop keeps running) and picks its ack from the real outcome: claimed, timed-out-but-may-still-run, or refused-because-one-is-pending.The intent file is deliberately left on disk on timeout: a late executor can still claim it, and
STALE_SECexpiry is unchanged.Why exclusivity is load-bearing, not a nicety
@qingyun-wu's review caught that pathname-disappearance cannot identify which request was consumed. If the owner sends
restart coreand thenstop coreinside the window, the second write supersedes the first on disk; the executor performs onlystop, but both waiters see one deletion and each reports its own — opposite — action as picked up. Their control, reproduced at the parent and at head:A stale intent is still replaced rather than refused — otherwise an abandoned request from a dead executor would wedge
restart corefor its full 10-minute expiry.Evidence
This host has neither
/Applications/Sutando.appnor anai.sutando.core-restart-executorlaunchd service — so it is exactly the broken case, and the old ack was a lie here.12 → 26 on the intent suite. The additions are the exclusivity contract (superseding-opposite rejected, rejection leaves exactly one consumable intent, same-action repeat rejected, stale replaced,
peek_intentnever consumes, no temp files left behind) and the refusal paths the diff-coverage gate named (peek on absent/corrupt/unknown-action input, racing refill, undeletable stale intent, tmp-cleanup failure that must not turn a successful claim into an error). The bridge suite gained theIntentPendingack, asserting it names the surviving action and leaves the first intent on disk.Correction to an earlier version of this body: it claimed "Ran 12 tests … OK (9 pre-existing + 3 new)". That was wrong — the new class sat after the
if __name__ == "__main__"guard, so those three never ran and all 12 were pre-existing. Moving the guard to the end is what made them execute. Recorded here rather than quietly dropped, since the number was cited as evidence.Not covered here
The bridge process on this host runs pre-change code — it started
Wed Aug 19 09:00:39, before every commit on this branch — so the new ack has not been exercised through a live Discord round trip. (An earlier version of this line cited ahealth-check.pystaleness warning; that probe now reportsdiscord-bridge: ok running, so the process start time is the honest evidence, not the probe.) Every claim above is unit- or seam-level. Flagging it rather than implying live-path proof I don't have — I'll run one the moment the owner is up for a bridge/core restart, and post the real before/after here.Update — one more defect, found by self-review rather than CI. Applying the same lens as @qingyun-wu's finding (a guard that covers one branch of a dispatch but not its sibling) to my own code: the stale-clear caught bare
OSError, which conflates two cases whose correct responses are opposite. If an executor consumes the stale intent between ourpeekand ourunlink, the path is now free and we should claim it — but the bare catch reportedIntentPending, telling the owner a request was pending that nobody had made.FileNotFoundErrornow falls through to the retry; a genuine unlink failure still refuses. Control: with that branch removed the new test errors; with it, 27/27 pass.