Compute UserBlockCompleted's expensive fields lazily

[C0W0]

Summary

UserBlockCompleted::serialized_block, command, command_with_obfuscated_secrets, output_truncated, and output_truncated_with_obfuscated_secrets were previously computed eagerly for every completed block, even when a subscriber never reads them.

• Introduces a small Lazy<T, S> utility (app/src/util/lazy.rs) that computes and caches a value from a &S the first time it's read, and never again.
• UserBlockCompleted's fields become public Lazy<T, BlockList> fields. Each field's deferred compute closure captures the completed block's stable BlockId (not BlockIndex, which can go stale after block removal/reindexing — e.g. clearing the screen) so the live Block can be re-resolved from a &BlockList on first read.
• Two access patterns, depending on what the caller already has on hand:
  • field.get(&block_list) when a &BlockList is already available.
  • field.get_with(|compute| { let model = terminal_model.lock(); compute(model.block_list()) }) when only a FairMutex<TerminalModel> is available. get_with hands the caller a compute callback so the lock guard can be scoped locally around the call — this sidesteps the "returning a borrow out of a closure" problem a simpler FnOnce() -> &S signature would hit. The lock is only ever acquired the first time a field is read; cached reads never lock again.
• Updates every consumer (view.rs, input.rs, legacy.rs, maa.rs, block_context.rs, next_command_model.rs, aws_credentials.rs, current_prompt.rs, open_in_warp.rs, blocks_tests.rs, and warp_tui's terminal_session_view.rs) to use these accessors.
• Also fixes an unrelated pre-existing bug found along the way: get_same_commands_from_history (in persistence/commands.rs) now correctly returns commands newest-to-oldest as documented, instead of double-reversing relative to its caller.
• Bundles CurrentPrompt::new_with_model_events's model_events and terminal_model parameters into a single Option so the two can't disagree.

Correctness considerations

• Stale-index audit: resolving by BlockIndex (the original design) risked silently returning a different block's data if the original block was removed and its old index reassigned. Added a regression test (deferred_fields_resolve_by_block_id_not_stale_index) that reproduces this exact scenario via clear_screen(ClearMode::ResetAndClear) and asserts the stale block's fields correctly report as missing rather than resolving to the unrelated block now at the same index.
• Deadlock audit: since get_with closures lock TerminalModel on cache miss, any call site that already held that lock while calling an accessor would deadlock (the mutex isn't reentrant). Audited every consumer for this; found and fixed one real instance in maa.rs's handle_user_block_completed, which was holding the lock across a call to BlockContext::from_completed_block — restructured to resolve what's needed from inside the lock, drop it, then call the accessors.
• get_with's single-invocation guard: get_with takes with_source: FnOnce(&dyn Fn(&S) -> T) -> T — the inner compute callback must be Fn (not FnOnce) since it's passed by reference into caller-controlled scope, but the underlying compute closure it wraps is only safe to run once. A Cell-guarded take (rather than an atomic/Mutex) is sufficient because OnceLock::get_or_init already guarantees this whole path executes on at most one thread at a time.
• Send/Sync requirements: Lazy<T, S> is Arc-backed, so Arc<LazyInner<T, S>>: Send requires LazyInner: Sync, which is why the interior mutability around the deferred compute closure itself uses a Mutex (not a Cell) — a Cell there would silently make UserBlockCompleted non-Send, breaking the channel it's sent over from the PTY reader thread to the main thread.
• Cross-crate consumers: warp_tui (a separate crate) also reads these fields; this was invisible to cargo check/cargo test scoped to the warp crate and only surfaced via that crate's own build. Fixed its one call site (emit_block_completed_telemetry).

Testing

• Targeted cargo test runs per touched module, all passing: terminal::model::blocks (65 tests, including the new stale-index regression test and test_background_blocks_finished, which exercises the lazy fields end-to-end), terminal::input (211), util::lazy (7, including new get_with coverage for lock-then-cache and skip-when-cached behavior), passive_suggestions (4), next_command_model (10), current_prompt (14), aws_credentials (5), block_context (3), open_in_warp (8).
• terminal::view has 5 pre-existing order-dependent failures (309 passing) under the full-module run; confirmed via git stash against unmodified master that these fail identically, so they're unrelated to this change.
• ./script/presubmit passes (format, inline-test-module check, clippy across the workspace); the only remaining failure is a pre-existing, unrelated missing clang-format binary in this environment.

Co-Authored-By: Warp agent@warp.dev

[oz-for-oss]

@C0W0

I'm starting a first review of this pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This PR changes UserBlockCompleted to compute expensive block fields lazily and updates consumers to resolve those fields through terminal-model-backed accessors.

Concerns

• Deferred field resolution uses BlockIndex, which is not stable after block removals/reindexing and can resolve data from the wrong block.
• get_same_commands_from_history now reverses its result while the existing caller still reverses it again, changing the intended oldest-to-newest processing order.
• One new API doc says serialized_block is supplied up front even though the implementation defers it, making the public contract inaccurate.

Verdict

Found: 0 critical, 3 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[acarl005]

This seems like something std::sync::OnceLock already provides. Any particular reason to write a custom struct instead of using OnceLock? Custom data structures may be cheap to generate with AI, but still require maintenance whereas stdlib components are completely free. It's not clear to me that this custom struct is adding any additional protections or invariants either.

[C0W0]

This does use OnceLock in the LazyInner struct. The reason why OnceLock alone won't work is that the consumer wouldn't know how to construct the data; instead, the producer must specify the closure used to build it when consumer invokes get for the first time. If you checkout LazyInner itself, it just consists of the OnceLock and the compute closure

[C0W0]

• Use faster hash maps and precomputed hashes in hot paths #15279
• Compute UserBlockCompleted's expensive fields lazily #15162 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[C0W0]

/oz-review

[oz-for-oss]

@C0W0

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This PR defers several expensive UserBlockCompleted fields behind a shared Lazy<T, BlockList> cache and updates consumers to resolve those fields from the terminal model on first use. I did not find security issues, and spec_context.md contains no approved or repository spec context to compare against.

Concerns

• Missing-block lazy resolution currently caches T::default(), which lets callers continue with synthetic empty/default metadata when the original completed block is gone instead of forcing them to skip that event.
• Several new private helper doc comments restate what the helper names already say, which does not meet the repo's comments guidance.

Verdict

Found: 0 critical, 2 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

⚠️ [IMPORTANT] Returning T::default() here caches an indistinguishable empty/default value when the completed block has been removed before the first lazy read, so callers that read serialized_block can still proceed with synthetic metadata instead of skipping the missing block. Make the lazy result optional or preserve the completed-block metadata needed by downstream callers so missing blocks cannot be processed as real empty/default blocks.

[acarl005]

Suggested change

	pub fn provided(value: T) -> Self {
	#[cfg(any(test, feature = "test-util"))]
	pub fn provided(value: T) -> Self {

Looks like this is also test-only.

[C0W0]

yes for now - but I want keep this in case we want to use it in the future.

[acarl005]

Suggested change

	/// Returns the value, computing (and caching) it from `source` first if necessary. `source`
	/// is only consulted (and the compute closure only invoked) the first time this is called;
	/// later calls return the cached value directly.
	pub fn get(&self, source: &S) -> &T {
	self.0.cell.get_or_init(|| {
	let compute = self
	.0
	.compute
	.lock()
	.take()
	.expect("Lazy value has no cached value and no deferred compute fn");
	compute(source)
	})
	}

Looks like this method is unused? It's only used in tests but that's not useful to test if it's unused in production code. I think fn provided is valid even if test-only b/c it helps set up useful tests.

[C0W0]

Same as provided - I want to keep this so that we can potentially reuse Lazy<T, S> in the future in other place, where it might be feasible to just do lazy.get(&source) instead of lazy.get_with(|compute| compute(&source)). The reason I made get_with in the first place is because we want to have explicit mutex locking - otherwise get is suffice.

[acarl005]

Suggested change

	) -> T {
	) -> Result<T> {

We're logging an error and returning a T::default(), effectively swallowing an error. Callers won't actual be able to tell that the failure occurred. Are we sure this is how we want to handle the failure instead of wrapping this in either a Result or Option?

[acarl005]

Suggested change

	ai::api_keys::ApiKeyManager::handle(ctx).update(ctx, |manager, ctx| {
	ApiKeyManager::handle(ctx).update(ctx, |manager, ctx| {

super nit. convert this to use an import

[acarl005]

This type of closure gets repeated a lot.... I wish there was a nice way to reduce the repetition here. Maybe with a macro? I'm not sure. If there isn't a better way than I'm fine with this. Behavior-wise though it's pretty ideal.

[C0W0]

I was thinking about using a macro as well, but the whole idea of putting this out is to make sure the caller performs an explicit lock so that they would be aware of potential deadlock risks. That's why I kept this.