deploy: incremental graph-extract (upstream #1268) - #1
Merged
Conversation
Two pushed commits on this branch state that the deployment carries "1798 files and roughly 350 MB" under /data: 54225a0 build(railway): give a cold engine room to accept the worker 960c8ad build(railway): raise the healthcheck timeout for a cold store That is wrong by about 7x. Measured from the container on 2026-08-27 at 05:39Z, /data held 2,421 files across 2,363 MB. A pushed commit message cannot be edited, so the corrected figure goes into deploy/README.md, which both of those bodies already send the reader to for the fresh-store baseline. The figure was already refutable from this branch's own log when it was written. 6a27bca, a day earlier, records the same store at 1013 MB. Nothing else in those two bodies depended on it. The 63 to 67 seconds from container start to "Worker registered" against a 60 second window was timed from the container logs, not derived from store size, and the conclusion (raise healthcheckTimeout to Railway's default of 300) is unchanged. The cold read argument is stronger at 2,363 MB than at 350 MB, but this commit claims only what was measured. The same paragraph in deploy/README.md still described healthcheckTimeout as 60 s, which 54225a0 had already changed to 300 in deploy/railway/railway.json. Correcting one wrong number there while leaving a second one two lines below would defeat the point, so both land together.
3005b67 wrote the two figures as one observation: "On a store of 2,363 MB across 2,421 files, the worker took 63 to 67 s to register." That pairing was never measured. The timing was taken during the healthcheck failures behind 960c8ad and 54225a0, in the early hours of 08-27. The store figure was measured at 05:39Z, hours after the last of those failures. 6a27bca records the same store at 1013 MB the previous day, so it more than doubled across that window. Both numbers are point samples of a quantity that was moving between them, and the store size at the moment of the timing was never read. State them as two separate observations and say the store was still growing, so a reader cannot take the size as the condition the timing was measured under. This is the defect the correction in 3005b67 exists to prevent, reached by a different route: a composite claim is as discountable as a wrong one.
Claude Code's per-turn Stop hook POSTs /agentmemory/session/end, so event::session::stopped runs on every agent turn, not once per session. It handed mem::graph-extract the session's entire compressed observation list each time. Downstream, persistGraphDelta takes the MERGE path for every node and edge it has already seen (kv.get name-index + kv.get nodes + kv.set per node, and the same three per edge), so turn N re-merged everything from turns 1..N-1. Measured on the new harness: eight turns of one observation each dispatched 36 observations before this change (1+2+...+8) and 8 after. The growth was quadratic in turn count, and per rohitg00#843 nothing written through kv.set is ever evicted from the iii engine, so that was quadratic permanent heap walking the engine toward its cap. The session record now carries a watermark. Two optional fields rather than one, because a bare max-timestamp watermark is not safe here: observe.ts dispatches mem::compress fire-and-forget and compress.ts stamps the capture time, not the write time, so a slow compression lands an older timestamp after a newer one was already extracted. The count is the tripwire. When the number of observations newer than the watermark does not exactly account for the growth since the watermark was recorded, something arrived out of order (or was evicted) and the whole session is re-sent. Losing a memory is worse than re-merging one. Both fields are optional, so existing session records stay valid and simply take the full-extract path once. The watermark advances only after mem::graph-extract's dispatch is accepted, so a hand-off that never left is retried on the next turn. fireVoid now resolves to a dispatch-success boolean to make that observable; the other callers ignore the value exactly as before. Limitations: - Completion is not observable through TriggerAction.Void(). An extract whose dispatch is accepted but which then fails downstream leaves its delta out of the graph until someone POSTs /agentmemory/graph/build. Advancing on completion would mean awaiting LLM work inside the stop handler, which is the latency this trigger exists to avoid. - How often the out-of-order fallback fires in a real deployment is not measured. Compression latency scales with payload size, so timestamp inversions may be common rather than rare. When the fallback fires it does exactly what the pre-fix code did on every turn, so this is never worse than the status quo, but the saving on a busy session is unquantified. - Interleaving is handled by construction, not by a lock: the (timestamp, count) pair is only ever written as a matched snapshot from a single run, and a pair that does not reconcile degrades to a full extract. A stale pair costs a re-merge; it cannot skip an observation. Ten tests added. Five fail against unmodified source, including the 36-vs-8 dispatch count. The five that pass by construction cover the fallback branch, whose correct behaviour is the pre-fix behaviour; they exist as mutation coverage and kill removing the count tripwire, making the comparison inclusive, dropping the empty-batch guard, and skipping instead of falling back.
…servation
Review of the previous commit found a hole in its tripwire. Comparing the
number of observations at or below the watermark proves the cardinality is
unchanged, not that they are the same observations. A deletion and a late
arrival in the same window cancel out:
snapshot {o1(T1)..o5(T5)} extracted, watermark (T5, 5); then evict removes
o3 and a slow compression lands o2b(T2b<T5). compressed.length is still 5,
nothing is above T5, so 5-5=0 reconciles and the whole turn dispatches
nothing. o2b never reaches the graph, and the corrupted count is now
self-consistent, so every later turn reconciles too and it stays missing
until someone POSTs /agentmemory/graph/build.
That is reachable on the live path. evict.ts:246-260 evicts by per-project cap
(default 10,000) sorted on importance, with no age filter and no session-status
filter, so it can delete an observation from the session that is still being
appended to. It is reachable precisely in the situation this work targets: a
large store with an operator running eviction while an agent is connected.
The previous commit's claim that "a stale pair costs a re-merge; it cannot skip
an observation" was too strong, and this is the case it missed.
No cardinality-only scheme can see a 1-for-1 swap, so `graphExtractedCount` is
replaced by `graphExtractedDigest`: an order-independent sum of the parsed
timestamps of the observations at or below the watermark, seeded with the set
size and reduced mod 2^32. Same one number on the session record, same two
engine calls per turn, but any change to the already-extracted set moves it —
a swap, a deletion, a late arrival, or a tie on the watermark timestamp — and
a mismatch falls back to a full extract. Storing the ids themselves would be
exact but grows without bound in a record rewritten every turn, which is the
rohitg00#843 heap behaviour this work exists to stop.
Measured: the new swap test dispatches nothing at all on the second stop
against the previous commit ("expected undefined to deeply equal
['a','e','b']"), which is the loss made visible, and dispatches all three
after this change.
Residual, unchanged from the previous commit: the digest is a sum mod 2^32, so
two observations whose epoch milliseconds are congruent mod 2^32 (49.7 days
apart to the millisecond) could still cancel. An observation stamped exactly at
the epoch contributes zero to the sum, which the set-size seed covers and a
test pins.
…change Review asked the question the batch-content tests cannot answer: the tests mock sdk.trigger, so mem::graph-extract never runs and they prove only that the payload shrank, not that the resulting graph is the same. Traced it. extractGraphHeuristics (graph.ts:457) loops per observation and every link() call joins nodes built from that one observation's own files and concepts. Its nodeByKey and edgeByPair maps are batch-scoped dedup, and persistGraphDelta performs the same dedup across batches through graphNameIndex and graphEdgeKey, with mergeNode and mergeEdge unioning sourceObservationIds. So on the default path the delta produces the identical node and edge set. That is what makes the previous "perf" framing accurate rather than a quiet behaviour change. The opt-in LLM pass is different and worth naming. isGraphExtractionEnabled requires GRAPH_EXTRACTION_ENABLED=true and is off by default; when on, buildGraphExtractionPrompt builds ONE prompt from the whole array, so a per-turn delta of a few observations gives the model less co-occurrence to work with than a whole-session batch did. Cross-turn relations it used to propose will not be proposed. Bounded batches are already the norm on that path, api::graph-build feeds it 25 observations at a time (api.ts:1616), so this narrows an existing property rather than introducing a new limit. Also corrects the residual stated in the previous commit, which was narrower than a sum actually guarantees. The digest is blind to any change to the already-extracted set whose timestamp total is congruent mod 2^32, not only to a single swap 49.7 days apart to the millisecond — deleting two and adding two with a coinciding total would also pass. Still an arithmetic coincidence at millisecond resolution, but the bound is a coinciding sum, not a coinciding pair. One further effect worth knowing, in the direction of better: mergeNode and mergeEdge union the WHOLE batch's obsIds into every node and edge they touch. Sending the entire session every turn therefore accumulated every observation id in the session into sourceObservationIds on every node it merged. The delta attributes only the observations that were actually in the batch, so provenance gets narrower and those arrays stop growing per turn. Comment only, no behaviour change.
The digest that guards the incremental watermark summed Date.parse of each timestamp, seeded with the set size. That collides on exactly the case it was written to catch: evict drops one observation while a late compression lands another carrying the SAME millisecond. Size is unchanged, the sum is unchanged, the digest matches, the incremental path is taken, and the late observation is never handed to mem::graph-extract. On a review of the watermark logic a skip is otherwise impossible, so the whole correctness argument rested on this digest. Fingerprint the sorted observation ids through the existing fingerprintId() helper instead. Ids differ whatever the timestamps do, and order-independence is now structural rather than an arithmetic property a test has to assert. graphExtractedDigest becomes a string. The pinned test previously used two different milliseconds, which is why it passed. Moved to the same millisecond; it fails against the arithmetic digest with the second stop dispatching nothing at all.
`newest` reduced over the full compressed list and the digest fingerprinted the full compressed list, while the batch handed to mem::graph-extract was only the tail. Those are equal today because the batch is always `timestamp > at`, so max(batch) === max(compressed). They stop being equal the moment anyone caps this batch, and the file's own comment cites api::graph-build capping at 25 as precedent. A cap would make `newest` advance past observations that were never dispatched, which is the exact silent skip the digest exists to prevent, and would leave the digest covering observations above the watermark so the pair could never match again. Reduce over `batch` and fingerprint everything at or below the resulting watermark. Both fields now come off one predicate, so the invariant holds locally instead of resting on a max-equality argument in prose.
Neither branch logged anything, so a session whose watermark keeps going stale re-sends its whole observation list every turn and looks identical to a healthy one from the outside. One line on the mismatch path, carrying the two set sizes so the cause (a deletion, a late arrival, a half-written pair) is readable from the log rather than guessed.
The comment claimed the heuristic graph is "identical either way" and hung that on persistGraphDelta's dedupe. The dedupe argument is right about the node and edge SETS and wrong about provenance: mergeNode and mergeEdge union the whole batch's obsIds (graph.ts), so a whole-session batch stamped every node and edge with every observation id in the session. The union cited as proof of equivalence is where the divergence lives. Post-change provenance is per-batch, which is narrower and more accurate, and it is read by graph retrieval and by supersede-staling. Net an improvement, but it ships on the default path and the comment should say so rather than assert equivalence. Also drops the GRAPH_EXTRACTION_ENABLED framing. That flag gates only the LLM pass; the heuristic pass that produces this behaviour always runs.
Three things that carried no weight: `typeof mark === "string"` was a no-op. The comparison on the next line already yields false for an absent or legacy-numeric digest, which is the same fallback the guard produced. fireVoid grew a boolean return for one of its four callers. Calling sdk.trigger directly for mem::graph-extract lets a rejected dispatch throw into the try/catch that was already there, which logs the same message and skips the watermark write the same way, and restores fireVoid to what the other three callers use. The "mem::compress is fire-and-forget so writes are not in timestamp order" paragraph was duplicated verbatim in types.ts and events.ts. types.ts now points at events.ts. Four test cases went with it. Each asserted a mutation another case already kills: the linear-vs-quadratic count is subsumed by the exact per-turn batch assertion, the matched-pair case by the same (a wrong watermark changes the next turn's batch), the synthetic torn pair by the realistic out-of-order case, and the half-written pair killed nothing at all — it passed with the guard present or absent. The fake KV's set/delete were never invoked.
…serve Nothing verified the claim the whole change rests on. The incremental suite sets concepts and files to [] and mocks sdk.trigger, so mem::graph-extract never runs and the graph is never built. Call extractGraphHeuristics directly with populated concepts and files and compare one batch of two observations against two batches of one, merged by (type, name) and edge endpoints the way persistGraphDelta merges them. Node ids are random, so the comparison is by name throughout. This is what would catch a future cross-observation link, which is the only way splitting the batch could change the set. Also pins order-independence of the fingerprint: the same observation set listed back in a different order must stay on the incremental path. Kills the mutation that drops the sort.
The mutation run showed the `typeof at === "string"` guard could be removed with every test still green: with no watermark the timestamp filter yields an empty set, that fingerprints to something no absent digest equals, and the full extract happens anyway. The guard's one remaining job is keeping the stale-watermark log off every session's first turn, and nothing checked that. Assert the log is silent on the first stop and carries the two set sizes on the fallback path. Also the only coverage the log line has.
graphExtractedDigest is new, so on the first stop after this ships every existing session record has graphExtractedAt and no digest. That branch is traversed once by 100% of real sessions and nothing exercised it — a permissiveness mutation (`!mark || fingerprint(seen) === mark`, the "do not punish upgraders with one re-extract" shortcut) passes the entire 1720-test suite untouched, while silently dropping every observation at or below the stale watermark. Assert the whole session goes out when the digest is absent and the observations straddle the watermark. Kills that mutation.
`extracted` filtered `compressed` down to observations at or below `newest`, and returned `compressed` on every reachable path. On the fallback branch `batch` IS `compressed`, so `newest` is its maximum. On the incremental branch `batch` is the tail above `at` and non-empty, so `newest` is the maximum of the whole list either way. Verified algebraically and by exhaustive enumeration. It was scaffolding for a batch cap that does not exist. Without it, a future cap makes the next turn fingerprint miss and the session falls back to a full re-extract, which the log added alongside it now makes visible rather than silent. Nothing is skipped either way, and `newest` reducing over `batch` is what actually closes that hazard. The comment claimed both halves of the watermark pair came off the dispatched batch. Only `newest` did; the fingerprint came off `compressed` through a filter that excluded nothing. Says what the code does now, and drops two paragraphs that restated what the tests already execute.
The quadratic-heap explanation lived in three files. An earlier commit replaced the copy in types.ts with a pointer to events.ts and left the one in this test header untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deploy PR. Railway builds
productionon push, so this is the review surface before the fix reaches the live service.Mirrors upstream rohitg00#1268 — read that for the full mechanism and limitations.
What ships
Thirteen commits cherry-picked from
fix/incremental-graph-extract, plus two doc corrections already on localproductionbut never pushed.event::session::stoppedfires on every agent turn (the per-turn Stop hook POSTs/agentmemory/session/end) and currently re-sends the entire session's observations tomem::graph-extract.persistGraphDeltathen re-merges every accumulated node and edge at 3 engine round-trips each, so cumulative engine invocations are quadratic in turn count.Measured, 8 turns of one observation each:
Per rohitg00#843, data written via
kv.setstays resident in the engine's in-memory state and the 0.11.xiii-stateworker has no TTL, no eviction, no max-entries. Quadratic writes into a store that never evicts is quadratic permanent heap.This branch was wrong until now — worth stating
It was originally cut from the first three commits, before two rounds of review. In that state it carried a watermark fingerprint built from a size-seeded sum of timestamps, which collided in exactly the case it existed to catch:
evictdeletes an observation while a late compression lands another in the same millisecond, leaving both the set size and the sum unchanged, so the new observation was never extracted.Merging this branch as it stood would have deployed that bug. It has been re-cherry-picked from the reviewed tip;
src/triggers/events.tsand the test file are now byte-identical to the reviewed branch, andsrc/types.tsdiffers only by production's ownheapSizeLimitfield and upstream'sviewerPort, neither related to this change.Pre-deploy baseline — the number this has to beat
Captured 2026-08-27 10:23Z, engine pid 75:
Gates, run against
productionnot just the fix branchnpm test— 161 files, 1771 passed, 1 skippednpx tsc --noEmit— 30 errors, exactly the pre-existing baselineHow this gets judged after merge
Rate against rate, not absolute memory. A deploy restarts the engine and resets RSS to near zero, so before/after on absolute is meaningless. Two timestamped points 60+ minutes apart minimum, under comparable traffic — the 8.24 MB/min baseline was taken under a live 20+ concurrent-session load, and measuring during a quiet period would produce a better number that proves nothing.
The 36-to-8 figure is invocation count, not heap. Whether the memory curve actually flattens is what the post-deploy measurement decides.
Rollback
Deployment
2268c51d-cf35-4ba1-89e6-2278351cc7c2, commita9f0288d7, 2026-08-27T04:11:19Z, status SUCCESS.Railway's auto-redeploy replays the last stopped deployment, not the branch tip, so a rollback has to name that deployment explicitly.
Known cost of merging
Volume-backed Railway services cannot overlap deployments (documented: "we prevent multiple deployments from being active and mounted to the same service"), so the cutover drops the service for roughly 63 seconds. Hooks fail soft and nothing replays, so every observation posted during that window is permanently lost. Accepted deliberately — measuring under real load is what makes the result valid.
Review disclosure
Two rounds, both lenses. Round 1 found the fingerprint collision. Round 2 removed a derived set provably equal to its source and a comment describing code that was not there. Round 2's correctness pass had no independent reviewer — its findings were lost twice to tool failures, so its five open questions were verified against the code by the same person who wrote the fixes. Individually code-verified, weaker than an independent pass.