Skip to content

[finding] Three residues from the #15358 contract review: an unpinned stale-hot drop, a thrown third read that leaves stranded, and a malformed host verdict that aborts the whole scan #16709

Description

@os-trump

Grouped from the §5 non-blocking observations of the isolated contract review on PR #16704 (card #15358, ruling B′, verdict PASS, head 1b4dad617). Filed as one card rather than three, per the lane rule that a review's non-blocking items land as a group.

⛔ None of these blocked #16704 and none is a defect in the ruled behaviour. All three are in @objectstack/service-automation / @objectstack/plugin-approvals.

⚠️ Every reading below is the reviewer's, at head 1b4dad617. The branch has since moved to 42ce7bf4d (a prose-only patch). ⭐ Re-measure before acting — do not treat these line numbers or counts as current. This card states them as claims to verify, ⛔ not as facts.


1. ⭐ The restore verb's drop of a stale hot copy is unpinned, package-wide

Reviewer's ablation E2 — make the shared helper never report staleHot, so the restore verb never deletes a stale journal entry:

70/70 green on the 8 restore-verb test files and 1486/1486 green on the full suite.

⇒ A real behaviour can be deleted and nothing in either package notices.

This is a pre-existing coverage gap — the old inline deletes had the same (absent) pins — and #16704's refactor preserved the behaviour by inspection. But after the refactor that drop lives behind one helper flag that no test observes, which makes the gap cheaper to fall into.

Suggested pin, from the reviewer: a stranding replica whose run is restored and finished by another replica must, on its next restore call, evict its own journal entry — e.g. a later inspectConsumedSuspension with the terminal row gone answers NO_CONSUMED_SUSPENSION, not witness: 'journal'.

⭐ Whoever takes this: the pin must be shown to fail before it passes. A pin added to a gap like this one is worthless unless the ablation that exposed the gap turns it red.

2. A thrown third read counts undetermined, so the row leaves stranded

getRun returns an in-memory entry before touching the store (engine.ts:4257 ff.), so a loadTerminal outage can throw only in the third read. Pre-B′ that row was reported 'failed'; post-B′ it is counted undetermined and logged with request/run id.

This is not the #15555 inversion — the row is never labelled unrepairable — and it is consistent with the method's #4469 idiom. The ruling is silent on it.

⇒ The open question is a judgement call, ⛔ not a bug report: for an operator's report, is 'failed' (the row stays visible as un-advanceable) more fail-closed than undetermined (the row leaves the list, with a log line)? A seat may reasonably prefer 'failed'. Decide it with the operator surface in mind, and ⛔ do not change it silently.

3. A malformed host verdict aborts the whole scan

refineFailedRunState(verdict) runs outside the try; a contract-violating host returning undefined throws a TypeError out of inspectStrandedRequests, whereas the two older oracles tolerate a falsy answer.

⇒ One misbehaving host implementation turns a partial answer into no answer — the inspector's whole point is to enumerate rows that cannot advance, and it enumerates none.

⚠️ Requires a host that violates the declared surface, which is why it is a robustness nit and not a p2. ⭐ It was deliberately left out of #16704's patch round: it is a behaviour change, and including it would have broken the carry-across of the contract-review PASS onto the patched head (the patch was proven to touch comment lines only — 36 changed lines, 0 non-comment). That exclusion was the seat's call, recorded on #15358 (5576769163), ⛔ not an oversight by the implementer.


Refs

#15358 (ruling B′) · PR #16704 · the review verdict and per-item disposition: #15358 comment 5576769163 · #15555 (the false-negative shape the ruled design avoids) · #15832 (loadSuspendedRunStrict phantom eviction) · #4469 (the undetermined idiom) · #16708 (the spec TSDoc residue, filed separately because it is domain:spec and blocked on #16704 landing)

Filed by the domain:services PM seat. Unassigned, untriaged priority — priority:p3 is the seat's estimate, ⛔ not a ruling.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions