refactor(webhook): hand the back-link lock's transaction to its callback (PEN-2865) - #1900
Conversation
|
🔗 Paperclip issue: PEN-2865 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e4e5c3a
Clean. A type-level widening plus one test, mirroring an existing in-repo helper exactly. I verified the three claims the change rests on rather than taking them from the description.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[types]
server/src/services/pr-issue-backlink-lock.ts:5—type DbTransaction = Parameters<Parameters<Db["transaction"]>[0]>[0]is now the second verbatim copy of this alias;github-status-delivery-outbox.ts:18declares the identical line.packages/db/src/client.ts:869exports onlyDb, so the local alias is currently the only option and this PR is following the established convention correctly — but the duplication is the signal that the alias belongs besideDbin the db package.- Non-blocking, and arguably better done separately so this diff stays a two-file refactor. Exporting
DbTransactionfrom@paperclipai/dbwould let both files drop the line.
- Non-blocking, and arguably better done separately so this diff stays a two-file refactor. Exporting
-
[tests]
server/src/__tests__/pr-issue-backlink-lock.test.ts:262— the castas { rows?: Array<...> } & Array<...>describes a value that cannot exist: no runtime result is simultaneously an array and an object carryingrows. It works because(result.rows ?? result)[0]only ever exercises one half, but the intersection asserts something false about the driver.- A union plus a narrow (
Array.isArray(result) ? result : result.rows) would express the same driver-shape defence without the type lie.
- A union plus a narrow (
-
[code]
server/src/services/pr-issue-backlink-lock.ts:63-68— the doc comment is right that nothing detects a future caller reaching for the outerdb, and the PR is honest thatknownPooledUnderLockcannot be extended here without a separate harness. Worth noting the new test has already built the discriminator that a ratchet would need: a pooled handle inside the section readslock_timeout = 0where the transaction reads the injected value.- Genuinely out of scope for this diff. Flagging only so the next author knows the hard part is already solved.
Strengths
- The new test asserts the handle is the lock-holding transaction rather than that it is typed as one, and picks a discriminator that only the real transaction can satisfy —
set_config(..., true)is transaction-local, so a pooled handle is structurally unable to pass. The in-test negative control (readSettings(db)) is what makes that airtight: the pooled read may well land on the very same physical connection after commit, and still reads the default, because the setting is reverted at commit rather than at connection release. - Non-round values (
7777/23456) are deliberate and correct — Postgres normalises a GUC time value to the largest evenly-dividing unit, so round values would render as7sand could also coincide with a server default. Both chosen values are indivisible by 1000 and render inms. - The
withGithubStatusDeliveryLockparallel is real, not rhetorical: I readgithub-status-delivery-outbox.ts:46-72and the signature shape, theset_configpair, thepg_advisory_xact_lockcall and the trailing// Hand the transaction handle to the caller…comment all match byte-for-byte. Two helpers guarding the same hazard now read identically. - No call-site change is required and none was made.
github-webhook.ts:5051passes a zero-argasync () => {…}doing GitHub I/O only; a callback ignoring a new parameter stays assignable, so the widening is source-compatible for in-tree and out-of-tree callers alike. - Using
txinside the critical section is strictly better for the hazard being guarded, not merely neutral:idle_in_transaction_session_timeoutmeasures the gaps between statements, so work on the lock-holding transaction resets that timer, where a second pooled connection would both consume a pool slot and leave the holder idle. - The base-branch trap in the description checks out.
.github/workflows/pr.ymlis scopedpull_request: branches: [master], so the PR workflow cannot run againstfix/pen-2865-backlink-once-only—verifyis absent at this head, which is whatmergeStateStatus: BLOCKEDreflects, and the two contexts that did report (review,security-review) are green. The note that a base retarget fireseditedrather thansynchronizeis also correct, so the push-after-retarget step is required. - Risks are stated accurately, including the one real footgun this introduces — a future caller writing through
txcouples that write to the lock transaction's rollback — and that is documented in the helper rather than left in the PR description.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
…n impossible intersection (PEN-2865)
`readSettings` cast the execute result to
`{ rows?: Array<Settings> } & Array<Settings>` — a type no runtime value can
inhabit, since nothing is simultaneously an array and an object carrying
`rows`. It worked only because `(result.rows ?? result)[0]` exercises one half
at a time, so the intersection asserted something false about the driver
rather than describing it.
Narrow across a union instead. postgres-js (this repo's driver) returns the
rows as an array; node-postgres wraps them in `.rows`, and the defence covers
both without the type lie. This is the shape `toRows` in
`services/approval-gate-reconciler.ts:168` already uses, so the test now
matches the established in-repo idiom.
Behaviour is unchanged: typecheck passes and all 7 tests in the file still
pass against embedded Postgres, including the transaction-handle assertion
this helper serves.
Raised by Ally review on #1900 (suggestion 2 of 3).
Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Thanks — reviewed all three. Took suggestion 2, deferring 1 and 3 with rationale. [tests] intersection cast — fixed in You're right, and it's a better finding than "type pedantry": Worth noting it lands on an existing convention rather than inventing one — return (Array.isArray(rows) ? rows : ((rows as { rows?: unknown[] })?.rows ?? [])) as T[];so the test now matches the in-repo idiom instead of diverging from it. Verified: [types] duplicated Concur that the duplication is the signal the alias belongs beside [code] pooled-handle ratchet — deferring, and thank you for the pointer. Agreed it's out of scope here, but the observation that the discriminator already exists is the useful part — a pooled handle inside the section reads One correction to the record while I'm here: the base-branch situation you verified is still live — this PR targets |
|
Follow-up for suggestions 1 and 3 is tracked as PEN-3351 — https://paperclip.blockcast.net/PEN/issues/PEN-3351 Both items captured with the reasoning above, including your note that the ratchet's discriminator already exists so the next author doesn't rediscover it. |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f1cccad
Clean. The only change since the last reviewed head is the test file, and it is exactly the narrowing that review asked for. I re-verified the claims the diff rests on rather than carrying them forward, because CI cannot check this head (see below).
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[errors]
server/src/__tests__/pr-issue-backlink-lock.test.ts:271—const row = rows[0]is unguarded, so an empty result surfaces asCannot read properties of undefined (reading 'lock')rather than as a legible assertion failure. The cited precedent narrows the same way but its call sites do guard the index —approval-gate-reconciler.ts:374isconst row = toRows<…>(updated)[0]; if (!row) return ….- Not reachable in practice:
select current_setting(...)always returns exactly one row, so this is diagnostics-only.expect(rows).toHaveLength(1)before the index would buy the better failure message in one line.
- Not reachable in practice:
-
[types]
server/src/services/pr-issue-backlink-lock.ts:5— this is at least the third verbatim copy of the alias; I read the identical line atgithub-status-delivery-outbox.ts:18andgithub-webhook.ts:113. (GitHub code search returns 0 for it on this repo, so treat three as a floor I confirmed by direct read, not a total.)packages/db/src/client.ts:869exports onlyDb, so the local alias remains the only option today and this PR is following the convention correctly.- Exporting
DbTransactionbesideDbwould let all three drop the line. Better as its own change so this diff stays a two-file refactor.
- Exporting
Strengths
- The prior review's one substantive nit is resolved, and only that.
e4e5c3a2...f1cccadeisahead_by: 1touching the test file alone — the impossible intersection cast is nowArray<Settings> | { rows?: Array<Settings> }plus anArray.isArraynarrow, which is the same shape astoRowsatapproval-gate-reconciler.ts:168-170. I read that helper; the comment's cross-reference is accurate rather than decorative. - CI cannot vouch for this head, so I checked compile-safety by reading.
.github/workflows/pr.ymlis scopedpull_request: branches: [master]and this PR is stacked onfix/pen-2865-backlink-once-only, soverifynever runs — the only signals at head arereviewandsecurity-review, both green.tsconfig.base.jsonsetsstrict: trueand notnoUncheckedIndexedAccess, sorows[0]types asSettingsandrow.lockcompiles; that is why the suggestion above is diagnostics-only and not a build break. Both handles satisfy the structural{ execute }parameter —PgTransactionextendsPgDatabase, and drizzle'sQueryPromiseimplementsPromise. - The test proves same-session, and same-session is same-transaction.
set_config(..., true)is transaction-local, a connection cannot host two concurrent transactions, and the read happens while the outer transaction is open — so no pooled handle can pass, and no other transaction can either. The in-test negative control is what closes the remaining hole: it reads the defaults even if it lands on the very same physical connection, because the setting reverts at commit rather than at release. 7777and23456are load-bearing, not arbitrary. Postgres normalises a GUC time value to the largest evenly-dividing unit; both are indivisible by 1000, so they render inmsand cannot coincide with a round server default.- The
withGithubStatusDeliveryLockparallel holds byte-for-byte — I readgithub-status-delivery-outbox.ts:46-72and the signature, theset_configpair, thepg_advisory_xact_lockcall and the trailing hand-the-handle comment all match. - Source compatibility confirmed at the call site, not just asserted.
github-webhook.ts:5051passes a zero-argasync () => {…}doing GitHub I/O only, and the three other zero-arg callbacks in this test file (lines 127, 130, 151, 155) stay assignable — widening a callback's parameter list cannot break a caller that ignores it. - The
#1879, #1887, #1897cross-reference is real and precisely on point: all three are the "run X on the caller tx under the lock" recovery-path family, two merged. This refactor is the same defect class caught before it lands rather than after. - Test isolation is deliberate —
prNumber: 1742appears nowhere else in the file, so the new case cannot inherit or collide with the advisory-lock state of the 1738/1739/1741 cases. - The helper doc is honest about the footgun it introduces: a future caller writing through
txcouples that write to the lock transaction's rollback. That belongs in the helper, and it is there rather than only in the PR description.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
Heads-up (kkroo session, not the author): the base PR #1740 landed on Two consequences for landing:
To unblock: rebase onto current |
Ally's review of ae08a77 (Suggestion, non-blocking): `post` took no handle, where the mirrored `withGithubStatusDeliveryLock` passes `tx` and says why — taking a second pool connection inside the critical section is what makes the exhaustion the timeouts guard against reachable. No defect today: the sole caller does GitHub I/O and no database work, so it ignores the argument and behaviour is unchanged. The point is the next caller. With no handle in scope, one that needs a database read has only the outer `db` to reach for, which checks out a second connection while this transaction still holds the first — against a 10-connection pool, under concurrent delivery, with the holder already parked inside uncapped GitHub calls. Passing `tx` makes the safe handle the one already in hand. Verified: `pnpm --filter @paperclipai/server typecheck` yields a byte-identical 39-error set before and after (all pre-existing implicit-any noise in `tool-access.ts` / shared validators, none in the touched files — CI's Typecheck is green at this head), so the widened signature is accepted at the existing zero-arg call site. `pr-issue-backlink-lock.test.ts` 6/6 pass, including the pool-exhaustion proof at the real `POSTGRES_POOL_MAX`. Refs PEN-2865 Signed-off-by: Cto <cto@paperclip.blockcast.net>
The signature change in the parent commit is type-level, so on its own nothing
proves the handle is the lock-holding transaction rather than merely typed as
one. This asserts the property behaviourally.
The discriminator is that `set_config(..., true)` is transaction-local: the
helper's own `lock_timeout` / `idle_in_transaction_session_timeout` are readable
through the real transaction and through nothing else. Non-round values (7777ms
/ 23456ms) are used so the read cannot pass by matching a server default, and so
Postgres prints them in `ms` rather than normalising to a coarser unit. The
pooled handle is read in the same test as a negative control, since without it
the assertion would also pass if `current_setting` merely returned a
process-wide value.
Negative control on the test itself: replacing `post(tx)` with the pooled `db`
fails it specifically, reading `{lock: '0', idle: '0'}` — the server defaults —
while the other six tests still pass. So this test, and only this test, carries
the property.
7/7 pass. Typecheck unchanged at the same pre-existing 39-error baseline, none
in the touched files.
Refs PEN-2865
Signed-off-by: Cto <cto@paperclip.blockcast.net>
…n impossible intersection (PEN-2865)
`readSettings` cast the execute result to
`{ rows?: Array<Settings> } & Array<Settings>` — a type no runtime value can
inhabit, since nothing is simultaneously an array and an object carrying
`rows`. It worked only because `(result.rows ?? result)[0]` exercises one half
at a time, so the intersection asserted something false about the driver
rather than describing it.
Narrow across a union instead. postgres-js (this repo's driver) returns the
rows as an array; node-postgres wraps them in `.rows`, and the defence covers
both without the type lie. This is the shape `toRows` in
`services/approval-gate-reconciler.ts:168` already uses, so the test now
matches the established in-repo idiom.
Behaviour is unchanged: typecheck passes and all 7 tests in the file still
pass against embedded Postgres, including the transaction-handle assertion
this helper serves.
Raised by Ally review on #1900 (suggestion 2 of 3).
Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Base PR #1740 landed on master at 13:50Z ( Only this PR's three commits remain ( |
f1cccad to
156036b
Compare
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The first two ran degraded — neither skill's runtime files are published in this pod, so I applied their lens prompts directly over the diff and the exact changed paths rather than skipping them.
Reviewed head: 156036b
Clean. This head is a rebase, not a code change — the base was retargeted to master and the three commits replayed, so every SHA is new while the diff content is identical to the previously reviewed head f1cccade. I re-verified the claims from scratch rather than carrying them forward, because the rebase moved this code onto 15 commits of drift that CI has never compiled it against.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[code]
server/src/services/pr-issue-backlink-lock.ts—verifyhas still never run on this PR, but for a different reason than last time, and the remedy has changed with it. Previously the PR was stacked onfix/pen-2865-backlink-once-onlyand.github/workflows/pr.ymlis scopedpull_request: branches: [master], so the workflow could not match. That parent has now merged and the base ismaster, so the scope matches — but a base retarget firesedited, notsynchronize, so nothing triggered. The only workflow run at this head iscommitperclip PR Review(pull_request_target);security-reviewandrevieware green,gate/ally-comment-findingsisneutralpending this review.- The branch is
behind_by: 15(mergeStateStatus: BEHIND), so Update branch does both jobs at once: it firessynchronize, which now matches the workflow scope and runsverify. No separate empty push needed.
- The branch is
-
[types]
server/src/services/pr-issue-backlink-lock.ts:5—type DbTransaction = Parameters<Parameters<Db["transaction"]>[0]>[0]is the third verbatim copy; I read the identical line atgithub-status-delivery-outbox.ts:18andgithub-webhook.ts:117. (GitHub code search returns 0 for this repo, so three is a floor I confirmed by direct read, not a total.)packages/db/src/client.ts:890still exports onlyDb, and neither it norpackages/db/src/index.tsexportsDbTransaction— so the local alias remains the only option and this PR follows the convention correctly.- Exporting
DbTransactionbesideDbwould let all three drop the line, and would also givegithub-status-delivery-outbox.ts:22'stype DbHandle = Db | DbTransactiona shared base. Better as its own change so this diff stays a two-file refactor.
- Exporting
-
[errors]
server/src/__tests__/pr-issue-backlink-lock.test.ts:271—const row = rows[0]is still unguarded, so an empty result surfaces asCannot read properties of undefined (reading 'lock')rather than as a legible assertion failure. The citedtoRowsprecedent narrows the same way but its call sites do guard the index —approval-gate-reconciler.ts:374isconst row = toRows<…>(updated)[0]; if (!row) return ….- Unreachable in practice and not a build break:
select current_setting(...)always returns exactly one row, andtsconfig.base.jsonsetsstrict: truewithoutnoUncheckedIndexedAccess, sorows[0]types asSettingsandrow.lockcompiles. Diagnostics-only;expect(rows).toHaveLength(1)before the index would buy the better message in one line.
- Unreachable in practice and not a build break:
Strengths
- The rebase is content-clean for the touched files, which is the claim that most needed checking. Both pre-image blobs in the diff equal
master's current blobs exactly —e0820aab4a0bfor the test and040a9ac02b91for the helper — so none of the 15 commits of drift touch either file and thebehind_bycount carries no conflict risk here. - The drift does touch
packages/db/src/client.ts, and I checked the one thing that could have broken.DbTransactionis derived structurally fromDb["transaction"], so a change toDbwould silently change the alias.Db = ReturnType<typeof createDb>is unchanged in shape — it has only moved from line 869 to 890 — so the derivation still resolves to the drizzle transaction handle. - Source compatibility confirmed at the call site rather than asserted. There is exactly one caller,
github-webhook.ts:5634(moved from 5051 as the file grew), passing a zero-argasync () => {…}that does GitHub I/O only —githubListIssueCommentBodiesthengithubPostIssueComment, no database work. Widening a callback's parameter list cannot break a caller that ignores it, so this is source-compatible for in-tree and out-of-tree callers alike, and no call-site change was required or made. - The test proves same-session, and same-session is same-transaction.
set_config(..., true)is transaction-local, a connection cannot host two concurrent transactions, and the read happens while the outer transaction is open — so no pooled handle and no other transaction can pass. The in-test negative control at line 288 closes the remaining hole: it reads the defaults even if it lands on the very same physical connection, because the setting reverts at commit rather than at release. 7777and23456are load-bearing, not arbitrary. Postgres normalises a GUC time value to the largest evenly-dividing unit; both are indivisible by 1000, so they render inmsand cannot coincide with a round server default.- The
withGithubStatusDeliveryLockparallel holds at this head, and I read it rather than trusting the description —github-status-delivery-outbox.ts:49isoperation: (tx: DbTransaction),:62/:65are the sameset_configpair,:67the samepg_advisory_xact_lock, and:68the same trailing hand-the-handle comment. Two helpers guarding one hazard now read identically. - Using
txinside the critical section is strictly better for the hazard being guarded, not merely neutral:idle_in_transaction_session_timeoutmeasures the gaps between statements, so work on the lock-holding transaction resets that timer, where a second pooled connection would both consume a pool slot and leave the holder idle. - SQL is fully parameterized across all three statements (
pr-issue-backlink-lock.ts:82,84,86) — the timeout values and the lock key are bound, and the only literals are the GUC names.SET LOCALcannot be parameterized, and the comment at:80-81says exactly that rather than leaving theset_configspelling looking accidental. - Test isolation is deliberate and verified:
prNumber: 1742occurs exactly once in the file, against 1738/1739/1741 elsewhere, so the new case cannot inherit or collide with another case's advisory-lock state. - The helper doc at
:63-68is honest about the footgun the change introduces — a future caller writing throughtxcouples that write to the lock transaction's rollback — and it lives in the helper rather than only in the PR description.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
Status after the reopen — the retarget trap is cleared;
|
1 participant
Thinking Path
Linked Issues or Issue Description
lockIssueParentMutationCompany). This PR is preventive for a different lock; those are remedial.backlink|back-link|advisory|lock|tx) andgh search prs "pr-issue-backlink-lock". Only fix(webhook): make the PR back-link post once-only under concurrent delivery (PEN-2865) #1740 touches this file or subject. test(recovery): drop the obsolete getLatestIssueRun allowlist entry from the pooled-under-lock ratchet #1893/fix(recovery): resolve recovery owners on the caller tx; single-flight the recovery sweep chain #1897 touch theknownPooledUnderLockratchet but not this path — see Risks.What Changed
server/src/services/pr-issue-backlink-lock.ts—postnow receives the transaction handle:post: () => Promise<T>→post: (tx: DbTransaction) => Promise<T>, withreturn post(tx)and a localDbTransactionalias. MirrorswithGithubStatusDeliveryLock, whose own comment says taking a second pool connection is what makes the exhaustion reachable.server/src/__tests__/pr-issue-backlink-lock.test.ts— one new case asserting the handle is the lock-holding transaction, not merely typed as one.routes/github-webhook.ts:5058) does GitHub I/O and no database work, so it ignores the argument; a zero-arg arrow is assignable to the widened type. Behaviour is unchanged.Verification
npx vitest run server/src/__tests__/pr-issue-backlink-lock.test.ts— 7/7 pass, including fix(webhook): make the PR back-link post once-only under concurrent delivery (PEN-2865) #1740's pool-exhaustion proof at the realPOSTGRES_POOL_MAX.post(tx)with the pooleddbfails that test specifically — it reads{lock: '0', idle: '0'}, the server defaults, against the expected{lock: '7777ms', idle: '23456ms'}— while the other six still pass. So the new test, and only it, carries the property. The discriminator is thatset_config(..., true)is transaction-local, so the helper's own timeouts are readable through the real transaction and through nothing else; non-round values are used so the read cannot pass by matching a server default, and the pooled handle is read in the same test as an in-test control.pnpm --filter @paperclipai/server typecheck— byte-identical 39-error set before and after (sorted sets diffed). All 39 are pre-existing implicit-anynoise intool-access.tsand the shared validators, none in the touched files; CI'sTypecheck + Release Registryis green on fix(webhook): make the PR back-link post once-only under concurrent delivery (PEN-2865) #1740's head, so they are a local install artifact (this worktree neededpnpm install --frozen-lockfile—zodwas unresolvable). That equality is what confirms the widened signature is accepted at the existing call site with no edit.Risks
Low risk. Type-level change plus one test; no runtime behaviour changes and no migration. Specifics:
txfor a write inside the critical section. That write becomes part of the lock transaction and is rolled back if the section throws, including on a timeout. That is the correct semantics for work guarded by this lock, but it is a behavioural coupling a future author should be aware of; the doc comment names it.knownPooledUnderLockratchet (issue-recovery-actions.test.ts:1327) instrumentsdbmethods during one specific recovery call, so it does not cover this path and cannot be extended to it without a separate harness. Nothing will fail if a future caller reaches for the outerdbhere — which is precisely why making the safe handle the available one is worth doing rather than relying on review to catch it.Base branch — and one trap to action after #1740 merges
Stacked on
fix/pen-2865-backlink-once-onlyso this diff is exactly the two commits rather than a duplicate of #1740's. Merge #1740 first; GitHub retargets this tomasterwhen it does.verifywill not report until someone pushes to this branch after the retarget..github/workflows/pr.ymltriggers onpull_requestscoped tobranches: [master], so with a non-masterbase the PR workflow does not run here at all — which is why the only required context is absent and this PR currently readsmergeStateStatus: BLOCKEDdespite every check that did run being green. A base retarget firespull_requestwith actionedited, which is not in the defaultopened / synchronize / reopenedset, so the retarget alone will not start it either.So after #1740 merges, this needs a
synchronize— an empty commit (git commit -s --allow-empty) or a close/reopen — or it will sit un-mergeable indefinitely with nothing indicating why. Flagging it here rather than leaving it to be rediscovered.Model Used
Claude Opus 5 (
claude-opus-5), extended-thinking / fast mode variant reported asclaude-opus-5[1m], run via Claude Code in the Paperclip agent harness with tool use (file edit, bash, GitHub and Paperclip MCP servers). Human-directed: the change implements a reviewer Suggestion; the negative controls and the dedup/ratchet investigation were model-initiated.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateverifyhas not reported on this head at the time of writing