Repository navigation
Conversation
f7562cd to
6aac8f4
Compare
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Cleanup can delay delivery of an already-confirmed send result. Native packaged-app acceptance remains unverified.
| .map_err(|error| publish_error(&event_id, error)) | ||
| .map_err(|error| publish_error(&event_id, error))?; | ||
| if let Some(root) = root { | ||
| crate::host_files::release_after_send( |
There was a problem hiding this comment.
🤖 [P2] Return the confirmed send before waiting for cleanup. confirmed_post awaits release_after_send, which can wait on the shared BYTE_WORK permit for its 30-second budget. A concurrent plugin upload, download, or managed save can therefore keep an already-accepted event's sent <id> response pending and the runner in work.sending(). Decouple best-effort cleanup from the confirmed response while preserving its owned deadline/admission/exclusion guards, and add a regression that holds the byte permit while confirming a send. This delay follows from the control flow; external-client timeout or duplicate-send consequences have not been reproduced.
There was a problem hiding this comment.
🤖 Fixed in 1bff02c. After the full native evidence wrapper confirms relay acceptance of the exact signed event, confirmed_post detaches cleanup on the app runtime and returns that event without waiting for BYTE_WORK. The existing runner then returns sent and clears work.sending(). The cleanup deadline is captured before spawning (scheduling/queue time included); existing actual-work byte/root/RAII reader-exclusion guards remain owned through completion. Cleanup failure cannot change the returned result; opt-in/default-off and non-Unix source retention are unchanged.
Red-first regression host_release_confirmed_send_returns_before_cleanup_byte_admission_and_preserves_evidence holds the real byte permit: it failed the two-second response bound on the old await, now returns the exact ID while the permit remains held, then verifies cleanup settlement after release. Runner test returns sent and leaves sending on native confirmation without waiting for detached cleanup covers the send-tool/runner/activity boundary. host_release_deadline_starts_before_first_poll_and_cancelled_queue_never_unlinks covers deadline capture and queued cancellation.
Runner disposal does not cancel app-runtime cleanup. App shutdown need not drain it: undispatched work can be lost with sources retained; already-dispatched unlink can finish with unknown outcome while actual-work guards remain held. No durable queue or app-outliving guarantee. Local affected suites and normal full hooks/locked all-target Clippy passed; exact-head CI is green (39 SUCCESS / 2 SKIPPED), https://github.com/block/buzz-app/actions/runs/41771670064. Packaged/Wry, human and Windows acceptance remain unverified.
| } | ||
|
|
||
| #[tokio::test(start_paused = true)] | ||
| async fn managed_gate_entry_has_a_diagnostic_bound_even_if_sender_stays_alive() { |
There was a problem hiding this comment.
🤖 [P3] Remove the harness-only watchdog test. managed_gate_entry_has_a_diagnostic_bound_even_if_sender_stays_alive only exercises the test helper wait_for_managed_gate, so it adds test maintenance without protecting production behavior. Retain the helper's diagnostic watchdog and the tests that exercise real cleanup, refusal, cancellation, and reader exclusion.
There was a problem hiding this comment.
🤖 Removed only managed_gate_entry_has_a_diagnostic_bound_even_if_sender_stays_alive in 1bff02c. The wait_for_managed_gate diagnostic watchdog and its four real gate-entry callers remain, along with real cleanup/refusal/cancellation/reader-exclusion tests. New real-file regression host_release_deadline_starts_before_first_poll_and_cancelled_queue_never_unlinks protects production deadline/queued-cancellation behavior rather than just the helper. Held-permit regression host_release_confirmed_send_returns_before_cleanup_byte_admission_and_preserves_evidence also passes. Local host_131/app_agents25 and normal full hooks/locked all-target Clippy passed; exact-head CI is green (39 SUCCESS / 2 SKIPPED), https://github.com/block/buzz-app/actions/runs/41771670064. Leaving thread resolution to you.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One nonblocking test-usefulness finding. The prior confirmed-send delay and harness-only watchdog findings are resolved.
| expect(f.run).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns sent and leaves sending on native confirmation without waiting for detached cleanup", async () => { |
There was a problem hiding this comment.
🤖 [P3] Remove the test-local cleanup assertion. cleanup, finishCleanup, and cleaned are only connected to one another inside this test; neither the service nor the native.publish mock observes them. Resolving that promise therefore cannot detect a regression in detached native cleanup. The remaining sent-response/sending-state assertions already have publication coverage at lines 1740, 1776, and 1811. Delete this redundant test and retain the native held-byte-permit regression, which exercises the actual response-versus-cleanup boundary.
There was a problem hiding this comment.
🤖 Removed exactly that redundant JS test in 09c11e9. The existing publication success/failure/uncertain-outcome tests at 1740/1776/1811 and native held-BYTE_WORK regression host_release_confirmed_send_returns_before_cleanup_byte_admission_and_preserves_evidence are unchanged. The deleted test's deferred response-gate teardown note is now moot.
Also strengthened host_release_deadline_starts_before_first_poll_and_cancelled_queue_never_unlinks to require immediate Ready on the first poll after explicit expiry while the byte permit is held. A compiling first-poll-deadline mutant fails the new assertion; restored unchanged production passes. No production/dependency/lockfile change.
Affected native suites, six JS files, types/fmt/Biome, locked all-target Clippy and normal full hooks passed. Exact-head CI is green, 39 SUCCESS / 2 SKIPPED: https://github.com/block/buzz-app/actions/runs/41838813924. Leaving thread resolution to you.
09c11e9 to
648a518
Compare
Record bounded process-local same-owner consent at managed save, pass snapshot-matching attachment hints outside signed events, and release exact verified sources only after native publication confirmation. Keep cleanup best-effort and preserve confirmed send success. Co-authored-by: goose <goose@block.xyz> Signed-off-by: Zach Marley <zmarley@squareup.com>
Gate verified-unlink success coverage to Unix, exercise confirmed-send retention on non-Unix, and bound native fixture gate entry with a red-first diagnostic regression. Narrow the explicit-sweep test wording without changing production identity checks. Co-authored-by: goose <goose@block.xyz> Signed-off-by: Zach Marley <zmarley@squareup.com>
Return the evidence-confirmed exact event before cleanup byte admission. Capture the owned cleanup deadline before spawning on the app runtime, retaining existing actual-work admission and exclusion. Cover held byte admission, runner sending completion, and expired or cancelled cleanup; remove the harness-only watchdog test. Co-authored-by: goose <goose@block.xyz> Signed-off-by: Zach Marley <zmarley@squareup.com>
Require immediate first-poll completion after cleanup deadline expiry, preserving real permit, file, capacity and cancellation assertions. Remove only the redundant test-local JS cleanup model; existing publication and native held-byte-permit regressions remain. Co-authored-by: goose <goose@block.xyz> Signed-off-by: Zach Marley <zmarley@squareup.com>
648a518 to
1c5a5f4
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head 1c5a5f46a475447d0402890a87bd3158eed617ef, target base 8c3bf75dd50fdbd13fcd98df0fe5adfc41766525 (merge-base 43747aeb01eb8c6801028ab68b7011e0b8065bcb).
No actionable correctness or security defects found in the reviewed change. The prior confirmed-send delay is resolved in source: the full native evidence wrapper confirms the exact event ID before cleanup is detached, so byte admission cannot delay the sent result. Reviewed consent/file matching, command cancellation and identity fencing, actual-work reader exclusion, and the shared cleanup deadline. The redundant test-local cleanup model was removed; source-inspected tests cover real native boundaries, including held byte admission and first-poll deadline expiry.
Optional documentation note: the description’s “Current head” and base still name 648a5186 / e121e527, which differ from the revision above. The linked CI run 41893748047 also belongs to that older head. Refresh those references and bind each validation claim to its actual snapshot; the independent current-head check snapshot below is separate evidence.
Source-only review of pinned Git objects, callers, regression-test source and public PR material; no local tests, builds, app launches or PR code execution. No attached images were present. A one-time hosted-check snapshot at this head reports CI required successful; Windows native validation and the unselected browser-flake lane were skipped. Packaged desktop/Wry, real media-server, native focus/accessibility, Windows and human acceptance remain unverified. Best-effort cleanup is not guaranteed erasure or a remote-storage guarantee. This COMMENT review is not approval or merge authorization.
What changed
Add default-off, best-effort release of temporary managed attachment sources after a natively confirmed post.
host.saveBytes(bytes, {sha256, releaseAfterSend: true}). Native must acknowledgereleaseAfterSend === true. Missing acknowledgement means an older host may have saved without consent: possibly saved, do not retry.No new dependency, grant, IPC command, durable service, timer, sidecar, consumer enablement or FOUNDATION entry-point change. Merge-base 43747ae; 21 existing files, +2331/-101, mostly test additions. The earlier Kalvin follow-up at
09c11e9352d45d1006c6b1edeac7e68b907c3a13changed two test files, +10/-71, with no production change.Best-effort semantics and limits
One message shares one 30-second cleanup-entry budget, captured before spawning and including scheduling/queue wait, using existing byte admission/root ownership and app-wide reader exclusion. The confirmed send does not wait for cleanup. Cleanup refusal/error never turns that result into an error or repost. Already-dispatched blocking I/O keeps its actual-work byte/root/reader guards through completion and may outlive the budget with an unknown deletion outcome.
Detached cleanup belongs to the native app runtime, not the runner: runner disposal does not cancel it. App shutdown need not drain cleanup. Undispatched work may be lost and sources retained; dispatched unlink may complete with unknown outcome. No durable work or guarantee of outliving the app.
Cleanup is not eventual erasure. Restart loses consent; refusals can retain sources indefinitely. Expiry only permits removal during a later-save sweep. Keep the posted link for reuse rather than reading a released path again. New app-wide reads promptly refuse while cleanup holds exclusion.
The app's read/upload mapping is trusted, not independent cryptographic lineage. Media transformations/hash mismatches safely omit cleanup. Windows keeps sources because safe platform file identity is unavailable. Windows was not executed or cross-compiled locally: only aarch64-apple-darwin is installed. The hosted Windows lane is manual-only and was skipped; it is not Windows-green evidence. Non-Unix confirmed-send retention coverage is present, and Unix-only unlink/hash success tests are gated accordingly. Production identity checks are unchanged.
Consumer opt-in remains separate, after a compatible released frontend/native pair and acceptance.
Validation and review
Current head: 1c5a5f4, based on main 43747ae (verified merge-base). The PR is ready for maintainer review but currently conflicts with newer main. It will be refreshed onto current main after maintainer review; test-only conflict resolution is expected. That future snapshot will need its own validation.
Current-head evidence — 1c5a5f4
43747aeb01eb8c6801028ab68b7011e0b8065bcbresolved one additive EOF conflict insrc/buzz-mcp/tools.test.ts, retaining the complete main Canvas boundary test and feature attachment tests. Only conflict markers were removed; no assertions, helpers or production behavior were edited. All 21 feature files retain identical ordered added/removed line bytes versus the previous snapshot. The automatic documentation, Canvas, native agent and runner/service merges were checked for preservation of both changes.Earlier-snapshot evidence — not new runs on the current head
host_131 passed (35.45s) andapp_agents25 passed (1.33s); these filters overlap and are not unique-test totals. These are earlier local native results, not a current-head native rerun.RwLockReadGuardis!Sendacross actual-work boundaries.1c5a5f46.The historical native IPC-watchdog failure remains unresolved. During the earlier d182ac5 investigation, one five-second fixture timeout caused 19 poisoned-lock followers; independent repetition passed 20/20 native invocations. A later independent review at ddeabaf passed 3/3 exact-fix-head parallel runs. Those passes neither identify the stalled operation nor prove the failure absent. No production/test timeout was increased or assertions weakened to hide it. Historical unchanged Profile timeouts also remain part of the earlier validation record; later normal hooks passed without test edits.
Remaining acceptance
The current-head automated review is a COMMENT, not maintainer approval. Earlier reviews and runtime evidence apply only to their named snapshots. Current-head CI success does not establish the untested surfaces below.
Packaged desktop/Wry and trusted HTTPS, a real media server, native focus/accessibility, Windows, and human acceptance remain unverified. No full-workspace local Rust test run is claimed; browser coverage here is hosted CI, not a local packaged-app exercise. No browser tests were added or removed; not every race/queue permutation has its own fixture.
Suggested acceptance in a safe disposable account/VM: send an opted-in source through direct and process readers; verify only an exact-ID confirmed post releases it on supported platforms, an ordinary sibling remains, and posted media stays usable. Verify upload/unconfirmed-send/active-reader refusal and restart retain sources without automatic retry or consent adoption.