Repository navigation
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Review of exact head c682972c.
This PR is stacked on #5902. Its 8 commits are #5902's 7 (c1caceb2...717841a0, the same SHAs) plus c682972c "notify about archived-task cleanup outside Settings", and c682972c^ == 717841a0. This review covers only c682972c: 13 files, +620/-17. #5902 is reviewed separately. This PR can only merge after #5902, and it inherits #5902's epoch-205 collision with #5709.
What it does. ArchiveRetentionNotices is mounted in the app shell. While the window is visible and focused, it polls storage.retention.query on every enabled, ready Host profile, once a minute and again on profile changes or focus/blur/visibility changes. It shows three kinds of toast:
- an info toast for a new
lastDeletion, throttled to one per Host every 15 minutes by Client time; - a sticky warning toast for a hold;
- a sticky warning toast for a backward-clock pause.
Results are identified by Host time and stored in localStorage per hostId. When the Settings retention section loads, it acknowledges the results it showed, so they are not announced again.
Correctness. I found no P0-P2 issues.
- Observing never starts a Host: only
enabled && readiness === 'ready'entries are polled. - A Host whose query fails is skipped, and the other Hosts are still checked.
- When the Host list changes or the observer is disposed mid-flight, late results are discarded (
version/closedchecks). - Duplicate profiles for one
hostIdare queried once. - Warnings are cleared when the Host clears them or the Host goes away.
- Corrupt localStorage decodes to
{}, so it cannot suppress notices. - The toast action goes to
archived-tasks, which rendersArchiveRetentionSection(tasks-settings-page.tsx:92,101). - The services object is created once, so the
useEffect([services])does not restart. - A clock-skew edge: if a persisted
deletionAtcomes from a Host clock that was ahead, it does not hide later deletions, because the Host itself pauses until real time passes its recordedobservedAt. - The new test passes locally 11/11, and CI is green.
Minor (P3):
- Polling cost on hosts where retention is off.
storage.retention.queryrunscountArchiveRetentionCandidates, aGROUP BYover all archived rows joined to the projection. The notices observer runs it every 60s for every ready Host while the window is focused, and also on each focus/blur. That includes Hosts where retention is disabled, which is the default and covers nearly everyone. Those Hosts can never produce a hold or pause, and produce alastDeletiononly if retention was on before. Options: back off whenenabled === false && !lastDeletion, dropblurfrom the triggers (blur cannot make a notice presentable), or add a preview-free query mode. - Settings acknowledgement leaves sticky warnings on screen.
acknowledgeRetentionResultswrites the seen-state, but a warning toast already shown withduration: 0stays until the Host clears the hold (up to a day) or the user closes it. That is a small UX inconsistency. - Unrelated refactors in
app-shell.tsx: theactiveCatalogSessiontoactiveSessionrename, the destructuring changes, and the inlinedparentId. They look like they exist to stay within thenonTriviaTokensbudget (7333 to 7330). They are behaviour-neutral but enlarge the diff. Consider splitting them out or noting them in the description.
CI / merge: all checks pass. The PR is MERGEABLE and BLOCKED on review. It is gated on #5902.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review of exact head 3f1409c0 (previous review: c682972c).
Stacking. The PR is still stacked on #5902's current head 717841a0, with no rebase (merge-base with main is unchanged). c682972c is identical. The only new commit is 3f1409c0 "address retention notice review feedback" (5 files, +149/-14), and this review covers only that commit. It can merge only after #5902.
Previous findings
- Polling cost when retention is off: fixed. After the observer reads a disabled Host, it skips that Host for 15 minutes, including on focus and visibility changes (
retention-notices.ts:135-139,148-149). A Client clock rollback forces a read, and entries for removed Hosts are pruned. blurtrigger that did nothing: fixed. The listener is removed.- Sticky warning toast stays after Settings shows it: fixed. A new
acknowledgedWarningis set only by the Settings acknowledgement, never by the observer's own announcement. Writing it notifies the change listeners, so the observer refreshes and dismisses the matching active toast (L129-134,L168). The persisted acknowledgement survives the observer's state merge (L157). - Unrelated
app-shell.tsxrenames: unchanged, since this commit doesn't touch that file. Not repeated inline.
New: one low-priority P3, inline: the disabled backoff isn't reset when retention is enabled from Settings. No P0-P2.
Verification. CI is all green, and the PR is MERGEABLE (blocked on review). In a local build at this head, the archive-retention-notices, -section and -candidates tests pass 19/19, including the three new tests. The new value import of the feature barrel from platform/desktop creates no import cycle.
| const elapsed = disabledAt === undefined ? undefined : now() - disabledAt; | ||
| // Focus changes do not turn the default-disabled policy into a full | ||
| // candidate scan every minute. A Client clock rollback forces a read. | ||
| if (elapsed !== undefined && elapsed >= 0 && elapsed < DISABLED_POLL_MS) continue; |
There was a problem hiding this comment.
P3 (low): the disabled backoff for a Host is cleared only when a later read shows it enabled (L148). Enabling retention in Settings (archive-retention-section.tsx:123) only reloads the section, and acknowledgeRetentionResults notifies listeners only for a new acknowledgedWarning. So for up to 15 minutes after enabling, the observer won't read that Host. The impact is small: the Host deletes nothing before enabledAt + days, so only a hold or pause toast could be late, and Settings shows that banner in the meantime. The same delay applies when another client enables retention. If you want to close the gap, clear disabledReadAt for the Host after a successful setRetention (for example, by notifying the observer's listeners).
Add one opt-in, per-Host setting that deletes archived tasks after 30, 60
or 90 days. It is off by default; the Host decides and deletes, and
Desktop shows the setting and what the last sweep did.
Setting. A dedicated Host document, archive-retention.json in the State
Root, holds { version, revision, enabled, days, enabledAt, observedAt,
latest }. It is not part of the runtime policy, agent settings or config
export/import; the only writer is the new storage.retention.set command.
enabledAt is stamped by the Host clock and re-stamped by any change
(enabling, or new days while enabled), so enabling never deletes a
backlog and shortening never deletes at once; disabling clears it. A
revision CAS rejects stale writes. A document that cannot be fully
validated, including one from a newer Host, reads as disabled.
Sweep. A new HostStorageMaintenance lane, started after Ready, runs one
bounded step a second while work remains and every 15 minutes otherwise,
deleting at most 8 revision families a tick. No SQL runs before
enabledAt + days. Candidates are the rows Settings > Archived tasks shows
(archived roots and orphaned archived subtasks, no graph operators, no
preparing copies) in a family no member of which is pinned, oldest first,
with a keyset cursor so kept families do not starve the rest. The
existing (is_flagged, is_archived, ...) index bounds the scan; no
migration.
Deletion goes through session.remove's own path: #remove becomes
removal admission, after the plan is stable and before any retirement
work. It rechecks the policy (no pending change, same revision, days and
enabledAt), that every member is archived and unpinned, and that the
family's newest clock start, max(archivedAt ?? enabledAt, enabledAt), is
more than `days` old. A family whose removal would archive an active
subtask or reclaim a subagent worktree is kept as needs-review. The
existing busy guards apply unchanged and count as skipped-busy. Manual
session.remove is unchanged.
Clock. Wall time with two guards: the Host keeps the latest time it has
observed (persisted with the document), and a sweep pauses, records the
pause once and deletes nothing while the clock reads earlier than that or
than the newest committed_at/archived_at in session_metadata.
Results are latest-only: lastSweep { at, deleted, skippedBusy,
needsReview, failed, paused? } and lastDeletion { at, count, bytes? },
with bytes measured before deletion as the batch preview measures them.
The document is written only when a sweep changed something, and logs
carry counts only.
Protocol (epoch 202 -> 203): storage.retention.query returns the setting,
a Host preview (candidate families and when the first becomes eligible;
previewDays previews enabling or changing days now) and the latest
results; storage.retention.set { expectedRevision, enabled, days }
answers committed or revision_conflict. Both are renderer pass-through
and remote-owner operations.
Desktop. Settings > Archived tasks gains an Automatic cleanup section
for the selected Host (the page now shows the Host picker): a switch, the
period, the Host preview, the last automatic cleanup with its size, a
needs-review count and a paused notice. Enabling or changing the period
asks a confirm that states the Host preview. The copy says the setting
covers every archived task, starts its clock when enabled, keeps pinned
tasks and deletes permanently. The legacy page only renders the feature
section.
Refs apache#5899
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Address the adversarial review of the opt-in retention commit.
Safety
- A setting change now waits for the sweep step in flight. It marks
itself pending first, so the guard admits nothing new and the loop
stops before the next family; the family already admitted finishes
and is recorded, and only then does storage.retention.set commit.
Once set answers, no deletion admitted under the old setting runs.
- A pass records its results only under the setting revision it
started with.
- Draining stops a sweep before the next family.
- enabledAt is max(now, the observed high-water, the newest metadata
time), so enabling behind a clock that went back cannot backdate the
deadline; any setting change clears a recorded pause.
- A candidate row that no longer decodes is returned as such, counted
as failed once per pass and passed over, so it cannot wedge a sweep.
One authority
- The guard's worktree check uses the count the removal preview uses
(one shared helper, so no worktree executor means none reclaimed).
- readSessionArchiveTimes is gone; the guard reads archivedAt through
readCatalogRecord, the reader the manual age guard uses.
- The archived-task row is one SQL predicate next to the catalog's
visibility predicate, which the catalog page query now shares.
Retention joins session_catalog_projection, and "orphaned" means the
parent is not a catalog-visible Session, as the rail treats it. A
Desktop test checks the candidates equal archivedTaskRows on a mixed
fixture.
- Sweep and deletion records have one decoder in core, used by the
State Root document and the protocol.
Simpler
- previewDays is gone. The query always returns preview { count,
eligibleAt? }; eligibleAt is present exactly while enabled with
candidates. Desktop states "at least N" and "no earlier than about
<its own now + days>" in the confirm.
- The preview is one aggregate query in storage.
- observedAt is no longer persisted; after a restart the floor is
enabledAt, the last sweep time and the newest metadata time.
- The guard compares the setting revision only.
- The section names the Host it applies to.
Refs apache#5899
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Address the review of the opt-in retention for archived tasks.
Forward clock jump. A wall clock set far ahead (bad RTC or NTP, VM
restore, a manual date change) could make a whole backlog eligible at
once. A sweep now holds deletions for 24 hours when the clock moved
ahead of the last time the Host observed by more than the smaller of
the retention window and 7 days. In a running Host that reference is
the in-memory high-water; after a restart it is the persisted floor
(enabledAt, the last sweep, a previous hold) and, once past the
deadline, the newest Session metadata time, which says when the Host
last ran. The hold is recorded as latest.hold { since, detectedAt,
until }, a further jump re-arms it, it is cleared once the clock
reaches `until`, and any setting change clears it. No uptime or
monotonic clock is used: a sleeping laptop would read as a jump, and a
credited clock was rejected in apache#5899. Desktop shows when cleanup
resumes and how far the clock moved, and suggests turning cleanup off
if the time is wrong.
Preview wording. The count includes families a sweep keeps (busy, for
review) and misses subtasks a deletion orphans later, so it is neither
bound; the copy now says "N archived tasks are subject to automatic
cleanup" in all three locales, in the section and the confirm.
Imported archived tasks. A Session bundle copies session_metadata rows
verbatim, so an archived task arrived with the archive time (or none)
of the machine it left and could be deleted right after import. The
import now stamps archived_at with the import time for every imported
archived Session.
Refs apache#5899
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ata write A Host restarted more than 7 days after enabling retention, but before the deadline, measured the forward gap from the persisted floor alone (effectively enabledAt). It recorded a false clock-jump hold even when the app had been in use a minute earlier, and because a hold was only cleared after the deadline and always reported, its banner stayed up until then. - A fresh process now measures the gap from the later of the persisted floor and the newest Session metadata time, before and after the deadline alike: one MAX query, once per process. No candidate is read before the deadline. - A hold expires on its own: once the clock reaches `until` it is cleared with one write, whatever the deadline, and the query never reports a hold whose day is over. Refs apache#5899 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The retention candidate predicate excluded pinned families with a correlated NOT EXISTS over session_metadata. No index covers is_flagged (migration 26 dropped session_metadata_by_flag, which the old comment still cited), so every candidate rescanned the table. The cost was quadratic and ran synchronously on the Host thread, for both the sweep page and the Settings preview count. Synthetic timings: about 47 ms at 2k Sessions, 1.1 s at 10k, 20 s at 40k. Use an uncorrelated NOT IN over the pinned family roots. SQLite builds it once per statement: 2 ms at 2k Sessions, 4 ms at 10k, 21 ms at 40k. The result set is identical (checked on the synthetic data). The family root is COALESCE(..., session_id), and session_id is NOT NULL, so NOT IN has no NULL pitfall. The parent check in the row predicate stays correlated: it is a primary-key lookup. Refs apache#5899 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
3f1409c to
bd5b626
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 3f1409c0 (our last reviewed head) to bd5b6265. The PR is restacked on #5902's new head 3fc3970b: that head is an ancestor, and the 3 commits after it are this PR's own. I reviewed only 3fc3970b..bd5b6265 and compared it with the previous own range 717841a0..3f1409c0. The two diffs are identical except for regenerated surface inventory totals (330 to 331 files, aligned 326 to 327) and hunk offsets/blob indexes in the inventory files. The notice observer, the Settings acknowledgement path, the ports and the tests are byte-identical to the previous own range.
Findings. Nothing new at P0-P2. The P3 we raised earlier about the disabled-host backoff (retention-notices.ts:139) is untouched, and so is the earlier unrelated app-shell.tsx rename note, so I am not raising either again. The PR inherits #5902's epoch, so the epoch-206 coordination note on #5902 applies here too.
Status: MERGEABLE (merge state BLOCKED). It merges cleanly with main 3597abe8 (git merge-tree). CI: 12 pass, 1 skipped, and test was still pending when I checked. Merge after #5902.
This branch has not been deployed
2 participants
Archived-task cleanup results and clock pauses are currently visible only in Storage settings. This follow-up shows a Host-scoped notice in the application shell, with an action that opens that Host's Archived tasks settings.
The observer reads only already-connected Hosts while the window is visible and focused. Hosts with retention enabled are checked at most once a minute; disabled Hosts are checked at most once every 15 minutes, including across focus changes. A policy enabled on another Client can therefore take up to 15 minutes to be discovered. Focus and visibility changes trigger refreshes; blur does not.
The last announced result is persisted per Host on this Desktop. Successive cleanup batches are coalesced with a 15-minute reminder cooldown. Clock warnings bypass that cooldown and are dismissed when the Host clears them or when the user views them in Settings. Settings acknowledgements trigger an immediate observer refresh, and are stored separately from announcement tokens so a warning is not withdrawn merely because it was announced. Hidden windows and stale responses do not acknowledge results. Same-name Hosts retain separate notices.
Depends on #5902. This PR currently includes its commits because the base PR has not merged. The notification changes are commits
0109b73f4,ab5651665andbd5b62650(inventory totals); review the incremental diff. I will rebase onto main after #5902 merges. It adds no Host protocol operation or retention policy change.Part of #5776 / #5899. Idle archiving remains tracked separately in #5919.
The AppShell alias/destructuring changes are behavior-preserving. They reduce its non-trivia token count from 7333 to 7330 while adding the notification mount, keeping this frozen root within its existing renderer architecture budget. The budget is not raised.
Validation: Desktop main build and all four typechecks pass; 22 focused tests pass, covering persistence, cooldown, clock warnings, Settings acknowledgements and toast withdrawal, disabled-Host backoff across focus changes, event subscriptions and cleanup, visibility, disconnection, stale responses, disposal, and real toast navigation in all three locales for same-name Hosts. Format, lint, Knip (Desktop), ASF headers, locale hygiene, app-shell hooks, and strict renderer architecture checks pass. Hosted checks passed on
c682972c7; the review-fix commit requires a fresh CI run.Limits: the notices are in-app rather than native OS notifications. The Host reports only its latest deletion batch, so the notice states the latest cleanup result rather than claiming a total across missed batches.